| Message ID | 1416459679-30944-1-git-send-email-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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 >>
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 >
> 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
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.
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);
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(-)