From: Tomer Maimon <[email protected]>
Add support for Nuvoton NPCM BMC hardware to the Platform Environment
Control Interface (PECI) subsystem.
Signed-off-by: Tomer Maimon <[email protected]>
Signed-off-by: Tyrone Ting <[email protected]>
Co-developed-by: Iwona Winiarska <[email protected]>
Signed-off-by: Iwona Winiarska <[email protected]>
---
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.
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/peci.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+/* NPCM GCR module */
+#define NPCM_INTCR3_OFFSET 0x9C
+#define NPCM_INTCR3_PECIVSEL BIT(19)
+
+/* NPCM PECI Registers */
+#define NPCM_PECI_CTL_STS 0x00
+#define NPCM_PECI_RD_LENGTH 0x04
+#define NPCM_PECI_ADDR 0x08
+#define NPCM_PECI_CMD 0x0C
+#define NPCM_PECI_CTL2 0x10
+#define NPCM_PECI_WR_LENGTH 0x1C
+#define NPCM_PECI_PDDR 0x2C
+#define NPCM_PECI_DAT_INOUT(n) (0x100 + ((n) * 4))
+
+#define NPCM_PECI_MAX_REG 0x200
+
+/* NPCM_PECI_CTL_STS - 0x00 : Control Register */
+#define NPCM_PECI_CTRL_DONE_INT_EN BIT(6)
+#define NPCM_PECI_CTRL_ABRT_ERR BIT(4)
+#define NPCM_PECI_CTRL_CRC_ERR BIT(3)
+#define NPCM_PECI_CTRL_DONE BIT(1)
+#define NPCM_PECI_CTRL_START_BUSY BIT(0)
+
+/* NPCM_PECI_RD_LENGTH - 0x04 : Command Register */
+#define NPCM_PECI_RD_LEN_MASK GENMASK(6, 0)
+
+/* NPCM_PECI_CMD - 0x10 : Command Register */
+#define NPCM_PECI_CTL2_MASK GENMASK(7, 6)
+
+/* NPCM_PECI_WR_LENGTH - 0x1C : Command Register */
+#define NPCM_PECI_WR_LEN_MASK GENMASK(6, 0)
+
+/* NPCM_PECI_PDDR - 0x2C : Command Register */
+#define NPCM_PECI_PDDR_MASK GENMASK(4, 0)
+
+#define NPCM_PECI_INT_MASK (NPCM_PECI_CTRL_ABRT_ERR | \
+ NPCM_PECI_CTRL_CRC_ERR | \
+ NPCM_PECI_CTRL_DONE)
+
+#define NPCM_PECI_IDLE_CHECK_TIMEOUT_USEC (50 * USEC_PER_MSEC)
+#define NPCM_PECI_IDLE_CHECK_INTERVAL_USEC (10 * USEC_PER_MSEC)
+#define NPCM_PECI_CMD_TIMEOUT_MS_DEFAULT 1000
+#define NPCM_PECI_CMD_TIMEOUT_MS_MAX 60000
+#define NPCM_PECI_HOST_NEG_BIT_RATE_DEFAULT 15
+#define NPCM_PECI_PULL_DOWN_DEFAULT 0
+
+struct npcm_peci {
+ u32 cmd_timeout_ms;
+ struct completion xfer_complete;
+ struct regmap *regmap;
+ u32 status;
+ spinlock_t lock; /* to sync completion status handling */
+ struct peci_controller *controller;
+ struct device *dev;
+ struct clk *clk;
+ int irq;
+};
+
+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
+
+ priv->status = 0;
+ regmap_update_bits(priv->regmap, NPCM_PECI_CTL_STS, NPCM_PECI_CTRL_START_BUSY,
+ NPCM_PECI_CTRL_START_BUSY);
+
+ spin_unlock_irq(&priv->lock);
+
+ ret = wait_for_completion_interruptible_timeout(&priv->xfer_complete, timeout);
+ if (ret < 0)
+ return ret;
+
+ if (ret == 0) {
+ dev_dbg(priv->dev, "timeout waiting for a response\n");
+ return -ETIMEDOUT;
+ }
+
+ spin_lock_irq(&priv->lock);
+
+ if (priv->status != NPCM_PECI_CTRL_DONE) {
+ spin_unlock_irq(&priv->lock);
+ dev_dbg(priv->dev, "no valid response, status: %#02x\n", priv->status);
+ return -EIO;
+ }
+
+ regmap_write(priv->regmap, NPCM_PECI_CMD, 0);
+
+ for (i = 0; i < req->rx.len; i++) {
+ regmap_read(priv->regmap, NPCM_PECI_DAT_INOUT(i), &msg_rd);
+ req->rx.buf[i] = (u8)msg_rd;
+ }
+
+ spin_unlock_irq(&priv->lock);
+
+#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
+ print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req->rx.len);
+#endif
+ return 0;
+}
+
+static irqreturn_t npcm_peci_irq_handler(int irq, void *arg)
+{
+ struct npcm_peci *priv = arg;
+ u32 status_ack = 0;
+ u32 status;
+
+ spin_lock(&priv->lock);
+ regmap_read(priv->regmap, NPCM_PECI_CTL_STS, &status);
+ priv->status |= (status & NPCM_PECI_INT_MASK);
+
+ if (status & NPCM_PECI_CTRL_CRC_ERR)
+ status_ack |= NPCM_PECI_CTRL_CRC_ERR;
+
+ if (status & NPCM_PECI_CTRL_ABRT_ERR)
+ status_ack |= NPCM_PECI_CTRL_ABRT_ERR;
+
+ /*
+ * All commands should be ended up with a NPCM_PECI_CTRL_DONE
+ * bit set even in an error case.
+ */
+ if (status & NPCM_PECI_CTRL_DONE) {
+ status_ack |= NPCM_PECI_CTRL_DONE;
+ complete(&priv->xfer_complete);
+ }
+
+ regmap_write_bits(priv->regmap, NPCM_PECI_CTL_STS, NPCM_PECI_INT_MASK, status_ack);
+
+ spin_unlock(&priv->lock);
+ return IRQ_HANDLED;
+}
+
+static int npcm_peci_init_ctrl(struct npcm_peci *priv)
+{
+ u32 cmd_sts;
+ int ret;
+
+ priv->clk = devm_clk_get_enabled(priv->dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ dev_err(priv->dev, "failed to get ref clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ ret = device_property_read_u32(priv->dev, "cmd-timeout-ms", &priv->cmd_timeout_ms);
+ if (ret) {
+ priv->cmd_timeout_ms = NPCM_PECI_CMD_TIMEOUT_MS_DEFAULT;
+ } else if (priv->cmd_timeout_ms > NPCM_PECI_CMD_TIMEOUT_MS_MAX ||
+ priv->cmd_timeout_ms == 0) {
+ dev_warn(priv->dev, "invalid cmd-timeout-ms: %u, falling back to: %u\n",
+ priv->cmd_timeout_ms, NPCM_PECI_CMD_TIMEOUT_MS_DEFAULT);
+
+ priv->cmd_timeout_ms = NPCM_PECI_CMD_TIMEOUT_MS_DEFAULT;
+ }
+
+ regmap_update_bits(priv->regmap, NPCM_PECI_CTL2, NPCM_PECI_CTL2_MASK,
+ NPCM_PECI_PULL_DOWN_DEFAULT << 6);
+
+ regmap_update_bits(priv->regmap, NPCM_PECI_PDDR, NPCM_PECI_PDDR_MASK,
+ NPCM_PECI_HOST_NEG_BIT_RATE_DEFAULT);
+
+ 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 */
+
+ /* PECI interrupt enable */
+ regmap_update_bits(priv->regmap, NPCM_PECI_CTL_STS, NPCM_PECI_CTRL_DONE_INT_EN,
+ NPCM_PECI_CTRL_DONE_INT_EN);
+
+ return 0;
+}
+
+static const struct regmap_config npcm_peci_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = NPCM_PECI_MAX_REG,
+ .fast_io = true,
+};
+
+static struct peci_controller_ops npcm_ops = {
+ .xfer = npcm_peci_xfer,
+};
+
+static int npcm_peci_probe(struct platform_device *pdev)
+{
+ struct peci_controller *controller;
+ struct npcm_peci *priv;
+ void __iomem *base;
+ int ret;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = &pdev->dev;
+ dev_set_drvdata(&pdev->dev, priv);
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ priv->regmap = devm_regmap_init_mmio(&pdev->dev, base, &npcm_peci_regmap_config);
+ if (IS_ERR(priv->regmap))
+ return PTR_ERR(priv->regmap);
+
+ priv->irq = platform_get_irq(pdev, 0);
+ if (priv->irq < 0)
+ return priv->irq;
+
+ ret = devm_request_irq(&pdev->dev, priv->irq, npcm_peci_irq_handler,
+ 0, "peci-npcm-irq", priv);
+ if (ret)
+ return ret;
+
+ init_completion(&priv->xfer_complete);
+ spin_lock_init(&priv->lock);
+
+ ret = npcm_peci_init_ctrl(priv);
+ if (ret)
+ return ret;
+
+ controller = devm_peci_controller_add(priv->dev, &npcm_ops);
+ if (IS_ERR(controller))
+ return dev_err_probe(priv->dev, PTR_ERR(controller),
+ "failed to add npcm peci controller\n");
+
+ priv->controller = controller;
+
+ return 0;
+}
+
+static const struct of_device_id npcm_peci_of_table[] = {
+ { .compatible = "nuvoton,npcm750-peci", },
+ { .compatible = "nuvoton,npcm845-peci", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, npcm_peci_of_table);
+
+static struct platform_driver npcm_peci_driver = {
+ .probe = npcm_peci_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = npcm_peci_of_table,
+ },
+};
+module_platform_driver(npcm_peci_driver);
+
+MODULE_AUTHOR("Tomer Maimon <[email protected]>");
+MODULE_DESCRIPTION("NPCM PECI driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PECI);
--
2.40.1
Dear Iwona,
Am 20.07.23 um 00:08 schrieb Iwona Winiarska:
> From: Tomer Maimon <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Tyrone Ting <[email protected]>
> Co-developed-by: Iwona Winiarska <[email protected]>
> Signed-off-by: Iwona Winiarska <[email protected]>
> ---
> 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 <[email protected]>");
> +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:20 +0200, Paul Menzel wrote:
> Dear Iwona,
>
>
> Am 20.07.23 um 00:08 schrieb Iwona Winiarska:
> > From: Tomer Maimon <[email protected]>
> >
> > 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.
>
> Additionally, please document how you tested this.
Are you asking to include this information in the commit message?
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.
>
> > Signed-off-by: Tomer Maimon <[email protected]>
> > Signed-off-by: Tyrone Ting <[email protected]>
> > Co-developed-by: Iwona Winiarska <[email protected]>
> > Signed-off-by: Iwona Winiarska <[email protected]>
> > ---
> > 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).
>
> […]
>
> > +module_platform_driver(npcm_peci_driver);
> > +
> > +MODULE_AUTHOR("Tomer Maimon <[email protected]>");
> > +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.
Thanks
-Iwona
>
>
> Kind regards,
>
> Paul
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 <[email protected]>
>>>
>>> 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.
>> 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.
>>> Signed-off-by: Tomer Maimon <[email protected]>
>>> Signed-off-by: Tyrone Ting <[email protected]>
>>> Co-developed-by: Iwona Winiarska <[email protected]>
>>> Signed-off-by: Iwona Winiarska <[email protected]>
>>> ---
>>> 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
>> […]
>>
>>> +module_platform_driver(npcm_peci_driver);
>>> +
>>> +MODULE_AUTHOR("Tomer Maimon <[email protected]>");
>>> +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 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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Tyrone Ting <[email protected]>
> > > > Co-developed-by: Iwona Winiarska <[email protected]>
> > > > Signed-off-by: Iwona Winiarska <[email protected]>
> > > > ---
> > > > 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 <[email protected]>");
> > > > +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
Dear Iwona,
Am 20.07.23 um 22:20 schrieb Winiarska, Iwona:
> On Thu, 2023-07-20 at 16:47 +0200, Paul Menzel wrote:
>> 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 <[email protected]>
>>>>>
>>>>> 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?
Hopefully. But I wonder, how can you develop and review the patch then?
>>>> 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.
I saw some commits document on what hardware it was tested.
> The explanation body in the commit message should explain *why* the patch was
> created, not how to test it.
I disagree. It should of course the why, but sometimes also the how
(implementation), the used datasheets, and all other details making it
easy to review and give reviewers without the hardware confidence, that
the patch is good.
> 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.
I just meant: Tested on server X with BMC Y using Nuvoton Z. Driver
registered correctly, and devices A were discovered.
>>>>> Signed-off-by: Tomer Maimon <[email protected]>
>>>>> Signed-off-by: Tyrone Ting <[email protected]>
>>>>> Co-developed-by: Iwona Winiarska <[email protected]>
>>>>> Signed-off-by: Iwona Winiarska <[email protected]>
>>>>> ---
>>>>> 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
[…]
>>>>> +#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 :)
Thank you for the explanation. Currently, this is only used in the PECI
subsystem:
$ git grep 'if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)'
drivers/mtd/nand/raw/nand_base.c:#if
IS_ENABLED(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
drivers/peci/controller/peci-aspeed.c:#if
IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
drivers/peci/controller/peci-aspeed.c:#if
IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
include/linux/mtd/rawnand.h:#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG) ||
defined(DEBUG)
I think, it will only cause confusing for users, wondering why it does
not show up with `-DDEBUG`. I assume the Linux kernel offers other ways
to do what you are trying to achieve. Maybe using a dump_traffic knob or
so in `/sys`.
Kind regards,
Paul
On Fri, 2023-07-21 at 08:30 +0200, Paul Menzel wrote:
> Dear Iwona,
>
>
> Am 20.07.23 um 22:20 schrieb Winiarska, Iwona:
> > On Thu, 2023-07-20 at 16:47 +0200, Paul Menzel wrote:
>
> > > 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 <[email protected]>
> > > > > >
> > > > > > 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?
>
> Hopefully. But I wonder, how can you develop and review the patch then?
It is explained in the cover letter.
Also, the review is not only about verifying driver/hardware interactions.
In fact - we often have to trust the author, because the docs are not available.
Interactions between software (other kernel components), whether the driver is a
good fit within its subsystem, etc. are just as important, and it's something
that we can review without the docs.
>
> > > > > 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.
>
> I saw some commits document on what hardware it was tested.
>
> > The explanation body in the commit message should explain *why* the patch
> > was
> > created, not how to test it.
>
> I disagree. It should of course the why, but sometimes also the how
> (implementation), the used datasheets, and all other details making it
> easy to review and give reviewers without the hardware confidence, that
> the patch is good.
But it will be persisted for eternity in the git log.
And it is only about the review of the series as a whole, which means that
ultimately, having this information in individual commits is just adding noise.
>
> > 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.
>
> I just meant: Tested on server X with BMC Y using Nuvoton Z. Driver
> registered correctly, and devices A were discovered.
The series (after my modifications) was tested by Tomer from Nuvoton:
https://lore.kernel.org/openbmc/CAP6Zq1h1if4hyubyh6N8EOdGOu+zp0qVUimF-9L2eXZ-QFAYjw@mail.gmail.com/
I can link this in the cover letter.
>
> > > > > > Signed-off-by: Tomer Maimon <[email protected]>
> > > > > > Signed-off-by: Tyrone Ting <[email protected]>
> > > > > > Co-developed-by: Iwona Winiarska <[email protected]>
> > > > > > Signed-off-by: Iwona Winiarska <[email protected]>
> > > > > > ---
> > > > > > 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
>
> […]
>
> > > > > > +#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 :)
>
> Thank you for the explanation. Currently, this is only used in the PECI
> subsystem:
That's simply not true.
The same approach is used in other subsystems as well, sometimes it covers
individual printk:
https://elixir.bootlin.com/linux/v6.4/source/drivers/rpmsg/rpmsg_ns.c#L40
In other cases it covers custom wrappers:
https://elixir.bootlin.com/linux/v6.4/source/drivers/usb/host/ehci-dbg.c#L8
There are more examples in the tree, but the general idea is the same - if the
log is verbose and printed often (lies on a hotpath), and we can't ratelimit,
hiding it behind dynamic debug availability is an option to consider.
>
> $ git grep 'if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)'
> drivers/mtd/nand/raw/nand_base.c:#if
> IS_ENABLED(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> drivers/peci/controller/peci-aspeed.c:#if
> IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
> drivers/peci/controller/peci-aspeed.c:#if
> IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
> include/linux/mtd/rawnand.h:#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG) ||
> defined(DEBUG)
>
> I think, it will only cause confusing for users, wondering why it does
> not show up with `-DDEBUG`. I assume the Linux kernel offers other ways
> to do what you are trying to achieve. Maybe using a dump_traffic knob or
> so in `/sys`.
Adding a new sysfs ABI for debug prints? No.
Alternative would be to use tracepoints, but that's semi-stable and until now we
only had one controller driver, so, for now, I would prefer to postpone any PECI
tracepoint conversions.
Thanks
-Iwona
>
>
> Kind regards,
>
> Paul
Hi Paul,
Thanks for your comments.
On Fri, 21 Jul 2023 at 12:22, Winiarska, Iwona
<[email protected]> wrote:
>
> On Fri, 2023-07-21 at 08:30 +0200, Paul Menzel wrote:
> > Dear Iwona,
> >
> >
> > Am 20.07.23 um 22:20 schrieb Winiarska, Iwona:
> > > On Thu, 2023-07-20 at 16:47 +0200, Paul Menzel wrote:
> >
> > > > 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 <[email protected]>
> > > > > > >
> > > > > > > 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?
> >
> > Hopefully. But I wonder, how can you develop and review the patch then?
>
> It is explained in the cover letter.
> Also, the review is not only about verifying driver/hardware interactions.
> In fact - we often have to trust the author, because the docs are not available.
> Interactions between software (other kernel components), whether the driver is a
> good fit within its subsystem, etc. are just as important, and it's something
> that we can review without the docs.
As Iwona mentions in the cover letter,
The series adds support for PECI on Nuvoton-based BMC boards.
It is based on patches that were sent by Nuvoton and we checking Iwona
upstream NPCM PECI driver on NPCM systems.
Iwona, in case you like to have the NPCM BMC datasheet, it can be
provided under NDA to relevant companies.
>
> >
> > > > > > 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.
> >
> > I saw some commits document on what hardware it was tested.
> >
> > > The explanation body in the commit message should explain *why* the patch
> > > was
> > > created, not how to test it.
> >
> > I disagree. It should of course the why, but sometimes also the how
> > (implementation), the used datasheets, and all other details making it
> > easy to review and give reviewers without the hardware confidence, that
> > the patch is good.
>
> But it will be persisted for eternity in the git log.
> And it is only about the review of the series as a whole, which means that
> ultimately, having this information in individual commits is just adding noise.
>
> >
> > > 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.
> >
> > I just meant: Tested on server X with BMC Y using Nuvoton Z. Driver
> > registered correctly, and devices A were discovered.
>
> The series (after my modifications) was tested by Tomer from Nuvoton:
> https://lore.kernel.org/openbmc/CAP6Zq1h1if4hyubyh6N8EOdGOu+zp0qVUimF-9L2eXZ-QFAYjw@mail.gmail.com/
> I can link this in the cover letter.
>
> >
> > > > > > > Signed-off-by: Tomer Maimon <[email protected]>
> > > > > > > Signed-off-by: Tyrone Ting <[email protected]>
> > > > > > > Co-developed-by: Iwona Winiarska <[email protected]>
> > > > > > > Signed-off-by: Iwona Winiarska <[email protected]>
> > > > > > > ---
> > > > > > > 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
> >
> > […]
> >
> > > > > > > +#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 :)
> >
> > Thank you for the explanation. Currently, this is only used in the PECI
> > subsystem:
>
> That's simply not true.
> The same approach is used in other subsystems as well, sometimes it covers
> individual printk:
> https://elixir.bootlin.com/linux/v6.4/source/drivers/rpmsg/rpmsg_ns.c#L40
> In other cases it covers custom wrappers:
> https://elixir.bootlin.com/linux/v6.4/source/drivers/usb/host/ehci-dbg.c#L8
>
> There are more examples in the tree, but the general idea is the same - if the
> log is verbose and printed often (lies on a hotpath), and we can't ratelimit,
> hiding it behind dynamic debug availability is an option to consider.
>
> >
> > $ git grep 'if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)'
> > drivers/mtd/nand/raw/nand_base.c:#if
> > IS_ENABLED(CONFIG_DYNAMIC_DEBUG) || defined(DEBUG)
> > drivers/peci/controller/peci-aspeed.c:#if
> > IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
> > drivers/peci/controller/peci-aspeed.c:#if
> > IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
> > include/linux/mtd/rawnand.h:#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG) ||
> > defined(DEBUG)
> >
> > I think, it will only cause confusing for users, wondering why it does
> > not show up with `-DDEBUG`. I assume the Linux kernel offers other ways
> > to do what you are trying to achieve. Maybe using a dump_traffic knob or
> > so in `/sys`.
>
> Adding a new sysfs ABI for debug prints? No.
> Alternative would be to use tracepoints, but that's semi-stable and until now we
> only had one controller driver, so, for now, I would prefer to postpone any PECI
> tracepoint conversions.
>
> Thanks
> -Iwona
>
> >
> >
> > Kind regards,
> >
> > Paul
>
Iwona, we highly appreciate that you upstream Nuvoton NPCM PECI driver :-)
Thanks,
Tomer