| Message ID | 20231213190528.3751583-1-tmaimon77@gmail.com |
|---|---|
| Headers | show |
| Series | soc: add NPCM BPC driver support | expand |
On 13/12/2023 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 <tmaimon77@gmail.com> > --- > 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 > > 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" ... > + > +static int npcm_bpc_config_irq(struct npcm_bpc *bpc, > + struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + int rc; > + > + bpc->irq = platform_get_irq(pdev, 0); > + if (bpc->irq < 0) { > + dev_err(dev, "get IRQ failed\n"); > + return bpc->irq; return dev_err_probe > + } > + > + rc = devm_request_irq(dev, bpc->irq, > + npcm_bpc_irq, IRQF_SHARED, > + DEVICE_NAME, bpc); > + if (rc < 0) { > + dev_warn(dev, "Unable to request IRQ %d\n", bpc->irq); return dev_err_probe > + return rc; > + } > + > + return 0; > +} > + > +static int npcm_enable_bpc(struct npcm_bpc *bpc, struct device *dev, > + int channel, u16 host_port) > +{ > + u8 addr_en, reg_en; > + int err; > + > + init_waitqueue_head(&bpc->ch[channel].wq); > + > + err = kfifo_alloc(&bpc->ch[channel].fifo, BPC_KFIFO_SIZE, > + GFP_KERNEL); > + if (err) > + return err; > + > + bpc->ch[channel].miscdev.minor = MISC_DYNAMIC_MINOR; > + bpc->ch[channel].miscdev.name = > + devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel); > + bpc->ch[channel].miscdev.fops = &npcm_bpc_fops; > + bpc->ch[channel].miscdev.parent = dev; > + err = misc_register(&bpc->ch[channel].miscdev); > + if (err) > + return err; > + > + bpc->ch[channel].data = bpc; > + bpc->ch[channel].host_reset = false; > + > + /* Enable host snoop channel at requested port */ > + switch (channel) { > + case 0: > + addr_en = FIFO_IOADDR1_ENABLE; > + iowrite8((u8)host_port & 0xFF, > + bpc->base + NPCM_BPCFA1L_REG); > + iowrite8((u8)(host_port >> 8), > + bpc->base + NPCM_BPCFA1M_REG); > + break; > + case 1: > + addr_en = FIFO_IOADDR2_ENABLE; > + iowrite8((u8)host_port & 0xFF, > + bpc->base + NPCM_BPCFA2L_REG); > + iowrite8((u8)(host_port >> 8), > + bpc->base + NPCM_BPCFA2M_REG); > + break; > + default: > + return -EINVAL; > + } > + > + if (bpc->en_dwcap) > + addr_en = FIFO_DWCAPTURE; > + > + /* > + * Enable FIFO Ready Interrupt, FIFO Capture of I/O addr, > + * and Host Reset > + */ > + reg_en = ioread8(bpc->base + NPCM_BPCFEN_REG); > + iowrite8(reg_en | addr_en | FIFO_READY_INT_ENABLE | > + HOST_RESET_INT_ENABLE, bpc->base + NPCM_BPCFEN_REG); > + > + return 0; > +} > + > +static void npcm_disable_bpc(struct npcm_bpc *bpc, int channel) > +{ > + u8 reg_en; > + > + switch (channel) { > + case 0: > + reg_en = ioread8(bpc->base + NPCM_BPCFEN_REG); > + if (bpc->en_dwcap) > + iowrite8(reg_en & ~FIFO_DWCAPTURE, > + bpc->base + NPCM_BPCFEN_REG); > + else > + iowrite8(reg_en & ~FIFO_IOADDR1_ENABLE, > + bpc->base + NPCM_BPCFEN_REG); > + break; > + case 1: > + reg_en = ioread8(bpc->base + NPCM_BPCFEN_REG); > + iowrite8(reg_en & ~FIFO_IOADDR2_ENABLE, > + bpc->base + NPCM_BPCFEN_REG); > + break; > + default: > + return; > + } > + > + if (!(reg_en & (FIFO_IOADDR1_ENABLE | FIFO_IOADDR2_ENABLE))) > + iowrite8(reg_en & > + ~(FIFO_READY_INT_ENABLE | HOST_RESET_INT_ENABLE), > + bpc->base + NPCM_BPCFEN_REG); > + > + kfifo_free(&bpc->ch[channel].fifo); > + misc_deregister(&bpc->ch[channel].miscdev); > +} > + > +static int npcm_bpc_probe(struct platform_device *pdev) > +{ > + struct npcm_bpc *bpc; > + struct device *dev; > + u32 port; > + int err; > + > + dev = &pdev->dev; > + > + bpc = devm_kzalloc(dev, sizeof(*bpc), GFP_KERNEL); > + if (!bpc) > + return -ENOMEM; > + > + bpc->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(bpc->base)) > + return PTR_ERR(bpc->base); > + > + dev_set_drvdata(&pdev->dev, bpc); Why do you use pdev again? Look earlier, you have local variable for this. > + > + err = of_property_read_u32_index(dev->of_node, "nuvoton,monitor-ports", > + 0, &port); > + if (err) { > + dev_err(dev, "no monitor ports configured\n"); > + return -ENODEV; > + } > + > + bpc->en_dwcap = > + of_property_read_bool(dev->of_node, "nuvoton,bpc-en-dwcapture"); > + > + bpc->dev = dev; > + > + err = npcm_bpc_config_irq(bpc, pdev); > + if (err) > + return err; > + > + err = npcm_enable_bpc(bpc, dev, 0, port); > + if (err) { > + dev_err(dev, "Enable BIOS post code I/O port 0 failed\n"); > + return err; > + } > + > + /* > + * Configuration of second BPC channel port is optional > + * Double-Word Capture ignoring address 2 > + */ > + if (!bpc->en_dwcap) { > + if (of_property_read_u32_index(dev->of_node, > + "nuvoton,monitor-ports", 1, > + &port) == 0) { > + err = npcm_enable_bpc(bpc, dev, 1, port); > + if (err) { > + dev_err(dev, "Enable BIOS post code I/O port 1 failed, disable I/O port 0\n"); > + npcm_disable_bpc(bpc, 0); > + return err; > + } > + } > + } > + > + pr_info("NPCM BIOS Post Code probe\n"); Drop > + > + return err; > +} > + > +static int npcm_bpc_remove(struct platform_device *pdev) > +{ > + struct npcm_bpc *bpc = dev_get_drvdata(&pdev->dev); > + u8 reg_en; > + > + reg_en = ioread8(bpc->base + NPCM_BPCFEN_REG); > + > + if (reg_en & FIFO_IOADDR1_ENABLE) > + npcm_disable_bpc(bpc, 0); > + if (reg_en & FIFO_IOADDR2_ENABLE) > + npcm_disable_bpc(bpc, 1); > + Best regards, Krzysztof
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 <tmaimon77@gmail.com> > --- > 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 <arnd@arndb.de> 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 <tmaimon77@gmail.com> > > --- > > 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 <arnd@arndb.de> 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 <arnd@arndb.de> wrote: > > On Thu, Dec 14, 2023, at 14:09, Tomer Maimon wrote: > > On Thu, 14 Dec 2023 at 14:44, Arnd Bergmann <arnd@arndb.de> 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