| Message ID | 20170202212000.10768-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
On Thu, Feb 02, 2017 at 04:20:00PM -0500, Chris Brandt wrote: > This enables the 128KB L2 cache in the RZ/A1 (R7S72100). > > The 'Write full line of zeros mode' of this Cortex-A9 cannot be used > because the sideband signals between the CA9 and PL310 are not connected. > Since there is no option to disable this feature in the cache-l2x0 driver, > our only option is to specify a secure write function which will then cause > the cache-l2x0 driver to not enable this feature. > > If you do not override a l2c_write_sec function which causes the line of > zeros mode to be enabled, then the system will crash pretty quickly after > the L2C is enabled. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Hi Chris, is it possible to apply the .dtsi and .c portions of this change separately and still get sane behaviour at each step? If so I would like to request that this patch be split into two patches, one for .c and one for .dtsi. This will allow me to apply the patches to separate branches which will in turn make it easier for me to get the change accepted. > --- > arch/arm/boot/dts/r7s72100.dtsi | 9 +++++++++ > arch/arm/mach-shmobile/setup-r7s72100.c | 21 +++++++++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi > index 74e684f..08aaaff 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -177,6 +177,7 @@ > compatible = "arm,cortex-a9"; > reg = <0>; > clock-frequency = <400000000>; > + next-level-cache = <&L2>; > }; > }; > > @@ -368,6 +369,14 @@ > <0xe8202000 0x1000>; > }; > > + L2: cache-controller@3ffff000 { > + compatible = "arm,pl310-cache"; > + reg = <0x3ffff000 0x1000>; > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > + cache-unified; > + cache-level = <2>; > + }; > + > i2c0: i2c@fcfee000 { > #address-cells = <1>; > #size-cells = <0>; > diff --git a/arch/arm/mach-shmobile/setup-r7s72100.c b/arch/arm/mach-shmobile/setup-r7s72100.c > index d46639f..655deba 100644 > --- a/arch/arm/mach-shmobile/setup-r7s72100.c > +++ b/arch/arm/mach-shmobile/setup-r7s72100.c > @@ -15,6 +15,7 @@ > */ > > #include <linux/kernel.h> > +#include <linux/io.h> > > #include <asm/mach/arch.h> > > @@ -25,7 +26,27 @@ static const char *const r7s72100_boards_compat_dt[] __initconst = { > NULL, > }; > > +/* > + * The 'Write full line of zeros mode' of this Cortex-A9 cannot be used because > + * the sideband signals between the CA9 and PL310 are not connected. Since there > + * is no option to disable this feature in the cache-l2x0 driver, our only > + * option is to specify a secure write function which will then cause the > + * cache-l2x0 driver to not enable this feature. > + */ > +static void r7s72100_l2c_write_sec(unsigned long val, unsigned int reg) > +{ > + static void __iomem *base; > + > + if (!base) > + base = ioremap_nocache(0x3ffff000, SZ_4K); > + > + writel_relaxed(val, base + reg); > +} > + > DT_MACHINE_START(R7S72100_DT, "Generic R7S72100 (Flattened Device Tree)") > + .l2c_aux_val = 0, > + .l2c_aux_mask = ~0, > + .l2c_write_sec = r7s72100_l2c_write_sec, > .init_early = shmobile_init_delay, > .init_late = shmobile_init_late, > .dt_compat = r7s72100_boards_compat_dt, > -- > 2.10.1 > >
Hi Chris, CC linux-arm-kernel On Thu, Feb 2, 2017 at 10:20 PM, Chris Brandt <chris.brandt@renesas.com> wrote: > This enables the 128KB L2 cache in the RZ/A1 (R7S72100). > > The 'Write full line of zeros mode' of this Cortex-A9 cannot be used > because the sideband signals between the CA9 and PL310 are not connected. > Since there is no option to disable this feature in the cache-l2x0 driver, > our only option is to specify a secure write function which will then cause > the cache-l2x0 driver to not enable this feature. What about adding a DT property (e.g. "arm,pl310-broken-sideband", cfr. "arm,pl330-broken-no-flushp"), and handling this in arch/arm/mm/cache-l2x0.c instead? > If you do not override a l2c_write_sec function which causes the line of > zeros mode to be enabled, then the system will crash pretty quickly after > the L2C is enabled. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > arch/arm/boot/dts/r7s72100.dtsi | 9 +++++++++ > arch/arm/mach-shmobile/setup-r7s72100.c | 21 +++++++++++++++++++++ > 2 files changed, 30 insertions(+) > > diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi > index 74e684f..08aaaff 100644 > --- a/arch/arm/boot/dts/r7s72100.dtsi > +++ b/arch/arm/boot/dts/r7s72100.dtsi > @@ -177,6 +177,7 @@ > compatible = "arm,cortex-a9"; > reg = <0>; > clock-frequency = <400000000>; > + next-level-cache = <&L2>; > }; > }; > > @@ -368,6 +369,14 @@ > <0xe8202000 0x1000>; > }; > > + L2: cache-controller@3ffff000 { > + compatible = "arm,pl310-cache"; > + reg = <0x3ffff000 0x1000>; > + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; > + cache-unified; > + cache-level = <2>; > + }; > + > i2c0: i2c@fcfee000 { > #address-cells = <1>; > #size-cells = <0>; > diff --git a/arch/arm/mach-shmobile/setup-r7s72100.c b/arch/arm/mach-shmobile/setup-r7s72100.c > index d46639f..655deba 100644 > --- a/arch/arm/mach-shmobile/setup-r7s72100.c > +++ b/arch/arm/mach-shmobile/setup-r7s72100.c > @@ -15,6 +15,7 @@ > */ > > #include <linux/kernel.h> > +#include <linux/io.h> > > #include <asm/mach/arch.h> > > @@ -25,7 +26,27 @@ static const char *const r7s72100_boards_compat_dt[] __initconst = { > NULL, > }; > > +/* > + * The 'Write full line of zeros mode' of this Cortex-A9 cannot be used because > + * the sideband signals between the CA9 and PL310 are not connected. Since there > + * is no option to disable this feature in the cache-l2x0 driver, our only > + * option is to specify a secure write function which will then cause the > + * cache-l2x0 driver to not enable this feature. > + */ > +static void r7s72100_l2c_write_sec(unsigned long val, unsigned int reg) > +{ > + static void __iomem *base; > + > + if (!base) > + base = ioremap_nocache(0x3ffff000, SZ_4K); FWIW, plain ioremap() is fine. > + > + writel_relaxed(val, base + reg); > +} > + > DT_MACHINE_START(R7S72100_DT, "Generic R7S72100 (Flattened Device Tree)") > + .l2c_aux_val = 0, > + .l2c_aux_mask = ~0, > + .l2c_write_sec = r7s72100_l2c_write_sec, > .init_early = shmobile_init_delay, > .init_late = shmobile_init_late, > .dt_compat = r7s72100_boards_compat_dt, Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Simon, On Monday, February 06, 2017, Simon Horman wrote: > is it possible to apply the .dtsi and .c portions of this change > separately and still get sane behaviour at each step? > > If so I would like to request that this patch be split into two patches, > one for .c and one for .dtsi. This will allow me to apply the patches to > separate branches which will in turn make it easier for me to get the > change accepted. OK, no problem. I tried thinking about which way you guys would want it...I had a 50/50 shot...I picked the wrong one ;) Chris
Hi Geert, On Monday, February 06, 2017, Geert Uytterhoeven wrote: > CC linux-arm-kernel > > On Thu, Feb 2, 2017 at 10:20 PM, Chris Brandt <chris.brandt@renesas.com> > wrote: > > This enables the 128KB L2 cache in the RZ/A1 (R7S72100). > > > > The 'Write full line of zeros mode' of this Cortex-A9 cannot be used > > because the sideband signals between the CA9 and PL310 are not connected. > > Since there is no option to disable this feature in the cache-l2x0 > > driver, our only option is to specify a secure write function which > > will then cause the cache-l2x0 driver to not enable this feature. > > What about adding a DT property (e.g. "arm,pl310-broken-sideband", cfr. > "arm,pl330-broken-no-flushp"), and handling this in arch/arm/mm/cache- > l2x0.c instead? Well, first I have to say that 'broken-sideband' is not actually "accurate" in this case. From the RZ/A1H Hardware Manual: 4. Secondary Cache 4.1 Features * Sideband signal from CA9: No So the chip designers knew the sideband signals were not connected. If you have a look at the next chapter "5. LSI Internal Bus", you'll notice that the CA9 is on the North bus (fig 5.2) but the PL310 is on the south bus (fig 5.3) in between the AXI and the SDRAM/QSPI controller. So in this in SoC, maybe the PL310 looks more like a L3 than an L2 cache??? So, I would say "arm,pl310-no-sideband" is a better name. I agree that faking out a secure write function just so the fill-zeros sideband feature is not enabled is a bit of a hack, but I'm not sure if modifying the cache-l2x0.c was an option. If you think so, I can try the "arm,pl310-no-sideband" path first, and if that doesn't get in I can fall back to what I'm doing now. Thoughts??? > > +static void r7s72100_l2c_write_sec(unsigned long val, unsigned int > > +reg) { > > + static void __iomem *base; > > + > > + if (!base) > > + base = ioremap_nocache(0x3ffff000, SZ_4K); > > FWIW, plain ioremap() is fine. Of course they both go to the same thing for ARM, but I thought the "_nocache" version should be used for sanity purposes (as ie, "yes, I know what I'm doing"). Thanks, Chris
Hi Chris, On Mon, Feb 6, 2017 at 3:58 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Monday, February 06, 2017, Geert Uytterhoeven wrote: >> CC linux-arm-kernel >> >> On Thu, Feb 2, 2017 at 10:20 PM, Chris Brandt <chris.brandt@renesas.com> >> wrote: >> > This enables the 128KB L2 cache in the RZ/A1 (R7S72100). >> > >> > The 'Write full line of zeros mode' of this Cortex-A9 cannot be used >> > because the sideband signals between the CA9 and PL310 are not connected. >> > Since there is no option to disable this feature in the cache-l2x0 >> > driver, our only option is to specify a secure write function which >> > will then cause the cache-l2x0 driver to not enable this feature. >> >> What about adding a DT property (e.g. "arm,pl310-broken-sideband", cfr. >> "arm,pl330-broken-no-flushp"), and handling this in arch/arm/mm/cache- >> l2x0.c instead? > > Well, first I have to say that 'broken-sideband' is not actually "accurate" > in this case. > > From the RZ/A1H Hardware Manual: > > 4. Secondary Cache > 4.1 Features > > * Sideband signal from CA9: No OK. > So the chip designers knew the sideband signals were not connected. > If you have a look at the next chapter "5. LSI Internal Bus", you'll notice > that the CA9 is on the North bus (fig 5.2) but the PL310 is on the south > bus (fig 5.3) in between the AXI and the SDRAM/QSPI controller. So in this > in SoC, maybe the PL310 looks more like a L3 than an L2 cache??? No, according to Figures 5.1 and 5.3, the CA9 is connected to both the North and South main buses. > So, I would say "arm,pl310-no-sideband" is a better name. OK. > I agree that faking out a secure write function just so the fill-zeros > sideband feature is not enabled is a bit of a hack, but I'm not sure if > modifying the cache-l2x0.c was an option. Given I've added "arm,shared-override" in the past, I'd say yes ;-) > If you think so, I can try the "arm,pl310-no-sideband" path first, > and if that doesn't get in I can fall back to what I'm doing now. > > Thoughts??? According to the "CoreLink Level 2 Cache Controller L2C-310" TRM, "no sideband signals" is even the default configuration? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Monday, February 06, 2017, Geert Uytterhoeven wrote: > > I agree that faking out a secure write function just so the fill-zeros > > sideband feature is not enabled is a bit of a hack, but I'm not sure > > if modifying the cache-l2x0.c was an option. > > Given I've added "arm,shared-override" in the past, I'd say yes ;-) > > > If you think so, I can try the "arm,pl310-no-sideband" path first, and > > if that doesn't get in I can fall back to what I'm doing now. > > > > Thoughts??? > > According to the "CoreLink Level 2 Cache Controller L2C-310" TRM, "no > sideband signals" is even the default configuration? Good point. So technically there is nothing "illegal" about hooking up only the AXI interface and not the sideband signals. I guess that also means things like "Early BRESP Enable" are not really going to work either. Of course you'll still see "L2C-310 enabling early BRESP for Cortex-A9" printed out on boot, which is true because it was enabled in the PL310, but in reality it will never get used. False hope :( At first I'll put together a patch just for Full line of zero write since that one changes the behavior of the CA9....and will break a system. Thanks, Chris
diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi index 74e684f..08aaaff 100644 --- a/arch/arm/boot/dts/r7s72100.dtsi +++ b/arch/arm/boot/dts/r7s72100.dtsi @@ -177,6 +177,7 @@ compatible = "arm,cortex-a9"; reg = <0>; clock-frequency = <400000000>; + next-level-cache = <&L2>; }; }; @@ -368,6 +369,14 @@ <0xe8202000 0x1000>; }; + L2: cache-controller@3ffff000 { + compatible = "arm,pl310-cache"; + reg = <0x3ffff000 0x1000>; + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>; + cache-unified; + cache-level = <2>; + }; + i2c0: i2c@fcfee000 { #address-cells = <1>; #size-cells = <0>; diff --git a/arch/arm/mach-shmobile/setup-r7s72100.c b/arch/arm/mach-shmobile/setup-r7s72100.c index d46639f..655deba 100644 --- a/arch/arm/mach-shmobile/setup-r7s72100.c +++ b/arch/arm/mach-shmobile/setup-r7s72100.c @@ -15,6 +15,7 @@ */ #include <linux/kernel.h> +#include <linux/io.h> #include <asm/mach/arch.h> @@ -25,7 +26,27 @@ static const char *const r7s72100_boards_compat_dt[] __initconst = { NULL, }; +/* + * The 'Write full line of zeros mode' of this Cortex-A9 cannot be used because + * the sideband signals between the CA9 and PL310 are not connected. Since there + * is no option to disable this feature in the cache-l2x0 driver, our only + * option is to specify a secure write function which will then cause the + * cache-l2x0 driver to not enable this feature. + */ +static void r7s72100_l2c_write_sec(unsigned long val, unsigned int reg) +{ + static void __iomem *base; + + if (!base) + base = ioremap_nocache(0x3ffff000, SZ_4K); + + writel_relaxed(val, base + reg); +} + DT_MACHINE_START(R7S72100_DT, "Generic R7S72100 (Flattened Device Tree)") + .l2c_aux_val = 0, + .l2c_aux_mask = ~0, + .l2c_write_sec = r7s72100_l2c_write_sec, .init_early = shmobile_init_delay, .init_late = shmobile_init_late, .dt_compat = r7s72100_boards_compat_dt,
This enables the 128KB L2 cache in the RZ/A1 (R7S72100). The 'Write full line of zeros mode' of this Cortex-A9 cannot be used because the sideband signals between the CA9 and PL310 are not connected. Since there is no option to disable this feature in the cache-l2x0 driver, our only option is to specify a secure write function which will then cause the cache-l2x0 driver to not enable this feature. If you do not override a l2c_write_sec function which causes the line of zeros mode to be enabled, then the system will crash pretty quickly after the L2C is enabled. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- arch/arm/boot/dts/r7s72100.dtsi | 9 +++++++++ arch/arm/mach-shmobile/setup-r7s72100.c | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+)