[PATCH v5 0/4] Fix Versa3 clock mapping

[PATCH v5 0/4] 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.

Also added a fix for 64 by 64 division.

v4->v5:
 * Added description for #clock-cells property for clock mapping.
 * Updated commit header and description to reflect this change.
 * Dropped fixes tag.
 * Retained Ack tag from Conor and Krzysztof as it is trivial change.
v3->v4:
 * Dropped clock-output-names as there is no validation for it and people
   can get it wrong.
 * Updated commit header, description and example to reflect this change
 * Retained Ack tag from Conor and Krzysztof as it is trivial change.
 * Used clamped value for rate calculation in vc3_pll_round_rate().
v2->v3:
 * Dropped dts patch and added fix for 64 byte division to this patch
   series.
 * Added Rb tag from Geert for patch#3
 * Added a patch to make vc3_clk_mux enum values depend on vc3_clk enum
   values.
v1->v2:
 * Updated binding commit description to make it clear it fixes
   "assigned-clock-rates" in the example based on 5P35023 datasheet.

Biju Das (4):
  dt-bindings: clock: versaclock3: Add description for #clock-cells
    property
  clk: vc3: Fix 64 by 64 division
  clk: vc3: Fix output clock mapping
  clk: vc3: Make vc3_clk_mux enum values based on vc3_clk enum values

 .../bindings/clock/renesas,5p35023.yaml       | 11 ++-
 drivers/clk/clk-versaclock3.c                 | 81 +++++++++----------
 2 files changed, 47 insertions(+), 45 deletions(-)

-- 
2.25.1

[PATCH v5 1/4] dt-bindings: clock: versaclock3: Add description for #clock-cells property

[Biju Das]

Add description for "#clock-cells" property to map indexes to the clock
output in the Table 3. ("Output Source") in the 5P35023 datasheet
(ie: {REF,SE1,SE2,SE3,DIFF1,DIFF2}. Also update the "assigned-clock-rates"
in the example.

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>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v4->v5:
 * Added description for "#clock-cells" property to map indexes to the
   clock output.
 * Updated commit header and description to reflect this change.
 * Dropped fixes tag.
 * Retained Ack tag from Conor and Krzysztof as it is trivial change.
v3->v4:
 * Dropped clock-output-names as there is no validation for it and people
   can get it wrong.
 * Updated commit header, description and example to reflect this change
 * Retained Ack tag from Conor and Krzysztof as it is trivial change.
v2->v3:
 * No change.
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    | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
index 839648e753d4..42b6f80613f3 100644
--- a/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
+++ b/Documentation/devicetree/bindings/clock/renesas,5p35023.yaml
@@ -37,6 +37,9 @@ properties:
     maxItems: 1
 
   '#clock-cells':
+    description:
+      The index in the assigned-clocks is mapped to the output clock as below
+      0 - REF, 1 - SE1, 2 - SE2, 3 - SE3, 4 - DIFF1, 5 - DIFF2.
     const: 1
 
   clocks:
@@ -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
@@ -79,8 +82,8 @@ examples:
             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

[PATCH v5 2/4] clk: vc3: Fix 64 by 64 division

[Biju Das]

Fix the below cocci warnings by replacing do_div()->div64_ul() and
bound the result with a max value of U16_MAX.

cocci warnings:
	drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a
	64-by-32 division, please consider using div64_ul instead.

Reported-by: Julia Lawall <julia.lawall@inria.fr>
Closes: https://lore.kernel.org/r/202307270841.yr5HxYIl-lkp@intel.com/
Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4->v5:
 * No change.
v3->v4:
 * Used clamped value for rate calculation in vc3_pll_round_rate().
v2->v3:
 * Added to this patch series.
v1->v2:
 * Added fixes tag.
---
 drivers/clk/clk-versaclock3.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
index 7ab2447bd203..b1a94db1f3c9 100644
--- a/drivers/clk/clk-versaclock3.c
+++ b/drivers/clk/clk-versaclock3.c
@@ -401,11 +401,10 @@ static long vc3_pll_round_rate(struct clk_hw *hw, unsigned long rate,
 		/* Determine best fractional part, which is 16 bit wide */
 		div_frc = rate % *parent_rate;
 		div_frc *= BIT(16) - 1;
-		do_div(div_frc, *parent_rate);
 
-		vc3->div_frc = (u32)div_frc;
+		vc3->div_frc = min_t(u64, div64_ul(div_frc, *parent_rate), U16_MAX);
 		rate = (*parent_rate *
-			(vc3->div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16);
+			(vc3->div_int * VC3_2_POW_16 + vc3->div_frc) / VC3_2_POW_16);
 	} else {
 		rate = *parent_rate * vc3->div_int;
 	}
-- 
2.25.1

[PATCH v5 3/4] 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>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v4->v5:
 * No change.
v3->v4:
 * No change.
v2->v3:
 * Added Rb tag from Geert.
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 b1a94db1f3c9..157cf510b23a 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

[PATCH v5 4/4] clk: vc3: Make vc3_clk_mux enum values based on vc3_clk enum values

[Biju Das]

Make vc3_clk_mux enum values depend upon vc3_clk enum values to avoid
any accidental breakage in the future.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v4->v5:
 * No change.
v3->v4:
 * No change.
v3:
 * New patch.
---
 drivers/clk/clk-versaclock3.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-versaclock3.c b/drivers/clk/clk-versaclock3.c
index 157cf510b23a..3d7de355f8f6 100644
--- a/drivers/clk/clk-versaclock3.c
+++ b/drivers/clk/clk-versaclock3.c
@@ -118,14 +118,6 @@ enum vc3_div {
 	VC3_DIV5,
 };
 
-enum vc3_clk_mux {
-	VC3_SE1_MUX,
-	VC3_SE2_MUX,
-	VC3_SE3_MUX,
-	VC3_DIFF1_MUX,
-	VC3_DIFF2_MUX,
-};
-
 enum vc3_clk {
 	VC3_REF,
 	VC3_SE1,
@@ -135,6 +127,14 @@ enum vc3_clk {
 	VC3_DIFF2,
 };
 
+enum vc3_clk_mux {
+	VC3_SE1_MUX = VC3_SE1 - 1,
+	VC3_SE2_MUX = VC3_SE2 - 1,
+	VC3_SE3_MUX = VC3_SE3 - 1,
+	VC3_DIFF1_MUX = VC3_DIFF1 - 1,
+	VC3_DIFF2_MUX = VC3_DIFF2 - 1,
+};
+
 struct vc3_clk_data {
 	u8 offs;
 	u8 bitmsk;
-- 
2.25.1

RE: [PATCH v5 0/4] Fix Versa3 clock mapping

[Biju Das]

Hi All,

Gentle ping.

Cheers,
Biju

> Subject: [PATCH v5 0/4] Fix Versa3 clock mapping
> 
> 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.
> 
> Also added a fix for 64 by 64 division.
> 
> v4->v5:
>  * Added description for #clock-cells property for clock mapping.
>  * Updated commit header and description to reflect this change.
>  * Dropped fixes tag.
>  * Retained Ack tag from Conor and Krzysztof as it is trivial change.
> v3->v4:
>  * Dropped clock-output-names as there is no validation for it and people
>    can get it wrong.
>  * Updated commit header, description and example to reflect this change
>  * Retained Ack tag from Conor and Krzysztof as it is trivial change.
>  * Used clamped value for rate calculation in vc3_pll_round_rate().
> v2->v3:
>  * Dropped dts patch and added fix for 64 byte division to this patch
>    series.
>  * Added Rb tag from Geert for patch#3
>  * Added a patch to make vc3_clk_mux enum values depend on vc3_clk enum
>    values.
> v1->v2:
>  * Updated binding commit description to make it clear it fixes
>    "assigned-clock-rates" in the example based on 5P35023 datasheet.
> 
> Biju Das (4):
>   dt-bindings: clock: versaclock3: Add description for #clock-cells
>     property
>   clk: vc3: Fix 64 by 64 division
>   clk: vc3: Fix output clock mapping
>   clk: vc3: Make vc3_clk_mux enum values based on vc3_clk enum values
> 
>  .../bindings/clock/renesas,5p35023.yaml       | 11 ++-
>  drivers/clk/clk-versaclock3.c                 | 81 +++++++++----------
>  2 files changed, 47 insertions(+), 45 deletions(-)
> 
> --
> 2.25.1

Re: [PATCH v5 1/4] dt-bindings: clock: versaclock3: Add description for #clock-cells property

[Geert Uytterhoeven]

On Thu, Aug 24, 2023 at 12:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add description for "#clock-cells" property to map indexes to the clock
> output in the Table 3. ("Output Source") in the 5P35023 datasheet
> (ie: {REF,SE1,SE2,SE3,DIFF1,DIFF2}. Also update the "assigned-clock-rates"
> in the example.
>
> 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>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> v4->v5:
>  * Added description for "#clock-cells" property to map indexes to the
>    clock output.
>  * Updated commit header and description to reflect this change.
>  * Dropped fixes tag.
>  * Retained Ack tag from Conor and Krzysztof as it is trivial change.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v5 4/4] clk: vc3: Make vc3_clk_mux enum values based on vc3_clk enum values

[Geert Uytterhoeven]

On Thu, Aug 24, 2023 at 12:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Make vc3_clk_mux enum values depend upon vc3_clk enum values to avoid
> any accidental breakage in the future.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v5 0/4] Fix Versa3 clock mapping

[Geert Uytterhoeven]

Hi Stephen,

On Wed, Sep 6, 2023 at 11:09 PM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Biju Das (2023-09-06 03:05:18)
> > Gentle ping.
>
> I'm waiting for Geert to review. I think we need to merge this soon to
> fix problems introduced this merge window?

Sorry, I didn't know you were waiting on my reviews (FWIW, I'm not
the maintainer... Oh, scripts/get_maintainer.pl disagrees, due to
the overzealous "renesas," wildcard, now matching all non-SoC devices
produced by companies acquired by Renesas recently :-(

I did review "[PATCH v5 3/4] clk: vc3: Fix output clock mapping" before,
which is the one I care about the most, as it is a hard dependency for
DT changes to be queued...

Thanks!

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

Re: [PATCH v5 1/4] dt-bindings: clock: versaclock3: Add description for #clock-cells property

[Stephen Boyd]

Quoting Biju Das (2023-08-24 03:48:09)
> Add description for "#clock-cells" property to map indexes to the clock
> output in the Table 3. ("Output Source") in the 5P35023 datasheet
> (ie: {REF,SE1,SE2,SE3,DIFF1,DIFF2}. Also update the "assigned-clock-rates"
> in the example.
> 
> 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>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---

Applied to clk-fixes

Re: [PATCH v5 2/4] clk: vc3: Fix 64 by 64 division

[Stephen Boyd]

Quoting Biju Das (2023-08-24 03:48:10)
> Fix the below cocci warnings by replacing do_div()->div64_ul() and
> bound the result with a max value of U16_MAX.
> 
> cocci warnings:
>         drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a
>         64-by-32 division, please consider using div64_ul instead.
> 
> Reported-by: Julia Lawall <julia.lawall@inria.fr>
> Closes: https://lore.kernel.org/r/202307270841.yr5HxYIl-lkp@intel.com/
> Fixes: 6e9aff555db7 ("clk: Add support for versa3 clock driver")
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---

Applied to clk-fixes

Re: [PATCH v5 3/4] clk: vc3: Fix output clock mapping

[Stephen Boyd]

Quoting Biju Das (2023-08-24 03:48:11)
> 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>
> ---

Applied to clk-fixes

Re: [PATCH v5 4/4] clk: vc3: Make vc3_clk_mux enum values based on vc3_clk enum values

[Stephen Boyd]

Quoting Biju Das (2023-08-24 03:48:12)
> Make vc3_clk_mux enum values depend upon vc3_clk enum values to avoid
> any accidental breakage in the future.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---

Applied to clk-fixes

Re: [PATCH v5 2/4] clk: vc3: Fix 64 by 64 division

[Andy Shevchenko]

On Thu, Aug 24, 2023 at 11:48:10AM +0100, Biju Das wrote:
> Fix the below cocci warnings by replacing do_div()->div64_ul() and
> bound the result with a max value of U16_MAX.
> 
> cocci warnings:
> 	drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a
> 	64-by-32 division, please consider using div64_ul instead.

It's nice, but there is a room for a couple of improvements. See below.

...

>  		/* Determine best fractional part, which is 16 bit wide */
>  		div_frc = rate % *parent_rate;
>  		div_frc *= BIT(16) - 1;
> -		do_div(div_frc, *parent_rate);
>  
> -		vc3->div_frc = (u32)div_frc;
> +		vc3->div_frc = min_t(u64, div64_ul(div_frc, *parent_rate), U16_MAX);

First of all, as Linus Torvalds pointed out [1] min_t() is often used as
a shortcut for clamp(). Second one, the BIT(16) - 1 is specifically used
as the value related to the bits in the hardware and u16 is a software
type that coincidentially has the same maximum as the above mentioned
bitfield.

That said, here this should be clamped to the [0 .. BIT(16) - 1] range.

Since the patch is applied perhaps you can cook a followup.

To everyone the message is simple: try to not use typed version of min()
and clamp() at all.

>  		rate = (*parent_rate *
> -			(vc3->div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16);
> +			(vc3->div_int * VC3_2_POW_16 + vc3->div_frc) / VC3_2_POW_16);

[1]: https://lore.kernel.org/lkml/CAHk-=whwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@mail.gmail.com/

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v5 2/4] clk: vc3: Fix 64 by 64 division

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v5 2/4] clk: vc3: Fix 64 by 64 division
> 
> On Thu, Aug 24, 2023 at 11:48:10AM +0100, Biju Das wrote:
> > Fix the below cocci warnings by replacing do_div()->div64_ul() and
> > bound the result with a max value of U16_MAX.
> >
> > cocci warnings:
> > 	drivers/clk/clk-versaclock3.c:404:2-8: WARNING: do_div() does a
> > 	64-by-32 division, please consider using div64_ul instead.
> 
> It's nice, but there is a room for a couple of improvements. See below.

Ok.

> 
> ...
> 
> >  		/* Determine best fractional part, which is 16 bit wide */
> >  		div_frc = rate % *parent_rate;
> >  		div_frc *= BIT(16) - 1;
> > -		do_div(div_frc, *parent_rate);
> >
> > -		vc3->div_frc = (u32)div_frc;
> > +		vc3->div_frc = min_t(u64, div64_ul(div_frc, *parent_rate),
> > +U16_MAX);

Ok, Will send follow up patch using clamp().

vc3->div_frc = clamp(div64_ul(div_frc, *parent_rate), 0, BIT(16) - 1);

Cheers,
Biju

> 
> First of all, as Linus Torvalds pointed out [1] min_t() is often used as a
> shortcut for clamp(). Second one, the BIT(16) - 1 is specifically used as
> the value related to the bits in the hardware and u16 is a software type
> that coincidentially has the same maximum as the above mentioned bitfield.
> 
> That said, here this should be clamped to the [0 .. BIT(16) - 1] range.
> 
> Since the patch is applied perhaps you can cook a followup.
> 
> To everyone the message is simple: try to not use typed version of min()
> and clamp() at all.

> 
> >  		rate = (*parent_rate *
> > -			(vc3->div_int * VC3_2_POW_16 + div_frc) / VC3_2_POW_16);
> > +			(vc3->div_int * VC3_2_POW_16 + vc3->div_frc) /
> VC3_2_POW_16);
>