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

[Nishanth Menon]

On 01/03/2015 10:16 AM, Tomasz Figa wrote:
> 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).


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.

---
Regards,
Nishanth Menon
--
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/