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

[Tomasz Figa]

2015-01-04 0:34 GMT+09:00 Nishanth Menon <nm@xxxxxx>:
> On 15:40-20150103, Tomasz Figa wrote:
>> Hi Nishanth,
>>
>> 2015-01-03 2:43 GMT+09:00 Nishanth Menon <nm@xxxxxx>:
>> > 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).

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/