[PATCH 0/4] Add support for PECI Nuvoton

[PATCH 0/4] Add support for PECI Nuvoton

[Iwona Winiarska]

Hi!

The series adds support for PECI on Nuvoton-based BMC boards.
It is based on patches that were sent by Tomer Maimon from
Nuvoton [1].
Similar to Aspeed driver, unused (as in, default values were used in
all of the available DTS files) vendor-specific properties were
removed.
If there is a use-case for such properties, they can be added in
a separate series.

Thank you Tomer for testing this series on Nuvoton hardware.

Thanks
-Iwona

[1] https://lore.kernel.org/openbmc/CAP6Zq1jnbQ8k9VEyf9WgVq5DRrEzf5V6kaYP30S7g9BV9jKtaQ@mail.gmail.com/

Iwona Winiarska (2):
  ARM: dts: nuvoton: Add PECI controller node
  arm64: dts: nuvoton: Add PECI controller node

Tomer Maimon (2):
  dt-bindings: Add bindings for peci-npcm
  peci: Add peci-npcm controller driver

 .../devicetree/bindings/peci/peci-npcm.yaml   |  56 ++++
 .../dts/nuvoton/nuvoton-common-npcm7xx.dtsi   |   9 +
 .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi   |   9 +
 drivers/peci/controller/Kconfig               |  16 +
 drivers/peci/controller/Makefile              |   1 +
 drivers/peci/controller/peci-npcm.c           | 298 ++++++++++++++++++
 6 files changed, 389 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/peci/peci-npcm.yaml
 create mode 100644 drivers/peci/controller/peci-npcm.c

-- 
2.40.1

[PATCH 1/4] dt-bindings: Add bindings for peci-npcm

[Iwona Winiarska]

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>
---
 .../devicetree/bindings/peci/peci-npcm.yaml   | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/peci/peci-npcm.yaml

diff --git a/Documentation/devicetree/bindings/peci/peci-npcm.yaml b/Documentation/devicetree/bindings/peci/peci-npcm.yaml
new file mode 100644
index 000000000000..6eafa9ccaa54
--- /dev/null
+++ b/Documentation/devicetree/bindings/peci/peci-npcm.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/peci/peci-npcm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton PECI Bus
+
+maintainers:
+  - Tomer Maimon <tmaimon77@gmail.com>
+
+allOf:
+  - $ref: peci-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm750-peci
+      - nuvoton,npcm845-peci
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    description:
+      Clock source for PECI controller. Should reference the APB clock.
+    maxItems: 1
+
+  cmd-timeout-ms:
+    minimum: 1
+    maximum: 1000
+    default: 1000
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    peci-controller@f0100000 {
+      compatible = "nuvoton,npcm750-peci";
+      reg = <0xf0100000 0x200>;
+      interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&clk NPCM7XX_CLK_APB3>;
+      cmd-timeout-ms = <1000>;
+    };
+...
-- 
2.40.1

Re: [PATCH 1/4] dt-bindings: Add bindings for peci-npcm

[Krzysztof Kozlowski]

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

Re: [PATCH 1/4] dt-bindings: Add bindings for peci-npcm

[Winiarska, Iwona]

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
> 

Re: [PATCH 1/4] dt-bindings: Add bindings for peci-npcm

[Rob Herring]

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/
https://lore.kernel.org/all/20191211194624.2872-8-jae.hyun.yoo@linux.intel.com/

Re: [PATCH 1/4] dt-bindings: Add bindings for peci-npcm

[Winiarska, Iwona]

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

[PATCH 2/4] peci: Add peci-npcm controller driver

[Iwona Winiarska]

From: Tomer Maimon <tmaimon77@gmail.com>

Add support for Nuvoton NPCM BMC hardware to the Platform Environment
Control Interface (PECI) subsystem.

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.
+
+#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 <tomer.maimon@nuvoton.com>");
+MODULE_DESCRIPTION("NPCM PECI driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(PECI);
-- 
2.40.1

Re: [PATCH 2/4] peci: Add peci-npcm controller driver

[Paul Menzel]

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

Re: [PATCH 2/4] peci: Add peci-npcm controller driver

[Winiarska, Iwona]

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 <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.

> 
> 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 <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).

> 
> […]
> 
> > +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.

Thanks
-Iwona

> 
> 
> Kind regards,
> 
> Paul

Re: [PATCH 2/4] peci: Add peci-npcm controller driver

[Paul Menzel]

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.

>> 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 <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

>> […]
>>
>>> +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

Re: [PATCH 2/4] peci: Add peci-npcm controller driver

[Winiarska, Iwona]

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

Re: [PATCH 2/4] peci: Add peci-npcm controller driver

[Paul Menzel]

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 <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?

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 <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

[…]

>>>>> +#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

Re: [PATCH 2/4] peci: Add peci-npcm controller driver

[Winiarska, Iwona]

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 <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?
> 
> 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 <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
> 
> […]
> 
> > > > > > +#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

Re: [PATCH 2/4] peci: Add peci-npcm controller driver

[Tomer Maimon]

Hi Paul,

Thanks for your comments.

On Fri, 21 Jul 2023 at 12:22, Winiarska, Iwona
<iwona.winiarska@intel.com> 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 <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?
> >
> > 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 <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
> >
> > […]
> >
> > > > > > > +#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

[PATCH 3/4] ARM: dts: nuvoton: Add PECI controller node

[Iwona Winiarska]

Add PECI controller node with all required information.

Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
---
 arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
index c7b5ef15b716..cccc33441050 100644
--- a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
+++ b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
@@ -220,6 +220,15 @@ kcs3: kcs3@0 {
 				};
 			};
 
+			peci0: peci-controller@f0100000 {
+				compatible = "nuvoton,npcm750-peci";
+				reg = <0xf0100000 0x200>;
+				interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk NPCM7XX_CLK_APB3>;
+				cmd-timeout-ms = <1000>;
+				status = "disabled";
+			};
+
 			spi0: spi@200000 {
 				compatible = "nuvoton,npcm750-pspi";
 				reg = <0x200000 0x1000>;
-- 
2.40.1

[PATCH 4/4] arm64: dts: nuvoton: Add PECI controller node

[Iwona Winiarska]

Add PECI controller node with all required information.

Signed-off-by: Iwona Winiarska <iwona.winiarska@intel.com>
---
 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
index aa7aac8c3774..b8326bbe9fde 100644
--- a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
+++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi
@@ -68,6 +68,15 @@ apb {
 			ranges = <0x0 0x0 0xf0000000 0x00300000>,
 				<0xfff00000 0x0 0xfff00000 0x00016000>;
 
+			peci0: peci-controller@100000 {
+				compatible = "nuvoton,npcm845-peci";
+				reg = <0x100000 0x1000>;
+				interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk NPCM8XX_CLK_APB3>;
+				cmd-timeout-ms = <1000>;
+				status = "disabled";
+			};
+
 			timer0: timer@8000 {
 				compatible = "nuvoton,npcm845-timer";
 				interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
-- 
2.40.1

Re: [PATCH 0/4] Add support for PECI Nuvoton

[Krzysztof Kozlowski]

On 20/07/2023 00:08, Iwona Winiarska wrote:
> Hi!
> 
> The series adds support for PECI on Nuvoton-based BMC boards.
> It is based on patches that were sent by Tomer Maimon from
> Nuvoton [1].
> Similar to Aspeed driver, unused (as in, default values were used in
> all of the available DTS files) vendor-specific properties were
> removed.
> If there is a use-case for such properties, they can be added in
> a separate series.
> 
> Thank you Tomer for testing this series on Nuvoton hardware.
> 
> Thanks
> -Iwona
> 
> [1] https://lore.kernel.org/openbmc/CAP6Zq1jnbQ8k9VEyf9WgVq5DRrEzf5V6kaYP30S7g9BV9jKtaQ@mail.gmail.com/
> 

This is not v1 but v3 or v4. Please provide proper changelog and versioning.

Best regards,
Krzysztof

Re: [PATCH 0/4] Add support for PECI Nuvoton

[Winiarska, Iwona]

On Thu, 2023-07-20 at 08:17 +0200, Krzysztof Kozlowski wrote:
> On 20/07/2023 00:08, Iwona Winiarska wrote:
> > Hi!
> > 
> > The series adds support for PECI on Nuvoton-based BMC boards.
> > It is based on patches that were sent by Tomer Maimon from
> > Nuvoton [1].
> > Similar to Aspeed driver, unused (as in, default values were used in
> > all of the available DTS files) vendor-specific properties were
> > removed.
> > If there is a use-case for such properties, they can be added in
> > a separate series.
> > 
> > Thank you Tomer for testing this series on Nuvoton hardware.
> > 
> > Thanks
> > -Iwona
> > 
> > [1]
> > https://lore.kernel.org/openbmc/CAP6Zq1jnbQ8k9VEyf9WgVq5DRrEzf5V6kaYP30S7g9BV9jKtaQ@mail.gmail.com/
> > 
> 
> This is not v1 but v3 or v4. Please provide proper changelog and versioning.

This is the first submission - also known as v1 :)
Could you elaborate on why do you believe that this is v3 or v4?

-Iwona

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 0/4] Add support for PECI Nuvoton

[Krzysztof Kozlowski]

On 20/07/2023 10:00, Winiarska, Iwona wrote:
> On Thu, 2023-07-20 at 08:17 +0200, Krzysztof Kozlowski wrote:
>> On 20/07/2023 00:08, Iwona Winiarska wrote:
>>> Hi!
>>>
>>> The series adds support for PECI on Nuvoton-based BMC boards.
>>> It is based on patches that were sent by Tomer Maimon from
>>> Nuvoton [1].
>>> Similar to Aspeed driver, unused (as in, default values were used in
>>> all of the available DTS files) vendor-specific properties were
>>> removed.
>>> If there is a use-case for such properties, they can be added in
>>> a separate series.
>>>
>>> Thank you Tomer for testing this series on Nuvoton hardware.
>>>
>>> Thanks
>>> -Iwona
>>>
>>> [1]
>>> https://lore.kernel.org/openbmc/CAP6Zq1jnbQ8k9VEyf9WgVq5DRrEzf5V6kaYP30S7g9BV9jKtaQ@mail.gmail.com/
>>>
>>
>> This is not v1 but v3 or v4. Please provide proper changelog and versioning.
> 
> This is the first submission - also known as v1 :)
> Could you elaborate on why do you believe that this is v3 or v4?

I had such impression because I saw it:
https://lore.kernel.org/all/20230616193450.413366-2-iwona.winiarska@intel.com/
https://lore.kernel.org/all/20230628090404.234965-2-tmaimon77@gmail.com/

Best regards,
Krzysztof

Re: [PATCH 0/4] Add support for PECI Nuvoton

[Winiarska, Iwona]

On Thu, 2023-07-20 at 10:40 +0200, Krzysztof Kozlowski wrote:
> On 20/07/2023 10:00, Winiarska, Iwona wrote:
> > On Thu, 2023-07-20 at 08:17 +0200, Krzysztof Kozlowski wrote:
> > > On 20/07/2023 00:08, Iwona Winiarska wrote:
> > > > Hi!
> > > > 
> > > > The series adds support for PECI on Nuvoton-based BMC boards.
> > > > It is based on patches that were sent by Tomer Maimon from
> > > > Nuvoton [1].
> > > > Similar to Aspeed driver, unused (as in, default values were used in
> > > > all of the available DTS files) vendor-specific properties were
> > > > removed.
> > > > If there is a use-case for such properties, they can be added in
> > > > a separate series.
> > > > 
> > > > Thank you Tomer for testing this series on Nuvoton hardware.
> > > > 
> > > > Thanks
> > > > -Iwona
> > > > 
> > > > [1]
> > > > https://lore.kernel.org/openbmc/CAP6Zq1jnbQ8k9VEyf9WgVq5DRrEzf5V6kaYP30S7g9BV9jKtaQ@mail.gmail.com/
> > > > 
> > > 
> > > This is not v1 but v3 or v4. Please provide proper changelog and
> > > versioning.
> > 
> > This is the first submission - also known as v1 :)
> > Could you elaborate on why do you believe that this is v3 or v4?
> 
> I had such impression because I saw it:
> https://lore.kernel.org/all/20230616193450.413366-2-iwona.winiarska@intel.com/
> https://lore.kernel.org/all/20230628090404.234965-2-tmaimon77@gmail.com/

Changelog / versioning is maintained for changes that are submitted for
inclusion in upstream Linux.
The series you're referring to are not upstream Linux submissions.
Additionally - there were no changes :)

-Iwona

> 
> Best regards,
> Krzysztof
>