diff mbox

[v5,4/7] ARM: l2c: Add support for overriding prefetch settings

Message ID 1411556741-5810-5-git-send-email-m.szyprowski@samsung.com
State New
Headers show

Commit Message

Marek Szyprowski Sept. 24, 2014, 11:05 a.m. UTC
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(+)

Comments

Mark Rutland Sept. 24, 2014, 11:14 a.m. UTC | #1
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
Tomasz Figa Sept. 24, 2014, 11:19 a.m. UTC | #2
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/
Mark Rutland Sept. 24, 2014, 12:10 p.m. UTC | #3
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/
Russell King - ARM Linux Sept. 24, 2014, 10:12 p.m. UTC | #4
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 mbox

Patch

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 = {