| Message ID | 1411556741-5810-5-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 24, 2014 at 12:05:38PM +0100, Marek Szyprowski wrote: > From: Tomasz Figa <t.figa@samsung.com> > > Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch > settings configured in registers leading to crashes if L2C is enabled > without overriding them. This patch introduces bindings to enable > prefetch settings to be specified from DT and necessary support in the > driver. > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > Documentation/devicetree/bindings/arm/l2cc.txt | 10 +++++++ > arch/arm/mm/cache-l2x0.c | 39 ++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > index af527ee111c2..3443d2d76788 100644 > --- a/Documentation/devicetree/bindings/arm/l2cc.txt > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > @@ -47,6 +47,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,double-linefill : Override double linefill enable setting. Enable if > + non-zero, disable if zero. > +- arm,double-linefill-incr : Override double linefill on INCR read. Enable > + if non-zero, disable if zero. > +- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable > + if non-zero, disable if zero. > +- arm,prefetch-drop : Override prefetch drop enable setting. Enable if non-zero, > + disable if zero. I'm not too keen on tristate properties. Is this level of flexibility really required? What exact overrides do you need for boards you know of? Why do these cause crashes if not overridden? Mark. > +- arm,prefetch-offset : Override prefetch offset value. Valid values are > + 0-7, 15, 23, and 31. > > Example: > > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c > index 84c6c55ab896..af90a6ff6b49 100644 > --- a/arch/arm/mm/cache-l2x0.c > +++ b/arch/arm/mm/cache-l2x0.c > @@ -1059,6 +1059,8 @@ static void __init l2c310_of_parse(const struct device_node *np, > u32 data[3] = { 0, 0, 0 }; > u32 tag[3] = { 0, 0, 0 }; > u32 filter[2] = { 0, 0 }; > + u32 prefetch; > + u32 val; > > of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag)); > if (tag[0] && tag[1] && tag[2]) > @@ -1083,6 +1085,43 @@ static void __init l2c310_of_parse(const struct device_node *np, > l2x0_saved_regs.filter_start = (filter[0] & ~(SZ_1M - 1)) > | L310_ADDR_FILTER_EN; > } > + > + prefetch = l2x0_saved_regs.prefetch_ctrl; > + > + if (!of_property_read_u32(np, "arm,double-linefill", &val)) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL; > + else > + prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL; > + } > + > + if (!of_property_read_u32(np, "arm,double-linefill-incr", &val)) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR; > + else > + prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL_INCR; > + } > + > + if (!of_property_read_u32(np, "arm,double-linefill-wrap", &val)) { > + if (!val) > + prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP; > + else > + prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP; > + } > + > + if (!of_property_read_u32(np, "arm,prefetch-drop", &val)) { > + if (val) > + prefetch |= L310_PREFETCH_CTRL_PREFETCH_DROP; > + else > + prefetch &= ~L310_PREFETCH_CTRL_PREFETCH_DROP; > + } > + > + if (!of_property_read_u32(np, "arm,prefetch-offset", &val)) { > + prefetch &= ~L310_PREFETCH_CTRL_OFFSET_MASK; > + prefetch |= val & L310_PREFETCH_CTRL_OFFSET_MASK; > + } > + > + l2x0_saved_regs.prefetch_ctrl = prefetch; > } > > static const struct l2c_init_data of_l2c310_data __initconst = { > -- > 1.9.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24.09.2014 13:14, Mark Rutland wrote: > On Wed, Sep 24, 2014 at 12:05:38PM +0100, Marek Szyprowski wrote: >> From: Tomasz Figa <t.figa@samsung.com> >> >> Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch >> settings configured in registers leading to crashes if L2C is enabled >> without overriding them. This patch introduces bindings to enable >> prefetch settings to be specified from DT and necessary support in the >> driver. >> >> Signed-off-by: Tomasz Figa <t.figa@samsung.com> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> Documentation/devicetree/bindings/arm/l2cc.txt | 10 +++++++ >> arch/arm/mm/cache-l2x0.c | 39 ++++++++++++++++++++++++++ >> 2 files changed, 49 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt >> index af527ee111c2..3443d2d76788 100644 >> --- a/Documentation/devicetree/bindings/arm/l2cc.txt >> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt >> @@ -47,6 +47,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,double-linefill : Override double linefill enable setting. Enable if >> + non-zero, disable if zero. >> +- arm,double-linefill-incr : Override double linefill on INCR read. Enable >> + if non-zero, disable if zero. >> +- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable >> + if non-zero, disable if zero. >> +- arm,prefetch-drop : Override prefetch drop enable setting. Enable if non-zero, >> + disable if zero. > > I'm not too keen on tristate properties. Is this level of flexibility > really required? > > What exact overrides do you need for boards you know of? Why do these > cause crashes if not overridden? Well, this is all thanks to broken firmware, which doesn't initialize those values properly on certain systems and they must be overridden. On the other side, there are systems with firmware that does it correctly and for those the boot defaults should be preferred. I don't see any other reasonable option of handling this. As for why they cause crashes, I'm not an expert if it is about L2C internals, so I can't really tell, but apparently the cache can work correctly only on certain settings. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, Sep 24, 2014 at 12:19:45PM +0100, Tomasz Figa wrote: > On 24.09.2014 13:14, Mark Rutland wrote: > > On Wed, Sep 24, 2014 at 12:05:38PM +0100, Marek Szyprowski wrote: > >> From: Tomasz Figa <t.figa@samsung.com> > >> > >> Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch > >> settings configured in registers leading to crashes if L2C is enabled > >> without overriding them. This patch introduces bindings to enable > >> prefetch settings to be specified from DT and necessary support in the > >> driver. > >> > >> Signed-off-by: Tomasz Figa <t.figa@samsung.com> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> --- > >> Documentation/devicetree/bindings/arm/l2cc.txt | 10 +++++++ > >> arch/arm/mm/cache-l2x0.c | 39 ++++++++++++++++++++++++++ > >> 2 files changed, 49 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt > >> index af527ee111c2..3443d2d76788 100644 > >> --- a/Documentation/devicetree/bindings/arm/l2cc.txt > >> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt > >> @@ -47,6 +47,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,double-linefill : Override double linefill enable setting. Enable if > >> + non-zero, disable if zero. > >> +- arm,double-linefill-incr : Override double linefill on INCR read. Enable > >> + if non-zero, disable if zero. > >> +- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable > >> + if non-zero, disable if zero. > >> +- arm,prefetch-drop : Override prefetch drop enable setting. Enable if non-zero, > >> + disable if zero. > > > > I'm not too keen on tristate properties. Is this level of flexibility > > really required? > > > > What exact overrides do you need for boards you know of? Why do these > > cause crashes if not overridden? > > Well, this is all thanks to broken firmware, which doesn't initialize > those values properly on certain systems and they must be overridden. On > the other side, there are systems with firmware that does it correctly > and for those the boot defaults should be preferred. I don't see any > other reasonable option of handling this. With the lack of warnings for present but empty properties, I can forsee people placing empty properties (as the names make them sound like booleans which enable features). Surely for enabling/disabling options we should only need to override one-way, disabling a feature that causes breakage for some reason? Otherwise we can keep the reset value (which might be sub-optimal). Perhaps a simple warning is sufficient if the property exists but is empty. > As for why they cause crashes, I'm not an expert if it is about L2C > internals, so I can't really tell, but apparently the cache can work > correctly only on certain settings. Likewise, I'm no expert on the l2x0 family. It would be nice to know if this justs masks an issue elsewhere in Linux, is required for some reason by the interconnect, etc. I guess we don't have enough information to figure that out. Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, Sep 24, 2014 at 01:10:51PM +0100, Mark Rutland wrote: > On Wed, Sep 24, 2014 at 12:19:45PM +0100, Tomasz Figa wrote: > > On 24.09.2014 13:14, Mark Rutland wrote: > > > I'm not too keen on tristate properties. Is this level of flexibility > > > really required? I would say that we need a way to enable _and_ disable these features, because: 1. When we converted the L2 code to DT, no one thought about creating a full and proper DT specification for the L2 code. So, we have the situation today where platform code (and firmware code) enables or disables these features depending on platform knowledge. 2. If we are going to specify these options as a boolean-enable value, this implies that the lack of the boolean disables the option. We have pre-existing DT files which do not contain this option, but the platform may rely on the feature being enabled. This is precisely the kind of mess that happens when incomplete DT bindings are accepted. DT bindings for any device should be _full_ bindings from the start, and not a half-hearted attempt at the job. As much as we don't like tristate properties, we only have ourselves to blame for them becoming a necessity in cases like this. > With the lack of warnings for present but empty properties, I can forsee > people placing empty properties (as the names make them sound like > booleans which enable features). > > Surely for enabling/disabling options we should only need to override > one-way, disabling a feature that causes breakage for some reason? > Otherwise we can keep the reset value (which might be sub-optimal). What if enabling prefetching on an existing platform causes a failure? That's a regression on a previously working DT file, and needing a DT update to resolve. The obvious alternative is we have two properties per feature - one boolean to enable, and one boolean to disable the feature. We already have a number of negative properties because of exactly the same problem I mention above (where the driver has assumed defaults for one platform which are not appropriate for all platforms.) > > As for why they cause crashes, I'm not an expert if it is about L2C > > internals, so I can't really tell, but apparently the cache can work > > correctly only on certain settings. > > Likewise, I'm no expert on the l2x0 family. It would be nice to know if > this justs masks an issue elsewhere in Linux, is required for some > reason by the interconnect, etc. I guess we don't have enough > information to figure that out. No, it would be nice to have some errata information...
diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt index af527ee111c2..3443d2d76788 100644 --- a/Documentation/devicetree/bindings/arm/l2cc.txt +++ b/Documentation/devicetree/bindings/arm/l2cc.txt @@ -47,6 +47,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,double-linefill : Override double linefill enable setting. Enable if + non-zero, disable if zero. +- arm,double-linefill-incr : Override double linefill on INCR read. Enable + if non-zero, disable if zero. +- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable + if non-zero, disable if zero. +- arm,prefetch-drop : Override prefetch drop enable setting. Enable if non-zero, + disable if zero. +- arm,prefetch-offset : Override prefetch offset value. Valid values are + 0-7, 15, 23, and 31. Example: diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c index 84c6c55ab896..af90a6ff6b49 100644 --- a/arch/arm/mm/cache-l2x0.c +++ b/arch/arm/mm/cache-l2x0.c @@ -1059,6 +1059,8 @@ static void __init l2c310_of_parse(const struct device_node *np, u32 data[3] = { 0, 0, 0 }; u32 tag[3] = { 0, 0, 0 }; u32 filter[2] = { 0, 0 }; + u32 prefetch; + u32 val; of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag)); if (tag[0] && tag[1] && tag[2]) @@ -1083,6 +1085,43 @@ static void __init l2c310_of_parse(const struct device_node *np, l2x0_saved_regs.filter_start = (filter[0] & ~(SZ_1M - 1)) | L310_ADDR_FILTER_EN; } + + prefetch = l2x0_saved_regs.prefetch_ctrl; + + if (!of_property_read_u32(np, "arm,double-linefill", &val)) { + if (val) + prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL; + else + prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL; + } + + if (!of_property_read_u32(np, "arm,double-linefill-incr", &val)) { + if (val) + prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR; + else + prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL_INCR; + } + + if (!of_property_read_u32(np, "arm,double-linefill-wrap", &val)) { + if (!val) + prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP; + else + prefetch &= ~L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP; + } + + if (!of_property_read_u32(np, "arm,prefetch-drop", &val)) { + if (val) + prefetch |= L310_PREFETCH_CTRL_PREFETCH_DROP; + else + prefetch &= ~L310_PREFETCH_CTRL_PREFETCH_DROP; + } + + if (!of_property_read_u32(np, "arm,prefetch-offset", &val)) { + prefetch &= ~L310_PREFETCH_CTRL_OFFSET_MASK; + prefetch |= val & L310_PREFETCH_CTRL_OFFSET_MASK; + } + + l2x0_saved_regs.prefetch_ctrl = prefetch; } static const struct l2c_init_data of_l2c310_data __initconst = {