[PATCH RESEND 0/2] update for versal net platform

[PATCH RESEND 0/2] update for versal net platform

[Jay Buddhabhatti]

Update clock driver to support for Versal NET platforms.
Versal Net is a new AMD/Xilinx  SoC.

Jay Buddhabhatti (2):
  drivers: clk: zynqmp: calculate closest mux rate
  drivers: clk: zynqmp: update divider round rate logic

 drivers/clk/zynqmp/clk-mux-zynqmp.c |  2 +-
 drivers/clk/zynqmp/divider.c        | 70 +++++------------------------
 2 files changed, 11 insertions(+), 61 deletions(-)

-- 
2.17.1

[PATCH RESEND 1/2] drivers: clk: zynqmp: calculate closest mux rate

[Jay Buddhabhatti]

Currently zynqmp clock driver is not calculating closest mux rate and
because of that Linux is not setting proper frequency for CPU and
not able to set given frequency for dynamic frequency scaling.

E.g., In current logic initial acpu clock parent and frequency as below
apll1                  0    0    0  2199999978    0     0  50000      Y
    acpu0_mux          0    0    0  2199999978    0     0  50000      Y
        acpu0_idiv1    0    0    0  2199999978    0     0  50000      Y
            acpu0      0    0    0  2199999978    0     0  50000      Y

After changing acpu frequency to 549999994 Hz using CPU freq scaling its
selecting incorrect parent which is not closest frequency.
rpll_to_xpd            0    0    0  1599999984    0     0  50000      Y
    acpu0_mux          0    0    0  1599999984    0     0  50000      Y
        acpu0_div1     0    0    0   533333328    0     0  50000      Y
            acpu0      0    0    0   533333328    0     0  50000      Y

Parent should remain same since 549999994 = 2199999978 / 4.

So use __clk_mux_determine_rate_closest() generic function to calculate
closest rate for mux clock. After this change its selecting correct
parent and correct clock rate.
apll1                  0    0    0  2199999978    0     0  50000      Y
    acpu0_mux          0    0    0  2199999978    0     0  50000      Y
        acpu0_div1     0    0    0   549999995    0     0  50000      Y
            acpu0      0    0    0   549999995    0     0  50000      Y

Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
---
 drivers/clk/zynqmp/clk-mux-zynqmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/zynqmp/clk-mux-zynqmp.c b/drivers/clk/zynqmp/clk-mux-zynqmp.c
index 60359333f26d..9b5d3050b742 100644
--- a/drivers/clk/zynqmp/clk-mux-zynqmp.c
+++ b/drivers/clk/zynqmp/clk-mux-zynqmp.c
@@ -89,7 +89,7 @@ static int zynqmp_clk_mux_set_parent(struct clk_hw *hw, u8 index)
 static const struct clk_ops zynqmp_clk_mux_ops = {
 	.get_parent = zynqmp_clk_mux_get_parent,
 	.set_parent = zynqmp_clk_mux_set_parent,
-	.determine_rate = __clk_mux_determine_rate,
+	.determine_rate = __clk_mux_determine_rate_closest,
 };
 
 static const struct clk_ops zynqmp_clk_mux_ro_ops = {
-- 
2.17.1

Re: [PATCH RESEND 1/2] drivers: clk: zynqmp: calculate closest mux rate

[Stephen Boyd]

Quoting Jay Buddhabhatti (2023-10-16 04:30:01)
> Currently zynqmp clock driver is not calculating closest mux rate and
> because of that Linux is not setting proper frequency for CPU and
> not able to set given frequency for dynamic frequency scaling.
> 
> E.g., In current logic initial acpu clock parent and frequency as below
> apll1                  0    0    0  2199999978    0     0  50000      Y
>     acpu0_mux          0    0    0  2199999978    0     0  50000      Y
>         acpu0_idiv1    0    0    0  2199999978    0     0  50000      Y
>             acpu0      0    0    0  2199999978    0     0  50000      Y
> 
> After changing acpu frequency to 549999994 Hz using CPU freq scaling its
> selecting incorrect parent which is not closest frequency.
> rpll_to_xpd            0    0    0  1599999984    0     0  50000      Y
>     acpu0_mux          0    0    0  1599999984    0     0  50000      Y
>         acpu0_div1     0    0    0   533333328    0     0  50000      Y
>             acpu0      0    0    0   533333328    0     0  50000      Y
> 
> Parent should remain same since 549999994 = 2199999978 / 4.
> 
> So use __clk_mux_determine_rate_closest() generic function to calculate
> closest rate for mux clock. After this change its selecting correct
> parent and correct clock rate.
> apll1                  0    0    0  2199999978    0     0  50000      Y
>     acpu0_mux          0    0    0  2199999978    0     0  50000      Y
>         acpu0_div1     0    0    0   549999995    0     0  50000      Y
>             acpu0      0    0    0   549999995    0     0  50000      Y
> 
> Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
> ---

Any Fixes tag here?

RE: [PATCH RESEND 1/2] drivers: clk: zynqmp: calculate closest mux rate

[Buddhabhatti, Jay]

Hi Stephen,

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Tuesday, October 24, 2023 9:09 AM
> To: Buddhabhatti, Jay <jay.buddhabhatti@amd.com>; Simek, Michal
> <michal.simek@amd.com>; mturquette@baylibre.com
> Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Buddhabhatti, Jay <jay.buddhabhatti@amd.com>
> Subject: Re: [PATCH RESEND 1/2] drivers: clk: zynqmp: calculate closest mux
> rate
> 
> Quoting Jay Buddhabhatti (2023-10-16 04:30:01)
> > Currently zynqmp clock driver is not calculating closest mux rate and
> > because of that Linux is not setting proper frequency for CPU and not
> > able to set given frequency for dynamic frequency scaling.
> >
> > E.g., In current logic initial acpu clock parent and frequency as below
> > apll1                  0    0    0  2199999978    0     0  50000      Y
> >     acpu0_mux          0    0    0  2199999978    0     0  50000      Y
> >         acpu0_idiv1    0    0    0  2199999978    0     0  50000      Y
> >             acpu0      0    0    0  2199999978    0     0  50000      Y
> >
> > After changing acpu frequency to 549999994 Hz using CPU freq scaling
> > its selecting incorrect parent which is not closest frequency.
> > rpll_to_xpd            0    0    0  1599999984    0     0  50000      Y
> >     acpu0_mux          0    0    0  1599999984    0     0  50000      Y
> >         acpu0_div1     0    0    0   533333328    0     0  50000      Y
> >             acpu0      0    0    0   533333328    0     0  50000      Y
> >
> > Parent should remain same since 549999994 = 2199999978 / 4.
> >
> > So use __clk_mux_determine_rate_closest() generic function to
> > calculate closest rate for mux clock. After this change its selecting
> > correct parent and correct clock rate.
> > apll1                  0    0    0  2199999978    0     0  50000      Y
> >     acpu0_mux          0    0    0  2199999978    0     0  50000      Y
> >         acpu0_div1     0    0    0   549999995    0     0  50000      Y
> >             acpu0      0    0    0   549999995    0     0  50000      Y
> >
> > Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
> > ---
> 
> Any Fixes tag here?
[Jay] Sure I will add fixes tag.

Thanks,
Jay

[PATCH RESEND 2/2] drivers: clk: zynqmp: update divider round rate logic

[Jay Buddhabhatti]

Currently zynqmp divider round rate is considering single parent and
calculating rate and parent rate accordingly. But if divider clock flag
is set to SET_RATE_PARENT then its not trying to traverse through all
parent rate and not selecting best parent rate from that. So use common
divider_round_rate() which is traversing through all clock parents and
its rate and calculating proper parent rate.

Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
---
 drivers/clk/zynqmp/divider.c | 70 ++++++------------------------------
 1 file changed, 10 insertions(+), 60 deletions(-)

diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
index 33a3b2a22659..a42c183d7e5d 100644
--- a/drivers/clk/zynqmp/divider.c
+++ b/drivers/clk/zynqmp/divider.c
@@ -110,52 +110,6 @@ static unsigned long zynqmp_clk_divider_recalc_rate(struct clk_hw *hw,
 	return DIV_ROUND_UP_ULL(parent_rate, value);
 }
 
-static void zynqmp_get_divider2_val(struct clk_hw *hw,
-				    unsigned long rate,
-				    struct zynqmp_clk_divider *divider,
-				    u32 *bestdiv)
-{
-	int div1;
-	int div2;
-	long error = LONG_MAX;
-	unsigned long div1_prate;
-	struct clk_hw *div1_parent_hw;
-	struct zynqmp_clk_divider *pdivider;
-	struct clk_hw *div2_parent_hw = clk_hw_get_parent(hw);
-
-	if (!div2_parent_hw)
-		return;
-
-	pdivider = to_zynqmp_clk_divider(div2_parent_hw);
-	if (!pdivider)
-		return;
-
-	div1_parent_hw = clk_hw_get_parent(div2_parent_hw);
-	if (!div1_parent_hw)
-		return;
-
-	div1_prate = clk_hw_get_rate(div1_parent_hw);
-	*bestdiv = 1;
-	for (div1 = 1; div1 <= pdivider->max_div;) {
-		for (div2 = 1; div2 <= divider->max_div;) {
-			long new_error = ((div1_prate / div1) / div2) - rate;
-
-			if (abs(new_error) < abs(error)) {
-				*bestdiv = div2;
-				error = new_error;
-			}
-			if (divider->flags & CLK_DIVIDER_POWER_OF_TWO)
-				div2 = div2 << 1;
-			else
-				div2++;
-		}
-		if (pdivider->flags & CLK_DIVIDER_POWER_OF_TWO)
-			div1 = div1 << 1;
-		else
-			div1++;
-	}
-}
-
 /**
  * zynqmp_clk_divider_round_rate() - Round rate of divider clock
  * @hw:			handle between common and hardware-specific interfaces
@@ -174,6 +128,8 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
 	u32 div_type = divider->div_type;
 	u32 bestdiv;
 	int ret;
+	u8 width = 0;
+	u16 max;
 
 	/* if read only, just return current value */
 	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
@@ -193,23 +149,17 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
 		return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
 	}
 
-	bestdiv = zynqmp_divider_get_val(*prate, rate, divider->flags);
-
-	/*
-	 * In case of two divisors, compute best divider values and return
-	 * divider2 value based on compute value. div1 will  be automatically
-	 * set to optimum based on required total divider value.
-	 */
-	if (div_type == TYPE_DIV2 &&
-	    (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) {
-		zynqmp_get_divider2_val(hw, rate, divider, &bestdiv);
+	max = divider->max_div;
+	while (max != 0) {
+		if ((max & 1) == 1)
+			width++;
+		max = max >> 1;
 	}
 
-	if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && divider->is_frac)
-		bestdiv = rate % *prate ? 1 : bestdiv;
+	rate = divider_round_rate(hw, rate, prate, NULL, width, divider->flags);
 
-	bestdiv = min_t(u32, bestdiv, divider->max_div);
-	*prate = rate * bestdiv;
+	if (divider->is_frac && (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && (rate % *prate))
+		*prate = rate;
 
 	return rate;
 }
-- 
2.17.1

Re: [PATCH RESEND 2/2] drivers: clk: zynqmp: update divider round rate logic

[Stephen Boyd]

Quoting Jay Buddhabhatti (2023-10-16 04:30:02)
> Currently zynqmp divider round rate is considering single parent and
> calculating rate and parent rate accordingly. But if divider clock flag
> is set to SET_RATE_PARENT then its not trying to traverse through all
> parent rate and not selecting best parent rate from that. So use common
> divider_round_rate() which is traversing through all clock parents and
> its rate and calculating proper parent rate.
> 
> Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
> ---
>  drivers/clk/zynqmp/divider.c | 70 ++++++------------------------------
>  1 file changed, 10 insertions(+), 60 deletions(-)

Can't argue against removing that many lines!

> 
> diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> index 33a3b2a22659..a42c183d7e5d 100644
> --- a/drivers/clk/zynqmp/divider.c
> +++ b/drivers/clk/zynqmp/divider.c
> @@ -193,23 +149,17 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
>                 return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
>         }
>  
> -       bestdiv = zynqmp_divider_get_val(*prate, rate, divider->flags);
> -
> -       /*
> -        * In case of two divisors, compute best divider values and return
> -        * divider2 value based on compute value. div1 will  be automatically
> -        * set to optimum based on required total divider value.
> -        */
> -       if (div_type == TYPE_DIV2 &&
> -           (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) {
> -               zynqmp_get_divider2_val(hw, rate, divider, &bestdiv);
> +       max = divider->max_div;
> +       while (max != 0) {
> +               if ((max & 1) == 1)
> +                       width++;
> +               max = max >> 1;

Is this ffs()?

>         }
>  
> -       if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && divider->is_frac)
> -               bestdiv = rate % *prate ? 1 : bestdiv;
> +       rate = divider_round_rate(hw, rate, prate, NULL, width, divider->flags);
>  
> -       bestdiv = min_t(u32, bestdiv, divider->max_div);
> -       *prate = rate * bestdiv;
> +       if (divider->is_frac && (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && (rate % *prate))
> +               *prate = rate;

RE: [PATCH RESEND 2/2] drivers: clk: zynqmp: update divider round rate logic

[Buddhabhatti, Jay]

Hi Stephen,

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Tuesday, October 24, 2023 9:08 AM
> To: Buddhabhatti, Jay <jay.buddhabhatti@amd.com>; Simek, Michal
> <michal.simek@amd.com>; mturquette@baylibre.com
> Cc: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; Buddhabhatti, Jay <jay.buddhabhatti@amd.com>
> Subject: Re: [PATCH RESEND 2/2] drivers: clk: zynqmp: update divider round
> rate logic
> 
> Quoting Jay Buddhabhatti (2023-10-16 04:30:02)
> > Currently zynqmp divider round rate is considering single parent and
> > calculating rate and parent rate accordingly. But if divider clock
> > flag is set to SET_RATE_PARENT then its not trying to traverse through
> > all parent rate and not selecting best parent rate from that. So use
> > common
> > divider_round_rate() which is traversing through all clock parents and
> > its rate and calculating proper parent rate.
> >
> > Signed-off-by: Jay Buddhabhatti <jay.buddhabhatti@amd.com>
> > ---
> >  drivers/clk/zynqmp/divider.c | 70
> > ++++++------------------------------
> >  1 file changed, 10 insertions(+), 60 deletions(-)
> 
> Can't argue against removing that many lines!
> 
> >
> > diff --git a/drivers/clk/zynqmp/divider.c
> > b/drivers/clk/zynqmp/divider.c index 33a3b2a22659..a42c183d7e5d 100644
> > --- a/drivers/clk/zynqmp/divider.c
> > +++ b/drivers/clk/zynqmp/divider.c
> > @@ -193,23 +149,17 @@ static long zynqmp_clk_divider_round_rate(struct
> clk_hw *hw,
> >                 return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
> >         }
> >
> > -       bestdiv = zynqmp_divider_get_val(*prate, rate, divider->flags);
> > -
> > -       /*
> > -        * In case of two divisors, compute best divider values and return
> > -        * divider2 value based on compute value. div1 will  be automatically
> > -        * set to optimum based on required total divider value.
> > -        */
> > -       if (div_type == TYPE_DIV2 &&
> > -           (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)) {
> > -               zynqmp_get_divider2_val(hw, rate, divider, &bestdiv);
> > +       max = divider->max_div;
> > +       while (max != 0) {
> > +               if ((max & 1) == 1)
> > +                       width++;
> > +               max = max >> 1;
> 
> Is this ffs()?
[Jay] We need last set bit to get max width. I will use fls() to get most significant set bit for this. Thanks for suggestion.

Thanks,
Jay
> 
> >         }
> >
> > -       if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) && divider->is_frac)
> > -               bestdiv = rate % *prate ? 1 : bestdiv;
> > +       rate = divider_round_rate(hw, rate, prate, NULL, width,
> > + divider->flags);
> >
> > -       bestdiv = min_t(u32, bestdiv, divider->max_div);
> > -       *prate = rate * bestdiv;
> > +       if (divider->is_frac && (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT)
> && (rate % *prate))
> > +               *prate = rate;