[PATCH 00/16] CSI2RX support on J721E

[PATCH 00/16] CSI2RX support on J721E

[Pratyush Yadav]

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

 .../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

[PATCH 01/16] phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes

[Pratyush Yadav]

From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

As some D-PHY controllers support both Rx and Tx mode, we need a way for
users to explicitly request one or the other. For instance, Rx mode can
be used along with MIPI CSI-2 while Tx mode can be used with MIPI DSI.

Introduce new MIPI D-PHY PHY submodes to use with PHY_MODE_MIPI_DPHY.
The default (zero value) is kept to Tx so only the rkisp1 driver, which
uses D-PHY in Rx mode, needs to be adapted.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 include/linux/phy/phy-mipi-dphy.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi-dphy.h
index a877ffee845d..0f57ef46a8b5 100644
--- a/include/linux/phy/phy-mipi-dphy.h
+++ b/include/linux/phy/phy-mipi-dphy.h
@@ -6,6 +6,19 @@
 #ifndef __PHY_MIPI_DPHY_H_
 #define __PHY_MIPI_DPHY_H_
 
+/**
+ * enum phy_mipi_dphy_submode - MIPI D-PHY sub-mode
+ *
+ * A MIPI D-PHY can be used to transmit or receive data.
+ * Since some controllers can support both, the direction to enable is specified
+ * with the PHY sub-mode. Transmit is assumed by default with phy_set_mode.
+ */
+
+enum phy_mipi_dphy_submode {
+	PHY_MIPI_DPHY_SUBMODE_TX = 0,
+	PHY_MIPI_DPHY_SUBMODE_RX,
+};
+
 /**
  * struct phy_configure_opts_mipi_dphy - MIPI D-PHY configuration set
  *
-- 
2.30.0

[PATCH 02/16] phy: cdns-dphy: Prepare for Rx support

[Pratyush Yadav]

The Rx programming sequence differs from the Tx programming sequence.
Currently only Tx mode is supported. Move all the Tx related parts into
a set of Tx-specific hooks that are then called by the main PHY
framework hooks. This way when Rx support is added all that is needed to
be done is to plug in the Rx hooks.

The clocks "psm" and "pll_ref" are not used by the Rx path so make them
optional in the probe and then check if they exist in the power_on()
hook.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/phy/cadence/cdns-dphy.c | 140 ++++++++++++++++++++++++--------
 1 file changed, 104 insertions(+), 36 deletions(-)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index ba042e39cfaf..8656f2102a91 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -75,6 +75,11 @@ struct cdns_dphy;
 struct cdns_dphy_ops {
 	int (*probe)(struct cdns_dphy *dphy);
 	void (*remove)(struct cdns_dphy *dphy);
+	int (*power_on)(struct cdns_dphy *dphy);
+	int (*power_off)(struct cdns_dphy *dphy);
+	int (*validate)(struct cdns_dphy *dphy, enum phy_mode mode, int submode,
+			union phy_configure_opts *opts);
+	int (*configure)(struct cdns_dphy *dphy, union phy_configure_opts *opts);
 	void (*set_psm_div)(struct cdns_dphy *dphy, u8 div);
 	void (*set_clk_lane_cfg)(struct cdns_dphy *dphy,
 				 enum cdns_dphy_clk_lane_cfg cfg);
@@ -86,12 +91,18 @@ struct cdns_dphy_ops {
 struct cdns_dphy {
 	struct cdns_dphy_cfg cfg;
 	void __iomem *regs;
+	struct device *dev;
 	struct clk *psm_clk;
 	struct clk *pll_ref_clk;
 	const struct cdns_dphy_ops *ops;
 	struct phy *phy;
 };
 
+struct cdns_dphy_driver_data {
+	const struct cdns_dphy_ops *tx;
+	const struct cdns_dphy_ops *rx;
+};
+
 static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
 				     struct cdns_dphy_cfg *cfg,
 				     struct phy_configure_opts_mipi_dphy *opts,
@@ -199,20 +210,9 @@ static void cdns_dphy_ref_set_psm_div(struct cdns_dphy *dphy, u8 div)
 	       dphy->regs + DPHY_PSM_CFG);
 }
 
-/*
- * This is the reference implementation of DPHY hooks. Specific integration of
- * this IP may have to re-implement some of them depending on how they decided
- * to wire things in the SoC.
- */
-static const struct cdns_dphy_ops ref_dphy_ops = {
-	.get_wakeup_time_ns = cdns_dphy_ref_get_wakeup_time_ns,
-	.set_pll_cfg = cdns_dphy_ref_set_pll_cfg,
-	.set_psm_div = cdns_dphy_ref_set_psm_div,
-};
-
-static int cdns_dphy_config_from_opts(struct phy *phy,
-				      struct phy_configure_opts_mipi_dphy *opts,
-				      struct cdns_dphy_cfg *cfg)
+static int cdns_dphy_tx_config_from_opts(struct phy *phy,
+					 struct phy_configure_opts_mipi_dphy *opts,
+					 struct cdns_dphy_cfg *cfg)
 {
 	struct cdns_dphy *dphy = phy_get_drvdata(phy);
 	unsigned int dsi_hfp_ext = 0;
@@ -232,24 +232,13 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
 	return 0;
 }
 
-static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
-			      union phy_configure_opts *opts)
+static int cdns_dphy_tx_configure(struct cdns_dphy *dphy,
+				  union phy_configure_opts *opts)
 {
 	struct cdns_dphy_cfg cfg = { 0 };
-
-	if (mode != PHY_MODE_MIPI_DPHY)
-		return -EINVAL;
-
-	return cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
-}
-
-static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
-{
-	struct cdns_dphy *dphy = phy_get_drvdata(phy);
-	struct cdns_dphy_cfg cfg = { 0 };
 	int ret;
 
-	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
+	ret = cdns_dphy_tx_config_from_opts(dphy->phy, &opts->mipi_dphy, &cfg);
 	if (ret)
 		return ret;
 
@@ -279,9 +268,21 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
 	return 0;
 }
 
-static int cdns_dphy_power_on(struct phy *phy)
+static int cdns_dphy_tx_validate(struct cdns_dphy *dphy, enum phy_mode mode,
+				 int submode, union phy_configure_opts *opts)
 {
-	struct cdns_dphy *dphy = phy_get_drvdata(phy);
+	struct cdns_dphy_cfg cfg = { 0 };
+
+	if (submode != PHY_MIPI_DPHY_SUBMODE_TX)
+		return -EINVAL;
+
+	return cdns_dphy_tx_config_from_opts(dphy->phy, &opts->mipi_dphy, &cfg);
+}
+
+static int cdns_dphy_tx_power_on(struct cdns_dphy *dphy)
+{
+	if (!dphy->psm_clk || !dphy->pll_ref_clk)
+		return -EINVAL;
 
 	clk_prepare_enable(dphy->psm_clk);
 	clk_prepare_enable(dphy->pll_ref_clk);
@@ -293,16 +294,77 @@ static int cdns_dphy_power_on(struct phy *phy)
 	return 0;
 }
 
-static int cdns_dphy_power_off(struct phy *phy)
+static int cdns_dphy_tx_power_off(struct cdns_dphy *dphy)
 {
-	struct cdns_dphy *dphy = phy_get_drvdata(phy);
-
 	clk_disable_unprepare(dphy->pll_ref_clk);
 	clk_disable_unprepare(dphy->psm_clk);
 
 	return 0;
 }
 
+static const struct cdns_dphy_ops tx_ref_dphy_ops = {
+	.power_on = cdns_dphy_tx_power_on,
+	.power_off = cdns_dphy_tx_power_off,
+	.validate = cdns_dphy_tx_validate,
+	.configure = cdns_dphy_tx_configure,
+	.get_wakeup_time_ns = cdns_dphy_ref_get_wakeup_time_ns,
+	.set_pll_cfg = cdns_dphy_ref_set_pll_cfg,
+	.set_psm_div = cdns_dphy_ref_set_psm_div,
+};
+
+/*
+ * This is the reference implementation of DPHY hooks. Specific integration of
+ * this IP may have to re-implement some of them depending on how they decided
+ * to wire things in the SoC.
+ */
+static const struct cdns_dphy_driver_data ref_dphy_ops = {
+	.tx = &tx_ref_dphy_ops,
+};
+
+static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
+			      union phy_configure_opts *opts)
+{
+	struct cdns_dphy *dphy = phy_get_drvdata(phy);
+
+	if (mode != PHY_MODE_MIPI_DPHY)
+		return -EINVAL;
+
+	if (dphy->ops->validate)
+		return dphy->ops->validate(dphy, mode, submode, opts);
+
+	return 0;
+}
+
+static int cdns_dphy_power_on(struct phy *phy)
+{
+	struct cdns_dphy *dphy = phy_get_drvdata(phy);
+
+	if (dphy->ops->power_on)
+		return dphy->ops->power_on(dphy);
+
+	return 0;
+}
+
+static int cdns_dphy_power_off(struct phy *phy)
+{
+	struct cdns_dphy *dphy = phy_get_drvdata(phy);
+
+	if (dphy->ops->power_off)
+		return dphy->ops->power_off(dphy);
+
+	return 0;
+}
+
+static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
+{
+	struct cdns_dphy *dphy = phy_get_drvdata(phy);
+
+	if (dphy->ops->configure)
+		return dphy->ops->configure(dphy, opts);
+
+	return 0;
+}
+
 static const struct phy_ops cdns_dphy_ops = {
 	.configure	= cdns_dphy_configure,
 	.validate	= cdns_dphy_validate,
@@ -314,14 +376,20 @@ static int cdns_dphy_probe(struct platform_device *pdev)
 {
 	struct phy_provider *phy_provider;
 	struct cdns_dphy *dphy;
+	const struct cdns_dphy_driver_data *ddata;
 	int ret;
 
 	dphy = devm_kzalloc(&pdev->dev, sizeof(*dphy), GFP_KERNEL);
 	if (!dphy)
 		return -ENOMEM;
 	dev_set_drvdata(&pdev->dev, dphy);
+	dphy->dev = &pdev->dev;
 
-	dphy->ops = of_device_get_match_data(&pdev->dev);
+	ddata = of_device_get_match_data(&pdev->dev);
+	if (!ddata)
+		return -EINVAL;
+
+	dphy->ops = ddata->tx;
 	if (!dphy->ops)
 		return -EINVAL;
 
@@ -329,11 +397,11 @@ static int cdns_dphy_probe(struct platform_device *pdev)
 	if (IS_ERR(dphy->regs))
 		return PTR_ERR(dphy->regs);
 
-	dphy->psm_clk = devm_clk_get(&pdev->dev, "psm");
+	dphy->psm_clk = devm_clk_get_optional(dphy->dev, "psm");
 	if (IS_ERR(dphy->psm_clk))
 		return PTR_ERR(dphy->psm_clk);
 
-	dphy->pll_ref_clk = devm_clk_get(&pdev->dev, "pll_ref");
+	dphy->pll_ref_clk = devm_clk_get_optional(dphy->dev, "pll_ref");
 	if (IS_ERR(dphy->pll_ref_clk))
 		return PTR_ERR(dphy->pll_ref_clk);
 
-- 
2.30.0

[PATCH 03/16] phy: cdns-dphy: Allow setting mode

[Pratyush Yadav]

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.

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)
-- 
2.30.0

Re: [PATCH 03/16] phy: cdns-dphy: Allow setting mode

[Laurent Pinchart]

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)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 03/16] phy: cdns-dphy: Allow setting mode

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH 04/16] phy: cdns-dphy: Add Rx support

[Pratyush Yadav]

The Cadence DPHY can be used to receive image data over the CSI-2
protocol. Add support for Rx mode. The programming sequence differs from
the Tx mode so it is added as a separate set of hooks to isolate the two
paths.

The PHY is in Tx mode by default and it needs to be set in Rx mode by
setting the submode to PHY_MIPI_DPHY_SUBMODE_RX in the set_mode()
callback.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/phy/cadence/cdns-dphy.c | 237 ++++++++++++++++++++++++++++++++
 1 file changed, 237 insertions(+)

diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
index 7d5f7b333893..7bbca679e2bb 100644
--- a/drivers/phy/cadence/cdns-dphy.c
+++ b/drivers/phy/cadence/cdns-dphy.c
@@ -1,11 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
  * Copyright: 2017-2018 Cadence Design Systems, Inc.
+ * Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com/
  */
 
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
@@ -25,10 +28,14 @@
 #define DPHY_PMA_RCLK(reg)		(0x600 + (reg))
 #define DPHY_PMA_RDATA(lane, reg)	(0x700 + ((lane) * 0x100) + (reg))
 #define DPHY_PCS(reg)			(0xb00 + (reg))
+#define DPHY_ISO(reg)			(0xc00 + (reg))
 
 #define DPHY_CMN_SSM			DPHY_PMA_CMN(0x20)
 #define DPHY_CMN_SSM_EN			BIT(0)
+#define DPHY_CMN_RX_BANDGAP_TIMER_MASK	GENMASK(8, 1)
 #define DPHY_CMN_TX_MODE_EN		BIT(9)
+#define DPHY_CMN_RX_MODE_EN		BIT(10)
+#define DPHY_CMN_RX_BANDGAP_TIMER	0x14
 
 #define DPHY_CMN_PWM			DPHY_PMA_CMN(0x40)
 #define DPHY_CMN_PWM_DIV(x)		((x) << 20)
@@ -45,10 +52,27 @@
 #define DPHY_CMN_OPDIV_FROM_REG		BIT(6)
 #define DPHY_CMN_OPDIV(x)		((x) << 7)
 
+#define DPHY_BAND_CFG			DPHY_PCS(0x0)
+#define DPHY_BAND_CFG_LEFT_BAND		GENMASK(4, 0)
+#define DPHY_BAND_CFG_RIGHT_BAND	GENMASK(9, 5)
+
 #define DPHY_PSM_CFG			DPHY_PCS(0x4)
 #define DPHY_PSM_CFG_FROM_REG		BIT(0)
 #define DPHY_PSM_CLK_DIV(x)		((x) << 1)
 
+#define DPHY_POWER_ISLAND_EN_DATA	DPHY_PCS(0x8)
+#define DPHY_POWER_ISLAND_EN_DATA_VAL	0xaaaaaaaa
+#define DPHY_POWER_ISLAND_EN_CLK	DPHY_PCS(0xc)
+#define DPHY_POWER_ISLAND_EN_CLK_VAL	0xaa
+
+#define DPHY_ISO_CL_CTRL_L		DPHY_ISO(0x10)
+#define DPHY_ISO_DL_CTRL_L0		DPHY_ISO(0x14)
+#define DPHY_ISO_DL_CTRL_L1		DPHY_ISO(0x20)
+#define DPHY_ISO_DL_CTRL_L2		DPHY_ISO(0x30)
+#define DPHY_ISO_DL_CTRL_L3		DPHY_ISO(0x3c)
+#define DPHY_ISO_LANE_READY_BIT		0
+#define DPHY_ISO_LANE_READY_TIMEOUT_MS	100UL
+
 #define DSI_HBP_FRAME_OVERHEAD		12
 #define DSI_HSA_FRAME_OVERHEAD		14
 #define DSI_HFP_FRAME_OVERHEAD		6
@@ -57,6 +81,9 @@
 #define DSI_NULL_FRAME_OVERHEAD		6
 #define DSI_EOT_PKT_SIZE		4
 
+#define DPHY_LANES_MIN			1
+#define DPHY_LANES_MAX			4
+
 struct cdns_dphy_cfg {
 	u8 pll_ipdiv;
 	u8 pll_opdiv;
@@ -312,6 +339,214 @@ static const struct cdns_dphy_ops tx_ref_dphy_ops = {
 	.set_psm_div = cdns_dphy_ref_set_psm_div,
 };
 
+static int cdns_dphy_rx_power_on(struct cdns_dphy *dphy)
+{
+	/* Start RX state machine. */
+	writel(DPHY_CMN_SSM_EN | DPHY_CMN_RX_MODE_EN |
+	       FIELD_PREP(DPHY_CMN_RX_BANDGAP_TIMER_MASK,
+			  DPHY_CMN_RX_BANDGAP_TIMER),
+	       dphy->regs + DPHY_CMN_SSM);
+
+	return 0;
+}
+
+static int cdns_dphy_rx_power_off(struct cdns_dphy *dphy)
+{
+	writel(0, dphy->regs + DPHY_CMN_SSM);
+
+	return 0;
+}
+
+static int cdns_dphy_rx_get_band_ctrl(unsigned long hs_clk_rate)
+{
+	unsigned int rate = hs_clk_rate / 1000000UL;
+
+	if (rate < 80 || rate >= 2500)
+		return -EOPNOTSUPP;
+
+	if (rate >= 80 && rate < 100)
+		return 0;
+
+	if (rate >= 100 && rate < 120)
+		return 1;
+
+	if (rate >= 120 && rate < 160)
+		return 2;
+
+	if (rate >= 160 && rate < 200)
+		return 3;
+
+	if (rate >= 200 && rate < 240)
+		return 4;
+
+	if (rate >= 240 && rate < 280)
+		return 5;
+
+	if (rate >= 280 && rate < 320)
+		return 6;
+
+	if (rate >= 320 && rate < 360)
+		return 7;
+
+	if (rate >= 360 && rate < 400)
+		return 8;
+
+	if (rate >= 400 && rate < 480)
+		return 9;
+
+	if (rate >= 480 && rate < 560)
+		return 10;
+
+	if (rate >= 560 && rate < 640)
+		return 11;
+
+	if (rate >= 640 && rate < 720)
+		return 12;
+
+	if (rate >= 720 && rate < 800)
+		return 13;
+
+	if (rate >= 800 && rate < 880)
+		return 14;
+
+	if (rate >= 880 && rate < 1040)
+		return 15;
+
+	if (rate >= 1040 && rate < 1200)
+		return 16;
+
+	if (rate >= 1200 && rate < 1350)
+		return 17;
+
+	if (rate >= 1350 && rate < 1500)
+		return 18;
+
+	if (rate >= 1500 && rate < 1750)
+		return 19;
+
+	if (rate >= 1750 && rate < 2000)
+		return 20;
+
+	if (rate >= 2000 && rate < 2250)
+		return 21;
+
+	if (rate >= 2250 && rate < 2500)
+		return 22;
+
+	/* Unreachable. */
+	WARN(1, "Reached unreachable code.");
+	return -EINVAL;
+}
+
+static int cdns_dphy_rx_wait_for_bit(void __iomem *addr, unsigned int bit)
+{
+	u32 val;
+
+	return readl_relaxed_poll_timeout(addr, val, val & BIT(bit), 10,
+					  DPHY_ISO_LANE_READY_TIMEOUT_MS * 1000);
+}
+
+static int cdns_dphy_rx_wait_lane_ready(struct cdns_dphy *dphy, int lanes)
+{
+	void __iomem *reg = dphy->regs;
+	int ret;
+
+	if (lanes < DPHY_LANES_MIN || lanes > DPHY_LANES_MAX)
+		return -EINVAL;
+
+	/* Clock lane */
+	ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_CL_CTRL_L,
+					DPHY_ISO_LANE_READY_BIT);
+	if (ret)
+		return ret;
+
+	/* Data lanes. Minimum one lane is mandatory. */
+	ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_DL_CTRL_L0,
+					DPHY_ISO_LANE_READY_BIT);
+	if (ret)
+		return ret;
+
+	if (lanes < 2)
+		return 0;
+
+	ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_DL_CTRL_L1,
+					DPHY_ISO_LANE_READY_BIT);
+	if (ret)
+		return ret;
+
+	if (lanes < 3)
+		return 0;
+
+	ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_DL_CTRL_L2,
+					DPHY_ISO_LANE_READY_BIT);
+	if (ret)
+		return ret;
+
+	if (lanes < 4)
+		return 0;
+
+	ret = cdns_dphy_rx_wait_for_bit(reg + DPHY_ISO_DL_CTRL_L3,
+					DPHY_ISO_LANE_READY_BIT);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int cdns_dphy_rx_configure(struct cdns_dphy *dphy,
+				  union phy_configure_opts *opts)
+{
+	unsigned int reg;
+	int band_ctrl, ret;
+
+	band_ctrl = cdns_dphy_rx_get_band_ctrl(opts->mipi_dphy.hs_clk_rate);
+	if (band_ctrl < 0)
+		return band_ctrl;
+
+	reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, band_ctrl) |
+	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, band_ctrl);
+	writel(reg, dphy->regs + DPHY_BAND_CFG);
+
+	/*
+	 * Set the required power island phase 2 time. This is mandated by DPHY
+	 * specs.
+	 */
+	reg = DPHY_POWER_ISLAND_EN_DATA_VAL;
+	writel(reg, dphy->regs + DPHY_POWER_ISLAND_EN_DATA);
+	reg = DPHY_POWER_ISLAND_EN_CLK_VAL;
+	writel(reg, dphy->regs + DPHY_POWER_ISLAND_EN_CLK);
+
+	ret = cdns_dphy_rx_wait_lane_ready(dphy, opts->mipi_dphy.lanes);
+	if (ret) {
+		dev_err(dphy->dev, "DPHY wait for lane ready timeout\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cdns_dphy_rx_validate(struct cdns_dphy *dphy, enum phy_mode mode,
+				 int submode, union phy_configure_opts *opts)
+{
+	int ret;
+
+	if (submode != PHY_MIPI_DPHY_SUBMODE_RX)
+		return -EINVAL;
+
+	ret = cdns_dphy_rx_get_band_ctrl(opts->mipi_dphy.hs_clk_rate);
+	if (ret < 0)
+		return ret;
+
+	return phy_mipi_dphy_config_validate(&opts->mipi_dphy);
+}
+
+static const struct cdns_dphy_ops rx_ref_dphy_ops = {
+	.power_on = cdns_dphy_rx_power_on,
+	.power_off = cdns_dphy_rx_power_off,
+	.configure = cdns_dphy_rx_configure,
+	.validate = cdns_dphy_rx_validate,
+};
+
 /*
  * This is the reference implementation of DPHY hooks. Specific integration of
  * this IP may have to re-implement some of them depending on how they decided
@@ -319,6 +554,7 @@ static const struct cdns_dphy_ops tx_ref_dphy_ops = {
  */
 static const struct cdns_dphy_driver_data ref_dphy_ops = {
 	.tx = &tx_ref_dphy_ops,
+	.rx = &rx_ref_dphy_ops,
 };
 
 static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
@@ -483,5 +719,6 @@ static struct platform_driver cdns_dphy_platform_driver = {
 module_platform_driver(cdns_dphy_platform_driver);
 
 MODULE_AUTHOR("Maxime Ripard <maxime.ripard@bootlin.com>");
+MODULE_AUTHOR("Pratyush Yadav <p.yadav@ti.com>");
 MODULE_DESCRIPTION("Cadence MIPI D-PHY Driver");
 MODULE_LICENSE("GPL");
-- 
2.30.0

[PATCH 05/16] media: cadence: csi2rx: Add external DPHY support

[Pratyush Yadav]

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;
+
+	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;
-- 
2.30.0

Re: [PATCH 05/16] media: cadence: csi2rx: Add external DPHY support

[Chunfeng Yun]

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;

Re: [PATCH 05/16] media: cadence: csi2rx: Add external DPHY support

[Pratyush Yadav]

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;
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 05/16] media: cadence: csi2rx: Add external DPHY support

[Chunfeng Yun]

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;
> > 
> 

Re: [PATCH 05/16] media: cadence: csi2rx: Add external DPHY support

[Tomi Valkeinen]

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

[PATCH 06/16] media: cadence: csi2rx: Soft reset the streams before starting capture

[Pratyush Yadav]

This resets the stream state machines and FIFOs, giving them a clean
slate. On J721E if the streams are not reset before starting the
capture, the captured frame gets wrapped around vertically on every run
after the first.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/media/platform/cadence/cdns-csi2rx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 31bd80e3f780..b03d2d2e6762 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -39,6 +39,7 @@
 #define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
 
 #define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
+#define CSI2RX_STREAM_CTRL_SOFT_RST			BIT(4)
 #define CSI2RX_STREAM_CTRL_START			BIT(0)
 
 #define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
@@ -150,12 +151,22 @@ struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
 
 static void csi2rx_reset(struct csi2rx_priv *csi2rx)
 {
+	int i;
+
 	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
 	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
 
 	udelay(10);
 
 	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
+
+	/* Reset individual streams. */
+	for (i = 0; i < csi2rx->max_streams; i++) {
+		writel(CSI2RX_STREAM_CTRL_SOFT_RST,
+		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+		usleep_range(10, 20);
+		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+	}
 }
 
 static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx)
-- 
2.30.0

[PATCH 07/16] media: cadence: csi2rx: Set the STOP bit when stopping a stream

[Pratyush Yadav]

The stream stop procedure says that the STOP bit should be set when the
stream is to be stopped, and then the ready bit in stream status
register polled to make sure the STOP operation is finished.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/media/platform/cadence/cdns-csi2rx.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index b03d2d2e6762..eca65b157f59 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -8,6 +8,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
@@ -40,8 +41,12 @@
 
 #define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
 #define CSI2RX_STREAM_CTRL_SOFT_RST			BIT(4)
+#define CSI2RX_STREAM_CTRL_STOP				BIT(1)
 #define CSI2RX_STREAM_CTRL_START			BIT(0)
 
+#define CSI2RX_STREAM_STATUS_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x004)
+#define CSI2RX_STREAM_STATUS_RDY			BIT(31)
+
 #define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
 #define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT		BIT(31)
 #define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
@@ -325,12 +330,23 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
 static void csi2rx_stop(struct csi2rx_priv *csi2rx)
 {
 	unsigned int i;
+	u32 val;
+	int ret;
 
 	clk_prepare_enable(csi2rx->p_clk);
 	clk_disable_unprepare(csi2rx->sys_clk);
 
 	for (i = 0; i < csi2rx->max_streams; i++) {
-		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+		writel(CSI2RX_STREAM_CTRL_STOP,
+		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+
+		ret = readl_relaxed_poll_timeout(csi2rx->base +
+						 CSI2RX_STREAM_STATUS_REG(i),
+						 val,
+						 (val & CSI2RX_STREAM_STATUS_RDY),
+						 10, 10000);
+		if (ret)
+			dev_warn(csi2rx->dev, "Failed to stop stream%d\n", i);
 
 		clk_disable_unprepare(csi2rx->pixel_clk[i]);
 	}
-- 
2.30.0

[PATCH 08/16] media: cadence: csi2rx: Fix stream data configuration

[Pratyush Yadav]

Firstly, there is no VC_EN bit present in the STREAM_DATA_CFG register.
Bit 31 is part of the VL_SELECT field. Remove it completely.

Secondly, it makes little sense to enable ith virtual channel for ith
stream. Sure, there might be a use-case that demands it. But there might
also be a use case that demands all streams to use the 0th virtual
channel. Prefer this case over the former because it is less arbitrary
and also makes it very clear what the limitations of the current driver
is instead of giving a false impression that multiple virtual channels
are supported.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/media/platform/cadence/cdns-csi2rx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index eca65b157f59..7d1ac51e0698 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -48,7 +48,6 @@
 #define CSI2RX_STREAM_STATUS_RDY			BIT(31)
 
 #define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
-#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT		BIT(31)
 #define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
 
 #define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
@@ -290,8 +289,11 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
 		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
 		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
 
-		writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
-		       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
+		/*
+		 * Enable one virtual channel. When multiple virtual channels
+		 * are supported this will have to be changed.
+		 */
+		writel(CSI2RX_STREAM_DATA_CFG_VC_SELECT(0),
 		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
 
 		writel(CSI2RX_STREAM_CTRL_START,
-- 
2.30.0

[PATCH 09/16] media: cadence: csi2rx: Turn subdev power on before starting stream

[Pratyush Yadav]

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.

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);
 
-- 
2.30.0

Re: [PATCH 09/16] media: cadence: csi2rx: Turn subdev power on before starting stream

[Laurent Pinchart]

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);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 09/16] media: cadence: csi2rx: Turn subdev power on before starting stream

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH 10/16] media: cadence: csi2rx: Add wrappers for subdev calls

[Pratyush Yadav]

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.

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,
-- 
2.30.0

Re: [PATCH 10/16] media: cadence: csi2rx: Add wrappers for subdev calls

[Laurent Pinchart]

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,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 10/16] media: cadence: csi2rx: Add wrappers for subdev calls

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX

[Pratyush Yadav]

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.

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 */
-- 
2.30.0

Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX

[Péter Ujfalusi]

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 */
> 

-- 
Péter

Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX

[Péter Ujfalusi]

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
> 

-- 
Péter

Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 11/16] dmaengine: ti: k3-psil-j721e: Add entry for CSI2RX

[Péter Ujfalusi]

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.

-- 
Péter

[PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Pratyush Yadav]

TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
parts together.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml

diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
new file mode 100644
index 000000000000..ebd894364391
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI CSI2RX Wrapper Device Tree Bindings
+
+maintainers:
+  - Pratyush Yadav <p.yadav@ti.com>
+
+properties:
+  compatible:
+    items:
+      - const: ti,csi2rx
+
+  dmas:
+    description: RX DMA Channel 0
+
+  dma-names:
+    items:
+      - const: rx0
+
+  reg:
+    maxItems: 1
+    description: Base address and size of the TI wrapper registers.
+
+  power-domains:
+    maxItems: 1
+    description:
+      PM domain provider node and an args specifier containing
+      the device id value.
+
+  ranges: true
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 2
+
+patternProperties:
+  "csi-bridge@":
+    type: object
+    description: CSI2 bridge node.
+
+required:
+  - compatible
+  - reg
+  - dmas
+  - dma-names
+  - power-domains
+  - "#address-cells"
+  - "#size-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    ti_csi2rx0: ticsi2rx {
+        compatible = "ti,csi2rx";
+        dmas = <&main_udmap 0x4940>;
+        dma-names = "rx0";
+        reg = <0x0 0x4500000 0x0 0x1000>;
+        power-domains = <&k3_pds 26 TI_SCI_PD_EXCLUSIVE>;
+        #address-cells = <2>;
+        #size-cells = <2>;
+    };
-- 
2.30.0

Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Rob Herring]

On Tue, 30 Mar 2021 23:03:44 +0530, Pratyush Yadav wrote:
> TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> parts together.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/media/ti,csi2rx.example.dts:21.30-29.11: Warning (unit_address_vs_reg): /example-0/ticsi2rx: node has a reg or ranges property, but no unit name

See https://patchwork.ozlabs.org/patch/1460166

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Rob Herring]

On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> parts together.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> new file mode 100644
> index 000000000000..ebd894364391
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI CSI2RX Wrapper Device Tree Bindings
> +
> +maintainers:
> +  - Pratyush Yadav <p.yadav@ti.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: ti,csi2rx
> +
> +  dmas:
> +    description: RX DMA Channel 0

items:
  - description: RX DMA Channel 0

Or just 'maxItems: 1'

> +
> +  dma-names:
> +    items:
> +      - const: rx0
> +
> +  reg:
> +    maxItems: 1
> +    description: Base address and size of the TI wrapper registers.

That's all 'reg' properties, drop 'description'.

> +
> +  power-domains:
> +    maxItems: 1
> +    description:
> +      PM domain provider node and an args specifier containing
> +      the device id value.

Drop.

> +
> +  ranges: true
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 2
> +
> +patternProperties:
> +  "csi-bridge@":

"^csi-bridge@"

> +    type: object
> +    description: CSI2 bridge node.

Just an empty node?

> +
> +required:
> +  - compatible
> +  - reg
> +  - dmas
> +  - dma-names
> +  - power-domains
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> +
> +    ti_csi2rx0: ticsi2rx {
> +        compatible = "ti,csi2rx";
> +        dmas = <&main_udmap 0x4940>;
> +        dma-names = "rx0";
> +        reg = <0x0 0x4500000 0x0 0x1000>;
> +        power-domains = <&k3_pds 26 TI_SCI_PD_EXCLUSIVE>;
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +    };
> -- 
> 2.30.0
> 

Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Laurent Pinchart]

On Thu, Apr 01, 2021 at 10:52:01AM -0500, Rob Herring wrote:
> On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > parts together.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > new file mode 100644
> > index 000000000000..ebd894364391
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TI CSI2RX Wrapper Device Tree Bindings
> > +

A description would be useful, especially given that the TRM doesn't
mention "CSI2RX".

> > +maintainers:
> > +  - Pratyush Yadav <p.yadav@ti.com>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: ti,csi2rx
> > +
> > +  dmas:
> > +    description: RX DMA Channel 0
> 
> items:
>   - description: RX DMA Channel 0
> 
> Or just 'maxItems: 1'
> 
> > +
> > +  dma-names:
> > +    items:
> > +      - const: rx0
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: Base address and size of the TI wrapper registers.
> 
> That's all 'reg' properties, drop 'description'.

According to SPRUIL1B, there are four register banks for the CSI_RX_IF,
and two register banks for the DPHY_RX. What's your plan to support
these ? Not everything need to be implemented at once, but backward
compatibility need to be taken into account in the design.

> > +
> > +  power-domains:
> > +    maxItems: 1
> > +    description:
> > +      PM domain provider node and an args specifier containing
> > +      the device id value.
> 
> Drop.
> 
> > +
> > +  ranges: true
> > +
> > +  "#address-cells":
> > +    const: 2
> > +
> > +  "#size-cells":
> > +    const: 2
> > +
> > +patternProperties:
> > +  "csi-bridge@":
> 
> "^csi-bridge@"
> 
> > +    type: object
> > +    description: CSI2 bridge node.
> 
> Just an empty node?

Even if the node is optional, it would be useful to include it in the
example below, to show how it's supposed to be used.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - dmas
> > +  - dma-names
> > +  - power-domains
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> > +
> > +    ti_csi2rx0: ticsi2rx {
> > +        compatible = "ti,csi2rx";
> > +        dmas = <&main_udmap 0x4940>;
> > +        dma-names = "rx0";
> > +        reg = <0x0 0x4500000 0x0 0x1000>;
> > +        power-domains = <&k3_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Laurent Pinchart]

On Fri, Apr 02, 2021 at 01:01:22PM +0300, Laurent Pinchart wrote:
> On Thu, Apr 01, 2021 at 10:52:01AM -0500, Rob Herring wrote:
> > On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > > parts together.
> > > 
> > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > ---
> > >  .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > new file mode 100644
> > > index 000000000000..ebd894364391
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > @@ -0,0 +1,70 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: TI CSI2RX Wrapper Device Tree Bindings
> > > +
> 
> A description would be useful, especially given that the TRM doesn't
> mention "CSI2RX".
> 
> > > +maintainers:
> > > +  - Pratyush Yadav <p.yadav@ti.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: ti,csi2rx
> > > +
> > > +  dmas:
> > > +    description: RX DMA Channel 0
> > 
> > items:
> >   - description: RX DMA Channel 0
> > 
> > Or just 'maxItems: 1'
> > 
> > > +
> > > +  dma-names:
> > > +    items:
> > > +      - const: rx0
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +    description: Base address and size of the TI wrapper registers.
> > 
> > That's all 'reg' properties, drop 'description'.
> 
> According to SPRUIL1B, there are four register banks for the CSI_RX_IF,
> and two register banks for the DPHY_RX. What's your plan to support
> these ? Not everything need to be implemented at once, but backward
> compatibility need to be taken into account in the design.
> 
> > > +
> > > +  power-domains:
> > > +    maxItems: 1
> > > +    description:
> > > +      PM domain provider node and an args specifier containing
> > > +      the device id value.
> > 
> > Drop.
> > 
> > > +
> > > +  ranges: true
> > > +
> > > +  "#address-cells":
> > > +    const: 2
> > > +
> > > +  "#size-cells":
> > > +    const: 2
> > > +
> > > +patternProperties:
> > > +  "csi-bridge@":
> > 
> > "^csi-bridge@"
> > 
> > > +    type: object
> > > +    description: CSI2 bridge node.
> > 
> > Just an empty node?
> 
> Even if the node is optional, it would be useful to include it in the
> example below, to show how it's supposed to be used.
> 
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - dmas
> > > +  - dma-names
> > > +  - power-domains
> > > +  - "#address-cells"
> > > +  - "#size-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> > > +
> > > +    ti_csi2rx0: ticsi2rx {
> > > +        compatible = "ti,csi2rx";
> > > +        dmas = <&main_udmap 0x4940>;
> > > +        dma-names = "rx0";
> > > +        reg = <0x0 0x4500000 0x0 0x1000>;
> > > +        power-domains = <&k3_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +    };

It would also be useful to expand this to a full example that includes
integration with the PHY.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Pratyush Yadav]

On 02/04/21 01:53PM, Laurent Pinchart wrote:
> On Fri, Apr 02, 2021 at 01:01:22PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 01, 2021 at 10:52:01AM -0500, Rob Herring wrote:
> > > On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > > > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > > > parts together.
> > > > 
> > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > > ---
> > > >  .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
> > > >  1 file changed, 70 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > > new file mode 100644
> > > > index 000000000000..ebd894364391
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > > @@ -0,0 +1,70 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: TI CSI2RX Wrapper Device Tree Bindings
> > > > +
> > 
> > A description would be useful, especially given that the TRM doesn't
> > mention "CSI2RX".
> > 
> > > > +maintainers:
> > > > +  - Pratyush Yadav <p.yadav@ti.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - const: ti,csi2rx
> > > > +
> > > > +  dmas:
> > > > +    description: RX DMA Channel 0
> > > 
> > > items:
> > >   - description: RX DMA Channel 0
> > > 
> > > Or just 'maxItems: 1'
> > > 
> > > > +
> > > > +  dma-names:
> > > > +    items:
> > > > +      - const: rx0
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +    description: Base address and size of the TI wrapper registers.
> > > 
> > > That's all 'reg' properties, drop 'description'.
> > 
> > According to SPRUIL1B, there are four register banks for the CSI_RX_IF,
> > and two register banks for the DPHY_RX. What's your plan to support
> > these ? Not everything need to be implemented at once, but backward
> > compatibility need to be taken into account in the design.
> > 
> > > > +
> > > > +  power-domains:
> > > > +    maxItems: 1
> > > > +    description:
> > > > +      PM domain provider node and an args specifier containing
> > > > +      the device id value.
> > > 
> > > Drop.
> > > 
> > > > +
> > > > +  ranges: true
> > > > +
> > > > +  "#address-cells":
> > > > +    const: 2
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 2
> > > > +
> > > > +patternProperties:
> > > > +  "csi-bridge@":
> > > 
> > > "^csi-bridge@"
> > > 
> > > > +    type: object
> > > > +    description: CSI2 bridge node.
> > > 
> > > Just an empty node?
> > 
> > Even if the node is optional, it would be useful to include it in the
> > example below, to show how it's supposed to be used.
> > 
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - dmas
> > > > +  - dma-names
> > > > +  - power-domains
> > > > +  - "#address-cells"
> > > > +  - "#size-cells"
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> > > > +
> > > > +    ti_csi2rx0: ticsi2rx {
> > > > +        compatible = "ti,csi2rx";
> > > > +        dmas = <&main_udmap 0x4940>;
> > > > +        dma-names = "rx0";
> > > > +        reg = <0x0 0x4500000 0x0 0x1000>;
> > > > +        power-domains = <&k3_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > > > +        #address-cells = <2>;
> > > > +        #size-cells = <2>;
> > > > +    };
> 
> It would also be useful to expand this to a full example that includes
> integration with the PHY.

Integration with PHY is Cadence CSI2RX schema's problem. But I will add 
the subnode here anyway so it should have the PHY related properties as 
well.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Pratyush Yadav]

On 02/04/21 01:01PM, Laurent Pinchart wrote:
> On Thu, Apr 01, 2021 at 10:52:01AM -0500, Rob Herring wrote:
> > On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > > parts together.
> > > 
> > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > > ---
> > >  .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > new file mode 100644
> > > index 000000000000..ebd894364391
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > > @@ -0,0 +1,70 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: TI CSI2RX Wrapper Device Tree Bindings
> > > +
> 
> A description would be useful, especially given that the TRM doesn't
> mention "CSI2RX".

Ok.

> 
> > > +maintainers:
> > > +  - Pratyush Yadav <p.yadav@ti.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: ti,csi2rx
> > > +
> > > +  dmas:
> > > +    description: RX DMA Channel 0
> > 
> > items:
> >   - description: RX DMA Channel 0
> > 
> > Or just 'maxItems: 1'
> > 
> > > +
> > > +  dma-names:
> > > +    items:
> > > +      - const: rx0
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +    description: Base address and size of the TI wrapper registers.
> > 
> > That's all 'reg' properties, drop 'description'.
> 
> According to SPRUIL1B, there are four register banks for the CSI_RX_IF,
> and two register banks for the DPHY_RX. What's your plan to support
> these ? Not everything need to be implemented at once, but backward
> compatibility need to be taken into account in the design.

The CSI_RX_IF0_ECC_AGGR_CFG register bank is for safety requirements. 
The driver does not use them. The CSI_RX_IF0_RX_SHIM_VBUSP_MMR_CSI2RXIF 
register bank is for the TI wrapper around the Cadence CSI2RX bridge 
that deals with DMA threads. This bank is what this binding is concerned 
with. The CSI_RX_IF0_VBUS2APB_WRAP_VBUS_APB_CSI2RX bank is for the 
Cadence CSI2RX bridge. The Cadence schema should deal with that. And 
lastly, I don't know what the CSI_RX_IF0_CP_INTD_CFG_INTD_CFG bank is 
for. The driver does not use it.

I don't forsee the first and last bank being used in Kernel, but if we 
want to be safe I can change maxItems to 3. Sounds good?

> 
> > > +
> > > +  power-domains:
> > > +    maxItems: 1
> > > +    description:
> > > +      PM domain provider node and an args specifier containing
> > > +      the device id value.
> > 
> > Drop.
> > 
> > > +
> > > +  ranges: true
> > > +
> > > +  "#address-cells":
> > > +    const: 2
> > > +
> > > +  "#size-cells":
> > > +    const: 2
> > > +
> > > +patternProperties:
> > > +  "csi-bridge@":
> > 
> > "^csi-bridge@"
> > 
> > > +    type: object
> > > +    description: CSI2 bridge node.
> > 
> > Just an empty node?
> 
> Even if the node is optional, it would be useful to include it in the
> example below, to show how it's supposed to be used.

It is not optional. It should be the Cadence CSI2RX bridge node. Will 
add it in the example. I also need to see if there is any way to make a 
patternProperty a required property.

> 
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - dmas
> > > +  - dma-names
> > > +  - power-domains
> > > +  - "#address-cells"
> > > +  - "#size-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> > > +
> > > +    ti_csi2rx0: ticsi2rx {
> > > +        compatible = "ti,csi2rx";
> > > +        dmas = <&main_udmap 0x4940>;
> > > +        dma-names = "rx0";
> > > +        reg = <0x0 0x4500000 0x0 0x1000>;
> > > +        power-domains = <&k3_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > > +        #address-cells = <2>;
> > > +        #size-cells = <2>;
> > > +    };
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Pratyush Yadav]

On 01/04/21 10:52AM, Rob Herring wrote:
> On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> > TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> > capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> > parts together.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > new file mode 100644
> > index 000000000000..ebd894364391
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: TI CSI2RX Wrapper Device Tree Bindings
> > +
> > +maintainers:
> > +  - Pratyush Yadav <p.yadav@ti.com>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: ti,csi2rx
> > +
> > +  dmas:
> > +    description: RX DMA Channel 0
> 
> items:
>   - description: RX DMA Channel 0
> 
> Or just 'maxItems: 1'

Ok.

> 
> > +
> > +  dma-names:
> > +    items:
> > +      - const: rx0
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: Base address and size of the TI wrapper registers.
> 
> That's all 'reg' properties, drop 'description'.

Ok.

> 
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +    description:
> > +      PM domain provider node and an args specifier containing
> > +      the device id value.
> 
> Drop.

Ok.

> 
> > +
> > +  ranges: true
> > +
> > +  "#address-cells":
> > +    const: 2
> > +
> > +  "#size-cells":
> > +    const: 2
> > +
> > +patternProperties:
> > +  "csi-bridge@":
> 
> "^csi-bridge@"

Ok.

> 
> > +    type: object
> > +    description: CSI2 bridge node.
> 
> Just an empty node?

No. It should be a node for the Cadence csi2rx IP (compatible 
"cdns,csi2rx"). I'm not sure how to model this. This subnode is needed 
but it should take its properties from the Cadence csi2rx schema. Will a 

  properties:
    allOf:
      - $ref: cdns,csi2rx.yaml#

be a good idea?

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - dmas
> > +  - dma-names
> > +  - power-domains
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/soc/ti,sci_pm_domain.h>
> > +
> > +    ti_csi2rx0: ticsi2rx {
> > +        compatible = "ti,csi2rx";
> > +        dmas = <&main_udmap 0x4940>;
> > +        dma-names = "rx0";
> > +        reg = <0x0 0x4500000 0x0 0x1000>;
> > +        power-domains = <&k3_pds 26 TI_SCI_PD_EXCLUSIVE>;
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +    };
> > -- 
> > 2.30.0
> > 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 12/16] dt-bindings: media: Add DT bindings for TI CSI2RX driver

[Rob Herring]

On Tue, Mar 30, 2021 at 11:03:44PM +0530, Pratyush Yadav wrote:
> TI's J721E uses the Cadence CSI2RX and DPHY peripherals to facilitate
> capture over a CSI-2 bus. The TI CSI2RX platform driver glues all the
> parts together.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  .../devicetree/bindings/media/ti,csi2rx.yaml  | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/ti,csi2rx.yaml b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> new file mode 100644
> index 000000000000..ebd894364391
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/ti,csi2rx.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/ti,csi2rx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI CSI2RX Wrapper Device Tree Bindings
> +
> +maintainers:
> +  - Pratyush Yadav <p.yadav@ti.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: ti,csi2rx

Also, needs an SoC specific compatible.

Rob

[PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support

[Pratyush Yadav]

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/MAINTAINERS b/MAINTAINERS
index 8c44fd8fd85d..06fb8fdb874d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18006,6 +18006,13 @@ S:	Odd Fixes
 F:	drivers/clk/ti/
 F:	include/linux/clk/ti.h
 
+TI CSI2RX DRIVER
+M:	Pratyush Yadav <p.yadav@ti.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/media/ti,csi2rx.yaml
+F:	drivers/media/platform/ti-vpe/ti-csi2rx.c
+
 TI DAVINCI MACHINE SUPPORT
 M:	Sekhar Nori <nsekhar@ti.com>
 R:	Bartosz Golaszewski <bgolaszewski@baylibre.com>
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index b238a923d1b4..3b240d8e4cb8 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -200,6 +200,17 @@ config VIDEO_TI_CAL_MC
 
 endif # VIDEO_TI_CAL
 
+config VIDEO_TI_CSI2RX
+	tristate "TI CSI2RX wrapper layer driver"
+	depends on VIDEO_DEV && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && MEDIA_SUPPORT
+	depends on PHY_CADENCE_DPHY && VIDEO_CADENCE_CSI2RX
+	depends on ARCH_K3 || COMPILE_TEST
+	select VIDEOBUF2_DMA_CONTIG
+	select V4L2_FWNODE
+	help
+	  Support for TI CSI2RX wrapper layer. This just enables the wrapper driver.
+	  The Cadence CSI2RX bridge driver needs to be enabled separately.
+
 endif # V4L_PLATFORM_DRIVERS
 
 menuconfig V4L_MEM2MEM_DRIVERS
diff --git a/drivers/media/platform/ti-vpe/Makefile b/drivers/media/platform/ti-vpe/Makefile
index ad624056e039..c4ee49484b73 100644
--- a/drivers/media/platform/ti-vpe/Makefile
+++ b/drivers/media/platform/ti-vpe/Makefile
@@ -3,6 +3,7 @@ obj-$(CONFIG_VIDEO_TI_VPE) += ti-vpe.o
 obj-$(CONFIG_VIDEO_TI_VPDMA) += ti-vpdma.o
 obj-$(CONFIG_VIDEO_TI_SC) += ti-sc.o
 obj-$(CONFIG_VIDEO_TI_CSC) += ti-csc.o
+obj-$(CONFIG_VIDEO_TI_CSI2RX) += ti-csi2rx.o
 
 ti-vpe-y := vpe.o
 ti-vpdma-y := vpdma.o
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
@@ -0,0 +1,964 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TI CSI2 RX driver.
+ *
+ * Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com/
+ *
+ * Author: Pratyush Yadav <p.yadav@ti.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/dmaengine.h>
+#include <linux/of_platform.h>
+
+#include <media/v4l2-device.h>
+#include <media/v4l2-ioctl.h>
+#include <media/videobuf2-dma-contig.h>
+
+#define TI_CSI2RX_MODULE_NAME		"ti-csi2rx"
+
+#define SHIM_CNTL			0x10
+#define SHIM_CNTL_PIX_RST		BIT(0)
+
+#define SHIM_DMACNTX			0x20
+#define SHIM_DMACNTX_EN			BIT(31)
+#define SHIM_DMACNTX_YUV422		GENMASK(27, 26)
+#define SHIM_DMACNTX_FMT		GENMASK(5, 0)
+#define SHIM_DMACNTX_UYVY		0
+#define SHIM_DMACNTX_VYUY		1
+#define SHIM_DMACNTX_YUYV		2
+#define SHIM_DMACNTX_YVYU		3
+
+#define SHIM_PSI_CFG0			0x24
+#define SHIM_PSI_CFG0_SRC_TAG		GENMASK(15, 0)
+#define SHIM_PSI_CFG0_DST_TAG		GENMASK(31, 15)
+
+#define CSI_DF_YUV420			0x18
+#define CSI_DF_YUV422			0x1e
+#define CSI_DF_RGB444			0x20
+#define CSI_DF_RGB888			0x24
+
+struct ti_csi2rx_fmt {
+	u32				fourcc;	/* Four character code. */
+	u32				code;	/* Mbus code. */
+	u32				csi_df;	/* CSI Data format. */
+	u8				bpp;	/* Bits per pixel. */
+};
+
+struct ti_csi2rx_buffer {
+	/* Common v4l2 buffer. Must be first. */
+	struct vb2_v4l2_buffer		vb;
+	struct list_head		list;
+};
+
+struct ti_csi2rx_dmaq {
+	struct list_head		list;
+};
+
+struct ti_csi2rx_dev {
+	struct device			*dev;
+	void __iomem			*shim;
+	struct v4l2_device		v4l2_dev;
+	struct video_device		vdev;
+	struct media_device		mdev;
+	struct v4l2_async_notifier	notifier;
+	struct v4l2_subdev		*subdev;
+	struct vb2_queue		vidq;
+	struct mutex			mutex; /* To serialize ioctls. */
+	const struct ti_csi2rx_fmt	**supported_fmts;
+	unsigned int			num_supported_fmts;
+	struct v4l2_format		v_fmt;
+	struct dma_chan			*dma;
+	struct ti_csi2rx_dmaq		dmaq;
+	u32				sequence;
+};
+
+static const struct ti_csi2rx_fmt formats[] = {
+	{
+		.fourcc			= V4L2_PIX_FMT_YUYV,
+		.code			= MEDIA_BUS_FMT_YUYV8_2X8,
+		.csi_df			= CSI_DF_YUV422,
+		.bpp			= 16,
+	}, {
+		.fourcc			= V4L2_PIX_FMT_UYVY,
+		.code			= MEDIA_BUS_FMT_UYVY8_2X8,
+		.csi_df			= CSI_DF_YUV422,
+		.bpp			= 16,
+	}, {
+		.fourcc			= V4L2_PIX_FMT_YVYU,
+		.code			= MEDIA_BUS_FMT_YVYU8_2X8,
+		.csi_df			= CSI_DF_YUV422,
+		.bpp			= 16,
+	}, {
+		.fourcc			= V4L2_PIX_FMT_VYUY,
+		.code			= MEDIA_BUS_FMT_VYUY8_2X8,
+		.csi_df			= CSI_DF_YUV422,
+		.bpp			= 16,
+	},
+
+	/* More formats can be supported but they are not listed for now. */
+};
+
+/* Forward declaration needed by ti_csi2rx_dma_callback. */
+static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi,
+			       struct ti_csi2rx_buffer *buf);
+
+static const struct ti_csi2rx_fmt *find_format_by_pix(struct ti_csi2rx_dev *csi,
+						      u32 pixelformat)
+{
+	const struct ti_csi2rx_fmt *fmt;
+	unsigned int i;
+
+	for (i = 0; i < csi->num_supported_fmts; i++) {
+		fmt = csi->supported_fmts[i];
+		if (fmt->fourcc == pixelformat)
+			return fmt;
+	}
+
+	return NULL;
+}
+
+static const struct ti_csi2rx_fmt *find_format_by_code(struct ti_csi2rx_dev *csi,
+						       u32 code)
+{
+	const struct ti_csi2rx_fmt *fmt;
+	unsigned int i;
+
+	for (i = 0; i < csi->num_supported_fmts; i++) {
+		fmt = csi->supported_fmts[i];
+		if (fmt->code == code)
+			return fmt;
+	}
+
+	return NULL;
+}
+
+static void ti_csi2rx_fill_fmt(struct v4l2_mbus_framefmt *mbus_fmt,
+			       const struct ti_csi2rx_fmt *csi_fmt,
+			       struct v4l2_format *v4l2_fmt)
+{
+	u32 bpl;
+
+	v4l2_fill_pix_format(&v4l2_fmt->fmt.pix, mbus_fmt);
+	v4l2_fmt->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	v4l2_fmt->fmt.pix.pixelformat = csi_fmt->fourcc;
+	v4l2_fmt->fmt.pix.sizeimage = v4l2_fmt->fmt.pix.height *
+				       v4l2_fmt->fmt.pix.width *
+				       (csi_fmt->bpp / 8);
+
+	bpl = (v4l2_fmt->fmt.pix.width * ALIGN(csi_fmt->bpp, 8)) >> 3;
+	v4l2_fmt->fmt.pix.bytesperline = ALIGN(bpl, 16);
+}
+
+static int ti_csi2rx_init_formats(struct ti_csi2rx_dev *csi)
+{
+	struct v4l2_subdev_mbus_code_enum mbus_code;
+	struct v4l2_mbus_framefmt mbus_fmt;
+	struct v4l2_subdev_format sd_fmt;
+	const struct ti_csi2rx_fmt *fmt;
+	unsigned int i, j, k;
+	int ret = 0;
+
+	csi->supported_fmts = devm_kcalloc(csi->dev, ARRAY_SIZE(formats),
+					   sizeof(*csi->supported_fmts),
+					   GFP_KERNEL);
+	if (!csi->supported_fmts)
+		return -ENOMEM;
+
+	csi->num_supported_fmts = 0;
+
+	/* Find a set of formarts supported by both CSI2RX and camera. */
+	for (j = 0, i = 0; ret != -EINVAL; j++) {
+		memset(&mbus_code, 0, sizeof(mbus_code));
+		mbus_code.index = j;
+		mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+		ret = v4l2_subdev_call(csi->subdev, pad, enum_mbus_code, NULL,
+				       &mbus_code);
+		if (ret)
+			continue;
+
+		for (k = 0; k < ARRAY_SIZE(formats); k++) {
+			const struct ti_csi2rx_fmt *fmt = &formats[k];
+
+			if (mbus_code.code == fmt->code) {
+				csi->supported_fmts[i] = fmt;
+				csi->num_supported_fmts = ++i;
+			}
+		}
+	}
+
+	if (i == 0)
+		return -EINVAL; /* No format mutually supported. */
+
+	/* Get the current format from the subdev. */
+	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sd_fmt.pad = 0;
+
+	ret = v4l2_subdev_call(csi->subdev, pad, get_fmt, NULL, &sd_fmt);
+	if (ret)
+		return ret;
+
+	mbus_fmt = sd_fmt.format;
+
+	fmt = find_format_by_code(csi, mbus_fmt.code);
+	if (!fmt)
+		return -EINVAL;
+
+	/*
+	 * YUV422 8-bit should always be sent by the camera in UYVY. Format
+	 * conversions to other YUV422 formats will be done later by SHIM.
+	 */
+	if (fmt->csi_df == CSI_DF_YUV422 &&
+	    fmt->code != MEDIA_BUS_FMT_UYVY8_2X8) {
+		sd_fmt.format.code = MEDIA_BUS_FMT_UYVY8_2X8;
+		ret = v4l2_subdev_call(csi->subdev, pad, set_fmt, NULL, &sd_fmt);
+		if (ret)
+			return ret;
+	}
+
+	ti_csi2rx_fill_fmt(&mbus_fmt, fmt, &csi->v_fmt);
+
+	return 0;
+}
+
+static int ti_csi2rx_querycap(struct file *file, void *priv,
+			      struct v4l2_capability *cap)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+
+	strscpy(cap->driver, TI_CSI2RX_MODULE_NAME, sizeof(cap->driver));
+	strscpy(cap->card, TI_CSI2RX_MODULE_NAME, sizeof(cap->card));
+
+	snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
+		 dev_name(csi->dev));
+
+	return 0;
+}
+
+static int ti_csi2rx_enum_fmt_vid_cap(struct file *file, void *priv,
+				      struct v4l2_fmtdesc *f)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+	const struct ti_csi2rx_fmt *fmt;
+
+	if (f->index >= csi->num_supported_fmts)
+		return -EINVAL;
+
+	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+
+	fmt = csi->supported_fmts[f->index];
+
+	f->pixelformat = fmt->fourcc;
+
+	return 0;
+}
+
+static int ti_csi2rx_g_fmt_vid_cap(struct file *file, void *prov,
+				   struct v4l2_format *f)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+
+	*f = csi->v_fmt;
+
+	return 0;
+}
+
+static int ti_csi2rx_try_fmt_vid_cap(struct file *file, void *priv,
+				     struct v4l2_format *f)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+	const struct ti_csi2rx_fmt *fmt;
+	struct v4l2_subdev_frame_size_enum fse;
+	int ret, found;
+
+	fmt = find_format_by_pix(csi, f->fmt.pix.pixelformat);
+	if (!fmt) {
+		/* Just get the first one enumerated */
+		fmt = csi->supported_fmts[0];
+		f->fmt.pix.pixelformat = fmt->fourcc;
+	}
+
+	f->fmt.pix.field = csi->v_fmt.fmt.pix.field;
+
+	/* Check for/find a valid width/height. */
+	ret = 0;
+	found = false;
+	fse.pad = 0;
+	fse.code = fmt->code;
+	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	for (fse.index = 0; ; fse.index++) {
+		ret = v4l2_subdev_call(csi->subdev, pad, enum_frame_size, NULL,
+				       &fse);
+		if (ret)
+			break;
+
+		if (f->fmt.pix.width == fse.max_width &&
+		    f->fmt.pix.height == fse.max_height) {
+			found = true;
+			break;
+		} else if (f->fmt.pix.width >= fse.min_width &&
+			   f->fmt.pix.width <= fse.max_width &&
+			   f->fmt.pix.height >= fse.min_height &&
+			   f->fmt.pix.height <= fse.max_height) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		/* use existing values as default */
+		f->fmt.pix.width = csi->v_fmt.fmt.pix.width;
+		f->fmt.pix.height =  csi->v_fmt.fmt.pix.height;
+	}
+
+	/*
+	 * Use current colorspace for now, it will get
+	 * updated properly during s_fmt
+	 */
+	f->fmt.pix.colorspace = csi->v_fmt.fmt.pix.colorspace;
+	f->fmt.pix.sizeimage = f->fmt.pix.width * f->fmt.pix.height *
+			       (fmt->bpp / 8);
+
+	return 0;
+}
+
+static int ti_csi2rx_s_fmt_vid_cap(struct file *file, void *priv,
+				   struct v4l2_format *f)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+	struct vb2_queue *q = &csi->vidq;
+	const struct ti_csi2rx_fmt *fmt;
+	struct v4l2_mbus_framefmt mbus_fmt;
+	struct v4l2_subdev_format sd_fmt;
+	int ret;
+
+	if (vb2_is_busy(q))
+		return -EBUSY;
+
+	ret = ti_csi2rx_try_fmt_vid_cap(file, priv, f);
+	if (ret < 0)
+		return ret;
+
+	fmt = find_format_by_pix(csi, f->fmt.pix.pixelformat);
+	if (!fmt)
+		return -EINVAL;
+
+	v4l2_fill_mbus_format(&mbus_fmt, &f->fmt.pix, fmt->code);
+
+	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sd_fmt.pad = 0;
+	sd_fmt.format = mbus_fmt;
+
+	/*
+	 * The MIPI CSI-2 spec says that YUV422 8-bit data transmission is
+	 * perfromed using UYVY sequence. So the hardware always expects UYVY
+	 * for YUV422 streams. Ask the subdev to send UYVY data and then we can
+	 * repack it later into the format requested by the application using
+	 * the SHIM layer.
+	 */
+	if (fmt->csi_df == CSI_DF_YUV422)
+		sd_fmt.format.code = MEDIA_BUS_FMT_UYVY8_2X8;
+	else
+		sd_fmt.format.code = fmt->code;
+
+	ret = v4l2_subdev_call(csi->subdev, pad, set_fmt, NULL, &sd_fmt);
+	if (ret)
+		return ret;
+
+	ti_csi2rx_fill_fmt(&mbus_fmt, fmt, &csi->v_fmt);
+	*f = csi->v_fmt;
+
+	return 0;
+}
+
+static int ti_csi2rx_enum_framesizes(struct file *file, void *fh,
+				     struct v4l2_frmsizeenum *fsize)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+	const struct ti_csi2rx_fmt *fmt;
+	struct v4l2_subdev_frame_size_enum fse;
+	int ret;
+
+	fmt = find_format_by_pix(csi, fsize->pixel_format);
+	if (!fmt)
+		return -EINVAL;
+
+	fse.index = fsize->index;
+	fse.code = fmt->code;
+	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	fse.pad = 0;
+
+	ret = v4l2_subdev_call(csi->subdev, pad, enum_frame_size, NULL, &fse);
+	if (ret)
+		return ret;
+
+	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+	fsize->discrete.width = fse.max_width;
+	fsize->discrete.height = fse.max_height;
+
+	return 0;
+}
+
+static int ti_csi2rx_enum_input(struct file *file, void *priv,
+				struct v4l2_input *inp)
+{
+	if (inp->index > 0)
+		return -EINVAL;
+
+	inp->type = V4L2_INPUT_TYPE_CAMERA;
+	snprintf(inp->name, sizeof(inp->name), "Camera %u", inp->index);
+
+	return 0;
+}
+
+static int ti_csi2rx_g_input(struct file *file, void *priv, unsigned int *i)
+{
+	*i = 0;
+	return 0;
+}
+
+static int ti_csi2rx_s_input(struct file *file, void *priv, unsigned int i)
+{
+	return i > 0 ? -EINVAL : 0;
+}
+
+static int ti_csi2rx_enum_frameintervals(struct file *file, void *priv,
+					 struct v4l2_frmivalenum *fival)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+	const struct ti_csi2rx_fmt *fmt;
+	struct v4l2_subdev_frame_interval_enum fie = {
+		.index = fival->index,
+		.width = fival->width,
+		.height = fival->height,
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	int ret;
+
+	fmt = find_format_by_pix(csi, fival->pixel_format);
+	if (!fmt)
+		return -EINVAL;
+
+	fie.code = fmt->code;
+	ret = v4l2_subdev_call(csi->subdev, pad, enum_frame_interval,
+			       NULL, &fie);
+	if (ret)
+		return ret;
+	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
+	fival->discrete = fie.interval;
+
+	return 0;
+}
+
+static int ti_csi2rx_g_parm(struct file *file, void *fh,
+			    struct v4l2_streamparm *a)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+
+	return v4l2_g_parm_cap(video_devdata(file), csi->subdev, a);
+}
+
+static int ti_csi2rx_s_parm(struct file *file, void *fh,
+			    struct v4l2_streamparm *a)
+{
+	struct ti_csi2rx_dev *csi = video_drvdata(file);
+
+	return v4l2_s_parm_cap(video_devdata(file), csi->subdev, a);
+}
+
+static const struct v4l2_ioctl_ops csi_ioctl_ops = {
+	.vidioc_querycap      = ti_csi2rx_querycap,
+	.vidioc_enum_fmt_vid_cap = ti_csi2rx_enum_fmt_vid_cap,
+	.vidioc_try_fmt_vid_cap = ti_csi2rx_try_fmt_vid_cap,
+	.vidioc_g_fmt_vid_cap = ti_csi2rx_g_fmt_vid_cap,
+	.vidioc_s_fmt_vid_cap = ti_csi2rx_s_fmt_vid_cap,
+	.vidioc_enum_framesizes = ti_csi2rx_enum_framesizes,
+	.vidioc_enum_input    = ti_csi2rx_enum_input,
+	.vidioc_g_input       = ti_csi2rx_g_input,
+	.vidioc_s_input       = ti_csi2rx_s_input,
+	.vidioc_enum_frameintervals = ti_csi2rx_enum_frameintervals,
+	.vidioc_g_parm        = ti_csi2rx_g_parm,
+	.vidioc_s_parm        = ti_csi2rx_s_parm,
+	.vidioc_reqbufs       = vb2_ioctl_reqbufs,
+	.vidioc_create_bufs   = vb2_ioctl_create_bufs,
+	.vidioc_prepare_buf   = vb2_ioctl_prepare_buf,
+	.vidioc_querybuf      = vb2_ioctl_querybuf,
+	.vidioc_qbuf          = vb2_ioctl_qbuf,
+	.vidioc_dqbuf         = vb2_ioctl_dqbuf,
+	.vidioc_expbuf        = vb2_ioctl_expbuf,
+	.vidioc_streamon      = vb2_ioctl_streamon,
+	.vidioc_streamoff     = vb2_ioctl_streamoff,
+};
+
+static const struct v4l2_file_operations csi_fops = {
+	.owner = THIS_MODULE,
+	.open = v4l2_fh_open,
+	.release = vb2_fop_release,
+	.read = vb2_fop_read,
+	.poll = vb2_fop_poll,
+	.unlocked_ioctl = video_ioctl2,
+	.mmap = vb2_fop_mmap,
+};
+
+static int ti_csi2rx_init_video(struct ti_csi2rx_dev *csi)
+{
+	struct video_device *vdev = &csi->vdev;
+	int ret;
+
+	strscpy(vdev->name, TI_CSI2RX_MODULE_NAME, sizeof(vdev->name));
+	vdev->v4l2_dev = &csi->v4l2_dev;
+	vdev->vfl_dir = VFL_DIR_RX;
+	vdev->fops = &csi_fops;
+	vdev->ioctl_ops = &csi_ioctl_ops;
+	vdev->release = video_device_release_empty;
+	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_READWRITE |
+			    V4L2_CAP_STREAMING;
+	vdev->lock = &csi->mutex;
+	video_set_drvdata(vdev, csi);
+
+	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int csi_async_notifier_bound(struct v4l2_async_notifier *notifier,
+				    struct v4l2_subdev *subdev,
+				    struct v4l2_async_subdev *asd)
+{
+	struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev);
+
+	csi->subdev = subdev;
+
+	return 0;
+}
+
+static int csi_async_notifier_complete(struct v4l2_async_notifier *notifier)
+{
+	struct ti_csi2rx_dev *csi = dev_get_drvdata(notifier->v4l2_dev->dev);
+	int ret;
+
+	ret = ti_csi2rx_init_formats(csi);
+	if (ret)
+		return ret;
+
+	ret = ti_csi2rx_init_video(csi);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static const struct v4l2_async_notifier_operations csi_async_notifier_ops = {
+	.bound = csi_async_notifier_bound,
+	.complete = csi_async_notifier_complete,
+};
+
+static int ti_csi2rx_init_subdev(struct ti_csi2rx_dev *csi)
+{
+	struct fwnode_handle *fwnode;
+	struct v4l2_async_subdev *asd;
+	struct device_node *node;
+	int ret;
+
+	node = of_get_child_by_name(csi->dev->of_node, "csi-bridge");
+	if (!node)
+		return -EINVAL;
+
+	v4l2_async_notifier_init(&csi->notifier);
+	csi->notifier.ops = &csi_async_notifier_ops;
+
+	fwnode = of_fwnode_handle(node);
+	if (!fwnode)
+		return -EINVAL;
+
+	asd = v4l2_async_notifier_add_fwnode_subdev(&csi->notifier, fwnode,
+						    struct v4l2_async_subdev);
+	if (IS_ERR(asd))
+		return PTR_ERR(asd);
+
+	ret = v4l2_async_notifier_register(&csi->v4l2_dev, &csi->notifier);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev *csi)
+{
+	const struct ti_csi2rx_fmt *fmt;
+	unsigned int reg;
+
+	fmt = find_format_by_pix(csi, csi->v_fmt.fmt.pix.pixelformat);
+	if (!fmt) {
+		dev_err(csi->dev, "Unknown format\n");
+		return;
+	}
+
+	/* De-assert the pixel interface reset. */
+	reg = SHIM_CNTL_PIX_RST;
+	writel(reg, csi->shim + SHIM_CNTL);
+
+	reg = SHIM_DMACNTX_EN;
+	reg |= FIELD_PREP(SHIM_DMACNTX_FMT, fmt->csi_df);
+
+	/*
+	 * FIXME: Using the values from the documentation gives incorrect
+	 * ordering for the luma and chroma components. In practice, the
+	 * "reverse" format gives the correct image. So for example, if the
+	 * image is in UYVY, the reverse would be YVYU.
+	 */
+	switch (fmt->fourcc) {
+	case V4L2_PIX_FMT_UYVY:
+		reg |= FIELD_PREP(SHIM_DMACNTX_YUV422,
+					SHIM_DMACNTX_YVYU);
+		break;
+	case V4L2_PIX_FMT_VYUY:
+		reg |= FIELD_PREP(SHIM_DMACNTX_YUV422,
+					SHIM_DMACNTX_YUYV);
+		break;
+	case V4L2_PIX_FMT_YUYV:
+		reg |= FIELD_PREP(SHIM_DMACNTX_YUV422,
+					SHIM_DMACNTX_VYUY);
+		break;
+	case V4L2_PIX_FMT_YVYU:
+		reg |= FIELD_PREP(SHIM_DMACNTX_YUV422,
+					SHIM_DMACNTX_UYVY);
+		break;
+	default:
+		/* Ignore if not YUV 4:2:2 */
+		break;
+	}
+
+	writel(reg, csi->shim + SHIM_DMACNTX);
+
+	reg = FIELD_PREP(SHIM_PSI_CFG0_SRC_TAG, 0) |
+	      FIELD_PREP(SHIM_PSI_CFG0_DST_TAG, 1);
+	writel(reg, csi->shim + SHIM_PSI_CFG0);
+}
+
+static void ti_csi2rx_dma_callback(void *param)
+{
+	struct ti_csi2rx_dev *csi = param;
+	struct ti_csi2rx_buffer *buf;
+	struct ti_csi2rx_dmaq *dmaq = &csi->dmaq;
+
+	buf = list_entry(dmaq->list.next, struct ti_csi2rx_buffer, list);
+	list_del(&buf->list);
+
+	buf->vb.vb2_buf.timestamp = ktime_get_ns();
+	buf->vb.sequence = csi->sequence++;
+
+	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
+
+	/* If there are more buffers to process then start their transfer. */
+	if (list_empty(&dmaq->list))
+		return;
+
+	buf = list_entry(dmaq->list.next, struct ti_csi2rx_buffer, list);
+
+	if (ti_csi2rx_start_dma(csi, buf))
+		dev_err(csi->dev, "Failed to queue the next buffer for DMA\n");
+}
+
+static int ti_csi2rx_start_dma(struct ti_csi2rx_dev *csi,
+			       struct ti_csi2rx_buffer *buf)
+{
+	unsigned long addr;
+	struct dma_async_tx_descriptor *desc;
+	size_t len = csi->v_fmt.fmt.pix.sizeimage;
+	dma_cookie_t cookie;
+	int ret = 0;
+
+	addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0);
+	desc = dmaengine_prep_slave_single(csi->dma, addr, len, DMA_DEV_TO_MEM,
+					   DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+	if (!desc)
+		return -EIO;
+
+	desc->callback = ti_csi2rx_dma_callback;
+	desc->callback_param = csi;
+
+	cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(cookie);
+	if (ret)
+		return ret;
+
+	dma_async_issue_pending(csi->dma);
+
+	return 0;
+}
+
+static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int *nbuffers,
+				 unsigned int *nplanes, unsigned int sizes[],
+				 struct device *alloc_devs[])
+{
+	struct ti_csi2rx_dev *csi = vb2_get_drv_priv(q);
+	unsigned int size = csi->v_fmt.fmt.pix.sizeimage;
+
+	if (*nplanes) {
+		if (sizes[0] < size)
+			return -EINVAL;
+		size = sizes[0];
+	}
+
+	*nplanes = 1;
+	sizes[0] = size;
+
+	return 0;
+}
+
+static int ti_csi2rx_buffer_prepare(struct vb2_buffer *vb)
+{
+	struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vb->vb2_queue);
+	unsigned long size = csi->v_fmt.fmt.pix.sizeimage;
+
+	if (vb2_plane_size(vb, 0) < size) {
+		dev_err(csi->dev, "Data will not fit into plane\n");
+		return -EINVAL;
+	}
+
+	vb2_set_plane_payload(vb, 0, size);
+	return 0;
+}
+
+static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb)
+{
+	struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vb->vb2_queue);
+	struct ti_csi2rx_buffer *buf;
+	struct ti_csi2rx_dmaq *dmaq = &csi->dmaq;
+
+	buf = container_of(vb, struct ti_csi2rx_buffer, vb.vb2_buf);
+
+	list_add_tail(&buf->list, &dmaq->list);
+}
+
+static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
+{
+	struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq);
+	struct ti_csi2rx_dmaq *dmaq = &csi->dmaq;
+	struct ti_csi2rx_buffer *buf, *tmp;
+	int ret;
+
+	if (list_empty(&dmaq->list))
+		return -EIO;
+
+	ti_csi2rx_setup_shim(csi);
+
+	ret = v4l2_subdev_call(csi->subdev, video, s_stream, 1);
+	if (ret)
+		goto err;
+
+	csi->sequence = 0;
+
+	buf = list_entry(dmaq->list.next, struct ti_csi2rx_buffer, list);
+	ret = ti_csi2rx_start_dma(csi, buf);
+	if (ret) {
+		dev_err(csi->dev, "Failed to start DMA: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	v4l2_subdev_call(csi->subdev, video, s_stream, 0);
+	list_for_each_entry_safe(buf, tmp, &dmaq->list, list) {
+		list_del(&buf->list);
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
+	}
+
+	return ret;
+}
+
+static void ti_csi2rx_stop_streaming(struct vb2_queue *vq)
+{
+	struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vq);
+	struct ti_csi2rx_buffer *buf = NULL, *tmp;
+	int ret;
+
+	ret = v4l2_subdev_call(csi->subdev, video, s_stream, 0);
+	if (ret)
+		dev_err(csi->dev, "Failed to stop subdev stream\n");
+
+	writel(0, csi->shim + SHIM_CNTL);
+
+	ret = dmaengine_terminate_sync(csi->dma);
+	if (ret)
+		dev_err(csi->dev, "Failed to stop DMA\n");
+
+	writel(0, csi->shim + SHIM_DMACNTX);
+
+	list_for_each_entry_safe(buf, tmp, &csi->dmaq.list, list) {
+		list_del(&buf->list);
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
+	}
+}
+
+static const struct vb2_ops csi_vb2_qops = {
+	.queue_setup = ti_csi2rx_queue_setup,
+	.buf_prepare = ti_csi2rx_buffer_prepare,
+	.buf_queue = ti_csi2rx_buffer_queue,
+	.start_streaming = ti_csi2rx_start_streaming,
+	.stop_streaming = ti_csi2rx_stop_streaming,
+	.wait_prepare = vb2_ops_wait_prepare,
+	.wait_finish = vb2_ops_wait_finish,
+};
+
+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->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;
+
+	ret = dmaengine_slave_config(csi->dma, &cfg);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ti_csi2rx_media_init(struct ti_csi2rx_dev *csi)
+{
+	struct media_device *mdev = &csi->mdev;
+
+	mdev->dev = csi->dev;
+	strscpy(mdev->model, "TI-CSI2RX", sizeof(mdev->model));
+	snprintf(mdev->bus_info, sizeof(mdev->bus_info), "platform:%s",
+		 dev_name(mdev->dev));
+
+	media_device_init(mdev);
+
+	return 0;
+}
+
+static int ti_csi2rx_probe(struct platform_device *pdev)
+{
+	struct ti_csi2rx_dev *csi;
+	struct resource *res;
+	int ret;
+
+	csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
+	if (!csi)
+		return -ENOMEM;
+
+	csi->dev = &pdev->dev;
+	platform_set_drvdata(pdev, csi);
+
+	mutex_init(&csi->mutex);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	csi->shim = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(csi->shim))
+		return PTR_ERR(csi->shim);
+
+	ret = ti_csi2rx_media_init(csi);
+	if (ret)
+		return ret;
+
+	csi->v4l2_dev.mdev = &csi->mdev;
+
+	ret = v4l2_device_register(csi->dev, &csi->v4l2_dev);
+	if (ret)
+		return ret;
+
+	ret = ti_csi2rx_init_dma(csi);
+	if (ret)
+		return ret;
+
+	ret = ti_csi2rx_init_vb2q(csi);
+	if (ret)
+		return ret;
+
+	ret = ti_csi2rx_init_subdev(csi);
+	if (ret)
+		return ret;
+
+	ret = of_platform_populate(csi->dev->of_node, NULL, NULL, csi->dev);
+	if (ret) {
+		dev_err(csi->dev, "Failed to create children: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ti_csi2rx_remove(struct platform_device *pdev)
+{
+	struct ti_csi2rx_dev *csi = platform_get_drvdata(pdev);
+
+	if (vb2_is_busy(&csi->vidq))
+		return -EBUSY;
+
+	vb2_queue_release(&csi->vidq);
+	v4l2_async_notifier_unregister(&csi->notifier);
+	v4l2_async_notifier_cleanup(&csi->notifier);
+	v4l2_device_unregister(&csi->v4l2_dev);
+	video_unregister_device(&csi->vdev);
+	dma_release_channel(csi->dma);
+
+	return 0;
+}
+
+static const struct of_device_id ti_csi2rx_of_match[] = {
+	{
+		.compatible = "ti,csi2rx",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ti_csi2rx_of_match);
+
+static struct platform_driver ti_csi2rx_pdrv = {
+	.probe = ti_csi2rx_probe,
+	.remove = ti_csi2rx_remove,
+	.driver = {
+		.name = TI_CSI2RX_MODULE_NAME,
+		.of_match_table = ti_csi2rx_of_match,
+	},
+};
+
+module_platform_driver(ti_csi2rx_pdrv);
+
+MODULE_DESCRIPTION("TI CSI2 RX Driver");
+MODULE_AUTHOR("Pratyush Yadav <p.yadav@ti.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("1.0");
-- 
2.30.0

Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support

[Tomi Valkeinen]

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

Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support

[Benoit Parrot]

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

Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support

[Laurent Pinchart]

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 :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 13/16] media: ti-vpe: csi2rx: Add CSI2RX support

[Péter Ujfalusi]

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;
> +}

-- 
Péter

[PATCH 14/16] dt-bindings: phy: Convert Cadence DPHY binding to YAML

[Pratyush Yadav]

Convert Cadence DPHY binding to YAML.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 .../devicetree/bindings/phy/cdns,dphy.txt     | 20 --------
 .../devicetree/bindings/phy/cdns,dphy.yaml    | 51 +++++++++++++++++++
 2 files changed, 51 insertions(+), 20 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt
 create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml

diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.txt b/Documentation/devicetree/bindings/phy/cdns,dphy.txt
deleted file mode 100644
index 1095bc4e72d9..000000000000
--- a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
+++ /dev/null
@@ -1,20 +0,0 @@
-Cadence DPHY
-============
-
-Cadence DPHY block.
-
-Required properties:
-- compatible: should be set to "cdns,dphy".
-- reg: physical base address and length of the DPHY registers.
-- clocks: DPHY reference clocks.
-- clock-names: must contain "psm" and "pll_ref".
-- #phy-cells: must be set to 0.
-
-Example:
-	dphy0: dphy@fd0e0000{
-		compatible = "cdns,dphy";
-		reg = <0x0 0xfd0e0000 0x0 0x1000>;
-		clocks = <&psm_clk>, <&pll_ref_clk>;
-		clock-names = "psm", "pll_ref";
-		#phy-cells = <0>;
-	};
diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
new file mode 100644
index 000000000000..d1bbf96a8250
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/cdns,dphy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence DPHY Device Tree Bindings
+
+maintainers:
+  - Pratyush Yadav <p.yadav@ti.com>
+
+properties:
+  compatible:
+    items:
+      - const: cdns,dphy
+
+  reg:
+    maxItems: 1
+    description: Physical base address and length of the DPHY registers.
+
+  clocks:
+    maxItems: 2
+    description: DPHY reference clocks.
+
+  clock-names:
+    items:
+      - const: psm
+      - const: pll_ref
+
+  "#phy-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - "#phy-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+
+    dphy0: dphy@fd0e0000{
+        compatible = "cdns,dphy";
+        reg = <0xfd0e0000 0x1000>;
+        clocks = <&psm_clk>, <&pll_ref_clk>;
+        clock-names = "psm", "pll_ref";
+        #phy-cells = <0>;
+    };
-- 
2.30.0

Re: [PATCH 14/16] dt-bindings: phy: Convert Cadence DPHY binding to YAML

[Laurent Pinchart]

Hi Pratyush,

Thank you for the patch.

On Tue, Mar 30, 2021 at 11:03:46PM +0530, Pratyush Yadav wrote:
> Convert Cadence DPHY binding to YAML.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  .../devicetree/bindings/phy/cdns,dphy.txt     | 20 --------
>  .../devicetree/bindings/phy/cdns,dphy.yaml    | 51 +++++++++++++++++++
>  2 files changed, 51 insertions(+), 20 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt
>  create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.txt b/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> deleted file mode 100644
> index 1095bc4e72d9..000000000000
> --- a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> +++ /dev/null
> @@ -1,20 +0,0 @@
> -Cadence DPHY
> -============
> -
> -Cadence DPHY block.
> -
> -Required properties:
> -- compatible: should be set to "cdns,dphy".
> -- reg: physical base address and length of the DPHY registers.
> -- clocks: DPHY reference clocks.
> -- clock-names: must contain "psm" and "pll_ref".
> -- #phy-cells: must be set to 0.
> -
> -Example:
> -	dphy0: dphy@fd0e0000{
> -		compatible = "cdns,dphy";
> -		reg = <0x0 0xfd0e0000 0x0 0x1000>;
> -		clocks = <&psm_clk>, <&pll_ref_clk>;
> -		clock-names = "psm", "pll_ref";
> -		#phy-cells = <0>;
> -	};
> diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> new file mode 100644
> index 000000000000..d1bbf96a8250
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/cdns,dphy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cadence DPHY Device Tree Bindings
> +
> +maintainers:
> +  - Pratyush Yadav <p.yadav@ti.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: cdns,dphy
> +
> +  reg:
> +    maxItems: 1
> +    description: Physical base address and length of the DPHY registers.

You can drop the description.

> +
> +  clocks:
> +    maxItems: 2
> +    description: DPHY reference clocks.

It's best to describe individual items, which will then allow dropping
the maxItems property:

  clocks:
    items:
      - description: Description of the psm clock
      - description: Description of the pll_ref clock

> +
> +  clock-names:
> +    items:
> +      - const: psm
> +      - const: pll_ref
> +
> +  "#phy-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    dphy0: dphy@fd0e0000{

This is copied verbatim from the existing description, but while at it,
I'd rename the node from dphy@... to phy@..., as DT node are supposed to
be named according to the class of devices, not the specific device
type.

With these small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +        compatible = "cdns,dphy";
> +        reg = <0xfd0e0000 0x1000>;
> +        clocks = <&psm_clk>, <&pll_ref_clk>;
> +        clock-names = "psm", "pll_ref";
> +        #phy-cells = <0>;
> +    };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 14/16] dt-bindings: phy: Convert Cadence DPHY binding to YAML

[Pratyush Yadav]

On 02/04/21 01:23PM, Laurent Pinchart wrote:
> Hi Pratyush,
> 
> Thank you for the patch.
> 
> On Tue, Mar 30, 2021 at 11:03:46PM +0530, Pratyush Yadav wrote:
> > Convert Cadence DPHY binding to YAML.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  .../devicetree/bindings/phy/cdns,dphy.txt     | 20 --------
> >  .../devicetree/bindings/phy/cdns,dphy.yaml    | 51 +++++++++++++++++++
> >  2 files changed, 51 insertions(+), 20 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt
> >  create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.txt b/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> > deleted file mode 100644
> > index 1095bc4e72d9..000000000000
> > --- a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> > +++ /dev/null
> > @@ -1,20 +0,0 @@
> > -Cadence DPHY
> > -============
> > -
> > -Cadence DPHY block.
> > -
> > -Required properties:
> > -- compatible: should be set to "cdns,dphy".
> > -- reg: physical base address and length of the DPHY registers.
> > -- clocks: DPHY reference clocks.
> > -- clock-names: must contain "psm" and "pll_ref".
> > -- #phy-cells: must be set to 0.
> > -
> > -Example:
> > -	dphy0: dphy@fd0e0000{
> > -		compatible = "cdns,dphy";
> > -		reg = <0x0 0xfd0e0000 0x0 0x1000>;
> > -		clocks = <&psm_clk>, <&pll_ref_clk>;
> > -		clock-names = "psm", "pll_ref";
> > -		#phy-cells = <0>;
> > -	};
> > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > new file mode 100644
> > index 000000000000..d1bbf96a8250
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > @@ -0,0 +1,51 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/cdns,dphy.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cadence DPHY Device Tree Bindings
> > +
> > +maintainers:
> > +  - Pratyush Yadav <p.yadav@ti.com>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: cdns,dphy
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: Physical base address and length of the DPHY registers.
> 
> You can drop the description.

Ok.

> 
> > +
> > +  clocks:
> > +    maxItems: 2
> > +    description: DPHY reference clocks.
> 
> It's best to describe individual items, which will then allow dropping
> the maxItems property:
> 
>   clocks:
>     items:
>       - description: Description of the psm clock
>       - description: Description of the pll_ref clock

Ok. Though I'd mention that I am not 100% sure what the pll_ref clock is 
supposed to be. The original binding doesn't have a description either.

> 
> > +
> > +  clock-names:
> > +    items:
> > +      - const: psm
> > +      - const: pll_ref
> > +
> > +  "#phy-cells":
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - "#phy-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    dphy0: dphy@fd0e0000{
> 
> This is copied verbatim from the existing description, but while at it,
> I'd rename the node from dphy@... to phy@..., as DT node are supposed to
> be named according to the class of devices, not the specific device
> type.

Ok.

> 
> With these small issues addressed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks.

> 
> > +        compatible = "cdns,dphy";
> > +        reg = <0xfd0e0000 0x1000>;
> > +        clocks = <&psm_clk>, <&pll_ref_clk>;
> > +        clock-names = "psm", "pll_ref";
> > +        #phy-cells = <0>;
> > +    };
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH 15/16] dt-bindings: phy: cdns,dphy: make clocks optional

[Pratyush Yadav]

The clocks are not used by the DPHY when used in Rx mode so make them
optional.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
index d1bbf96a8250..0807ba68284d 100644
--- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
+++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
@@ -33,8 +33,6 @@ properties:
 required:
   - compatible
   - reg
-  - clocks
-  - clock-names
   - "#phy-cells"
 
 additionalProperties: false
-- 
2.30.0

Re: [PATCH 15/16] dt-bindings: phy: cdns,dphy: make clocks optional

[Laurent Pinchart]

Hi Pratyush,

Thank you for the patch.

On Tue, Mar 30, 2021 at 11:03:47PM +0530, Pratyush Yadav wrote:
> The clocks are not used by the DPHY when used in Rx mode so make them
> optional.

Isn't there a main functional clock (DPHY_RX_MAIN_CLK in the J721E TRM)
that is needed in RX mode ?

> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> index d1bbf96a8250..0807ba68284d 100644
> --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> @@ -33,8 +33,6 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - clocks
> -  - clock-names
>    - "#phy-cells"
>  
>  additionalProperties: false

-- 
Regards,

Laurent Pinchart

Re: [PATCH 15/16] dt-bindings: phy: cdns,dphy: make clocks optional

[Pratyush Yadav]

On 02/04/21 01:31PM, Laurent Pinchart wrote:
> Hi Pratyush,
> 
> Thank you for the patch.
> 
> On Tue, Mar 30, 2021 at 11:03:47PM +0530, Pratyush Yadav wrote:
> > The clocks are not used by the DPHY when used in Rx mode so make them
> > optional.
> 
> Isn't there a main functional clock (DPHY_RX_MAIN_CLK in the J721E TRM)
> that is needed in RX mode ?

That clock is different from the clocks being used in this binding. The 
"psm" clock is for the PMA state machine (the internal state machine for 
the DPHY). The divider for this clock should be set such that the 
resultant clock is as close to 1 MHz as possible. This can be done 
either by programming the register value or by setting the correct value 
on the psm_clock_freq pin. On J721E the pin already has the correct 
value so there is no need for setting it via the register.

The other clock is "pll_ref" which is used to set the input clock 
divider. Setting this divider is part of the DPHY TX programming 
sequence but is not part of the RX programming sequence. I'm not sure 
what exactly the divider does but I think it is supposed to divide the 
clock from the input stream to the TX DPHY to make sure the internal 
state machine is running at the correct speed. Anyway, it is not needed 
on the RX side because for that there is another register used (see 
cdns_dphy_rx_get_band_ctrl() in patch 4).

The DPHY_RX_MAIN_CLK does eventually get divided into the PSM clock but 
it is not used directly.

> 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > index d1bbf96a8250..0807ba68284d 100644
> > --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > @@ -33,8 +33,6 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - clocks
> > -  - clock-names
> >    - "#phy-cells"
> >  
> >  additionalProperties: false
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH 16/16] dt-bindings: phy: cdns,dphy: add power-domains property

[Pratyush Yadav]

This property is needed on TI platforms to enable the PD of the DPHY
before it can be used.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
index 0807ba68284d..ddcd4de0aef6 100644
--- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
+++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
@@ -30,6 +30,9 @@ properties:
   "#phy-cells":
     const: 0
 
+  power-domains:
+    maxItems: 1
+
 required:
   - compatible
   - reg
-- 
2.30.0

Re: [PATCH 16/16] dt-bindings: phy: cdns,dphy: add power-domains property

[Laurent Pinchart]

Hi Pratyush,

Thank you for the patch.

On Tue, Mar 30, 2021 at 11:03:48PM +0530, Pratyush Yadav wrote:
> This property is needed on TI platforms to enable the PD of the DPHY
> before it can be used.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> index 0807ba68284d..ddcd4de0aef6 100644
> --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> @@ -30,6 +30,9 @@ properties:
>    "#phy-cells":
>      const: 0
>  
> +  power-domains:
> +    maxItems: 1
> +

Would it be useful to add power-domains to the example ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  required:
>    - compatible
>    - reg

-- 
Regards,

Laurent Pinchart

Re: [PATCH 16/16] dt-bindings: phy: cdns,dphy: add power-domains property

[Pratyush Yadav]

On 02/04/21 01:35PM, Laurent Pinchart wrote:
> Hi Pratyush,
> 
> Thank you for the patch.
> 
> On Tue, Mar 30, 2021 at 11:03:48PM +0530, Pratyush Yadav wrote:
> > This property is needed on TI platforms to enable the PD of the DPHY
> > before it can be used.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  Documentation/devicetree/bindings/phy/cdns,dphy.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > index 0807ba68284d..ddcd4de0aef6 100644
> > --- a/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.yaml
> > @@ -30,6 +30,9 @@ properties:
> >    "#phy-cells":
> >      const: 0
> >  
> > +  power-domains:
> > +    maxItems: 1
> > +
> 
> Would it be useful to add power-domains to the example ?

Ok. Will add it in v2.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks.

> 
> >  required:
> >    - compatible
> >    - reg
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 00/16] CSI2RX support on J721E

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH 00/16] CSI2RX support on J721E

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 00/16] CSI2RX support on J721E

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH 00/16] CSI2RX support on J721E

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 00/16] CSI2RX support on J721E

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart