Messages in this thread | | | From | Wenhu Wang <> | Subject | Re: [PATCH 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver implementation | Date | Tue, 14 Jun 2022 07:53:46 +0000 |
| |
>> >> + >> >> +struct mpc85xx_l2ctlr { >> >> + u32 ctl; /* 0x000 - L2 control */ >> > >> >What is the endian of these u32 values? You map them directly to >> >memory, so they must be specified some way, right? Please make it >> >obvious what they are. >> > >> >> Surely, the values should be u32 here, modified in v2 >> The controller info could be found in >> "QorIQ™ P2020 Integrated Processor Reference Manual" >> "Chapter 6 L2 Look-Aside Cache/SRAM" >> See: http://m4udit.dinauz.org/P2020RM_rev0.pdf > >That's not the answer to my question :) > >These are big-endian, right? Please mark them as such and access them >properly with the correct functions.
Yes, they are big-edian. Does it work to add comments(about order and access functions) for the structure ahead of it? And appending like "_be", "_access_be" or "_big_endian"? (struct mpc85xx_l2ctlr_be {……};
>Tabs in Linux are always 8 spaces wide. >
I will re-confirm the v2 of identation.
>> >> I looked at that patch. >> >> I don't think you can just drop the #ifdef in function >> __access_remote_vm() in mm/memory.c >> >> You have to replace it with something like: >> >> if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) >> break; >> > > >Another thing in that patch: > >By making generic_access_phys() a static inline, it means that everytime >you refer to the address of that function in a vm_operations_struct >struct, the compiler has to provide an outlined instance of the >function. It means you'll likely have several instances of a >generic_access_phys(). > >What you could do instead is to add the following at the start of >generic_access_phys() in mm/memory.c : > > if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) > return 0; >
It is really a better chmoce, thanks for the advice. Multiple instances exist as you mentioned, the block returns 0 with no-op instance which makes no difference with the function return value.
I will update the patch after a re-confirming.
Thanks, Wenhu | |