Re: [PATCH v1 2/6] peci: controller: Add StarFive JHB100 PECI driver
From: Changhuang Liang
Date: Thu Sep 17 2026 - 23:04:32 EST
Hi, Iwona
Thanks for the review.
> On Thu, 2026-09-03 at 06:34 -0700, Changhuang Liang wrote:
> > Add PECI controller driver for StarFive JHB100 SoC. The driver
> > supports PECI protocol communication for CPU thermal management.
> >
> > For this controller, the special clock and reset operation sequence is:
> > probe: clk_prepare_enable() then reset_control_deassert()
> > remove: clk_disable_unprepare() then reset_control_assert()
> >
> > Co-developed-by: Mason Huo <mason.huo@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Mason Huo <mason.huo@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > ---
> > MAINTAINERS | 8 +
> > drivers/peci/controller/Kconfig | 17 +
> > drivers/peci/controller/Makefile | 1 +
> > drivers/peci/controller/peci-starfive.c | 405
> > ++++++++++++++++++++++++
> > 4 files changed, 431 insertions(+)
> > create mode 100644 drivers/peci/controller/peci-starfive.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 834f88b7a41b..a8d7ece2d199 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -26224,6 +26224,14 @@ S: Supported
> > F: Documentation/devicetree/bindings/interrupt-
> > controller/starfive,jhb100-intc.yaml
> > F: drivers/irqchip/irq-starfive-jhb100-intc.c
> >
> > +STARFIVE JHB100 PECI DRIVER
> > +M: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > +M: Mason Huo <mason.huo@xxxxxxxxxxxxxxxx>
> > +L: openbmc@xxxxxxxxxxxxxxxx (moderated for non-subscribers)
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/peci/starfive,jhb100-peci.yaml
> > +F: drivers/peci/controller/peci-starfive.c
> > +
> > STARFIVE JHB100 PINCTRL DRIVERS
> > M: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > M: Lianfeng Ouyang <lianfeng.ouyang@xxxxxxxxxxxxxxxx>
> > diff --git a/drivers/peci/controller/Kconfig
> > b/drivers/peci/controller/Kconfig index 4f9c245ad042..c0c35bc179ef
> > 100644
> > --- a/drivers/peci/controller/Kconfig
> > +++ b/drivers/peci/controller/Kconfig
> > @@ -32,3 +32,20 @@ config PECI_NPCM
> >
> > This support is also available as a module. If so, the module
> > will be called peci-npcm.
> > +
> > +config PECI_STARFIVE
> > + tristate "STARFIVE PECI support"
> > + depends on ARCH_STARFIVE || COMPILE_TEST
> > + depends on OF
> > + depends on HAS_IOMEM
> > + select REGMAP_MMIO
> > + help
> > + This option enables PECI controller driver for StarFive JHB100
> > + SoC. It allows BMC to discover devices connected to it, and
> > + communicate with them using PECI protocol.
> > +
> > + Say Y here if your system runs on StarFive JHB100 SoC and you are
> > + using it as BMC for Intel platform.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called peci-starfive.
> > diff --git a/drivers/peci/controller/Makefile
> > b/drivers/peci/controller/Makefile
> > index e247449bb423..935e356b058c 100644
> > --- a/drivers/peci/controller/Makefile
> > +++ b/drivers/peci/controller/Makefile
> > @@ -2,3 +2,4 @@
> >
> > obj-$(CONFIG_PECI_ASPEED) += peci-aspeed.o
> > obj-$(CONFIG_PECI_NPCM) += peci-npcm.o
> > +obj-$(CONFIG_PECI_STARFIVE) += peci-starfive.o
> > diff --git a/drivers/peci/controller/peci-starfive.c
> > b/drivers/peci/controller/peci-starfive.c
> > new file mode 100644
> > index 000000000000..54a87ebae009
> > --- /dev/null
> > +++ b/drivers/peci/controller/peci-starfive.c
> > @@ -0,0 +1,405 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2024 StarFive Technology Co., Ltd.
> > + */
> > +
> > +#include <linux/unaligned.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/minmax.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>
> > +
> > +/* Control register */
> > +#define STARFIVE_PECI_CTRL 0x00
> > +/* automatically clears after transfer started */
> > +#define STARFIVE_PECI_CTRL_START BIT(0)
> > +#define STARFIVE_PECI_CTRL_FCS_MODE BIT(2)
> > +#define STARFIVE_PECI_CTRL_AW_FCS_EN BIT(3)
> > +#define STARFIVE_PECI_CTRL_RDY BIT(4)
> > +
> > +/* Read/Write length register */
> > +#define STARFIVE_PECI_HDR 0x04
> > +#define STARFIVE_PECI_HDR_RD_LEN_MASK GENMASK(23, 16)
> > +#define STARFIVE_PECI_HDR_WR_LEN_MASK GENMASK(15, 8)
> > +#define STARFIVE_PECI_HDR_TARGET_ADDR_MASK GENMASK(7,
> 0)
> > +
> > +/* Feature control register */
> > +#define STARFIVE_PECI_F_CTRL 0x0c
> > +#define STARFIVE_PECI_F_CTRL_EN BIT(0)
> > +
> > +/* Interrupt enable register */
> > +#define STARFIVE_PECI_INT_EN 0x10
> > +#define STARFIVE_PECI_INT_XFER_DONE BIT(0)
> > +#define STARFIVE_PECI_INT_CFG_ERR BIT(1)
> > +#define STARFIVE_PECI_INT_TBIT_ERR BIT(2)
> > +#define STARFIVE_PECI_INT_BAD_WR_FCS BIT(3)
> > +#define STARFIVE_PECI_INT_ABORT_WR_FCS BIT(4)
> > +#define STARFIVE_PECI_INT_BAD_RD_FCS BIT(5)
> > +#define STARFIVE_PECI_INT_BUS_CONTENTION BIT(6)
> > +#define STARFIVE_PECI_INT_TBIT_OVER_LIMIT BIT(7)
> > +#define STARFIVE_PECI_INT_ALL GENMASK(7, 0)
> > +
> > +/* Interrupt status register */
> > +#define STARFIVE_PECI_INT_STS 0x14
> > +#define STARFIVE_PECI_STS_XFER_DONE BIT(0)
> > +#define STARFIVE_PECI_STS_CFG_ERR BIT(1)
> > +#define STARFIVE_PECI_STS_TBIT_ERR BIT(2)
> > +#define STARFIVE_PECI_STS_BAD_WR_FCS BIT(3)
> > +#define STARFIVE_PECI_STS_ABORT_WR_FCS BIT(4)
> > +#define STARFIVE_PECI_STS_BAD_RD_FCS BIT(5)
> > +#define STARFIVE_PECI_STS_BUS_CONTENTION BIT(6)
> > +#define STARFIVE_PECI_STS_TBIT_OVER_LIMIT BIT(7)
> > +#define STARFIVE_PECI_STS_MASK GENMASK(7, 0)
> > +
> > +/* Received FCS data register */
> > +#define STARFIVE_PECI_FCS_RCVD 0x1C
> > +#define STARFIVE_PECI_RCVD_WR_FCS_MASK GENMASK(7, 0)
> > +#define STARFIVE_PECI_RCVD_RD_FCS_MASK GENMASK(15, 8)
> > +
> > +/* Rx/Tx Data Buffer Registers */
> > +#define STARFIVE_PECI_WR_DATA(n) (0x70 + ((n) * 4))
> > +#define STARFIVE_PECI_RD_DATA(n) (0xB0 + ((n) * 4))
> > +
> > +/* Hardware TX/RX data FIFOs are 64 bytes, but PECI core caps
> > +requests lower
> > */
> > +#define STARFIVE_PECI_DATA_BUF_SIZE_MAX min(64,
> > PECI_REQUEST_MAX_BUF_SIZE)
> > +#define STARFIVE_PECI_MAX_REG 0x100
> > +
> > +/* Timeout */
> > +#define STARFIVE_PECI_IDLE_CHECK_TIMEOUT_US (50 *
> USEC_PER_MSEC)
> > +#define STARFIVE_PECI_IDLE_CHECK_INTERVAL_US (10 *
> USEC_PER_MSEC)
> > +#define STARFIVE_PECI_CMD_TIMEOUT_MS_DEFAULT 1000
> > +#define STARFIVE_PECI_CMD_TIMEOUT_MS_MAX 1000
> > +
> > +/*
> > + * All PECI write commands (WrPkgConfig 0xa5, WrPCIConfigLocal 0xe5,
> > + * WrEndPointConfig 0xc5, ...) share the same low nibble, which is
> > +what the
> > + * controller uses to decide whether the assured-write FCS has to be
> > appended.
> > + */
> > +#define STARFIVE_PECI_CMD_WRITE_NIBBLE 0x5
> > +
> > +struct starfive_peci {
> > + u32 cmd_timeout_ms;
> > + struct completion xfer_complete;
> > + struct regmap *regmap;
> > + u32 status;
> > + spinlock_t lock; /* sync completion status */
> > + struct peci_controller *controller;
> > + struct device *dev;
> > + struct clk *clk;
> > + struct reset_control *rst;
> > + int irq;
> > +};
> > +
> > +static int starfive_peci_xfer(struct peci_controller *controller,
> > + u8 addr, struct peci_request *req) {
> > + struct starfive_peci *priv = dev_get_drvdata(controller->dev.parent);
> > + unsigned long timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
> > + u32 msg_rd;
> > + u32 cmd_sts;
> > + u32 peci_hdr;
> > + int i, ret, j;
> > +
> > + if (req->tx.len > STARFIVE_PECI_DATA_BUF_SIZE_MAX ||
> > + req->rx.len > STARFIVE_PECI_DATA_BUF_SIZE_MAX)
> > + return -EINVAL;
> > +
> > + /* Check command sts and bus idle state */
> > + ret = regmap_read_poll_timeout(priv->regmap,
> > + STARFIVE_PECI_CTRL, cmd_sts,
> > + cmd_sts & STARFIVE_PECI_CTRL_RDY,
> > + STARFIVE_PECI_IDLE_CHECK_INTERVAL_US,
> > + STARFIVE_PECI_IDLE_CHECK_TIMEOUT_US);
> > + if (ret)
> > + return ret;
> > +
> > + spin_lock_irq(&priv->lock);
> > + reinit_completion(&priv->xfer_complete);
> > +
> > + peci_hdr = FIELD_PREP(STARFIVE_PECI_HDR_TARGET_ADDR_MASK,
> addr) |
> > + FIELD_PREP(STARFIVE_PECI_HDR_WR_LEN_MASK, req->tx.len)
> |
> > + FIELD_PREP(STARFIVE_PECI_HDR_RD_LEN_MASK, req->rx.len);
> > + regmap_write(priv->regmap, STARFIVE_PECI_HDR, peci_hdr);
> > +
> > + if (req->tx.len) {
> > + /*
> > + * req->tx.buf[0] always store the command code.
> > + * Use command code set different configuration.
> > + */
> > + u8 cmd_nibble = FIELD_GET(GENMASK(3, 0), req->tx.buf[0]);
> > +
> > + if (cmd_nibble == STARFIVE_PECI_CMD_WRITE_NIBBLE) {
> > + /*
> > + * This indicates current command code is write.
> > + * Only write command should enable has_awfcs.
> > + */
>
> Technically, we don't have support for any write commands at this point in the
> tree. Are you planning to add the usage for write commands in the near
> future?
I'm not quite sure either. I previously adapted libpeci based on the current tree, and
tested the write commands. So I kept the write commands branch here. I'm not quite
sure why libpeci wasn't adapted here—is it because there wasn't time to push this
part forward?
>
> > + regmap_write_bits(priv->regmap, STARFIVE_PECI_CTRL,
> > + STARFIVE_PECI_CTRL_AW_FCS_EN,
> > + STARFIVE_PECI_CTRL_AW_FCS_EN);
> > + } else {
> > + /* Ensure other command code disable has_awfcs. */
> > + regmap_write_bits(priv->regmap, STARFIVE_PECI_CTRL,
> > + STARFIVE_PECI_CTRL_AW_FCS_EN, 0);
> > + }
> > + } else {
> > + /* Ping command code also need to disable has_awfcs. */
> > + regmap_write_bits(priv->regmap, STARFIVE_PECI_CTRL,
> > + STARFIVE_PECI_CTRL_AW_FCS_EN, 0);
> > + }
> > +
> > + for (i = 0; i < ALIGN(req->tx.len, 4) / 4; i++)
> > + regmap_write(priv->regmap, STARFIVE_PECI_WR_DATA(i),
> > + get_unaligned_le32(&req->tx.buf[4 * i]));
> > +
> > + 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);
>
> Can we wrap the most verbose debug logs using CONFIG_DYNAMIC_DEBUG?
>
print_hex_dump_bytes itself only works when CONFIG_DYNAMIC_DEBUG is
enabled, so we shouldn't need to explicitly add this condition, right?
Best Regards,
Changhuang