| Message ID | 1389094888-24348-1-git-send-email-tushar.behera@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tuesday 07 January 2014 17:11:28 Tushar Behera wrote: > Parsed auxiliary control properties for PL310 cache controller. > > Signed-off-by: Tushar Behera <tushar.behera@linaro.org> > --- > These properties are set for Exynos4 platform. If we can pass these properties > through device tree for Exynos4, then we can remove the hard-coded L2_AUX_VAL. The explanation would be good to have in the actual changeset comment above the '---' line. Are all of them actually needed? You shouldn't havet o set the ones that are already turned on by the boot loader. > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > index b513cb8..213546d 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -44,6 +44,16 @@ Optional properties: > - cache-id-part: cache id part number to be used if it is not present > on hardware > - wt-override: If present then L2 is forced to Write through mode > +- arm,early-write: If present then BRSEP mode (early write response) is enabled. > +- arm,data-prefetch: If present then data prefetching is enabled. > +- arm,instruction-prefetch: If present then instruction prefetching is enabled. > +- arm,ns-interrupt-access: If present then interrupt mask and interrupt clear > + registers can be read or modified in both secure or non-secure accesses. > +- arm,ns-lockdown: If present then non-secure accesses can write to lockdown > + register. > +- arm,share-override: If present then shared attribute is ignored internally. > +- arm,full-line-of-zero: If present then 'full line of write zero' behaviour is > + enabled. Is it intentional that you have the "arm," prefix for the new ones, while it's not there for the existing ones? Should we drop it for consistency? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7 January 2014 17:27, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 07 January 2014 17:11:28 Tushar Behera wrote: >> Parsed auxiliary control properties for PL310 cache controller. >> >> Signed-off-by: Tushar Behera <tushar.behera@linaro.org> >> --- >> These properties are set for Exynos4 platform. If we can pass these properties >> through device tree for Exynos4, then we can remove the hard-coded L2_AUX_VAL. > > > The explanation would be good to have in the actual changeset Ok, I will add these explanation to comment section. > comment above the '---' line. Are all of them actually needed? > You shouldn't havet o set the ones that are already turned on > by the boot loader. These are the extra options that we used to pass in the kernel. For normal operation, I couldn't perceive any difference without these properties. The only reason for adding these properties was to be in sync with existing auxiliary control value. >> >> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >> index b513cb8..213546d 100644 >> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >> @@ -44,6 +44,16 @@ Optional properties: >> - cache-id-part: cache id part number to be used if it is not present >> on hardware >> - wt-override: If present then L2 is forced to Write through mode >> +- arm,early-write: If present then BRSEP mode (early write response) is enabled. >> +- arm,data-prefetch: If present then data prefetching is enabled. >> +- arm,instruction-prefetch: If present then instruction prefetching is enabled. >> +- arm,ns-interrupt-access: If present then interrupt mask and interrupt clear >> + registers can be read or modified in both secure or non-secure accesses. >> +- arm,ns-lockdown: If present then non-secure accesses can write to lockdown >> + register. >> +- arm,share-override: If present then shared attribute is ignored internally. >> +- arm,full-line-of-zero: If present then 'full line of write zero' behaviour is >> + enabled. > > Is it intentional that you have the "arm," prefix for the new ones, while > it's not there for the existing ones? Should we drop it for consistency? > > Arnd No specific reasons for this. I was following "arm,data-latency" format. We can drop the "arm," prefix. Thanks for the review.
On 07/01/14 11:41, Tushar Behera wrote: > Parsed auxiliary control properties for PL310 cache controller. > > Signed-off-by: Tushar Behera <tushar.behera@linaro.org> > --- > These properties are set for Exynos4 platform. If we can pass these properties > through device tree for Exynos4, then we can remove the hard-coded L2_AUX_VAL. > > Documentation/devicetree/bindings/arm/l2cc.txt | 10 ++++++++++ > arch/arm/include/asm/hardware/cache-l2x0.h | 1 + > arch/arm/mm/cache-l2x0.c | 25 ++++++++++++++++++++++++ > 3 files changed, 36 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > index b513cb8..213546d 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -44,6 +44,16 @@ Optional properties: > - cache-id-part: cache id part number to be used if it is not present > on hardware > - wt-override: If present then L2 is forced to Write through mode > +- arm,early-write: If present then BRSEP mode (early write response) is enabled. > +- arm,data-prefetch: If present then data prefetching is enabled. > +- arm,instruction-prefetch: If present then instruction prefetching is enabled. > +- arm,ns-interrupt-access: If present then interrupt mask and interrupt clear > + registers can be read or modified in both secure or non-secure accesses. > +- arm,ns-lockdown: If present then non-secure accesses can write to lockdown > + register. > +- arm,share-override: If present then shared attribute is ignored internally. > +- arm,full-line-of-zero: If present then 'full line of write zero' behaviour is > + enabled. > Hi Tushar, This has been discussed couple of times in past[1][2], and the opinion was not to have these in DT as they are more configuration data than the actual hardware description. Regards, Sudeep [1] https://lkml.org/lkml/2013/11/7/8 [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/185610.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 07 January 2014 12:41:42 Sudeep Holla wrote: > Hi Tushar, > > This has been discussed couple of times in past[1][2], and the opinion was not > to have these in DT as they are more configuration data than the actual hardware > description. How do you suggest we get rid of the magic constants in platform code then? I definitely don't want to keep the current state, and having configuration data in DT seems the lesser evil. Are there some reasonable defaults that Linux could use independent of the platform and of what the boot loader defaults to? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/01/14 12:54, Arnd Bergmann wrote: > On Tuesday 07 January 2014 12:41:42 Sudeep Holla wrote: >> Hi Tushar, >> >> This has been discussed couple of times in past[1][2], and the opinion was not >> to have these in DT as they are more configuration data than the actual hardware >> description. > > How do you suggest we get rid of the magic constants in platform code then? > I definitely don't want to keep the current state, and having configuration > data in DT seems the lesser evil. > I agree, but since these are more L2CC configuration than hardware description, IMO chosen node is one option. However it's good to get opinion from DT guys. > Are there some reasonable defaults that Linux could use independent of the > platform and of what the boot loader defaults to? > Most of these registers can't be programmed in Non-secure mode. So as mentioned already in previous discussions it is better to avoid these settings in kernel. It would be better if bootloader programs these settings even if Linux runs in secure mode for simplicity. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 07 January 2014 15:55:45 Sudeep Holla wrote: > On 07/01/14 12:54, Arnd Bergmann wrote: > > On Tuesday 07 January 2014 12:41:42 Sudeep Holla wrote: > >> Hi Tushar, > >> > >> This has been discussed couple of times in past[1][2], and the opinion was not > >> to have these in DT as they are more configuration data than the actual hardware > >> description. > > > > How do you suggest we get rid of the magic constants in platform code then? > > I definitely don't want to keep the current state, and having configuration > > data in DT seems the lesser evil. > > > > I agree, but since these are more L2CC configuration than hardware description, > IMO chosen node is one option. However it's good to get opinion from DT guys. I think one of the discussions we had during the Edinburgh kernel summit resulted in being more relaxed towards configuration data in device nodes. > > Are there some reasonable defaults that Linux could use independent of the > > platform and of what the boot loader defaults to? > > > > Most of these registers can't be programmed in Non-secure mode. So as mentioned > already in previous discussions it is better to avoid these settings in kernel. > It would be better if bootloader programs these settings even if Linux runs in > secure mode for simplicity. Yes, that would be ideal, but I fear we have to live with the boot loaders that are in existence already. Whether or not the registers can be programmed in non-secure mode is certainly a piece of information that belongs into the DT node, so we don't try to write them when we shouldn't. We could also have a property that identifies whether the boot loader has in fact set up the cache controller correctly or whether Linux has to do it. There should be no argument about this being out of scope, since it describes a platform property (presence of a sane boot loader) rather than the actual configuration. If we decide to have such properties and we boot on a system where we can and should change the settings, is there a way for Linux to know what the settings are supposed to be other than reading them from a DT file or from a per-platform default? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/01/14 16:12, Arnd Bergmann wrote: > On Tuesday 07 January 2014 15:55:45 Sudeep Holla wrote: >> On 07/01/14 12:54, Arnd Bergmann wrote: >>> On Tuesday 07 January 2014 12:41:42 Sudeep Holla wrote: >>>> Hi Tushar, >>>> >>>> This has been discussed couple of times in past[1][2], and the opinion was not >>>> to have these in DT as they are more configuration data than the actual hardware >>>> description. >>> >>> How do you suggest we get rid of the magic constants in platform code then? >>> I definitely don't want to keep the current state, and having configuration >>> data in DT seems the lesser evil. >>> >> >> I agree, but since these are more L2CC configuration than hardware description, >> IMO chosen node is one option. However it's good to get opinion from DT guys. > > I think one of the discussions we had during the Edinburgh kernel summit resulted > in being more relaxed towards configuration data in device nodes. > Ok. >>> Are there some reasonable defaults that Linux could use independent of the >>> platform and of what the boot loader defaults to? >>> >> >> Most of these registers can't be programmed in Non-secure mode. So as mentioned >> already in previous discussions it is better to avoid these settings in kernel. >> It would be better if bootloader programs these settings even if Linux runs in >> secure mode for simplicity. > > Yes, that would be ideal, but I fear we have to live with the boot loaders that > are in existence already. Whether or not the registers can be programmed in > non-secure mode is certainly a piece of information that belongs into the DT > node, so we don't try to write them when we shouldn't. We could also have a But the main problem is that there is no way to detect the current mode (i.e. secure or non-secure). So it won't be of any help even if we can get these information from DT. It's better to assume Linux runs in non-secure mode and avoid using any registers with secure only access. Any attempt to such restricted access in non-secure mode results in error. > property that identifies whether the boot loader has in fact set up the cache > controller correctly or whether Linux has to do it. There should be no > argument about this being out of scope, since it describes a platform property > (presence of a sane boot loader) rather than the actual configuration. > > If we decide to have such properties and we boot on a system where we can > and should change the settings, is there a way for Linux to know what the > settings are supposed to be other than reading them from a DT file or > from a per-platform default? > Yes I understand, but for above mentioned reason it's difficult. Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt index b513cb8..213546d 100644 --- a/Documentation/devicetree/bindings/arm/l2cc.txt +++ b/Documentation/devicetree/bindings/arm/l2cc.txt @@ -44,6 +44,16 @@ Optional properties: - cache-id-part: cache id part number to be used if it is not present on hardware - wt-override: If present then L2 is forced to Write through mode +- arm,early-write: If present then BRSEP mode (early write response) is enabled. +- arm,data-prefetch: If present then data prefetching is enabled. +- arm,instruction-prefetch: If present then instruction prefetching is enabled. +- arm,ns-interrupt-access: If present then interrupt mask and interrupt clear + registers can be read or modified in both secure or non-secure accesses. +- arm,ns-lockdown: If present then non-secure accesses can write to lockdown + register. +- arm,share-override: If present then shared attribute is ignored internally. +- arm,full-line-of-zero: If present then 'full line of write zero' behaviour is + enabled. Example: diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h index 6795ff7..aefdec0 100644 --- a/arch/arm/include/asm/hardware/cache-l2x0.h +++ b/arch/arm/include/asm/hardware/cache-l2x0.h @@ -78,6 +78,7 @@ #define L2X0_CACHE_ID_RTL_R3P2 0x8 #define L2X0_AUX_CTRL_MASK 0xc0000fff +#define L2X0_AUX_CTRL_FULL_LINE_OF_ZERO_SHIFT 0 #define L2X0_AUX_CTRL_DATA_RD_LATENCY_SHIFT 0 #define L2X0_AUX_CTRL_DATA_RD_LATENCY_MASK 0x7 #define L2X0_AUX_CTRL_DATA_WR_LATENCY_SHIFT 3 diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c index 7abde2c..03357f1 100644 --- a/arch/arm/mm/cache-l2x0.c +++ b/arch/arm/mm/cache-l2x0.c @@ -705,6 +705,7 @@ static void __init pl310_of_setup(const struct device_node *np, u32 data[3] = { 0, 0, 0 }; u32 tag[3] = { 0, 0, 0 }; u32 filter[2] = { 0, 0 }; + u32 val = 0; of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag)); if (tag[0] && tag[1] && tag[2]) @@ -731,6 +732,30 @@ static void __init pl310_of_setup(const struct device_node *np, writel_relaxed((filter[0] & ~(SZ_1M - 1)) | L2X0_ADDR_FILTER_EN, l2x0_base + L2X0_ADDR_FILTER_START); } + + if (of_find_property(np, "arm,early-write", NULL)) + val |= BIT(L2X0_AUX_CTRL_EARLY_BRESP_SHIFT); + + if (of_find_property(np, "arm,instruction-prefetch", NULL)) + val |= BIT(L2X0_AUX_CTRL_INSTR_PREFETCH_SHIFT); + + if (of_find_property(np, "arm,data-prefetch", NULL)) + val |= BIT(L2X0_AUX_CTRL_DATA_PREFETCH_SHIFT); + + if (of_find_property(np, "arm,ns-interrupt-access", NULL)) + val |= BIT(L2X0_AUX_CTRL_NS_INT_CTRL_SHIFT); + + if (of_find_property(np, "arm,ns-lockdown", NULL)) + val |= BIT(L2X0_AUX_CTRL_NS_LOCKDOWN_SHIFT); + + if (of_find_property(np, "arm,share-override", NULL)) + val |= BIT(L2X0_AUX_CTRL_SHARE_OVERRIDE_SHIFT); + + if (of_find_property(np, "arm,full-line-of-zero", NULL)) + val |= BIT(L2X0_AUX_CTRL_FULL_LINE_OF_ZERO_SHIFT); + + *aux_val |= val; + *aux_mask &= ~val; } static void __init pl310_save(void)
Parsed auxiliary control properties for PL310 cache controller. Signed-off-by: Tushar Behera <tushar.behera@linaro.org> --- These properties are set for Exynos4 platform. If we can pass these properties through device tree for Exynos4, then we can remove the hard-coded L2_AUX_VAL. Documentation/devicetree/bindings/arm/l2cc.txt | 10 ++++++++++ arch/arm/include/asm/hardware/cache-l2x0.h | 1 + arch/arm/mm/cache-l2x0.c | 25 ++++++++++++++++++++++++ 3 files changed, 36 insertions(+)