Move SPI related defines to per interface from the global header. This
makes it harder to shoot oneself into foot.
Some bitfield defs are also moved to intel-m10-bmc-core which seems
more appropriate for them.
Reviewed-by: Russ Weight <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
drivers/mfd/intel-m10-bmc-core.c | 11 ++++++++
drivers/mfd/intel-m10-bmc-spi.c | 39 +++++++++++++++++++++++++
include/linux/mfd/intel-m10-bmc.h | 47 -------------------------------
3 files changed, 50 insertions(+), 47 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 e3b2edb8bc07..4a7a16d9f8d6 100644
--- a/drivers/mfd/intel-m10-bmc-spi.c
+++ b/drivers/mfd/intel-m10-bmc-spi.c
@@ -13,6 +13,45 @@
#include <linux/regmap.h>
#include <linux/spi/spi.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
+
+/* Register offset of system registers */
+#define NIOS2_FW_VERSION 0x0
+#define M10BMC_MAC_LOW 0x10
+#define M10BMC_MAC_HIGH 0x14
+#define M10BMC_TEST_REG 0x3c
+#define M10BMC_BUILD_VER 0x68
+#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
+
+/* 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
+
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),
diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
index 860408aa8db3..ed920f76d3c8 100644
--- a/include/linux/mfd/intel-m10-bmc.h
+++ b/include/linux/mfd/intel-m10-bmc.h
@@ -15,39 +15,8 @@ enum m10bmc_type {
M10_N5010,
};
-#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)
@@ -108,22 +77,6 @@ enum m10bmc_type {
#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
On 2022-11-08 at 16:43:00 +0200, Ilpo J?rvinen wrote:
> Move SPI related defines to per interface from the global header. This
These definitions are not for SPI, maybe more precisely like "SPI based
board definitions".
> makes it harder to shoot oneself into foot.
Maybe better to use plain text in commit message.
>
> Some bitfield defs are also moved to intel-m10-bmc-core which seems
> more appropriate for them.
I'm still not clear about the motivation of these movements. For example:
We move the MAC registers in core, then should we also move sec update
registers in sec update driver?
We move the DOORBELL reg addr out, but keep DOORBELL reg fields in
header file, seems make harder for people to get the whole picture.
Thanks,
Yilun
>
> Reviewed-by: Russ Weight <[email protected]>
> Signed-off-by: Ilpo J?rvinen <[email protected]>
> ---
> drivers/mfd/intel-m10-bmc-core.c | 11 ++++++++
> drivers/mfd/intel-m10-bmc-spi.c | 39 +++++++++++++++++++++++++
> include/linux/mfd/intel-m10-bmc.h | 47 -------------------------------
> 3 files changed, 50 insertions(+), 47 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 e3b2edb8bc07..4a7a16d9f8d6 100644
> --- a/drivers/mfd/intel-m10-bmc-spi.c
> +++ b/drivers/mfd/intel-m10-bmc-spi.c
> @@ -13,6 +13,45 @@
> #include <linux/regmap.h>
> #include <linux/spi/spi.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
> +
> +/* Register offset of system registers */
> +#define NIOS2_FW_VERSION 0x0
> +#define M10BMC_MAC_LOW 0x10
> +#define M10BMC_MAC_HIGH 0x14
> +#define M10BMC_TEST_REG 0x3c
> +#define M10BMC_BUILD_VER 0x68
> +#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
> +
> +/* 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
> +
> 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),
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index 860408aa8db3..ed920f76d3c8 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -15,39 +15,8 @@ enum m10bmc_type {
> M10_N5010,
> };
>
> -#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)
> @@ -108,22 +77,6 @@ enum m10bmc_type {
> #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
>
On Fri, 11 Nov 2022, Xu Yilun wrote:
> On 2022-11-08 at 16:43:00 +0200, Ilpo J?rvinen wrote:
> > Move SPI related defines to per interface from the global header. This
>
> These definitions are not for SPI, maybe more precisely like "SPI based
> board definitions".
>
> > makes it harder to shoot oneself into foot.
>
> Maybe better to use plain text in commit message.
Ok, I'll try to incorporate your suggestions.
> > Some bitfield defs are also moved to intel-m10-bmc-core which seems
> > more appropriate for them.
>
> I'm still not clear about the motivation of these movements. For example:
>
> We move the MAC registers in core, then should we also move sec update
> registers in sec update driver?
I'd actually prefer to do exactly as you suggest for sec update defines
too but it didn't feel like their move would fit well into this patch
series.
> We move the DOORBELL reg addr out, but keep DOORBELL reg fields in
> header file, seems make harder for people to get the whole picture.
Those reg fields are common between boards, no? Unlike the DOORBELL reg
addr whose value depends on which interface the board is based on?
The point of this move is that this header file would without this split
give a _wrong_ "whole picture" if it keeps the defines that are not
shared for all boards but vary per interface.
But if you still insist, I can of course drop this patch. It's in no way
absolutely necessary even if I personnally think it's useful to downscope
defines (but I can understand there might be different opinions such as
the one you stated above).
--
i.
> > Reviewed-by: Russ Weight <[email protected]>
> > Signed-off-by: Ilpo J?rvinen <[email protected]>
> > ---
> > drivers/mfd/intel-m10-bmc-core.c | 11 ++++++++
> > drivers/mfd/intel-m10-bmc-spi.c | 39 +++++++++++++++++++++++++
> > include/linux/mfd/intel-m10-bmc.h | 47 -------------------------------
> > 3 files changed, 50 insertions(+), 47 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 e3b2edb8bc07..4a7a16d9f8d6 100644
> > --- a/drivers/mfd/intel-m10-bmc-spi.c
> > +++ b/drivers/mfd/intel-m10-bmc-spi.c
> > @@ -13,6 +13,45 @@
> > #include <linux/regmap.h>
> > #include <linux/spi/spi.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
> > +
> > +/* Register offset of system registers */
> > +#define NIOS2_FW_VERSION 0x0
> > +#define M10BMC_MAC_LOW 0x10
> > +#define M10BMC_MAC_HIGH 0x14
> > +#define M10BMC_TEST_REG 0x3c
> > +#define M10BMC_BUILD_VER 0x68
> > +#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
> > +
> > +/* 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
> > +
> > 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),
> > diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> > index 860408aa8db3..ed920f76d3c8 100644
> > --- a/include/linux/mfd/intel-m10-bmc.h
> > +++ b/include/linux/mfd/intel-m10-bmc.h
> > @@ -15,39 +15,8 @@ enum m10bmc_type {
> > M10_N5010,
> > };
> >
> > -#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)
> > @@ -108,22 +77,6 @@ enum m10bmc_type {
> > #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
> >
>
On 2022-11-11 at 13:20:18 +0200, Ilpo J?rvinen wrote:
> On Fri, 11 Nov 2022, Xu Yilun wrote:
>
> > On 2022-11-08 at 16:43:00 +0200, Ilpo J?rvinen wrote:
> > > Move SPI related defines to per interface from the global header. This
> >
> > These definitions are not for SPI, maybe more precisely like "SPI based
> > board definitions".
> >
> > > makes it harder to shoot oneself into foot.
> >
> > Maybe better to use plain text in commit message.
>
> Ok, I'll try to incorporate your suggestions.
>
> > > Some bitfield defs are also moved to intel-m10-bmc-core which seems
> > > more appropriate for them.
> >
> > I'm still not clear about the motivation of these movements. For example:
> >
> > We move the MAC registers in core, then should we also move sec update
> > registers in sec update driver?
>
> I'd actually prefer to do exactly as you suggest for sec update defines
> too but it didn't feel like their move would fit well into this patch
> series.
It's good to me.
>
> > We move the DOORBELL reg addr out, but keep DOORBELL reg fields in
> > header file, seems make harder for people to get the whole picture.
>
> Those reg fields are common between boards, no? Unlike the DOORBELL reg
> addr whose value depends on which interface the board is based on?
>
> The point of this move is that this header file would without this split
> give a _wrong_ "whole picture" if it keeps the defines that are not
> shared for all boards but vary per interface.
Fine, it's good to me.
Thanks,
Yilun
>
> But if you still insist, I can of course drop this patch. It's in no way
> absolutely necessary even if I personnally think it's useful to downscope
> defines (but I can understand there might be different opinions such as
> the one you stated above).
>
> --
> i.
>
>
> > > Reviewed-by: Russ Weight <[email protected]>
> > > Signed-off-by: Ilpo J?rvinen <[email protected]>
> > > ---
> > > drivers/mfd/intel-m10-bmc-core.c | 11 ++++++++
> > > drivers/mfd/intel-m10-bmc-spi.c | 39 +++++++++++++++++++++++++
> > > include/linux/mfd/intel-m10-bmc.h | 47 -------------------------------
> > > 3 files changed, 50 insertions(+), 47 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 e3b2edb8bc07..4a7a16d9f8d6 100644
> > > --- a/drivers/mfd/intel-m10-bmc-spi.c
> > > +++ b/drivers/mfd/intel-m10-bmc-spi.c
> > > @@ -13,6 +13,45 @@
> > > #include <linux/regmap.h>
> > > #include <linux/spi/spi.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
> > > +
> > > +/* Register offset of system registers */
> > > +#define NIOS2_FW_VERSION 0x0
> > > +#define M10BMC_MAC_LOW 0x10
> > > +#define M10BMC_MAC_HIGH 0x14
> > > +#define M10BMC_TEST_REG 0x3c
> > > +#define M10BMC_BUILD_VER 0x68
> > > +#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
> > > +
> > > +/* 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
> > > +
> > > 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),
> > > diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> > > index 860408aa8db3..ed920f76d3c8 100644
> > > --- a/include/linux/mfd/intel-m10-bmc.h
> > > +++ b/include/linux/mfd/intel-m10-bmc.h
> > > @@ -15,39 +15,8 @@ enum m10bmc_type {
> > > M10_N5010,
> > > };
> > >
> > > -#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)
> > > @@ -108,22 +77,6 @@ enum m10bmc_type {
> > > #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
> > >
> >