Hi Stephen, On 18 October 2016 at 17:33, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 10/18/2016 05:08 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 18 October 2016 at 16:54, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> >>> On 10/18/2016 01:56 PM, Simon Glass wrote: >>>> >>>> >>>> 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? >>> >>> >>> >>> As I explicitly stated above, each SoC can have a different >>> implementation, >>> yet only a single implementation is ever needed for a particular U-Boot >>> build. >>> >>>> Just call them directly. >>> >>> >>> >>> The code is doing that, both before and after my patch. >> >> >> I mean call them without needing weak functions. >> >>> >>>> 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. >>> >>> >>> >>> In my opinion, this patch doesn't change the code structure at all. There >>> is >>> already an interface between the core (L1/L2) cache management code and >>> the >>> SoC-specific cache management code. That interface already uses a weak >>> function to provide the default no-op implementation. This patch doesn't >>> change any of that. All this patch does is fix that existing interface to >>> cover all 3 combinations of dcache_flush, dcache_invalidate, and >>> icache_invalidate, rather than just one of those combinations. It's more >>> of >>> a bug-fix than anything else. >> >> >> Yes I see that. >> >>> >>> If you want to rework this interface sometime, be my guest. However, I >>> don't >>> think it's fair to require that someone who simply wants to fix the >>> existing >>> code (in a way that is orthogonal to your desired interface refactoring) >>> do >>> that refactoring first, rather than doing it yourself. >> >> >> I understand what you are saying, but isn't that how open source >> software works? Believe me, I have done my fair share of refactoring >> :-) >> >> At least can you look at not making it any harder to fix up later? The >> more we pile onto this interface, the harder it will be later. We >> should aim to make ARMv8 really nice as it is the new thing. > > > I believe moving one or three functions into any replacement scheme will > have an identical level of difficulty. So, I believe the patch already > satisfies the "no harder" requirement.
Well it seems your mind is already made up! Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
