On Mon, Nov 27, 2023 at 09:08:11PM +0000, Jiaxun Yang wrote: > > > 在2023年11月27日十一月 下午4:23，Serge Semin写道： > [...] > >> I just made a quick grep in tree, and it seems like we don't have much > >> user of ioremap_cache (as well as ioremap_uc/wc) here so I think it is > >> a safe assumption. > > > > I wouldn't say there aren't much users. ioremap_wc() and it's > > devm-version is widely utilized in the GPU and network and some other > > subsystems. ioremap_cache() isn't widespread indeed. In anyway even a > > single user must be supported in safely calling the method if it's > > provided by the arch-code, otherwise the method could be considered as > > just a bogus stub to have the kernel successfully built. I bet you'll > > agree with that. But that's not the point in this case, > > > > A bit later you also noted: > > > > On Fri, Nov 24, 2023 at 10:34:49PM +0000, Jiaxun Yang wrote: > >> A nip, _page_cachable_default is set in cpu_cache_init() as well. We'd > >> better move it to cpu-probe.c, or give it a reasonable default value. > > > > Right. Thanks. To be honest I haven't noticed that before your > > message. _page_cachable_default is indeed initialized in the > > cpu_cache_init() method, several steps after it would be used in the > > framework of dmi_remap_early(). On the other hand ioremap_cache() is > > defined as ioremap_prot() with the _page_cachable_default variable > > passed. So my code will still correctly work unless > > _page_cachable_default is pre-initialized with something other than > > zero. On the other hand we can't easily change its default value > > because it will affect and likely break the r3k (CPU_R3000) and Octeon > > based platforms, because it's utilized to initialize the > > protection-map table. Of course we can fix the r3k_cache_init() and > > octeon_cache_init() methods too so they would get the > > _page_cachable_default variable back to zero, but it will also make > > things around it more complicated. > > > > Also note, moving the _page_cachable_default initialization to the > > earlier stages like cpu_probe() won't work better because the field > > value may get change for instance in the framework of the smp_setup() > > function (see cps_smp_setup()). > > > > So after all the considerations above this solution now looks even > > clumsier than before.( Any idea how to make it better? > > I think the best solution maybe just use CKSEG0 to setup map here. > > Btw I was thinking about 64 bit here, I thought for 64bit we would > just embedded prot into XKPHYS, however I quickly figure out > ioremap_cache was never implemented properly on 64-bit system, > so does ioremap_wc. > > > u64 base = (flags == _CACHE_UNCACHED ? IO_BASE : UNCAC_BASE); > > Which is always uncached mapping. Indeed. Thanks for pointing that out. In the last days several times I was looking at that line and for some reason UNCAC_BASE seemed as CAC_BASE to me.) Based on what both IO_BASE and UNCAC_BASE are defined as of the uncached region anyway, then it should be safe for any currently supported MIPS64 (including the Loongson's) to use ioremap() in place of dmi_early_remap(). So basically my current patch in the subject won't change the method semantics. Let's not to try to fix a problem which doesn't exist then, and keep the patch as is especially seeing that the alternatives might still cause some troubles. Will you be ok with that? -Serge(y) > > >> > [...] > > > > Note the memory might be clobbered even before dmi_setup() for > > instance by means of the early_memtest() method. In anyway it would be > > better if the system booloader would have already reserved the DMI > > memory (in DTB) or it would have been done by the platform-specific > > plat_mem_setup() method. > > Unfortunately, too many machines are shipped with those badly designed > firmware. We rely on dmi_setup code to scan and preserve dmi table from > random location in memory. > > > > >> The second is we may have some early quirks depends on DMI > >> information. > > > > Which quirks do you mean to be dependent in between the current > > dmi_setup() call place and the cpu_cache_init() method invocation? > > I think we don't have any for now. > > -- > - Jiaxun
