* [PATCH v2 0/3] Fix Versa3 clock mapping
@ 2023-08-17 9:08 Biju Das
2023-08-17 9:08 ` [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names Biju Das
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Biju Das @ 2023-08-17 9:08 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Biju Das, Geert Uytterhoeven, Magnus Damm, linux-renesas-soc,
linux-clk, devicetree, Prabhakar Mahadev Lad
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.
v1->v2:
* Updated binding commit description to make it clear it fixes
"assigned-clock-rates" in the example based on 5P35023 datasheet.
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names
2023-08-17 9:08 [PATCH v2 0/3] Fix Versa3 clock mapping Biju Das
@ 2023-08-17 9:08 ` Biju Das
2023-08-19 8:30 ` Conor Dooley
2023-08-21 8:00 ` Geert Uytterhoeven
2023-08-17 9:08 ` [PATCH v2 2/3] clk: vc3: Fix output clock mapping Biju Das
2023-08-17 9:08 ` [PATCH v2 3/3] arm64: dts: renesas: rz-smarc-common: Use versa3 clk for audio mclk Biju Das
2 siblings, 2 replies; 10+ messages in thread
From: Biju Das @ 2023-08-17 9:08 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: Biju Das, Geert Uytterhoeven, Magnus Damm, linux-renesas-soc,
linux-clk, devicetree, Prabhakar Mahadev Lad
Document clock-output-names property and fix the "assigned-clock-rates"
for each clock output in the example based on Table 3. ("Output Source")
in the 5P35023 datasheet(ie: {REF,SE1,SE2,SE3,DIFF1,DIFF2}).
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>
---
v1->v2:
* Updated commit description to make it clear it fixes
"assigned-clock-rates" in the example based on 5P35023 datasheet.
---
.../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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] clk: vc3: Fix output clock mapping
2023-08-17 9:08 [PATCH v2 0/3] Fix Versa3 clock mapping Biju Das
2023-08-17 9:08 ` [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names Biju Das
@ 2023-08-17 9:08 ` Biju Das
2023-08-17 9:51 ` Geert Uytterhoeven
2023-08-17 9:08 ` [PATCH v2 3/3] arm64: dts: renesas: rz-smarc-common: Use versa3 clk for audio mclk Biju Das
2 siblings, 1 reply; 10+ messages in thread
From: Biju Das @ 2023-08-17 9:08 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: Biju Das, linux-clk, Geert Uytterhoeven, Prabhakar Mahadev Lad,
linux-renesas-soc
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>
---
v1->v2:
* No change.
---
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 7ab2447bd203..4ee9078f4bf4 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 {
@@ -897,33 +897,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
@@ -945,33 +945,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
@@ -1110,7 +1110,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] arm64: dts: renesas: rz-smarc-common: Use versa3 clk for audio mclk
2023-08-17 9:08 [PATCH v2 0/3] Fix Versa3 clock mapping Biju Das
2023-08-17 9:08 ` [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names Biju Das
2023-08-17 9:08 ` [PATCH v2 2/3] clk: vc3: Fix output clock mapping Biju Das
@ 2023-08-17 9:08 ` Biju Das
2 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2023-08-17 9:08 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Biju Das, Geert Uytterhoeven, Magnus Damm, linux-renesas-soc,
devicetree, Prabhakar Mahadev Lad
Currently audio mclk uses a fixed clk of 11.2896MHz (multiple of 44.1kHz).
Replace this fixed clk with the programmable versa3 clk that can provide
the clocking to support both 44.1kHz (with a clock of 11.2896MHz) and
48kHz (with a clock of 12.2880MHz), based on audio sampling rate for
playback and record.
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
* No change.
v1:
* Added this patch as part of this series.
* Replaced xtal->x1-clock and x1_x2->x1.
* Added clock-output-names.
* Updated clock-frequency = <400000> for RZ/G2UL i2c0
* Updated assigned-clocks and assigned-clock-rates as per bindings.
* Replaced mclk from '<&versa3 3>'->'<&versa3 2>'.
---
.../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 +++++++++++++++++++
4 files changed, 80 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/rz-smarc-common.dtsi b/arch/arm64/boot/dts/renesas/rz-smarc-common.dtsi
index a7594ba3a998..b7a3e6caa386 100644
--- a/arch/arm64/boot/dts/renesas/rz-smarc-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/rz-smarc-common.dtsi
@@ -32,12 +32,6 @@ chosen {
stdout-path = "serial0:115200n8";
};
- audio_mclock: audio_mclock {
- compatible = "fixed-clock";
- #clock-cells = <0>;
- clock-frequency = <11289600>;
- };
-
snd_rzg2l: sound {
compatible = "simple-audio-card";
simple-audio-card,format = "i2s";
@@ -55,7 +49,7 @@ cpu_dai: simple-audio-card,cpu {
};
codec_dai: simple-audio-card,codec {
- clocks = <&audio_mclock>;
+ clocks = <&versa3 2>;
sound-dai = <&wm8978>;
};
};
@@ -76,6 +70,12 @@ vccq_sdhi1: regulator-vccq-sdhi1 {
gpios-states = <1>;
states = <3300000 1>, <1800000 0>;
};
+
+ x1: x1-clock {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <24000000>;
+ };
};
&audio_clk1 {
diff --git a/arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi
index 68eab8e26bf2..186ca8f305db 100644
--- a/arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg2l-smarc.dtsi
@@ -105,6 +105,29 @@ &i2c3 {
status = "okay";
+ versa3: versa3@68 {
+ compatible = "renesas,5p35023";
+ reg = <0x68>;
+ #clock-cells = <1>;
+ clocks = <&x1>;
+
+ renesas,settings = [
+ 80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
+ 00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
+ 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 = <24000000>, <11289600>,
+ <11289600>, <12000000>,
+ <25000000>, <12288000>;
+ };
+
wm8978: codec@1a {
compatible = "wlf,wm8978";
#sound-dai-cells = <0>;
diff --git a/arch/arm64/boot/dts/renesas/rzg2lc-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg2lc-smarc.dtsi
index 83fce96a2575..5abac6bc03c9 100644
--- a/arch/arm64/boot/dts/renesas/rzg2lc-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg2lc-smarc.dtsi
@@ -121,6 +121,29 @@ &i2c2 {
status = "okay";
+ versa3: versa3@68 {
+ compatible = "renesas,5p35023";
+ reg = <0x68>;
+ #clock-cells = <1>;
+ clocks = <&x1>;
+
+ renesas,settings = [
+ 80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
+ 00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
+ 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 = <24000000>, <11289600>,
+ <11289600>, <12000000>,
+ <25000000>, <12288000>;
+ };
+
wm8978: codec@1a {
compatible = "wlf,wm8978";
#sound-dai-cells = <0>;
diff --git a/arch/arm64/boot/dts/renesas/rzg2ul-smarc.dtsi b/arch/arm64/boot/dts/renesas/rzg2ul-smarc.dtsi
index 8eb411aac80d..7e0a5814824e 100644
--- a/arch/arm64/boot/dts/renesas/rzg2ul-smarc.dtsi
+++ b/arch/arm64/boot/dts/renesas/rzg2ul-smarc.dtsi
@@ -20,6 +20,33 @@ &cpu_dai {
sound-dai = <&ssi1>;
};
+&i2c0 {
+ clock-frequency = <400000>;
+
+ versa3: versa3@68 {
+ compatible = "renesas,5p35023";
+ reg = <0x68>;
+ #clock-cells = <1>;
+ clocks = <&x1>;
+
+ renesas,settings = [
+ 80 00 11 19 4c 02 23 7f 83 19 08 a9 5f 25 24 bf
+ 00 14 7a e1 00 00 00 00 01 55 59 bb 3f 30 90 b6
+ 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 = <24000000>, <11289600>,
+ <11289600>, <12000000>,
+ <25000000>, <12288000>;
+ };
+};
+
&i2c1 {
wm8978: codec@1a {
compatible = "wlf,wm8978";
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] clk: vc3: Fix output clock mapping
2023-08-17 9:08 ` [PATCH v2 2/3] clk: vc3: Fix output clock mapping Biju Das
@ 2023-08-17 9:51 ` Geert Uytterhoeven
2023-08-17 10:20 ` Biju Das
0 siblings, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2023-08-17 9:51 UTC (permalink / raw)
To: Biju Das
Cc: Michael Turquette, Stephen Boyd, linux-clk, Geert Uytterhoeven,
Prabhakar Mahadev Lad, linux-renesas-soc
Hi Biju,
On Thu, Aug 17, 2023 at 11:08 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 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>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
One suggestion for future improvement (which can be a separate patch)
below...
> --- 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 {
> @@ -1110,7 +1110,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);
This is (and was before) fragile, as it implies a strict relation between
the vc3_clk_mux and vc3_clk enum values. To avoid accidental
breakage, I think it would be wise to make this explicit, e.g.
enum vc3_clk_mux {
VC3_SE1_MUX = VC3_SE1 - 1,
VC3_SE2_MUX = VC3_SE2 - 1,
[...]
};
>
> if (IS_ERR(clk_out[i]))
> return PTR_ERR(clk_out[i]);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 2/3] clk: vc3: Fix output clock mapping
2023-08-17 9:51 ` Geert Uytterhoeven
@ 2023-08-17 10:20 ` Biju Das
0 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2023-08-17 10:20 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Michael Turquette, Stephen Boyd, linux-clk, Geert Uytterhoeven,
Prabhakar Mahadev Lad, linux-renesas-soc
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v2 2/3] clk: vc3: Fix output clock mapping
>
> Hi Biju,
>
> On Thu, Aug 17, 2023 at 11:08 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > 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>
> > Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> One suggestion for future improvement (which can be a separate patch)
> below...
Ok.
>
> > --- 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 {
>
> > @@ -1110,7 +1110,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);
>
> This is (and was before) fragile, as it implies a strict relation between
> the vc3_clk_mux and vc3_clk enum values. To avoid accidental breakage, I
> think it would be wise to make this explicit, e.g.
>
> enum vc3_clk_mux {
> VC3_SE1_MUX = VC3_SE1 - 1,
> VC3_SE2_MUX = VC3_SE2 - 1,
> [...]
> };
Agreed.
Cheers,
Biju
>
> >
> > if (IS_ERR(clk_out[i]))
> > return PTR_ERR(clk_out[i]);
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
>
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
> -- Linus Torvalds
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names
2023-08-17 9:08 ` [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names Biju Das
@ 2023-08-19 8:30 ` Conor Dooley
2023-08-21 8:00 ` Geert Uytterhoeven
1 sibling, 0 replies; 10+ messages in thread
From: Conor Dooley @ 2023-08-19 8:30 UTC (permalink / raw)
To: Biju Das
Cc: Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, linux-renesas-soc, linux-clk, devicetree,
Prabhakar Mahadev Lad
[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]
On Thu, Aug 17, 2023 at 10:08:08AM +0100, Biju Das wrote:
> Document clock-output-names property and fix the "assigned-clock-rates"
> for each clock output in the example based on Table 3. ("Output Source")
> in the 5P35023 datasheet(ie: {REF,SE1,SE2,SE3,DIFF1,DIFF2}).
>
> 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>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> ---
> v1->v2:
> * Updated commit description to make it clear it fixes
> "assigned-clock-rates" in the example based on 5P35023 datasheet.
> ---
> .../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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names
2023-08-17 9:08 ` [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names Biju Das
2023-08-19 8:30 ` Conor Dooley
@ 2023-08-21 8:00 ` Geert Uytterhoeven
2023-08-21 8:10 ` Biju Das
1 sibling, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2023-08-21 8:00 UTC (permalink / raw)
To: Biju Das
Cc: Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, linux-renesas-soc, linux-clk, devicetree,
Prabhakar Mahadev Lad
Hi Biju,
On Thu, Aug 17, 2023 at 11:08 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Document clock-output-names property and fix the "assigned-clock-rates"
> for each clock output in the example based on Table 3. ("Output Source")
> in the 5P35023 datasheet(ie: {REF,SE1,SE2,SE3,DIFF1,DIFF2}).
>
> 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>
> ---
> v1->v2:
> * Updated commit description to make it clear it fixes
> "assigned-clock-rates" in the example based on 5P35023 datasheet.
Thanks for your patch!
> ---
> .../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
> +
Why do you need clock-output-names?
The clock output names should be created by the driver (taking into
account the instance number, so it works with multiple instances).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names
2023-08-21 8:00 ` Geert Uytterhoeven
@ 2023-08-21 8:10 ` Biju Das
2023-08-21 8:32 ` Geert Uytterhoeven
0 siblings, 1 reply; 10+ messages in thread
From: Biju Das @ 2023-08-21 8:10 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, linux-renesas-soc, linux-clk, devicetree,
Prabhakar Mahadev Lad
Hi Geert Uytterhoeven,
Thanks for the feedback.
> Subject: Re: [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document
> clock-output-names
>
> Hi Biju,
>
> On Thu, Aug 17, 2023 at 11:08 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Document clock-output-names property and fix the "assigned-clock-rates"
> > for each clock output in the example based on Table 3. ("Output
> > Source") in the 5P35023 datasheet(ie: {REF,SE1,SE2,SE3,DIFF1,DIFF2}).
> >
> > 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:
> > Fixes: a03d23f860eb ("dt-bindings: clock: Add Renesas versa3 clock
> > generator bindings")
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v1->v2:
> > * Updated commit description to make it clear it fixes
> > "assigned-clock-rates" in the example based on 5P35023 datasheet.
>
> Thanks for your patch!
> > ---
> > .../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
> > +
>
> Why do you need clock-output-names?
I thought it will be useful information for a user, by looking at the example the name of clock-output-name and corresponding assigned-clocks and assigned-clock-rates.
See below, from this one can understand the relation between index and actual clock output.
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 = <24000000>, <11289600>,
<11289600>, <12000000>,
<25000000>, <12288000>;
> The clock output names should be created by the driver (taking into account
> the instance number, so it works with multiple instances).
OK, so shall I remove it from bindings then?
Cheers,
Biju
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names
2023-08-21 8:10 ` Biju Das
@ 2023-08-21 8:32 ` Geert Uytterhoeven
0 siblings, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2023-08-21 8:32 UTC (permalink / raw)
To: Biju Das
Cc: Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm, linux-renesas-soc, linux-clk, devicetree,
Prabhakar Mahadev Lad
Hi Biju,
On Mon, Aug 21, 2023 at 10:11 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document
> > clock-output-names
> > On Thu, Aug 17, 2023 at 11:08 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > Document clock-output-names property and fix the "assigned-clock-rates"
> > > for each clock output in the example based on Table 3. ("Output
> > > Source") in the 5P35023 datasheet(ie: {REF,SE1,SE2,SE3,DIFF1,DIFF2}).
> > >
> > > 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:
> > > Fixes: a03d23f860eb ("dt-bindings: clock: Add Renesas versa3 clock
> > > generator bindings")
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > ---
> > > v1->v2:
> > > * Updated commit description to make it clear it fixes
> > > "assigned-clock-rates" in the example based on 5P35023 datasheet.
> >
> > Thanks for your patch!
> > > ---
> > > .../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
> > > +
> >
> > Why do you need clock-output-names?
>
> I thought it will be useful information for a user, by looking at the example the name of clock-output-name and corresponding assigned-clocks and assigned-clock-rates.
>
> See below, from this one can understand the relation between index and actual clock output.
>
> 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 = <24000000>, <11289600>,
> <11289600>, <12000000>,
> <25000000>, <12288000>;
>
> > The clock output names should be created by the driver (taking into account
> > the instance number, so it works with multiple instances).
>
> OK, so shall I remove it from bindings then?
I think so, as it is not needed.
What is still missing (contrary to the Closes tag) is the mapping from
clock IDs to actual clock outputs.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-08-21 8:33 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-17 9:08 [PATCH v2 0/3] Fix Versa3 clock mapping Biju Das
2023-08-17 9:08 ` [PATCH v2 1/3] dt-bindings: clock: versaclock3: Document clock-output-names Biju Das
2023-08-19 8:30 ` Conor Dooley
2023-08-21 8:00 ` Geert Uytterhoeven
2023-08-21 8:10 ` Biju Das
2023-08-21 8:32 ` Geert Uytterhoeven
2023-08-17 9:08 ` [PATCH v2 2/3] clk: vc3: Fix output clock mapping Biju Das
2023-08-17 9:51 ` Geert Uytterhoeven
2023-08-17 10:20 ` Biju Das
2023-08-17 9:08 ` [PATCH v2 3/3] arm64: dts: renesas: rz-smarc-common: Use versa3 clk for audio mclk Biju Das
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.