| Message ID | 20230719220853.1029316-1-iwona.winiarska@intel.com |
|---|---|
| Headers | show |
| Series | Add support for PECI Nuvoton | expand |
On 20/07/2023 00:08, Iwona Winiarska wrote: > From: Tomer Maimon <tmaimon77@gmail.com> > > Add device tree bindings for the peci-npcm controller driver. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > Signed-off-by: Tyrone Ting <warp5tw@gmail.com> > Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com> > Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com> > --- No changes from previous versions? Nothing improved? > .../devicetree/bindings/peci/peci-npcm.yaml | 56 +++++++++++++++++++ > 1 file changed, 56 insertions(+) > create mode 100644 Documentation/devicetree/bindings/peci/peci-npcm.yaml Use compatible as filename. Best regards, Krzysztof
Dear Iwona, Am 20.07.23 um 00:08 schrieb Iwona Winiarska: > From: Tomer Maimon <tmaimon77@gmail.com> > > Add support for Nuvoton NPCM BMC hardware to the Platform Environment > Control Interface (PECI) subsystem. Please elaborate on the implementation, and document the used datasheets. Additionally, please document how you tested this. > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > Signed-off-by: Tyrone Ting <warp5tw@gmail.com> > Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com> > Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com> > --- > drivers/peci/controller/Kconfig | 16 ++ > drivers/peci/controller/Makefile | 1 + > drivers/peci/controller/peci-npcm.c | 298 ++++++++++++++++++++++++++++ > 3 files changed, 315 insertions(+) > create mode 100644 drivers/peci/controller/peci-npcm.c > > diff --git a/drivers/peci/controller/Kconfig b/drivers/peci/controller/Kconfig > index 2fc5e2abb74a..4f9c245ad042 100644 > --- a/drivers/peci/controller/Kconfig > +++ b/drivers/peci/controller/Kconfig > @@ -16,3 +16,19 @@ config PECI_ASPEED > > This driver can also be built as a module. If so, the module will > be called peci-aspeed. > + > +config PECI_NPCM > + tristate "Nuvoton NPCM PECI support" > + depends on ARCH_NPCM || COMPILE_TEST > + depends on OF > + select REGMAP_MMIO > + help > + This option enables PECI controller driver for Nuvoton NPCM7XX > + and NPCM8XX SoCs. It allows BMC to discover devices connected > + to it and communicate with them using PECI protocol. > + > + Say Y here if you want support for the Platform Environment Control > + Interface (PECI) bus adapter driver on the Nuvoton NPCM SoCs. > + > + This support is also available as a module. If so, the module > + will be called peci-npcm. > diff --git a/drivers/peci/controller/Makefile b/drivers/peci/controller/Makefile > index 022c28ef1bf0..e247449bb423 100644 > --- a/drivers/peci/controller/Makefile > +++ b/drivers/peci/controller/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > > obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o > +obj-$(CONFIG_PECI_NPCM) += peci-npcm.o > diff --git a/drivers/peci/controller/peci-npcm.c b/drivers/peci/controller/peci-npcm.c > new file mode 100644 > index 000000000000..3647e3628a17 > --- /dev/null > +++ b/drivers/peci/controller/peci-npcm.c > @@ -0,0 +1,298 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2019 Nuvoton Technology corporation. No dot/period at the end. […] > +static int npcm_peci_xfer(struct peci_controller *controller, u8 addr, struct peci_request *req) > +{ > + struct npcm_peci *priv = dev_get_drvdata(controller->dev.parent); > + unsigned long timeout = msecs_to_jiffies(priv->cmd_timeout_ms); > + unsigned int msg_rd; > + u32 cmd_sts; > + int i, ret; > + > + /* Check command sts and bus idle state */ > + ret = regmap_read_poll_timeout(priv->regmap, NPCM_PECI_CTL_STS, cmd_sts, > + !(cmd_sts & NPCM_PECI_CTRL_START_BUSY), > + NPCM_PECI_IDLE_CHECK_INTERVAL_USEC, > + NPCM_PECI_IDLE_CHECK_TIMEOUT_USEC); > + if (ret) > + return ret; /* -ETIMEDOUT */ > + > + spin_lock_irq(&priv->lock); > + reinit_completion(&priv->xfer_complete); > + > + regmap_write(priv->regmap, NPCM_PECI_ADDR, addr); > + regmap_write(priv->regmap, NPCM_PECI_RD_LENGTH, NPCM_PECI_WR_LEN_MASK & req->rx.len); > + regmap_write(priv->regmap, NPCM_PECI_WR_LENGTH, NPCM_PECI_WR_LEN_MASK & req->tx.len); > + > + if (req->tx.len) { > + regmap_write(priv->regmap, NPCM_PECI_CMD, req->tx.buf[0]); > + > + for (i = 0; i < (req->tx.len - 1); i++) > + regmap_write(priv->regmap, NPCM_PECI_DAT_INOUT(i), req->tx.buf[i + 1]); > + } > + > +#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG) > + dev_dbg(priv->dev, "addr : %#02x, tx.len : %#02x, rx.len : %#02x\n", > + addr, req->tx.len, req->rx.len); > + print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req->tx.len); > +#endif The preprocessor guards are not needed, as it’s taken care of in `include/linux/printk.h`. Also in other parts of the patch. […] > +module_platform_driver(npcm_peci_driver); > + > +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>"); > +MODULE_DESCRIPTION("NPCM PECI driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS(PECI); Also add an entry to `MAINTAINERS`, if Tomer is going to be the maintainer? Kind regards, Paul
On Thu, 2023-07-20 at 08:18 +0200, Krzysztof Kozlowski wrote: > On 20/07/2023 00:08, Iwona Winiarska wrote: > > From: Tomer Maimon <tmaimon77@gmail.com> > > > > Add device tree bindings for the peci-npcm controller driver. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > Signed-off-by: Tyrone Ting <warp5tw@gmail.com> > > Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com> > > Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com> > > --- > > No changes from previous versions? Nothing improved? This is the first submission. > > > .../devicetree/bindings/peci/peci-npcm.yaml | 56 +++++++++++++++++++ > > 1 file changed, 56 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/peci/peci-npcm.yaml > > Use compatible as filename. Sure - there are two compatible entries, so I'm going to change it to nuvoton,npcm-peci.yaml Thanks -Iwona > > Best regards, > Krzysztof >
On Thu, 2023-07-20 at 16:47 +0200, Paul Menzel wrote: > Dear Iwona, > > > Thank you for the quick reply. > > Am 20.07.23 um 10:38 schrieb Winiarska, Iwona: > > On Thu, 2023-07-20 at 08:20 +0200, Paul Menzel wrote: > > > > Am 20.07.23 um 00:08 schrieb Iwona Winiarska: > > > > From: Tomer Maimon <tmaimon77@gmail.com> > > > > > > > > Add support for Nuvoton NPCM BMC hardware to the Platform Environment > > > > Control Interface (PECI) subsystem. > > > > > > Please elaborate on the implementation, and document the used datasheets. > > > > As far as I know, there is no publicly available documentation. > > Too bad. Documenting the datasheet name and version is still important, > so developers could request it, and it can be mapped, once they are made > public. Sorry, unfortunately I can't help with that, I don't have access to any Nuvoton docs. Perhaps Tomer can provide more information? > > > > Additionally, please document how you tested this. > > > > Are you asking to include this information in the commit message? > > Yes. > > > That would be unusual. > > But in general - it's a controller driver, it allows PECI subsystem to > > detect > > devices behind it and for PECI drivers to bind to those devices. > > Having a test line in the commit message is not unusual at. So people > with systems where it doesn’t work, could replicate the test setup to at > least verify that it works in that configuration. It's unusual as almost none of the commits in Linux kernel contain "how to test it" description. The explanation body in the commit message should explain *why* the patch was created, not how to test it. And when taken as a series - it's self documenting. There's a Kconfig which allows to enable/disable the driver, and there are bindings which show what platform contains the hardware that is compatible with it. > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > > Signed-off-by: Tyrone Ting <warp5tw@gmail.com> > > > > Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com> > > > > Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com> > > > > --- > > > > drivers/peci/controller/Kconfig | 16 ++ > > > > drivers/peci/controller/Makefile | 1 + > > > > drivers/peci/controller/peci-npcm.c | 298 > > > > ++++++++++++++++++++++++++++ > > > > 3 files changed, 315 insertions(+) > > > > create mode 100644 drivers/peci/controller/peci-npcm.c > > > > > > > > diff --git a/drivers/peci/controller/Kconfig > > > > b/drivers/peci/controller/Kconfig > > > > index 2fc5e2abb74a..4f9c245ad042 100644 > > > > --- a/drivers/peci/controller/Kconfig > > > > +++ b/drivers/peci/controller/Kconfig > > > > @@ -16,3 +16,19 @@ config PECI_ASPEED > > > > > > > > This driver can also be built as a module. If so, the module > > > > will > > > > be called peci-aspeed. > > > > + > > > > +config PECI_NPCM > > > > + tristate "Nuvoton NPCM PECI support" > > > > + depends on ARCH_NPCM || COMPILE_TEST > > > > + depends on OF > > > > + select REGMAP_MMIO > > > > + help > > > > + This option enables PECI controller driver for Nuvoton NPCM7XX > > > > + and NPCM8XX SoCs. It allows BMC to discover devices connected > > > > + to it and communicate with them using PECI protocol. > > > > + > > > > + Say Y here if you want support for the Platform Environment > > > > Control > > > > + Interface (PECI) bus adapter driver on the Nuvoton NPCM SoCs. > > > > + > > > > + This support is also available as a module. If so, the module > > > > + will be called peci-npcm. > > > > diff --git a/drivers/peci/controller/Makefile > > > > b/drivers/peci/controller/Makefile > > > > index 022c28ef1bf0..e247449bb423 100644 > > > > --- a/drivers/peci/controller/Makefile > > > > +++ b/drivers/peci/controller/Makefile > > > > @@ -1,3 +1,4 @@ > > > > # SPDX-License-Identifier: GPL-2.0-only > > > > > > > > obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o > > > > +obj-$(CONFIG_PECI_NPCM) += peci-npcm.o > > > > diff --git a/drivers/peci/controller/peci-npcm.c > > > > b/drivers/peci/controller/peci-npcm.c > > > > new file mode 100644 > > > > index 000000000000..3647e3628a17 > > > > --- /dev/null > > > > +++ b/drivers/peci/controller/peci-npcm.c > > > > @@ -0,0 +1,298 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +// Copyright (c) 2019 Nuvoton Technology corporation. > > > > > > No dot/period at the end. > > > > > > […] > > > > > > > +static int npcm_peci_xfer(struct peci_controller *controller, u8 addr, > > > > struct peci_request *req) > > > > +{ > > > > + struct npcm_peci *priv = dev_get_drvdata(controller- > > > > >dev.parent); > > > > + unsigned long timeout = msecs_to_jiffies(priv->cmd_timeout_ms); > > > > + unsigned int msg_rd; > > > > + u32 cmd_sts; > > > > + int i, ret; > > > > + > > > > + /* Check command sts and bus idle state */ > > > > + ret = regmap_read_poll_timeout(priv->regmap, NPCM_PECI_CTL_STS, > > > > cmd_sts, > > > > + !(cmd_sts & > > > > NPCM_PECI_CTRL_START_BUSY), > > > > + > > > > NPCM_PECI_IDLE_CHECK_INTERVAL_USEC, > > > > + > > > > NPCM_PECI_IDLE_CHECK_TIMEOUT_USEC); > > > > + if (ret) > > > > + return ret; /* -ETIMEDOUT */ > > > > + > > > > + spin_lock_irq(&priv->lock); > > > > + reinit_completion(&priv->xfer_complete); > > > > + > > > > + regmap_write(priv->regmap, NPCM_PECI_ADDR, addr); > > > > + regmap_write(priv->regmap, NPCM_PECI_RD_LENGTH, > > > > NPCM_PECI_WR_LEN_MASK & req->rx.len); > > > > + regmap_write(priv->regmap, NPCM_PECI_WR_LENGTH, > > > > NPCM_PECI_WR_LEN_MASK & req->tx.len); > > > > + > > > > + if (req->tx.len) { > > > > + regmap_write(priv->regmap, NPCM_PECI_CMD, req- > > > > >tx.buf[0]); > > > > + > > > > + for (i = 0; i < (req->tx.len - 1); i++) > > > > + regmap_write(priv->regmap, > > > > NPCM_PECI_DAT_INOUT(i), req->tx.buf[i + 1]); > > > > + } > > > > + > > > > +#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG) > > > > + dev_dbg(priv->dev, "addr : %#02x, tx.len : %#02x, rx.len : > > > > %#02x\n", > > > > + addr, req->tx.len, req->rx.len); > > > > + print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, > > > > req-tx.len); > > > > +#endif > > > > > > The preprocessor guards are not needed, as it’s taken care of in > > > `include/linux/printk.h`. Also in other parts of the patch. > > > > Since this is dumping the raw contents of PECI messages, it's going to be > > quite > > verbose. The idea behind preprocessor guard is that we don't ever want this > > to > > be converted to regular printk. If there's no dynamic debug available - this > > won't be printed unconditionally (even with -DDEBUG). > > How will it be converted to a regular printk? > > #if defined(CONFIG_DYNAMIC_DEBUG) || \ > (defined(CONFIG_DYNAMIC_DEBUG_CORE) && > defined(DYNAMIC_DEBUG_MODULE)) > #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \ > groupsize, buf, len, ascii) \ > dynamic_hex_dump(prefix_str, prefix_type, rowsize, \ > groupsize, buf, len, ascii) > #elif defined(DEBUG) > #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, > \ > groupsize, buf, len, ascii) \ > print_hex_dump(KERN_DEBUG, prefix_str, prefix_type, rowsize, \ > groupsize, buf, len, ascii) > #else > static inline void print_hex_dump_debug(const char *prefix_str, int > prefix_type, > int rowsize, int groupsize, > const void *buf, size_t len, > bool ascii) > { > } > #endif Let's consider 3 scenarios 1) Dynamic debug is available 2) Dynamic debug is not available, but we're built with -DDEBUG 3) Dynamic debug is not available, we're built without -DDEBUG For 1), print_hex_dump_debug is dynamic - it can be controlled (enabled/disabled) using dynamic debug knobs (debugfs / dyndbg kernel arg). For 2), print_hex_dump_debug is using print_hex_dump, which is just using printk with KERN_DEBUG level under the hood. For 3), it's compiled out. And it's scenario 2) that we would like to avoid, as hex-dumping all PECI communication into dmesg is likely going to make dmesg output unusable (can overflow, printing that to terminal is going to be slow, etc). The dump can be useful, it's just that in order to be useful it needs the dynamic debug facilities :) Thanks -Iwona > > > > […] > > > > > > > +module_platform_driver(npcm_peci_driver); > > > > + > > > > +MODULE_AUTHOR("Tomer Maimon <tomer.maimon@nuvoton.com>"); > > > > +MODULE_DESCRIPTION("NPCM PECI driver"); > > > > +MODULE_LICENSE("GPL"); > > > > +MODULE_IMPORT_NS(PECI); > > > > > > Also add an entry to `MAINTAINERS`, if Tomer is going to be the > > > maintainer? > > > > All of the newly added files should already be covered by either ARM/NUVOTON > > NPCM ARCHITECTURE or PECI SUBSYSTEM. > > Good to know. Thank you. > > > Kind regards, > > Paul
On Mon, 2023-07-24 at 08:38 -0600, Rob Herring wrote: > On Thu, Jul 20, 2023 at 08:03:28AM +0000, Winiarska, Iwona wrote: > > On Thu, 2023-07-20 at 08:18 +0200, Krzysztof Kozlowski wrote: > > > On 20/07/2023 00:08, Iwona Winiarska wrote: > > > > From: Tomer Maimon <tmaimon77@gmail.com> > > > > > > > > Add device tree bindings for the peci-npcm controller driver. > > > > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > > Signed-off-by: Tyrone Ting <warp5tw@gmail.com> > > > > Co-developed-by: Iwona Winiarska <iwona.winiarska@intel.com> > > > > Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com> > > > > --- > > > > > > No changes from previous versions? Nothing improved? > > > > This is the first submission. > > 13th by my count. > > https://lore.kernel.org/all/20230616193450.413366-2-iwona.winiarska@intel.com/ This was just a request for testing sent exclusively to openbmc mailinglist (and btw - there were no changes in between). > https://lore.kernel.org/all/20191211194624.2872-8-jae.hyun.yoo@linux.intel.com/ This is part of a series that contains whole, currently obsoleted, PECI subsystem, in a form that never got merged into mainline Linux. The PECI subsystem that is currently present in mainline Linux didn't include Nuvoton driver: https://lore.kernel.org/all/20220208153639.255278-1-iwona.winiarska@intel.com/ There is a subset of patches that is common between this submission and the obsoleted PECI subsystem series (the linked v11), but I don't see how continuing that numbering would provide any useful information to reviewers. Thanks -Iwona