clk: mvebu: armada-xp: Support for MSYS SoC
Subsystems (2)
RFC PATCH 1/1 clk: mvebu: armada-xp: Support for MSYS SoC
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,
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(-)
drivers/clk/mvebu/armada-xp.c +19 -1
@@ -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);
Signature · 1 line
2.2.0.rc0
Andrew Lunn
maintainerRFC PATCH 1/1 clk: mvebu: armada-xp: Support for MSYS SoC
Quoted 5 lines · 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.
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
Quoted 5 lines
Signed-off-by Chris Packham <[email protected]>---
Hi,
Quoted 8 lines
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
Quoted 57 lines
Thansk,
Chrisdrivers/clk/mvebu/armada-xp.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)drivers/clk/mvebu/armada-xp.c +19
-1@@ -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);Signature · 1 line
2.2.0.rc0
RFC PATCH 1/1 clk: mvebu: armada-xp: Support for MSYS SoC
Hi Andrew,
Quoted 10 lines · Andrew Lunn
Quoted 5 lines · 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.
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.
Quoted 4 lines
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.
Quoted 5 lines
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?
Quoted 15 lines
Quoted 6 lines
Signed-off-by Chris Packham <[email protected]>---
Hi,Quoted 8 lines
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.
Quoted 4 lines
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.
Quoted 60 lines
Andrew
Quoted 57 lines
Thansk,
Chrisdrivers/clk/mvebu/armada-xp.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)drivers/clk/mvebu/armada-xp.c +19
-2@@ -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
Andrew Lunn
maintainerRFC PATCH 1/1 clk: mvebu: armada-xp: Support for MSYS SoC
Quoted 11 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
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
See Lesson #0.
I also like Lesson #13 :-)
Andrew
RFC PATCH 1/1 clk: mvebu: armada-xp: Support for MSYS SoC
On 11/21/2014 10:36 AM, Andrew Lunn wrote:
(snip)
Quoted 9 lines
Quoted 3 lines
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.
Quoted 7 lines
There is an interesting presentation by Thomas from ELC 2014
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 1/1 clk: mvebu: armada-xp: Support for MSYS SoC
In reply to Chris PackhamDear Chris Packham,
Quoted 5 lines · 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.
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.
Quoted 8 lines
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Andrew Lunn
maintainerRFC PATCH 1/1 clk: mvebu: armada-xp: Support for MSYS SoC
Quoted 9 lines · Thomas Petazzoni
Dear Chris Packham,
Quoted 5 lines · 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.
Quoted 3 lines
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 1/1 clk: mvebu: armada-xp: Support for MSYS SoC
In reply to Thomas PetazzoniQuoted 9 lines · Thomas Petazzoni
Dear Chris Packham,
Quoted 5 lines · 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.
Quoted 3 lines
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).
Quoted 19 lines
Quoted 9 lines
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.
Quoted 4 lines
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).
Quoted 5 lines
Best regards,
Thomas