[PATCH 0/3] fpga: dfl: add support for Intel D5005 card

[PATCH 0/3] fpga: dfl: add support for Intel D5005 card

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

This patch set adds basic support for the Intel D5005 card to the
dfl driver framework.

Patch 1 adds the PCIe device id of the D5005 to the dfl-pci id table.

Patch 2 adds a dfl driver for the Altera SPI Master which is connected
to the board's BMC.

Patch 3 adds hwmon support for the sensors monitored by the board's BMC.

Matthew Gerlach (2):
  fpga: dfl: Add DFL bus driver for Altera SPI Master
  hwmon: intel-m10-bmc-hwmon: add sensor support of Intel D5005 card

Russ Weight (1):
  fpga: dfl: pci: add DID for D5005 PAC cards

 drivers/fpga/Kconfig                |   9 ++
 drivers/fpga/Makefile               |   1 +
 drivers/fpga/dfl-pci.c              |  18 +--
 drivers/fpga/dfl-spi-altera.c       | 221 ++++++++++++++++++++++++++++++++++++
 drivers/hwmon/intel-m10-bmc-hwmon.c | 122 ++++++++++++++++++++
 drivers/mfd/intel-m10-bmc.c         |  10 ++
 6 files changed, 374 insertions(+), 7 deletions(-)
 create mode 100644 drivers/fpga/dfl-spi-altera.c

-- 
1.8.3.1

[PATCH 1/3] fpga: dfl: pci: add DID for D5005 PAC cards

[matthew.gerlach]

From: Russ Weight <russell.h.weight@intel.com>

This patch adds the approved PCI Express Device IDs for the
PF and VF for the card for D5005 PAC cards.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/fpga/dfl-pci.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 04e47e2..b44523e 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -69,14 +69,16 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
 }
 
 /* PCI Device ID */
-#define PCIE_DEVICE_ID_PF_INT_5_X	0xBCBD
-#define PCIE_DEVICE_ID_PF_INT_6_X	0xBCC0
-#define PCIE_DEVICE_ID_PF_DSC_1_X	0x09C4
-#define PCIE_DEVICE_ID_INTEL_PAC_N3000	0x0B30
+#define PCIE_DEVICE_ID_PF_INT_5_X		0xBCBD
+#define PCIE_DEVICE_ID_PF_INT_6_X		0xBCC0
+#define PCIE_DEVICE_ID_PF_DSC_1_X		0x09C4
+#define PCIE_DEVICE_ID_INTEL_PAC_N3000		0x0B30
+#define PCIE_DEVICE_ID_INTEL_PAC_D5005		0x0B2B
 /* VF Device */
-#define PCIE_DEVICE_ID_VF_INT_5_X	0xBCBF
-#define PCIE_DEVICE_ID_VF_INT_6_X	0xBCC1
-#define PCIE_DEVICE_ID_VF_DSC_1_X	0x09C5
+#define PCIE_DEVICE_ID_VF_INT_5_X		0xBCBF
+#define PCIE_DEVICE_ID_VF_INT_6_X		0xBCC1
+#define PCIE_DEVICE_ID_VF_DSC_1_X		0x09C5
+#define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF	0x0B2C
 
 static struct pci_device_id cci_pcie_id_tbl[] = {
 	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
@@ -86,6 +88,8 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
 	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_DSC_1_X),},
 	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_VF_DSC_1_X),},
 	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_N3000),},
+	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_D5005),},
+	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
 	{0,}
 };
 MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
-- 
1.8.3.1

Re: [PATCH 1/3] fpga: dfl: pci: add DID for D5005 PAC cards

[Moritz Fischer]

On Mon, Apr 05, 2021 at 04:52:59PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> This patch adds the approved PCI Express Device IDs for the
> PF and VF for the card for D5005 PAC cards.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/fpga/dfl-pci.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index 04e47e2..b44523e 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -69,14 +69,16 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
>  }
>  
>  /* PCI Device ID */
> -#define PCIE_DEVICE_ID_PF_INT_5_X	0xBCBD
> -#define PCIE_DEVICE_ID_PF_INT_6_X	0xBCC0
> -#define PCIE_DEVICE_ID_PF_DSC_1_X	0x09C4
> -#define PCIE_DEVICE_ID_INTEL_PAC_N3000	0x0B30
> +#define PCIE_DEVICE_ID_PF_INT_5_X		0xBCBD
> +#define PCIE_DEVICE_ID_PF_INT_6_X		0xBCC0
> +#define PCIE_DEVICE_ID_PF_DSC_1_X		0x09C4
> +#define PCIE_DEVICE_ID_INTEL_PAC_N3000		0x0B30
> +#define PCIE_DEVICE_ID_INTEL_PAC_D5005		0x0B2B
>  /* VF Device */
> -#define PCIE_DEVICE_ID_VF_INT_5_X	0xBCBF
> -#define PCIE_DEVICE_ID_VF_INT_6_X	0xBCC1
> -#define PCIE_DEVICE_ID_VF_DSC_1_X	0x09C5
> +#define PCIE_DEVICE_ID_VF_INT_5_X		0xBCBF
> +#define PCIE_DEVICE_ID_VF_INT_6_X		0xBCC1
> +#define PCIE_DEVICE_ID_VF_DSC_1_X		0x09C5
> +#define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF	0x0B2C
>  
>  static struct pci_device_id cci_pcie_id_tbl[] = {
>  	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
> @@ -86,6 +88,8 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
>  	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_DSC_1_X),},
>  	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_VF_DSC_1_X),},
>  	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_N3000),},
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_D5005),},
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
>  	{0,}
>  };
>  MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
> -- 
> 1.8.3.1
> 

Applied to for-next,

Thanks

Re: [PATCH 1/3] fpga: dfl: pci: add DID for D5005 PAC cards

[matthew.gerlach]

On Mon, 5 Apr 2021, Moritz Fischer wrote:

> On Mon, Apr 05, 2021 at 04:52:59PM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Russ Weight <russell.h.weight@intel.com>
>>
>> This patch adds the approved PCI Express Device IDs for the
>> PF and VF for the card for D5005 PAC cards.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/fpga/dfl-pci.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index 04e47e2..b44523e 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -69,14 +69,16 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
>>  }
>>
>>  /* PCI Device ID */
>> -#define PCIE_DEVICE_ID_PF_INT_5_X	0xBCBD
>> -#define PCIE_DEVICE_ID_PF_INT_6_X	0xBCC0
>> -#define PCIE_DEVICE_ID_PF_DSC_1_X	0x09C4
>> -#define PCIE_DEVICE_ID_INTEL_PAC_N3000	0x0B30
>> +#define PCIE_DEVICE_ID_PF_INT_5_X		0xBCBD
>> +#define PCIE_DEVICE_ID_PF_INT_6_X		0xBCC0
>> +#define PCIE_DEVICE_ID_PF_DSC_1_X		0x09C4
>> +#define PCIE_DEVICE_ID_INTEL_PAC_N3000		0x0B30
>> +#define PCIE_DEVICE_ID_INTEL_PAC_D5005		0x0B2B
>>  /* VF Device */
>> -#define PCIE_DEVICE_ID_VF_INT_5_X	0xBCBF
>> -#define PCIE_DEVICE_ID_VF_INT_6_X	0xBCC1
>> -#define PCIE_DEVICE_ID_VF_DSC_1_X	0x09C5
>> +#define PCIE_DEVICE_ID_VF_INT_5_X		0xBCBF
>> +#define PCIE_DEVICE_ID_VF_INT_6_X		0xBCC1
>> +#define PCIE_DEVICE_ID_VF_DSC_1_X		0x09C5
>> +#define PCIE_DEVICE_ID_INTEL_PAC_D5005_VF	0x0B2C
>>
>>  static struct pci_device_id cci_pcie_id_tbl[] = {
>>  	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_INT_5_X),},
>> @@ -86,6 +88,8 @@ static void cci_pci_free_irq(struct pci_dev *pcidev)
>>  	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_PF_DSC_1_X),},
>>  	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_VF_DSC_1_X),},
>>  	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_N3000),},
>> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_D5005),},
>> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCIE_DEVICE_ID_INTEL_PAC_D5005_VF),},
>>  	{0,}
>>  };
>>  MODULE_DEVICE_TABLE(pci, cci_pcie_id_tbl);
>> --
>> 1.8.3.1
>>
>
> Applied to for-next,
>
> Thanks
>

Thank you for the review.

Matthew

[PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

This patch adds DFL bus driver for the Altera SPI Master
controller.  The SPI master is connected to an Intel SPI Slave to
Avalon Master Bridge, inside an Intel MAX10 BMC Chip.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/fpga/Kconfig          |   9 ++
 drivers/fpga/Makefile         |   1 +
 drivers/fpga/dfl-spi-altera.c | 221 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 231 insertions(+)
 create mode 100644 drivers/fpga/dfl-spi-altera.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index d591dd9..0a86994 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
 	  the card. It also instantiates the SPI master (spi-altera) for
 	  the card's BMC (Board Management Controller).
 
+config FPGA_DFL_SPI_ALTERA
+	tristate "FPGA DFL Altera SPI Master Driver"
+	depends on FPGA_DFL
+	select REGMAP
+	help
+	  This is a DFL bus driver for the Altera SPI master controller.
+	  The SPI master is connected to a SPI slave to Avalon Master
+	  bridge in a Intel MAX BMC.
+
 config FPGA_DFL_PCI
 	tristate "FPGA DFL PCIe Device Driver"
 	depends on PCI && FPGA_DFL
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 18dc9885..58a42ad 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
 dfl-afu-objs += dfl-afu-error.o
 
 obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
+obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)	+= dfl-spi-altera.o
 
 # Drivers for FPGAs which implement DFL
 obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
new file mode 100644
index 0000000..9bec25fd
--- /dev/null
+++ b/drivers/fpga/dfl-spi-altera.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DFL bus driver for Altera SPI Master
+ *
+ * Copyright (C) 2020 Intel Corporation, Inc.
+ *
+ * Authors:
+ *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
+ */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/stddef.h>
+#include <linux/errno.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/bitfield.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/altera.h>
+#include <linux/dfl.h>
+
+struct dfl_altera_spi {
+	void __iomem *base;
+	struct regmap *regmap;
+	struct device *dev;
+	struct platform_device *altr_spi;
+};
+
+#define SPI_CORE_PARAMETER      0x8
+#define SHIFT_MODE              BIT_ULL(1)
+#define SHIFT_MODE_MSB          0
+#define SHIFT_MODE_LSB          1
+#define DATA_WIDTH              GENMASK_ULL(7, 2)
+#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
+#define CLK_POLARITY            BIT_ULL(14)
+#define CLK_PHASE               BIT_ULL(15)
+#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
+#define SPI_CLK                 GENMASK_ULL(31, 22)
+#define SPI_INDIRECT_ACC_OFST   0x10
+
+#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
+#define INDIRECT_WR             BIT_ULL(8)
+#define INDIRECT_RD             BIT_ULL(9)
+#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
+#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
+#define INDIRECT_DEBUG          BIT_ULL(32)
+#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
+#define INDIRECT_TIMEOUT        10000
+
+static int indirect_bus_reg_read(void *context, unsigned int reg,
+				 unsigned int *val)
+{
+	struct dfl_altera_spi *aspi = context;
+	void __iomem *base = aspi->base;
+	int loops;
+	u64 v;
+
+	writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
+
+	loops = 0;
+	while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
+	       (loops++ < INDIRECT_TIMEOUT))
+		cpu_relax();
+
+	if (loops >= INDIRECT_TIMEOUT) {
+		pr_err("%s timed out %d\n", __func__, loops);
+		return -ETIME;
+	}
+
+	v = readq(base + INDIRECT_RD_DATA);
+
+	*val = v & INDIRECT_DATA_MASK;
+
+	return 0;
+}
+
+static int indirect_bus_reg_write(void *context, unsigned int reg,
+				  unsigned int val)
+{
+	struct dfl_altera_spi *aspi = context;
+	void __iomem *base = aspi->base;
+	int loops;
+
+	writeq(val, base + INDIRECT_WR_DATA);
+	writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
+
+	loops = 0;
+	while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
+	       (loops++ < INDIRECT_TIMEOUT))
+		cpu_relax();
+
+	if (loops >= INDIRECT_TIMEOUT) {
+		pr_err("%s timed out %d\n", __func__, loops);
+		return -ETIME;
+	}
+	return 0;
+}
+
+static const struct regmap_config indirect_regbus_cfg = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+
+	.reg_write = indirect_bus_reg_write,
+	.reg_read = indirect_bus_reg_read,
+};
+
+static struct spi_board_info m10_bmc_info = {
+	.modalias = "m10-d5005",
+	.max_speed_hz = 12500000,
+	.bus_num = 0,
+	.chip_select = 0,
+};
+
+static struct platform_device *create_cntrl(struct device *dev,
+					    void __iomem *base,
+					    struct spi_board_info *m10_info)
+{
+	struct altera_spi_platform_data pdata;
+	struct platform_device_info pdevinfo;
+	u64 v;
+
+	v = readq(base + SPI_CORE_PARAMETER);
+
+	memset(&pdata, 0, sizeof(pdata));
+	pdata.mode_bits = SPI_CS_HIGH;
+	if (FIELD_GET(CLK_POLARITY, v))
+		pdata.mode_bits |= SPI_CPOL;
+	if (FIELD_GET(CLK_PHASE, v))
+		pdata.mode_bits |= SPI_CPHA;
+
+	pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
+	pdata.bits_per_word_mask =
+		SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
+
+	pdata.num_devices = 1;
+	pdata.devices = m10_info;
+
+	dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
+		pdata.num_chipselect, pdata.bits_per_word_mask,
+		pdata.mode_bits);
+
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+
+	pdevinfo.name = "subdev_spi_altera";
+	pdevinfo.id = PLATFORM_DEVID_AUTO;
+	pdevinfo.parent = dev;
+	pdevinfo.data = &pdata;
+	pdevinfo.size_data = sizeof(pdata);
+
+	return platform_device_register_full(&pdevinfo);
+}
+static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
+{
+	struct device *dev = &dfl_dev->dev;
+	struct dfl_altera_spi *aspi;
+
+	aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
+
+	if (!aspi)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, aspi);
+
+	aspi->dev = dev;
+
+	aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
+
+	if (IS_ERR(aspi->base)) {
+		dev_err(dev, "%s get mem resource fail!\n", __func__);
+		return PTR_ERR(aspi->base);
+	}
+
+	aspi->regmap = devm_regmap_init(dev, NULL, aspi, &indirect_regbus_cfg);
+	if (IS_ERR(aspi->regmap))
+		return PTR_ERR(aspi->regmap);
+
+	aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
+
+	if (IS_ERR(aspi->altr_spi)) {
+		dev_err(dev, "%s failed to create spi platform driver\n",
+			__func__);
+		return PTR_ERR(aspi->base);
+	}
+
+	return 0;
+}
+
+static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
+{
+	struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
+
+	platform_device_unregister(aspi->altr_spi);
+}
+
+#define FME_FEATURE_ID_MAX10_SPI        0xe
+
+static const struct dfl_device_id dfl_spi_altera_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
+	{ }
+};
+
+static struct dfl_driver dfl_spi_altera_driver = {
+	.drv	= {
+		.name       = "dfl-spi-altera",
+	},
+	.id_table = dfl_spi_altera_ids,
+	.probe   = dfl_spi_altera_probe,
+	.remove  = dfl_spi_altera_remove,
+};
+
+module_dfl_driver(dfl_spi_altera_driver);
+
+MODULE_DEVICE_TABLE(dfl, dfl_spi_altera_ids);
+MODULE_DESCRIPTION("DFL spi altera driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
-- 
1.8.3.1

Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Moritz Fischer]

Hi Matthew,

On Mon, Apr 05, 2021 at 04:53:00PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> This patch adds DFL bus driver for the Altera SPI Master
> controller.  The SPI master is connected to an Intel SPI Slave to
> Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/fpga/Kconfig          |   9 ++
>  drivers/fpga/Makefile         |   1 +
>  drivers/fpga/dfl-spi-altera.c | 221 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 231 insertions(+)
>  create mode 100644 drivers/fpga/dfl-spi-altera.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d591dd9..0a86994 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
>  	  the card. It also instantiates the SPI master (spi-altera) for
>  	  the card's BMC (Board Management Controller).
>  
> +config FPGA_DFL_SPI_ALTERA
> +	tristate "FPGA DFL Altera SPI Master Driver"
> +	depends on FPGA_DFL
> +	select REGMAP
> +	help
> +	  This is a DFL bus driver for the Altera SPI master controller.
> +	  The SPI master is connected to a SPI slave to Avalon Master
> +	  bridge in a Intel MAX BMC.
> +
>  config FPGA_DFL_PCI
>  	tristate "FPGA DFL PCIe Device Driver"
>  	depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 18dc9885..58a42ad 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
>  dfl-afu-objs += dfl-afu-error.o
>  
>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)	+= dfl-spi-altera.o
>  
>  # Drivers for FPGAs which implement DFL
>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
> new file mode 100644
> index 0000000..9bec25fd
> --- /dev/null
> +++ b/drivers/fpga/dfl-spi-altera.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DFL bus driver for Altera SPI Master
> + *
> + * Copyright (C) 2020 Intel Corporation, Inc.
> + *
> + * Authors:
> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/stddef.h>
> +#include <linux/errno.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/bitfield.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/altera.h>
> +#include <linux/dfl.h>
> +
> +struct dfl_altera_spi {
> +	void __iomem *base;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct platform_device *altr_spi;
> +};
> +
> +#define SPI_CORE_PARAMETER      0x8
> +#define SHIFT_MODE              BIT_ULL(1)
> +#define SHIFT_MODE_MSB          0
> +#define SHIFT_MODE_LSB          1
> +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> +#define CLK_POLARITY            BIT_ULL(14)
> +#define CLK_PHASE               BIT_ULL(15)
> +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> +#define SPI_CLK                 GENMASK_ULL(31, 22)
> +#define SPI_INDIRECT_ACC_OFST   0x10
> +
> +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> +#define INDIRECT_WR             BIT_ULL(8)
> +#define INDIRECT_RD             BIT_ULL(9)
> +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> +#define INDIRECT_DEBUG          BIT_ULL(32)
> +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> +#define INDIRECT_TIMEOUT        10000
> +
> +static int indirect_bus_reg_read(void *context, unsigned int reg,
> +				 unsigned int *val)
> +{
> +	struct dfl_altera_spi *aspi = context;
> +	void __iomem *base = aspi->base;
> +	int loops;
> +	u64 v;
> +
> +	writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> +
> +	loops = 0;
> +	while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> +	       (loops++ < INDIRECT_TIMEOUT))
> +		cpu_relax();
> +
> +	if (loops >= INDIRECT_TIMEOUT) {
> +		pr_err("%s timed out %d\n", __func__, loops);
> +		return -ETIME;
> +	}
> +
> +	v = readq(base + INDIRECT_RD_DATA);
> +
> +	*val = v & INDIRECT_DATA_MASK;
> +
> +	return 0;
> +}
> +
> +static int indirect_bus_reg_write(void *context, unsigned int reg,
> +				  unsigned int val)
> +{
> +	struct dfl_altera_spi *aspi = context;
> +	void __iomem *base = aspi->base;
> +	int loops;
> +
> +	writeq(val, base + INDIRECT_WR_DATA);
> +	writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> +
> +	loops = 0;
> +	while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> +	       (loops++ < INDIRECT_TIMEOUT))
> +		cpu_relax();
> +
> +	if (loops >= INDIRECT_TIMEOUT) {
> +		pr_err("%s timed out %d\n", __func__, loops);
> +		return -ETIME;
> +	}
> +	return 0;
> +}
> +
> +static const struct regmap_config indirect_regbus_cfg = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.fast_io = true,
> +
> +	.reg_write = indirect_bus_reg_write,
> +	.reg_read = indirect_bus_reg_read,
> +};
> +
> +static struct spi_board_info m10_bmc_info = {
> +	.modalias = "m10-d5005",
> +	.max_speed_hz = 12500000,
> +	.bus_num = 0,
> +	.chip_select = 0,
> +};
> +
> +static struct platform_device *create_cntrl(struct device *dev,
> +					    void __iomem *base,
> +					    struct spi_board_info *m10_info)
> +{
> +	struct altera_spi_platform_data pdata;
> +	struct platform_device_info pdevinfo;
> +	u64 v;
> +
> +	v = readq(base + SPI_CORE_PARAMETER);
> +
> +	memset(&pdata, 0, sizeof(pdata));
> +	pdata.mode_bits = SPI_CS_HIGH;
> +	if (FIELD_GET(CLK_POLARITY, v))
> +		pdata.mode_bits |= SPI_CPOL;
> +	if (FIELD_GET(CLK_PHASE, v))
> +		pdata.mode_bits |= SPI_CPHA;
> +
> +	pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> +	pdata.bits_per_word_mask =
> +		SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> +
> +	pdata.num_devices = 1;
> +	pdata.devices = m10_info;
> +
> +	dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> +		pdata.num_chipselect, pdata.bits_per_word_mask,
> +		pdata.mode_bits);
> +
> +	memset(&pdevinfo, 0, sizeof(pdevinfo));
> +
> +	pdevinfo.name = "subdev_spi_altera";
> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> +	pdevinfo.parent = dev;
> +	pdevinfo.data = &pdata;
> +	pdevinfo.size_data = sizeof(pdata);
> +
> +	return platform_device_register_full(&pdevinfo);

Should this be a SPI driver? I think looking at the UIO case we had
decided against this pattern?

> +}
> +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	struct dfl_altera_spi *aspi;
> +
> +	aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> +
> +	if (!aspi)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, aspi);
> +
> +	aspi->dev = dev;
> +
> +	aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> +
> +	if (IS_ERR(aspi->base)) {
> +		dev_err(dev, "%s get mem resource fail!\n", __func__);
> +		return PTR_ERR(aspi->base);
> +	}
> +
> +	aspi->regmap = devm_regmap_init(dev, NULL, aspi, &indirect_regbus_cfg);
> +	if (IS_ERR(aspi->regmap))
> +		return PTR_ERR(aspi->regmap);
> +
> +	aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> +
> +	if (IS_ERR(aspi->altr_spi)) {
> +		dev_err(dev, "%s failed to create spi platform driver\n",
> +			__func__);
> +		return PTR_ERR(aspi->base);
> +	}
> +
> +	return 0;
> +}
> +
> +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> +{
> +	struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> +
> +	platform_device_unregister(aspi->altr_spi);
> +}
> +
> +#define FME_FEATURE_ID_MAX10_SPI        0xe
> +
> +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> +	{ }
> +};

Maybe you can extend the Altera SPI driver with this part?
> +
> +static struct dfl_driver dfl_spi_altera_driver = {
> +	.drv	= {
> +		.name       = "dfl-spi-altera",
> +	},
> +	.id_table = dfl_spi_altera_ids,
> +	.probe   = dfl_spi_altera_probe,
> +	.remove  = dfl_spi_altera_remove,
> +};
> +
> +module_dfl_driver(dfl_spi_altera_driver);
> +
> +MODULE_DEVICE_TABLE(dfl, dfl_spi_altera_ids);
> +MODULE_DESCRIPTION("DFL spi altera driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.8.3.1
> 

- Moritz

Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[matthew.gerlach]

Hi Moritz,

On Mon, 5 Apr 2021, Moritz Fischer wrote:

> Hi Matthew,
>
> On Mon, Apr 05, 2021 at 04:53:00PM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> This patch adds DFL bus driver for the Altera SPI Master
>> controller.  The SPI master is connected to an Intel SPI Slave to
>> Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/fpga/Kconfig          |   9 ++
>>  drivers/fpga/Makefile         |   1 +
>>  drivers/fpga/dfl-spi-altera.c | 221 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 231 insertions(+)
>>  create mode 100644 drivers/fpga/dfl-spi-altera.c
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index d591dd9..0a86994 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
>>  	  the card. It also instantiates the SPI master (spi-altera) for
>>  	  the card's BMC (Board Management Controller).
>>
>> +config FPGA_DFL_SPI_ALTERA
>> +	tristate "FPGA DFL Altera SPI Master Driver"
>> +	depends on FPGA_DFL
>> +	select REGMAP
>> +	help
>> +	  This is a DFL bus driver for the Altera SPI master controller.
>> +	  The SPI master is connected to a SPI slave to Avalon Master
>> +	  bridge in a Intel MAX BMC.
>> +
>>  config FPGA_DFL_PCI
>>  	tristate "FPGA DFL PCIe Device Driver"
>>  	depends on PCI && FPGA_DFL
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 18dc9885..58a42ad 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
>>  dfl-afu-objs += dfl-afu-error.o
>>
>>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
>> +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)	+= dfl-spi-altera.o
>>
>>  # Drivers for FPGAs which implement DFL
>>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
>> diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
>> new file mode 100644
>> index 0000000..9bec25fd
>> --- /dev/null
>> +++ b/drivers/fpga/dfl-spi-altera.c
>> @@ -0,0 +1,221 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * DFL bus driver for Altera SPI Master
>> + *
>> + * Copyright (C) 2020 Intel Corporation, Inc.
>> + *
>> + * Authors:
>> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> + */
>> +
>> +#include <linux/types.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/stddef.h>
>> +#include <linux/errno.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/io-64-nonatomic-lo-hi.h>
>> +#include <linux/regmap.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/spi/altera.h>
>> +#include <linux/dfl.h>
>> +
>> +struct dfl_altera_spi {
>> +	void __iomem *base;
>> +	struct regmap *regmap;
>> +	struct device *dev;
>> +	struct platform_device *altr_spi;
>> +};
>> +
>> +#define SPI_CORE_PARAMETER      0x8
>> +#define SHIFT_MODE              BIT_ULL(1)
>> +#define SHIFT_MODE_MSB          0
>> +#define SHIFT_MODE_LSB          1
>> +#define DATA_WIDTH              GENMASK_ULL(7, 2)
>> +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
>> +#define CLK_POLARITY            BIT_ULL(14)
>> +#define CLK_PHASE               BIT_ULL(15)
>> +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
>> +#define SPI_CLK                 GENMASK_ULL(31, 22)
>> +#define SPI_INDIRECT_ACC_OFST   0x10
>> +
>> +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
>> +#define INDIRECT_WR             BIT_ULL(8)
>> +#define INDIRECT_RD             BIT_ULL(9)
>> +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
>> +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
>> +#define INDIRECT_DEBUG          BIT_ULL(32)
>> +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
>> +#define INDIRECT_TIMEOUT        10000
>> +
>> +static int indirect_bus_reg_read(void *context, unsigned int reg,
>> +				 unsigned int *val)
>> +{
>> +	struct dfl_altera_spi *aspi = context;
>> +	void __iomem *base = aspi->base;
>> +	int loops;
>> +	u64 v;
>> +
>> +	writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
>> +
>> +	loops = 0;
>> +	while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
>> +	       (loops++ < INDIRECT_TIMEOUT))
>> +		cpu_relax();
>> +
>> +	if (loops >= INDIRECT_TIMEOUT) {
>> +		pr_err("%s timed out %d\n", __func__, loops);
>> +		return -ETIME;
>> +	}
>> +
>> +	v = readq(base + INDIRECT_RD_DATA);
>> +
>> +	*val = v & INDIRECT_DATA_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +static int indirect_bus_reg_write(void *context, unsigned int reg,
>> +				  unsigned int val)
>> +{
>> +	struct dfl_altera_spi *aspi = context;
>> +	void __iomem *base = aspi->base;
>> +	int loops;
>> +
>> +	writeq(val, base + INDIRECT_WR_DATA);
>> +	writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
>> +
>> +	loops = 0;
>> +	while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
>> +	       (loops++ < INDIRECT_TIMEOUT))
>> +		cpu_relax();
>> +
>> +	if (loops >= INDIRECT_TIMEOUT) {
>> +		pr_err("%s timed out %d\n", __func__, loops);
>> +		return -ETIME;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static const struct regmap_config indirect_regbus_cfg = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 32,
>> +	.fast_io = true,
>> +
>> +	.reg_write = indirect_bus_reg_write,
>> +	.reg_read = indirect_bus_reg_read,
>> +};
>> +
>> +static struct spi_board_info m10_bmc_info = {
>> +	.modalias = "m10-d5005",
>> +	.max_speed_hz = 12500000,
>> +	.bus_num = 0,
>> +	.chip_select = 0,
>> +};
>> +
>> +static struct platform_device *create_cntrl(struct device *dev,
>> +					    void __iomem *base,
>> +					    struct spi_board_info *m10_info)
>> +{
>> +	struct altera_spi_platform_data pdata;
>> +	struct platform_device_info pdevinfo;
>> +	u64 v;
>> +
>> +	v = readq(base + SPI_CORE_PARAMETER);
>> +
>> +	memset(&pdata, 0, sizeof(pdata));
>> +	pdata.mode_bits = SPI_CS_HIGH;
>> +	if (FIELD_GET(CLK_POLARITY, v))
>> +		pdata.mode_bits |= SPI_CPOL;
>> +	if (FIELD_GET(CLK_PHASE, v))
>> +		pdata.mode_bits |= SPI_CPHA;
>> +
>> +	pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
>> +	pdata.bits_per_word_mask =
>> +		SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
>> +
>> +	pdata.num_devices = 1;
>> +	pdata.devices = m10_info;
>> +
>> +	dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
>> +		pdata.num_chipselect, pdata.bits_per_word_mask,
>> +		pdata.mode_bits);
>> +
>> +	memset(&pdevinfo, 0, sizeof(pdevinfo));
>> +
>> +	pdevinfo.name = "subdev_spi_altera";
>> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
>> +	pdevinfo.parent = dev;
>> +	pdevinfo.data = &pdata;
>> +	pdevinfo.size_data = sizeof(pdata);
>> +
>> +	return platform_device_register_full(&pdevinfo);
>
> Should this be a SPI driver? I think looking at the UIO case we had
> decided against this pattern?

This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which 
uses this design pattern.  Is it okay to keep this design pattern for 
consistency?


>
>> +}
>> +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
>> +{
>> +	struct device *dev = &dfl_dev->dev;
>> +	struct dfl_altera_spi *aspi;
>> +
>> +	aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
>> +
>> +	if (!aspi)
>> +		return -ENOMEM;
>> +
>> +	dev_set_drvdata(dev, aspi);
>> +
>> +	aspi->dev = dev;
>> +
>> +	aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
>> +
>> +	if (IS_ERR(aspi->base)) {
>> +		dev_err(dev, "%s get mem resource fail!\n", __func__);
>> +		return PTR_ERR(aspi->base);
>> +	}
>> +
>> +	aspi->regmap = devm_regmap_init(dev, NULL, aspi, &indirect_regbus_cfg);
>> +	if (IS_ERR(aspi->regmap))
>> +		return PTR_ERR(aspi->regmap);
>> +
>> +	aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
>> +
>> +	if (IS_ERR(aspi->altr_spi)) {
>> +		dev_err(dev, "%s failed to create spi platform driver\n",
>> +			__func__);
>> +		return PTR_ERR(aspi->base);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
>> +{
>> +	struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
>> +
>> +	platform_device_unregister(aspi->altr_spi);
>> +}
>> +
>> +#define FME_FEATURE_ID_MAX10_SPI        0xe
>> +
>> +static const struct dfl_device_id dfl_spi_altera_ids[] = {
>> +	{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
>> +	{ }
>> +};
>
> Maybe you can extend the Altera SPI driver with this part?

The file, drivers/spi/spi-altera.c, already has platform MODULE_ related 
code.  Wouldn't moving this code to that file produce conflicts?


>> +
>> +static struct dfl_driver dfl_spi_altera_driver = {
>> +	.drv	= {
>> +		.name       = "dfl-spi-altera",
>> +	},
>> +	.id_table = dfl_spi_altera_ids,
>> +	.probe   = dfl_spi_altera_probe,
>> +	.remove  = dfl_spi_altera_remove,
>> +};
>> +
>> +module_dfl_driver(dfl_spi_altera_driver);
>> +
>> +MODULE_DEVICE_TABLE(dfl, dfl_spi_altera_ids);
>> +MODULE_DESCRIPTION("DFL spi altera driver");
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.8.3.1
>>
>
> - Moritz
>

Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Moritz Fischer]

Hi Matthew,

On Tue, Apr 06, 2021 at 09:05:35AM -0700, matthew.gerlach@linux.intel.com wrote:
> 
> Hi Moritz,
> 
> On Mon, 5 Apr 2021, Moritz Fischer wrote:
> 
> > Hi Matthew,
> > 
> > On Mon, Apr 05, 2021 at 04:53:00PM -0700, matthew.gerlach@linux.intel.com wrote:
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > 
> > > This patch adds DFL bus driver for the Altera SPI Master
> > > controller.  The SPI master is connected to an Intel SPI Slave to
> > > Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> > > 
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > >  drivers/fpga/Kconfig          |   9 ++
> > >  drivers/fpga/Makefile         |   1 +
> > >  drivers/fpga/dfl-spi-altera.c | 221 ++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 231 insertions(+)
> > >  create mode 100644 drivers/fpga/dfl-spi-altera.c
> > > 
> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > index d591dd9..0a86994 100644
> > > --- a/drivers/fpga/Kconfig
> > > +++ b/drivers/fpga/Kconfig
> > > @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > >  	  the card. It also instantiates the SPI master (spi-altera) for
> > >  	  the card's BMC (Board Management Controller).
> > > 
> > > +config FPGA_DFL_SPI_ALTERA
> > > +	tristate "FPGA DFL Altera SPI Master Driver"
> > > +	depends on FPGA_DFL
> > > +	select REGMAP
> > > +	help
> > > +	  This is a DFL bus driver for the Altera SPI master controller.
> > > +	  The SPI master is connected to a SPI slave to Avalon Master
> > > +	  bridge in a Intel MAX BMC.
> > > +
> > >  config FPGA_DFL_PCI
> > >  	tristate "FPGA DFL PCIe Device Driver"
> > >  	depends on PCI && FPGA_DFL
> > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > index 18dc9885..58a42ad 100644
> > > --- a/drivers/fpga/Makefile
> > > +++ b/drivers/fpga/Makefile
> > > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
> > >  dfl-afu-objs += dfl-afu-error.o
> > > 
> > >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> > > +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)	+= dfl-spi-altera.o
> > > 
> > >  # Drivers for FPGAs which implement DFL
> > >  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> > > diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
> > > new file mode 100644
> > > index 0000000..9bec25fd
> > > --- /dev/null
> > > +++ b/drivers/fpga/dfl-spi-altera.c
> > > @@ -0,0 +1,221 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * DFL bus driver for Altera SPI Master
> > > + *
> > > + * Copyright (C) 2020 Intel Corporation, Inc.
> > > + *
> > > + * Authors:
> > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > + */
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/stddef.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/io.h>
> > > +#include <linux/bitfield.h>
> > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/spi/altera.h>
> > > +#include <linux/dfl.h>
> > > +
> > > +struct dfl_altera_spi {
> > > +	void __iomem *base;
> > > +	struct regmap *regmap;
> > > +	struct device *dev;
> > > +	struct platform_device *altr_spi;
> > > +};
> > > +
> > > +#define SPI_CORE_PARAMETER      0x8
> > > +#define SHIFT_MODE              BIT_ULL(1)
> > > +#define SHIFT_MODE_MSB          0
> > > +#define SHIFT_MODE_LSB          1
> > > +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> > > +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> > > +#define CLK_POLARITY            BIT_ULL(14)
> > > +#define CLK_PHASE               BIT_ULL(15)
> > > +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> > > +#define SPI_CLK                 GENMASK_ULL(31, 22)
> > > +#define SPI_INDIRECT_ACC_OFST   0x10
> > > +
> > > +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> > > +#define INDIRECT_WR             BIT_ULL(8)
> > > +#define INDIRECT_RD             BIT_ULL(9)
> > > +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> > > +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> > > +#define INDIRECT_DEBUG          BIT_ULL(32)
> > > +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> > > +#define INDIRECT_TIMEOUT        10000
> > > +
> > > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > > +				 unsigned int *val)
> > > +{
> > > +	struct dfl_altera_spi *aspi = context;
> > > +	void __iomem *base = aspi->base;
> > > +	int loops;
> > > +	u64 v;
> > > +
> > > +	writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> > > +
> > > +	loops = 0;
> > > +	while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> > > +	       (loops++ < INDIRECT_TIMEOUT))
> > > +		cpu_relax();
> > > +
> > > +	if (loops >= INDIRECT_TIMEOUT) {
> > > +		pr_err("%s timed out %d\n", __func__, loops);
> > > +		return -ETIME;
> > > +	}
> > > +
> > > +	v = readq(base + INDIRECT_RD_DATA);
> > > +
> > > +	*val = v & INDIRECT_DATA_MASK;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int indirect_bus_reg_write(void *context, unsigned int reg,
> > > +				  unsigned int val)
> > > +{
> > > +	struct dfl_altera_spi *aspi = context;
> > > +	void __iomem *base = aspi->base;
> > > +	int loops;
> > > +
> > > +	writeq(val, base + INDIRECT_WR_DATA);
> > > +	writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> > > +
> > > +	loops = 0;
> > > +	while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> > > +	       (loops++ < INDIRECT_TIMEOUT))
> > > +		cpu_relax();
> > > +
> > > +	if (loops >= INDIRECT_TIMEOUT) {
> > > +		pr_err("%s timed out %d\n", __func__, loops);
> > > +		return -ETIME;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct regmap_config indirect_regbus_cfg = {
> > > +	.reg_bits = 32,
> > > +	.reg_stride = 4,
> > > +	.val_bits = 32,
> > > +	.fast_io = true,
> > > +
> > > +	.reg_write = indirect_bus_reg_write,
> > > +	.reg_read = indirect_bus_reg_read,
> > > +};
> > > +
> > > +static struct spi_board_info m10_bmc_info = {
> > > +	.modalias = "m10-d5005",
> > > +	.max_speed_hz = 12500000,
> > > +	.bus_num = 0,
> > > +	.chip_select = 0,
> > > +};
> > > +
> > > +static struct platform_device *create_cntrl(struct device *dev,
> > > +					    void __iomem *base,
> > > +					    struct spi_board_info *m10_info)
> > > +{
> > > +	struct altera_spi_platform_data pdata;
> > > +	struct platform_device_info pdevinfo;
> > > +	u64 v;
> > > +
> > > +	v = readq(base + SPI_CORE_PARAMETER);
> > > +
> > > +	memset(&pdata, 0, sizeof(pdata));
> > > +	pdata.mode_bits = SPI_CS_HIGH;
> > > +	if (FIELD_GET(CLK_POLARITY, v))
> > > +		pdata.mode_bits |= SPI_CPOL;
> > > +	if (FIELD_GET(CLK_PHASE, v))
> > > +		pdata.mode_bits |= SPI_CPHA;
> > > +
> > > +	pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > > +	pdata.bits_per_word_mask =
> > > +		SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> > > +
> > > +	pdata.num_devices = 1;
> > > +	pdata.devices = m10_info;
> > > +
> > > +	dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > > +		pdata.num_chipselect, pdata.bits_per_word_mask,
> > > +		pdata.mode_bits);
> > > +
> > > +	memset(&pdevinfo, 0, sizeof(pdevinfo));
> > > +
> > > +	pdevinfo.name = "subdev_spi_altera";
> > > +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> > > +	pdevinfo.parent = dev;
> > > +	pdevinfo.data = &pdata;
> > > +	pdevinfo.size_data = sizeof(pdata);
> > > +
> > > +	return platform_device_register_full(&pdevinfo);
> > 
> > Should this be a SPI driver? I think looking at the UIO case we had
> > decided against this pattern?
> 
> This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which
> uses this design pattern.  Is it okay to keep this design pattern for
> consistency?
> 
> 
> > 
> > > +}
> > > +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> > > +{
> > > +	struct device *dev = &dfl_dev->dev;
> > > +	struct dfl_altera_spi *aspi;
> > > +
> > > +	aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> > > +
> > > +	if (!aspi)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_set_drvdata(dev, aspi);
> > > +
> > > +	aspi->dev = dev;
> > > +
> > > +	aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > +
> > > +	if (IS_ERR(aspi->base)) {
> > > +		dev_err(dev, "%s get mem resource fail!\n", __func__);
> > > +		return PTR_ERR(aspi->base);
> > > +	}
> > > +
> > > +	aspi->regmap = devm_regmap_init(dev, NULL, aspi, &indirect_regbus_cfg);
> > > +	if (IS_ERR(aspi->regmap))
> > > +		return PTR_ERR(aspi->regmap);
> > > +
> > > +	aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> > > +
> > > +	if (IS_ERR(aspi->altr_spi)) {
> > > +		dev_err(dev, "%s failed to create spi platform driver\n",
> > > +			__func__);
> > > +		return PTR_ERR(aspi->base);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > +{
> > > +	struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > +
> > > +	platform_device_unregister(aspi->altr_spi);
> > > +}
> > > +
> > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > +
> > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > +	{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > +	{ }
> > > +};
> > 
> > Maybe you can extend the Altera SPI driver with this part?
> 
> The file, drivers/spi/spi-altera.c, already has platform MODULE_ related
> code.  Wouldn't moving this code to that file produce conflicts?

I've seen other drivers support multiple busses, so it should be
possible, there might be nuances I'm missing in my brief look at this,
though.

I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
and the other one MODULE_DEVICE_TABLE(dfl, ...)

See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
they might be guarding against what you describe with CONFIG_OF vs
CONFIG_ACPI)

If that doesn't work we could split it up into

altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
or something of that sort?

My point being, now that we have a bus, let's use it and develop drivers
according to the Linux device model where possible :)

Cheers,
Moritz

RE: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Wu, Hao]

> > On Mon, 5 Apr 2021, Moritz Fischer wrote:
> >
> > > Hi Matthew,
> > >
> > > On Mon, Apr 05, 2021 at 04:53:00PM -0700,
> matthew.gerlach@linux.intel.com wrote:
> > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > >
> > > > This patch adds DFL bus driver for the Altera SPI Master
> > > > controller.  The SPI master is connected to an Intel SPI Slave to
> > > > Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> > > >
> > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > ---
> > > >  drivers/fpga/Kconfig          |   9 ++
> > > >  drivers/fpga/Makefile         |   1 +
> > > >  drivers/fpga/dfl-spi-altera.c | 221
> ++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 231 insertions(+)
> > > >  create mode 100644 drivers/fpga/dfl-spi-altera.c
> > > >
> > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > index d591dd9..0a86994 100644
> > > > --- a/drivers/fpga/Kconfig
> > > > +++ b/drivers/fpga/Kconfig
> > > > @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > > >  	  the card. It also instantiates the SPI master (spi-altera) for
> > > >  	  the card's BMC (Board Management Controller).
> > > >
> > > > +config FPGA_DFL_SPI_ALTERA
> > > > +	tristate "FPGA DFL Altera SPI Master Driver"
> > > > +	depends on FPGA_DFL
> > > > +	select REGMAP
> > > > +	help
> > > > +	  This is a DFL bus driver for the Altera SPI master controller.
> > > > +	  The SPI master is connected to a SPI slave to Avalon Master
> > > > +	  bridge in a Intel MAX BMC.
> > > > +
> > > >  config FPGA_DFL_PCI
> > > >  	tristate "FPGA DFL PCIe Device Driver"
> > > >  	depends on PCI && FPGA_DFL
> > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > index 18dc9885..58a42ad 100644
> > > > --- a/drivers/fpga/Makefile
> > > > +++ b/drivers/fpga/Makefile
> > > > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-
> afu-dma-region.o
> > > >  dfl-afu-objs += dfl-afu-error.o
> > > >
> > > >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-
> nios.o
> > > > +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)	+= dfl-spi-altera.o
> > > >
> > > >  # Drivers for FPGAs which implement DFL
> > > >  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> > > > diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
> > > > new file mode 100644
> > > > index 0000000..9bec25fd
> > > > --- /dev/null
> > > > +++ b/drivers/fpga/dfl-spi-altera.c
> > > > @@ -0,0 +1,221 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * DFL bus driver for Altera SPI Master
> > > > + *
> > > > + * Copyright (C) 2020 Intel Corporation, Inc.
> > > > + *
> > > > + * Authors:
> > > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > + */
> > > > +
> > > > +#include <linux/types.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/stddef.h>
> > > > +#include <linux/errno.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/spi/spi.h>
> > > > +#include <linux/spi/altera.h>
> > > > +#include <linux/dfl.h>
> > > > +
> > > > +struct dfl_altera_spi {
> > > > +	void __iomem *base;
> > > > +	struct regmap *regmap;
> > > > +	struct device *dev;
> > > > +	struct platform_device *altr_spi;
> > > > +};
> > > > +
> > > > +#define SPI_CORE_PARAMETER      0x8
> > > > +#define SHIFT_MODE              BIT_ULL(1)
> > > > +#define SHIFT_MODE_MSB          0
> > > > +#define SHIFT_MODE_LSB          1
> > > > +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> > > > +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> > > > +#define CLK_POLARITY            BIT_ULL(14)
> > > > +#define CLK_PHASE               BIT_ULL(15)
> > > > +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> > > > +#define SPI_CLK                 GENMASK_ULL(31, 22)
> > > > +#define SPI_INDIRECT_ACC_OFST   0x10
> > > > +
> > > > +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> > > > +#define INDIRECT_WR             BIT_ULL(8)
> > > > +#define INDIRECT_RD             BIT_ULL(9)
> > > > +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> > > > +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> > > > +#define INDIRECT_DEBUG          BIT_ULL(32)
> > > > +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> > > > +#define INDIRECT_TIMEOUT        10000
> > > > +
> > > > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > > > +				 unsigned int *val)
> > > > +{
> > > > +	struct dfl_altera_spi *aspi = context;
> > > > +	void __iomem *base = aspi->base;
> > > > +	int loops;
> > > > +	u64 v;
> > > > +
> > > > +	writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> > > > +
> > > > +	loops = 0;
> > > > +	while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> > > > +	       (loops++ < INDIRECT_TIMEOUT))
> > > > +		cpu_relax();
> > > > +
> > > > +	if (loops >= INDIRECT_TIMEOUT) {
> > > > +		pr_err("%s timed out %d\n", __func__, loops);
> > > > +		return -ETIME;
> > > > +	}
> > > > +
> > > > +	v = readq(base + INDIRECT_RD_DATA);
> > > > +
> > > > +	*val = v & INDIRECT_DATA_MASK;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int indirect_bus_reg_write(void *context, unsigned int reg,
> > > > +				  unsigned int val)
> > > > +{
> > > > +	struct dfl_altera_spi *aspi = context;
> > > > +	void __iomem *base = aspi->base;
> > > > +	int loops;
> > > > +
> > > > +	writeq(val, base + INDIRECT_WR_DATA);
> > > > +	writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> > > > +
> > > > +	loops = 0;
> > > > +	while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> > > > +	       (loops++ < INDIRECT_TIMEOUT))
> > > > +		cpu_relax();
> > > > +
> > > > +	if (loops >= INDIRECT_TIMEOUT) {
> > > > +		pr_err("%s timed out %d\n", __func__, loops);
> > > > +		return -ETIME;
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct regmap_config indirect_regbus_cfg = {
> > > > +	.reg_bits = 32,
> > > > +	.reg_stride = 4,
> > > > +	.val_bits = 32,
> > > > +	.fast_io = true,
> > > > +
> > > > +	.reg_write = indirect_bus_reg_write,
> > > > +	.reg_read = indirect_bus_reg_read,
> > > > +};
> > > > +
> > > > +static struct spi_board_info m10_bmc_info = {
> > > > +	.modalias = "m10-d5005",
> > > > +	.max_speed_hz = 12500000,
> > > > +	.bus_num = 0,
> > > > +	.chip_select = 0,
> > > > +};
> > > > +
> > > > +static struct platform_device *create_cntrl(struct device *dev,
> > > > +					    void __iomem *base,
> > > > +					    struct spi_board_info *m10_info)
> > > > +{
> > > > +	struct altera_spi_platform_data pdata;
> > > > +	struct platform_device_info pdevinfo;
> > > > +	u64 v;
> > > > +
> > > > +	v = readq(base + SPI_CORE_PARAMETER);
> > > > +
> > > > +	memset(&pdata, 0, sizeof(pdata));
> > > > +	pdata.mode_bits = SPI_CS_HIGH;
> > > > +	if (FIELD_GET(CLK_POLARITY, v))
> > > > +		pdata.mode_bits |= SPI_CPOL;
> > > > +	if (FIELD_GET(CLK_PHASE, v))
> > > > +		pdata.mode_bits |= SPI_CPHA;
> > > > +
> > > > +	pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > > > +	pdata.bits_per_word_mask =
> > > > +		SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> > > > +
> > > > +	pdata.num_devices = 1;
> > > > +	pdata.devices = m10_info;
> > > > +
> > > > +	dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > > > +		pdata.num_chipselect, pdata.bits_per_word_mask,
> > > > +		pdata.mode_bits);
> > > > +
> > > > +	memset(&pdevinfo, 0, sizeof(pdevinfo));
> > > > +
> > > > +	pdevinfo.name = "subdev_spi_altera";
> > > > +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> > > > +	pdevinfo.parent = dev;
> > > > +	pdevinfo.data = &pdata;
> > > > +	pdevinfo.size_data = sizeof(pdata);
> > > > +
> > > > +	return platform_device_register_full(&pdevinfo);
> > >
> > > Should this be a SPI driver? I think looking at the UIO case we had
> > > decided against this pattern?
> >
> > This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which
> > uses this design pattern.  Is it okay to keep this design pattern for
> > consistency?
> >
> >
> > >
> > > > +}
> > > > +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> > > > +{
> > > > +	struct device *dev = &dfl_dev->dev;
> > > > +	struct dfl_altera_spi *aspi;
> > > > +
> > > > +	aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> > > > +
> > > > +	if (!aspi)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	dev_set_drvdata(dev, aspi);
> > > > +
> > > > +	aspi->dev = dev;
> > > > +
> > > > +	aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > > +
> > > > +	if (IS_ERR(aspi->base)) {
> > > > +		dev_err(dev, "%s get mem resource fail!\n", __func__);
> > > > +		return PTR_ERR(aspi->base);
> > > > +	}
> > > > +
> > > > +	aspi->regmap = devm_regmap_init(dev, NULL, aspi,
> &indirect_regbus_cfg);
> > > > +	if (IS_ERR(aspi->regmap))
> > > > +		return PTR_ERR(aspi->regmap);
> > > > +
> > > > +	aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> > > > +
> > > > +	if (IS_ERR(aspi->altr_spi)) {
> > > > +		dev_err(dev, "%s failed to create spi platform driver\n",
> > > > +			__func__);
> > > > +		return PTR_ERR(aspi->base);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > +{
> > > > +	struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > +
> > > > +	platform_device_unregister(aspi->altr_spi);
> > > > +}
> > > > +
> > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > +
> > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > +	{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > +	{ }
> > > > +};
> > >
> > > Maybe you can extend the Altera SPI driver with this part?
> >
> > The file, drivers/spi/spi-altera.c, already has platform MODULE_ related
> > code.  Wouldn't moving this code to that file produce conflicts?
> 
> I've seen other drivers support multiple busses, so it should be
> possible, there might be nuances I'm missing in my brief look at this,
> though.
> 
> I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> and the other one MODULE_DEVICE_TABLE(dfl, ...)
> 
> See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> they might be guarding against what you describe with CONFIG_OF vs
> CONFIG_ACPI)
> 
> If that doesn't work we could split it up into
> 
> altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> or something of that sort?
> 
> My point being, now that we have a bus, let's use it and develop drivers
> according to the Linux device model where possible :)

Agree. This does make sense from my side too. DFL core provides the mechanism
to enumerate different IPs on FPGA, but each function driver needs to go to
related subsystem for review.  : )

I understand that for FPGA case, it may have some additional logics for specific
purposes based on common altera spi master IP, then additional code for 
a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-xxxx.c

Thanks
Hao

> 
> Cheers,
> Moritz

Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Xu Yilun]

On Thu, Apr 08, 2021 at 03:30:15PM +0800, Wu, Hao wrote:
> > > On Mon, 5 Apr 2021, Moritz Fischer wrote:
> > >
> > > > Hi Matthew,
> > > >
> > > > On Mon, Apr 05, 2021 at 04:53:00PM -0700,
> > matthew.gerlach@linux.intel.com wrote:
> > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > >
> > > > > This patch adds DFL bus driver for the Altera SPI Master
> > > > > controller.  The SPI master is connected to an Intel SPI Slave to
> > > > > Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> > > > >
> > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > ---
> > > > >  drivers/fpga/Kconfig          |   9 ++
> > > > >  drivers/fpga/Makefile         |   1 +
> > > > >  drivers/fpga/dfl-spi-altera.c | 221
> > ++++++++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 231 insertions(+)
> > > > >  create mode 100644 drivers/fpga/dfl-spi-altera.c
> > > > >
> > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > > index d591dd9..0a86994 100644
> > > > > --- a/drivers/fpga/Kconfig
> > > > > +++ b/drivers/fpga/Kconfig
> > > > > @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > > > >    the card. It also instantiates the SPI master (spi-altera) for
> > > > >    the card's BMC (Board Management Controller).
> > > > >
> > > > > +config FPGA_DFL_SPI_ALTERA
> > > > > +tristate "FPGA DFL Altera SPI Master Driver"
> > > > > +depends on FPGA_DFL
> > > > > +select REGMAP
> > > > > +help
> > > > > +  This is a DFL bus driver for the Altera SPI master controller.
> > > > > +  The SPI master is connected to a SPI slave to Avalon Master
> > > > > +  bridge in a Intel MAX BMC.
> > > > > +
> > > > >  config FPGA_DFL_PCI
> > > > >  tristate "FPGA DFL PCIe Device Driver"
> > > > >  depends on PCI && FPGA_DFL
> > > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > > index 18dc9885..58a42ad 100644
> > > > > --- a/drivers/fpga/Makefile
> > > > > +++ b/drivers/fpga/Makefile
> > > > > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-
> > afu-dma-region.o
> > > > >  dfl-afu-objs += dfl-afu-error.o
> > > > >
> > > > >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)+= dfl-n3000-
> > nios.o
> > > > > +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)+= dfl-spi-altera.o
> > > > >
> > > > >  # Drivers for FPGAs which implement DFL
> > > > >  obj-$(CONFIG_FPGA_DFL_PCI)+= dfl-pci.o
> > > > > diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
> > > > > new file mode 100644
> > > > > index 0000000..9bec25fd
> > > > > --- /dev/null
> > > > > +++ b/drivers/fpga/dfl-spi-altera.c
> > > > > @@ -0,0 +1,221 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * DFL bus driver for Altera SPI Master
> > > > > + *
> > > > > + * Copyright (C) 2020 Intel Corporation, Inc.
> > > > > + *
> > > > > + * Authors:
> > > > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > + */
> > > > > +
> > > > > +#include <linux/types.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/stddef.h>
> > > > > +#include <linux/errno.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/bitfield.h>
> > > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > > +#include <linux/regmap.h>
> > > > > +#include <linux/spi/spi.h>
> > > > > +#include <linux/spi/altera.h>
> > > > > +#include <linux/dfl.h>
> > > > > +
> > > > > +struct dfl_altera_spi {
> > > > > +void __iomem *base;
> > > > > +struct regmap *regmap;
> > > > > +struct device *dev;
> > > > > +struct platform_device *altr_spi;
> > > > > +};
> > > > > +
> > > > > +#define SPI_CORE_PARAMETER      0x8
> > > > > +#define SHIFT_MODE              BIT_ULL(1)
> > > > > +#define SHIFT_MODE_MSB          0
> > > > > +#define SHIFT_MODE_LSB          1
> > > > > +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> > > > > +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> > > > > +#define CLK_POLARITY            BIT_ULL(14)
> > > > > +#define CLK_PHASE               BIT_ULL(15)
> > > > > +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> > > > > +#define SPI_CLK                 GENMASK_ULL(31, 22)
> > > > > +#define SPI_INDIRECT_ACC_OFST   0x10
> > > > > +
> > > > > +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> > > > > +#define INDIRECT_WR             BIT_ULL(8)
> > > > > +#define INDIRECT_RD             BIT_ULL(9)
> > > > > +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> > > > > +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> > > > > +#define INDIRECT_DEBUG          BIT_ULL(32)
> > > > > +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> > > > > +#define INDIRECT_TIMEOUT        10000
> > > > > +
> > > > > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > > > > + unsigned int *val)
> > > > > +{
> > > > > +struct dfl_altera_spi *aspi = context;
> > > > > +void __iomem *base = aspi->base;
> > > > > +int loops;
> > > > > +u64 v;
> > > > > +
> > > > > +writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> > > > > +
> > > > > +loops = 0;
> > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > +cpu_relax();
> > > > > +
> > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > +return -ETIME;
> > > > > +}
> > > > > +
> > > > > +v = readq(base + INDIRECT_RD_DATA);
> > > > > +
> > > > > +*val = v & INDIRECT_DATA_MASK;
> > > > > +
> > > > > +return 0;
> > > > > +}
> > > > > +
> > > > > +static int indirect_bus_reg_write(void *context, unsigned int reg,
> > > > > +  unsigned int val)
> > > > > +{
> > > > > +struct dfl_altera_spi *aspi = context;
> > > > > +void __iomem *base = aspi->base;
> > > > > +int loops;
> > > > > +
> > > > > +writeq(val, base + INDIRECT_WR_DATA);
> > > > > +writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> > > > > +
> > > > > +loops = 0;
> > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > +cpu_relax();
> > > > > +
> > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > +return -ETIME;
> > > > > +}
> > > > > +return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct regmap_config indirect_regbus_cfg = {
> > > > > +.reg_bits = 32,
> > > > > +.reg_stride = 4,
> > > > > +.val_bits = 32,
> > > > > +.fast_io = true,
> > > > > +
> > > > > +.reg_write = indirect_bus_reg_write,
> > > > > +.reg_read = indirect_bus_reg_read,
> > > > > +};
> > > > > +
> > > > > +static struct spi_board_info m10_bmc_info = {
> > > > > +.modalias = "m10-d5005",
> > > > > +.max_speed_hz = 12500000,
> > > > > +.bus_num = 0,
> > > > > +.chip_select = 0,
> > > > > +};
> > > > > +
> > > > > +static struct platform_device *create_cntrl(struct device *dev,
> > > > > +    void __iomem *base,
> > > > > +    struct spi_board_info *m10_info)
> > > > > +{
> > > > > +struct altera_spi_platform_data pdata;
> > > > > +struct platform_device_info pdevinfo;
> > > > > +u64 v;
> > > > > +
> > > > > +v = readq(base + SPI_CORE_PARAMETER);
> > > > > +
> > > > > +memset(&pdata, 0, sizeof(pdata));
> > > > > +pdata.mode_bits = SPI_CS_HIGH;
> > > > > +if (FIELD_GET(CLK_POLARITY, v))
> > > > > +pdata.mode_bits |= SPI_CPOL;
> > > > > +if (FIELD_GET(CLK_PHASE, v))
> > > > > +pdata.mode_bits |= SPI_CPHA;
> > > > > +
> > > > > +pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > > > > +pdata.bits_per_word_mask =
> > > > > +SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> > > > > +
> > > > > +pdata.num_devices = 1;
> > > > > +pdata.devices = m10_info;
> > > > > +
> > > > > +dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > > > > +pdata.num_chipselect, pdata.bits_per_word_mask,
> > > > > +pdata.mode_bits);
> > > > > +
> > > > > +memset(&pdevinfo, 0, sizeof(pdevinfo));
> > > > > +
> > > > > +pdevinfo.name = "subdev_spi_altera";
> > > > > +pdevinfo.id = PLATFORM_DEVID_AUTO;
> > > > > +pdevinfo.parent = dev;
> > > > > +pdevinfo.data = &pdata;
> > > > > +pdevinfo.size_data = sizeof(pdata);
> > > > > +
> > > > > +return platform_device_register_full(&pdevinfo);
> > > >
> > > > Should this be a SPI driver? I think looking at the UIO case we had
> > > > decided against this pattern?
> > >
> > > This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which
> > > uses this design pattern.  Is it okay to keep this design pattern for
> > > consistency?
> > >
> > >
> > > >
> > > > > +}
> > > > > +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> > > > > +{
> > > > > +struct device *dev = &dfl_dev->dev;
> > > > > +struct dfl_altera_spi *aspi;
> > > > > +
> > > > > +aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> > > > > +
> > > > > +if (!aspi)
> > > > > +return -ENOMEM;
> > > > > +
> > > > > +dev_set_drvdata(dev, aspi);
> > > > > +
> > > > > +aspi->dev = dev;
> > > > > +
> > > > > +aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > > > +
> > > > > +if (IS_ERR(aspi->base)) {
> > > > > +dev_err(dev, "%s get mem resource fail!\n", __func__);
> > > > > +return PTR_ERR(aspi->base);
> > > > > +}
> > > > > +
> > > > > +aspi->regmap = devm_regmap_init(dev, NULL, aspi,
> > &indirect_regbus_cfg);
> > > > > +if (IS_ERR(aspi->regmap))
> > > > > +return PTR_ERR(aspi->regmap);
> > > > > +
> > > > > +aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> > > > > +
> > > > > +if (IS_ERR(aspi->altr_spi)) {
> > > > > +dev_err(dev, "%s failed to create spi platform driver\n",
> > > > > +__func__);
> > > > > +return PTR_ERR(aspi->base);
> > > > > +}
> > > > > +
> > > > > +return 0;
> > > > > +}
> > > > > +
> > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > +{
> > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > +
> > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > +}
> > > > > +
> > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > +
> > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > +{ }
> > > > > +};
> > > >
> > > > Maybe you can extend the Altera SPI driver with this part?
> > >
> > > The file, drivers/spi/spi-altera.c, already has platform MODULE_ related
> > > code.  Wouldn't moving this code to that file produce conflicts?
> >
> > I've seen other drivers support multiple busses, so it should be
> > possible, there might be nuances I'm missing in my brief look at this,
> > though.
> >
> > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> >
> > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > they might be guarding against what you describe with CONFIG_OF vs
> > CONFIG_ACPI)
> >
> > If that doesn't work we could split it up into
> >
> > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > or something of that sort?
> >
> > My point being, now that we have a bus, let's use it and develop drivers
> > according to the Linux device model where possible :)
> 
> Agree. This does make sense from my side too. DFL core provides the mechanism
> to enumerate different IPs on FPGA, but each function driver needs to go to
> related subsystem for review.  : )
> 
> I understand that for FPGA case, it may have some additional logics for specific
> purposes based on common altera spi master IP, then additional code for

I'm wondering if the additional logics are extensions for common spi-altera. Like the
SPI_CORE_PARAMETER register, it is not within the register space of
spi-altera, 


  |   |      +-------------+
  |DFL|------| +--------+  |              
  |BUS|      | |SPI CORE|  |             
  |   |      | |PARAM   |  |             
  |   |      | +--------+  |             
  |   |      |             |             
  |   |      | +--------+  |   +-------+
             | |Indirect|  |   |spi    |
             | |access  +--+---|altera |
             | |master  |  |   +-------+
             | +--------+  |             
             +-------------+
> a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-xxxx.c

So is it proper we integrate this feature into spi-altera? Previously
we have merged the dfl-n3000-nios, its spi part is very similar as
this driver. The dfl-n3000-nios make the spi-altera as a sub device.
Could we borrow the idea, or could we just integrate this driver in
dfl-n3000-nios?

Thanks,
Yilun

> 
> Thanks
> Hao
> 
> >
> > Cheers,
> > Moritz

RE: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Wu, Hao]

> On Thu, Apr 08, 2021 at 03:30:15PM +0800, Wu, Hao wrote:
> > > > On Mon, 5 Apr 2021, Moritz Fischer wrote:
> > > >
> > > > > Hi Matthew,
> > > > >
> > > > > On Mon, Apr 05, 2021 at 04:53:00PM -0700,
> > > matthew.gerlach@linux.intel.com wrote:
> > > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > >
> > > > > > This patch adds DFL bus driver for the Altera SPI Master
> > > > > > controller.  The SPI master is connected to an Intel SPI Slave to
> > > > > > Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> > > > > >
> > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > ---
> > > > > >  drivers/fpga/Kconfig          |   9 ++
> > > > > >  drivers/fpga/Makefile         |   1 +
> > > > > >  drivers/fpga/dfl-spi-altera.c | 221
> > > ++++++++++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 231 insertions(+)
> > > > > >  create mode 100644 drivers/fpga/dfl-spi-altera.c
> > > > > >
> > > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > > > index d591dd9..0a86994 100644
> > > > > > --- a/drivers/fpga/Kconfig
> > > > > > +++ b/drivers/fpga/Kconfig
> > > > > > @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > > > > >    the card. It also instantiates the SPI master (spi-altera) for
> > > > > >    the card's BMC (Board Management Controller).
> > > > > >
> > > > > > +config FPGA_DFL_SPI_ALTERA
> > > > > > +tristate "FPGA DFL Altera SPI Master Driver"
> > > > > > +depends on FPGA_DFL
> > > > > > +select REGMAP
> > > > > > +help
> > > > > > +  This is a DFL bus driver for the Altera SPI master controller.
> > > > > > +  The SPI master is connected to a SPI slave to Avalon Master
> > > > > > +  bridge in a Intel MAX BMC.
> > > > > > +
> > > > > >  config FPGA_DFL_PCI
> > > > > >  tristate "FPGA DFL PCIe Device Driver"
> > > > > >  depends on PCI && FPGA_DFL
> > > > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > > > index 18dc9885..58a42ad 100644
> > > > > > --- a/drivers/fpga/Makefile
> > > > > > +++ b/drivers/fpga/Makefile
> > > > > > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-
> > > afu-dma-region.o
> > > > > >  dfl-afu-objs += dfl-afu-error.o
> > > > > >
> > > > > >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)+= dfl-n3000-
> > > nios.o
> > > > > > +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)+= dfl-spi-altera.o
> > > > > >
> > > > > >  # Drivers for FPGAs which implement DFL
> > > > > >  obj-$(CONFIG_FPGA_DFL_PCI)+= dfl-pci.o
> > > > > > diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
> > > > > > new file mode 100644
> > > > > > index 0000000..9bec25fd
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/fpga/dfl-spi-altera.c
> > > > > > @@ -0,0 +1,221 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * DFL bus driver for Altera SPI Master
> > > > > > + *
> > > > > > + * Copyright (C) 2020 Intel Corporation, Inc.
> > > > > > + *
> > > > > > + * Authors:
> > > > > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/types.h>
> > > > > > +#include <linux/kernel.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/stddef.h>
> > > > > > +#include <linux/errno.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/io.h>
> > > > > > +#include <linux/bitfield.h>
> > > > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > > > +#include <linux/regmap.h>
> > > > > > +#include <linux/spi/spi.h>
> > > > > > +#include <linux/spi/altera.h>
> > > > > > +#include <linux/dfl.h>
> > > > > > +
> > > > > > +struct dfl_altera_spi {
> > > > > > +void __iomem *base;
> > > > > > +struct regmap *regmap;
> > > > > > +struct device *dev;
> > > > > > +struct platform_device *altr_spi;
> > > > > > +};
> > > > > > +
> > > > > > +#define SPI_CORE_PARAMETER      0x8
> > > > > > +#define SHIFT_MODE              BIT_ULL(1)
> > > > > > +#define SHIFT_MODE_MSB          0
> > > > > > +#define SHIFT_MODE_LSB          1
> > > > > > +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> > > > > > +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> > > > > > +#define CLK_POLARITY            BIT_ULL(14)
> > > > > > +#define CLK_PHASE               BIT_ULL(15)
> > > > > > +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> > > > > > +#define SPI_CLK                 GENMASK_ULL(31, 22)
> > > > > > +#define SPI_INDIRECT_ACC_OFST   0x10
> > > > > > +
> > > > > > +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> > > > > > +#define INDIRECT_WR             BIT_ULL(8)
> > > > > > +#define INDIRECT_RD             BIT_ULL(9)
> > > > > > +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> > > > > > +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> > > > > > +#define INDIRECT_DEBUG          BIT_ULL(32)
> > > > > > +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> > > > > > +#define INDIRECT_TIMEOUT        10000
> > > > > > +
> > > > > > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > > > > > + unsigned int *val)
> > > > > > +{
> > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > +void __iomem *base = aspi->base;
> > > > > > +int loops;
> > > > > > +u64 v;
> > > > > > +
> > > > > > +writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> > > > > > +
> > > > > > +loops = 0;
> > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > +cpu_relax();
> > > > > > +
> > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > +return -ETIME;
> > > > > > +}
> > > > > > +
> > > > > > +v = readq(base + INDIRECT_RD_DATA);
> > > > > > +
> > > > > > +*val = v & INDIRECT_DATA_MASK;
> > > > > > +
> > > > > > +return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int indirect_bus_reg_write(void *context, unsigned int reg,
> > > > > > +  unsigned int val)
> > > > > > +{
> > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > +void __iomem *base = aspi->base;
> > > > > > +int loops;
> > > > > > +
> > > > > > +writeq(val, base + INDIRECT_WR_DATA);
> > > > > > +writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> > > > > > +
> > > > > > +loops = 0;
> > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > +cpu_relax();
> > > > > > +
> > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > +return -ETIME;
> > > > > > +}
> > > > > > +return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static const struct regmap_config indirect_regbus_cfg = {
> > > > > > +.reg_bits = 32,
> > > > > > +.reg_stride = 4,
> > > > > > +.val_bits = 32,
> > > > > > +.fast_io = true,
> > > > > > +
> > > > > > +.reg_write = indirect_bus_reg_write,
> > > > > > +.reg_read = indirect_bus_reg_read,
> > > > > > +};
> > > > > > +
> > > > > > +static struct spi_board_info m10_bmc_info = {
> > > > > > +.modalias = "m10-d5005",
> > > > > > +.max_speed_hz = 12500000,
> > > > > > +.bus_num = 0,
> > > > > > +.chip_select = 0,
> > > > > > +};
> > > > > > +
> > > > > > +static struct platform_device *create_cntrl(struct device *dev,
> > > > > > +    void __iomem *base,
> > > > > > +    struct spi_board_info *m10_info)
> > > > > > +{
> > > > > > +struct altera_spi_platform_data pdata;
> > > > > > +struct platform_device_info pdevinfo;
> > > > > > +u64 v;
> > > > > > +
> > > > > > +v = readq(base + SPI_CORE_PARAMETER);
> > > > > > +
> > > > > > +memset(&pdata, 0, sizeof(pdata));
> > > > > > +pdata.mode_bits = SPI_CS_HIGH;
> > > > > > +if (FIELD_GET(CLK_POLARITY, v))
> > > > > > +pdata.mode_bits |= SPI_CPOL;
> > > > > > +if (FIELD_GET(CLK_PHASE, v))
> > > > > > +pdata.mode_bits |= SPI_CPHA;
> > > > > > +
> > > > > > +pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > > > > > +pdata.bits_per_word_mask =
> > > > > > +SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> > > > > > +
> > > > > > +pdata.num_devices = 1;
> > > > > > +pdata.devices = m10_info;
> > > > > > +
> > > > > > +dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > > > > > +pdata.num_chipselect, pdata.bits_per_word_mask,
> > > > > > +pdata.mode_bits);
> > > > > > +
> > > > > > +memset(&pdevinfo, 0, sizeof(pdevinfo));
> > > > > > +
> > > > > > +pdevinfo.name = "subdev_spi_altera";
> > > > > > +pdevinfo.id = PLATFORM_DEVID_AUTO;
> > > > > > +pdevinfo.parent = dev;
> > > > > > +pdevinfo.data = &pdata;
> > > > > > +pdevinfo.size_data = sizeof(pdata);
> > > > > > +
> > > > > > +return platform_device_register_full(&pdevinfo);
> > > > >
> > > > > Should this be a SPI driver? I think looking at the UIO case we had
> > > > > decided against this pattern?
> > > >
> > > > This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which
> > > > uses this design pattern.  Is it okay to keep this design pattern for
> > > > consistency?
> > > >
> > > >
> > > > >
> > > > > > +}
> > > > > > +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> > > > > > +{
> > > > > > +struct device *dev = &dfl_dev->dev;
> > > > > > +struct dfl_altera_spi *aspi;
> > > > > > +
> > > > > > +aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> > > > > > +
> > > > > > +if (!aspi)
> > > > > > +return -ENOMEM;
> > > > > > +
> > > > > > +dev_set_drvdata(dev, aspi);
> > > > > > +
> > > > > > +aspi->dev = dev;
> > > > > > +
> > > > > > +aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > > > > +
> > > > > > +if (IS_ERR(aspi->base)) {
> > > > > > +dev_err(dev, "%s get mem resource fail!\n", __func__);
> > > > > > +return PTR_ERR(aspi->base);
> > > > > > +}
> > > > > > +
> > > > > > +aspi->regmap = devm_regmap_init(dev, NULL, aspi,
> > > &indirect_regbus_cfg);
> > > > > > +if (IS_ERR(aspi->regmap))
> > > > > > +return PTR_ERR(aspi->regmap);
> > > > > > +
> > > > > > +aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> > > > > > +
> > > > > > +if (IS_ERR(aspi->altr_spi)) {
> > > > > > +dev_err(dev, "%s failed to create spi platform driver\n",
> > > > > > +__func__);
> > > > > > +return PTR_ERR(aspi->base);
> > > > > > +}
> > > > > > +
> > > > > > +return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > +{
> > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > +
> > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > +}
> > > > > > +
> > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > +
> > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > +{ }
> > > > > > +};
> > > > >
> > > > > Maybe you can extend the Altera SPI driver with this part?
> > > >
> > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_ related
> > > > code.  Wouldn't moving this code to that file produce conflicts?
> > >
> > > I've seen other drivers support multiple busses, so it should be
> > > possible, there might be nuances I'm missing in my brief look at this,
> > > though.
> > >
> > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > >
> > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > > they might be guarding against what you describe with CONFIG_OF vs
> > > CONFIG_ACPI)
> > >
> > > If that doesn't work we could split it up into
> > >
> > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > or something of that sort?
> > >
> > > My point being, now that we have a bus, let's use it and develop drivers
> > > according to the Linux device model where possible :)
> >
> > Agree. This does make sense from my side too. DFL core provides the
> mechanism
> > to enumerate different IPs on FPGA, but each function driver needs to go to
> > related subsystem for review.  : )
> >
> > I understand that for FPGA case, it may have some additional logics for specific
> > purposes based on common altera spi master IP, then additional code for
> 
> I'm wondering if the additional logics are extensions for common spi-altera. Like
> the
> SPI_CORE_PARAMETER register, it is not within the register space of
> spi-altera,
> 
> 
>   |   |      +-------------+
>   |DFL|------| +--------+  |
>   |BUS|      | |SPI CORE|  |
>   |   |      | |PARAM   |  |
>   |   |      | +--------+  |
>   |   |      |             |
>   |   |      | +--------+  |   +-------+
>              | |Indirect|  |   |spi    |
>              | |access  +--+---|altera |
>              | |master  |  |   +-------+
>              | +--------+  |
>              +-------------+
> > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-xxxx.c
> 
> So is it proper we integrate this feature into spi-altera? Previously
> we have merged the dfl-n3000-nios, its spi part is very similar as
> this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> Could we borrow the idea, or could we just integrate this driver in
> dfl-n3000-nios?

Looks like those are enhancements of the IP. They can be applied even
other buses are used, not only for DFL, like PCI device or platform device,
right? then why not put related code together with the original IP?

The reason I suggested that function drivers which use DFL bus, still need
to go to related subsystem, because we know DFL quite well but may
not be the experts for every subsystem (e.g. SPI), and every IPs (e.g. 
Altera SPI Master). Altera SPI Master driver maintainer could give more
valuable suggestions on your question if this feature can be integrated
into spi-altera or not. : )

Hao

> 
> Thanks,
> Yilun
> 
> >
> > Thanks
> > Hao
> >
> > >
> > > Cheers,
> > > Moritz

Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Moritz Fischer]

On Thu, Apr 08, 2021 at 09:20:19AM +0000, Wu, Hao wrote:
> > On Thu, Apr 08, 2021 at 03:30:15PM +0800, Wu, Hao wrote:
> > > > > On Mon, 5 Apr 2021, Moritz Fischer wrote:
> > > > >
> > > > > > Hi Matthew,
> > > > > >
> > > > > > On Mon, Apr 05, 2021 at 04:53:00PM -0700,
> > > > matthew.gerlach@linux.intel.com wrote:
> > > > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > >
> > > > > > > This patch adds DFL bus driver for the Altera SPI Master
> > > > > > > controller.  The SPI master is connected to an Intel SPI Slave to
> > > > > > > Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> > > > > > >
> > > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/fpga/Kconfig          |   9 ++
> > > > > > >  drivers/fpga/Makefile         |   1 +
> > > > > > >  drivers/fpga/dfl-spi-altera.c | 221
> > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 231 insertions(+)
> > > > > > >  create mode 100644 drivers/fpga/dfl-spi-altera.c
> > > > > > >
> > > > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > > > > index d591dd9..0a86994 100644
> > > > > > > --- a/drivers/fpga/Kconfig
> > > > > > > +++ b/drivers/fpga/Kconfig
> > > > > > > @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > > > > > >    the card. It also instantiates the SPI master (spi-altera) for
> > > > > > >    the card's BMC (Board Management Controller).
> > > > > > >
> > > > > > > +config FPGA_DFL_SPI_ALTERA
> > > > > > > +tristate "FPGA DFL Altera SPI Master Driver"
> > > > > > > +depends on FPGA_DFL
> > > > > > > +select REGMAP
> > > > > > > +help
> > > > > > > +  This is a DFL bus driver for the Altera SPI master controller.
> > > > > > > +  The SPI master is connected to a SPI slave to Avalon Master
> > > > > > > +  bridge in a Intel MAX BMC.
> > > > > > > +
> > > > > > >  config FPGA_DFL_PCI
> > > > > > >  tristate "FPGA DFL PCIe Device Driver"
> > > > > > >  depends on PCI && FPGA_DFL
> > > > > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > > > > index 18dc9885..58a42ad 100644
> > > > > > > --- a/drivers/fpga/Makefile
> > > > > > > +++ b/drivers/fpga/Makefile
> > > > > > > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-
> > > > afu-dma-region.o
> > > > > > >  dfl-afu-objs += dfl-afu-error.o
> > > > > > >
> > > > > > >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)+= dfl-n3000-
> > > > nios.o
> > > > > > > +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)+= dfl-spi-altera.o
> > > > > > >
> > > > > > >  # Drivers for FPGAs which implement DFL
> > > > > > >  obj-$(CONFIG_FPGA_DFL_PCI)+= dfl-pci.o
> > > > > > > diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..9bec25fd
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/fpga/dfl-spi-altera.c
> > > > > > > @@ -0,0 +1,221 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/*
> > > > > > > + * DFL bus driver for Altera SPI Master
> > > > > > > + *
> > > > > > > + * Copyright (C) 2020 Intel Corporation, Inc.
> > > > > > > + *
> > > > > > > + * Authors:
> > > > > > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/types.h>
> > > > > > > +#include <linux/kernel.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/stddef.h>
> > > > > > > +#include <linux/errno.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/io.h>
> > > > > > > +#include <linux/bitfield.h>
> > > > > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > > > > +#include <linux/regmap.h>
> > > > > > > +#include <linux/spi/spi.h>
> > > > > > > +#include <linux/spi/altera.h>
> > > > > > > +#include <linux/dfl.h>
> > > > > > > +
> > > > > > > +struct dfl_altera_spi {
> > > > > > > +void __iomem *base;
> > > > > > > +struct regmap *regmap;
> > > > > > > +struct device *dev;
> > > > > > > +struct platform_device *altr_spi;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define SPI_CORE_PARAMETER      0x8
> > > > > > > +#define SHIFT_MODE              BIT_ULL(1)
> > > > > > > +#define SHIFT_MODE_MSB          0
> > > > > > > +#define SHIFT_MODE_LSB          1
> > > > > > > +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> > > > > > > +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> > > > > > > +#define CLK_POLARITY            BIT_ULL(14)
> > > > > > > +#define CLK_PHASE               BIT_ULL(15)
> > > > > > > +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> > > > > > > +#define SPI_CLK                 GENMASK_ULL(31, 22)
> > > > > > > +#define SPI_INDIRECT_ACC_OFST   0x10
> > > > > > > +
> > > > > > > +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> > > > > > > +#define INDIRECT_WR             BIT_ULL(8)
> > > > > > > +#define INDIRECT_RD             BIT_ULL(9)
> > > > > > > +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> > > > > > > +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> > > > > > > +#define INDIRECT_DEBUG          BIT_ULL(32)
> > > > > > > +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> > > > > > > +#define INDIRECT_TIMEOUT        10000
> > > > > > > +
> > > > > > > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > > > > > > + unsigned int *val)
> > > > > > > +{
> > > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > > +void __iomem *base = aspi->base;
> > > > > > > +int loops;
> > > > > > > +u64 v;
> > > > > > > +
> > > > > > > +writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> > > > > > > +
> > > > > > > +loops = 0;
> > > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> > > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > > +cpu_relax();
> > > > > > > +
> > > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > > +return -ETIME;
> > > > > > > +}
> > > > > > > +
> > > > > > > +v = readq(base + INDIRECT_RD_DATA);
> > > > > > > +
> > > > > > > +*val = v & INDIRECT_DATA_MASK;
> > > > > > > +
> > > > > > > +return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int indirect_bus_reg_write(void *context, unsigned int reg,
> > > > > > > +  unsigned int val)
> > > > > > > +{
> > > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > > +void __iomem *base = aspi->base;
> > > > > > > +int loops;
> > > > > > > +
> > > > > > > +writeq(val, base + INDIRECT_WR_DATA);
> > > > > > > +writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> > > > > > > +
> > > > > > > +loops = 0;
> > > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> > > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > > +cpu_relax();
> > > > > > > +
> > > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > > +return -ETIME;
> > > > > > > +}
> > > > > > > +return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static const struct regmap_config indirect_regbus_cfg = {
> > > > > > > +.reg_bits = 32,
> > > > > > > +.reg_stride = 4,
> > > > > > > +.val_bits = 32,
> > > > > > > +.fast_io = true,
> > > > > > > +
> > > > > > > +.reg_write = indirect_bus_reg_write,
> > > > > > > +.reg_read = indirect_bus_reg_read,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static struct spi_board_info m10_bmc_info = {
> > > > > > > +.modalias = "m10-d5005",
> > > > > > > +.max_speed_hz = 12500000,
> > > > > > > +.bus_num = 0,
> > > > > > > +.chip_select = 0,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static struct platform_device *create_cntrl(struct device *dev,
> > > > > > > +    void __iomem *base,
> > > > > > > +    struct spi_board_info *m10_info)
> > > > > > > +{
> > > > > > > +struct altera_spi_platform_data pdata;
> > > > > > > +struct platform_device_info pdevinfo;
> > > > > > > +u64 v;
> > > > > > > +
> > > > > > > +v = readq(base + SPI_CORE_PARAMETER);
> > > > > > > +
> > > > > > > +memset(&pdata, 0, sizeof(pdata));
> > > > > > > +pdata.mode_bits = SPI_CS_HIGH;
> > > > > > > +if (FIELD_GET(CLK_POLARITY, v))
> > > > > > > +pdata.mode_bits |= SPI_CPOL;
> > > > > > > +if (FIELD_GET(CLK_PHASE, v))
> > > > > > > +pdata.mode_bits |= SPI_CPHA;
> > > > > > > +
> > > > > > > +pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > > > > > > +pdata.bits_per_word_mask =
> > > > > > > +SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> > > > > > > +
> > > > > > > +pdata.num_devices = 1;
> > > > > > > +pdata.devices = m10_info;
> > > > > > > +
> > > > > > > +dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > > > > > > +pdata.num_chipselect, pdata.bits_per_word_mask,
> > > > > > > +pdata.mode_bits);
> > > > > > > +
> > > > > > > +memset(&pdevinfo, 0, sizeof(pdevinfo));
> > > > > > > +
> > > > > > > +pdevinfo.name = "subdev_spi_altera";
> > > > > > > +pdevinfo.id = PLATFORM_DEVID_AUTO;
> > > > > > > +pdevinfo.parent = dev;
> > > > > > > +pdevinfo.data = &pdata;
> > > > > > > +pdevinfo.size_data = sizeof(pdata);
> > > > > > > +
> > > > > > > +return platform_device_register_full(&pdevinfo);
> > > > > >
> > > > > > Should this be a SPI driver? I think looking at the UIO case we had
> > > > > > decided against this pattern?
> > > > >
> > > > > This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which
> > > > > uses this design pattern.  Is it okay to keep this design pattern for
> > > > > consistency?
> > > > >
> > > > >
> > > > > >
> > > > > > > +}
> > > > > > > +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> > > > > > > +{
> > > > > > > +struct device *dev = &dfl_dev->dev;
> > > > > > > +struct dfl_altera_spi *aspi;
> > > > > > > +
> > > > > > > +aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> > > > > > > +
> > > > > > > +if (!aspi)
> > > > > > > +return -ENOMEM;
> > > > > > > +
> > > > > > > +dev_set_drvdata(dev, aspi);
> > > > > > > +
> > > > > > > +aspi->dev = dev;
> > > > > > > +
> > > > > > > +aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > > > > > +
> > > > > > > +if (IS_ERR(aspi->base)) {
> > > > > > > +dev_err(dev, "%s get mem resource fail!\n", __func__);
> > > > > > > +return PTR_ERR(aspi->base);
> > > > > > > +}
> > > > > > > +
> > > > > > > +aspi->regmap = devm_regmap_init(dev, NULL, aspi,
> > > > &indirect_regbus_cfg);
> > > > > > > +if (IS_ERR(aspi->regmap))
> > > > > > > +return PTR_ERR(aspi->regmap);
> > > > > > > +
> > > > > > > +aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> > > > > > > +
> > > > > > > +if (IS_ERR(aspi->altr_spi)) {
> > > > > > > +dev_err(dev, "%s failed to create spi platform driver\n",
> > > > > > > +__func__);
> > > > > > > +return PTR_ERR(aspi->base);
> > > > > > > +}
> > > > > > > +
> > > > > > > +return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > > +{
> > > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > > +
> > > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > > +}
> > > > > > > +
> > > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > > +
> > > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > > +{ }
> > > > > > > +};
> > > > > >
> > > > > > Maybe you can extend the Altera SPI driver with this part?
> > > > >
> > > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_ related
> > > > > code.  Wouldn't moving this code to that file produce conflicts?
> > > >
> > > > I've seen other drivers support multiple busses, so it should be
> > > > possible, there might be nuances I'm missing in my brief look at this,
> > > > though.
> > > >
> > > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > > >
> > > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > > > they might be guarding against what you describe with CONFIG_OF vs
> > > > CONFIG_ACPI)
> > > >
> > > > If that doesn't work we could split it up into
> > > >
> > > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > > or something of that sort?
> > > >
> > > > My point being, now that we have a bus, let's use it and develop drivers
> > > > according to the Linux device model where possible :)
> > >
> > > Agree. This does make sense from my side too. DFL core provides the
> > mechanism
> > > to enumerate different IPs on FPGA, but each function driver needs to go to
> > > related subsystem for review.  : )
> > >
> > > I understand that for FPGA case, it may have some additional logics for specific
> > > purposes based on common altera spi master IP, then additional code for
> > 
> > I'm wondering if the additional logics are extensions for common spi-altera. Like
> > the
> > SPI_CORE_PARAMETER register, it is not within the register space of
> > spi-altera,
> > 
> > 
> >   |   |      +-------------+
> >   |DFL|------| +--------+  |
> >   |BUS|      | |SPI CORE|  |
> >   |   |      | |PARAM   |  |
> >   |   |      | +--------+  |
> >   |   |      |             |
> >   |   |      | +--------+  |   +-------+
> >              | |Indirect|  |   |spi    |
> >              | |access  +--+---|altera |
> >              | |master  |  |   +-------+
> >              | +--------+  |
> >              +-------------+
> > > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-xxxx.c
> > 
> > So is it proper we integrate this feature into spi-altera? Previously
> > we have merged the dfl-n3000-nios, its spi part is very similar as
> > this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> > Could we borrow the idea, or could we just integrate this driver in
> > dfl-n3000-nios?
> 
> Looks like those are enhancements of the IP. They can be applied even
> other buses are used, not only for DFL, like PCI device or platform device,
> right? then why not put related code together with the original IP?

Do you maybe need to extend struct dfl_device to have multiple mmio_res,
then?

Can DFL describe such a scenario?

That seems the logical step to support what's been drawn up there?

> 
> The reason I suggested that function drivers which use DFL bus, still need
> to go to related subsystem, because we know DFL quite well but may
> not be the experts for every subsystem (e.g. SPI), and every IPs (e.g. 
> Altera SPI Master). Altera SPI Master driver maintainer could give more
> valuable suggestions on your question if this feature can be integrated
> into spi-altera or not. : )
> 
> Hao
> 
> > 
> > Thanks,
> > Yilun
> > 
> > >
> > > Thanks
> > > Hao
> > >
> > > >
> > > > Cheers,
> > > > Moritz

Cheers,
Moritz

Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Xu Yilun]

On Thu, Apr 08, 2021 at 11:53:06AM -0700, Moritz Fischer wrote:
> On Thu, Apr 08, 2021 at 09:20:19AM +0000, Wu, Hao wrote:
> > > On Thu, Apr 08, 2021 at 03:30:15PM +0800, Wu, Hao wrote:
> > > > > > On Mon, 5 Apr 2021, Moritz Fischer wrote:
> > > > > >
> > > > > > > Hi Matthew,
> > > > > > >
> > > > > > > On Mon, Apr 05, 2021 at 04:53:00PM -0700,
> > > > > matthew.gerlach@linux.intel.com wrote:
> > > > > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > >
> > > > > > > > This patch adds DFL bus driver for the Altera SPI Master
> > > > > > > > controller.  The SPI master is connected to an Intel SPI Slave to
> > > > > > > > Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> > > > > > > >
> > > > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/fpga/Kconfig          |   9 ++
> > > > > > > >  drivers/fpga/Makefile         |   1 +
> > > > > > > >  drivers/fpga/dfl-spi-altera.c | 221
> > > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  3 files changed, 231 insertions(+)
> > > > > > > >  create mode 100644 drivers/fpga/dfl-spi-altera.c
> > > > > > > >
> > > > > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > > > > > index d591dd9..0a86994 100644
> > > > > > > > --- a/drivers/fpga/Kconfig
> > > > > > > > +++ b/drivers/fpga/Kconfig
> > > > > > > > @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > > > > > > >    the card. It also instantiates the SPI master (spi-altera) for
> > > > > > > >    the card's BMC (Board Management Controller).
> > > > > > > >
> > > > > > > > +config FPGA_DFL_SPI_ALTERA
> > > > > > > > +tristate "FPGA DFL Altera SPI Master Driver"
> > > > > > > > +depends on FPGA_DFL
> > > > > > > > +select REGMAP
> > > > > > > > +help
> > > > > > > > +  This is a DFL bus driver for the Altera SPI master controller.
> > > > > > > > +  The SPI master is connected to a SPI slave to Avalon Master
> > > > > > > > +  bridge in a Intel MAX BMC.
> > > > > > > > +
> > > > > > > >  config FPGA_DFL_PCI
> > > > > > > >  tristate "FPGA DFL PCIe Device Driver"
> > > > > > > >  depends on PCI && FPGA_DFL
> > > > > > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > > > > > index 18dc9885..58a42ad 100644
> > > > > > > > --- a/drivers/fpga/Makefile
> > > > > > > > +++ b/drivers/fpga/Makefile
> > > > > > > > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-
> > > > > afu-dma-region.o
> > > > > > > >  dfl-afu-objs += dfl-afu-error.o
> > > > > > > >
> > > > > > > >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)+= dfl-n3000-
> > > > > nios.o
> > > > > > > > +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)+= dfl-spi-altera.o
> > > > > > > >
> > > > > > > >  # Drivers for FPGAs which implement DFL
> > > > > > > >  obj-$(CONFIG_FPGA_DFL_PCI)+= dfl-pci.o
> > > > > > > > diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..9bec25fd
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/fpga/dfl-spi-altera.c
> > > > > > > > @@ -0,0 +1,221 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > +/*
> > > > > > > > + * DFL bus driver for Altera SPI Master
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2020 Intel Corporation, Inc.
> > > > > > > > + *
> > > > > > > > + * Authors:
> > > > > > > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/types.h>
> > > > > > > > +#include <linux/kernel.h>
> > > > > > > > +#include <linux/module.h>
> > > > > > > > +#include <linux/stddef.h>
> > > > > > > > +#include <linux/errno.h>
> > > > > > > > +#include <linux/platform_device.h>
> > > > > > > > +#include <linux/io.h>
> > > > > > > > +#include <linux/bitfield.h>
> > > > > > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > > > > > +#include <linux/regmap.h>
> > > > > > > > +#include <linux/spi/spi.h>
> > > > > > > > +#include <linux/spi/altera.h>
> > > > > > > > +#include <linux/dfl.h>
> > > > > > > > +
> > > > > > > > +struct dfl_altera_spi {
> > > > > > > > +void __iomem *base;
> > > > > > > > +struct regmap *regmap;
> > > > > > > > +struct device *dev;
> > > > > > > > +struct platform_device *altr_spi;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +#define SPI_CORE_PARAMETER      0x8
> > > > > > > > +#define SHIFT_MODE              BIT_ULL(1)
> > > > > > > > +#define SHIFT_MODE_MSB          0
> > > > > > > > +#define SHIFT_MODE_LSB          1
> > > > > > > > +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> > > > > > > > +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> > > > > > > > +#define CLK_POLARITY            BIT_ULL(14)
> > > > > > > > +#define CLK_PHASE               BIT_ULL(15)
> > > > > > > > +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> > > > > > > > +#define SPI_CLK                 GENMASK_ULL(31, 22)
> > > > > > > > +#define SPI_INDIRECT_ACC_OFST   0x10
> > > > > > > > +
> > > > > > > > +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> > > > > > > > +#define INDIRECT_WR             BIT_ULL(8)
> > > > > > > > +#define INDIRECT_RD             BIT_ULL(9)
> > > > > > > > +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> > > > > > > > +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> > > > > > > > +#define INDIRECT_DEBUG          BIT_ULL(32)
> > > > > > > > +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> > > > > > > > +#define INDIRECT_TIMEOUT        10000
> > > > > > > > +
> > > > > > > > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > > > > > > > + unsigned int *val)
> > > > > > > > +{
> > > > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > > > +void __iomem *base = aspi->base;
> > > > > > > > +int loops;
> > > > > > > > +u64 v;
> > > > > > > > +
> > > > > > > > +writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> > > > > > > > +
> > > > > > > > +loops = 0;
> > > > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> > > > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > > > +cpu_relax();
> > > > > > > > +
> > > > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > > > +return -ETIME;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +v = readq(base + INDIRECT_RD_DATA);
> > > > > > > > +
> > > > > > > > +*val = v & INDIRECT_DATA_MASK;
> > > > > > > > +
> > > > > > > > +return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int indirect_bus_reg_write(void *context, unsigned int reg,
> > > > > > > > +  unsigned int val)
> > > > > > > > +{
> > > > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > > > +void __iomem *base = aspi->base;
> > > > > > > > +int loops;
> > > > > > > > +
> > > > > > > > +writeq(val, base + INDIRECT_WR_DATA);
> > > > > > > > +writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> > > > > > > > +
> > > > > > > > +loops = 0;
> > > > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> > > > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > > > +cpu_relax();
> > > > > > > > +
> > > > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > > > +return -ETIME;
> > > > > > > > +}
> > > > > > > > +return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static const struct regmap_config indirect_regbus_cfg = {
> > > > > > > > +.reg_bits = 32,
> > > > > > > > +.reg_stride = 4,
> > > > > > > > +.val_bits = 32,
> > > > > > > > +.fast_io = true,
> > > > > > > > +
> > > > > > > > +.reg_write = indirect_bus_reg_write,
> > > > > > > > +.reg_read = indirect_bus_reg_read,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static struct spi_board_info m10_bmc_info = {
> > > > > > > > +.modalias = "m10-d5005",
> > > > > > > > +.max_speed_hz = 12500000,
> > > > > > > > +.bus_num = 0,
> > > > > > > > +.chip_select = 0,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static struct platform_device *create_cntrl(struct device *dev,
> > > > > > > > +    void __iomem *base,
> > > > > > > > +    struct spi_board_info *m10_info)
> > > > > > > > +{
> > > > > > > > +struct altera_spi_platform_data pdata;
> > > > > > > > +struct platform_device_info pdevinfo;
> > > > > > > > +u64 v;
> > > > > > > > +
> > > > > > > > +v = readq(base + SPI_CORE_PARAMETER);
> > > > > > > > +
> > > > > > > > +memset(&pdata, 0, sizeof(pdata));
> > > > > > > > +pdata.mode_bits = SPI_CS_HIGH;
> > > > > > > > +if (FIELD_GET(CLK_POLARITY, v))
> > > > > > > > +pdata.mode_bits |= SPI_CPOL;
> > > > > > > > +if (FIELD_GET(CLK_PHASE, v))
> > > > > > > > +pdata.mode_bits |= SPI_CPHA;
> > > > > > > > +
> > > > > > > > +pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > > > > > > > +pdata.bits_per_word_mask =
> > > > > > > > +SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> > > > > > > > +
> > > > > > > > +pdata.num_devices = 1;
> > > > > > > > +pdata.devices = m10_info;
> > > > > > > > +
> > > > > > > > +dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > > > > > > > +pdata.num_chipselect, pdata.bits_per_word_mask,
> > > > > > > > +pdata.mode_bits);
> > > > > > > > +
> > > > > > > > +memset(&pdevinfo, 0, sizeof(pdevinfo));
> > > > > > > > +
> > > > > > > > +pdevinfo.name = "subdev_spi_altera";
> > > > > > > > +pdevinfo.id = PLATFORM_DEVID_AUTO;
> > > > > > > > +pdevinfo.parent = dev;
> > > > > > > > +pdevinfo.data = &pdata;
> > > > > > > > +pdevinfo.size_data = sizeof(pdata);
> > > > > > > > +
> > > > > > > > +return platform_device_register_full(&pdevinfo);
> > > > > > >
> > > > > > > Should this be a SPI driver? I think looking at the UIO case we had
> > > > > > > decided against this pattern?
> > > > > >
> > > > > > This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which
> > > > > > uses this design pattern.  Is it okay to keep this design pattern for
> > > > > > consistency?
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > +}
> > > > > > > > +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> > > > > > > > +{
> > > > > > > > +struct device *dev = &dfl_dev->dev;
> > > > > > > > +struct dfl_altera_spi *aspi;
> > > > > > > > +
> > > > > > > > +aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> > > > > > > > +
> > > > > > > > +if (!aspi)
> > > > > > > > +return -ENOMEM;
> > > > > > > > +
> > > > > > > > +dev_set_drvdata(dev, aspi);
> > > > > > > > +
> > > > > > > > +aspi->dev = dev;
> > > > > > > > +
> > > > > > > > +aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > > > > > > +
> > > > > > > > +if (IS_ERR(aspi->base)) {
> > > > > > > > +dev_err(dev, "%s get mem resource fail!\n", __func__);
> > > > > > > > +return PTR_ERR(aspi->base);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +aspi->regmap = devm_regmap_init(dev, NULL, aspi,
> > > > > &indirect_regbus_cfg);
> > > > > > > > +if (IS_ERR(aspi->regmap))
> > > > > > > > +return PTR_ERR(aspi->regmap);
> > > > > > > > +
> > > > > > > > +aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> > > > > > > > +
> > > > > > > > +if (IS_ERR(aspi->altr_spi)) {
> > > > > > > > +dev_err(dev, "%s failed to create spi platform driver\n",
> > > > > > > > +__func__);
> > > > > > > > +return PTR_ERR(aspi->base);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > > > +{
> > > > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > > > +
> > > > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > > > +
> > > > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > > > +{ }
> > > > > > > > +};
> > > > > > >
> > > > > > > Maybe you can extend the Altera SPI driver with this part?
> > > > > >
> > > > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_ related
> > > > > > code.  Wouldn't moving this code to that file produce conflicts?
> > > > >
> > > > > I've seen other drivers support multiple busses, so it should be
> > > > > possible, there might be nuances I'm missing in my brief look at this,
> > > > > though.
> > > > >
> > > > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > > > >
> > > > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > > > > they might be guarding against what you describe with CONFIG_OF vs
> > > > > CONFIG_ACPI)
> > > > >
> > > > > If that doesn't work we could split it up into
> > > > >
> > > > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > > > or something of that sort?
> > > > >
> > > > > My point being, now that we have a bus, let's use it and develop drivers
> > > > > according to the Linux device model where possible :)
> > > >
> > > > Agree. This does make sense from my side too. DFL core provides the
> > > mechanism
> > > > to enumerate different IPs on FPGA, but each function driver needs to go to
> > > > related subsystem for review.  : )
> > > >
> > > > I understand that for FPGA case, it may have some additional logics for specific
> > > > purposes based on common altera spi master IP, then additional code for
> > > 
> > > I'm wondering if the additional logics are extensions for common spi-altera. Like
> > > the
> > > SPI_CORE_PARAMETER register, it is not within the register space of
> > > spi-altera,
> > > 
> > > 
> > >   |   |      +-------------+
> > >   |DFL|------| +--------+  |
> > >   |BUS|      | |SPI CORE|  |
> > >   |   |      | |PARAM   |  |
> > >   |   |      | +--------+  |
> > >   |   |      |             |
> > >   |   |      | +--------+  |   +-------+
> > >              | |Indirect|  |   |spi    |
> > >              | |access  +--+---|altera |
> > >              | |master  |  |   +-------+
> > >              | +--------+  |
> > >              +-------------+
> > > > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-xxxx.c
> > > 
> > > So is it proper we integrate this feature into spi-altera? Previously
> > > we have merged the dfl-n3000-nios, its spi part is very similar as
> > > this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> > > Could we borrow the idea, or could we just integrate this driver in
> > > dfl-n3000-nios?
> > 
> > Looks like those are enhancements of the IP. They can be applied even
> > other buses are used, not only for DFL, like PCI device or platform device,
> > right? then why not put related code together with the original IP?
> 
> Do you maybe need to extend struct dfl_device to have multiple mmio_res,
> then?
> 
> Can DFL describe such a scenario?

The current version of DFL is not describing multiple mmio regions
within features, but yes we are on working on some extention to get
it supported.

> 
> That seems the logical step to support what's been drawn up there?

Could you describe more about your idea? It's valuable discussion for
DFL design, not only for this case.

Thanks,
Yilun

> 
> > 
> > The reason I suggested that function drivers which use DFL bus, still need
> > to go to related subsystem, because we know DFL quite well but may
> > not be the experts for every subsystem (e.g. SPI), and every IPs (e.g. 
> > Altera SPI Master). Altera SPI Master driver maintainer could give more
> > valuable suggestions on your question if this feature can be integrated
> > into spi-altera or not. : )
> > 
> > Hao
> > 
> > > 
> > > Thanks,
> > > Yilun
> > > 
> > > >
> > > > Thanks
> > > > Hao
> > > >
> > > > >
> > > > > Cheers,
> > > > > Moritz
> 
> Cheers,
> Moritz

RE: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Wu, Hao]

> On Thu, Apr 08, 2021 at 09:20:19AM +0000, Wu, Hao wrote:
> > > On Thu, Apr 08, 2021 at 03:30:15PM +0800, Wu, Hao wrote:
> > > > > > On Mon, 5 Apr 2021, Moritz Fischer wrote:
> > > > > >
> > > > > > > Hi Matthew,
> > > > > > >
> > > > > > > On Mon, Apr 05, 2021 at 04:53:00PM -0700,
> > > > > matthew.gerlach@linux.intel.com wrote:
> > > > > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > >
> > > > > > > > This patch adds DFL bus driver for the Altera SPI Master
> > > > > > > > controller.  The SPI master is connected to an Intel SPI Slave to
> > > > > > > > Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> > > > > > > >
> > > > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/fpga/Kconfig          |   9 ++
> > > > > > > >  drivers/fpga/Makefile         |   1 +
> > > > > > > >  drivers/fpga/dfl-spi-altera.c | 221
> > > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  3 files changed, 231 insertions(+)
> > > > > > > >  create mode 100644 drivers/fpga/dfl-spi-altera.c
> > > > > > > >
> > > > > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > > > > > index d591dd9..0a86994 100644
> > > > > > > > --- a/drivers/fpga/Kconfig
> > > > > > > > +++ b/drivers/fpga/Kconfig
> > > > > > > > @@ -210,6 +210,15 @@ config
> FPGA_DFL_NIOS_INTEL_PAC_N3000
> > > > > > > >    the card. It also instantiates the SPI master (spi-altera) for
> > > > > > > >    the card's BMC (Board Management Controller).
> > > > > > > >
> > > > > > > > +config FPGA_DFL_SPI_ALTERA
> > > > > > > > +tristate "FPGA DFL Altera SPI Master Driver"
> > > > > > > > +depends on FPGA_DFL
> > > > > > > > +select REGMAP
> > > > > > > > +help
> > > > > > > > +  This is a DFL bus driver for the Altera SPI master controller.
> > > > > > > > +  The SPI master is connected to a SPI slave to Avalon Master
> > > > > > > > +  bridge in a Intel MAX BMC.
> > > > > > > > +
> > > > > > > >  config FPGA_DFL_PCI
> > > > > > > >  tristate "FPGA DFL PCIe Device Driver"
> > > > > > > >  depends on PCI && FPGA_DFL
> > > > > > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > > > > > index 18dc9885..58a42ad 100644
> > > > > > > > --- a/drivers/fpga/Makefile
> > > > > > > > +++ b/drivers/fpga/Makefile
> > > > > > > > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o
> dfl-
> > > > > afu-dma-region.o
> > > > > > > >  dfl-afu-objs += dfl-afu-error.o
> > > > > > > >
> > > > > > > >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)+= dfl-n3000-
> > > > > nios.o
> > > > > > > > +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)+= dfl-spi-altera.o
> > > > > > > >
> > > > > > > >  # Drivers for FPGAs which implement DFL
> > > > > > > >  obj-$(CONFIG_FPGA_DFL_PCI)+= dfl-pci.o
> > > > > > > > diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-
> altera.c
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..9bec25fd
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/fpga/dfl-spi-altera.c
> > > > > > > > @@ -0,0 +1,221 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > +/*
> > > > > > > > + * DFL bus driver for Altera SPI Master
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2020 Intel Corporation, Inc.
> > > > > > > > + *
> > > > > > > > + * Authors:
> > > > > > > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/types.h>
> > > > > > > > +#include <linux/kernel.h>
> > > > > > > > +#include <linux/module.h>
> > > > > > > > +#include <linux/stddef.h>
> > > > > > > > +#include <linux/errno.h>
> > > > > > > > +#include <linux/platform_device.h>
> > > > > > > > +#include <linux/io.h>
> > > > > > > > +#include <linux/bitfield.h>
> > > > > > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > > > > > +#include <linux/regmap.h>
> > > > > > > > +#include <linux/spi/spi.h>
> > > > > > > > +#include <linux/spi/altera.h>
> > > > > > > > +#include <linux/dfl.h>
> > > > > > > > +
> > > > > > > > +struct dfl_altera_spi {
> > > > > > > > +void __iomem *base;
> > > > > > > > +struct regmap *regmap;
> > > > > > > > +struct device *dev;
> > > > > > > > +struct platform_device *altr_spi;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +#define SPI_CORE_PARAMETER      0x8
> > > > > > > > +#define SHIFT_MODE              BIT_ULL(1)
> > > > > > > > +#define SHIFT_MODE_MSB          0
> > > > > > > > +#define SHIFT_MODE_LSB          1
> > > > > > > > +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> > > > > > > > +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> > > > > > > > +#define CLK_POLARITY            BIT_ULL(14)
> > > > > > > > +#define CLK_PHASE               BIT_ULL(15)
> > > > > > > > +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> > > > > > > > +#define SPI_CLK                 GENMASK_ULL(31, 22)
> > > > > > > > +#define SPI_INDIRECT_ACC_OFST   0x10
> > > > > > > > +
> > > > > > > > +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> > > > > > > > +#define INDIRECT_WR             BIT_ULL(8)
> > > > > > > > +#define INDIRECT_RD             BIT_ULL(9)
> > > > > > > > +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> > > > > > > > +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> > > > > > > > +#define INDIRECT_DEBUG          BIT_ULL(32)
> > > > > > > > +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> > > > > > > > +#define INDIRECT_TIMEOUT        10000
> > > > > > > > +
> > > > > > > > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > > > > > > > + unsigned int *val)
> > > > > > > > +{
> > > > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > > > +void __iomem *base = aspi->base;
> > > > > > > > +int loops;
> > > > > > > > +u64 v;
> > > > > > > > +
> > > > > > > > +writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> > > > > > > > +
> > > > > > > > +loops = 0;
> > > > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> > > > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > > > +cpu_relax();
> > > > > > > > +
> > > > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > > > +return -ETIME;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +v = readq(base + INDIRECT_RD_DATA);
> > > > > > > > +
> > > > > > > > +*val = v & INDIRECT_DATA_MASK;
> > > > > > > > +
> > > > > > > > +return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int indirect_bus_reg_write(void *context, unsigned int reg,
> > > > > > > > +  unsigned int val)
> > > > > > > > +{
> > > > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > > > +void __iomem *base = aspi->base;
> > > > > > > > +int loops;
> > > > > > > > +
> > > > > > > > +writeq(val, base + INDIRECT_WR_DATA);
> > > > > > > > +writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> > > > > > > > +
> > > > > > > > +loops = 0;
> > > > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> > > > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > > > +cpu_relax();
> > > > > > > > +
> > > > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > > > +return -ETIME;
> > > > > > > > +}
> > > > > > > > +return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static const struct regmap_config indirect_regbus_cfg = {
> > > > > > > > +.reg_bits = 32,
> > > > > > > > +.reg_stride = 4,
> > > > > > > > +.val_bits = 32,
> > > > > > > > +.fast_io = true,
> > > > > > > > +
> > > > > > > > +.reg_write = indirect_bus_reg_write,
> > > > > > > > +.reg_read = indirect_bus_reg_read,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static struct spi_board_info m10_bmc_info = {
> > > > > > > > +.modalias = "m10-d5005",
> > > > > > > > +.max_speed_hz = 12500000,
> > > > > > > > +.bus_num = 0,
> > > > > > > > +.chip_select = 0,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static struct platform_device *create_cntrl(struct device *dev,
> > > > > > > > +    void __iomem *base,
> > > > > > > > +    struct spi_board_info *m10_info)
> > > > > > > > +{
> > > > > > > > +struct altera_spi_platform_data pdata;
> > > > > > > > +struct platform_device_info pdevinfo;
> > > > > > > > +u64 v;
> > > > > > > > +
> > > > > > > > +v = readq(base + SPI_CORE_PARAMETER);
> > > > > > > > +
> > > > > > > > +memset(&pdata, 0, sizeof(pdata));
> > > > > > > > +pdata.mode_bits = SPI_CS_HIGH;
> > > > > > > > +if (FIELD_GET(CLK_POLARITY, v))
> > > > > > > > +pdata.mode_bits |= SPI_CPOL;
> > > > > > > > +if (FIELD_GET(CLK_PHASE, v))
> > > > > > > > +pdata.mode_bits |= SPI_CPHA;
> > > > > > > > +
> > > > > > > > +pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > > > > > > > +pdata.bits_per_word_mask =
> > > > > > > > +SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> > > > > > > > +
> > > > > > > > +pdata.num_devices = 1;
> > > > > > > > +pdata.devices = m10_info;
> > > > > > > > +
> > > > > > > > +dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > > > > > > > +pdata.num_chipselect, pdata.bits_per_word_mask,
> > > > > > > > +pdata.mode_bits);
> > > > > > > > +
> > > > > > > > +memset(&pdevinfo, 0, sizeof(pdevinfo));
> > > > > > > > +
> > > > > > > > +pdevinfo.name = "subdev_spi_altera";
> > > > > > > > +pdevinfo.id = PLATFORM_DEVID_AUTO;
> > > > > > > > +pdevinfo.parent = dev;
> > > > > > > > +pdevinfo.data = &pdata;
> > > > > > > > +pdevinfo.size_data = sizeof(pdata);
> > > > > > > > +
> > > > > > > > +return platform_device_register_full(&pdevinfo);
> > > > > > >
> > > > > > > Should this be a SPI driver? I think looking at the UIO case we had
> > > > > > > decided against this pattern?
> > > > > >
> > > > > > This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which
> > > > > > uses this design pattern.  Is it okay to keep this design pattern for
> > > > > > consistency?
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > +}
> > > > > > > > +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> > > > > > > > +{
> > > > > > > > +struct device *dev = &dfl_dev->dev;
> > > > > > > > +struct dfl_altera_spi *aspi;
> > > > > > > > +
> > > > > > > > +aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> > > > > > > > +
> > > > > > > > +if (!aspi)
> > > > > > > > +return -ENOMEM;
> > > > > > > > +
> > > > > > > > +dev_set_drvdata(dev, aspi);
> > > > > > > > +
> > > > > > > > +aspi->dev = dev;
> > > > > > > > +
> > > > > > > > +aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > > > > > > +
> > > > > > > > +if (IS_ERR(aspi->base)) {
> > > > > > > > +dev_err(dev, "%s get mem resource fail!\n", __func__);
> > > > > > > > +return PTR_ERR(aspi->base);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +aspi->regmap = devm_regmap_init(dev, NULL, aspi,
> > > > > &indirect_regbus_cfg);
> > > > > > > > +if (IS_ERR(aspi->regmap))
> > > > > > > > +return PTR_ERR(aspi->regmap);
> > > > > > > > +
> > > > > > > > +aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> > > > > > > > +
> > > > > > > > +if (IS_ERR(aspi->altr_spi)) {
> > > > > > > > +dev_err(dev, "%s failed to create spi platform driver\n",
> > > > > > > > +__func__);
> > > > > > > > +return PTR_ERR(aspi->base);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > > > +{
> > > > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > > > +
> > > > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > > > +
> > > > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > > > +{ }
> > > > > > > > +};
> > > > > > >
> > > > > > > Maybe you can extend the Altera SPI driver with this part?
> > > > > >
> > > > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_
> related
> > > > > > code.  Wouldn't moving this code to that file produce conflicts?
> > > > >
> > > > > I've seen other drivers support multiple busses, so it should be
> > > > > possible, there might be nuances I'm missing in my brief look at this,
> > > > > though.
> > > > >
> > > > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > > > >
> > > > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > > > > they might be guarding against what you describe with CONFIG_OF vs
> > > > > CONFIG_ACPI)
> > > > >
> > > > > If that doesn't work we could split it up into
> > > > >
> > > > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > > > or something of that sort?
> > > > >
> > > > > My point being, now that we have a bus, let's use it and develop drivers
> > > > > according to the Linux device model where possible :)
> > > >
> > > > Agree. This does make sense from my side too. DFL core provides the
> > > mechanism
> > > > to enumerate different IPs on FPGA, but each function driver needs to go
> to
> > > > related subsystem for review.  : )
> > > >
> > > > I understand that for FPGA case, it may have some additional logics for
> specific
> > > > purposes based on common altera spi master IP, then additional code for
> > >
> > > I'm wondering if the additional logics are extensions for common spi-altera.
> Like
> > > the
> > > SPI_CORE_PARAMETER register, it is not within the register space of
> > > spi-altera,
> > >
> > >
> > >   |   |      +-------------+
> > >   |DFL|------| +--------+  |
> > >   |BUS|      | |SPI CORE|  |
> > >   |   |      | |PARAM   |  |
> > >   |   |      | +--------+  |
> > >   |   |      |             |
> > >   |   |      | +--------+  |   +-------+
> > >              | |Indirect|  |   |spi    |
> > >              | |access  +--+---|altera |
> > >              | |master  |  |   +-------+
> > >              | +--------+  |
> > >              +-------------+
> > > > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-
> xxxx.c
> > >
> > > So is it proper we integrate this feature into spi-altera? Previously
> > > we have merged the dfl-n3000-nios, its spi part is very similar as
> > > this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> > > Could we borrow the idea, or could we just integrate this driver in
> > > dfl-n3000-nios?
> >
> > Looks like those are enhancements of the IP. They can be applied even
> > other buses are used, not only for DFL, like PCI device or platform device,
> > right? then why not put related code together with the original IP?
> 
> Do you maybe need to extend struct dfl_device to have multiple mmio_res,
> then?
> 
> Can DFL describe such a scenario?
> 
> That seems the logical step to support what's been drawn up there?

Current DFH version 0 only supports 1 mmio range, then everything has to
be put into the same mmio range but different offsets I think. DFH doesn't
care about what are inside this range, even it's possible to put multiple IPs
into one range ( DFH doesn't prevent this usage, but then mfd or something
similar need to be used in device driver on DFL bus).

It's should be fine to add multiple mmio ranges support for one DFH device
which requires a new version DFH, so that it can match the platform devices
which have multiple mmio resources or just like pci devices which have multiple
bars, but it is not something must to have for above case I think. : )

Thanks
Hao

> 
> >
> > The reason I suggested that function drivers which use DFL bus, still need
> > to go to related subsystem, because we know DFL quite well but may
> > not be the experts for every subsystem (e.g. SPI), and every IPs (e.g.
> > Altera SPI Master). Altera SPI Master driver maintainer could give more
> > valuable suggestions on your question if this feature can be integrated
> > into spi-altera or not. : )
> >
> > Hao
> >
> > >
> > > Thanks,
> > > Yilun
> > >
> > > >
> > > > Thanks
> > > > Hao
> > > >
> > > > >
> > > > > Cheers,
> > > > > Moritz
> 
> Cheers,
> Moritz

Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Xu Yilun]

On Thu, Apr 08, 2021 at 05:20:19PM +0800, Wu, Hao wrote:
> > On Thu, Apr 08, 2021 at 03:30:15PM +0800, Wu, Hao wrote:
> > > > > On Mon, 5 Apr 2021, Moritz Fischer wrote:
> > > > >
> > > > > > Hi Matthew,
> > > > > >
> > > > > > On Mon, Apr 05, 2021 at 04:53:00PM -0700,
> > > > matthew.gerlach@linux.intel.com wrote:
> > > > > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > >
> > > > > > > This patch adds DFL bus driver for the Altera SPI Master
> > > > > > > controller.  The SPI master is connected to an Intel SPI Slave to
> > > > > > > Avalon Master Bridge, inside an Intel MAX10 BMC Chip.
> > > > > > >
> > > > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > ---
> > > > > > >  drivers/fpga/Kconfig          |   9 ++
> > > > > > >  drivers/fpga/Makefile         |   1 +
> > > > > > >  drivers/fpga/dfl-spi-altera.c | 221
> > > > ++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 231 insertions(+)
> > > > > > >  create mode 100644 drivers/fpga/dfl-spi-altera.c
> > > > > > >
> > > > > > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > > > > > index d591dd9..0a86994 100644
> > > > > > > --- a/drivers/fpga/Kconfig
> > > > > > > +++ b/drivers/fpga/Kconfig
> > > > > > > @@ -210,6 +210,15 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > > > > > >    the card. It also instantiates the SPI master (spi-altera) for
> > > > > > >    the card's BMC (Board Management Controller).
> > > > > > >
> > > > > > > +config FPGA_DFL_SPI_ALTERA
> > > > > > > +tristate "FPGA DFL Altera SPI Master Driver"
> > > > > > > +depends on FPGA_DFL
> > > > > > > +select REGMAP
> > > > > > > +help
> > > > > > > +  This is a DFL bus driver for the Altera SPI master controller.
> > > > > > > +  The SPI master is connected to a SPI slave to Avalon Master
> > > > > > > +  bridge in a Intel MAX BMC.
> > > > > > > +
> > > > > > >  config FPGA_DFL_PCI
> > > > > > >  tristate "FPGA DFL PCIe Device Driver"
> > > > > > >  depends on PCI && FPGA_DFL
> > > > > > > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > > > > > > index 18dc9885..58a42ad 100644
> > > > > > > --- a/drivers/fpga/Makefile
> > > > > > > +++ b/drivers/fpga/Makefile
> > > > > > > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-
> > > > afu-dma-region.o
> > > > > > >  dfl-afu-objs += dfl-afu-error.o
> > > > > > >
> > > > > > >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)+= dfl-n3000-
> > > > nios.o
> > > > > > > +obj-$(CONFIG_FPGA_DFL_SPI_ALTERA)+= dfl-spi-altera.o
> > > > > > >
> > > > > > >  # Drivers for FPGAs which implement DFL
> > > > > > >  obj-$(CONFIG_FPGA_DFL_PCI)+= dfl-pci.o
> > > > > > > diff --git a/drivers/fpga/dfl-spi-altera.c b/drivers/fpga/dfl-spi-altera.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..9bec25fd
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/fpga/dfl-spi-altera.c
> > > > > > > @@ -0,0 +1,221 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/*
> > > > > > > + * DFL bus driver for Altera SPI Master
> > > > > > > + *
> > > > > > > + * Copyright (C) 2020 Intel Corporation, Inc.
> > > > > > > + *
> > > > > > > + * Authors:
> > > > > > > + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/types.h>
> > > > > > > +#include <linux/kernel.h>
> > > > > > > +#include <linux/module.h>
> > > > > > > +#include <linux/stddef.h>
> > > > > > > +#include <linux/errno.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/io.h>
> > > > > > > +#include <linux/bitfield.h>
> > > > > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > > > > > > +#include <linux/regmap.h>
> > > > > > > +#include <linux/spi/spi.h>
> > > > > > > +#include <linux/spi/altera.h>
> > > > > > > +#include <linux/dfl.h>
> > > > > > > +
> > > > > > > +struct dfl_altera_spi {
> > > > > > > +void __iomem *base;
> > > > > > > +struct regmap *regmap;
> > > > > > > +struct device *dev;
> > > > > > > +struct platform_device *altr_spi;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define SPI_CORE_PARAMETER      0x8
> > > > > > > +#define SHIFT_MODE              BIT_ULL(1)
> > > > > > > +#define SHIFT_MODE_MSB          0
> > > > > > > +#define SHIFT_MODE_LSB          1
> > > > > > > +#define DATA_WIDTH              GENMASK_ULL(7, 2)
> > > > > > > +#define NUM_CHIPSELECT          GENMASK_ULL(13, 8)
> > > > > > > +#define CLK_POLARITY            BIT_ULL(14)
> > > > > > > +#define CLK_PHASE               BIT_ULL(15)
> > > > > > > +#define PERIPHERAL_ID           GENMASK_ULL(47, 32)
> > > > > > > +#define SPI_CLK                 GENMASK_ULL(31, 22)
> > > > > > > +#define SPI_INDIRECT_ACC_OFST   0x10
> > > > > > > +
> > > > > > > +#define INDIRECT_ADDR           (SPI_INDIRECT_ACC_OFST+0x0)
> > > > > > > +#define INDIRECT_WR             BIT_ULL(8)
> > > > > > > +#define INDIRECT_RD             BIT_ULL(9)
> > > > > > > +#define INDIRECT_RD_DATA        (SPI_INDIRECT_ACC_OFST+0x8)
> > > > > > > +#define INDIRECT_DATA_MASK      GENMASK_ULL(31, 0)
> > > > > > > +#define INDIRECT_DEBUG          BIT_ULL(32)
> > > > > > > +#define INDIRECT_WR_DATA        (SPI_INDIRECT_ACC_OFST+0x10)
> > > > > > > +#define INDIRECT_TIMEOUT        10000
> > > > > > > +
> > > > > > > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > > > > > > + unsigned int *val)
> > > > > > > +{
> > > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > > +void __iomem *base = aspi->base;
> > > > > > > +int loops;
> > > > > > > +u64 v;
> > > > > > > +
> > > > > > > +writeq((reg >> 2) | INDIRECT_RD, base + INDIRECT_ADDR);
> > > > > > > +
> > > > > > > +loops = 0;
> > > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_RD) &&
> > > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > > +cpu_relax();
> > > > > > > +
> > > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > > +return -ETIME;
> > > > > > > +}
> > > > > > > +
> > > > > > > +v = readq(base + INDIRECT_RD_DATA);
> > > > > > > +
> > > > > > > +*val = v & INDIRECT_DATA_MASK;
> > > > > > > +
> > > > > > > +return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int indirect_bus_reg_write(void *context, unsigned int reg,
> > > > > > > +  unsigned int val)
> > > > > > > +{
> > > > > > > +struct dfl_altera_spi *aspi = context;
> > > > > > > +void __iomem *base = aspi->base;
> > > > > > > +int loops;
> > > > > > > +
> > > > > > > +writeq(val, base + INDIRECT_WR_DATA);
> > > > > > > +writeq((reg >> 2) | INDIRECT_WR, base + INDIRECT_ADDR);
> > > > > > > +
> > > > > > > +loops = 0;
> > > > > > > +while ((readq(base + INDIRECT_ADDR) & INDIRECT_WR) &&
> > > > > > > +       (loops++ < INDIRECT_TIMEOUT))
> > > > > > > +cpu_relax();
> > > > > > > +
> > > > > > > +if (loops >= INDIRECT_TIMEOUT) {
> > > > > > > +pr_err("%s timed out %d\n", __func__, loops);
> > > > > > > +return -ETIME;
> > > > > > > +}
> > > > > > > +return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static const struct regmap_config indirect_regbus_cfg = {
> > > > > > > +.reg_bits = 32,
> > > > > > > +.reg_stride = 4,
> > > > > > > +.val_bits = 32,
> > > > > > > +.fast_io = true,
> > > > > > > +
> > > > > > > +.reg_write = indirect_bus_reg_write,
> > > > > > > +.reg_read = indirect_bus_reg_read,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static struct spi_board_info m10_bmc_info = {
> > > > > > > +.modalias = "m10-d5005",
> > > > > > > +.max_speed_hz = 12500000,
> > > > > > > +.bus_num = 0,
> > > > > > > +.chip_select = 0,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static struct platform_device *create_cntrl(struct device *dev,
> > > > > > > +    void __iomem *base,
> > > > > > > +    struct spi_board_info *m10_info)
> > > > > > > +{
> > > > > > > +struct altera_spi_platform_data pdata;
> > > > > > > +struct platform_device_info pdevinfo;
> > > > > > > +u64 v;
> > > > > > > +
> > > > > > > +v = readq(base + SPI_CORE_PARAMETER);
> > > > > > > +
> > > > > > > +memset(&pdata, 0, sizeof(pdata));
> > > > > > > +pdata.mode_bits = SPI_CS_HIGH;
> > > > > > > +if (FIELD_GET(CLK_POLARITY, v))
> > > > > > > +pdata.mode_bits |= SPI_CPOL;
> > > > > > > +if (FIELD_GET(CLK_PHASE, v))
> > > > > > > +pdata.mode_bits |= SPI_CPHA;
> > > > > > > +
> > > > > > > +pdata.num_chipselect = FIELD_GET(NUM_CHIPSELECT, v);
> > > > > > > +pdata.bits_per_word_mask =
> > > > > > > +SPI_BPW_RANGE_MASK(1, FIELD_GET(DATA_WIDTH, v));
> > > > > > > +
> > > > > > > +pdata.num_devices = 1;
> > > > > > > +pdata.devices = m10_info;
> > > > > > > +
> > > > > > > +dev_dbg(dev, "%s cs %u bpm 0x%x mode 0x%x\n", __func__,
> > > > > > > +pdata.num_chipselect, pdata.bits_per_word_mask,
> > > > > > > +pdata.mode_bits);
> > > > > > > +
> > > > > > > +memset(&pdevinfo, 0, sizeof(pdevinfo));
> > > > > > > +
> > > > > > > +pdevinfo.name = "subdev_spi_altera";
> > > > > > > +pdevinfo.id = PLATFORM_DEVID_AUTO;
> > > > > > > +pdevinfo.parent = dev;
> > > > > > > +pdevinfo.data = &pdata;
> > > > > > > +pdevinfo.size_data = sizeof(pdata);
> > > > > > > +
> > > > > > > +return platform_device_register_full(&pdevinfo);
> > > > > >
> > > > > > Should this be a SPI driver? I think looking at the UIO case we had
> > > > > > decided against this pattern?
> > > > >
> > > > > This driver is similar in function to drivers/fpga/dfl-n3000-nios.c which
> > > > > uses this design pattern.  Is it okay to keep this design pattern for
> > > > > consistency?
> > > > >
> > > > >
> > > > > >
> > > > > > > +}
> > > > > > > +static int dfl_spi_altera_probe(struct dfl_device *dfl_dev)
> > > > > > > +{
> > > > > > > +struct device *dev = &dfl_dev->dev;
> > > > > > > +struct dfl_altera_spi *aspi;
> > > > > > > +
> > > > > > > +aspi = devm_kzalloc(dev, sizeof(*aspi), GFP_KERNEL);
> > > > > > > +
> > > > > > > +if (!aspi)
> > > > > > > +return -ENOMEM;
> > > > > > > +
> > > > > > > +dev_set_drvdata(dev, aspi);
> > > > > > > +
> > > > > > > +aspi->dev = dev;
> > > > > > > +
> > > > > > > +aspi->base = devm_ioremap_resource(dev, &dfl_dev->mmio_res);
> > > > > > > +
> > > > > > > +if (IS_ERR(aspi->base)) {
> > > > > > > +dev_err(dev, "%s get mem resource fail!\n", __func__);
> > > > > > > +return PTR_ERR(aspi->base);
> > > > > > > +}
> > > > > > > +
> > > > > > > +aspi->regmap = devm_regmap_init(dev, NULL, aspi,
> > > > &indirect_regbus_cfg);
> > > > > > > +if (IS_ERR(aspi->regmap))
> > > > > > > +return PTR_ERR(aspi->regmap);
> > > > > > > +
> > > > > > > +aspi->altr_spi = create_cntrl(dev, aspi->base, &m10_bmc_info);
> > > > > > > +
> > > > > > > +if (IS_ERR(aspi->altr_spi)) {
> > > > > > > +dev_err(dev, "%s failed to create spi platform driver\n",
> > > > > > > +__func__);
> > > > > > > +return PTR_ERR(aspi->base);
> > > > > > > +}
> > > > > > > +
> > > > > > > +return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > > +{
> > > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > > +
> > > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > > +}
> > > > > > > +
> > > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > > +
> > > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > > +{ }
> > > > > > > +};
> > > > > >
> > > > > > Maybe you can extend the Altera SPI driver with this part?
> > > > >
> > > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_ related
> > > > > code.  Wouldn't moving this code to that file produce conflicts?
> > > >
> > > > I've seen other drivers support multiple busses, so it should be
> > > > possible, there might be nuances I'm missing in my brief look at this,
> > > > though.
> > > >
> > > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > > >
> > > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > > > they might be guarding against what you describe with CONFIG_OF vs
> > > > CONFIG_ACPI)
> > > >
> > > > If that doesn't work we could split it up into
> > > >
> > > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > > or something of that sort?
> > > >
> > > > My point being, now that we have a bus, let's use it and develop drivers
> > > > according to the Linux device model where possible :)
> > >
> > > Agree. This does make sense from my side too. DFL core provides the
> > mechanism
> > > to enumerate different IPs on FPGA, but each function driver needs to go to
> > > related subsystem for review.  : )
> > >
> > > I understand that for FPGA case, it may have some additional logics for specific
> > > purposes based on common altera spi master IP, then additional code for
> >
> > I'm wondering if the additional logics are extensions for common spi-altera. Like
> > the
> > SPI_CORE_PARAMETER register, it is not within the register space of
> > spi-altera,
> >
> >
> >   |   |      +-------------+
> >   |DFL|------| +--------+  |
> >   |BUS|      | |SPI CORE|  |
> >   |   |      | |PARAM   |  |
> >   |   |      | +--------+  |
> >   |   |      |             |
> >   |   |      | +--------+  |   +-------+
> >              | |Indirect|  |   |spi    |
> >              | |access  +--+---|altera |
> >              | |master  |  |   +-------+
> >              | +--------+  |
> >              +-------------+
> > > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-xxxx.c
> >
> > So is it proper we integrate this feature into spi-altera? Previously
> > we have merged the dfl-n3000-nios, its spi part is very similar as
> > this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> > Could we borrow the idea, or could we just integrate this driver in
> > dfl-n3000-nios?
> 
> Looks like those are enhancements of the IP. They can be applied even

I don't think the extra registers are the enhancement of the IP. They
are not part of the IP because they are not within the IP's register
space. They are like some external way of describing the IP like
Devicetree or ACPI.

> other buses are used, not only for DFL, like PCI device or platform device,
> right? then why not put related code together with the original IP?

The code of devicetree or ACPI parsing are integrated in the IP drivers,
but for this case, it may not be proper for now, cause this style is not
formally introduced by any standard. IP specific parameters description
are not within the scope of DFL now.

> 
> The reason I suggested that function drivers which use DFL bus, still need
> to go to related subsystem, because we know DFL quite well but may
> not be the experts for every subsystem (e.g. SPI), and every IPs (e.g.
> Altera SPI Master). Altera SPI Master driver maintainer could give more
> valuable suggestions on your question if this feature can be integrated
> into spi-altera or not. : )

I agree that we put drivers to their related subsystem. I'm just
wondering if drivers/spi is the proper domain for it. Anyway getting
some inputs from SPI maintainers is a good suggestion.

Thanks,
Yilun

> 
> Hao
> 
> >
> > Thanks,
> > Yilun
> >
> > >
> > > Thanks
> > > Hao
> > >
> > > >
> > > > Cheers,
> > > > Moritz

RE: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Wu, Hao]

> > > > > > > > +
> > > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > > > +{
> > > > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > > > +
> > > > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > > > +
> > > > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > > > +{ }
> > > > > > > > +};
> > > > > > >
> > > > > > > Maybe you can extend the Altera SPI driver with this part?
> > > > > >
> > > > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_
> related
> > > > > > code.  Wouldn't moving this code to that file produce conflicts?
> > > > >
> > > > > I've seen other drivers support multiple busses, so it should be
> > > > > possible, there might be nuances I'm missing in my brief look at this,
> > > > > though.
> > > > >
> > > > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > > > >
> > > > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > > > > they might be guarding against what you describe with CONFIG_OF vs
> > > > > CONFIG_ACPI)
> > > > >
> > > > > If that doesn't work we could split it up into
> > > > >
> > > > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > > > or something of that sort?
> > > > >
> > > > > My point being, now that we have a bus, let's use it and develop drivers
> > > > > according to the Linux device model where possible :)
> > > >
> > > > Agree. This does make sense from my side too. DFL core provides the
> > > mechanism
> > > > to enumerate different IPs on FPGA, but each function driver needs to go
> to
> > > > related subsystem for review.  : )
> > > >
> > > > I understand that for FPGA case, it may have some additional logics for
> specific
> > > > purposes based on common altera spi master IP, then additional code for
> > >
> > > I'm wondering if the additional logics are extensions for common spi-altera.
> Like
> > > the
> > > SPI_CORE_PARAMETER register, it is not within the register space of
> > > spi-altera,
> > >
> > >
> > >   |   |      +-------------+
> > >   |DFL|------| +--------+  |
> > >   |BUS|      | |SPI CORE|  |
> > >   |   |      | |PARAM   |  |
> > >   |   |      | +--------+  |
> > >   |   |      |             |
> > >   |   |      | +--------+  |   +-------+
> > >              | |Indirect|  |   |spi    |
> > >              | |access  +--+---|altera |
> > >              | |master  |  |   +-------+
> > >              | +--------+  |
> > >              +-------------+
> > > > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-
> xxxx.c
> > >
> > > So is it proper we integrate this feature into spi-altera? Previously
> > > we have merged the dfl-n3000-nios, its spi part is very similar as
> > > this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> > > Could we borrow the idea, or could we just integrate this driver in
> > > dfl-n3000-nios?
> >
> > Looks like those are enhancements of the IP. They can be applied even
> 
> I don't think the extra registers are the enhancement of the IP. They
> are not part of the IP because they are not within the IP's register
> space. They are like some external way of describing the IP like
> Devicetree or ACPI.

Why adding new registers can't be consider as enhancement, those
changes serve the original IP and make it better, right? small mmio
footprint and parameter registers?

> 
> > other buses are used, not only for DFL, like PCI device or platform device,
> > right? then why not put related code together with the original IP?
> 
> The code of devicetree or ACPI parsing are integrated in the IP drivers,
> but for this case, it may not be proper for now, cause this style is not
> formally introduced by any standard. IP specific parameters description
> are not within the scope of DFL now.

Not sure if I get your point, but it's possible that we add some enhancements
to one IP then driver could be simplified and doesn't need devicetree any more.
For sure, it's IP specific thing, not the scope of DFL.

Then things become this: extension to IP to allow this IP to be used without
device tree, so that this IP can be used in DFL or PCI or other buses without
device tree?

Thanks
Hao

> 
> >
> > The reason I suggested that function drivers which use DFL bus, still need
> > to go to related subsystem, because we know DFL quite well but may
> > not be the experts for every subsystem (e.g. SPI), and every IPs (e.g.
> > Altera SPI Master). Altera SPI Master driver maintainer could give more
> > valuable suggestions on your question if this feature can be integrated
> > into spi-altera or not. : )
> 
> I agree that we put drivers to their related subsystem. I'm just
> wondering if drivers/spi is the proper domain for it. Anyway getting
> some inputs from SPI maintainers is a good suggestion.
> 
> Thanks,
> Yilun
> 
> >
> > Hao
> >
> > >
> > > Thanks,
> > > Yilun
> > >
> > > >
> > > > Thanks
> > > > Hao
> > > >
> > > > >
> > > > > Cheers,
> > > > > Moritz

Re: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Xu Yilun]

On Fri, Apr 09, 2021 at 12:02:47PM +0800, Wu, Hao wrote:
> > > > > > > > > +
> > > > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > > > > +{
> > > > > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > > > > +
> > > > > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > > > > +
> > > > > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > > > > +{ }
> > > > > > > > > +};
> > > > > > > >
> > > > > > > > Maybe you can extend the Altera SPI driver with this part?
> > > > > > >
> > > > > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_
> > related
> > > > > > > code.  Wouldn't moving this code to that file produce conflicts?
> > > > > >
> > > > > > I've seen other drivers support multiple busses, so it should be
> > > > > > possible, there might be nuances I'm missing in my brief look at this,
> > > > > > though.
> > > > > >
> > > > > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > > > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > > > > >
> > > > > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example (though
> > > > > > they might be guarding against what you describe with CONFIG_OF vs
> > > > > > CONFIG_ACPI)
> > > > > >
> > > > > > If that doesn't work we could split it up into
> > > > > >
> > > > > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > > > > or something of that sort?
> > > > > >
> > > > > > My point being, now that we have a bus, let's use it and develop drivers
> > > > > > according to the Linux device model where possible :)
> > > > >
> > > > > Agree. This does make sense from my side too. DFL core provides the
> > > > mechanism
> > > > > to enumerate different IPs on FPGA, but each function driver needs to go
> > to
> > > > > related subsystem for review.  : )
> > > > >
> > > > > I understand that for FPGA case, it may have some additional logics for
> > specific
> > > > > purposes based on common altera spi master IP, then additional code for
> > > >
> > > > I'm wondering if the additional logics are extensions for common spi-altera.
> > Like
> > > > the
> > > > SPI_CORE_PARAMETER register, it is not within the register space of
> > > > spi-altera,
> > > >
> > > >
> > > >   |   |      +-------------+
> > > >   |DFL|------| +--------+  |
> > > >   |BUS|      | |SPI CORE|  |
> > > >   |   |      | |PARAM   |  |
> > > >   |   |      | +--------+  |
> > > >   |   |      |             |
> > > >   |   |      | +--------+  |   +-------+
> > > >              | |Indirect|  |   |spi    |
> > > >              | |access  +--+---|altera |
> > > >              | |master  |  |   +-------+
> > > >              | +--------+  |
> > > >              +-------------+
> > > > > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-
> > xxxx.c
> > > >
> > > > So is it proper we integrate this feature into spi-altera? Previously
> > > > we have merged the dfl-n3000-nios, its spi part is very similar as
> > > > this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> > > > Could we borrow the idea, or could we just integrate this driver in
> > > > dfl-n3000-nios?
> > >
> > > Looks like those are enhancements of the IP. They can be applied even
> >
> > I don't think the extra registers are the enhancement of the IP. They
> > are not part of the IP because they are not within the IP's register
> > space. They are like some external way of describing the IP like
> > Devicetree or ACPI.
> 
> Why adding new registers can't be consider as enhancement, those
> changes serve the original IP and make it better, right? small mmio
> footprint and parameter registers?
> 
> >
> > > other buses are used, not only for DFL, like PCI device or platform device,
> > > right? then why not put related code together with the original IP?
> >
> > The code of devicetree or ACPI parsing are integrated in the IP drivers,
> > but for this case, it may not be proper for now, cause this style is not
> > formally introduced by any standard. IP specific parameters description
> > are not within the scope of DFL now.
> 
> Not sure if I get your point, but it's possible that we add some enhancements
> to one IP then driver could be simplified and doesn't need devicetree any more.
> For sure, it's IP specific thing, not the scope of DFL.
> 
> Then things become this: extension to IP to allow this IP to be used without
> device tree, so that this IP can be used in DFL or PCI or other buses without
> device tree?

It's good to extend an IP, but it needs a published SPEC and stable
register interfaces. For now, the spi-altera driver conforms to the
"SPI Core" chapter of the following spec:

https://www.intel.com/content/www/us/en/programmable/documentation/sfo1400787952932.html

There is no info about the core parameter register and this specific
indirect access bus. That's why I don't see these additional parts as
the enhancements to spi-altera. This DFL feature is like a wrapper for
the spi-altera sub device.

Thanks
Yilun

RE: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[Wu, Hao]

> On Fri, Apr 09, 2021 at 12:02:47PM +0800, Wu, Hao wrote:
> > > > > > > > > > +
> > > > > > > > > > +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
> > > > > > > > > > +{
> > > > > > > > > > +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
> > > > > > > > > > +
> > > > > > > > > > +platform_device_unregister(aspi->altr_spi);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +#define FME_FEATURE_ID_MAX10_SPI        0xe
> > > > > > > > > > +
> > > > > > > > > > +static const struct dfl_device_id dfl_spi_altera_ids[] = {
> > > > > > > > > > +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
> > > > > > > > > > +{ }
> > > > > > > > > > +};
> > > > > > > > >
> > > > > > > > > Maybe you can extend the Altera SPI driver with this part?
> > > > > > > >
> > > > > > > > The file, drivers/spi/spi-altera.c, already has platform MODULE_
> > > related
> > > > > > > > code.  Wouldn't moving this code to that file produce conflicts?
> > > > > > >
> > > > > > > I've seen other drivers support multiple busses, so it should be
> > > > > > > possible, there might be nuances I'm missing in my brief look at this,
> > > > > > > though.
> > > > > > >
> > > > > > > I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
> > > > > > > and the other one MODULE_DEVICE_TABLE(dfl, ...)
> > > > > > >
> > > > > > > See drivers/i2c/busses/i2c-designware-platdrv.c for an example
> (though
> > > > > > > they might be guarding against what you describe with CONFIG_OF vs
> > > > > > > CONFIG_ACPI)
> > > > > > >
> > > > > > > If that doesn't work we could split it up into
> > > > > > >
> > > > > > > altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
> > > > > > > or something of that sort?
> > > > > > >
> > > > > > > My point being, now that we have a bus, let's use it and develop
> drivers
> > > > > > > according to the Linux device model where possible :)
> > > > > >
> > > > > > Agree. This does make sense from my side too. DFL core provides the
> > > > > mechanism
> > > > > > to enumerate different IPs on FPGA, but each function driver needs to
> go
> > > to
> > > > > > related subsystem for review.  : )
> > > > > >
> > > > > > I understand that for FPGA case, it may have some additional logics for
> > > specific
> > > > > > purposes based on common altera spi master IP, then additional code
> for
> > > > >
> > > > > I'm wondering if the additional logics are extensions for common spi-
> altera.
> > > Like
> > > > > the
> > > > > SPI_CORE_PARAMETER register, it is not within the register space of
> > > > > spi-altera,
> > > > >
> > > > >
> > > > >   |   |      +-------------+
> > > > >   |DFL|------| +--------+  |
> > > > >   |BUS|      | |SPI CORE|  |
> > > > >   |   |      | |PARAM   |  |
> > > > >   |   |      | +--------+  |
> > > > >   |   |      |             |
> > > > >   |   |      | +--------+  |   +-------+
> > > > >              | |Indirect|  |   |spi    |
> > > > >              | |access  +--+---|altera |
> > > > >              | |master  |  |   +-------+
> > > > >              | +--------+  |
> > > > >              +-------------+
> > > > > > a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-
> > > xxxx.c
> > > > >
> > > > > So is it proper we integrate this feature into spi-altera? Previously
> > > > > we have merged the dfl-n3000-nios, its spi part is very similar as
> > > > > this driver. The dfl-n3000-nios make the spi-altera as a sub device.
> > > > > Could we borrow the idea, or could we just integrate this driver in
> > > > > dfl-n3000-nios?
> > > >
> > > > Looks like those are enhancements of the IP. They can be applied even
> > >
> > > I don't think the extra registers are the enhancement of the IP. They
> > > are not part of the IP because they are not within the IP's register
> > > space. They are like some external way of describing the IP like
> > > Devicetree or ACPI.
> >
> > Why adding new registers can't be consider as enhancement, those
> > changes serve the original IP and make it better, right? small mmio
> > footprint and parameter registers?
> >
> > >
> > > > other buses are used, not only for DFL, like PCI device or platform device,
> > > > right? then why not put related code together with the original IP?
> > >
> > > The code of devicetree or ACPI parsing are integrated in the IP drivers,
> > > but for this case, it may not be proper for now, cause this style is not
> > > formally introduced by any standard. IP specific parameters description
> > > are not within the scope of DFL now.
> >
> > Not sure if I get your point, but it's possible that we add some enhancements
> > to one IP then driver could be simplified and doesn't need devicetree any more.
> > For sure, it's IP specific thing, not the scope of DFL.
> >
> > Then things become this: extension to IP to allow this IP to be used without
> > device tree, so that this IP can be used in DFL or PCI or other buses without
> > device tree?
> 
> It's good to extend an IP, but it needs a published SPEC and stable
> register interfaces. For now, the spi-altera driver conforms to the
> "SPI Core" chapter of the following spec:
> 
> https://www.intel.com/content/www/us/en/programmable/documentation/sf
> o1400787952932.html
> 
> There is no info about the core parameter register and this specific
> indirect access bus. That's why I don't see these additional parts as
> the enhancements to spi-altera. This DFL feature is like a wrapper for
> the spi-altera sub device.

It really doesn't matter, even if you consider this as an new IP, it's still a SPI
Master, it's driver still need to be reviewed in drivers/spi subsystem. The
worst case is that we need to write a new spi-xxx.c driver, that's it.

From DFL part, DFL only can enumerate the common hardware resources,
but no good way to help thing like this, specific IP parameters (which handle
by devicetree in platform driver). So for some IPs , they still need some
extensions to avoid such dependency (on device tree for parameters). I guess
we may see more similar cases in the future.

Anyway, I think we reached agreement that for device drivers on DFL bus, it
needs to be reviewed in its own subsystem. : )

Thanks
Hao

> 
> Thanks
> Yilun

RE: [PATCH 2/3] fpga: dfl: Add DFL bus driver for Altera SPI Master

[matthew.gerlach]

On Fri, 9 Apr 2021, Wu, Hao wrote:

>> On Fri, Apr 09, 2021 at 12:02:47PM +0800, Wu, Hao wrote:
>>>>>>>>>>> +
>>>>>>>>>>> +static void dfl_spi_altera_remove(struct dfl_device *dfl_dev)
>>>>>>>>>>> +{
>>>>>>>>>>> +struct dfl_altera_spi *aspi = dev_get_drvdata(&dfl_dev->dev);
>>>>>>>>>>> +
>>>>>>>>>>> +platform_device_unregister(aspi->altr_spi);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +#define FME_FEATURE_ID_MAX10_SPI        0xe
>>>>>>>>>>> +
>>>>>>>>>>> +static const struct dfl_device_id dfl_spi_altera_ids[] = {
>>>>>>>>>>> +{ FME_ID, FME_FEATURE_ID_MAX10_SPI },
>>>>>>>>>>> +{ }
>>>>>>>>>>> +};
>>>>>>>>>>
>>>>>>>>>> Maybe you can extend the Altera SPI driver with this part?
>>>>>>>>>
>>>>>>>>> The file, drivers/spi/spi-altera.c, already has platform MODULE_
>>>> related
>>>>>>>>> code.  Wouldn't moving this code to that file produce conflicts?
>>>>>>>>
>>>>>>>> I've seen other drivers support multiple busses, so it should be
>>>>>>>> possible, there might be nuances I'm missing in my brief look at this,
>>>>>>>> though.
>>>>>>>>
>>>>>>>> I think one of them would be MODULE_DEVICE_TABLE(platform, ...)
>>>>>>>> and the other one MODULE_DEVICE_TABLE(dfl, ...)
>>>>>>>>
>>>>>>>> See drivers/i2c/busses/i2c-designware-platdrv.c for an example
>> (though
>>>>>>>> they might be guarding against what you describe with CONFIG_OF vs
>>>>>>>> CONFIG_ACPI)
>>>>>>>>
>>>>>>>> If that doesn't work we could split it up into
>>>>>>>>
>>>>>>>> altera-spi-plat.c and altera-spi-dfl.c and altera-spi-core.c
>>>>>>>> or something of that sort?
>>>>>>>>
>>>>>>>> My point being, now that we have a bus, let's use it and develop
>> drivers
>>>>>>>> according to the Linux device model where possible :)
>>>>>>>
>>>>>>> Agree. This does make sense from my side too. DFL core provides the
>>>>>> mechanism
>>>>>>> to enumerate different IPs on FPGA, but each function driver needs to
>> go
>>>> to
>>>>>>> related subsystem for review.  : )
>>>>>>>
>>>>>>> I understand that for FPGA case, it may have some additional logics for
>>>> specific
>>>>>>> purposes based on common altera spi master IP, then additional code
>> for
>>>>>>
>>>>>> I'm wondering if the additional logics are extensions for common spi-
>> altera.
>>>> Like
>>>>>> the
>>>>>> SPI_CORE_PARAMETER register, it is not within the register space of
>>>>>> spi-altera,
>>>>>>
>>>>>>
>>>>>>   |   |      +-------------+
>>>>>>   |DFL|------| +--------+  |
>>>>>>   |BUS|      | |SPI CORE|  |
>>>>>>   |   |      | |PARAM   |  |
>>>>>>   |   |      | +--------+  |
>>>>>>   |   |      |             |
>>>>>>   |   |      | +--------+  |   +-------+
>>>>>>              | |Indirect|  |   |spi    |
>>>>>>              | |access  +--+---|altera |
>>>>>>              | |master  |  |   +-------+
>>>>>>              | +--------+  |
>>>>>>              +-------------+
>>>>>>> a specific product still can be put into altera-spi-xxxx.c or altera-spi-dfl-
>>>> xxxx.c
>>>>>>
>>>>>> So is it proper we integrate this feature into spi-altera? Previously
>>>>>> we have merged the dfl-n3000-nios, its spi part is very similar as
>>>>>> this driver. The dfl-n3000-nios make the spi-altera as a sub device.
>>>>>> Could we borrow the idea, or could we just integrate this driver in
>>>>>> dfl-n3000-nios?
>>>>>
>>>>> Looks like those are enhancements of the IP. They can be applied even
>>>>
>>>> I don't think the extra registers are the enhancement of the IP. They
>>>> are not part of the IP because they are not within the IP's register
>>>> space. They are like some external way of describing the IP like
>>>> Devicetree or ACPI.
>>>
>>> Why adding new registers can't be consider as enhancement, those
>>> changes serve the original IP and make it better, right? small mmio
>>> footprint and parameter registers?
>>>
>>>>
>>>>> other buses are used, not only for DFL, like PCI device or platform device,
>>>>> right? then why not put related code together with the original IP?
>>>>
>>>> The code of devicetree or ACPI parsing are integrated in the IP drivers,
>>>> but for this case, it may not be proper for now, cause this style is not
>>>> formally introduced by any standard. IP specific parameters description
>>>> are not within the scope of DFL now.
>>>
>>> Not sure if I get your point, but it's possible that we add some enhancements
>>> to one IP then driver could be simplified and doesn't need devicetree any more.
>>> For sure, it's IP specific thing, not the scope of DFL.
>>>
>>> Then things become this: extension to IP to allow this IP to be used without
>>> device tree, so that this IP can be used in DFL or PCI or other buses without
>>> device tree?
>>
>> It's good to extend an IP, but it needs a published SPEC and stable
>> register interfaces. For now, the spi-altera driver conforms to the
>> "SPI Core" chapter of the following spec:
>>
>> https://www.intel.com/content/www/us/en/programmable/documentation/sf
>> o1400787952932.html
>>
>> There is no info about the core parameter register and this specific
>> indirect access bus. That's why I don't see these additional parts as
>> the enhancements to spi-altera. This DFL feature is like a wrapper for
>> the spi-altera sub device.
>
> It really doesn't matter, even if you consider this as an new IP, it's still a SPI
> Master, it's driver still need to be reviewed in drivers/spi subsystem. The
> worst case is that we need to write a new spi-xxx.c driver, that's it.
>
> From DFL part, DFL only can enumerate the common hardware resources,
> but no good way to help thing like this, specific IP parameters (which handle
> by devicetree in platform driver). So for some IPs , they still need some
> extensions to avoid such dependency (on device tree for parameters). I guess
> we may see more similar cases in the future.
>
> Anyway, I think we reached agreement that for device drivers on DFL bus, it
> needs to be reviewed in its own subsystem. : )

Yes, I agree that device drivers on the DFL bus should go in the 
appropriate the directory of the appropriate  framework.  As Moritz has 
pointed out there are already examples bus specific driver code, for 
multiple buses, being the driver framework directory (e.g. Designware 
I2C).

In this particular case, the Device Feature is wraps a particular 
instantiation of an Altera SPI Master controller.  As Yilun pointed out, this 
wrapper has a SPI_CORE_PARAMETER register which contains the values 
of all the configuration parameters of the Altera SPI Master IP block. 
This kind of information would also be included in a Device Tree 
description of the instantiation. The Device Feature also implements 
indirect register access to the actual registers of the Altera SPI Master 
that provides a bridge from the PCIe bus to the Avalon Slave containing 
the registers of the Altera SPI Master.

Given the Device Feature wraps an Altera SPI Master, it might still make 
sense for the DFL driver to instantiate a sub driver instance for the 
Altera SPI Master.  I will resubmit the current patches with 
drivers/fpga/dfl-spi-altera.c moved to drivers/spi/spi-altera-dfl.c for 
feedback from the SPI maintainers and continue investigating collapsing 
the DFL driver and the platform sub-driver into a single DFL driver.  My 
first attempt did not go well.

Matthew

directory of the app >
> Thanks
> Hao
>
>>
>> Thanks
>> Yilun
>

[PATCH 3/3] hwmon: intel-m10-bmc-hwmon: add sensor support of Intel D5005 card

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Like the Intel N3000 card, the Intel D5005 has a MAX10 based
BMC.  This commit adds support for the D5005 sensors that are
monitored by the MAX10 BMC.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@linux.intel.com>
---
 drivers/hwmon/intel-m10-bmc-hwmon.c | 122 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/intel-m10-bmc.c         |  10 +++
 2 files changed, 132 insertions(+)

diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
index 17d5e6b..bd7ed2e 100644
--- a/drivers/hwmon/intel-m10-bmc-hwmon.c
+++ b/drivers/hwmon/intel-m10-bmc-hwmon.c
@@ -99,6 +99,50 @@ struct m10bmc_hwmon {
 	NULL
 };
 
+static const struct m10bmc_sdata d5005bmc_temp_tbl[] = {
+	{ 0x100, 0x104, 0x108, 0x10c, 0x0, 500, "Board Inlet Air Temperature" },
+	{ 0x110, 0x114, 0x118, 0x0, 0x0, 500, "FPGA Core Temperature" },
+	{ 0x11c, 0x120, 0x124, 0x128, 0x0, 500, "Board Exhaust Air Temperature" },
+	{ 0x12c, 0x130, 0x134, 0x0, 0x0, 500, "FPGA Transceiver Temperature" },
+	{ 0x138, 0x13c, 0x140, 0x144, 0x0, 500, "RDIMM0 Temperature" },
+	{ 0x148, 0x14c, 0x150, 0x154, 0x0, 500, "RDIMM1 Temperature" },
+	{ 0x158, 0x15c, 0x160, 0x164, 0x0, 500, "RDIMM2 Temperature" },
+	{ 0x168, 0x16c, 0x170, 0x174, 0x0, 500, "RDIMM3 Temperature" },
+	{ 0x178, 0x17c, 0x180, 0x0, 0x0, 500, "QSFP0 Temperature" },
+	{ 0x188, 0x18c, 0x190, 0x0, 0x0, 500, "QSFP1 Temperature" },
+	{ 0x1a0, 0x1a4, 0x1a8, 0x0, 0x0, 500, "3.3v Temperature" },
+	{ 0x1bc, 0x1c0, 0x1c4, 0x0, 0x0, 500, "VCCERAM Temperature" },
+	{ 0x1d8, 0x1dc, 0x1e0, 0x0, 0x0, 500, "VCCR Temperature" },
+	{ 0x1f4, 0x1f8, 0x1fc, 0x0, 0x0, 500, "VCCT Temperature" },
+	{ 0x210, 0x214, 0x218, 0x0, 0x0, 500, "1.8v Temperature" },
+	{ 0x22c, 0x230, 0x234, 0x0, 0x0, 500, "12v Backplane Temperature" },
+	{ 0x248, 0x24c, 0x250, 0x0, 0x0, 500, "12v AUX Temperature" },
+};
+
+static const struct m10bmc_sdata d5005bmc_in_tbl[] = {
+	{ 0x184, 0x0, 0x0, 0x0, 0x0, 1, "QSFP0 Supply Voltage" },
+	{ 0x194, 0x0, 0x0, 0x0, 0x0, 1, "QSFP1 Supply Voltage" },
+	{ 0x198, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Voltage" },
+	{ 0x1ac, 0x1b0, 0x1b4, 0x0, 0x0, 1, "3.3v Voltage" },
+	{ 0x1c8, 0x1cc, 0x1d0, 0x0, 0x0, 1, "VCCERAM Voltage" },
+	{ 0x1e4, 0x1e8, 0x1ec, 0x0, 0x0, 1, "VCCR Voltage" },
+	{ 0x200, 0x204, 0x208, 0x0, 0x0, 1, "VCCT Voltage" },
+	{ 0x21c, 0x220, 0x224, 0x0, 0x0, 1, "1.8v Voltage" },
+	{ 0x238, 0x0, 0x0, 0x0, 0x23c, 1, "12v Backplane Voltage" },
+	{ 0x254, 0x0, 0x0, 0x0, 0x258, 1, "12v AUX Voltage" },
+};
+
+static const struct m10bmc_sdata d5005bmc_curr_tbl[] = {
+	{ 0x19c, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Current" },
+	{ 0x1b8, 0x0, 0x0, 0x0, 0x0, 1, "3.3v Current" },
+	{ 0x1d4, 0x0, 0x0, 0x0, 0x0, 1, "VCCERAM Current" },
+	{ 0x1f0, 0x0, 0x0, 0x0, 0x0, 1, "VCCR Current" },
+	{ 0x20c, 0x0, 0x0, 0x0, 0x0, 1, "VCCT Current" },
+	{ 0x228, 0x0, 0x0, 0x0, 0x0, 1, "1.8v Current" },
+	{ 0x240, 0x244, 0x0, 0x0, 0x0, 1, "12v Backplane Current" },
+	{ 0x25c, 0x260, 0x0, 0x0, 0x0, 1, "12v AUX Current" },
+};
+
 static const struct m10bmc_hwmon_board_data n3000bmc_hwmon_bdata = {
 	.tables = {
 		[hwmon_temp] = n3000bmc_temp_tbl,
@@ -110,6 +154,80 @@ struct m10bmc_hwmon {
 	.hinfo = n3000bmc_hinfo,
 };
 
+static const struct hwmon_channel_info *d5005bmc_hinfo[] = {
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
+			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
+			   HWMON_T_LABEL),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
+			   HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
+			   HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
+			   HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
+			   HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
+			   HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL),
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL),
+	NULL
+};
+
+static const struct m10bmc_hwmon_board_data d5005bmc_hwmon_bdata = {
+	.tables = {
+		[hwmon_temp] = d5005bmc_temp_tbl,
+		[hwmon_in] = d5005bmc_in_tbl,
+		[hwmon_curr] = d5005bmc_curr_tbl,
+	},
+
+	.hinfo = d5005bmc_hinfo,
+};
+
 static umode_t
 m10bmc_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
 			u32 attr, int channel)
@@ -316,6 +434,10 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
 		.name = "n3000bmc-hwmon",
 		.driver_data = (unsigned long)&n3000bmc_hwmon_bdata,
 	},
+	{
+		.name = "d5005bmc-hwmon",
+		.driver_data = (unsigned long)&d5005bmc_hwmon_bdata,
+	},
 	{ }
 };
 
diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
index 1161933..dbb4975 100644
--- a/drivers/mfd/intel-m10-bmc.c
+++ b/drivers/mfd/intel-m10-bmc.c
@@ -15,6 +15,11 @@
 
 enum m10bmc_type {
 	M10_N3000,
+	M10_D5005
+};
+
+static struct mfd_cell m10bmc_bmc_subdevs[] = {
+	{ .name = "d5005bmc-hwmon" },
 };
 
 static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
@@ -183,6 +188,10 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 		cells = m10bmc_pacn3000_subdevs;
 		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
 		break;
+	case M10_D5005:
+		cells = m10bmc_bmc_subdevs;
+		n_cell = ARRAY_SIZE(m10bmc_bmc_subdevs);
+		break;
 	default:
 		return -ENODEV;
 	}
@@ -197,6 +206,7 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 
 static const struct spi_device_id m10bmc_spi_id[] = {
 	{ "m10-n3000", M10_N3000 },
+	{ "m10-d5005", M10_D5005 },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
-- 
1.8.3.1

Re: [PATCH 3/3] hwmon: intel-m10-bmc-hwmon: add sensor support of Intel D5005 card

[Guenter Roeck]

On 4/5/21 4:53 PM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Like the Intel N3000 card, the Intel D5005 has a MAX10 based
> BMC.  This commit adds support for the D5005 sensors that are
> monitored by the MAX10 BMC.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@linux.intel.com>
> ---
>  drivers/hwmon/intel-m10-bmc-hwmon.c | 122 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/intel-m10-bmc.c         |  10 +++
>  2 files changed, 132 insertions(+)
> 
> diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
> index 17d5e6b..bd7ed2e 100644
> --- a/drivers/hwmon/intel-m10-bmc-hwmon.c
> +++ b/drivers/hwmon/intel-m10-bmc-hwmon.c
> @@ -99,6 +99,50 @@ struct m10bmc_hwmon {
>  	NULL
>  };
>  
> +static const struct m10bmc_sdata d5005bmc_temp_tbl[] = {
> +	{ 0x100, 0x104, 0x108, 0x10c, 0x0, 500, "Board Inlet Air Temperature" },
> +	{ 0x110, 0x114, 0x118, 0x0, 0x0, 500, "FPGA Core Temperature" },
> +	{ 0x11c, 0x120, 0x124, 0x128, 0x0, 500, "Board Exhaust Air Temperature" },
> +	{ 0x12c, 0x130, 0x134, 0x0, 0x0, 500, "FPGA Transceiver Temperature" },
> +	{ 0x138, 0x13c, 0x140, 0x144, 0x0, 500, "RDIMM0 Temperature" },
> +	{ 0x148, 0x14c, 0x150, 0x154, 0x0, 500, "RDIMM1 Temperature" },
> +	{ 0x158, 0x15c, 0x160, 0x164, 0x0, 500, "RDIMM2 Temperature" },
> +	{ 0x168, 0x16c, 0x170, 0x174, 0x0, 500, "RDIMM3 Temperature" },
> +	{ 0x178, 0x17c, 0x180, 0x0, 0x0, 500, "QSFP0 Temperature" },
> +	{ 0x188, 0x18c, 0x190, 0x0, 0x0, 500, "QSFP1 Temperature" },
> +	{ 0x1a0, 0x1a4, 0x1a8, 0x0, 0x0, 500, "3.3v Temperature" },
> +	{ 0x1bc, 0x1c0, 0x1c4, 0x0, 0x0, 500, "VCCERAM Temperature" },
> +	{ 0x1d8, 0x1dc, 0x1e0, 0x0, 0x0, 500, "VCCR Temperature" },
> +	{ 0x1f4, 0x1f8, 0x1fc, 0x0, 0x0, 500, "VCCT Temperature" },
> +	{ 0x210, 0x214, 0x218, 0x0, 0x0, 500, "1.8v Temperature" },
> +	{ 0x22c, 0x230, 0x234, 0x0, 0x0, 500, "12v Backplane Temperature" },
> +	{ 0x248, 0x24c, 0x250, 0x0, 0x0, 500, "12v AUX Temperature" },
> +};
> +
> +static const struct m10bmc_sdata d5005bmc_in_tbl[] = {
> +	{ 0x184, 0x0, 0x0, 0x0, 0x0, 1, "QSFP0 Supply Voltage" },
> +	{ 0x194, 0x0, 0x0, 0x0, 0x0, 1, "QSFP1 Supply Voltage" },
> +	{ 0x198, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Voltage" },
> +	{ 0x1ac, 0x1b0, 0x1b4, 0x0, 0x0, 1, "3.3v Voltage" },
> +	{ 0x1c8, 0x1cc, 0x1d0, 0x0, 0x0, 1, "VCCERAM Voltage" },
> +	{ 0x1e4, 0x1e8, 0x1ec, 0x0, 0x0, 1, "VCCR Voltage" },
> +	{ 0x200, 0x204, 0x208, 0x0, 0x0, 1, "VCCT Voltage" },
> +	{ 0x21c, 0x220, 0x224, 0x0, 0x0, 1, "1.8v Voltage" },
> +	{ 0x238, 0x0, 0x0, 0x0, 0x23c, 1, "12v Backplane Voltage" },
> +	{ 0x254, 0x0, 0x0, 0x0, 0x258, 1, "12v AUX Voltage" },
> +};
> +
> +static const struct m10bmc_sdata d5005bmc_curr_tbl[] = {
> +	{ 0x19c, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Current" },
> +	{ 0x1b8, 0x0, 0x0, 0x0, 0x0, 1, "3.3v Current" },
> +	{ 0x1d4, 0x0, 0x0, 0x0, 0x0, 1, "VCCERAM Current" },
> +	{ 0x1f0, 0x0, 0x0, 0x0, 0x0, 1, "VCCR Current" },
> +	{ 0x20c, 0x0, 0x0, 0x0, 0x0, 1, "VCCT Current" },
> +	{ 0x228, 0x0, 0x0, 0x0, 0x0, 1, "1.8v Current" },
> +	{ 0x240, 0x244, 0x0, 0x0, 0x0, 1, "12v Backplane Current" },
> +	{ 0x25c, 0x260, 0x0, 0x0, 0x0, 1, "12v AUX Current" },
> +};
> +
>  static const struct m10bmc_hwmon_board_data n3000bmc_hwmon_bdata = {
>  	.tables = {
>  		[hwmon_temp] = n3000bmc_temp_tbl,
> @@ -110,6 +154,80 @@ struct m10bmc_hwmon {
>  	.hinfo = n3000bmc_hinfo,
>  };
>  
> +static const struct hwmon_channel_info *d5005bmc_hinfo[] = {
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL),
> +	NULL
> +};
> +
> +static const struct m10bmc_hwmon_board_data d5005bmc_hwmon_bdata = {
> +	.tables = {
> +		[hwmon_temp] = d5005bmc_temp_tbl,
> +		[hwmon_in] = d5005bmc_in_tbl,
> +		[hwmon_curr] = d5005bmc_curr_tbl,
> +	},
> +
> +	.hinfo = d5005bmc_hinfo,
> +};
> +
>  static umode_t
>  m10bmc_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
>  			u32 attr, int channel)
> @@ -316,6 +434,10 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
>  		.name = "n3000bmc-hwmon",
>  		.driver_data = (unsigned long)&n3000bmc_hwmon_bdata,
>  	},
> +	{
> +		.name = "d5005bmc-hwmon",
> +		.driver_data = (unsigned long)&d5005bmc_hwmon_bdata,
> +	},
>  	{ }
>  };
>  
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c

It may be better to split this patch into two parts to simplify
its review and scope of responsibility.

Guenter

> index 1161933..dbb4975 100644
> --- a/drivers/mfd/intel-m10-bmc.c
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -15,6 +15,11 @@
>  
>  enum m10bmc_type {
>  	M10_N3000,
> +	M10_D5005
> +};
> +
> +static struct mfd_cell m10bmc_bmc_subdevs[] = {
> +	{ .name = "d5005bmc-hwmon" },
>  };
>  
>  static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
> @@ -183,6 +188,10 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  		cells = m10bmc_pacn3000_subdevs;
>  		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
>  		break;
> +	case M10_D5005:
> +		cells = m10bmc_bmc_subdevs;
> +		n_cell = ARRAY_SIZE(m10bmc_bmc_subdevs);
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> @@ -197,6 +206,7 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  
>  static const struct spi_device_id m10bmc_spi_id[] = {
>  	{ "m10-n3000", M10_N3000 },
> +	{ "m10-d5005", M10_D5005 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> 

Re: [PATCH 3/3] hwmon: intel-m10-bmc-hwmon: add sensor support of Intel D5005 card

[Lee Jones]

On Mon, 05 Apr 2021, Guenter Roeck wrote:

> On 4/5/21 4:53 PM, matthew.gerlach@linux.intel.com wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > 
> > Like the Intel N3000 card, the Intel D5005 has a MAX10 based
> > BMC.  This commit adds support for the D5005 sensors that are
> > monitored by the MAX10 BMC.
> > 
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@linux.intel.com>
> > ---
> >  drivers/hwmon/intel-m10-bmc-hwmon.c | 122 ++++++++++++++++++++++++++++++++++++
> >  drivers/mfd/intel-m10-bmc.c         |  10 +++
> >  2 files changed, 132 insertions(+)
> > 
> > diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
> > index 17d5e6b..bd7ed2e 100644
> > --- a/drivers/hwmon/intel-m10-bmc-hwmon.c
> > +++ b/drivers/hwmon/intel-m10-bmc-hwmon.c
> > @@ -99,6 +99,50 @@ struct m10bmc_hwmon {
> >  	NULL
> >  };
> >  
> > +static const struct m10bmc_sdata d5005bmc_temp_tbl[] = {
> > +	{ 0x100, 0x104, 0x108, 0x10c, 0x0, 500, "Board Inlet Air Temperature" },
> > +	{ 0x110, 0x114, 0x118, 0x0, 0x0, 500, "FPGA Core Temperature" },
> > +	{ 0x11c, 0x120, 0x124, 0x128, 0x0, 500, "Board Exhaust Air Temperature" },
> > +	{ 0x12c, 0x130, 0x134, 0x0, 0x0, 500, "FPGA Transceiver Temperature" },
> > +	{ 0x138, 0x13c, 0x140, 0x144, 0x0, 500, "RDIMM0 Temperature" },
> > +	{ 0x148, 0x14c, 0x150, 0x154, 0x0, 500, "RDIMM1 Temperature" },
> > +	{ 0x158, 0x15c, 0x160, 0x164, 0x0, 500, "RDIMM2 Temperature" },
> > +	{ 0x168, 0x16c, 0x170, 0x174, 0x0, 500, "RDIMM3 Temperature" },
> > +	{ 0x178, 0x17c, 0x180, 0x0, 0x0, 500, "QSFP0 Temperature" },
> > +	{ 0x188, 0x18c, 0x190, 0x0, 0x0, 500, "QSFP1 Temperature" },
> > +	{ 0x1a0, 0x1a4, 0x1a8, 0x0, 0x0, 500, "3.3v Temperature" },
> > +	{ 0x1bc, 0x1c0, 0x1c4, 0x0, 0x0, 500, "VCCERAM Temperature" },
> > +	{ 0x1d8, 0x1dc, 0x1e0, 0x0, 0x0, 500, "VCCR Temperature" },
> > +	{ 0x1f4, 0x1f8, 0x1fc, 0x0, 0x0, 500, "VCCT Temperature" },
> > +	{ 0x210, 0x214, 0x218, 0x0, 0x0, 500, "1.8v Temperature" },
> > +	{ 0x22c, 0x230, 0x234, 0x0, 0x0, 500, "12v Backplane Temperature" },
> > +	{ 0x248, 0x24c, 0x250, 0x0, 0x0, 500, "12v AUX Temperature" },
> > +};
> > +
> > +static const struct m10bmc_sdata d5005bmc_in_tbl[] = {
> > +	{ 0x184, 0x0, 0x0, 0x0, 0x0, 1, "QSFP0 Supply Voltage" },
> > +	{ 0x194, 0x0, 0x0, 0x0, 0x0, 1, "QSFP1 Supply Voltage" },
> > +	{ 0x198, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Voltage" },
> > +	{ 0x1ac, 0x1b0, 0x1b4, 0x0, 0x0, 1, "3.3v Voltage" },
> > +	{ 0x1c8, 0x1cc, 0x1d0, 0x0, 0x0, 1, "VCCERAM Voltage" },
> > +	{ 0x1e4, 0x1e8, 0x1ec, 0x0, 0x0, 1, "VCCR Voltage" },
> > +	{ 0x200, 0x204, 0x208, 0x0, 0x0, 1, "VCCT Voltage" },
> > +	{ 0x21c, 0x220, 0x224, 0x0, 0x0, 1, "1.8v Voltage" },
> > +	{ 0x238, 0x0, 0x0, 0x0, 0x23c, 1, "12v Backplane Voltage" },
> > +	{ 0x254, 0x0, 0x0, 0x0, 0x258, 1, "12v AUX Voltage" },
> > +};
> > +
> > +static const struct m10bmc_sdata d5005bmc_curr_tbl[] = {
> > +	{ 0x19c, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Current" },
> > +	{ 0x1b8, 0x0, 0x0, 0x0, 0x0, 1, "3.3v Current" },
> > +	{ 0x1d4, 0x0, 0x0, 0x0, 0x0, 1, "VCCERAM Current" },
> > +	{ 0x1f0, 0x0, 0x0, 0x0, 0x0, 1, "VCCR Current" },
> > +	{ 0x20c, 0x0, 0x0, 0x0, 0x0, 1, "VCCT Current" },
> > +	{ 0x228, 0x0, 0x0, 0x0, 0x0, 1, "1.8v Current" },
> > +	{ 0x240, 0x244, 0x0, 0x0, 0x0, 1, "12v Backplane Current" },
> > +	{ 0x25c, 0x260, 0x0, 0x0, 0x0, 1, "12v AUX Current" },
> > +};
> > +
> >  static const struct m10bmc_hwmon_board_data n3000bmc_hwmon_bdata = {
> >  	.tables = {
> >  		[hwmon_temp] = n3000bmc_temp_tbl,
> > @@ -110,6 +154,80 @@ struct m10bmc_hwmon {
> >  	.hinfo = n3000bmc_hinfo,
> >  };
> >  
> > +static const struct hwmon_channel_info *d5005bmc_hinfo[] = {
> > +	HWMON_CHANNEL_INFO(temp,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> > +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> > +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> > +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> > +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> > +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> > +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL,
> > +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> > +			   HWMON_T_LABEL),
> > +	HWMON_CHANNEL_INFO(in,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> > +			   HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> > +			   HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> > +			   HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> > +			   HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> > +			   HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL,
> > +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL),
> > +	HWMON_CHANNEL_INFO(curr,
> > +			   HWMON_C_INPUT | HWMON_C_LABEL,
> > +			   HWMON_C_INPUT | HWMON_C_LABEL,
> > +			   HWMON_C_INPUT | HWMON_C_LABEL,
> > +			   HWMON_C_INPUT | HWMON_C_LABEL,
> > +			   HWMON_C_INPUT | HWMON_C_LABEL,
> > +			   HWMON_C_INPUT | HWMON_C_LABEL,
> > +			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL,
> > +			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL),
> > +	NULL
> > +};
> > +
> > +static const struct m10bmc_hwmon_board_data d5005bmc_hwmon_bdata = {
> > +	.tables = {
> > +		[hwmon_temp] = d5005bmc_temp_tbl,
> > +		[hwmon_in] = d5005bmc_in_tbl,
> > +		[hwmon_curr] = d5005bmc_curr_tbl,
> > +	},
> > +
> > +	.hinfo = d5005bmc_hinfo,
> > +};
> > +
> >  static umode_t
> >  m10bmc_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> >  			u32 attr, int channel)
> > @@ -316,6 +434,10 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
> >  		.name = "n3000bmc-hwmon",
> >  		.driver_data = (unsigned long)&n3000bmc_hwmon_bdata,
> >  	},
> > +	{
> > +		.name = "d5005bmc-hwmon",
> > +		.driver_data = (unsigned long)&d5005bmc_hwmon_bdata,
> > +	},
> >  	{ }
> >  };
> >  
> > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> 
> It may be better to split this patch into two parts to simplify
> its review and scope of responsibility.

No need.

Not withstanding significant changes:

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

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

Re: [PATCH 3/3] hwmon: intel-m10-bmc-hwmon: add sensor support of Intel D5005 card

[Xu Yilun]

On Mon, Apr 05, 2021 at 04:53:01PM -0700, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Like the Intel N3000 card, the Intel D5005 has a MAX10 based
> BMC.  This commit adds support for the D5005 sensors that are
> monitored by the MAX10 BMC.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@linux.intel.com>
> ---
>  drivers/hwmon/intel-m10-bmc-hwmon.c | 122 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/intel-m10-bmc.c         |  10 +++
>  2 files changed, 132 insertions(+)
> 
> diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
> index 17d5e6b..bd7ed2e 100644
> --- a/drivers/hwmon/intel-m10-bmc-hwmon.c
> +++ b/drivers/hwmon/intel-m10-bmc-hwmon.c
> @@ -99,6 +99,50 @@ struct m10bmc_hwmon {
>  	NULL
>  };
>  
> +static const struct m10bmc_sdata d5005bmc_temp_tbl[] = {
> +	{ 0x100, 0x104, 0x108, 0x10c, 0x0, 500, "Board Inlet Air Temperature" },
> +	{ 0x110, 0x114, 0x118, 0x0, 0x0, 500, "FPGA Core Temperature" },
> +	{ 0x11c, 0x120, 0x124, 0x128, 0x0, 500, "Board Exhaust Air Temperature" },
> +	{ 0x12c, 0x130, 0x134, 0x0, 0x0, 500, "FPGA Transceiver Temperature" },
> +	{ 0x138, 0x13c, 0x140, 0x144, 0x0, 500, "RDIMM0 Temperature" },
> +	{ 0x148, 0x14c, 0x150, 0x154, 0x0, 500, "RDIMM1 Temperature" },
> +	{ 0x158, 0x15c, 0x160, 0x164, 0x0, 500, "RDIMM2 Temperature" },
> +	{ 0x168, 0x16c, 0x170, 0x174, 0x0, 500, "RDIMM3 Temperature" },
> +	{ 0x178, 0x17c, 0x180, 0x0, 0x0, 500, "QSFP0 Temperature" },
> +	{ 0x188, 0x18c, 0x190, 0x0, 0x0, 500, "QSFP1 Temperature" },
> +	{ 0x1a0, 0x1a4, 0x1a8, 0x0, 0x0, 500, "3.3v Temperature" },
> +	{ 0x1bc, 0x1c0, 0x1c4, 0x0, 0x0, 500, "VCCERAM Temperature" },
> +	{ 0x1d8, 0x1dc, 0x1e0, 0x0, 0x0, 500, "VCCR Temperature" },
> +	{ 0x1f4, 0x1f8, 0x1fc, 0x0, 0x0, 500, "VCCT Temperature" },
> +	{ 0x210, 0x214, 0x218, 0x0, 0x0, 500, "1.8v Temperature" },
> +	{ 0x22c, 0x230, 0x234, 0x0, 0x0, 500, "12v Backplane Temperature" },
> +	{ 0x248, 0x24c, 0x250, 0x0, 0x0, 500, "12v AUX Temperature" },
> +};
> +
> +static const struct m10bmc_sdata d5005bmc_in_tbl[] = {
> +	{ 0x184, 0x0, 0x0, 0x0, 0x0, 1, "QSFP0 Supply Voltage" },
> +	{ 0x194, 0x0, 0x0, 0x0, 0x0, 1, "QSFP1 Supply Voltage" },
> +	{ 0x198, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Voltage" },
> +	{ 0x1ac, 0x1b0, 0x1b4, 0x0, 0x0, 1, "3.3v Voltage" },
> +	{ 0x1c8, 0x1cc, 0x1d0, 0x0, 0x0, 1, "VCCERAM Voltage" },
> +	{ 0x1e4, 0x1e8, 0x1ec, 0x0, 0x0, 1, "VCCR Voltage" },
> +	{ 0x200, 0x204, 0x208, 0x0, 0x0, 1, "VCCT Voltage" },
> +	{ 0x21c, 0x220, 0x224, 0x0, 0x0, 1, "1.8v Voltage" },
> +	{ 0x238, 0x0, 0x0, 0x0, 0x23c, 1, "12v Backplane Voltage" },
> +	{ 0x254, 0x0, 0x0, 0x0, 0x258, 1, "12v AUX Voltage" },
> +};
> +
> +static const struct m10bmc_sdata d5005bmc_curr_tbl[] = {
> +	{ 0x19c, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Current" },
> +	{ 0x1b8, 0x0, 0x0, 0x0, 0x0, 1, "3.3v Current" },
> +	{ 0x1d4, 0x0, 0x0, 0x0, 0x0, 1, "VCCERAM Current" },
> +	{ 0x1f0, 0x0, 0x0, 0x0, 0x0, 1, "VCCR Current" },
> +	{ 0x20c, 0x0, 0x0, 0x0, 0x0, 1, "VCCT Current" },
> +	{ 0x228, 0x0, 0x0, 0x0, 0x0, 1, "1.8v Current" },
> +	{ 0x240, 0x244, 0x0, 0x0, 0x0, 1, "12v Backplane Current" },
> +	{ 0x25c, 0x260, 0x0, 0x0, 0x0, 1, "12v AUX Current" },
> +};
> +
>  static const struct m10bmc_hwmon_board_data n3000bmc_hwmon_bdata = {
>  	.tables = {
>  		[hwmon_temp] = n3000bmc_temp_tbl,
> @@ -110,6 +154,80 @@ struct m10bmc_hwmon {
>  	.hinfo = n3000bmc_hinfo,
>  };
>  
> +static const struct hwmon_channel_info *d5005bmc_hinfo[] = {
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
> +			   HWMON_T_LABEL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
> +			   HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL),
> +	NULL
> +};
> +
> +static const struct m10bmc_hwmon_board_data d5005bmc_hwmon_bdata = {
> +	.tables = {
> +		[hwmon_temp] = d5005bmc_temp_tbl,
> +		[hwmon_in] = d5005bmc_in_tbl,
> +		[hwmon_curr] = d5005bmc_curr_tbl,
> +	},
> +
> +	.hinfo = d5005bmc_hinfo,
> +};
> +
>  static umode_t
>  m10bmc_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
>  			u32 attr, int channel)
> @@ -316,6 +434,10 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
>  		.name = "n3000bmc-hwmon",
>  		.driver_data = (unsigned long)&n3000bmc_hwmon_bdata,
>  	},
> +	{
> +		.name = "d5005bmc-hwmon",
> +		.driver_data = (unsigned long)&d5005bmc_hwmon_bdata,
> +	},
>  	{ }
>  };
>  
> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
> index 1161933..dbb4975 100644
> --- a/drivers/mfd/intel-m10-bmc.c
> +++ b/drivers/mfd/intel-m10-bmc.c
> @@ -15,6 +15,11 @@
>  
>  enum m10bmc_type {
>  	M10_N3000,
> +	M10_D5005
> +};
> +
> +static struct mfd_cell m10bmc_bmc_subdevs[] = {

Do we need a better name here? m10bmc_d5005_subdevs?

Thanks,
Yilun

> +	{ .name = "d5005bmc-hwmon" },
>  };
>  
>  static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
> @@ -183,6 +188,10 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  		cells = m10bmc_pacn3000_subdevs;
>  		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
>  		break;
> +	case M10_D5005:
> +		cells = m10bmc_bmc_subdevs;
> +		n_cell = ARRAY_SIZE(m10bmc_bmc_subdevs);
> +		break;
>  	default:
>  		return -ENODEV;
>  	}
> @@ -197,6 +206,7 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  
>  static const struct spi_device_id m10bmc_spi_id[] = {
>  	{ "m10-n3000", M10_N3000 },
> +	{ "m10-d5005", M10_D5005 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
> -- 
> 1.8.3.1

Re: [PATCH 3/3] hwmon: intel-m10-bmc-hwmon: add sensor support of Intel D5005 card

[matthew.gerlach]

Hi Yilun,


On Tue, 6 Apr 2021, Xu Yilun wrote:

> On Mon, Apr 05, 2021 at 04:53:01PM -0700, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Like the Intel N3000 card, the Intel D5005 has a MAX10 based
>> BMC.  This commit adds support for the D5005 sensors that are
>> monitored by the MAX10 BMC.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> Signed-off-by: Russ Weight <russell.h.weight@linux.intel.com>
>> ---
>>  drivers/hwmon/intel-m10-bmc-hwmon.c | 122 ++++++++++++++++++++++++++++++++++++
>>  drivers/mfd/intel-m10-bmc.c         |  10 +++
>>  2 files changed, 132 insertions(+)
>>
>> diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
>> index 17d5e6b..bd7ed2e 100644
>> --- a/drivers/hwmon/intel-m10-bmc-hwmon.c
>> +++ b/drivers/hwmon/intel-m10-bmc-hwmon.c
>> @@ -99,6 +99,50 @@ struct m10bmc_hwmon {
>>  	NULL
>>  };
>>
>> +static const struct m10bmc_sdata d5005bmc_temp_tbl[] = {
>> +	{ 0x100, 0x104, 0x108, 0x10c, 0x0, 500, "Board Inlet Air Temperature" },
>> +	{ 0x110, 0x114, 0x118, 0x0, 0x0, 500, "FPGA Core Temperature" },
>> +	{ 0x11c, 0x120, 0x124, 0x128, 0x0, 500, "Board Exhaust Air Temperature" },
>> +	{ 0x12c, 0x130, 0x134, 0x0, 0x0, 500, "FPGA Transceiver Temperature" },
>> +	{ 0x138, 0x13c, 0x140, 0x144, 0x0, 500, "RDIMM0 Temperature" },
>> +	{ 0x148, 0x14c, 0x150, 0x154, 0x0, 500, "RDIMM1 Temperature" },
>> +	{ 0x158, 0x15c, 0x160, 0x164, 0x0, 500, "RDIMM2 Temperature" },
>> +	{ 0x168, 0x16c, 0x170, 0x174, 0x0, 500, "RDIMM3 Temperature" },
>> +	{ 0x178, 0x17c, 0x180, 0x0, 0x0, 500, "QSFP0 Temperature" },
>> +	{ 0x188, 0x18c, 0x190, 0x0, 0x0, 500, "QSFP1 Temperature" },
>> +	{ 0x1a0, 0x1a4, 0x1a8, 0x0, 0x0, 500, "3.3v Temperature" },
>> +	{ 0x1bc, 0x1c0, 0x1c4, 0x0, 0x0, 500, "VCCERAM Temperature" },
>> +	{ 0x1d8, 0x1dc, 0x1e0, 0x0, 0x0, 500, "VCCR Temperature" },
>> +	{ 0x1f4, 0x1f8, 0x1fc, 0x0, 0x0, 500, "VCCT Temperature" },
>> +	{ 0x210, 0x214, 0x218, 0x0, 0x0, 500, "1.8v Temperature" },
>> +	{ 0x22c, 0x230, 0x234, 0x0, 0x0, 500, "12v Backplane Temperature" },
>> +	{ 0x248, 0x24c, 0x250, 0x0, 0x0, 500, "12v AUX Temperature" },
>> +};
>> +
>> +static const struct m10bmc_sdata d5005bmc_in_tbl[] = {
>> +	{ 0x184, 0x0, 0x0, 0x0, 0x0, 1, "QSFP0 Supply Voltage" },
>> +	{ 0x194, 0x0, 0x0, 0x0, 0x0, 1, "QSFP1 Supply Voltage" },
>> +	{ 0x198, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Voltage" },
>> +	{ 0x1ac, 0x1b0, 0x1b4, 0x0, 0x0, 1, "3.3v Voltage" },
>> +	{ 0x1c8, 0x1cc, 0x1d0, 0x0, 0x0, 1, "VCCERAM Voltage" },
>> +	{ 0x1e4, 0x1e8, 0x1ec, 0x0, 0x0, 1, "VCCR Voltage" },
>> +	{ 0x200, 0x204, 0x208, 0x0, 0x0, 1, "VCCT Voltage" },
>> +	{ 0x21c, 0x220, 0x224, 0x0, 0x0, 1, "1.8v Voltage" },
>> +	{ 0x238, 0x0, 0x0, 0x0, 0x23c, 1, "12v Backplane Voltage" },
>> +	{ 0x254, 0x0, 0x0, 0x0, 0x258, 1, "12v AUX Voltage" },
>> +};
>> +
>> +static const struct m10bmc_sdata d5005bmc_curr_tbl[] = {
>> +	{ 0x19c, 0x0, 0x0, 0x0, 0x0, 1, "FPGA Core Current" },
>> +	{ 0x1b8, 0x0, 0x0, 0x0, 0x0, 1, "3.3v Current" },
>> +	{ 0x1d4, 0x0, 0x0, 0x0, 0x0, 1, "VCCERAM Current" },
>> +	{ 0x1f0, 0x0, 0x0, 0x0, 0x0, 1, "VCCR Current" },
>> +	{ 0x20c, 0x0, 0x0, 0x0, 0x0, 1, "VCCT Current" },
>> +	{ 0x228, 0x0, 0x0, 0x0, 0x0, 1, "1.8v Current" },
>> +	{ 0x240, 0x244, 0x0, 0x0, 0x0, 1, "12v Backplane Current" },
>> +	{ 0x25c, 0x260, 0x0, 0x0, 0x0, 1, "12v AUX Current" },
>> +};
>> +
>>  static const struct m10bmc_hwmon_board_data n3000bmc_hwmon_bdata = {
>>  	.tables = {
>>  		[hwmon_temp] = n3000bmc_temp_tbl,
>> @@ -110,6 +154,80 @@ struct m10bmc_hwmon {
>>  	.hinfo = n3000bmc_hinfo,
>>  };
>>
>> +static const struct hwmon_channel_info *d5005bmc_hinfo[] = {
>> +	HWMON_CHANNEL_INFO(temp,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
>> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
>> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
>> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
>> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
>> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MAX_HYST |
>> +			   HWMON_T_CRIT | HWMON_T_CRIT_HYST | HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL,
>> +			   HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT |
>> +			   HWMON_T_LABEL),
>> +	HWMON_CHANNEL_INFO(in,
>> +			   HWMON_I_INPUT | HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
>> +			   HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
>> +			   HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
>> +			   HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
>> +			   HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_MAX | HWMON_I_CRIT |
>> +			   HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL,
>> +			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_LABEL),
>> +	HWMON_CHANNEL_INFO(curr,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL,
>> +			   HWMON_C_INPUT | HWMON_C_MAX | HWMON_C_LABEL),
>> +	NULL
>> +};
>> +
>> +static const struct m10bmc_hwmon_board_data d5005bmc_hwmon_bdata = {
>> +	.tables = {
>> +		[hwmon_temp] = d5005bmc_temp_tbl,
>> +		[hwmon_in] = d5005bmc_in_tbl,
>> +		[hwmon_curr] = d5005bmc_curr_tbl,
>> +	},
>> +
>> +	.hinfo = d5005bmc_hinfo,
>> +};
>> +
>>  static umode_t
>>  m10bmc_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
>>  			u32 attr, int channel)
>> @@ -316,6 +434,10 @@ static int m10bmc_hwmon_probe(struct platform_device *pdev)
>>  		.name = "n3000bmc-hwmon",
>>  		.driver_data = (unsigned long)&n3000bmc_hwmon_bdata,
>>  	},
>> +	{
>> +		.name = "d5005bmc-hwmon",
>> +		.driver_data = (unsigned long)&d5005bmc_hwmon_bdata,
>> +	},
>>  	{ }
>>  };
>>
>> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c
>> index 1161933..dbb4975 100644
>> --- a/drivers/mfd/intel-m10-bmc.c
>> +++ b/drivers/mfd/intel-m10-bmc.c
>> @@ -15,6 +15,11 @@
>>
>>  enum m10bmc_type {
>>  	M10_N3000,
>> +	M10_D5005
>> +};
>> +
>> +static struct mfd_cell m10bmc_bmc_subdevs[] = {
>
> Do we need a better name here? m10bmc_d5005_subdevs?
>
> Thanks,
> Yilun

I agree that m10bmc_d5005_subdevs is a much better name.  I will change 
it.

>
>> +	{ .name = "d5005bmc-hwmon" },
>>  };
>>
>>  static struct mfd_cell m10bmc_pacn3000_subdevs[] = {
>> @@ -183,6 +188,10 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>>  		cells = m10bmc_pacn3000_subdevs;
>>  		n_cell = ARRAY_SIZE(m10bmc_pacn3000_subdevs);
>>  		break;
>> +	case M10_D5005:
>> +		cells = m10bmc_bmc_subdevs;
>> +		n_cell = ARRAY_SIZE(m10bmc_bmc_subdevs);
>> +		break;
>>  	default:
>>  		return -ENODEV;
>>  	}
>> @@ -197,6 +206,7 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>>
>>  static const struct spi_device_id m10bmc_spi_id[] = {
>>  	{ "m10-n3000", M10_N3000 },
>> +	{ "m10-d5005", M10_D5005 },
>>  	{ }
>>  };
>>  MODULE_DEVICE_TABLE(spi, m10bmc_spi_id);
>> --
>> 1.8.3.1
>