On Wed, Dec 13, 2023, at 20:05, Tomer Maimon wrote:
> Add Nuvoton BMC NPCM BIOS post code (BPC) driver.
>
> The NPCM BPC monitoring two configurable I/O address written by the host
> on the bus.
>
> Signed-off-by: Tomer Maimon <[email protected]>
> ---
> drivers/soc/nuvoton/Kconfig | 9 +
> drivers/soc/nuvoton/Makefile | 1 +
> drivers/soc/nuvoton/npcm-bpc.c | 387 +++++++++++++++++++++++++++++++++
> 3 files changed, 397 insertions(+)
> create mode 100644 drivers/soc/nuvoton/npcm-bpc.c
I try hard to avoid having user interfaces in drivers/soc/, that
subsystem should primarily be used for things that don't have an
existing subsystem in the kernel and are used by other in-kernel
drivers but don't export hteir own misc device.
> diff --git a/drivers/soc/nuvoton/Kconfig b/drivers/soc/nuvoton/Kconfig
> index d5102f5f0c28..ebd162633942 100644
> --- a/drivers/soc/nuvoton/Kconfig
> +++ b/drivers/soc/nuvoton/Kconfig
> @@ -2,6 +2,15 @@
>
> menu "NUVOTON SoC drivers"
>
> +config NPCM_BPC
> + tristate "NPCM BIOS Post Code support"
> + depends on (ARCH_NPCM || COMPILE_TEST)
> + help
> + Provides NPCM driver to control the BIOS Post Code
> + interface which allows the BMC to monitor and save
> + the data written by the host to an arbitrary I/O port,
> + the BPC is connected to the host thourgh LPC or eSPI bus.
> +
This one in particular looks like this might be implemented
by more than one BMC type, it's a fairly generic functionality.
Have you talked to the other maintainers of SoCs used in
OpenBMC about coming up with a common interface?
> +#define DEVICE_NAME "npcm-bpc"
[nitpicking] No need for macros like this one, open-coding the
string is usually more readable.
Arnd
Hi Arnd,
Thanks for your comments.
On Thu, 14 Dec 2023 at 14:44, Arnd Bergmann <[email protected]> wrote:
>
> On Wed, Dec 13, 2023, at 20:05, Tomer Maimon wrote:
> > Add Nuvoton BMC NPCM BIOS post code (BPC) driver.
> >
> > The NPCM BPC monitoring two configurable I/O address written by the host
> > on the bus.
> >
> > Signed-off-by: Tomer Maimon <[email protected]>
> > ---
> > drivers/soc/nuvoton/Kconfig | 9 +
> > drivers/soc/nuvoton/Makefile | 1 +
> > drivers/soc/nuvoton/npcm-bpc.c | 387 +++++++++++++++++++++++++++++++++
> > 3 files changed, 397 insertions(+)
> > create mode 100644 drivers/soc/nuvoton/npcm-bpc.c
>
> I try hard to avoid having user interfaces in drivers/soc/, that
> subsystem should primarily be used for things that don't have an
> existing subsystem in the kernel and are used by other in-kernel
> drivers but don't export hteir own misc device.
>
> > diff --git a/drivers/soc/nuvoton/Kconfig b/drivers/soc/nuvoton/Kconfig
> > index d5102f5f0c28..ebd162633942 100644
> > --- a/drivers/soc/nuvoton/Kconfig
> > +++ b/drivers/soc/nuvoton/Kconfig
> > @@ -2,6 +2,15 @@
> >
> > menu "NUVOTON SoC drivers"
> >
> > +config NPCM_BPC
> > + tristate "NPCM BIOS Post Code support"
> > + depends on (ARCH_NPCM || COMPILE_TEST)
> > + help
> > + Provides NPCM driver to control the BIOS Post Code
> > + interface which allows the BMC to monitor and save
> > + the data written by the host to an arbitrary I/O port,
> > + the BPC is connected to the host thourgh LPC or eSPI bus.
> > +
>
> This one in particular looks like this might be implemented
> by more than one BMC type, it's a fairly generic functionality.
>
> Have you talked to the other maintainers of SoCs used in
> OpenBMC about coming up with a common interface?
Yes, Both Nuvoton and Aspeed use the same user-facing code to manage
the host snooping.
https://github.com/openbmc/phosphor-host-postd
>
> > +#define DEVICE_NAME "npcm-bpc"
Will do.
>
> [nitpicking] No need for macros like this one, open-coding the
> string is usually more readable.
>
> Arnd
Thanks,
Tomer
On Thu, Dec 14, 2023, at 14:09, Tomer Maimon wrote:
> On Thu, 14 Dec 2023 at 14:44, Arnd Bergmann <[email protected]> wrote:
>> >
>> > +config NPCM_BPC
>> > + tristate "NPCM BIOS Post Code support"
>> > + depends on (ARCH_NPCM || COMPILE_TEST)
>> > + help
>> > + Provides NPCM driver to control the BIOS Post Code
>> > + interface which allows the BMC to monitor and save
>> > + the data written by the host to an arbitrary I/O port,
>> > + the BPC is connected to the host thourgh LPC or eSPI bus.
>> > +
>>
>> This one in particular looks like this might be implemented
>> by more than one BMC type, it's a fairly generic functionality.
>>
>> Have you talked to the other maintainers of SoCs used in
>> OpenBMC about coming up with a common interface?
> Yes, Both Nuvoton and Aspeed use the same user-facing code to manage
> the host snooping.
> https://github.com/openbmc/phosphor-host-postd
Ok, that's good. I found the driver in drivers/soc/aspeed/aspeed-lpc-snoop.c
now and see that the implementation looks very similar.
I think we should do two things here:
- split out the common code into a shared module that exports the
symbols to be used by either one
- find a better place for both drivers outside of drivers/soc.
I would suggest drivers/misc/bmc/ but am open to other suggestions.
Arnd
Hi Arnd,
Thanks for your suggestion.
Appreciate it if Joel, OpenBMC Linux kernel maintainer, could share
his thoughts about it.
On Thu, 14 Dec 2023 at 17:49, Arnd Bergmann <[email protected]> wrote:
>
> On Thu, Dec 14, 2023, at 14:09, Tomer Maimon wrote:
> > On Thu, 14 Dec 2023 at 14:44, Arnd Bergmann <[email protected]> wrote:
> >> >
> >> > +config NPCM_BP
> >> > + tristate "NPCM BIOS Post Code support"
> >> > + depends on (ARCH_NPCM || COMPILE_TEST)
> >> > + help
> >> > + Provides NPCM driver to control the BIOS Post Code
> >> > + interface which allows the BMC to monitor and save
> >> > + the data written by the host to an arbitrary I/O port,
> >> > + the BPC is connected to the host thourgh LPC or eSPI bus.
> >> > +
> >>
> >> This one in particular looks like this might be implemented
> >> by more than one BMC type, it's a fairly generic functionality.
> >>
> >> Have you talked to the other maintainers of SoCs used in
> >> OpenBMC about coming up with a common interface?
> > Yes, Both Nuvoton and Aspeed use the same user-facing code to manage
> > the host snooping.
> > https://github.com/openbmc/phosphor-host-postd
>
> Ok, that's good. I found the driver in drivers/soc/aspeed/aspeed-lpc-snoop.c
> now and see that the implementation looks very similar.
>
> I think we should do two things here:
>
> - split out the common code into a shared module that exports the
> symbols to be used by either one
>
> - find a better place for both drivers outside of drivers/soc.
> I would suggest drivers/misc/bmc/ but am open to other suggestions.
>
> Arnd
Best regards,
Tomer