Thanks!
I know about the 'offset' parameter, but in this particular case I use these
structures as layouts only and don't 'switch' over them. So I decided to set
the offsets to 0 in order to simplify the code.
And extra thanks for highlighting the potential issue with memcpy() 😊. I'll fix
it in [v5] as well as the comments to first 10 patches.
Have a nice holidays!
Sergey Kambalin
Software Developer,
Auriga Inc.
________________________________
От: Peter Maydell <peter.mayd...@linaro.org>
Отправлено: 19 декабря 2023 г. 10:39:13
Кому: Kambalin, Sergey
Копия: Sergey Kambalin; qemu-...@nongnu.org; qemu-devel@nongnu.org
Тема: Re: [PATCH v4 00/45] Raspberry Pi 4B machine
On Tue, 19 Dec 2023 at 16:18, Kambalin, Sergey
<sergey.kamba...@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
