Re: [PATCH v8 00/10] Introduce model for IBM's FSI

[Cédric Le Goater]

Hello Ninad,

Here are comments on the file organization and configs.

On 11/29/23 00:56, Ninad Palsule wrote:
> Hello,
>
> Please review the patch-set version 8.
> I have incorporated review comments from Cedric.
>    - Fixed checkpatch failures.
>    - Fixed commit messages.
>    - Fixed LBUS memory map size.
>
> Ninad Palsule (10):
>    hw/fsi: Introduce IBM's Local bus
>    hw/fsi: Introduce IBM's FSI Bus
>    hw/fsi: Introduce IBM's cfam,fsi-slave,scratchpad
>    hw/fsi: IBM's On-chip Peripheral Bus
>    hw/fsi: Introduce IBM's FSI master
>    hw/fsi: Aspeed APB2OPB interface
>    hw/arm: Hook up FSI module in AST2600
>    hw/fsi: Added qtest
>    hw/fsi: Added FSI documentation
>    hw/fsi: Update MAINTAINER list
>
>   MAINTAINERS                     |   8 +
>   docs/specs/fsi.rst              | 138 ++++++++++++++
>   docs/specs/index.rst            |   1 +
>   meson.build                     |   1 +
>   hw/fsi/trace.h                  |   1 +
>   include/hw/arm/aspeed_soc.h     |   4 +
>   include/hw/fsi/aspeed-apb2opb.h |  34 ++++

aspeed-apb2opb is a HW logic bridging the FSI world and Aspeed. It
doesn't belong to the FSI susbsytem. Since we don't have a directory
for platform specific devices, I think the model shoud go under hw/misc/.


>   include/hw/fsi/cfam.h           |  45 +++++

scratchpad is the only lbus device and it is quite generic, we could
move it to lbus files. It would be nice to implement more than one
reg.

 
>   include/hw/fsi/fsi-master.h     |  32 ++++
>   include/hw/fsi/fsi-slave.h      |  29 +++
>   include/hw/fsi/fsi.h            |  24 +++

I would move the definitions and implementation of the fsi bus and
the fsi slave under the fsi.h and fsi.c files


>   include/hw/fsi/lbus.h           |  40 ++++
>   include/hw/fsi/opb.h            |  25 +++

opb is quite minimal now and I think it could be hidden under
aspeed-apb2opb.

>   hw/arm/aspeed_ast2600.c         |  19 ++
>   hw/fsi/aspeed-apb2opb.c         | 316 ++++++++++++++++++++++++++++++++
>   hw/fsi/cfam.c                   | 261 ++++++++++++++++++++++++++
>   hw/fsi/fsi-master.c             | 165 +++++++++++++++++
>   hw/fsi/fsi-slave.c              |  78 ++++++++
>   hw/fsi/fsi.c                    |  22 +++
>   hw/fsi/lbus.c                   |  51 ++++++
>   hw/fsi/opb.c                    |  36 ++++
>   tests/qtest/aspeed-fsi-test.c   | 205 +++++++++++++++++++++
>   hw/Kconfig                      |   1 +
>   hw/arm/Kconfig                  |   1 +
>   hw/fsi/Kconfig                  |  21 +++

one CONFIG_FSI option and one CONFIG_FSI_APB2OPB should be enough.
CONFIG_FSI_APB2OPB should select FSI and depends on CONFIG_ASPEED_SOC.


Thanks,

C.




>   hw/fsi/meson.build              |   5 +
>   hw/fsi/trace-events             |  13 ++
>   hw/meson.build                  |   1 +
>   tests/qtest/meson.build         |   1 +
>   29 files changed, 1578 insertions(+)
>   create mode 100644 docs/specs/fsi.rst
>   create mode 100644 hw/fsi/trace.h
>   create mode 100644 include/hw/fsi/aspeed-apb2opb.h
>   create mode 100644 include/hw/fsi/cfam.h
>   create mode 100644 include/hw/fsi/fsi-master.h
>   create mode 100644 include/hw/fsi/fsi-slave.h
>   create mode 100644 include/hw/fsi/fsi.h
>   create mode 100644 include/hw/fsi/lbus.h
>   create mode 100644 include/hw/fsi/opb.h
>   create mode 100644 hw/fsi/aspeed-apb2opb.c
>   create mode 100644 hw/fsi/cfam.c
>   create mode 100644 hw/fsi/fsi-master.c
>   create mode 100644 hw/fsi/fsi-slave.c
>   create mode 100644 hw/fsi/fsi.c
>   create mode 100644 hw/fsi/lbus.c
>   create mode 100644 hw/fsi/opb.c
>   create mode 100644 tests/qtest/aspeed-fsi-test.c
>   create mode 100644 hw/fsi/Kconfig
>   create mode 100644 hw/fsi/meson.build
>   create mode 100644 hw/fsi/trace-events