[PATCH 10/21] ARM: MM: Add DT binding for Feroceon L2 cache

[Jason Cooper]

On Fri, Feb 07, 2014 at 10:32:06AM +0100, Andrew Lunn wrote:
> On Thu, Feb 06, 2014 at 08:27:52PM -0500, Jason Cooper wrote:
> > On Fri, Feb 07, 2014 at 12:42:06AM +0100, Andrew Lunn wrote:
> > > Instantiate the L2 cache from DT. Indicate in DT where the cache
> > > control register is and if write through should be made.
> > > 
> > > Signed-off-by: Andrew Lunn <andrew at lunn.ch>
> > > ---
> > >  .../devicetree/bindings/arm/mrvl/foroceon.txt      | 19 +++++++++
> > >  arch/arm/boot/dts/kirkwood.dtsi                    |  5 +++
> > >  arch/arm/include/asm/hardware/cache-feroceon-l2.h  |  2 +
> > >  arch/arm/mach-kirkwood/board-dt.c                  | 15 +------
> > >  arch/arm/mm/cache-feroceon-l2.c                    | 46 ++++++++++++++++++++++
> > >  5 files changed, 73 insertions(+), 14 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/arm/mrvl/foroceon.txt
> > 
> > for the next revision, please split out the dtsi changes into a separate
> > patch.
> > 
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/mrvl/foroceon.txt b/Documentation/devicetree/bindings/arm/mrvl/foroceon.txt
> > > new file mode 100644
> > > index 000000000000..8058676d1476
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm/mrvl/foroceon.txt
> > 
> > name typo.
> > 
> > > @@ -0,0 +1,19 @@
> > > +* Marvell Feroceon Cache
> > > +
> > > +Required properties:
> > > +- compatible : Should be "marvell,feroceon-kirkwood".
> > 
> > This is a little ambiguous, I'd like to see 'l2' in the compatible string.
> 
> I will take Jason's advice on naming and change this..

Ack.  (I assume you meant JasonG)

> > > +#ifdef CONFIG_OF
> > > +static const struct of_device_id feroceon_ids[] __initconst = {
> > > +	{ .compatible = "marvell,feroceon-kirkwood"},
> > > +	{}
> > > +};
> > > +
> > > +int __init feroceon_of_init(void)
> > > +{
> > > +	struct device_node *node;
> > > +	void __iomem *base;
> > > +	bool writethrough = false;
> > > +	struct resource res;
> > > +
> > > +	node = of_find_matching_node(NULL, feroceon_ids);
> > > +	if (!node) {
> > > +		pr_info("Didn't find marvell,feroceon-*, not enabling it\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > I'd prefer to fallback to hardcoded register address here.  We know
> > we're on kirkwood at this point.
> 
> We could also be on mv78xx0, sometime in the future. So we need to at
> least look at the compatible string to see what SoC we are. It is also
> rather ugly having hard coded addresses. We might as well go back to
> platform devices.

reverting to default behavior for missing dt information is an
acceptable compromise to the evolving DT landscape.  However...

> I would prefer upping this to pr_err(FW_INFO, and not falling back to
> hard coded values. This is not a fatal error, the machine continues to
> run, just a bit slower.

You are correct, and it would encourage the user to update their DT
blob.  So I'm fine with the way you have it.

thx,

Jason.