[v4,2/2] mfd: intel-m10-bmc: add Max10 BMC chip support for Intel FPGA PAC
diff mbox series

Message ID 1597822497-25107-3-git-send-email-yilun.xu@intel.com
State In Next
Commit 53be8bbc2f4058d4a6bfff3dadf34164bcaafa87
Headers show
Series
  • add regmap-spi-avmm & Intel Max10 BMC chip support
Related show

Commit Message

Xu Yilun Aug. 19, 2020, 7:34 a.m. UTC
This patch implements the basic functions of the BMC chip for some Intel
FPGA PCIe Acceleration Cards (PAC). The BMC is implemented using the
intel max10 CPLD.

This BMC chip is connected to FPGA by a SPI bus. To provide direct
register access from FPGA, the "SPI slave to Avalon Master Bridge"
(spi-avmm) IP is integrated in the chip. It converts encoded streams of
bytes from the host to the internal register read/write on Avalon bus.
So This driver uses the regmap-spi-avmm for register accessing.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v2: Split out the regmap-spi-avmm part.
    Rename the file intel-m10-bmc-main.c to intel-m10-bmc.c, cause
     there is only one source file left for this module now.
v3: add the sub devices in mfd_cell.
    some minor fixes.
v4: no change.
---
 .../ABI/testing/sysfs-driver-intel-m10-bmc         |  15 ++
 drivers/mfd/Kconfig                                |  13 ++
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/intel-m10-bmc.c                        | 169 +++++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h                  |  57 +++++++
 5 files changed, 256 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
 create mode 100644 drivers/mfd/intel-m10-bmc.c
 create mode 100644 include/linux/mfd/intel-m10-bmc.h

Comments

Lee Jones Aug. 28, 2020, 10:02 a.m. UTC | #1
On Wed, 19 Aug 2020, Xu Yilun wrote:

> This patch implements the basic functions of the BMC chip for some Intel
> FPGA PCIe Acceleration Cards (PAC). The BMC is implemented using the
> intel max10 CPLD.

Nit: "Intel MAX 10"

> This BMC chip is connected to FPGA by a SPI bus. To provide direct

Nit: "to *the* FPGA"

> register access from FPGA, the "SPI slave to Avalon Master Bridge"

Nit: "from *the* FPGA"

> (spi-avmm) IP is integrated in the chip. It converts encoded streams of
> bytes from the host to the internal register read/write on Avalon bus.

Nit: "on *the* Avalon bus"

> So This driver uses the regmap-spi-avmm for register accessing.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v2: Split out the regmap-spi-avmm part.
>     Rename the file intel-m10-bmc-main.c to intel-m10-bmc.c, cause
>      there is only one source file left for this module now.
> v3: add the sub devices in mfd_cell.
>     some minor fixes.
> v4: no change.
> ---
>  .../ABI/testing/sysfs-driver-intel-m10-bmc         |  15 ++
>  drivers/mfd/Kconfig                                |  13 ++
>  drivers/mfd/Makefile                               |   2 +
>  drivers/mfd/intel-m10-bmc.c                        | 169 +++++++++++++++++++++
>  include/linux/mfd/intel-m10-bmc.h                  |  57 +++++++
>  5 files changed, 256 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
>  create mode 100644 drivers/mfd/intel-m10-bmc.c
>  create mode 100644 include/linux/mfd/intel-m10-bmc.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> new file mode 100644
> index 0000000..979a2d6
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> @@ -0,0 +1,15 @@
> +What:		/sys/bus/spi/devices/.../bmc_version
> +Date:		June 2020
> +KernelVersion:	5.10
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the hardware build version of Intel
> +		MAX10 BMC chip.
> +		Format: "0x%x".
> +
> +What:		/sys/bus/spi/devices/.../bmcfw_version
> +Date:		June 2020
> +KernelVersion:	5.10
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read only. Returns the firmware version of Intel MAX10
> +		BMC chip.
> +		Format: "0x%x".
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df083..57745f5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2118,5 +2118,18 @@ config SGI_MFD_IOC3
>  	  If you have an SGI Origin, Octane, or a PCI IOC3 card,
>  	  then say Y. Otherwise say N.
>  
> +config MFD_INTEL_M10_BMC
> +	tristate "Intel MAX10 Board Management Controller"
> +	depends on SPI_MASTER
> +	select REGMAP_SPI_AVMM
> +	select MFD_CORE
> +	help
> +	  Support for the Intel MAX10 board management controller using the
> +	  SPI interface.
> +
> +	  This driver provides common support for accessing the device,
> +	  additional drivers must be enabled in order to use the functionality
> +	  of the device.
> +
>  endmenu
>  endif
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a60e5f8..dd2cc7b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -264,3 +264,5 @@ obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
>  obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  
>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
> +
> +obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> new file mode 100644
> index 0000000..0dfd73a
> --- /dev/null
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Max10 Board Management Controller chip

"Intel MAX 10"

> + * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> + *

Remove this line.

> + */
> +#include <linux/bitfield.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>

Alphabetical.

> +enum m10bmc_type {
> +	M10_N3000,
> +};

Seems over-kill.  Will there be others?

> +static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
> +	{
> +		.name = "n3000bmc-hwmon",
> +	},
> +	{
> +		.name = "n3000bmc-pkvl",
> +	},
> +	{
> +		.name = "m10bmc-secure",
> +	},

Each entry on one line please.

> +

Remove this line.

> +};
> +
> +static struct regmap_config intel_m10bmc_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = M10BMC_MEM_END,
> +};
> +
> +static ssize_t bmc_version_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{

Does this line up to the '(' in code?

> +	struct intel_m10bmc *m10bmc = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = m10bmc_sys_read(m10bmc, M10BMC_BUILD_VER, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "0x%x\n", val);

Is this safe?  Have you considered snprintf()?

> +}
> +static DEVICE_ATTR_RO(bmc_version);
> +
> +static ssize_t bmcfw_version_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct intel_m10bmc *max10 = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = m10bmc_sys_read(max10, NIOS2_FW_VERSION, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "0x%x\n", val);

As above.

> +}
> +static DEVICE_ATTR_RO(bmcfw_version);
> +
> +static struct attribute *m10bmc_attrs[] = {
> +	&dev_attr_bmc_version.attr,
> +	&dev_attr_bmcfw_version.attr,
> +	NULL,
> +};

Can this be const?

> +static struct attribute_group m10bmc_attr_group = {
> +	.attrs = m10bmc_attrs,
> +};

Can this be const?

> +static const struct attribute_group *m10bmc_dev_groups[] = {
> +	&m10bmc_attr_group,
> +	NULL

Comma (like above).

> +};
> +
> +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> +{
> +	unsigned int v;
> +
> +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> +			    &v))
> +		return -ENODEV;

Please break functions out of if statements.

Does m10bmc_raw_read() return 0 on success?

Seems odd for a read function.

> +	if (v != 0xffffffff) {
> +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> +		return -ENODEV;
> +	}

The only acceptable version is -1?

> +	return 0;
> +}
> +
> +static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	struct device *dev = &spi->dev;
> +	struct mfd_cell *cells;
> +	struct intel_m10bmc *m10bmc;

Prefer the generic 'ddata'.

> +	int ret, n_cell;
> +
> +	m10bmc = devm_kzalloc(dev, sizeof(*m10bmc), GFP_KERNEL);
> +	if (!m10bmc)
> +		return -ENOMEM;
> +
> +	m10bmc->dev = dev;
> +
> +	m10bmc->regmap =
> +		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
> +	if (IS_ERR(m10bmc->regmap)) {
> +		ret = PTR_ERR(m10bmc->regmap);
> +		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	spi_set_drvdata(spi, m10bmc);
> +
> +	ret = check_m10bmc_version(m10bmc);
> +	if (ret) {
> +		dev_err(dev, "Failed to identify m10bmc hardware\n");
> +		return ret;
> +	}
> +
> +	switch (id->driver_data) {
> +	case M10_N3000:
> +		cells = m10bmc_pacn3000_subdevs;
> +		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> +		break;
> +	default:
> +		return -ENODEV;
> +	}

Will there be other versions?

> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> +				   NULL, 0, NULL);
> +	if (ret)
> +		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static const struct spi_device_id m10bmc_spi_id[] = {
> +	{ "m10-n3000", M10_N3000 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> +
> +static struct spi_driver intel_m10bmc_spi_driver = {
> +	.driver = {
> +		.name = "intel-m10-bmc",
> +		.dev_groups = m10bmc_dev_groups,
> +	},
> +	.probe = intel_m10_bmc_spi_probe,
> +	.id_table = m10bmc_spi_id,
> +};
> +

Remove this line.

> +module_spi_driver(intel_m10bmc_spi_driver);
> +
> +MODULE_DESCRIPTION("Intel Max10 BMC Device Driver");

"Intel MAX 10"

> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:intel-m10-bmc");
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> new file mode 100644
> index 0000000..4979756
> --- /dev/null
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Driver Header File for Intel Max10 Board Management Controller chip.

Please drop the "Driver Header File for" part.

> + * Copyright (C) 2018-2020 Intel Corporation, Inc.
> + *

Remove this line.

> + */
> +#ifndef __INTEL_M10_BMC_H
> +#define __INTEL_M10_BMC_H

"__MFD_INTEL..."

> +#include <linux/regmap.h>
> +
> +#define M10BMC_LEGACY_SYS_BASE	0x300400
> +#define M10BMC_SYS_BASE		0x300800
> +#define M10BMC_MEM_END		0x200000fc
> +
> +/* Register offset of system registers */
> +#define NIOS2_FW_VERSION	0x0
> +#define M10BMC_TEST_REG		0x3c
> +#define M10BMC_BUILD_VER	0x68
> +#define M10BMC_VER_MAJOR_MSK	GENMASK(23, 16)
> +#define M10BMC_VER_PCB_INFO_MSK	GENMASK(31, 24)
> +
> +/**
> + * struct intel_m10bmc - Intel Max10 BMC MFD device private data structure

"Intel MAX 10 BMC parent driver data structure"

> + * @dev: this device
> + * @regmap: the regmap used to access registers by m10bmc itself
> + */
> +struct intel_m10bmc {
> +	struct device *dev;
> +	struct regmap *regmap;
> +};
> +
> +/*
> + * register access helper functions.
> + *
> + * m10bmc_raw_read - read m10bmc register per addr
> + * m10bmc_sys_read - read m10bmc system register per offset
> + */
> +static inline int
> +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> +		unsigned int *val)
> +{
> +	int ret;
> +
> +	ret = regmap_read(m10bmc->regmap, addr, val);
> +	if (ret)
> +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> +			addr, ret);
> +
> +	return ret;
> +}
> +
> +#define m10bmc_sys_read(m10bmc, offset, val) \
> +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)

No unnecessary abstractions.

Just use the Regmap API directly please.

> +#endif /* __INTEL_M10_BMC_H */
Tom Rix Aug. 28, 2020, 1:50 p.m. UTC | #2
>> +
>> +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
>> +{
>> +	unsigned int v;
>> +
>> +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
>> +			    &v))
>> +		return -ENODEV;
> Please break functions out of if statements.
>
> Does m10bmc_raw_read() return 0 on success?
>
> Seems odd for a read function.
>
>> +	if (v != 0xffffffff) {
>> +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
>> +		return -ENODEV;
>> +	}
> The only acceptable version is -1?

I ran into this in testing.  This is a check if the board is using a very old legacy bmc version. The M10BMC_LEGACY_SYS_BASE is the offset to this old block of mmio regs.  On the old boards, v would have not been f's, on the current boards it is f's. The check is necessary because future calls use the M10BMC_SYS_BASE offset which was not valid on the old boards.

Tom
Xu Yilun Aug. 29, 2020, 6:24 p.m. UTC | #3
Thanks for your comments, I'll fix them. Some comments inline.

On Fri, Aug 28, 2020 at 11:02:36AM +0100, Lee Jones wrote:
> On Wed, 19 Aug 2020, Xu Yilun wrote:
> 
> 
> > +enum m10bmc_type {
> > +	M10_N3000,
> > +};
> 
> Seems over-kill.  Will there be others?

There will be another version of the BMC which support the Intel PAC
D5005. The functionalities are similar with N3000. So I defined the
device type enum here.

> > +static ssize_t bmc_version_show(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> 
> Does this line up to the '(' in code?

Yes, It does.

> 
> > +	struct intel_m10bmc *m10bmc = dev_get_drvdata(dev);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = m10bmc_sys_read(m10bmc, M10BMC_BUILD_VER, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sprintf(buf, "0x%x\n", val);
> 
> Is this safe?  Have you considered snprintf()?

It formats a 32 bits value to string, so the strlen should be 8 chars at
most. So I think it should be safe here.

I see in Documentation/filesystems/sysfs.rst:
- show() must not use snprintf() when formatting the value to be
  returned to user space. If you can guarantee that an overflow
  will never happen you can use sprintf() otherwise you must use
  scnprintf().

And seeing from this mail https://lkml.org/lkml/2019/4/25/1050
Greg is discouraging use of scnprintf for sysfs attributes where it's not
needed.

> > +}
> > +static DEVICE_ATTR_RO(bmcfw_version);
> > +
> > +static struct attribute *m10bmc_attrs[] = {
> > +	&dev_attr_bmc_version.attr,
> > +	&dev_attr_bmcfw_version.attr,
> > +	NULL,
> > +};
> 
> Can this be const?

Seems we can not const this pointer or this array.

If we const the array, static const struct attribute *m10bmc_attrs[],
then:
	error: initialization from incompatible pointer type

If we const the pointer, static struct attribute * const m10bmc_attrs[],
then:
	warning: initialization discards ‘const’ qualifier from pointer
		 target type

> 
> > +static struct attribute_group m10bmc_attr_group = {
> > +	.attrs = m10bmc_attrs,
> > +};
> 
> Can this be const?

Yes, we can const it.

> 
> > +static const struct attribute_group *m10bmc_dev_groups[] = {
> > +	&m10bmc_attr_group,
> > +	NULL
> 
> > +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> > +{
> > +	unsigned int v;
> > +
> > +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> > +			    &v))
> > +		return -ENODEV;
> 
> Please break functions out of if statements.
> 
> Does m10bmc_raw_read() return 0 on success?

Yes, this function just wrappered the regmap_read()

> 
> Seems odd for a read function.
> 
> > +	if (v != 0xffffffff) {
> > +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> > +		return -ENODEV;
> > +	}
> 
> The only acceptable version is -1?

As mentioned by Tom, this is a check if the board is using a very old legacy
bmc version, the driver doesn't mean to support this old legacy bmc
version.


> > +	case M10_N3000:
> > +		cells = m10bmc_pacn3000_subdevs;
> > +		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> > +		break;
> > +	default:
> > +		return -ENODEV;
> > +	}
> 
> Will there be other versions?

There will be a M10_D5005, we haven't fully prepared the code yet, but I
mean to reserve the place for it.

> > + * m10bmc_raw_read - read m10bmc register per addr
> > + * m10bmc_sys_read - read m10bmc system register per offset
> > + */
> > +static inline int
> > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > +		unsigned int *val)
> > +{
> > +	int ret;
> > +
> > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > +	if (ret)
> > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > +			addr, ret);
> > +
> > +	return ret;
> > +}
> > +
> > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> 
> No unnecessary abstractions.
> 
> Just use the Regmap API directly please.

Could we keep the 2 definition?

For m10bmc_raw_read(), we make it to help print some error info if
regmap RW fail. So we don't have to write "if (ret) dev_err" every time
we use regmap.

For m10bmc_sys_read(), the offset of BMC system registers could be
configured by HW developers (The MAX 10 is an CPLD, it could be easily
reprogrammed). And the HW SPEC will not add the offset when describing
the addresses of system registers. So:
1. It makes the definition of system registers in code align with HW SPEC.
2. It makes developers easier to make changes when the offset is adjusted
   in HW (I've been told by HW guys, it is sometimes necessary to adjust
   the offset when changing RTL, required by Altera EDA tool - Quartus).

Thanks,
Yilun

> 
> -- 
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Lee Jones Sept. 8, 2020, 11:56 a.m. UTC | #4
On Fri, 28 Aug 2020, Tom Rix wrote:

> 
> >> +
> >> +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> >> +{
> >> +	unsigned int v;
> >> +
> >> +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> >> +			    &v))
> >> +		return -ENODEV;
> > Please break functions out of if statements.
> >
> > Does m10bmc_raw_read() return 0 on success?
> >
> > Seems odd for a read function.
> >
> >> +	if (v != 0xffffffff) {
> >> +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> >> +		return -ENODEV;
> >> +	}
> > The only acceptable version is -1?
> 
> I ran into this in testing.  This is a check if the board is using a
> very old legacy bmc version. The M10BMC_LEGACY_SYS_BASE is the
> offset to this old block of mmio regs.  On the old boards, v would
> have not been f's, on the current boards it is f's. The check is
> necessary because future calls use the M10BMC_SYS_BASE offset which
> was not valid on the old boards.

This should be made more clear.  Either as a comment or as a define.

Preferably both!
Lee Jones Sept. 8, 2020, 12:03 p.m. UTC | #5
> > > +static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
> > > +{
> > > +	unsigned int v;
> > > +
> > > +	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
> > > +			    &v))
> > > +		return -ENODEV;
> > 
> > Please break functions out of if statements.
> > 
> > Does m10bmc_raw_read() return 0 on success?
> 
> Yes, this function just wrappered the regmap_read()

Avoid unnecessarily wrapping functions if possible.

> > Seems odd for a read function.
> > 
> > > +	if (v != 0xffffffff) {
> > > +		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > The only acceptable version is -1?
> 
> As mentioned by Tom, this is a check if the board is using a very old legacy
> bmc version, the driver doesn't mean to support this old legacy bmc
> version.

Please add a descriptive comment and define the value.

> > > + * m10bmc_raw_read - read m10bmc register per addr
> > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > + */
> > > +static inline int
> > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > +		unsigned int *val)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > +	if (ret)
> > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > +			addr, ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > 
> > No unnecessary abstractions.
> > 
> > Just use the Regmap API directly please.
> 
> Could we keep the 2 definition?
> 
> For m10bmc_raw_read(), we make it to help print some error info if
> regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> we use regmap.

How many call sites are there?

> For m10bmc_sys_read(), the offset of BMC system registers could be
> configured by HW developers (The MAX 10 is an CPLD, it could be easily
> reprogrammed). And the HW SPEC will not add the offset when describing
> the addresses of system registers. So:
> 1. It makes the definition of system registers in code align with HW SPEC.
> 2. It makes developers easier to make changes when the offset is adjusted
>    in HW (I've been told by HW guys, it is sometimes necessary to adjust
>    the offset when changing RTL, required by Altera EDA tool - Quartus).

Make sure you justify this for the function(s) you keep.

I'll take a closer look on the next submission.
Xu Yilun Sept. 9, 2020, 6:01 a.m. UTC | #6
> > > > + * m10bmc_raw_read - read m10bmc register per addr
> > > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > > + */
> > > > +static inline int
> > > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > > +		unsigned int *val)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > > +	if (ret)
> > > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > > +			addr, ret);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > > 
> > > No unnecessary abstractions.
> > > 
> > > Just use the Regmap API directly please.
> > 
> > Could we keep the 2 definition?
> > 
> > For m10bmc_raw_read(), we make it to help print some error info if
> > regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> > we use regmap.
> 
> How many call sites are there?

There are about 20 calls of the register read in m10bmc base driver and
sub device drivers. Most of them calls m10bmc_sys_read().
I prefer to keep the function for unified error log, but I'm also good
to follow your opinion. How do you think?

I also realize that it is not necessary that we define so many
m10bmc_raw_bulk_read/bulk_write/update_bits ... which are not frequently
used. We could change them.

> 
> > For m10bmc_sys_read(), the offset of BMC system registers could be
> > configured by HW developers (The MAX 10 is an CPLD, it could be easily
> > reprogrammed). And the HW SPEC will not add the offset when describing
> > the addresses of system registers. So:
> > 1. It makes the definition of system registers in code align with HW SPEC.
> > 2. It makes developers easier to make changes when the offset is adjusted
> >    in HW (I've been told by HW guys, it is sometimes necessary to adjust
> >    the offset when changing RTL, required by Altera EDA tool - Quartus).
> 
> Make sure you justify this for the function(s) you keep.

Yes, I could add some comments.

Thanks,
Yilun
Lee Jones Sept. 9, 2020, 7:31 a.m. UTC | #7
On Wed, 09 Sep 2020, Xu Yilun wrote:

> > > > > + * m10bmc_raw_read - read m10bmc register per addr
> > > > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > > > + */
> > > > > +static inline int
> > > > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > > > +		unsigned int *val)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > > > +	if (ret)
> > > > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > > > +			addr, ret);
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > > > 
> > > > No unnecessary abstractions.
> > > > 
> > > > Just use the Regmap API directly please.
> > > 
> > > Could we keep the 2 definition?
> > > 
> > > For m10bmc_raw_read(), we make it to help print some error info if
> > > regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> > > we use regmap.
> > 
> > How many call sites are there?
> 
> There are about 20 calls of the register read in m10bmc base driver and
> sub device drivers. Most of them calls m10bmc_sys_read().
> I prefer to keep the function for unified error log, but I'm also good
> to follow your opinion. How do you think?

Avoidable abstraction is one of my pet hates.  However,
unified/centralised error handling is a valid(ish) reason for
abstraction to exist.  Do you really need to know which read failed?
Is there a case where a read from only a particular register would
fail where others succeed?

> I also realize that it is not necessary that we define so many
> m10bmc_raw_bulk_read/bulk_write/update_bits ... which are not frequently
> used. We could change them.

Yes please.

> > > For m10bmc_sys_read(), the offset of BMC system registers could be
> > > configured by HW developers (The MAX 10 is an CPLD, it could be easily
> > > reprogrammed). And the HW SPEC will not add the offset when describing
> > > the addresses of system registers. So:
> > > 1. It makes the definition of system registers in code align with HW SPEC.
> > > 2. It makes developers easier to make changes when the offset is adjusted
> > >    in HW (I've been told by HW guys, it is sometimes necessary to adjust
> > >    the offset when changing RTL, required by Altera EDA tool - Quartus).
> > 
> > Make sure you justify this for the function(s) you keep.
> 
> Yes, I could add some comments.
> 
> Thanks,
> Yilun
Xu Yilun Sept. 9, 2020, 8:29 a.m. UTC | #8
On Wed, Sep 09, 2020 at 08:31:40AM +0100, Lee Jones wrote:
> On Wed, 09 Sep 2020, Xu Yilun wrote:
> 
> > > > > > + * m10bmc_raw_read - read m10bmc register per addr
> > > > > > + * m10bmc_sys_read - read m10bmc system register per offset
> > > > > > + */
> > > > > > +static inline int
> > > > > > +m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
> > > > > > +		unsigned int *val)
> > > > > > +{
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	ret = regmap_read(m10bmc->regmap, addr, val);
> > > > > > +	if (ret)
> > > > > > +		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
> > > > > > +			addr, ret);
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}
> > > > > > +
> > > > > > +#define m10bmc_sys_read(m10bmc, offset, val) \
> > > > > > +	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > > > > 
> > > > > No unnecessary abstractions.
> > > > > 
> > > > > Just use the Regmap API directly please.
> > > > 
> > > > Could we keep the 2 definition?
> > > > 
> > > > For m10bmc_raw_read(), we make it to help print some error info if
> > > > regmap RW fail. So we don't have to write "if (ret) dev_err" every time
> > > > we use regmap.
> > > 
> > > How many call sites are there?
> > 
> > There are about 20 calls of the register read in m10bmc base driver and
> > sub device drivers. Most of them calls m10bmc_sys_read().
> > I prefer to keep the function for unified error log, but I'm also good
> > to follow your opinion. How do you think?
> 
> Avoidable abstraction is one of my pet hates.  However,
> unified/centralised error handling is a valid(ish) reason for
> abstraction to exist.  Do you really need to know which read failed?
> Is there a case where a read from only a particular register would
> fail where others succeed?

I think it do helps we know which read failed in the first place when
communication error happens between FPGA & BMC.

Generally, if the error happens randomly on all registers, it may be the
problem of SPI bus.

But it is possible in some case error happens on some registers while
others are fine. The BMC has a internal Avalon mmio bus inside, and sub devices
connect on the bus. But the sub devices may response to the bus read/write
request differently according to hardware design. Once I run into a case
that the sub device stucks on one particular register read for long time
cause it prepares data so slowly. And the driver always timeout on that
register.

Thanks,
Yilun

> 
> > I also realize that it is not necessary that we define so many
> > m10bmc_raw_bulk_read/bulk_write/update_bits ... which are not frequently
> > used. We could change them.
> 
> Yes please.
>

Patch
diff mbox series

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
new file mode 100644
index 0000000..979a2d6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
@@ -0,0 +1,15 @@ 
+What:		/sys/bus/spi/devices/.../bmc_version
+Date:		June 2020
+KernelVersion:	5.10
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the hardware build version of Intel
+		MAX10 BMC chip.
+		Format: "0x%x".
+
+What:		/sys/bus/spi/devices/.../bmcfw_version
+Date:		June 2020
+KernelVersion:	5.10
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read only. Returns the firmware version of Intel MAX10
+		BMC chip.
+		Format: "0x%x".
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 33df083..57745f5 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2118,5 +2118,18 @@  config SGI_MFD_IOC3
 	  If you have an SGI Origin, Octane, or a PCI IOC3 card,
 	  then say Y. Otherwise say N.
 
+config MFD_INTEL_M10_BMC
+	tristate "Intel MAX10 Board Management Controller"
+	depends on SPI_MASTER
+	select REGMAP_SPI_AVMM
+	select MFD_CORE
+	help
+	  Support for the Intel MAX10 board management controller using the
+	  SPI interface.
+
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the functionality
+	  of the device.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a60e5f8..dd2cc7b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -264,3 +264,5 @@  obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
 obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
+
+obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
new file mode 100644
index 0000000..0dfd73a
--- /dev/null
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -0,0 +1,169 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Max10 Board Management Controller chip
+ *
+ * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+enum m10bmc_type {
+	M10_N3000,
+};
+
+static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
+	{
+		.name = "n3000bmc-hwmon",
+	},
+	{
+		.name = "n3000bmc-pkvl",
+	},
+	{
+		.name = "m10bmc-secure",
+	},
+
+};
+
+static struct regmap_config intel_m10bmc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = M10BMC_MEM_END,
+};
+
+static ssize_t bmc_version_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct intel_m10bmc *m10bmc = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(m10bmc, M10BMC_BUILD_VER, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", val);
+}
+static DEVICE_ATTR_RO(bmc_version);
+
+static ssize_t bmcfw_version_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct intel_m10bmc *max10 = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(max10, NIOS2_FW_VERSION, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", val);
+}
+static DEVICE_ATTR_RO(bmcfw_version);
+
+static struct attribute *m10bmc_attrs[] = {
+	&dev_attr_bmc_version.attr,
+	&dev_attr_bmcfw_version.attr,
+	NULL,
+};
+
+static struct attribute_group m10bmc_attr_group = {
+	.attrs = m10bmc_attrs,
+};
+
+static const struct attribute_group *m10bmc_dev_groups[] = {
+	&m10bmc_attr_group,
+	NULL
+};
+
+static int check_m10bmc_version(struct intel_m10bmc *m10bmc)
+{
+	unsigned int v;
+
+	if (m10bmc_raw_read(m10bmc, M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER,
+			    &v))
+		return -ENODEV;
+
+	if (v != 0xffffffff) {
+		dev_err(m10bmc->dev, "bad version M10BMC detected\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int intel_m10_bmc_spi_probe(struct spi_device *spi)
+{
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	struct device *dev = &spi->dev;
+	struct mfd_cell *cells;
+	struct intel_m10bmc *m10bmc;
+	int ret, n_cell;
+
+	m10bmc = devm_kzalloc(dev, sizeof(*m10bmc), GFP_KERNEL);
+	if (!m10bmc)
+		return -ENOMEM;
+
+	m10bmc->dev = dev;
+
+	m10bmc->regmap =
+		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
+	if (IS_ERR(m10bmc->regmap)) {
+		ret = PTR_ERR(m10bmc->regmap);
+		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
+		return ret;
+	}
+
+	spi_set_drvdata(spi, m10bmc);
+
+	ret = check_m10bmc_version(m10bmc);
+	if (ret) {
+		dev_err(dev, "Failed to identify m10bmc hardware\n");
+		return ret;
+	}
+
+	switch (id->driver_data) {
+	case M10_N3000:
+		cells = m10bmc_pacn3000_subdevs;
+		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
+				   NULL, 0, NULL);
+	if (ret)
+		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
+
+	return ret;
+}
+
+static const struct spi_device_id m10bmc_spi_id[] = {
+	{ "m10-n3000", M10_N3000 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
+
+static struct spi_driver intel_m10bmc_spi_driver = {
+	.driver = {
+		.name = "intel-m10-bmc",
+		.dev_groups = m10bmc_dev_groups,
+	},
+	.probe = intel_m10_bmc_spi_probe,
+	.id_table = m10bmc_spi_id,
+};
+
+module_spi_driver(intel_m10bmc_spi_driver);
+
+MODULE_DESCRIPTION("Intel Max10 BMC Device Driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:intel-m10-bmc");
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
new file mode 100644
index 0000000..4979756
--- /dev/null
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -0,0 +1,57 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Driver Header File for Intel Max10 Board Management Controller chip.
+ *
+ * Copyright (C) 2018-2020 Intel Corporation, Inc.
+ *
+ */
+#ifndef __INTEL_M10_BMC_H
+#define __INTEL_M10_BMC_H
+
+#include <linux/regmap.h>
+
+#define M10BMC_LEGACY_SYS_BASE	0x300400
+#define M10BMC_SYS_BASE		0x300800
+#define M10BMC_MEM_END		0x200000fc
+
+/* Register offset of system registers */
+#define NIOS2_FW_VERSION	0x0
+#define M10BMC_TEST_REG		0x3c
+#define M10BMC_BUILD_VER	0x68
+#define M10BMC_VER_MAJOR_MSK	GENMASK(23, 16)
+#define M10BMC_VER_PCB_INFO_MSK	GENMASK(31, 24)
+
+/**
+ * struct intel_m10bmc - Intel Max10 BMC MFD device private data structure
+ * @dev: this device
+ * @regmap: the regmap used to access registers by m10bmc itself
+ */
+struct intel_m10bmc {
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+/*
+ * register access helper functions.
+ *
+ * m10bmc_raw_read - read m10bmc register per addr
+ * m10bmc_sys_read - read m10bmc system register per offset
+ */
+static inline int
+m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
+		unsigned int *val)
+{
+	int ret;
+
+	ret = regmap_read(m10bmc->regmap, addr, val);
+	if (ret)
+		dev_err(m10bmc->dev, "fail to read raw reg %x: %d\n",
+			addr, ret);
+
+	return ret;
+}
+
+#define m10bmc_sys_read(m10bmc, offset, val) \
+	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
+
+#endif /* __INTEL_M10_BMC_H */