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 <russell.h.weight@xxxxxxxxx>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > > ---
> > > 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
> > >
> >
