Re: [PATCH v1 2/6] peci: controller: Add StarFive JHB100 PECI driver

["Winiarska, Iwona" <iwona.winiarska@intel.com>]

On Fri, 2026-09-18 at 02:30 +0000, Changhuang Liang wrote:
> 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@starfivetech.com>
> > > Signed-off-by: Mason Huo <mason.huo@starfivetech.com>
> > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> > > ---
> > >  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@starfivetech.com>
> > > +M:	Mason Huo <mason.huo@starfivetech.com>
> > > +L:	openbmc@lists.ozlabs.org (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@starfivetech.com>
> > >  M:	Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
> > > 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?

No, we don't want to add a uAPI that exposes raw PECI command access.
If we need some functionality that could be implemented using PECI, we should
consider adding a dedicated driver for that and then define uAPI for that case
(please see hwmon drivers).

I think it would be better to skip this part for now, and reintroduce it in all
drivers across tree, when it is needed for write support.

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

It makes sense.

Thanks
-Iwona

> 
> Best Regards,
> Changhuang