Re: [PATCH] clk: mvebu: armada-38x: add support for missing clocks

[Gregory CLEMENT]

Hi Richard,
 
 On mar., mars 13 2018, Richard Genoud <richard.genoud@xxxxxxxxx> wrote:

> On 08/03/2018 14:23, Gregory CLEMENT wrote:
>> Hi,
>>  
>>  On jeu., mars 08 2018, Gregory CLEMENT <gregory.clement@xxxxxxxxxxx> wrote:
>> 
>>> Hi Richard,
>>>  
>>>  On jeu., mars 08 2018, Richard Genoud <richard.genoud@xxxxxxxxx> wrote:
>>>
>>>> Clearfog boards can come with a CPU clocked at 1600MHz (commercial)
>>>> or 1333MHz (industrial).
>>>>
>>>> They have also some dip-switches to select a different clock (666, 800,
>>>> 1066, 1200).
>>>
>>> The patch looks goo and it will also be usefull for any other board
>>> using these frequencies, thanks for this. I have only one small comment,
>>> see below.
>> 
>> 
>> I forgot to mention that you can add my
>> 
>> Acked-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxx>
>> 
>> Thanks,
>> 
>> Gregory
>> 
>>>
>>>
>>>>
>>>> The funny thing is that the recovery button is on the MPP34 fq selector.
>>>> So, when booting an industrial board with this button down, the frequency
>>>> 666MHz is selected (and the kernel didn't boot).
>>>>
>>>> This patch add all the missing clocks.
>>>>
>>>> The only mode I didn't test is 2GHz (uboot found 4294MHz instead :/ ).
>>>>
>>>> Signed-off-by: Richard Genoud <richard.genoud@xxxxxxxxx>
>>>> ---
>>>>  drivers/clk/mvebu/armada-38x.c | 14 +++++++-------
>>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/mvebu/armada-38x.c b/drivers/clk/mvebu/armada-38x.c
>>>> index 394aa6f03f01..9ff4ea63932d 100644
>>>> --- a/drivers/clk/mvebu/armada-38x.c
>>>> +++ b/drivers/clk/mvebu/armada-38x.c
>>>> @@ -46,11 +46,11 @@ static u32 __init armada_38x_get_tclk_freq(void __iomem *sar)
>>>>  }
>>>>  
>>>>  static const u32 armada_38x_cpu_frequencies[] __initconst = {
>>>> -	0, 0, 0, 0,
>>>> -	1066 * 1000 * 1000, 0, 0, 0,
>>>> +	666 * 1000 * 1000,  0, 800 * 1000 * 1000, 0,
>>>> +	1066 * 1000 * 1000, 0, 1200 * 1000 * 1000, 0,
>>>>  	1332 * 1000 * 1000, 0, 0, 0,
>>>>  	1600 * 1000 * 1000, 0, 0, 0,
>>>> -	1866 * 1000 * 1000,
>>>> +	1866 * 1000 * 1000, 0, 0, 2000 * 1000 * 1000,
>>>
>>> Maybe you could add a comment here to say that the 2GHz mode didn't have
>>> been tested.
> Well, if I add a comment there saying "2GHz mode hasn't been tested",
> I'm afraid it will stay there forever, even after being tested.
> (if it's working, nobody will look at it, and if it's not, it's in the
> commit message anyway).

OK fair enough.

>
> I was also thinking that this could go in -stable since it fixes a hang.
> In this case, commit 9593f4f56cf5 ("clk: mvebu: armada-38x: add support for 1866MHz variants")
> should also go with it.

In this case you have to send a new version of your patch with the cc:
stabale and Fixes: tags.

Gregory

>
>
>
> Thanks !
>
>>>
>>> Thanks,
>>> Gregory
>>>
>>>
>>>>  };
>>>>  
>>>>  static u32 __init armada_38x_get_cpu_freq(void __iomem *sar)
>>>> @@ -76,11 +76,11 @@ static const struct coreclk_ratio armada_38x_coreclk_ratios[] __initconst = {
>>>>  };
>>>>  
>>>>  static const int armada_38x_cpu_l2_ratios[32][2] __initconst = {
>>>> -	{0, 1}, {0, 1}, {0, 1}, {0, 1},
>>>> -	{1, 2}, {0, 1}, {0, 1}, {0, 1},
>>>> -	{1, 2}, {0, 1}, {0, 1}, {0, 1},
>>>> +	{1, 2}, {0, 1}, {1, 2}, {0, 1},
>>>> +	{1, 2}, {0, 1}, {1, 2}, {0, 1},
>>>>  	{1, 2}, {0, 1}, {0, 1}, {0, 1},
>>>>  	{1, 2}, {0, 1}, {0, 1}, {0, 1},
>>>> +	{1, 2}, {0, 1}, {0, 1}, {1, 2},
>>>>  	{0, 1}, {0, 1}, {0, 1}, {0, 1},
>>>>  	{0, 1}, {0, 1}, {0, 1}, {0, 1},
>>>>  	{0, 1}, {0, 1}, {0, 1}, {0, 1},
>>>> @@ -91,7 +91,7 @@ static const int armada_38x_cpu_ddr_ratios[32][2] __initconst = {
>>>>  	{1, 2}, {0, 1}, {0, 1}, {0, 1},
>>>>  	{1, 2}, {0, 1}, {0, 1}, {0, 1},
>>>>  	{1, 2}, {0, 1}, {0, 1}, {0, 1},
>>>> -	{1, 2}, {0, 1}, {0, 1}, {0, 1},
>>>> +	{1, 2}, {0, 1}, {0, 1}, {7, 15},
>>>>  	{0, 1}, {0, 1}, {0, 1}, {0, 1},
>>>>  	{0, 1}, {0, 1}, {0, 1}, {0, 1},
>>>>  	{0, 1}, {0, 1}, {0, 1}, {0, 1},
>>>
>>> -- 
>>> Gregory Clement, Bootlin (formerly Free Electrons)
>>> Embedded Linux and Kernel engineering
>>> http://bootlin.com
>> 
>

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com