* [PATCH 1/2] mfd: intel-m10-bmc: fix the register access range @ 2021-01-20 2:34 Xu Yilun 2021-01-20 2:34 ` [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap Xu Yilun 2021-01-20 15:42 ` [PATCH 1/2] mfd: intel-m10-bmc: fix the register access range Tom Rix 0 siblings, 2 replies; 7+ messages in thread From: Xu Yilun @ 2021-01-20 2:34 UTC (permalink / raw) To: lee.jones, linux-kernel Cc: trix, yilun.xu, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu This patch fixes the max register address of MAX 10 BMC. The range 0x20000000 ~ 0x200000fc are for control registers of the QSPI flash controller, which are not accessible to host. Signed-off-by: Xu Yilun <yilun.xu@intel.com> --- include/linux/mfd/intel-m10-bmc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h index c8ef2f1..06da62c 100644 --- a/include/linux/mfd/intel-m10-bmc.h +++ b/include/linux/mfd/intel-m10-bmc.h @@ -11,7 +11,7 @@ #define M10BMC_LEGACY_SYS_BASE 0x300400 #define M10BMC_SYS_BASE 0x300800 -#define M10BMC_MEM_END 0x200000fc +#define M10BMC_MEM_END 0x1fffffff /* Register offset of system registers */ #define NIOS2_FW_VERSION 0x0 -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap 2021-01-20 2:34 [PATCH 1/2] mfd: intel-m10-bmc: fix the register access range Xu Yilun @ 2021-01-20 2:34 ` Xu Yilun 2021-01-20 15:32 ` Tom Rix 2021-01-20 15:42 ` [PATCH 1/2] mfd: intel-m10-bmc: fix the register access range Tom Rix 1 sibling, 1 reply; 7+ messages in thread From: Xu Yilun @ 2021-01-20 2:34 UTC (permalink / raw) To: lee.jones, linux-kernel Cc: trix, yilun.xu, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu From: Matthew Gerlach <matthew.gerlach@linux.intel.com> This patch adds access tables to the MAX 10 BMC regmap. This prevents the host from accessing the unwanted I/O space. It also filters out the invalid outputs when reading the regmap debugfs interface. Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> Signed-off-by: Xu Yilun <yilun.xu@intel.com> --- drivers/mfd/intel-m10-bmc.c | 14 ++++++++++++++ include/linux/mfd/intel-m10-bmc.h | 5 ++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c index b84579b..0ae3053 100644 --- a/drivers/mfd/intel-m10-bmc.c +++ b/drivers/mfd/intel-m10-bmc.c @@ -23,10 +23,24 @@ static struct mfd_cell m10bmc_pacn3000_subdevs[] = { { .name = "n3000bmc-secure" }, }; +static const struct regmap_range m10bmc_regmap_range[] = { + regmap_reg_range(M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER, + M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER), + regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END), + regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END), +}; + +static const struct regmap_access_table m10bmc_access_table = { + .yes_ranges = m10bmc_regmap_range, + .n_yes_ranges = ARRAY_SIZE(m10bmc_regmap_range), +}; + static struct regmap_config intel_m10bmc_regmap_config = { .reg_bits = 32, .val_bits = 32, .reg_stride = 4, + .wr_table = &m10bmc_access_table, + .rd_table = &m10bmc_access_table, .max_register = M10BMC_MEM_END, }; diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h index 06da62c..4ba88ed 100644 --- a/include/linux/mfd/intel-m10-bmc.h +++ b/include/linux/mfd/intel-m10-bmc.h @@ -11,7 +11,10 @@ #define M10BMC_LEGACY_SYS_BASE 0x300400 #define M10BMC_SYS_BASE 0x300800 -#define M10BMC_MEM_END 0x1fffffff +#define M10BMC_SYS_END 0x300fff +#define M10BMC_FLASH_BASE 0x10000000 +#define M10BMC_FLASH_END 0x1fffffff +#define M10BMC_MEM_END M10BMC_FLASH_END /* Register offset of system registers */ #define NIOS2_FW_VERSION 0x0 -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap 2021-01-20 2:34 ` [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap Xu Yilun @ 2021-01-20 15:32 ` Tom Rix 2021-01-21 8:05 ` Xu Yilun 0 siblings, 1 reply; 7+ messages in thread From: Tom Rix @ 2021-01-20 15:32 UTC (permalink / raw) To: Xu Yilun, lee.jones, linux-kernel Cc: matthew.gerlach, russell.h.weight, lgoncalv, hao.wu On 1/19/21 6:34 PM, Xu Yilun wrote: > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > This patch adds access tables to the MAX 10 BMC regmap. This prevents > the host from accessing the unwanted I/O space. It also filters out the > invalid outputs when reading the regmap debugfs interface. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > --- > drivers/mfd/intel-m10-bmc.c | 14 ++++++++++++++ > include/linux/mfd/intel-m10-bmc.h | 5 ++++- > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c > index b84579b..0ae3053 100644 > --- a/drivers/mfd/intel-m10-bmc.c > +++ b/drivers/mfd/intel-m10-bmc.c > @@ -23,10 +23,24 @@ static struct mfd_cell m10bmc_pacn3000_subdevs[] = { > { .name = "n3000bmc-secure" }, > }; > > +static const struct regmap_range m10bmc_regmap_range[] = { > + regmap_reg_range(M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER, > + M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER), If this is the only value in the legacy map to be accessed, could it have its own #define ? Something like #define M10BMC_LEGACY_BUILD_VER ? > + regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END), > + regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END), > +}; > + > +static const struct regmap_access_table m10bmc_access_table = { > + .yes_ranges = m10bmc_regmap_range, > + .n_yes_ranges = ARRAY_SIZE(m10bmc_regmap_range), > +}; > + > static struct regmap_config intel_m10bmc_regmap_config = { > .reg_bits = 32, > .val_bits = 32, > .reg_stride = 4, > + .wr_table = &m10bmc_access_table, > + .rd_table = &m10bmc_access_table, The legacy build ver should only be read, so shouldn't these tables be different ? Tom > .max_register = M10BMC_MEM_END, > }; > > diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h > index 06da62c..4ba88ed 100644 > --- a/include/linux/mfd/intel-m10-bmc.h > +++ b/include/linux/mfd/intel-m10-bmc.h > @@ -11,7 +11,10 @@ > > #define M10BMC_LEGACY_SYS_BASE 0x300400 > #define M10BMC_SYS_BASE 0x300800 > -#define M10BMC_MEM_END 0x1fffffff > +#define M10BMC_SYS_END 0x300fff > +#define M10BMC_FLASH_BASE 0x10000000 > +#define M10BMC_FLASH_END 0x1fffffff > +#define M10BMC_MEM_END M10BMC_FLASH_END > > /* Register offset of system registers */ > #define NIOS2_FW_VERSION 0x0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap 2021-01-20 15:32 ` Tom Rix @ 2021-01-21 8:05 ` Xu Yilun 2021-01-21 13:19 ` Tom Rix 0 siblings, 1 reply; 7+ messages in thread From: Xu Yilun @ 2021-01-21 8:05 UTC (permalink / raw) To: Tom Rix Cc: lee.jones, linux-kernel, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu, yilun.xu On Wed, Jan 20, 2021 at 07:32:53AM -0800, Tom Rix wrote: > > On 1/19/21 6:34 PM, Xu Yilun wrote: > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > > > This patch adds access tables to the MAX 10 BMC regmap. This prevents > > the host from accessing the unwanted I/O space. It also filters out the > > invalid outputs when reading the regmap debugfs interface. > > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > > --- > > drivers/mfd/intel-m10-bmc.c | 14 ++++++++++++++ > > include/linux/mfd/intel-m10-bmc.h | 5 ++++- > > 2 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c > > index b84579b..0ae3053 100644 > > --- a/drivers/mfd/intel-m10-bmc.c > > +++ b/drivers/mfd/intel-m10-bmc.c > > @@ -23,10 +23,24 @@ static struct mfd_cell m10bmc_pacn3000_subdevs[] = { > > { .name = "n3000bmc-secure" }, > > }; > > > > +static const struct regmap_range m10bmc_regmap_range[] = { > > + regmap_reg_range(M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER, > > + M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER), > > If this is the only value in the legacy map to be accessed, could it have its own #define ? > > Something like > > #define M10BMC_LEGACY_BUILD_VER ? Yes, it could be more clear. I'll change it. > > > + regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END), > > + regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END), > > +}; > > + > > +static const struct regmap_access_table m10bmc_access_table = { > > + .yes_ranges = m10bmc_regmap_range, > > + .n_yes_ranges = ARRAY_SIZE(m10bmc_regmap_range), > > +}; > > + > > static struct regmap_config intel_m10bmc_regmap_config = { > > .reg_bits = 32, > > .val_bits = 32, > > .reg_stride = 4, > > + .wr_table = &m10bmc_access_table, > > + .rd_table = &m10bmc_access_table, > > The legacy build ver should only be read, so shouldn't these tables be different ? I'm not sure if a register could be regarded as writable if hardware ensures writing it has no effect but makes no harm. Usually these registers are marked as RO in spec. I think it could be quite common case in hardware design. But it could be trivial if we pick every such register out of wr_table. I just want to define the valid reg range. So could I keep the current implementation? Thanks, Yilun ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap 2021-01-21 8:05 ` Xu Yilun @ 2021-01-21 13:19 ` Tom Rix 2021-01-22 3:23 ` Xu Yilun 0 siblings, 1 reply; 7+ messages in thread From: Tom Rix @ 2021-01-21 13:19 UTC (permalink / raw) To: Xu Yilun Cc: lee.jones, linux-kernel, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu On 1/21/21 12:05 AM, Xu Yilun wrote: > On Wed, Jan 20, 2021 at 07:32:53AM -0800, Tom Rix wrote: >> On 1/19/21 6:34 PM, Xu Yilun wrote: >>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> >>> This patch adds access tables to the MAX 10 BMC regmap. This prevents >>> the host from accessing the unwanted I/O space. It also filters out the >>> invalid outputs when reading the regmap debugfs interface. >>> >>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> Signed-off-by: Xu Yilun <yilun.xu@intel.com> >>> --- >>> drivers/mfd/intel-m10-bmc.c | 14 ++++++++++++++ >>> include/linux/mfd/intel-m10-bmc.h | 5 ++++- >>> 2 files changed, 18 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c >>> index b84579b..0ae3053 100644 >>> --- a/drivers/mfd/intel-m10-bmc.c >>> +++ b/drivers/mfd/intel-m10-bmc.c >>> @@ -23,10 +23,24 @@ static struct mfd_cell m10bmc_pacn3000_subdevs[] = { >>> { .name = "n3000bmc-secure" }, >>> }; >>> >>> +static const struct regmap_range m10bmc_regmap_range[] = { >>> + regmap_reg_range(M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER, >>> + M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER), >> If this is the only value in the legacy map to be accessed, could it have its own #define ? >> >> Something like >> >> #define M10BMC_LEGACY_BUILD_VER ? > Yes, it could be more clear. I'll change it. > >>> + regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END), >>> + regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END), >>> +}; >>> + >>> +static const struct regmap_access_table m10bmc_access_table = { >>> + .yes_ranges = m10bmc_regmap_range, >>> + .n_yes_ranges = ARRAY_SIZE(m10bmc_regmap_range), >>> +}; >>> + >>> static struct regmap_config intel_m10bmc_regmap_config = { >>> .reg_bits = 32, >>> .val_bits = 32, >>> .reg_stride = 4, >>> + .wr_table = &m10bmc_access_table, >>> + .rd_table = &m10bmc_access_table, >> The legacy build ver should only be read, so shouldn't these tables be different ? > I'm not sure if a register could be regarded as writable if hardware > ensures writing it has no effect but makes no harm. Usually these > registers are marked as RO in spec. > > I think it could be quite common case in hardware design. But it could > be trivial if we pick every such register out of wr_table. I just want > to define the valid reg range. > > So could I keep the current implementation? I mean that the write table would not have first element the read table has because it has the single ro entry. The other ranges i am sure have ro's and are not worth breaking apart. If something like .wr_table = &m10bmc_access_table[1] doesn't work or looks to hacky, i don't mind leaving it as-is. Tom > > Thanks, > Yilun > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap 2021-01-21 13:19 ` Tom Rix @ 2021-01-22 3:23 ` Xu Yilun 0 siblings, 0 replies; 7+ messages in thread From: Xu Yilun @ 2021-01-22 3:23 UTC (permalink / raw) To: Tom Rix Cc: lee.jones, linux-kernel, matthew.gerlach, russell.h.weight, lgoncalv, hao.wu, yilun.xu On Thu, Jan 21, 2021 at 05:19:56AM -0800, Tom Rix wrote: > > On 1/21/21 12:05 AM, Xu Yilun wrote: > > On Wed, Jan 20, 2021 at 07:32:53AM -0800, Tom Rix wrote: > >> On 1/19/21 6:34 PM, Xu Yilun wrote: > >>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > >>> > >>> This patch adds access tables to the MAX 10 BMC regmap. This prevents > >>> the host from accessing the unwanted I/O space. It also filters out the > >>> invalid outputs when reading the regmap debugfs interface. > >>> > >>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > >>> Signed-off-by: Xu Yilun <yilun.xu@intel.com> > >>> --- > >>> drivers/mfd/intel-m10-bmc.c | 14 ++++++++++++++ > >>> include/linux/mfd/intel-m10-bmc.h | 5 ++++- > >>> 2 files changed, 18 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/mfd/intel-m10-bmc.c b/drivers/mfd/intel-m10-bmc.c > >>> index b84579b..0ae3053 100644 > >>> --- a/drivers/mfd/intel-m10-bmc.c > >>> +++ b/drivers/mfd/intel-m10-bmc.c > >>> @@ -23,10 +23,24 @@ static struct mfd_cell m10bmc_pacn3000_subdevs[] = { > >>> { .name = "n3000bmc-secure" }, > >>> }; > >>> > >>> +static const struct regmap_range m10bmc_regmap_range[] = { > >>> + regmap_reg_range(M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER, > >>> + M10BMC_LEGACY_SYS_BASE + M10BMC_BUILD_VER), > >> If this is the only value in the legacy map to be accessed, could it have its own #define ? > >> > >> Something like > >> > >> #define M10BMC_LEGACY_BUILD_VER ? > > Yes, it could be more clear. I'll change it. > > > >>> + regmap_reg_range(M10BMC_SYS_BASE, M10BMC_SYS_END), > >>> + regmap_reg_range(M10BMC_FLASH_BASE, M10BMC_FLASH_END), > >>> +}; > >>> + > >>> +static const struct regmap_access_table m10bmc_access_table = { > >>> + .yes_ranges = m10bmc_regmap_range, > >>> + .n_yes_ranges = ARRAY_SIZE(m10bmc_regmap_range), > >>> +}; > >>> + > >>> static struct regmap_config intel_m10bmc_regmap_config = { > >>> .reg_bits = 32, > >>> .val_bits = 32, > >>> .reg_stride = 4, > >>> + .wr_table = &m10bmc_access_table, > >>> + .rd_table = &m10bmc_access_table, > >> The legacy build ver should only be read, so shouldn't these tables be different ? > > I'm not sure if a register could be regarded as writable if hardware > > ensures writing it has no effect but makes no harm. Usually these > > registers are marked as RO in spec. > > > > I think it could be quite common case in hardware design. But it could > > be trivial if we pick every such register out of wr_table. I just want > > to define the valid reg range. > > > > So could I keep the current implementation? > > I mean that the write table would not have first element the read table has because it has the single ro entry. > > The other ranges i am sure have ro's and are not worth breaking apart. > > If something like > > .wr_table = &m10bmc_access_table[1] doesn't work or looks to hacky, i don't mind leaving it as-is. It looks hacky to me. If the first one has to be marked RO, I think it could be like: static const struct regmap_range m10bmc_regmap_ro_range[] = { regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER), }; static const struct regmap_range m10bmc_regmap_range[] = { regmap_reg_range(M10BMC_LEGACY_BUILD_VER, M10BMC_LEGACY_BUILD_VER), regmap_reg_range(...), ... }; static const struct regmap_access_table m10bmc_write_table = { .yes_ranges = m10bmc_regmap_range, .n_yes_ranges = ARRAY_SIZE(m10bmc_regmap_range), .no_range = m10bmc_regmap_ro_range, .n_no_range = ARRAY_SIZE(m10bmc_regmap_ro_range), }; static const struct regmap_access_table m10bmc_read_table = { .yes_ranges = m10bmc_regmap_range, .n_yes_ranges = ARRAY_SIZE(m10bmc_regmap_range), }; But I think this is unnecessary. I feel it is indicating all the other registers are RW in spec, actually they are not. So I tend to keep it simple, just tell the valid range. Thanks, Yilun ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] mfd: intel-m10-bmc: fix the register access range 2021-01-20 2:34 [PATCH 1/2] mfd: intel-m10-bmc: fix the register access range Xu Yilun 2021-01-20 2:34 ` [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap Xu Yilun @ 2021-01-20 15:42 ` Tom Rix 1 sibling, 0 replies; 7+ messages in thread From: Tom Rix @ 2021-01-20 15:42 UTC (permalink / raw) To: Xu Yilun, lee.jones, linux-kernel Cc: matthew.gerlach, russell.h.weight, lgoncalv, hao.wu A side note.. I think it would be good if the intel-m10-bmc.* files were tracked in the MAINTAINERS files. I was surprised the Lee was the only reviewer. Maybe Yilun or Matt should also be added. On 1/19/21 6:34 PM, Xu Yilun wrote: > This patch fixes the max register address of MAX 10 BMC. The range > 0x20000000 ~ 0x200000fc are for control registers of the QSPI flash > controller, which are not accessible to host. > > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > --- > include/linux/mfd/intel-m10-bmc.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h > index c8ef2f1..06da62c 100644 > --- a/include/linux/mfd/intel-m10-bmc.h > +++ b/include/linux/mfd/intel-m10-bmc.h > @@ -11,7 +11,7 @@ > > #define M10BMC_LEGACY_SYS_BASE 0x300400 > #define M10BMC_SYS_BASE 0x300800 > -#define M10BMC_MEM_END 0x200000fc > +#define M10BMC_MEM_END 0x1fffffff Reviewed-by: Tom Rix <trix@redhat.com> > > /* Register offset of system registers */ > #define NIOS2_FW_VERSION 0x0 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-01-22 3:29 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-20 2:34 [PATCH 1/2] mfd: intel-m10-bmc: fix the register access range Xu Yilun 2021-01-20 2:34 ` [PATCH 2/2] mfd: intel-m10-bmc: add access table configuration to the regmap Xu Yilun 2021-01-20 15:32 ` Tom Rix 2021-01-21 8:05 ` Xu Yilun 2021-01-21 13:19 ` Tom Rix 2021-01-22 3:23 ` Xu Yilun 2021-01-20 15:42 ` [PATCH 1/2] mfd: intel-m10-bmc: fix the register access range Tom Rix
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.