On Tue, 19 Dec 2023 at 16:18, Kambalin, Sergey
<sergey.kambalin@auriga.com> wrote:
>
> Thank you a lot for the review Peter!
>
>
> May I kindly ask you to take just a brief look at the first patches of GENET? I'd like to know if I've chosen the right way to replace bitfields with QEMU REG32/FIELD32 macros.
The FIELD and REG32 uses look mostly OK, but the
second argument to REG32() should not be 0 each time:
REG32(GENET_SYS_REV_CTRL, 0)
REG32(GENET_INTRL_0, 0)
etc. The idea is that that second argument is the offset
in the register file of the register; then the macro
defines you an A_GENET_SYS_REV_CTRL which is that value,
and you can use it as a case label in the "switch (offset) {"
in the read/write function.
I'm a also a bit confused about the use of offsetof() in patch 27.
In patch 28 the implementation of bcm2838_genet_read() and
bcm2838_genet_write() use a memcpy() between a local variable
and memory which I'm assuming is an index into one of these
register structs, which won't do the right thing if the host
is big-endian. If you need a "load/store N bytes to memory in
host order", we have ldn_he_p() and stn_he_p(); also available
in _le_ and _be_ flavours for load/store in specific endianness.
thanks
-- PMM