Hi Stephen, On 18 October 2016 at 13:10, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 10/18/2016 01:03 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 18 October 2016 at 12:58, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> >>> On 10/18/2016 10:23 AM, Simon Glass wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On 17 October 2016 at 15:35, Stephen Warren <swar...@wwwdotorg.org> >>>> wrote: >>>>> >>>>> >>>>> From: Stephen Warren <swar...@nvidia.com> >>>>> >>>>> SoC-specific logic may be required for all forms of cache-wide >>>>> operations; invalidate and flush of both dcache and icache (note that >>>>> only 3 of the 4 possible combinations make sense, since the icache >>>>> never >>>>> contains dirty lines). This patch adds an optional hook for all >>>>> implemented cache-wide operations, and renames the one existing hook to >>>>> better represent exactly which operation it is implementing. A dummy >>>>> no-op implementation of each hook is provided. These dummy >>>>> implementations are moved into C code, since there's no need to >>>>> implement them in assembly. >>>>> >>>>> Signed-off-by: Stephen Warren <swar...@nvidia.com> >>>>> --- >>>>> arch/arm/cpu/armv8/cache.S | 6 ------ >>>>> arch/arm/cpu/armv8/cache_v8.c | 23 >>>>> ++++++++++++++++++++--- >>>>> arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S | 4 ++-- >>>>> arch/arm/include/asm/system.h | 5 ++++- >>>>> arch/arm/mach-tegra/tegra186/cache.c | 2 +- >>>>> 5 files changed, 27 insertions(+), 13 deletions(-) >>>>> >>>> >>>> I think we should have a proper interface for this stuff rather than >>>> weak functions. Maybe we need a linker-list approach, or a cache >>>> uclass? >>> >>> >>> >>> What's improper about this interface? Presumably we could argue that no >>> function in the entire of U-Boot be called by symbol name, which doesn't >>> seem useful. >>> >>> I'm not sure exactly what you envisage by a linker-list approach. Can you >>> provide some background? I understand how the linker can construct list >>> of >>> objects/implementations/..., but that doesn't seem useful here since >>> there's >>> a known-ahead-of-time single implementation of these functions in a >>> single >>> build of U-Boot. >> >> >> Your own commit messages says that this is SoC-specific. I'm >> suggesting that we define an interface (which I think you have already >> done with your header file additions), and allow SoCs to implement it >> via a linker list. >> >> IMO the cache code in U-Boot is starting to get a bit creaky. >> >>> A cache uclass seems like /massive/ overkill, especially since I'd expect >>> these very low-level functions to be required well before any higher >>> level >>> code like DM/classes/... to be available. >> >> >> DM is available very early. But it's not clear from your patch when >> this code is actually called. > > > I believe that weak functions are a perfectly acceptable approach here. > > Yes, the implementation of these functions is SoC-specific. The Makefiles > will pull in the appropriate implementation for that SoC whenever U-Boot is > built, just like every other board- or SoC-specific function in the entire > of U-Boot. > > There's no need for linker lists since there is only ever one > implementation.
If there is only ever one implementation, why do you need weak functions? Just call them directly. I think in fact you mean that there can be no implementation (but perhaps an empty one), or one implementation. You are effectively using multiple weak functions to provide default code. I think it would be better if this were explicit. I still think that the cache functions could do with a rethink. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
