| Message ID | 20210330173348.30135-1-p.yadav@ti.com |
|---|---|
| Headers | show |
| Series | CSI2RX support on J721E | expand |
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.
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
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.
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 >
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
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
Hi Pratyush, Thank you for the patch. On Tue, Mar 30, 2021 at 11:03:41PM +0530, Pratyush Yadav wrote: > The subdevice power needs to be turned on before the stream is started. > Otherwise it might not be in the proper state to stream the data. Turn > it off when stopping the stream. The .s_power() operation is deprecated. Subdev drivers should control power internally in their .s_stream() operation, and they should use runtime PM to do so (this will allow usage of runtime PM autosuspend, to avoid expensive power off/on cycles when stopping and restarting video capture). > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/media/platform/cadence/cdns-csi2rx.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > index 7d1ac51e0698..3385e1bc213e 100644 > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > @@ -256,6 +256,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > writel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG); > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, true); > + if (ret && ret != -ENOIOCTLCMD) > + goto err_disable_pclk; > + > ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true); > if (ret) > goto err_disable_pclk; > @@ -358,6 +362,10 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > > + ret = v4l2_subdev_call(csi2rx->source_subdev, core, s_power, false); > + if (ret && ret != -ENOIOCTLCMD) > + dev_warn(csi2rx->dev, "Couldn't power off subdev\n"); > + > if (csi2rx->dphy) { > writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); >
On 04/04/21 04:24PM, Péter Ujfalusi wrote: > Hi Pratyush, > > On 3/30/21 8:33 PM, Pratyush Yadav wrote: > > The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > > have up to 32 threads but the current driver only supports using one. So > > add an entry for that one thread. > > If you are absolutely sure that the other threads are not going to be > used, then: The opposite in fact. I do expect other threads to be used in the future. But the current driver can only use one so I figured it is better to add just the thread that is currently needed and then I can always add the rest later. Why does this have to be a one-and-done deal? Is there anything wrong with adding the other threads when the driver can actually use them? > Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com> > > but I would consider adding the other threads if there is a chance that > the cs2rx will need to support it in the future. > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/dma/ti/k3-psil-j721e.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c > > index 7580870ed746..19ffa31e6dc6 100644 > > --- a/drivers/dma/ti/k3-psil-j721e.c > > +++ b/drivers/dma/ti/k3-psil-j721e.c > > @@ -58,6 +58,14 @@ > > }, \ > > } > > > > +#define PSIL_CSI2RX(x) \ > > + { \ > > + .thread_id = x, \ > > + .ep_config = { \ > > + .ep_type = PSIL_EP_NATIVE, \ > > + }, \ > > + } > > + > > /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */ > > static struct psil_ep j721e_src_ep_map[] = { > > /* SA2UL */ > > @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = { > > PSIL_PDMA_XY_PKT(0x4707), > > PSIL_PDMA_XY_PKT(0x4708), > > PSIL_PDMA_XY_PKT(0x4709), > > + /* CSI2RX */ > > + PSIL_CSI2RX(0x4940), > > /* CPSW9 */ > > PSIL_ETHERNET(0x4a00), > > /* CPSW0 */ > > > > -- > Péter
On 06/04/21 06:33PM, Péter Ujfalusi wrote: > > > On 4/6/21 6:09 PM, Pratyush Yadav wrote: > > On 04/04/21 04:24PM, Péter Ujfalusi wrote: > >> Hi Pratyush, > >> > >> On 3/30/21 8:33 PM, Pratyush Yadav wrote: > >>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > >>> have up to 32 threads but the current driver only supports using one. So > >>> add an entry for that one thread. > >> > >> If you are absolutely sure that the other threads are not going to be > >> used, then: > > > > The opposite in fact. I do expect other threads to be used in the > > future. But the current driver can only use one so I figured it is > > better to add just the thread that is currently needed and then I can > > always add the rest later. > > > > Why does this have to be a one-and-done deal? Is there anything wrong > > with adding the other threads when the driver can actually use them? > > You can skip CCing DMAengine (and me ;) ). Less subsystems is the better > when sending patches... I'm a bit confused here. If you are no longer interested in maintaining the TI DMA drivers then that's fine, I can skip CCing you. But the patch is still relevant to the dmaengine list so why should I skip CCing it? And if I don't CC the dmaengine list then on which list would I get comments/reviews for the patch? > > > > >> Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com> > >> > >> but I would consider adding the other threads if there is a chance that > >> the cs2rx will need to support it in the future. > >> > >>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > >>> --- > >>> drivers/dma/ti/k3-psil-j721e.c | 10 ++++++++++ > >>> 1 file changed, 10 insertions(+) > >>> > >>> diff --git a/drivers/dma/ti/k3-psil-j721e.c b/drivers/dma/ti/k3-psil-j721e.c > >>> index 7580870ed746..19ffa31e6dc6 100644 > >>> --- a/drivers/dma/ti/k3-psil-j721e.c > >>> +++ b/drivers/dma/ti/k3-psil-j721e.c > >>> @@ -58,6 +58,14 @@ > >>> }, \ > >>> } > >>> > >>> +#define PSIL_CSI2RX(x) \ > >>> + { \ > >>> + .thread_id = x, \ > >>> + .ep_config = { \ > >>> + .ep_type = PSIL_EP_NATIVE, \ > >>> + }, \ > >>> + } > >>> + > >>> /* PSI-L source thread IDs, used for RX (DMA_DEV_TO_MEM) */ > >>> static struct psil_ep j721e_src_ep_map[] = { > >>> /* SA2UL */ > >>> @@ -138,6 +146,8 @@ static struct psil_ep j721e_src_ep_map[] = { > >>> PSIL_PDMA_XY_PKT(0x4707), > >>> PSIL_PDMA_XY_PKT(0x4708), > >>> PSIL_PDMA_XY_PKT(0x4709), > >>> + /* CSI2RX */ > >>> + PSIL_CSI2RX(0x4940), > >>> /* CPSW9 */ > >>> PSIL_ETHERNET(0x4a00), > >>> /* CPSW0 */ > >>> > >> > >> -- > >> Péter > > > > -- > Péter
On 06/04/21 10:25PM, Pratyush Yadav wrote: > On 06/04/21 06:33PM, Péter Ujfalusi wrote: > > > > > > On 4/6/21 6:09 PM, Pratyush Yadav wrote: > > > On 04/04/21 04:24PM, Péter Ujfalusi wrote: > > >> Hi Pratyush, > > >> > > >> On 3/30/21 8:33 PM, Pratyush Yadav wrote: > > >>> The CSI2RX subsystem uses PSI-L DMA to transfer frames to memory. It can > > >>> have up to 32 threads but the current driver only supports using one. So > > >>> add an entry for that one thread. > > >> > > >> If you are absolutely sure that the other threads are not going to be > > >> used, then: > > > > > > The opposite in fact. I do expect other threads to be used in the > > > future. But the current driver can only use one so I figured it is > > > better to add just the thread that is currently needed and then I can > > > always add the rest later. > > > > > > Why does this have to be a one-and-done deal? Is there anything wrong > > > with adding the other threads when the driver can actually use them? > > > > You can skip CCing DMAengine (and me ;) ). Less subsystems is the better > > when sending patches... > > I'm a bit confused here. If you are no longer interested in maintaining > the TI DMA drivers then that's fine, I can skip CCing you. But the patch > is still relevant to the dmaengine list so why should I skip CCing it? > And if I don't CC the dmaengine list then on which list would I get > comments/reviews for the patch? Ignore this. Got your point. Will do it in v2.
On 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 > >
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.
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.
On 31/03/21 05:24PM, Chunfeng Yun wrote: > On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote: > > Some platforms like TI's J721E can have the CSI2RX paired with an > > external DPHY. Add support to enable and configure the DPHY using the > > generic PHY framework. > > > > Get the pixel rate and bpp from the subdev and pass them on to the DPHY > > along with the number of lanes. All other settings are left to their > > default values. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > drivers/media/platform/cadence/cdns-csi2rx.c | 147 +++++++++++++++++-- > > 1 file changed, 137 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > index c68a3eac62cd..31bd80e3f780 100644 > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > @@ -30,6 +30,12 @@ > > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > > +#define CSI2RX_DPHY_CL_RST BIT(16) > > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > > +#define CSI2RX_DPHY_CL_EN BIT(4) > > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > > + > > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > > @@ -54,6 +60,11 @@ enum csi2rx_pads { > > CSI2RX_PAD_MAX, > > }; > > > > +struct csi2rx_fmt { > > + u32 code; > > + u8 bpp; > > +}; > > + > > struct csi2rx_priv { > > struct device *dev; > > unsigned int count; > > @@ -85,6 +96,52 @@ struct csi2rx_priv { > > int source_pad; > > }; > > > > +static const struct csi2rx_fmt formats[] = { > > + { > > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > > + .bpp = 16, > > + }, > > + { > > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > > + .bpp = 16, > > + }, > > +}; > > + > > +static u8 csi2rx_get_bpp(u32 code) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > > + if (formats[i].code == code) > > + return formats[i].bpp; > > + } > > + > > + return 0; > > +} > > + > > +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) > > +{ > > + struct v4l2_ctrl *ctrl; > > + > > + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, > > + V4L2_CID_PIXEL_RATE); > > + if (!ctrl) { > > + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", > > + csi2rx->source_subdev->name); > > + return -EINVAL; > > + } > > + > > + return v4l2_ctrl_g_ctrl_int64(ctrl); > > +} > > + > > static inline > > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > > { > > @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > > } > > > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > > +{ > > + union phy_configure_opts opts = { }; > > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > > + struct v4l2_subdev_format sd_fmt; > > + s64 pixel_rate; > > + int ret; > > + u8 bpp; > > + > > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > + sd_fmt.pad = 0; > > + > > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, > > + &sd_fmt); > > + if (ret) > > + return ret; > > + > > + bpp = csi2rx_get_bpp(sd_fmt.format.code); > > + if (!bpp) > > + return -EINVAL; > > + > > + pixel_rate = csi2rx_get_pixel_rate(csi2rx); > > + if (pixel_rate < 0) > > + return pixel_rate; > > + > > + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, > > + csi2rx->num_lanes, cfg); > > + if (ret) > > + return ret; > > + > > + ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY, > > + PHY_MIPI_DPHY_SUBMODE_RX); > > + if (ret) > > + return ret; > > + > > + ret = phy_power_on(csi2rx->dphy); > > + if (ret) > > + return ret; > Seems phy_power_on, then phy_set_mode_ext? Shouldn't the mode be set before the PHY is powered on so the correct power on procedure can be performed based on the mode of operation? > > > + > > + ret = phy_configure(csi2rx->dphy, &opts); > > + if (ret) { > > + /* Can't do anything if it fails. Ignore the return value. */ > > + phy_power_off(csi2rx->dphy); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > static int csi2rx_start(struct csi2rx_priv *csi2rx) > > { > > unsigned int i; > > @@ -139,6 +245,17 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > if (ret) > > goto err_disable_pclk; > > > > + /* Enable DPHY clk and data lanes. */ > > + if (csi2rx->dphy) { > > + reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST; > > + for (i = 0; i < csi2rx->num_lanes; i++) { > > + reg |= CSI2RX_DPHY_DL_EN(csi2rx->lanes[i] - 1); > > + reg |= CSI2RX_DPHY_DL_RST(csi2rx->lanes[i] - 1); > > + } > > + > > + writel(reg, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > + } > > + > > /* > > * Create a static mapping between the CSI virtual channels > > * and the output stream. > > @@ -169,10 +286,21 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > if (ret) > > goto err_disable_pixclk; > > > > + if (csi2rx->dphy) { > > + ret = csi2rx_configure_external_dphy(csi2rx); > > + if (ret) { > > + dev_err(csi2rx->dev, > > + "Failed to configure external DPHY: %d\n", ret); > > + goto err_disable_sysclk; > > + } > > + } > > + > > clk_disable_unprepare(csi2rx->p_clk); > > > > return 0; > > > > +err_disable_sysclk: > > + clk_disable_unprepare(csi2rx->sys_clk); > > err_disable_pixclk: > > for (; i > 0; i--) > > clk_disable_unprepare(csi2rx->pixel_clk[i - 1]); > > @@ -200,6 +328,13 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > > > > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > > + > > + if (csi2rx->dphy) { > > + writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > + > > + if (phy_power_off(csi2rx->dphy)) > > + dev_warn(csi2rx->dev, "Couldn't power off DPHY\n"); > > + } > > } > > > > static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > @@ -306,15 +441,6 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > return PTR_ERR(csi2rx->dphy); > > } > > > > - /* > > - * FIXME: Once we'll have external D-PHY support, the check > > - * will need to be removed. > > - */ > > - if (csi2rx->dphy) { > > - dev_err(&pdev->dev, "External D-PHY not supported yet\n"); > > - return -EINVAL; > > - } > > - > > clk_prepare_enable(csi2rx->p_clk); > > dev_cfg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG); > > clk_disable_unprepare(csi2rx->p_clk); > > @@ -339,7 +465,7 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > * FIXME: Once we'll have internal D-PHY support, the check > > * will need to be removed. > > */ > > - if (csi2rx->has_internal_dphy) { > > + if (!csi2rx->dphy && csi2rx->has_internal_dphy) { > > dev_err(&pdev->dev, "Internal D-PHY not supported yet\n"); > > return -EINVAL; > > } > > @@ -460,6 +586,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > dev_info(&pdev->dev, > > "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n", > > csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams, > > + csi2rx->dphy ? "external" : > > csi2rx->has_internal_dphy ? "internal" : "no"); > > > > return 0; >
On Wed, 2021-04-07 at 00:24 +0530, Pratyush Yadav wrote: > On 31/03/21 05:24PM, Chunfeng Yun wrote: > > On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote: > > > Some platforms like TI's J721E can have the CSI2RX paired with an > > > external DPHY. Add support to enable and configure the DPHY using the > > > generic PHY framework. > > > > > > Get the pixel rate and bpp from the subdev and pass them on to the DPHY > > > along with the number of lanes. All other settings are left to their > > > default values. > > > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > --- > > > drivers/media/platform/cadence/cdns-csi2rx.c | 147 +++++++++++++++++-- > > > 1 file changed, 137 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c > > > index c68a3eac62cd..31bd80e3f780 100644 > > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c > > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c > > > @@ -30,6 +30,12 @@ > > > #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) > > > #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) > > > > > > +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 > > > +#define CSI2RX_DPHY_CL_RST BIT(16) > > > +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) > > > +#define CSI2RX_DPHY_CL_EN BIT(4) > > > +#define CSI2RX_DPHY_DL_EN(i) BIT(i) > > > + > > > #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) > > > > > > #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) > > > @@ -54,6 +60,11 @@ enum csi2rx_pads { > > > CSI2RX_PAD_MAX, > > > }; > > > > > > +struct csi2rx_fmt { > > > + u32 code; > > > + u8 bpp; > > > +}; > > > + > > > struct csi2rx_priv { > > > struct device *dev; > > > unsigned int count; > > > @@ -85,6 +96,52 @@ struct csi2rx_priv { > > > int source_pad; > > > }; > > > > > > +static const struct csi2rx_fmt formats[] = { > > > + { > > > + .code = MEDIA_BUS_FMT_YUYV8_2X8, > > > + .bpp = 16, > > > + }, > > > + { > > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > > + .bpp = 16, > > > + }, > > > + { > > > + .code = MEDIA_BUS_FMT_YVYU8_2X8, > > > + .bpp = 16, > > > + }, > > > + { > > > + .code = MEDIA_BUS_FMT_VYUY8_2X8, > > > + .bpp = 16, > > > + }, > > > +}; > > > + > > > +static u8 csi2rx_get_bpp(u32 code) > > > +{ > > > + int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > > > + if (formats[i].code == code) > > > + return formats[i].bpp; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) > > > +{ > > > + struct v4l2_ctrl *ctrl; > > > + > > > + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, > > > + V4L2_CID_PIXEL_RATE); > > > + if (!ctrl) { > > > + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", > > > + csi2rx->source_subdev->name); > > > + return -EINVAL; > > > + } > > > + > > > + return v4l2_ctrl_g_ctrl_int64(ctrl); > > > +} > > > + > > > static inline > > > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) > > > { > > > @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) > > > writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); > > > } > > > > > > +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) > > > +{ > > > + union phy_configure_opts opts = { }; > > > + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; > > > + struct v4l2_subdev_format sd_fmt; > > > + s64 pixel_rate; > > > + int ret; > > > + u8 bpp; > > > + > > > + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > > + sd_fmt.pad = 0; > > > + > > > + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, > > > + &sd_fmt); > > > + if (ret) > > > + return ret; > > > + > > > + bpp = csi2rx_get_bpp(sd_fmt.format.code); > > > + if (!bpp) > > > + return -EINVAL; > > > + > > > + pixel_rate = csi2rx_get_pixel_rate(csi2rx); > > > + if (pixel_rate < 0) > > > + return pixel_rate; > > > + > > > + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, > > > + csi2rx->num_lanes, cfg); > > > + if (ret) > > > + return ret; > > > + > > > + ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY, > > > + PHY_MIPI_DPHY_SUBMODE_RX); > > > + if (ret) > > > + return ret; > > > + > > > + ret = phy_power_on(csi2rx->dphy); > > > + if (ret) > > > + return ret; > > Seems phy_power_on, then phy_set_mode_ext? > > Shouldn't the mode be set before the PHY is powered on so the correct > power on procedure can be performed based on the mode of operation? Of course, it is fine for cnds-dphy. But it depends on HW design and also phy driver; if the mode is controlled in PHY IP register, we can't access it before phy_power_on if no phy_init called (e.g. clock/power is not enabled). Just let you pay attention on the phy sequence. Thanks > > > > > > + > > > + ret = phy_configure(csi2rx->dphy, &opts); > > > + if (ret) { > > > + /* Can't do anything if it fails. Ignore the return value. */ > > > + phy_power_off(csi2rx->dphy); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > static int csi2rx_start(struct csi2rx_priv *csi2rx) > > > { > > > unsigned int i; > > > @@ -139,6 +245,17 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > > if (ret) > > > goto err_disable_pclk; > > > > > > + /* Enable DPHY clk and data lanes. */ > > > + if (csi2rx->dphy) { > > > + reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST; > > > + for (i = 0; i < csi2rx->num_lanes; i++) { > > > + reg |= CSI2RX_DPHY_DL_EN(csi2rx->lanes[i] - 1); > > > + reg |= CSI2RX_DPHY_DL_RST(csi2rx->lanes[i] - 1); > > > + } > > > + > > > + writel(reg, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > > + } > > > + > > > /* > > > * Create a static mapping between the CSI virtual channels > > > * and the output stream. > > > @@ -169,10 +286,21 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx) > > > if (ret) > > > goto err_disable_pixclk; > > > > > > + if (csi2rx->dphy) { > > > + ret = csi2rx_configure_external_dphy(csi2rx); > > > + if (ret) { > > > + dev_err(csi2rx->dev, > > > + "Failed to configure external DPHY: %d\n", ret); > > > + goto err_disable_sysclk; > > > + } > > > + } > > > + > > > clk_disable_unprepare(csi2rx->p_clk); > > > > > > return 0; > > > > > > +err_disable_sysclk: > > > + clk_disable_unprepare(csi2rx->sys_clk); > > > err_disable_pixclk: > > > for (; i > 0; i--) > > > clk_disable_unprepare(csi2rx->pixel_clk[i - 1]); > > > @@ -200,6 +328,13 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx) > > > > > > if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false)) > > > dev_warn(csi2rx->dev, "Couldn't disable our subdev\n"); > > > + > > > + if (csi2rx->dphy) { > > > + writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG); > > > + > > > + if (phy_power_off(csi2rx->dphy)) > > > + dev_warn(csi2rx->dev, "Couldn't power off DPHY\n"); > > > + } > > > } > > > > > > static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable) > > > @@ -306,15 +441,6 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > > return PTR_ERR(csi2rx->dphy); > > > } > > > > > > - /* > > > - * FIXME: Once we'll have external D-PHY support, the check > > > - * will need to be removed. > > > - */ > > > - if (csi2rx->dphy) { > > > - dev_err(&pdev->dev, "External D-PHY not supported yet\n"); > > > - return -EINVAL; > > > - } > > > - > > > clk_prepare_enable(csi2rx->p_clk); > > > dev_cfg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG); > > > clk_disable_unprepare(csi2rx->p_clk); > > > @@ -339,7 +465,7 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > > * FIXME: Once we'll have internal D-PHY support, the check > > > * will need to be removed. > > > */ > > > - if (csi2rx->has_internal_dphy) { > > > + if (!csi2rx->dphy && csi2rx->has_internal_dphy) { > > > dev_err(&pdev->dev, "Internal D-PHY not supported yet\n"); > > > return -EINVAL; > > > } > > > @@ -460,6 +586,7 @@ static int csi2rx_probe(struct platform_device *pdev) > > > dev_info(&pdev->dev, > > > "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n", > > > csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams, > > > + csi2rx->dphy ? "external" : > > > csi2rx->has_internal_dphy ? "internal" : "no"); > > > > > > return 0; > > >
On 08/04/2021 11:13, Chunfeng Yun wrote: > On Wed, 2021-04-07 at 00:24 +0530, Pratyush Yadav wrote: >> On 31/03/21 05:24PM, Chunfeng Yun wrote: >>> On Tue, 2021-03-30 at 23:03 +0530, Pratyush Yadav wrote: >>>> Some platforms like TI's J721E can have the CSI2RX paired with an >>>> external DPHY. Add support to enable and configure the DPHY using the >>>> generic PHY framework. >>>> >>>> Get the pixel rate and bpp from the subdev and pass them on to the DPHY >>>> along with the number of lanes. All other settings are left to their >>>> default values. >>>> >>>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com> >>>> --- >>>> drivers/media/platform/cadence/cdns-csi2rx.c | 147 +++++++++++++++++-- >>>> 1 file changed, 137 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c >>>> index c68a3eac62cd..31bd80e3f780 100644 >>>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c >>>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c >>>> @@ -30,6 +30,12 @@ >>>> #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane) ((plane) << (16 + (llane) * 4)) >>>> #define CSI2RX_STATIC_CFG_LANES_MASK GENMASK(11, 8) >>>> >>>> +#define CSI2RX_DPHY_LANE_CTRL_REG 0x40 >>>> +#define CSI2RX_DPHY_CL_RST BIT(16) >>>> +#define CSI2RX_DPHY_DL_RST(i) BIT((i) + 12) >>>> +#define CSI2RX_DPHY_CL_EN BIT(4) >>>> +#define CSI2RX_DPHY_DL_EN(i) BIT(i) >>>> + >>>> #define CSI2RX_STREAM_BASE(n) (((n) + 1) * 0x100) >>>> >>>> #define CSI2RX_STREAM_CTRL_REG(n) (CSI2RX_STREAM_BASE(n) + 0x000) >>>> @@ -54,6 +60,11 @@ enum csi2rx_pads { >>>> CSI2RX_PAD_MAX, >>>> }; >>>> >>>> +struct csi2rx_fmt { >>>> + u32 code; >>>> + u8 bpp; >>>> +}; >>>> + >>>> struct csi2rx_priv { >>>> struct device *dev; >>>> unsigned int count; >>>> @@ -85,6 +96,52 @@ struct csi2rx_priv { >>>> int source_pad; >>>> }; >>>> >>>> +static const struct csi2rx_fmt formats[] = { >>>> + { >>>> + .code = MEDIA_BUS_FMT_YUYV8_2X8, >>>> + .bpp = 16, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_UYVY8_2X8, >>>> + .bpp = 16, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_YVYU8_2X8, >>>> + .bpp = 16, >>>> + }, >>>> + { >>>> + .code = MEDIA_BUS_FMT_VYUY8_2X8, >>>> + .bpp = 16, >>>> + }, >>>> +}; >>>> + >>>> +static u8 csi2rx_get_bpp(u32 code) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(formats); i++) { >>>> + if (formats[i].code == code) >>>> + return formats[i].bpp; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx) >>>> +{ >>>> + struct v4l2_ctrl *ctrl; >>>> + >>>> + ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler, >>>> + V4L2_CID_PIXEL_RATE); >>>> + if (!ctrl) { >>>> + dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n", >>>> + csi2rx->source_subdev->name); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return v4l2_ctrl_g_ctrl_int64(ctrl); >>>> +} >>>> + >>>> static inline >>>> struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev) >>>> { >>>> @@ -101,6 +158,55 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx) >>>> writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG); >>>> } >>>> >>>> +static int csi2rx_configure_external_dphy(struct csi2rx_priv *csi2rx) >>>> +{ >>>> + union phy_configure_opts opts = { }; >>>> + struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy; >>>> + struct v4l2_subdev_format sd_fmt; >>>> + s64 pixel_rate; >>>> + int ret; >>>> + u8 bpp; >>>> + >>>> + sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE; >>>> + sd_fmt.pad = 0; >>>> + >>>> + ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL, >>>> + &sd_fmt); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + bpp = csi2rx_get_bpp(sd_fmt.format.code); >>>> + if (!bpp) >>>> + return -EINVAL; >>>> + >>>> + pixel_rate = csi2rx_get_pixel_rate(csi2rx); >>>> + if (pixel_rate < 0) >>>> + return pixel_rate; >>>> + >>>> + ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp, >>>> + csi2rx->num_lanes, cfg); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = phy_set_mode_ext(csi2rx->dphy, PHY_MODE_MIPI_DPHY, >>>> + PHY_MIPI_DPHY_SUBMODE_RX); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = phy_power_on(csi2rx->dphy); >>>> + if (ret) >>>> + return ret; >>> Seems phy_power_on, then phy_set_mode_ext? >> >> Shouldn't the mode be set before the PHY is powered on so the correct >> power on procedure can be performed based on the mode of operation? > Of course, it is fine for cnds-dphy. > But it depends on HW design and also phy driver; > if the mode is controlled in PHY IP register, we can't access it before > phy_power_on if no phy_init called (e.g. clock/power is not enabled). > > Just let you pay attention on the phy sequence. I don't think the phy configuration should depend on phy_power_on, but the runtime PM. I guess this could be solved with: phy_pm_runtime_get_sync(); phy_set_mode_ext(); phy_power_on(); phy_pm_runtime_put(); Tomi