Message ID | 201404052104.11363.arnd@arndb.de |
---|---|
State | New |
Headers | show |
Hi Paul, On 06/04/2014 11:37, Paul Bolle wrote: > On Sat, 2014-04-05 at 20:04 +0100, Arnd Bergmann wrote: >> Gregory CLEMENT (1): >> ARM: mvebu: add initial support for the Armada 375 SOCs >> >> [...] >> >> Thomas Petazzoni (7): >> [...] >> ARM: mvebu: add initial support for the Armada 380/385 SOCs > > These two commits added select statements for ARM_ERRATA_753970. But I > couldn't find that Kconfig symbol. (I checked master of Linus' tree and > current linux-next.) So it seems it was intended to select > PL310_ERRATA_753970 here. Is that correct? Yes it is correct. ARM_ERRATA_753970 was renamed to PL310_ERRATA_753970 in kernel 3.2. I had just carry on the symbol name without noticed that it have changed. Do you want to send a fix, or do you prefer I take care of it? Thanks, Gregory > > > Paul Bolle >
On Sun, 2014-04-06 at 19:02 +0200, Gregory CLEMENT wrote: > Yes it is correct. ARM_ERRATA_753970 was renamed to PL310_ERRATA_753970 > in kernel 3.2. Thanks! That was commit fa0ce4035d48 ("ARM: 7162/1: errata: tidy up Kconfig options for PL310 errata workarounds"), by the way. I hadn't checked the history of the tree. Had I bothered to do so this all would have been less mysterious. > I had just carry on the symbol name without noticed that > it have changed. > > Do you want to send a fix, or do you prefer I take care of it? It is a trivial fix, but it might need some testing, which I can't do. So I guess it's easiest if you take care of it. Paul Bolle
Paul, Working through my back-log... On Mon, May 26, 2014 at 11:01:11AM +0200, Paul Bolle wrote: > ARM_ERRATA_753970 was renamed to PL310_ERRATA_753970 in v3.2, through > commit fa0ce4035d48 ("ARM: 7162/1: errata: tidy up Kconfig options for > PL310 errata workarounds"). Two selects were added in v3.15-rc1 that > still use the previous name. Rename these. > > Make these statements depend on CACHE_PL310, like all other selects of > PL310_ERRATA_753970. That way it will only be selected if its dependency > is met. > > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > I reported this before v3.15-rc1. I don't know whether any fixes are > pending. None are in linux-next. It looks like rmk just added a patch to fixing the selection of the erratas. Could you please take a look and either rebase or drop this patch? thx, Jason. > And ignoring an errata were one > apparently could be needed sounds, well, scary. Perhaps it is not. > Anyhow, to make sure this gets fixed, hopefully before v3.15, I'm > submitting this (untested!) patch. > > A related observation. There are three PL310 errata options: one depends > on CACHE_PL310, three depend on CACHE_L2X0. The one depending on > CACHE_PL310 is selected only if CACHE_PL310 is set. > > But the three depending on CACHE_L2X0 are selected a few times if > CACHE_L2X0 is set, in other cases if CACHE_PL310 is set, and in some > cases always. There may be good reasons for this, but it looks odd. I > know nothing about the PL310 cache and its erratas, so I haven't looked > into this any further. > > arch/arm/mach-mvebu/Kconfig | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig > index 3f73eecbcfb0..501d0f42e7b2 100644 > --- a/arch/arm/mach-mvebu/Kconfig > +++ b/arch/arm/mach-mvebu/Kconfig > @@ -35,7 +35,7 @@ config MACH_ARMADA_370 > config MACH_ARMADA_375 > bool "Marvell Armada 375 boards" if ARCH_MULTI_V7 > select ARM_ERRATA_720789 > - select ARM_ERRATA_753970 > + select PL310_ERRATA_753970 if CACHE_PL310 > select ARM_GIC > select ARMADA_375_CLK > select CPU_V7 > @@ -48,7 +48,7 @@ config MACH_ARMADA_375 > config MACH_ARMADA_38X > bool "Marvell Armada 380/385 boards" if ARCH_MULTI_V7 > select ARM_ERRATA_720789 > - select ARM_ERRATA_753970 > + select PL310_ERRATA_753970 if CACHE_PL310 > select ARM_GIC > select ARMADA_38X_CLK > select CPU_V7 > -- > 1.9.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Jason Cooper schreef op vr 20-06-2014 om 16:21 [-0400]: > It looks like rmk just added a patch to fixing the selection of the > erratas. Could you please take a look and either rebase or drop this > patch? Sure, no problem. What tree should I check? Paul Bolle
On Fri, Jun 20, 2014 at 04:21:00PM -0400, Jason Cooper wrote: > It looks like rmk just added a patch to fixing the selection of the > erratas. Could you please take a look and either rebase or drop this > patch? Yes please. If it's needed, just update it to be: > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig > > index 3f73eecbcfb0..501d0f42e7b2 100644 > > --- a/arch/arm/mach-mvebu/Kconfig > > +++ b/arch/arm/mach-mvebu/Kconfig > > @@ -35,7 +35,7 @@ config MACH_ARMADA_370 > > config MACH_ARMADA_375 > > bool "Marvell Armada 375 boards" if ARCH_MULTI_V7 > > select ARM_ERRATA_720789 > > - select ARM_ERRATA_753970 > > + select PL310_ERRATA_753970 if CACHE_PL310 select PL310_ERRATA_753970 if CACHE_L2X0 for both platforms. Although there's no harm in using CACHE_PL310 at the moment, my longer term plan is to eventually kill CACHE_PL310 as it's entirely redundant for ARM versions of the L2 cache (it's only used by the old L2x0 code which I've been unable to eliminate entirely.)
On Fri, Jun 20, 2014 at 10:42:57PM +0200, Paul Bolle wrote: > Jason Cooper schreef op vr 20-06-2014 om 16:21 [-0400]: > > It looks like rmk just added a patch to fixing the selection of the > > erratas. Could you please take a look and either rebase or drop this > > patch? > > Sure, no problem. What tree should I check? It's a641f3a6abce ARM: l2c: fix dependencies on PL310 errata symbols in Russell's fixes branch: http://ftp.arm.linux.org.uk/pub/linux/arm/kernel/git-cur/linux-arm.git fixes thx, Jason.
On Fri, 2014-06-20 at 17:10 -0400, Jason Cooper wrote: > On Fri, Jun 20, 2014 at 10:42:57PM +0200, Paul Bolle wrote: > > Jason Cooper schreef op vr 20-06-2014 om 16:21 [-0400]: > > > It looks like rmk just added a patch to fixing the selection of the > > > erratas. Could you please take a look and either rebase or drop this > > > patch? > > > > Sure, no problem. What tree should I check? > > It's > > a641f3a6abce ARM: l2c: fix dependencies on PL310 errata symbols > > in Russell's fixes branch: > > http://ftp.arm.linux.org.uk/pub/linux/arm/kernel/git-cur/linux-arm.git fixes Thanks. That commit landed in linux-next in next-20140620. It addresses all comments I made, below the --- marker, in https://lkml.org/lkml/2014/5/26/103 . It makes it a bit easier to reason about the PL310_ERRATA_* symbols. I hope to submit a v2 soon. Paul Bolle
It's nice to be ignored... On Mon, Jun 23, 2014 at 11:01:44AM +0200, Paul Bolle wrote: > ARM_ERRATA_753970 was renamed to PL310_ERRATA_753970 in v3.2, through > commit fa0ce4035d48 ("ARM: 7162/1: errata: tidy up Kconfig options for > PL310 errata workarounds"). Two selects were added in v3.15 that still > use the previous name. Rename these. > > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > v2: redone on top of next-21040623, and therefor on top of commit > a641f3a6abce ("ARM: l2c: fix dependencies on PL310 errata symbols"). And you obviously didn't read the commit or understand it. > Dropped the "if CACHE_PL310" fragments used in v1. No need to use "if > CACHE_L2X0" instead. Both CACHE_PL310 and CACHE_L2X0 will be > (indirectly) selected if MACH_ARMADA_375 or MACH_ARMADA_38X are set. Yes there is. We've recently seen the selection of CACHE_L2X0 be removed from platforms, while leaving the selection of the errata behind. The result is Kconfig warnings. My commit above ensures that this doesn't happen in the future by adding the proper dependencies onto those errata selects. Please do the same. NAK.
On Mon, 2014-06-23 at 10:07 +0100, Russell King - ARM Linux wrote: > On Mon, Jun 23, 2014 at 11:01:44AM +0200, Paul Bolle wrote: > > Dropped the "if CACHE_PL310" fragments used in v1. No need to use "if > > CACHE_L2X0" instead. Both CACHE_PL310 and CACHE_L2X0 will be > > (indirectly) selected if MACH_ARMADA_375 or MACH_ARMADA_38X are set. > > Yes there is. We've recently seen the selection of CACHE_L2X0 be removed > from platforms, while leaving the selection of the errata behind. The > result is Kconfig warnings. My commit above ensures that this doesn't > happen in the future by adding the proper dependencies onto those errata > selects. The entries I touched read (in summary): config MACH_MVEBU_V7 bool select CACHE_L2X0 config MACH_ARMADA_370 bool "Marvell Armada 370 boards" if ARCH_MULTI_V7 select MACH_MVEBU_V7 help [...]. config MACH_ARMADA_375 bool "Marvell Armada 375 boards" if ARCH_MULTI_V7 select MACH_MVEBU_V7 help [...]. So the choice I faced was between: - using "select PL310_ERRATA_753970 if CACHE_L2X0": that matches all current occurrences of "select PL310_ERRATA_*" but adds a superfluous dependency on CACHE_L2X0 - using just "select PL310_ERRATA_753970": sufficient, but will break if CACHE_L2X0 isn't selected through MACH_MVEBU_V7 anymore. I chose to just "select PL310_ERRATA_753970" but adding "if CACHE_L2X0" is fine with me too. > Please do the same. Will do later today. Thanks, Paul Bolle
On Fri, Sep 12, 2014 at 01:10:05PM +0200, Paul Bolle wrote: > ARM_ERRATA_753970 was renamed to PL310_ERRATA_753970 in v3.2, through > commit fa0ce4035d48 ("ARM: 7162/1: errata: tidy up Kconfig options for > PL310 errata workarounds"). Two selects were added in v3.15 that still > use the previous name. Rename these. Make these select statements depend > on CACHE_L2X0, like all other select statements for PL310_ERRATA_753970 > do, to be safe we don't inadvertently start to allow pointless > configurations in the future. > > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > v3: add CACHE_L2X0 dependency, as Russell requested. > > Tested with "git grep" and fiddling with "make ARCH=arm [...]config". > Please shout if that's deemed insufficient. > > Note that I promised this v3 over two months ago. Taking two months to > update this patch is not really something to be proud of. On the other > hand: no one bothered to prod me for this v3 or send in a proper fix > themselves. Add to this that I first reported these selects for an > outdated Kconfig symbol in April. So I begin to wonder whether these > selects statements are really needed to begin with. I wonder whether we should deal with this a different way: rather than having this stuff as a configuration option, have the work-arounds default to being enabled, and have kernel command parameters to disable them should that be necessary. I expect the normal case is that everyone normally runs with these errata workarounds enabled (especially with a single zImage kernel), so there will be relatively few who need to disable them. Plus, this gives flexibility on single zImage to turn the errata workarounds off without needing to rebuild. The other major advantage is less Kconfig options with dependencies which seem all to easily to result in select abuse.
Hi Russell, On Fri, 2014-09-12 at 14:31 +0100, Russell King - ARM Linux wrote: > On Fri, Sep 12, 2014 at 01:10:05PM +0200, Paul Bolle wrote: > > ARM_ERRATA_753970 was renamed to PL310_ERRATA_753970 in v3.2, through > > commit fa0ce4035d48 ("ARM: 7162/1: errata: tidy up Kconfig options for > > PL310 errata workarounds"). Two selects were added in v3.15 that still > > use the previous name. Rename these. Make these select statements depend > > on CACHE_L2X0, like all other select statements for PL310_ERRATA_753970 > > do, to be safe we don't inadvertently start to allow pointless > > configurations in the future. > > > > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> > > --- > > v3: add CACHE_L2X0 dependency, as Russell requested. > > > > Tested with "git grep" and fiddling with "make ARCH=arm [...]config". > > Please shout if that's deemed insufficient. > > > > Note that I promised this v3 over two months ago. Taking two months to > > update this patch is not really something to be proud of. On the other > > hand: no one bothered to prod me for this v3 or send in a proper fix > > themselves. Add to this that I first reported these selects for an > > outdated Kconfig symbol in April. So I begin to wonder whether these > > selects statements are really needed to begin with. Whatever will be done in the long run: no one else said anything about my v3. It's now nearly seven months since I first reported this issue. In the mean time three releases contained the bogus selects. Apparently neither Marvell Armada 375 boards nor Marvell Armada 380/385 boards actually need to select PL310_ERRATA_753970. I think I'll send a trivial cleanup shortly. > I wonder whether we should deal with this a different way: rather than > having this stuff as a configuration option, have the work-arounds > default to being enabled, and have kernel command parameters to disable > them should that be necessary. > > I expect the normal case is that everyone normally runs with these > errata workarounds enabled (especially with a single zImage kernel), so > there will be relatively few who need to disable them. Plus, this gives > flexibility on single zImage to turn the errata workarounds off without > needing to rebuild. > > The other major advantage is less Kconfig options with dependencies > which seem all to easily to result in select abuse. I have no idea what CACHE_L2X0, ie the L2x0 outer cache controller, actually does nor what those PL310 errata are all about. So I can't comment on the feasibility of your idea. Obviously, dropping Kconfig symbols, and making configuration a bit easier would be one of the benefits of that idea. But I wouldn't know whether it outweighs the costs involved. Thanks, Paul Bolle