| Message ID | 20210330173348.30135-1-p.yadav@ti.com |
|---|---|
| Headers | show |
| Series | CSI2RX support on J721E | expand |
Hi, On 30/03/2021 20:33, Pratyush Yadav wrote: > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > capture over a CSI-2 bus. > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > called the SHIM layer. It takes in data from stream 0, repacks it, and > sends it to memory over PSI-L DMA. > > This driver acts as the "front end" to V4L2 client applications. It > implements the required ioctls and buffer operations, passes the > necessary calls on to the bridge, programs the SHIM layer, and performs > DMA via the dmaengine API to finally return the data to a buffer > supplied by the application. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > MAINTAINERS | 7 + > drivers/media/platform/Kconfig | 11 + > drivers/media/platform/ti-vpe/Makefile | 1 + > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++++++ > 4 files changed, 983 insertions(+) > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c Some quick comments: "ti-vpe" directory is not correct, this has nothing to do with VPE. That said, the directory has already been abused by having CAL driver there, perhaps we should rename the directory just to "ti". But if we do that, I think we should have subdirs for cal, vpe and this new one. "ti-csi2rx" is rather generic name. TI has had CSI-2 RX IPs before (CAL) and probably will also have new ones in the future. If there's no clear model name for the IP, as I think is the case here, it's probably best to just use the SoC model in the name. E.g. the DSS on J7 is "ti,j721e-dss". This driver implements the legacy video API. I think it would be better (and easier to maintain) to only implement the media-controller API, unless you specifically need to support the legacy API for existing userspace. Tomi
On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote: > Some platforms like TI's J721E can have the CSI2RX paired with an > external DPHY. Add support to enable and configure the DPHY using the > generic PHY framework. > > Get the pixel rate and bpp from the subdev and pass them on to the DPHY > along with the number of lanes. All other settings are left to their > default values. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/media/platform/cadence/cdns-csi2rx.c | 147 +++++++++++++++++-- > 1 file changed, 137 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index c68a3eac62cd..31bd80e3f780 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -30,6 +30,12 @@ > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > +#define CSI2RX_DPHY_CL_RST BIT(16) > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > +#define CSI2RX_DPHY_CL_EN BIT(4) > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > + > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > @@ -54,6 +60,11 @@ enum csi2rx_pads { > CSI2RX_PAD_MAX, > }; > > +struct csi2rx_fmt { > + u32 code; > + u8 bpp; > +}; > + > struct csi2rx_priv { > struct device *dev; > unsigned int count; > @@ -85,6 +96,52 @@ struct csi2rx_priv { > int source_pad; > }; > > +static const struct csi2rx_fmt formats[] = { > + { > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > + .bpp = 16, > + }, > + { > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > + .bpp = 16, > + }, > +}; > + > +static u8 csi2rx_get_bpp(u32 code) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > + if (formats[i].code == code) > + return formats[i].bpp; > + } > + > + return 0; > +} > + > +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) > +{ > + struct v4l2_ctrl *ctrl; > + > + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, > + V4L2_CID_PIXEL_RATE); > + if (!ctrl) { > + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", > + csi2rx->source_subdev->name); > + return -EINVAL; > + } > + > + return v4l2_ctrl_g_ctrl_int64(ctrl); > +} > + > static inline > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > { > @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > } > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > +{ > + union phy_configure_opts opts = { }; > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > + struct v4l2_subdev_format sd_fmt; > + s64 pixel_rate; > + int ret; > + u8 bpp; > + > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > + sd_fmt.pad = 0; > + > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, > + &sd_fmt); > + if (ret) > + return ret; > + > + bpp = csi2rx_get_bpp(sd_fmt.format.code); > + if (!bpp) > + return -EINVAL; > + > + pixel_rate = csi2rx_get_pixel_rate(csi2rx); > + if (pixel_rate < 0) > + return pixel_rate; > + > + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, > + csi2rx->num_lanes, cfg); > + if (ret) > + return ret; > + > + ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY, > + PHY_MIPI_DPHY_SUBMODE_RX); > + if (ret) > + return ret; > + > + ret = phy_power_on(csi2rx->dphy); > + if (ret) > + return ret; Seems phy_power_on, then phy_set_mode_ext? > + > + ret = phy_configure(csi2rx->dphy, &opts); > + if (ret) { > + /* Can't do anything if it fails. Ignore the return value. */ > + phy_power_off(csi2rx->dphy); > + return ret; > + } > + > + return 0; > +} > + > static int csi2rx_start(struct csi2rx_priv *csi2rx) > { > unsigned int i; > @@ -139,6 +245,17 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > if (ret) > goto err_disable_pclk; > > + /* Enable DPHY clk and data lanes. */ > + if (csi2rx->dphy) { > + reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST; > + for (i = 0; i < csi2rx->num_lanes; i++) { > + reg |= CSI2RX_DPHY_DL_EN(csi2rx->lanes[i] - 1); > + reg |= CSI2RX_DPHY_DL_RST(csi2rx->lanes[i] - 1); > + } > + > + writel(reg, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > + } > + > /* > * Create a static mapping between the CSI virtual channels > * and the output stream. > @@ -169,10 +286,21 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > if (ret) > goto err_disable_pixclk; > > + if (csi2rx->dphy) { > + ret = csi2rx_configure_external_dphy(csi2rx); > + if (ret) { > + dev_err(csi2rx->dev, > + "Failed to configure external DPHY: %d\n", ret); > + goto err_disable_sysclk; > + } > + } > + > clk_disable_unprepare(csi2rx->p_clk); > > return 0; > > +err_disable_sysclk: > + clk_disable_unprepare(csi2rx->sys_clk); > err_disable_pixclk: > for (; i > 0; i--) > clk_disable_unprepare(csi2rx->pixel_clk[i - 1]); > @@ -200,6 +328,13 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > + > + if (csi2rx->dphy) { > + writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > + > + if (phy_power_off(csi2rx->dphy)) > + dev_warn(csi2rx->dev, "Couldn't power off DPHY\n"); > + } > } > > static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > @@ -306,15 +441,6 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > return PTR_ERR(csi2rx->dphy); > } > > - /* > - * FIXME: Once we'll have external D-PHY support, the check > - * will need to be removed. > - */ > - if (csi2rx->dphy) { > - dev_err(&pdev->dev, "External D-PHY not supported yet\n"); > - return -EINVAL; > - } > - > clk_prepare_enable(csi2rx->p_clk); > dev_cfg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG); > clk_disable_unprepare(csi2rx->p_clk); > @@ -339,7 +465,7 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > * FIXME: Once we'll have internal D-PHY support, the check > * will need to be removed. > */ > - if (csi2rx->has_internal_dphy) { > + if (!csi2rx->dphy && csi2rx->has_internal_dphy) { > dev_err(&pdev->dev, "Internal D-PHY not supported yet\n"); > return -EINVAL; > } > @@ -460,6 +586,7 @@ static int csi2rx_probe(struct platform_device *pdev) > dev_info(&pdev->dev, > "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n", > csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams, > + csi2rx->dphy ? "external" : > csi2rx->has_internal_dphy ? "internal" : "no"); > > return 0;
On 30-03-21, 23:03, Pratyush Yadav wrote: > Hi, > > This series adds support for CSI2 capture on J721E. It includes some > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > driver, and finally adds the TI CSI2RX wrapper driver. > > Tested on TI's J721E with OV5640 sensor. > > Paul Kocialkowski (1): > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > Pratyush Yadav (15): > phy: cdns-dphy: Prepare for Rx support > phy: cdns-dphy: Allow setting mode > phy: cdns-dphy: Add Rx support > media: cadence: csi2rx: Add external DPHY support > media: cadence: csi2rx: Soft reset the streams before starting capture > media: cadence: csi2rx: Set the STOP bit when stopping a stream > media: cadence: csi2rx: Fix stream data configuration > media: cadence: csi2rx: Turn subdev power on before starting stream > media: cadence: csi2rx: Add wrappers for subdev calls > dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX > dt-bindings: media: Add DT bindings for TI CSI2RX driver > media: ti-vpe: csi2rx: Add CSI2RX support > dt-bindings: phy: Convert Cadence DPHY binding to YAML > dt-bindings: phy: cdns,dphy: make clocks optional > dt-bindings: phy: cdns,dphy: add power-domains property Is there any dependency between patches to various subsystems, if not please do consider sending a series per subsystem... Thanks > > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 ++ > .../devicetree/bindings/phy/cdns,dphy.txt | 20 - > .../devicetree/bindings/phy/cdns,dphy.yaml | 52 + > MAINTAINERS | 7 + > drivers/dma/ti/k3-psil-j721e.c | 10 + > drivers/media/platform/Kconfig | 11 + > drivers/media/platform/cadence/cdns-csi2rx.c | 269 ++++- > drivers/media/platform/ti-vpe/Makefile | 1 + > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++ > drivers/phy/cadence/cdns-dphy.c | 407 +++++++- > include/linux/phy/phy-mipi-dphy.h | 13 + > 11 files changed, 1754 insertions(+), 70 deletions(-) > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml > delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt > create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c > > -- > 2.30.0
On 31/03/21 03:03PM, Vinod Koul wrote: > On 30-03-21, 23:03, Pratyush Yadav wrote: > > Hi, > > > > This series adds support for CSI2 capture on J721E. It includes some > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > Tested on TI's J721E with OV5640 sensor. > > > > Paul Kocialkowski (1): > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > > > Pratyush Yadav (15): > > phy: cdns-dphy: Prepare for Rx support > > phy: cdns-dphy: Allow setting mode > > phy: cdns-dphy: Add Rx support > > media: cadence: csi2rx: Add external DPHY support > > media: cadence: csi2rx: Soft reset the streams before starting capture > > media: cadence: csi2rx: Set the STOP bit when stopping a stream > > media: cadence: csi2rx: Fix stream data configuration > > media: cadence: csi2rx: Turn subdev power on before starting stream > > media: cadence: csi2rx: Add wrappers for subdev calls > > dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX > > dt-bindings: media: Add DT bindings for TI CSI2RX driver > > media: ti-vpe: csi2rx: Add CSI2RX support > > dt-bindings: phy: Convert Cadence DPHY binding to YAML > > dt-bindings: phy: cdns,dphy: make clocks optional > > dt-bindings: phy: cdns,dphy: add power-domains property > > Is there any dependency between patches to various subsystems, if not > please do consider sending a series per subsystem... Without patch 1, patch 5 and later won't build. Without patch 11, patch 13 will not work. > > Thanks > > > > > > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 ++ > > .../devicetree/bindings/phy/cdns,dphy.txt | 20 - > > .../devicetree/bindings/phy/cdns,dphy.yaml | 52 + > > MAINTAINERS | 7 + > > drivers/dma/ti/k3-psil-j721e.c | 10 + > > drivers/media/platform/Kconfig | 11 + > > drivers/media/platform/cadence/cdns-csi2rx.c | 269 ++++- > > drivers/media/platform/ti-vpe/Makefile | 1 + > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++ > > drivers/phy/cadence/cdns-dphy.c | 407 +++++++- > > include/linux/phy/phy-mipi-dphy.h | 13 + > > 11 files changed, 1754 insertions(+), 70 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml > > delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt > > create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c > > > > -- > > 2.30.0 > > -- > ~Vinod
On 31-03-21, 17:10, Pratyush Yadav wrote: > On 31/03/21 03:03PM, Vinod Koul wrote: > > On 30-03-21, 23:03, Pratyush Yadav wrote: > > > Hi, > > > > > > This series adds support for CSI2 capture on J721E. It includes some > > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > > > Tested on TI's J721E with OV5640 sensor. > > > > > > Paul Kocialkowski (1): > > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > > > > > Pratyush Yadav (15): > > > phy: cdns-dphy: Prepare for Rx support > > > phy: cdns-dphy: Allow setting mode > > > phy: cdns-dphy: Add Rx support > > > media: cadence: csi2rx: Add external DPHY support > > > media: cadence: csi2rx: Soft reset the streams before starting capture > > > media: cadence: csi2rx: Set the STOP bit when stopping a stream > > > media: cadence: csi2rx: Fix stream data configuration > > > media: cadence: csi2rx: Turn subdev power on before starting stream > > > media: cadence: csi2rx: Add wrappers for subdev calls > > > dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX > > > dt-bindings: media: Add DT bindings for TI CSI2RX driver > > > media: ti-vpe: csi2rx: Add CSI2RX support > > > dt-bindings: phy: Convert Cadence DPHY binding to YAML > > > dt-bindings: phy: cdns,dphy: make clocks optional > > > dt-bindings: phy: cdns,dphy: add power-domains property > > > > Is there any dependency between patches to various subsystems, if not > > please do consider sending a series per subsystem... > > Without patch 1, patch 5 and later won't build. Without patch 11, patch > 13 will not work. Cover letter is a good place to mention this! And what do you mean by not working, do we have compile time dependencies? I agree that for everything to work all the pieces need to land
On 31/03/21 06:36PM, Vinod Koul wrote: > On 31-03-21, 17:10, Pratyush Yadav wrote: > > On 31/03/21 03:03PM, Vinod Koul wrote: > > > On 30-03-21, 23:03, Pratyush Yadav wrote: > > > > Hi, > > > > > > > > This series adds support for CSI2 capture on J721E. It includes some > > > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > > > > > Tested on TI's J721E with OV5640 sensor. > > > > > > > > Paul Kocialkowski (1): > > > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > > > > > > > Pratyush Yadav (15): > > > > phy: cdns-dphy: Prepare for Rx support > > > > phy: cdns-dphy: Allow setting mode > > > > phy: cdns-dphy: Add Rx support > > > > media: cadence: csi2rx: Add external DPHY support > > > > media: cadence: csi2rx: Soft reset the streams before starting capture > > > > media: cadence: csi2rx: Set the STOP bit when stopping a stream > > > > media: cadence: csi2rx: Fix stream data configuration > > > > media: cadence: csi2rx: Turn subdev power on before starting stream > > > > media: cadence: csi2rx: Add wrappers for subdev calls > > > > dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX > > > > dt-bindings: media: Add DT bindings for TI CSI2RX driver > > > > media: ti-vpe: csi2rx: Add CSI2RX support > > > > dt-bindings: phy: Convert Cadence DPHY binding to YAML > > > > dt-bindings: phy: cdns,dphy: make clocks optional > > > > dt-bindings: phy: cdns,dphy: add power-domains property > > > > > > Is there any dependency between patches to various subsystems, if not > > > please do consider sending a series per subsystem... > > > > Without patch 1, patch 5 and later won't build. Without patch 11, patch > > 13 will not work. > > Cover letter is a good place to mention this! And what do you mean by > not working, do we have compile time dependencies? I agree that for > everything to work all the pieces need to land I have not tried it but patch 11 is not a compile time dependency. It should be a run time dependency. Without it the driver will probably fail to acquire the DMA channels and its probe will fail.
Pratyush, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote on Wed [2021-Mar-31 09:06:35 +0300]: > Hi, > > On 30/03/2021 20:33, Pratyush Yadav wrote: > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > > capture over a CSI-2 bus. > > > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > > called the SHIM layer. It takes in data from stream 0, repacks it, and > > sends it to memory over PSI-L DMA. > > > > This driver acts as the "front end" to V4L2 client applications. It > > implements the required ioctls and buffer operations, passes the > > necessary calls on to the bridge, programs the SHIM layer, and performs > > DMA via the dmaengine API to finally return the data to a buffer > > supplied by the application. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > MAINTAINERS | 7 + > > drivers/media/platform/Kconfig | 11 + > > drivers/media/platform/ti-vpe/Makefile | 1 + > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++++++ > > 4 files changed, 983 insertions(+) > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c > > Some quick comments: > > "ti-vpe" directory is not correct, this has nothing to do with VPE. That > said, the directory has already been abused by having CAL driver there, > perhaps we should rename the directory just to "ti". But if we do that, > I think we should have subdirs for cal, vpe and this new one. I agree with Tomi here. This should create a ti directory under media/platform and then add a directory under that specifically for this driver/IP as a first step. Not sure what the correct name for that directory should be but it should meaningful. As a follow on step then the other drivers can be relocated to a proper directory structure. > > "ti-csi2rx" is rather generic name. TI has had CSI-2 RX IPs before (CAL) > and probably will also have new ones in the future. If there's no clear > model name for the IP, as I think is the case here, it's probably best > to just use the SoC model in the name. E.g. the DSS on J7 is > "ti,j721e-dss". > > This driver implements the legacy video API. I think it would be better > (and easier to maintain) to only implement the media-controller API, > unless you specifically need to support the legacy API for existing > userspace. We just went through a major rework with CAL to make it media controller compatible in order to be able to handle CSI2 virtual channels. I think as this is a new driver/IP which perform the same type of service it makes sense to make use the more current API instead of the legacy one. > > Tomi Benoit
On Wed, Mar 31, 2021 at 10:44:57AM -0500, Benoit Parrot wrote: > Pratyush, > > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote on Wed [2021-Mar-31 09:06:35 +0300]: > > Hi, > > > > On 30/03/2021 20:33, Pratyush Yadav wrote: > > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > > > capture over a CSI-2 bus. > > > > > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > > > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > > > called the SHIM layer. It takes in data from stream 0, repacks it, and > > > sends it to memory over PSI-L DMA. > > > > > > This driver acts as the "front end" to V4L2 client applications. It > > > implements the required ioctls and buffer operations, passes the > > > necessary calls on to the bridge, programs the SHIM layer, and performs > > > DMA via the dmaengine API to finally return the data to a buffer > > > supplied by the application. > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > --- > > > MAINTAINERS | 7 + > > > drivers/media/platform/Kconfig | 11 + > > > drivers/media/platform/ti-vpe/Makefile | 1 + > > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++++++ > > > 4 files changed, 983 insertions(+) > > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c > > > > Some quick comments: > > > > "ti-vpe" directory is not correct, this has nothing to do with VPE. That > > said, the directory has already been abused by having CAL driver there, > > perhaps we should rename the directory just to "ti". But if we do that, > > I think we should have subdirs for cal, vpe and this new one. > > I agree with Tomi here. This should create a ti directory under > media/platform and then add a directory under that specifically for this > driver/IP as a first step. Not sure what the correct name for that > directory should be but it should meaningful. As a follow on step then the > other drivers can be relocated to a proper directory structure. +1, including for the relocation if possible. > > "ti-csi2rx" is rather generic name. TI has had CSI-2 RX IPs before (CAL) > > and probably will also have new ones in the future. If there's no clear > > model name for the IP, as I think is the case here, it's probably best > > to just use the SoC model in the name. E.g. the DSS on J7 is > > "ti,j721e-dss". > > > > This driver implements the legacy video API. I think it would be better > > (and easier to maintain) to only implement the media-controller API, > > unless you specifically need to support the legacy API for existing > > userspace. > > We just went through a major rework with CAL to make it media controller > compatible in order to be able to handle CSI2 virtual channels. > I think as this is a new driver/IP which perform the same type of service > it makes sense to make use the more current API instead of the legacy one. +2 :-)
Hi Pratyush, Thank you for the patch. On Tue, Mar 30, 2021 at 11:03:35PM +0530, Pratyush Yadav wrote: > Allow callers to set the PHY mode. The main mode should always be > PHY_MODE_MIPI_DPHY but the submode can either be > PHY_MIPI_DPHY_SUBMODE_RX or PHY_MIPI_DPHY_SUBMODE_TX. Update the ops > based on the requested submode. Isn't a given DPHY instance always meant to work in one particular mode ? I can't really imagine a single instance of this IP core being integrated in a way that it can be used in either RX or TX mode. It seems better to select the mode through DT, by describing if the DPHY is an RX or TX (possibly through different compatible strings). > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/phy/cadence/cdns-dphy.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c > index 8656f2102a91..7d5f7b333893 100644 > --- a/drivers/phy/cadence/cdns-dphy.c > +++ b/drivers/phy/cadence/cdns-dphy.c > @@ -365,11 +365,41 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts) > return 0; > } > > +static int cdns_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode) > +{ > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > + const struct cdns_dphy_driver_data *ddata; > + > + ddata = of_device_get_match_data(dphy->dev); > + if (!ddata) > + return -EINVAL; > + > + if (mode != PHY_MODE_MIPI_DPHY) > + return -EINVAL; > + > + if (submode == PHY_MIPI_DPHY_SUBMODE_TX) { > + if (!ddata->tx) > + return -EOPNOTSUPP; > + > + dphy->ops = ddata->tx; > + } else if (submode == PHY_MIPI_DPHY_SUBMODE_RX) { > + if (!ddata->rx) > + return -EOPNOTSUPP; > + > + dphy->ops = ddata->rx; > + } else { > + return -EOPNOTSUPP; > + } > + > + return 0; > +} > + > static const struct phy_ops cdns_dphy_ops = { > .configure = cdns_dphy_configure, > .validate = cdns_dphy_validate, > .power_on = cdns_dphy_power_on, > .power_off = cdns_dphy_power_off, > + .set_mode = cdns_dphy_set_mode, > }; > > static int cdns_dphy_probe(struct platform_device *pdev)
Hi Pratyush, Thank you for the patch. On Tue, Mar 30, 2021 at 11:03:42PM +0530, Pratyush Yadav wrote: > When this bridge driver is being user by another platform driver, it > might want to call subdev operations like getting format, setting > format, enumerating format codes, etc. Add wrapper functions that pass > that call through to the sensor. > > Currently wrappers are added only for the ops used by TI's platform > driver. More can be added later as needed. This isn't the direction we want to take. For new platforms, propagation of subdev configuration should be handled by userspace, using the V4L2 userspace subdev API. This subdev should not call any subdev operation from its source other than .s_stream(). > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/media/platform/cadence/cdns-csi2rx.c | 77 ++++++++++++++++++++ > 1 file changed, 77 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 3385e1bc213e..2e8bbc53cb8b 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -408,12 +408,89 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > return ret; > } > > +static int csi2rx_g_frame_interval(struct v4l2_subdev *subdev, > + struct v4l2_subdev_frame_interval *fi) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + > + return v4l2_subdev_call(csi2rx->source_subdev, video, g_frame_interval, > + fi); > +} > + > +static int csi2rx_s_frame_interval(struct v4l2_subdev *subdev, > + struct v4l2_subdev_frame_interval *fi) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + > + return v4l2_subdev_call(csi2rx->source_subdev, video, s_frame_interval, > + fi); > +} > + > +static int csi2rx_enum_mbus_code(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_mbus_code, > + cfg, code); > +} > + > +static int csi2rx_get_fmt(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + > + return v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, cfg, fmt); > +} > + > +static int csi2rx_set_fmt(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + > + return v4l2_subdev_call(csi2rx->source_subdev, pad, set_fmt, cfg, fmt); > +} > + > +static int csi2rx_enum_frame_size(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_size, > + cfg, fse); > +} > + > +static int csi2rx_enum_frame_interval(struct v4l2_subdev *subdev, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_interval_enum *fie) > +{ > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > + > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_interval, > + cfg, fie); > +} > + > static const struct v4l2_subdev_video_ops csi2rx_video_ops = { > .s_stream = csi2rx_s_stream, > + .g_frame_interval = csi2rx_g_frame_interval, > + .s_frame_interval = csi2rx_s_frame_interval, > +}; > + > +static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = { > + .enum_mbus_code = csi2rx_enum_mbus_code, > + .get_fmt = csi2rx_get_fmt, > + .set_fmt = csi2rx_set_fmt, > + .enum_frame_size = csi2rx_enum_frame_size, > + .enum_frame_interval = csi2rx_enum_frame_interval, > }; > > static const struct v4l2_subdev_ops csi2rx_subdev_ops = { > .video = &csi2rx_video_ops, > + .pad = &csi2rx_pad_ops, > }; > > static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
Hi Pratyush, Thank you for the patch. On Tue, Mar 30, 2021 at 11:03:41PM +0530, Pratyush Yadav wrote: > The subdevice power needs to be turned on before the stream is started. > Otherwise it might not be in the proper state to stream the data. Turn > it off when stopping the stream. The .s_power() operation is deprecated. Subdev drivers should control power internally in their .s_stream() operation, and they should use runtime PM to do so (this will allow usage of runtime PM autosuspend, to avoid expensive power off/on cycles when stopping and restarting video capture). > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/media/platform/cadence/cdns-csi2rx.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 7d1ac51e0698..3385e1bc213e 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG); > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true); > + if (ret && ret != -ENOIOCTLCMD) > + goto err_disable_pclk; > + > ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true); > if (ret) > goto err_disable_pclk; > @@ -358,6 +362,10 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false); > + if (ret && ret != -ENOIOCTLCMD) > + dev_warn(csi2rx->dev, "Couldn't power off subdev\n"); > + > if (csi2rx->dphy) { > writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); >
On Wed, Mar 31, 2021 at 03:03:51PM +0530, Vinod Koul wrote: > On 30-03-21, 23:03, Pratyush Yadav wrote: > > Hi, > > > > This series adds support for CSI2 capture on J721E. It includes some > > fixes to the Cadence CSI2RX driver, adds Rx support to Cadence DPHY > > driver, and finally adds the TI CSI2RX wrapper driver. > > > > Tested on TI's J721E with OV5640 sensor. > > > > Paul Kocialkowski (1): > > phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes > > > > Pratyush Yadav (15): > > phy: cdns-dphy: Prepare for Rx support > > phy: cdns-dphy: Allow setting mode > > phy: cdns-dphy: Add Rx support > > media: cadence: csi2rx: Add external DPHY support > > media: cadence: csi2rx: Soft reset the streams before starting capture > > media: cadence: csi2rx: Set the STOP bit when stopping a stream > > media: cadence: csi2rx: Fix stream data configuration > > media: cadence: csi2rx: Turn subdev power on before starting stream > > media: cadence: csi2rx: Add wrappers for subdev calls > > dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX > > dt-bindings: media: Add DT bindings for TI CSI2RX driver > > media: ti-vpe: csi2rx: Add CSI2RX support > > dt-bindings: phy: Convert Cadence DPHY binding to YAML > > dt-bindings: phy: cdns,dphy: make clocks optional > > dt-bindings: phy: cdns,dphy: add power-domains property > > Is there any dependency between patches to various subsystems, if not > please do consider sending a series per subsystem... Splitting the series per subsystem will facilitate merging, but for the first versions, keeping all patches together facilitate review. I'd prefer if we could have a v2 that still includes everything, until we agree on the interface between the two subsystems. At that point, we can split the series if needed. > > > > .../devicetree/bindings/media/ti,csi2rx.yaml | 70 ++ > > .../devicetree/bindings/phy/cdns,dphy.txt | 20 - > > .../devicetree/bindings/phy/cdns,dphy.yaml | 52 + > > MAINTAINERS | 7 + > > drivers/dma/ti/k3-psil-j721e.c | 10 + > > drivers/media/platform/Kconfig | 11 + > > drivers/media/platform/cadence/cdns-csi2rx.c | 269 ++++- > > drivers/media/platform/ti-vpe/Makefile | 1 + > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++ > > drivers/phy/cadence/cdns-dphy.c | 407 +++++++- > > include/linux/phy/phy-mipi-dphy.h | 13 + > > 11 files changed, 1754 insertions(+), 70 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml > > delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt > > create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c
Hi Pratyush, On 3/30/21 8:33 PM, Pratyush Yadav wrote: > The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > have up to 32 threads but the current driver only supports using one. So > add an entry for that one thread. If you are absolutely sure that the other threads are not going to be used, then: Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com> but I would consider adding the other threads if there is a chance that the cs2rx will need to support it in the future. > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/dma/ti/k3-psil-j721e.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c > index 7580870ed746..19ffa31e6dc6 100644 > --- a/drivers/dma/ti/k3-psil-j721e.c > +++ b/drivers/dma/ti/k3-psil-j721e.c > @@ -58,6 +58,14 @@ > }, \ > } > > +#define PSIL_CSI2RX(x) \ > + { \ > + .thread_id = x, \ > + .ep_config = { \ > + .ep_type = PSIL_EP_NATIVE, \ > + }, \ > + } > + > /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */ > static struct psil_ep j721e_src_ep_map[] = { > /* SA2UL */ > @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = { > PSIL_PDMA_XY_PKT(0x4707), > PSIL_PDMA_XY_PKT(0x4708), > PSIL_PDMA_XY_PKT(0x4709), > + /* CSI2RX */ > + PSIL_CSI2RX(0x4940), > /* CPSW9 */ > PSIL_ETHERNET(0x4a00), > /* CPSW0 */ >
Hi Pratyush, +1 from me also to the points Tomi raised. few minor comments on the DMAengie side. On 3/30/21 8:33 PM, Pratyush Yadav wrote: > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > capture over a CSI-2 bus. > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > called the SHIM layer. It takes in data from stream 0, repacks it, and > sends it to memory over PSI-L DMA. > > This driver acts as the "front end" to V4L2 client applications. It > implements the required ioctls and buffer operations, passes the > necessary calls on to the bridge, programs the SHIM layer, and performs > DMA via the dmaengine API to finally return the data to a buffer > supplied by the application. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > MAINTAINERS | 7 + > drivers/media/platform/Kconfig | 11 + > drivers/media/platform/ti-vpe/Makefile | 1 + > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++++++ > 4 files changed, 983 insertions(+) > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c ... > diff --git a/drivers/media/platform/ti-vpe/ti-csi2rx.c b/drivers/media/platform/ti-vpe/ti-csi2rx.c > new file mode 100644 > index 000000000000..355204ae473b > --- /dev/null > +++ b/drivers/media/platform/ti-vpe/ti-csi2rx.c ... > +static int ti_csi2rx_init_vb2q(struct ti_csi2rx_dev *csi) > +{ > + struct vb2_queue *q = &csi->vidq; > + int ret; > + > + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > + q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ; > + q->drv_priv = csi; > + q->buf_struct_size = sizeof(struct ti_csi2rx_buffer); > + q->ops = &csi_vb2_qops; > + q->mem_ops = &vb2_dma_contig_memops; > + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > + q->dev = csi->dma->device->dev; q->dev = dmaengine_get_dma_device(csi->dma); > + q->lock = &csi->mutex; > + > + ret = vb2_queue_init(q); > + if (ret) > + return ret; > + > + csi->vdev.queue = q; > + > + return 0; > +} > + > +static int ti_csi2rx_init_dma(struct ti_csi2rx_dev *csi) > +{ > + struct dma_slave_config cfg; > + int ret; > + > + INIT_LIST_HEAD(&csi->dmaq.list); > + > + csi->dma = NULL; > + > + csi->dma = dma_request_chan(csi->dev, "rx0"); > + if (IS_ERR(csi->dma)) > + return PTR_ERR(csi->dma); > + > + memset(&cfg, 0, sizeof(cfg)); > + > + cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_16_BYTES; > + cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_16_BYTES; No need to set the dst_addr_width as you only support RX. Another note: UDMA with PSI-L native peripherals ignores this cofniguration, only used for PDMAs, but I can remain to future proof the driver and to keep it generic. > + > + ret = dmaengine_slave_config(csi->dma, &cfg); > + if (ret) > + return ret; > + > + return 0; > +}
On 31/03/21 09:06AM, Tomi Valkeinen wrote: > Hi, > > On 30/03/2021 20:33, Pratyush Yadav wrote: > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate > > capture over a CSI-2 bus. > > > > The Cadence CSI2RX IP acts as a bridge between the TI specific parts and > > the CSI-2 protocol parts. TI then has a wrapper on top of this bridge > > called the SHIM layer. It takes in data from stream 0, repacks it, and > > sends it to memory over PSI-L DMA. > > > > This driver acts as the "front end" to V4L2 client applications. It > > implements the required ioctls and buffer operations, passes the > > necessary calls on to the bridge, programs the SHIM layer, and performs > > DMA via the dmaengine API to finally return the data to a buffer > > supplied by the application. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > MAINTAINERS | 7 + > > drivers/media/platform/Kconfig | 11 + > > drivers/media/platform/ti-vpe/Makefile | 1 + > > drivers/media/platform/ti-vpe/ti-csi2rx.c | 964 ++++++++++++++++++++++ > > 4 files changed, 983 insertions(+) > > create mode 100644 drivers/media/platform/ti-vpe/ti-csi2rx.c > > Some quick comments: > > "ti-vpe" directory is not correct, this has nothing to do with VPE. That > said, the directory has already been abused by having CAL driver there, > perhaps we should rename the directory just to "ti". But if we do that, I > think we should have subdirs for cal, vpe and this new one. Right. I thought about doing this but then figured "let's just follow what CAL does". Will move them into separate subdirectories. > > "ti-csi2rx" is rather generic name. TI has had CSI-2 RX IPs before (CAL) and > probably will also have new ones in the future. If there's no clear model > name for the IP, as I think is the case here, it's probably best to just use > the SoC model in the name. E.g. the DSS on J7 is "ti,j721e-dss". Ok. > > This driver implements the legacy video API. I think it would be better (and > easier to maintain) to only implement the media-controller API, unless you > specifically need to support the legacy API for existing userspace. :-( I'm afraid the documentation has let me down here. There is no clear mention about the fact that media controller API replaces the "legacy" API. In fact, [0] seems to suggest that the media controller API is optional. Bridge drivers traditionally expose one or multiple video nodes to userspace, and control subdevices through the v4l2_subdev_ops operations in response to video node operations. This hides the complexity of the underlying hardware from applications. For complex devices, finer-grained control of the device than what the video nodes offer may be required. In those cases, bridge drivers that implement the media controller API may opt for making the subdevice operations directly accessible from userpace. Anyway, I don't think supporting the legacy API makes much sense since this is a new driver. Will convert it to use the MC API. We can always add the legacy API support if some application demands it. [0] https://www.kernel.org/doc/html/latest/driver-api/media/v4l2-subdev.html#v4l2-sub-device-userspace-api
On 04/04/21 04:24PM, Péter Ujfalusi wrote: > Hi Pratyush, > > On 3/30/21 8:33 PM, Pratyush Yadav wrote: > > The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > > have up to 32 threads but the current driver only supports using one. So > > add an entry for that one thread. > > If you are absolutely sure that the other threads are not going to be > used, then: The opposite in fact. I do expect other threads to be used in the future. But the current driver can only use one so I figured it is better to add just the thread that is currently needed and then I can always add the rest later. Why does this have to be a one-and-done deal? Is there anything wrong with adding the other threads when the driver can actually use them? > Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com> > > but I would consider adding the other threads if there is a chance that > the cs2rx will need to support it in the future. > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/dma/ti/k3-psil-j721e.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c > > index 7580870ed746..19ffa31e6dc6 100644 > > --- a/drivers/dma/ti/k3-psil-j721e.c > > +++ b/drivers/dma/ti/k3-psil-j721e.c > > @@ -58,6 +58,14 @@ > > }, \ > > } > > > > +#define PSIL_CSI2RX(x) \ > > + { \ > > + .thread_id = x, \ > > + .ep_config = { \ > > + .ep_type = PSIL_EP_NATIVE, \ > > + }, \ > > + } > > + > > /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */ > > static struct psil_ep j721e_src_ep_map[] = { > > /* SA2UL */ > > @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = { > > PSIL_PDMA_XY_PKT(0x4707), > > PSIL_PDMA_XY_PKT(0x4708), > > PSIL_PDMA_XY_PKT(0x4709), > > + /* CSI2RX */ > > + PSIL_CSI2RX(0x4940), > > /* CPSW9 */ > > PSIL_ETHERNET(0x4a00), > > /* CPSW0 */ > > > > -- > Péter
On 02/04/21 01:47PM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. Thank you for the review :-) > > On Tue, Mar 30, 2021 at 11:03:42PM +0530, Pratyush Yadav wrote: > > When this bridge driver is being user by another platform driver, it > > might want to call subdev operations like getting format, setting > > format, enumerating format codes, etc. Add wrapper functions that pass > > that call through to the sensor. > > > > Currently wrappers are added only for the ops used by TI's platform > > driver. More can be added later as needed. > > This isn't the direction we want to take. For new platforms, propagation > of subdev configuration should be handled by userspace, using the V4L2 > userspace subdev API. This subdev should not call any subdev operation > from its source other than .s_stream(). Right. I have replied to Tomi's message about this too. Will move the driver to use the media controller API. > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/media/platform/cadence/cdns-csi2rx.c | 77 ++++++++++++++++++++ > > 1 file changed, 77 insertions(+) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index 3385e1bc213e..2e8bbc53cb8b 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -408,12 +408,89 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > return ret; > > } > > > > +static int csi2rx_g_frame_interval(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_frame_interval *fi) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + > > + return v4l2_subdev_call(csi2rx->source_subdev, video, g_frame_interval, > > + fi); > > +} > > + > > +static int csi2rx_s_frame_interval(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_frame_interval *fi) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + > > + return v4l2_subdev_call(csi2rx->source_subdev, video, s_frame_interval, > > + fi); > > +} > > + > > +static int csi2rx_enum_mbus_code(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_mbus_code_enum *code) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + > > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_mbus_code, > > + cfg, code); > > +} > > + > > +static int csi2rx_get_fmt(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + > > + return v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, cfg, fmt); > > +} > > + > > +static int csi2rx_set_fmt(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_format *fmt) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + > > + return v4l2_subdev_call(csi2rx->source_subdev, pad, set_fmt, cfg, fmt); > > +} > > + > > +static int csi2rx_enum_frame_size(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_frame_size_enum *fse) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + > > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_size, > > + cfg, fse); > > +} > > + > > +static int csi2rx_enum_frame_interval(struct v4l2_subdev *subdev, > > + struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_frame_interval_enum *fie) > > +{ > > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev); > > + > > + return v4l2_subdev_call(csi2rx->source_subdev, pad, enum_frame_interval, > > + cfg, fie); > > +} > > + > > static const struct v4l2_subdev_video_ops csi2rx_video_ops = { > > .s_stream = csi2rx_s_stream, > > + .g_frame_interval = csi2rx_g_frame_interval, > > + .s_frame_interval = csi2rx_s_frame_interval, > > +}; > > + > > +static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = { > > + .enum_mbus_code = csi2rx_enum_mbus_code, > > + .get_fmt = csi2rx_get_fmt, > > + .set_fmt = csi2rx_set_fmt, > > + .enum_frame_size = csi2rx_enum_frame_size, > > + .enum_frame_interval = csi2rx_enum_frame_interval, > > }; > > > > static const struct v4l2_subdev_ops csi2rx_subdev_ops = { > > .video = &csi2rx_video_ops, > > + .pad = &csi2rx_pad_ops, > > }; > > > > static int csi2rx_async_bound(struct v4l2_async_notifier *notifier, > > -- > Regards, > > Laurent Pinchart
On 4/6/21 6:09 PM, Pratyush Yadav wrote: > On 04/04/21 04:24PM, Péter Ujfalusi wrote: >> Hi Pratyush, >> >> On 3/30/21 8:33 PM, Pratyush Yadav wrote: >>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can >>> have up to 32 threads but the current driver only supports using one. So >>> add an entry for that one thread. >> >> If you are absolutely sure that the other threads are not going to be >> used, then: > > The opposite in fact. I do expect other threads to be used in the > future. But the current driver can only use one so I figured it is > better to add just the thread that is currently needed and then I can > always add the rest later. > > Why does this have to be a one-and-done deal? Is there anything wrong > with adding the other threads when the driver can actually use them? You can skip CCing DMAengine (and me ;) ). Less subsystems is the better when sending patches... > >> Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com> >> >> but I would consider adding the other threads if there is a chance that >> the cs2rx will need to support it in the future. >> >>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> >>> --- >>> drivers/dma/ti/k3-psil-j721e.c | 10 ++++++++++ >>> 1 file changed, 10 insertions(+) >>> >>> diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c >>> index 7580870ed746..19ffa31e6dc6 100644 >>> --- a/drivers/dma/ti/k3-psil-j721e.c >>> +++ b/drivers/dma/ti/k3-psil-j721e.c >>> @@ -58,6 +58,14 @@ >>> }, \ >>> } >>> >>> +#define PSIL_CSI2RX(x) \ >>> + { \ >>> + .thread_id = x, \ >>> + .ep_config = { \ >>> + .ep_type = PSIL_EP_NATIVE, \ >>> + }, \ >>> + } >>> + >>> /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */ >>> static struct psil_ep j721e_src_ep_map[] = { >>> /* SA2UL */ >>> @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = { >>> PSIL_PDMA_XY_PKT(0x4707), >>> PSIL_PDMA_XY_PKT(0x4708), >>> PSIL_PDMA_XY_PKT(0x4709), >>> + /* CSI2RX */ >>> + PSIL_CSI2RX(0x4940), >>> /* CPSW9 */ >>> PSIL_ETHERNET(0x4a00), >>> /* CPSW0 */ >>> >> >> -- >> Péter >
On 06/04/21 06:33PM, Péter Ujfalusi wrote: > > > On 4/6/21 6:09 PM, Pratyush Yadav wrote: > > On 04/04/21 04:24PM, Péter Ujfalusi wrote: > >> Hi Pratyush, > >> > >> On 3/30/21 8:33 PM, Pratyush Yadav wrote: > >>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > >>> have up to 32 threads but the current driver only supports using one. So > >>> add an entry for that one thread. > >> > >> If you are absolutely sure that the other threads are not going to be > >> used, then: > > > > The opposite in fact. I do expect other threads to be used in the > > future. But the current driver can only use one so I figured it is > > better to add just the thread that is currently needed and then I can > > always add the rest later. > > > > Why does this have to be a one-and-done deal? Is there anything wrong > > with adding the other threads when the driver can actually use them? > > You can skip CCing DMAengine (and me ;) ). Less subsystems is the better > when sending patches... I'm a bit confused here. If you are no longer interested in maintaining the TI DMA drivers then that's fine, I can skip CCing you. But the patch is still relevant to the dmaengine list so why should I skip CCing it? And if I don't CC the dmaengine list then on which list would I get comments/reviews for the patch? > > > > >> Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com> > >> > >> but I would consider adding the other threads if there is a chance that > >> the cs2rx will need to support it in the future. > >> > >>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > >>> --- > >>> drivers/dma/ti/k3-psil-j721e.c | 10 ++++++++++ > >>> 1 file changed, 10 insertions(+) > >>> > >>> diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c > >>> index 7580870ed746..19ffa31e6dc6 100644 > >>> --- a/drivers/dma/ti/k3-psil-j721e.c > >>> +++ b/drivers/dma/ti/k3-psil-j721e.c > >>> @@ -58,6 +58,14 @@ > >>> }, \ > >>> } > >>> > >>> +#define PSIL_CSI2RX(x) \ > >>> + { \ > >>> + .thread_id = x, \ > >>> + .ep_config = { \ > >>> + .ep_type = PSIL_EP_NATIVE, \ > >>> + }, \ > >>> + } > >>> + > >>> /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */ > >>> static struct psil_ep j721e_src_ep_map[] = { > >>> /* SA2UL */ > >>> @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = { > >>> PSIL_PDMA_XY_PKT(0x4707), > >>> PSIL_PDMA_XY_PKT(0x4708), > >>> PSIL_PDMA_XY_PKT(0x4709), > >>> + /* CSI2RX */ > >>> + PSIL_CSI2RX(0x4940), > >>> /* CPSW9 */ > >>> PSIL_ETHERNET(0x4a00), > >>> /* CPSW0 */ > >>> > >> > >> -- > >> Péter > > > > -- > Péter
On 06/04/21 10:25PM, Pratyush Yadav wrote: > On 06/04/21 06:33PM, Péter Ujfalusi wrote: > > > > > > On 4/6/21 6:09 PM, Pratyush Yadav wrote: > > > On 04/04/21 04:24PM, Péter Ujfalusi wrote: > > >> Hi Pratyush, > > >> > > >> On 3/30/21 8:33 PM, Pratyush Yadav wrote: > > >>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > > >>> have up to 32 threads but the current driver only supports using one. So > > >>> add an entry for that one thread. > > >> > > >> If you are absolutely sure that the other threads are not going to be > > >> used, then: > > > > > > The opposite in fact. I do expect other threads to be used in the > > > future. But the current driver can only use one so I figured it is > > > better to add just the thread that is currently needed and then I can > > > always add the rest later. > > > > > > Why does this have to be a one-and-done deal? Is there anything wrong > > > with adding the other threads when the driver can actually use them? > > > > You can skip CCing DMAengine (and me ;) ). Less subsystems is the better > > when sending patches... > > I'm a bit confused here. If you are no longer interested in maintaining > the TI DMA drivers then that's fine, I can skip CCing you. But the patch > is still relevant to the dmaengine list so why should I skip CCing it? > And if I don't CC the dmaengine list then on which list would I get > comments/reviews for the patch? Ignore this. Got your point. Will do it in v2.
On 02/04/21 01:55PM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Tue, Mar 30, 2021 at 11:03:41PM +0530, Pratyush Yadav wrote: > > The subdevice power needs to be turned on before the stream is started. > > Otherwise it might not be in the proper state to stream the data. Turn > > it off when stopping the stream. > > The .s_power() operation is deprecated. Subdev drivers should control > power internally in their .s_stream() operation, and they should use > runtime PM to do so (this will allow usage of runtime PM autosuspend, to > avoid expensive power off/on cycles when stopping and restarting video > capture). The s_power documentation in v4l2-subdev.h does not mention that it is depreciated. A documentation update is in order. I will send a separate patch to do it. I tested this with OV5640. Not doing an s_power() operation before s_stream() does not work. The application freezes forever waiting for the first frame. So the OV5640 driver needs to be updated to use runtime PM to do this, right? > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/media/platform/cadence/cdns-csi2rx.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index 7d1ac51e0698..3385e1bc213e 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > > > writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG); > > > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true); > > + if (ret && ret != -ENOIOCTLCMD) > > + goto err_disable_pclk; > > + > > ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true); > > if (ret) > > goto err_disable_pclk; > > @@ -358,6 +362,10 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > > > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false); > > + if (ret && ret != -ENOIOCTLCMD) > > + dev_warn(csi2rx->dev, "Couldn't power off subdev\n"); > > + > > if (csi2rx->dphy) { > > writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > > > -- > Regards, > > Laurent Pinchart
On 02/04/21 01:38PM, Laurent Pinchart wrote: > Hi Pratyush, > > Thank you for the patch. > > On Tue, Mar 30, 2021 at 11:03:35PM +0530, Pratyush Yadav wrote: > > Allow callers to set the PHY mode. The main mode should always be > > PHY_MODE_MIPI_DPHY but the submode can either be > > PHY_MIPI_DPHY_SUBMODE_RX or PHY_MIPI_DPHY_SUBMODE_TX. Update the ops > > based on the requested submode. > > Isn't a given DPHY instance always meant to work in one particular mode > ? I can't really imagine a single instance of this IP core being > integrated in a way that it can be used in either RX or TX mode. It > seems better to select the mode through DT, by describing if the DPHY is > an RX or TX (possibly through different compatible strings). I'm not sure if the DPHY can work in both RX and TX mode but the documentation for Cadence DPHY on J721E does include both RX and TX related registers. Also, take a look at [0] which says that the Allwinner A31 DPHY can work in both RX and TX mode. So apparently there are some DPHYs like that in the wild. [0] https://lore.kernel.org/linux-arm-kernel/20201023174546.504028-3-paul.kocialkowski@bootlin.com/ > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/phy/cadence/cdns-dphy.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c > > index 8656f2102a91..7d5f7b333893 100644 > > --- a/drivers/phy/cadence/cdns-dphy.c > > +++ b/drivers/phy/cadence/cdns-dphy.c > > @@ -365,11 +365,41 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts) > > return 0; > > } > > > > +static int cdns_dphy_set_mode(struct phy *phy, enum phy_mode mode, int submode) > > +{ > > + struct cdns_dphy *dphy = phy_get_drvdata(phy); > > + const struct cdns_dphy_driver_data *ddata; > > + > > + ddata = of_device_get_match_data(dphy->dev); > > + if (!ddata) > > + return -EINVAL; > > + > > + if (mode != PHY_MODE_MIPI_DPHY) > > + return -EINVAL; > > + > > + if (submode == PHY_MIPI_DPHY_SUBMODE_TX) { > > + if (!ddata->tx) > > + return -EOPNOTSUPP; > > + > > + dphy->ops = ddata->tx; > > + } else if (submode == PHY_MIPI_DPHY_SUBMODE_RX) { > > + if (!ddata->rx) > > + return -EOPNOTSUPP; > > + > > + dphy->ops = ddata->rx; > > + } else { > > + return -EOPNOTSUPP; > > + } > > + > > + return 0; > > +} > > + > > static const struct phy_ops cdns_dphy_ops = { > > .configure = cdns_dphy_configure, > > .validate = cdns_dphy_validate, > > .power_on = cdns_dphy_power_on, > > .power_off = cdns_dphy_power_off, > > + .set_mode = cdns_dphy_set_mode, > > }; > > > > static int cdns_dphy_probe(struct platform_device *pdev) > > -- > Regards, > > Laurent Pinchart
On 31/03/21 05:24PM, Chunfeng Yun wrote: > On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote: > > Some platforms like TI's J721E can have the CSI2RX paired with an > > external DPHY. Add support to enable and configure the DPHY using the > > generic PHY framework. > > > > Get the pixel rate and bpp from the subdev and pass them on to the DPHY > > along with the number of lanes. All other settings are left to their > > default values. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/media/platform/cadence/cdns-csi2rx.c | 147 +++++++++++++++++-- > > 1 file changed, 137 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index c68a3eac62cd..31bd80e3f780 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -30,6 +30,12 @@ > > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > > +#define CSI2RX_DPHY_CL_RST BIT(16) > > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > > +#define CSI2RX_DPHY_CL_EN BIT(4) > > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > > + > > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > > @@ -54,6 +60,11 @@ enum csi2rx_pads { > > CSI2RX_PAD_MAX, > > }; > > > > +struct csi2rx_fmt { > > + u32 code; > > + u8 bpp; > > +}; > > + > > struct csi2rx_priv { > > struct device *dev; > > unsigned int count; > > @@ -85,6 +96,52 @@ struct csi2rx_priv { > > int source_pad; > > }; > > > > +static const struct csi2rx_fmt formats[] = { > > + { > > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > > + .bpp = 16, > > + }, > > +}; > > + > > +static u8 csi2rx_get_bpp(u32 code) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > > + if (formats[i].code == code) > > + return formats[i].bpp; > > + } > > + > > + return 0; > > +} > > + > > +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) > > +{ > > + struct v4l2_ctrl *ctrl; > > + > > + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, > > + V4L2_CID_PIXEL_RATE); > > + if (!ctrl) { > > + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", > > + csi2rx->source_subdev->name); > > + return -EINVAL; > > + } > > + > > + return v4l2_ctrl_g_ctrl_int64(ctrl); > > +} > > + > > static inline > > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > > { > > @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > > } > > > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > > +{ > > + union phy_configure_opts opts = { }; > > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > > + struct v4l2_subdev_format sd_fmt; > > + s64 pixel_rate; > > + int ret; > > + u8 bpp; > > + > > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > + sd_fmt.pad = 0; > > + > > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, > > + &sd_fmt); > > + if (ret) > > + return ret; > > + > > + bpp = csi2rx_get_bpp(sd_fmt.format.code); > > + if (!bpp) > > + return -EINVAL; > > + > > + pixel_rate = csi2rx_get_pixel_rate(csi2rx); > > + if (pixel_rate < 0) > > + return pixel_rate; > > + > > + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, > > + csi2rx->num_lanes, cfg); > > + if (ret) > > + return ret; > > + > > + ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY, > > + PHY_MIPI_DPHY_SUBMODE_RX); > > + if (ret) > > + return ret; > > + > > + ret = phy_power_on(csi2rx->dphy); > > + if (ret) > > + return ret; > Seems phy_power_on, then phy_set_mode_ext? Shouldn't the mode be set before the PHY is powered on so the correct power on procedure can be performed based on the mode of operation? > > > + > > + ret = phy_configure(csi2rx->dphy, &opts); > > + if (ret) { > > + /* Can't do anything if it fails. Ignore the return value. */ > > + phy_power_off(csi2rx->dphy); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > static int csi2rx_start(struct csi2rx_priv *csi2rx) > > { > > unsigned int i; > > @@ -139,6 +245,17 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > if (ret) > > goto err_disable_pclk; > > > > + /* Enable DPHY clk and data lanes. */ > > + if (csi2rx->dphy) { > > + reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST; > > + for (i = 0; i < csi2rx->num_lanes; i++) { > > + reg |= CSI2RX_DPHY_DL_EN(csi2rx->lanes[i] - 1); > > + reg |= CSI2RX_DPHY_DL_RST(csi2rx->lanes[i] - 1); > > + } > > + > > + writel(reg, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > + } > > + > > /* > > * Create a static mapping between the CSI virtual channels > > * and the output stream. > > @@ -169,10 +286,21 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > if (ret) > > goto err_disable_pixclk; > > > > + if (csi2rx->dphy) { > > + ret = csi2rx_configure_external_dphy(csi2rx); > > + if (ret) { > > + dev_err(csi2rx->dev, > > + "Failed to configure external DPHY: %d\n", ret); > > + goto err_disable_sysclk; > > + } > > + } > > + > > clk_disable_unprepare(csi2rx->p_clk); > > > > return 0; > > > > +err_disable_sysclk: > > + clk_disable_unprepare(csi2rx->sys_clk); > > err_disable_pixclk: > > for (; i > 0; i--) > > clk_disable_unprepare(csi2rx->pixel_clk[i - 1]); > > @@ -200,6 +328,13 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > > > > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > > + > > + if (csi2rx->dphy) { > > + writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > + > > + if (phy_power_off(csi2rx->dphy)) > > + dev_warn(csi2rx->dev, "Couldn't power off DPHY\n"); > > + } > > } > > > > static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > @@ -306,15 +441,6 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > return PTR_ERR(csi2rx->dphy); > > } > > > > - /* > > - * FIXME: Once we'll have external D-PHY support, the check > > - * will need to be removed. > > - */ > > - if (csi2rx->dphy) { > > - dev_err(&pdev->dev, "External D-PHY not supported yet\n"); > > - return -EINVAL; > > - } > > - > > clk_prepare_enable(csi2rx->p_clk); > > dev_cfg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG); > > clk_disable_unprepare(csi2rx->p_clk); > > @@ -339,7 +465,7 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > * FIXME: Once we'll have internal D-PHY support, the check > > * will need to be removed. > > */ > > - if (csi2rx->has_internal_dphy) { > > + if (!csi2rx->dphy && csi2rx->has_internal_dphy) { > > dev_err(&pdev->dev, "Internal D-PHY not supported yet\n"); > > return -EINVAL; > > } > > @@ -460,6 +586,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > dev_info(&pdev->dev, > > "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n", > > csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams, > > + csi2rx->dphy ? "external" : > > csi2rx->has_internal_dphy ? "internal" : "no"); > > > > return 0; >
On 4/6/21 8:10 PM, Pratyush Yadav wrote: > On 06/04/21 10:25PM, Pratyush Yadav wrote: >> On 06/04/21 06:33PM, Péter Ujfalusi wrote: >>> >>> >>> On 4/6/21 6:09 PM, Pratyush Yadav wrote: >>>> On 04/04/21 04:24PM, Péter Ujfalusi wrote: >>>>> Hi Pratyush, >>>>> >>>>> On 3/30/21 8:33 PM, Pratyush Yadav wrote: >>>>>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can >>>>>> have up to 32 threads but the current driver only supports using one. So >>>>>> add an entry for that one thread. >>>>> >>>>> If you are absolutely sure that the other threads are not going to be >>>>> used, then: >>>> >>>> The opposite in fact. I do expect other threads to be used in the >>>> future. But the current driver can only use one so I figured it is >>>> better to add just the thread that is currently needed and then I can >>>> always add the rest later. >>>> >>>> Why does this have to be a one-and-done deal? Is there anything wrong >>>> with adding the other threads when the driver can actually use them? >>> >>> You can skip CCing DMAengine (and me ;) ). Less subsystems is the better >>> when sending patches... >> >> I'm a bit confused here. If you are no longer interested in maintaining >> the TI DMA drivers then that's fine, I can skip CCing you. But the patch >> is still relevant to the dmaengine list so why should I skip CCing it? >> And if I don't CC the dmaengine list then on which list would I get >> comments/reviews for the patch? > > Ignore this. Got your point. Will do it in v2. If you know that you will be adding the other threads in the near future then do it with one patch. When you add the support in the csi2rx driver then you don't need to change the DMAengine files, thus no need to CC dmaengine or me, thus you need to gather less acked-bys, thus the time to mainline might be shorter.
On Wed, 2021-04-07 at 00:24 +0530, Pratyush Yadav wrote: > On 31/03/21 05:24PM, Chunfeng Yun wrote: > > On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote: > > > Some platforms like TI's J721E can have the CSI2RX paired with an > > > external DPHY. Add support to enable and configure the DPHY using the > > > generic PHY framework. > > > > > > Get the pixel rate and bpp from the subdev and pass them on to the DPHY > > > along with the number of lanes. All other settings are left to their > > > default values. > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > --- > > > drivers/media/platform/cadence/cdns-csi2rx.c | 147 +++++++++++++++++-- > > > 1 file changed, 137 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > > index c68a3eac62cd..31bd80e3f780 100644 > > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > > @@ -30,6 +30,12 @@ > > > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > > > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > > > > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > > > +#define CSI2RX_DPHY_CL_RST BIT(16) > > > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > > > +#define CSI2RX_DPHY_CL_EN BIT(4) > > > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > > > + > > > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > > > > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > > > @@ -54,6 +60,11 @@ enum csi2rx_pads { > > > CSI2RX_PAD_MAX, > > > }; > > > > > > +struct csi2rx_fmt { > > > + u32 code; > > > + u8 bpp; > > > +}; > > > + > > > struct csi2rx_priv { > > > struct device *dev; > > > unsigned int count; > > > @@ -85,6 +96,52 @@ struct csi2rx_priv { > > > int source_pad; > > > }; > > > > > > +static const struct csi2rx_fmt formats[] = { > > > + { > > > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > > > + .bpp = 16, > > > + }, > > > + { > > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > > + .bpp = 16, > > > + }, > > > + { > > > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > > > + .bpp = 16, > > > + }, > > > + { > > > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > > > + .bpp = 16, > > > + }, > > > +}; > > > + > > > +static u8 csi2rx_get_bpp(u32 code) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > > > + if (formats[i].code == code) > > > + return formats[i].bpp; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) > > > +{ > > > + struct v4l2_ctrl *ctrl; > > > + > > > + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, > > > + V4L2_CID_PIXEL_RATE); > > > + if (!ctrl) { > > > + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", > > > + csi2rx->source_subdev->name); > > > + return -EINVAL; > > > + } > > > + > > > + return v4l2_ctrl_g_ctrl_int64(ctrl); > > > +} > > > + > > > static inline > > > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > > > { > > > @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > > > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > > > } > > > > > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > > > +{ > > > + union phy_configure_opts opts = { }; > > > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > > > + struct v4l2_subdev_format sd_fmt; > > > + s64 pixel_rate; > > > + int ret; > > > + u8 bpp; > > > + > > > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > > + sd_fmt.pad = 0; > > > + > > > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, > > > + &sd_fmt); > > > + if (ret) > > > + return ret; > > > + > > > + bpp = csi2rx_get_bpp(sd_fmt.format.code); > > > + if (!bpp) > > > + return -EINVAL; > > > + > > > + pixel_rate = csi2rx_get_pixel_rate(csi2rx); > > > + if (pixel_rate < 0) > > > + return pixel_rate; > > > + > > > + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, > > > + csi2rx->num_lanes, cfg); > > > + if (ret) > > > + return ret; > > > + > > > + ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY, > > > + PHY_MIPI_DPHY_SUBMODE_RX); > > > + if (ret) > > > + return ret; > > > + > > > + ret = phy_power_on(csi2rx->dphy); > > > + if (ret) > > > + return ret; > > Seems phy_power_on, then phy_set_mode_ext? > > Shouldn't the mode be set before the PHY is powered on so the correct > power on procedure can be performed based on the mode of operation? Of course, it is fine for cnds-dphy. But it depends on HW design and also phy driver; if the mode is controlled in PHY IP register, we can't access it before phy_power_on if no phy_init called (e.g. clock/power is not enabled). Just let you pay attention on the phy sequence. Thanks > > > > > > + > > > + ret = phy_configure(csi2rx->dphy, &opts); > > > + if (ret) { > > > + /* Can't do anything if it fails. Ignore the return value. */ > > > + phy_power_off(csi2rx->dphy); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static int csi2rx_start(struct csi2rx_priv *csi2rx) > > > { > > > unsigned int i; > > > @@ -139,6 +245,17 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > > if (ret) > > > goto err_disable_pclk; > > > > > > + /* Enable DPHY clk and data lanes. */ > > > + if (csi2rx->dphy) { > > > + reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST; > > > + for (i = 0; i < csi2rx->num_lanes; i++) { > > > + reg |= CSI2RX_DPHY_DL_EN(csi2rx->lanes[i] - 1); > > > + reg |= CSI2RX_DPHY_DL_RST(csi2rx->lanes[i] - 1); > > > + } > > > + > > > + writel(reg, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > > + } > > > + > > > /* > > > * Create a static mapping between the CSI virtual channels > > > * and the output stream. > > > @@ -169,10 +286,21 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > > if (ret) > > > goto err_disable_pixclk; > > > > > > + if (csi2rx->dphy) { > > > + ret = csi2rx_configure_external_dphy(csi2rx); > > > + if (ret) { > > > + dev_err(csi2rx->dev, > > > + "Failed to configure external DPHY: %d\n", ret); > > > + goto err_disable_sysclk; > > > + } > > > + } > > > + > > > clk_disable_unprepare(csi2rx->p_clk); > > > > > > return 0; > > > > > > +err_disable_sysclk: > > > + clk_disable_unprepare(csi2rx->sys_clk); > > > err_disable_pixclk: > > > for (; i > 0; i--) > > > clk_disable_unprepare(csi2rx->pixel_clk[i - 1]); > > > @@ -200,6 +328,13 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > > > > > > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > > > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > > > + > > > + if (csi2rx->dphy) { > > > + writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > > + > > > + if (phy_power_off(csi2rx->dphy)) > > > + dev_warn(csi2rx->dev, "Couldn't power off DPHY\n"); > > > + } > > > } > > > > > > static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > > @@ -306,15 +441,6 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > > return PTR_ERR(csi2rx->dphy); > > > } > > > > > > - /* > > > - * FIXME: Once we'll have external D-PHY support, the check > > > - * will need to be removed. > > > - */ > > > - if (csi2rx->dphy) { > > > - dev_err(&pdev->dev, "External D-PHY not supported yet\n"); > > > - return -EINVAL; > > > - } > > > - > > > clk_prepare_enable(csi2rx->p_clk); > > > dev_cfg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG); > > > clk_disable_unprepare(csi2rx->p_clk); > > > @@ -339,7 +465,7 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > > * FIXME: Once we'll have internal D-PHY support, the check > > > * will need to be removed. > > > */ > > > - if (csi2rx->has_internal_dphy) { > > > + if (!csi2rx->dphy && csi2rx->has_internal_dphy) { > > > dev_err(&pdev->dev, "Internal D-PHY not supported yet\n"); > > > return -EINVAL; > > > } > > > @@ -460,6 +586,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > > dev_info(&pdev->dev, > > > "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n", > > > csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams, > > > + csi2rx->dphy ? "external" : > > > csi2rx->has_internal_dphy ? "internal" : "no"); > > > > > > return 0; > > >
On 08/04/2021 11:13, Chunfeng Yun wrote: > On Wed, 2021-04-07 at 00:24 +0530, Pratyush Yadav wrote: >> On 31/03/21 05:24PM, Chunfeng Yun wrote: >>> On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote: >>>> Some platforms like TI's J721E can have the CSI2RX paired with an >>>> external DPHY. Add support to enable and configure the DPHY using the >>>> generic PHY framework. >>>> >>>> Get the pixel rate and bpp from the subdev and pass them on to the DPHY >>>> along with the number of lanes. All other settings are left to their >>>> default values. >>>> >>>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> >>>> --- >>>> drivers/media/platform/cadence/cdns-csi2rx.c | 147 +++++++++++++++++-- >>>> 1 file changed, 137 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c >>>> index c68a3eac62cd..31bd80e3f780 100644 >>>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c >>>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c >>>> @@ -30,6 +30,12 @@ >>>> #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) >>>> #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) >>>> >>>> +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 >>>> +#define CSI2RX_DPHY_CL_RST BIT(16) >>>> +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) >>>> +#define CSI2RX_DPHY_CL_EN BIT(4) >>>> +#define CSI2RX_DPHY_DL_EN(i) BIT(i) >>>> + >>>> #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) >>>> >>>> #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) >>>> @@ -54,6 +60,11 @@ enum csi2rx_pads { >>>> CSI2RX_PAD_MAX, >>>> }; >>>> >>>> +struct csi2rx_fmt { >>>> + u32 code; >>>> + u8 bpp; >>>> +}; >>>> + >>>> struct csi2rx_priv { >>>> struct device *dev; >>>> unsigned int count; >>>> @@ -85,6 +96,52 @@ struct csi2rx_priv { >>>> int source_pad; >>>> }; >>>> >>>> +static const struct csi2rx_fmt formats[] = { >>>> + { >>>> + .code = MEDIA_BUS_FMT_YUYV8_2X8, >>>> + .bpp = 16, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_UYVY8_2X8, >>>> + .bpp = 16, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_YVYU8_2X8, >>>> + .bpp = 16, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_VYUY8_2X8, >>>> + .bpp = 16, >>>> + }, >>>> +}; >>>> + >>>> +static u8 csi2rx_get_bpp(u32 code) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(formats); i++) { >>>> + if (formats[i].code == code) >>>> + return formats[i].bpp; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) >>>> +{ >>>> + struct v4l2_ctrl *ctrl; >>>> + >>>> + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, >>>> + V4L2_CID_PIXEL_RATE); >>>> + if (!ctrl) { >>>> + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", >>>> + csi2rx->source_subdev->name); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return v4l2_ctrl_g_ctrl_int64(ctrl); >>>> +} >>>> + >>>> static inline >>>> struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) >>>> { >>>> @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) >>>> writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); >>>> } >>>> >>>> +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) >>>> +{ >>>> + union phy_configure_opts opts = { }; >>>> + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; >>>> + struct v4l2_subdev_format sd_fmt; >>>> + s64 pixel_rate; >>>> + int ret; >>>> + u8 bpp; >>>> + >>>> + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; >>>> + sd_fmt.pad = 0; >>>> + >>>> + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, >>>> + &sd_fmt); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + bpp = csi2rx_get_bpp(sd_fmt.format.code); >>>> + if (!bpp) >>>> + return -EINVAL; >>>> + >>>> + pixel_rate = csi2rx_get_pixel_rate(csi2rx); >>>> + if (pixel_rate < 0) >>>> + return pixel_rate; >>>> + >>>> + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, >>>> + csi2rx->num_lanes, cfg); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY, >>>> + PHY_MIPI_DPHY_SUBMODE_RX); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = phy_power_on(csi2rx->dphy); >>>> + if (ret) >>>> + return ret; >>> Seems phy_power_on, then phy_set_mode_ext? >> >> Shouldn't the mode be set before the PHY is powered on so the correct >> power on procedure can be performed based on the mode of operation? > Of course, it is fine for cnds-dphy. > But it depends on HW design and also phy driver; > if the mode is controlled in PHY IP register, we can't access it before > phy_power_on if no phy_init called (e.g. clock/power is not enabled). > > Just let you pay attention on the phy sequence. I don't think the phy configuration should depend on phy_power_on, but the runtime PM. I guess this could be solved with: phy_pm_runtime_get_sync(); phy_set_mode_ext(); phy_power_on(); phy_pm_runtime_put(); Tomi