2018-03-08 09:04:34

by Richard Genoud

[permalink] [raw]
Subject: [PATCH] clk: mvebu: armada-38x: add support for missing clocks

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 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 <[email protected]>
---
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,
};

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},


2018-03-08 13:23:27

by Gregory CLEMENT

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

Hi Richard,

On jeu., mars 08 2018, Richard Genoud <[email protected]> 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.


>
> 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 <[email protected]>
> ---
> 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.

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

2018-03-08 13:25:00

by Gregory CLEMENT

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

Hi,

On jeu., mars 08 2018, Gregory CLEMENT <[email protected]> wrote:

> Hi Richard,
>
> On jeu., mars 08 2018, Richard Genoud <[email protected]> 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 <[email protected]>

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 <[email protected]>
>> ---
>> 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.
>
> 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

2018-03-13 10:21:58

by Richard Genoud

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

On 08/03/2018 14:23, Gregory CLEMENT wrote:
> Hi,
>
> On jeu., mars 08 2018, Gregory CLEMENT <[email protected]> wrote:
>
>> Hi Richard,
>>
>> On jeu., mars 08 2018, Richard Genoud <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>>> ---
>>> 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).

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.



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
>


2018-03-13 10:34:25

by Gregory CLEMENT

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

Hi Richard,

On mar., mars 13 2018, Richard Genoud <[email protected]> wrote:

> On 08/03/2018 14:23, Gregory CLEMENT wrote:
>> Hi,
>>
>> On jeu., mars 08 2018, Gregory CLEMENT <[email protected]> wrote:
>>
>>> Hi Richard,
>>>
>>> On jeu., mars 08 2018, Richard Genoud <[email protected]> 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 <[email protected]>
>>
>> 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 <[email protected]>
>>>> ---
>>>> 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