Re: [PATCH] ARM: at91: sama5: configure L2 cache

[Alexandre Belloni]

On 18/09/2014 at 22:02:12 +0100, Russell King - ARM Linux wrote :
> On Thu, Sep 18, 2014 at 10:38:36PM +0200, Alexandre Belloni wrote:
> > From: Wenyou Yang <wenyou.yang@xxxxxxxxx>
> > 
> > Ensure that the L2 cache configuration is optimal to avoid depending on the
> > bootloader to set it correctly.
> > 
> > Signed-off-by: Wenyou Yang <wenyou.yang@xxxxxxxxx>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx>
> > ---
> >  arch/arm/mach-at91/board-dt-sama5.c | 47 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 46 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-at91/board-dt-sama5.c b/arch/arm/mach-at91/board-dt-sama5.c
> > index 548de4ad6937..7cbe0d9daf2b 100644
> > --- a/arch/arm/mach-at91/board-dt-sama5.c
> > +++ b/arch/arm/mach-at91/board-dt-sama5.c
> > @@ -13,12 +13,14 @@
> >  #include <linux/gpio.h>
> >  #include <linux/micrel_phy.h>
> >  #include <linux/of.h>
> > +#include <linux/of_address.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/phy.h>
> >  #include <linux/clk-provider.h>
> >  
> >  #include <asm/setup.h>
> > +#include <asm/hardware/cache-l2x0.h>
> >  #include <asm/irq.h>
> >  #include <asm/mach/arch.h>
> >  #include <asm/mach/map.h>
> > @@ -35,8 +37,52 @@ static void __init sama5_dt_timer_init(void)
> >  	at91sam926x_pit_init();
> >  }
> >  
> > +static void __init at91_l2x0_init(void)
> > +{
> > +	struct device_node *np;
> > +	void __iomem *l2cc_base;
> > +	u32 reg;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> > +	if (!np)
> > +		return;
> > +
> > +	l2cc_base = of_iomap(np, 0);
> > +	of_node_put(np);
> > +
> > +	if (!l2cc_base) {
> > +		pr_err("L2C-310 unable to map registers\n");
> > +		return;
> > +	}
> > +
> > +	/* Prefetch Control */
> > +	reg = readl_relaxed(l2cc_base + L310_PREFETCH_CTRL);
> > +	reg &= ~L310_PREFETCH_CTRL_OFFSET_MASK;
> > +	reg |= 0x01 & L310_PREFETCH_CTRL_OFFSET_MASK;
> > +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
> > +	reg |= L310_PREFETCH_CTRL_PREFETCH_DROP;
> > +	reg |= L310_PREFETCH_CTRL_DATA_PREFETCH;
> > +	reg |= L310_PREFETCH_CTRL_INSTR_PREFETCH;
> > +	reg |= L310_PREFETCH_CTRL_DBL_LINEFILL;
> > +	writel_relaxed(reg, l2cc_base + L310_PREFETCH_CTRL);
> > +
> > +	/* Power Control */
> > +	reg = readl_relaxed(l2cc_base + L310_POWER_CTRL);
> > +	reg |= L310_STNDBY_MODE_EN;
> > +	reg |= L310_DYNAMIC_CLK_GATING_EN;
> > +	writel_relaxed(reg, l2cc_base + L310_POWER_CTRL);
> > +
> > +	/* Disable interrupts */
> > +	writel_relaxed(0x00, l2cc_base + L2X0_INTR_MASK);
> > +	writel_relaxed(0x01ff, l2cc_base + L2X0_INTR_CLEAR);
> > +
> > +	l2x0_of_init(0, ~0UL);
> 
> NAK.  Really, nak.  Stop this mentality of working around shortcomings
> of generic code by adding yet more platform junk.  Such approaches are
> not acceptable.
> 

Thank you for that confirmation, that was the main reason to Cc you.

> The power control is already done by generic code.  I know that you've
> developed the above code against the exact copy of generic code which
> has this, because you're using the new symbols.
> 

Yeah I actually forgot to remove that part when "porting" from vendor
code to use your defines.

> You shouldn't need to disable interrupts; the interrupts should already
> be disabled unless your bootloaders are doing something weird with them.
> 
> There have been DT bindings proposed for prefetch control register.  I
> suggest that you search this mailing list for that patch, and check
> whether it is acceptable for your platform.
> 

I'm really wondering whether we should really put that in the device
tree... We will soon end up with a property for each bit of each
registers and the binding will end up being huge. Also, that is
configuration, not HW description.

I actually tried multiple things, without any satisfaction:
 - using DT, with the main issue that we will definitely end up with one
   property per bit of configuration

 - adding an .l2c_prefetch_val to the machine start but that is kind of
   ugly.

 - adding a new parameter to l2x0_of_init()

So I ended up choosing to do it in the platform code. But if everybody
is fine with adding more properties to DT, I can go that way.

> Taking all that together, you should need /zero/ code in your platform
> for L2 caches, which is *the way it should be*.
> 

I would also prefer it that way.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/