Re: [PATCH v3 7/9] mfd: intel-m10-bmc: Add PMCI driver

[Ilpo Järvinen]

On Sat, 3 Dec 2022, Xu Yilun wrote:

> On 2022-12-02 at 12:08:39 +0200, Ilpo Järvinen wrote:
> > Add the mfd driver for the Platform Management Component Interface
> > (PMCI) based interface of Intel MAX10 BMC controller.
> > 
> > PMCI is a software-visible interface, connected to card BMC which
> > provided the basic functionality of read/write BMC register. This
> > driver leverages the regmap APIs to support Intel specific Indirect
> > Register Interface for register read/write on PMCI.
> > 
> > Previously, intel-m10-bmc provided sysfs under
> > /sys/bus/spi/devices/... which is generalized in this change because
> > not all MAX10 BMC appear under SPI anymore.
> > 
> > This patch also adds support for indirect register access via a regmap
> > interface. The access to the register goes via a hardware
> > controller/bridge that handles read/write/clear commands and
> > acknowledgments for the commands. On Intel FPGA IPs with e.g. PMCI or
> > HSSI, indirect register access is a generic way to access registers.
> > 
> > Co-developed-by: Tianfei zhang <tianfei.zhang@xxxxxxxxx>
> > Signed-off-by: Tianfei zhang <tianfei.zhang@xxxxxxxxx>
> > Co-developed-by: Russ Weight <russell.h.weight@xxxxxxxxx>
> > Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
> > Co-developed-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> >  .../ABI/testing/sysfs-driver-intel-m10-bmc    |   8 +-
> >  drivers/mfd/Kconfig                           |  12 ++
> >  drivers/mfd/Makefile                          |   2 +
> >  drivers/mfd/intel-m10-bmc-pmci-indirect.c     | 133 ++++++++++++++++
> >  drivers/mfd/intel-m10-bmc-pmci-main.c         | 147 ++++++++++++++++++
> >  include/linux/mfd/intel-m10-bmc.h             |  22 +++
> >  6 files changed, 320 insertions(+), 4 deletions(-)
> >  create mode 100644 drivers/mfd/intel-m10-bmc-pmci-indirect.c
> >  create mode 100644 drivers/mfd/intel-m10-bmc-pmci-main.c
> > 
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -2238,6 +2238,18 @@ config MFD_INTEL_M10_BMC_SPI
> >            additional drivers must be enabled in order to use the functionality
> >            of the device.
> >  
> > +config MFD_INTEL_M10_BMC_PMCI
> > +	tristate "Intel MAX 10 Board Management Controller with PMCI"
> > +	depends on FPGA_DFL
> > +	select MFD_INTEL_M10_BMC_CORE
> > +	select REGMAP
> > +	help
> > +	  Support for the Intel MAX 10 board management controller via PMCI.
> > +
> > +	  This driver provides common support for accessing the device,
> > +	  additional drivers must be enabled in order to use the functionality
> > +	  of the device.
> > +
> >  config MFD_RSMU_I2C
> >  	tristate "Renesas Synchronization Management Unit with I2C"
> >  	depends on I2C && OF
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 5d1f308ee2a7..603c0a8f1241 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -274,6 +274,8 @@ obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
> >  
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC_CORE)	+= intel-m10-bmc-core.o
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC_SPI)	+= intel-m10-bmc-spi.o
> > +intel-m10-bmc-pmci-objs			:= intel-m10-bmc-pmci-main.o intel-m10-bmc-pmci-indirect.o
> > +obj-$(CONFIG_MFD_INTEL_M10_BMC_PMCI)	+= intel-m10-bmc-pmci.o
> >  
> >  obj-$(CONFIG_MFD_ATC260X)	+= atc260x-core.o
> >  obj-$(CONFIG_MFD_ATC260X_I2C)	+= atc260x-i2c.o
> > diff --git a/drivers/mfd/intel-m10-bmc-pmci-indirect.c b/drivers/mfd/intel-m10-bmc-pmci-indirect.c
> > new file mode 100644
> > index 000000000000..cf347f93c05d
> > --- /dev/null
> > +++ b/drivers/mfd/intel-m10-bmc-pmci-indirect.c
> > +
> > +struct regmap *__devm_m10_regmap_indirect(struct device *dev,
> 
> We name the file intel-m10-bmc-pmci-xxx.c, and this function
> xx_m10_regmap_xx(). But I can see the implementation is just about the indirect
> bus which from your commit message could be used by various DFL features
> like HSSI or PMCI. So is it better we put the implementation in
> drivers/fpga and name the file dfl-indirect-regmap.c and the
> initialization function dfl_indirect_regmap_init()?

I guess that would be doable unless Mark objects. My understanding was 
that he preferred to have in the driver that is currently using it.

Mark, any opinion on this?

> > +					  void __iomem *base,
> > +					  struct regmap_config *cfg,
> > +					  struct lock_class_key *lock_key,
> > +					  const char *lock_name)
> > +{
> > +	struct indirect_ctx *ctx;
> > +
> > +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return NULL;
> > +
> > +	ctx->base = base;
> > +	ctx->dev = dev;
> > +
> > +	indirect_bus_clear_cmd(ctx);
> > +
> > +	return __devm_regmap_init(dev, &indirect_bus, ctx, cfg, lock_key, lock_name);
> 
> Sorry, I just can't remember why don't we just call devm_regmap_init() and
> get rid of all lock stuff?

At this point, we're already entered into __-domain though a
__regmap_lockdep_wrapper(). If I call devm_regmap_init() here, the 
second call into the wrapper would create another key which doesn't seem 
right.

> > +#define M10BMC_PMCI_STAGING_FLASH_COUNT	0x7ff5000
> 
> The same concern here, should all PMCI based M10 BMC have the same
> register layout? I assume no. I still think the layout should be decided
> by board type.
> 
> So some concern about these naming.

Noted, lets discuss the solution in the other patch.


-- 
 i.