by Ilpo Järvinen

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Tianfei zhang <[email protected]>
> > Co-developed-by: Russ Weight <[email protected]>
> > Signed-off-by: Russ Weight <[email protected]>
> > Co-developed-by: Matthew Gerlach <[email protected]>
> > Signed-off-by: Matthew Gerlach <[email protected]>
> > Signed-off-by: Ilpo J?rvinen <[email protected]>
> > ---
> > .../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.

2022-12-05 12:32:40

On Mon, Dec 05, 2022 at 11:08:24PM +0800, Xu Yilun wrote:

> It is good for now to implement the indirect access interface in
> regmap_config, as intel-m10-bmc is the only one who uses it. But I'm not
> sure when a second IP block(like HSSI) in intel FPGA uses it, how to
> implement? A shared library?

The short answer is that I'm not really clear what this looks like so
it's hard to say.

Usually things for anything generic end up in drivers/base/regmap but it
should be generic in some way and thus far the code that's been posted
has been very much looking specific to a single device even where it's
been named as something generic. I've not been able to extract a clear
picture of what the hardware that's being described is and the code has
looked like it's more some vaugely related designs than anything really
shared, it's really felt like people just want to just merge whatever
they have in which case just putting it in the driver is the most
expedient thing.

Clearly the concept of a register map accessed via indirection is a
generic thing but to implement that at the very least the underlying
register map should be another regmap rather than hard coded to MMIO.

> Some background about hardware:
> Several IP blocks in intel FPGA integrate the same mmio register layout
> (so called indirect access interface here) as the bridge to the IP's real
> registers address space. Like:

> +---------+ +---------+
> | m10 BMC | | HSSI |
> +---------+ +---------+
> |indirect | |indirect |
> | access | | access |
> | MMIOs | | MMIOs |
> +----+----+ +----+----+
> | |
> | |
> +----+-----+ +---------+
> |m10 bmc | | HSSI |
> |registers | |registers|
> +----------+ +---------+

One of the things I've been unable to tell thus far is if this is a
single device with a consistent IP for register access (thus far I've
only seen clear evidence of one device) or if there's multiple devices
that have been designed this way for some unclear reason. AIUI these
are IPs within a single FPGA which is the top level MFD here? If this
is one FPGA then perhaps the top level driver for the FPGA ought to just
handle whatever weirdness the FPGA has? The fact that there doesn't
seem to be a name for this stuff makes it seem device specific.

The code that I've seen posted has looked like the register layout isn't
shared, all the register offsets have been variable, but there's this
thing with there being what looks like a command queue/IO completion
thing which seemed to be the only kind of substantial thing being
shared.

Attachments:

(No filename) (2.70 kB)
signature.asc (499.00 B)
Download all attachments

2022-12-06 03:00:03

by Xu Yilun

[permalink] [raw]

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

On 2022-12-05 at 18:22:14 +0000, Mark Brown wrote:
> On Mon, Dec 05, 2022 at 11:08:24PM +0800, Xu Yilun wrote:
>
> > It is good for now to implement the indirect access interface in
> > regmap_config, as intel-m10-bmc is the only one who uses it. But I'm not
> > sure when a second IP block(like HSSI) in intel FPGA uses it, how to
> > implement? A shared library?
>
> The short answer is that I'm not really clear what this looks like so
> it's hard to say.
>
> Usually things for anything generic end up in drivers/base/regmap but it
> should be generic in some way and thus far the code that's been posted
> has been very much looking specific to a single device even where it's
> been named as something generic. I've not been able to extract a clear
> picture of what the hardware that's being described is and the code has

I'm trying to describe it later in this thread.

> looked like it's more some vaugely related designs than anything really
> shared, it's really felt like people just want to just merge whatever
> they have in which case just putting it in the driver is the most
> expedient thing.

I agree.

>
> Clearly the concept of a register map accessed via indirection is a
> generic thing but to implement that at the very least the underlying
> register map should be another regmap rather than hard coded to MMIO.
>
> > Some background about hardware:
> > Several IP blocks in intel FPGA integrate the same mmio register layout
> > (so called indirect access interface here) as the bridge to the IP's real
> > registers address space. Like:
>
> > +---------+ +---------+
> > | m10 BMC | | HSSI |
> > +---------+ +---------+
> > |indirect | |indirect |
> > | access | | access |
> > | MMIOs | | MMIOs |
> > +----+----+ +----+----+
> > | |
> > | |
> > +----+-----+ +---------+
> > |m10 bmc | | HSSI |
> > |registers | |registers|
> > +----------+ +---------+
>
> One of the things I've been unable to tell thus far is if this is a
> single device with a consistent IP for register access (thus far I've
> only seen clear evidence of one device) or if there's multiple devices
> that have been designed this way for some unclear reason. AIUI these

Multiple devices, or a series of similar Intel PCIe based FPGA card.

> are IPs within a single FPGA which is the top level MFD here? If this

Yes, the Intel FPGA PCI device acts similarly as top level MFD, but it has
its own enumeration mechanism for these IPs, called Device Feature
list(DFL).

+--------------------------------------------------------------------+
| Intel PCIe based FPGA device |
| |
| +---------+--next--->+---------+---next--->+--------+---> [...] |
| | node for| | node for| |node for| |
| | m10 BMC | | HSSI | | Another| |
| +---------+ +---------+ | IP | |
| |indirect | |indirect | +--------+ |
| | access | | access | |specific| |
| | MMIOs | | MMIOs | | MMIOs | |
| +----+----+ +----+----+ +--------+ |
| | | |
| | | |
| +----+-----+ +---------+ |
| |m10 bmc | | HSSI | |
| |registers | |registers| |
| +----------+ +---------+ |
+--------------------------------------------------------------------+

> is one FPGA then perhaps the top level driver for the FPGA ought to just
> handle whatever weirdness the FPGA has? The fact that there doesn't

I think this could be a choice. A helper in top level driver that deal
with the creation of the regmap for sub device drivers.

> seem to be a name for this stuff makes it seem device specific.
>
> The code that I've seen posted has looked like the register layout isn't
> shared, all the register offsets have been variable, but there's this
> thing with there being what looks like a command queue/IO completion
> thing which seemed to be the only kind of substantial thing being
> shared.

Yes, the indirect access register block are embedded in each IP's
MMIO space, its base or offset is specific to each IP. But within
each copy of the indirect access register block, the layout is the
same.

And thanks for your detailed explaination.
Yilun

2022-12-08 12:35:53

by Ilpo Järvinen

[permalink] [raw]

Subject: Re: [PATCH v3 6/9] mfd: intel-m10-bmc: Downscope SPI defines & prefix with M10BMC_SPI

On Mon, 5 Dec 2022, Xu Yilun wrote:

> On 2022-12-05 at 11:31:06 +0200, Ilpo J?rvinen wrote:
> > On Fri, 2 Dec 2022, Russ Weight wrote:
> > > On 12/2/22 08:28, Xu Yilun wrote:
> > > > On 2022-12-02 at 12:08:38 +0200, Ilpo J?rvinen wrote:
> > > >> Move SPI based board definitions to per interface file from the global
> > > >> header. This makes it harder to use them accidently in the
> > > >> generic/interface agnostic code. Prefix the defines with M10BMC_SPI
> > > > I'm not sure if the register layout is actually bound to the bus
> > > > interface. My experience is the register layout is always decided by
> > > > board type. Is it possible there will be a new SPI based board but
> > > > has different register layout in future?
> > > >
> > > > So is M10BMC_SPI_XXX a good nam
> > >
> > > There could be future devices, spi or pmci based, that require different
> > > addresses for some of these values, and at that time we would need to
> > > additional versions of some of these macros using different names.
> > > Right now, spi and pmci are the primary differentiating factors. I'm not
> > > sure how to improve on the naming. Do you have any suggestions?
> >
> > It's per board type yes, but there's a strong clustering currently on
> > spi/pmci differentiation. That implies a one define applies to multiple
> > board types so naming it, e.g., after a single board type seems not much
> > better than the current approach.
>
> I think it is better to name after one of the board type among all its
> supported types. At least it clearly indicates they are related to board
> type.
>
> Actually it is normal for many driver modules. A driver was initially
> implemented for one board type, and was named by the initial board.
> But later you have more board types compatible to the driver, you don't
> change the driver name, just use it.

Ok, I'll do it that way then.

--
i.