| Message ID | 1397555342-25644-1-git-send-email-21cnbao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 15, 2014 at 05:49:02PM +0800, Barry Song wrote: > From: Barry Song <Baohua.Song@csr.com> > > L2 cache size has been read by l2x0_init(), it is useless to set > WAY_SIZE and ASSOCIATIVITY in prima2. > > Cc: Russell King <linux@arm.linux.org.uk> > Signed-off-by: Barry Song <Baohua.Song@csr.com> > --- > -Derived from Russell's "ARM: l2c: prima2: remove cache size override" You're a SMP architecture... it would be much better to initialise the L2 cache before the secondary CPUs are brought online. In any case, across the board, it's preferable that the L2 cache is enabled before the delay loop calibration as it can have an effect on the calibrated value.
2014-04-15 18:00 GMT+08:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > On Tue, Apr 15, 2014 at 05:49:02PM +0800, Barry Song wrote: >> From: Barry Song <Baohua.Song@csr.com> >> >> L2 cache size has been read by l2x0_init(), it is useless to set >> WAY_SIZE and ASSOCIATIVITY in prima2. >> >> Cc: Russell King <linux@arm.linux.org.uk> >> Signed-off-by: Barry Song <Baohua.Song@csr.com> >> --- >> -Derived from Russell's "ARM: l2c: prima2: remove cache size override" > > You're a SMP architecture... it would be much better to initialise the > L2 cache before the secondary CPUs are brought online. ok, it seems we still need to hook this into the callback of this specific machine instead of using a global early_initcall(). since now the dt compatible field has been generic enough, it will cause other platforms to execute l2x0_of_init() too. > > In any case, across the board, it's preferable that the L2 cache is > enabled before the delay loop calibration as it can have an effect on > the calibrated value. what if the real case is that l2x0 has been enabled in bootloader? > > -- > FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly > improving, and getting towards what was expected from it. -barry
On Tue, Apr 15, 2014 at 06:13:27PM +0800, Barry Song wrote: > 2014-04-15 18:00 GMT+08:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > > On Tue, Apr 15, 2014 at 05:49:02PM +0800, Barry Song wrote: > >> From: Barry Song <Baohua.Song@csr.com> > >> > >> L2 cache size has been read by l2x0_init(), it is useless to set > >> WAY_SIZE and ASSOCIATIVITY in prima2. > >> > >> Cc: Russell King <linux@arm.linux.org.uk> > >> Signed-off-by: Barry Song <Baohua.Song@csr.com> > >> --- > >> -Derived from Russell's "ARM: l2c: prima2: remove cache size override" > > > > You're a SMP architecture... it would be much better to initialise the > > L2 cache before the secondary CPUs are brought online. > > ok, it seems we still need to hook this into the callback of this > specific machine instead of using a global early_initcall(). since now > the dt compatible field has been generic enough, it will cause other > platforms to execute l2x0_of_init() too. What I did for Versatile Express was to move it to the init_irq stage. I'm not condoning using init_irq() for non-IRQ stuff, but that was just to see whether it was possible (the .init_early is too early for it.) Let me put it another way: I'm not thrilled by the idea of having it in init_irq() but that's a better location to call L2 cache initialisation than half-way through the kernel's initialisation. > > In any case, across the board, it's preferable that the L2 cache is > > enabled before the delay loop calibration as it can have an effect on > > the calibrated value. > > what if the real case is that l2x0 has been enabled in bootloader? If the L2 cache has already been enabled, l2x0_of_init() handles it - in that situation, we can't re-do the enable (because writing to the aux ctrl register is not allowed) so we merely setup the outer cache function pointers and don't try to do any configuration or enabling.
2014-04-16 5:56 GMT+08:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > On Tue, Apr 15, 2014 at 06:13:27PM +0800, Barry Song wrote: >> 2014-04-15 18:00 GMT+08:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: >> > On Tue, Apr 15, 2014 at 05:49:02PM +0800, Barry Song wrote: >> >> From: Barry Song <Baohua.Song@csr.com> >> >> >> >> L2 cache size has been read by l2x0_init(), it is useless to set >> >> WAY_SIZE and ASSOCIATIVITY in prima2. >> >> >> >> Cc: Russell King <linux@arm.linux.org.uk> >> >> Signed-off-by: Barry Song <Baohua.Song@csr.com> >> >> --- >> >> -Derived from Russell's "ARM: l2c: prima2: remove cache size override" >> > >> > You're a SMP architecture... it would be much better to initialise the >> > L2 cache before the secondary CPUs are brought online. >> >> ok, it seems we still need to hook this into the callback of this >> specific machine instead of using a global early_initcall(). since now >> the dt compatible field has been generic enough, it will cause other >> platforms to execute l2x0_of_init() too. > > What I did for Versatile Express was to move it to the init_irq stage. > I'm not condoning using init_irq() for non-IRQ stuff, but that was > just to see whether it was possible (the .init_early is too early for > it.) Let me put it another way: I'm not thrilled by the idea of > having it in init_irq() but that's a better location to call L2 cache > initialisation than half-way through the kernel's initialisation. i would like to believe you are right since you have dug into the callbacks and finally thinks init_irq() is the suitable stage for it even though it looks misnamed. > >> > In any case, across the board, it's preferable that the L2 cache is >> > enabled before the delay loop calibration as it can have an effect on >> > the calibrated value. >> >> what if the real case is that l2x0 has been enabled in bootloader? > > If the L2 cache has already been enabled, l2x0_of_init() handles it - > in that situation, we can't re-do the enable (because writing to the > aux ctrl register is not allowed) so we merely setup the outer cache > function pointers and don't try to do any configuration or enabling. yes. i meant if that has been done in bootloader, it turns out we don't care about which callback is better any more. and for linux running in non-security, same. > > -- > FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly > improving, and getting towards what was expected from it. - barry
On Wed, Apr 16, 2014 at 04:24:34PM +0800, Barry Song wrote: > yes. i meant if that has been done in bootloader, it turns out we > don't care about which callback is better any more. and for linux > running in non-security, same. However, I don't think it's quite as simple as that, because things like the Cortex A9 Full-Line-Of-Zeros feature need a specific enable sequence which (I think) can't be done in the boot loader.
On Tuesday 15 April 2014 22:56:15 Russell King - ARM Linux wrote: > On Tue, Apr 15, 2014 at 06:13:27PM +0800, Barry Song wrote: > > > > ok, it seems we still need to hook this into the callback of this > > specific machine instead of using a global early_initcall(). since now > > the dt compatible field has been generic enough, it will cause other > > platforms to execute l2x0_of_init() too. > > What I did for Versatile Express was to move it to the init_irq stage. > I'm not condoning using init_irq() for non-IRQ stuff, but that was > just to see whether it was possible (the .init_early is too early for > it.) Let me put it another way: I'm not thrilled by the idea of > having it in init_irq() but that's a better location to call L2 cache > initialisation than half-way through the kernel's initialisation. Would it help to introduce a new .init_l2_cache() callback? That way, the architecture code can default to initializing any l2x0 that it finds in DT, and have give platforms the option of overriding the call if they need to, and we can do it at the same point during early boot for everyone. A number of platforms currently have the same static void __init this_platform_init(void) { l2x0_of_init(0, ~0UL); of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); } Where the second line is already the default for .init_machine, and it would be nice to get rid of the callback entirely, especially for the machines that don't do anything else in their platform code. Arnd
2014-04-16 16:52 GMT+08:00 Arnd Bergmann <arnd@arndb.de>: > On Tuesday 15 April 2014 22:56:15 Russell King - ARM Linux wrote: >> On Tue, Apr 15, 2014 at 06:13:27PM +0800, Barry Song wrote: >> > >> > ok, it seems we still need to hook this into the callback of this >> > specific machine instead of using a global early_initcall(). since now >> > the dt compatible field has been generic enough, it will cause other >> > platforms to execute l2x0_of_init() too. >> >> What I did for Versatile Express was to move it to the init_irq stage. >> I'm not condoning using init_irq() for non-IRQ stuff, but that was >> just to see whether it was possible (the .init_early is too early for >> it.) Let me put it another way: I'm not thrilled by the idea of >> having it in init_irq() but that's a better location to call L2 cache >> initialisation than half-way through the kernel's initialisation. > > Would it help to introduce a new .init_l2_cache() callback? That > way, the architecture code can default to initializing any l2x0 > that it finds in DT, and have give platforms the option of > overriding the call if they need to, and we can do it at the > same point during early boot for everyone. > > A number of platforms currently have the same > > static void __init this_platform_init(void) > { > l2x0_of_init(0, ~0UL); > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > } > > Where the second line is already the default for .init_machine, > and it would be nice to get rid of the callback entirely, especially > for the machines that don't do anything else in their platform > code. it seems you mean put a general customize_l2x0_init() like customize_machine(). what is the reason stopping l2x0 from becoming a standalone driver which can initialize itself? > > Arnd -barry
On Wed, Apr 16, 2014 at 10:52:27AM +0200, Arnd Bergmann wrote: > On Tuesday 15 April 2014 22:56:15 Russell King - ARM Linux wrote: > > On Tue, Apr 15, 2014 at 06:13:27PM +0800, Barry Song wrote: > > > > > > ok, it seems we still need to hook this into the callback of this > > > specific machine instead of using a global early_initcall(). since now > > > the dt compatible field has been generic enough, it will cause other > > > platforms to execute l2x0_of_init() too. > > > > What I did for Versatile Express was to move it to the init_irq stage. > > I'm not condoning using init_irq() for non-IRQ stuff, but that was > > just to see whether it was possible (the .init_early is too early for > > it.) Let me put it another way: I'm not thrilled by the idea of > > having it in init_irq() but that's a better location to call L2 cache > > initialisation than half-way through the kernel's initialisation. > > Would it help to introduce a new .init_l2_cache() callback? That > way, the architecture code can default to initializing any l2x0 > that it finds in DT, and have give platforms the option of > overriding the call if they need to, and we can do it at the > same point during early boot for everyone. As my ideal target is to remove l2x0_*init from platforms, I'd rather not do that unless we have a clear idea that we are going to keep this around. Another potential solution would be to have the two auxctl values specified, and do nothing if they're both zero. First though, we really need to work out what each platform requires from these, and turn everything else into either something the core code handles, or if there is no good reason for it, it is simply removed.
On Wed, Apr 16, 2014 at 05:13:18PM +0800, Barry Song wrote: > 2014-04-16 16:52 GMT+08:00 Arnd Bergmann <arnd@arndb.de>: > > On Tuesday 15 April 2014 22:56:15 Russell King - ARM Linux wrote: > >> On Tue, Apr 15, 2014 at 06:13:27PM +0800, Barry Song wrote: > >> > > >> > ok, it seems we still need to hook this into the callback of this > >> > specific machine instead of using a global early_initcall(). since now > >> > the dt compatible field has been generic enough, it will cause other > >> > platforms to execute l2x0_of_init() too. > >> > >> What I did for Versatile Express was to move it to the init_irq stage. > >> I'm not condoning using init_irq() for non-IRQ stuff, but that was > >> just to see whether it was possible (the .init_early is too early for > >> it.) Let me put it another way: I'm not thrilled by the idea of > >> having it in init_irq() but that's a better location to call L2 cache > >> initialisation than half-way through the kernel's initialisation. > > > > Would it help to introduce a new .init_l2_cache() callback? That > > way, the architecture code can default to initializing any l2x0 > > that it finds in DT, and have give platforms the option of > > overriding the call if they need to, and we can do it at the > > same point during early boot for everyone. > > > > A number of platforms currently have the same > > > > static void __init this_platform_init(void) > > { > > l2x0_of_init(0, ~0UL); > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > } > > > > Where the second line is already the default for .init_machine, > > and it would be nice to get rid of the callback entirely, especially > > for the machines that don't do anything else in their platform > > code. > > it seems you mean put a general customize_l2x0_init() like customize_machine(). > > what is the reason stopping l2x0 from becoming a standalone driver > which can initialize itself? Well... if you didn't need to give this: l2x0_of_init(0, 0xFDFFFFFFUL); but instead could cope with: l2x0_of_init(0, ~0); then that would be one less reason for it /not/ to be a standalone driver. It's every "tweak" that the platform does that stands in the way.
On Tue, Apr 15, 2014 at 05:49:02PM +0800, Barry Song wrote: > +static __init void sirfsoc_init_mach(void) > +{ > + l2x0_of_init(0, 0xFDFFFFFFUL); BTW, this should be just the plain: 0xfdffffff - no need for UL on the end. Also lower case is preferred for hex.
On Wed, Apr 16, 2014 at 10:24:33AM +0100, Russell King - ARM Linux wrote: > On Tue, Apr 15, 2014 at 05:49:02PM +0800, Barry Song wrote: > > +static __init void sirfsoc_init_mach(void) > > +{ > > + l2x0_of_init(0, 0xFDFFFFFFUL); > > BTW, this should be just the plain: 0xfdffffff - no need for UL on the > end. Also lower case is preferred for hex. So... I provide feedback, and nothing happens. FYI, tonight, I'm pushing out my L2C branch into linux-next with the original patch for prima2 incorporated, as my concerns against your patch have not been addressed.
2014-04-24 2:21 GMT+08:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > On Wed, Apr 16, 2014 at 10:24:33AM +0100, Russell King - ARM Linux wrote: >> On Tue, Apr 15, 2014 at 05:49:02PM +0800, Barry Song wrote: >> > +static __init void sirfsoc_init_mach(void) >> > +{ >> > + l2x0_of_init(0, 0xFDFFFFFFUL); >> >> BTW, this should be just the plain: 0xfdffffff - no need for UL on the >> end. Also lower case is preferred for hex. > > So... I provide feedback, and nothing happens. > > FYI, tonight, I'm pushing out my L2C branch into linux-next with the > original patch for prima2 incorporated, as my concerns against your > patch have not been addressed. Russell, i do agree with your feedback. but, in your original patch, moving to early_initcall will make other non-csr platforms call l2x0_of_init() and finish initialization by (0, ~0) directly since other platforms also put same general compatible prop in dts. in your patch, you have dropped "sirf,prima2-pl310-cache" and "sirf,marco-pl310-cache", so it happens by matching general "arm,pl310-cache". i am still on the way to send v2. if it is not too urgent to you, will you wait for me to send v2 by fixing issues in your feedback? > -barry
On Thu, Apr 24, 2014 at 07:11:46AM +0800, Barry Song wrote: > 2014-04-24 2:21 GMT+08:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > > On Wed, Apr 16, 2014 at 10:24:33AM +0100, Russell King - ARM Linux wrote: > >> On Tue, Apr 15, 2014 at 05:49:02PM +0800, Barry Song wrote: > >> > +static __init void sirfsoc_init_mach(void) > >> > +{ > >> > + l2x0_of_init(0, 0xFDFFFFFFUL); > >> > >> BTW, this should be just the plain: 0xfdffffff - no need for UL on the > >> end. Also lower case is preferred for hex. > > > > So... I provide feedback, and nothing happens. > > > > FYI, tonight, I'm pushing out my L2C branch into linux-next with the > > original patch for prima2 incorporated, as my concerns against your > > patch have not been addressed. > > Russell, i do agree with your feedback. > but, in your original patch, moving to early_initcall will make other > non-csr platforms call l2x0_of_init() and finish initialization by (0, > ~0) directly since other platforms also put same general compatible > prop in dts. in your patch, you have dropped "sirf,prima2-pl310-cache" > and "sirf,marco-pl310-cache", so it happens by matching general > "arm,pl310-cache". > i am still on the way to send v2. if it is not too urgent to you, will > you wait for me to send v2 by fixing issues in your feedback? I have no problem adding a patch, or replacing my patch in the series. All I ask is that it isn't forgotten, and that it's sent before -rc6.
diff --git a/arch/arm/boot/dts/marco.dtsi b/arch/arm/boot/dts/marco.dtsi index 0c9647d..fb35422 100644 --- a/arch/arm/boot/dts/marco.dtsi +++ b/arch/arm/boot/dts/marco.dtsi @@ -36,7 +36,7 @@ ranges = <0x40000000 0x40000000 0xa0000000>; l2-cache-controller@c0030000 { - compatible = "sirf,marco-pl310-cache", "arm,pl310-cache"; + compatible = "arm,pl310-cache"; reg = <0xc0030000 0x1000>; interrupts = <0 59 0>; arm,tag-latency = <1 1 1>; diff --git a/arch/arm/boot/dts/prima2.dtsi b/arch/arm/boot/dts/prima2.dtsi index 1e82571..0d6588d 100644 --- a/arch/arm/boot/dts/prima2.dtsi +++ b/arch/arm/boot/dts/prima2.dtsi @@ -48,7 +48,7 @@ ranges = <0x40000000 0x40000000 0x80000000>; l2-cache-controller@80040000 { - compatible = "arm,pl310-cache", "sirf,prima2-pl310-cache"; + compatible = "arm,pl310-cache"; reg = <0x80040000 0x1000>; interrupts = <59>; arm,tag-latency = <1 1 1>; diff --git a/arch/arm/mach-prima2/common.c b/arch/arm/mach-prima2/common.c index 47c7819..23fcade 100644 --- a/arch/arm/mach-prima2/common.c +++ b/arch/arm/mach-prima2/common.c @@ -8,11 +8,10 @@ #include <linux/init.h> #include <linux/kernel.h> -#include <asm/sizes.h> -#include <asm/mach-types.h> -#include <asm/mach/arch.h> #include <linux/of.h> #include <linux/of_platform.h> +#include <asm/hardware/cache-l2x0.h> +#include <asm/mach/arch.h> #include "common.h" static void __init sirfsoc_init_late(void) @@ -26,6 +25,12 @@ static __init void sirfsoc_map_io(void) sirfsoc_map_scu(); } +static __init void sirfsoc_init_mach(void) +{ + l2x0_of_init(0, 0xFDFFFFFFUL); + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); +} + #ifdef CONFIG_ARCH_ATLAS6 static const char *atlas6_dt_match[] __initconst = { "sirf,atlas6", @@ -35,6 +40,7 @@ static const char *atlas6_dt_match[] __initconst = { DT_MACHINE_START(ATLAS6_DT, "Generic ATLAS6 (Flattened Device Tree)") /* Maintainer: Barry Song <baohua.song@csr.com> */ .map_io = sirfsoc_map_io, + .init_machine = sirfsoc_init_mach, .init_late = sirfsoc_init_late, .dt_compat = atlas6_dt_match, MACHINE_END @@ -50,6 +56,7 @@ DT_MACHINE_START(PRIMA2_DT, "Generic PRIMA2 (Flattened Device Tree)") /* Maintainer: Barry Song <baohua.song@csr.com> */ .map_io = sirfsoc_map_io, .dma_zone_size = SZ_256M, + .init_machine = sirfsoc_init_mach, .init_late = sirfsoc_init_late, .dt_compat = prima2_dt_match, MACHINE_END @@ -65,6 +72,7 @@ DT_MACHINE_START(MARCO_DT, "Generic MARCO (Flattened Device Tree)") /* Maintainer: Barry Song <baohua.song@csr.com> */ .smp = smp_ops(sirfsoc_smp_ops), .map_io = sirfsoc_map_io, + .init_machine = sirfsoc_init_mach, .init_late = sirfsoc_init_late, .dt_compat = marco_dt_match, MACHINE_END diff --git a/arch/arm/mach-prima2/l2x0.c b/arch/arm/mach-prima2/l2x0.c deleted file mode 100644 index c710253..0000000 --- a/arch/arm/mach-prima2/l2x0.c +++ /dev/null @@ -1,49 +0,0 @@ -/* - * l2 cache initialization for CSR SiRFprimaII - * - * Copyright (c) 2011 Cambridge Silicon Radio Limited, a CSR plc group company. - * - * Licensed under GPLv2 or later. - */ - -#include <linux/init.h> -#include <linux/kernel.h> -#include <linux/of.h> -#include <asm/hardware/cache-l2x0.h> - -struct l2x0_aux { - u32 val; - u32 mask; -}; - -static const struct l2x0_aux prima2_l2x0_aux __initconst = { - .val = 2 << L2X0_AUX_CTRL_WAY_SIZE_SHIFT, - .mask = 0, -}; - -static const struct l2x0_aux marco_l2x0_aux __initconst = { - .val = (2 << L2X0_AUX_CTRL_WAY_SIZE_SHIFT) | - (1 << L2X0_AUX_CTRL_ASSOCIATIVITY_SHIFT), - .mask = L2X0_AUX_CTRL_MASK, -}; - -static const struct of_device_id sirf_l2x0_ids[] __initconst = { - { .compatible = "sirf,prima2-pl310-cache", .data = &prima2_l2x0_aux, }, - { .compatible = "sirf,marco-pl310-cache", .data = &marco_l2x0_aux, }, - {}, -}; - -static int __init sirfsoc_l2x0_init(void) -{ - struct device_node *np; - const struct l2x0_aux *aux; - - np = of_find_matching_node(NULL, sirf_l2x0_ids); - if (np) { - aux = of_match_node(sirf_l2x0_ids, np)->data; - return l2x0_of_init(aux->val, aux->mask); - } - - return 0; -} -early_initcall(sirfsoc_l2x0_init);