Re: [PATCH 0/7] Additional NPCM7xx devices and IPMI BMC emulation support

[Corey Minyard]

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
>