Tl,dr: We'll remove the IPMI changes from the current patch set and refactor
them in a separate patch set.
Thank you for your review! On high level, we are trying to emulate the BMC
side of the IPMI protocol. So we cannot directly use the existing IPMI code.
However, they do have a lot in duplication as you pointed out. So we'll refactor
the existing IPMI code and update in a way that we only add the required
functionality.
As for the KCS module, the BMC side of the protocol is the opposite direction
of the existing ipmi_kcs.c code which is on the host/CPU side. For example,
in READ_STATE the CPU would read data while the BMC would write data.
So we can't directly use the same implementation. (They're different files in
Linux either.) However, we can refactor it to re-use some of the common definitions.
We would like to remove the IPMI and KCS stuff from the current patch set.
We'll send the refactored code in a separate patch set after addressing
your concerns.
Thanks again for the review!
On Thu, Dec 10, 2020 at 05:51:49PM -0800, Hao Wu wrote:
> This patch series include a few more NPCM7XX devices including
>
> - Analog Digital Converter (ADC)
> - Pulse Width Modulation (PWM)
> - Keyboard Style Controller (KSC)
>
> To utilize these modules we also add two extra functionalities:
>
> 1. We modified the CLK module to generate clock values using qdev_clock.
> These clocks are used to determine various clocks in NPCM7XX devices.
> 2. We added support for emulating IPMI responder devices in BMC machines,
> similar to the existing IPMI device support for CPU emulation. This allows
> a qemu instance running BMC firmware to serve as an external BMC for a qemu
> instance running server software. It utilizes the KCS module we implemented.
Looking at the IPMI changes, why didn't you just re-use the existing
IPMI infrastructure? ipmi_host.[ch] is basically a subset of ipmi.[ch],
and the ipmi_host_extern looks like a copy of of ipmi_bmc_extern with
some names changed. That kind of code duplication is not acceptable.
Plus you copied my code and removed my copyrights, which is really
not acceptable and illegal.
I'm not exactly sure why you needed you own KCS interface, either. It
looks like the interface is somewhat different in some ways, but
integrating it into the current KCS code is probably a better choice if
that can be done.
-corey
>
> Hao Wu (7):
> hw/misc: Add clock converter in NPCM7XX CLK module
> hw/timer: Refactor NPCM7XX Timer to use CLK clock
> hw/adc: Add an ADC module for NPCM7XX
> hw/misc: Add a PWM module for NPCM7XX
> hw/ipmi: Add an IPMI host interface
> hw/ipmi: Add a KCS Module for NPCM7XX
> hw/ipmi: Add an IPMI external host device
>
> default-configs/devices/arm-softmmu.mak | 2 +
> docs/system/arm/nuvoton.rst | 6 +-
> hw/adc/meson.build | 1 +
> hw/adc/npcm7xx_adc.c | 318 ++++++++++
> hw/arm/npcm7xx.c | 65 +-
> hw/ipmi/Kconfig | 5 +
> hw/ipmi/ipmi_host.c | 40 ++
> hw/ipmi/ipmi_host_extern.c | 435 +++++++++++++
> hw/ipmi/meson.build | 3 +
> hw/ipmi/npcm7xx_kcs.c | 570 +++++++++++++++++
> hw/misc/meson.build | 1 +
> hw/misc/npcm7xx_clk.c | 795 +++++++++++++++++++++++-
> hw/misc/npcm7xx_pwm.c | 535 ++++++++++++++++
> hw/timer/npcm7xx_timer.c | 25 +-
> include/hw/adc/npcm7xx_adc.h | 72 +++
> include/hw/arm/npcm7xx.h | 6 +
> include/hw/ipmi/ipmi_host.h | 56 ++
> include/hw/ipmi/ipmi_responder.h | 66 ++
> include/hw/ipmi/npcm7xx_kcs.h | 105 ++++
> include/hw/misc/npcm7xx_clk.h | 146 ++++-
> include/hw/misc/npcm7xx_pwm.h | 106 ++++
> include/hw/timer/npcm7xx_timer.h | 1 +
> tests/qtest/meson.build | 4 +-
> tests/qtest/npcm7xx_adc-test.c | 400 ++++++++++++
> tests/qtest/npcm7xx_pwm-test.c | 490 +++++++++++++++
> 25 files changed, 4221 insertions(+), 32 deletions(-)
> create mode 100644 hw/adc/npcm7xx_adc.c
> create mode 100644 hw/ipmi/ipmi_host.c
> create mode 100644 hw/ipmi/ipmi_host_extern.c
> create mode 100644 hw/ipmi/npcm7xx_kcs.c
> create mode 100644 hw/misc/npcm7xx_pwm.c
> create mode 100644 include/hw/adc/npcm7xx_adc.h
> create mode 100644 include/hw/ipmi/ipmi_host.h
> create mode 100644 include/hw/ipmi/ipmi_responder.h
> create mode 100644 include/hw/ipmi/npcm7xx_kcs.h
> create mode 100644 include/hw/misc/npcm7xx_pwm.h
> create mode 100644 tests/qtest/npcm7xx_adc-test.c
> create mode 100644 tests/qtest/npcm7xx_pwm-test.c
>
> --
> 2.29.2.684.gfbc64c5ab5-goog
>