[v12,09/11] media: staging: dt-bindings: add Rockchip MIPI RX D-PHY yaml bindings
diff mbox series

Message ID 20191227200116.2612137-10-helen.koike@collabora.com
State New
Headers show
Series
  • Rockchip ISP Driver
Related show

Commit Message

Helen Koike Dec. 27, 2019, 8:01 p.m. UTC
Add yaml DT bindings for Rockchip MIPI D-PHY RX

This was tested and verified with:
mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml  Documentation/devicetree/bindings/phy/
make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

Changes in v12:
- The commit replaces the following commit in previous series named
media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
This new patch adds yaml binding and was verified with
make dtbs_check and make dt_binding_check

Changes in v11: None
Changes in v10:
- unsquash

Changes in v9:
- fix title division style
- squash
- move to staging

Changes in v8: None
Changes in v7:
- updated doc with new design and tested example

 .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml

Comments

Rob Herring Jan. 6, 2020, 10:29 p.m. UTC | #1
On Fri, Dec 27, 2019 at 2:02 PM Helen Koike <helen.koike@collabora.com> wrote:
>
> Add yaml DT bindings for Rockchip MIPI D-PHY RX
>
> This was tested and verified with:
> mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml  Documentation/devicetree/bindings/phy/
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>
> ---
>
> Changes in v12:
> - The commit replaces the following commit in previous series named
> media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> This new patch adds yaml binding and was verified with
> make dtbs_check and make dt_binding_check
>
> Changes in v11: None
> Changes in v10:
> - unsquash
>
> Changes in v9:
> - fix title division style
> - squash
> - move to staging
>
> Changes in v8: None
> Changes in v7:
> - updated doc with new design and tested example
>
>  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml

Reviewed-by: Rob Herring <robh@kernel.org>
Laurent Pinchart Jan. 7, 2020, 12:10 a.m. UTC | #2
Hi Helen,

Thank you for the patch.

On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> Add yaml DT bindings for Rockchip MIPI D-PHY RX
> 
> This was tested and verified with:
> mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml  Documentation/devicetree/bindings/phy/
> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
> Changes in v12:
> - The commit replaces the following commit in previous series named
> media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> This new patch adds yaml binding and was verified with
> make dtbs_check and make dt_binding_check
> 
> Changes in v11: None
> Changes in v10:
> - unsquash
> 
> Changes in v9:
> - fix title division style
> - squash
> - move to staging
> 
> Changes in v8: None
> Changes in v7:
> - updated doc with new design and tested example
> 
>  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> 
> diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> new file mode 100644
> index 000000000000..af97f1b3e005
> --- /dev/null
> +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings

Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
Looking at the PHY driver, it seems to handle all PHYs with a single
struct device. Should we thus use #phy-cells = <1> to select the PHY ?

> +
> +maintainers:
> +  - Helen Koike <helen.koike@collabora.com>
> +  - Ezequiel Garcia <ezequiel@collabora.com>
> +
> +description: |
> +  The Rockchip SoC has a MIPI D-PHY bus with an RX0 entry which connects to
> +  the ISP1 (Image Signal Processing unit v1.0) for CSI cameras.
> +
> +properties:
> +  compatible:
> +    const: rockchip,rk3399-mipi-dphy
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Mipi d-phy ref clock
> +      - description: Mipi d-phy rx0 cfg clock

s/Mipi d-phy/MIPI D-PHY/

> +      - description: Video in/out general register file clock
> +
> +  clock-names:
> +    items:
> +      - const: dphy-ref
> +      - const: dphy-cfg
> +      - const: grf
> +
> +  '#phy-cells':
> +    const: 0
> +
> +  power-domains:
> +    description: Video in/out power domain.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - '#phy-cells'
> +  - power-domains
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    /*
> +     * MIPI RX D-PHY use registers in "general register files", it
> +     * should be a child of the GRF.
> +     *
> +     * grf: syscon@ff770000 {
> +     *  compatible = "rockchip,rk3399-grf", "syscon", "simple-mfd";
> +     *  ...

missing

	* };

> +     */
> +
> +    #include <dt-bindings/clock/rk3399-cru.h>
> +    #include <dt-bindings/power/rk3399-power.h>
> +
> +    dphy: mipi-dphy {
> +        compatible = "rockchip,rk3399-mipi-dphy";
> +        clocks = <&cru SCLK_MIPIDPHY_REF>,
> +                 <&cru SCLK_DPHY_RX0_CFG>,
> +                 <&cru PCLK_VIO_GRF>;
> +        clock-names = "dphy-ref", "dphy-cfg", "grf";
> +        power-domains = <&power RK3399_PD_VIO>;
> +        #phy-cells = <0>;
> +    };
Ezequiel Garcia Jan. 7, 2020, 2:06 a.m. UTC | #3
Hi Laurent,

Thanks a lot for reviewing this.

On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> Hi Helen,
> 
> Thank you for the patch.
> 
> On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > 
> > This was tested and verified with:
> > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml  Documentation/devicetree/bindings/phy/
> > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > 
> > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > 
> > ---
> > 
> > Changes in v12:
> > - The commit replaces the following commit in previous series named
> > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > This new patch adds yaml binding and was verified with
> > make dtbs_check and make dt_binding_check
> > 
> > Changes in v11: None
> > Changes in v10:
> > - unsquash
> > 
> > Changes in v9:
> > - fix title division style
> > - squash
> > - move to staging
> > 
> > Changes in v8: None
> > Changes in v7:
> > - updated doc with new design and tested example
> > 
> >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > 
> > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml b/drivers/staging/media/phy-
> > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > new file mode 100644
> > index 000000000000..af97f1b3e005
> > --- /dev/null
> > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > @@ -0,0 +1,75 @@
> > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> 
> Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?

The driver currently only supports RX0, but I think you are right,
it should say RX here. This binding could be extended for RX1.

> Looking at the PHY driver, it seems to handle all PHYs with a single
> struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> 

I am not following this. The driver handles just one PHY. Each PHY
should have its own node.

> > +
> > +maintainers:
> > +  - Helen Koike <helen.koike@collabora.com>
> > +  - Ezequiel Garcia <ezequiel@collabora.com>
> > +
> > +description: |
> > +  The Rockchip SoC has a MIPI D-PHY bus with an RX0 entry which connects to
> > +  the ISP1 (Image Signal Processing unit v1.0) for CSI cameras.
> > +
> > +properties:
> > +  compatible:
> > +    const: rockchip,rk3399-mipi-dphy
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: Mipi d-phy ref clock
> > +      - description: Mipi d-phy rx0 cfg clock
> 
> s/Mipi d-phy/MIPI D-PHY/
> 

Yep.

> > +      - description: Video in/out general register file clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: dphy-ref
> > +      - const: dphy-cfg
> > +      - const: grf
> > +
> > +  '#phy-cells':
> > +    const: 0
> > +
> > +  power-domains:
> > +    description: Video in/out power domain.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - clocks
> > +  - clock-names
> > +  - '#phy-cells'
> > +  - power-domains
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    /*
> > +     * MIPI RX D-PHY use registers in "general register files", it
> > +     * should be a child of the GRF.
> > +     *
> > +     * grf: syscon@ff770000 {
> > +     *  compatible = "rockchip,rk3399-grf", "syscon", "simple-mfd";
> > +     *  ...
> 
> missing
> 
> 	* };
> 

OK.

> > +     */
> > +
> > +    #include <dt-bindings/clock/rk3399-cru.h>
> > +    #include <dt-bindings/power/rk3399-power.h>
> > +
> > +    dphy: mipi-dphy {
> > +        compatible = "rockchip,rk3399-mipi-dphy";
> > +        clocks = <&cru SCLK_MIPIDPHY_REF>,
> > +                 <&cru SCLK_DPHY_RX0_CFG>,
> > +                 <&cru PCLK_VIO_GRF>;
> > +        clock-names = "dphy-ref", "dphy-cfg", "grf";
> > +        power-domains = <&power RK3399_PD_VIO>;
> > +        #phy-cells = <0>;
> > +    };
Laurent Pinchart Jan. 7, 2020, 2:37 a.m. UTC | #4
On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > Hi Helen,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > 
> > > This was tested and verified with:
> > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml  Documentation/devicetree/bindings/phy/
> > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > 
> > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > 
> > > ---
> > > 
> > > Changes in v12:
> > > - The commit replaces the following commit in previous series named
> > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > This new patch adds yaml binding and was verified with
> > > make dtbs_check and make dt_binding_check
> > > 
> > > Changes in v11: None
> > > Changes in v10:
> > > - unsquash
> > > 
> > > Changes in v9:
> > > - fix title division style
> > > - squash
> > > - move to staging
> > > 
> > > Changes in v8: None
> > > Changes in v7:
> > > - updated doc with new design and tested example
> > > 
> > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > >  1 file changed, 75 insertions(+)
> > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > 
> > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml b/drivers/staging/media/phy-
> > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > new file mode 100644
> > > index 000000000000..af97f1b3e005
> > > --- /dev/null
> > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > @@ -0,0 +1,75 @@
> > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > 
> > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> 
> The driver currently only supports RX0, but I think you are right,
> it should say RX here. This binding could be extended for RX1.
> 
> > Looking at the PHY driver, it seems to handle all PHYs with a single
> > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> 
> I am not following this. The driver handles just one PHY. Each PHY
> should have its own node.

Looking at the registers, it seems that the different PHYs are
intertwined and we would could have trouble handling the different PHYs
with different DT nodes and thus struct device instances.

> > > +
> > > +maintainers:
> > > +  - Helen Koike <helen.koike@collabora.com>
> > > +  - Ezequiel Garcia <ezequiel@collabora.com>
> > > +
> > > +description: |
> > > +  The Rockchip SoC has a MIPI D-PHY bus with an RX0 entry which connects to
> > > +  the ISP1 (Image Signal Processing unit v1.0) for CSI cameras.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: rockchip,rk3399-mipi-dphy
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: Mipi d-phy ref clock
> > > +      - description: Mipi d-phy rx0 cfg clock
> > 
> > s/Mipi d-phy/MIPI D-PHY/
> 
> Yep.
> 
> > > +      - description: Video in/out general register file clock
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: dphy-ref
> > > +      - const: dphy-cfg
> > > +      - const: grf
> > > +
> > > +  '#phy-cells':
> > > +    const: 0
> > > +
> > > +  power-domains:
> > > +    description: Video in/out power domain.
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - clocks
> > > +  - clock-names
> > > +  - '#phy-cells'
> > > +  - power-domains
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +
> > > +    /*
> > > +     * MIPI RX D-PHY use registers in "general register files", it
> > > +     * should be a child of the GRF.
> > > +     *
> > > +     * grf: syscon@ff770000 {
> > > +     *  compatible = "rockchip,rk3399-grf", "syscon", "simple-mfd";
> > > +     *  ...
> > 
> > missing
> > 
> > 	* };
> 
> OK.
> 
> > > +     */
> > > +
> > > +    #include <dt-bindings/clock/rk3399-cru.h>
> > > +    #include <dt-bindings/power/rk3399-power.h>
> > > +
> > > +    dphy: mipi-dphy {
> > > +        compatible = "rockchip,rk3399-mipi-dphy";
> > > +        clocks = <&cru SCLK_MIPIDPHY_REF>,
> > > +                 <&cru SCLK_DPHY_RX0_CFG>,
> > > +                 <&cru PCLK_VIO_GRF>;
> > > +        clock-names = "dphy-ref", "dphy-cfg", "grf";
> > > +        power-domains = <&power RK3399_PD_VIO>;
> > > +        #phy-cells = <0>;
> > > +    };
Heiko Stuebner Jan. 7, 2020, 9:28 a.m. UTC | #5
Am Dienstag, 7. Januar 2020, 03:37:21 CET schrieb Laurent Pinchart:
> On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> > On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > > Hi Helen,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > > 
> > > > This was tested and verified with:
> > > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml  Documentation/devicetree/bindings/phy/
> > > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > 
> > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > 
> > > > ---
> > > > 
> > > > Changes in v12:
> > > > - The commit replaces the following commit in previous series named
> > > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > > This new patch adds yaml binding and was verified with
> > > > make dtbs_check and make dt_binding_check
> > > > 
> > > > Changes in v11: None
> > > > Changes in v10:
> > > > - unsquash
> > > > 
> > > > Changes in v9:
> > > > - fix title division style
> > > > - squash
> > > > - move to staging
> > > > 
> > > > Changes in v8: None
> > > > Changes in v7:
> > > > - updated doc with new design and tested example
> > > > 
> > > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > > >  1 file changed, 75 insertions(+)
> > > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > 
> > > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml b/drivers/staging/media/phy-
> > > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > new file mode 100644
> > > > index 000000000000..af97f1b3e005
> > > > --- /dev/null
> > > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > @@ -0,0 +1,75 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > > 
> > > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> > 
> > The driver currently only supports RX0, but I think you are right,
> > it should say RX here. This binding could be extended for RX1.
> > 
> > > Looking at the PHY driver, it seems to handle all PHYs with a single
> > > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> > 
> > I am not following this. The driver handles just one PHY. Each PHY
> > should have its own node.
> 
> Looking at the registers, it seems that the different PHYs are
> intertwined and we would could have trouble handling the different PHYs
> with different DT nodes and thus struct device instances.

I have to confess to not following _ALL_ of the threads, so may say
something stupid, but I don't think the PHYs are intertwined so much.

Where RX0 is controlled from the "General Register Files" alone
[register dumping ground for soc designers], the TX1RX1-phy
actually gets controlled from inside the dsi1 register area it seems.

So in my previous (still unsucessful) tests, I was rolling with something like
https://github.com/mmind/linux-rockchip/commit/e0d4b03976d2aab85a8c1630be937ea003b5df88

With the actual "logic" picked from the vendor kernel, that just double-
maps the dsi1-registers in both dsi and dphy driver, which was strange.


Heiko
Ezequiel Garcia Jan. 7, 2020, 1:20 p.m. UTC | #6
Hi Heiko, Laurent,

On Tue, 2020-01-07 at 10:28 +0100, Heiko Stübner wrote:
> Am Dienstag, 7. Januar 2020, 03:37:21 CET schrieb Laurent Pinchart:
> > On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> > > On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > > > Hi Helen,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > > > 
> > > > > This was tested and verified with:
> > > > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-
> > > > > dphy.yaml  Documentation/devicetree/bindings/phy/
> > > > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > 
> > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Changes in v12:
> > > > > - The commit replaces the following commit in previous series named
> > > > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > > > This new patch adds yaml binding and was verified with
> > > > > make dtbs_check and make dt_binding_check
> > > > > 
> > > > > Changes in v11: None
> > > > > Changes in v10:
> > > > > - unsquash
> > > > > 
> > > > > Changes in v9:
> > > > > - fix title division style
> > > > > - squash
> > > > > - move to staging
> > > > > 
> > > > > Changes in v8: None
> > > > > Changes in v7:
> > > > > - updated doc with new design and tested example
> > > > > 
> > > > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > > > >  1 file changed, 75 insertions(+)
> > > > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > 
> > > > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > b/drivers/staging/media/phy-
> > > > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..af97f1b3e005
> > > > > --- /dev/null
> > > > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > @@ -0,0 +1,75 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > > > 
> > > > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> > > 
> > > The driver currently only supports RX0, but I think you are right,
> > > it should say RX here. This binding could be extended for RX1.
> > > 
> > > > Looking at the PHY driver, it seems to handle all PHYs with a single
> > > > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> > > 
> > > I am not following this. The driver handles just one PHY. Each PHY
> > > should have its own node.
> > 
> > Looking at the registers, it seems that the different PHYs are
> > intertwined and we would could have trouble handling the different PHYs
> > with different DT nodes and thus struct device instances.
> 
> I have to confess to not following _ALL_ of the threads, so may say
> something stupid, but I don't think the PHYs are intertwined so much.
> 
> Where RX0 is controlled from the "General Register Files" alone
> [register dumping ground for soc designers], the TX1RX1-phy
> actually gets controlled from inside the dsi1 register area it seems.
> 
> So in my previous (still unsucessful) tests, I was rolling with something like
> https://github.com/mmind/linux-rockchip/commit/e0d4b03976d2aab85a8c1630be937ea003b5df88
> 
> With the actual "logic" picked from the vendor kernel, that just double-
> maps the dsi1-registers in both dsi and dphy driver, which was strange.
> 
> 

Describing each PHY in its own device node (as we currently do)
results in:

        mipi_dphy_tx1rx1: mipi-dphy-tx1rx1@ff968000 {
                compatible = "rockchip,rk3399-mipi-dphy";
                reg = <0x0 0xff968000 0x0 0x8000>;
                rockchip,grf = <&grf>;
        };

        grf: syscon@ff770000 {
                mipi_dphy_rx0: mipi-dphy-rx0 {
                        compatible = "rockchip,rk3399-mipi-dphy";
                };
        };

Which is mildly ugly, as it uses two mechanism to describe
the GRF resource. In addition, the driver will then _infer_
which device node is RX0 and which is TX1RX1, from this.

Perhaps Laurent's proposal, describing each PHY explicitly,
would be cleaner?

Thanks,
Ezequiel
Heiko Stuebner Jan. 7, 2020, 9:30 p.m. UTC | #7
Hi Ezequiel,

Am Dienstag, 7. Januar 2020, 14:20:10 CET schrieb Ezequiel Garcia:
> Hi Heiko, Laurent,
> 
> On Tue, 2020-01-07 at 10:28 +0100, Heiko Stübner wrote:
> > Am Dienstag, 7. Januar 2020, 03:37:21 CET schrieb Laurent Pinchart:
> > > On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> > > > On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > > > > Hi Helen,
> > > > > 
> > > > > Thank you for the patch.
> > > > > 
> > > > > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > > > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > > > > 
> > > > > > This was tested and verified with:
> > > > > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-
> > > > > > dphy.yaml  Documentation/devicetree/bindings/phy/
> > > > > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > 
> > > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > 
> > > > > > ---
> > > > > > 
> > > > > > Changes in v12:
> > > > > > - The commit replaces the following commit in previous series named
> > > > > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > > > > This new patch adds yaml binding and was verified with
> > > > > > make dtbs_check and make dt_binding_check
> > > > > > 
> > > > > > Changes in v11: None
> > > > > > Changes in v10:
> > > > > > - unsquash
> > > > > > 
> > > > > > Changes in v9:
> > > > > > - fix title division style
> > > > > > - squash
> > > > > > - move to staging
> > > > > > 
> > > > > > Changes in v8: None
> > > > > > Changes in v7:
> > > > > > - updated doc with new design and tested example
> > > > > > 
> > > > > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > > > > >  1 file changed, 75 insertions(+)
> > > > > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > 
> > > > > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > b/drivers/staging/media/phy-
> > > > > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > new file mode 100644
> > > > > > index 000000000000..af97f1b3e005
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > @@ -0,0 +1,75 @@
> > > > > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > > +%YAML 1.2
> > > > > > +---
> > > > > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > +
> > > > > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > > > > 
> > > > > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> > > > 
> > > > The driver currently only supports RX0, but I think you are right,
> > > > it should say RX here. This binding could be extended for RX1.
> > > > 
> > > > > Looking at the PHY driver, it seems to handle all PHYs with a single
> > > > > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> > > > 
> > > > I am not following this. The driver handles just one PHY. Each PHY
> > > > should have its own node.
> > > 
> > > Looking at the registers, it seems that the different PHYs are
> > > intertwined and we would could have trouble handling the different PHYs
> > > with different DT nodes and thus struct device instances.
> > 
> > I have to confess to not following _ALL_ of the threads, so may say
> > something stupid, but I don't think the PHYs are intertwined so much.
> > 
> > Where RX0 is controlled from the "General Register Files" alone
> > [register dumping ground for soc designers], the TX1RX1-phy
> > actually gets controlled from inside the dsi1 register area it seems.
> > 
> > So in my previous (still unsucessful) tests, I was rolling with something like
> > https://github.com/mmind/linux-rockchip/commit/e0d4b03976d2aab85a8c1630be937ea003b5df88
> > 
> > With the actual "logic" picked from the vendor kernel, that just double-
> > maps the dsi1-registers in both dsi and dphy driver, which was strange.
> > 
> > 
> 
> Describing each PHY in its own device node (as we currently do)
> results in:
> 
>         mipi_dphy_tx1rx1: mipi-dphy-tx1rx1@ff968000 {
>                 compatible = "rockchip,rk3399-mipi-dphy";
>                 reg = <0x0 0xff968000 0x0 0x8000>;
>                 rockchip,grf = <&grf>;
>         };

0xff968000 actually really is the dsi1 controller, so we'll already
have a node for that area. That is the reason I went that way to make
the rockchip-dsi optionally also behave as phy-provider.

So when it's used in combination with drm and a panel or so it will
behave as dsi controller, but when requested via the phy-framework
it will expose the dphy functionality.


>         grf: syscon@ff770000 {
>                 mipi_dphy_rx0: mipi-dphy-rx0 {
>                         compatible = "rockchip,rk3399-mipi-dphy";
>                 };
>         };
> 
> Which is mildly ugly, as it uses two mechanism to describe
> the GRF resource. In addition, the driver will then _infer_
> which device node is RX0 and which is TX1RX1, from this.
> 
> Perhaps Laurent's proposal, describing each PHY explicitly,
> would be cleaner?

so I really think we shouldn't merge these two things together,
especially to not break the dsi1 controller part.


Heiko
Laurent Pinchart Jan. 7, 2020, 9:57 p.m. UTC | #8
Hi Heiko,

On Tue, Jan 07, 2020 at 10:30:28PM +0100, Heiko Stübner wrote:
> Am Dienstag, 7. Januar 2020, 14:20:10 CET schrieb Ezequiel Garcia:
> > On Tue, 2020-01-07 at 10:28 +0100, Heiko Stübner wrote:
> > > Am Dienstag, 7. Januar 2020, 03:37:21 CET schrieb Laurent Pinchart:
> > > > On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> > > > > On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > > > > > Hi Helen,
> > > > > > 
> > > > > > Thank you for the patch.
> > > > > > 
> > > > > > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > > > > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > > > > > 
> > > > > > > This was tested and verified with:
> > > > > > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-
> > > > > > > dphy.yaml  Documentation/devicetree/bindings/phy/
> > > > > > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > 
> > > > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > > 
> > > > > > > ---
> > > > > > > 
> > > > > > > Changes in v12:
> > > > > > > - The commit replaces the following commit in previous series named
> > > > > > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > > > > > This new patch adds yaml binding and was verified with
> > > > > > > make dtbs_check and make dt_binding_check
> > > > > > > 
> > > > > > > Changes in v11: None
> > > > > > > Changes in v10:
> > > > > > > - unsquash
> > > > > > > 
> > > > > > > Changes in v9:
> > > > > > > - fix title division style
> > > > > > > - squash
> > > > > > > - move to staging
> > > > > > > 
> > > > > > > Changes in v8: None
> > > > > > > Changes in v7:
> > > > > > > - updated doc with new design and tested example
> > > > > > > 
> > > > > > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > > > > > >  1 file changed, 75 insertions(+)
> > > > > > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > 
> > > > > > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > b/drivers/staging/media/phy-
> > > > > > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..af97f1b3e005
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > @@ -0,0 +1,75 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > > > > > 
> > > > > > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> > > > > 
> > > > > The driver currently only supports RX0, but I think you are right,
> > > > > it should say RX here. This binding could be extended for RX1.
> > > > > 
> > > > > > Looking at the PHY driver, it seems to handle all PHYs with a single
> > > > > > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> > > > > 
> > > > > I am not following this. The driver handles just one PHY. Each PHY
> > > > > should have its own node.
> > > > 
> > > > Looking at the registers, it seems that the different PHYs are
> > > > intertwined and we would could have trouble handling the different PHYs
> > > > with different DT nodes and thus struct device instances.
> > > 
> > > I have to confess to not following _ALL_ of the threads, so may say
> > > something stupid, but I don't think the PHYs are intertwined so much.
> > > 
> > > Where RX0 is controlled from the "General Register Files" alone
> > > [register dumping ground for soc designers], the TX1RX1-phy
> > > actually gets controlled from inside the dsi1 register area it seems.
> > > 
> > > So in my previous (still unsucessful) tests, I was rolling with something like
> > > https://github.com/mmind/linux-rockchip/commit/e0d4b03976d2aab85a8c1630be937ea003b5df88
> > > 
> > > With the actual "logic" picked from the vendor kernel, that just double-
> > > maps the dsi1-registers in both dsi and dphy driver, which was strange.
> > 
> > Describing each PHY in its own device node (as we currently do)
> > results in:
> > 
> >         mipi_dphy_tx1rx1: mipi-dphy-tx1rx1@ff968000 {
> >                 compatible = "rockchip,rk3399-mipi-dphy";
> >                 reg = <0x0 0xff968000 0x0 0x8000>;
> >                 rockchip,grf = <&grf>;
> >         };
> 
> 0xff968000 actually really is the dsi1 controller, so we'll already
> have a node for that area. That is the reason I went that way to make
> the rockchip-dsi optionally also behave as phy-provider.
> 
> So when it's used in combination with drm and a panel or so it will
> behave as dsi controller, but when requested via the phy-framework
> it will expose the dphy functionality.

Doesn't RX1/TX1 also expose controls through GRF ? For instance
GRF_SOC_CON9 has a dphy_rx1_clk_inv_sel bit.

> >         grf: syscon@ff770000 {
> >                 mipi_dphy_rx0: mipi-dphy-rx0 {
> >                         compatible = "rockchip,rk3399-mipi-dphy";
> >                 };
> >         };
> > 
> > Which is mildly ugly, as it uses two mechanism to describe
> > the GRF resource. In addition, the driver will then _infer_
> > which device node is RX0 and which is TX1RX1, from this.
> > 
> > Perhaps Laurent's proposal, describing each PHY explicitly,
> > would be cleaner?
> 
> so I really think we shouldn't merge these two things together,
> especially to not break the dsi1 controller part.
Ezequiel Garcia Jan. 7, 2020, 10:03 p.m. UTC | #9
On Tue, 2020-01-07 at 22:30 +0100, Heiko Stübner wrote:
> Hi Ezequiel,
> 
> Am Dienstag, 7. Januar 2020, 14:20:10 CET schrieb Ezequiel Garcia:
> > Hi Heiko, Laurent,
> > 
> > On Tue, 2020-01-07 at 10:28 +0100, Heiko Stübner wrote:
> > > Am Dienstag, 7. Januar 2020, 03:37:21 CET schrieb Laurent Pinchart:
> > > > On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> > > > > On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > > > > > Hi Helen,
> > > > > > 
> > > > > > Thank you for the patch.
> > > > > > 
> > > > > > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > > > > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > > > > > 
> > > > > > > This was tested and verified with:
> > > > > > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-
> > > > > > > dphy.yaml  Documentation/devicetree/bindings/phy/
> > > > > > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > 
> > > > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > > 
> > > > > > > ---
> > > > > > > 
> > > > > > > Changes in v12:
> > > > > > > - The commit replaces the following commit in previous series named
> > > > > > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > > > > > This new patch adds yaml binding and was verified with
> > > > > > > make dtbs_check and make dt_binding_check
> > > > > > > 
> > > > > > > Changes in v11: None
> > > > > > > Changes in v10:
> > > > > > > - unsquash
> > > > > > > 
> > > > > > > Changes in v9:
> > > > > > > - fix title division style
> > > > > > > - squash
> > > > > > > - move to staging
> > > > > > > 
> > > > > > > Changes in v8: None
> > > > > > > Changes in v7:
> > > > > > > - updated doc with new design and tested example
> > > > > > > 
> > > > > > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > > > > > >  1 file changed, 75 insertions(+)
> > > > > > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > 
> > > > > > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > b/drivers/staging/media/phy-
> > > > > > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..af97f1b3e005
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > @@ -0,0 +1,75 @@
> > > > > > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > > > +%YAML 1.2
> > > > > > > +---
> > > > > > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > +
> > > > > > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > > > > > 
> > > > > > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> > > > > 
> > > > > The driver currently only supports RX0, but I think you are right,
> > > > > it should say RX here. This binding could be extended for RX1.
> > > > > 
> > > > > > Looking at the PHY driver, it seems to handle all PHYs with a single
> > > > > > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> > > > > 
> > > > > I am not following this. The driver handles just one PHY. Each PHY
> > > > > should have its own node.
> > > > 
> > > > Looking at the registers, it seems that the different PHYs are
> > > > intertwined and we would could have trouble handling the different PHYs
> > > > with different DT nodes and thus struct device instances.
> > > 
> > > I have to confess to not following _ALL_ of the threads, so may say
> > > something stupid, but I don't think the PHYs are intertwined so much.
> > > 
> > > Where RX0 is controlled from the "General Register Files" alone
> > > [register dumping ground for soc designers], the TX1RX1-phy
> > > actually gets controlled from inside the dsi1 register area it seems.
> > > 
> > > So in my previous (still unsucessful) tests, I was rolling with something like
> > > https://github.com/mmind/linux-rockchip/commit/e0d4b03976d2aab85a8c1630be937ea003b5df88
> > > 
> > > With the actual "logic" picked from the vendor kernel, that just double-
> > > maps the dsi1-registers in both dsi and dphy driver, which was strange.
> > > 
> > > 
> > 
> > Describing each PHY in its own device node (as we currently do)
> > results in:
> > 
> >         mipi_dphy_tx1rx1: mipi-dphy-tx1rx1@ff968000 {
> >                 compatible = "rockchip,rk3399-mipi-dphy";
> >                 reg = <0x0 0xff968000 0x0 0x8000>;
> >                 rockchip,grf = <&grf>;
> >         };
> 
> 0xff968000 actually really is the dsi1 controller, so we'll already
> have a node for that area. That is the reason I went that way to make
> the rockchip-dsi optionally also behave as phy-provider.
> 
> So when it's used in combination with drm and a panel or so it will
> behave as dsi controller, but when requested via the phy-framework
> it will expose the dphy functionality.
> 

Hm, and will this driver also support RX1?

> 
> >         grf: syscon@ff770000 {
> >                 mipi_dphy_rx0: mipi-dphy-rx0 {
> >                         compatible = "rockchip,rk3399-mipi-dphy";
> >                 };
> >         };
> > 
> > Which is mildly ugly, as it uses two mechanism to describe
> > the GRF resource. In addition, the driver will then _infer_
> > which device node is RX0 and which is TX1RX1, from this.
> > 
> > Perhaps Laurent's proposal, describing each PHY explicitly,
> > would be cleaner?
> 
> so I really think we shouldn't merge these two things together,
> especially to not break the dsi1 controller part.
> 

I don't think it would necesarily break the dsi1 controller part.

You can declare both device nodes as sharing the address region,
and then the driver can request the I/O resource only when it needs to,
i.e. in the PHY .init hook.

It's not super nice, but there's no real reason two devices
can't share an I/O memory resource.

I like this approach because it exposes the different PHYs
explicitly, instead of implicitly.

Thanks,
Ezequiel
Heiko Stuebner Jan. 7, 2020, 10:12 p.m. UTC | #10
Am Dienstag, 7. Januar 2020, 22:57:39 CET schrieb Laurent Pinchart:
> Hi Heiko,
> 
> On Tue, Jan 07, 2020 at 10:30:28PM +0100, Heiko Stübner wrote:
> > Am Dienstag, 7. Januar 2020, 14:20:10 CET schrieb Ezequiel Garcia:
> > > On Tue, 2020-01-07 at 10:28 +0100, Heiko Stübner wrote:
> > > > Am Dienstag, 7. Januar 2020, 03:37:21 CET schrieb Laurent Pinchart:
> > > > > On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> > > > > > On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > > > > > > Hi Helen,
> > > > > > > 
> > > > > > > Thank you for the patch.
> > > > > > > 
> > > > > > > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > > > > > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > > > > > > 
> > > > > > > > This was tested and verified with:
> > > > > > > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-
> > > > > > > > dphy.yaml  Documentation/devicetree/bindings/phy/
> > > > > > > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > 
> > > > > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > > > 
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Changes in v12:
> > > > > > > > - The commit replaces the following commit in previous series named
> > > > > > > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > > > > > > This new patch adds yaml binding and was verified with
> > > > > > > > make dtbs_check and make dt_binding_check
> > > > > > > > 
> > > > > > > > Changes in v11: None
> > > > > > > > Changes in v10:
> > > > > > > > - unsquash
> > > > > > > > 
> > > > > > > > Changes in v9:
> > > > > > > > - fix title division style
> > > > > > > > - squash
> > > > > > > > - move to staging
> > > > > > > > 
> > > > > > > > Changes in v8: None
> > > > > > > > Changes in v7:
> > > > > > > > - updated doc with new design and tested example
> > > > > > > > 
> > > > > > > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > > > > > > >  1 file changed, 75 insertions(+)
> > > > > > > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > 
> > > > > > > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > b/drivers/staging/media/phy-
> > > > > > > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..af97f1b3e005
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > @@ -0,0 +1,75 @@
> > > > > > > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > > > > +%YAML 1.2
> > > > > > > > +---
> > > > > > > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > +
> > > > > > > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > > > > > > 
> > > > > > > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> > > > > > 
> > > > > > The driver currently only supports RX0, but I think you are right,
> > > > > > it should say RX here. This binding could be extended for RX1.
> > > > > > 
> > > > > > > Looking at the PHY driver, it seems to handle all PHYs with a single
> > > > > > > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> > > > > > 
> > > > > > I am not following this. The driver handles just one PHY. Each PHY
> > > > > > should have its own node.
> > > > > 
> > > > > Looking at the registers, it seems that the different PHYs are
> > > > > intertwined and we would could have trouble handling the different PHYs
> > > > > with different DT nodes and thus struct device instances.
> > > > 
> > > > I have to confess to not following _ALL_ of the threads, so may say
> > > > something stupid, but I don't think the PHYs are intertwined so much.
> > > > 
> > > > Where RX0 is controlled from the "General Register Files" alone
> > > > [register dumping ground for soc designers], the TX1RX1-phy
> > > > actually gets controlled from inside the dsi1 register area it seems.
> > > > 
> > > > So in my previous (still unsucessful) tests, I was rolling with something like
> > > > https://github.com/mmind/linux-rockchip/commit/e0d4b03976d2aab85a8c1630be937ea003b5df88
> > > > 
> > > > With the actual "logic" picked from the vendor kernel, that just double-
> > > > maps the dsi1-registers in both dsi and dphy driver, which was strange.
> > > 
> > > Describing each PHY in its own device node (as we currently do)
> > > results in:
> > > 
> > >         mipi_dphy_tx1rx1: mipi-dphy-tx1rx1@ff968000 {
> > >                 compatible = "rockchip,rk3399-mipi-dphy";
> > >                 reg = <0x0 0xff968000 0x0 0x8000>;
> > >                 rockchip,grf = <&grf>;
> > >         };
> > 
> > 0xff968000 actually really is the dsi1 controller, so we'll already
> > have a node for that area. That is the reason I went that way to make
> > the rockchip-dsi optionally also behave as phy-provider.
> > 
> > So when it's used in combination with drm and a panel or so it will
> > behave as dsi controller, but when requested via the phy-framework
> > it will expose the dphy functionality.
> 
> Doesn't RX1/TX1 also expose controls through GRF ? For instance
> GRF_SOC_CON9 has a dphy_rx1_clk_inv_sel bit.

There are parts in the GRF but the whole phy-write stuff is inside
the dsi controller area. See the vendor kernel at
	https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/phy/rockchip/phy-rockchip-mipi-rx.c#L1427

where you get write_grf_reg() for GRF bits but also the write_txrx_reg()
and mipidphy1_wr_reg() calls that access registers inside the dsi1
controller space.


Heiko

> 
> > >         grf: syscon@ff770000 {
> > >                 mipi_dphy_rx0: mipi-dphy-rx0 {
> > >                         compatible = "rockchip,rk3399-mipi-dphy";
> > >                 };
> > >         };
> > > 
> > > Which is mildly ugly, as it uses two mechanism to describe
> > > the GRF resource. In addition, the driver will then _infer_
> > > which device node is RX0 and which is TX1RX1, from this.
> > > 
> > > Perhaps Laurent's proposal, describing each PHY explicitly,
> > > would be cleaner?
> > 
> > so I really think we shouldn't merge these two things together,
> > especially to not break the dsi1 controller part.
> 
>
Heiko Stuebner Jan. 7, 2020, 10:25 p.m. UTC | #11
Am Dienstag, 7. Januar 2020, 23:03:54 CET schrieb Ezequiel Garcia:
> On Tue, 2020-01-07 at 22:30 +0100, Heiko Stübner wrote:
> > Hi Ezequiel,
> > 
> > Am Dienstag, 7. Januar 2020, 14:20:10 CET schrieb Ezequiel Garcia:
> > > Hi Heiko, Laurent,
> > > 
> > > On Tue, 2020-01-07 at 10:28 +0100, Heiko Stübner wrote:
> > > > Am Dienstag, 7. Januar 2020, 03:37:21 CET schrieb Laurent Pinchart:
> > > > > On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> > > > > > On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > > > > > > Hi Helen,
> > > > > > > 
> > > > > > > Thank you for the patch.
> > > > > > > 
> > > > > > > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > > > > > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > > > > > > 
> > > > > > > > This was tested and verified with:
> > > > > > > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-
> > > > > > > > dphy.yaml  Documentation/devicetree/bindings/phy/
> > > > > > > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > 
> > > > > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > > > 
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Changes in v12:
> > > > > > > > - The commit replaces the following commit in previous series named
> > > > > > > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > > > > > > This new patch adds yaml binding and was verified with
> > > > > > > > make dtbs_check and make dt_binding_check
> > > > > > > > 
> > > > > > > > Changes in v11: None
> > > > > > > > Changes in v10:
> > > > > > > > - unsquash
> > > > > > > > 
> > > > > > > > Changes in v9:
> > > > > > > > - fix title division style
> > > > > > > > - squash
> > > > > > > > - move to staging
> > > > > > > > 
> > > > > > > > Changes in v8: None
> > > > > > > > Changes in v7:
> > > > > > > > - updated doc with new design and tested example
> > > > > > > > 
> > > > > > > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > > > > > > >  1 file changed, 75 insertions(+)
> > > > > > > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > 
> > > > > > > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > b/drivers/staging/media/phy-
> > > > > > > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..af97f1b3e005
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > @@ -0,0 +1,75 @@
> > > > > > > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > > > > +%YAML 1.2
> > > > > > > > +---
> > > > > > > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > +
> > > > > > > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > > > > > > 
> > > > > > > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> > > > > > 
> > > > > > The driver currently only supports RX0, but I think you are right,
> > > > > > it should say RX here. This binding could be extended for RX1.
> > > > > > 
> > > > > > > Looking at the PHY driver, it seems to handle all PHYs with a single
> > > > > > > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> > > > > > 
> > > > > > I am not following this. The driver handles just one PHY. Each PHY
> > > > > > should have its own node.
> > > > > 
> > > > > Looking at the registers, it seems that the different PHYs are
> > > > > intertwined and we would could have trouble handling the different PHYs
> > > > > with different DT nodes and thus struct device instances.
> > > > 
> > > > I have to confess to not following _ALL_ of the threads, so may say
> > > > something stupid, but I don't think the PHYs are intertwined so much.
> > > > 
> > > > Where RX0 is controlled from the "General Register Files" alone
> > > > [register dumping ground for soc designers], the TX1RX1-phy
> > > > actually gets controlled from inside the dsi1 register area it seems.
> > > > 
> > > > So in my previous (still unsucessful) tests, I was rolling with something like
> > > > https://github.com/mmind/linux-rockchip/commit/e0d4b03976d2aab85a8c1630be937ea003b5df88
> > > > 
> > > > With the actual "logic" picked from the vendor kernel, that just double-
> > > > maps the dsi1-registers in both dsi and dphy driver, which was strange.
> > > > 
> > > > 
> > > 
> > > Describing each PHY in its own device node (as we currently do)
> > > results in:
> > > 
> > >         mipi_dphy_tx1rx1: mipi-dphy-tx1rx1@ff968000 {
> > >                 compatible = "rockchip,rk3399-mipi-dphy";
> > >                 reg = <0x0 0xff968000 0x0 0x8000>;
> > >                 rockchip,grf = <&grf>;
> > >         };
> > 
> > 0xff968000 actually really is the dsi1 controller, so we'll already
> > have a node for that area. That is the reason I went that way to make
> > the rockchip-dsi optionally also behave as phy-provider.
> > 
> > So when it's used in combination with drm and a panel or so it will
> > behave as dsi controller, but when requested via the phy-framework
> > it will expose the dphy functionality.
> > 
> 
> Hm, and will this driver also support RX1?

what is RX1 in your book? :-)

According to the TRM the rk3399 has 3 DPHYs,
tx0 - connected exclusively to dsi0
      (this is handled internally by the dw-mipi-dsi driver with controls
       in the dsi0 register space)
rx0 - connected exclusively to isp0
      (this is handled by the individual dphy driver from Helen's series)
tx1rx1 - shared between dsi1 and isp1
      (again inside the dsi1 register space)


> 
> > 
> > >         grf: syscon@ff770000 {
> > >                 mipi_dphy_rx0: mipi-dphy-rx0 {
> > >                         compatible = "rockchip,rk3399-mipi-dphy";
> > >                 };
> > >         };
> > > 
> > > Which is mildly ugly, as it uses two mechanism to describe
> > > the GRF resource. In addition, the driver will then _infer_
> > > which device node is RX0 and which is TX1RX1, from this.
> > > 
> > > Perhaps Laurent's proposal, describing each PHY explicitly,
> > > would be cleaner?
> > 
> > so I really think we shouldn't merge these two things together,
> > especially to not break the dsi1 controller part.
> > 
> 
> I don't think it would necesarily break the dsi1 controller part.
> 
> You can declare both device nodes as sharing the address region,
> and then the driver can request the I/O resource only when it needs to,
> i.e. in the PHY .init hook.

dsi1 is of course a dw-mipi-dsi one, which in turn shares a common bridge
driver over multiple variants (non-rockchip), which expects its registers
mapped during probe.

I think it would not really work well if you need to make the whole world
follow that idea ;-) .


Hence my approach with exposing the phy interface from the dsi driver.
If you look at the dts part, it also just looks like it should be ... as
a regular phy:
	https://github.com/mmind/linux-rockchip/blob/wip/tc358749/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L1764

And on the driver side there is even some short circuit protection.
When used as phy, it won't allow to be used as a component and
vice versa.


> It's not super nice, but there's no real reason two devices
> can't share an I/O memory resource.

Counter argument, devicetree is not a means to handle Linux
peculiarites - instead it should describe the hardware ...
and the area there _is_ the dsi1 controller ;-)

Heiko
Ezequiel Garcia Jan. 7, 2020, 10:41 p.m. UTC | #12
On Tue, 2020-01-07 at 23:25 +0100, Heiko Stuebner wrote:
> Am Dienstag, 7. Januar 2020, 23:03:54 CET schrieb Ezequiel Garcia:
> > On Tue, 2020-01-07 at 22:30 +0100, Heiko Stübner wrote:
> > > Hi Ezequiel,
> > > 
> > > Am Dienstag, 7. Januar 2020, 14:20:10 CET schrieb Ezequiel Garcia:
> > > > Hi Heiko, Laurent,
> > > > 
> > > > On Tue, 2020-01-07 at 10:28 +0100, Heiko Stübner wrote:
> > > > > Am Dienstag, 7. Januar 2020, 03:37:21 CET schrieb Laurent Pinchart:
> > > > > > On Mon, Jan 06, 2020 at 11:06:12PM -0300, Ezequiel Garcia wrote:
> > > > > > > On Tue, 2020-01-07 at 02:10 +0200, Laurent Pinchart wrote:
> > > > > > > > Hi Helen,
> > > > > > > > 
> > > > > > > > Thank you for the patch.
> > > > > > > > 
> > > > > > > > On Fri, Dec 27, 2019 at 05:01:14PM -0300, Helen Koike wrote:
> > > > > > > > > Add yaml DT bindings for Rockchip MIPI D-PHY RX
> > > > > > > > > 
> > > > > > > > > This was tested and verified with:
> > > > > > > > > mv drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-
> > > > > > > > > dphy.yaml  Documentation/devicetree/bindings/phy/
> > > > > > > > > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > > make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Helen Koike <helen.koike@collabora.com>
> > > > > > > > > 
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > Changes in v12:
> > > > > > > > > - The commit replaces the following commit in previous series named
> > > > > > > > > media: staging: dt-bindings: Document the Rockchip MIPI RX D-PHY bindings
> > > > > > > > > This new patch adds yaml binding and was verified with
> > > > > > > > > make dtbs_check and make dt_binding_check
> > > > > > > > > 
> > > > > > > > > Changes in v11: None
> > > > > > > > > Changes in v10:
> > > > > > > > > - unsquash
> > > > > > > > > 
> > > > > > > > > Changes in v9:
> > > > > > > > > - fix title division style
> > > > > > > > > - squash
> > > > > > > > > - move to staging
> > > > > > > > > 
> > > > > > > > > Changes in v8: None
> > > > > > > > > Changes in v7:
> > > > > > > > > - updated doc with new design and tested example
> > > > > > > > > 
> > > > > > > > >  .../bindings/phy/rockchip-mipi-dphy.yaml      | 75 +++++++++++++++++++
> > > > > > > > >  1 file changed, 75 insertions(+)
> > > > > > > > >  create mode 100644 drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > > b/drivers/staging/media/phy-
> > > > > > > > > rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > > new file mode 100644
> > > > > > > > > index 000000000000..af97f1b3e005
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
> > > > > > > > > @@ -0,0 +1,75 @@
> > > > > > > > > +# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > > > > > +%YAML 1.2
> > > > > > > > > +---
> > > > > > > > > +$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
> > > > > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > > > > > +
> > > > > > > > > +title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
> > > > > > > > 
> > > > > > > > Should this be s/RX0/RX/ ? Or do you expect different bindings for RX1 ?
> > > > > > > 
> > > > > > > The driver currently only supports RX0, but I think you are right,
> > > > > > > it should say RX here. This binding could be extended for RX1.
> > > > > > > 
> > > > > > > > Looking at the PHY driver, it seems to handle all PHYs with a single
> > > > > > > > struct device. Should we thus use #phy-cells = <1> to select the PHY ?
> > > > > > > 
> > > > > > > I am not following this. The driver handles just one PHY. Each PHY
> > > > > > > should have its own node.
> > > > > > 
> > > > > > Looking at the registers, it seems that the different PHYs are
> > > > > > intertwined and we would could have trouble handling the different PHYs
> > > > > > with different DT nodes and thus struct device instances.
> > > > > 
> > > > > I have to confess to not following _ALL_ of the threads, so may say
> > > > > something stupid, but I don't think the PHYs are intertwined so much.
> > > > > 
> > > > > Where RX0 is controlled from the "General Register Files" alone
> > > > > [register dumping ground for soc designers], the TX1RX1-phy
> > > > > actually gets controlled from inside the dsi1 register area it seems.
> > > > > 
> > > > > So in my previous (still unsucessful) tests, I was rolling with something like
> > > > > https://github.com/mmind/linux-rockchip/commit/e0d4b03976d2aab85a8c1630be937ea003b5df88
> > > > > 
> > > > > With the actual "logic" picked from the vendor kernel, that just double-
> > > > > maps the dsi1-registers in both dsi and dphy driver, which was strange.
> > > > > 
> > > > > 
> > > > 
> > > > Describing each PHY in its own device node (as we currently do)
> > > > results in:
> > > > 
> > > >         mipi_dphy_tx1rx1: mipi-dphy-tx1rx1@ff968000 {
> > > >                 compatible = "rockchip,rk3399-mipi-dphy";
> > > >                 reg = <0x0 0xff968000 0x0 0x8000>;
> > > >                 rockchip,grf = <&grf>;
> > > >         };
> > > 
> > > 0xff968000 actually really is the dsi1 controller, so we'll already
> > > have a node for that area. That is the reason I went that way to make
> > > the rockchip-dsi optionally also behave as phy-provider.
> > > 
> > > So when it's used in combination with drm and a panel or so it will
> > > behave as dsi controller, but when requested via the phy-framework
> > > it will expose the dphy functionality.
> > > 
> > 
> > Hm, and will this driver also support RX1?
> 
> what is RX1 in your book? :-)
> 

I meant tx1rx1 configured as a receiver,
sorry for the confusion!

> According to the TRM the rk3399 has 3 DPHYs,
> tx0 - connected exclusively to dsi0
>       (this is handled internally by the dw-mipi-dsi driver with controls
>        in the dsi0 register space)
> rx0 - connected exclusively to isp0
>       (this is handled by the individual dphy driver from Helen's series)
> tx1rx1 - shared between dsi1 and isp1
>       (again inside the dsi1 register space)
> 

Exactly.

> 
> > > >         grf: syscon@ff770000 {
> > > >                 mipi_dphy_rx0: mipi-dphy-rx0 {
> > > >                         compatible = "rockchip,rk3399-mipi-dphy";
> > > >                 };
> > > >         };
> > > > 
> > > > Which is mildly ugly, as it uses two mechanism to describe
> > > > the GRF resource. In addition, the driver will then _infer_
> > > > which device node is RX0 and which is TX1RX1, from this.
> > > > 
> > > > Perhaps Laurent's proposal, describing each PHY explicitly,
> > > > would be cleaner?
> > > 
> > > so I really think we shouldn't merge these two things together,
> > > especially to not break the dsi1 controller part.
> > > 
> > 
> > I don't think it would necesarily break the dsi1 controller part.
> > 
> > You can declare both device nodes as sharing the address region,
> > and then the driver can request the I/O resource only when it needs to,
> > i.e. in the PHY .init hook.
> 
> dsi1 is of course a dw-mipi-dsi one, which in turn shares a common bridge
> driver over multiple variants (non-rockchip), which expects its registers
> mapped during probe.
> 
> I think it would not really work well if you need to make the whole world
> follow that idea ;-) .
> 
> 
> Hence my approach with exposing the phy interface from the dsi driver.
> If you look at the dts part, it also just looks like it should be ... as
> a regular phy:
> 	https://github.com/mmind/linux-rockchip/blob/wip/tc358749/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L1764
> 
> And on the driver side there is even some short circuit protection.
> When used as phy, it won't allow to be used as a component and
> vice versa.
> 

OK, it's more clear now.

If this bindings will only ever support the RX0 PHY,
will not support anything else, then we don't need
to worry.

It can stay as a GRF syscon child, and will have
phy-cells = 0.

Thanks,
Ezequiel

Patch
diff mbox series

diff --git a/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
new file mode 100644
index 000000000000..af97f1b3e005
--- /dev/null
+++ b/drivers/staging/media/phy-rockchip-dphy/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy.yaml
@@ -0,0 +1,75 @@ 
+# SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/rockchip-mipi-dphy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip SoC MIPI RX0 D-PHY Device Tree Bindings
+
+maintainers:
+  - Helen Koike <helen.koike@collabora.com>
+  - Ezequiel Garcia <ezequiel@collabora.com>
+
+description: |
+  The Rockchip SoC has a MIPI D-PHY bus with an RX0 entry which connects to
+  the ISP1 (Image Signal Processing unit v1.0) for CSI cameras.
+
+properties:
+  compatible:
+    const: rockchip,rk3399-mipi-dphy
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Mipi d-phy ref clock
+      - description: Mipi d-phy rx0 cfg clock
+      - description: Video in/out general register file clock
+
+  clock-names:
+    items:
+      - const: dphy-ref
+      - const: dphy-cfg
+      - const: grf
+
+  '#phy-cells':
+    const: 0
+
+  power-domains:
+    description: Video in/out power domain.
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - clock-names
+  - '#phy-cells'
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  - |
+
+    /*
+     * MIPI RX D-PHY use registers in "general register files", it
+     * should be a child of the GRF.
+     *
+     * grf: syscon@ff770000 {
+     *  compatible = "rockchip,rk3399-grf", "syscon", "simple-mfd";
+     *  ...
+     */
+
+    #include <dt-bindings/clock/rk3399-cru.h>
+    #include <dt-bindings/power/rk3399-power.h>
+
+    dphy: mipi-dphy {
+        compatible = "rockchip,rk3399-mipi-dphy";
+        clocks = <&cru SCLK_MIPIDPHY_REF>,
+                 <&cru SCLK_DPHY_RX0_CFG>,
+                 <&cru PCLK_VIO_GRF>;
+        clock-names = "dphy-ref", "dphy-cfg", "grf";
+        power-domains = <&power RK3399_PD_VIO>;
+        #phy-cells = <0>;
+    };