[PATCH V2 2/2] ARM: l2c: AM437x: Introduce support for cache filter programming

[Tomasz Figa]

2015-01-04 1:45 GMT+09:00 Nishanth Menon <nm at ti.com>:
> On 01/03/2015 10:16 AM, Tomasz Figa wrote:
>>
>> 2015-01-04 0:34 GMT+09:00 Nishanth Menon <nm at ti.com>:
>>>
>>> On 15:40-20150103, Tomasz Figa wrote:
>>>>
>>>> Hi Nishanth,
>>>>
>>>> 2015-01-03 2:43 GMT+09:00 Nishanth Menon <nm at ti.com>:
>>>>>
>>>>> AM437x generation of processors support programming the PL310 L2Cache
>>>>> controller's address filter start and end registers using a secure
>>>>> montior service.
>>>>
>>>>
>>>> typo: s/montior/monitor/
>>>>
>>>> [snip]
>>>
>>>
>>> Uggh.. yes indeed. I will post a v3 updating the comments. If the
>>> following is ok.
>>>>
>>>>
>>>>> +               base = omap4_get_l2cache_base();
>>>>> +               filter_start = (reg == L310_ADDR_FILTER_START) ? val :
>>>>> +                              readl_relaxed(base +
>>>>> L310_ADDR_FILTER_START);
>>>>> +               filter_end = (reg == L310_ADDR_FILTER_END) ? val :
>>>>> +                              readl_relaxed(base +
>>>>> L310_ADDR_FILTER_END);
>>>>> +               omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX,
>>>>> filter_start,
>>>>> +                           filter_end);
>>>>> +               return;
>>>>
>>>>
>>>> I don't have any significant comments about this patch in particular,
>>>> but just noticed that you need to do read-backs here (and the typo
>>>> thanks to the spell checker of my mailing app). Maybe you should
>>>> consider switching to the .configure() API I introduced in my series?
>>>> This would let you get rid of the hardcoded static mapping.
>>>
>>>
>>> Yeah, I have two choices there.. Either I provide the fundamental
>>> write function for the generic l2c code to use OR I provide a
>>> duplicate of resultant l2c_configure(aux write) + l2c310_configure.
>>>
>>> To allow for reuse of improvements or anything like errata
>>> implementations in the future, OMAP L2C implementation has chosen to
>>> provide the
>>> low level code and allow the higherlevel configure/write/whatever of the
>>> future to stay in arch/arm/mm/cache-l2x0.c. The write_sec operation is
>>> not too complicated enough to warrant a replication of l2c310_configure.
>>>
>>> So, I prefer the current implementation than providing a .configure
>>> handler for outer_cache.configure from SoC level.
>>>
>>> Let me know if anyone has a strong objection to this.
>>
>>
>> Well, what l2c310_configure() does after my series is just writing the
>> registers. If they cannot be written normally (without some tricks
>> such as reading back other registers) then IMHO a separate function
>> should be provided.
>>
>> This is becomes possible after patch 3/8 (ARM: l2c: Add interface to
>> ask hypervisor to configure L2C) and what is used on Exynos which also
>> updates multiple registers in single SMC calls. You can find an
>> example of use in patch 6/8 (ARM: EXYNOS: Add .write_sec outer cache
>> callback for L2C-310). What's even more interesting is that approaches
>> similar to the one currently used on OMAP had been NAKed, when
>> proposed for Exynos and this is why we have the solution proposed by
>> my patches.
>>
>> Note that .write_sec() callback is still used for L2X0_CTRL and
>> L2X0_DEBUG_CTRL registers, because there might be a need to write them
>> separately (e.g. to disable the controller and to perform debug
>> operations/workarounds when the controller is already enabled).
>
>
>
> we dont have a machine descriptor for configure instead we overide the
> logic(in you case after firmware load, in OMAP case, I need to figure out).
> my point being unlike the exynos configure code, OMAP code will look exactly
> like current pl310_configure-2 lines of code which really does not benefit
> anything.
>
>
> Thinking again, in fact, i'd rather drop this series than have to do a
> duplicated configure code(and force a resultant maintenance for the future)
> in OMAP code since none of OMAP4 or AM437x series need these patches.
> Interest for this series was non-mandatory, but just to be complete from SoC
> definition point of view.
>
> Let me know which way you guys want me to go.

Right, dropping this series would definitely solve the the read-back issue. :)

After all, if you don't need to override the latencies in the kernel
(i.e. have sane firmware, unlike certain Exynos boards ;)), then I
don't see a point of having this feature.

Best regards,
Tomasz