[PATCH] ARM: l2x0: add prefetch and power ctrl registers configuration support

[PATCH] ARM: l2x0: add prefetch and power ctrl registers configuration support

[Jisheng Zhang]

PL310 supports Prefetch offset/control register from r2p0 and Power
control register from r3p0. This patch adds the support to configure
these two registers if there are. The dt binding document is also updated.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 Documentation/devicetree/bindings/arm/l2cc.txt |  4 ++++
 arch/arm/mm/cache-l2x0.c                       | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
index c0c7626..32cd08c 100644
--- a/Documentation/devicetree/bindings/arm/l2cc.txt
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -39,6 +39,10 @@ Optional properties:
 - arm,filter-ranges : <start length> Starting address and length of window to
   filter. Addresses in the filter window are directed to the M1 port. Other
   addresses will go to the M0 port.
+- arm,prefetch-ctrl : The value for Prefetch Offset/Control Register if there
+  is. This is a single cell.
+- arm,pwr-ctrl : The value for Power Control Register if there is. This is a
+  single cell.
 - interrupts : 1 combined interrupt.
 - cache-id-part: cache id part number to be used if it is not present
   on hardware
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 447da6f..8f536ea 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -704,6 +704,8 @@ static void __init pl310_of_setup(const struct device_node *np,
 	u32 data[3] = { 0, 0, 0 };
 	u32 tag[3] = { 0, 0, 0 };
 	u32 filter[2] = { 0, 0 };
+	u32 l2x0_revision = readl_relaxed(l2x0_base + L2X0_CACHE_ID) &
+		L2X0_CACHE_ID_RTL_MASK;
 
 	of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag));
 	if (tag[0] && tag[1] && tag[2])
@@ -730,6 +732,23 @@ static void __init pl310_of_setup(const struct device_node *np,
 		writel_relaxed((filter[0] & ~(SZ_1M - 1)) | L2X0_ADDR_FILTER_EN,
 			       l2x0_base + L2X0_ADDR_FILTER_START);
 	}
+
+	if (l2x0_revision >= L2X0_CACHE_ID_RTL_R2P0) {
+		u32 prefetch_ctrl = 0;
+
+		of_property_read_u32(np, "arm,prefetch-ctrl",
+				     &prefetch_ctrl);
+		if (prefetch_ctrl)
+			writel_relaxed(prefetch_ctrl, l2x0_base +
+					L2X0_PREFETCH_CTRL);
+		if (l2x0_revision >= L2X0_CACHE_ID_RTL_R3P0) {
+			u32 pwr_ctrl = 0;
+			of_property_read_u32(np, "arm,pwr-ctrl", &pwr_ctrl);
+			if (pwr_ctrl)
+				writel_relaxed(pwr_ctrl, l2x0_base +
+						L2X0_POWER_CTRL);
+		}
+	}
 }
 
 static void __init pl310_save(void)
-- 
1.8.4.2

Re: [PATCH] ARM: l2x0: add prefetch and power ctrl registers configuration support

[Sebastian Hesselbarth]

On 11/07/13 06:07, Jisheng Zhang wrote:
> PL310 supports Prefetch offset/control register from r2p0 and Power
> control register from r3p0. This patch adds the support to configure
> these two registers if there are. The dt binding document is also updated.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   Documentation/devicetree/bindings/arm/l2cc.txt |  4 ++++
>   arch/arm/mm/cache-l2x0.c                       | 19 +++++++++++++++++++
>   2 files changed, 23 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
> index c0c7626..32cd08c 100644
> --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> @@ -39,6 +39,10 @@ Optional properties:
>   - arm,filter-ranges : <start length> Starting address and length of window to
>     filter. Addresses in the filter window are directed to the M1 port. Other
>     addresses will go to the M0 port.
> +- arm,prefetch-ctrl : The value for Prefetch Offset/Control Register if there
> +  is. This is a single cell.
> +- arm,pwr-ctrl : The value for Power Control Register if there is. This is a
> +  single cell.

Jisheng,

I did some research over L2x0/L310 aux-ctrl registers and possible
properties for them. Both properties introduced here, just allow you
to put in a magic number, while IMHO properties should better reflect
the actual function rather than hard-coded values.

A quick look at L310 TRM for Power Control Register gives:

Bit 1: dynamic_clk_gating_en (1=enabled, 0=masked)
Bit 0: standby mode en (1=enabled, 0=masked)

So, properties should one of:

(a) arm,dynamic-clock-gating = <0/1>
     arm,standby-mode = <0/1>
(b) arm,dynamic-clock-gating-enable/-disable;
     arm,standby-mode-enable/-disable

If we want to have set/clear/don't touch behavior, I'd prefer (a) as
it is easier to deal with in code. Moreover, -enable/-disable of (b)
may not always fit the property name, so there will also be
arm,no-feature-foo or arm,force-feature-bar.

The same also applies for Prefetch Control register, too.

Sebastian

>   - interrupts : 1 combined interrupt.
>   - cache-id-part: cache id part number to be used if it is not present
>     on hardware
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 447da6f..8f536ea 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -704,6 +704,8 @@ static void __init pl310_of_setup(const struct device_node *np,
>   	u32 data[3] = { 0, 0, 0 };
>   	u32 tag[3] = { 0, 0, 0 };
>   	u32 filter[2] = { 0, 0 };
> +	u32 l2x0_revision = readl_relaxed(l2x0_base + L2X0_CACHE_ID) &
> +		L2X0_CACHE_ID_RTL_MASK;
>
>   	of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag));
>   	if (tag[0] && tag[1] && tag[2])
> @@ -730,6 +732,23 @@ static void __init pl310_of_setup(const struct device_node *np,
>   		writel_relaxed((filter[0] & ~(SZ_1M - 1)) | L2X0_ADDR_FILTER_EN,
>   			       l2x0_base + L2X0_ADDR_FILTER_START);
>   	}
> +
> +	if (l2x0_revision >= L2X0_CACHE_ID_RTL_R2P0) {
> +		u32 prefetch_ctrl = 0;
> +
> +		of_property_read_u32(np, "arm,prefetch-ctrl",
> +				     &prefetch_ctrl);
> +		if (prefetch_ctrl)
> +			writel_relaxed(prefetch_ctrl, l2x0_base +
> +					L2X0_PREFETCH_CTRL);
> +		if (l2x0_revision >= L2X0_CACHE_ID_RTL_R3P0) {
> +			u32 pwr_ctrl = 0;
> +			of_property_read_u32(np, "arm,pwr-ctrl", &pwr_ctrl);
> +			if (pwr_ctrl)
> +				writel_relaxed(pwr_ctrl, l2x0_base +
> +						L2X0_POWER_CTRL);
> +		}
> +	}
>   }
>
>   static void __init pl310_save(void)
>

Re: [PATCH] ARM: l2x0: add prefetch and power ctrl registers configuration support

[Andrew Lunn]

On Thu, Nov 07, 2013 at 01:46:44PM +0100, Sebastian Hesselbarth wrote:
> On 11/07/13 06:07, Jisheng Zhang wrote:
> >PL310 supports Prefetch offset/control register from r2p0 and Power
> >control register from r3p0. This patch adds the support to configure
> >these two registers if there are. The dt binding document is also updated.
> >
> >Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >---
> >  Documentation/devicetree/bindings/arm/l2cc.txt |  4 ++++
> >  arch/arm/mm/cache-l2x0.c                       | 19 +++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> >
> >diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
> >index c0c7626..32cd08c 100644
> >--- a/Documentation/devicetree/bindings/arm/l2cc.txt
> >+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> >@@ -39,6 +39,10 @@ Optional properties:
> >  - arm,filter-ranges : <start length> Starting address and length of window to
> >    filter. Addresses in the filter window are directed to the M1 port. Other
> >    addresses will go to the M0 port.
> >+- arm,prefetch-ctrl : The value for Prefetch Offset/Control Register if there
> >+  is. This is a single cell.
> >+- arm,pwr-ctrl : The value for Power Control Register if there is. This is a
> >+  single cell.
> 
> Jisheng,
> 
> I did some research over L2x0/L310 aux-ctrl registers and possible
> properties for them. Both properties introduced here, just allow you
> to put in a magic number, while IMHO properties should better reflect
> the actual function rather than hard-coded values.
> 
> A quick look at L310 TRM for Power Control Register gives:
> 
> Bit 1: dynamic_clk_gating_en (1=enabled, 0=masked)
> Bit 0: standby mode en (1=enabled, 0=masked)
> 
> So, properties should one of:
> 
> (a) arm,dynamic-clock-gating = <0/1>
>     arm,standby-mode = <0/1>
> (b) arm,dynamic-clock-gating-enable/-disable;
>     arm,standby-mode-enable/-disable
> 
> If we want to have set/clear/don't touch behavior, I'd prefer (a) as
> it is easier to deal with in code. Moreover, -enable/-disable of (b)
> may not always fit the property name, so there will also be
> arm,no-feature-foo or arm,force-feature-bar.

Hi Sebastian

There have been some comments about properties with Boolean
values. Rather than having a Boolean, you simply don't list the
property.

What you have here is slightly different, since you have
set/clear/don't touch. How to represent this should be decided once by
the DT folks, and used consistently in all bindings.

    Andrew

Re: [PATCH] ARM: l2x0: add prefetch and power ctrl registers configuration support

[Mark Rutland]

On Thu, Nov 07, 2013 at 05:07:52AM +0000, Jisheng Zhang wrote:
> PL310 supports Prefetch offset/control register from r2p0 and Power
> control register from r3p0. This patch adds the support to configure
> these two registers if there are. The dt binding document is also updated.

I'd like to see a reasoning as to _why_ these should be in the DT.

While we have tag and data RAM latency information, those are hardware
properties that we cannot probe. I'm not so clear on the filter-ranges
property.

These bits seem to be configuration rather than a hardware description.
If there are some portions of this that we can describe with higher
level properties, I'd prefer to do that.

But primarily, the question to answer is do we need these, and if so do
they belong in the DT?

> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  Documentation/devicetree/bindings/arm/l2cc.txt |  4 ++++
>  arch/arm/mm/cache-l2x0.c                       | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
> index c0c7626..32cd08c 100644
> --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> @@ -39,6 +39,10 @@ Optional properties:
>  - arm,filter-ranges : <start length> Starting address and length of window to
>    filter. Addresses in the filter window are directed to the M1 port. Other
>    addresses will go to the M0 port.
> +- arm,prefetch-ctrl : The value for Prefetch Offset/Control Register if there
> +  is. This is a single cell.
> +- arm,pwr-ctrl : The value for Power Control Register if there is. This is a
> +  single cell.
>  - interrupts : 1 combined interrupt.
>  - cache-id-part: cache id part number to be used if it is not present
>    on hardware
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index 447da6f..8f536ea 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -704,6 +704,8 @@ static void __init pl310_of_setup(const struct device_node *np,
>  	u32 data[3] = { 0, 0, 0 };
>  	u32 tag[3] = { 0, 0, 0 };
>  	u32 filter[2] = { 0, 0 };
> +	u32 l2x0_revision = readl_relaxed(l2x0_base + L2X0_CACHE_ID) &
> +		L2X0_CACHE_ID_RTL_MASK;
>  
>  	of_property_read_u32_array(np, "arm,tag-latency", tag, ARRAY_SIZE(tag));
>  	if (tag[0] && tag[1] && tag[2])
> @@ -730,6 +732,23 @@ static void __init pl310_of_setup(const struct device_node *np,
>  		writel_relaxed((filter[0] & ~(SZ_1M - 1)) | L2X0_ADDR_FILTER_EN,
>  			       l2x0_base + L2X0_ADDR_FILTER_START);
>  	}
> +
> +	if (l2x0_revision >= L2X0_CACHE_ID_RTL_R2P0) {
> +		u32 prefetch_ctrl = 0;
> +
> +		of_property_read_u32(np, "arm,prefetch-ctrl",
> +				     &prefetch_ctrl);
> +		if (prefetch_ctrl)
> +			writel_relaxed(prefetch_ctrl, l2x0_base +
> +					L2X0_PREFETCH_CTRL);

Some of the prefetch control regsiter bits are reserved, and the
prefetch offset may only take a subset of possible values. I wouldn't
want to poke the hardware without performing some sanity checking on
these values.

> +		if (l2x0_revision >= L2X0_CACHE_ID_RTL_R3P0) {
> +			u32 pwr_ctrl = 0;
> +			of_property_read_u32(np, "arm,pwr-ctrl", &pwr_ctrl);
> +			if (pwr_ctrl)
> +				writel_relaxed(pwr_ctrl, l2x0_base +
> +						L2X0_POWER_CTRL);

Similarly, all but the lower 2 bits are reserved...

Thanks,
Mark.

Re: [PATCH] ARM: l2x0: add prefetch and power ctrl registers configuration support

[Jisheng Zhang]

Hi Mark,

On Wed, 27 Nov 2013 09:42:04 -0800
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Nov 07, 2013 at 05:07:52AM +0000, Jisheng Zhang wrote:
> > PL310 supports Prefetch offset/control register from r2p0 and Power
> > control register from r3p0. This patch adds the support to configure
> > these two registers if there are. The dt binding document is also updated.
> 
> I'd like to see a reasoning as to _why_ these should be in the DT.
> 
> While we have tag and data RAM latency information, those are hardware
> properties that we cannot probe. I'm not so clear on the filter-ranges
> property.
> 
> These bits seem to be configuration rather than a hardware description.
> If there are some portions of this that we can describe with higher
> level properties, I'd prefer to do that.
> 
> But primarily, the question to answer is do we need these, and if so do
> they belong in the DT?


After more consideration, configuring these two registers in bootloader or
TrustZone firmware is more reasonable since the prefetch controller can only be
written in Secure World.

PS: the tag/ram latency must also be written in secure world, and IMHO, the
latency and filter-ranges are also configurations

Thanks for having look at this patch,
Jisheng

> 
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >  Documentation/devicetree/bindings/arm/l2cc.txt |  4 ++++
> >  arch/arm/mm/cache-l2x0.c                       | 19 +++++++++++++++++++
> >  2 files changed, 23 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt
> > b/Documentation/devicetree/bindings/arm/l2cc.txt index c0c7626..32cd08c
> > 100644 --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> > +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> > @@ -39,6 +39,10 @@ Optional properties:
> >  - arm,filter-ranges : <start length> Starting address and length of
> > window to filter. Addresses in the filter window are directed to the M1
> > port. Other addresses will go to the M0 port.
> > +- arm,prefetch-ctrl : The value for Prefetch Offset/Control Register if
> > there
> > +  is. This is a single cell.
> > +- arm,pwr-ctrl : The value for Power Control Register if there is. This
> > is a
> > +  single cell.
> >  - interrupts : 1 combined interrupt.
> >  - cache-id-part: cache id part number to be used if it is not present
> >    on hardware
> > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> > index 447da6f..8f536ea 100644
> > --- a/arch/arm/mm/cache-l2x0.c
> > +++ b/arch/arm/mm/cache-l2x0.c
> > @@ -704,6 +704,8 @@ static void __init pl310_of_setup(const struct
> > device_node *np, u32 data[3] = { 0, 0, 0 };
> >  	u32 tag[3] = { 0, 0, 0 };
> >  	u32 filter[2] = { 0, 0 };
> > +	u32 l2x0_revision = readl_relaxed(l2x0_base + L2X0_CACHE_ID) &
> > +		L2X0_CACHE_ID_RTL_MASK;
> >  
> >  	of_property_read_u32_array(np, "arm,tag-latency", tag,
> > ARRAY_SIZE(tag)); if (tag[0] && tag[1] && tag[2])
> > @@ -730,6 +732,23 @@ static void __init pl310_of_setup(const struct
> > device_node *np, writel_relaxed((filter[0] & ~(SZ_1M - 1)) |
> > L2X0_ADDR_FILTER_EN, l2x0_base + L2X0_ADDR_FILTER_START);
> >  	}
> > +
> > +	if (l2x0_revision >= L2X0_CACHE_ID_RTL_R2P0) {
> > +		u32 prefetch_ctrl = 0;
> > +
> > +		of_property_read_u32(np, "arm,prefetch-ctrl",
> > +				     &prefetch_ctrl);
> > +		if (prefetch_ctrl)
> > +			writel_relaxed(prefetch_ctrl, l2x0_base +
> > +					L2X0_PREFETCH_CTRL);
> 
> Some of the prefetch control regsiter bits are reserved, and the
> prefetch offset may only take a subset of possible values. I wouldn't
> want to poke the hardware without performing some sanity checking on
> these values.
> 
> > +		if (l2x0_revision >= L2X0_CACHE_ID_RTL_R3P0) {
> > +			u32 pwr_ctrl = 0;
> > +			of_property_read_u32(np, "arm,pwr-ctrl",
> > &pwr_ctrl);
> > +			if (pwr_ctrl)
> > +				writel_relaxed(pwr_ctrl, l2x0_base +
> > +						L2X0_POWER_CTRL);
> 
> Similarly, all but the lower 2 bits are reserved...
> 
> Thanks,
> Mark.