[PATCH v2 0/4] add PMCI driver support

[PATCH v2 0/4] add PMCI driver support

[Tianfei Zhang]

PMCI(Platform Management Control Interface) is a software-visible
interface, connected to card BMC which provided telemetry and mailbox
functionalities for Intel PAC FPGA card.

Currently, intel-m10-bmc driver support Intel MAX10 BMC functions via
SPI interface. To support multiple bus interfaces, splits the common
code from intel-m10-bmc driver into intel-m10-bmc-core. On the other
hand, it leverages the regmap APIs to support Intel specific Indirect
Register Interface for register read/write on PMCI driver. 

This patchset adding a driver for the PMCI-base interface of Intel MAX10
BMC controller.

patch 1: use ddata for local variables which directly interacts with
dev_get_drvdata()/dev_set_drvdata().
patch 2: split the common code from intel-m10-bmc driver into
intel-m10-bmc-core.
patch 3: add a driver for PMCI.
patch 4: introduce a new data structure m10bmc_csr for the different
register definition of MAX10 CSRs.

v2:
  - use regmap APIs to support Intel specific Indirect Register Interface
    on PMCI driver.
  - fix compile warning reported by lkp.
  - rebased on 5.19-rc2

Tianfei Zhang (4):
  mfd: intel-m10-bmc: rename the local variables
  mfd: intel-m10-bmc: split into core and spi
  mfd: intel-m10-bmc: add PMCI driver
  mfd: intel-m10-bmc: support multiple register layouts

 .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
 drivers/mfd/Kconfig                           |  34 +++-
 drivers/mfd/Makefile                          |   6 +-
 .../{intel-m10-bmc.c => intel-m10-bmc-core.c} | 148 ++++++--------
 drivers/mfd/intel-m10-bmc-pmci.c              | 190 ++++++++++++++++++
 drivers/mfd/intel-m10-bmc-spi.c               |  83 ++++++++
 include/linux/mfd/intel-m10-bmc.h             |  43 +++-
 7 files changed, 413 insertions(+), 99 deletions(-)
 rename drivers/mfd/{intel-m10-bmc.c => intel-m10-bmc-core.c} (56%)
 create mode 100644 drivers/mfd/intel-m10-bmc-pmci.c
 create mode 100644 drivers/mfd/intel-m10-bmc-spi.c

-- 
2.26.2

[PATCH v2 1/4] mfd: intel-m10-bmc: rename the local variables

[Tianfei Zhang]

It had better use ddata for local variables which
directly interacts with dev_get_drvdata()/dev_set_drvdata().

Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
 drivers/mfd/intel-m10-bmc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
index 8db3bcf5fccc..7e521df29c72 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -86,15 +86,15 @@ static DEVICE_ATTR_RO(bmcfw_version);
 static ssize_t mac_address_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
-	struct intel_m10bmc *max10 = dev_get_drvdata(dev);
+	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
 	unsigned int macaddr_low, macaddr_high;
 	int ret;
 
-	ret = m10bmc_sys_read(max10, M10BMC_MAC_LOW, &macaddr_low);
+	ret = m10bmc_sys_read(ddata, M10BMC_MAC_LOW, &macaddr_low);
 	if (ret)
 		return ret;
 
-	ret = m10bmc_sys_read(max10, M10BMC_MAC_HIGH, &macaddr_high);
+	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
 	if (ret)
 		return ret;
 
@@ -111,11 +111,11 @@ static DEVICE_ATTR_RO(mac_address);
 static ssize_t mac_count_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
-	struct intel_m10bmc *max10 = dev_get_drvdata(dev);
+	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
 	unsigned int macaddr_high;
 	int ret;
 
-	ret = m10bmc_sys_read(max10, M10BMC_MAC_HIGH, &macaddr_high);
+	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
 	if (ret)
 		return ret;
 
-- 
2.26.2

Re: [PATCH v2 1/4] mfd: intel-m10-bmc: rename the local variables

[Lee Jones]

On Thu, 16 Jun 2022, Tianfei Zhang wrote:

> It had better use ddata for local variables which
> directly interacts with dev_get_drvdata()/dev_set_drvdata().
> 
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
>  drivers/mfd/intel-m10-bmc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

For my own reference (apply this as-is to your sign-off block):

  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 2/4] mfd: intel-m10-bmc: split into core and spi

[Tianfei Zhang]

Split the common code from intel-m10-bmc driver into intel-m10-bmc-core.
intel-m10-bmc-core is the core MFD functions which can support multiple
bus interfaces like SPI bus.

Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
 drivers/mfd/Kconfig                           |  30 +++---
 drivers/mfd/Makefile                          |   5 +-
 .../{intel-m10-bmc.c => intel-m10-bmc-core.c} | 101 +++++-------------
 drivers/mfd/intel-m10-bmc-spi.c               |  83 ++++++++++++++
 include/linux/mfd/intel-m10-bmc.h             |  15 +++
 5 files changed, 144 insertions(+), 90 deletions(-)
 rename drivers/mfd/{intel-m10-bmc.c => intel-m10-bmc-core.c} (63%)
 create mode 100644 drivers/mfd/intel-m10-bmc-spi.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3b59456f5545..ee8398b02321 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2158,18 +2158,24 @@ 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 MAX 10 Board Management Controller"
-	depends on SPI_MASTER
-	select REGMAP_SPI_AVMM
-	select MFD_CORE
-	help
-	  Support for the Intel MAX 10 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.
+config MFD_INTEL_M10_BMC_CORE
+        tristate
+        select MFD_CORE
+        select REGMAP
+        default n
+
+config MFD_INTEL_M10_BMC_SPI
+        tristate "Intel MAX 10 Board Management Controller with SPI"
+        depends on SPI_MASTER
+        select MFD_INTEL_M10_BMC_CORE
+        select REGMAP_SPI_AVMM
+        help
+          Support for the Intel MAX 10 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.
 
 config MFD_RSMU_I2C
 	tristate "Renesas Synchronization Management Unit with I2C"
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 858cacf659d6..b5d3263c1205 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -266,7 +266,10 @@ obj-$(CONFIG_MFD_QCOM_PM8008)	+= qcom-pm8008.o
 
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
 obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
-obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
+
+intel-m10-bmc-objs             := intel-m10-bmc-core.o
+obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)   += intel-m10-bmc.o
+obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)   += intel-m10-bmc-spi.o
 
 obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
 obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc-core.c
similarity index 63%
rename from drivers/mfd/intel-m10-bmc.c
rename to drivers/mfd/intel-m10-bmc-core.c
index 7e521df29c72..f6dc549e1bc3 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc-core.c
@@ -1,23 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Intel MAX 10 Board Management Controller chip
+ * Intel MAX 10 Board Management Controller chip - common code
  *
- * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
+ * Copyright (C) 2018-2022 Intel Corporation. All rights reserved.
  */
 #include <linux/bitfield.h>
 #include <linux/init.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/intel-m10-bmc.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/regmap.h>
-#include <linux/spi/spi.h>
-
-enum m10bmc_type {
-	M10_N3000,
-	M10_D5005,
-	M10_N5010,
-};
 
 static struct mfd_cell m10bmc_d5005_subdevs[] = {
 	{ .name = "d5005bmc-hwmon" },
@@ -33,26 +24,6 @@ static struct mfd_cell m10bmc_n5010_subdevs[] = {
 	{ .name = "n5010bmc-hwmon" },
 };
 
-static const struct regmap_range m10bmc_regmap_range[] = {
-	regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER),
-	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
-	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END),
-};
-
-static const struct regmap_access_table m10bmc_access_table = {
-	.yes_ranges	= m10bmc_regmap_range,
-	.n_yes_ranges	= ARRAY_SIZE(m10bmc_regmap_range),
-};
-
-static struct regmap_config intel_m10bmc_regmap_config = {
-	.reg_bits = 32,
-	.val_bits = 32,
-	.reg_stride = 4,
-	.wr_table = &m10bmc_access_table,
-	.rd_table = &m10bmc_access_table,
-	.max_register = M10BMC_MEM_END,
-};
-
 static ssize_t bmc_version_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -131,7 +102,16 @@ static struct attribute *m10bmc_attrs[] = {
 	&dev_attr_mac_count.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(m10bmc);
+
+static const struct attribute_group m10bmc_group = {
+	.attrs = m10bmc_attrs,
+};
+
+const struct attribute_group *m10bmc_dev_groups[] = {
+	&m10bmc_group,
+	NULL,
+};
+EXPORT_SYMBOL_GPL(m10bmc_dev_groups);
 
 static int check_m10bmc_version(struct intel_m10bmc *ddata)
 {
@@ -158,37 +138,21 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
 	return 0;
 }
 
-static int intel_m10_bmc_spi_probe(struct spi_device *spi)
+int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
 {
-	const struct spi_device_id *id = spi_get_device_id(spi);
-	struct device *dev = &spi->dev;
+	enum m10bmc_type type = m10bmc->type;
 	struct mfd_cell *cells;
-	struct intel_m10bmc *ddata;
 	int ret, n_cell;
 
-	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
-	if (!ddata)
-		return -ENOMEM;
-
-	ddata->dev = dev;
-
-	ddata->regmap =
-		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
-	if (IS_ERR(ddata->regmap)) {
-		ret = PTR_ERR(ddata->regmap);
-		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
-		return ret;
-	}
-
-	spi_set_drvdata(spi, ddata);
+	dev_set_drvdata(m10bmc->dev, m10bmc);
 
-	ret = check_m10bmc_version(ddata);
+	ret = check_m10bmc_version(m10bmc);
 	if (ret) {
-		dev_err(dev, "Failed to identify m10bmc hardware\n");
+		dev_err(m10bmc->dev, "Failed to identify m10bmc hardware\n");
 		return ret;
 	}
 
-	switch (id->driver_data) {
+	switch (type) {
 	case M10_N3000:
 		cells = m10bmc_pacn3000_subdevs;
 		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
@@ -205,33 +169,16 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 		return -ENODEV;
 	}
 
-	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
-				   NULL, 0, NULL);
+	ret = devm_mfd_add_devices(m10bmc->dev, PLATFORM_DEVID_AUTO,
+				   cells, n_cell, NULL, 0, NULL);
 	if (ret)
-		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
+		dev_err(m10bmc->dev, "Failed to register sub-devices: %d\n",
+			ret);
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(m10bmc_dev_init);
 
-static const struct spi_device_id m10bmc_spi_id[] = {
-	{ "m10-n3000", M10_N3000 },
-	{ "m10-d5005", M10_D5005 },
-	{ "m10-n5010", M10_N5010 },
-	{ }
-};
-MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
-
-static struct spi_driver intel_m10bmc_spi_driver = {
-	.driver = {
-		.name = "intel-m10-bmc",
-		.dev_groups = m10bmc_groups,
-	},
-	.probe = intel_m10_bmc_spi_probe,
-	.id_table = m10bmc_spi_id,
-};
-module_spi_driver(intel_m10bmc_spi_driver);
-
-MODULE_DESCRIPTION("Intel MAX 10 BMC Device Driver");
+MODULE_DESCRIPTION("Intel MAX 10 BMC core MFD driver");
 MODULE_AUTHOR("Intel Corporation");
 MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("spi:intel-m10-bmc");
diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
new file mode 100644
index 000000000000..9cafbc0f89f0
--- /dev/null
+++ b/drivers/mfd/intel-m10-bmc-spi.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel MAX 10 Board Management Controller chip
+ *
+ * Copyright (C) 2018-2021 Intel Corporation. All rights reserved.
+ */
+#include <linux/bitfield.h>
+#include <linux/init.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+static const struct regmap_range m10bmc_regmap_range[] = {
+	regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER),
+	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
+	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END),
+};
+
+static const struct regmap_access_table m10bmc_access_table = {
+	.yes_ranges	= m10bmc_regmap_range,
+	.n_yes_ranges	= ARRAY_SIZE(m10bmc_regmap_range),
+};
+
+static struct regmap_config intel_m10bmc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.wr_table = &m10bmc_access_table,
+	.rd_table = &m10bmc_access_table,
+	.max_register = M10BMC_MEM_END,
+};
+
+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 intel_m10bmc *ddata;
+	int ret;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	ddata->dev = dev;
+	ddata->type = (enum m10bmc_type)(id->driver_data);
+
+	ddata->regmap =
+		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
+	if (IS_ERR(ddata->regmap)) {
+		ret = PTR_ERR(ddata->regmap);
+		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
+		return ret;
+	}
+
+	spi_set_drvdata(spi, ddata);
+
+	return m10bmc_dev_init(ddata);
+}
+
+static const struct spi_device_id m10bmc_spi_id[] = {
+	{ "m10-n3000", M10_N3000 },
+	{ "m10-d5005", M10_D5005 },
+	{ "m10-n5010", M10_N5010 },
+	{ }
+};
+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 MAX 10 BMC SPI bus interface");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:intel-m10-bmc");
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index f0044b14136e..dd81ffdcf168 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -118,14 +118,23 @@
 /* Address of 4KB inverted bit vector containing staging area FLASH count */
 #define STAGING_FLASH_COUNT	0x17ffb000
 
+/* Supported MAX10 BMC types */
+enum m10bmc_type {
+	M10_N3000,
+	M10_D5005,
+	M10_N5010,
+};
+
 /**
  * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
  * @dev: this device
  * @regmap: the regmap used to access registers by m10bmc itself
+ * @type: the type of MAX10 BMC
  */
 struct intel_m10bmc {
 	struct device *dev;
 	struct regmap *regmap;
+	enum m10bmc_type type;
 };
 
 /*
@@ -159,4 +168,10 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
 #define m10bmc_sys_read(m10bmc, offset, val) \
 	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
 
+/*
+ * MAX10 BMC Core support
+ */
+int m10bmc_dev_init(struct intel_m10bmc *m10bmc);
+extern const struct attribute_group *m10bmc_dev_groups[];
+
 #endif /* __MFD_INTEL_M10_BMC_H */
-- 
2.26.2

Re: [PATCH v2 2/4] mfd: intel-m10-bmc: split into core and spi

[Xu Yilun]

On Thu, Jun 16, 2022 at 10:04:03PM -0400, Tianfei Zhang wrote:
> Split the common code from intel-m10-bmc driver into intel-m10-bmc-core.
> intel-m10-bmc-core is the core MFD functions which can support multiple
> bus interfaces like SPI bus.

Please specify which else are you going to support, for better
understanding.

> 
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
>  drivers/mfd/Kconfig                           |  30 +++---
>  drivers/mfd/Makefile                          |   5 +-
>  .../{intel-m10-bmc.c => intel-m10-bmc-core.c} | 101 +++++-------------
>  drivers/mfd/intel-m10-bmc-spi.c               |  83 ++++++++++++++
>  include/linux/mfd/intel-m10-bmc.h             |  15 +++
>  5 files changed, 144 insertions(+), 90 deletions(-)
>  rename drivers/mfd/{intel-m10-bmc.c => intel-m10-bmc-core.c} (63%)
>  create mode 100644 drivers/mfd/intel-m10-bmc-spi.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3b59456f5545..ee8398b02321 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2158,18 +2158,24 @@ 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 MAX 10 Board Management Controller"
> -	depends on SPI_MASTER
> -	select REGMAP_SPI_AVMM
> -	select MFD_CORE
> -	help
> -	  Support for the Intel MAX 10 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.
> +config MFD_INTEL_M10_BMC_CORE
> +        tristate
> +        select MFD_CORE
> +        select REGMAP
> +        default n
> +
> +config MFD_INTEL_M10_BMC_SPI
> +        tristate "Intel MAX 10 Board Management Controller with SPI"
> +        depends on SPI_MASTER
> +        select MFD_INTEL_M10_BMC_CORE
> +        select REGMAP_SPI_AVMM
> +        help
> +          Support for the Intel MAX 10 board management controller using the
> +          SPI interface.
> +
> +          This driver provides common support for accessing the device,

                                                                   device by SPI,

> +          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"
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf659d6..b5d3263c1205 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -266,7 +266,10 @@ obj-$(CONFIG_MFD_QCOM_PM8008)	+= qcom-pm8008.o
>  
>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
> -obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> +
> +intel-m10-bmc-objs             := intel-m10-bmc-core.o
> +obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)   += intel-m10-bmc.o

Why make a intel-m10-bmc obj here? What's the problem of
intel-m10-bmc-core.o?

> +obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)   += intel-m10-bmc-spi.o
>  
>  obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
>  obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc-core.c
> similarity index 63%
> rename from drivers/mfd/intel-m10-bmc.c
> rename to drivers/mfd/intel-m10-bmc-core.c
> index 7e521df29c72..f6dc549e1bc3 100644
> --- a/drivers/mfd/intel-m10-bmc.c
> +++ b/drivers/mfd/intel-m10-bmc-core.c
> @@ -1,23 +1,14 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * Intel MAX 10 Board Management Controller chip
> + * Intel MAX 10 Board Management Controller chip - common code
>   *
> - * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> + * Copyright (C) 2018-2022 Intel Corporation. All rights reserved.
>   */
>  #include <linux/bitfield.h>
>  #include <linux/init.h>
>  #include <linux/mfd/core.h>
>  #include <linux/mfd/intel-m10-bmc.h>
>  #include <linux/module.h>
> -#include <linux/mutex.h>
> -#include <linux/regmap.h>
> -#include <linux/spi/spi.h>
> -
> -enum m10bmc_type {
> -	M10_N3000,
> -	M10_D5005,
> -	M10_N5010,
> -};
>  
>  static struct mfd_cell m10bmc_d5005_subdevs[] = {
>  	{ .name = "d5005bmc-hwmon" },
> @@ -33,26 +24,6 @@ static struct mfd_cell m10bmc_n5010_subdevs[] = {
>  	{ .name = "n5010bmc-hwmon" },
>  };
>  
> -static const struct regmap_range m10bmc_regmap_range[] = {
> -	regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER),
> -	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
> -	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END),
> -};
> -
> -static const struct regmap_access_table m10bmc_access_table = {
> -	.yes_ranges	= m10bmc_regmap_range,
> -	.n_yes_ranges	= ARRAY_SIZE(m10bmc_regmap_range),
> -};
> -
> -static struct regmap_config intel_m10bmc_regmap_config = {
> -	.reg_bits = 32,
> -	.val_bits = 32,
> -	.reg_stride = 4,
> -	.wr_table = &m10bmc_access_table,
> -	.rd_table = &m10bmc_access_table,
> -	.max_register = M10BMC_MEM_END,
> -};

Why remove all these configurations, I suppose it is not related to bus
type.

> -
>  static ssize_t bmc_version_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> @@ -131,7 +102,16 @@ static struct attribute *m10bmc_attrs[] = {
>  	&dev_attr_mac_count.attr,
>  	NULL,
>  };
> -ATTRIBUTE_GROUPS(m10bmc);
> +
> +static const struct attribute_group m10bmc_group = {
> +	.attrs = m10bmc_attrs,
> +};
> +
> +const struct attribute_group *m10bmc_dev_groups[] = {
> +	&m10bmc_group,
> +	NULL,
> +};
> +EXPORT_SYMBOL_GPL(m10bmc_dev_groups);
>  
>  static int check_m10bmc_version(struct intel_m10bmc *ddata)
>  {
> @@ -158,37 +138,21 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
>  	return 0;
>  }
>  
> -static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> +int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
>  {
> -	const struct spi_device_id *id = spi_get_device_id(spi);
> -	struct device *dev = &spi->dev;
> +	enum m10bmc_type type = m10bmc->type;
>  	struct mfd_cell *cells;
> -	struct intel_m10bmc *ddata;
>  	int ret, n_cell;
>  
> -	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> -	if (!ddata)
> -		return -ENOMEM;
> -
> -	ddata->dev = dev;
> -
> -	ddata->regmap =
> -		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
> -	if (IS_ERR(ddata->regmap)) {
> -		ret = PTR_ERR(ddata->regmap);
> -		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
> -		return ret;
> -	}
> -
> -	spi_set_drvdata(spi, ddata);
> +	dev_set_drvdata(m10bmc->dev, m10bmc);

The naming is not consistent here, some are ddata, some are m10bmc,
please keep a unified name acress all the patches.

>  
> -	ret = check_m10bmc_version(ddata);
> +	ret = check_m10bmc_version(m10bmc);
>  	if (ret) {
> -		dev_err(dev, "Failed to identify m10bmc hardware\n");
> +		dev_err(m10bmc->dev, "Failed to identify m10bmc hardware\n");

Keep the dev local variable so that we don't have to change every
related lines.

>  		return ret;
>  	}
>  
> -	switch (id->driver_data) {
> +	switch (type) {
>  	case M10_N3000:
>  		cells = m10bmc_pacn3000_subdevs;
>  		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> @@ -205,33 +169,16 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  		return -ENODEV;
>  	}
>  
> -	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> -				   NULL, 0, NULL);
> +	ret = devm_mfd_add_devices(m10bmc->dev, PLATFORM_DEVID_AUTO,
> +				   cells, n_cell, NULL, 0, NULL);

ditto

>  	if (ret)
> -		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
> +		dev_err(m10bmc->dev, "Failed to register sub-devices: %d\n",
> +			ret);

ditto

>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(m10bmc_dev_init);
>  
> -static const struct spi_device_id m10bmc_spi_id[] = {
> -	{ "m10-n3000", M10_N3000 },
> -	{ "m10-d5005", M10_D5005 },
> -	{ "m10-n5010", M10_N5010 },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> -
> -static struct spi_driver intel_m10bmc_spi_driver = {
> -	.driver = {
> -		.name = "intel-m10-bmc",
> -		.dev_groups = m10bmc_groups,
> -	},
> -	.probe = intel_m10_bmc_spi_probe,
> -	.id_table = m10bmc_spi_id,
> -};
> -module_spi_driver(intel_m10bmc_spi_driver);
> -
> -MODULE_DESCRIPTION("Intel MAX 10 BMC Device Driver");
> +MODULE_DESCRIPTION("Intel MAX 10 BMC core MFD driver");
>  MODULE_AUTHOR("Intel Corporation");
>  MODULE_LICENSE("GPL v2");
> -MODULE_ALIAS("spi:intel-m10-bmc");
> diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
> new file mode 100644
> index 000000000000..9cafbc0f89f0
> --- /dev/null
> +++ b/drivers/mfd/intel-m10-bmc-spi.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel MAX 10 Board Management Controller chip

SPI driver for Intel MAX 10 ...

> + *
> + * Copyright (C) 2018-2021 Intel Corporation. All rights reserved.
> + */
> +#include <linux/bitfield.h>
> +#include <linux/init.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +static const struct regmap_range m10bmc_regmap_range[] = {
> +	regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER),
> +	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
> +	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END),
> +};
> +
> +static const struct regmap_access_table m10bmc_access_table = {
> +	.yes_ranges	= m10bmc_regmap_range,
> +	.n_yes_ranges	= ARRAY_SIZE(m10bmc_regmap_range),
> +};
> +
> +static struct regmap_config intel_m10bmc_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.wr_table = &m10bmc_access_table,
> +	.rd_table = &m10bmc_access_table,
> +	.max_register = M10BMC_MEM_END,
> +};
> +
> +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 intel_m10bmc *ddata;
> +	int ret;
> +
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return -ENOMEM;
> +
> +	ddata->dev = dev;
> +	ddata->type = (enum m10bmc_type)(id->driver_data);
> +
> +	ddata->regmap =
> +		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
> +	if (IS_ERR(ddata->regmap)) {
> +		ret = PTR_ERR(ddata->regmap);
> +		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	spi_set_drvdata(spi, ddata);

Why we need this? I saw the dev.drvdata is set in core driver.

> +
> +	return m10bmc_dev_init(ddata);
> +}
> +
> +static const struct spi_device_id m10bmc_spi_id[] = {
> +	{ "m10-n3000", M10_N3000 },
> +	{ "m10-d5005", M10_D5005 },
> +	{ "m10-n5010", M10_N5010 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> +
> +static struct spi_driver intel_m10bmc_spi_driver = {
> +	.driver = {
> +		.name = "intel-m10-bmc",

Need renaming?

> +		.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 MAX 10 BMC SPI bus interface");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:intel-m10-bmc");
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index f0044b14136e..dd81ffdcf168 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -118,14 +118,23 @@
>  /* Address of 4KB inverted bit vector containing staging area FLASH count */
>  #define STAGING_FLASH_COUNT	0x17ffb000
>  
> +/* Supported MAX10 BMC types */

Please use correct format for kernel doc comments

> +enum m10bmc_type {
> +	M10_N3000,
> +	M10_D5005,
> +	M10_N5010,
> +};
> +
>  /**
>   * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
>   * @dev: this device
>   * @regmap: the regmap used to access registers by m10bmc itself
> + * @type: the type of MAX10 BMC
>   */
>  struct intel_m10bmc {
>  	struct device *dev;
>  	struct regmap *regmap;
> +	enum m10bmc_type type;
>  };
>  
>  /*
> @@ -159,4 +168,10 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
>  #define m10bmc_sys_read(m10bmc, offset, val) \
>  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
>  
> +/*
> + * MAX10 BMC Core support
> + */
> +int m10bmc_dev_init(struct intel_m10bmc *m10bmc);
> +extern const struct attribute_group *m10bmc_dev_groups[];
> +
>  #endif /* __MFD_INTEL_M10_BMC_H */
> -- 
> 2.26.2

RE: [PATCH v2 2/4] mfd: intel-m10-bmc: split into core and spi

[Zhang, Tianfei]

> -----Original Message-----
> From: Xu, Yilun <yilun.xu@intel.com>
> Sent: Monday, June 20, 2022 5:59 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>
> Cc: lee.jones@linaro.org; Wu, Hao <hao.wu@intel.com>; trix@redhat.com;
> linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; Weight, Russell H
> <russell.h.weight@intel.com>; matthew.gerlach@linux.intel.com
> Subject: Re: [PATCH v2 2/4] mfd: intel-m10-bmc: split into core and spi
> 
> On Thu, Jun 16, 2022 at 10:04:03PM -0400, Tianfei Zhang wrote:
> > Split the common code from intel-m10-bmc driver into intel-m10-bmc-core.
> > intel-m10-bmc-core is the core MFD functions which can support
> > multiple bus interfaces like SPI bus.
> 
> Please specify which else are you going to support, for better understanding.

There are lots of common code for intel-m10-bmc-spi and intel-m10-bmc-pmci driver.
To better support multiple bus interfaces and  reduce  the duplicate code , 
we like to split the common code into a core file.

> 
> >
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> >  drivers/mfd/Kconfig                           |  30 +++---
> >  drivers/mfd/Makefile                          |   5 +-
> >  .../{intel-m10-bmc.c => intel-m10-bmc-core.c} | 101 +++++-------------
> >  drivers/mfd/intel-m10-bmc-spi.c               |  83 ++++++++++++++
> >  include/linux/mfd/intel-m10-bmc.h             |  15 +++
> >  5 files changed, 144 insertions(+), 90 deletions(-)  rename
> > drivers/mfd/{intel-m10-bmc.c => intel-m10-bmc-core.c} (63%)  create
> > mode 100644 drivers/mfd/intel-m10-bmc-spi.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 3b59456f5545..ee8398b02321 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2158,18 +2158,24 @@ 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 MAX 10 Board Management Controller"
> > -	depends on SPI_MASTER
> > -	select REGMAP_SPI_AVMM
> > -	select MFD_CORE
> > -	help
> > -	  Support for the Intel MAX 10 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.
> > +config MFD_INTEL_M10_BMC_CORE
> > +        tristate
> > +        select MFD_CORE
> > +        select REGMAP
> > +        default n
> > +
> > +config MFD_INTEL_M10_BMC_SPI
> > +        tristate "Intel MAX 10 Board Management Controller with SPI"
> > +        depends on SPI_MASTER
> > +        select MFD_INTEL_M10_BMC_CORE
> > +        select REGMAP_SPI_AVMM
> > +        help
> > +          Support for the Intel MAX 10 board management controller using the
> > +          SPI interface.
> > +
> > +          This driver provides common support for accessing the
> > + device,
> 
>                                                                    device by SPI,

I  will fix it on next version patch.
> 
> > +          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"
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > 858cacf659d6..b5d3263c1205 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -266,7 +266,10 @@ obj-$(CONFIG_MFD_QCOM_PM8008)	+= qcom-
> pm8008.o
> >
> >  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
> >  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
> > -obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> > +
> > +intel-m10-bmc-objs             := intel-m10-bmc-core.o
> > +obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)   += intel-m10-bmc.o
> 
> Why make a intel-m10-bmc obj here? What's the problem of intel-m10-bmc-
> core.o?

This intel-m10-bmc-objs is for CONFIG_MFD_INTEL_M10_BMC_CORE.
So if want to support intel-m10-bmc-spi driver, it should set both CONFIG_MFD_INTEL_M10_BMC_CORE
and CONFIG_MFD_INTEL_M10_BMC_SPI in .config file.

Here is another example in drivers/mfd/Makefile.

intel-soc-pmic-objs             := intel_soc_pmic_core.o intel_soc_pmic_crc.o
obj-$(CONFIG_INTEL_SOC_PMIC)    += intel-soc-pmic.o
obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC)      += intel_soc_pmic_bxtwc.o

> 
> > +obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)   += intel-m10-bmc-spi.o
> >
> >  obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
> >  obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
> > diff --git a/drivers/mfd/intel-m10-bmc.c
> > b/drivers/mfd/intel-m10-bmc-core.c
> > similarity index 63%
> > rename from drivers/mfd/intel-m10-bmc.c rename to
> > drivers/mfd/intel-m10-bmc-core.c index 7e521df29c72..f6dc549e1bc3
> > 100644
> > --- a/drivers/mfd/intel-m10-bmc.c
> > +++ b/drivers/mfd/intel-m10-bmc-core.c
> > @@ -1,23 +1,14 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*
> > - * Intel MAX 10 Board Management Controller chip
> > + * Intel MAX 10 Board Management Controller chip - common code
> >   *
> > - * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
> > + * Copyright (C) 2018-2022 Intel Corporation. All rights reserved.
> >   */
> >  #include <linux/bitfield.h>
> >  #include <linux/init.h>
> >  #include <linux/mfd/core.h>
> >  #include <linux/mfd/intel-m10-bmc.h>
> >  #include <linux/module.h>
> > -#include <linux/mutex.h>
> > -#include <linux/regmap.h>
> > -#include <linux/spi/spi.h>
> > -
> > -enum m10bmc_type {
> > -	M10_N3000,
> > -	M10_D5005,
> > -	M10_N5010,
> > -};
> >
> >  static struct mfd_cell m10bmc_d5005_subdevs[] = {
> >  	{ .name = "d5005bmc-hwmon" },
> > @@ -33,26 +24,6 @@ static struct mfd_cell m10bmc_n5010_subdevs[] = {
> >  	{ .name = "n5010bmc-hwmon" },
> >  };
> >
> > -static const struct regmap_range m10bmc_regmap_range[] = {
> > -	regmap_reg_range(M10BMC_LEGACY_BUILD_VER,
> M10BMC_LEGACY_BUILD_VER),
> > -	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
> > -	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END),
> > -};
> > -
> > -static const struct regmap_access_table m10bmc_access_table = {
> > -	.yes_ranges	= m10bmc_regmap_range,
> > -	.n_yes_ranges	= ARRAY_SIZE(m10bmc_regmap_range),
> > -};
> > -
> > -static struct regmap_config intel_m10bmc_regmap_config = {
> > -	.reg_bits = 32,
> > -	.val_bits = 32,
> > -	.reg_stride = 4,
> > -	.wr_table = &m10bmc_access_table,
> > -	.rd_table = &m10bmc_access_table,
> > -	.max_register = M10BMC_MEM_END,
> > -};
> 
> Why remove all these configurations, I suppose it is not related to bus type.

This intel_m10bmc_regmap_config has removed to intel-m10-bmc-spi driver, it is related to SPI bus.

> 
> > -
> >  static ssize_t bmc_version_show(struct device *dev,
> >  				struct device_attribute *attr, char *buf)  { @@
> -131,7 +102,16 @@
> > static struct attribute *m10bmc_attrs[] = {
> >  	&dev_attr_mac_count.attr,
> >  	NULL,
> >  };
> > -ATTRIBUTE_GROUPS(m10bmc);
> > +
> > +static const struct attribute_group m10bmc_group = {
> > +	.attrs = m10bmc_attrs,
> > +};
> > +
> > +const struct attribute_group *m10bmc_dev_groups[] = {
> > +	&m10bmc_group,
> > +	NULL,
> > +};
> > +EXPORT_SYMBOL_GPL(m10bmc_dev_groups);
> >
> >  static int check_m10bmc_version(struct intel_m10bmc *ddata)  { @@
> > -158,37 +138,21 @@ static int check_m10bmc_version(struct intel_m10bmc
> *ddata)
> >  	return 0;
> >  }
> >
> > -static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> > +int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
> >  {
> > -	const struct spi_device_id *id = spi_get_device_id(spi);
> > -	struct device *dev = &spi->dev;
> > +	enum m10bmc_type type = m10bmc->type;
> >  	struct mfd_cell *cells;
> > -	struct intel_m10bmc *ddata;
> >  	int ret, n_cell;
> >
> > -	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > -	if (!ddata)
> > -		return -ENOMEM;
> > -
> > -	ddata->dev = dev;
> > -
> > -	ddata->regmap =
> > -		devm_regmap_init_spi_avmm(spi,
> &intel_m10bmc_regmap_config);
> > -	if (IS_ERR(ddata->regmap)) {
> > -		ret = PTR_ERR(ddata->regmap);
> > -		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
> > -		return ret;
> > -	}
> > -
> > -	spi_set_drvdata(spi, ddata);
> > +	dev_set_drvdata(m10bmc->dev, m10bmc);
> 
> The naming is not consistent here, some are ddata, some are m10bmc, please
> keep a unified name acress all the patches.

I agree, I will fix it.

> 
> >
> > -	ret = check_m10bmc_version(ddata);
> > +	ret = check_m10bmc_version(m10bmc);
> >  	if (ret) {
> > -		dev_err(dev, "Failed to identify m10bmc hardware\n");
> > +		dev_err(m10bmc->dev, "Failed to identify m10bmc
> hardware\n");
> 
> Keep the dev local variable so that we don't have to change every related lines.

I agree, I will fix it.

> 
> >  		return ret;
> >  	}
> >
> > -	switch (id->driver_data) {
> > +	switch (type) {
> >  	case M10_N3000:
> >  		cells = m10bmc_pacn3000_subdevs;
> >  		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> > @@ -205,33 +169,16 @@ static int intel_m10_bmc_spi_probe(struct
> spi_device *spi)
> >  		return -ENODEV;
> >  	}
> >
> > -	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells,
> n_cell,
> > -				   NULL, 0, NULL);
> > +	ret = devm_mfd_add_devices(m10bmc->dev, PLATFORM_DEVID_AUTO,
> > +				   cells, n_cell, NULL, 0, NULL);
> 
> ditto
> 
> >  	if (ret)
> > -		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
> > +		dev_err(m10bmc->dev, "Failed to register sub-devices: %d\n",
> > +			ret);
> 
> ditto
> 
> >
> >  	return ret;
> >  }
> > +EXPORT_SYMBOL_GPL(m10bmc_dev_init);
> >
> > -static const struct spi_device_id m10bmc_spi_id[] = {
> > -	{ "m10-n3000", M10_N3000 },
> > -	{ "m10-d5005", M10_D5005 },
> > -	{ "m10-n5010", M10_N5010 },
> > -	{ }
> > -};
> > -MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> > -
> > -static struct spi_driver intel_m10bmc_spi_driver = {
> > -	.driver = {
> > -		.name = "intel-m10-bmc",
> > -		.dev_groups = m10bmc_groups,
> > -	},
> > -	.probe = intel_m10_bmc_spi_probe,
> > -	.id_table = m10bmc_spi_id,
> > -};
> > -module_spi_driver(intel_m10bmc_spi_driver);
> > -
> > -MODULE_DESCRIPTION("Intel MAX 10 BMC Device Driver");
> > +MODULE_DESCRIPTION("Intel MAX 10 BMC core MFD driver");
> >  MODULE_AUTHOR("Intel Corporation");
> >  MODULE_LICENSE("GPL v2");
> > -MODULE_ALIAS("spi:intel-m10-bmc");
> > diff --git a/drivers/mfd/intel-m10-bmc-spi.c
> > b/drivers/mfd/intel-m10-bmc-spi.c new file mode 100644 index
> > 000000000000..9cafbc0f89f0
> > --- /dev/null
> > +++ b/drivers/mfd/intel-m10-bmc-spi.c
> > @@ -0,0 +1,83 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel MAX 10 Board Management Controller chip
> 
> SPI driver for Intel MAX 10 ...

This is good for me.

> 
> > + *
> > + * Copyright (C) 2018-2021 Intel Corporation. All rights reserved.
> > + */
> > +#include <linux/bitfield.h>
> > +#include <linux/init.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/intel-m10-bmc.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/spi/spi.h>
> > +
> > +static const struct regmap_range m10bmc_regmap_range[] = {
> > +	regmap_reg_range(M10BMC_LEGACY_BUILD_VER,
> M10BMC_LEGACY_BUILD_VER),
> > +	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
> > +	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END), };
> > +
> > +static const struct regmap_access_table m10bmc_access_table = {
> > +	.yes_ranges	= m10bmc_regmap_range,
> > +	.n_yes_ranges	= ARRAY_SIZE(m10bmc_regmap_range),
> > +};
> > +
> > +static struct regmap_config intel_m10bmc_regmap_config = {
> > +	.reg_bits = 32,
> > +	.val_bits = 32,
> > +	.reg_stride = 4,
> > +	.wr_table = &m10bmc_access_table,
> > +	.rd_table = &m10bmc_access_table,
> > +	.max_register = M10BMC_MEM_END,
> > +};
> > +
> > +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 intel_m10bmc *ddata;
> > +	int ret;
> > +
> > +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > +	if (!ddata)
> > +		return -ENOMEM;
> > +
> > +	ddata->dev = dev;
> > +	ddata->type = (enum m10bmc_type)(id->driver_data);
> > +
> > +	ddata->regmap =
> > +		devm_regmap_init_spi_avmm(spi,
> &intel_m10bmc_regmap_config);
> > +	if (IS_ERR(ddata->regmap)) {
> > +		ret = PTR_ERR(ddata->regmap);
> > +		dev_err(dev, "Failed to allocate regmap: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	spi_set_drvdata(spi, ddata);
> 
> Why we need this? I saw the dev.drvdata is set in core driver.

I agree, it is duplicate, we can remove it.

> 
> > +
> > +	return m10bmc_dev_init(ddata);
> > +}
> > +
> > +static const struct spi_device_id m10bmc_spi_id[] = {
> > +	{ "m10-n3000", M10_N3000 },
> > +	{ "m10-d5005", M10_D5005 },
> > +	{ "m10-n5010", M10_N5010 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> > +
> > +static struct spi_driver intel_m10bmc_spi_driver = {
> > +	.driver = {
> > +		.name = "intel-m10-bmc",
> 
> Need renaming?

How about "intel-m10-bmc-spi"?

> 
> > +		.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 MAX 10 BMC SPI bus interface");
> > +MODULE_AUTHOR("Intel Corporation"); MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("spi:intel-m10-bmc");
> > diff --git a/include/linux/mfd/intel-m10-bmc.h
> > b/include/linux/mfd/intel-m10-bmc.h
> > index f0044b14136e..dd81ffdcf168 100644
> > --- a/include/linux/mfd/intel-m10-bmc.h
> > +++ b/include/linux/mfd/intel-m10-bmc.h
> > @@ -118,14 +118,23 @@
> >  /* Address of 4KB inverted bit vector containing staging area FLASH count */
> >  #define STAGING_FLASH_COUNT	0x17ffb000
> >
> > +/* Supported MAX10 BMC types */
> 
> Please use correct format for kernel doc comments

I will fix it on next version patch.

> 
> > +enum m10bmc_type {
> > +	M10_N3000,
> > +	M10_D5005,
> > +	M10_N5010,
> > +};
> > +
> >  /**
> >   * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
> >   * @dev: this device
> >   * @regmap: the regmap used to access registers by m10bmc itself
> > + * @type: the type of MAX10 BMC
> >   */
> >  struct intel_m10bmc {
> >  	struct device *dev;
> >  	struct regmap *regmap;
> > +	enum m10bmc_type type;
> >  };
> >
> >  /*
> > @@ -159,4 +168,10 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc,
> > unsigned int addr,  #define m10bmc_sys_read(m10bmc, offset, val) \
> >  	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> >
> > +/*
> > + * MAX10 BMC Core support
> > + */
> > +int m10bmc_dev_init(struct intel_m10bmc *m10bmc); extern const struct
> > +attribute_group *m10bmc_dev_groups[];
> > +
> >  #endif /* __MFD_INTEL_M10_BMC_H */
> > --
> > 2.26.2

[PATCH v2 3/4] mfd: intel-m10-bmc: add PMCI driver

[Tianfei Zhang]

Adding a driver for the PMCI-base interface of Intel MAX10
BMC controller.

PMCI(Platform Management Control Interface) is a software-visible
interface, connected to card BMC which provided telemetry and mailbox
functionalities. On the other hand, this driver leverages the regmap
APIs to support Intel specific Indirect Register Interface for register
read/write on PMCI.

Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v2:
  - fix compile warning reported by lkp
  - use regmap API for Indirect Register Interface.
---
 .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
 drivers/mfd/Kconfig                           |  12 ++
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/intel-m10-bmc-core.c              |  19 +-
 drivers/mfd/intel-m10-bmc-pmci.c              | 190 ++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h             |   8 +
 6 files changed, 230 insertions(+), 8 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@intel.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@intel.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@intel.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@intel.com>
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ee8398b02321..7300efec3617 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2177,6 +2177,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_INDIRECT_REGISTER
+	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 b5d3263c1205..a8ffdc223cf7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -270,6 +270,7 @@ obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 intel-m10-bmc-objs             := intel-m10-bmc-core.o
 obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)   += intel-m10-bmc.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-core.c b/drivers/mfd/intel-m10-bmc-core.c
index f6dc549e1bc3..c6a1a4c28357 100644
--- a/drivers/mfd/intel-m10-bmc-core.c
+++ b/drivers/mfd/intel-m10-bmc-core.c
@@ -10,6 +10,11 @@
 #include <linux/mfd/intel-m10-bmc.h>
 #include <linux/module.h>
 
+static struct mfd_cell m10bmc_n6000_bmc_subdevs[] = {
+	{ .name = "n6000bmc-hwmon" },
+	{ .name = "n6000bmc-sec-update" }
+};
+
 static struct mfd_cell m10bmc_d5005_subdevs[] = {
 	{ .name = "d5005bmc-hwmon" },
 };
@@ -146,10 +151,12 @@ int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
 
 	dev_set_drvdata(m10bmc->dev, m10bmc);
 
-	ret = check_m10bmc_version(m10bmc);
-	if (ret) {
-		dev_err(m10bmc->dev, "Failed to identify m10bmc hardware\n");
-		return ret;
+	if (type == M10_N3000 || type == M10_D5005 || type == M10_N5010) {
+		ret = check_m10bmc_version(m10bmc);
+		if (ret) {
+			dev_err(m10bmc->dev, "Failed to identify m10bmc hardware\n");
+			return ret;
+		}
 	}
 
 	switch (type) {
@@ -165,6 +172,10 @@ int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
 		cells = m10bmc_n5010_subdevs;
 		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
 		break;
+	case M10_N6000:
+		cells = m10bmc_n6000_bmc_subdevs;
+		n_cell = ARRAY_SIZE(m10bmc_n6000_bmc_subdevs);
+		break;
 	default:
 		return -ENODEV;
 	}
diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
new file mode 100644
index 000000000000..249a2db5e742
--- /dev/null
+++ b/drivers/mfd/intel-m10-bmc-pmci.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PMCI-based interface to MAX10 BMC
+ *
+ * Copyright (C) 2020-2022 Intel Corporation, Inc.
+ *
+ */
+
+#include <linux/dfl.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define M10BMC_PMCI_INDIRECT_BASE 0x400
+#define INDIRECT_INT_US        1
+#define INDIRECT_TIMEOUT_US    10000
+
+#define INDIRECT_CMD_OFF	0x0
+#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
+
+struct indirect_ctx {
+	void __iomem *base;
+	struct device *dev;
+	unsigned long sleep_us;
+	unsigned long timeout_us;
+};
+
+struct pmci_device {
+	void __iomem *base;
+	struct device *dev;
+	struct intel_m10bmc m10bmc;
+	struct indirect_ctx ctx;
+};
+
+static int pmci_indirect_bus_clr_cmd(struct indirect_ctx *ctx)
+{
+	unsigned int cmd;
+	int ret;
+
+	writel(0, ctx->base + INDIRECT_CMD_OFF);
+	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
+				 (!cmd), ctx->sleep_us, ctx->timeout_us);
+	if (ret)
+		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn", __func__, cmd);
+
+	return ret;
+}
+
+static int pmci_indirect_bus_reg_read(void *context, unsigned int reg,
+				      unsigned int *val)
+{
+	struct indirect_ctx *ctx = context;
+	unsigned int cmd;
+	int ret;
+
+	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
+	if (cmd)
+		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__, 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), cmd,
+				 (cmd & INDIRECT_CMD_ACK), ctx->sleep_us,
+				 ctx->timeout_us);
+	if (ret) {
+		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n", __func__, reg, cmd);
+		goto out;
+	}
+
+	*val = readl(ctx->base + INDIRECT_RD_OFF);
+
+	if (pmci_indirect_bus_clr_cmd(ctx))
+		ret = -ETIMEDOUT;
+
+out:
+	return ret;
+}
+
+static int pmci_indirect_bus_reg_write(void *context, unsigned int reg,
+				       unsigned int val)
+{
+	struct indirect_ctx *ctx = context;
+	unsigned int cmd;
+	int ret;
+
+	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
+	if (cmd)
+		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__, 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), cmd,
+				 (cmd & INDIRECT_CMD_ACK), ctx->sleep_us,
+				 ctx->timeout_us);
+	if (ret) {
+		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n", __func__, reg, cmd);
+		goto out;
+	}
+
+	if (pmci_indirect_bus_clr_cmd(ctx))
+		ret = -ETIMEDOUT;
+
+out:
+	return ret;
+}
+
+static const struct regmap_bus pmci_indirect_bus = {
+	.reg_write = pmci_indirect_bus_reg_write,
+	.reg_read =  pmci_indirect_bus_reg_read,
+};
+
+static const struct regmap_range m10bmc_pmci_regmap_range[] = {
+	regmap_reg_range(M10BMC_PMCI_SYS_BASE, M10BMC_PMCI_SYS_END),
+};
+
+static const struct regmap_access_table m10_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 = &m10_access_table,
+	.rd_table = &m10_access_table,
+	.max_register = M10BMC_PMCI_SYS_END,
+};
+
+static int pmci_probe(struct dfl_device *ddev)
+{
+	struct device *dev = &ddev->dev;
+	struct pmci_device *pmci;
+
+	pmci = devm_kzalloc(dev, sizeof(*pmci), GFP_KERNEL);
+	if (!pmci)
+		return -ENOMEM;
+
+	pmci->m10bmc.dev = dev;
+	pmci->dev = dev;
+	pmci->m10bmc.type = M10_N6000;
+
+	pmci->base = devm_ioremap_resource(dev, &ddev->mmio_res);
+	if (IS_ERR(pmci->base))
+		return PTR_ERR(pmci->base);
+
+	pmci->ctx.base = pmci->base + M10BMC_PMCI_INDIRECT_BASE;
+	pmci->ctx.sleep_us = INDIRECT_INT_US;
+	pmci->ctx.timeout_us = INDIRECT_TIMEOUT_US;
+	pmci->ctx.dev = dev;
+	pmci->m10bmc.regmap =
+		devm_regmap_init(dev,
+				 &pmci_indirect_bus,
+				 &pmci->ctx,
+				 &m10bmc_pmci_regmap_config);
+	if (IS_ERR(pmci->m10bmc.regmap))
+		return PTR_ERR(pmci->m10bmc.regmap);
+
+	return m10bmc_dev_init(&pmci->m10bmc);
+}
+
+#define FME_FEATURE_ID_PMCI_BMC	0x12
+
+static const struct dfl_device_id pmci_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_PMCI_BMC },
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, pmci_ids);
+
+static struct dfl_driver pmci_driver = {
+	.drv	= {
+		.name       = "dfl-pmci-bmc",
+		.dev_groups = m10bmc_dev_groups,
+	},
+	.id_table = pmci_ids,
+	.probe    = pmci_probe,
+};
+
+module_dfl_driver(pmci_driver);
+
+MODULE_DESCRIPTION("MAX10 BMC PMCI-based interface");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index dd81ffdcf168..83c4d3993dcb 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -118,11 +118,19 @@
 /* Address of 4KB inverted bit vector containing staging area FLASH count */
 #define STAGING_FLASH_COUNT	0x17ffb000
 
+#define M10BMC_PMCI_SYS_BASE 0x0
+#define M10BMC_PMCI_SYS_END  0xfff
+
+/* Telemetry registers */
+#define M10BMC_PMCI_TELEM_START		0x400
+#define M10BMC_PMCI_TELEM_END		0x78c
+
 /* Supported MAX10 BMC types */
 enum m10bmc_type {
 	M10_N3000,
 	M10_D5005,
 	M10_N5010,
+	M10_N6000
 };
 
 /**
-- 
2.26.2

Re: [PATCH v2 3/4] mfd: intel-m10-bmc: add PMCI driver

[Xu Yilun]

On Thu, Jun 16, 2022 at 10:04:04PM -0400, Tianfei Zhang wrote:
> Adding a driver for the PMCI-base interface of Intel MAX10
> BMC controller.
> 
> PMCI(Platform Management Control Interface) is a software-visible
> interface, connected to card BMC which provided telemetry and mailbox

The PMCI interface or the BMC provides the telemetry functionality?

Maybe you should first describe the basic register access
functionality for PMCI interface. This is the main purpose of this
patch.

> functionalities. On the other hand, this driver leverages the regmap
> APIs to support Intel specific Indirect Register Interface for register
> read/write on PMCI.
> 
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
> v2:
>   - fix compile warning reported by lkp
>   - use regmap API for Indirect Register Interface.
> ---
>  .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
>  drivers/mfd/Kconfig                           |  12 ++
>  drivers/mfd/Makefile                          |   1 +
>  drivers/mfd/intel-m10-bmc-core.c              |  19 +-
>  drivers/mfd/intel-m10-bmc-pmci.c              | 190 ++++++++++++++++++

Refering to intel-m10-bmc-spi.c, this is the DFL bus driver, PMCI is just
its internal register accessing implementation.

So maybe intel-m10-bmc-dfl.c?

>  include/linux/mfd/intel-m10-bmc.h             |   8 +
>  6 files changed, 230 insertions(+), 8 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

The same driver for both PMCI & spi? From your patch, the pmci driver is
named "dfl-pmci-bmc", is it?

>  Date:		June 2020
>  KernelVersion:	5.10
>  Contact:	Xu Yilun <yilun.xu@intel.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@intel.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@intel.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@intel.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ee8398b02321..7300efec3617 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2177,6 +2177,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_INDIRECT_REGISTER
> +	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 b5d3263c1205..a8ffdc223cf7 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -270,6 +270,7 @@ obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>  intel-m10-bmc-objs             := intel-m10-bmc-core.o
>  obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)   += intel-m10-bmc.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-core.c b/drivers/mfd/intel-m10-bmc-core.c
> index f6dc549e1bc3..c6a1a4c28357 100644
> --- a/drivers/mfd/intel-m10-bmc-core.c
> +++ b/drivers/mfd/intel-m10-bmc-core.c
> @@ -10,6 +10,11 @@
>  #include <linux/mfd/intel-m10-bmc.h>
>  #include <linux/module.h>
>  
> +static struct mfd_cell m10bmc_n6000_bmc_subdevs[] = {
> +	{ .name = "n6000bmc-hwmon" },
> +	{ .name = "n6000bmc-sec-update" }
> +};
> +
>  static struct mfd_cell m10bmc_d5005_subdevs[] = {
>  	{ .name = "d5005bmc-hwmon" },
>  };
> @@ -146,10 +151,12 @@ int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
>  
>  	dev_set_drvdata(m10bmc->dev, m10bmc);
>  
> -	ret = check_m10bmc_version(m10bmc);
> -	if (ret) {
> -		dev_err(m10bmc->dev, "Failed to identify m10bmc hardware\n");
> -		return ret;
> +	if (type == M10_N3000 || type == M10_D5005 || type == M10_N5010) {
> +		ret = check_m10bmc_version(m10bmc);
> +		if (ret) {
> +			dev_err(m10bmc->dev, "Failed to identify m10bmc hardware\n");
> +			return ret;
> +		}
>  	}
>  
>  	switch (type) {
> @@ -165,6 +172,10 @@ int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
>  		cells = m10bmc_n5010_subdevs;
>  		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
>  		break;
> +	case M10_N6000:
> +		cells = m10bmc_n6000_bmc_subdevs;
> +		n_cell = ARRAY_SIZE(m10bmc_n6000_bmc_subdevs);
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
> new file mode 100644
> index 000000000000..249a2db5e742
> --- /dev/null
> +++ b/drivers/mfd/intel-m10-bmc-pmci.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PMCI-based interface to MAX10 BMC
> + *
> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
> + *
> + */
> +
> +#include <linux/dfl.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define M10BMC_PMCI_INDIRECT_BASE 0x400
> +#define INDIRECT_INT_US        1
> +#define INDIRECT_TIMEOUT_US    10000
> +
> +#define INDIRECT_CMD_OFF	0x0
> +#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
> +
> +struct indirect_ctx {
> +	void __iomem *base;
> +	struct device *dev;
> +	unsigned long sleep_us;
> +	unsigned long timeout_us;
> +};
> +
> +struct pmci_device {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct intel_m10bmc m10bmc;
> +	struct indirect_ctx ctx;
> +};
> +
> +static int pmci_indirect_bus_clr_cmd(struct indirect_ctx *ctx)
> +{
> +	unsigned int cmd;
> +	int ret;
> +
> +	writel(0, ctx->base + INDIRECT_CMD_OFF);
> +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> +				 (!cmd), ctx->sleep_us, ctx->timeout_us);
> +	if (ret)
> +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn", __func__, cmd);
> +
> +	return ret;
> +}
> +
> +static int pmci_indirect_bus_reg_read(void *context, unsigned int reg,
> +				      unsigned int *val)
> +{
> +	struct indirect_ctx *ctx = context;
> +	unsigned int cmd;
> +	int ret;
> +
> +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> +	if (cmd)
> +		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__, 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), cmd,
> +				 (cmd & INDIRECT_CMD_ACK), ctx->sleep_us,
> +				 ctx->timeout_us);
> +	if (ret) {
> +		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n", __func__, reg, cmd);
> +		goto out;
> +	}
> +
> +	*val = readl(ctx->base + INDIRECT_RD_OFF);
> +
> +	if (pmci_indirect_bus_clr_cmd(ctx))
> +		ret = -ETIMEDOUT;
> +
> +out:
> +	return ret;
> +}
> +
> +static int pmci_indirect_bus_reg_write(void *context, unsigned int reg,
> +				       unsigned int val)
> +{
> +	struct indirect_ctx *ctx = context;
> +	unsigned int cmd;
> +	int ret;
> +
> +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> +	if (cmd)
> +		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__, 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), cmd,
> +				 (cmd & INDIRECT_CMD_ACK), ctx->sleep_us,
> +				 ctx->timeout_us);
> +	if (ret) {
> +		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n", __func__, reg, cmd);
> +		goto out;
> +	}
> +
> +	if (pmci_indirect_bus_clr_cmd(ctx))
> +		ret = -ETIMEDOUT;
> +
> +out:
> +	return ret;
> +}
> +
> +static const struct regmap_bus pmci_indirect_bus = {
> +	.reg_write = pmci_indirect_bus_reg_write,
> +	.reg_read =  pmci_indirect_bus_reg_read,
> +};
> +
> +static const struct regmap_range m10bmc_pmci_regmap_range[] = {
> +	regmap_reg_range(M10BMC_PMCI_SYS_BASE, M10BMC_PMCI_SYS_END),
> +};

If you have just one range, why bother defining it?

> +
> +static const struct regmap_access_table m10_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 = &m10_access_table,
> +	.rd_table = &m10_access_table,
> +	.max_register = M10BMC_PMCI_SYS_END,
> +};

The same concern, these should be specific to BMC, not PMCI interface,
is it?

> +
> +static int pmci_probe(struct dfl_device *ddev)
> +{
> +	struct device *dev = &ddev->dev;
> +	struct pmci_device *pmci;
> +
> +	pmci = devm_kzalloc(dev, sizeof(*pmci), GFP_KERNEL);
> +	if (!pmci)
> +		return -ENOMEM;
> +
> +	pmci->m10bmc.dev = dev;
> +	pmci->dev = dev;
> +	pmci->m10bmc.type = M10_N6000;

The DFL feature only binds to N6000 BMC? Then the driver should be named
dfl-n6000-bmc I think.

> +
> +	pmci->base = devm_ioremap_resource(dev, &ddev->mmio_res);
> +	if (IS_ERR(pmci->base))
> +		return PTR_ERR(pmci->base);
> +
> +	pmci->ctx.base = pmci->base + M10BMC_PMCI_INDIRECT_BASE;
> +	pmci->ctx.sleep_us = INDIRECT_INT_US;
> +	pmci->ctx.timeout_us = INDIRECT_TIMEOUT_US;
> +	pmci->ctx.dev = dev;
> +	pmci->m10bmc.regmap =
> +		devm_regmap_init(dev,
> +				 &pmci_indirect_bus,
> +				 &pmci->ctx,
> +				 &m10bmc_pmci_regmap_config);
> +	if (IS_ERR(pmci->m10bmc.regmap))
> +		return PTR_ERR(pmci->m10bmc.regmap);
> +
> +	return m10bmc_dev_init(&pmci->m10bmc);
> +}
> +
> +#define FME_FEATURE_ID_PMCI_BMC	0x12
> +
> +static const struct dfl_device_id pmci_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_PMCI_BMC },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dfl, pmci_ids);
> +
> +static struct dfl_driver pmci_driver = {
> +	.drv	= {
> +		.name       = "dfl-pmci-bmc",
> +		.dev_groups = m10bmc_dev_groups,
> +	},
> +	.id_table = pmci_ids,
> +	.probe    = pmci_probe,
> +};
> +
> +module_dfl_driver(pmci_driver);
> +
> +MODULE_DESCRIPTION("MAX10 BMC PMCI-based interface");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index dd81ffdcf168..83c4d3993dcb 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -118,11 +118,19 @@
>  /* Address of 4KB inverted bit vector containing staging area FLASH count */
>  #define STAGING_FLASH_COUNT	0x17ffb000
>  
> +#define M10BMC_PMCI_SYS_BASE 0x0
> +#define M10BMC_PMCI_SYS_END  0xfff
> +
> +/* Telemetry registers */
> +#define M10BMC_PMCI_TELEM_START		0x400
> +#define M10BMC_PMCI_TELEM_END		0x78c

The same concern, I asumme they are specific to BMC, not PMCI interface.

Thanks,
Yilun

> +
>  /* Supported MAX10 BMC types */
>  enum m10bmc_type {
>  	M10_N3000,
>  	M10_D5005,
>  	M10_N5010,
> +	M10_N6000
>  };
>  
>  /**
> -- 
> 2.26.2

RE: [PATCH v2 3/4] mfd: intel-m10-bmc: add PMCI driver

[Zhang, Tianfei]

> -----Original Message-----
> From: Xu, Yilun <yilun.xu@intel.com>
> Sent: Monday, June 20, 2022 7:55 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>
> Cc: lee.jones@linaro.org; Wu, Hao <hao.wu@intel.com>; trix@redhat.com;
> linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; Weight, Russell H
> <russell.h.weight@intel.com>; matthew.gerlach@linux.intel.com
> Subject: Re: [PATCH v2 3/4] mfd: intel-m10-bmc: add PMCI driver
> 
> On Thu, Jun 16, 2022 at 10:04:04PM -0400, Tianfei Zhang wrote:
> > Adding a driver for the PMCI-base interface of Intel MAX10 BMC
> > controller.
> >
> > PMCI(Platform Management Control Interface) is a software-visible
> > interface, connected to card BMC which provided telemetry and mailbox
> 
> The PMCI interface or the BMC provides the telemetry functionality?
> 
> Maybe you should first describe the basic register access functionality for PMCI
> interface. This is the main purpose of this patch.

Yes, I agree, the telemetry was provided by BMC via PMCI.

> 
> > functionalities. On the other hand, this driver leverages the regmap
> > APIs to support Intel specific Indirect Register Interface for
> > register read/write on PMCI.
> >
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > ---
> > v2:
> >   - fix compile warning reported by lkp
> >   - use regmap API for Indirect Register Interface.
> > ---
> >  .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
> >  drivers/mfd/Kconfig                           |  12 ++
> >  drivers/mfd/Makefile                          |   1 +
> >  drivers/mfd/intel-m10-bmc-core.c              |  19 +-
> >  drivers/mfd/intel-m10-bmc-pmci.c              | 190 ++++++++++++++++++
> 
> Refering to intel-m10-bmc-spi.c, this is the DFL bus driver, PMCI is just its
> internal register accessing implementation.
> 
> So maybe intel-m10-bmc-dfl.c?

If we have other bus interface like I2C in future,  "intel-m10-bmc-i2c.c" is better.
Like this, PMCI is other bus interface, I think " intel-m10-bmc-pmci.c" will be better.

> 
> >  include/linux/mfd/intel-m10-bmc.h             |   8 +
> >  6 files changed, 230 insertions(+), 8 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
> 
> The same driver for both PMCI & spi? From your patch, the pmci driver is named
> "dfl-pmci-bmc", is it?

Yes, the path is for both PMCI and SPI.  This is should be "dfl-pmci-bmc". 

For PMCI driver:
/sys/bus/dfl/drivers/dfl-pmci-bmc/dfl_dev.X/ bmc_version
For SPI driver:
/sys/bus/spi/drivers/intel-m10-bmc/spi0.0/bmc_version

So this path should be:
/sys/bus/.../drivers/.../.../bmc_version

> 
> >  Date:		June 2020
> >  KernelVersion:	5.10
> >  Contact:	Xu Yilun <yilun.xu@intel.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@intel.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@intel.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@intel.com>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > ee8398b02321..7300efec3617 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2177,6 +2177,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_INDIRECT_REGISTER
> > +	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
> > b5d3263c1205..a8ffdc223cf7 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -270,6 +270,7 @@ obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-
> mfd-i2c.o
> >  intel-m10-bmc-objs             := intel-m10-bmc-core.o
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)   += intel-m10-bmc.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-core.c
> > b/drivers/mfd/intel-m10-bmc-core.c
> > index f6dc549e1bc3..c6a1a4c28357 100644
> > --- a/drivers/mfd/intel-m10-bmc-core.c
> > +++ b/drivers/mfd/intel-m10-bmc-core.c
> > @@ -10,6 +10,11 @@
> >  #include <linux/mfd/intel-m10-bmc.h>
> >  #include <linux/module.h>
> >
> > +static struct mfd_cell m10bmc_n6000_bmc_subdevs[] = {
> > +	{ .name = "n6000bmc-hwmon" },
> > +	{ .name = "n6000bmc-sec-update" }
> > +};
> > +
> >  static struct mfd_cell m10bmc_d5005_subdevs[] = {
> >  	{ .name = "d5005bmc-hwmon" },
> >  };
> > @@ -146,10 +151,12 @@ int m10bmc_dev_init(struct intel_m10bmc
> *m10bmc)
> >
> >  	dev_set_drvdata(m10bmc->dev, m10bmc);
> >
> > -	ret = check_m10bmc_version(m10bmc);
> > -	if (ret) {
> > -		dev_err(m10bmc->dev, "Failed to identify m10bmc
> hardware\n");
> > -		return ret;
> > +	if (type == M10_N3000 || type == M10_D5005 || type == M10_N5010) {
> > +		ret = check_m10bmc_version(m10bmc);
> > +		if (ret) {
> > +			dev_err(m10bmc->dev, "Failed to identify m10bmc
> hardware\n");
> > +			return ret;
> > +		}
> >  	}
> >
> >  	switch (type) {
> > @@ -165,6 +172,10 @@ int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
> >  		cells = m10bmc_n5010_subdevs;
> >  		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
> >  		break;
> > +	case M10_N6000:
> > +		cells = m10bmc_n6000_bmc_subdevs;
> > +		n_cell = ARRAY_SIZE(m10bmc_n6000_bmc_subdevs);
> > +		break;
> >  	default:
> >  		return -ENODEV;
> >  	}
> > diff --git a/drivers/mfd/intel-m10-bmc-pmci.c
> > b/drivers/mfd/intel-m10-bmc-pmci.c
> > new file mode 100644
> > index 000000000000..249a2db5e742
> > --- /dev/null
> > +++ b/drivers/mfd/intel-m10-bmc-pmci.c
> > @@ -0,0 +1,190 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PMCI-based interface to MAX10 BMC
> > + *
> > + * Copyright (C) 2020-2022 Intel Corporation, Inc.
> > + *
> > + */
> > +
> > +#include <linux/dfl.h>
> > +#include <linux/mfd/intel-m10-bmc.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#define M10BMC_PMCI_INDIRECT_BASE 0x400
> > +#define INDIRECT_INT_US        1
> > +#define INDIRECT_TIMEOUT_US    10000
> > +
> > +#define INDIRECT_CMD_OFF	0x0
> > +#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
> > +
> > +struct indirect_ctx {
> > +	void __iomem *base;
> > +	struct device *dev;
> > +	unsigned long sleep_us;
> > +	unsigned long timeout_us;
> > +};
> > +
> > +struct pmci_device {
> > +	void __iomem *base;
> > +	struct device *dev;
> > +	struct intel_m10bmc m10bmc;
> > +	struct indirect_ctx ctx;
> > +};
> > +
> > +static int pmci_indirect_bus_clr_cmd(struct indirect_ctx *ctx) {
> > +	unsigned int cmd;
> > +	int ret;
> > +
> > +	writel(0, ctx->base + INDIRECT_CMD_OFF);
> > +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> > +				 (!cmd), ctx->sleep_us, ctx->timeout_us);
> > +	if (ret)
> > +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn",
> __func__,
> > +cmd);
> > +
> > +	return ret;
> > +}
> > +
> > +static int pmci_indirect_bus_reg_read(void *context, unsigned int reg,
> > +				      unsigned int *val)
> > +{
> > +	struct indirect_ctx *ctx = context;
> > +	unsigned int cmd;
> > +	int ret;
> > +
> > +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> > +	if (cmd)
> > +		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__,
> 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), cmd,
> > +				 (cmd & INDIRECT_CMD_ACK), ctx->sleep_us,
> > +				 ctx->timeout_us);
> > +	if (ret) {
> > +		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n",
> __func__, reg, cmd);
> > +		goto out;
> > +	}
> > +
> > +	*val = readl(ctx->base + INDIRECT_RD_OFF);
> > +
> > +	if (pmci_indirect_bus_clr_cmd(ctx))
> > +		ret = -ETIMEDOUT;
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int pmci_indirect_bus_reg_write(void *context, unsigned int reg,
> > +				       unsigned int val)
> > +{
> > +	struct indirect_ctx *ctx = context;
> > +	unsigned int cmd;
> > +	int ret;
> > +
> > +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> > +	if (cmd)
> > +		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__,
> 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), cmd,
> > +				 (cmd & INDIRECT_CMD_ACK), ctx->sleep_us,
> > +				 ctx->timeout_us);
> > +	if (ret) {
> > +		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n",
> __func__, reg, cmd);
> > +		goto out;
> > +	}
> > +
> > +	if (pmci_indirect_bus_clr_cmd(ctx))
> > +		ret = -ETIMEDOUT;
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static const struct regmap_bus pmci_indirect_bus = {
> > +	.reg_write = pmci_indirect_bus_reg_write,
> > +	.reg_read =  pmci_indirect_bus_reg_read, };
> > +
> > +static const struct regmap_range m10bmc_pmci_regmap_range[] = {
> > +	regmap_reg_range(M10BMC_PMCI_SYS_BASE,
> M10BMC_PMCI_SYS_END), };
> 
> If you have just one range, why bother defining it?

There is different register range between SPI bus and PMCI bus of BMC.
For SPI bus interface, the register  range of BMC is: 0x300800 - 0x300fff
For PMIC bus interface, the register range of BMC is: 0x0 - 0xfff

> 
> > +
> > +static const struct regmap_access_table m10_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 = &m10_access_table,
> > +	.rd_table = &m10_access_table,
> > +	.max_register = M10BMC_PMCI_SYS_END, };
> 
> The same concern, these should be specific to BMC, not PMCI interface, is it?

I think this is related to different regmap configuration for different bus interface like SPI or PMCI, so
It should be in PMCI interface.

> 
> > +
> > +static int pmci_probe(struct dfl_device *ddev) {
> > +	struct device *dev = &ddev->dev;
> > +	struct pmci_device *pmci;
> > +
> > +	pmci = devm_kzalloc(dev, sizeof(*pmci), GFP_KERNEL);
> > +	if (!pmci)
> > +		return -ENOMEM;
> > +
> > +	pmci->m10bmc.dev = dev;
> > +	pmci->dev = dev;
> > +	pmci->m10bmc.type = M10_N6000;
> 
> The DFL feature only binds to N6000 BMC? Then the driver should be named dfl-
> n6000-bmc I think.

Yes, maybe M10_DFL should be better.

> 
> > +
> > +	pmci->base = devm_ioremap_resource(dev, &ddev->mmio_res);
> > +	if (IS_ERR(pmci->base))
> > +		return PTR_ERR(pmci->base);
> > +
> > +	pmci->ctx.base = pmci->base + M10BMC_PMCI_INDIRECT_BASE;
> > +	pmci->ctx.sleep_us = INDIRECT_INT_US;
> > +	pmci->ctx.timeout_us = INDIRECT_TIMEOUT_US;
> > +	pmci->ctx.dev = dev;
> > +	pmci->m10bmc.regmap =
> > +		devm_regmap_init(dev,
> > +				 &pmci_indirect_bus,
> > +				 &pmci->ctx,
> > +				 &m10bmc_pmci_regmap_config);
> > +	if (IS_ERR(pmci->m10bmc.regmap))
> > +		return PTR_ERR(pmci->m10bmc.regmap);
> > +
> > +	return m10bmc_dev_init(&pmci->m10bmc); }
> > +
> > +#define FME_FEATURE_ID_PMCI_BMC	0x12
> > +
> > +static const struct dfl_device_id pmci_ids[] = {
> > +	{ FME_ID, FME_FEATURE_ID_PMCI_BMC },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(dfl, pmci_ids);
> > +
> > +static struct dfl_driver pmci_driver = {
> > +	.drv	= {
> > +		.name       = "dfl-pmci-bmc",
> > +		.dev_groups = m10bmc_dev_groups,
> > +	},
> > +	.id_table = pmci_ids,
> > +	.probe    = pmci_probe,
> > +};
> > +
> > +module_dfl_driver(pmci_driver);
> > +
> > +MODULE_DESCRIPTION("MAX10 BMC PMCI-based interface");
> > +MODULE_AUTHOR("Intel Corporation"); MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/intel-m10-bmc.h
> > b/include/linux/mfd/intel-m10-bmc.h
> > index dd81ffdcf168..83c4d3993dcb 100644
> > --- a/include/linux/mfd/intel-m10-bmc.h
> > +++ b/include/linux/mfd/intel-m10-bmc.h
> > @@ -118,11 +118,19 @@
> >  /* Address of 4KB inverted bit vector containing staging area FLASH count */
> >  #define STAGING_FLASH_COUNT	0x17ffb000
> >
> > +#define M10BMC_PMCI_SYS_BASE 0x0
> > +#define M10BMC_PMCI_SYS_END  0xfff
> > +
> > +/* Telemetry registers */
> > +#define M10BMC_PMCI_TELEM_START		0x400
> > +#define M10BMC_PMCI_TELEM_END		0x78c
> 
> The same concern, I asumme they are specific to BMC, not PMCI interface.

Yes,  I agree, I will remove it.

> 
> Thanks,
> Yilun
> 
> > +
> >  /* Supported MAX10 BMC types */
> >  enum m10bmc_type {
> >  	M10_N3000,
> >  	M10_D5005,
> >  	M10_N5010,
> > +	M10_N6000
> >  };
> >
> >  /**
> > --
> > 2.26.2

[PATCH v2 4/4] mfd: intel-m10-bmc: support multiple register layouts

[Tianfei Zhang]

There are different base addresses for the MAX10 CSR register.
Introducing a new data structure m10bmc_csr for the register
definition of MAX10 CSR. Embedded m10bmc_csr into struct
intel_m10bmc to support multiple register layouts.

Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
 drivers/mfd/intel-m10-bmc-core.c  | 30 +++++++++++++++++++++++++-----
 include/linux/mfd/intel-m10-bmc.h | 20 +++++++++++++++++++-
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/intel-m10-bmc-core.c b/drivers/mfd/intel-m10-bmc-core.c
index c6a1a4c28357..f85f8e2aa9a1 100644
--- a/drivers/mfd/intel-m10-bmc-core.c
+++ b/drivers/mfd/intel-m10-bmc-core.c
@@ -10,6 +10,22 @@
 #include <linux/mfd/intel-m10-bmc.h>
 #include <linux/module.h>
 
+static const struct m10bmc_csr m10bmc_pmci_csr = {
+	.base = M10BMC_PMCI_SYS_BASE,
+	.build_version = M10BMC_PMCI_BUILD_VER,
+	.fw_version = NIOS2_PMCI_FW_VERSION,
+	.mac_low = M10BMC_PMCI_MAC_LOW,
+	.mac_high = M10BMC_PMCI_MAC_HIGH,
+};
+
+static const struct m10bmc_csr m10bmc_spi_csr = {
+	.base = M10BMC_SYS_BASE,
+	.build_version = M10BMC_BUILD_VER,
+	.fw_version = NIOS2_FW_VERSION,
+	.mac_low = M10BMC_MAC_LOW,
+	.mac_high = M10BMC_MAC_HIGH,
+};
+
 static struct mfd_cell m10bmc_n6000_bmc_subdevs[] = {
 	{ .name = "n6000bmc-hwmon" },
 	{ .name = "n6000bmc-sec-update" }
@@ -36,7 +52,7 @@ static ssize_t bmc_version_show(struct device *dev,
 	unsigned int val;
 	int ret;
 
-	ret = m10bmc_sys_read(ddata, M10BMC_BUILD_VER, &val);
+	ret = m10bmc_sys_read(ddata, ddata->csr->build_version, &val);
 	if (ret)
 		return ret;
 
@@ -51,7 +67,7 @@ static ssize_t bmcfw_version_show(struct device *dev,
 	unsigned int val;
 	int ret;
 
-	ret = m10bmc_sys_read(ddata, NIOS2_FW_VERSION, &val);
+	ret = m10bmc_sys_read(ddata, ddata->csr->fw_version, &val);
 	if (ret)
 		return ret;
 
@@ -66,11 +82,11 @@ static ssize_t mac_address_show(struct device *dev,
 	unsigned int macaddr_low, macaddr_high;
 	int ret;
 
-	ret = m10bmc_sys_read(ddata, M10BMC_MAC_LOW, &macaddr_low);
+	ret = m10bmc_sys_read(ddata, ddata->csr->mac_low, &macaddr_low);
 	if (ret)
 		return ret;
 
-	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
+	ret = m10bmc_sys_read(ddata, ddata->csr->mac_high, &macaddr_high);
 	if (ret)
 		return ret;
 
@@ -91,7 +107,7 @@ static ssize_t mac_count_show(struct device *dev,
 	unsigned int macaddr_high;
 	int ret;
 
-	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
+	ret = m10bmc_sys_read(ddata, ddata->csr->mac_high, &macaddr_high);
 	if (ret)
 		return ret;
 
@@ -163,18 +179,22 @@ int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
 	case M10_N3000:
 		cells = m10bmc_pacn3000_subdevs;
 		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
+		m10bmc->csr = &m10bmc_spi_csr;
 		break;
 	case M10_D5005:
 		cells = m10bmc_d5005_subdevs;
 		n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs);
+		m10bmc->csr = &m10bmc_spi_csr;
 		break;
 	case M10_N5010:
 		cells = m10bmc_n5010_subdevs;
 		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
+		m10bmc->csr = &m10bmc_spi_csr;
 		break;
 	case M10_N6000:
 		cells = m10bmc_n6000_bmc_subdevs;
 		n_cell = ARRAY_SIZE(m10bmc_n6000_bmc_subdevs);
+		m10bmc->csr = &m10bmc_pmci_csr;
 		break;
 	default:
 		return -ENODEV;
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index 83c4d3993dcb..3a4fdab2acbd 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -125,6 +125,11 @@
 #define M10BMC_PMCI_TELEM_START		0x400
 #define M10BMC_PMCI_TELEM_END		0x78c
 
+#define M10BMC_PMCI_BUILD_VER   0x0
+#define NIOS2_PMCI_FW_VERSION   0x4
+#define M10BMC_PMCI_MAC_LOW    0x20
+#define M10BMC_PMCI_MAC_HIGH    0x24
+
 /* Supported MAX10 BMC types */
 enum m10bmc_type {
 	M10_N3000,
@@ -133,16 +138,29 @@ enum m10bmc_type {
 	M10_N6000
 };
 
+/**
+ * struct m10bmc_csr - Intel MAX 10 BMC CSR register
+ */
+struct m10bmc_csr {
+	unsigned int base;
+	unsigned int build_version;
+	unsigned int fw_version;
+	unsigned int mac_low;
+	unsigned int mac_high;
+};
+
 /**
  * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
  * @dev: this device
  * @regmap: the regmap used to access registers by m10bmc itself
  * @type: the type of MAX10 BMC
+ * @csr: the register definition of MAX10 BMC
  */
 struct intel_m10bmc {
 	struct device *dev;
 	struct regmap *regmap;
 	enum m10bmc_type type;
+	const struct m10bmc_csr *csr;
 };
 
 /*
@@ -174,7 +192,7 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
  * M10BMC_SYS_BASE accordingly.
  */
 #define m10bmc_sys_read(m10bmc, offset, val) \
-	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
+	m10bmc_raw_read(m10bmc, m10bmc->csr->base + (offset), val)
 
 /*
  * MAX10 BMC Core support
-- 
2.26.2

Re: [PATCH v2 4/4] mfd: intel-m10-bmc: support multiple register layouts

[Xu Yilun]

On Thu, Jun 16, 2022 at 10:04:05PM -0400, Tianfei Zhang wrote:
> There are different base addresses for the MAX10 CSR register.

Actually I see differences for each register, not only the register
base...

> Introducing a new data structure m10bmc_csr for the register
> definition of MAX10 CSR. Embedded m10bmc_csr into struct
> intel_m10bmc to support multiple register layouts.

Since the new BMC has different connections to host, different register
layouts, different sub devices. Actually I can hardly find anything to
share between them, so how about we just create a new driver for your
new BMC?

Thanks,
Yilun

> 
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
>  drivers/mfd/intel-m10-bmc-core.c  | 30 +++++++++++++++++++++++++-----
>  include/linux/mfd/intel-m10-bmc.h | 20 +++++++++++++++++++-
>  2 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/intel-m10-bmc-core.c b/drivers/mfd/intel-m10-bmc-core.c
> index c6a1a4c28357..f85f8e2aa9a1 100644
> --- a/drivers/mfd/intel-m10-bmc-core.c
> +++ b/drivers/mfd/intel-m10-bmc-core.c
> @@ -10,6 +10,22 @@
>  #include <linux/mfd/intel-m10-bmc.h>
>  #include <linux/module.h>
>  
> +static const struct m10bmc_csr m10bmc_pmci_csr = {
> +	.base = M10BMC_PMCI_SYS_BASE,
> +	.build_version = M10BMC_PMCI_BUILD_VER,
> +	.fw_version = NIOS2_PMCI_FW_VERSION,
> +	.mac_low = M10BMC_PMCI_MAC_LOW,
> +	.mac_high = M10BMC_PMCI_MAC_HIGH,
> +};
> +
> +static const struct m10bmc_csr m10bmc_spi_csr = {
> +	.base = M10BMC_SYS_BASE,
> +	.build_version = M10BMC_BUILD_VER,
> +	.fw_version = NIOS2_FW_VERSION,
> +	.mac_low = M10BMC_MAC_LOW,
> +	.mac_high = M10BMC_MAC_HIGH,
> +};
> +
>  static struct mfd_cell m10bmc_n6000_bmc_subdevs[] = {
>  	{ .name = "n6000bmc-hwmon" },
>  	{ .name = "n6000bmc-sec-update" }
> @@ -36,7 +52,7 @@ static ssize_t bmc_version_show(struct device *dev,
>  	unsigned int val;
>  	int ret;
>  
> -	ret = m10bmc_sys_read(ddata, M10BMC_BUILD_VER, &val);
> +	ret = m10bmc_sys_read(ddata, ddata->csr->build_version, &val);
>  	if (ret)
>  		return ret;
>  
> @@ -51,7 +67,7 @@ static ssize_t bmcfw_version_show(struct device *dev,
>  	unsigned int val;
>  	int ret;
>  
> -	ret = m10bmc_sys_read(ddata, NIOS2_FW_VERSION, &val);
> +	ret = m10bmc_sys_read(ddata, ddata->csr->fw_version, &val);
>  	if (ret)
>  		return ret;
>  
> @@ -66,11 +82,11 @@ static ssize_t mac_address_show(struct device *dev,
>  	unsigned int macaddr_low, macaddr_high;
>  	int ret;
>  
> -	ret = m10bmc_sys_read(ddata, M10BMC_MAC_LOW, &macaddr_low);
> +	ret = m10bmc_sys_read(ddata, ddata->csr->mac_low, &macaddr_low);
>  	if (ret)
>  		return ret;
>  
> -	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
> +	ret = m10bmc_sys_read(ddata, ddata->csr->mac_high, &macaddr_high);
>  	if (ret)
>  		return ret;
>  
> @@ -91,7 +107,7 @@ static ssize_t mac_count_show(struct device *dev,
>  	unsigned int macaddr_high;
>  	int ret;
>  
> -	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
> +	ret = m10bmc_sys_read(ddata, ddata->csr->mac_high, &macaddr_high);
>  	if (ret)
>  		return ret;
>  
> @@ -163,18 +179,22 @@ int m10bmc_dev_init(struct intel_m10bmc *m10bmc)
>  	case M10_N3000:
>  		cells = m10bmc_pacn3000_subdevs;
>  		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> +		m10bmc->csr = &m10bmc_spi_csr;
>  		break;
>  	case M10_D5005:
>  		cells = m10bmc_d5005_subdevs;
>  		n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs);
> +		m10bmc->csr = &m10bmc_spi_csr;
>  		break;
>  	case M10_N5010:
>  		cells = m10bmc_n5010_subdevs;
>  		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
> +		m10bmc->csr = &m10bmc_spi_csr;
>  		break;
>  	case M10_N6000:
>  		cells = m10bmc_n6000_bmc_subdevs;
>  		n_cell = ARRAY_SIZE(m10bmc_n6000_bmc_subdevs);
> +		m10bmc->csr = &m10bmc_pmci_csr;
>  		break;
>  	default:
>  		return -ENODEV;
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index 83c4d3993dcb..3a4fdab2acbd 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -125,6 +125,11 @@
>  #define M10BMC_PMCI_TELEM_START		0x400
>  #define M10BMC_PMCI_TELEM_END		0x78c
>  
> +#define M10BMC_PMCI_BUILD_VER   0x0
> +#define NIOS2_PMCI_FW_VERSION   0x4
> +#define M10BMC_PMCI_MAC_LOW    0x20
> +#define M10BMC_PMCI_MAC_HIGH    0x24
> +
>  /* Supported MAX10 BMC types */
>  enum m10bmc_type {
>  	M10_N3000,
> @@ -133,16 +138,29 @@ enum m10bmc_type {
>  	M10_N6000
>  };
>  
> +/**
> + * struct m10bmc_csr - Intel MAX 10 BMC CSR register
> + */
> +struct m10bmc_csr {
> +	unsigned int base;
> +	unsigned int build_version;
> +	unsigned int fw_version;
> +	unsigned int mac_low;
> +	unsigned int mac_high;
> +};
> +
>  /**
>   * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
>   * @dev: this device
>   * @regmap: the regmap used to access registers by m10bmc itself
>   * @type: the type of MAX10 BMC
> + * @csr: the register definition of MAX10 BMC
>   */
>  struct intel_m10bmc {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	enum m10bmc_type type;
> +	const struct m10bmc_csr *csr;
>  };
>  
>  /*
> @@ -174,7 +192,7 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
>   * M10BMC_SYS_BASE accordingly.
>   */
>  #define m10bmc_sys_read(m10bmc, offset, val) \
> -	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> +	m10bmc_raw_read(m10bmc, m10bmc->csr->base + (offset), val)
>  
>  /*
>   * MAX10 BMC Core support
> -- 
> 2.26.2

RE: [PATCH v2 4/4] mfd: intel-m10-bmc: support multiple register layouts

[Zhang, Tianfei]

> -----Original Message-----
> From: Xu, Yilun <yilun.xu@intel.com>
> Sent: Monday, June 20, 2022 10:19 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>
> Cc: lee.jones@linaro.org; Wu, Hao <hao.wu@intel.com>; trix@redhat.com;
> linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; Weight, Russell H
> <russell.h.weight@intel.com>; matthew.gerlach@linux.intel.com
> Subject: Re: [PATCH v2 4/4] mfd: intel-m10-bmc: support multiple register
> layouts
> 
> On Thu, Jun 16, 2022 at 10:04:05PM -0400, Tianfei Zhang wrote:
> > There are different base addresses for the MAX10 CSR register.
> 
> Actually I see differences for each register, not only the register base...
> 
> > Introducing a new data structure m10bmc_csr for the register
> > definition of MAX10 CSR. Embedded m10bmc_csr into struct intel_m10bmc
> > to support multiple register layouts.
> 
> Since the new BMC has different connections to host, different register layouts,
> different sub devices. Actually I can hardly find anything to share between them,
> so how about we just create a new driver for your new BMC?

There is two version of BMC for Intel PAC FPGA card, one is SPI bus interface other is PMCI bus interface.
They are different register layouts. But the working flow of some functionality  are very similar, and just slight different, for example,
access the BMC version/MAC address, the doorbell mechanism of read/write the flash and so on.

If we create a new driver, there are will be lots of duplicate code between old driver and new driver.

> 
> Thanks,
> Yilun
> 
> >
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> >  drivers/mfd/intel-m10-bmc-core.c  | 30 +++++++++++++++++++++++++-----
> > include/linux/mfd/intel-m10-bmc.h | 20 +++++++++++++++++++-
> >  2 files changed, 44 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mfd/intel-m10-bmc-core.c
> > b/drivers/mfd/intel-m10-bmc-core.c
> > index c6a1a4c28357..f85f8e2aa9a1 100644
> > --- a/drivers/mfd/intel-m10-bmc-core.c
> > +++ b/drivers/mfd/intel-m10-bmc-core.c
> > @@ -10,6 +10,22 @@
> >  #include <linux/mfd/intel-m10-bmc.h>
> >  #include <linux/module.h>
> >
> > +static const struct m10bmc_csr m10bmc_pmci_csr = {
> > +	.base = M10BMC_PMCI_SYS_BASE,
> > +	.build_version = M10BMC_PMCI_BUILD_VER,
> > +	.fw_version = NIOS2_PMCI_FW_VERSION,
> > +	.mac_low = M10BMC_PMCI_MAC_LOW,
> > +	.mac_high = M10BMC_PMCI_MAC_HIGH,
> > +};
> > +
> > +static const struct m10bmc_csr m10bmc_spi_csr = {
> > +	.base = M10BMC_SYS_BASE,
> > +	.build_version = M10BMC_BUILD_VER,
> > +	.fw_version = NIOS2_FW_VERSION,
> > +	.mac_low = M10BMC_MAC_LOW,
> > +	.mac_high = M10BMC_MAC_HIGH,
> > +};
> > +
> >  static struct mfd_cell m10bmc_n6000_bmc_subdevs[] = {
> >  	{ .name = "n6000bmc-hwmon" },
> >  	{ .name = "n6000bmc-sec-update" }
> > @@ -36,7 +52,7 @@ static ssize_t bmc_version_show(struct device *dev,
> >  	unsigned int val;
> >  	int ret;
> >
> > -	ret = m10bmc_sys_read(ddata, M10BMC_BUILD_VER, &val);
> > +	ret = m10bmc_sys_read(ddata, ddata->csr->build_version, &val);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -51,7 +67,7 @@ static ssize_t bmcfw_version_show(struct device *dev,
> >  	unsigned int val;
> >  	int ret;
> >
> > -	ret = m10bmc_sys_read(ddata, NIOS2_FW_VERSION, &val);
> > +	ret = m10bmc_sys_read(ddata, ddata->csr->fw_version, &val);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -66,11 +82,11 @@ static ssize_t mac_address_show(struct device *dev,
> >  	unsigned int macaddr_low, macaddr_high;
> >  	int ret;
> >
> > -	ret = m10bmc_sys_read(ddata, M10BMC_MAC_LOW, &macaddr_low);
> > +	ret = m10bmc_sys_read(ddata, ddata->csr->mac_low, &macaddr_low);
> >  	if (ret)
> >  		return ret;
> >
> > -	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
> > +	ret = m10bmc_sys_read(ddata, ddata->csr->mac_high,
> &macaddr_high);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -91,7 +107,7 @@ static ssize_t mac_count_show(struct device *dev,
> >  	unsigned int macaddr_high;
> >  	int ret;
> >
> > -	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
> > +	ret = m10bmc_sys_read(ddata, ddata->csr->mac_high,
> &macaddr_high);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -163,18 +179,22 @@ int m10bmc_dev_init(struct intel_m10bmc
> *m10bmc)
> >  	case M10_N3000:
> >  		cells = m10bmc_pacn3000_subdevs;
> >  		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> > +		m10bmc->csr = &m10bmc_spi_csr;
> >  		break;
> >  	case M10_D5005:
> >  		cells = m10bmc_d5005_subdevs;
> >  		n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs);
> > +		m10bmc->csr = &m10bmc_spi_csr;
> >  		break;
> >  	case M10_N5010:
> >  		cells = m10bmc_n5010_subdevs;
> >  		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
> > +		m10bmc->csr = &m10bmc_spi_csr;
> >  		break;
> >  	case M10_N6000:
> >  		cells = m10bmc_n6000_bmc_subdevs;
> >  		n_cell = ARRAY_SIZE(m10bmc_n6000_bmc_subdevs);
> > +		m10bmc->csr = &m10bmc_pmci_csr;
> >  		break;
> >  	default:
> >  		return -ENODEV;
> > diff --git a/include/linux/mfd/intel-m10-bmc.h
> > b/include/linux/mfd/intel-m10-bmc.h
> > index 83c4d3993dcb..3a4fdab2acbd 100644
> > --- a/include/linux/mfd/intel-m10-bmc.h
> > +++ b/include/linux/mfd/intel-m10-bmc.h
> > @@ -125,6 +125,11 @@
> >  #define M10BMC_PMCI_TELEM_START		0x400
> >  #define M10BMC_PMCI_TELEM_END		0x78c
> >
> > +#define M10BMC_PMCI_BUILD_VER   0x0
> > +#define NIOS2_PMCI_FW_VERSION   0x4
> > +#define M10BMC_PMCI_MAC_LOW    0x20
> > +#define M10BMC_PMCI_MAC_HIGH    0x24
> > +
> >  /* Supported MAX10 BMC types */
> >  enum m10bmc_type {
> >  	M10_N3000,
> > @@ -133,16 +138,29 @@ enum m10bmc_type {
> >  	M10_N6000
> >  };
> >
> > +/**
> > + * struct m10bmc_csr - Intel MAX 10 BMC CSR register  */ struct
> > +m10bmc_csr {
> > +	unsigned int base;
> > +	unsigned int build_version;
> > +	unsigned int fw_version;
> > +	unsigned int mac_low;
> > +	unsigned int mac_high;
> > +};
> > +
> >  /**
> >   * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
> >   * @dev: this device
> >   * @regmap: the regmap used to access registers by m10bmc itself
> >   * @type: the type of MAX10 BMC
> > + * @csr: the register definition of MAX10 BMC
> >   */
> >  struct intel_m10bmc {
> >  	struct device *dev;
> >  	struct regmap *regmap;
> >  	enum m10bmc_type type;
> > +	const struct m10bmc_csr *csr;
> >  };
> >
> >  /*
> > @@ -174,7 +192,7 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc,
> unsigned int addr,
> >   * M10BMC_SYS_BASE accordingly.
> >   */
> >  #define m10bmc_sys_read(m10bmc, offset, val) \
> > -	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
> > +	m10bmc_raw_read(m10bmc, m10bmc->csr->base + (offset), val)
> >
> >  /*
> >   * MAX10 BMC Core support
> > --
> > 2.26.2