[PATCH] arm: l2x0: add support for prefetch and pwr control register configuration
Mark Rutland
mark.rutland at arm.com
Tue Jul 23 06:22:12 EDT 2013
Hi,
[adding Rob Herring to Cc]
On Tue, Jul 23, 2013 at 10:39:05AM +0100, Chander Kashyap wrote:
> This patch adds support to configure prefetch and pwr control registers
> of pl310 cache controllor.
>
> Signed-off-by: Chander Kashyap <chander.kashyap at linaro.org>
> ---
> 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(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
> index cbef09b..66876b6 100644
> --- a/Documentation/devicetree/bindings/arm/l2cc.txt
> +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
> @@ -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".
> +- 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...).
> + cells of standby-mode-en and dynamic_clk_gating_en.
More boolean properties.
> - 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?
> diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
> index 3b2c40b..188f4da 100644
> --- a/arch/arm/include/asm/hardware/cache-l2x0.h
> +++ b/arch/arm/include/asm/hardware/cache-l2x0.h
> @@ -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)
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index c465fac..490e182 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -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])
> @@ -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.
> + 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) |
> + ((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).
> + 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.
> + 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.
> + }
> + }
> }
>
> 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
>
More information about the linux-arm-kernel
mailing list