* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-15 21:52 ` Hauke Mehrtens 0 siblings, 0 replies; 16+ messages in thread From: Hauke Mehrtens @ 2015-05-15 21:52 UTC (permalink / raw) To: linux-arm-kernel These options make it possible to overwrites the data and instruction prefetching behavior of the arm pl310 cache controller. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- v2: only set prefetch v1: set prefetch and aux Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt index 0dbabe9..528821a 100644 --- a/Documentation/devicetree/bindings/arm/l2cc.txt +++ b/Documentation/devicetree/bindings/arm/l2cc.txt @@ -67,6 +67,10 @@ Optional properties: disable if zero. - arm,prefetch-offset : Override prefetch offset value. Valid values are 0-7, 15, 23, and 31. +- arm,prefetch-data : Enable data prefetch. Enabling prefetching + can improve performance. +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching + can improve performance. Example: diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c index e309c8f..1aa970a 100644 --- a/arch/arm/mm/cache-l2x0.c +++ b/arch/arm/mm/cache-l2x0.c @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); } + ret = of_property_read_u32(np, "arm,prefetch-data", &val); + if (ret == 0) { + if (val) + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; + else + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; + } else if (ret != -EINVAL) { + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); + } + + ret = of_property_read_u32(np, "arm,prefetch-instr", &val); + if (ret == 0) { + if (val) + prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH; + else + prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH; + } else if (ret != -EINVAL) { + pr_err("L2C-310 OF arm,prefetch-instr property value is missing\n"); + } + l2x0_saved_regs.prefetch_ctrl = prefetch; } -- 2.1.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-15 21:52 ` Hauke Mehrtens 0 siblings, 0 replies; 16+ messages in thread From: Hauke Mehrtens @ 2015-05-15 21:52 UTC (permalink / raw) To: linux-lFZ/pmaqli7XmaaqVzeoHQ, arnd-r2nGTMty4D4 Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, catalin.marinas-5wv7dgnIgG8, Hauke Mehrtens These options make it possible to overwrites the data and instruction prefetching behavior of the arm pl310 cache controller. Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org> --- v2: only set prefetch v1: set prefetch and aux Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt index 0dbabe9..528821a 100644 --- a/Documentation/devicetree/bindings/arm/l2cc.txt +++ b/Documentation/devicetree/bindings/arm/l2cc.txt @@ -67,6 +67,10 @@ Optional properties: disable if zero. - arm,prefetch-offset : Override prefetch offset value. Valid values are 0-7, 15, 23, and 31. +- arm,prefetch-data : Enable data prefetch. Enabling prefetching + can improve performance. +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching + can improve performance. Example: diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c index e309c8f..1aa970a 100644 --- a/arch/arm/mm/cache-l2x0.c +++ b/arch/arm/mm/cache-l2x0.c @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); } + ret = of_property_read_u32(np, "arm,prefetch-data", &val); + if (ret == 0) { + if (val) + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; + else + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; + } else if (ret != -EINVAL) { + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); + } + + ret = of_property_read_u32(np, "arm,prefetch-instr", &val); + if (ret == 0) { + if (val) + prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH; + else + prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH; + } else if (ret != -EINVAL) { + pr_err("L2C-310 OF arm,prefetch-instr property value is missing\n"); + } + l2x0_saved_regs.prefetch_ctrl = prefetch; } -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 17:33 ` Hauke Mehrtens 0 siblings, 0 replies; 16+ messages in thread From: Hauke Mehrtens @ 2015-05-29 17:33 UTC (permalink / raw) To: linux-arm-kernel Will this make it into kernel 4.2 or do I have to do something so that this will make it into 4.2? Hauke On 05/15/2015 11:52 PM, Hauke Mehrtens wrote: > These options make it possible to overwrites the data and instruction > prefetching behavior of the arm pl310 cache controller. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > v2: only set prefetch > v1: set prefetch and aux > > Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ > arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > index 0dbabe9..528821a 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -67,6 +67,10 @@ Optional properties: > disable if zero. > - arm,prefetch-offset : Override prefetch offset value. Valid values are > 0-7, 15, 23, and 31. > +- arm,prefetch-data : Enable data prefetch. Enabling prefetching > + can improve performance. > +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching > + can improve performance. > > Example: > > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c > index e309c8f..1aa970a 100644 > --- a/arch/arm/mm/cache-l2x0.c > +++ b/arch/arm/mm/cache-l2x0.c > @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, > pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); > } > > + ret = of_property_read_u32(np, "arm,prefetch-data", &val); > + if (ret == 0) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; > + else > + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; > + } else if (ret != -EINVAL) { > + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); > + } > + > + ret = of_property_read_u32(np, "arm,prefetch-instr", &val); > + if (ret == 0) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH; > + else > + prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH; > + } else if (ret != -EINVAL) { > + pr_err("L2C-310 OF arm,prefetch-instr property value is missing\n"); > + } > + > l2x0_saved_regs.prefetch_ctrl = prefetch; > } > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 17:33 ` Hauke Mehrtens 0 siblings, 0 replies; 16+ messages in thread From: Hauke Mehrtens @ 2015-05-29 17:33 UTC (permalink / raw) To: linux-lFZ/pmaqli7XmaaqVzeoHQ, arnd-r2nGTMty4D4 Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, catalin.marinas-5wv7dgnIgG8, Rafał Miłecki Will this make it into kernel 4.2 or do I have to do something so that this will make it into 4.2? Hauke On 05/15/2015 11:52 PM, Hauke Mehrtens wrote: > These options make it possible to overwrites the data and instruction > prefetching behavior of the arm pl310 cache controller. > > Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org> > --- > v2: only set prefetch > v1: set prefetch and aux > > Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ > arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > index 0dbabe9..528821a 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -67,6 +67,10 @@ Optional properties: > disable if zero. > - arm,prefetch-offset : Override prefetch offset value. Valid values are > 0-7, 15, 23, and 31. > +- arm,prefetch-data : Enable data prefetch. Enabling prefetching > + can improve performance. > +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching > + can improve performance. > > Example: > > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c > index e309c8f..1aa970a 100644 > --- a/arch/arm/mm/cache-l2x0.c > +++ b/arch/arm/mm/cache-l2x0.c > @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, > pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); > } > > + ret = of_property_read_u32(np, "arm,prefetch-data", &val); > + if (ret == 0) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; > + else > + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; > + } else if (ret != -EINVAL) { > + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); > + } > + > + ret = of_property_read_u32(np, "arm,prefetch-instr", &val); > + if (ret == 0) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH; > + else > + prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH; > + } else if (ret != -EINVAL) { > + pr_err("L2C-310 OF arm,prefetch-instr property value is missing\n"); > + } > + > l2x0_saved_regs.prefetch_ctrl = prefetch; > } > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 17:49 ` Russell King - ARM Linux 0 siblings, 0 replies; 16+ messages in thread From: Russell King - ARM Linux @ 2015-05-29 17:49 UTC (permalink / raw) To: linux-arm-kernel On Fri, May 29, 2015 at 07:33:33PM +0200, Hauke Mehrtens wrote: > Will this make it into kernel 4.2 or do I have to do something so that > this will make it into 4.2? I don't see anything wrong with it, but as ever, it needs to end up in the patch system if it's not going to get buried beneath a huge pile of email. Thanks. > > Hauke > > On 05/15/2015 11:52 PM, Hauke Mehrtens wrote: > > These options make it possible to overwrites the data and instruction > > prefetching behavior of the arm pl310 cache controller. > > > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > > --- > > v2: only set prefetch > > v1: set prefetch and aux > > > > Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ > > arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > > index 0dbabe9..528821a 100644 > > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > > @@ -67,6 +67,10 @@ Optional properties: > > disable if zero. > > - arm,prefetch-offset : Override prefetch offset value. Valid values are > > 0-7, 15, 23, and 31. > > +- arm,prefetch-data : Enable data prefetch. Enabling prefetching > > + can improve performance. > > +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching > > + can improve performance. > > > > Example: > > > > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c > > index e309c8f..1aa970a 100644 > > --- a/arch/arm/mm/cache-l2x0.c > > +++ b/arch/arm/mm/cache-l2x0.c > > @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, > > pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); > > } > > > > + ret = of_property_read_u32(np, "arm,prefetch-data", &val); > > + if (ret == 0) { > > + if (val) > > + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; > > + else > > + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; > > + } else if (ret != -EINVAL) { > > + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); > > + } > > + > > + ret = of_property_read_u32(np, "arm,prefetch-instr", &val); > > + if (ret == 0) { > > + if (val) > > + prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH; > > + else > > + prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH; > > + } else if (ret != -EINVAL) { > > + pr_err("L2C-310 OF arm,prefetch-instr property value is missing\n"); > > + } > > + > > l2x0_saved_regs.prefetch_ctrl = prefetch; > > } > > > > -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 17:49 ` Russell King - ARM Linux 0 siblings, 0 replies; 16+ messages in thread From: Russell King - ARM Linux @ 2015-05-29 17:49 UTC (permalink / raw) To: Hauke Mehrtens Cc: arnd-r2nGTMty4D4, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, catalin.marinas-5wv7dgnIgG8, Rafał Miłecki On Fri, May 29, 2015 at 07:33:33PM +0200, Hauke Mehrtens wrote: > Will this make it into kernel 4.2 or do I have to do something so that > this will make it into 4.2? I don't see anything wrong with it, but as ever, it needs to end up in the patch system if it's not going to get buried beneath a huge pile of email. Thanks. > > Hauke > > On 05/15/2015 11:52 PM, Hauke Mehrtens wrote: > > These options make it possible to overwrites the data and instruction > > prefetching behavior of the arm pl310 cache controller. > > > > Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org> > > --- > > v2: only set prefetch > > v1: set prefetch and aux > > > > Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ > > arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > > index 0dbabe9..528821a 100644 > > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > > @@ -67,6 +67,10 @@ Optional properties: > > disable if zero. > > - arm,prefetch-offset : Override prefetch offset value. Valid values are > > 0-7, 15, 23, and 31. > > +- arm,prefetch-data : Enable data prefetch. Enabling prefetching > > + can improve performance. > > +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching > > + can improve performance. > > > > Example: > > > > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c > > index e309c8f..1aa970a 100644 > > --- a/arch/arm/mm/cache-l2x0.c > > +++ b/arch/arm/mm/cache-l2x0.c > > @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, > > pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); > > } > > > > + ret = of_property_read_u32(np, "arm,prefetch-data", &val); > > + if (ret == 0) { > > + if (val) > > + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; > > + else > > + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; > > + } else if (ret != -EINVAL) { > > + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); > > + } > > + > > + ret = of_property_read_u32(np, "arm,prefetch-instr", &val); > > + if (ret == 0) { > > + if (val) > > + prefetch |= L310_PREFETCH_CTRL_INSTR_PREFETCH; > > + else > > + prefetch &= ~L310_PREFETCH_CTRL_INSTR_PREFETCH; > > + } else if (ret != -EINVAL) { > > + pr_err("L2C-310 OF arm,prefetch-instr property value is missing\n"); > > + } > > + > > l2x0_saved_regs.prefetch_ctrl = prefetch; > > } > > > > -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 17:52 ` Florian Fainelli 0 siblings, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2015-05-29 17:52 UTC (permalink / raw) To: linux-arm-kernel On 15/05/15 14:52, Hauke Mehrtens wrote: > These options make it possible to overwrites the data and instruction > prefetching behavior of the arm pl310 cache controller. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > v2: only set prefetch > v1: set prefetch and aux > > Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ > arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > index 0dbabe9..528821a 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -67,6 +67,10 @@ Optional properties: > disable if zero. > - arm,prefetch-offset : Override prefetch offset value. Valid values are > 0-7, 15, 23, and 31. > +- arm,prefetch-data : Enable data prefetch. Enabling prefetching > + can improve performance. I do not think the "can improve performance" has a place in a binding, this is either not technical enough about what this does, or marketing enough it does not buy us much. data/instruction pre-fetching are commonly found on cache controller these days, so I would be tempted to remove the "arm," prefixing here since this can be generalized to other kinds of cache controllers. Documenting that this can be either a boolean, or accept a value (see below) could help. > +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching > + can improve performance. > > Example: > > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c > index e309c8f..1aa970a 100644 > --- a/arch/arm/mm/cache-l2x0.c > +++ b/arch/arm/mm/cache-l2x0.c > @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, > pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); > } > > + ret = of_property_read_u32(np, "arm,prefetch-data", &val); > + if (ret == 0) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; > + else > + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; > + } else if (ret != -EINVAL) { > + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); > + } If we want to generalize the use of this property, there could indeed be a value associated with it, if the cache controller supports different pre-fetching strides, however this is not the cache for these cache controllers it seems, are not we going to show error messages more often than desired? -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 17:52 ` Florian Fainelli 0 siblings, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2015-05-29 17:52 UTC (permalink / raw) To: Hauke Mehrtens, linux-lFZ/pmaqli7XmaaqVzeoHQ, arnd-r2nGTMty4D4 Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, catalin.marinas-5wv7dgnIgG8 On 15/05/15 14:52, Hauke Mehrtens wrote: > These options make it possible to overwrites the data and instruction > prefetching behavior of the arm pl310 cache controller. > > Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org> > --- > v2: only set prefetch > v1: set prefetch and aux > > Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ > arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ > 2 files changed, 24 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > index 0dbabe9..528821a 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -67,6 +67,10 @@ Optional properties: > disable if zero. > - arm,prefetch-offset : Override prefetch offset value. Valid values are > 0-7, 15, 23, and 31. > +- arm,prefetch-data : Enable data prefetch. Enabling prefetching > + can improve performance. I do not think the "can improve performance" has a place in a binding, this is either not technical enough about what this does, or marketing enough it does not buy us much. data/instruction pre-fetching are commonly found on cache controller these days, so I would be tempted to remove the "arm," prefixing here since this can be generalized to other kinds of cache controllers. Documenting that this can be either a boolean, or accept a value (see below) could help. > +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching > + can improve performance. > > Example: > > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c > index e309c8f..1aa970a 100644 > --- a/arch/arm/mm/cache-l2x0.c > +++ b/arch/arm/mm/cache-l2x0.c > @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, > pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); > } > > + ret = of_property_read_u32(np, "arm,prefetch-data", &val); > + if (ret == 0) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; > + else > + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; > + } else if (ret != -EINVAL) { > + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); > + } If we want to generalize the use of this property, there could indeed be a value associated with it, if the cache controller supports different pre-fetching strides, however this is not the cache for these cache controllers it seems, are not we going to show error messages more often than desired? -- Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 18:11 ` Hauke Mehrtens 0 siblings, 0 replies; 16+ messages in thread From: Hauke Mehrtens @ 2015-05-29 18:11 UTC (permalink / raw) To: linux-arm-kernel On 05/29/2015 07:52 PM, Florian Fainelli wrote: > On 15/05/15 14:52, Hauke Mehrtens wrote: >> These options make it possible to overwrites the data and instruction >> prefetching behavior of the arm pl310 cache controller. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> --- >> v2: only set prefetch >> v1: set prefetch and aux >> >> Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ >> arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >> index 0dbabe9..528821a 100644 >> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >> @@ -67,6 +67,10 @@ Optional properties: >> disable if zero. >> - arm,prefetch-offset : Override prefetch offset value. Valid values are >> 0-7, 15, 23, and 31. >> +- arm,prefetch-data : Enable data prefetch. Enabling prefetching >> + can improve performance. > > I do not think the "can improve performance" has a place in a binding, > this is either not technical enough about what this does, or marketing > enough it does not buy us much. > > data/instruction pre-fetching are commonly found on cache controller > these days, so I would be tempted to remove the "arm," prefixing here > since this can be generalized to other kinds of cache controllers. > Documenting that this can be either a boolean, or accept a value (see > below) could help. So you think I should only add prefetch-data and prefetch-instr without the arm prefix. > >> +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching >> + can improve performance. >> >> Example: >> >> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c >> index e309c8f..1aa970a 100644 >> --- a/arch/arm/mm/cache-l2x0.c >> +++ b/arch/arm/mm/cache-l2x0.c >> @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, >> pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); >> } >> >> + ret = of_property_read_u32(np, "arm,prefetch-data", &val); >> + if (ret == 0) { >> + if (val) >> + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; >> + else >> + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; >> + } else if (ret != -EINVAL) { >> + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); >> + } > > If we want to generalize the use of this property, there could indeed be > a value associated with it, if the cache controller supports different > pre-fetching strides, however this is not the cache for these cache > controllers it seems, are not we going to show error messages more often > than desired? I did this so it is possible to deactivate the prefech mode. I do not know if somebody wants to do that. I haven't understood how you suggest I should change. When you do not associate a value with an entry in device tree it is there or not there, so we could only activate it when it was not automatically detected, but we could not deactivate it, because the case when this value is not specified in device tree would be, use to auto detected the value. Hauke ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 18:11 ` Hauke Mehrtens 0 siblings, 0 replies; 16+ messages in thread From: Hauke Mehrtens @ 2015-05-29 18:11 UTC (permalink / raw) To: Florian Fainelli, linux-lFZ/pmaqli7XmaaqVzeoHQ, arnd-r2nGTMty4D4 Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, catalin.marinas-5wv7dgnIgG8 On 05/29/2015 07:52 PM, Florian Fainelli wrote: > On 15/05/15 14:52, Hauke Mehrtens wrote: >> These options make it possible to overwrites the data and instruction >> prefetching behavior of the arm pl310 cache controller. >> >> Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org> >> --- >> v2: only set prefetch >> v1: set prefetch and aux >> >> Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ >> arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >> index 0dbabe9..528821a 100644 >> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >> @@ -67,6 +67,10 @@ Optional properties: >> disable if zero. >> - arm,prefetch-offset : Override prefetch offset value. Valid values are >> 0-7, 15, 23, and 31. >> +- arm,prefetch-data : Enable data prefetch. Enabling prefetching >> + can improve performance. > > I do not think the "can improve performance" has a place in a binding, > this is either not technical enough about what this does, or marketing > enough it does not buy us much. > > data/instruction pre-fetching are commonly found on cache controller > these days, so I would be tempted to remove the "arm," prefixing here > since this can be generalized to other kinds of cache controllers. > Documenting that this can be either a boolean, or accept a value (see > below) could help. So you think I should only add prefetch-data and prefetch-instr without the arm prefix. > >> +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching >> + can improve performance. >> >> Example: >> >> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c >> index e309c8f..1aa970a 100644 >> --- a/arch/arm/mm/cache-l2x0.c >> +++ b/arch/arm/mm/cache-l2x0.c >> @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, >> pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); >> } >> >> + ret = of_property_read_u32(np, "arm,prefetch-data", &val); >> + if (ret == 0) { >> + if (val) >> + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; >> + else >> + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; >> + } else if (ret != -EINVAL) { >> + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); >> + } > > If we want to generalize the use of this property, there could indeed be > a value associated with it, if the cache controller supports different > pre-fetching strides, however this is not the cache for these cache > controllers it seems, are not we going to show error messages more often > than desired? I did this so it is possible to deactivate the prefech mode. I do not know if somebody wants to do that. I haven't understood how you suggest I should change. When you do not associate a value with an entry in device tree it is there or not there, so we could only activate it when it was not automatically detected, but we could not deactivate it, because the case when this value is not specified in device tree would be, use to auto detected the value. Hauke -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 18:30 ` Florian Fainelli 0 siblings, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2015-05-29 18:30 UTC (permalink / raw) To: linux-arm-kernel On 29/05/15 11:11, Hauke Mehrtens wrote: > > > On 05/29/2015 07:52 PM, Florian Fainelli wrote: >> On 15/05/15 14:52, Hauke Mehrtens wrote: >>> These options make it possible to overwrites the data and instruction >>> prefetching behavior of the arm pl310 cache controller. >>> >>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >>> --- >>> v2: only set prefetch >>> v1: set prefetch and aux >>> >>> Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ >>> arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ >>> 2 files changed, 24 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >>> index 0dbabe9..528821a 100644 >>> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >>> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >>> @@ -67,6 +67,10 @@ Optional properties: >>> disable if zero. >>> - arm,prefetch-offset : Override prefetch offset value. Valid values are >>> 0-7, 15, 23, and 31. >>> +- arm,prefetch-data : Enable data prefetch. Enabling prefetching >>> + can improve performance. >> >> I do not think the "can improve performance" has a place in a binding, >> this is either not technical enough about what this does, or marketing >> enough it does not buy us much. >> >> data/instruction pre-fetching are commonly found on cache controller >> these days, so I would be tempted to remove the "arm," prefixing here >> since this can be generalized to other kinds of cache controllers. >> Documenting that this can be either a boolean, or accept a value (see >> below) could help. > > So you think I should only add prefetch-data and prefetch-instr without > the arm prefix. That's what I think yes, others may disagree. >> >>> +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching >>> + can improve performance. >>> >>> Example: >>> >>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c >>> index e309c8f..1aa970a 100644 >>> --- a/arch/arm/mm/cache-l2x0.c >>> +++ b/arch/arm/mm/cache-l2x0.c >>> @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, >>> pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); >>> } >>> >>> + ret = of_property_read_u32(np, "arm,prefetch-data", &val); >>> + if (ret == 0) { >>> + if (val) >>> + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; >>> + else >>> + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; >>> + } else if (ret != -EINVAL) { >>> + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); >>> + } >> >> If we want to generalize the use of this property, there could indeed be >> a value associated with it, if the cache controller supports different >> pre-fetching strides, however this is not the cache for these cache >> controllers it seems, are not we going to show error messages more often >> than desired? > I did this so it is possible to deactivate the prefech mode. I do not > know if somebody wants to do that. I haven't understood how you suggest > I should change. > When you do not associate a value with an entry in device tree it is > there or not there, so we could only activate it when it was not > automatically detected, but we could not deactivate it, because the case > when this value is not specified in device tree would be, use to auto > detected the value. My point is that you use of_read_property_u32, but your example does not state what should be the value associated with this property in the binding, so it is unclear what are the results without looking at the code between these examples: /* Enable data pre-fetching */ #1 arm,data-prefetch; #2 arm,data-prefetch = <1>; /* Disable data pre-fetching */ #3 arm,data-prefetch = <0>; /* do nothing, empty aka retain existing settings set by firmware */ #4 Based on the code, only #2 and #3 are intended, which raises the question, do not we want of_read_property_bool() instead then? The binding does seem to suggest that only #1 and #4 are valid though. -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 18:30 ` Florian Fainelli 0 siblings, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2015-05-29 18:30 UTC (permalink / raw) To: Hauke Mehrtens, linux-lFZ/pmaqli7XmaaqVzeoHQ, arnd-r2nGTMty4D4 Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, catalin.marinas-5wv7dgnIgG8 On 29/05/15 11:11, Hauke Mehrtens wrote: > > > On 05/29/2015 07:52 PM, Florian Fainelli wrote: >> On 15/05/15 14:52, Hauke Mehrtens wrote: >>> These options make it possible to overwrites the data and instruction >>> prefetching behavior of the arm pl310 cache controller. >>> >>> Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org> >>> --- >>> v2: only set prefetch >>> v1: set prefetch and aux >>> >>> Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ >>> arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ >>> 2 files changed, 24 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >>> index 0dbabe9..528821a 100644 >>> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >>> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >>> @@ -67,6 +67,10 @@ Optional properties: >>> disable if zero. >>> - arm,prefetch-offset : Override prefetch offset value. Valid values are >>> 0-7, 15, 23, and 31. >>> +- arm,prefetch-data : Enable data prefetch. Enabling prefetching >>> + can improve performance. >> >> I do not think the "can improve performance" has a place in a binding, >> this is either not technical enough about what this does, or marketing >> enough it does not buy us much. >> >> data/instruction pre-fetching are commonly found on cache controller >> these days, so I would be tempted to remove the "arm," prefixing here >> since this can be generalized to other kinds of cache controllers. >> Documenting that this can be either a boolean, or accept a value (see >> below) could help. > > So you think I should only add prefetch-data and prefetch-instr without > the arm prefix. That's what I think yes, others may disagree. >> >>> +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching >>> + can improve performance. >>> >>> Example: >>> >>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c >>> index e309c8f..1aa970a 100644 >>> --- a/arch/arm/mm/cache-l2x0.c >>> +++ b/arch/arm/mm/cache-l2x0.c >>> @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, >>> pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); >>> } >>> >>> + ret = of_property_read_u32(np, "arm,prefetch-data", &val); >>> + if (ret == 0) { >>> + if (val) >>> + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; >>> + else >>> + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; >>> + } else if (ret != -EINVAL) { >>> + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); >>> + } >> >> If we want to generalize the use of this property, there could indeed be >> a value associated with it, if the cache controller supports different >> pre-fetching strides, however this is not the cache for these cache >> controllers it seems, are not we going to show error messages more often >> than desired? > I did this so it is possible to deactivate the prefech mode. I do not > know if somebody wants to do that. I haven't understood how you suggest > I should change. > When you do not associate a value with an entry in device tree it is > there or not there, so we could only activate it when it was not > automatically detected, but we could not deactivate it, because the case > when this value is not specified in device tree would be, use to auto > detected the value. My point is that you use of_read_property_u32, but your example does not state what should be the value associated with this property in the binding, so it is unclear what are the results without looking at the code between these examples: /* Enable data pre-fetching */ #1 arm,data-prefetch; #2 arm,data-prefetch = <1>; /* Disable data pre-fetching */ #3 arm,data-prefetch = <0>; /* do nothing, empty aka retain existing settings set by firmware */ #4 Based on the code, only #2 and #3 are intended, which raises the question, do not we want of_read_property_bool() instead then? The binding does seem to suggest that only #1 and #4 are valid though. -- Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 18:35 ` Florian Fainelli 0 siblings, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2015-05-29 18:35 UTC (permalink / raw) To: linux-arm-kernel On 29/05/15 11:30, Florian Fainelli wrote: > On 29/05/15 11:11, Hauke Mehrtens wrote: >> >> >> On 05/29/2015 07:52 PM, Florian Fainelli wrote: >>> On 15/05/15 14:52, Hauke Mehrtens wrote: >>>> These options make it possible to overwrites the data and instruction >>>> prefetching behavior of the arm pl310 cache controller. >>>> >>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >>>> --- >>>> v2: only set prefetch >>>> v1: set prefetch and aux >>>> >>>> Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ >>>> arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ >>>> 2 files changed, 24 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >>>> index 0dbabe9..528821a 100644 >>>> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >>>> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >>>> @@ -67,6 +67,10 @@ Optional properties: >>>> disable if zero. >>>> - arm,prefetch-offset : Override prefetch offset value. Valid values are >>>> 0-7, 15, 23, and 31. >>>> +- arm,prefetch-data : Enable data prefetch. Enabling prefetching >>>> + can improve performance. >>> >>> I do not think the "can improve performance" has a place in a binding, >>> this is either not technical enough about what this does, or marketing >>> enough it does not buy us much. >>> >>> data/instruction pre-fetching are commonly found on cache controller >>> these days, so I would be tempted to remove the "arm," prefixing here >>> since this can be generalized to other kinds of cache controllers. >>> Documenting that this can be either a boolean, or accept a value (see >>> below) could help. >> >> So you think I should only add prefetch-data and prefetch-instr without >> the arm prefix. > > That's what I think yes, others may disagree. > >>> >>>> +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching >>>> + can improve performance. >>>> >>>> Example: >>>> >>>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c >>>> index e309c8f..1aa970a 100644 >>>> --- a/arch/arm/mm/cache-l2x0.c >>>> +++ b/arch/arm/mm/cache-l2x0.c >>>> @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, >>>> pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); >>>> } >>>> >>>> + ret = of_property_read_u32(np, "arm,prefetch-data", &val); >>>> + if (ret == 0) { >>>> + if (val) >>>> + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; >>>> + else >>>> + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; >>>> + } else if (ret != -EINVAL) { >>>> + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); >>>> + } >>> >>> If we want to generalize the use of this property, there could indeed be >>> a value associated with it, if the cache controller supports different >>> pre-fetching strides, however this is not the cache for these cache >>> controllers it seems, are not we going to show error messages more often >>> than desired? >> I did this so it is possible to deactivate the prefech mode. I do not >> know if somebody wants to do that. I haven't understood how you suggest >> I should change. >> When you do not associate a value with an entry in device tree it is >> there or not there, so we could only activate it when it was not >> automatically detected, but we could not deactivate it, because the case >> when this value is not specified in device tree would be, use to auto >> detected the value. > > My point is that you use of_read_property_u32, but your example does not > state what should be the value associated with this property in the > binding, so it is unclear what are the results without looking at the > code between these examples: > > /* Enable data pre-fetching */ > #1 arm,data-prefetch; > #2 arm,data-prefetch = <1>; > > /* Disable data pre-fetching */ > #3 arm,data-prefetch = <0>; > > /* do nothing, empty aka retain existing settings set by firmware */ > #4 > > Based on the code, only #2 and #3 are intended, which raises the > question, do not we want of_read_property_bool() instead then? The > binding does seem to suggest that only #1 and #4 are valid though. I re-read the code, and I think if you just clarify the values in the binding to be: 0 (forcibly disable), 1 (forcibly enable), property absent (retain settings), this would be crystal clear. Thanks! -- Florian ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 18:35 ` Florian Fainelli 0 siblings, 0 replies; 16+ messages in thread From: Florian Fainelli @ 2015-05-29 18:35 UTC (permalink / raw) To: Hauke Mehrtens, linux-lFZ/pmaqli7XmaaqVzeoHQ, arnd-r2nGTMty4D4 Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, catalin.marinas-5wv7dgnIgG8 On 29/05/15 11:30, Florian Fainelli wrote: > On 29/05/15 11:11, Hauke Mehrtens wrote: >> >> >> On 05/29/2015 07:52 PM, Florian Fainelli wrote: >>> On 15/05/15 14:52, Hauke Mehrtens wrote: >>>> These options make it possible to overwrites the data and instruction >>>> prefetching behavior of the arm pl310 cache controller. >>>> >>>> Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org> >>>> --- >>>> v2: only set prefetch >>>> v1: set prefetch and aux >>>> >>>> Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ >>>> arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ >>>> 2 files changed, 24 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >>>> index 0dbabe9..528821a 100644 >>>> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >>>> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >>>> @@ -67,6 +67,10 @@ Optional properties: >>>> disable if zero. >>>> - arm,prefetch-offset : Override prefetch offset value. Valid values are >>>> 0-7, 15, 23, and 31. >>>> +- arm,prefetch-data : Enable data prefetch. Enabling prefetching >>>> + can improve performance. >>> >>> I do not think the "can improve performance" has a place in a binding, >>> this is either not technical enough about what this does, or marketing >>> enough it does not buy us much. >>> >>> data/instruction pre-fetching are commonly found on cache controller >>> these days, so I would be tempted to remove the "arm," prefixing here >>> since this can be generalized to other kinds of cache controllers. >>> Documenting that this can be either a boolean, or accept a value (see >>> below) could help. >> >> So you think I should only add prefetch-data and prefetch-instr without >> the arm prefix. > > That's what I think yes, others may disagree. > >>> >>>> +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching >>>> + can improve performance. >>>> >>>> Example: >>>> >>>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c >>>> index e309c8f..1aa970a 100644 >>>> --- a/arch/arm/mm/cache-l2x0.c >>>> +++ b/arch/arm/mm/cache-l2x0.c >>>> @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, >>>> pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); >>>> } >>>> >>>> + ret = of_property_read_u32(np, "arm,prefetch-data", &val); >>>> + if (ret == 0) { >>>> + if (val) >>>> + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; >>>> + else >>>> + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; >>>> + } else if (ret != -EINVAL) { >>>> + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); >>>> + } >>> >>> If we want to generalize the use of this property, there could indeed be >>> a value associated with it, if the cache controller supports different >>> pre-fetching strides, however this is not the cache for these cache >>> controllers it seems, are not we going to show error messages more often >>> than desired? >> I did this so it is possible to deactivate the prefech mode. I do not >> know if somebody wants to do that. I haven't understood how you suggest >> I should change. >> When you do not associate a value with an entry in device tree it is >> there or not there, so we could only activate it when it was not >> automatically detected, but we could not deactivate it, because the case >> when this value is not specified in device tree would be, use to auto >> detected the value. > > My point is that you use of_read_property_u32, but your example does not > state what should be the value associated with this property in the > binding, so it is unclear what are the results without looking at the > code between these examples: > > /* Enable data pre-fetching */ > #1 arm,data-prefetch; > #2 arm,data-prefetch = <1>; > > /* Disable data pre-fetching */ > #3 arm,data-prefetch = <0>; > > /* do nothing, empty aka retain existing settings set by firmware */ > #4 > > Based on the code, only #2 and #3 are intended, which raises the > question, do not we want of_read_property_bool() instead then? The > binding does seem to suggest that only #1 and #4 are valid though. I re-read the code, and I think if you just clarify the values in the binding to be: 0 (forcibly disable), 1 (forcibly enable), property absent (retain settings), this would be crystal clear. Thanks! -- Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 18:46 ` Hauke Mehrtens 0 siblings, 0 replies; 16+ messages in thread From: Hauke Mehrtens @ 2015-05-29 18:46 UTC (permalink / raw) To: linux-arm-kernel On 05/29/2015 08:35 PM, Florian Fainelli wrote: > On 29/05/15 11:30, Florian Fainelli wrote: >> On 29/05/15 11:11, Hauke Mehrtens wrote: >>> >>> >>> On 05/29/2015 07:52 PM, Florian Fainelli wrote: >>>> On 15/05/15 14:52, Hauke Mehrtens wrote: >>>>> These options make it possible to overwrites the data and instruction >>>>> prefetching behavior of the arm pl310 cache controller. >>>>> >>>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >>>>> --- >>>>> v2: only set prefetch >>>>> v1: set prefetch and aux >>>>> >>>>> Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ >>>>> arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ >>>>> 2 files changed, 24 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >>>>> index 0dbabe9..528821a 100644 >>>>> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >>>>> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >>>>> @@ -67,6 +67,10 @@ Optional properties: >>>>> disable if zero. >>>>> - arm,prefetch-offset : Override prefetch offset value. Valid values are >>>>> 0-7, 15, 23, and 31. >>>>> +- arm,prefetch-data : Enable data prefetch. Enabling prefetching >>>>> + can improve performance. >>>> >>>> I do not think the "can improve performance" has a place in a binding, >>>> this is either not technical enough about what this does, or marketing >>>> enough it does not buy us much. >>>> >>>> data/instruction pre-fetching are commonly found on cache controller >>>> these days, so I would be tempted to remove the "arm," prefixing here >>>> since this can be generalized to other kinds of cache controllers. >>>> Documenting that this can be either a boolean, or accept a value (see >>>> below) could help. >>> >>> So you think I should only add prefetch-data and prefetch-instr without >>> the arm prefix. >> >> That's what I think yes, others may disagree. Are there any other opinions on this issue? >> >>>> >>>>> +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching >>>>> + can improve performance. >>>>> >>>>> Example: >>>>> >>>>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c >>>>> index e309c8f..1aa970a 100644 >>>>> --- a/arch/arm/mm/cache-l2x0.c >>>>> +++ b/arch/arm/mm/cache-l2x0.c >>>>> @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, >>>>> pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); >>>>> } >>>>> >>>>> + ret = of_property_read_u32(np, "arm,prefetch-data", &val); >>>>> + if (ret == 0) { >>>>> + if (val) >>>>> + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; >>>>> + else >>>>> + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; >>>>> + } else if (ret != -EINVAL) { >>>>> + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); >>>>> + } >>>> >>>> If we want to generalize the use of this property, there could indeed be >>>> a value associated with it, if the cache controller supports different >>>> pre-fetching strides, however this is not the cache for these cache >>>> controllers it seems, are not we going to show error messages more often >>>> than desired? >>> I did this so it is possible to deactivate the prefech mode. I do not >>> know if somebody wants to do that. I haven't understood how you suggest >>> I should change. >>> When you do not associate a value with an entry in device tree it is >>> there or not there, so we could only activate it when it was not >>> automatically detected, but we could not deactivate it, because the case >>> when this value is not specified in device tree would be, use to auto >>> detected the value. >> >> My point is that you use of_read_property_u32, but your example does not >> state what should be the value associated with this property in the >> binding, so it is unclear what are the results without looking at the >> code between these examples: >> >> /* Enable data pre-fetching */ >> #1 arm,data-prefetch; >> #2 arm,data-prefetch = <1>; >> >> /* Disable data pre-fetching */ >> #3 arm,data-prefetch = <0>; >> >> /* do nothing, empty aka retain existing settings set by firmware */ >> #4 >> >> Based on the code, only #2 and #3 are intended, which raises the >> question, do not we want of_read_property_bool() instead then? The >> binding does seem to suggest that only #1 and #4 are valid though. > > I re-read the code, and I think if you just clarify the values in the > binding to be: 0 (forcibly disable), 1 (forcibly enable), property > absent (retain settings), this would be crystal clear. Ok I will do that. Hauke ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior @ 2015-05-29 18:46 ` Hauke Mehrtens 0 siblings, 0 replies; 16+ messages in thread From: Hauke Mehrtens @ 2015-05-29 18:46 UTC (permalink / raw) To: Florian Fainelli, linux-lFZ/pmaqli7XmaaqVzeoHQ, arnd-r2nGTMty4D4 Cc: geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, catalin.marinas-5wv7dgnIgG8 On 05/29/2015 08:35 PM, Florian Fainelli wrote: > On 29/05/15 11:30, Florian Fainelli wrote: >> On 29/05/15 11:11, Hauke Mehrtens wrote: >>> >>> >>> On 05/29/2015 07:52 PM, Florian Fainelli wrote: >>>> On 15/05/15 14:52, Hauke Mehrtens wrote: >>>>> These options make it possible to overwrites the data and instruction >>>>> prefetching behavior of the arm pl310 cache controller. >>>>> >>>>> Signed-off-by: Hauke Mehrtens <hauke-5/S+JYg5SzeELgA04lAiVw@public.gmane.org> >>>>> --- >>>>> v2: only set prefetch >>>>> v1: set prefetch and aux >>>>> >>>>> Documentation/devicetree/bindings/arm/l2cc.txt | 4 ++++ >>>>> arch/arm/mm/cache-l2x0.c | 20 ++++++++++++++++++++ >>>>> 2 files changed, 24 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >>>>> index 0dbabe9..528821a 100644 >>>>> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >>>>> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >>>>> @@ -67,6 +67,10 @@ Optional properties: >>>>> disable if zero. >>>>> - arm,prefetch-offset : Override prefetch offset value. Valid values are >>>>> 0-7, 15, 23, and 31. >>>>> +- arm,prefetch-data : Enable data prefetch. Enabling prefetching >>>>> + can improve performance. >>>> >>>> I do not think the "can improve performance" has a place in a binding, >>>> this is either not technical enough about what this does, or marketing >>>> enough it does not buy us much. >>>> >>>> data/instruction pre-fetching are commonly found on cache controller >>>> these days, so I would be tempted to remove the "arm," prefixing here >>>> since this can be generalized to other kinds of cache controllers. >>>> Documenting that this can be either a boolean, or accept a value (see >>>> below) could help. >>> >>> So you think I should only add prefetch-data and prefetch-instr without >>> the arm prefix. >> >> That's what I think yes, others may disagree. Are there any other opinions on this issue? >> >>>> >>>>> +- arm,prefetch-instr : Enable instruction prefetch. Enabling prefetching >>>>> + can improve performance. >>>>> >>>>> Example: >>>>> >>>>> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c >>>>> index e309c8f..1aa970a 100644 >>>>> --- a/arch/arm/mm/cache-l2x0.c >>>>> +++ b/arch/arm/mm/cache-l2x0.c >>>>> @@ -1199,6 +1199,26 @@ static void __init l2c310_of_parse(const struct device_node *np, >>>>> pr_err("L2C-310 OF arm,prefetch-offset property value is missing\n"); >>>>> } >>>>> >>>>> + ret = of_property_read_u32(np, "arm,prefetch-data", &val); >>>>> + if (ret == 0) { >>>>> + if (val) >>>>> + prefetch |= L310_PREFETCH_CTRL_DATA_PREFETCH; >>>>> + else >>>>> + prefetch &= ~L310_PREFETCH_CTRL_DATA_PREFETCH; >>>>> + } else if (ret != -EINVAL) { >>>>> + pr_err("L2C-310 OF arm,prefetch-data property value is missing\n"); >>>>> + } >>>> >>>> If we want to generalize the use of this property, there could indeed be >>>> a value associated with it, if the cache controller supports different >>>> pre-fetching strides, however this is not the cache for these cache >>>> controllers it seems, are not we going to show error messages more often >>>> than desired? >>> I did this so it is possible to deactivate the prefech mode. I do not >>> know if somebody wants to do that. I haven't understood how you suggest >>> I should change. >>> When you do not associate a value with an entry in device tree it is >>> there or not there, so we could only activate it when it was not >>> automatically detected, but we could not deactivate it, because the case >>> when this value is not specified in device tree would be, use to auto >>> detected the value. >> >> My point is that you use of_read_property_u32, but your example does not >> state what should be the value associated with this property in the >> binding, so it is unclear what are the results without looking at the >> code between these examples: >> >> /* Enable data pre-fetching */ >> #1 arm,data-prefetch; >> #2 arm,data-prefetch = <1>; >> >> /* Disable data pre-fetching */ >> #3 arm,data-prefetch = <0>; >> >> /* do nothing, empty aka retain existing settings set by firmware */ >> #4 >> >> Based on the code, only #2 and #3 are intended, which raises the >> question, do not we want of_read_property_bool() instead then? The >> binding does seem to suggest that only #1 and #4 are valid though. > > I re-read the code, and I think if you just clarify the values in the > binding to be: 0 (forcibly disable), 1 (forcibly enable), property > absent (retain settings), this would be crystal clear. Ok I will do that. Hauke -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-05-29 18:46 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-15 21:52 [PATCH v3] ARM: l2c: add options to overwrite prefetching behavior Hauke Mehrtens 2015-05-15 21:52 ` Hauke Mehrtens 2015-05-29 17:33 ` Hauke Mehrtens 2015-05-29 17:33 ` Hauke Mehrtens 2015-05-29 17:49 ` Russell King - ARM Linux 2015-05-29 17:49 ` Russell King - ARM Linux 2015-05-29 17:52 ` Florian Fainelli 2015-05-29 17:52 ` Florian Fainelli 2015-05-29 18:11 ` Hauke Mehrtens 2015-05-29 18:11 ` Hauke Mehrtens 2015-05-29 18:30 ` Florian Fainelli 2015-05-29 18:30 ` Florian Fainelli 2015-05-29 18:35 ` Florian Fainelli 2015-05-29 18:35 ` Florian Fainelli 2015-05-29 18:46 ` Hauke Mehrtens 2015-05-29 18:46 ` Hauke Mehrtens
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.