| Message ID | 1391730137-14814-11-git-send-email-andrew@lunn.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 07, 2014 at 12:42:06AM +0100, Andrew Lunn wrote: > +#define L2_WRITETHROUGH_KIRKWOOD 0x00000010 BIT(x)? > +#ifdef CONFIG_OF > +static const struct of_device_id feroceon_ids[] __initconst = { > + { .compatible = "marvell,feroceon-kirkwood"}, > + {} > +} How about following the naming convention from l2x0: "marvell,kirkwood-cache" "marvell,feroceon-cache" > + if (writethrough) { > + writel(readl(base) | L2_WRITETHROUGH_KIRKWOOD, base); > + feroceon_l2_init(1); > + } else { > + writel(readl(base) & ~L2_WRITETHROUGH_KIRKWOOD, base); > + feroceon_l2_init(0); This should only happen for "marvell,kirkwood-cache" - it is very kirkwood specific.. Someday mv78xx0 will have a different bit of code. Maybe pass -1 to feroceon_l2_init and don't print the writethrough type at all for the "marvell,feroceon-cache" case? Regards, Jason
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@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. > +- reg : Address of the L2 cache control register > + > +Optional properties: > +- writethrough : only if present, the cache will be used in write through mode. > + > +Example: > + l2: l2-cache@20128 { > + compatible = "marvell,marvell,feroceon-kirkwood"; only need a single 'marvell'. > + reg = <0x20128 0x4>; > + }; > + > +There are at least two variants of the Feroceon, differing in how > +write through is enabled or not. If mv78xx0 support is added, it is > +expected to have a different compatibility string. > + > diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi > index 6abf44d257df..1d8129ac2672 100644 > --- a/arch/arm/boot/dts/kirkwood.dtsi > +++ b/arch/arm/boot/dts/kirkwood.dtsi > @@ -161,6 +161,11 @@ > #clock-cells = <1>; > }; > > + l2: l2-cache@20128 { > + compatible = "marvell,feroceon-kirkwood"; > + reg = <0x20128 0x4>; > + }; > + > intc: main-interrupt-ctrl@20200 { > compatible = "marvell,orion-intc"; > interrupt-controller; > diff --git a/arch/arm/include/asm/hardware/cache-feroceon-l2.h b/arch/arm/include/asm/hardware/cache-feroceon-l2.h > index 8edd330aabf6..12e1588dc4f1 100644 > --- a/arch/arm/include/asm/hardware/cache-feroceon-l2.h > +++ b/arch/arm/include/asm/hardware/cache-feroceon-l2.h > @@ -9,3 +9,5 @@ > */ > > extern void __init feroceon_l2_init(int l2_wt_override); > +extern int __init feroceon_of_init(void); > + > diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c > index 34c35510fd17..2ef59ee2182d 100644 > --- a/arch/arm/mach-kirkwood/board-dt.c > +++ b/arch/arm/mach-kirkwood/board-dt.c > @@ -42,19 +42,6 @@ static void __init kirkwood_map_io(void) > iotable_init(kirkwood_io_desc, ARRAY_SIZE(kirkwood_io_desc)); > } > > -static void __init kirkwood_l2_init(void) > -{ > -#ifdef CONFIG_CACHE_FEROCEON_L2 > -#ifdef CONFIG_CACHE_FEROCEON_L2_WRITETHROUGH > - writel(readl(L2_CONFIG_REG) | L2_WRITETHROUGH, L2_CONFIG_REG); > - feroceon_l2_init(1); > -#else > - writel(readl(L2_CONFIG_REG) & ~L2_WRITETHROUGH, L2_CONFIG_REG); > - feroceon_l2_init(0); > -#endif > -#endif > -} > - > static struct resource kirkwood_cpufreq_resources[] = { > [0] = { > .start = CPU_CONTROL_PHYS, > @@ -211,7 +198,7 @@ static void __init kirkwood_dt_init(void) > > BUG_ON(mvebu_mbus_dt_init()); > > - kirkwood_l2_init(); > + feroceon_of_init(); > > kirkwood_cpufreq_init(); > kirkwood_cpuidle_init(); > diff --git a/arch/arm/mm/cache-feroceon-l2.c b/arch/arm/mm/cache-feroceon-l2.c > index 898362e7972b..193fe7dbdb12 100644 > --- a/arch/arm/mm/cache-feroceon-l2.c > +++ b/arch/arm/mm/cache-feroceon-l2.c > @@ -13,11 +13,17 @@ > */ > > #include <linux/init.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > #include <linux/highmem.h> > +#include <linux/io.h> > #include <asm/cacheflush.h> > #include <asm/cp15.h> > #include <asm/hardware/cache-feroceon-l2.h> > > +#define L2_WRITETHROUGH_KIRKWOOD 0x00000010 > + > + > /* > * Low-level cache maintenance operations. > * > @@ -350,3 +356,43 @@ void __init feroceon_l2_init(int __l2_wt_override) > printk(KERN_INFO "Feroceon L2: Cache support initialised%s.\n", > l2_wt_override ? ", in WT override mode" : ""); > } > +#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. thx, Jason. > + > + if (of_property_read_bool(node, "writethrough")) > + writethrough = true; > + > + if (of_address_to_resource(node, 0, &res)) > + return -ENODEV; > + > + base = ioremap(res.start, resource_size(&res)); > + if (!base) > + return -ENOMEM; > + > + if (writethrough) { > + writel(readl(base) | L2_WRITETHROUGH_KIRKWOOD, base); > + feroceon_l2_init(1); > + } else { > + writel(readl(base) & ~L2_WRITETHROUGH_KIRKWOOD, base); > + feroceon_l2_init(0); > + } > + > + return 0; > +} > +#endif > -- > 1.8.5.3 >
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@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.. > > +#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. 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. Andrew
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@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.
On 06/02/14 23:42, 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@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 > > 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 > @@ -0,0 +1,19 @@ > +* Marvell Feroceon Cache > + > +Required properties: > +- compatible : Should be "marvell,feroceon-kirkwood". > +- reg : Address of the L2 cache control register > + > +Optional properties: > +- writethrough : only if present, the cache will be used in write through mode. > + Looks more like a software configuration for me unless I am missing something. It should not be here IMO if its pure software construct, may be you can use already existing cachepolicy kernel parameter instead. Also its better to Cc DT mailing list for any binding updates. Regards, Sudeep
On Fri, Feb 07, 2014 at 10:32:06AM +0100, Andrew Lunn wrote: > > 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. The driver should have a default mode "marvell,feroceon-cache" which doesn't manipulate the write through register at all. In this mode you don't need any register mapping or DT block, just do the mcr/mrc operations to switch the cache on and inhibit printing of the write through status. So, I'd suggest rough logic like: int feroceon_of_init() { writethrough = -1; if (np = of_find_compatible("marvell,kirkwood-cache")) { writethrough = set_writethrough(np, L2_WT_REG_KIRKWOOD, L2_WRITETHROUGH_KIRKWOOD); else if (np = of_find_compatible("marvell,mv78xx0-cache")) { writethrough = set_writethrough(np, L2_WT_REG_MV78XX0, L2_WRITETHROUGH_MV78XX0); feroceon_l2_init(writethrough); [..] void __init feroceon_l2_init(int __l2_wt_override) { /* If we don't know the write through state then assume it is write back, as that is the safest option. */ if (__l2_wt_override == -1) l2_wt_override = 0; else l2_wt_override = __l2_wt_override; [..] printk(KERN_INFO "Feroceon L2: Cache support initialised%s.\n", l2_wt_override == 1 ? ", write through enabled" : ""); > 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. The only loss with not having the register mappings is you must assume write back and thus the system must do possibly useless cache flushes in the DMA path. Otherwise the cache can be fully enabled without needing model specific registers. The writethrough disable is such an obscure feature, I think we can just skip having a compatibility fall back for missing DT node. BTW, if you have a git URL for your patch set I can probably give v2 a try here on Kirkwood.. Regards, Jason
On Fri, Feb 07, 2014 at 02:48:26PM +0000, Sudeep Holla wrote: > On 06/02/14 23:42, Andrew Lunn wrote: > Looks more like a software configuration for me unless I am missing > something. There is precedence for this, see Documentation/devicetree/bindings/arm/l2cc.txt Though the feorceon name should match 'wt-override' > It should not be here IMO if its pure software construct, may be you > can use already existing cachepolicy kernel parameter instead. cachepolicy is already doing something very different, it would not be appropriate to use it for this. Jason
On 07/02/14 18:21, Jason Gunthorpe wrote: > On Fri, Feb 07, 2014 at 02:48:26PM +0000, Sudeep Holla wrote: >> On 06/02/14 23:42, Andrew Lunn wrote: > >> Looks more like a software configuration for me unless I am missing >> something. > > There is precedence for this, see > Documentation/devicetree/bindings/arm/l2cc.txt > > Though the feorceon name should match 'wt-override' > Thanks for pointing out, may be its good to add reference to l2cc binding if its applicable. Do we need both wt-override and writethrough ? >> It should not be here IMO if its pure software construct, may be you >> can use already existing cachepolicy kernel parameter instead. > > cachepolicy is already doing something very different, it would not be > appropriate to use it for this. > Yes I suspected that by a quick inspection at the code using it, but thought of checking instead. Regards, Sudeep
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 @@ -0,0 +1,19 @@ +* Marvell Feroceon Cache + +Required properties: +- compatible : Should be "marvell,feroceon-kirkwood". +- reg : Address of the L2 cache control register + +Optional properties: +- writethrough : only if present, the cache will be used in write through mode. + +Example: + l2: l2-cache@20128 { + compatible = "marvell,marvell,feroceon-kirkwood"; + reg = <0x20128 0x4>; + }; + +There are at least two variants of the Feroceon, differing in how +write through is enabled or not. If mv78xx0 support is added, it is +expected to have a different compatibility string. + diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi index 6abf44d257df..1d8129ac2672 100644 --- a/arch/arm/boot/dts/kirkwood.dtsi +++ b/arch/arm/boot/dts/kirkwood.dtsi @@ -161,6 +161,11 @@ #clock-cells = <1>; }; + l2: l2-cache@20128 { + compatible = "marvell,feroceon-kirkwood"; + reg = <0x20128 0x4>; + }; + intc: main-interrupt-ctrl@20200 { compatible = "marvell,orion-intc"; interrupt-controller; diff --git a/arch/arm/include/asm/hardware/cache-feroceon-l2.h b/arch/arm/include/asm/hardware/cache-feroceon-l2.h index 8edd330aabf6..12e1588dc4f1 100644 --- a/arch/arm/include/asm/hardware/cache-feroceon-l2.h +++ b/arch/arm/include/asm/hardware/cache-feroceon-l2.h @@ -9,3 +9,5 @@ */ extern void __init feroceon_l2_init(int l2_wt_override); +extern int __init feroceon_of_init(void); + diff --git a/arch/arm/mach-kirkwood/board-dt.c b/arch/arm/mach-kirkwood/board-dt.c index 34c35510fd17..2ef59ee2182d 100644 --- a/arch/arm/mach-kirkwood/board-dt.c +++ b/arch/arm/mach-kirkwood/board-dt.c @@ -42,19 +42,6 @@ static void __init kirkwood_map_io(void) iotable_init(kirkwood_io_desc, ARRAY_SIZE(kirkwood_io_desc)); } -static void __init kirkwood_l2_init(void) -{ -#ifdef CONFIG_CACHE_FEROCEON_L2 -#ifdef CONFIG_CACHE_FEROCEON_L2_WRITETHROUGH - writel(readl(L2_CONFIG_REG) | L2_WRITETHROUGH, L2_CONFIG_REG); - feroceon_l2_init(1); -#else - writel(readl(L2_CONFIG_REG) & ~L2_WRITETHROUGH, L2_CONFIG_REG); - feroceon_l2_init(0); -#endif -#endif -} - static struct resource kirkwood_cpufreq_resources[] = { [0] = { .start = CPU_CONTROL_PHYS, @@ -211,7 +198,7 @@ static void __init kirkwood_dt_init(void) BUG_ON(mvebu_mbus_dt_init()); - kirkwood_l2_init(); + feroceon_of_init(); kirkwood_cpufreq_init(); kirkwood_cpuidle_init(); diff --git a/arch/arm/mm/cache-feroceon-l2.c b/arch/arm/mm/cache-feroceon-l2.c index 898362e7972b..193fe7dbdb12 100644 --- a/arch/arm/mm/cache-feroceon-l2.c +++ b/arch/arm/mm/cache-feroceon-l2.c @@ -13,11 +13,17 @@ */ #include <linux/init.h> +#include <linux/of.h> +#include <linux/of_address.h> #include <linux/highmem.h> +#include <linux/io.h> #include <asm/cacheflush.h> #include <asm/cp15.h> #include <asm/hardware/cache-feroceon-l2.h> +#define L2_WRITETHROUGH_KIRKWOOD 0x00000010 + + /* * Low-level cache maintenance operations. * @@ -350,3 +356,43 @@ void __init feroceon_l2_init(int __l2_wt_override) printk(KERN_INFO "Feroceon L2: Cache support initialised%s.\n", l2_wt_override ? ", in WT override mode" : ""); } +#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; + } + + if (of_property_read_bool(node, "writethrough")) + writethrough = true; + + if (of_address_to_resource(node, 0, &res)) + return -ENODEV; + + base = ioremap(res.start, resource_size(&res)); + if (!base) + return -ENOMEM; + + if (writethrough) { + writel(readl(base) | L2_WRITETHROUGH_KIRKWOOD, base); + feroceon_l2_init(1); + } else { + writel(readl(base) & ~L2_WRITETHROUGH_KIRKWOOD, base); + feroceon_l2_init(0); + } + + return 0; +} +#endif
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@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