arm: l2x0: add support for prefetch and pwr control register configuration
Subsystems (2)
PATCH 1/1 arm: l2x0: add support for prefetch and pwr control register configuration
This patch adds support to configure prefetch and pwr control registers of pl310 cache controllor.
--- Documentation/devicetree/bindings/arm/l2cc.txt | 6 ++++ arch/arm/include/asm/hardware/cache-l2x0.h | 23 +++++++++++++ arch/arm/mm/cache-l2x0.c | 44 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+)
Documentation/devicetree/bindings/arm/l2cc.txt +6 -0
@@ -34,6 +34,12 @@ 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-control : Prefetch related configurations. Specifies 8 cells of + prefetch-offset, not-same-ID-on-exclusive-sequence-en, incr-double-Linefill-en, + prefetch-drop-en, data-prefetch-en, instruction-prefetch-en and + double-linefill-en. +- arm,pwr-control : Operting mode clock and power mode selection. Specifies two + cells of standby-mode-en and dynamic_clk_gating_en. - interrupts : 1 combined interrupt. - cache-id-part: cache id part number to be used if it is not present on hardware
arch/arm/include/asm/hardware/cache-l2x0.h +23 -0
@@ -106,6 +106,29 @@#define L2X0_WAY_SIZE_SHIFT 3 +#define L2X0_PREFETCH_CTRL_OFFSET_SHIFT 0 +#define L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT 21 +#define L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT 23 +#define L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT 24 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT 27 +#define L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT 28 +#define L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT 29 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT 30 + +#define PREFETCH_R2P0_MASK (~(1 << L2X0_PREFETCH_CTRL_OFFSET_SHIFT)) +#define PREFETCH_R3P0_MASK \ + (~((1 << L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT))) +#define PREFETCH_R3P1_MASK \ + (~(1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT)) + +#define L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT 0 +#define L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT 1 +#ifndef __ASSEMBLY__extern void __init l2x0_init (void __iomem *base,u32 aux_val,u32 aux_mask);#if defined(CONFIG_CACHE_L2X0 ) && defined(CONFIG_OF )
arch/arm/mm/cache-l2x0.c +44 -0
56 lines not on this page · raw message
Signature · 1 line
1.7.9.5
PATCH 1/1 arm: l2x0: add support for prefetch and pwr control register configuration
Hi,
[adding Rob Herring to Cc]
Quoted 15 lines · Chander Kashyap
This patch adds support to configure prefetch and pwr control registers of pl310 cache controllor.
Signed-off-by Chander Kashyap <[email protected]>--- Documentation/devicetree/bindings/arm/l2cc.txt | 6 ++++ arch/arm/include/asm/hardware/cache-l2x0.h | 23 +++++++++++++ arch/arm/mm/cache-l2x0.c | 44 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+)Documentation/devicetree/bindings/arm/l2cc.txt +0
-0
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 40
Quoted 7 lines
Documentation/devicetree/bindings/arm/l2cc.txt +4
-0@@ -34,6 +34,12 @@ 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-control : Prefetch related configurations. Specifies 8 cells of + prefetch-offset, not-same-ID-on-exclusive-sequence-en, incr-double-Linefill-en, + prefetch-drop-en, data-prefetch-en, instruction-prefetch-en and + double-linefill-en.
This looks like configuration rather than hardware specification.
Is there any reason we couldn't have Linux configure this itself? I'm not sure having a fixed configuration of these values makes sense - the configuration values could interact badly with future changes to Linux (e.g. performance improvements or errata workarounds), and then we'd have to ignore the described values and/or decide on more suitable ones dynamically anyway.
I assume that these cells are meant to either be zero or one? If they're necessary, they should be separate boolean properties rather than grouped together by the register they're configured by (see the way arm,tag-latency and arm,data-latency are handled).
Also, I'd prefer "-enable" to "-en".
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 41
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+- arm,pwr-control : Operting mode clock and power mode selection. Specifies two
s/Operting/Operating/
I'd prefer "power" to "pwr" (though there's precedent for both in bindings...).
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 42
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+ cells of standby-mode-en and dynamic_clk_gating_en.
More boolean properties.
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 45
Quoted 3 lines
Documentation/devicetree/bindings/arm/l2cc.txt +0
-0- interrupts : 1 combined interrupt. - cache-id-part: cache id part number to be used if it is not present on hardware
No amendments to the example?
Quoted 51 lines
arch/arm/include/asm/hardware/cache-l2x0.h +23
-0@@ -106,6 +106,29 @@#define L2X0_WAY_SIZE_SHIFT 3 +#define L2X0_PREFETCH_CTRL_OFFSET_SHIFT 0 +#define L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT 21 +#define L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT 23 +#define L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT 24 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT 27 +#define L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT 28 +#define L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT 29 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT 30 + +#define PREFETCH_R2P0_MASK (~(1 << L2X0_PREFETCH_CTRL_OFFSET_SHIFT)) +#define PREFETCH_R3P0_MASK \ + (~((1 << L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT))) +#define PREFETCH_R3P1_MASK \ + (~(1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT)) + +#define L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT 0 +#define L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT 1 +#ifndef __ASSEMBLY__extern void __init l2x0_init (void __iomem *base,u32 aux_val,u32 aux_mask);#if defined(CONFIG_CACHE_L2X0 ) && defined(CONFIG_OF )arch/arm/mm/cache-l2x0.c +5
-0@@ -563,6 +563,11 @@ 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 prefetch[8 ] = {0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 }; +u32 prefetch_val; +u32 pwr[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 ])
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 598
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +2
-0@@ -589,6 +594,45 @@ 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 ) {
You didn't describe that some properties were dependent on a particular revision.
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+of_property_read_u32_array (np,"arm,prefetch-control" , + prefetch,ARRAY_SIZE (prefetch)); + prefetch_val =readl_relaxed (l2x0_base +L2X0_PREFETCH_CTRL ); + prefetch_val &=PREFETCH_R2P0_MASK ; + prefetch_val |= prefetch[0 ] <<L2X0_PREFETCH_CTRL_OFFSET_SHIFT ; +if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P0 ) { + prefetch_val &=PREFETCH_R3P0_MASK ; + prefetch_val |= + ((prefetch[1 ]) << + L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) |
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 618
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+ ((prefetch[2 ]) << +L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT ) | + ((prefetch[3 ]) << +L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT ) | + ((prefetch[5 ]) << +L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT ) | + ((prefetch[6 ]) << +L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT ) | + ((prefetch[7 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT );
You've not sanity checked these properties. What If I typo a value as something other than 0 or 1?
Use boolean properties and of_property_read_bool (or even better, configure this dynamically based on revision rather than having a dt description).
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 624
Quoted 6 lines
arch/arm/mm/cache-l2x0.c +6
-0+if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P1 ) { + prefetch_val &=PREFETCH_R3P1_MASK ; + prefetch_val |= + ((prefetch[4 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT ); + }
Another undocumented revision dependency.
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 633
Quoted 9 lines
arch/arm/mm/cache-l2x0.c +9
-0+writel_relaxed (prefetch_val, + l2x0_base +L2X0_PREFETCH_CTRL ); + +of_property_read_u32_array (np,"arm,pwr-control" , + pwr,ARRAY_SIZE (pwr)); +writel_relaxed ( + ((pwr[0 ]) <<L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT ) | + ((pwr[1 ]) <<L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT ), + l2x0_base +L2X0_POWER_CTRL );
Again, use boolean properties or do this dynamically.
Do none of these values need to be saved for the resume callback?
Thanks,
Mark.
Quoted 14 lines
arch/arm/mm/cache-l2x0.c +2
-0+ } + } }static void __init pl310_save (void )Signature · 7 lines
1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 arm: l2x0: add support for prefetch and pwr control register configuration
Quoted 3 lines · Mark Rutland
Hi,
[adding Rob Herring to Cc]
Thanks.
Quoted 11 lines · Chander Kashyap
Quoted 10 lines
This patch adds support to configure prefetch and pwr control registers of pl310 cache controllor.
Signed-off-by Chander Kashyap <[email protected]>--- Documentation/devicetree/bindings/arm/l2cc.txt | 6 ++++ arch/arm/include/asm/hardware/cache-l2x0.h | 23 +++++++++++++ arch/arm/mm/cache-l2x0.c | 44 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+)
Quoted 14 lines
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 40
Quoted 12 lines
Documentation/devicetree/bindings/arm/l2cc.txt +4
-0@@ -34,6 +34,12 @@ 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-control : Prefetch related configurations. Specifies 8 cells of + prefetch-offset, not-same-ID-on-exclusive-sequence-en, incr-double-Linefill-en, + prefetch-drop-en, data-prefetch-en, instruction-prefetch-en and + double-linefill-en.This looks like configuration rather than hardware specification.
I think weigh whether this is configuration based on the h/w or just user preference. This would be the former, but...
Quoted 6 lines
Is there any reason we couldn't have Linux configure this itself? I'm not sure having a fixed configuration of these values makes sense - the configuration values could interact badly with future changes to Linux (e.g. performance improvements or errata workarounds), and then we'd have to ignore the described values and/or decide on more suitable ones dynamically anyway.
Does having a variable configuration for these make sense either? I mean other than an errata work-around, why would you ever turn off prefetch or double linefill?
None of these fields can be set in NS world. We've already laid down the law in regards to setting secure only bits for errata work-arounds. I think this is no different and we should move towards the kernel doesn't touch aux ctrl at all. There might be an exception with the event bus enable, but you would still have the secure vs. non-secure issue and no way to detect that.
Rob
Quoted 153 lines
I assume that these cells are meant to either be zero or one? If they're necessary, they should be separate boolean properties rather than grouped together by the register they're configured by (see the way arm,tag-latency and arm,data-latency are handled).
Also, I'd prefer "-enable" to "-en".
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 41
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+- arm,pwr-control : Operting mode clock and power mode selection. Specifies twos/Operting/Operating/
I'd prefer "power" to "pwr" (though there's precedent for both in bindings...).
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 42
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+ cells of standby-mode-en and dynamic_clk_gating_en.More boolean properties.
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 45
Quoted 3 lines
Documentation/devicetree/bindings/arm/l2cc.txt +0
-0- interrupts : 1 combined interrupt. - cache-id-part: cache id part number to be used if it is not present on hardwareNo amendments to the example?
Quoted 51 lines
arch/arm/include/asm/hardware/cache-l2x0.h +23
-0@@ -106,6 +106,29 @@#define L2X0_WAY_SIZE_SHIFT 3 +#define L2X0_PREFETCH_CTRL_OFFSET_SHIFT 0 +#define L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT 21 +#define L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT 23 +#define L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT 24 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT 27 +#define L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT 28 +#define L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT 29 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT 30 + +#define PREFETCH_R2P0_MASK (~(1 << L2X0_PREFETCH_CTRL_OFFSET_SHIFT)) +#define PREFETCH_R3P0_MASK \ + (~((1 << L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT))) +#define PREFETCH_R3P1_MASK \ + (~(1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT)) + +#define L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT 0 +#define L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT 1 +#ifndef __ASSEMBLY__extern void __init l2x0_init (void __iomem *base,u32 aux_val,u32 aux_mask);#if defined(CONFIG_CACHE_L2X0 ) && defined(CONFIG_OF )arch/arm/mm/cache-l2x0.c +5
-0@@ -563,6 +563,11 @@ 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 prefetch[8 ] = {0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 }; +u32 prefetch_val; +u32 pwr[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 ])arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 598
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +2
-0@@ -589,6 +594,45 @@ 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 ) {You didn't describe that some properties were dependent on a particular revision.
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+of_property_read_u32_array (np,"arm,prefetch-control" , + prefetch,ARRAY_SIZE (prefetch)); + prefetch_val =readl_relaxed (l2x0_base +L2X0_PREFETCH_CTRL ); + prefetch_val &=PREFETCH_R2P0_MASK ; + prefetch_val |= prefetch[0 ] <<L2X0_PREFETCH_CTRL_OFFSET_SHIFT ; +if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P0 ) { + prefetch_val &=PREFETCH_R3P0_MASK ; + prefetch_val |= + ((prefetch[1 ]) << + L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) |arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 618
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+ ((prefetch[2 ]) << +L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT ) | + ((prefetch[3 ]) << +L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT ) | + ((prefetch[5 ]) << +L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT ) | + ((prefetch[6 ]) << +L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT ) | + ((prefetch[7 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT );You've not sanity checked these properties. What If I typo a value as something other than 0 or 1?
Use boolean properties and
of_property_read_bool(or even better, configure this dynamically based on revision rather than having a dt description).arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 624
Quoted 6 lines
arch/arm/mm/cache-l2x0.c +6
-0+if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P1 ) { + prefetch_val &=PREFETCH_R3P1_MASK ; + prefetch_val |= + ((prefetch[4 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT ); + }Another undocumented revision dependency.
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 633
Quoted 9 lines
arch/arm/mm/cache-l2x0.c +9
-0+writel_relaxed (prefetch_val, + l2x0_base +L2X0_PREFETCH_CTRL ); + +of_property_read_u32_array (np,"arm,pwr-control" , + pwr,ARRAY_SIZE (pwr)); +writel_relaxed ( + ((pwr[0 ]) <<L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT ) | + ((pwr[1 ]) <<L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT ), + l2x0_base +L2X0_POWER_CTRL );Again, use boolean properties or do this dynamically.
Do none of these values need to be saved for the resume callback?
Thanks,
Mark.Quoted 14 lines
arch/arm/mm/cache-l2x0.c +2
-0+ } + } }static void __init pl310_save (void )Signature · 7 lines
1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 arm: l2x0: add support for prefetch and pwr control register configuration
Quoted 47 lines · Rob Herring
Quoted 3 lines · Mark Rutland
Hi,
[adding Rob Herring to Cc]
Thanks.
Quoted 11 lines · Chander Kashyap
Quoted 10 lines
This patch adds support to configure prefetch and pwr control registers of pl310 cache controllor.
Signed-off-by Chander Kashyap <[email protected]>--- Documentation/devicetree/bindings/arm/l2cc.txt | 6 ++++ arch/arm/include/asm/hardware/cache-l2x0.h | 23 +++++++++++++ arch/arm/mm/cache-l2x0.c | 44 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+)Quoted 14 lines
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 40
Quoted 12 lines
Documentation/devicetree/bindings/arm/l2cc.txt +4
-0@@ -34,6 +34,12 @@ 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-control : Prefetch related configurations. Specifies 8 cells of + prefetch-offset, not-same-ID-on-exclusive-sequence-en, incr-double-Linefill-en, + prefetch-drop-en, data-prefetch-en, instruction-prefetch-en and + double-linefill-en.This looks like configuration rather than hardware specification.
I think weigh whether this is configuration based on the h/w or just user preference. This would be the former, but...
Quoted 6 lines
Is there any reason we couldn't have Linux configure this itself? I'm not sure having a fixed configuration of these values makes sense - the configuration values could interact badly with future changes to Linux (e.g. performance improvements or errata workarounds), and then we'd have to ignore the described values and/or decide on more suitable ones dynamically anyway.
Does having a variable configuration for these make sense either? I mean other than an errata work-around, why would you ever turn off prefetch or double linefill?
Quoted 6 lines
None of these fields can be set in NS world. We've already laid down the law in regards to setting secure only bits for errata work-arounds. I think this is no different and we should move towards the kernel doesn't touch aux ctrl at all. There might be an exception with the event bus enable, but you would still have the secure vs. non-secure issue and no way to detect that.
So what is the suggestion here. Not to touch the prefetch register at all. But prefetch register values are saved and resumed in existing code, how to take care for that.
Quoted 157 lines
Rob
Quoted 153 lines
I assume that these cells are meant to either be zero or one? If they're necessary, they should be separate boolean properties rather than grouped together by the register they're configured by (see the way arm,tag-latency and arm,data-latency are handled).
Also, I'd prefer "-enable" to "-en".
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 41
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+- arm,pwr-control : Operting mode clock and power mode selection. Specifies twos/Operting/Operating/
I'd prefer "power" to "pwr" (though there's precedent for both in bindings...).
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 42
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+ cells of standby-mode-en and dynamic_clk_gating_en.More boolean properties.
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 45
Quoted 3 lines
Documentation/devicetree/bindings/arm/l2cc.txt +0
-0- interrupts : 1 combined interrupt. - cache-id-part: cache id part number to be used if it is not present on hardwareNo amendments to the example?
Quoted 51 lines
arch/arm/include/asm/hardware/cache-l2x0.h +23
-0@@ -106,6 +106,29 @@#define L2X0_WAY_SIZE_SHIFT 3 +#define L2X0_PREFETCH_CTRL_OFFSET_SHIFT 0 +#define L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT 21 +#define L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT 23 +#define L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT 24 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT 27 +#define L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT 28 +#define L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT 29 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT 30 + +#define PREFETCH_R2P0_MASK (~(1 << L2X0_PREFETCH_CTRL_OFFSET_SHIFT)) +#define PREFETCH_R3P0_MASK \ + (~((1 << L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT))) +#define PREFETCH_R3P1_MASK \ + (~(1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT)) + +#define L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT 0 +#define L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT 1 +#ifndef __ASSEMBLY__extern void __init l2x0_init (void __iomem *base,u32 aux_val,u32 aux_mask);#if defined(CONFIG_CACHE_L2X0 ) && defined(CONFIG_OF )arch/arm/mm/cache-l2x0.c +5
-0@@ -563,6 +563,11 @@ 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 prefetch[8 ] = {0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 }; +u32 prefetch_val; +u32 pwr[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 ])arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 598
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +2
-0@@ -589,6 +594,45 @@ 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 ) {You didn't describe that some properties were dependent on a particular revision.
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+of_property_read_u32_array (np,"arm,prefetch-control" , + prefetch,ARRAY_SIZE (prefetch)); + prefetch_val =readl_relaxed (l2x0_base +L2X0_PREFETCH_CTRL ); + prefetch_val &=PREFETCH_R2P0_MASK ; + prefetch_val |= prefetch[0 ] <<L2X0_PREFETCH_CTRL_OFFSET_SHIFT ; +if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P0 ) { + prefetch_val &=PREFETCH_R3P0_MASK ; + prefetch_val |= + ((prefetch[1 ]) << + L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) |arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 618
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+ ((prefetch[2 ]) << +L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT ) | + ((prefetch[3 ]) << +L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT ) | + ((prefetch[5 ]) << +L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT ) | + ((prefetch[6 ]) << +L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT ) | + ((prefetch[7 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT );You've not sanity checked these properties. What If I typo a value as something other than 0 or 1?
Use boolean properties and
of_property_read_bool(or even better, configure this dynamically based on revision rather than having a dt description).arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 624
Quoted 6 lines
arch/arm/mm/cache-l2x0.c +6
-0+if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P1 ) { + prefetch_val &=PREFETCH_R3P1_MASK ; + prefetch_val |= + ((prefetch[4 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT ); + }Another undocumented revision dependency.
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 633
Quoted 9 lines
arch/arm/mm/cache-l2x0.c +9
-0+writel_relaxed (prefetch_val, + l2x0_base +L2X0_PREFETCH_CTRL ); + +of_property_read_u32_array (np,"arm,pwr-control" , + pwr,ARRAY_SIZE (pwr)); +writel_relaxed ( + ((pwr[0 ]) <<L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT ) | + ((pwr[1 ]) <<L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT ), + l2x0_base +L2X0_POWER_CTRL );Again, use boolean properties or do this dynamically.
Do none of these values need to be saved for the resume callback?
Thanks,
Mark.Quoted 14 lines
arch/arm/mm/cache-l2x0.c +2
-1+ } + } }static void __init pl310_save (void ) --1.7.9.5
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
thanks for reviewing.
Signature · 2 lines
with warm regards, Chander Kashyap
PATCH 1/1 arm: l2x0: add support for prefetch and pwr control register configuration
Quoted 55 lines · Chander Kashyap
Quoted 47 lines · Rob Herring
Quoted 3 lines · Mark Rutland
Hi,
[adding Rob Herring to Cc]
Thanks.
Quoted 11 lines · Chander Kashyap
Quoted 10 lines
This patch adds support to configure prefetch and pwr control registers of pl310 cache controllor.
Signed-off-by Chander Kashyap <[email protected]>--- Documentation/devicetree/bindings/arm/l2cc.txt | 6 ++++ arch/arm/include/asm/hardware/cache-l2x0.h | 23 +++++++++++++ arch/arm/mm/cache-l2x0.c | 44 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+)Quoted 14 lines
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 40
Quoted 12 lines
Documentation/devicetree/bindings/arm/l2cc.txt +4
-0@@ -34,6 +34,12 @@ 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-control : Prefetch related configurations. Specifies 8 cells of + prefetch-offset, not-same-ID-on-exclusive-sequence-en, incr-double-Linefill-en, + prefetch-drop-en, data-prefetch-en, instruction-prefetch-en and + double-linefill-en.This looks like configuration rather than hardware specification.
I think weigh whether this is configuration based on the h/w or just user preference. This would be the former, but...
Quoted 6 lines
Is there any reason we couldn't have Linux configure this itself? I'm not sure having a fixed configuration of these values makes sense - the configuration values could interact badly with future changes to Linux (e.g. performance improvements or errata workarounds), and then we'd have to ignore the described values and/or decide on more suitable ones dynamically anyway.
Does having a variable configuration for these make sense either? I mean other than an errata work-around, why would you ever turn off prefetch or double linefill?
Quoted 6 lines
None of these fields can be set in NS world. We've already laid down the law in regards to setting secure only bits for errata work-arounds. I think this is no different and we should move towards the kernel doesn't touch aux ctrl at all. There might be an exception with the event bus enable, but you would still have the secure vs. non-secure issue and no way to detect that.
Quoted 3 lines
So what is the suggestion here. Not to touch the prefetch register at all. But prefetch register values are saved and resumed in existing code, how to take care for that.
As I said, I think all aux ctrl setup should be in the bootloader and out of the kernel.
If you need to do platform specific things like save and restore then fine, but let's not encourage it since it can't work universally.
Rob
PATCH 1/1 arm: l2x0: add support for prefetch and pwr control register configuration
In reply to Rob HerringQuoted 43 lines · Rob Herring
Quoted 3 lines · Mark Rutland
Hi,
[adding Rob Herring to Cc]
Thanks.
Quoted 11 lines · Chander Kashyap
Quoted 10 lines
This patch adds support to configure prefetch and pwr control registers of pl310 cache controllor.
Signed-off-by Chander Kashyap <[email protected]>--- Documentation/devicetree/bindings/arm/l2cc.txt | 6 ++++ arch/arm/include/asm/hardware/cache-l2x0.h | 23 +++++++++++++ arch/arm/mm/cache-l2x0.c | 44 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+)Quoted 14 lines
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 40
Quoted 12 lines
Documentation/devicetree/bindings/arm/l2cc.txt +4
-0@@ -34,6 +34,12 @@ 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-control : Prefetch related configurations. Specifies 8 cells of + prefetch-offset, not-same-ID-on-exclusive-sequence-en, incr-double-Linefill-en, + prefetch-drop-en, data-prefetch-en, instruction-prefetch-en and + double-linefill-en.This looks like configuration rather than hardware specification.
I think weigh whether this is configuration based on the h/w or just user preference. This would be the former, but...
Quoted 6 lines
Is there any reason we couldn't have Linux configure this itself? I'm not sure having a fixed configuration of these values makes sense - the configuration values could interact badly with future changes to Linux (e.g. performance improvements or errata workarounds), and then we'd have to ignore the described values and/or decide on more suitable ones dynamically anyway.
Quoted 3 lines
Does having a variable configuration for these make sense either? I mean other than an errata work-around, why would you ever turn off prefetch or double linefill?
I guess not.
Quoted 7 lines
None of these fields can be set in NS world. We've already laid down the law in regards to setting secure only bits for errata work-arounds. I think this is no different and we should move towards the kernel doesn't touch aux ctrl at all. There might be an exception with the event bus enable, but you would still have the secure vs. non-secure issue and no way to detect that.
I would certainly be much happier if we didn't have to touch aux ctrl at all in the kernel.
Thanks,
Mark.
Quoted 158 lines
Rob
Quoted 153 lines
I assume that these cells are meant to either be zero or one? If they're necessary, they should be separate boolean properties rather than grouped together by the register they're configured by (see the way arm,tag-latency and arm,data-latency are handled).
Also, I'd prefer "-enable" to "-en".
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 41
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+- arm,pwr-control : Operting mode clock and power mode selection. Specifies twos/Operting/Operating/
I'd prefer "power" to "pwr" (though there's precedent for both in bindings...).
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 42
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+ cells of standby-mode-en and dynamic_clk_gating_en.More boolean properties.
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 45
Quoted 3 lines
Documentation/devicetree/bindings/arm/l2cc.txt +0
-0- interrupts : 1 combined interrupt. - cache-id-part: cache id part number to be used if it is not present on hardwareNo amendments to the example?
Quoted 51 lines
arch/arm/include/asm/hardware/cache-l2x0.h +23
-0@@ -106,6 +106,29 @@#define L2X0_WAY_SIZE_SHIFT 3 +#define L2X0_PREFETCH_CTRL_OFFSET_SHIFT 0 +#define L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT 21 +#define L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT 23 +#define L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT 24 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT 27 +#define L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT 28 +#define L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT 29 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT 30 + +#define PREFETCH_R2P0_MASK (~(1 << L2X0_PREFETCH_CTRL_OFFSET_SHIFT)) +#define PREFETCH_R3P0_MASK \ + (~((1 << L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT))) +#define PREFETCH_R3P1_MASK \ + (~(1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT)) + +#define L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT 0 +#define L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT 1 +#ifndef __ASSEMBLY__extern void __init l2x0_init (void __iomem *base,u32 aux_val,u32 aux_mask);#if defined(CONFIG_CACHE_L2X0 ) && defined(CONFIG_OF )arch/arm/mm/cache-l2x0.c +5
-0@@ -563,6 +563,11 @@ 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 prefetch[8 ] = {0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 }; +u32 prefetch_val; +u32 pwr[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 ])arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 598
Quoted 5 lines
arch/arm/mm/cache-l2x0.c +2
-0@@ -589,6 +594,45 @@ 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 ) {You didn't describe that some properties were dependent on a particular revision.
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+of_property_read_u32_array (np,"arm,prefetch-control" , + prefetch,ARRAY_SIZE (prefetch)); + prefetch_val =readl_relaxed (l2x0_base +L2X0_PREFETCH_CTRL ); + prefetch_val &=PREFETCH_R2P0_MASK ; + prefetch_val |= prefetch[0 ] <<L2X0_PREFETCH_CTRL_OFFSET_SHIFT ; +if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P0 ) { + prefetch_val &=PREFETCH_R3P0_MASK ; + prefetch_val |= + ((prefetch[1 ]) << + L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) |arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 618
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+ ((prefetch[2 ]) << +L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT ) | + ((prefetch[3 ]) << +L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT ) | + ((prefetch[5 ]) << +L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT ) | + ((prefetch[6 ]) << +L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT ) | + ((prefetch[7 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT );You've not sanity checked these properties. What If I typo a value as something other than 0 or 1?
Use boolean properties and
of_property_read_bool(or even better, configure this dynamically based on revision rather than having a dt description).arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 624
Quoted 6 lines
arch/arm/mm/cache-l2x0.c +6
-0+if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P1 ) { + prefetch_val &=PREFETCH_R3P1_MASK ; + prefetch_val |= + ((prefetch[4 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT ); + }Another undocumented revision dependency.
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 633
Quoted 9 lines
arch/arm/mm/cache-l2x0.c +9
-0+writel_relaxed (prefetch_val, + l2x0_base +L2X0_PREFETCH_CTRL ); + +of_property_read_u32_array (np,"arm,pwr-control" , + pwr,ARRAY_SIZE (pwr)); +writel_relaxed ( + ((pwr[0 ]) <<L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT ) | + ((pwr[1 ]) <<L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT ), + l2x0_base +L2X0_POWER_CTRL );Again, use boolean properties or do this dynamically.
Do none of these values need to be saved for the resume callback?
Thanks,
Mark.Quoted 14 lines
arch/arm/mm/cache-l2x0.c +2
-0+ } + } }static void __init pl310_save (void )Signature · 7 lines
1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 arm: l2x0: add support for prefetch and pwr control register configuration
In reply to Mark RutlandQuoted 37 lines · Mark Rutland
Hi,
[adding Rob Herring to Cc]
Quoted 15 lines · Chander Kashyap
This patch adds support to configure prefetch and pwr control registers of pl310 cache controllor.
Signed-off-by Chander Kashyap <[email protected]>--- Documentation/devicetree/bindings/arm/l2cc.txt | 6 ++++ arch/arm/include/asm/hardware/cache-l2x0.h | 23 +++++++++++++ arch/arm/mm/cache-l2x0.c | 44 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+)Documentation/devicetree/bindings/arm/l2cc.txt +0
-0Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 40
Quoted 7 lines
Documentation/devicetree/bindings/arm/l2cc.txt +4
-0@@ -34,6 +34,12 @@ 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-control : Prefetch related configurations. Specifies 8 cells of + prefetch-offset, not-same-ID-on-exclusive-sequence-en, incr-double-Linefill-en, + prefetch-drop-en, data-prefetch-en, instruction-prefetch-en and + double-linefill-en.This looks like configuration rather than hardware specification.
Is there any reason we couldn't have Linux configure this itself? I'm not sure having a fixed configuration of these values makes sense - the configuration values could interact badly with future changes to Linux (e.g. performance improvements or errata workarounds), and then we'd have to ignore the described values and/or decide on more suitable ones dynamically anyway.
Quoted 5 lines
I assume that these cells are meant to either be zero or one? If they're necessary, they should be separate boolean properties rather than grouped together by the register they're configured by (see the way arm,tag-latency and arm,data-latency are handled).
prefetch-offset is not boolean. Its configurable and can have different values. I am extracting the values in same way data-latency and tag-latency values are handled. Only extra thing i am doing is clubbing into single variable before writing in order to preserve the reserved values.
Quoted 1 line
Also, I'd prefer "-enable" to "-en".
ok
Quoted 4 lines
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 41
Quoted 2 lines
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+- arm,pwr-control : Operting mode clock and power mode selection. Specifies twos/Operting/Operating/
Ok
Quoted 6 lines
I'd prefer "power" to "pwr" (though there's precedent for both in bindings...).
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 42
Quoted 1 line
Documentation/devicetree/bindings/arm/l2cc.txt +1
-0+ cells of standby-mode-en and dynamic_clk_gating_en.
Quoted 7 lines
More boolean properties.
Documentation/devicetree/bindings/arm/l2cc.txt Optional properties: · line 45
Quoted 3 lines
Documentation/devicetree/bindings/arm/l2cc.txt +0
-0- interrupts : 1 combined interrupt. - cache-id-part: cache id part number to be used if it is not present on hardwareNo amendments to the example?
thanks for pointing. I will add the same.
Quoted 49 lines
Quoted 49 lines
arch/arm/include/asm/hardware/cache-l2x0.h +23
-0@@ -106,6 +106,29 @@#define L2X0_WAY_SIZE_SHIFT 3 +#define L2X0_PREFETCH_CTRL_OFFSET_SHIFT 0 +#define L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT 21 +#define L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT 23 +#define L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT 24 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT 27 +#define L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT 28 +#define L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT 29 +#define L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT 30 + +#define PREFETCH_R2P0_MASK (~(1 << L2X0_PREFETCH_CTRL_OFFSET_SHIFT)) +#define PREFETCH_R3P0_MASK \ + (~((1 << L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT) | \ + (1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT))) +#define PREFETCH_R3P1_MASK \ + (~(1 << L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT)) + +#define L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT 0 +#define L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT 1 +#ifndef __ASSEMBLY__extern void __init l2x0_init (void __iomem *base,u32 aux_val,u32 aux_mask);#if defined(CONFIG_CACHE_L2X0 ) && defined(CONFIG_OF )arch/arm/mm/cache-l2x0.c +5
-0@@ -563,6 +563,11 @@ 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 prefetch[8 ] = {0 ,0 ,0 ,0 ,0 ,0 ,0 ,0 }; +u32 prefetch_val; +u32 pwr[2 ] = {0 ,0 }; +u32 l2x0_revision =readl_relaxed (l2x0_base +L2X0_CACHE_ID ) & +L2X0_CACHE_ID_RTL_MASK ;
Quoted 11 lines
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 598
Quoted 8 lines
arch/arm/mm/cache-l2x0.c +2
-0of_property_read_u32_array (np,"arm,tag-latency" , tag,ARRAY_SIZE (tag));if (tag[0 ] && tag[1 ] && tag[2 ])@@ -589,6 +594,45 @@ 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 ) {You didn't describe that some properties were dependent on a particular revision.
I will add the description.
Quoted 22 lines
Quoted 11 lines
arch/arm/mm/cache-l2x0.c +10
-0+of_property_read_u32_array (np,"arm,prefetch-control" , + prefetch,ARRAY_SIZE (prefetch)); + prefetch_val =readl_relaxed (l2x0_base +L2X0_PREFETCH_CTRL ); + prefetch_val &=PREFETCH_R2P0_MASK ; + prefetch_val |= prefetch[0 ] <<L2X0_PREFETCH_CTRL_OFFSET_SHIFT ; +if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P0 ) { + prefetch_val &=PREFETCH_R3P0_MASK ; + prefetch_val |= + ((prefetch[1 ]) << + L2X0_PREFETCH_CTRL_NOT_SAME_ID_SHIFT) |arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 618
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +10
-0+ ((prefetch[2 ]) << +L2X0_PREFETCH_CTRL_INC_DOUBLE_LINEFILL_SHIFT ) | + ((prefetch[3 ]) << +L2X0_PREFETCH_CTRL_PREFETCH_DROP_SHIFT ) | + ((prefetch[5 ]) << +L2X0_PREFETCH_CTRL_DATA_PREFETCH_EN_SHIFT ) | + ((prefetch[6 ]) << +L2X0_PREFETCH_CTRL_INSTR_PREFETCH_EN_SHIFT ) | + ((prefetch[7 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_EN_SHIFT );
Quoted 2 lines
You've not sanity checked these properties. What If I typo a value as something other than 0 or 1?
I was simply extending the driver, by keeping in mind the existing convention.
Quoted 4 lines
Use boolean properties and
of_property_read_bool(or even better, configure this dynamically based on revision rather than having a dt description).
As prefetch-offset is not boolean, hence gathered in array.
Quoted 9 lines
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 624
Quoted 7 lines
arch/arm/mm/cache-l2x0.c +6
-0+if (l2x0_revision >=L2X0_CACHE_ID_RTL_R3P1 ) { + prefetch_val &=PREFETCH_R3P1_MASK ; + prefetch_val |= + ((prefetch[4 ]) << +L2X0_PREFETCH_CTRL_DOUBLE_LINEFILL_WR_SHIFT ); + }Another undocumented revision dependency.
Will add documentation.
Quoted 11 lines
arch/arm/mm/cache-l2x0.c static void __init pl310_of_setup(const struct device_node *np, · line 633
Quoted 10 lines
arch/arm/mm/cache-l2x0.c +9
-0+writel_relaxed (prefetch_val, + l2x0_base +L2X0_PREFETCH_CTRL ); + +of_property_read_u32_array (np,"arm,pwr-control" , + pwr,ARRAY_SIZE (pwr)); +writel_relaxed ( + ((pwr[0 ]) <<L2X0_PWR_CTRL_STANDBY_MODE_EN_SHIFT ) | + ((pwr[1 ]) <<L2X0_PWR_CTRL_DYNAMIC_CLK_GATE_EN_SHIFT ), + l2x0_base +L2X0_POWER_CTRL );
Quoted 3 lines
Again, use boolean properties or do this dynamically.
Do none of these values need to be saved for the resume callback?
They are already getting saved in "pl310_save" function and resumed in pl310_resume function
Quoted 17 lines
Thanks,
Mark.Quoted 14 lines
arch/arm/mm/cache-l2x0.c +2
-1+ } + } }static void __init pl310_save (void ) --1.7.9.5
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Thanks for reviewing it.
Signature · 2 lines
with warm regards, Chander Kashyap