* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).