[v10,5/6] arm64: dts: allwinner: a64: Add MIPI DSI pipeline
diff mbox series

Message ID 20191005141913.22020-6-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm/sun4i: Allwinner A64 MIPI-DSI support
Related show

Commit Message

Jagan Teki Oct. 5, 2019, 2:19 p.m. UTC
Add MIPI DSI pipeline for Allwinner A64.

- dsi node, with A64 compatible since it doesn't support
  DSI_SCLK gating unlike A33
- dphy node, with A64 compatible with A33 fallback since
  DPHY on A64 and A33 is similar
- finally, attach the dsi_in to tcon0 for complete MIPI DSI

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
Tested-by: Merlijn Wajer <merlijn@wizzup.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Maxime Ripard Oct. 7, 2019, 10:57 a.m. UTC | #1
On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> Add MIPI DSI pipeline for Allwinner A64.
>
> - dsi node, with A64 compatible since it doesn't support
>   DSI_SCLK gating unlike A33
> - dphy node, with A64 compatible with A33 fallback since
>   DPHY on A64 and A33 is similar
> - finally, attach the dsi_in to tcon0 for complete MIPI DSI
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 69128a6dfc46..ad4170b8aee0 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -382,6 +382,12 @@
>  					#address-cells = <1>;
>  					#size-cells = <0>;
>  					reg = <1>;
> +
> +					tcon0_out_dsi: endpoint@1 {
> +						reg = <1>;
> +						remote-endpoint = <&dsi_in_tcon0>;
> +						allwinner,tcon-channel = <1>;
> +					};
>  				};
>  			};
>  		};
> @@ -1003,6 +1009,38 @@
>  			status = "disabled";
>  		};
>
> +		dsi: dsi@1ca0000 {
> +			compatible = "allwinner,sun50i-a64-mipi-dsi";
> +			reg = <0x01ca0000 0x1000>;
> +			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ccu CLK_BUS_MIPI_DSI>;
> +			clock-names = "bus";

This won't validate with the bindings you have either here, since it
still expects bus and mod.

I guess in that cas, we can just drop clock-names, which will require
a bit of work on the driver side as well.

Maxime
Jagan Teki Oct. 14, 2019, 12:07 p.m. UTC | #2
On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > Add MIPI DSI pipeline for Allwinner A64.
> >
> > - dsi node, with A64 compatible since it doesn't support
> >   DSI_SCLK gating unlike A33
> > - dphy node, with A64 compatible with A33 fallback since
> >   DPHY on A64 and A33 is similar
> > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > ---
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > index 69128a6dfc46..ad4170b8aee0 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > @@ -382,6 +382,12 @@
> >                                       #address-cells = <1>;
> >                                       #size-cells = <0>;
> >                                       reg = <1>;
> > +
> > +                                     tcon0_out_dsi: endpoint@1 {
> > +                                             reg = <1>;
> > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > +                                             allwinner,tcon-channel = <1>;
> > +                                     };
> >                               };
> >                       };
> >               };
> > @@ -1003,6 +1009,38 @@
> >                       status = "disabled";
> >               };
> >
> > +             dsi: dsi@1ca0000 {
> > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > +                     reg = <0x01ca0000 0x1000>;
> > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > +                     clock-names = "bus";
>
> This won't validate with the bindings you have either here, since it
> still expects bus and mod.
>
> I guess in that cas, we can just drop clock-names, which will require
> a bit of work on the driver side as well.

Okay.
mod clock is not required for a64, ie reason we have has_mod_clk quirk
patch. Adjust the clock-names: on dt-bindings would make sense here,
what do you think?
Maxime Ripard Oct. 16, 2019, 8:03 a.m. UTC | #3
On Mon, Oct 14, 2019 at 05:37:50PM +0530, Jagan Teki wrote:
> On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > > Add MIPI DSI pipeline for Allwinner A64.
> > >
> > > - dsi node, with A64 compatible since it doesn't support
> > >   DSI_SCLK gating unlike A33
> > > - dphy node, with A64 compatible with A33 fallback since
> > >   DPHY on A64 and A33 is similar
> > > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > > ---
> > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > index 69128a6dfc46..ad4170b8aee0 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > @@ -382,6 +382,12 @@
> > >                                       #address-cells = <1>;
> > >                                       #size-cells = <0>;
> > >                                       reg = <1>;
> > > +
> > > +                                     tcon0_out_dsi: endpoint@1 {
> > > +                                             reg = <1>;
> > > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > > +                                             allwinner,tcon-channel = <1>;
> > > +                                     };
> > >                               };
> > >                       };
> > >               };
> > > @@ -1003,6 +1009,38 @@
> > >                       status = "disabled";
> > >               };
> > >
> > > +             dsi: dsi@1ca0000 {
> > > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > > +                     reg = <0x01ca0000 0x1000>;
> > > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > > +                     clock-names = "bus";
> >
> > This won't validate with the bindings you have either here, since it
> > still expects bus and mod.
> >
> > I guess in that cas, we can just drop clock-names, which will require
> > a bit of work on the driver side as well.
>
> Okay.
> mod clock is not required for a64, ie reason we have has_mod_clk quirk
> patch. Adjust the clock-names: on dt-bindings would make sense here,
> what do you think?

I'm confused, what are you suggesting?

Maxime
Jagan Teki Oct. 16, 2019, 8:49 a.m. UTC | #4
On Wed, Oct 16, 2019 at 1:33 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Mon, Oct 14, 2019 at 05:37:50PM +0530, Jagan Teki wrote:
> > On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > > > Add MIPI DSI pipeline for Allwinner A64.
> > > >
> > > > - dsi node, with A64 compatible since it doesn't support
> > > >   DSI_SCLK gating unlike A33
> > > > - dphy node, with A64 compatible with A33 fallback since
> > > >   DPHY on A64 and A33 is similar
> > > > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > > > ---
> > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> > > >  1 file changed, 38 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > index 69128a6dfc46..ad4170b8aee0 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > @@ -382,6 +382,12 @@
> > > >                                       #address-cells = <1>;
> > > >                                       #size-cells = <0>;
> > > >                                       reg = <1>;
> > > > +
> > > > +                                     tcon0_out_dsi: endpoint@1 {
> > > > +                                             reg = <1>;
> > > > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > > > +                                             allwinner,tcon-channel = <1>;
> > > > +                                     };
> > > >                               };
> > > >                       };
> > > >               };
> > > > @@ -1003,6 +1009,38 @@
> > > >                       status = "disabled";
> > > >               };
> > > >
> > > > +             dsi: dsi@1ca0000 {
> > > > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > > > +                     reg = <0x01ca0000 0x1000>;
> > > > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > > > +                     clock-names = "bus";
> > >
> > > This won't validate with the bindings you have either here, since it
> > > still expects bus and mod.
> > >
> > > I guess in that cas, we can just drop clock-names, which will require
> > > a bit of work on the driver side as well.
> >
> > Okay.
> > mod clock is not required for a64, ie reason we have has_mod_clk quirk
> > patch. Adjust the clock-names: on dt-bindings would make sense here,
> > what do you think?
>
> I'm confused, what are you suggesting?

Sorry for the confusion.

The mod clock is not required for A64 and we have a patch for handling
mod clock using has_mod_clk quirk(on the series), indeed the mod clock
is available in A31 and not needed for A64. So, to satisfy this
requirement the clock-names on dt-bindings can update to make mod
clock-name is optional and bus clock is required.

I'm not exactly sure, this is correct but trying to understand if it
is possible or not? something like

   clocks:
      minItems: 1
      maxItems: 2
     items:
       - description: Bus Clock
       - description: Module Clock

   clock-names:
      minItems: 1
      maxItems: 2
     items:
       - const: bus
       - const: mod
Maxime Ripard Oct. 17, 2019, 9:52 a.m. UTC | #5
On Wed, Oct 16, 2019 at 02:19:44PM +0530, Jagan Teki wrote:
> On Wed, Oct 16, 2019 at 1:33 PM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Mon, Oct 14, 2019 at 05:37:50PM +0530, Jagan Teki wrote:
> > > On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > > > > Add MIPI DSI pipeline for Allwinner A64.
> > > > >
> > > > > - dsi node, with A64 compatible since it doesn't support
> > > > >   DSI_SCLK gating unlike A33
> > > > > - dphy node, with A64 compatible with A33 fallback since
> > > > >   DPHY on A64 and A33 is similar
> > > > > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > > > > ---
> > > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> > > > >  1 file changed, 38 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > index 69128a6dfc46..ad4170b8aee0 100644
> > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > @@ -382,6 +382,12 @@
> > > > >                                       #address-cells = <1>;
> > > > >                                       #size-cells = <0>;
> > > > >                                       reg = <1>;
> > > > > +
> > > > > +                                     tcon0_out_dsi: endpoint@1 {
> > > > > +                                             reg = <1>;
> > > > > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > > > > +                                             allwinner,tcon-channel = <1>;
> > > > > +                                     };
> > > > >                               };
> > > > >                       };
> > > > >               };
> > > > > @@ -1003,6 +1009,38 @@
> > > > >                       status = "disabled";
> > > > >               };
> > > > >
> > > > > +             dsi: dsi@1ca0000 {
> > > > > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > > > > +                     reg = <0x01ca0000 0x1000>;
> > > > > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > > > > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > > > > +                     clock-names = "bus";
> > > >
> > > > This won't validate with the bindings you have either here, since it
> > > > still expects bus and mod.
> > > >
> > > > I guess in that cas, we can just drop clock-names, which will require
> > > > a bit of work on the driver side as well.
> > >
> > > Okay.
> > > mod clock is not required for a64, ie reason we have has_mod_clk quirk
> > > patch. Adjust the clock-names: on dt-bindings would make sense here,
> > > what do you think?
> >
> > I'm confused, what are you suggesting?
>
> Sorry for the confusion.
>
> The mod clock is not required for A64 and we have a patch for handling
> mod clock using has_mod_clk quirk(on the series), indeed the mod clock
> is available in A31 and not needed for A64. So, to satisfy this
> requirement the clock-names on dt-bindings can update to make mod
> clock-name is optional and bus clock is required.

No, the bus clock name is not needed if there's only one clock.

> I'm not exactly sure, this is correct but trying to understand if it
> is possible or not? something like
>
>    clocks:
>       minItems: 1
>       maxItems: 2
>      items:
>        - description: Bus Clock
>        - description: Module Clock

That's correct.

>    clock-names:
>       minItems: 1
>       maxItems: 2
>      items:
>        - const: bus
>        - const: mod

Here, just keep the current clock-names definition, and make it
required only for SoCs that are not the A64

Maxime
Jagan Teki Oct. 24, 2019, 7:58 a.m. UTC | #6
On Thu, Oct 17, 2019 at 3:22 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Wed, Oct 16, 2019 at 02:19:44PM +0530, Jagan Teki wrote:
> > On Wed, Oct 16, 2019 at 1:33 PM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Mon, Oct 14, 2019 at 05:37:50PM +0530, Jagan Teki wrote:
> > > > On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > > > > > Add MIPI DSI pipeline for Allwinner A64.
> > > > > >
> > > > > > - dsi node, with A64 compatible since it doesn't support
> > > > > >   DSI_SCLK gating unlike A33
> > > > > > - dphy node, with A64 compatible with A33 fallback since
> > > > > >   DPHY on A64 and A33 is similar
> > > > > > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> > > > > >
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > > > > > ---
> > > > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> > > > > >  1 file changed, 38 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > index 69128a6dfc46..ad4170b8aee0 100644
> > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > @@ -382,6 +382,12 @@
> > > > > >                                       #address-cells = <1>;
> > > > > >                                       #size-cells = <0>;
> > > > > >                                       reg = <1>;
> > > > > > +
> > > > > > +                                     tcon0_out_dsi: endpoint@1 {
> > > > > > +                                             reg = <1>;
> > > > > > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > > > > > +                                             allwinner,tcon-channel = <1>;
> > > > > > +                                     };
> > > > > >                               };
> > > > > >                       };
> > > > > >               };
> > > > > > @@ -1003,6 +1009,38 @@
> > > > > >                       status = "disabled";
> > > > > >               };
> > > > > >
> > > > > > +             dsi: dsi@1ca0000 {
> > > > > > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > > > > > +                     reg = <0x01ca0000 0x1000>;
> > > > > > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > > > > > +                     clock-names = "bus";
> > > > >
> > > > > This won't validate with the bindings you have either here, since it
> > > > > still expects bus and mod.
> > > > >
> > > > > I guess in that cas, we can just drop clock-names, which will require
> > > > > a bit of work on the driver side as well.
> > > >
> > > > Okay.
> > > > mod clock is not required for a64, ie reason we have has_mod_clk quirk
> > > > patch. Adjust the clock-names: on dt-bindings would make sense here,
> > > > what do you think?
> > >
> > > I'm confused, what are you suggesting?
> >
> > Sorry for the confusion.
> >
> > The mod clock is not required for A64 and we have a patch for handling
> > mod clock using has_mod_clk quirk(on the series), indeed the mod clock
> > is available in A31 and not needed for A64. So, to satisfy this
> > requirement the clock-names on dt-bindings can update to make mod
> > clock-name is optional and bus clock is required.
>
> No, the bus clock name is not needed if there's only one clock.

Okay, is it because the same clock handle it on PHY side?

>
> > I'm not exactly sure, this is correct but trying to understand if it
> > is possible or not? something like
> >
> >    clocks:
> >       minItems: 1
> >       maxItems: 2
> >      items:
> >        - description: Bus Clock
> >        - description: Module Clock
>
> That's correct.
>
> >    clock-names:
> >       minItems: 1
> >       maxItems: 2
> >      items:
> >        - const: bus
> >        - const: mod
>
> Here, just keep the current clock-names definition, and make it
> required only for SoCs that are not the A64

Okay, please have a look here I have pasted the diff for comments.

   clocks:
+    minItems: 2
     items:
       - description: Bus Clock
       - description: Module Clock
@@ -64,14 +65,26 @@ required:
   - compatible
   - reg
   - interrupts
-  - clocks
-  - clock-names
   - phys
   - phy-names
   - resets
   - vcc-dsi-supply
   - port

+allOf:
+  - if:
+      properties:
+         compatible:
+           contains:
+             const: allwinner,sun6i-a31-mipi-dsi
+      then:
+        properties:
+          clocks:
+            minItems: 2
+        required:
+          - clocks
+          - clock-names
+
 additionalProperties: false

I have marked minItems: 2 on clocks since we need to use minimum of 2
clocks like both bus and mod not mod clock alone.

Please let me know your comments.

Jagan.
Jagan Teki Oct. 24, 2019, 12:56 p.m. UTC | #7
On Thu, Oct 17, 2019 at 3:22 PM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Wed, Oct 16, 2019 at 02:19:44PM +0530, Jagan Teki wrote:
> > On Wed, Oct 16, 2019 at 1:33 PM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Mon, Oct 14, 2019 at 05:37:50PM +0530, Jagan Teki wrote:
> > > > On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > > > > > Add MIPI DSI pipeline for Allwinner A64.
> > > > > >
> > > > > > - dsi node, with A64 compatible since it doesn't support
> > > > > >   DSI_SCLK gating unlike A33
> > > > > > - dphy node, with A64 compatible with A33 fallback since
> > > > > >   DPHY on A64 and A33 is similar
> > > > > > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> > > > > >
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > > > > > ---
> > > > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> > > > > >  1 file changed, 38 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > index 69128a6dfc46..ad4170b8aee0 100644
> > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > @@ -382,6 +382,12 @@
> > > > > >                                       #address-cells = <1>;
> > > > > >                                       #size-cells = <0>;
> > > > > >                                       reg = <1>;
> > > > > > +
> > > > > > +                                     tcon0_out_dsi: endpoint@1 {
> > > > > > +                                             reg = <1>;
> > > > > > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > > > > > +                                             allwinner,tcon-channel = <1>;
> > > > > > +                                     };
> > > > > >                               };
> > > > > >                       };
> > > > > >               };
> > > > > > @@ -1003,6 +1009,38 @@
> > > > > >                       status = "disabled";
> > > > > >               };
> > > > > >
> > > > > > +             dsi: dsi@1ca0000 {
> > > > > > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > > > > > +                     reg = <0x01ca0000 0x1000>;
> > > > > > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > > > > > +                     clock-names = "bus";
> > > > >
> > > > > This won't validate with the bindings you have either here, since it
> > > > > still expects bus and mod.
> > > > >
> > > > > I guess in that cas, we can just drop clock-names, which will require
> > > > > a bit of work on the driver side as well.
> > > >
> > > > Okay.
> > > > mod clock is not required for a64, ie reason we have has_mod_clk quirk
> > > > patch. Adjust the clock-names: on dt-bindings would make sense here,
> > > > what do you think?
> > >
> > > I'm confused, what are you suggesting?
> >
> > Sorry for the confusion.
> >
> > The mod clock is not required for A64 and we have a patch for handling
> > mod clock using has_mod_clk quirk(on the series), indeed the mod clock
> > is available in A31 and not needed for A64. So, to satisfy this
> > requirement the clock-names on dt-bindings can update to make mod
> > clock-name is optional and bus clock is required.
>
> No, the bus clock name is not needed if there's only one clock.

Looks like we need "bus" clock required since the
devm_regmap_init_mmio_clk is created only if bus clock-names added in
dt.
Maxime Ripard Oct. 24, 2019, 6:27 p.m. UTC | #8
On Thu, Oct 24, 2019 at 01:28:28PM +0530, Jagan Teki wrote:
> On Thu, Oct 17, 2019 at 3:22 PM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Wed, Oct 16, 2019 at 02:19:44PM +0530, Jagan Teki wrote:
> > > On Wed, Oct 16, 2019 at 1:33 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > On Mon, Oct 14, 2019 at 05:37:50PM +0530, Jagan Teki wrote:
> > > > > On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > > > >
> > > > > > On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > > > > > > Add MIPI DSI pipeline for Allwinner A64.
> > > > > > >
> > > > > > > - dsi node, with A64 compatible since it doesn't support
> > > > > > >   DSI_SCLK gating unlike A33
> > > > > > > - dphy node, with A64 compatible with A33 fallback since
> > > > > > >   DPHY on A64 and A33 is similar
> > > > > > > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> > > > > > >
> > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > > > > > > ---
> > > > > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> > > > > > >  1 file changed, 38 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > index 69128a6dfc46..ad4170b8aee0 100644
> > > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > @@ -382,6 +382,12 @@
> > > > > > >                                       #address-cells = <1>;
> > > > > > >                                       #size-cells = <0>;
> > > > > > >                                       reg = <1>;
> > > > > > > +
> > > > > > > +                                     tcon0_out_dsi: endpoint@1 {
> > > > > > > +                                             reg = <1>;
> > > > > > > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > > > > > > +                                             allwinner,tcon-channel = <1>;
> > > > > > > +                                     };
> > > > > > >                               };
> > > > > > >                       };
> > > > > > >               };
> > > > > > > @@ -1003,6 +1009,38 @@
> > > > > > >                       status = "disabled";
> > > > > > >               };
> > > > > > >
> > > > > > > +             dsi: dsi@1ca0000 {
> > > > > > > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > > > > > > +                     reg = <0x01ca0000 0x1000>;
> > > > > > > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > > > > > > +                     clock-names = "bus";
> > > > > >
> > > > > > This won't validate with the bindings you have either here, since it
> > > > > > still expects bus and mod.
> > > > > >
> > > > > > I guess in that cas, we can just drop clock-names, which will require
> > > > > > a bit of work on the driver side as well.
> > > > >
> > > > > Okay.
> > > > > mod clock is not required for a64, ie reason we have has_mod_clk quirk
> > > > > patch. Adjust the clock-names: on dt-bindings would make sense here,
> > > > > what do you think?
> > > >
> > > > I'm confused, what are you suggesting?
> > >
> > > Sorry for the confusion.
> > >
> > > The mod clock is not required for A64 and we have a patch for handling
> > > mod clock using has_mod_clk quirk(on the series), indeed the mod clock
> > > is available in A31 and not needed for A64. So, to satisfy this
> > > requirement the clock-names on dt-bindings can update to make mod
> > > clock-name is optional and bus clock is required.
> >
> > No, the bus clock name is not needed if there's only one clock.
>
> Okay, is it because the same clock handle it on PHY side?

No, because there's only one clock and thus you don't need to
differentiate them.

> >
> > > I'm not exactly sure, this is correct but trying to understand if it
> > > is possible or not? something like
> > >
> > >    clocks:
> > >       minItems: 1
> > >       maxItems: 2
> > >      items:
> > >        - description: Bus Clock
> > >        - description: Module Clock
> >
> > That's correct.
> >
> > >    clock-names:
> > >       minItems: 1
> > >       maxItems: 2
> > >      items:
> > >        - const: bus
> > >        - const: mod
> >
> > Here, just keep the current clock-names definition, and make it
> > required only for SoCs that are not the A64
>
> Okay, please have a look here I have pasted the diff for comments.
>
>    clocks:
> +    minItems: 2
>      items:
>        - description: Bus Clock
>        - description: Module Clock

Didn't you tell me that you didn't need the module clock?

How do you handle the case were you just have the bus clock then?

Maxime
Maxime Ripard Oct. 24, 2019, 6:28 p.m. UTC | #9
On Thu, Oct 24, 2019 at 06:26:36PM +0530, Jagan Teki wrote:
> On Thu, Oct 17, 2019 at 3:22 PM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Wed, Oct 16, 2019 at 02:19:44PM +0530, Jagan Teki wrote:
> > > On Wed, Oct 16, 2019 at 1:33 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > On Mon, Oct 14, 2019 at 05:37:50PM +0530, Jagan Teki wrote:
> > > > > On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > > > >
> > > > > > On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > > > > > > Add MIPI DSI pipeline for Allwinner A64.
> > > > > > >
> > > > > > > - dsi node, with A64 compatible since it doesn't support
> > > > > > >   DSI_SCLK gating unlike A33
> > > > > > > - dphy node, with A64 compatible with A33 fallback since
> > > > > > >   DPHY on A64 and A33 is similar
> > > > > > > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> > > > > > >
> > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > > > > > > ---
> > > > > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> > > > > > >  1 file changed, 38 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > index 69128a6dfc46..ad4170b8aee0 100644
> > > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > @@ -382,6 +382,12 @@
> > > > > > >                                       #address-cells = <1>;
> > > > > > >                                       #size-cells = <0>;
> > > > > > >                                       reg = <1>;
> > > > > > > +
> > > > > > > +                                     tcon0_out_dsi: endpoint@1 {
> > > > > > > +                                             reg = <1>;
> > > > > > > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > > > > > > +                                             allwinner,tcon-channel = <1>;
> > > > > > > +                                     };
> > > > > > >                               };
> > > > > > >                       };
> > > > > > >               };
> > > > > > > @@ -1003,6 +1009,38 @@
> > > > > > >                       status = "disabled";
> > > > > > >               };
> > > > > > >
> > > > > > > +             dsi: dsi@1ca0000 {
> > > > > > > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > > > > > > +                     reg = <0x01ca0000 0x1000>;
> > > > > > > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > > > > > > +                     clock-names = "bus";
> > > > > >
> > > > > > This won't validate with the bindings you have either here, since it
> > > > > > still expects bus and mod.
> > > > > >
> > > > > > I guess in that cas, we can just drop clock-names, which will require
> > > > > > a bit of work on the driver side as well.
> > > > >
> > > > > Okay.
> > > > > mod clock is not required for a64, ie reason we have has_mod_clk quirk
> > > > > patch. Adjust the clock-names: on dt-bindings would make sense here,
> > > > > what do you think?
> > > >
> > > > I'm confused, what are you suggesting?
> > >
> > > Sorry for the confusion.
> > >
> > > The mod clock is not required for A64 and we have a patch for handling
> > > mod clock using has_mod_clk quirk(on the series), indeed the mod clock
> > > is available in A31 and not needed for A64. So, to satisfy this
> > > requirement the clock-names on dt-bindings can update to make mod
> > > clock-name is optional and bus clock is required.
> >
> > No, the bus clock name is not needed if there's only one clock.
>
> Looks like we need "bus" clock required since the
> devm_regmap_init_mmio_clk is created only if bus clock-names added in
> dt.

Yeah, hence why I said you'd need "a bit of work on the driver side"

Replacing the clock name by NULL should work.

Maxime
Jagan Teki Oct. 25, 2019, 1:23 p.m. UTC | #10
On Fri, Oct 25, 2019 at 12:33 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Thu, Oct 24, 2019 at 01:28:28PM +0530, Jagan Teki wrote:
> > On Thu, Oct 17, 2019 at 3:22 PM Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > On Wed, Oct 16, 2019 at 02:19:44PM +0530, Jagan Teki wrote:
> > > > On Wed, Oct 16, 2019 at 1:33 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > > >
> > > > > On Mon, Oct 14, 2019 at 05:37:50PM +0530, Jagan Teki wrote:
> > > > > > On Mon, Oct 7, 2019 at 4:27 PM Maxime Ripard <mripard@kernel.org> wrote:
> > > > > > >
> > > > > > > On Sat, Oct 05, 2019 at 07:49:12PM +0530, Jagan Teki wrote:
> > > > > > > > Add MIPI DSI pipeline for Allwinner A64.
> > > > > > > >
> > > > > > > > - dsi node, with A64 compatible since it doesn't support
> > > > > > > >   DSI_SCLK gating unlike A33
> > > > > > > > - dphy node, with A64 compatible with A33 fallback since
> > > > > > > >   DPHY on A64 and A33 is similar
> > > > > > > > - finally, attach the dsi_in to tcon0 for complete MIPI DSI
> > > > > > > >
> > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > > Tested-by: Merlijn Wajer <merlijn@wizzup.org>
> > > > > > > > ---
> > > > > > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 38 +++++++++++++++++++
> > > > > > > >  1 file changed, 38 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > > index 69128a6dfc46..ad4170b8aee0 100644
> > > > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > > > > > @@ -382,6 +382,12 @@
> > > > > > > >                                       #address-cells = <1>;
> > > > > > > >                                       #size-cells = <0>;
> > > > > > > >                                       reg = <1>;
> > > > > > > > +
> > > > > > > > +                                     tcon0_out_dsi: endpoint@1 {
> > > > > > > > +                                             reg = <1>;
> > > > > > > > +                                             remote-endpoint = <&dsi_in_tcon0>;
> > > > > > > > +                                             allwinner,tcon-channel = <1>;
> > > > > > > > +                                     };
> > > > > > > >                               };
> > > > > > > >                       };
> > > > > > > >               };
> > > > > > > > @@ -1003,6 +1009,38 @@
> > > > > > > >                       status = "disabled";
> > > > > > > >               };
> > > > > > > >
> > > > > > > > +             dsi: dsi@1ca0000 {
> > > > > > > > +                     compatible = "allwinner,sun50i-a64-mipi-dsi";
> > > > > > > > +                     reg = <0x01ca0000 0x1000>;
> > > > > > > > +                     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > +                     clocks = <&ccu CLK_BUS_MIPI_DSI>;
> > > > > > > > +                     clock-names = "bus";
> > > > > > >
> > > > > > > This won't validate with the bindings you have either here, since it
> > > > > > > still expects bus and mod.
> > > > > > >
> > > > > > > I guess in that cas, we can just drop clock-names, which will require
> > > > > > > a bit of work on the driver side as well.
> > > > > >
> > > > > > Okay.
> > > > > > mod clock is not required for a64, ie reason we have has_mod_clk quirk
> > > > > > patch. Adjust the clock-names: on dt-bindings would make sense here,
> > > > > > what do you think?
> > > > >
> > > > > I'm confused, what are you suggesting?
> > > >
> > > > Sorry for the confusion.
> > > >
> > > > The mod clock is not required for A64 and we have a patch for handling
> > > > mod clock using has_mod_clk quirk(on the series), indeed the mod clock
> > > > is available in A31 and not needed for A64. So, to satisfy this
> > > > requirement the clock-names on dt-bindings can update to make mod
> > > > clock-name is optional and bus clock is required.
> > >
> > > No, the bus clock name is not needed if there's only one clock.
> >
> > Okay, is it because the same clock handle it on PHY side?
>
> No, because there's only one clock and thus you don't need to
> differentiate them.
>
> > >
> > > > I'm not exactly sure, this is correct but trying to understand if it
> > > > is possible or not? something like
> > > >
> > > >    clocks:
> > > >       minItems: 1
> > > >       maxItems: 2
> > > >      items:
> > > >        - description: Bus Clock
> > > >        - description: Module Clock
> > >
> > > That's correct.
> > >
> > > >    clock-names:
> > > >       minItems: 1
> > > >       maxItems: 2
> > > >      items:
> > > >        - const: bus
> > > >        - const: mod
> > >
> > > Here, just keep the current clock-names definition, and make it
> > > required only for SoCs that are not the A64
> >
> > Okay, please have a look here I have pasted the diff for comments.
> >
> >    clocks:
> > +    minItems: 2
> >      items:
> >        - description: Bus Clock
> >        - description: Module Clock
>
> Didn't you tell me that you didn't need the module clock?
>
> How do you handle the case were you just have the bus clock then?

Make sense, it is my mistake then. we don't require to specify here I
think since it implies globally. I think it should be sufficient to
mention on allOf: section based on the SoC like I mentioned in above
snippet.

Patch
diff mbox series

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 69128a6dfc46..ad4170b8aee0 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -382,6 +382,12 @@ 
 					#address-cells = <1>;
 					#size-cells = <0>;
 					reg = <1>;
+
+					tcon0_out_dsi: endpoint@1 {
+						reg = <1>;
+						remote-endpoint = <&dsi_in_tcon0>;
+						allwinner,tcon-channel = <1>;
+					};
 				};
 			};
 		};
@@ -1003,6 +1009,38 @@ 
 			status = "disabled";
 		};
 
+		dsi: dsi@1ca0000 {
+			compatible = "allwinner,sun50i-a64-mipi-dsi";
+			reg = <0x01ca0000 0x1000>;
+			interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_MIPI_DSI>;
+			clock-names = "bus";
+			resets = <&ccu RST_BUS_MIPI_DSI>;
+			phys = <&dphy>;
+			phy-names = "dphy";
+			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port {
+				dsi_in_tcon0: endpoint {
+					remote-endpoint = <&tcon0_out_dsi>;
+				};
+			};
+		};
+
+		dphy: d-phy@1ca1000 {
+			compatible = "allwinner,sun50i-a64-mipi-dphy",
+				     "allwinner,sun6i-a31-mipi-dphy";
+			reg = <0x01ca1000 0x1000>;
+			clocks = <&ccu CLK_BUS_MIPI_DSI>,
+				 <&ccu CLK_DSI_DPHY>;
+			clock-names = "bus", "mod";
+			resets = <&ccu RST_BUS_MIPI_DSI>;
+			status = "disabled";
+			#phy-cells = <0>;
+		};
+
 		hdmi: hdmi@1ee0000 {
 			compatible = "allwinner,sun50i-a64-dw-hdmi",
 				     "allwinner,sun8i-a83t-dw-hdmi";