[v2] clk: at91: fix programmable clock for sama5d2

Commit Message

Nicolas Ferre March 18, 2019, 10:50 a.m. UTC

From: Matthias Wieloch <[e-mail]>

The prescaler formula of the programmable clock has changed for sama5d2. Update
the driver accordingly.

Fixes: a2038077de9a ("clk: at91: add sama5d2 PMC driver")
Cc: <[e-mail]> # v4.20+
Signed-off-by: Nicolas Ferre <[e-mail]>
[[e-mail]: adapt the prescaler range,
		fix clk_programmable_recalc_rate, split patch]
Signed-off-by: Matthias Wieloch <[e-mail]>
Signed-off-by: Alexandre Belloni <[e-mail]>
---
v2: adapt to v5.1-rc1
    remove unneeded sentence about DT in commit message

Stephen,

I think it would be good to see this fix going upstream during v5.1-rc phase.

Best regards,
  Nicolas


 drivers/clk/at91/clk-programmable.c | 57 ++++++++++++++++++++++-------
 drivers/clk/at91/pmc.h              |  2 +
 drivers/clk/at91/sama5d2.c          | 10 ++++-
 3 files changed, 54 insertions(+), 15 deletions(-)

Comments

Stephen Boyd March 18, 2019, 7:54 p.m. UTC | #1

Quoting Nicolas Ferre (2019-03-18 03:50:45)
> From: Matthias Wieloch <[e-mail]>
> 
> The prescaler formula of the programmable clock has changed for sama5d2. Update
> the driver accordingly.
> 
> Fixes: a2038077de9a ("clk: at91: add sama5d2 PMC driver")
> Cc: <[e-mail]> # v4.20+
> Signed-off-by: Nicolas Ferre <[e-mail]>
> [[e-mail]: adapt the prescaler range,
>                 fix clk_programmable_recalc_rate, split patch]
> Signed-off-by: Matthias Wieloch <[e-mail]>
> Signed-off-by: Alexandre Belloni <[e-mail]>
> ---
> v2: adapt to v5.1-rc1
>     remove unneeded sentence about DT in commit message
> 
> Stephen,
> 
> I think it would be good to see this fix going upstream during v5.1-rc phase.

Ok. I can apply this clk-fixes. I presume that things are real bad and
it can't wait until v5.2?

> @@ -60,10 +68,18 @@ static int clk_programmable_determine_rate(struct clk_hw *hw,
>                         continue;
>  
>                 parent_rate = clk_hw_get_rate(parent);
> -               for (shift = 0; shift < PROG_PRES_MASK; shift++) {
> -                       tmp_rate = parent_rate >> shift;
> -                       if (tmp_rate <= req->rate)
> -                               break;
> +               if (layout->is_pres_direct) {
> +                       for (shift = 0; shift <= layout->pres_mask; shift++) {
> +                               tmp_rate = parent_rate / (shift + 1);
> +                               if (tmp_rate <= req->rate)
> +                                       break;
> +                       }
> +               } else {
> +                       for (shift = 0; shift < layout->pres_mask; shift++) {
> +                               tmp_rate = parent_rate >> shift;
> +                               if (tmp_rate <= req->rate)
> +                                       break;
> +                       }

This looks like a lot of copy paste when the if statement could have been
pulled into the for loop instead of duplicating the loops and
surrounding if condition check for tmp_rate.

>                 }
>  
>                 if (tmp_rate > req->rate)

Nicolas Ferre March 19, 2019, 8:28 a.m. UTC | #2

Stephen,

Thanks for the review

On 18/03/2019 at 20:54, Stephen Boyd wrote:
> Quoting Nicolas Ferre (2019-03-18 03:50:45)
>> From: Matthias Wieloch <[e-mail]>
>>
>> The prescaler formula of the programmable clock has changed for sama5d2. Update
>> the driver accordingly.
>>
>> Fixes: a2038077de9a ("clk: at91: add sama5d2 PMC driver")
>> Cc: <[e-mail]> # v4.20+
>> Signed-off-by: Nicolas Ferre <[e-mail]>
>> [[e-mail]: adapt the prescaler range,
>>                  fix clk_programmable_recalc_rate, split patch]
>> Signed-off-by: Matthias Wieloch <[e-mail]>
>> Signed-off-by: Alexandre Belloni <[e-mail]>
>> ---
>> v2: adapt to v5.1-rc1
>>      remove unneeded sentence about DT in commit message
>>
>> Stephen,
>>
>> I think it would be good to see this fix going upstream during v5.1-rc phase.
> 
> Ok. I can apply this clk-fixes. I presume that things are real bad and
> it can't wait until v5.2?

To be perfectly clear, it's not a regression.
But as we're at the very beginning of the '-rc' phase and as it's a bug, 
I was thinking about adding it now. But you to choose, no problem either 
way.

>> @@ -60,10 +68,18 @@ static int clk_programmable_determine_rate(struct clk_hw *hw,
>>                          continue;
>>   
>>                  parent_rate = clk_hw_get_rate(parent);
>> -               for (shift = 0; shift < PROG_PRES_MASK; shift++) {
>> -                       tmp_rate = parent_rate >> shift;
>> -                       if (tmp_rate <= req->rate)
>> -                               break;
>> +               if (layout->is_pres_direct) {
>> +                       for (shift = 0; shift <= layout->pres_mask; shift++) {
>> +                               tmp_rate = parent_rate / (shift + 1);
>> +                               if (tmp_rate <= req->rate)
>> +                                       break;
>> +                       }
>> +               } else {
>> +                       for (shift = 0; shift < layout->pres_mask; shift++) {
>> +                               tmp_rate = parent_rate >> shift;
>> +                               if (tmp_rate <= req->rate)
>> +                                       break;
>> +                       }
> 
> This looks like a lot of copy paste when the if statement could have been
> pulled into the for loop instead of duplicating the loops and
> surrounding if condition check for tmp_rate.

Stop condition of loops not being the same made me separate them instead 
of adding artificial test conditions for shift == layout->pres_mask. I'm 
not sure the other way around is more obvious then...

> 
>>                  }
>>   
>>                  if (tmp_rate > req->rate)

Alexandre Belloni March 19, 2019, 9:24 a.m. UTC | #3

On 19/03/2019 08:28:40+0000, Nicolas Ferre wrote:
> > Ok. I can apply this clk-fixes. I presume that things are real bad and
> > it can't wait until v5.2?
> 
> To be perfectly clear, it's not a regression.
> But as we're at the very beginning of the '-rc' phase and as it's a bug, 
> I was thinking about adding it now. But you to choose, no problem either 
> way.
> 
> >> @@ -60,10 +68,18 @@ static int clk_programmable_determine_rate(struct clk_hw *hw,
> >>                          continue;
> >>   
> >>                  parent_rate = clk_hw_get_rate(parent);
> >> -               for (shift = 0; shift < PROG_PRES_MASK; shift++) {
> >> -                       tmp_rate = parent_rate >> shift;
> >> -                       if (tmp_rate <= req->rate)
> >> -                               break;
> >> +               if (layout->is_pres_direct) {
> >> +                       for (shift = 0; shift <= layout->pres_mask; shift++) {
> >> +                               tmp_rate = parent_rate / (shift + 1);
> >> +                               if (tmp_rate <= req->rate)
> >> +                                       break;
> >> +                       }
> >> +               } else {
> >> +                       for (shift = 0; shift < layout->pres_mask; shift++) {
> >> +                               tmp_rate = parent_rate >> shift;
> >> +                               if (tmp_rate <= req->rate)
> >> +                                       break;
> >> +                       }
> > 
> > This looks like a lot of copy paste when the if statement could have been
> > pulled into the for loop instead of duplicating the loops and
> > surrounding if condition check for tmp_rate.
> 
> Stop condition of loops not being the same made me separate them instead 
> of adding artificial test conditions for shift == layout->pres_mask. I'm 
> not sure the other way around is more obvious then...
> 

I also tried different ways (e.g. setting up a different determine_rate
for the sama5d2) but this ended up being the more concise one.

Stephen Boyd March 19, 2019, 7:44 p.m. UTC | #4

Quoting Nicolas Ferre (2019-03-18 03:50:45)
> From: Matthias Wieloch <[e-mail]>
> 
> The prescaler formula of the programmable clock has changed for sama5d2. Update
> the driver accordingly.
> 
> Fixes: a2038077de9a ("clk: at91: add sama5d2 PMC driver")
> Cc: <[e-mail]> # v4.20+
> Signed-off-by: Nicolas Ferre <[e-mail]>
> [[e-mail]: adapt the prescaler range,
>                 fix clk_programmable_recalc_rate, split patch]
> Signed-off-by: Matthias Wieloch <[e-mail]>
> Signed-off-by: Alexandre Belloni <[e-mail]>
> ---

Applied to clk-fixes

Patch

diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c
index 89d6f3736dbf..f8edbb65eda3 100644
--- a/drivers/clk/at91/clk-programmable.c
+++ b/drivers/clk/at91/clk-programmable.c
@@ -20,8 +20,7 @@ 
 #define PROG_ID_MAX		7
 
 #define PROG_STATUS_MASK(id)	(1 << ((id) + 8))
-#define PROG_PRES_MASK		0x7
-#define PROG_PRES(layout, pckr)	((pckr >> layout->pres_shift) & PROG_PRES_MASK)
+#define PROG_PRES(layout, pckr)	((pckr >> layout->pres_shift) & layout->pres_mask)
 #define PROG_MAX_RM9200_CSS	3
 
 struct clk_programmable {
@@ -37,20 +36,29 @@  static unsigned long clk_programmable_recalc_rate(struct clk_hw *hw,
 						  unsigned long parent_rate)
 {
 	struct clk_programmable *prog = to_clk_programmable(hw);
+	const struct clk_programmable_layout *layout = prog->layout;
 	unsigned int pckr;
+	unsigned long rate;
 
 	regmap_read(prog->regmap, AT91_PMC_PCKR(prog->id), &pckr);
 
-	return parent_rate >> PROG_PRES(prog->layout, pckr);
+	if (layout->is_pres_direct)
+		rate = parent_rate / (PROG_PRES(layout, pckr) + 1);
+	else
+		rate = parent_rate >> PROG_PRES(layout, pckr);
+
+	return rate;
 }
 
 static int clk_programmable_determine_rate(struct clk_hw *hw,
 					   struct clk_rate_request *req)
 {
+	struct clk_programmable *prog = to_clk_programmable(hw);
+	const struct clk_programmable_layout *layout = prog->layout;
 	struct clk_hw *parent;
 	long best_rate = -EINVAL;
 	unsigned long parent_rate;
-	unsigned long tmp_rate;
+	unsigned long tmp_rate = 0;
 	int shift;
 	int i;
 
@@ -60,10 +68,18 @@  static int clk_programmable_determine_rate(struct clk_hw *hw,
 			continue;
 
 		parent_rate = clk_hw_get_rate(parent);
-		for (shift = 0; shift < PROG_PRES_MASK; shift++) {
-			tmp_rate = parent_rate >> shift;
-			if (tmp_rate <= req->rate)
-				break;
+		if (layout->is_pres_direct) {
+			for (shift = 0; shift <= layout->pres_mask; shift++) {
+				tmp_rate = parent_rate / (shift + 1);
+				if (tmp_rate <= req->rate)
+					break;
+			}
+		} else {
+			for (shift = 0; shift < layout->pres_mask; shift++) {
+				tmp_rate = parent_rate >> shift;
+				if (tmp_rate <= req->rate)
+					break;
+			}
 		}
 
 		if (tmp_rate > req->rate)
@@ -137,16 +153,23 @@  static int clk_programmable_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (!div)
 		return -EINVAL;
 
-	shift = fls(div) - 1;
+	if (layout->is_pres_direct) {
+		shift = div - 1;
 
-	if (div != (1 << shift))
-		return -EINVAL;
+		if (shift > layout->pres_mask)
+			return -EINVAL;
+	} else {
+		shift = fls(div) - 1;
 
-	if (shift >= PROG_PRES_MASK)
-		return -EINVAL;
+		if (div != (1 << shift))
+			return -EINVAL;
+
+		if (shift >= layout->pres_mask)
+			return -EINVAL;
+	}
 
 	regmap_update_bits(prog->regmap, AT91_PMC_PCKR(prog->id),
-			   PROG_PRES_MASK << layout->pres_shift,
+			   layout->pres_mask << layout->pres_shift,
 			   shift << layout->pres_shift);
 
 	return 0;
@@ -202,19 +225,25 @@  at91_clk_register_programmable(struct regmap *regmap,
 }
 
 const struct clk_programmable_layout at91rm9200_programmable_layout = {
+	.pres_mask = 0x7,
 	.pres_shift = 2,
 	.css_mask = 0x3,
 	.have_slck_mck = 0,
+	.is_pres_direct = 0,
 };
 
 const struct clk_programmable_layout at91sam9g45_programmable_layout = {
+	.pres_mask = 0x7,
 	.pres_shift = 2,
 	.css_mask = 0x3,
 	.have_slck_mck = 1,
+	.is_pres_direct = 0,
 };
 
 const struct clk_programmable_layout at91sam9x5_programmable_layout = {
+	.pres_mask = 0x7,
 	.pres_shift = 4,
 	.css_mask = 0x7,
 	.have_slck_mck = 0,
+	.is_pres_direct = 0,
 };
diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
index 672a79bda88c..a0e5ce9c9b9e 100644
--- a/drivers/clk/at91/pmc.h
+++ b/drivers/clk/at91/pmc.h
@@ -71,9 +71,11 @@  struct clk_pll_characteristics {
 };
 
 struct clk_programmable_layout {
+	u8 pres_mask;
 	u8 pres_shift;
 	u8 css_mask;
 	u8 have_slck_mck;
+	u8 is_pres_direct;
 };
 
 extern const struct clk_programmable_layout at91rm9200_programmable_layout;
diff --git a/drivers/clk/at91/sama5d2.c b/drivers/clk/at91/sama5d2.c
index 1f70cb164b06..81943fac4537 100644
--- a/drivers/clk/at91/sama5d2.c
+++ b/drivers/clk/at91/sama5d2.c
@@ -125,6 +125,14 @@  static const struct {
 	  .pll = true },
 };
 
+static const struct clk_programmable_layout sama5d2_programmable_layout = {
+	.pres_mask = 0xff,
+	.pres_shift = 4,
+	.css_mask = 0x7,
+	.have_slck_mck = 0,
+	.is_pres_direct = 1,
+};
+
 static void __init sama5d2_pmc_setup(struct device_node *np)
 {
 	struct clk_range range = CLK_RANGE(0, 0);
@@ -249,7 +257,7 @@  static void __init sama5d2_pmc_setup(struct device_node *np)
 
 		hw = at91_clk_register_programmable(regmap, name,
 						    parent_names, 6, i,
-						    &at91sam9x5_programmable_layout);
+						    &sama5d2_programmable_layout);
 		if (IS_ERR(hw))
 			goto err_free;
 	}