[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[Chris Packham]

The MSYS SoCs are a range of packet processors with integrated CPUs based
on armada-xp. One difference is that the TCLK frequency is fixed at 200MHz
as opposed to the fixed 250MHz used on armada-xp. The clock-gating options
are a subset of what's available on the armada-xp so this code should be
compatible.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Hi,

This patch is enough to get the uart clock dividers correct so I get some
output. As far as I've been able to tell there is no way of dynamically
detecting the TCLK frequency.

The core clock frequency and ratio calculations are probably not correct but
for these CPU inside a packet processor systems I'm not sure how much that
actually matter since these systems aren't likely to do any kind of dynamic
frequency scaling.

Thansk,
Chris

 drivers/clk/mvebu/armada-xp.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/mvebu/armada-xp.c b/drivers/clk/mvebu/armada-xp.c
index b309431..9f852f8 100644
--- a/drivers/clk/mvebu/armada-xp.c
+++ b/drivers/clk/mvebu/armada-xp.c
@@ -52,6 +52,12 @@ static u32 __init axp_get_tclk_freq(void __iomem *sar)
 	return 250000000;
 }
 
+/* MSYS TCLK frequency is fixed to 200MHz */
+static u32 __init msys_get_tclk_freq(void __iomem *sar)
+{
+	return 200000000;
+}
+
 static const u32 axp_cpu_freqs[] __initconst = {
 	1000000000,
 	1066000000,
@@ -158,6 +164,14 @@ static const struct coreclk_soc_desc axp_coreclks = {
 	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
 };
 
+static const struct coreclk_soc_desc msys_coreclks = {
+	.get_tclk_freq = msys_get_tclk_freq,
+	.get_cpu_freq = axp_get_cpu_freq,
+	.get_clk_ratio = axp_get_clk_ratio,
+	.ratios = axp_coreclk_ratios,
+	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
+};
+
 /*
  * Clock Gating Control
  */
@@ -200,9 +214,13 @@ static void __init axp_clk_init(struct device_node *np)
 	struct device_node *cgnp =
 		of_find_compatible_node(NULL, NULL, "marvell,armada-xp-gating-clock");
 
-	mvebu_coreclk_setup(np, &axp_coreclks);
+	if (of_device_is_compatible(np, "marvell,msys-core-clock"))
+		mvebu_coreclk_setup(np, &msys_coreclks);
+	else
+		mvebu_coreclk_setup(np, &axp_coreclks);
 
 	if (cgnp)
 		mvebu_clk_gating_setup(cgnp, axp_gating_desc);
 }
 CLK_OF_DECLARE(axp_clk, "marvell,armada-xp-core-clock", axp_clk_init);
+CLK_OF_DECLARE(msys_clk, "marvell,msys-core-clock", axp_clk_init);
-- 
2.2.0.rc0

[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[Andrew Lunn]

On Thu, Nov 20, 2014 at 06:01:19PM +1300, Chris Packham wrote:
> The MSYS SoCs are a range of packet processors with integrated CPUs based
> on armada-xp. One difference is that the TCLK frequency is fixed at 200MHz
> as opposed to the fixed 250MHz used on armada-xp. The clock-gating options
> are a subset of what's available on the armada-xp so this code should be
> compatible.

Hi Chris

How generic/specific is the name msys? We need to be careful here,
because there could be other packet processors with embedded Armada-XP
cores with different tclk speeds. Rather than using msys, it might be
better to use the specific packet processor product ID.

Whatever we call it, this new compatibility string also needs adding
to the device tree binding document in 

Documentation/devicetree/bindings/clock/mvebu-core-clock.txt

> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Hi,
> 
> This patch is enough to get the uart clock dividers correct so I get some
> output. As far as I've been able to tell there is no way of dynamically
> detecting the TCLK frequency.
> 
> The core clock frequency and ratio calculations are probably not correct but
> for these CPU inside a packet processor systems I'm not sure how much that
> actually matter since these systems aren't likely to do any kind of dynamic
> frequency scaling.

Do you have u-boot running? It generally prints out these frequencies.
You can at least verify if they are {in}consistent with Linux. If you
have the u-boot sources, you might also be able to use it get these
clocks right in Linux.

       Andrew

> 
> Thansk,
> Chris
> 
>  drivers/clk/mvebu/armada-xp.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/mvebu/armada-xp.c b/drivers/clk/mvebu/armada-xp.c
> index b309431..9f852f8 100644
> --- a/drivers/clk/mvebu/armada-xp.c
> +++ b/drivers/clk/mvebu/armada-xp.c
> @@ -52,6 +52,12 @@ static u32 __init axp_get_tclk_freq(void __iomem *sar)
>  	return 250000000;
>  }
>  
> +/* MSYS TCLK frequency is fixed to 200MHz */
> +static u32 __init msys_get_tclk_freq(void __iomem *sar)
> +{
> +	return 200000000;
> +}
> +
>  static const u32 axp_cpu_freqs[] __initconst = {
>  	1000000000,
>  	1066000000,
> @@ -158,6 +164,14 @@ static const struct coreclk_soc_desc axp_coreclks = {
>  	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
>  };
>  
> +static const struct coreclk_soc_desc msys_coreclks = {
> +	.get_tclk_freq = msys_get_tclk_freq,
> +	.get_cpu_freq = axp_get_cpu_freq,
> +	.get_clk_ratio = axp_get_clk_ratio,
> +	.ratios = axp_coreclk_ratios,
> +	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
> +};
> +
>  /*
>   * Clock Gating Control
>   */
> @@ -200,9 +214,13 @@ static void __init axp_clk_init(struct device_node *np)
>  	struct device_node *cgnp =
>  		of_find_compatible_node(NULL, NULL, "marvell,armada-xp-gating-clock");
>  
> -	mvebu_coreclk_setup(np, &axp_coreclks);
> +	if (of_device_is_compatible(np, "marvell,msys-core-clock"))
> +		mvebu_coreclk_setup(np, &msys_coreclks);
> +	else
> +		mvebu_coreclk_setup(np, &axp_coreclks);
>  
>  	if (cgnp)
>  		mvebu_clk_gating_setup(cgnp, axp_gating_desc);
>  }
>  CLK_OF_DECLARE(axp_clk, "marvell,armada-xp-core-clock", axp_clk_init);
> +CLK_OF_DECLARE(msys_clk, "marvell,msys-core-clock", axp_clk_init);
> -- 
> 2.2.0.rc0
> 

[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[Chris Packham]

Hi Andrew,

On 11/20/2014 06:17 PM, Andrew Lunn wrote:
> On Thu, Nov 20, 2014 at 06:01:19PM +1300, Chris Packham wrote:
>> The MSYS SoCs are a range of packet processors with integrated CPUs based
>> on armada-xp. One difference is that the TCLK frequency is fixed at 200MHz
>> as opposed to the fixed 250MHz used on armada-xp. The clock-gating options
>> are a subset of what's available on the armada-xp so this code should be
>> compatible.
>
> Hi Chris
>
> How generic/specific is the name msys?

msys is the name Marvell use for the embedded dual core CPU across 
several product lines. I believe the CPU core is the same on all of 
them. It's also the name used in the LSP Marvell provide. This was the 
main reason I went with "msys" despite the possible confusion with 
MSYS/MINGW.

> We need to be careful here,
> because there could be other packet processors with embedded Armada-XP
> cores with different tclk speeds. Rather than using msys, it might be
> better to use the specific packet processor product ID.

The specific chip I'm working with is the 98DX4251 but there are at 
least 4 variants in that product line. I could probably go with 98DX42xx 
to cover all those variants but there is a whole other product line 
(sorry don't know the model numbers) with the same embedded core.

>
> Whatever we call it, this new compatibility string also needs adding
> to the device tree binding document in
>
> Documentation/devicetree/bindings/clock/mvebu-core-clock.txt

Will include that in v2.

On a side note would people prefer I send my entire work in progress 
get-linux-working-on-this-board series or drip feed individual patches 
as I have been doing?

>
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>> Hi,
>>
>> This patch is enough to get the uart clock dividers correct so I get some
>> output. As far as I've been able to tell there is no way of dynamically
>> detecting the TCLK frequency.
>>
>> The core clock frequency and ratio calculations are probably not correct but
>> for these CPU inside a packet processor systems I'm not sure how much that
>> actually matter since these systems aren't likely to do any kind of dynamic
>> frequency scaling.
>
> Do you have u-boot running? It generally prints out these frequencies.
> You can at least verify if they are {in}consistent with Linux. If you
> have the u-boot sources, you might also be able to use it get these
> clocks right in Linux.

Yes I do have a Marvell supplied u-boot, and yes the frequencies are 
inconsistent. But I'm not even sure the frequencies reported by u-boot 
are correct.

>
>         Andrew
>
>>
>> Thansk,
>> Chris
>>
>>   drivers/clk/mvebu/armada-xp.c | 20 +++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/mvebu/armada-xp.c b/drivers/clk/mvebu/armada-xp.c
>> index b309431..9f852f8 100644
>> --- a/drivers/clk/mvebu/armada-xp.c
>> +++ b/drivers/clk/mvebu/armada-xp.c
>> @@ -52,6 +52,12 @@ static u32 __init axp_get_tclk_freq(void __iomem *sar)
>>   	return 250000000;
>>   }
>>
>> +/* MSYS TCLK frequency is fixed to 200MHz */
>> +static u32 __init msys_get_tclk_freq(void __iomem *sar)
>> +{
>> +	return 200000000;
>> +}
>> +
>>   static const u32 axp_cpu_freqs[] __initconst = {
>>   	1000000000,
>>   	1066000000,
>> @@ -158,6 +164,14 @@ static const struct coreclk_soc_desc axp_coreclks = {
>>   	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
>>   };
>>
>> +static const struct coreclk_soc_desc msys_coreclks = {
>> +	.get_tclk_freq = msys_get_tclk_freq,
>> +	.get_cpu_freq = axp_get_cpu_freq,
>> +	.get_clk_ratio = axp_get_clk_ratio,
>> +	.ratios = axp_coreclk_ratios,
>> +	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
>> +};
>> +
>>   /*
>>    * Clock Gating Control
>>    */
>> @@ -200,9 +214,13 @@ static void __init axp_clk_init(struct device_node *np)
>>   	struct device_node *cgnp =
>>   		of_find_compatible_node(NULL, NULL, "marvell,armada-xp-gating-clock");
>>
>> -	mvebu_coreclk_setup(np, &axp_coreclks);
>> +	if (of_device_is_compatible(np, "marvell,msys-core-clock"))
>> +		mvebu_coreclk_setup(np, &msys_coreclks);
>> +	else
>> +		mvebu_coreclk_setup(np, &axp_coreclks);
>>
>>   	if (cgnp)
>>   		mvebu_clk_gating_setup(cgnp, axp_gating_desc);
>>   }
>>   CLK_OF_DECLARE(axp_clk, "marvell,armada-xp-core-clock", axp_clk_init);
>> +CLK_OF_DECLARE(msys_clk, "marvell,msys-core-clock", axp_clk_init);
>> --
>> 2.2.0.rc0
>>

[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[Andrew Lunn]

> msys is the name Marvell use for the embedded dual core CPU across 
> several product lines. I believe the CPU core is the same on all of 
> them. It's also the name used in the LSP Marvell provide. This was the 
> main reason I went with "msys" despite the possible confusion with 
> MSYS/MINGW.
> 
> > We need to be careful here,
> > because there could be other packet processors with embedded Armada-XP
> > cores with different tclk speeds. Rather than using msys, it might be
> > better to use the specific packet processor product ID.
> 
> The specific chip I'm working with is the 98DX4251 but there are at 
> least 4 variants in that product line. I could probably go with 98DX42xx 
> to cover all those variants but there is a whole other product line 
> (sorry don't know the model numbers) with the same embedded core.

In the DT world, you generally base the compatibility string on the
first device which gets added. So i would go with 98DX4251. The other
products in the family just need to say they are compatible with the
98DX4251.

> On a side note would people prefer I send my entire work in progress 
> get-linux-working-on-this-board series or drip feed individual patches 
> as I have been doing?

Release early, release often. It makes no sense to spend a lot of time
on polishing something, if we are going to rip it to shreds and tell
you to do it another way. Post something when it works, so that we can
review it and let you know if you are going in the right direction.

There is an interesting presentation by Thomas from ELC 2014

http://free-electrons.com/pub/conferences/2014/elc/petazzoni-soc-mainlining-lessons-learned/petazzoni-soc-mainlining-lessons-learned.pdf

See Lesson #0.

I also like Lesson #13 :-)

    Andrew

[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[Chris Packham]

On 11/21/2014 10:36 AM, Andrew Lunn wrote:
(snip)
>> On a side note would people prefer I send my entire work in progress
>> get-linux-working-on-this-board series or drip feed individual patches
>> as I have been doing?
>
> Release early, release often. It makes no sense to spend a lot of time
> on polishing something, if we are going to rip it to shreds and tell
> you to do it another way. Post something when it works, so that we can
> review it and let you know if you are going in the right direction.
>
> There is an interesting presentation by Thomas from ELC 2014
>
> http://free-electrons.com/pub/conferences/2014/elc/petazzoni-soc-mainlining-lessons-learned/petazzoni-soc-mainlining-lessons-learned.pdf
>
> See Lesson #0.
>
> I also like Lesson #13 :-)

Thanks. I'll try do some more submissions but I don't want to waste 
other peoples time especially during an rc phase.

[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[Thomas Petazzoni]

Dear Chris Packham,

On Thu, 20 Nov 2014 18:01:19 +1300, Chris Packham wrote:
> The MSYS SoCs are a range of packet processors with integrated CPUs based
> on armada-xp. One difference is that the TCLK frequency is fixed at 200MHz
> as opposed to the fixed 250MHz used on armada-xp. The clock-gating options
> are a subset of what's available on the armada-xp so this code should be
> compatible.

Well, if you have only a subset of what's available, then I would also
suggest to introduce a separate compatible string for the gatable
clocks.

> This patch is enough to get the uart clock dividers correct so I get some
> output. As far as I've been able to tell there is no way of dynamically
> detecting the TCLK frequency.
> 
> The core clock frequency and ratio calculations are probably not correct but
> for these CPU inside a packet processor systems I'm not sure how much that
> actually matter since these systems aren't likely to do any kind of dynamic
> frequency scaling.

Knowing the frequency is not only about dynamic frequency scaling. Some
drivers (i2c, spi, probably sdio) use the input clock frequency, look
at the requested output frequency, and calculate some dividers to reach
the desired output frequency from the input frequency. If your input
frequency is wrong, then your divider calculation will be wrong, and
therefore your output frequency will be wrong. This could lead to
having an incorrect I2C or SPI bus frequency, for example. As you can
see, nothing to do with dynamic frequency scaling.

But indeed, those concerns are more around peripheral clocks, which
normally derive from tclk. Still, it'd be better to not have the core
clocks defined at all rather than having incorrect core clock
frequencies.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[Andrew Lunn]

On Thu, Nov 20, 2014 at 03:56:30PM +0100, Thomas Petazzoni wrote:
> Dear Chris Packham,
> 
> On Thu, 20 Nov 2014 18:01:19 +1300, Chris Packham wrote:
> > The MSYS SoCs are a range of packet processors with integrated CPUs based
> > on armada-xp. One difference is that the TCLK frequency is fixed at 200MHz
> > as opposed to the fixed 250MHz used on armada-xp. The clock-gating options
> > are a subset of what's available on the armada-xp so this code should be
> > compatible.
> 
> Well, if you have only a subset of what's available, then I would also
> suggest to introduce a separate compatible string for the gatable
> clocks.

For the kirkwood based 98dx4122, we avoided a separate gatable
compatible string, by simply not listing devices which don't
exist. For that SoC, accessing clocks which don't exist is not a
problem. However, accesses devices which don't exist does lock the
SoC.

There is a specific pin-controller compatible string:
marvell,98dx4122-pinctrl, and i guess one is also needed here.

	Andrew

[RFC PATCH] clk: mvebu: armada-xp: Support for MSYS SoC

[Chris Packham]

On 11/21/2014 03:56 AM, Thomas Petazzoni wrote:
> Dear Chris Packham,
>
> On Thu, 20 Nov 2014 18:01:19 +1300, Chris Packham wrote:
>> The MSYS SoCs are a range of packet processors with integrated CPUs based
>> on armada-xp. One difference is that the TCLK frequency is fixed at 200MHz
>> as opposed to the fixed 250MHz used on armada-xp. The clock-gating options
>> are a subset of what's available on the armada-xp so this code should be
>> compatible.
>
> Well, if you have only a subset of what's available, then I would also
> suggest to introduce a separate compatible string for the gatable
> clocks.

I could do that. Based on the datasheet I thought it would be 
unnecessary because the other bits are ignored on write. As Andrew has 
said for the 98DX4122 the peripherals that aren't present are just not 
listed, which would be the same for msys (or 98DX4251 or whatever we end 
up calling it).

>
>> This patch is enough to get the uart clock dividers correct so I get some
>> output. As far as I've been able to tell there is no way of dynamically
>> detecting the TCLK frequency.
>>
>> The core clock frequency and ratio calculations are probably not correct but
>> for these CPU inside a packet processor systems I'm not sure how much that
>> actually matter since these systems aren't likely to do any kind of dynamic
>> frequency scaling.
>
> Knowing the frequency is not only about dynamic frequency scaling. Some
> drivers (i2c, spi, probably sdio) use the input clock frequency, look
> at the requested output frequency, and calculate some dividers to reach
> the desired output frequency from the input frequency. If your input
> frequency is wrong, then your divider calculation will be wrong, and
> therefore your output frequency will be wrong. This could lead to
> having an incorrect I2C or SPI bus frequency, for example. As you can
> see, nothing to do with dynamic frequency scaling.
>
> But indeed, those concerns are more around peripheral clocks, which
> normally derive from tclk. Still, it'd be better to not have the core
> clocks defined at all rather than having incorrect core clock
> frequencies.

As you say most of the peripherals use the TCLK as an input to their 
dividers. That's the thing that needs to be correct. I must admit I 
don't really understand how get_cpu_freq() is actually used (I 
incorrectly guessed frequency scaling). My system seems to be happy 
(depending on your definition of happy) with whatever information it's 
getting.

As an alternative what about making the TCLK frequency something 
configurable by the dts? That way it could default to 250MHz but be 
overridden if a chip has a different frequency (in lieu of some way of 
actually detecting it).

>
> Best regards,
>
> Thomas
>