linux-clk.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] clk: at91: remove the checking of parent_name
@ 2020-06-25 10:09 Claudiu Beznea
  2020-06-25 10:09 ` [PATCH 2/2] clk: at91: main: do not continue if oscillators already prepared Claudiu Beznea
  2020-06-26 20:58 ` [PATCH 1/2] clk: at91: remove the checking of parent_name Alexandre Belloni
  0 siblings, 2 replies; 5+ messages in thread
From: Claudiu Beznea @ 2020-06-25 10:09 UTC (permalink / raw)
  To: mturquette, sboyd, nicolas.ferre, alexandre.belloni,
	ludovic.desroches, linux-kernel
  Cc: mturquette, bbrezillon, linux-clk, linux-arm-kernel, Claudiu Beznea

There is no need to check parent_name variable while assigning it to
init.parent_names. parent_name variable is already checked at
the beginning of at91_clk_register_sam9x5_peripheral() function.
Same thing with init.num_parents: it could only be 1.

Fixes: 6114067e437eb ("clk: at91: add PMC peripheral clocks")
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/clk/at91/clk-peripheral.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
index c2ab4860a2bf..4a0f40738fe3 100644
--- a/drivers/clk/at91/clk-peripheral.c
+++ b/drivers/clk/at91/clk-peripheral.c
@@ -111,8 +111,8 @@ at91_clk_register_peripheral(struct regmap *regmap, const char *name,
 
 	init.name = name;
 	init.ops = &peripheral_ops;
-	init.parent_names = (parent_name ? &parent_name : NULL);
-	init.num_parents = (parent_name ? 1 : 0);
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
 	init.flags = 0;
 
 	periph->id = id;
@@ -340,8 +340,8 @@ at91_clk_register_sam9x5_peripheral(struct regmap *regmap, spinlock_t *lock,
 
 	init.name = name;
 	init.ops = &sam9x5_peripheral_ops;
-	init.parent_names = (parent_name ? &parent_name : NULL);
-	init.num_parents = (parent_name ? 1 : 0);
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
 	init.flags = 0;
 
 	periph->id = id;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 2/2] clk: at91: main: do not continue if oscillators already prepared
  2020-06-25 10:09 [PATCH 1/2] clk: at91: remove the checking of parent_name Claudiu Beznea
@ 2020-06-25 10:09 ` Claudiu Beznea
  2020-06-26 21:03   ` Alexandre Belloni
  2020-06-26 20:58 ` [PATCH 1/2] clk: at91: remove the checking of parent_name Alexandre Belloni
  1 sibling, 1 reply; 5+ messages in thread
From: Claudiu Beznea @ 2020-06-25 10:09 UTC (permalink / raw)
  To: mturquette, sboyd, nicolas.ferre, alexandre.belloni,
	ludovic.desroches, linux-kernel
  Cc: mturquette, bbrezillon, linux-clk, linux-arm-kernel, Claudiu Beznea

Return in clk_main_osc_prepare()/clk_main_rc_osc_prepare() if
oscillators are already enabled.

Fixes: 27cb1c2083373 ("clk: at91: rework main clk implementation")
Fixes: 1bdf02326b71e ("clk: at91: make use of syscon/regmap internally")
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/clk/at91/clk-main.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c
index 37c22667e831..46b4d2131989 100644
--- a/drivers/clk/at91/clk-main.c
+++ b/drivers/clk/at91/clk-main.c
@@ -74,13 +74,11 @@ static int clk_main_osc_prepare(struct clk_hw *hw)
 	regmap_read(regmap, AT91_CKGR_MOR, &tmp);
 	tmp &= ~MOR_KEY_MASK;
 
-	if (tmp & AT91_PMC_OSCBYPASS)
+	if (tmp & (AT91_PMC_OSCBYPASS | AT91_PMC_MOSCEN))
 		return 0;
 
-	if (!(tmp & AT91_PMC_MOSCEN)) {
-		tmp |= AT91_PMC_MOSCEN | AT91_PMC_KEY;
-		regmap_write(regmap, AT91_CKGR_MOR, tmp);
-	}
+	tmp |= AT91_PMC_MOSCEN | AT91_PMC_KEY;
+	regmap_write(regmap, AT91_CKGR_MOR, tmp);
 
 	while (!clk_main_osc_ready(regmap))
 		cpu_relax();
@@ -186,10 +184,12 @@ static int clk_main_rc_osc_prepare(struct clk_hw *hw)
 
 	regmap_read(regmap, AT91_CKGR_MOR, &mor);
 
-	if (!(mor & AT91_PMC_MOSCRCEN))
-		regmap_update_bits(regmap, AT91_CKGR_MOR,
-				   MOR_KEY_MASK | AT91_PMC_MOSCRCEN,
-				   AT91_PMC_MOSCRCEN | AT91_PMC_KEY);
+	if (mor & AT91_PMC_MOSCRCEN)
+		return 0;
+
+	regmap_update_bits(regmap, AT91_CKGR_MOR,
+			   MOR_KEY_MASK | AT91_PMC_MOSCRCEN,
+			   AT91_PMC_MOSCRCEN | AT91_PMC_KEY);
 
 	while (!clk_main_rc_osc_ready(regmap))
 		cpu_relax();
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] clk: at91: remove the checking of parent_name
  2020-06-25 10:09 [PATCH 1/2] clk: at91: remove the checking of parent_name Claudiu Beznea
  2020-06-25 10:09 ` [PATCH 2/2] clk: at91: main: do not continue if oscillators already prepared Claudiu Beznea
@ 2020-06-26 20:58 ` Alexandre Belloni
  1 sibling, 0 replies; 5+ messages in thread
From: Alexandre Belloni @ 2020-06-26 20:58 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: mturquette, sboyd, nicolas.ferre, ludovic.desroches,
	linux-kernel, mturquette, bbrezillon, linux-clk,
	linux-arm-kernel

On 25/06/2020 13:09:27+0300, Claudiu Beznea wrote:
> There is no need to check parent_name variable while assigning it to
> init.parent_names. parent_name variable is already checked at
> the beginning of at91_clk_register_sam9x5_peripheral() function.
> Same thing with init.num_parents: it could only be 1.
> 
> Fixes: 6114067e437eb ("clk: at91: add PMC peripheral clocks")
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
>  drivers/clk/at91/clk-peripheral.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
> index c2ab4860a2bf..4a0f40738fe3 100644
> --- a/drivers/clk/at91/clk-peripheral.c
> +++ b/drivers/clk/at91/clk-peripheral.c
> @@ -111,8 +111,8 @@ at91_clk_register_peripheral(struct regmap *regmap, const char *name,
>  
>  	init.name = name;
>  	init.ops = &peripheral_ops;
> -	init.parent_names = (parent_name ? &parent_name : NULL);
> -	init.num_parents = (parent_name ? 1 : 0);
> +	init.parent_names = &parent_name;
> +	init.num_parents = 1;
>  	init.flags = 0;
>  
>  	periph->id = id;
> @@ -340,8 +340,8 @@ at91_clk_register_sam9x5_peripheral(struct regmap *regmap, spinlock_t *lock,
>  
>  	init.name = name;
>  	init.ops = &sam9x5_peripheral_ops;
> -	init.parent_names = (parent_name ? &parent_name : NULL);
> -	init.num_parents = (parent_name ? 1 : 0);
> +	init.parent_names = &parent_name;
> +	init.num_parents = 1;
>  	init.flags = 0;
>  
>  	periph->id = id;
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] clk: at91: main: do not continue if oscillators already prepared
  2020-06-25 10:09 ` [PATCH 2/2] clk: at91: main: do not continue if oscillators already prepared Claudiu Beznea
@ 2020-06-26 21:03   ` Alexandre Belloni
  2020-07-01  9:47     ` Claudiu.Beznea
  0 siblings, 1 reply; 5+ messages in thread
From: Alexandre Belloni @ 2020-06-26 21:03 UTC (permalink / raw)
  To: Claudiu Beznea
  Cc: mturquette, sboyd, nicolas.ferre, ludovic.desroches,
	linux-kernel, mturquette, bbrezillon, linux-clk,
	linux-arm-kernel

On 25/06/2020 13:09:28+0300, Claudiu Beznea wrote:
> Return in clk_main_osc_prepare()/clk_main_rc_osc_prepare() if
> oscillators are already enabled.
> 
> Fixes: 27cb1c2083373 ("clk: at91: rework main clk implementation")
> Fixes: 1bdf02326b71e ("clk: at91: make use of syscon/regmap internally")

Is this really a fix? What is the observed issue?

> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/clk/at91/clk-main.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c
> index 37c22667e831..46b4d2131989 100644
> --- a/drivers/clk/at91/clk-main.c
> +++ b/drivers/clk/at91/clk-main.c
> @@ -74,13 +74,11 @@ static int clk_main_osc_prepare(struct clk_hw *hw)
>  	regmap_read(regmap, AT91_CKGR_MOR, &tmp);
>  	tmp &= ~MOR_KEY_MASK;
>  
> -	if (tmp & AT91_PMC_OSCBYPASS)
> +	if (tmp & (AT91_PMC_OSCBYPASS | AT91_PMC_MOSCEN))
>  		return 0;

While this seems like a good optimization, it is also not correct.
Having AT91_PMC_MOSCEN set doesn't mean that the clock is ready, you
need to at least check MOSCS once.

>  
> -	if (!(tmp & AT91_PMC_MOSCEN)) {
> -		tmp |= AT91_PMC_MOSCEN | AT91_PMC_KEY;
> -		regmap_write(regmap, AT91_CKGR_MOR, tmp);
> -	}
> +	tmp |= AT91_PMC_MOSCEN | AT91_PMC_KEY;
> +	regmap_write(regmap, AT91_CKGR_MOR, tmp);
>  
>  	while (!clk_main_osc_ready(regmap))
>  		cpu_relax();
> @@ -186,10 +184,12 @@ static int clk_main_rc_osc_prepare(struct clk_hw *hw)
>  
>  	regmap_read(regmap, AT91_CKGR_MOR, &mor);
>  
> -	if (!(mor & AT91_PMC_MOSCRCEN))
> -		regmap_update_bits(regmap, AT91_CKGR_MOR,
> -				   MOR_KEY_MASK | AT91_PMC_MOSCRCEN,
> -				   AT91_PMC_MOSCRCEN | AT91_PMC_KEY);
> +	if (mor & AT91_PMC_MOSCRCEN)
> +		return 0;
> +
> +	regmap_update_bits(regmap, AT91_CKGR_MOR,
> +			   MOR_KEY_MASK | AT91_PMC_MOSCRCEN,
> +			   AT91_PMC_MOSCRCEN | AT91_PMC_KEY);
>  
>  	while (!clk_main_rc_osc_ready(regmap))
>  		cpu_relax();
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] clk: at91: main: do not continue if oscillators already prepared
  2020-06-26 21:03   ` Alexandre Belloni
@ 2020-07-01  9:47     ` Claudiu.Beznea
  0 siblings, 0 replies; 5+ messages in thread
From: Claudiu.Beznea @ 2020-07-01  9:47 UTC (permalink / raw)
  To: alexandre.belloni
  Cc: mturquette, sboyd, Nicolas.Ferre, Ludovic.Desroches,
	linux-kernel, mturquette, bbrezillon, linux-clk,
	linux-arm-kernel



On 27.06.2020 00:03, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 25/06/2020 13:09:28+0300, Claudiu Beznea wrote:
>> Return in clk_main_osc_prepare()/clk_main_rc_osc_prepare() if
>> oscillators are already enabled.
>>
>> Fixes: 27cb1c2083373 ("clk: at91: rework main clk implementation")
>> Fixes: 1bdf02326b71e ("clk: at91: make use of syscon/regmap internally")
> 
> Is this really a fix? What is the observed issue?
> 
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/clk/at91/clk-main.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c
>> index 37c22667e831..46b4d2131989 100644
>> --- a/drivers/clk/at91/clk-main.c
>> +++ b/drivers/clk/at91/clk-main.c
>> @@ -74,13 +74,11 @@ static int clk_main_osc_prepare(struct clk_hw *hw)
>>       regmap_read(regmap, AT91_CKGR_MOR, &tmp);
>>       tmp &= ~MOR_KEY_MASK;
>>
>> -     if (tmp & AT91_PMC_OSCBYPASS)
>> +     if (tmp & (AT91_PMC_OSCBYPASS | AT91_PMC_MOSCEN))
>>               return 0;
> 
> While this seems like a good optimization, it is also not correct.
> Having AT91_PMC_MOSCEN set doesn't mean that the clock is ready, you
> need to at least check MOSCS once.

I agree! This is may introduce issues. Thank you for reviewing it.

> 
>>
>> -     if (!(tmp & AT91_PMC_MOSCEN)) {
>> -             tmp |= AT91_PMC_MOSCEN | AT91_PMC_KEY;
>> -             regmap_write(regmap, AT91_CKGR_MOR, tmp);
>> -     }
>> +     tmp |= AT91_PMC_MOSCEN | AT91_PMC_KEY;
>> +     regmap_write(regmap, AT91_CKGR_MOR, tmp);
>>
>>       while (!clk_main_osc_ready(regmap))
>>               cpu_relax();
>> @@ -186,10 +184,12 @@ static int clk_main_rc_osc_prepare(struct clk_hw *hw)
>>
>>       regmap_read(regmap, AT91_CKGR_MOR, &mor);
>>
>> -     if (!(mor & AT91_PMC_MOSCRCEN))
>> -             regmap_update_bits(regmap, AT91_CKGR_MOR,
>> -                                MOR_KEY_MASK | AT91_PMC_MOSCRCEN,
>> -                                AT91_PMC_MOSCRCEN | AT91_PMC_KEY);
>> +     if (mor & AT91_PMC_MOSCRCEN)
>> +             return 0;
>> +
>> +     regmap_update_bits(regmap, AT91_CKGR_MOR,
>> +                        MOR_KEY_MASK | AT91_PMC_MOSCRCEN,
>> +                        AT91_PMC_MOSCRCEN | AT91_PMC_KEY);
>>
>>       while (!clk_main_rc_osc_ready(regmap))
>>               cpu_relax();
>> --
>> 2.7.4
>>
> 
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-01  9:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 10:09 [PATCH 1/2] clk: at91: remove the checking of parent_name Claudiu Beznea
2020-06-25 10:09 ` [PATCH 2/2] clk: at91: main: do not continue if oscillators already prepared Claudiu Beznea
2020-06-26 21:03   ` Alexandre Belloni
2020-07-01  9:47     ` Claudiu.Beznea
2020-06-26 20:58 ` [PATCH 1/2] clk: at91: remove the checking of parent_name Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).