Differential D10218
Implement workaround for Armada 38X family HW issue between CPU and devices Authored by zbb on Mar 31 2017, 2:25 PM. Tags None Referenced Files
Details There is a hardware problem between Cortex-A9 CPUs and on-chip devices Submitted by: Zbigniew Bodek <zbb@semihalf.com>
Diff Detail
Event TimelineComment Actions Please tell me, do you really think that hellish "#ifdef SOC_xxx" style used in Marvell subdirectory is the right one? And, in this case, right solution for this problem is pretty easy, something like: Comment Actions IMO, such function will need quite good comment about when it's safe to use it and why it's safe only at that time. Comment Actions The SOC_XXX checks are correct when it's to enable code that will only be run on a single SoC, e.g. the platform code. Unfortunately the Marvell code uses it to change definitions and I would like to move away from that style.
Is the issue that PCIe expects a stronger memory model than ARM provides? It might be enough to add pmap_change_attr calls to the PCIe driver to remap the device memory with a stronger memory ordering leaving the other device memory with the weaker memory model. Comment Actions Yep, I agree. Thanks for clarification.
Do you have pointer to any documentation about this? Comment Actions In the ARMv8 ARM there is the recommendation to use the no early write acknowledgement (nE) attribute with PCIe memory. ARM also documents Device-nGnRnE as "Equivalent to the Strongly-ordered memory type in earlier versions of the architecture". This is the only memory type with the nE property, device memory ordering cannot be any stronger on ARM. Comment Actions @meloun-miracle-cz your solution presented here https://github.com/strejda/tegra/commit/3b5138751ee5643992b20fcb21b280fab433bb20 looks very good. Comment Actions Hi @meloun-miracle-cz Would it be possible that you commit change pointed by zbb?
Comment Actions Note that pmap_remap_vm_attr() calls pmap_set_tex() which assumes that all caches are disabled (see last two lines in this function). Caches are enabled in reinit_mmu() which is called from pmap_bootstrap_prepare() and init_secondary(). It's unfortunate that Michal did not add some comment about this to pmap_remap_vm_attr(). However, it's just a note for now as use of platform_cpu_init() is not clear so far. Comment Actions Yes. This must be called where it is being called now. Unfortunately platform_ code seems to be unusable for our case. BTW. What is unclear for you regarding the use of platform_cpu_init()? There was a need to modify the default, hard-coded settings of the mappings as well as CPUs auxiliary control register. Use of another platform callback was suggested instead of ugly ifdef or another hard-coded setting of ACTLR (that was undesirable on some Cortex revisions). That is why we created this function and we use it in the related patches (please see dependencies) to modify default CPU/MMU settings.
Comment Actions pmap_remap_vm_attr isn't very useful if we can only call it with the cache disabled. I would expect we don't need to handle the cache for remapping device memory so this should be safe, however am not as familiar with the 32 bit ARM code as arm64. Comment Actions Just wanted to say that pmap_remap_vm_attr() must be called before pmap_bootstrap_prepare(). And even if platform cpu_init() method is introduced in another place (D10682) where platform_cpu_init() is called before pmap_bootstrap_prepare(), it's used before platform code is initialized and it's wrong. So, it's unclear for now if this patch will satisfy what was noted by me. Comment Actions OK. Now I understand what you meant. Yes, I admit this is a mistake to use platform code so early. It will not work for the GENERIC kernel. Comment Actions @skra Why are we not able to pmap_set_tex() while caches are enabled? We are merely modifying two registers. TLB invalidation should be the only thing that is required to be done. Could you elaborate this constraint a little? It seems that this is the only thing that is holding us back from the acceptable solution so I would like to understand the issue. Comment Actions My note was about two aspects of pmap_remap_vm_attr(). (1) Generally, when cache is enabled, changing cache mode of memory in active use (i.e. any memory which is mapped (keep in mind speculative reads)) is a tricky thing if possible at all. It's a problem of atomicity of such action. It's also a thing of how cache itself and cache maintainance functions work. Imagine that a page is remapped from cached mode to uncached one. All cachelines associated with this page must be removed from cache. To do this while the page is mapped as cached is not enough because of speculative reads. To do this after the page is remapped will not work as the mapping is uncached and cache maintainance function may do nothing for uncached memory. Note that access to uncached memory may still go thru cache. Some cache modes may be change to others without problem, but it's not true for every combination of cache modes. Maybe, under very strict circumstances, it could be possible to change cache mode in some memory region. However, pmap_remap_vm_attr() works globally for every TEX class combination. It touches all memory mapped with TEXs being changed. So, my concern was to comment such function properly to specify when it's safe to use it. (2) Code consistency. Either some explanation should have been done why pmap_set_tex() can be used in pmap_remap_vm_attr() or comment in pmap_set_tex() should have been changed. Well, it looks that r318251 answered my note. Comment Actions Thanks, this is very informative. So we can proceed with our platform change (next round). | ||||||||||||||||||||||||||||||||||||||||