| Message ID | 20231128235700.599584-1-ninad@linux.ibm.com |
|---|---|
| Headers | show |
| Series | Introduce model for IBM's FSI | expand |
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 >
Hello Cedric, >> 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/. > Moved it to hw/misc directory > >> 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. Moved scratchpad to lbus files. > > >> 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 Moved fsi-slave to fsi 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. Moved opb to aspeed-apb2opb files. > >> 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. Reduced number of configs as you suggested. Thanks for the review. Regards, Ninad