Re: [PATCH v4 00/45] Raspberry Pi 4B machine

["Kambalin, Sergey" <sergey.kambalin@auriga.com>]

[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]

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.maydell@linaro.org>
Отправлено: 19 декабря 2023 г. 10:39:13
Кому: Kambalin, Sergey
Копия: Sergey Kambalin; qemu-arm@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.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

[-- Attachment #2: Type: text/html, Size: 4594 bytes --]