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

[Zhang, Tianfei]

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

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

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

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