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

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

[Kambalin, Sergey]

[-- 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 --]

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

[Sergey Kambalin]

Introducing Raspberry Pi 4B model.
It contains new BCM2838 SoC, PCIE subsystem,
RNG200, Thermal sensor and Genet network controller.

It can work with recent linux kernels 6.x.x.
Two avocado tests was added to check that.

Unit tests has been made as read/write operations
via mailbox properties.

Genet integration test is under development.

Every single commit
1) builds without errors
2) passes regression tests
3) passes style check*
*the only exception is bcm2838-mbox-property-test.c file
containing heavy macros usage which cause a lot of
false-positives of checkpatch.pl.

I did my best to keep the commits less than 200 changes,
but had to make some of them a bit more in order to
keep their integrity.

This is v2 patchset with the most of v1 remarks fixed.
I named it as 'v4' because of mistakes while attempts to send previous patchsets
Please ignore all other v1...v3 patchsets except the very first one.

Sergey Kambalin (45):
  Split out common part of BCM283X classes
  Split out common part of peripherals
  Split out raspi machine common part
  Introduce BCM2838 SoC
  Add GIC-400 to BCM2838 SoC
  Add BCM2838 GPIO stub
  Implement BCM2838 GPIO functionality
  Connect SD controller to BCM2838 GPIO
  Add GPIO and SD to BCM2838 periph
  Add BCM2838 checkpoint support
  Introduce Raspberry PI 4 machine
  Temporarily disable unimplemented rpi4b devices
  Add memory region for BCM2837 RPiVid ASB
  Add BCM2838 PCIE Root Complex
  Add BCM2838 PCIE host
  Enable BCM2838 PCIE
  Add RNG200 skeleton
  Add RNG200 RNG and RBG
  Get rid of RNG200 timer
  Implement BCM2838 thermal sensor
  Add clock_isp stub
  Add GENET stub
  Add GENET register structs. Part 1
  Add GENET register structs. Part 2
  Add GENET register structs. Part 3
  Add GENET register structs. Part 4
  Add GENET register access macros
  Implement GENET register ops
  Implement GENET MDIO
  Implement GENET TX path
  Implement GENET RX path
  Enable BCM2838 GENET controller
  Connect RNG200, PCIE and GENET to GIC
  Add Rpi4b boot tests
  Add mailbox test stub
  Add mailbox test constants
  Add mailbox tests tags. Part 1
  Add mailbox tests tags. Part 2
  Add mailbox tests tags. Part 3
  Add mailbox property tests. Part 1
  Add mailbox property tests. Part 2
  Add mailbox property tests. Part 3
  Add missed BCM2835 properties
  Append added properties to mailbox test
  Add RPi4B to paspi4.rst

 docs/system/arm/raspi.rst                |   11 +-
 hw/arm/bcm2835_peripherals.c             |  218 +++--
 hw/arm/bcm2836.c                         |  116 ++-
 hw/arm/bcm2838.c                         |  288 ++++++
 hw/arm/bcm2838_pcie.c                    |  289 ++++++
 hw/arm/bcm2838_peripherals.c             |  292 ++++++
 hw/arm/meson.build                       |    8 +-
 hw/arm/raspi.c                           |  131 +--
 hw/arm/raspi4b.c                         |  112 +++
 hw/arm/trace-events                      |    6 +
 hw/gpio/bcm2838_gpio.c                   |  389 ++++++++
 hw/gpio/meson.build                      |    5 +-
 hw/misc/bcm2835_property.c               |   47 +
 hw/misc/bcm2838_rng200.c                 |  420 +++++++++
 hw/misc/bcm2838_thermal.c                |   98 ++
 hw/misc/meson.build                      |    2 +
 hw/misc/trace-events                     |    9 +
 hw/net/bcm2838_genet.c                   | 1088 ++++++++++++++++++++++
 hw/net/meson.build                       |    2 +
 hw/net/trace-events                      |   16 +
 include/hw/arm/bcm2835_peripherals.h     |   29 +-
 include/hw/arm/bcm2836.h                 |   27 +-
 include/hw/arm/bcm2838.h                 |   31 +
 include/hw/arm/bcm2838_pcie.h            |   75 ++
 include/hw/arm/bcm2838_peripherals.h     |   97 ++
 include/hw/arm/raspberrypi-fw-defs.h     |   12 +-
 include/hw/arm/raspi_platform.h          |   37 +
 include/hw/display/bcm2835_fb.h          |    2 +
 include/hw/gpio/bcm2838_gpio.h           |   45 +
 include/hw/misc/bcm2838_rng200.h         |   43 +
 include/hw/misc/bcm2838_thermal.h        |   24 +
 include/hw/net/bcm2838_genet.h           |  426 +++++++++
 tests/avocado/boot_linux_console.py      |   92 ++
 tests/qtest/bcm2838-mailbox.c            |   61 ++
 tests/qtest/bcm2838-mailbox.h            |  584 ++++++++++++
 tests/qtest/bcm2838-mbox-property-test.c |  621 ++++++++++++
 tests/qtest/meson.build                  |    3 +-
 37 files changed, 5551 insertions(+), 205 deletions(-)
 create mode 100644 hw/arm/bcm2838.c
 create mode 100644 hw/arm/bcm2838_pcie.c
 create mode 100644 hw/arm/bcm2838_peripherals.c
 create mode 100644 hw/arm/raspi4b.c
 create mode 100644 hw/gpio/bcm2838_gpio.c
 create mode 100644 hw/misc/bcm2838_rng200.c
 create mode 100644 hw/misc/bcm2838_thermal.c
 create mode 100644 hw/net/bcm2838_genet.c
 create mode 100644 include/hw/arm/bcm2838.h
 create mode 100644 include/hw/arm/bcm2838_pcie.h
 create mode 100644 include/hw/arm/bcm2838_peripherals.h
 create mode 100644 include/hw/gpio/bcm2838_gpio.h
 create mode 100644 include/hw/misc/bcm2838_rng200.h
 create mode 100644 include/hw/misc/bcm2838_thermal.h
 create mode 100644 include/hw/net/bcm2838_genet.h
 create mode 100644 tests/qtest/bcm2838-mailbox.c
 create mode 100644 tests/qtest/bcm2838-mailbox.h
 create mode 100644 tests/qtest/bcm2838-mbox-property-test.c

-- 
2.34.1

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

[Philippe Mathieu-Daudé]

Hi Sergey,

On 8/12/23 03:31, Sergey Kambalin wrote:
> Introducing Raspberry Pi 4B model.
> It contains new BCM2838 SoC, PCIE subsystem,
> RNG200, Thermal sensor and Genet network controller.
> 
> It can work with recent linux kernels 6.x.x.
> Two avocado tests was added to check that.
> 
> Unit tests has been made as read/write operations
> via mailbox properties.
> 
> Genet integration test is under development.
> 
> Every single commit
> 1) builds without errors
> 2) passes regression tests
> 3) passes style check*
> *the only exception is bcm2838-mbox-property-test.c file
> containing heavy macros usage which cause a lot of
> false-positives of checkpatch.pl.
> 
> I did my best to keep the commits less than 200 changes,
> but had to make some of them a bit more in order to
> keep their integrity.
> 
> This is v2 patchset with the most of v1 remarks fixed.
> I named it as 'v4' because of mistakes while attempts to send previous patchsets
> Please ignore all other v1...v3 patchsets except the very first one.

I really want to review your series, but I have been busy and
won't have time to look at it the next 2 weeks :/

Thanks Peter for reviewing the previous version :)

> Sergey Kambalin (45):
>    Split out common part of BCM283X classes
>    Split out common part of peripherals
>    Split out raspi machine common part
>    Introduce BCM2838 SoC
>    Add GIC-400 to BCM2838 SoC
>    Add BCM2838 GPIO stub
>    Implement BCM2838 GPIO functionality
>    Connect SD controller to BCM2838 GPIO
>    Add GPIO and SD to BCM2838 periph
>    Add BCM2838 checkpoint support
>    Introduce Raspberry PI 4 machine
>    Temporarily disable unimplemented rpi4b devices
>    Add memory region for BCM2837 RPiVid ASB
>    Add BCM2838 PCIE Root Complex
>    Add BCM2838 PCIE host
>    Enable BCM2838 PCIE
>    Add RNG200 skeleton
>    Add RNG200 RNG and RBG
>    Get rid of RNG200 timer
>    Implement BCM2838 thermal sensor
>    Add clock_isp stub
>    Add GENET stub
>    Add GENET register structs. Part 1
>    Add GENET register structs. Part 2
>    Add GENET register structs. Part 3
>    Add GENET register structs. Part 4
>    Add GENET register access macros
>    Implement GENET register ops
>    Implement GENET MDIO
>    Implement GENET TX path
>    Implement GENET RX path
>    Enable BCM2838 GENET controller
>    Connect RNG200, PCIE and GENET to GIC
>    Add Rpi4b boot tests
>    Add mailbox test stub
>    Add mailbox test constants
>    Add mailbox tests tags. Part 1
>    Add mailbox tests tags. Part 2
>    Add mailbox tests tags. Part 3
>    Add mailbox property tests. Part 1
>    Add mailbox property tests. Part 2
>    Add mailbox property tests. Part 3
>    Add missed BCM2835 properties
>    Append added properties to mailbox test
>    Add RPi4B to paspi4.rst

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

[Peter Maydell]

On Fri, 8 Dec 2023 at 02:32, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Introducing Raspberry Pi 4B model.
> It contains new BCM2838 SoC, PCIE subsystem,
> RNG200, Thermal sensor and Genet network controller.
>
> It can work with recent linux kernels 6.x.x.
> Two avocado tests was added to check that.
>
> Unit tests has been made as read/write operations
> via mailbox properties.
>
> Genet integration test is under development.
>
> Every single commit
> 1) builds without errors
> 2) passes regression tests
> 3) passes style check*
> *the only exception is bcm2838-mbox-property-test.c file
> containing heavy macros usage which cause a lot of
> false-positives of checkpatch.pl.
>
> I did my best to keep the commits less than 200 changes,
> but had to make some of them a bit more in order to
> keep their integrity.
>
> This is v2 patchset with the most of v1 remarks fixed.
> I named it as 'v4' because of mistakes while attempts to send previous patchsets
> Please ignore all other v1...v3 patchsets except the very first one.

Thanks for the rework and resending these patches. As
you've probably seen, I've reviewed patches 1-10, but
I won't have time to do more than that before I break
for the holiday season now; I will get back to the
rest of the series in January.

-- PMM

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

[Kambalin, Sergey]

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

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.


Thanks,
Sergey Kambalin
Software Developer,
Auriga Inc.


________________________________
От: Peter Maydell <peter.maydell@linaro.org>
Отправлено: 19 декабря 2023 г. 10:11:43
Кому: Sergey Kambalin
Копия: qemu-arm@nongnu.org; qemu-devel@nongnu.org; Kambalin, Sergey
Тема: Re: [PATCH v4 00/45] Raspberry Pi 4B machine

On Fri, 8 Dec 2023 at 02:32, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Introducing Raspberry Pi 4B model.
> It contains new BCM2838 SoC, PCIE subsystem,
> RNG200, Thermal sensor and Genet network controller.
>
> It can work with recent linux kernels 6.x.x.
> Two avocado tests was added to check that.
>
> Unit tests has been made as read/write operations
> via mailbox properties.
>
> Genet integration test is under development.
>
> Every single commit
> 1) builds without errors
> 2) passes regression tests
> 3) passes style check*
> *the only exception is bcm2838-mbox-property-test.c file
> containing heavy macros usage which cause a lot of
> false-positives of checkpatch.pl.
>
> I did my best to keep the commits less than 200 changes,
> but had to make some of them a bit more in order to
> keep their integrity.
>
> This is v2 patchset with the most of v1 remarks fixed.
> I named it as 'v4' because of mistakes while attempts to send previous patchsets
> Please ignore all other v1...v3 patchsets except the very first one.

Thanks for the rework and resending these patches. As
you've probably seen, I've reviewed patches 1-10, but
I won't have time to do more than that before I break
for the holiday season now; I will get back to the
rest of the series in January.

-- PMM

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

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

[Peter Maydell]

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

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

[Kambalin, Sergey]

[-- 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: 4442 bytes --]

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

[Peter Maydell]

On Tue, 19 Dec 2023 at 16:11, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 8 Dec 2023 at 02:32, Sergey Kambalin <serg.oker@gmail.com> wrote:
> >
> > Introducing Raspberry Pi 4B model.
> > It contains new BCM2838 SoC, PCIE subsystem,
> > RNG200, Thermal sensor and Genet network controller.
> >
> > It can work with recent linux kernels 6.x.x.
> > Two avocado tests was added to check that.
> >
> > Unit tests has been made as read/write operations
> > via mailbox properties.
> >
> > Genet integration test is under development.
> >
> > Every single commit
> > 1) builds without errors
> > 2) passes regression tests
> > 3) passes style check*
> > *the only exception is bcm2838-mbox-property-test.c file
> > containing heavy macros usage which cause a lot of
> > false-positives of checkpatch.pl.
> >
> > I did my best to keep the commits less than 200 changes,
> > but had to make some of them a bit more in order to
> > keep their integrity.
> >
> > This is v2 patchset with the most of v1 remarks fixed.
> > I named it as 'v4' because of mistakes while attempts to send previous patchsets
> > Please ignore all other v1...v3 patchsets except the very first one.
>
> Thanks for the rework and resending these patches. As
> you've probably seen, I've reviewed patches 1-10, but
> I won't have time to do more than that before I break
> for the holiday season now; I will get back to the
> rest of the series in January.

I'm now done with my review of this v4. I haven't left
comments on every patch, but I have read through the others
(including the ethernet controller ones) and I didn't see
anything obviously wrong beyond what we've already
discussed in this thread. I'll do a more full review
for v5.

thanks
-- PMM