[v3,6/9] mfd: intel-m10-bmc: Downscope SPI defines & prefix with M10BMC_SPI

Message ID 20221202100841.4741-7-ilpo.jarvinen@linux.intel.com
State New
Headers
Series intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support |

Commit Message

Ilpo Järvinen Dec. 2, 2022, 10:08 a.m. UTC
  Move SPI based board definitions to per interface file from the global
header. This makes it harder to use them accidently in the
generic/interface agnostic code. Prefix the defines with M10BMC_SPI
to make it more obvious these are related to SPI only.

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

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

Comments

Xu Yilun Dec. 2, 2022, 4:28 p.m. UTC | #1
On 2022-12-02 at 12:08:38 +0200, Ilpo Järvinen wrote:
> Move SPI based board definitions to per interface file from the global
> header. This makes it harder to use them accidently in the
> generic/interface agnostic code. Prefix the defines with M10BMC_SPI

I'm not sure if the register layout is actually bound to the bus
interface. My experience is the register layout is always decided by
board type. Is it possible there will be a new SPI based board but
has different register layout in future?

So is M10BMC_SPI_XXX a good name?

The same concern for PMCI in patch #7.

Thanks,
Yilun

> to make it more obvious these are related to SPI only.
> 
> Some bitfield defs are also moved to intel-m10-bmc-core which seems
> more appropriate for them.
> 
> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/mfd/intel-m10-bmc-core.c  | 11 ++++
>  drivers/mfd/intel-m10-bmc-spi.c   | 87 ++++++++++++++++++++++---------
>  include/linux/mfd/intel-m10-bmc.h | 46 ----------------
>  3 files changed, 73 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/mfd/intel-m10-bmc-core.c b/drivers/mfd/intel-m10-bmc-core.c
> index 51b78b868235..50a4ec758bdb 100644
> --- a/drivers/mfd/intel-m10-bmc-core.c
> +++ b/drivers/mfd/intel-m10-bmc-core.c
> @@ -12,6 +12,17 @@
>  #include <linux/mfd/intel-m10-bmc.h>
>  #include <linux/module.h>
>  
> +/* Register fields of system registers */
> +#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
> +#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
> +#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
> +#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
> +#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
> +#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
> +#define M10BMC_MAC_COUNT		GENMASK(23, 16)
> +#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
> +#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
> +
>  static ssize_t bmc_version_show(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
> index 611a4ab42717..e99fe7c43314 100644
> --- a/drivers/mfd/intel-m10-bmc-spi.c
> +++ b/drivers/mfd/intel-m10-bmc-spi.c
> @@ -13,10 +13,47 @@
>  #include <linux/regmap.h>
>  #include <linux/spi/spi.h>
>  
> +#define M10BMC_SPI_LEGACY_BUILD_VER	0x300468
> +#define M10BMC_SPI_SYS_BASE		0x300800
> +#define M10BMC_SPI_SYS_END		0x300fff
> +#define M10BMC_SPI_FLASH_BASE		0x10000000
> +#define M10BMC_SPI_FLASH_END		0x1fffffff
> +#define M10BMC_SPI_MEM_END		M10BMC_SPI_FLASH_END
> +
> +/* Register offset of system registers */
> +#define NIOS2_FW_VERSION		0x0
> +#define M10BMC_SPI_MAC_LOW		0x10
> +#define M10BMC_SPI_MAC_HIGH		0x14
> +#define M10BMC_SPI_TEST_REG		0x3c
> +#define M10BMC_SPI_BUILD_VER		0x68
> +#define M10BMC_SPI_VER_LEGACY_INVALID	0xffffffff
> +
> +/* Secure update doorbell register, in system register region */
> +#define M10BMC_SPI_DOORBELL		0x400
> +
> +/* Authorization Result register, in system register region */
> +#define M10BMC_SPI_AUTH_RESULT		0x404
> +
> +/* Addresses for security related data in FLASH */
> +#define M10BMC_SPI_BMC_REH_ADDR		0x17ffc004
> +#define M10BMC_SPI_BMC_PROG_ADDR	0x17ffc000
> +#define M10BMC_SPI_BMC_PROG_MAGIC	0x5746
> +
> +#define M10BMC_SPI_SR_REH_ADDR		0x17ffd004
> +#define M10BMC_SPI_SR_PROG_ADDR		0x17ffd000
> +#define M10BMC_SPI_SR_PROG_MAGIC	0x5253
> +
> +#define M10BMC_SPI_PR_REH_ADDR		0x17ffe004
> +#define M10BMC_SPI_PR_PROG_ADDR		0x17ffe000
> +#define M10BMC_SPI_PR_PROG_MAGIC	0x5250
> +
> +/* Address of 4KB inverted bit vector containing staging area FLASH count */
> +#define M10BMC_SPI_STAGING_FLASH_COUNT	0x17ffb000
> +
>  static const struct regmap_range m10bmc_regmap_range[] = {
> -	regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER),
> -	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
> -	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END),
> +	regmap_reg_range(M10BMC_SPI_LEGACY_BUILD_VER, M10BMC_SPI_LEGACY_BUILD_VER),
> +	regmap_reg_range(M10BMC_SPI_SYS_BASE, M10BMC_SPI_SYS_END),
> +	regmap_reg_range(M10BMC_SPI_FLASH_BASE, M10BMC_SPI_FLASH_END),
>  };
>  
>  static const struct regmap_access_table m10bmc_access_table = {
> @@ -30,7 +67,7 @@ static struct regmap_config intel_m10bmc_regmap_config = {
>  	.reg_stride = 4,
>  	.wr_table = &m10bmc_access_table,
>  	.rd_table = &m10bmc_access_table,
> -	.max_register = M10BMC_MEM_END,
> +	.max_register = M10BMC_SPI_MEM_END,
>  };
>  
>  static int check_m10bmc_version(struct intel_m10bmc *ddata)
> @@ -41,16 +78,16 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
>  	/*
>  	 * This check is to filter out the very old legacy BMC versions. In the
>  	 * old BMC chips, the BMC version info is stored in the old version
> -	 * register (M10BMC_LEGACY_BUILD_VER), so its read out value would have
> -	 * not been M10BMC_VER_LEGACY_INVALID (0xffffffff). But in new BMC
> +	 * register (M10BMC_SPI_LEGACY_BUILD_VER), so its read out value would have
> +	 * not been M10BMC_SPI_VER_LEGACY_INVALID (0xffffffff). But in new BMC
>  	 * chips that the driver supports, the value of this register should be
> -	 * M10BMC_VER_LEGACY_INVALID.
> +	 * M10BMC_SPI_VER_LEGACY_INVALID.
>  	 */
> -	ret = m10bmc_raw_read(ddata, M10BMC_LEGACY_BUILD_VER, &v);
> +	ret = m10bmc_raw_read(ddata, M10BMC_SPI_LEGACY_BUILD_VER, &v);
>  	if (ret)
>  		return -ENODEV;
>  
> -	if (v != M10BMC_VER_LEGACY_INVALID) {
> +	if (v != M10BMC_SPI_VER_LEGACY_INVALID) {
>  		dev_err(ddata->dev, "bad version M10BMC detected\n");
>  		return -ENODEV;
>  	}
> @@ -92,23 +129,23 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>  }
>  
>  static const struct m10bmc_csr_map m10bmc_spi_csr_map = {
> -	.base = M10BMC_SYS_BASE,
> -	.build_version = M10BMC_BUILD_VER,
> +	.base = M10BMC_SPI_SYS_BASE,
> +	.build_version = M10BMC_SPI_BUILD_VER,
>  	.fw_version = NIOS2_FW_VERSION,
> -	.mac_low = M10BMC_MAC_LOW,
> -	.mac_high = M10BMC_MAC_HIGH,
> -	.doorbell = M10BMC_DOORBELL,
> -	.auth_result = M10BMC_AUTH_RESULT,
> -	.bmc_prog_addr = BMC_PROG_ADDR,
> -	.bmc_reh_addr = BMC_REH_ADDR,
> -	.bmc_magic = BMC_PROG_MAGIC,
> -	.sr_prog_addr = SR_PROG_ADDR,
> -	.sr_reh_addr = SR_REH_ADDR,
> -	.sr_magic = SR_PROG_MAGIC,
> -	.pr_prog_addr = PR_PROG_ADDR,
> -	.pr_reh_addr = PR_REH_ADDR,
> -	.pr_magic = PR_PROG_MAGIC,
> -	.rsu_update_counter = STAGING_FLASH_COUNT,
> +	.mac_low = M10BMC_SPI_MAC_LOW,
> +	.mac_high = M10BMC_SPI_MAC_HIGH,
> +	.doorbell = M10BMC_SPI_DOORBELL,
> +	.auth_result = M10BMC_SPI_AUTH_RESULT,
> +	.bmc_prog_addr = M10BMC_SPI_BMC_PROG_ADDR,
> +	.bmc_reh_addr = M10BMC_SPI_BMC_REH_ADDR,
> +	.bmc_magic = M10BMC_SPI_BMC_PROG_MAGIC,
> +	.sr_prog_addr = M10BMC_SPI_SR_PROG_ADDR,
> +	.sr_reh_addr = M10BMC_SPI_SR_REH_ADDR,
> +	.sr_magic = M10BMC_SPI_SR_PROG_MAGIC,
> +	.pr_prog_addr = M10BMC_SPI_PR_PROG_ADDR,
> +	.pr_reh_addr = M10BMC_SPI_PR_REH_ADDR,
> +	.pr_magic = M10BMC_SPI_PR_PROG_MAGIC,
> +	.rsu_update_counter = M10BMC_SPI_STAGING_FLASH_COUNT,
>  };
>  
>  static struct mfd_cell m10bmc_d5005_subdevs[] = {
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index 91567375f1bf..71ace732bb48 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -9,39 +9,9 @@
>  
>  #include <linux/regmap.h>
>  
> -#define M10BMC_LEGACY_BUILD_VER		0x300468
> -#define M10BMC_SYS_BASE			0x300800
> -#define M10BMC_SYS_END			0x300fff
> -#define M10BMC_FLASH_BASE		0x10000000
> -#define M10BMC_FLASH_END		0x1fffffff
> -#define M10BMC_MEM_END			M10BMC_FLASH_END
> -
>  #define M10BMC_STAGING_BASE		0x18000000
>  #define M10BMC_STAGING_SIZE		0x3800000
>  
> -/* Register offset of system registers */
> -#define NIOS2_FW_VERSION		0x0
> -#define M10BMC_MAC_LOW			0x10
> -#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
> -#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
> -#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
> -#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
> -#define M10BMC_MAC_HIGH			0x14
> -#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
> -#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
> -#define M10BMC_MAC_COUNT		GENMASK(23, 16)
> -#define M10BMC_TEST_REG			0x3c
> -#define M10BMC_BUILD_VER		0x68
> -#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
> -#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
> -#define M10BMC_VER_LEGACY_INVALID	0xffffffff
> -
> -/* Secure update doorbell register, in system register region */
> -#define M10BMC_DOORBELL			0x400
> -
> -/* Authorization Result register, in system register region */
> -#define M10BMC_AUTH_RESULT		0x404
> -
>  /* Doorbell register fields */
>  #define DRBL_RSU_REQUEST		BIT(0)
>  #define DRBL_RSU_PROGRESS		GENMASK(7, 4)
> @@ -102,22 +72,6 @@
>  #define RSU_COMPLETE_INTERVAL_MS	1000
>  #define RSU_COMPLETE_TIMEOUT_MS		(40 * 60 * 1000)
>  
> -/* Addresses for security related data in FLASH */
> -#define BMC_REH_ADDR	0x17ffc004
> -#define BMC_PROG_ADDR	0x17ffc000
> -#define BMC_PROG_MAGIC	0x5746
> -
> -#define SR_REH_ADDR	0x17ffd004
> -#define SR_PROG_ADDR	0x17ffd000
> -#define SR_PROG_MAGIC	0x5253
> -
> -#define PR_REH_ADDR	0x17ffe004
> -#define PR_PROG_ADDR	0x17ffe000
> -#define PR_PROG_MAGIC	0x5250
> -
> -/* Address of 4KB inverted bit vector containing staging area FLASH count */
> -#define STAGING_FLASH_COUNT	0x17ffb000
> -
>  /**
>   * struct m10bmc_csr_map - Intel MAX 10 BMC CSR register map
>   */
> -- 
> 2.30.2
>
  
Russ Weight Dec. 2, 2022, 5:29 p.m. UTC | #2
On 12/2/22 08:28, Xu Yilun wrote:
> On 2022-12-02 at 12:08:38 +0200, Ilpo Järvinen wrote:
>> Move SPI based board definitions to per interface file from the global
>> header. This makes it harder to use them accidently in the
>> generic/interface agnostic code. Prefix the defines with M10BMC_SPI
> I'm not sure if the register layout is actually bound to the bus
> interface. My experience is the register layout is always decided by
> board type. Is it possible there will be a new SPI based board but
> has different register layout in future?
>
> So is M10BMC_SPI_XXX a good nam

There could be future devices, spi or pmci based, that require different
addresses for some of these values, and at that time we would need to
additional versions of some of these macros using different names.
Right now, spi and pmci are the primary differentiating factors. I'm not
sure how to improve on the naming. Do you have any suggestions?

- Russ

>
> The same concern for PMCI in patch #7.
>
> Thanks,
> Yilun
>
>> to make it more obvious these are related to SPI only.
>>
>> Some bitfield defs are also moved to intel-m10-bmc-core which seems
>> more appropriate for them.
>>
>> Reviewed-by: Russ Weight <russell.h.weight@intel.com>
>> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>>  drivers/mfd/intel-m10-bmc-core.c  | 11 ++++
>>  drivers/mfd/intel-m10-bmc-spi.c   | 87 ++++++++++++++++++++++---------
>>  include/linux/mfd/intel-m10-bmc.h | 46 ----------------
>>  3 files changed, 73 insertions(+), 71 deletions(-)
>>
>> diff --git a/drivers/mfd/intel-m10-bmc-core.c b/drivers/mfd/intel-m10-bmc-core.c
>> index 51b78b868235..50a4ec758bdb 100644
>> --- a/drivers/mfd/intel-m10-bmc-core.c
>> +++ b/drivers/mfd/intel-m10-bmc-core.c
>> @@ -12,6 +12,17 @@
>>  #include <linux/mfd/intel-m10-bmc.h>
>>  #include <linux/module.h>
>>  
>> +/* Register fields of system registers */
>> +#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
>> +#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
>> +#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
>> +#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
>> +#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
>> +#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
>> +#define M10BMC_MAC_COUNT		GENMASK(23, 16)
>> +#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
>> +#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
>> +
>>  static ssize_t bmc_version_show(struct device *dev,
>>  				struct device_attribute *attr, char *buf)
>>  {
>> diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
>> index 611a4ab42717..e99fe7c43314 100644
>> --- a/drivers/mfd/intel-m10-bmc-spi.c
>> +++ b/drivers/mfd/intel-m10-bmc-spi.c
>> @@ -13,10 +13,47 @@
>>  #include <linux/regmap.h>
>>  #include <linux/spi/spi.h>
>>  
>> +#define M10BMC_SPI_LEGACY_BUILD_VER	0x300468
>> +#define M10BMC_SPI_SYS_BASE		0x300800
>> +#define M10BMC_SPI_SYS_END		0x300fff
>> +#define M10BMC_SPI_FLASH_BASE		0x10000000
>> +#define M10BMC_SPI_FLASH_END		0x1fffffff
>> +#define M10BMC_SPI_MEM_END		M10BMC_SPI_FLASH_END
>> +
>> +/* Register offset of system registers */
>> +#define NIOS2_FW_VERSION		0x0
>> +#define M10BMC_SPI_MAC_LOW		0x10
>> +#define M10BMC_SPI_MAC_HIGH		0x14
>> +#define M10BMC_SPI_TEST_REG		0x3c
>> +#define M10BMC_SPI_BUILD_VER		0x68
>> +#define M10BMC_SPI_VER_LEGACY_INVALID	0xffffffff
>> +
>> +/* Secure update doorbell register, in system register region */
>> +#define M10BMC_SPI_DOORBELL		0x400
>> +
>> +/* Authorization Result register, in system register region */
>> +#define M10BMC_SPI_AUTH_RESULT		0x404
>> +
>> +/* Addresses for security related data in FLASH */
>> +#define M10BMC_SPI_BMC_REH_ADDR		0x17ffc004
>> +#define M10BMC_SPI_BMC_PROG_ADDR	0x17ffc000
>> +#define M10BMC_SPI_BMC_PROG_MAGIC	0x5746
>> +
>> +#define M10BMC_SPI_SR_REH_ADDR		0x17ffd004
>> +#define M10BMC_SPI_SR_PROG_ADDR		0x17ffd000
>> +#define M10BMC_SPI_SR_PROG_MAGIC	0x5253
>> +
>> +#define M10BMC_SPI_PR_REH_ADDR		0x17ffe004
>> +#define M10BMC_SPI_PR_PROG_ADDR		0x17ffe000
>> +#define M10BMC_SPI_PR_PROG_MAGIC	0x5250
>> +
>> +/* Address of 4KB inverted bit vector containing staging area FLASH count */
>> +#define M10BMC_SPI_STAGING_FLASH_COUNT	0x17ffb000
>> +
>>  static const struct regmap_range m10bmc_regmap_range[] = {
>> -	regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER),
>> -	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
>> -	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END),
>> +	regmap_reg_range(M10BMC_SPI_LEGACY_BUILD_VER, M10BMC_SPI_LEGACY_BUILD_VER),
>> +	regmap_reg_range(M10BMC_SPI_SYS_BASE, M10BMC_SPI_SYS_END),
>> +	regmap_reg_range(M10BMC_SPI_FLASH_BASE, M10BMC_SPI_FLASH_END),
>>  };
>>  
>>  static const struct regmap_access_table m10bmc_access_table = {
>> @@ -30,7 +67,7 @@ static struct regmap_config intel_m10bmc_regmap_config = {
>>  	.reg_stride = 4,
>>  	.wr_table = &m10bmc_access_table,
>>  	.rd_table = &m10bmc_access_table,
>> -	.max_register = M10BMC_MEM_END,
>> +	.max_register = M10BMC_SPI_MEM_END,
>>  };
>>  
>>  static int check_m10bmc_version(struct intel_m10bmc *ddata)
>> @@ -41,16 +78,16 @@ static int check_m10bmc_version(struct intel_m10bmc *ddata)
>>  	/*
>>  	 * This check is to filter out the very old legacy BMC versions. In the
>>  	 * old BMC chips, the BMC version info is stored in the old version
>> -	 * register (M10BMC_LEGACY_BUILD_VER), so its read out value would have
>> -	 * not been M10BMC_VER_LEGACY_INVALID (0xffffffff). But in new BMC
>> +	 * register (M10BMC_SPI_LEGACY_BUILD_VER), so its read out value would have
>> +	 * not been M10BMC_SPI_VER_LEGACY_INVALID (0xffffffff). But in new BMC
>>  	 * chips that the driver supports, the value of this register should be
>> -	 * M10BMC_VER_LEGACY_INVALID.
>> +	 * M10BMC_SPI_VER_LEGACY_INVALID.
>>  	 */
>> -	ret = m10bmc_raw_read(ddata, M10BMC_LEGACY_BUILD_VER, &v);
>> +	ret = m10bmc_raw_read(ddata, M10BMC_SPI_LEGACY_BUILD_VER, &v);
>>  	if (ret)
>>  		return -ENODEV;
>>  
>> -	if (v != M10BMC_VER_LEGACY_INVALID) {
>> +	if (v != M10BMC_SPI_VER_LEGACY_INVALID) {
>>  		dev_err(ddata->dev, "bad version M10BMC detected\n");
>>  		return -ENODEV;
>>  	}
>> @@ -92,23 +129,23 @@ static int intel_m10_bmc_spi_probe(struct spi_device *spi)
>>  }
>>  
>>  static const struct m10bmc_csr_map m10bmc_spi_csr_map = {
>> -	.base = M10BMC_SYS_BASE,
>> -	.build_version = M10BMC_BUILD_VER,
>> +	.base = M10BMC_SPI_SYS_BASE,
>> +	.build_version = M10BMC_SPI_BUILD_VER,
>>  	.fw_version = NIOS2_FW_VERSION,
>> -	.mac_low = M10BMC_MAC_LOW,
>> -	.mac_high = M10BMC_MAC_HIGH,
>> -	.doorbell = M10BMC_DOORBELL,
>> -	.auth_result = M10BMC_AUTH_RESULT,
>> -	.bmc_prog_addr = BMC_PROG_ADDR,
>> -	.bmc_reh_addr = BMC_REH_ADDR,
>> -	.bmc_magic = BMC_PROG_MAGIC,
>> -	.sr_prog_addr = SR_PROG_ADDR,
>> -	.sr_reh_addr = SR_REH_ADDR,
>> -	.sr_magic = SR_PROG_MAGIC,
>> -	.pr_prog_addr = PR_PROG_ADDR,
>> -	.pr_reh_addr = PR_REH_ADDR,
>> -	.pr_magic = PR_PROG_MAGIC,
>> -	.rsu_update_counter = STAGING_FLASH_COUNT,
>> +	.mac_low = M10BMC_SPI_MAC_LOW,
>> +	.mac_high = M10BMC_SPI_MAC_HIGH,
>> +	.doorbell = M10BMC_SPI_DOORBELL,
>> +	.auth_result = M10BMC_SPI_AUTH_RESULT,
>> +	.bmc_prog_addr = M10BMC_SPI_BMC_PROG_ADDR,
>> +	.bmc_reh_addr = M10BMC_SPI_BMC_REH_ADDR,
>> +	.bmc_magic = M10BMC_SPI_BMC_PROG_MAGIC,
>> +	.sr_prog_addr = M10BMC_SPI_SR_PROG_ADDR,
>> +	.sr_reh_addr = M10BMC_SPI_SR_REH_ADDR,
>> +	.sr_magic = M10BMC_SPI_SR_PROG_MAGIC,
>> +	.pr_prog_addr = M10BMC_SPI_PR_PROG_ADDR,
>> +	.pr_reh_addr = M10BMC_SPI_PR_REH_ADDR,
>> +	.pr_magic = M10BMC_SPI_PR_PROG_MAGIC,
>> +	.rsu_update_counter = M10BMC_SPI_STAGING_FLASH_COUNT,
>>  };
>>  
>>  static struct mfd_cell m10bmc_d5005_subdevs[] = {
>> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
>> index 91567375f1bf..71ace732bb48 100644
>> --- a/include/linux/mfd/intel-m10-bmc.h
>> +++ b/include/linux/mfd/intel-m10-bmc.h
>> @@ -9,39 +9,9 @@
>>  
>>  #include <linux/regmap.h>
>>  
>> -#define M10BMC_LEGACY_BUILD_VER		0x300468
>> -#define M10BMC_SYS_BASE			0x300800
>> -#define M10BMC_SYS_END			0x300fff
>> -#define M10BMC_FLASH_BASE		0x10000000
>> -#define M10BMC_FLASH_END		0x1fffffff
>> -#define M10BMC_MEM_END			M10BMC_FLASH_END
>> -
>>  #define M10BMC_STAGING_BASE		0x18000000
>>  #define M10BMC_STAGING_SIZE		0x3800000
>>  
>> -/* Register offset of system registers */
>> -#define NIOS2_FW_VERSION		0x0
>> -#define M10BMC_MAC_LOW			0x10
>> -#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
>> -#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
>> -#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
>> -#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
>> -#define M10BMC_MAC_HIGH			0x14
>> -#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
>> -#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
>> -#define M10BMC_MAC_COUNT		GENMASK(23, 16)
>> -#define M10BMC_TEST_REG			0x3c
>> -#define M10BMC_BUILD_VER		0x68
>> -#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
>> -#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
>> -#define M10BMC_VER_LEGACY_INVALID	0xffffffff
>> -
>> -/* Secure update doorbell register, in system register region */
>> -#define M10BMC_DOORBELL			0x400
>> -
>> -/* Authorization Result register, in system register region */
>> -#define M10BMC_AUTH_RESULT		0x404
>> -
>>  /* Doorbell register fields */
>>  #define DRBL_RSU_REQUEST		BIT(0)
>>  #define DRBL_RSU_PROGRESS		GENMASK(7, 4)
>> @@ -102,22 +72,6 @@
>>  #define RSU_COMPLETE_INTERVAL_MS	1000
>>  #define RSU_COMPLETE_TIMEOUT_MS		(40 * 60 * 1000)
>>  
>> -/* Addresses for security related data in FLASH */
>> -#define BMC_REH_ADDR	0x17ffc004
>> -#define BMC_PROG_ADDR	0x17ffc000
>> -#define BMC_PROG_MAGIC	0x5746
>> -
>> -#define SR_REH_ADDR	0x17ffd004
>> -#define SR_PROG_ADDR	0x17ffd000
>> -#define SR_PROG_MAGIC	0x5253
>> -
>> -#define PR_REH_ADDR	0x17ffe004
>> -#define PR_PROG_ADDR	0x17ffe000
>> -#define PR_PROG_MAGIC	0x5250
>> -
>> -/* Address of 4KB inverted bit vector containing staging area FLASH count */
>> -#define STAGING_FLASH_COUNT	0x17ffb000
>> -
>>  /**
>>   * struct m10bmc_csr_map - Intel MAX 10 BMC CSR register map
>>   */
>> -- 
>> 2.30.2
>>
  
Ilpo Järvinen Dec. 5, 2022, 9:31 a.m. UTC | #3
On Fri, 2 Dec 2022, Russ Weight wrote:
> On 12/2/22 08:28, Xu Yilun wrote:
> > On 2022-12-02 at 12:08:38 +0200, Ilpo Järvinen wrote:
> >> Move SPI based board definitions to per interface file from the global
> >> header. This makes it harder to use them accidently in the
> >> generic/interface agnostic code. Prefix the defines with M10BMC_SPI
> > I'm not sure if the register layout is actually bound to the bus
> > interface. My experience is the register layout is always decided by
> > board type. Is it possible there will be a new SPI based board but
> > has different register layout in future?
> >
> > So is M10BMC_SPI_XXX a good nam
> 
> There could be future devices, spi or pmci based, that require different
> addresses for some of these values, and at that time we would need to
> additional versions of some of these macros using different names.
> Right now, spi and pmci are the primary differentiating factors. I'm not
> sure how to improve on the naming. Do you have any suggestions?

It's per board type yes, but there's a strong clustering currently on 
spi/pmci differentiation. That implies a one define applies to multiple 
board types so naming it, e.g., after a single board type seems not much 
better than the current approach.

I've even thought myself of removing those defines as they seem one-time 
use ones after introducing the csr_map. Defining the csr_map using members
kinda documents what a literal is about if I'd put just a number there.
The added benefit a few capital letters in a define provides is IMHO very
questionable.

Also m10bmc_spi_csr_map name suffers from the same problem, BTW.

I could, of course now that they're downscoped, drop _SPI_ or _PMCI_ from 
their names if that's ok for you? ...But that wouldn't address the next 
version naming problem at all. But I don't anyway know, without a crystal 
ball, know how to address the future naming needs.
  
Xu Yilun Dec. 5, 2022, 3:41 p.m. UTC | #4
On 2022-12-05 at 11:31:06 +0200, Ilpo Järvinen wrote:
> On Fri, 2 Dec 2022, Russ Weight wrote:
> > On 12/2/22 08:28, Xu Yilun wrote:
> > > On 2022-12-02 at 12:08:38 +0200, Ilpo Järvinen wrote:
> > >> Move SPI based board definitions to per interface file from the global
> > >> header. This makes it harder to use them accidently in the
> > >> generic/interface agnostic code. Prefix the defines with M10BMC_SPI
> > > I'm not sure if the register layout is actually bound to the bus
> > > interface. My experience is the register layout is always decided by
> > > board type. Is it possible there will be a new SPI based board but
> > > has different register layout in future?
> > >
> > > So is M10BMC_SPI_XXX a good nam
> > 
> > There could be future devices, spi or pmci based, that require different
> > addresses for some of these values, and at that time we would need to
> > additional versions of some of these macros using different names.
> > Right now, spi and pmci are the primary differentiating factors. I'm not
> > sure how to improve on the naming. Do you have any suggestions?
> 
> It's per board type yes, but there's a strong clustering currently on 
> spi/pmci differentiation. That implies a one define applies to multiple 
> board types so naming it, e.g., after a single board type seems not much 
> better than the current approach.

I think it is better to name after one of the board type among all its
supported types. At least it clearly indicates they are related to board
type.

Actually it is normal for many driver modules. A driver was initially
implemented for one board type, and was named by the initial board.
But later you have more board types compatible to the driver, you don't
change the driver name, just use it.

Thanks,
Yilun

> 
> I've even thought myself of removing those defines as they seem one-time 
> use ones after introducing the csr_map. Defining the csr_map using members
> kinda documents what a literal is about if I'd put just a number there.
> The added benefit a few capital letters in a define provides is IMHO very
> questionable.
> 
> Also m10bmc_spi_csr_map name suffers from the same problem, BTW.
> 
> I could, of course now that they're downscoped, drop _SPI_ or _PMCI_ from 
> their names if that's ok for you? ...But that wouldn't address the next 
> version naming problem at all. But I don't anyway know, without a crystal 
> ball, know how to address the future naming needs.
> 
> -- 
>  i.
  
Ilpo Järvinen Dec. 8, 2022, 11:57 a.m. UTC | #5
On Mon, 5 Dec 2022, Xu Yilun wrote:

> On 2022-12-05 at 11:31:06 +0200, Ilpo Järvinen wrote:
> > On Fri, 2 Dec 2022, Russ Weight wrote:
> > > On 12/2/22 08:28, Xu Yilun wrote:
> > > > On 2022-12-02 at 12:08:38 +0200, Ilpo Järvinen wrote:
> > > >> Move SPI based board definitions to per interface file from the global
> > > >> header. This makes it harder to use them accidently in the
> > > >> generic/interface agnostic code. Prefix the defines with M10BMC_SPI
> > > > I'm not sure if the register layout is actually bound to the bus
> > > > interface. My experience is the register layout is always decided by
> > > > board type. Is it possible there will be a new SPI based board but
> > > > has different register layout in future?
> > > >
> > > > So is M10BMC_SPI_XXX a good nam
> > > 
> > > There could be future devices, spi or pmci based, that require different
> > > addresses for some of these values, and at that time we would need to
> > > additional versions of some of these macros using different names.
> > > Right now, spi and pmci are the primary differentiating factors. I'm not
> > > sure how to improve on the naming. Do you have any suggestions?
> > 
> > It's per board type yes, but there's a strong clustering currently on 
> > spi/pmci differentiation. That implies a one define applies to multiple 
> > board types so naming it, e.g., after a single board type seems not much 
> > better than the current approach.
> 
> I think it is better to name after one of the board type among all its
> supported types. At least it clearly indicates they are related to board
> type.
> 
> Actually it is normal for many driver modules. A driver was initially
> implemented for one board type, and was named by the initial board.
> But later you have more board types compatible to the driver, you don't
> change the driver name, just use it.

Ok, I'll do it that way then.
  

Patch

diff --git a/drivers/mfd/intel-m10-bmc-core.c b/drivers/mfd/intel-m10-bmc-core.c
index 51b78b868235..50a4ec758bdb 100644
--- a/drivers/mfd/intel-m10-bmc-core.c
+++ b/drivers/mfd/intel-m10-bmc-core.c
@@ -12,6 +12,17 @@ 
 #include <linux/mfd/intel-m10-bmc.h>
 #include <linux/module.h>
 
+/* Register fields of system registers */
+#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
+#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
+#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
+#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
+#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
+#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
+#define M10BMC_MAC_COUNT		GENMASK(23, 16)
+#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
+#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
+
 static ssize_t bmc_version_show(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
index 611a4ab42717..e99fe7c43314 100644
--- a/drivers/mfd/intel-m10-bmc-spi.c
+++ b/drivers/mfd/intel-m10-bmc-spi.c
@@ -13,10 +13,47 @@ 
 #include <linux/regmap.h>
 #include <linux/spi/spi.h>
 
+#define M10BMC_SPI_LEGACY_BUILD_VER	0x300468
+#define M10BMC_SPI_SYS_BASE		0x300800
+#define M10BMC_SPI_SYS_END		0x300fff
+#define M10BMC_SPI_FLASH_BASE		0x10000000
+#define M10BMC_SPI_FLASH_END		0x1fffffff
+#define M10BMC_SPI_MEM_END		M10BMC_SPI_FLASH_END
+
+/* Register offset of system registers */
+#define NIOS2_FW_VERSION		0x0
+#define M10BMC_SPI_MAC_LOW		0x10
+#define M10BMC_SPI_MAC_HIGH		0x14
+#define M10BMC_SPI_TEST_REG		0x3c
+#define M10BMC_SPI_BUILD_VER		0x68
+#define M10BMC_SPI_VER_LEGACY_INVALID	0xffffffff
+
+/* Secure update doorbell register, in system register region */
+#define M10BMC_SPI_DOORBELL		0x400
+
+/* Authorization Result register, in system register region */
+#define M10BMC_SPI_AUTH_RESULT		0x404
+
+/* Addresses for security related data in FLASH */
+#define M10BMC_SPI_BMC_REH_ADDR		0x17ffc004
+#define M10BMC_SPI_BMC_PROG_ADDR	0x17ffc000
+#define M10BMC_SPI_BMC_PROG_MAGIC	0x5746
+
+#define M10BMC_SPI_SR_REH_ADDR		0x17ffd004
+#define M10BMC_SPI_SR_PROG_ADDR		0x17ffd000
+#define M10BMC_SPI_SR_PROG_MAGIC	0x5253
+
+#define M10BMC_SPI_PR_REH_ADDR		0x17ffe004
+#define M10BMC_SPI_PR_PROG_ADDR		0x17ffe000
+#define M10BMC_SPI_PR_PROG_MAGIC	0x5250
+
+/* Address of 4KB inverted bit vector containing staging area FLASH count */
+#define M10BMC_SPI_STAGING_FLASH_COUNT	0x17ffb000
+
 static const struct regmap_range m10bmc_regmap_range[] = {
-	regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER),
-	regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END),
-	regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END),
+	regmap_reg_range(M10BMC_SPI_LEGACY_BUILD_VER, M10BMC_SPI_LEGACY_BUILD_VER),
+	regmap_reg_range(M10BMC_SPI_SYS_BASE, M10BMC_SPI_SYS_END),
+	regmap_reg_range(M10BMC_SPI_FLASH_BASE, M10BMC_SPI_FLASH_END),
 };
 
 static const struct regmap_access_table m10bmc_access_table = {
@@ -30,7 +67,7 @@  static struct regmap_config intel_m10bmc_regmap_config = {
 	.reg_stride = 4,
 	.wr_table = &m10bmc_access_table,
 	.rd_table = &m10bmc_access_table,
-	.max_register = M10BMC_MEM_END,
+	.max_register = M10BMC_SPI_MEM_END,
 };
 
 static int check_m10bmc_version(struct intel_m10bmc *ddata)
@@ -41,16 +78,16 @@  static int check_m10bmc_version(struct intel_m10bmc *ddata)
 	/*
 	 * This check is to filter out the very old legacy BMC versions. In the
 	 * old BMC chips, the BMC version info is stored in the old version
-	 * register (M10BMC_LEGACY_BUILD_VER), so its read out value would have
-	 * not been M10BMC_VER_LEGACY_INVALID (0xffffffff). But in new BMC
+	 * register (M10BMC_SPI_LEGACY_BUILD_VER), so its read out value would have
+	 * not been M10BMC_SPI_VER_LEGACY_INVALID (0xffffffff). But in new BMC
 	 * chips that the driver supports, the value of this register should be
-	 * M10BMC_VER_LEGACY_INVALID.
+	 * M10BMC_SPI_VER_LEGACY_INVALID.
 	 */
-	ret = m10bmc_raw_read(ddata, M10BMC_LEGACY_BUILD_VER, &v);
+	ret = m10bmc_raw_read(ddata, M10BMC_SPI_LEGACY_BUILD_VER, &v);
 	if (ret)
 		return -ENODEV;
 
-	if (v != M10BMC_VER_LEGACY_INVALID) {
+	if (v != M10BMC_SPI_VER_LEGACY_INVALID) {
 		dev_err(ddata->dev, "bad version M10BMC detected\n");
 		return -ENODEV;
 	}
@@ -92,23 +129,23 @@  static int intel_m10_bmc_spi_probe(struct spi_device *spi)
 }
 
 static const struct m10bmc_csr_map m10bmc_spi_csr_map = {
-	.base = M10BMC_SYS_BASE,
-	.build_version = M10BMC_BUILD_VER,
+	.base = M10BMC_SPI_SYS_BASE,
+	.build_version = M10BMC_SPI_BUILD_VER,
 	.fw_version = NIOS2_FW_VERSION,
-	.mac_low = M10BMC_MAC_LOW,
-	.mac_high = M10BMC_MAC_HIGH,
-	.doorbell = M10BMC_DOORBELL,
-	.auth_result = M10BMC_AUTH_RESULT,
-	.bmc_prog_addr = BMC_PROG_ADDR,
-	.bmc_reh_addr = BMC_REH_ADDR,
-	.bmc_magic = BMC_PROG_MAGIC,
-	.sr_prog_addr = SR_PROG_ADDR,
-	.sr_reh_addr = SR_REH_ADDR,
-	.sr_magic = SR_PROG_MAGIC,
-	.pr_prog_addr = PR_PROG_ADDR,
-	.pr_reh_addr = PR_REH_ADDR,
-	.pr_magic = PR_PROG_MAGIC,
-	.rsu_update_counter = STAGING_FLASH_COUNT,
+	.mac_low = M10BMC_SPI_MAC_LOW,
+	.mac_high = M10BMC_SPI_MAC_HIGH,
+	.doorbell = M10BMC_SPI_DOORBELL,
+	.auth_result = M10BMC_SPI_AUTH_RESULT,
+	.bmc_prog_addr = M10BMC_SPI_BMC_PROG_ADDR,
+	.bmc_reh_addr = M10BMC_SPI_BMC_REH_ADDR,
+	.bmc_magic = M10BMC_SPI_BMC_PROG_MAGIC,
+	.sr_prog_addr = M10BMC_SPI_SR_PROG_ADDR,
+	.sr_reh_addr = M10BMC_SPI_SR_REH_ADDR,
+	.sr_magic = M10BMC_SPI_SR_PROG_MAGIC,
+	.pr_prog_addr = M10BMC_SPI_PR_PROG_ADDR,
+	.pr_reh_addr = M10BMC_SPI_PR_REH_ADDR,
+	.pr_magic = M10BMC_SPI_PR_PROG_MAGIC,
+	.rsu_update_counter = M10BMC_SPI_STAGING_FLASH_COUNT,
 };
 
 static struct mfd_cell m10bmc_d5005_subdevs[] = {
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index 91567375f1bf..71ace732bb48 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -9,39 +9,9 @@ 
 
 #include <linux/regmap.h>
 
-#define M10BMC_LEGACY_BUILD_VER		0x300468
-#define M10BMC_SYS_BASE			0x300800
-#define M10BMC_SYS_END			0x300fff
-#define M10BMC_FLASH_BASE		0x10000000
-#define M10BMC_FLASH_END		0x1fffffff
-#define M10BMC_MEM_END			M10BMC_FLASH_END
-
 #define M10BMC_STAGING_BASE		0x18000000
 #define M10BMC_STAGING_SIZE		0x3800000
 
-/* Register offset of system registers */
-#define NIOS2_FW_VERSION		0x0
-#define M10BMC_MAC_LOW			0x10
-#define M10BMC_MAC_BYTE4		GENMASK(7, 0)
-#define M10BMC_MAC_BYTE3		GENMASK(15, 8)
-#define M10BMC_MAC_BYTE2		GENMASK(23, 16)
-#define M10BMC_MAC_BYTE1		GENMASK(31, 24)
-#define M10BMC_MAC_HIGH			0x14
-#define M10BMC_MAC_BYTE6		GENMASK(7, 0)
-#define M10BMC_MAC_BYTE5		GENMASK(15, 8)
-#define M10BMC_MAC_COUNT		GENMASK(23, 16)
-#define M10BMC_TEST_REG			0x3c
-#define M10BMC_BUILD_VER		0x68
-#define M10BMC_VER_MAJOR_MSK		GENMASK(23, 16)
-#define M10BMC_VER_PCB_INFO_MSK		GENMASK(31, 24)
-#define M10BMC_VER_LEGACY_INVALID	0xffffffff
-
-/* Secure update doorbell register, in system register region */
-#define M10BMC_DOORBELL			0x400
-
-/* Authorization Result register, in system register region */
-#define M10BMC_AUTH_RESULT		0x404
-
 /* Doorbell register fields */
 #define DRBL_RSU_REQUEST		BIT(0)
 #define DRBL_RSU_PROGRESS		GENMASK(7, 4)
@@ -102,22 +72,6 @@ 
 #define RSU_COMPLETE_INTERVAL_MS	1000
 #define RSU_COMPLETE_TIMEOUT_MS		(40 * 60 * 1000)
 
-/* Addresses for security related data in FLASH */
-#define BMC_REH_ADDR	0x17ffc004
-#define BMC_PROG_ADDR	0x17ffc000
-#define BMC_PROG_MAGIC	0x5746
-
-#define SR_REH_ADDR	0x17ffd004
-#define SR_PROG_ADDR	0x17ffd000
-#define SR_PROG_MAGIC	0x5253
-
-#define PR_REH_ADDR	0x17ffe004
-#define PR_PROG_ADDR	0x17ffe000
-#define PR_PROG_MAGIC	0x5250
-
-/* Address of 4KB inverted bit vector containing staging area FLASH count */
-#define STAGING_FLASH_COUNT	0x17ffb000
-
 /**
  * struct m10bmc_csr_map - Intel MAX 10 BMC CSR register map
  */