[PATCH] arm: l2x0: add support for prefetch and pwr control register configuration

Chander Kashyap chander.kashyap at linaro.org
Thu Jul 25 00:09:00 EDT 2013


On 23 July 2013 15:52, Mark Rutland <mark.rutland at arm.com> wrote:
> 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).
>

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.

> Also, I'd prefer "-enable" to "-en".
ok
>
>> +- arm,pwr-control : Operting mode clock and power mode selection. Specifies two
>
> s/Operting/Operating/

Ok

>
> 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?

thanks for pointing. I will add the same.

>
>> 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.

I will add the description.

>
>> +             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?

I was simply extending the driver, by keeping in mind the existing convention.

>
> 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.

>
>> +                     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.

>
>> +                     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?

They are already getting saved in "pl310_save" function and resumed in
pl310_resume function

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

Thanks for reviewing it.


-- 
with warm regards,
Chander Kashyap



More information about the linux-arm-kernel mailing list