Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Zhang, Tianfei" <tianfei.zhang@intel.com>
To:     "Xu, Yilun" <yilun.xu@intel.com>
CC:     "lee.jones@linaro.org" <lee.jones@linaro.org>,
        "Wu, Hao" <hao.wu@intel.com>, "trix@redhat.com" <trix@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "Weight, Russell H" <russell.h.weight@intel.com>,
        "matthew.gerlach@linux.intel.com" <matthew.gerlach@linux.intel.com>
Subject: RE: [PATCH v2 4/4] mfd: intel-m10-bmc: support multiple register
 layouts
Thread-Topic: [PATCH v2 4/4] mfd: intel-m10-bmc: support multiple register
 layouts
Thread-Index: AQHYge8UJIM6KFNt7kqviVQUxyhnm61YXZeAgADDbHA=
Date:   Tue, 21 Jun 2022 02:05:22 +0000
Message-ID: <BN9PR11MB54830DB2418CA5321F489C19E3B39@BN9PR11MB5483.namprd11.prod.outlook.com>
References: <20220617020405.128352-1-tianfei.zhang@intel.com>
 <20220617020405.128352-5-tianfei.zhang@intel.com>
 <20220620141919.GA1417169@yilunxu-OptiPlex-7050>
In-Reply-To: <20220620141919.GA1417169@yilunxu-OptiPlex-7050>
Accept-Language: zh-CN, en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-version: 11.6.500.17
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 2
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?jP96JacSJ63RdwsfE7CEv+vudt7L1NYCo/NY8ZP6v62GCLxC7BM/W1H48VDZ?=
 =?us-ascii?Q?Bas3Sb5MBQLdU0Cs4vDaMOCyiKXlg2+AkV+8TE6BCYsPSklI3m95pASViKXr?=
 =?us-ascii?Q?pCnIOrJ8qKINQlM8rnbHz6aUKF38+Q9iJIl2cWfX8E9YeBgr9fqOUUFyZklz?=
 =?us-ascii?Q?ok0ltcA1kCaJrQ6w8/nulCFqADkXJgRjxpUB76vShUMQ2oiuMSTXG2mhIxHg?=
 =?us-ascii?Q?/gFDukV9sC/Q5Hq8sUrhWt7O23HmO7DGPtiK+WqaltuUaOHQ+ayr2WkB0bk0?=
 =?us-ascii?Q?eY9y7XKnE4yAgD6gfnP2osGAsJ2ShYs7Sk+sKZWnH7CwIBY4iPw+MhDtLoL6?=
 =?us-ascii?Q?9ZUZZwJ00atWFmLmvnSGkBrThsvjPEsaZXGFnv7lr9MRvzpv4dV6yWW/X/w6?=
 =?us-ascii?Q?RKdWL1dTRlvcDrXTL/MxAM1xYyLRX1JKRjOehYXqKX4xILbjKWmZMy3MgXCq?=
 =?us-ascii?Q?R8r5BqCFpLyf/+JiMahfMerrcXTAZQ53aEWENkuHyps899wKlprOymlyIMh7?=
 =?us-ascii?Q?XuomCdiZjHKqWYZU3F2LQh+mGUTuqgARBUJiqFNu4Q67hwU8thj89VizCxG3?=
 =?us-ascii?Q?7YfyRJFU+51NaMeRnBamnw1cDfnSU3Bjd0QTVZinktj5ibkOy2i+x0XkR5oY?=
 =?us-ascii?Q?0+4Fr3AzbSKNQ7C+eLK3m1jaJLKkPGnRLzgAayIxlEAsMaxc0t55q6D2T7Ho?=
 =?us-ascii?Q?GK5GSWllDYw9ZSDqTAh9KJIu65+yWglruQfaD/SM9cC67aE4vVc8m14ZjrY7?=
 =?us-ascii?Q?SfZ1n0RogI7TMlJeUUZcvQo6Dtbpj5OP+K7GRGH6Ok8NjNZms00U9XLbG+sF?=
 =?us-ascii?Q?bWcLhk5Zrgu+7FnkwlAjYATfpI1bSgzpy0zwIr/0Ts8i/ywBw71ukA5OTmX3?=
 =?us-ascii?Q?k/hSSsPnpjxG6Fc7BQoimv9KkBfAylZsbEjJiJuxD9Hju7/KPD3CkpTrUHK1?=
 =?us-ascii?Q?iJ1uAlPT/JjRvEUzu3xJuWceQ84DxwtBervmdytpwLG3BIT0PEnFz3SgX+EA?=
 =?us-ascii?Q?6Wz6a1tmGxetP6zejil+ZZGhZdQxznaAbUiLJ4q+KE8r4BMR8BqjCEIq8CjG?=
 =?us-ascii?Q?J6+sNh2BLehEbTAwfd+isTRQygzz6IWO+jDd9RVVJaSkNlZ7EYmYt1IDIXtI?=
 =?us-ascii?Q?SvBu5AENi7d948DG+2rNyZIdg4CPscxrg2rJ+FJllyf5haCBHCbuzdskG8Q6?=
 =?us-ascii?Q?ycqyIrlMhs/sLPIf5SvfLVS7GvyQeVP8Pb6qtsVIIyt3eeUWkUOectd8eYdc?=
 =?us-ascii?Q?IX/HuoSU9/y6eSMip/83AWsj8sMQEbRYct2x8Sobjq46VGDqH2omkVIW61/d?=
 =?us-ascii?Q?ZDbsbs6zuwBulTiey/P40NCD4YGa44wKoXNM1lX/PIs/mKPoRSQC18SZWArT?=
 =?us-ascii?Q?HwduezdOv2oRDb5pkfcZpSu4Fw6t03zsvVBOrglkGHx1voU70YqM4TDLOhSk?=
 =?us-ascii?Q?vDkfhpoYDFeJOy/YkPBrFyUdjn8qpzyRY7+9XH4NxCh7XXpQZGuDkVr2lcpi?=
 =?us-ascii?Q?IjpL58qAc+2jizyutllW2QFzdnWpoozKr++sUJCObdVMzuyLhGW9eSKGbl4j?=
 =?us-ascii?Q?9Qe7EUAvI2xMEWuq1u08VKyoxdq2Pe0uUF3ixD320kg4PYduYHkUZ8GbkzbD?=
 =?us-ascii?Q?FI79Ni5HLU72QITl9oqwIalTtLWgz24e+ZmwUZ0kPRxBggwJ5nmztZffYf5f?=
 =?us-ascii?Q?xpeFhK9lRAP2AQZ84KquPzb2+VVKtTGJYGOyNwl1Xj/ZIReQ1LIK5MdPXynx?=
 =?us-ascii?Q?HQm8AtuqjdJzEIGtlSIB6k6aX5CMMlykzDsk6bWdHNZo/zssM+n6m41HCYIJ?=
x-ms-exchange-antispam-messagedata-1: GTrEcumKrlnyJQ==
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5483.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 12950eb1-51ad-4fc9-2064-08da532a7fe8
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Jun 2022 02:05:22.5841
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: r1souK7zB5cEdoyyzVWx4nrwA6dH8iB1OSfAtX1zs4zbgciYgtIizopnQGhyGUmN8EVAmkQA5tM8nwORlezwLQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5061
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Xu, Yilun <yilun.xu@intel.com>
> Sent: Monday, June 20, 2022 10:19 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>
> Cc: lee.jones@linaro.org; Wu, Hao <hao.wu@intel.com>; trix@redhat.com;
> linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org; Weight, Russell=
 H
> <russell.h.weight@intel.com>; matthew.gerlach@linux.intel.com
> Subject: Re: [PATCH v2 4/4] mfd: intel-m10-bmc: support multiple register
> layouts
>=20
> On Thu, Jun 16, 2022 at 10:04:05PM -0400, Tianfei Zhang wrote:
> > There are different base addresses for the MAX10 CSR register.
>=20
> Actually I see differences for each register, not only the register base.=
..
>=20
> > 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.
>=20
> Since the new BMC has different connections to host, different register l=
ayouts,
> different sub devices. Actually I can hardly find anything to share betwe=
en 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 interfa=
ce other is PMCI bus interface.
They are different register layouts. But the working flow of some functiona=
lity  are very similar, and just slight different, for example,
access the BMC version/MAC address, the doorbell mechanism of read/write th=
e flash and so on.

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

>=20
> Thanks,
> Yilun
>=20
> >
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> >  drivers/mfd/intel-m10-bmc-core.c  | 30 +++++++++++++++++++++++++-----
> > include/linux/mfd/intel-m10-bmc.h | 20 +++++++++++++++++++-
> >  2 files changed, 44 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mfd/intel-m10-bmc-core.c
> > b/drivers/mfd/intel-m10-bmc-core.c
> > index c6a1a4c28357..f85f8e2aa9a1 100644
> > --- a/drivers/mfd/intel-m10-bmc-core.c
> > +++ b/drivers/mfd/intel-m10-bmc-core.c
> > @@ -10,6 +10,22 @@
> >  #include <linux/mfd/intel-m10-bmc.h>
> >  #include <linux/module.h>
> >
> > +static const struct m10bmc_csr m10bmc_pmci_csr =3D {
> > +	.base =3D M10BMC_PMCI_SYS_BASE,
> > +	.build_version =3D M10BMC_PMCI_BUILD_VER,
> > +	.fw_version =3D NIOS2_PMCI_FW_VERSION,
> > +	.mac_low =3D M10BMC_PMCI_MAC_LOW,
> > +	.mac_high =3D M10BMC_PMCI_MAC_HIGH,
> > +};
> > +
> > +static const struct m10bmc_csr m10bmc_spi_csr =3D {
> > +	.base =3D M10BMC_SYS_BASE,
> > +	.build_version =3D M10BMC_BUILD_VER,
> > +	.fw_version =3D NIOS2_FW_VERSION,
> > +	.mac_low =3D M10BMC_MAC_LOW,
> > +	.mac_high =3D M10BMC_MAC_HIGH,
> > +};
> > +
> >  static struct mfd_cell m10bmc_n6000_bmc_subdevs[] =3D {
> >  	{ .name =3D "n6000bmc-hwmon" },
> >  	{ .name =3D "n6000bmc-sec-update" }
> > @@ -36,7 +52,7 @@ static ssize_t bmc_version_show(struct device *dev,
> >  	unsigned int val;
> >  	int ret;
> >
> > -	ret =3D m10bmc_sys_read(ddata, M10BMC_BUILD_VER, &val);
> > +	ret =3D 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 =3D m10bmc_sys_read(ddata, NIOS2_FW_VERSION, &val);
> > +	ret =3D 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 =3D m10bmc_sys_read(ddata, M10BMC_MAC_LOW, &macaddr_low);
> > +	ret =3D m10bmc_sys_read(ddata, ddata->csr->mac_low, &macaddr_low);
> >  	if (ret)
> >  		return ret;
> >
> > -	ret =3D m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
> > +	ret =3D 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 =3D m10bmc_sys_read(ddata, M10BMC_MAC_HIGH, &macaddr_high);
> > +	ret =3D 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 =3D m10bmc_pacn3000_subdevs;
> >  		n_cell =3D ARRAY_SIZE(m10bmc_pacn3000_subdevs);
> > +		m10bmc->csr =3D &m10bmc_spi_csr;
> >  		break;
> >  	case M10_D5005:
> >  		cells =3D m10bmc_d5005_subdevs;
> >  		n_cell =3D ARRAY_SIZE(m10bmc_d5005_subdevs);
> > +		m10bmc->csr =3D &m10bmc_spi_csr;
> >  		break;
> >  	case M10_N5010:
> >  		cells =3D m10bmc_n5010_subdevs;
> >  		n_cell =3D ARRAY_SIZE(m10bmc_n5010_subdevs);
> > +		m10bmc->csr =3D &m10bmc_spi_csr;
> >  		break;
> >  	case M10_N6000:
> >  		cells =3D m10bmc_n6000_bmc_subdevs;
> >  		n_cell =3D ARRAY_SIZE(m10bmc_n6000_bmc_subdevs);
> > +		m10bmc->csr =3D &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