[PATCH 0/3] Fix Versa3 clock mapping

[PATCH 0/3] Fix Versa3 clock mapping

[Biju Das]

According to Table 3. ("Output Source") in the 5P35023 datasheet,
the output clock mapping should be 0=REF, 1=SE1, 2=SE2, 3=SE3,
4=DIFF1, 5=DIFF2. But the code uses inverse.

This patch series aims to document clock-output-names in bindings and
fix the mapping in driver.

Biju Das (3):
  dt-bindings: clock: versaclock3: Document clock-output-names
  clk: vc3: Fix output clock mapping
  arm64: dts: renesas: rz-smarc-common: Use versa3 clk for audio mclk

 .../bindings/clock/renesas,5p35023.yaml       | 14 ++--
 .../boot/dts/renesas/rz-smarc-common.dtsi     | 14 ++--
 arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi  | 23 +++++++
 arch/arm64/boot/dts/renesas/rzg2lc-smarc.dtsi | 23 +++++++
 arch/arm64/boot/dts/renesas/rzg2ul-smarc.dtsi | 27 ++++++++
 drivers/clk/clk-versaclock3.c                 | 68 +++++++++----------
 6 files changed, 124 insertions(+), 45 deletions(-)

-- 
2.25.1

[PATCH 1/3] dt-bindings: clock: versaclock3: Document clock-output-names

[Biju Das]

Document clock-output-names property. Update the example according to
Table 3. ("Output Source") in the 5P35023 datasheet.

While at it, replace clocks phandle in the example from x1_x2->x1 as
X2 is a different 32768 kHz crystal.

Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Closes: https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpaejss0Y2ZpLCMNg@mail.gmail.com/
Fixes: a03d23f860eb ("dt-bindings: clock: Add Renesas versa3 clock generator bindings")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 .../devicetree/bindings/clock/renesas,5p35023.yaml | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
index 839648e753d4..db8d01b291dd 100644
--- a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
+++ b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
@@ -49,6 +49,9 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint8-array
     maxItems: 37
 
+  clock-output-names:
+    maxItems: 6
+
 required:
   - compatible
   - reg
@@ -68,7 +71,7 @@ examples:
             reg = <0x68>;
             #clock-cells = <1>;
 
-            clocks = <&x1_x2>;
+            clocks = <&x1>;
 
             renesas,settings = [
                 80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
@@ -76,11 +79,14 @@ examples:
                 80 b0 45 c4 95
             ];
 
+            clock-output-names = "ref", "se1", "se2", "se3",
+                                 "diff1", "diff2";
+
             assigned-clocks = <&versa3 0>, <&versa3 1>,
                               <&versa3 2>, <&versa3 3>,
                               <&versa3 4>, <&versa3 5>;
-            assigned-clock-rates = <12288000>, <25000000>,
-                                   <12000000>, <11289600>,
-                                   <11289600>, <24000000>;
+            assigned-clock-rates = <24000000>, <11289600>,
+                                   <11289600>, <12000000>,
+                                   <25000000>, <12288000>;
         };
     };
-- 
2.25.1

Re: [PATCH 1/3] dt-bindings: clock: versaclock3: Document clock-output-names

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2507 bytes --]

Hey Biju,

On Wed, Aug 02, 2023 at 01:25:08PM +0100, Biju Das wrote:
> Document clock-output-names property. Update the example according to
> Table 3. ("Output Source") in the 5P35023 datasheet.
> 
> While at it, replace clocks phandle in the example from x1_x2->x1 as
> X2 is a different 32768 kHz crystal.
> 
> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Closes: https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpaejss0Y2ZpLCMNg@mail.gmail.com/
> Fixes: a03d23f860eb ("dt-bindings: clock: Add Renesas versa3 clock generator bindings")

Nothing in this commit message explains why this is a fix for this
binding addition :(

> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../devicetree/bindings/clock/renesas,5p35023.yaml | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
> index 839648e753d4..db8d01b291dd 100644
> --- a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
> +++ b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
> @@ -49,6 +49,9 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint8-array
>      maxItems: 37
>  
> +  clock-output-names:
> +    maxItems: 6
> +
>  required:
>    - compatible
>    - reg
> @@ -68,7 +71,7 @@ examples:
>              reg = <0x68>;
>              #clock-cells = <1>;
>  
> -            clocks = <&x1_x2>;
> +            clocks = <&x1>;
>  
>              renesas,settings = [
>                  80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
> @@ -76,11 +79,14 @@ examples:
>                  80 b0 45 c4 95
>              ];
>  
> +            clock-output-names = "ref", "se1", "se2", "se3",
> +                                 "diff1", "diff2";
> +
>              assigned-clocks = <&versa3 0>, <&versa3 1>,
>                                <&versa3 2>, <&versa3 3>,
>                                <&versa3 4>, <&versa3 5>;
> -            assigned-clock-rates = <12288000>, <25000000>,
> -                                   <12000000>, <11289600>,
> -                                   <11289600>, <24000000>;
> +            assigned-clock-rates = <24000000>, <11289600>,
> +                                   <11289600>, <12000000>,
> +                                   <25000000>, <12288000>;
>          };
>      };
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH 1/3] dt-bindings: clock: versaclock3: Document clock-output-names

[Biju Das]

Hi Conor Dooley,

Thanks for the feedback.

> Subject: Re: [PATCH 1/3] dt-bindings: clock: versaclock3: Document
> clock-output-names
> 
> Hey Biju,
> 
> On Wed, Aug 02, 2023 at 01:25:08PM +0100, Biju Das wrote:
> > Document clock-output-names property. Update the example according to
> > Table 3. ("Output Source") in the 5P35023 datasheet.
> >
> > While at it, replace clocks phandle in the example from x1_x2->x1 as
> > X2 is a different 32768 kHz crystal.
> >
> > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Closes:
> > https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpaejss0
> > Y2ZpLCMNg@mail.gmail.com/
> > Fixes: a03d23f860eb ("dt-bindings: clock: Add Renesas versa3 clock
> > generator bindings")
> 
> Nothing in this commit message explains why this is a fix for this
> binding addition :(

Basically, it fixes "assigned-clock-rates" for each clock output in the example. Now it is based on Table 3. ("Output Source") in the 5P35023 datasheet(ie: 0= REF, 1=SE1, 2=SE2, 3=SE3, 4=DIFF1, 5=DIFF2).

The newly added clock-output-names in the example are based on the above table.

I have added fixes tag, because this patch fixes the clock mapping in the example as per the HW manual.

Please let me know should I drop fixes tag??

Cheers,
Biju


> 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  .../devicetree/bindings/clock/renesas,5p35023.yaml | 14
> > ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
> > b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
> > index 839648e753d4..db8d01b291dd 100644
> > --- a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
> > +++ b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
> > @@ -49,6 +49,9 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/uint8-array
> >      maxItems: 37
> >
> > +  clock-output-names:
> > +    maxItems: 6
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -68,7 +71,7 @@ examples:
> >              reg = <0x68>;
> >              #clock-cells = <1>;
> >
> > -            clocks = <&x1_x2>;
> > +            clocks = <&x1>;
> >
> >              renesas,settings = [
> >                  80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf @@
> > -76,11 +79,14 @@ examples:
> >                  80 b0 45 c4 95
> >              ];
> >
> > +            clock-output-names = "ref", "se1", "se2", "se3",
> > +                                 "diff1", "diff2";
> > +
> >              assigned-clocks = <&versa3 0>, <&versa3 1>,
> >                                <&versa3 2>, <&versa3 3>,
> >                                <&versa3 4>, <&versa3 5>;
> > -            assigned-clock-rates = <12288000>, <25000000>,
> > -                                   <12000000>, <11289600>,
> > -                                   <11289600>, <24000000>;
> > +            assigned-clock-rates = <24000000>, <11289600>,
> > +                                   <11289600>, <12000000>,
> > +                                   <25000000>, <12288000>;
> >          };
> >      };
> > --
> > 2.25.1
> >

Re: [PATCH 1/3] dt-bindings: clock: versaclock3: Document clock-output-names

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

On Thu, Aug 03, 2023 at 04:25:40PM +0000, Biju Das wrote:
> > Subject: Re: [PATCH 1/3] dt-bindings: clock: versaclock3: Document
> > clock-output-names
> > 
> > On Wed, Aug 02, 2023 at 01:25:08PM +0100, Biju Das wrote:
> > > Document clock-output-names property. Update the example according to
> > > Table 3. ("Output Source") in the 5P35023 datasheet.
> > >
> > > While at it, replace clocks phandle in the example from x1_x2->x1 as
> > > X2 is a different 32768 kHz crystal.
> > >
> > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Closes:
> > > https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpaejss0
> > > Y2ZpLCMNg@mail.gmail.com/
> > > Fixes: a03d23f860eb ("dt-bindings: clock: Add Renesas versa3 clock
> > > generator bindings")
> > 
> > Nothing in this commit message explains why this is a fix for this
> > binding addition :(
> 
> Basically, it fixes "assigned-clock-rates" for each clock output in the example. Now it is based on Table 3. ("Output Source") in the 5P35023 datasheet(ie: 0= REF, 1=SE1, 2=SE2, 3=SE3, 4=DIFF1, 5=DIFF2).
> 
> The newly added clock-output-names in the example are based on the above table.
> 
> I have added fixes tag, because this patch fixes the clock mapping in the example as per the HW manual.
> 
> Please let me know should I drop fixes tag??

I'm just asking for an explanation in the commit message as to what was
actually wrong in the first place. The commit message says 3 things of
which it's hard to know what is actually a fix without opening & reading
the linked thread on lore.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH 1/3] dt-bindings: clock: versaclock3: Document clock-output-names

[Biju Das]

Hi Conor,

Thanks for the feedback.

> Subject: Re: [PATCH 1/3] dt-bindings: clock: versaclock3: Document
> clock-output-names
> 
> On Thu, Aug 03, 2023 at 04:25:40PM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH 1/3] dt-bindings: clock: versaclock3: Document
> > > clock-output-names
> > >
> > > On Wed, Aug 02, 2023 at 01:25:08PM +0100, Biju Das wrote:
> > > > Document clock-output-names property. Update the example according
> > > > to Table 3. ("Output Source") in the 5P35023 datasheet.
> > > >
> > > > While at it, replace clocks phandle in the example from x1_x2->x1
> > > > as
> > > > X2 is a different 32768 kHz crystal.
> > > >
> > > > Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > Closes:
> > > > https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpae
> > > > jss0
> > > > Y2ZpLCMNg@mail.gmail.com/
> > > > Fixes: a03d23f860eb ("dt-bindings: clock: Add Renesas versa3 clock
> > > > generator bindings")
> > >
> > > Nothing in this commit message explains why this is a fix for this
> > > binding addition :(
> >
> > Basically, it fixes "assigned-clock-rates" for each clock output in
> the example. Now it is based on Table 3. ("Output Source") in the
> 5P35023 datasheet(ie: 0= REF, 1=SE1, 2=SE2, 3=SE3, 4=DIFF1, 5=DIFF2).
> >
> > The newly added clock-output-names in the example are based on the
> above table.
> >
> > I have added fixes tag, because this patch fixes the clock mapping in
> the example as per the HW manual.
> >
> > Please let me know should I drop fixes tag??
> 
> I'm just asking for an explanation in the commit message as to what was
> actually wrong in the first place. The commit message says 3 things of
> which it's hard to know what is actually a fix without opening & reading
> the linked thread on lore.

OK, Will explicitly mention it fixes "assigned-clock-rates" for each clock output in the example based on Table 3.("Output Source") in the 5P35023 datasheet.

Cheers,
Biju

[PATCH 2/3] clk: vc3: Fix output clock mapping

[Biju Das]

According to Table 3. ("Output Source") in the 5P35023 datasheet,
the output clock mapping should be 0=REF, 1=SE1, 2=SE2, 3=SE3,
4=DIFF1, 5=DIFF2. But the code uses inverse. Fix this mapping issue.

Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Closes: https://lore.kernel.org/all/CAMuHMdUHD+bEco=WYTYWsTAyRt3dTQQt4Xpaejss0Y2ZpLCMNg@mail.gmail.com/
Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/clk/clk-versaclock3.c | 68 +++++++++++++++++------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
index 7ca413a5b1fb..80f9f13f0e70 100644
--- a/drivers/clk/clk-versaclock3.c
+++ b/drivers/clk/clk-versaclock3.c
@@ -119,20 +119,20 @@ enum vc3_div {
 };
 
 enum vc3_clk_mux {
-	VC3_DIFF2_MUX,
-	VC3_DIFF1_MUX,
-	VC3_SE3_MUX,
-	VC3_SE2_MUX,
 	VC3_SE1_MUX,
+	VC3_SE2_MUX,
+	VC3_SE3_MUX,
+	VC3_DIFF1_MUX,
+	VC3_DIFF2_MUX,
 };
 
 enum vc3_clk {
-	VC3_DIFF2,
-	VC3_DIFF1,
-	VC3_SE3,
-	VC3_SE2,
-	VC3_SE1,
 	VC3_REF,
+	VC3_SE1,
+	VC3_SE2,
+	VC3_SE3,
+	VC3_DIFF1,
+	VC3_DIFF2,
 };
 
 struct vc3_clk_data {
@@ -896,33 +896,33 @@ static struct vc3_hw_data clk_div[] = {
 };
 
 static struct vc3_hw_data clk_mux[] = {
-	[VC3_DIFF2_MUX] = {
+	[VC3_SE1_MUX] = {
 		.data = &(struct vc3_clk_data) {
-			.offs = VC3_DIFF2_CTRL_REG,
-			.bitmsk = VC3_DIFF2_CTRL_REG_DIFF2_CLK_SEL
+			.offs = VC3_SE1_DIV4_CTRL,
+			.bitmsk = VC3_SE1_DIV4_CTRL_SE1_CLK_SEL
 		},
 		.hw.init = &(struct clk_init_data){
-			.name = "diff2_mux",
+			.name = "se1_mux",
 			.ops = &vc3_clk_mux_ops,
 			.parent_hws = (const struct clk_hw *[]) {
-				&clk_div[VC3_DIV1].hw,
-				&clk_div[VC3_DIV3].hw
+				&clk_div[VC3_DIV5].hw,
+				&clk_div[VC3_DIV4].hw
 			},
 			.num_parents = 2,
 			.flags = CLK_SET_RATE_PARENT
 		}
 	},
-	[VC3_DIFF1_MUX] = {
+	[VC3_SE2_MUX] = {
 		.data = &(struct vc3_clk_data) {
-			.offs = VC3_DIFF1_CTRL_REG,
-			.bitmsk = VC3_DIFF1_CTRL_REG_DIFF1_CLK_SEL
+			.offs = VC3_SE2_CTRL_REG0,
+			.bitmsk = VC3_SE2_CTRL_REG0_SE2_CLK_SEL
 		},
 		.hw.init = &(struct clk_init_data){
-			.name = "diff1_mux",
+			.name = "se2_mux",
 			.ops = &vc3_clk_mux_ops,
 			.parent_hws = (const struct clk_hw *[]) {
-				&clk_div[VC3_DIV1].hw,
-				&clk_div[VC3_DIV3].hw
+				&clk_div[VC3_DIV5].hw,
+				&clk_div[VC3_DIV4].hw
 			},
 			.num_parents = 2,
 			.flags = CLK_SET_RATE_PARENT
@@ -944,33 +944,33 @@ static struct vc3_hw_data clk_mux[] = {
 			.flags = CLK_SET_RATE_PARENT
 		}
 	},
-	[VC3_SE2_MUX] = {
+	[VC3_DIFF1_MUX] = {
 		.data = &(struct vc3_clk_data) {
-			.offs = VC3_SE2_CTRL_REG0,
-			.bitmsk = VC3_SE2_CTRL_REG0_SE2_CLK_SEL
+			.offs = VC3_DIFF1_CTRL_REG,
+			.bitmsk = VC3_DIFF1_CTRL_REG_DIFF1_CLK_SEL
 		},
 		.hw.init = &(struct clk_init_data){
-			.name = "se2_mux",
+			.name = "diff1_mux",
 			.ops = &vc3_clk_mux_ops,
 			.parent_hws = (const struct clk_hw *[]) {
-				&clk_div[VC3_DIV5].hw,
-				&clk_div[VC3_DIV4].hw
+				&clk_div[VC3_DIV1].hw,
+				&clk_div[VC3_DIV3].hw
 			},
 			.num_parents = 2,
 			.flags = CLK_SET_RATE_PARENT
 		}
 	},
-	[VC3_SE1_MUX] = {
+	[VC3_DIFF2_MUX] = {
 		.data = &(struct vc3_clk_data) {
-			.offs = VC3_SE1_DIV4_CTRL,
-			.bitmsk = VC3_SE1_DIV4_CTRL_SE1_CLK_SEL
+			.offs = VC3_DIFF2_CTRL_REG,
+			.bitmsk = VC3_DIFF2_CTRL_REG_DIFF2_CLK_SEL
 		},
 		.hw.init = &(struct clk_init_data){
-			.name = "se1_mux",
+			.name = "diff2_mux",
 			.ops = &vc3_clk_mux_ops,
 			.parent_hws = (const struct clk_hw *[]) {
-				&clk_div[VC3_DIV5].hw,
-				&clk_div[VC3_DIV4].hw
+				&clk_div[VC3_DIV1].hw,
+				&clk_div[VC3_DIV3].hw
 			},
 			.num_parents = 2,
 			.flags = CLK_SET_RATE_PARENT
@@ -1109,7 +1109,7 @@ static int vc3_probe(struct i2c_client *client)
 				name, 0, CLK_SET_RATE_PARENT, 1, 1);
 		else
 			clk_out[i] = devm_clk_hw_register_fixed_factor_parent_hw(dev,
-				name, &clk_mux[i].hw, CLK_SET_RATE_PARENT, 1, 1);
+				name, &clk_mux[i - 1].hw, CLK_SET_RATE_PARENT, 1, 1);
 
 		if (IS_ERR(clk_out[i]))
 			return PTR_ERR(clk_out[i]);
-- 
2.25.1