[PATCH v5 00/10] intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support

[PATCH v5 00/10] intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support

[Ilpo Järvinen]

Hi all,

Here are the patches for MAX 10 BMC core/SPI interface split and
addition of the PMCI interface. There are a few supporting rearrangement
patches prior to the actual split. In this version, the indirect register
access became part of the BMC PMCI module.

The current downside of the split is that there's not that much code
remaining in the core part when all the type specific definitions are
moved to the file with the relevant interface.

The patch set is based on top of the "fpga: m10bmc-sec: Fix probe
rollback" and commit dfd10332596e ("fpga: m10bmc-sec: Fix kconfig
dependencies") to avoid triggering conflicts.

v5:
- Explain flash MUX rollback path in commit message
- Fix RSU status field register location

v4:
- Use board type for naming defines & structs
- Set .reg_read/write and call devm_regmap_init() directly
- Rename indirect_bus_*() funcs to indirect_*()
- Move indirect code into intel-m10-bmc-pmci so funcs can be static
- Drop module licence  GPL v2 - GPL change

v3:
- Move regmap indirect into BMC PMCI module
        - Get rid of "generalization" of cmd offsets and values, back to v1 #defines
        - Tweak Kconfig & Makefile
        - Rename intel-m10-bmc-pmci to intel-m10-bmc-pmci-main
- Rework sec update read/write paths
        - Sec update driver code effectively uses the SPI side code from v2
        - Rename m10bmc_sec_write() to m10bmc_sec_fw_write()
        - Rename flash_ops to flash_bulk_ops and make them optional
        - Move flash MUX and flash_mutex handling into sec update driver
        - Prevent simultaneous flash bulk write & read using flash_mutex
        - Keep M10BMC_STAGING_BASE in header (now used in the sec update code)
- Rebased on top of leak fix "fpga: m10bmc-sec: Fix probe rollback"

v2:
- Drop type from mfd side, the platform info takes care of differentation
- Explain introducing ->info to struct m10bmc in commit message (belongs logically there)
- Intro PMCI better
- Improve naming
        - Rename M10BMC_PMCI_FLASH_CTRL to M10BMC_PMCI_FLASH_MUX_CTRL
        - Use m10bmc_pmci/M10BMC_PMCI prefix consistently
        - Use M10BMC_SPI prefix for SPI related defines
        - Newly added stuff gets m10bmc_spi prefix
- Removed dev from struct m10bmc_pmci_device
- Rename "n_offset" variable to "offset" in PMCI flash ops
- Always issue idle command in regmap indirect to clear RD/WR/ACK bits
- Handle stride misaligned sizes in flash read/write ops


Ilpo Järvinen (10):
  mfd: intel-m10-bmc: Create m10bmc_platform_info for type specific info
  mfd: intel-m10-bmc: Rename the local variables
  mfd: intel-m10-bmc: Split into core and spi specific parts
  mfd: intel-m10-bmc: Support multiple CSR register layouts
  fpga: intel-m10-bmc: Rework flash read/write
  mfd: intel-m10-bmc: Downscope SPI defines & prefix with M10BMC_N3000
  fpga: m10bmc-sec: Create helpers for rsu status/progress checks
  fpga: m10bmc-sec: Differentiate rsu status from doorbell in csr map
  mfd: intel-m10-bmc: Add PMCI driver
  fpga: m10bmc-sec: Add support for N6000

 .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
 MAINTAINERS                                   |   2 +-
 drivers/fpga/Kconfig                          |   2 +-
 drivers/fpga/intel-m10-bmc-sec-update.c       | 386 +++++++++++------
 drivers/hwmon/Kconfig                         |   2 +-
 drivers/mfd/Kconfig                           |  32 +-
 drivers/mfd/Makefile                          |   5 +-
 drivers/mfd/intel-m10-bmc-core.c              | 133 ++++++
 drivers/mfd/intel-m10-bmc-pmci.c              | 398 ++++++++++++++++++
 drivers/mfd/intel-m10-bmc-spi.c               | 206 +++++++++
 drivers/mfd/intel-m10-bmc.c                   | 238 -----------
 include/linux/mfd/intel-m10-bmc.h             | 111 ++---
 12 files changed, 1094 insertions(+), 429 deletions(-)
 create mode 100644 drivers/mfd/intel-m10-bmc-core.c
 create mode 100644 drivers/mfd/intel-m10-bmc-pmci.c
 create mode 100644 drivers/mfd/intel-m10-bmc-spi.c
 delete mode 100644 drivers/mfd/intel-m10-bmc.c

-- 
2.30.2

[PATCH v5 01/10] mfd: intel-m10-bmc: Create m10bmc_platform_info for type specific info

[Ilpo Järvinen]

BMC type specific info is currently set by a switch/case block. The
size of this info is expected to grow as more dev types and features
are added which would have made the switch block bloaty.

Store type specific info into struct and place them into .driver_data
instead because it makes things a bit cleaner.

The m10bmc_type enum can be dropped as the differentiation is now
fully handled by the platform info.

The info member of struct intel_m10bmc that is added here is not used
yet in this change but its addition logically still belongs to this
change. The CSR map change that comes after this change needs to have
the info member.

Reviewed-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/mfd/intel-m10-bmc.c       | 53 ++++++++++++++-----------------
 include/linux/mfd/intel-m10-bmc.h | 12 +++++++
 2 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
index 7e3319e5b22f..12c522c16d83 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -13,12 +13,6 @@
 #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" },
 	{ .name = "d5005bmc-sec-update" }
@@ -162,15 +156,17 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
 static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 {
 	const struct spi_device_id *id = spi_get_device_id(spi);
+	const struct intel_m10bmc_platform_info *info;
 	struct device *dev = &spi->dev;
-	struct mfd_cell *cells;
 	struct intel_m10bmc *ddata;
-	int ret, n_cell;
+	int ret;
 
 	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
 	if (!ddata)
 		return -ENOMEM;
 
+	info = (struct intel_m10bmc_platform_info *)id->driver_data;
+	ddata->info = info;
 	ddata->dev = dev;
 
 	ddata->regmap =
@@ -189,24 +185,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	switch (id->driver_data) {
-	case M10_N3000:
-		cells = m10bmc_pacn3000_subdevs;
-		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
-		break;
-	case M10_D5005:
-		cells = m10bmc_d5005_subdevs;
-		n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs);
-		break;
-	case M10_N5010:
-		cells = m10bmc_n5010_subdevs;
-		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
-		break;
-	default:
-		return -ENODEV;
-	}
-
-	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
+	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
+				   info->cells, info->n_cells,
 				   NULL, 0, NULL);
 	if (ret)
 		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
@@ -214,10 +194,25 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 	return ret;
 }
 
+static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = {
+	.cells = m10bmc_pacn3000_subdevs,
+	.n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs),
+};
+
+static const struct intel_m10bmc_platform_info m10bmc_spi_d5005 = {
+	.cells = m10bmc_d5005_subdevs,
+	.n_cells = ARRAY_SIZE(m10bmc_d5005_subdevs),
+};
+
+static const struct intel_m10bmc_platform_info m10bmc_spi_n5010 = {
+	.cells = m10bmc_n5010_subdevs,
+	.n_cells = ARRAY_SIZE(m10bmc_n5010_subdevs),
+};
+
 static const struct spi_device_id m10bmc_spi_id[] = {
-	{ "m10-n3000", M10_N3000 },
-	{ "m10-d5005", M10_D5005 },
-	{ "m10-n5010", M10_N5010 },
+	{ "m10-n3000", (kernel_ulong_t)&m10bmc_spi_n3000 },
+	{ "m10-d5005", (kernel_ulong_t)&m10bmc_spi_d5005 },
+	{ "m10-n5010", (kernel_ulong_t)&m10bmc_spi_n5010 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index f0044b14136e..725b51ea4aee 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -118,14 +118,26 @@
 /* Address of 4KB inverted bit vector containing staging area FLASH count */
 #define STAGING_FLASH_COUNT	0x17ffb000
 
+/**
+ * struct intel_m10bmc_platform_info - Intel MAX 10 BMC platform specific information
+ * @cells: MFD cells
+ * @n_cells: MFD cells ARRAY_SIZE()
+ */
+struct intel_m10bmc_platform_info {
+	struct mfd_cell *cells;
+	int n_cells;
+};
+
 /**
  * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
  * @dev: this device
  * @regmap: the regmap used to access registers by m10bmc itself
+ * @info: the platform information for MAX10 BMC
  */
 struct intel_m10bmc {
 	struct device *dev;
 	struct regmap *regmap;
+	const struct intel_m10bmc_platform_info *info;
 };
 
 /*
-- 
2.30.2

[PATCH v5 02/10] mfd: intel-m10-bmc: Rename the local variables

[Ilpo Järvinen]

Local variables directly interact with dev_get_drvdata/dev_set_drvdata
should be named ddata.

Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
Reviewed-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.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 12c522c16d83..2c26203c4799 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -81,15 +81,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;
 
@@ -106,11 +106,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.30.2

[PATCH v5 03/10] mfd: intel-m10-bmc: Split into core and spi specific parts

[Ilpo Järvinen]

Split the common code from intel-m10-bmc driver into intel-m10-bmc-core
and move the SPI bus parts into an interface specific file.

intel-m10-bmc-core becomes the core MFD functions which can support
multiple bus interface like SPI bus.

Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
Reviewed-by: Russ Weight <russell.h.weight@intel.com>
Acked-by: Guenter Roeck <linux@roeck-us.net> # hwmon
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 MAINTAINERS                                   |   2 +-
 drivers/fpga/Kconfig                          |   2 +-
 drivers/hwmon/Kconfig                         |   2 +-
 drivers/mfd/Kconfig                           |  30 ++--
 drivers/mfd/Makefile                          |   4 +-
 drivers/mfd/intel-m10-bmc-core.c              | 122 +++++++++++++++++
 .../{intel-m10-bmc.c => intel-m10-bmc-spi.c}  | 128 +++---------------
 include/linux/mfd/intel-m10-bmc.h             |   6 +
 8 files changed, 172 insertions(+), 124 deletions(-)
 create mode 100644 drivers/mfd/intel-m10-bmc-core.c
 rename drivers/mfd/{intel-m10-bmc.c => intel-m10-bmc-spi.c} (59%)

diff --git a/MAINTAINERS b/MAINTAINERS
index cf0f18502372..ddfa4f8b3c80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10452,7 +10452,7 @@ S:	Maintained
 F:	Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
 F:	Documentation/hwmon/intel-m10-bmc-hwmon.rst
 F:	drivers/hwmon/intel-m10-bmc-hwmon.c
-F:	drivers/mfd/intel-m10-bmc.c
+F:	drivers/mfd/intel-m10-bmc*
 F:	include/linux/mfd/intel-m10-bmc.h
 
 INTEL MENLOW THERMAL DRIVER
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 6ce143dafd04..0a00763b9f28 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -246,7 +246,7 @@ config FPGA_MGR_VERSAL_FPGA
 
 config FPGA_M10_BMC_SEC_UPDATE
 	tristate "Intel MAX10 BMC Secure Update driver"
-	depends on MFD_INTEL_M10_BMC
+	depends on MFD_INTEL_M10_BMC_CORE
 	select FW_LOADER
 	select FW_UPLOAD
 	help
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ac3daaf59ce..984a55e0f313 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2321,7 +2321,7 @@ config SENSORS_XGENE
 
 config SENSORS_INTEL_M10_BMC_HWMON
 	tristate "Intel MAX10 BMC Hardware Monitoring"
-	depends on MFD_INTEL_M10_BMC
+	depends on MFD_INTEL_M10_BMC_CORE
 	help
 	  This driver provides support for the hardware monitoring functionality
 	  on Intel MAX10 BMC chip.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b93856de432..a09d4ac60dc7 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2219,18 +2219,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 7ed3ef4a698c..5d1f308ee2a7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -271,7 +271,9 @@ 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
+
+obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)	+= intel-m10-bmc-core.o
+obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)	+= intel-m10-bmc-spi.o
 
 obj-$(CONFIG_MFD_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
new file mode 100644
index 000000000000..6630b81b10c4
--- /dev/null
+++ b/drivers/mfd/intel-m10-bmc-core.c
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel MAX 10 Board Management Controller chip - common code
+ *
+ * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+
+static ssize_t bmc_version_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(ddata, M10BMC_BUILD_VER, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", val);
+}
+static DEVICE_ATTR_RO(bmc_version);
+
+static ssize_t bmcfw_version_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = m10bmc_sys_read(ddata, NIOS2_FW_VERSION, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "0x%x\n", val);
+}
+static DEVICE_ATTR_RO(bmcfw_version);
+
+static ssize_t mac_address_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
+	unsigned int macaddr_low, macaddr_high;
+	int ret;
+
+	ret = m10bmc_sys_read(ddata, M10BMC_MAC_LOW, &macaddr_low);
+	if (ret)
+		return ret;
+
+	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+			  (u8)FIELD_GET(M10BMC_MAC_BYTE1, macaddr_low),
+			  (u8)FIELD_GET(M10BMC_MAC_BYTE2, macaddr_low),
+			  (u8)FIELD_GET(M10BMC_MAC_BYTE3, macaddr_low),
+			  (u8)FIELD_GET(M10BMC_MAC_BYTE4, macaddr_low),
+			  (u8)FIELD_GET(M10BMC_MAC_BYTE5, macaddr_high),
+			  (u8)FIELD_GET(M10BMC_MAC_BYTE6, macaddr_high));
+}
+static DEVICE_ATTR_RO(mac_address);
+
+static ssize_t mac_count_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
+	unsigned int macaddr_high;
+	int ret;
+
+	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", (u8)FIELD_GET(M10BMC_MAC_COUNT, macaddr_high));
+}
+static DEVICE_ATTR_RO(mac_count);
+
+static struct attribute *m10bmc_attrs[] = {
+	&dev_attr_bmc_version.attr,
+	&dev_attr_bmcfw_version.attr,
+	&dev_attr_mac_address.attr,
+	&dev_attr_mac_count.attr,
+	NULL,
+};
+
+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);
+
+int m10bmc_dev_init(struct intel_m10bmc *m10bmc, const struct intel_m10bmc_platform_info *info)
+{
+	int ret;
+
+	m10bmc->info = info;
+	dev_set_drvdata(m10bmc->dev, m10bmc);
+
+	ret = devm_mfd_add_devices(m10bmc->dev, PLATFORM_DEVID_AUTO,
+				   info->cells, info->n_cells,
+				   NULL, 0, NULL);
+	if (ret)
+		dev_err(m10bmc->dev, "Failed to register sub-devices: %d\n", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(m10bmc_dev_init);
+
+MODULE_DESCRIPTION("Intel MAX 10 BMC core MFD driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc-spi.c
similarity index 59%
rename from drivers/mfd/intel-m10-bmc.c
rename to drivers/mfd/intel-m10-bmc-spi.c
index 2c26203c4799..be1d4ddedabb 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc-spi.c
@@ -5,29 +5,14 @@
  * Copyright (C) 2018-2020 Intel Corporation. All rights reserved.
  */
 #include <linux/bitfield.h>
+#include <linux/dev_printk.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>
 
-static struct mfd_cell m10bmc_d5005_subdevs[] = {
-	{ .name = "d5005bmc-hwmon" },
-	{ .name = "d5005bmc-sec-update" }
-};
-
-static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
-	{ .name = "n3000bmc-hwmon" },
-	{ .name = "n3000bmc-retimer" },
-	{ .name = "n3000bmc-sec-update" },
-};
-
-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),
@@ -48,86 +33,6 @@ static struct regmap_config intel_m10bmc_regmap_config = {
 	.max_register = M10BMC_MEM_END,
 };
 
-static ssize_t bmc_version_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
-	unsigned int val;
-	int ret;
-
-	ret = m10bmc_sys_read(ddata, M10BMC_BUILD_VER, &val);
-	if (ret)
-		return ret;
-
-	return sprintf(buf, "0x%x\n", val);
-}
-static DEVICE_ATTR_RO(bmc_version);
-
-static ssize_t bmcfw_version_show(struct device *dev,
-				  struct device_attribute *attr, char *buf)
-{
-	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
-	unsigned int val;
-	int ret;
-
-	ret = m10bmc_sys_read(ddata, NIOS2_FW_VERSION, &val);
-	if (ret)
-		return ret;
-
-	return sprintf(buf, "0x%x\n", val);
-}
-static DEVICE_ATTR_RO(bmcfw_version);
-
-static ssize_t mac_address_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
-	unsigned int macaddr_low, macaddr_high;
-	int ret;
-
-	ret = m10bmc_sys_read(ddata, M10BMC_MAC_LOW, &macaddr_low);
-	if (ret)
-		return ret;
-
-	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%02x:%02x:%02x:%02x:%02x:%02x\n",
-			  (u8)FIELD_GET(M10BMC_MAC_BYTE1, macaddr_low),
-			  (u8)FIELD_GET(M10BMC_MAC_BYTE2, macaddr_low),
-			  (u8)FIELD_GET(M10BMC_MAC_BYTE3, macaddr_low),
-			  (u8)FIELD_GET(M10BMC_MAC_BYTE4, macaddr_low),
-			  (u8)FIELD_GET(M10BMC_MAC_BYTE5, macaddr_high),
-			  (u8)FIELD_GET(M10BMC_MAC_BYTE6, macaddr_high));
-}
-static DEVICE_ATTR_RO(mac_address);
-
-static ssize_t mac_count_show(struct device *dev,
-			      struct device_attribute *attr, char *buf)
-{
-	struct intel_m10bmc *ddata = dev_get_drvdata(dev);
-	unsigned int macaddr_high;
-	int ret;
-
-	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
-	if (ret)
-		return ret;
-
-	return sysfs_emit(buf, "%u\n",
-			  (u8)FIELD_GET(M10BMC_MAC_COUNT, macaddr_high));
-}
-static DEVICE_ATTR_RO(mac_count);
-
-static struct attribute *m10bmc_attrs[] = {
-	&dev_attr_bmc_version.attr,
-	&dev_attr_bmcfw_version.attr,
-	&dev_attr_mac_address.attr,
-	&dev_attr_mac_count.attr,
-	NULL,
-};
-ATTRIBUTE_GROUPS(m10bmc);
-
 static int check_m10bmc_version(struct intel_m10bmc *ddata)
 {
 	unsigned int v;
@@ -166,11 +71,9 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	info = (struct intel_m10bmc_platform_info *)id->driver_data;
-	ddata->info = info;
 	ddata->dev = dev;
 
-	ddata->regmap =
-		devm_regmap_init_spi_avmm(spi, &intel_m10bmc_regmap_config);
+	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);
@@ -185,15 +88,24 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
-				   info->cells, info->n_cells,
-				   NULL, 0, NULL);
-	if (ret)
-		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
-
-	return ret;
+	return m10bmc_dev_init(ddata, info);
 }
 
+static struct mfd_cell m10bmc_d5005_subdevs[] = {
+	{ .name = "d5005bmc-hwmon" },
+	{ .name = "d5005bmc-sec-update" },
+};
+
+static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
+	{ .name = "n3000bmc-hwmon" },
+	{ .name = "n3000bmc-retimer" },
+	{ .name = "n3000bmc-sec-update" },
+};
+
+static struct mfd_cell m10bmc_n5010_subdevs[] = {
+	{ .name = "n5010bmc-hwmon" },
+};
+
 static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = {
 	.cells = m10bmc_pacn3000_subdevs,
 	.n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs),
@@ -220,14 +132,14 @@ MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
 static struct spi_driver intel_m10bmc_spi_driver = {
 	.driver = {
 		.name = "intel-m10-bmc",
-		.dev_groups = m10bmc_groups,
+		.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 Device Driver");
+MODULE_DESCRIPTION("Intel MAX 10 BMC SPI bus interface");
 MODULE_AUTHOR("Intel Corporation");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("spi:intel-m10-bmc");
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index 725b51ea4aee..82e0820e146e 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -171,4 +171,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, const struct intel_m10bmc_platform_info *info);
+extern const struct attribute_group *m10bmc_dev_groups[];
+
 #endif /* __MFD_INTEL_M10_BMC_H */
-- 
2.30.2

[PATCH v5 04/10] mfd: intel-m10-bmc: Support multiple CSR register layouts

[Ilpo Järvinen]

There are different addresses for the MAX10 CSR registers. Introducing
a new data structure m10bmc_csr_map for the register definition of
MAX10 CSR.

Provide the csr_map for SPI.

Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
Reviewed-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/fpga/intel-m10-bmc-sec-update.c | 73 +++++++++++++++++--------
 drivers/mfd/intel-m10-bmc-core.c        | 10 ++--
 drivers/mfd/intel-m10-bmc-spi.c         | 24 ++++++++
 include/linux/mfd/intel-m10-bmc.h       | 39 +++++++++++--
 4 files changed, 113 insertions(+), 33 deletions(-)

diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
index 03f1bd81c434..798c1828899b 100644
--- a/drivers/fpga/intel-m10-bmc-sec-update.c
+++ b/drivers/fpga/intel-m10-bmc-sec-update.c
@@ -73,16 +73,24 @@ show_root_entry_hash(struct device *dev, u32 exp_magic,
 	return cnt;
 }
 
-#define DEVICE_ATTR_SEC_REH_RO(_name, _magic, _prog_addr, _reh_addr) \
+#define DEVICE_ATTR_SEC_REH_RO(_name)						\
 static ssize_t _name##_root_entry_hash_show(struct device *dev, \
 					    struct device_attribute *attr, \
 					    char *buf) \
-{ return show_root_entry_hash(dev, _magic, _prog_addr, _reh_addr, buf); } \
+{										\
+	struct m10bmc_sec *sec = dev_get_drvdata(dev);				\
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;	\
+										\
+	return show_root_entry_hash(dev, csr_map->_name##_magic,		\
+				    csr_map->_name##_prog_addr,			\
+				    csr_map->_name##_reh_addr,			\
+				    buf);					\
+}										\
 static DEVICE_ATTR_RO(_name##_root_entry_hash)
 
-DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
-DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
-DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
+DEVICE_ATTR_SEC_REH_RO(bmc);
+DEVICE_ATTR_SEC_REH_RO(sr);
+DEVICE_ATTR_SEC_REH_RO(pr);
 
 #define CSK_BIT_LEN		128U
 #define CSK_32ARRAY_SIZE	DIV_ROUND_UP(CSK_BIT_LEN, 32)
@@ -122,18 +130,25 @@ show_canceled_csk(struct device *dev, u32 addr, char *buf)
 	return bitmap_print_to_pagebuf(1, buf, csk_map, CSK_BIT_LEN);
 }
 
-#define DEVICE_ATTR_SEC_CSK_RO(_name, _addr) \
+#define DEVICE_ATTR_SEC_CSK_RO(_name)						\
 static ssize_t _name##_canceled_csks_show(struct device *dev, \
 					  struct device_attribute *attr, \
 					  char *buf) \
-{ return show_canceled_csk(dev, _addr, buf); } \
+{										\
+	struct m10bmc_sec *sec = dev_get_drvdata(dev);				\
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;	\
+										\
+	return show_canceled_csk(dev,						\
+				 csr_map->_name##_prog_addr + CSK_VEC_OFFSET,	\
+				 buf);						\
+}										\
 static DEVICE_ATTR_RO(_name##_canceled_csks)
 
 #define CSK_VEC_OFFSET 0x34
 
-DEVICE_ATTR_SEC_CSK_RO(bmc, BMC_PROG_ADDR + CSK_VEC_OFFSET);
-DEVICE_ATTR_SEC_CSK_RO(sr, SR_PROG_ADDR + CSK_VEC_OFFSET);
-DEVICE_ATTR_SEC_CSK_RO(pr, PR_PROG_ADDR + CSK_VEC_OFFSET);
+DEVICE_ATTR_SEC_CSK_RO(bmc);
+DEVICE_ATTR_SEC_CSK_RO(sr);
+DEVICE_ATTR_SEC_CSK_RO(pr);
 
 #define FLASH_COUNT_SIZE 4096	/* count stored as inverted bit vector */
 
@@ -141,6 +156,7 @@ static ssize_t flash_count_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
 	struct m10bmc_sec *sec = dev_get_drvdata(dev);
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	unsigned int stride, num_bits;
 	u8 *flash_buf;
 	int cnt, ret;
@@ -160,12 +176,12 @@ static ssize_t flash_count_show(struct device *dev,
 	if (!flash_buf)
 		return -ENOMEM;
 
-	ret = regmap_bulk_read(sec->m10bmc->regmap, STAGING_FLASH_COUNT,
+	ret = regmap_bulk_read(sec->m10bmc->regmap, csr_map->rsu_update_counter,
 			       flash_buf, FLASH_COUNT_SIZE / stride);
 	if (ret) {
 		dev_err(sec->dev,
 			"failed to read flash count: %x cnt %x: %d\n",
-			STAGING_FLASH_COUNT, FLASH_COUNT_SIZE / stride, ret);
+			csr_map->rsu_update_counter, FLASH_COUNT_SIZE / stride, ret);
 		goto exit_free;
 	}
 	cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
@@ -200,20 +216,22 @@ static const struct attribute_group *m10bmc_sec_attr_groups[] = {
 
 static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
 {
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	u32 auth_result;
 
 	dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell);
 
-	if (!m10bmc_sys_read(sec->m10bmc, M10BMC_AUTH_RESULT, &auth_result))
+	if (!m10bmc_sys_read(sec->m10bmc, csr_map->auth_result, &auth_result))
 		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
 }
 
 static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
 {
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	u32 doorbell;
 	int ret;
 
-	ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, &doorbell);
 	if (ret)
 		return FW_UPLOAD_ERR_RW_ERROR;
 
@@ -246,11 +264,12 @@ static inline bool rsu_start_done(u32 doorbell)
 
 static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
 {
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	u32 doorbell, status;
 	int ret;
 
 	ret = regmap_update_bits(sec->m10bmc->regmap,
-				 M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				 csr_map->base + csr_map->doorbell,
 				 DRBL_RSU_REQUEST | DRBL_HOST_STATUS,
 				 DRBL_RSU_REQUEST |
 				 FIELD_PREP(DRBL_HOST_STATUS,
@@ -259,7 +278,7 @@ static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
 		return FW_UPLOAD_ERR_RW_ERROR;
 
 	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
-				       M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				       csr_map->base + csr_map->doorbell,
 				       doorbell,
 				       rsu_start_done(doorbell),
 				       NIOS_HANDSHAKE_INTERVAL_US,
@@ -286,11 +305,12 @@ static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
 
 static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec)
 {
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	unsigned long poll_timeout;
 	u32 doorbell, progress;
 	int ret;
 
-	ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, &doorbell);
 	if (ret)
 		return FW_UPLOAD_ERR_RW_ERROR;
 
@@ -300,7 +320,7 @@ static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec)
 		if (time_after(jiffies, poll_timeout))
 			break;
 
-		ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+		ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, &doorbell);
 		if (ret)
 			return FW_UPLOAD_ERR_RW_ERROR;
 	}
@@ -319,11 +339,12 @@ static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec)
 
 static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
 {
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	u32 doorbell;
 	int ret;
 
 	ret = regmap_update_bits(sec->m10bmc->regmap,
-				 M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				 csr_map->base + csr_map->doorbell,
 				 DRBL_HOST_STATUS,
 				 FIELD_PREP(DRBL_HOST_STATUS,
 					    HOST_STATUS_WRITE_DONE));
@@ -331,7 +352,7 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
 		return FW_UPLOAD_ERR_RW_ERROR;
 
 	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
-				       M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				       csr_map->base + csr_map->doorbell,
 				       doorbell,
 				       rsu_prog(doorbell) != RSU_PROG_READY,
 				       NIOS_HANDSHAKE_INTERVAL_US,
@@ -360,7 +381,9 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
 
 static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell)
 {
-	if (m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, doorbell))
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
+
+	if (m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell))
 		return -EIO;
 
 	switch (rsu_stat(*doorbell)) {
@@ -389,10 +412,11 @@ static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell)
 
 static enum fw_upload_err rsu_cancel(struct m10bmc_sec *sec)
 {
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	u32 doorbell;
 	int ret;
 
-	ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, &doorbell);
 	if (ret)
 		return FW_UPLOAD_ERR_RW_ERROR;
 
@@ -400,7 +424,7 @@ static enum fw_upload_err rsu_cancel(struct m10bmc_sec *sec)
 		return FW_UPLOAD_ERR_BUSY;
 
 	ret = regmap_update_bits(sec->m10bmc->regmap,
-				 M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				 csr_map->base + csr_map->doorbell,
 				 DRBL_HOST_STATUS,
 				 FIELD_PREP(DRBL_HOST_STATUS,
 					    HOST_STATUS_ABORT_RSU));
@@ -445,6 +469,7 @@ static enum fw_upload_err m10bmc_sec_write(struct fw_upload *fwl, const u8 *data
 					   u32 offset, u32 size, u32 *written)
 {
 	struct m10bmc_sec *sec = fwl->dd_handle;
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	u32 blk_size, doorbell, extra_offset;
 	unsigned int stride, extra = 0;
 	int ret;
@@ -453,7 +478,7 @@ static enum fw_upload_err m10bmc_sec_write(struct fw_upload *fwl, const u8 *data
 	if (sec->cancel_request)
 		return rsu_cancel(sec);
 
-	ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, &doorbell);
 	if (ret) {
 		return FW_UPLOAD_ERR_RW_ERROR;
 	} else if (rsu_prog(doorbell) != RSU_PROG_READY) {
diff --git a/drivers/mfd/intel-m10-bmc-core.c b/drivers/mfd/intel-m10-bmc-core.c
index 6630b81b10c4..51b78b868235 100644
--- a/drivers/mfd/intel-m10-bmc-core.c
+++ b/drivers/mfd/intel-m10-bmc-core.c
@@ -19,7 +19,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->info->csr_map->build_version, &val);
 	if (ret)
 		return ret;
 
@@ -34,7 +34,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->info->csr_map->fw_version, &val);
 	if (ret)
 		return ret;
 
@@ -49,11 +49,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->info->csr_map->mac_low, &macaddr_low);
 	if (ret)
 		return ret;
 
-	ret = m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
+	ret = m10bmc_sys_read(ddata, ddata->info->csr_map->mac_high, &macaddr_high);
 	if (ret)
 		return ret;
 
@@ -74,7 +74,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->info->csr_map->mac_high, &macaddr_high);
 	if (ret)
 		return ret;
 
diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
index be1d4ddedabb..e8986154e965 100644
--- a/drivers/mfd/intel-m10-bmc-spi.c
+++ b/drivers/mfd/intel-m10-bmc-spi.c
@@ -91,6 +91,27 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 	return m10bmc_dev_init(ddata, info);
 }
 
+static const struct m10bmc_csr_map m10bmc_n3000_csr_map = {
+	.base = M10BMC_SYS_BASE,
+	.build_version = M10BMC_BUILD_VER,
+	.fw_version = NIOS2_FW_VERSION,
+	.mac_low = M10BMC_MAC_LOW,
+	.mac_high = M10BMC_MAC_HIGH,
+	.doorbell = M10BMC_DOORBELL,
+	.auth_result = M10BMC_AUTH_RESULT,
+	.rsu_status = M10BMC_DOORBELL,
+	.bmc_prog_addr = BMC_PROG_ADDR,
+	.bmc_reh_addr = BMC_REH_ADDR,
+	.bmc_magic = BMC_PROG_MAGIC,
+	.sr_prog_addr = SR_PROG_ADDR,
+	.sr_reh_addr = SR_REH_ADDR,
+	.sr_magic = SR_PROG_MAGIC,
+	.pr_prog_addr = PR_PROG_ADDR,
+	.pr_reh_addr = PR_REH_ADDR,
+	.pr_magic = PR_PROG_MAGIC,
+	.rsu_update_counter = STAGING_FLASH_COUNT,
+};
+
 static struct mfd_cell m10bmc_d5005_subdevs[] = {
 	{ .name = "d5005bmc-hwmon" },
 	{ .name = "d5005bmc-sec-update" },
@@ -109,16 +130,19 @@ static struct mfd_cell m10bmc_n5010_subdevs[] = {
 static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = {
 	.cells = m10bmc_pacn3000_subdevs,
 	.n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs),
+	.csr_map = &m10bmc_n3000_csr_map,
 };
 
 static const struct intel_m10bmc_platform_info m10bmc_spi_d5005 = {
 	.cells = m10bmc_d5005_subdevs,
 	.n_cells = ARRAY_SIZE(m10bmc_d5005_subdevs),
+	.csr_map = &m10bmc_n3000_csr_map,
 };
 
 static const struct intel_m10bmc_platform_info m10bmc_spi_n5010 = {
 	.cells = m10bmc_n5010_subdevs,
 	.n_cells = ARRAY_SIZE(m10bmc_n5010_subdevs),
+	.csr_map = &m10bmc_n3000_csr_map,
 };
 
 static const struct spi_device_id m10bmc_spi_id[] = {
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index 82e0820e146e..a009dd82f698 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -118,14 +118,40 @@
 /* Address of 4KB inverted bit vector containing staging area FLASH count */
 #define STAGING_FLASH_COUNT	0x17ffb000
 
+/**
+ * struct m10bmc_csr_map - Intel MAX 10 BMC CSR register map
+ */
+struct m10bmc_csr_map {
+	unsigned int base;
+	unsigned int build_version;
+	unsigned int fw_version;
+	unsigned int mac_low;
+	unsigned int mac_high;
+	unsigned int doorbell;
+	unsigned int auth_result;
+	unsigned int rsu_status;
+	unsigned int bmc_prog_addr;
+	unsigned int bmc_reh_addr;
+	unsigned int bmc_magic;
+	unsigned int sr_prog_addr;
+	unsigned int sr_reh_addr;
+	unsigned int sr_magic;
+	unsigned int pr_prog_addr;
+	unsigned int pr_reh_addr;
+	unsigned int pr_magic;
+	unsigned int rsu_update_counter;
+};
+
 /**
  * struct intel_m10bmc_platform_info - Intel MAX 10 BMC platform specific information
  * @cells: MFD cells
  * @n_cells: MFD cells ARRAY_SIZE()
+ * @csr_map: the mappings for register definition of MAX10 BMC
  */
 struct intel_m10bmc_platform_info {
 	struct mfd_cell *cells;
 	int n_cells;
+	const struct m10bmc_csr_map *csr_map;
 };
 
 /**
@@ -164,12 +190,17 @@ m10bmc_raw_read(struct intel_m10bmc *m10bmc, unsigned int addr,
  * The base of the system registers could be configured by HW developers, and
  * in HW SPEC, the base is not added to the addresses of the system registers.
  *
- * This macro helps to simplify the accessing of the system registers. And if
+ * This function helps to simplify the accessing of the system registers. And if
  * the base is reconfigured in HW, SW developers could simply change the
- * M10BMC_SYS_BASE accordingly.
+ * csr_map's base accordingly.
  */
-#define m10bmc_sys_read(m10bmc, offset, val) \
-	m10bmc_raw_read(m10bmc, M10BMC_SYS_BASE + (offset), val)
+static inline int m10bmc_sys_read(struct intel_m10bmc *m10bmc, unsigned int offset,
+				  unsigned int *val)
+{
+	const struct m10bmc_csr_map *csr_map = m10bmc->info->csr_map;
+
+	return m10bmc_raw_read(m10bmc, csr_map->base + offset, val);
+}
 
 /*
  * MAX10 BMC Core support
-- 
2.30.2

[PATCH v5 05/10] fpga: intel-m10-bmc: Rework flash read/write

[Ilpo Järvinen]

Access to flash staging area is different for N6000 from that of the
SPI interfaced counterparts. To make it easier to differentiate flash
access path, move read/write into new functions where the new access
path can be easily placed into. Rework the unaligned access such the
behavior it matches for both read and write.

This change also renames m10bmc_sec_write() to m10bmc_sec_fw_write() as
it would have a name conflict otherwise.

Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
Co-developed-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Acked-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/fpga/intel-m10-bmc-sec-update.c | 143 +++++++++++++-----------
 1 file changed, 79 insertions(+), 64 deletions(-)

diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
index 798c1828899b..9922027856a4 100644
--- a/drivers/fpga/intel-m10-bmc-sec-update.c
+++ b/drivers/fpga/intel-m10-bmc-sec-update.c
@@ -31,6 +31,65 @@ static DEFINE_XARRAY_ALLOC(fw_upload_xa);
 #define REH_MAGIC		GENMASK(15, 0)
 #define REH_SHA_NUM_BYTES	GENMASK(31, 16)
 
+static int m10bmc_sec_write(struct m10bmc_sec *sec, const u8 *buf, u32 offset, u32 size)
+{
+	struct intel_m10bmc *m10bmc = sec->m10bmc;
+	unsigned int stride = regmap_get_reg_stride(m10bmc->regmap);
+	u32 write_count = size / stride;
+	u32 leftover_offset = write_count * stride;
+	u32 leftover_size = size - leftover_offset;
+	u32 leftover_tmp = 0;
+	int ret;
+
+	if (WARN_ON_ONCE(stride > sizeof(leftover_tmp)))
+		return -EINVAL;
+
+	ret = regmap_bulk_write(m10bmc->regmap, M10BMC_STAGING_BASE + offset,
+				buf + offset, write_count);
+	if (ret)
+		return ret;
+
+	/* If size is not aligned to stride, handle the remainder bytes with regmap_write() */
+	if (leftover_size) {
+		memcpy(&leftover_tmp, buf + leftover_offset, leftover_size);
+		ret = regmap_write(m10bmc->regmap, M10BMC_STAGING_BASE + offset + leftover_offset,
+				   leftover_tmp);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int m10bmc_sec_read(struct m10bmc_sec *sec, u8 *buf, u32 addr, u32 size)
+{
+	struct intel_m10bmc *m10bmc = sec->m10bmc;
+	unsigned int stride = regmap_get_reg_stride(m10bmc->regmap);
+	u32 read_count = size / stride;
+	u32 leftover_offset = read_count * stride;
+	u32 leftover_size = size - leftover_offset;
+	u32 leftover_tmp;
+	int ret;
+
+	if (WARN_ON_ONCE(stride > sizeof(leftover_tmp)))
+		return -EINVAL;
+
+	ret = regmap_bulk_read(m10bmc->regmap, addr, buf, read_count);
+	if (ret)
+		return ret;
+
+	/* If size is not aligned to stride, handle the remainder bytes with regmap_read() */
+	if (leftover_size) {
+		ret = regmap_read(m10bmc->regmap, addr + leftover_offset, &leftover_tmp);
+		if (ret)
+			return ret;
+		memcpy(buf + leftover_offset, &leftover_tmp, leftover_size);
+	}
+
+	return 0;
+}
+
+
 static ssize_t
 show_root_entry_hash(struct device *dev, u32 exp_magic,
 		     u32 prog_addr, u32 reh_addr, char *buf)
@@ -38,11 +97,9 @@ show_root_entry_hash(struct device *dev, u32 exp_magic,
 	struct m10bmc_sec *sec = dev_get_drvdata(dev);
 	int sha_num_bytes, i, ret, cnt = 0;
 	u8 hash[REH_SHA384_SIZE];
-	unsigned int stride;
 	u32 magic;
 
-	stride = regmap_get_reg_stride(sec->m10bmc->regmap);
-	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
+	ret = m10bmc_sec_read(sec, (u8 *)&magic, prog_addr, sizeof(magic));
 	if (ret)
 		return ret;
 
@@ -50,19 +107,16 @@ show_root_entry_hash(struct device *dev, u32 exp_magic,
 		return sysfs_emit(buf, "hash not programmed\n");
 
 	sha_num_bytes = FIELD_GET(REH_SHA_NUM_BYTES, magic) / 8;
-	if ((sha_num_bytes % stride) ||
-	    (sha_num_bytes != REH_SHA256_SIZE &&
-	     sha_num_bytes != REH_SHA384_SIZE))   {
+	if (sha_num_bytes != REH_SHA256_SIZE &&
+	    sha_num_bytes != REH_SHA384_SIZE) {
 		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
 			sha_num_bytes);
 		return -EINVAL;
 	}
 
-	ret = regmap_bulk_read(sec->m10bmc->regmap, reh_addr,
-			       hash, sha_num_bytes / stride);
+	ret = m10bmc_sec_read(sec, hash, reh_addr, sha_num_bytes);
 	if (ret) {
-		dev_err(dev, "failed to read root entry hash: %x cnt %x: %d\n",
-			reh_addr, sha_num_bytes / stride, ret);
+		dev_err(dev, "failed to read root entry hash\n");
 		return ret;
 	}
 
@@ -98,27 +152,16 @@ DEVICE_ATTR_SEC_REH_RO(pr);
 static ssize_t
 show_canceled_csk(struct device *dev, u32 addr, char *buf)
 {
-	unsigned int i, stride, size = CSK_32ARRAY_SIZE * sizeof(u32);
+	unsigned int i, size = CSK_32ARRAY_SIZE * sizeof(u32);
 	struct m10bmc_sec *sec = dev_get_drvdata(dev);
 	DECLARE_BITMAP(csk_map, CSK_BIT_LEN);
 	__le32 csk_le32[CSK_32ARRAY_SIZE];
 	u32 csk32[CSK_32ARRAY_SIZE];
 	int ret;
 
-	stride = regmap_get_reg_stride(sec->m10bmc->regmap);
-	if (size % stride) {
-		dev_err(sec->dev,
-			"CSK vector size (0x%x) not aligned to stride (0x%x)\n",
-			size, stride);
-		WARN_ON_ONCE(1);
-		return -EINVAL;
-	}
-
-	ret = regmap_bulk_read(sec->m10bmc->regmap, addr, csk_le32,
-			       size / stride);
+	ret = m10bmc_sec_read(sec, (u8 *)&csk_le32, addr, size);
 	if (ret) {
-		dev_err(sec->dev, "failed to read CSK vector: %x cnt %x: %d\n",
-			addr, size / stride, ret);
+		dev_err(sec->dev, "failed to read CSK vector\n");
 		return ret;
 	}
 
@@ -157,31 +200,20 @@ static ssize_t flash_count_show(struct device *dev,
 {
 	struct m10bmc_sec *sec = dev_get_drvdata(dev);
 	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
-	unsigned int stride, num_bits;
+	unsigned int num_bits;
 	u8 *flash_buf;
 	int cnt, ret;
 
-	stride = regmap_get_reg_stride(sec->m10bmc->regmap);
 	num_bits = FLASH_COUNT_SIZE * 8;
 
-	if (FLASH_COUNT_SIZE % stride) {
-		dev_err(sec->dev,
-			"FLASH_COUNT_SIZE (0x%x) not aligned to stride (0x%x)\n",
-			FLASH_COUNT_SIZE, stride);
-		WARN_ON_ONCE(1);
-		return -EINVAL;
-	}
-
 	flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
 	if (!flash_buf)
 		return -ENOMEM;
 
-	ret = regmap_bulk_read(sec->m10bmc->regmap, csr_map->rsu_update_counter,
-			       flash_buf, FLASH_COUNT_SIZE / stride);
+	ret = m10bmc_sec_read(sec, flash_buf, csr_map->rsu_update_counter,
+			      FLASH_COUNT_SIZE);
 	if (ret) {
-		dev_err(sec->dev,
-			"failed to read flash count: %x cnt %x: %d\n",
-			csr_map->rsu_update_counter, FLASH_COUNT_SIZE / stride, ret);
+		dev_err(sec->dev, "failed to read flash count\n");
 		goto exit_free;
 	}
 	cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
@@ -465,20 +497,19 @@ static enum fw_upload_err m10bmc_sec_prepare(struct fw_upload *fwl,
 
 #define WRITE_BLOCK_SIZE 0x4000	/* Default write-block size is 0x4000 bytes */
 
-static enum fw_upload_err m10bmc_sec_write(struct fw_upload *fwl, const u8 *data,
-					   u32 offset, u32 size, u32 *written)
+static enum fw_upload_err m10bmc_sec_fw_write(struct fw_upload *fwl, const u8 *data,
+					      u32 offset, u32 size, u32 *written)
 {
 	struct m10bmc_sec *sec = fwl->dd_handle;
 	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
-	u32 blk_size, doorbell, extra_offset;
-	unsigned int stride, extra = 0;
+	struct intel_m10bmc *m10bmc = sec->m10bmc;
+	u32 blk_size, doorbell;
 	int ret;
 
-	stride = regmap_get_reg_stride(sec->m10bmc->regmap);
 	if (sec->cancel_request)
 		return rsu_cancel(sec);
 
-	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, &doorbell);
+	ret = m10bmc_sys_read(m10bmc, csr_map->doorbell, &doorbell);
 	if (ret) {
 		return FW_UPLOAD_ERR_RW_ERROR;
 	} else if (rsu_prog(doorbell) != RSU_PROG_READY) {
@@ -486,28 +517,12 @@ static enum fw_upload_err m10bmc_sec_write(struct fw_upload *fwl, const u8 *data
 		return FW_UPLOAD_ERR_HW_ERROR;
 	}
 
-	WARN_ON_ONCE(WRITE_BLOCK_SIZE % stride);
+	WARN_ON_ONCE(WRITE_BLOCK_SIZE % regmap_get_reg_stride(m10bmc->regmap));
 	blk_size = min_t(u32, WRITE_BLOCK_SIZE, size);
-	ret = regmap_bulk_write(sec->m10bmc->regmap,
-				M10BMC_STAGING_BASE + offset,
-				(void *)data + offset,
-				blk_size / stride);
+	ret = m10bmc_sec_write(sec, data, offset, blk_size);
 	if (ret)
 		return FW_UPLOAD_ERR_RW_ERROR;
 
-	/*
-	 * If blk_size is not aligned to stride, then handle the extra
-	 * bytes with regmap_write.
-	 */
-	if (blk_size % stride) {
-		extra_offset = offset + ALIGN_DOWN(blk_size, stride);
-		memcpy(&extra, (u8 *)(data + extra_offset), blk_size % stride);
-		ret = regmap_write(sec->m10bmc->regmap,
-				   M10BMC_STAGING_BASE + extra_offset, extra);
-		if (ret)
-			return FW_UPLOAD_ERR_RW_ERROR;
-	}
-
 	*written = blk_size;
 	return FW_UPLOAD_ERR_NONE;
 }
@@ -568,7 +583,7 @@ static void m10bmc_sec_cleanup(struct fw_upload *fwl)
 
 static const struct fw_upload_ops m10bmc_ops = {
 	.prepare = m10bmc_sec_prepare,
-	.write = m10bmc_sec_write,
+	.write = m10bmc_sec_fw_write,
 	.poll_complete = m10bmc_sec_poll_complete,
 	.cancel = m10bmc_sec_cancel,
 	.cleanup = m10bmc_sec_cleanup,
-- 
2.30.2

[PATCH v5 06/10] mfd: intel-m10-bmc: Downscope SPI defines & prefix with M10BMC_N3000

[Ilpo Järvinen]

Move SPI based board definitions to per interface file from the global
header. This makes it harder to use them accidently in the
generic/interface agnostic code. Prefix the defines with M10BMC_N3000
to make it more obvious these are related to some board type. All
current non-N3000 board types have the same layout so they'll be
reused.

Some bitfield defs are also moved to intel-m10-bmc-core which seems
more appropriate for them.

Reviewed-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/mfd/intel-m10-bmc-core.c  | 11 ++++
 drivers/mfd/intel-m10-bmc-spi.c   | 89 ++++++++++++++++++++++---------
 include/linux/mfd/intel-m10-bmc.h | 46 ----------------
 3 files changed, 74 insertions(+), 72 deletions(-)

diff --git a/drivers/mfd/intel-m10-bmc-core.c b/drivers/mfd/intel-m10-bmc-core.c
index 51b78b868235..50a4ec758bdb 100644
--- a/drivers/mfd/intel-m10-bmc-core.c
+++ b/drivers/mfd/intel-m10-bmc-core.c
@@ -12,6 +12,17 @@
 #include <linux/mfd/intel-m10-bmc.h>
 #include <linux/module.h>
 
+/* Register fields of system registers */
+#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
+#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
+#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
+#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
+#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
+#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
+#define M10BMC_MAC_COUNT		GENMASK(23, 16)
+#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
+#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
+
 static ssize_t bmc_version_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
index e8986154e965..04c83f9c6492 100644
--- a/drivers/mfd/intel-m10-bmc-spi.c
+++ b/drivers/mfd/intel-m10-bmc-spi.c
@@ -13,10 +13,47 @@
 #include <linux/regmap.h>
 #include <linux/spi/spi.h>
 
+#define M10BMC_N3000_LEGACY_BUILD_VER	0x300468
+#define M10BMC_N3000_SYS_BASE		0x300800
+#define M10BMC_N3000_SYS_END		0x300fff
+#define M10BMC_N3000_FLASH_BASE		0x10000000
+#define M10BMC_N3000_FLASH_END		0x1fffffff
+#define M10BMC_N3000_MEM_END		M10BMC_N3000_FLASH_END
+
+/* Register offset of system registers */
+#define NIOS2_FW_VERSION		0x0
+#define M10BMC_N3000_MAC_LOW		0x10
+#define M10BMC_N3000_MAC_HIGH		0x14
+#define M10BMC_N3000_TEST_REG		0x3c
+#define M10BMC_N3000_BUILD_VER		0x68
+#define M10BMC_N3000_VER_LEGACY_INVALID	0xffffffff
+
+/* Secure update doorbell register, in system register region */
+#define M10BMC_N3000_DOORBELL		0x400
+
+/* Authorization Result register, in system register region */
+#define M10BMC_N3000_AUTH_RESULT		0x404
+
+/* Addresses for security related data in FLASH */
+#define M10BMC_N3000_BMC_REH_ADDR	0x17ffc004
+#define M10BMC_N3000_BMC_PROG_ADDR	0x17ffc000
+#define M10BMC_N3000_BMC_PROG_MAGIC	0x5746
+
+#define M10BMC_N3000_SR_REH_ADDR	0x17ffd004
+#define M10BMC_N3000_SR_PROG_ADDR	0x17ffd000
+#define M10BMC_N3000_SR_PROG_MAGIC	0x5253
+
+#define M10BMC_N3000_PR_REH_ADDR	0x17ffe004
+#define M10BMC_N3000_PR_PROG_ADDR	0x17ffe000
+#define M10BMC_N3000_PR_PROG_MAGIC	0x5250
+
+/* Address of 4KB inverted bit vector containing staging area FLASH count */
+#define M10BMC_N3000_STAGING_FLASH_COUNT	0x17ffb000
+
 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),
+	regmap_reg_range(M10BMC_N3000_LEGACY_BUILD_VER, M10BMC_N3000_LEGACY_BUILD_VER),
+	regmap_reg_range(M10BMC_N3000_SYS_BASE, M10BMC_N3000_SYS_END),
+	regmap_reg_range(M10BMC_N3000_FLASH_BASE, M10BMC_N3000_FLASH_END),
 };
 
 static const struct regmap_access_table m10bmc_access_table = {
@@ -30,7 +67,7 @@ static struct regmap_config intel_m10bmc_regmap_config = {
 	.reg_stride = 4,
 	.wr_table = &m10bmc_access_table,
 	.rd_table = &m10bmc_access_table,
-	.max_register = M10BMC_MEM_END,
+	.max_register = M10BMC_N3000_MEM_END,
 };
 
 static int check_m10bmc_version(struct intel_m10bmc *ddata)
@@ -41,16 +78,16 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
 	/*
 	 * This check is to filter out the very old legacy BMC versions. In the
 	 * old BMC chips, the BMC version info is stored in the old version
-	 * register (M10BMC_LEGACY_BUILD_VER), so its read out value would have
-	 * not been M10BMC_VER_LEGACY_INVALID (0xffffffff). But in new BMC
+	 * register (M10BMC_N3000_LEGACY_BUILD_VER), so its read out value would have
+	 * not been M10BMC_N3000_VER_LEGACY_INVALID (0xffffffff). But in new BMC
 	 * chips that the driver supports, the value of this register should be
-	 * M10BMC_VER_LEGACY_INVALID.
+	 * M10BMC_N3000_VER_LEGACY_INVALID.
 	 */
-	ret = m10bmc_raw_read(ddata, M10BMC_LEGACY_BUILD_VER, &v);
+	ret = m10bmc_raw_read(ddata, M10BMC_N3000_LEGACY_BUILD_VER, &v);
 	if (ret)
 		return -ENODEV;
 
-	if (v != M10BMC_VER_LEGACY_INVALID) {
+	if (v != M10BMC_N3000_VER_LEGACY_INVALID) {
 		dev_err(ddata->dev, "bad version M10BMC detected\n");
 		return -ENODEV;
 	}
@@ -92,24 +129,24 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 }
 
 static const struct m10bmc_csr_map m10bmc_n3000_csr_map = {
-	.base = M10BMC_SYS_BASE,
-	.build_version = M10BMC_BUILD_VER,
+	.base = M10BMC_N3000_SYS_BASE,
+	.build_version = M10BMC_N3000_BUILD_VER,
 	.fw_version = NIOS2_FW_VERSION,
-	.mac_low = M10BMC_MAC_LOW,
-	.mac_high = M10BMC_MAC_HIGH,
-	.doorbell = M10BMC_DOORBELL,
-	.auth_result = M10BMC_AUTH_RESULT,
-	.rsu_status = M10BMC_DOORBELL,
-	.bmc_prog_addr = BMC_PROG_ADDR,
-	.bmc_reh_addr = BMC_REH_ADDR,
-	.bmc_magic = BMC_PROG_MAGIC,
-	.sr_prog_addr = SR_PROG_ADDR,
-	.sr_reh_addr = SR_REH_ADDR,
-	.sr_magic = SR_PROG_MAGIC,
-	.pr_prog_addr = PR_PROG_ADDR,
-	.pr_reh_addr = PR_REH_ADDR,
-	.pr_magic = PR_PROG_MAGIC,
-	.rsu_update_counter = STAGING_FLASH_COUNT,
+	.mac_low = M10BMC_N3000_MAC_LOW,
+	.mac_high = M10BMC_N3000_MAC_HIGH,
+	.doorbell = M10BMC_N3000_DOORBELL,
+	.auth_result = M10BMC_N3000_AUTH_RESULT,
+	.rsu_status = M10BMC_N3000_DOORBELL,
+	.bmc_prog_addr = M10BMC_N3000_BMC_PROG_ADDR,
+	.bmc_reh_addr = M10BMC_N3000_BMC_REH_ADDR,
+	.bmc_magic = M10BMC_N3000_BMC_PROG_MAGIC,
+	.sr_prog_addr = M10BMC_N3000_SR_PROG_ADDR,
+	.sr_reh_addr = M10BMC_N3000_SR_REH_ADDR,
+	.sr_magic = M10BMC_N3000_SR_PROG_MAGIC,
+	.pr_prog_addr = M10BMC_N3000_PR_PROG_ADDR,
+	.pr_reh_addr = M10BMC_N3000_PR_REH_ADDR,
+	.pr_magic = M10BMC_N3000_PR_PROG_MAGIC,
+	.rsu_update_counter = M10BMC_N3000_STAGING_FLASH_COUNT,
 };
 
 static struct mfd_cell m10bmc_d5005_subdevs[] = {
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index a009dd82f698..42e2ce7fe439 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -9,39 +9,9 @@
 
 #include <linux/regmap.h>
 
-#define M10BMC_LEGACY_BUILD_VER		0x300468
-#define M10BMC_SYS_BASE			0x300800
-#define M10BMC_SYS_END			0x300fff
-#define M10BMC_FLASH_BASE		0x10000000
-#define M10BMC_FLASH_END		0x1fffffff
-#define M10BMC_MEM_END			M10BMC_FLASH_END
-
 #define M10BMC_STAGING_BASE		0x18000000
 #define M10BMC_STAGING_SIZE		0x3800000
 
-/* Register offset of system registers */
-#define NIOS2_FW_VERSION		0x0
-#define M10BMC_MAC_LOW			0x10
-#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
-#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
-#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
-#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
-#define M10BMC_MAC_HIGH			0x14
-#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
-#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
-#define M10BMC_MAC_COUNT		GENMASK(23, 16)
-#define M10BMC_TEST_REG			0x3c
-#define M10BMC_BUILD_VER		0x68
-#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
-#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
-#define M10BMC_VER_LEGACY_INVALID	0xffffffff
-
-/* Secure update doorbell register, in system register region */
-#define M10BMC_DOORBELL			0x400
-
-/* Authorization Result register, in system register region */
-#define M10BMC_AUTH_RESULT		0x404
-
 /* Doorbell register fields */
 #define DRBL_RSU_REQUEST		BIT(0)
 #define DRBL_RSU_PROGRESS		GENMASK(7, 4)
@@ -102,22 +72,6 @@
 #define RSU_COMPLETE_INTERVAL_MS	1000
 #define RSU_COMPLETE_TIMEOUT_MS		(40 * 60 * 1000)
 
-/* Addresses for security related data in FLASH */
-#define BMC_REH_ADDR	0x17ffc004
-#define BMC_PROG_ADDR	0x17ffc000
-#define BMC_PROG_MAGIC	0x5746
-
-#define SR_REH_ADDR	0x17ffd004
-#define SR_PROG_ADDR	0x17ffd000
-#define SR_PROG_MAGIC	0x5253
-
-#define PR_REH_ADDR	0x17ffe004
-#define PR_PROG_ADDR	0x17ffe000
-#define PR_PROG_MAGIC	0x5250
-
-/* Address of 4KB inverted bit vector containing staging area FLASH count */
-#define STAGING_FLASH_COUNT	0x17ffb000
-
 /**
  * struct m10bmc_csr_map - Intel MAX 10 BMC CSR register map
  */
-- 
2.30.2

[PATCH v5 07/10] fpga: m10bmc-sec: Create helpers for rsu status/progress checks

[Ilpo Järvinen]

RSU_STAT_* and RSU_PROG_* checks are done in more than one place in the sec
update code. Move the checks into new helper functions.

No function changes intended.

Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
Co-developed-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/fpga/intel-m10-bmc-sec-update.c | 59 +++++++++++++------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
index 9922027856a4..6e58a463619c 100644
--- a/drivers/fpga/intel-m10-bmc-sec-update.c
+++ b/drivers/fpga/intel-m10-bmc-sec-update.c
@@ -257,6 +257,28 @@ static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
 		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
 }
 
+static bool rsu_status_ok(u32 status)
+{
+	return (status == RSU_STAT_NORMAL ||
+		status == RSU_STAT_NIOS_OK ||
+		status == RSU_STAT_USER_OK ||
+		status == RSU_STAT_FACTORY_OK);
+}
+
+static bool rsu_progress_done(u32 progress)
+{
+	return (progress == RSU_PROG_IDLE ||
+		progress == RSU_PROG_RSU_DONE);
+}
+
+static bool rsu_progress_busy(u32 progress)
+{
+	return (progress == RSU_PROG_AUTHENTICATING ||
+		progress == RSU_PROG_COPYING ||
+		progress == RSU_PROG_UPDATE_CANCEL ||
+		progress == RSU_PROG_PROGRAM_KEY_HASH);
+}
+
 static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
 {
 	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
@@ -267,8 +289,7 @@ static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
 	if (ret)
 		return FW_UPLOAD_ERR_RW_ERROR;
 
-	if (rsu_prog(doorbell) != RSU_PROG_IDLE &&
-	    rsu_prog(doorbell) != RSU_PROG_RSU_DONE) {
+	if (!rsu_progress_done(rsu_prog(doorbell))) {
 		log_error_regs(sec, doorbell);
 		return FW_UPLOAD_ERR_BUSY;
 	}
@@ -288,7 +309,7 @@ static inline bool rsu_start_done(u32 doorbell)
 		return true;
 
 	progress = rsu_prog(doorbell);
-	if (progress != RSU_PROG_IDLE && progress != RSU_PROG_RSU_DONE)
+	if (!rsu_progress_done(progress))
 		return true;
 
 	return false;
@@ -397,13 +418,7 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
 		return FW_UPLOAD_ERR_RW_ERROR;
 	}
 
-	switch (rsu_stat(doorbell)) {
-	case RSU_STAT_NORMAL:
-	case RSU_STAT_NIOS_OK:
-	case RSU_STAT_USER_OK:
-	case RSU_STAT_FACTORY_OK:
-		break;
-	default:
+	if (!rsu_status_ok(rsu_stat(doorbell))) {
 		log_error_regs(sec, doorbell);
 		return FW_UPLOAD_ERR_HW_ERROR;
 	}
@@ -418,28 +433,16 @@ static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell)
 	if (m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell))
 		return -EIO;
 
-	switch (rsu_stat(*doorbell)) {
-	case RSU_STAT_NORMAL:
-	case RSU_STAT_NIOS_OK:
-	case RSU_STAT_USER_OK:
-	case RSU_STAT_FACTORY_OK:
-		break;
-	default:
+	if (!rsu_status_ok(rsu_stat(*doorbell)))
 		return -EINVAL;
-	}
 
-	switch (rsu_prog(*doorbell)) {
-	case RSU_PROG_IDLE:
-	case RSU_PROG_RSU_DONE:
+	if (rsu_progress_done(rsu_prog(*doorbell)))
 		return 0;
-	case RSU_PROG_AUTHENTICATING:
-	case RSU_PROG_COPYING:
-	case RSU_PROG_UPDATE_CANCEL:
-	case RSU_PROG_PROGRAM_KEY_HASH:
+
+	if (rsu_progress_busy(rsu_prog(*doorbell)))
 		return -EAGAIN;
-	default:
-		return -EINVAL;
-	}
+
+	return -EINVAL;
 }
 
 static enum fw_upload_err rsu_cancel(struct m10bmc_sec *sec)
-- 
2.30.2

[PATCH v5 08/10] fpga: m10bmc-sec: Differentiate rsu status from doorbell in csr map

[Ilpo Järvinen]

The rsu_status field moves from the doorbell register to the auth
result register in the PMCI implementation of the MAX10 BMC. Refactor
the sec update driver code to handle two distinct registers (rsu_status
field was added into csr map already when it was introduced but it was
unused until now).

Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
Co-developed-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/fpga/intel-m10-bmc-sec-update.c | 68 ++++++++++++++++---------
 include/linux/mfd/intel-m10-bmc.h       |  2 +-
 2 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
index 6e58a463619c..1fe8b7ff594c 100644
--- a/drivers/fpga/intel-m10-bmc-sec-update.c
+++ b/drivers/fpga/intel-m10-bmc-sec-update.c
@@ -251,7 +251,7 @@ static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
 	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
 	u32 auth_result;
 
-	dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell);
+	dev_err(sec->dev, "Doorbell: 0x%08x\n", doorbell);
 
 	if (!m10bmc_sys_read(sec->m10bmc, csr_map->auth_result, &auth_result))
 		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
@@ -279,6 +279,30 @@ static bool rsu_progress_busy(u32 progress)
 		progress == RSU_PROG_PROGRAM_KEY_HASH);
 }
 
+static int m10bmc_sec_progress_status(struct m10bmc_sec *sec, u32 *doorbell,
+				      u32 *progress, u32 *status)
+{
+	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
+	u32 status_reg;
+	int ret;
+
+	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell);
+	if (ret)
+		return ret;
+
+	if (csr_map->doorbell != csr_map->rsu_status) {
+		ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status_reg);
+		if (ret)
+			return ret;
+		*status = rsu_stat(status_reg);
+	} else {
+		*status = rsu_stat(*doorbell);
+	}
+	*progress = rsu_prog(*doorbell);
+
+	return 0;
+}
+
 static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
 {
 	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
@@ -297,18 +321,14 @@ static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
 	return FW_UPLOAD_ERR_NONE;
 }
 
-static inline bool rsu_start_done(u32 doorbell)
+static inline bool rsu_start_done(u32 doorbell, u32 progress, u32 status)
 {
-	u32 status, progress;
-
 	if (doorbell & DRBL_RSU_REQUEST)
 		return false;
 
-	status = rsu_stat(doorbell);
 	if (status == RSU_STAT_ERASE_FAIL || status == RSU_STAT_WEAROUT)
 		return true;
 
-	progress = rsu_prog(doorbell);
 	if (!rsu_progress_done(progress))
 		return true;
 
@@ -318,8 +338,8 @@ static inline bool rsu_start_done(u32 doorbell)
 static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
 {
 	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
-	u32 doorbell, status;
-	int ret;
+	u32 doorbell, progress, status;
+	int ret, err;
 
 	ret = regmap_update_bits(sec->m10bmc->regmap,
 				 csr_map->base + csr_map->doorbell,
@@ -330,21 +350,20 @@ static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
 	if (ret)
 		return FW_UPLOAD_ERR_RW_ERROR;
 
-	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
-				       csr_map->base + csr_map->doorbell,
-				       doorbell,
-				       rsu_start_done(doorbell),
-				       NIOS_HANDSHAKE_INTERVAL_US,
-				       NIOS_HANDSHAKE_TIMEOUT_US);
+	ret = read_poll_timeout(m10bmc_sec_progress_status, err,
+				err < 0 || rsu_start_done(doorbell, progress, status),
+				NIOS_HANDSHAKE_INTERVAL_US,
+				NIOS_HANDSHAKE_TIMEOUT_US,
+				false,
+				sec, &doorbell, &progress, &status);
 
 	if (ret == -ETIMEDOUT) {
 		log_error_regs(sec, doorbell);
 		return FW_UPLOAD_ERR_TIMEOUT;
-	} else if (ret) {
+	} else if (err) {
 		return FW_UPLOAD_ERR_RW_ERROR;
 	}
 
-	status = rsu_stat(doorbell);
 	if (status == RSU_STAT_WEAROUT) {
 		dev_warn(sec->dev, "Excessive flash update count detected\n");
 		return FW_UPLOAD_ERR_WEAROUT;
@@ -393,7 +412,7 @@ static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec)
 static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
 {
 	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
-	u32 doorbell;
+	u32 doorbell, status;
 	int ret;
 
 	ret = regmap_update_bits(sec->m10bmc->regmap,
@@ -418,7 +437,10 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
 		return FW_UPLOAD_ERR_RW_ERROR;
 	}
 
-	if (!rsu_status_ok(rsu_stat(doorbell))) {
+	ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status);
+	if (ret)
+		return ret;
+	if (!rsu_status_ok(rsu_stat(status))) {
 		log_error_regs(sec, doorbell);
 		return FW_UPLOAD_ERR_HW_ERROR;
 	}
@@ -428,18 +450,18 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
 
 static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell)
 {
-	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
+	u32 progress, status;
 
-	if (m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell))
+	if (m10bmc_sec_progress_status(sec, doorbell, &progress, &status))
 		return -EIO;
 
-	if (!rsu_status_ok(rsu_stat(*doorbell)))
+	if (!rsu_status_ok(status))
 		return -EINVAL;
 
-	if (rsu_progress_done(rsu_prog(*doorbell)))
+	if (rsu_progress_done(progress))
 		return 0;
 
-	if (rsu_progress_busy(rsu_prog(*doorbell)))
+	if (rsu_progress_busy(progress))
 		return -EAGAIN;
 
 	return -EINVAL;
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index 42e2ce7fe439..cc2d9eb597b0 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -58,7 +58,7 @@
 #define HOST_STATUS_ABORT_RSU		0x2
 
 #define rsu_prog(doorbell)	FIELD_GET(DRBL_RSU_PROGRESS, doorbell)
-#define rsu_stat(doorbell)	FIELD_GET(DRBL_RSU_STATUS, doorbell)
+#define rsu_stat(status_reg)	FIELD_GET(DRBL_RSU_STATUS, status_reg)
 
 /* interval 100ms and timeout 5s */
 #define NIOS_HANDSHAKE_INTERVAL_US	(100 * 1000)
-- 
2.30.2

[PATCH v5 09/10] mfd: intel-m10-bmc: Add PMCI driver

[Ilpo Järvinen]

Add the mfd driver for the Platform Management Component Interface
(PMCI) based interface of Intel MAX10 BMC controller.

PMCI is a software-visible interface, connected to card BMC which
provided the basic functionality of read/write BMC register. The access
to the register is done indirectly via a hardware controller/bridge
that handles read/write/clear commands and acknowledgments for the
commands.

Previously, intel-m10-bmc provided sysfs under
/sys/bus/spi/devices/... which is generalized in this change because
not all MAX10 BMC appear under SPI anymore.

Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
Co-developed-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Co-developed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reviewed-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
 drivers/mfd/Kconfig                           |  12 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/intel-m10-bmc-pmci.c              | 253 ++++++++++++++++++
 4 files changed, 270 insertions(+), 4 deletions(-)
 create mode 100644 drivers/mfd/intel-m10-bmc-pmci.c

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
index 9773925138af..a8ab58035c95 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
@@ -1,4 +1,4 @@
-What:		/sys/bus/spi/devices/.../bmc_version
+What:		/sys/bus/.../drivers/intel-m10-bmc/.../bmc_version
 Date:		June 2020
 KernelVersion:	5.10
 Contact:	Xu Yilun <yilun.xu@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 a09d4ac60dc7..82f13614d98a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2238,6 +2238,18 @@ config MFD_INTEL_M10_BMC_SPI
           additional drivers must be enabled in order to use the functionality
           of the device.
 
+config MFD_INTEL_M10_BMC_PMCI
+	tristate "Intel MAX 10 Board Management Controller with PMCI"
+	depends on FPGA_DFL
+	select MFD_INTEL_M10_BMC_CORE
+	select REGMAP
+	help
+	  Support for the Intel MAX 10 board management controller via PMCI.
+
+	  This driver provides common support for accessing the device,
+	  additional drivers must be enabled in order to use the functionality
+	  of the device.
+
 config MFD_RSMU_I2C
 	tristate "Renesas Synchronization Management Unit with I2C"
 	depends on I2C && OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 5d1f308ee2a7..c90fb96cad2a 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -274,6 +274,7 @@ obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 
 obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)	+= intel-m10-bmc-core.o
 obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)	+= intel-m10-bmc-spi.o
+obj-$(CONFIG_MFD_INTEL_M10_BMC_PMCI)	+= intel-m10-bmc-pmci.o
 
 obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
 obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
new file mode 100644
index 000000000000..63ec0f6aba8b
--- /dev/null
+++ b/drivers/mfd/intel-m10-bmc-pmci.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MFD driver for Platform Management Component Interface (PMCI) based
+ * interface to MAX10 BMC.
+ *
+ * Copyright (C) 2020-2022 Intel Corporation.
+ */
+
+#include <linux/device.h>
+#include <linux/dfl.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define M10BMC_PMCI_INDIRECT_BASE	0x400
+
+#define M10BMC_N6000_SYS_BASE		0x0
+#define M10BMC_N6000_SYS_END		0xfff
+
+#define M10BMC_N6000_DOORBELL		0x1c0
+#define M10BMC_N6000_AUTH_RESULT	0x1c4
+
+/* Telemetry registers */
+#define M10BMC_N6000_TELEM_START	0x400
+#define M10BMC_N6000_TELEM_END		0x78c
+
+#define M10BMC_N6000_BUILD_VER		0x0
+#define NIOS2_N6000_FW_VERSION		0x4
+#define M10BMC_N6000_MAC_LOW		0x20
+#define M10BMC_N6000_MAC_HIGH		(M10BMC_N6000_MAC_LOW + 4)
+
+/* Addresses for security related data in FLASH */
+#define M10BMC_N6000_BMC_REH_ADDR	0x7ffc004
+#define M10BMC_N6000_BMC_PROG_ADDR	0x7ffc000
+#define M10BMC_N6000_BMC_PROG_MAGIC	0x5746
+
+#define M10BMC_N6000_SR_REH_ADDR	0x7ffd004
+#define M10BMC_N6000_SR_PROG_ADDR	0x7ffd000
+#define M10BMC_N6000_SR_PROG_MAGIC	0x5253
+
+#define M10BMC_N6000_PR_REH_ADDR	0x7ffe004
+#define M10BMC_N6000_PR_PROG_ADDR	0x7ffe000
+#define M10BMC_N6000_PR_PROG_MAGIC	0x5250
+
+#define M10BMC_N6000_STAGING_FLASH_COUNT	0x7ff5000
+
+struct m10bmc_pmci_device {
+	void __iomem *base;
+	struct intel_m10bmc m10bmc;
+};
+
+/*
+ * Intel FGPA indirect register access via hardware controller/bridge.
+ */
+#define INDIRECT_CMD_OFF	0
+#define INDIRECT_CMD_CLR	0
+#define INDIRECT_CMD_RD		BIT(0)
+#define INDIRECT_CMD_WR		BIT(1)
+#define INDIRECT_CMD_ACK	BIT(2)
+
+#define INDIRECT_ADDR_OFF	0x4
+#define INDIRECT_RD_OFF		0x8
+#define INDIRECT_WR_OFF		0xc
+
+#define INDIRECT_INT_US		1
+#define INDIRECT_TIMEOUT_US	10000
+
+struct indirect_ctx {
+        void __iomem *base;
+        struct device *dev;
+};
+
+static int indirect_clear_cmd(struct indirect_ctx *ctx)
+{
+	unsigned int cmd;
+	int ret;
+
+	writel(INDIRECT_CMD_CLR, ctx->base + INDIRECT_CMD_OFF);
+
+	ret = readl_poll_timeout(ctx->base + INDIRECT_CMD_OFF, cmd,
+				 cmd == INDIRECT_CMD_CLR,
+				 INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
+	if (ret)
+		dev_err(ctx->dev, "timed out waiting clear cmd (residual cmd=0x%x)\n", cmd);
+
+	return ret;
+}
+
+static int indirect_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct indirect_ctx *ctx = context;
+	unsigned int cmd, ack, tmpval;
+	int ret;
+
+	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
+	if (cmd != INDIRECT_CMD_CLR)
+		dev_warn(ctx->dev, "residual cmd 0x%x on read entry\n", cmd);
+
+	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
+	writel(INDIRECT_CMD_RD, ctx->base + INDIRECT_CMD_OFF);
+
+	ret = readl_poll_timeout(ctx->base + INDIRECT_CMD_OFF, ack,
+				 (ack & INDIRECT_CMD_ACK) == INDIRECT_CMD_ACK,
+				 INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
+	if (ret)
+		dev_err(ctx->dev, "read timed out on reg 0x%x ack 0x%x\n", reg, ack);
+	else
+		tmpval = readl(ctx->base + INDIRECT_RD_OFF);
+
+	if (indirect_clear_cmd(ctx)) {
+		if (!ret)
+			ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	*val = tmpval;
+out:
+	return ret;
+}
+
+static int indirect_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct indirect_ctx *ctx = context;
+	unsigned int cmd, ack;
+	int ret;
+
+	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
+	if (cmd != INDIRECT_CMD_CLR)
+		dev_warn(ctx->dev, "residual cmd 0x%x on write entry\n", cmd);
+
+	writel(val, ctx->base + INDIRECT_WR_OFF);
+	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
+	writel(INDIRECT_CMD_WR, ctx->base + INDIRECT_CMD_OFF);
+
+	ret = readl_poll_timeout(ctx->base + INDIRECT_CMD_OFF, ack,
+				 (ack & INDIRECT_CMD_ACK) == INDIRECT_CMD_ACK,
+				 INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
+	if (ret)
+		dev_err(ctx->dev, "write timed out on reg 0x%x ack 0x%x\n", reg, ack);
+
+	if (indirect_clear_cmd(ctx)) {
+		if (!ret)
+			ret = -ETIMEDOUT;
+	}
+
+	return ret;
+}
+
+static const struct regmap_range m10bmc_pmci_regmap_range[] = {
+	regmap_reg_range(M10BMC_N6000_SYS_BASE, M10BMC_N6000_SYS_END),
+};
+
+static const struct regmap_access_table m10bmc_pmci_access_table = {
+	.yes_ranges	= m10bmc_pmci_regmap_range,
+	.n_yes_ranges	= ARRAY_SIZE(m10bmc_pmci_regmap_range),
+};
+
+static struct regmap_config m10bmc_pmci_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.wr_table = &m10bmc_pmci_access_table,
+	.rd_table = &m10bmc_pmci_access_table,
+	.reg_read = &indirect_reg_read,
+	.reg_write = &indirect_reg_write,
+	.max_register = M10BMC_N6000_SYS_END,
+};
+
+static struct mfd_cell m10bmc_pmci_n6000_bmc_subdevs[] = {
+	{ .name = "n6000bmc-hwmon" },
+};
+
+static const struct m10bmc_csr_map m10bmc_n6000_csr_map = {
+	.base = M10BMC_N6000_SYS_BASE,
+	.build_version = M10BMC_N6000_BUILD_VER,
+	.fw_version = NIOS2_N6000_FW_VERSION,
+	.mac_low = M10BMC_N6000_MAC_LOW,
+	.mac_high = M10BMC_N6000_MAC_HIGH,
+	.doorbell = M10BMC_N6000_DOORBELL,
+	.auth_result = M10BMC_N6000_AUTH_RESULT,
+	.rsu_status = M10BMC_N6000_AUTH_RESULT,
+	.bmc_prog_addr = M10BMC_N6000_BMC_PROG_ADDR,
+	.bmc_reh_addr = M10BMC_N6000_BMC_REH_ADDR,
+	.bmc_magic = M10BMC_N6000_BMC_PROG_MAGIC,
+	.sr_prog_addr = M10BMC_N6000_SR_PROG_ADDR,
+	.sr_reh_addr = M10BMC_N6000_SR_REH_ADDR,
+	.sr_magic = M10BMC_N6000_SR_PROG_MAGIC,
+	.pr_prog_addr = M10BMC_N6000_PR_PROG_ADDR,
+	.pr_reh_addr = M10BMC_N6000_PR_REH_ADDR,
+	.pr_magic = M10BMC_N6000_PR_PROG_MAGIC,
+	.rsu_update_counter = M10BMC_N6000_STAGING_FLASH_COUNT,
+};
+
+static const struct intel_m10bmc_platform_info m10bmc_pmci_n6000 = {
+	.cells = m10bmc_pmci_n6000_bmc_subdevs,
+	.n_cells = ARRAY_SIZE(m10bmc_pmci_n6000_bmc_subdevs),
+	.csr_map = &m10bmc_n6000_csr_map,
+};
+
+static int m10bmc_pmci_probe(struct dfl_device *ddev)
+{
+	struct device *dev = &ddev->dev;
+	struct m10bmc_pmci_device *pmci;
+	struct indirect_ctx *ctx;
+
+	pmci = devm_kzalloc(dev, sizeof(*pmci), GFP_KERNEL);
+	if (!pmci)
+		return -ENOMEM;
+
+	pmci->m10bmc.dev = dev;
+
+	pmci->base = devm_ioremap_resource(dev, &ddev->mmio_res);
+	if (IS_ERR(pmci->base))
+		return PTR_ERR(pmci->base);
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->base = pmci->base + M10BMC_PMCI_INDIRECT_BASE;
+	ctx->dev = dev;
+	indirect_clear_cmd(ctx);
+	pmci->m10bmc.regmap = devm_regmap_init(dev, NULL, ctx, &m10bmc_pmci_regmap_config);
+
+	if (IS_ERR(pmci->m10bmc.regmap))
+		return PTR_ERR(pmci->m10bmc.regmap);
+
+	return m10bmc_dev_init(&pmci->m10bmc, &m10bmc_pmci_n6000);
+}
+
+#define FME_FEATURE_ID_M10BMC_PMCI	0x12
+
+static const struct dfl_device_id m10bmc_pmci_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_M10BMC_PMCI },
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, m10bmc_pmci_ids);
+
+static struct dfl_driver m10bmc_pmci_driver = {
+	.drv	= {
+		.name       = "intel-m10-bmc",
+		.dev_groups = m10bmc_dev_groups,
+	},
+	.id_table = m10bmc_pmci_ids,
+	.probe    = m10bmc_pmci_probe,
+};
+
+module_dfl_driver(m10bmc_pmci_driver);
+
+MODULE_DESCRIPTION("MAX10 BMC PMCI-based interface");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
-- 
2.30.2

[PATCH v5 10/10] fpga: m10bmc-sec: Add support for N6000

[Ilpo Järvinen]

Add support for PMCI-based flash access path and N6000 sec update
support. Access to flash staging area is different for N6000 from that
of the SPI interfaced counterparts.

Introduce intel_m10bmc_flash_bulk_ops to allow interface specific
differentiations for the flash access path for sec update and make
m10bmc_sec_read/write() in sec update driver to use the new operations.

If a failure is detected while altering the flash MUX, it seems safer
to try to set it back and doesn't seem harmful. Likely there are enough
troubles in that case anyway so setting it back fails too (which is
harmless sans the small extra delay) or just confirms that the value
wasn't changed.

Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
Co-developed-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/fpga/intel-m10-bmc-sec-update.c |  65 ++++++++++-
 drivers/mfd/intel-m10-bmc-pmci.c        | 145 ++++++++++++++++++++++++
 include/linux/mfd/intel-m10-bmc.h       |  14 +++
 3 files changed, 223 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
index 1fe8b7ff594c..13f429860b03 100644
--- a/drivers/fpga/intel-m10-bmc-sec-update.c
+++ b/drivers/fpga/intel-m10-bmc-sec-update.c
@@ -14,6 +14,20 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+#define M10BMC_PMCI_FLASH_MUX_CTRL	0x1d0
+#define FLASH_MUX_SELECTION		GENMASK(2, 0)
+#define FLASH_MUX_IDLE			0
+#define FLASH_MUX_NIOS			1
+#define FLASH_MUX_HOST			2
+#define FLASH_MUX_PFL			4
+#define get_flash_mux(mux)		FIELD_GET(FLASH_MUX_SELECTION, mux)
+
+#define FLASH_NIOS_REQUEST		BIT(4)
+#define FLASH_HOST_REQUEST		BIT(5)
+
+#define M10_FLASH_INT_US		1
+#define M10_FLASH_TIMEOUT_US		10000
+
 struct m10bmc_sec {
 	struct device *dev;
 	struct intel_m10bmc *m10bmc;
@@ -21,6 +35,7 @@ struct m10bmc_sec {
 	char *fw_name;
 	u32 fw_name_id;
 	bool cancel_request;
+	struct mutex flash_mutex;
 };
 
 static DEFINE_XARRAY_ALLOC(fw_upload_xa);
@@ -31,6 +46,24 @@ static DEFINE_XARRAY_ALLOC(fw_upload_xa);
 #define REH_MAGIC		GENMASK(15, 0)
 #define REH_SHA_NUM_BYTES	GENMASK(31, 16)
 
+static int m10bmc_sec_set_flash_host_mux(struct intel_m10bmc *m10bmc, bool request)
+{
+	u32 ctrl;
+	int ret;
+
+	ret = regmap_update_bits(m10bmc->regmap, M10BMC_PMCI_FLASH_MUX_CTRL,
+				 FLASH_HOST_REQUEST,
+				 FIELD_PREP(FLASH_HOST_REQUEST, request));
+	if (ret)
+		return ret;
+
+	return regmap_read_poll_timeout(m10bmc->regmap,
+					M10BMC_PMCI_FLASH_MUX_CTRL, ctrl,
+					request ? (get_flash_mux(ctrl) == FLASH_MUX_HOST) :
+						  (get_flash_mux(ctrl) != FLASH_MUX_HOST),
+					M10_FLASH_INT_US, M10_FLASH_TIMEOUT_US);
+}
+
 static int m10bmc_sec_write(struct m10bmc_sec *sec, const u8 *buf, u32 offset, u32 size)
 {
 	struct intel_m10bmc *m10bmc = sec->m10bmc;
@@ -41,6 +74,15 @@ static int m10bmc_sec_write(struct m10bmc_sec *sec, const u8 *buf, u32 offset, u
 	u32 leftover_tmp = 0;
 	int ret;
 
+	if (sec->m10bmc->flash_bulk_ops) {
+		mutex_lock(&sec->flash_mutex);
+		/* On write, firmware manages flash MUX */
+		ret = sec->m10bmc->flash_bulk_ops->write(m10bmc, buf, offset, size);
+		mutex_unlock(&sec->flash_mutex);
+
+		return ret;
+	}
+
 	if (WARN_ON_ONCE(stride > sizeof(leftover_tmp)))
 		return -EINVAL;
 
@@ -69,7 +111,21 @@ static int m10bmc_sec_read(struct m10bmc_sec *sec, u8 *buf, u32 addr, u32 size)
 	u32 leftover_offset = read_count * stride;
 	u32 leftover_size = size - leftover_offset;
 	u32 leftover_tmp;
-	int ret;
+	int ret, ret2;
+
+	if (sec->m10bmc->flash_bulk_ops) {
+		mutex_lock(&sec->flash_mutex);
+		ret = m10bmc_sec_set_flash_host_mux(m10bmc, true);
+		if (ret)
+			goto mux_fail;
+		ret = sec->m10bmc->flash_bulk_ops->read(m10bmc, buf, addr, size);
+mux_fail:
+		ret2 = m10bmc_sec_set_flash_host_mux(m10bmc, false);
+		mutex_unlock(&sec->flash_mutex);
+		if (ret)
+			return ret;
+		return ret2;
+	}
 
 	if (WARN_ON_ONCE(stride > sizeof(leftover_tmp)))
 		return -EINVAL;
@@ -636,6 +692,8 @@ static int m10bmc_sec_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	mutex_init(&sec->flash_mutex);
+
 	len = scnprintf(buf, SEC_UPDATE_LEN_MAX, "secure-update%d",
 			sec->fw_name_id);
 	sec->fw_name = kmemdup_nul(buf, len, GFP_KERNEL);
@@ -658,6 +716,7 @@ static int m10bmc_sec_probe(struct platform_device *pdev)
 fw_uploader_fail:
 	kfree(sec->fw_name);
 fw_name_fail:
+	mutex_destroy(&sec->flash_mutex);
 	xa_erase(&fw_upload_xa, sec->fw_name_id);
 	return ret;
 }
@@ -668,6 +727,7 @@ static int m10bmc_sec_remove(struct platform_device *pdev)
 
 	firmware_upload_unregister(sec->fwl);
 	kfree(sec->fw_name);
+	mutex_destroy(&sec->flash_mutex);
 	xa_erase(&fw_upload_xa, sec->fw_name_id);
 
 	return 0;
@@ -680,6 +740,9 @@ static const struct platform_device_id intel_m10bmc_sec_ids[] = {
 	{
 		.name = "d5005bmc-sec-update",
 	},
+	{
+		.name = "n6000bmc-sec-update",
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(platform, intel_m10bmc_sec_ids);
diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
index 63ec0f6aba8b..1d8819ace535 100644
--- a/drivers/mfd/intel-m10-bmc-pmci.c
+++ b/drivers/mfd/intel-m10-bmc-pmci.c
@@ -6,10 +6,12 @@
  * Copyright (C) 2020-2022 Intel Corporation.
  */
 
+#include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/dfl.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/intel-m10-bmc.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 
@@ -45,6 +47,24 @@
 
 #define M10BMC_N6000_STAGING_FLASH_COUNT	0x7ff5000
 
+#define M10BMC_PMCI_FLASH_CTRL			0x40
+#define M10BMC_PMCI_FLASH_WR_MODE		BIT(0)
+#define M10BMC_PMCI_FLASH_RD_MODE		BIT(1)
+#define M10BMC_PMCI_FLASH_BUSY			BIT(2)
+#define M10BMC_PMCI_FLASH_FIFO_SPACE		GENMASK(13, 4)
+#define M10BMC_PMCI_FLASH_READ_COUNT		GENMASK(25, 16)
+
+#define M10BMC_PMCI_FLASH_ADDR			0x44
+#define M10BMC_PMCI_FLASH_FIFO			0x800
+#define M10BMC_PMCI_READ_BLOCK_SIZE		0x800
+#define M10BMC_PMCI_FIFO_MAX_BYTES		0x800
+#define M10BMC_PMCI_FIFO_WORD_SIZE		4
+#define M10BMC_PMCI_FIFO_MAX_WORDS		(M10BMC_PMCI_FIFO_MAX_BYTES / \
+						 M10BMC_PMCI_FIFO_WORD_SIZE)
+
+#define M10BMC_PMCI_FLASH_INT_US		1
+#define M10BMC_PMCI_FLASH_TIMEOUT_US		10000
+
 struct m10bmc_pmci_device {
 	void __iomem *base;
 	struct intel_m10bmc m10bmc;
@@ -147,6 +167,128 @@ static int indirect_reg_write(void *context, unsigned int reg, unsigned int val)
 	return ret;
 }
 
+static void pmci_write_fifo(void __iomem *base, const u32 *buf, size_t count)
+{
+	while (count--)
+		writel(*buf++, base);
+}
+
+static void pmci_read_fifo(void __iomem *base, u32 *buf, size_t count)
+{
+	while (count--)
+		*buf++ = readl(base);
+}
+
+static u32 pmci_get_write_space(struct m10bmc_pmci_device *pmci)
+{
+	u32 val;
+	int ret;
+
+	ret = read_poll_timeout(readl, val,
+				FIELD_GET(M10BMC_PMCI_FLASH_FIFO_SPACE, val) ==
+				M10BMC_PMCI_FIFO_MAX_WORDS,
+				M10BMC_PMCI_FLASH_INT_US, M10BMC_PMCI_FLASH_TIMEOUT_US,
+				false, pmci->base + M10BMC_PMCI_FLASH_CTRL);
+	if (ret == -ETIMEDOUT)
+		return 0;
+
+	return FIELD_GET(M10BMC_PMCI_FLASH_FIFO_SPACE, val) * M10BMC_PMCI_FIFO_WORD_SIZE;
+}
+
+static int pmci_flash_bulk_write(struct intel_m10bmc *m10bmc, const u8 *buf, u32 size)
+{
+	struct m10bmc_pmci_device *pmci = container_of(m10bmc, struct m10bmc_pmci_device, m10bmc);
+	u32 blk_size, offset = 0, write_count;
+
+	while (size) {
+		blk_size = min(pmci_get_write_space(pmci), size);
+		if (blk_size == 0) {
+			dev_err(m10bmc->dev, "get FIFO available size fail\n");
+			return -EIO;
+		}
+
+		if (size < M10BMC_PMCI_FIFO_WORD_SIZE)
+			break;
+
+		write_count = blk_size / M10BMC_PMCI_FIFO_WORD_SIZE;
+		pmci_write_fifo(pmci->base + M10BMC_PMCI_FLASH_FIFO,
+				(u32 *)(buf + offset), write_count);
+
+		size -= blk_size;
+		offset += blk_size;
+	}
+
+	/* Handle remainder (less than M10BMC_PMCI_FIFO_WORD_SIZE bytes) */
+	if (size) {
+		u32 tmp = 0;
+
+		memcpy(&tmp, buf + offset, size);
+		pmci_write_fifo(pmci->base + M10BMC_PMCI_FLASH_FIFO, &tmp, 1);
+	}
+
+	return 0;
+}
+
+static int pmci_flash_bulk_read(struct intel_m10bmc *m10bmc, u8 *buf, u32 addr, u32 size)
+{
+	struct m10bmc_pmci_device *pmci = container_of(m10bmc, struct m10bmc_pmci_device, m10bmc);
+	u32 blk_size, offset = 0, val, full_read_count, read_count;
+	int ret;
+
+	while (size) {
+		blk_size = min_t(u32, size, M10BMC_PMCI_READ_BLOCK_SIZE);
+		full_read_count = blk_size / M10BMC_PMCI_FIFO_WORD_SIZE;
+
+		read_count = full_read_count;
+		if (full_read_count * M10BMC_PMCI_FIFO_WORD_SIZE < blk_size)
+			read_count++;
+
+		writel(addr + offset, pmci->base + M10BMC_PMCI_FLASH_ADDR);
+		writel(FIELD_PREP(M10BMC_PMCI_FLASH_READ_COUNT, read_count) |
+		       M10BMC_PMCI_FLASH_RD_MODE,
+		       pmci->base + M10BMC_PMCI_FLASH_CTRL);
+
+		ret = readl_poll_timeout((pmci->base + M10BMC_PMCI_FLASH_CTRL), val,
+					 !(val & M10BMC_PMCI_FLASH_BUSY),
+					 M10BMC_PMCI_FLASH_INT_US, M10BMC_PMCI_FLASH_TIMEOUT_US);
+		if (ret) {
+			dev_err(m10bmc->dev, "read timed out on reading flash 0x%xn", val);
+			return ret;
+		}
+
+		if (size < M10BMC_PMCI_FIFO_WORD_SIZE)
+			break;
+
+		pmci_read_fifo(pmci->base + M10BMC_PMCI_FLASH_FIFO,
+			       (u32 *)(buf + offset), full_read_count);
+
+		size -= blk_size;
+		offset += blk_size;
+
+		writel(0, pmci->base + M10BMC_PMCI_FLASH_CTRL);
+	}
+
+	/* Handle remainder (less than M10BMC_PMCI_FIFO_WORD_SIZE bytes) */
+	if (size) {
+		u32 tmp;
+
+		pmci_read_fifo(pmci->base + M10BMC_PMCI_FLASH_FIFO, &tmp, 1);
+		memcpy(buf + offset, &tmp, size);
+	}
+
+	return 0;
+}
+
+static int m10bmc_pmci_flash_write(struct intel_m10bmc *m10bmc, const u8 *buf, u32 offset, u32 size)
+{
+	return pmci_flash_bulk_write(m10bmc, buf + offset, size);
+}
+
+static const struct intel_m10bmc_flash_bulk_ops m10bmc_pmci_flash_bulk_ops = {
+	.read = pmci_flash_bulk_read,
+	.write = m10bmc_pmci_flash_write,
+};
+
 static const struct regmap_range m10bmc_pmci_regmap_range[] = {
 	regmap_reg_range(M10BMC_N6000_SYS_BASE, M10BMC_N6000_SYS_END),
 };
@@ -169,6 +311,7 @@ static struct regmap_config m10bmc_pmci_regmap_config = {
 
 static struct mfd_cell m10bmc_pmci_n6000_bmc_subdevs[] = {
 	{ .name = "n6000bmc-hwmon" },
+	{ .name = "n6000bmc-sec-update" },
 };
 
 static const struct m10bmc_csr_map m10bmc_n6000_csr_map = {
@@ -208,6 +351,8 @@ static int m10bmc_pmci_probe(struct dfl_device *ddev)
 	if (!pmci)
 		return -ENOMEM;
 
+	pmci->m10bmc.flash_bulk_ops = &m10bmc_pmci_flash_bulk_ops;
+
 	pmci->m10bmc.dev = dev;
 
 	pmci->base = devm_ioremap_resource(dev, &ddev->mmio_res);
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index cc2d9eb597b0..8e437b996738 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -108,16 +108,30 @@ struct intel_m10bmc_platform_info {
 	const struct m10bmc_csr_map *csr_map;
 };
 
+struct intel_m10bmc;
+
+/**
+ * struct intel_m10bmc_flash_bulk_ops - device specific operations for flash R/W
+ * @read: read a block of data from flash
+ * @write: write a block of data to flash
+ */
+struct intel_m10bmc_flash_bulk_ops {
+	int (*read)(struct intel_m10bmc *m10bmc, u8 *buf, u32 addr, u32 size);
+	int (*write)(struct intel_m10bmc *m10bmc, const u8 *buf, u32 offset, u32 size);
+};
+
 /**
  * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
  * @dev: this device
  * @regmap: the regmap used to access registers by m10bmc itself
  * @info: the platform information for MAX10 BMC
+ * @flash_bulk_ops: optional device specific operations for flash R/W
  */
 struct intel_m10bmc {
 	struct device *dev;
 	struct regmap *regmap;
 	const struct intel_m10bmc_platform_info *info;
+	const struct intel_m10bmc_flash_bulk_ops *flash_bulk_ops;
 };
 
 /*
-- 
2.30.2

Re: [PATCH v5 07/10] fpga: m10bmc-sec: Create helpers for rsu status/progress checks

[Xu Yilun]

On 2022-12-26 at 19:58:46 +0200, Ilpo Järvinen wrote:
> RSU_STAT_* and RSU_PROG_* checks are done in more than one place in the sec
> update code. Move the checks into new helper functions.
> 
> No function changes intended.
> 
> Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Acked-by: Xu Yilun <yilun.xu@intel.com>

> ---
>  drivers/fpga/intel-m10-bmc-sec-update.c | 59 +++++++++++++------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> index 9922027856a4..6e58a463619c 100644
> --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> @@ -257,6 +257,28 @@ static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
>  		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
>  }
>  
> +static bool rsu_status_ok(u32 status)
> +{
> +	return (status == RSU_STAT_NORMAL ||
> +		status == RSU_STAT_NIOS_OK ||
> +		status == RSU_STAT_USER_OK ||
> +		status == RSU_STAT_FACTORY_OK);
> +}
> +
> +static bool rsu_progress_done(u32 progress)
> +{
> +	return (progress == RSU_PROG_IDLE ||
> +		progress == RSU_PROG_RSU_DONE);
> +}
> +
> +static bool rsu_progress_busy(u32 progress)
> +{
> +	return (progress == RSU_PROG_AUTHENTICATING ||
> +		progress == RSU_PROG_COPYING ||
> +		progress == RSU_PROG_UPDATE_CANCEL ||
> +		progress == RSU_PROG_PROGRAM_KEY_HASH);
> +}
> +
>  static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
>  {
>  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> @@ -267,8 +289,7 @@ static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
>  	if (ret)
>  		return FW_UPLOAD_ERR_RW_ERROR;
>  
> -	if (rsu_prog(doorbell) != RSU_PROG_IDLE &&
> -	    rsu_prog(doorbell) != RSU_PROG_RSU_DONE) {
> +	if (!rsu_progress_done(rsu_prog(doorbell))) {
>  		log_error_regs(sec, doorbell);
>  		return FW_UPLOAD_ERR_BUSY;
>  	}
> @@ -288,7 +309,7 @@ static inline bool rsu_start_done(u32 doorbell)
>  		return true;
>  
>  	progress = rsu_prog(doorbell);
> -	if (progress != RSU_PROG_IDLE && progress != RSU_PROG_RSU_DONE)
> +	if (!rsu_progress_done(progress))
>  		return true;
>  
>  	return false;
> @@ -397,13 +418,7 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
>  		return FW_UPLOAD_ERR_RW_ERROR;
>  	}
>  
> -	switch (rsu_stat(doorbell)) {
> -	case RSU_STAT_NORMAL:
> -	case RSU_STAT_NIOS_OK:
> -	case RSU_STAT_USER_OK:
> -	case RSU_STAT_FACTORY_OK:
> -		break;
> -	default:
> +	if (!rsu_status_ok(rsu_stat(doorbell))) {
>  		log_error_regs(sec, doorbell);
>  		return FW_UPLOAD_ERR_HW_ERROR;
>  	}
> @@ -418,28 +433,16 @@ static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell)
>  	if (m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell))
>  		return -EIO;
>  
> -	switch (rsu_stat(*doorbell)) {
> -	case RSU_STAT_NORMAL:
> -	case RSU_STAT_NIOS_OK:
> -	case RSU_STAT_USER_OK:
> -	case RSU_STAT_FACTORY_OK:
> -		break;
> -	default:
> +	if (!rsu_status_ok(rsu_stat(*doorbell)))
>  		return -EINVAL;
> -	}
>  
> -	switch (rsu_prog(*doorbell)) {
> -	case RSU_PROG_IDLE:
> -	case RSU_PROG_RSU_DONE:
> +	if (rsu_progress_done(rsu_prog(*doorbell)))
>  		return 0;
> -	case RSU_PROG_AUTHENTICATING:
> -	case RSU_PROG_COPYING:
> -	case RSU_PROG_UPDATE_CANCEL:
> -	case RSU_PROG_PROGRAM_KEY_HASH:
> +
> +	if (rsu_progress_busy(rsu_prog(*doorbell)))
>  		return -EAGAIN;
> -	default:
> -		return -EINVAL;
> -	}
> +
> +	return -EINVAL;
>  }
>  
>  static enum fw_upload_err rsu_cancel(struct m10bmc_sec *sec)
> -- 
> 2.30.2
> 

Re: [PATCH v5 08/10] fpga: m10bmc-sec: Differentiate rsu status from doorbell in csr map

[Xu Yilun]

On 2022-12-26 at 19:58:47 +0200, Ilpo Järvinen wrote:
> The rsu_status field moves from the doorbell register to the auth
> result register in the PMCI implementation of the MAX10 BMC. Refactor
> the sec update driver code to handle two distinct registers (rsu_status
> field was added into csr map already when it was introduced but it was
> unused until now).
> 
> Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/fpga/intel-m10-bmc-sec-update.c | 68 ++++++++++++++++---------
>  include/linux/mfd/intel-m10-bmc.h       |  2 +-
>  2 files changed, 46 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> index 6e58a463619c..1fe8b7ff594c 100644
> --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> @@ -251,7 +251,7 @@ static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
>  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
>  	u32 auth_result;
>  
> -	dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell);
> +	dev_err(sec->dev, "Doorbell: 0x%08x\n", doorbell);
>  
>  	if (!m10bmc_sys_read(sec->m10bmc, csr_map->auth_result, &auth_result))
>  		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
> @@ -279,6 +279,30 @@ static bool rsu_progress_busy(u32 progress)
>  		progress == RSU_PROG_PROGRAM_KEY_HASH);
>  }
>  
> +static int m10bmc_sec_progress_status(struct m10bmc_sec *sec, u32 *doorbell,

Please try to rename the parameters, to indicate u32 *doorbell is the
raw value from doorbell register, and u32 *progress & status are
software managed info.

> +				      u32 *progress, u32 *status)
> +{
> +	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> +	u32 status_reg;
> +	int ret;
> +
> +	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell);
> +	if (ret)
> +		return ret;
> +
> +	if (csr_map->doorbell != csr_map->rsu_status) {

I prefer not to complicate the csr map filling in intel-m10-bmc, just invalid
the addr value if there is no such register for the board.

> +		ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status_reg);
> +		if (ret)
> +			return ret;
> +		*status = rsu_stat(status_reg);
> +	} else {
> +		*status = rsu_stat(*doorbell);
> +	}
> +	*progress = rsu_prog(*doorbell);
> +
> +	return 0;
> +}
> +
>  static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
>  {
>  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> @@ -297,18 +321,14 @@ static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec)
>  	return FW_UPLOAD_ERR_NONE;
>  }
>  
> -static inline bool rsu_start_done(u32 doorbell)
> +static inline bool rsu_start_done(u32 doorbell, u32 progress, u32 status)

Same concern for the naming, some more below I didn't list.

>  {
> -	u32 status, progress;
> -
>  	if (doorbell & DRBL_RSU_REQUEST)
>  		return false;
>  
> -	status = rsu_stat(doorbell);
>  	if (status == RSU_STAT_ERASE_FAIL || status == RSU_STAT_WEAROUT)
>  		return true;
>  
> -	progress = rsu_prog(doorbell);
>  	if (!rsu_progress_done(progress))
>  		return true;
>  
> @@ -318,8 +338,8 @@ static inline bool rsu_start_done(u32 doorbell)
>  static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
>  {
>  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> -	u32 doorbell, status;
> -	int ret;
> +	u32 doorbell, progress, status;
> +	int ret, err;
>  
>  	ret = regmap_update_bits(sec->m10bmc->regmap,
>  				 csr_map->base + csr_map->doorbell,
> @@ -330,21 +350,20 @@ static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
>  	if (ret)
>  		return FW_UPLOAD_ERR_RW_ERROR;
>  
> -	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
> -				       csr_map->base + csr_map->doorbell,
> -				       doorbell,
> -				       rsu_start_done(doorbell),
> -				       NIOS_HANDSHAKE_INTERVAL_US,
> -				       NIOS_HANDSHAKE_TIMEOUT_US);
> +	ret = read_poll_timeout(m10bmc_sec_progress_status, err,
> +				err < 0 || rsu_start_done(doorbell, progress, status),
> +				NIOS_HANDSHAKE_INTERVAL_US,
> +				NIOS_HANDSHAKE_TIMEOUT_US,
> +				false,
> +				sec, &doorbell, &progress, &status);
>  
>  	if (ret == -ETIMEDOUT) {
>  		log_error_regs(sec, doorbell);
>  		return FW_UPLOAD_ERR_TIMEOUT;
> -	} else if (ret) {
> +	} else if (err) {
>  		return FW_UPLOAD_ERR_RW_ERROR;
>  	}
>  
> -	status = rsu_stat(doorbell);
>  	if (status == RSU_STAT_WEAROUT) {
>  		dev_warn(sec->dev, "Excessive flash update count detected\n");
>  		return FW_UPLOAD_ERR_WEAROUT;
> @@ -393,7 +412,7 @@ static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec)
>  static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
>  {
>  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> -	u32 doorbell;
> +	u32 doorbell, status;
>  	int ret;
>  
>  	ret = regmap_update_bits(sec->m10bmc->regmap,
> @@ -418,7 +437,10 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
>  		return FW_UPLOAD_ERR_RW_ERROR;
>  	}
>  
> -	if (!rsu_status_ok(rsu_stat(doorbell))) {
> +	ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status);

Same as above, please just handle the detailed register definition differences
in this driver, not in csr map.

Thanks,
Yilun

> +	if (ret)
> +		return ret;
> +	if (!rsu_status_ok(rsu_stat(status))) {
>  		log_error_regs(sec, doorbell);
>  		return FW_UPLOAD_ERR_HW_ERROR;
>  	}
> @@ -428,18 +450,18 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
>  
>  static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell)
>  {
> -	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> +	u32 progress, status;
>  
> -	if (m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell))
> +	if (m10bmc_sec_progress_status(sec, doorbell, &progress, &status))
>  		return -EIO;
>  
> -	if (!rsu_status_ok(rsu_stat(*doorbell)))
> +	if (!rsu_status_ok(status))
>  		return -EINVAL;
>  
> -	if (rsu_progress_done(rsu_prog(*doorbell)))
> +	if (rsu_progress_done(progress))
>  		return 0;
>  
> -	if (rsu_progress_busy(rsu_prog(*doorbell)))
> +	if (rsu_progress_busy(progress))
>  		return -EAGAIN;
>  
>  	return -EINVAL;
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index 42e2ce7fe439..cc2d9eb597b0 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -58,7 +58,7 @@
>  #define HOST_STATUS_ABORT_RSU		0x2
>  
>  #define rsu_prog(doorbell)	FIELD_GET(DRBL_RSU_PROGRESS, doorbell)
> -#define rsu_stat(doorbell)	FIELD_GET(DRBL_RSU_STATUS, doorbell)
> +#define rsu_stat(status_reg)	FIELD_GET(DRBL_RSU_STATUS, status_reg)
>  
>  /* interval 100ms and timeout 5s */
>  #define NIOS_HANDSHAKE_INTERVAL_US	(100 * 1000)
> -- 
> 2.30.2
> 

Re: [PATCH v5 10/10] fpga: m10bmc-sec: Add support for N6000

[Xu Yilun]

On 2022-12-26 at 19:58:49 +0200, Ilpo Järvinen wrote:
> Add support for PMCI-based flash access path and N6000 sec update
> support. Access to flash staging area is different for N6000 from that
> of the SPI interfaced counterparts.
> 
> Introduce intel_m10bmc_flash_bulk_ops to allow interface specific
> differentiations for the flash access path for sec update and make
> m10bmc_sec_read/write() in sec update driver to use the new operations.
> 
> If a failure is detected while altering the flash MUX, it seems safer
> to try to set it back and doesn't seem harmful. Likely there are enough
> troubles in that case anyway so setting it back fails too (which is
> harmless sans the small extra delay) or just confirms that the value
> wasn't changed.
> 
> Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Acked-by: Xu Yilun <yilun.xu@intel.com>

> ---
>  drivers/fpga/intel-m10-bmc-sec-update.c |  65 ++++++++++-
>  drivers/mfd/intel-m10-bmc-pmci.c        | 145 ++++++++++++++++++++++++
>  include/linux/mfd/intel-m10-bmc.h       |  14 +++
>  3 files changed, 223 insertions(+), 1 deletion(-)

Re: [PATCH v5 08/10] fpga: m10bmc-sec: Differentiate rsu status from doorbell in csr map

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5055 bytes --]

On Fri, 30 Dec 2022, Xu Yilun wrote:

> On 2022-12-26 at 19:58:47 +0200, Ilpo Järvinen wrote:
> > The rsu_status field moves from the doorbell register to the auth
> > result register in the PMCI implementation of the MAX10 BMC. Refactor
> > the sec update driver code to handle two distinct registers (rsu_status
> > field was added into csr map already when it was introduced but it was
> > unused until now).
> > 
> > Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> > Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  drivers/fpga/intel-m10-bmc-sec-update.c | 68 ++++++++++++++++---------
> >  include/linux/mfd/intel-m10-bmc.h       |  2 +-
> >  2 files changed, 46 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> > index 6e58a463619c..1fe8b7ff594c 100644
> > --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> > +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> > @@ -251,7 +251,7 @@ static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
> >  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> >  	u32 auth_result;
> >  
> > -	dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell);
> > +	dev_err(sec->dev, "Doorbell: 0x%08x\n", doorbell);
> >  
> >  	if (!m10bmc_sys_read(sec->m10bmc, csr_map->auth_result, &auth_result))
> >  		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
> > @@ -279,6 +279,30 @@ static bool rsu_progress_busy(u32 progress)
> >  		progress == RSU_PROG_PROGRAM_KEY_HASH);
> >  }
> >  
> > +static int m10bmc_sec_progress_status(struct m10bmc_sec *sec, u32 *doorbell,
> 
> Please try to rename the parameters, to indicate u32 *doorbell is the
> raw value from doorbell register, and u32 *progress & status are
> software managed info.

I'll try to do that.
 
> > +				      u32 *progress, u32 *status)
> > +{
> > +	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > +	u32 status_reg;
> > +	int ret;
> > +
> > +	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (csr_map->doorbell != csr_map->rsu_status) {
> 
> I prefer not to complicate the csr map filling in intel-m10-bmc, just invalid
> the addr value if there is no such register for the board.

I'm sorry but I didn't get the meaning of your comment. Could you please 
rephrase?

My guess is that you might have tried to say that if there's no register 
for rsu_status, mark it not existing in csr map? But the field exists in 
both cases, it's just part of a different register (doorbell or 
auth_result) so if I use that kind of "register doesn't exist" condition, 
it would apply to both cases.

> > @@ -330,21 +350,20 @@ static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
> >  	if (ret)
> >  		return FW_UPLOAD_ERR_RW_ERROR;
> >  
> > -	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
> > -				       csr_map->base + csr_map->doorbell,
> > -				       doorbell,
> > -				       rsu_start_done(doorbell),
> > -				       NIOS_HANDSHAKE_INTERVAL_US,
> > -				       NIOS_HANDSHAKE_TIMEOUT_US);
> > +	ret = read_poll_timeout(m10bmc_sec_progress_status, err,
> > +				err < 0 || rsu_start_done(doorbell, progress, status),
> > +				NIOS_HANDSHAKE_INTERVAL_US,
> > +				NIOS_HANDSHAKE_TIMEOUT_US,
> > +				false,
> > +				sec, &doorbell, &progress, &status);
> >  
> >  	if (ret == -ETIMEDOUT) {
> >  		log_error_regs(sec, doorbell);
> >  		return FW_UPLOAD_ERR_TIMEOUT;
> > -	} else if (ret) {
> > +	} else if (err) {
> >  		return FW_UPLOAD_ERR_RW_ERROR;
> >  	}
> >  
> > -	status = rsu_stat(doorbell);
> >  	if (status == RSU_STAT_WEAROUT) {
> >  		dev_warn(sec->dev, "Excessive flash update count detected\n");
> >  		return FW_UPLOAD_ERR_WEAROUT;
> > @@ -393,7 +412,7 @@ static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec)
> >  static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
> >  {
> >  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > -	u32 doorbell;
> > +	u32 doorbell, status;
> >  	int ret;
> >  
> >  	ret = regmap_update_bits(sec->m10bmc->regmap,
> > @@ -418,7 +437,10 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
> >  		return FW_UPLOAD_ERR_RW_ERROR;
> >  	}
> >  
> > -	if (!rsu_status_ok(rsu_stat(doorbell))) {
> > +	ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status);
> 
> Same as above, please just handle the detailed register definition 
> differences in this driver, not in csr map.

Earlier you were having the exactly opposite opinion:

https://lore.kernel.org/linux-fpga/20221108144305.45424-1-ilpo.jarvinen@linux.intel.com/T/#me2d20e60d7feeafcdeeab4d58bd82787acf3ada9

So which way you want it? Should I have the board types here in the sec 
update drivers as a second layer of differentiation or not?


-- 
 i.

Re: [PATCH v5 08/10] fpga: m10bmc-sec: Differentiate rsu status from doorbell in csr map

[Xu Yilun]

On 2022-12-30 at 12:23:18 +0200, Ilpo Järvinen wrote:
> On Fri, 30 Dec 2022, Xu Yilun wrote:
> 
> > On 2022-12-26 at 19:58:47 +0200, Ilpo Järvinen wrote:
> > > The rsu_status field moves from the doorbell register to the auth
> > > result register in the PMCI implementation of the MAX10 BMC. Refactor
> > > the sec update driver code to handle two distinct registers (rsu_status
> > > field was added into csr map already when it was introduced but it was
> > > unused until now).
> > > 
> > > Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> > > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> > > Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> > > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > >  drivers/fpga/intel-m10-bmc-sec-update.c | 68 ++++++++++++++++---------
> > >  include/linux/mfd/intel-m10-bmc.h       |  2 +-
> > >  2 files changed, 46 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> > > index 6e58a463619c..1fe8b7ff594c 100644
> > > --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> > > +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> > > @@ -251,7 +251,7 @@ static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
> > >  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > >  	u32 auth_result;
> > >  
> > > -	dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell);
> > > +	dev_err(sec->dev, "Doorbell: 0x%08x\n", doorbell);
> > >  
> > >  	if (!m10bmc_sys_read(sec->m10bmc, csr_map->auth_result, &auth_result))
> > >  		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
> > > @@ -279,6 +279,30 @@ static bool rsu_progress_busy(u32 progress)
> > >  		progress == RSU_PROG_PROGRAM_KEY_HASH);
> > >  }
> > >  
> > > +static int m10bmc_sec_progress_status(struct m10bmc_sec *sec, u32 *doorbell,
> > 
> > Please try to rename the parameters, to indicate u32 *doorbell is the
> > raw value from doorbell register, and u32 *progress & status are
> > software managed info.
> 
> I'll try to do that.
>  
> > > +				      u32 *progress, u32 *status)
> > > +{
> > > +	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > > +	u32 status_reg;
> > > +	int ret;
> > > +
> > > +	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (csr_map->doorbell != csr_map->rsu_status) {
> > 
> > I prefer not to complicate the csr map filling in intel-m10-bmc, just invalid
> > the addr value if there is no such register for the board.
> 
> I'm sorry but I didn't get the meaning of your comment. Could you please 
> rephrase?
> 
> My guess is that you might have tried to say that if there's no register 
> for rsu_status, mark it not existing in csr map? But the field exists in 

Yes, this is what I mean, but I see I was wrong.

> both cases, it's just part of a different register (doorbell or 

I was thinking there was no AUTH_RESULT for N3000, sorry for the
mistake.

> auth_result) so if I use that kind of "register doesn't exist" condition, 
> it would apply to both cases.
> 
> > > @@ -330,21 +350,20 @@ static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
> > >  	if (ret)
> > >  		return FW_UPLOAD_ERR_RW_ERROR;
> > >  
> > > -	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
> > > -				       csr_map->base + csr_map->doorbell,
> > > -				       doorbell,
> > > -				       rsu_start_done(doorbell),
> > > -				       NIOS_HANDSHAKE_INTERVAL_US,
> > > -				       NIOS_HANDSHAKE_TIMEOUT_US);
> > > +	ret = read_poll_timeout(m10bmc_sec_progress_status, err,
> > > +				err < 0 || rsu_start_done(doorbell, progress, status),
> > > +				NIOS_HANDSHAKE_INTERVAL_US,
> > > +				NIOS_HANDSHAKE_TIMEOUT_US,
> > > +				false,
> > > +				sec, &doorbell, &progress, &status);
> > >  
> > >  	if (ret == -ETIMEDOUT) {
> > >  		log_error_regs(sec, doorbell);
> > >  		return FW_UPLOAD_ERR_TIMEOUT;
> > > -	} else if (ret) {
> > > +	} else if (err) {
> > >  		return FW_UPLOAD_ERR_RW_ERROR;
> > >  	}
> > >  
> > > -	status = rsu_stat(doorbell);
> > >  	if (status == RSU_STAT_WEAROUT) {
> > >  		dev_warn(sec->dev, "Excessive flash update count detected\n");
> > >  		return FW_UPLOAD_ERR_WEAROUT;
> > > @@ -393,7 +412,7 @@ static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec)
> > >  static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
> > >  {
> > >  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > > -	u32 doorbell;
> > > +	u32 doorbell, status;
> > >  	int ret;
> > >  
> > >  	ret = regmap_update_bits(sec->m10bmc->regmap,
> > > @@ -418,7 +437,10 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
> > >  		return FW_UPLOAD_ERR_RW_ERROR;
> > >  	}
> > >  
> > > -	if (!rsu_status_ok(rsu_stat(doorbell))) {
> > > +	ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status);
> > 
> > Same as above, please just handle the detailed register definition 
> > differences in this driver, not in csr map.
> 
> Earlier you were having the exactly opposite opinion:
> 
> https://lore.kernel.org/linux-fpga/20221108144305.45424-1-ilpo.jarvinen@linux.intel.com/T/#me2d20e60d7feeafcdeeab4d58bd82787acf3ada9

Ah, I'm sorry. I was thinking just move one register to another addr at
that time. I was not aware that actually the detailed register field
definitions are changed in same registers.

> 
> So which way you want it? Should I have the board types here in the sec 
> update drivers as a second layer of differentiation or not?

I think the different register field definitions for the same registers
are specific to secure driver. So please differentiate them in secure
driver.

But with the change, enum m10bmc_type could still be removed, is it?

And having the register addr differentiations in m10bmc mfd driver is good to
me, cause with a different board type, the register offsets for all subdevs
are often globally re-arranged. But I don't want the HW change within a
single IP block been specified in m10bmc mfd driver.

Thanks,
Yilun
> 
> 
> -- 
>  i.

Re: [PATCH v5 08/10] fpga: m10bmc-sec: Differentiate rsu status from doorbell in csr map

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 6724 bytes --]

On Tue, 3 Jan 2023, Xu Yilun wrote:

> On 2022-12-30 at 12:23:18 +0200, Ilpo Järvinen wrote:
> > On Fri, 30 Dec 2022, Xu Yilun wrote:
> > 
> > > On 2022-12-26 at 19:58:47 +0200, Ilpo Järvinen wrote:
> > > > The rsu_status field moves from the doorbell register to the auth
> > > > result register in the PMCI implementation of the MAX10 BMC. Refactor
> > > > the sec update driver code to handle two distinct registers (rsu_status
> > > > field was added into csr map already when it was introduced but it was
> > > > unused until now).
> > > > 
> > > > Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> > > > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> > > > Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> > > > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > ---
> > > >  drivers/fpga/intel-m10-bmc-sec-update.c | 68 ++++++++++++++++---------
> > > >  include/linux/mfd/intel-m10-bmc.h       |  2 +-
> > > >  2 files changed, 46 insertions(+), 24 deletions(-)
> > > > 
> > > > diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> > > > index 6e58a463619c..1fe8b7ff594c 100644
> > > > --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> > > > +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> > > > @@ -251,7 +251,7 @@ static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
> > > >  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > > >  	u32 auth_result;
> > > >  
> > > > -	dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell);
> > > > +	dev_err(sec->dev, "Doorbell: 0x%08x\n", doorbell);
> > > >  
> > > >  	if (!m10bmc_sys_read(sec->m10bmc, csr_map->auth_result, &auth_result))
> > > >  		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
> > > > @@ -279,6 +279,30 @@ static bool rsu_progress_busy(u32 progress)
> > > >  		progress == RSU_PROG_PROGRAM_KEY_HASH);
> > > >  }
> > > >  
> > > > +static int m10bmc_sec_progress_status(struct m10bmc_sec *sec, u32 *doorbell,
> > > 
> > > Please try to rename the parameters, to indicate u32 *doorbell is the
> > > raw value from doorbell register, and u32 *progress & status are
> > > software managed info.
> > 
> > I'll try to do that.
> >  
> > > > +				      u32 *progress, u32 *status)
> > > > +{
> > > > +	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > > > +	u32 status_reg;
> > > > +	int ret;
> > > > +
> > > > +	ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	if (csr_map->doorbell != csr_map->rsu_status) {
> > > 
> > > I prefer not to complicate the csr map filling in intel-m10-bmc, just invalid
> > > the addr value if there is no such register for the board.
> > 
> > I'm sorry but I didn't get the meaning of your comment. Could you please 
> > rephrase?
> > 
> > My guess is that you might have tried to say that if there's no register 
> > for rsu_status, mark it not existing in csr map? But the field exists in 
> 
> Yes, this is what I mean, but I see I was wrong.
> 
> > both cases, it's just part of a different register (doorbell or 
> 
> I was thinking there was no AUTH_RESULT for N3000, sorry for the
> mistake.
> 
> > auth_result) so if I use that kind of "register doesn't exist" condition, 
> > it would apply to both cases.
> > 
> > > > @@ -330,21 +350,20 @@ static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec)
> > > >  	if (ret)
> > > >  		return FW_UPLOAD_ERR_RW_ERROR;
> > > >  
> > > > -	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
> > > > -				       csr_map->base + csr_map->doorbell,
> > > > -				       doorbell,
> > > > -				       rsu_start_done(doorbell),
> > > > -				       NIOS_HANDSHAKE_INTERVAL_US,
> > > > -				       NIOS_HANDSHAKE_TIMEOUT_US);
> > > > +	ret = read_poll_timeout(m10bmc_sec_progress_status, err,
> > > > +				err < 0 || rsu_start_done(doorbell, progress, status),
> > > > +				NIOS_HANDSHAKE_INTERVAL_US,
> > > > +				NIOS_HANDSHAKE_TIMEOUT_US,
> > > > +				false,
> > > > +				sec, &doorbell, &progress, &status);
> > > >  
> > > >  	if (ret == -ETIMEDOUT) {
> > > >  		log_error_regs(sec, doorbell);
> > > >  		return FW_UPLOAD_ERR_TIMEOUT;
> > > > -	} else if (ret) {
> > > > +	} else if (err) {
> > > >  		return FW_UPLOAD_ERR_RW_ERROR;
> > > >  	}
> > > >  
> > > > -	status = rsu_stat(doorbell);
> > > >  	if (status == RSU_STAT_WEAROUT) {
> > > >  		dev_warn(sec->dev, "Excessive flash update count detected\n");
> > > >  		return FW_UPLOAD_ERR_WEAROUT;
> > > > @@ -393,7 +412,7 @@ static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec)
> > > >  static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
> > > >  {
> > > >  	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > > > -	u32 doorbell;
> > > > +	u32 doorbell, status;
> > > >  	int ret;
> > > >  
> > > >  	ret = regmap_update_bits(sec->m10bmc->regmap,
> > > > @@ -418,7 +437,10 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec)
> > > >  		return FW_UPLOAD_ERR_RW_ERROR;
> > > >  	}
> > > >  
> > > > -	if (!rsu_status_ok(rsu_stat(doorbell))) {
> > > > +	ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status);
> > > 
> > > Same as above, please just handle the detailed register definition 
> > > differences in this driver, not in csr map.
> > 
> > Earlier you were having the exactly opposite opinion:
> > 
> > https://lore.kernel.org/linux-fpga/20221108144305.45424-1-ilpo.jarvinen@linux.intel.com/T/#me2d20e60d7feeafcdeeab4d58bd82787acf3ada9
> 
> Ah, I'm sorry. I was thinking just move one register to another addr at
> that time. I was not aware that actually the detailed register field
> definitions are changed in same registers.
> 
> > 
> > So which way you want it? Should I have the board types here in the sec 
> > update drivers as a second layer of differentiation or not?
> 
> I think the different register field definitions for the same registers
> are specific to secure driver. So please differentiate them in secure
> driver.
> 
> But with the change, enum m10bmc_type could still be removed, is it?
>
> And having the register addr differentiations in m10bmc mfd driver is good to
> me, cause with a different board type, the register offsets for all subdevs
> are often globally re-arranged. But I don't want the HW change within a
> single IP block been specified in m10bmc mfd driver.

Okay. I'll add ops for it then:

struct m10bmc_sec_ops {
       int (*rsu_status)(struct m10bmc_sec *sec);
};

Type enum won't be necessary. Those ops will be useful for other things 
too which are not included to this patch set.

-- 
 i.

Re: [PATCH v5 01/10] mfd: intel-m10-bmc: Create m10bmc_platform_info for type specific info

[Lee Jones]

On Mon, 26 Dec 2022, Ilpo Järvinen wrote:

> BMC type specific info is currently set by a switch/case block. The
> size of this info is expected to grow as more dev types and features
> are added which would have made the switch block bloaty.
> 
> Store type specific info into struct and place them into .driver_data
> instead because it makes things a bit cleaner.
> 
> The m10bmc_type enum can be dropped as the differentiation is now
> fully handled by the platform info.
> 
> The info member of struct intel_m10bmc that is added here is not used
> yet in this change but its addition logically still belongs to this
> change. The CSR map change that comes after this change needs to have
> the info member.
> 
> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/mfd/intel-m10-bmc.c       | 53 ++++++++++++++-----------------
>  include/linux/mfd/intel-m10-bmc.h | 12 +++++++
>  2 files changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> index 7e3319e5b22f..12c522c16d83 100644
> --- a/drivers/mfd/intel-m10-bmc.c
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -13,12 +13,6 @@
>  #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" },
>  	{ .name = "d5005bmc-sec-update" }
> @@ -162,15 +156,17 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
>  static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
> +	const struct intel_m10bmc_platform_info *info;
>  	struct device *dev = &spi->dev;
> -	struct mfd_cell *cells;
>  	struct intel_m10bmc *ddata;
> -	int ret, n_cell;
> +	int ret;
>  
>  	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
>  	if (!ddata)
>  		return -ENOMEM;
>  
> +	info = (struct intel_m10bmc_platform_info *)id->driver_data;
> +	ddata->info = info;

Why are you keeping it?

>  	ddata->dev = dev;
>  
>  	ddata->regmap =
> @@ -189,24 +185,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  		return ret;
>  	}
>  
> -	switch (id->driver_data) {
> -	case M10_N3000:
> -		cells = m10bmc_pacn3000_subdevs;
> -		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> -		break;
> -	case M10_D5005:
> -		cells = m10bmc_d5005_subdevs;
> -		n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs);
> -		break;
> -	case M10_N5010:
> -		cells = m10bmc_n5010_subdevs;
> -		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
> -		break;
> -	default:
> -		return -ENODEV;
> -	}
> -
> -	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> +				   info->cells, info->n_cells,
>  				   NULL, 0, NULL);
>  	if (ret)
>  		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
> @@ -214,10 +194,25 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  	return ret;
>  }
>  
> +static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = {
> +	.cells = m10bmc_pacn3000_subdevs,
> +	.n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs),
> +};

Not seeing how adding a whole new structure and swapping out 4 lines to
describe a device for a different 4 lines per device is better?

I'm not necessarily against it.  Just seems like a bit of a pointless
exercise.

> +static const struct intel_m10bmc_platform_info m10bmc_spi_d5005 = {
> +	.cells = m10bmc_d5005_subdevs,
> +	.n_cells = ARRAY_SIZE(m10bmc_d5005_subdevs),
> +};
> +
> +static const struct intel_m10bmc_platform_info m10bmc_spi_n5010 = {
> +	.cells = m10bmc_n5010_subdevs,
> +	.n_cells = ARRAY_SIZE(m10bmc_n5010_subdevs),
> +};
> +
>  static const struct spi_device_id m10bmc_spi_id[] = {
> -	{ "m10-n3000", M10_N3000 },
> -	{ "m10-d5005", M10_D5005 },
> -	{ "m10-n5010", M10_N5010 },
> +	{ "m10-n3000", (kernel_ulong_t)&m10bmc_spi_n3000 },
> +	{ "m10-d5005", (kernel_ulong_t)&m10bmc_spi_d5005 },
> +	{ "m10-n5010", (kernel_ulong_t)&m10bmc_spi_n5010 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index f0044b14136e..725b51ea4aee 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -118,14 +118,26 @@
>  /* Address of 4KB inverted bit vector containing staging area FLASH count */
>  #define STAGING_FLASH_COUNT	0x17ffb000
>  
> +/**
> + * struct intel_m10bmc_platform_info - Intel MAX 10 BMC platform specific information
> + * @cells: MFD cells
> + * @n_cells: MFD cells ARRAY_SIZE()
> + */
> +struct intel_m10bmc_platform_info {
> +	struct mfd_cell *cells;
> +	int n_cells;
> +};
> +
>  /**
>   * struct intel_m10bmc - Intel MAX 10 BMC parent driver data structure
>   * @dev: this device
>   * @regmap: the regmap used to access registers by m10bmc itself
> + * @info: the platform information for MAX10 BMC
>   */
>  struct intel_m10bmc {
>  	struct device *dev;
>  	struct regmap *regmap;
> +	const struct intel_m10bmc_platform_info *info;
>  };
>  
>  /*
> -- 
> 2.30.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 01/10] mfd: intel-m10-bmc: Create m10bmc_platform_info for type specific info

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4416 bytes --]

On Mon, 9 Jan 2023, Lee Jones wrote:

> On Mon, 26 Dec 2022, Ilpo Järvinen wrote:
> 
> > BMC type specific info is currently set by a switch/case block. The
> > size of this info is expected to grow as more dev types and features
> > are added which would have made the switch block bloaty.
> > 
> > Store type specific info into struct and place them into .driver_data
> > instead because it makes things a bit cleaner.
> > 
> > The m10bmc_type enum can be dropped as the differentiation is now
> > fully handled by the platform info.
> > 
> > The info member of struct intel_m10bmc that is added here is not used
> > yet in this change but its addition logically still belongs to this
> > change. The CSR map change that comes after this change needs to have
> > the info member.
> > 
> > Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> > Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  drivers/mfd/intel-m10-bmc.c       | 53 ++++++++++++++-----------------
> >  include/linux/mfd/intel-m10-bmc.h | 12 +++++++
> >  2 files changed, 36 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> > index 7e3319e5b22f..12c522c16d83 100644
> > --- a/drivers/mfd/intel-m10-bmc.c
> > +++ b/drivers/mfd/intel-m10-bmc.c
> > @@ -13,12 +13,6 @@
> >  #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" },
> >  	{ .name = "d5005bmc-sec-update" }
> > @@ -162,15 +156,17 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
> >  static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> >  {
> >  	const struct spi_device_id *id = spi_get_device_id(spi);
> > +	const struct intel_m10bmc_platform_info *info;
> >  	struct device *dev = &spi->dev;
> > -	struct mfd_cell *cells;
> >  	struct intel_m10bmc *ddata;
> > -	int ret, n_cell;
> > +	int ret;
> >  
> >  	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> >  	if (!ddata)
> >  		return -ENOMEM;
> >  
> > +	info = (struct intel_m10bmc_platform_info *)id->driver_data;
> > +	ddata->info = info;
> 
> Why are you keeping it?

There are plenty of users starting from patch 04. There will more users 
and members in the changes not included into this series. Thus, storing 
csr_map instead of info would not be forward-looking enough.

> >  	ddata->dev = dev;
> >  
> >  	ddata->regmap =
> > @@ -189,24 +185,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> >  		return ret;
> >  	}
> >  
> > -	switch (id->driver_data) {
> > -	case M10_N3000:
> > -		cells = m10bmc_pacn3000_subdevs;
> > -		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> > -		break;
> > -	case M10_D5005:
> > -		cells = m10bmc_d5005_subdevs;
> > -		n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs);
> > -		break;
> > -	case M10_N5010:
> > -		cells = m10bmc_n5010_subdevs;
> > -		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
> > -		break;
> > -	default:
> > -		return -ENODEV;
> > -	}
> > -
> > -	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> > +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> > +				   info->cells, info->n_cells,
> >  				   NULL, 0, NULL);
> >  	if (ret)
> >  		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
> > @@ -214,10 +194,25 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> >  	return ret;
> >  }
> >  
> > +static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = {
> > +	.cells = m10bmc_pacn3000_subdevs,
> > +	.n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs),
> > +};
> 
> Not seeing how adding a whole new structure and swapping out 4 lines to
> describe a device for a different 4 lines per device is better?
> 
> I'm not necessarily against it.  Just seems like a bit of a pointless
> exercise.

After the BMC core/SPI split in a later patch in this series, there will 
be an init func in m10bmc core that will be called from spi side and 
after PMCI is added, from there too. 

With a structure, only a pointer to that will have to be passed to the 
init func rather than n parameters (there will be more members added into 
the info structure too both by changes in this series and in the ones not 
included to this series).


Thanks for the review.

-- 
 i.

Re: [PATCH v5 02/10] mfd: intel-m10-bmc: Rename the local variables

[Lee Jones]

On Mon, 26 Dec 2022, Ilpo Järvinen wrote:

> Local variables directly interact with dev_get_drvdata/dev_set_drvdata
> should be named ddata.
> 
> Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>

It took 2 people to rename some variables? :)

> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.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 12c522c16d83..2c26203c4799 100644
> --- a/drivers/mfd/intel-m10-bmc.c
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -81,15 +81,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);

In general I'm all for the use of 'ddata' for driver data.

For my own reference (apply this as-is to your sign-off block):
                                                       
Acked-for-MFD-by: Lee Jones <lee@kernel.org>

>  	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;
>  
> @@ -106,11 +106,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.30.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 02/10] mfd: intel-m10-bmc: Rename the local variables

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

On Mon, 9 Jan 2023, Lee Jones wrote:

> On Mon, 26 Dec 2022, Ilpo Järvinen wrote:
> 
> > Local variables directly interact with dev_get_drvdata/dev_set_drvdata
> > should be named ddata.
> > 
> > Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> 
> It took 2 people to rename some variables? :)

It took one person to rename the variables, and other to prepare it into a 
series which required some changes to the original patch (which is when 
I added my SoB). But I can remove Tianfei (Andy mentioned earlier for 
simple changes it's okay).

-- 
 i.

> > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> > Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> > Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.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 12c522c16d83..2c26203c4799 100644
> > --- a/drivers/mfd/intel-m10-bmc.c
> > +++ b/drivers/mfd/intel-m10-bmc.c
> > @@ -81,15 +81,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);
> 
> In general I'm all for the use of 'ddata' for driver data.
> 
> For my own reference (apply this as-is to your sign-off block):
>                                                        
> Acked-for-MFD-by: Lee Jones <lee@kernel.org>

Re: [PATCH v5 01/10] mfd: intel-m10-bmc: Create m10bmc_platform_info for type specific info

[Lee Jones]

On Mon, 09 Jan 2023, Ilpo Järvinen wrote:

> On Mon, 9 Jan 2023, Lee Jones wrote:
> 
> > On Mon, 26 Dec 2022, Ilpo Järvinen wrote:
> > 
> > > BMC type specific info is currently set by a switch/case block. The
> > > size of this info is expected to grow as more dev types and features
> > > are added which would have made the switch block bloaty.
> > > 
> > > Store type specific info into struct and place them into .driver_data
> > > instead because it makes things a bit cleaner.
> > > 
> > > The m10bmc_type enum can be dropped as the differentiation is now
> > > fully handled by the platform info.
> > > 
> > > The info member of struct intel_m10bmc that is added here is not used
> > > yet in this change but its addition logically still belongs to this
> > > change. The CSR map change that comes after this change needs to have
> > > the info member.
> > > 
> > > Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> > > Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > >  drivers/mfd/intel-m10-bmc.c       | 53 ++++++++++++++-----------------
> > >  include/linux/mfd/intel-m10-bmc.h | 12 +++++++
> > >  2 files changed, 36 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> > > index 7e3319e5b22f..12c522c16d83 100644
> > > --- a/drivers/mfd/intel-m10-bmc.c
> > > +++ b/drivers/mfd/intel-m10-bmc.c
> > > @@ -13,12 +13,6 @@
> > >  #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" },
> > >  	{ .name = "d5005bmc-sec-update" }
> > > @@ -162,15 +156,17 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
> > >  static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> > >  {
> > >  	const struct spi_device_id *id = spi_get_device_id(spi);
> > > +	const struct intel_m10bmc_platform_info *info;
> > >  	struct device *dev = &spi->dev;
> > > -	struct mfd_cell *cells;
> > >  	struct intel_m10bmc *ddata;
> > > -	int ret, n_cell;
> > > +	int ret;
> > >  
> > >  	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > >  	if (!ddata)
> > >  		return -ENOMEM;
> > >  
> > > +	info = (struct intel_m10bmc_platform_info *)id->driver_data;
> > > +	ddata->info = info;
> > 
> > Why are you keeping it?
> 
> There are plenty of users starting from patch 04. There will more users 
> and members in the changes not included into this series. Thus, storing 
> csr_map instead of info would not be forward-looking enough.
> 
> > >  	ddata->dev = dev;
> > >  
> > >  	ddata->regmap =
> > > @@ -189,24 +185,8 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> > >  		return ret;
> > >  	}
> > >  
> > > -	switch (id->driver_data) {
> > > -	case M10_N3000:
> > > -		cells = m10bmc_pacn3000_subdevs;
> > > -		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> > > -		break;
> > > -	case M10_D5005:
> > > -		cells = m10bmc_d5005_subdevs;
> > > -		n_cell = ARRAY_SIZE(m10bmc_d5005_subdevs);
> > > -		break;
> > > -	case M10_N5010:
> > > -		cells = m10bmc_n5010_subdevs;
> > > -		n_cell = ARRAY_SIZE(m10bmc_n5010_subdevs);
> > > -		break;
> > > -	default:
> > > -		return -ENODEV;
> > > -	}
> > > -
> > > -	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, cells, n_cell,
> > > +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> > > +				   info->cells, info->n_cells,
> > >  				   NULL, 0, NULL);
> > >  	if (ret)
> > >  		dev_err(dev, "Failed to register sub-devices: %d\n", ret);
> > > @@ -214,10 +194,25 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
> > >  	return ret;
> > >  }
> > >  
> > > +static const struct intel_m10bmc_platform_info m10bmc_spi_n3000 = {
> > > +	.cells = m10bmc_pacn3000_subdevs,
> > > +	.n_cells = ARRAY_SIZE(m10bmc_pacn3000_subdevs),
> > > +};
> > 
> > Not seeing how adding a whole new structure and swapping out 4 lines to
> > describe a device for a different 4 lines per device is better?
> > 
> > I'm not necessarily against it.  Just seems like a bit of a pointless
> > exercise.
> 
> After the BMC core/SPI split in a later patch in this series, there will 
> be an init func in m10bmc core that will be called from spi side and 
> after PMCI is added, from there too. 
> 
> With a structure, only a pointer to that will have to be passed to the 
> init func rather than n parameters (there will be more members added into 
> the info structure too both by changes in this series and in the ones not 
> included to this series).

Very well.  Please consider these review comments as tentative, until I
get a chance to dig deeper into the patch-set.

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 02/10] mfd: intel-m10-bmc: Rename the local variables

[Lee Jones]

On Mon, 09 Jan 2023, Ilpo Järvinen wrote:

> On Mon, 9 Jan 2023, Lee Jones wrote:
> 
> > On Mon, 26 Dec 2022, Ilpo Järvinen wrote:
> > 
> > > Local variables directly interact with dev_get_drvdata/dev_set_drvdata
> > > should be named ddata.
> > > 
> > > Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> > 
> > It took 2 people to rename some variables? :)
> 
> It took one person to rename the variables, and other to prepare it into a 
> series which required some changes to the original patch (which is when 
> I added my SoB). But I can remove Tianfei (Andy mentioned earlier for 
> simple changes it's okay).

It's not a blocker.  I just found it humorous. :)

> > > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> > > Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> > > Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.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 12c522c16d83..2c26203c4799 100644
> > > --- a/drivers/mfd/intel-m10-bmc.c
> > > +++ b/drivers/mfd/intel-m10-bmc.c
> > > @@ -81,15 +81,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);
> > 
> > In general I'm all for the use of 'ddata' for driver data.
> > 
> > For my own reference (apply this as-is to your sign-off block):
> >                                                        
> > Acked-for-MFD-by: Lee Jones <lee@kernel.org>


-- 
Lee Jones [李琼斯]

Re: [PATCH v5 06/10] mfd: intel-m10-bmc: Downscope SPI defines & prefix with M10BMC_N3000

[Lee Jones]

On Mon, 26 Dec 2022, Ilpo Järvinen wrote:

> Move SPI based board definitions to per interface file from the global
> header. This makes it harder to use them accidently in the
> generic/interface agnostic code. Prefix the defines with M10BMC_N3000
> to make it more obvious these are related to some board type. All
> current non-N3000 board types have the same layout so they'll be
> reused.
> 
> Some bitfield defs are also moved to intel-m10-bmc-core which seems
> more appropriate for them.
> 
> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/mfd/intel-m10-bmc-core.c  | 11 ++++
>  drivers/mfd/intel-m10-bmc-spi.c   | 89 ++++++++++++++++++++++---------
>  include/linux/mfd/intel-m10-bmc.h | 46 ----------------
>  3 files changed, 74 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/mfd/intel-m10-bmc-core.c b/drivers/mfd/intel-m10-bmc-core.c
> index 51b78b868235..50a4ec758bdb 100644
> --- a/drivers/mfd/intel-m10-bmc-core.c
> +++ b/drivers/mfd/intel-m10-bmc-core.c
> @@ -12,6 +12,17 @@
>  #include <linux/mfd/intel-m10-bmc.h>
>  #include <linux/module.h>
>  
> +/* Register fields of system registers */
> +#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
> +#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
> +#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
> +#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
> +#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
> +#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
> +#define M10BMC_MAC_COUNT		GENMASK(23, 16)
> +#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
> +#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
> +
>  static ssize_t bmc_version_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
> index e8986154e965..04c83f9c6492 100644
> --- a/drivers/mfd/intel-m10-bmc-spi.c
> +++ b/drivers/mfd/intel-m10-bmc-spi.c
> @@ -13,10 +13,47 @@
>  #include <linux/regmap.h>
>  #include <linux/spi/spi.h>
>  
> +#define M10BMC_N3000_LEGACY_BUILD_VER	0x300468
> +#define M10BMC_N3000_SYS_BASE		0x300800
> +#define M10BMC_N3000_SYS_END		0x300fff
> +#define M10BMC_N3000_FLASH_BASE		0x10000000
> +#define M10BMC_N3000_FLASH_END		0x1fffffff
> +#define M10BMC_N3000_MEM_END		M10BMC_N3000_FLASH_END
> +
> +/* Register offset of system registers */
> +#define NIOS2_FW_VERSION		0x0
> +#define M10BMC_N3000_MAC_LOW		0x10
> +#define M10BMC_N3000_MAC_HIGH		0x14
> +#define M10BMC_N3000_TEST_REG		0x3c
> +#define M10BMC_N3000_BUILD_VER		0x68
> +#define M10BMC_N3000_VER_LEGACY_INVALID	0xffffffff
> +
> +/* Secure update doorbell register, in system register region */
> +#define M10BMC_N3000_DOORBELL		0x400
> +
> +/* Authorization Result register, in system register region */
> +#define M10BMC_N3000_AUTH_RESULT		0x404
> +
> +/* Addresses for security related data in FLASH */
> +#define M10BMC_N3000_BMC_REH_ADDR	0x17ffc004
> +#define M10BMC_N3000_BMC_PROG_ADDR	0x17ffc000
> +#define M10BMC_N3000_BMC_PROG_MAGIC	0x5746
> +
> +#define M10BMC_N3000_SR_REH_ADDR	0x17ffd004
> +#define M10BMC_N3000_SR_PROG_ADDR	0x17ffd000
> +#define M10BMC_N3000_SR_PROG_MAGIC	0x5253
> +
> +#define M10BMC_N3000_PR_REH_ADDR	0x17ffe004
> +#define M10BMC_N3000_PR_PROG_ADDR	0x17ffe000
> +#define M10BMC_N3000_PR_PROG_MAGIC	0x5250

My preference would definitely be to keep these blocks of defines tucked
away inside a header file somewhere.

Premise of the change looks fine, however.

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 09/10] mfd: intel-m10-bmc: Add PMCI driver

[Lee Jones]

On Mon, 26 Dec 2022, Ilpo Järvinen wrote:

> Add the mfd driver for the Platform Management Component Interface
> (PMCI) based interface of Intel MAX10 BMC controller.
> 
> PMCI is a software-visible interface, connected to card BMC which
> provided the basic functionality of read/write BMC register. The access
> to the register is done indirectly via a hardware controller/bridge
> that handles read/write/clear commands and acknowledgments for the
> commands.
> 
> Previously, intel-m10-bmc provided sysfs under
> /sys/bus/spi/devices/... which is generalized in this change because
> not all MAX10 BMC appear under SPI anymore.
> 
> Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Co-developed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
>  drivers/mfd/Kconfig                           |  12 +
>  drivers/mfd/Makefile                          |   1 +
>  drivers/mfd/intel-m10-bmc-pmci.c              | 253 ++++++++++++++++++
>  4 files changed, 270 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/mfd/intel-m10-bmc-pmci.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> index 9773925138af..a8ab58035c95 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc
> @@ -1,4 +1,4 @@
> -What:		/sys/bus/spi/devices/.../bmc_version
> +What:		/sys/bus/.../drivers/intel-m10-bmc/.../bmc_version
>  Date:		June 2020
>  KernelVersion:	5.10
>  Contact:	Xu Yilun <yilun.xu@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 a09d4ac60dc7..82f13614d98a 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2238,6 +2238,18 @@ config MFD_INTEL_M10_BMC_SPI
>            additional drivers must be enabled in order to use the functionality
>            of the device.
>  
> +config MFD_INTEL_M10_BMC_PMCI
> +	tristate "Intel MAX 10 Board Management Controller with PMCI"
> +	depends on FPGA_DFL
> +	select MFD_INTEL_M10_BMC_CORE
> +	select REGMAP
> +	help
> +	  Support for the Intel MAX 10 board management controller via PMCI.
> +
> +	  This driver provides common support for accessing the device,
> +	  additional drivers must be enabled in order to use the functionality
> +	  of the device.
> +
>  config MFD_RSMU_I2C
>  	tristate "Renesas Synchronization Management Unit with I2C"
>  	depends on I2C && OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5d1f308ee2a7..c90fb96cad2a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -274,6 +274,7 @@ obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>  
>  obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)	+= intel-m10-bmc-core.o
>  obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)	+= intel-m10-bmc-spi.o
> +obj-$(CONFIG_MFD_INTEL_M10_BMC_PMCI)	+= intel-m10-bmc-pmci.o
>  
>  obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
>  obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
> diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
> new file mode 100644
> index 000000000000..63ec0f6aba8b
> --- /dev/null
> +++ b/drivers/mfd/intel-m10-bmc-pmci.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MFD driver for Platform Management Component Interface (PMCI) based

Please remove any mention of MFD.

This includes the MODULE_*() macros in all patches please.

Better to use terms like 'core' instead.

> + * interface to MAX10 BMC.
> + *
> + * Copyright (C) 2020-2022 Intel Corporation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/dfl.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel-m10-bmc.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define M10BMC_PMCI_INDIRECT_BASE	0x400
> +
> +#define M10BMC_N6000_SYS_BASE		0x0
> +#define M10BMC_N6000_SYS_END		0xfff
> +
> +#define M10BMC_N6000_DOORBELL		0x1c0
> +#define M10BMC_N6000_AUTH_RESULT	0x1c4
> +
> +/* Telemetry registers */
> +#define M10BMC_N6000_TELEM_START	0x400
> +#define M10BMC_N6000_TELEM_END		0x78c
> +
> +#define M10BMC_N6000_BUILD_VER		0x0
> +#define NIOS2_N6000_FW_VERSION		0x4
> +#define M10BMC_N6000_MAC_LOW		0x20
> +#define M10BMC_N6000_MAC_HIGH		(M10BMC_N6000_MAC_LOW + 4)
> +
> +/* Addresses for security related data in FLASH */
> +#define M10BMC_N6000_BMC_REH_ADDR	0x7ffc004
> +#define M10BMC_N6000_BMC_PROG_ADDR	0x7ffc000
> +#define M10BMC_N6000_BMC_PROG_MAGIC	0x5746
> +
> +#define M10BMC_N6000_SR_REH_ADDR	0x7ffd004
> +#define M10BMC_N6000_SR_PROG_ADDR	0x7ffd000
> +#define M10BMC_N6000_SR_PROG_MAGIC	0x5253
> +
> +#define M10BMC_N6000_PR_REH_ADDR	0x7ffe004
> +#define M10BMC_N6000_PR_PROG_ADDR	0x7ffe000
> +#define M10BMC_N6000_PR_PROG_MAGIC	0x5250
> +
> +#define M10BMC_N6000_STAGING_FLASH_COUNT	0x7ff5000
> +
> +struct m10bmc_pmci_device {
> +	void __iomem *base;
> +	struct intel_m10bmc m10bmc;
> +};
> +
> +/*
> + * Intel FGPA indirect register access via hardware controller/bridge.
> + */
> +#define INDIRECT_CMD_OFF	0
> +#define INDIRECT_CMD_CLR	0
> +#define INDIRECT_CMD_RD		BIT(0)
> +#define INDIRECT_CMD_WR		BIT(1)
> +#define INDIRECT_CMD_ACK	BIT(2)
> +
> +#define INDIRECT_ADDR_OFF	0x4
> +#define INDIRECT_RD_OFF		0x8
> +#define INDIRECT_WR_OFF		0xc
> +
> +#define INDIRECT_INT_US		1
> +#define INDIRECT_TIMEOUT_US	10000
> +
> +struct indirect_ctx {
> +        void __iomem *base;
> +        struct device *dev;
> +};
> +
> +static int indirect_clear_cmd(struct indirect_ctx *ctx)
> +{
> +	unsigned int cmd;
> +	int ret;
> +
> +	writel(INDIRECT_CMD_CLR, ctx->base + INDIRECT_CMD_OFF);
> +
> +	ret = readl_poll_timeout(ctx->base + INDIRECT_CMD_OFF, cmd,
> +				 cmd == INDIRECT_CMD_CLR,
> +				 INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
> +	if (ret)
> +		dev_err(ctx->dev, "timed out waiting clear cmd (residual cmd=0x%x)\n", cmd);
> +
> +	return ret;
> +}
> +
> +static int indirect_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct indirect_ctx *ctx = context;
> +	unsigned int cmd, ack, tmpval;
> +	int ret;
> +
> +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> +	if (cmd != INDIRECT_CMD_CLR)
> +		dev_warn(ctx->dev, "residual cmd 0x%x on read entry\n", cmd);
> +
> +	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
> +	writel(INDIRECT_CMD_RD, ctx->base + INDIRECT_CMD_OFF);
> +
> +	ret = readl_poll_timeout(ctx->base + INDIRECT_CMD_OFF, ack,
> +				 (ack & INDIRECT_CMD_ACK) == INDIRECT_CMD_ACK,
> +				 INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
> +	if (ret)
> +		dev_err(ctx->dev, "read timed out on reg 0x%x ack 0x%x\n", reg, ack);
> +	else
> +		tmpval = readl(ctx->base + INDIRECT_RD_OFF);
> +
> +	if (indirect_clear_cmd(ctx)) 

What are the chances of the above readl_poll_timeout() failing, then the
subsequent one in indirect_clear_cmd() passing?

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

> +		if (!ret)
> +			ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*val = tmpval;

If readl_poll_timeout() fails and indirect_clear_cmd() succeeds, you'll
return, at best, junk.

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

Why do we need to cache dev twice in two different structures? 

> +	indirect_clear_cmd(ctx);
> +	pmci->m10bmc.regmap = devm_regmap_init(dev, NULL, ctx, &m10bmc_pmci_regmap_config);
> +
> +	if (IS_ERR(pmci->m10bmc.regmap))
> +		return PTR_ERR(pmci->m10bmc.regmap);
> +
> +	return m10bmc_dev_init(&pmci->m10bmc, &m10bmc_pmci_n6000);
> +}
> +
> +#define FME_FEATURE_ID_M10BMC_PMCI	0x12
> +
> +static const struct dfl_device_id m10bmc_pmci_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_M10BMC_PMCI },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dfl, m10bmc_pmci_ids);
> +
> +static struct dfl_driver m10bmc_pmci_driver = {
> +	.drv	= {
> +		.name       = "intel-m10-bmc",
> +		.dev_groups = m10bmc_dev_groups,
> +	},
> +	.id_table = m10bmc_pmci_ids,
> +	.probe    = m10bmc_pmci_probe,
b> +};
> +
> +module_dfl_driver(m10bmc_pmci_driver);
> +
> +MODULE_DESCRIPTION("MAX10 BMC PMCI-based interface");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");
> -- 
> 2.30.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 03/10] mfd: intel-m10-bmc: Split into core and spi specific parts

[Lee Jones]

On Mon, 26 Dec 2022, Ilpo Järvinen wrote:

> Split the common code from intel-m10-bmc driver into intel-m10-bmc-core
> and move the SPI bus parts into an interface specific file.
> 
> intel-m10-bmc-core becomes the core MFD functions which can support
> multiple bus interface like SPI bus.
> 
> Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> Acked-by: Guenter Roeck <linux@roeck-us.net> # hwmon
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  MAINTAINERS                                   |   2 +-
>  drivers/fpga/Kconfig                          |   2 +-
>  drivers/hwmon/Kconfig                         |   2 +-
>  drivers/mfd/Kconfig                           |  30 ++--
>  drivers/mfd/Makefile                          |   4 +-
>  drivers/mfd/intel-m10-bmc-core.c              | 122 +++++++++++++++++
>  .../{intel-m10-bmc.c => intel-m10-bmc-spi.c}  | 128 +++---------------
>  include/linux/mfd/intel-m10-bmc.h             |   6 +
>  8 files changed, 172 insertions(+), 124 deletions(-)
>  create mode 100644 drivers/mfd/intel-m10-bmc-core.c
>  rename drivers/mfd/{intel-m10-bmc.c => intel-m10-bmc-spi.c} (59%)

Looks okay:

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

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

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 04/10] mfd: intel-m10-bmc: Support multiple CSR register layouts

[Lee Jones]

On Mon, 26 Dec 2022, Ilpo Järvinen wrote:

> There are different addresses for the MAX10 CSR registers. Introducing
> a new data structure m10bmc_csr_map for the register definition of
> MAX10 CSR.
> 
> Provide the csr_map for SPI.
> 
> Co-developed-by: Tianfei zhang <tianfei.zhang@intel.com>
> Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com>
> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/fpga/intel-m10-bmc-sec-update.c | 73 +++++++++++++++++--------
>  drivers/mfd/intel-m10-bmc-core.c        | 10 ++--
>  drivers/mfd/intel-m10-bmc-spi.c         | 24 ++++++++
>  include/linux/mfd/intel-m10-bmc.h       | 39 +++++++++++--
>  4 files changed, 113 insertions(+), 33 deletions(-)

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

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

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 09/10] mfd: intel-m10-bmc: Add PMCI driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 13660 bytes --]

On Fri, 13 Jan 2023, Lee Jones wrote:

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

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

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

I can add the second variable for that.

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

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

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

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

Do you prefer I use m10bmc_pmci_device instead?

-- 
 i.