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.
