Message-ID: <8ba4963-cd39-4ab7-dd28-65434264443@linux.intel.com>
Date:   Fri, 13 Jan 2023 17:08:15 +0200 (EET)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Lee Jones <lee@...nel.org>
cc:     linux-fpga@...r.kernel.org, Xu Yilun <yilun.xu@...el.com>,
        Wu Hao <hao.wu@...el.com>, Tom Rix <trix@...hat.com>,
        Moritz Fischer <mdf@...nel.org>,
        Matthew Gerlach <matthew.gerlach@...ux.intel.com>,
        Russ Weight <russell.h.weight@...el.com>,
        Tianfei zhang <tianfei.zhang@...el.com>,
        Mark Brown <broonie@...nel.org>,
        Marco Pagani <marpagan@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 09/10] mfd: intel-m10-bmc: Add PMCI driver

On Fri, 13 Jan 2023, Lee Jones wrote:

> On Mon, 26 Dec 2022, Ilpo Järvinen wrote:
> 
> > Add the mfd driver for the Platform Management Component Interface
> > (PMCI) based interface of Intel MAX10 BMC controller.
> > 
> > PMCI is a software-visible interface, connected to card BMC which
> > provided the basic functionality of read/write BMC register. The access
> > to the register is done indirectly via a hardware controller/bridge
> > that handles read/write/clear commands and acknowledgments for the
> > commands.
> > 
> > Previously, intel-m10-bmc provided sysfs under
> > /sys/bus/spi/devices/... which is generalized in this change because
> > not all MAX10 BMC appear under SPI anymore.
> > 
> > Co-developed-by: Tianfei zhang <tianfei.zhang@...el.com>
> > Signed-off-by: Tianfei zhang <tianfei.zhang@...el.com>
> > Co-developed-by: Russ Weight <russell.h.weight@...el.com>
> > Signed-off-by: Russ Weight <russell.h.weight@...el.com>
> > Co-developed-by: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
> > Reviewed-by: Xu Yilun <yilun.xu@...el.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > ---
> >  .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
> >  drivers/mfd/Kconfig                           |  12 +
> >  drivers/mfd/Makefile                          |   1 +
> >  drivers/mfd/intel-m10-bmc-pmci.c              | 253 ++++++++++++++++++
> >  4 files changed, 270 insertions(+), 4 deletions(-)
> >  create mode 100644 drivers/mfd/intel-m10-bmc-pmci.c
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> > index 9773925138af..a8ab58035c95 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> > +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> > @@ -1,4 +1,4 @@
> > -What:		/sys/bus/spi/devices/.../bmc_version
> > +What:		/sys/bus/.../drivers/intel-m10-bmc/.../bmc_version
> >  Date:		June 2020
> >  KernelVersion:	5.10
> >  Contact:	Xu Yilun <yilun.xu@...el.com>
> > @@ -6,7 +6,7 @@ Description:	Read only. Returns the hardware build version of Intel
> >  		MAX10 BMC chip.
> >  		Format: "0x%x".
> >  
> > -What:		/sys/bus/spi/devices/.../bmcfw_version
> > +What:		/sys/bus/.../drivers/intel-m10-bmc/.../bmcfw_version
> >  Date:		June 2020
> >  KernelVersion:	5.10
> >  Contact:	Xu Yilun <yilun.xu@...el.com>
> > @@ -14,7 +14,7 @@ Description:	Read only. Returns the firmware version of Intel MAX10
> >  		BMC chip.
> >  		Format: "0x%x".
> >  
> > -What:		/sys/bus/spi/devices/.../mac_address
> > +What:		/sys/bus/.../drivers/intel-m10-bmc/.../mac_address
> >  Date:		January 2021
> >  KernelVersion:  5.12
> >  Contact:	Russ Weight <russell.h.weight@...el.com>
> > @@ -25,7 +25,7 @@ Description:	Read only. Returns the first MAC address in a block
> >  		space.
> >  		Format: "%02x:%02x:%02x:%02x:%02x:%02x".
> >  
> > -What:		/sys/bus/spi/devices/.../mac_count
> > +What:		/sys/bus/.../drivers/intel-m10-bmc/.../mac_count
> >  Date:		January 2021
> >  KernelVersion:  5.12
> >  Contact:	Russ Weight <russell.h.weight@...el.com>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index a09d4ac60dc7..82f13614d98a 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2238,6 +2238,18 @@ config MFD_INTEL_M10_BMC_SPI
> >            additional drivers must be enabled in order to use the functionality
> >            of the device.
> >  
> > +config MFD_INTEL_M10_BMC_PMCI
> > +	tristate "Intel MAX 10 Board Management Controller with PMCI"
> > +	depends on FPGA_DFL
> > +	select MFD_INTEL_M10_BMC_CORE
> > +	select REGMAP
> > +	help
> > +	  Support for the Intel MAX 10 board management controller via PMCI.
> > +
> > +	  This driver provides common support for accessing the device,
> > +	  additional drivers must be enabled in order to use the functionality
> > +	  of the device.
> > +
> >  config MFD_RSMU_I2C
> >  	tristate "Renesas Synchronization Management Unit with I2C"
> >  	depends on I2C && OF
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 5d1f308ee2a7..c90fb96cad2a 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -274,6 +274,7 @@ obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
> >  
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)	+= intel-m10-bmc-core.o
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)	+= intel-m10-bmc-spi.o
> > +obj-$(CONFIG_MFD_INTEL_M10_BMC_PMCI)	+= intel-m10-bmc-pmci.o
> >  
> >  obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
> >  obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
> > diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
> > new file mode 100644
> > index 000000000000..63ec0f6aba8b
> > --- /dev/null
> > +++ b/drivers/mfd/intel-m10-bmc-pmci.c
> > @@ -0,0 +1,253 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * MFD driver for Platform Management Component Interface (PMCI) based
> 
> Please remove any mention of MFD.
> 
> This includes the MODULE_*() macros in all patches please.
> 
> Better to use terms like 'core' instead.
> 
> > + * interface to MAX10 BMC.
> > + *
> > + * Copyright (C) 2020-2022 Intel Corporation.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/dfl.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/intel-m10-bmc.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#define M10BMC_PMCI_INDIRECT_BASE	0x400
> > +
> > +#define M10BMC_N6000_SYS_BASE		0x0
> > +#define M10BMC_N6000_SYS_END		0xfff
> > +
> > +#define M10BMC_N6000_DOORBELL		0x1c0
> > +#define M10BMC_N6000_AUTH_RESULT	0x1c4
> > +
> > +/* Telemetry registers */
> > +#define M10BMC_N6000_TELEM_START	0x400
> > +#define M10BMC_N6000_TELEM_END		0x78c
> > +
> > +#define M10BMC_N6000_BUILD_VER		0x0
> > +#define NIOS2_N6000_FW_VERSION		0x4
> > +#define M10BMC_N6000_MAC_LOW		0x20
> > +#define M10BMC_N6000_MAC_HIGH		(M10BMC_N6000_MAC_LOW + 4)
> > +
> > +/* Addresses for security related data in FLASH */
> > +#define M10BMC_N6000_BMC_REH_ADDR	0x7ffc004
> > +#define M10BMC_N6000_BMC_PROG_ADDR	0x7ffc000
> > +#define M10BMC_N6000_BMC_PROG_MAGIC	0x5746
> > +
> > +#define M10BMC_N6000_SR_REH_ADDR	0x7ffd004
> > +#define M10BMC_N6000_SR_PROG_ADDR	0x7ffd000
> > +#define M10BMC_N6000_SR_PROG_MAGIC	0x5253
> > +
> > +#define M10BMC_N6000_PR_REH_ADDR	0x7ffe004
> > +#define M10BMC_N6000_PR_PROG_ADDR	0x7ffe000
> > +#define M10BMC_N6000_PR_PROG_MAGIC	0x5250
> > +
> > +#define M10BMC_N6000_STAGING_FLASH_COUNT	0x7ff5000
> > +
> > +struct m10bmc_pmci_device {
> > +	void __iomem *base;
> > +	struct intel_m10bmc m10bmc;
> > +};
> > +
> > +/*
> > + * Intel FGPA indirect register access via hardware controller/bridge.
> > + */
> > +#define INDIRECT_CMD_OFF	0
> > +#define INDIRECT_CMD_CLR	0
> > +#define INDIRECT_CMD_RD		BIT(0)
> > +#define INDIRECT_CMD_WR		BIT(1)
> > +#define INDIRECT_CMD_ACK	BIT(2)
> > +
> > +#define INDIRECT_ADDR_OFF	0x4
> > +#define INDIRECT_RD_OFF		0x8
> > +#define INDIRECT_WR_OFF		0xc
> > +
> > +#define INDIRECT_INT_US		1
> > +#define INDIRECT_TIMEOUT_US	10000
> > +
> > +struct indirect_ctx {
> > +        void __iomem *base;
> > +        struct device *dev;
> > +};
> > +
> > +static int indirect_clear_cmd(struct indirect_ctx *ctx)
> > +{
> > +	unsigned int cmd;
> > +	int ret;
> > +
> > +	writel(INDIRECT_CMD_CLR, ctx->base + INDIRECT_CMD_OFF);
> > +
> > +	ret = readl_poll_timeout(ctx->base + INDIRECT_CMD_OFF, cmd,
> > +				 cmd == INDIRECT_CMD_CLR,
> > +				 INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
> > +	if (ret)
> > +		dev_err(ctx->dev, "timed out waiting clear cmd (residual cmd=0x%x)\n", cmd);
> > +
> > +	return ret;
> > +}
> > +
> > +static int indirect_reg_read(void *context, unsigned int reg, unsigned int *val)
> > +{
> > +	struct indirect_ctx *ctx = context;
> > +	unsigned int cmd, ack, tmpval;
> > +	int ret;
> > +
> > +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> > +	if (cmd != INDIRECT_CMD_CLR)
> > +		dev_warn(ctx->dev, "residual cmd 0x%x on read entry\n", cmd);
> > +
> > +	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
> > +	writel(INDIRECT_CMD_RD, ctx->base + INDIRECT_CMD_OFF);
> > +
> > +	ret = readl_poll_timeout(ctx->base + INDIRECT_CMD_OFF, ack,
> > +				 (ack & INDIRECT_CMD_ACK) == INDIRECT_CMD_ACK,
> > +				 INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
> > +	if (ret)
> > +		dev_err(ctx->dev, "read timed out on reg 0x%x ack 0x%x\n", reg, ack);
> > +	else
> > +		tmpval = readl(ctx->base + INDIRECT_RD_OFF);
> > +
> > +	if (indirect_clear_cmd(ctx)) 
> 
> What are the chances of the above readl_poll_timeout() failing, then the
> subsequent one in indirect_clear_cmd() passing?

I don't have a good answer to what the chances are but the clear command 
deals with the indirect controller/bridge so failures aren't necessarily 
1:1. At worst it just causes wait that is double of the poll timeout which 
doesn't seem that big problem.

> As I rule, I tend to dislike calling functions from inside if()
> statements.  If there is no way to work this hunk around, probably
> better to place the return value inside another variable or use another
> variable entirely and call indirect_clear_cmd() in the traditional way,
> like you do everywhere else.

I can add the second variable for that.

> > +		if (!ret)
> > +			ret = -ETIMEDOUT;
> > +		goto out;
> > +	}
> > +
> > +	*val = tmpval;
> 
> If readl_poll_timeout() fails and indirect_clear_cmd() succeeds, you'll
> return, at best, junk.

Thanks, I already fixed this problem once and now managed to reintroduce 
it myself. :-(

> > +out:
> > +	return ret;
> > +}
> > +
> > +static int indirect_reg_write(void *context, unsigned int reg, unsigned int val)
> > +{
> > +	struct indirect_ctx *ctx = context;
> > +	unsigned int cmd, ack;
> > +	int ret;
> > +
> > +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> > +	if (cmd != INDIRECT_CMD_CLR)
> > +		dev_warn(ctx->dev, "residual cmd 0x%x on write entry\n", cmd);
> > +
> > +	writel(val, ctx->base + INDIRECT_WR_OFF);
> > +	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
> > +	writel(INDIRECT_CMD_WR, ctx->base + INDIRECT_CMD_OFF);
> > +
> > +	ret = readl_poll_timeout(ctx->base + INDIRECT_CMD_OFF, ack,
> > +				 (ack & INDIRECT_CMD_ACK) == INDIRECT_CMD_ACK,
> > +				 INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
> > +	if (ret)
> > +		dev_err(ctx->dev, "write timed out on reg 0x%x ack 0x%x\n", reg, ack);
> > +
> > +	if (indirect_clear_cmd(ctx)) {
> > +		if (!ret)
> > +			ret = -ETIMEDOUT;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct regmap_range m10bmc_pmci_regmap_range[] = {
> > +	regmap_reg_range(M10BMC_N6000_SYS_BASE, M10BMC_N6000_SYS_END),
> > +};
> > +
> > +static const struct regmap_access_table m10bmc_pmci_access_table = {
> > +	.yes_ranges	= m10bmc_pmci_regmap_range,
> > +	.n_yes_ranges	= ARRAY_SIZE(m10bmc_pmci_regmap_range),
> > +};
> > +
> > +static struct regmap_config m10bmc_pmci_regmap_config = {
> > +	.reg_bits = 32,
> > +	.reg_stride = 4,
> > +	.val_bits = 32,
> > +	.wr_table = &m10bmc_pmci_access_table,
> > +	.rd_table = &m10bmc_pmci_access_table,
> > +	.reg_read = &indirect_reg_read,
> > +	.reg_write = &indirect_reg_write,
> > +	.max_register = M10BMC_N6000_SYS_END,
> > +};
> > +
> > +static struct mfd_cell m10bmc_pmci_n6000_bmc_subdevs[] = {
> > +	{ .name = "n6000bmc-hwmon" },
> > +};
> > +
> > +static const struct m10bmc_csr_map m10bmc_n6000_csr_map = {
> > +	.base = M10BMC_N6000_SYS_BASE,
> > +	.build_version = M10BMC_N6000_BUILD_VER,
> > +	.fw_version = NIOS2_N6000_FW_VERSION,
> > +	.mac_low = M10BMC_N6000_MAC_LOW,
> > +	.mac_high = M10BMC_N6000_MAC_HIGH,
> > +	.doorbell = M10BMC_N6000_DOORBELL,
> > +	.auth_result = M10BMC_N6000_AUTH_RESULT,
> > +	.rsu_status = M10BMC_N6000_AUTH_RESULT,
> > +	.bmc_prog_addr = M10BMC_N6000_BMC_PROG_ADDR,
> > +	.bmc_reh_addr = M10BMC_N6000_BMC_REH_ADDR,
> > +	.bmc_magic = M10BMC_N6000_BMC_PROG_MAGIC,
> > +	.sr_prog_addr = M10BMC_N6000_SR_PROG_ADDR,
> > +	.sr_reh_addr = M10BMC_N6000_SR_REH_ADDR,
> > +	.sr_magic = M10BMC_N6000_SR_PROG_MAGIC,
> > +	.pr_prog_addr = M10BMC_N6000_PR_PROG_ADDR,
> > +	.pr_reh_addr = M10BMC_N6000_PR_REH_ADDR,
> > +	.pr_magic = M10BMC_N6000_PR_PROG_MAGIC,
> > +	.rsu_update_counter = M10BMC_N6000_STAGING_FLASH_COUNT,
> > +};
> > +
> > +static const struct intel_m10bmc_platform_info m10bmc_pmci_n6000 = {
> > +	.cells = m10bmc_pmci_n6000_bmc_subdevs,
> > +	.n_cells = ARRAY_SIZE(m10bmc_pmci_n6000_bmc_subdevs),
> > +	.csr_map = &m10bmc_n6000_csr_map,
> > +};
> > +
> > +static int m10bmc_pmci_probe(struct dfl_device *ddev)
> > +{
> > +	struct device *dev = &ddev->dev;
> > +	struct m10bmc_pmci_device *pmci;
> > +	struct indirect_ctx *ctx;
> > +
> > +	pmci = devm_kzalloc(dev, sizeof(*pmci), GFP_KERNEL);
> > +	if (!pmci)
> > +		return -ENOMEM;
> > +
> > +	pmci->m10bmc.dev = dev;
> > +
> > +	pmci->base = devm_ioremap_resource(dev, &ddev->mmio_res);
> > +	if (IS_ERR(pmci->base))
> > +		return PTR_ERR(pmci->base);
> > +
> > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	ctx->base = pmci->base + M10BMC_PMCI_INDIRECT_BASE;
> > +	ctx->dev = dev;
> 
> Why do we need to cache dev twice in two different structures? 

This indirect register access thing is kinda different from this PMCI 
driver. It's a generic mechanism to access registers in Intel FPGAs. The
indirect register access code used to be separately among regmap code in 
the earlier versions of this series but since PMCI driver is currently the 
only user and lacks in the genericness of the indirect code, it eventually 
got included into this PMCI driver file.

Do you prefer I use m10bmc_pmci_device instead?

-- 
 i.

Powered by blists - more mailing lists