[PATCH v1] regmap: add generic indirect regmap support

[PATCH v1] regmap: add generic indirect regmap support

[Tianfei Zhang]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

This patch adds support for regmap APIs that are intended to be used by
the drivers of some devices which support generic indirect register access,
for example PMCI (Platform Management Control Interface) device, HSSI
(High Speed Serial Interface) device in FPGA.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
---
 drivers/base/regmap/Kconfig                   |   3 +
 drivers/base/regmap/Makefile                  |   1 +
 .../base/regmap/regmap-indirect-register.c    | 133 ++++++++++++++++++
 include/linux/regmap.h                        |  12 ++
 4 files changed, 149 insertions(+)
 create mode 100644 drivers/base/regmap/regmap-indirect-register.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 159bac6c5046..4ea590604f8d 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -65,3 +65,6 @@ config REGMAP_I3C
 config REGMAP_SPI_AVMM
 	tristate
 	depends on SPI
+
+config REGMAP_INDIRECT_REGISTER
+	tristate
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 11facb32a027..504a32b79d8c 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
 obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o
 obj-$(CONFIG_REGMAP_SPI_AVMM) += regmap-spi-avmm.o
 obj-$(CONFIG_REGMAP_MDIO) += regmap-mdio.o
+obj-$(CONFIG_REGMAP_INDIRECT_REGISTER) += regmap-indirect-register.o
diff --git a/drivers/base/regmap/regmap-indirect-register.c b/drivers/base/regmap/regmap-indirect-register.c
new file mode 100644
index 000000000000..0b748e48bd87
--- /dev/null
+++ b/drivers/base/regmap/regmap-indirect-register.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Indirect Register Access.
+//
+// Copyright (C) 2022 Intel Corporation, Inc.
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define INDIRECT_CMD_OFF	0x0
+#define INDIRECT_CMD_RD	BIT(0)
+#define INDIRECT_CMD_WR	BIT(1)
+#define INDIRECT_CMD_ACK	BIT(2)
+
+#define INDIRECT_ADDR_OFF	0x4
+#define INDIRECT_RD_OFF	0x8
+#define INDIRECT_WR_OFF	0xc
+
+#define INDIRECT_INT_US	1
+#define INDIRECT_TIMEOUT_US	10000
+
+struct indirect_ctx {
+	void __iomem *base;
+	struct device *dev;
+};
+
+static int indirect_bus_clr_cmd(struct indirect_ctx *ctx)
+{
+	unsigned int cmd;
+	int ret;
+
+	writel(0, ctx->base + INDIRECT_CMD_OFF);
+	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
+				 (!cmd), INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
+	if (ret)
+		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn", __func__, cmd);
+
+	return ret;
+}
+
+static int indirect_bus_reg_read(void *context, unsigned int reg,
+				 unsigned int *val)
+{
+	struct indirect_ctx *ctx = context;
+	unsigned int cmd;
+	int ret;
+
+	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
+	if (cmd)
+		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__, cmd);
+
+	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
+	writel(INDIRECT_CMD_RD, ctx->base + INDIRECT_CMD_OFF);
+	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
+				 (cmd & INDIRECT_CMD_ACK), INDIRECT_INT_US,
+				 INDIRECT_TIMEOUT_US);
+	if (ret) {
+		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n", __func__, reg, cmd);
+		goto out;
+	}
+
+	*val = readl(ctx->base + INDIRECT_RD_OFF);
+
+	if (indirect_bus_clr_cmd(ctx))
+		ret = -ETIMEDOUT;
+
+out:
+	return ret;
+}
+
+static int indirect_bus_reg_write(void *context, unsigned int reg,
+				  unsigned int val)
+{
+	struct indirect_ctx *ctx = context;
+	unsigned int cmd;
+	int ret;
+
+	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
+	if (cmd)
+		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__, cmd);
+
+	writel(val, ctx->base + INDIRECT_WR_OFF);
+	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
+	writel(INDIRECT_CMD_WR, ctx->base + INDIRECT_CMD_OFF);
+	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
+				 (cmd & INDIRECT_CMD_ACK), INDIRECT_INT_US,
+				 INDIRECT_TIMEOUT_US);
+	if (ret) {
+		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n", __func__, reg, cmd);
+		goto out;
+	}
+
+	if (indirect_bus_clr_cmd(ctx))
+		ret = -ETIMEDOUT;
+
+out:
+	return ret;
+}
+
+static const struct regmap_bus indirect_bus = {
+	.reg_write = indirect_bus_reg_write,
+	.reg_read =  indirect_bus_reg_read,
+};
+
+/**
+ * devm_regmap_init_indirect_register - create a regmap for indirect register access
+ * @dev: device creating the regmap
+ * @base: __iomem point to base of memory with mailbox
+ * @cfg: regmap_config describing interface
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+struct regmap *devm_regmap_init_indirect_register(struct device *dev,
+						  void __iomem *base,
+						  struct regmap_config *cfg)
+{
+	struct indirect_ctx *ctx;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return NULL;
+
+	ctx->base = base;
+	ctx->dev = dev;
+
+	return devm_regmap_init(dev, &indirect_bus, ctx, cfg);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_indirect_register);
+
+MODULE_DESCRIPTION("Indirect Register Access");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index de81a94d7b30..72eb38883e88 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -670,6 +670,18 @@ struct regmap *__devm_regmap_init_spi_avmm(struct spi_device *spi,
 					   const struct regmap_config *config,
 					   struct lock_class_key *lock_key,
 					   const char *lock_name);
+/**
+ * devm_regmap_init_indirect_register - create a regmap for indirect register access
+ * @dev: device creating the regmap
+ * @base: __iomem point to base of memory with mailbox
+ * @cfg: regmap_config describing interface
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+struct regmap *devm_regmap_init_indirect_register(struct device *dev,
+						  void __iomem *base,
+						  struct regmap_config *cfg);
+
 /*
  * Wrapper for regmap_init macros to include a unique lockdep key and name
  * for each call. No-op if CONFIG_LOCKDEP is not set.
-- 
2.26.2

Re: [PATCH v1] regmap: add generic indirect regmap support

[Greg KH]

On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> This patch adds support for regmap APIs that are intended to be used by
> the drivers of some devices which support generic indirect register access,
> for example PMCI (Platform Management Control Interface) device, HSSI
> (High Speed Serial Interface) device in FPGA.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
>  drivers/base/regmap/Kconfig                   |   3 +
>  drivers/base/regmap/Makefile                  |   1 +
>  .../base/regmap/regmap-indirect-register.c    | 133 ++++++++++++++++++
>  include/linux/regmap.h                        |  12 ++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/base/regmap/regmap-indirect-register.c

Don't we need users of these before we can take them?

thanks,

greg k-h

RE: [PATCH v1] regmap: add generic indirect regmap support

[Zhang, Tianfei]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Tuesday, June 7, 2022 12:50 PM
> To: Zhang, Tianfei <tianfei.zhang@intel.com>
> Cc: broonie@kernel.org; rafael@kernel.org; linux-kernel@vger.kernel.org; Wu,
> Hao <hao.wu@intel.com>; trix@redhat.com; Xu, Yilun <yilun.xu@intel.com>;
> Weight, Russell H <russell.h.weight@intel.com>; Matthew Gerlach
> <matthew.gerlach@linux.intel.com>
> Subject: Re: [PATCH v1] regmap: add generic indirect regmap support
> 
> On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
> > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> >
> > This patch adds support for regmap APIs that are intended to be used
> > by the drivers of some devices which support generic indirect register
> > access, for example PMCI (Platform Management Control Interface)
> > device, HSSI (High Speed Serial Interface) device in FPGA.
> >
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> > ---
> >  drivers/base/regmap/Kconfig                   |   3 +
> >  drivers/base/regmap/Makefile                  |   1 +
> >  .../base/regmap/regmap-indirect-register.c    | 133 ++++++++++++++++++
> >  include/linux/regmap.h                        |  12 ++
> >  4 files changed, 149 insertions(+)
> >  create mode 100644 drivers/base/regmap/regmap-indirect-register.c
> 
> Don't we need users of these before we can take them?

One of user is the PMCI (Platform Management Control Interface) driver of Intel FPGA PAC Card, 
PMCI device was connected to card BMC which provided telemetry and mailbox functionalities.

The PMCI driver is under review now:
https://lore.kernel.org/all/20220607032833.3482-1-tianfei.zhang@intel.com/

Re: [PATCH v1] regmap: add generic indirect regmap support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 984 bytes --]

On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> This patch adds support for regmap APIs that are intended to be used by
> the drivers of some devices which support generic indirect register access,
> for example PMCI (Platform Management Control Interface) device, HSSI
> (High Speed Serial Interface) device in FPGA.

What is "generic indirect register access"?  I'm not clear what this is
intended to support...

> +static int indirect_bus_clr_cmd(struct indirect_ctx *ctx)
> +{
> +	unsigned int cmd;
> +	int ret;
> +
> +	writel(0, ctx->base + INDIRECT_CMD_OFF);
> +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> +				 (!cmd), INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
> +	if (ret)
> +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn", __func__, cmd);

...and this doesn't look particularly generic, it looks like it's for
some particular controller/bridge?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] regmap: add generic indirect regmap support

[matthew.gerlach]

On Tue, 7 Jun 2022, Mark Brown wrote:

> On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> This patch adds support for regmap APIs that are intended to be used by
>> the drivers of some devices which support generic indirect register access,
>> for example PMCI (Platform Management Control Interface) device, HSSI
>> (High Speed Serial Interface) device in FPGA.
>
> What is "generic indirect register access"?  I'm not clear what this is
> intended to support...

"indirect register access" is a RTL design pattern we use in FPGAs 
frequently.  The design pattern involves a small number of registers plus 
a little handshake code to access various register spaces inside the FPGA 
fabric.  The design pattern is "generic" in the sense that the same small 
number of registers and handshake can be used with many different IP 
components in the FPGA.  Historically, the bit definitions and handshaking 
was slightly different for each IP component.  This is an attempt at a 
consistent usage across IP components.

Would a different name help?


>
>> +static int indirect_bus_clr_cmd(struct indirect_ctx *ctx)
>> +{
>> +	unsigned int cmd;
>> +	int ret;
>> +
>> +	writel(0, ctx->base + INDIRECT_CMD_OFF);
>> +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
>> +				 (!cmd), INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
>> +	if (ret)
>> +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn", __func__, cmd);
>
> ...and this doesn't look particularly generic, it looks like it's for
> some particular controller/bridge?
>

Re: [PATCH v1] regmap: add generic indirect regmap support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

On Tue, Jun 07, 2022 at 05:27:38PM -0700, matthew.gerlach@linux.intel.com wrote:
> On Tue, 7 Jun 2022, Mark Brown wrote:
> > On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

> > > This patch adds support for regmap APIs that are intended to be used by
> > > the drivers of some devices which support generic indirect register access,
> > > for example PMCI (Platform Management Control Interface) device, HSSI
> > > (High Speed Serial Interface) device in FPGA.

> > What is "generic indirect register access"?  I'm not clear what this is
> > intended to support...

> "indirect register access" is a RTL design pattern we use in FPGAs
> frequently.  The design pattern involves a small number of registers plus a
> little handshake code to access various register spaces inside the FPGA
> fabric.  The design pattern is "generic" in the sense that the same small
> number of registers and handshake can be used with many different IP
> components in the FPGA.  Historically, the bit definitions and handshaking
> was slightly different for each IP component.  This is an attempt at a
> consistent usage across IP components.

> Would a different name help?

This wouldn't address the major problem which is...

> > > +	writel(0, ctx->base + INDIRECT_CMD_OFF);
> > > +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> > > +				 (!cmd), INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
> > > +	if (ret)
> > > +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn", __func__, cmd);

> > ...and this doesn't look particularly generic, it looks like it's for
> > some particular controller/bridge?

...that this appears to be entirely specific to some particular device,
it's got things like hard coded register addresses and timeouts which
mean it can't be reused.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH v1] regmap: add generic indirect regmap support

[Zhang, Tianfei]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Wednesday, June 8, 2022 6:43 PM
> To: matthew.gerlach@linux.intel.com
> Cc: Zhang, Tianfei <tianfei.zhang@intel.com>; gregkh@linuxfoundation.org;
> rafael@kernel.org; linux-kernel@vger.kernel.org; Wu, Hao
> <hao.wu@intel.com>; trix@redhat.com; Xu, Yilun <yilun.xu@intel.com>;
> Weight, Russell H <russell.h.weight@intel.com>
> Subject: Re: [PATCH v1] regmap: add generic indirect regmap support
> 
> On Tue, Jun 07, 2022 at 05:27:38PM -0700, matthew.gerlach@linux.intel.com
> wrote:
> > On Tue, 7 Jun 2022, Mark Brown wrote:
> > > On Mon, Jun 06, 2022 at 09:37:55PM -0400, Tianfei Zhang wrote:
> > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> > > > This patch adds support for regmap APIs that are intended to be
> > > > used by the drivers of some devices which support generic indirect
> > > > register access, for example PMCI (Platform Management Control
> > > > Interface) device, HSSI (High Speed Serial Interface) device in FPGA.
> 
> > > What is "generic indirect register access"?  I'm not clear what this
> > > is intended to support...
> 
> > "indirect register access" is a RTL design pattern we use in FPGAs
> > frequently.  The design pattern involves a small number of registers
> > plus a little handshake code to access various register spaces inside
> > the FPGA fabric.  The design pattern is "generic" in the sense that
> > the same small number of registers and handshake can be used with many
> > different IP components in the FPGA.  Historically, the bit
> > definitions and handshaking was slightly different for each IP
> > component.  This is an attempt at a consistent usage across IP components.
> 
> > Would a different name help?
> 
> This wouldn't address the major problem which is...
> 
> > > > +	writel(0, ctx->base + INDIRECT_CMD_OFF);
> > > > +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> > > > +				 (!cmd), INDIRECT_INT_US,
> INDIRECT_TIMEOUT_US);
> > > > +	if (ret)
> > > > +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn",
> > > > +__func__, cmd);
> 
> > > ...and this doesn't look particularly generic, it looks like it's
> > > for some particular controller/bridge?
> 
> ...that this appears to be entirely specific to some particular device, it's got
> things like hard coded register addresses and timeouts which mean it can't be
> reused.

Yet, this is a register access hardware controller/bridge widely used in FPGA IP blocks, like PMCI, HSSI.
How about we change the patch title like this:

regmap: add indirect register controller support

Re: [PATCH v1] regmap: add generic indirect regmap support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

On Wed, Jun 08, 2022 at 11:54:26PM +0000, Zhang, Tianfei wrote:

> > > Would a different name help?
> > 
> > This wouldn't address the major problem which is...
> > 
> > > > > +	writel(0, ctx->base + INDIRECT_CMD_OFF);
> > > > > +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> > > > > +				 (!cmd), INDIRECT_INT_US,
> > INDIRECT_TIMEOUT_US);
> > > > > +	if (ret)
> > > > > +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn",
> > > > > +__func__, cmd);
> > 
> > > > ...and this doesn't look particularly generic, it looks like it's
> > > > for some particular controller/bridge?
> > 
> > ...that this appears to be entirely specific to some particular device, it's got
> > things like hard coded register addresses and timeouts which mean it can't be
> > reused.
> 
> Yet, this is a register access hardware controller/bridge widely used in FPGA IP blocks, like PMCI, HSSI.
> How about we change the patch title like this:

> regmap: add indirect register controller support

No, please enage with my feedback above.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] regmap: add generic indirect regmap support

[matthew.gerlach]

On Thu, 9 Jun 2022, Mark Brown wrote:

> On Wed, Jun 08, 2022 at 11:54:26PM +0000, Zhang, Tianfei wrote:
>
>>>> Would a different name help?
>>>
>>> This wouldn't address the major problem which is...
>>>
>>>>>> +	writel(0, ctx->base + INDIRECT_CMD_OFF);
>>>>>> +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
>>>>>> +				 (!cmd), INDIRECT_INT_US,
>>> INDIRECT_TIMEOUT_US);
>>>>>> +	if (ret)
>>>>>> +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn",
>>>>>> +__func__, cmd);
>>>
>>>>> ...and this doesn't look particularly generic, it looks like it's
>>>>> for some particular controller/bridge?
>>>
>>> ...that this appears to be entirely specific to some particular device, it's got
>>> things like hard coded register addresses and timeouts which mean it can't be
>>> reused.
>>
>> Yet, this is a register access hardware controller/bridge widely used in FPGA IP blocks, like PMCI, HSSI.
>> How about we change the patch title like this:
>
>> regmap: add indirect register controller support
>
> No, please enage with my feedback above.
>

Hi Mark,

I think part of the confusion is that this patch should have been included 
in a patch set that actually uses this regmap.  This patch really should 
be included with the following:

https://lore.kernel.org/all/20220607032833.3482-1-tianfei.zhang@intel.com

The hard coded register definitions are offsets to the passed in void 
__iomem base address.  This set of registers provides the semantics of 
indirect register read/write to whatever the register set is connected 
to on the back end.  Conceptually this could be considered a specific type 
indirect register access controller, but currently we have very different 
backend implementations in RTL.  Part of our intent is to have consistent 
register interfaces for our FPGA IP so multiple drivers can reuse this 
regmap.

I totally agree the hardcoded timeout values used for polling should be 
parameterized.

We would like to submit a v2 patch set that combines this patch with the 
first consumer of the regmap, PMCI.  We would also parameterize the 
timeout values, but most importantly the name must be better.  It is a 
long name, but how about something like "Intel FPGA Indirect Register 
Interface"?

Matthew Gerlach

Re: [PATCH v1] regmap: add generic indirect regmap support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On Thu, Jun 09, 2022 at 05:30:43PM -0700, matthew.gerlach@linux.intel.com wrote:
> On Thu, 9 Jun 2022, Mark Brown wrote:
> > On Wed, Jun 08, 2022 at 11:54:26PM +0000, Zhang, Tianfei wrote:

> > > > ...that this appears to be entirely specific to some particular device, it's got
> > > > things like hard coded register addresses and timeouts which mean it can't be
> > > > reused.

> I think part of the confusion is that this patch should have been included
> in a patch set that actually uses this regmap.  This patch really should be
> included with the following:

> https://lore.kernel.org/all/20220607032833.3482-1-tianfei.zhang@intel.com

> The hard coded register definitions are offsets to the passed in void
> __iomem base address.  This set of registers provides the semantics of
> indirect register read/write to whatever the register set is connected to on
> the back end.  Conceptually this could be considered a specific type
> indirect register access controller, but currently we have very different
> backend implementations in RTL.  Part of our intent is to have consistent
> register interfaces for our FPGA IP so multiple drivers can reuse this
> regmap.

...which is all specific to your particular implmentation of this and
not a generic thing that will work for anyone else who implements the
concept of indirected register access and happens to use a different
control mechanism.  If you have to use the same register interfaces to
reuse this code then the code isn't generic.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] regmap: add generic indirect regmap support

[Russ Weight]

On 6/6/22 18:37, Tianfei Zhang wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> This patch adds support for regmap APIs that are intended to be used by
> the drivers of some devices which support generic indirect register access,
> for example PMCI (Platform Management Control Interface) device, HSSI
> (High Speed Serial Interface) device in FPGA.
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Tianfei Zhang <tianfei.zhang@intel.com>
> ---
>  drivers/base/regmap/Kconfig                   |   3 +
>  drivers/base/regmap/Makefile                  |   1 +
>  .../base/regmap/regmap-indirect-register.c    | 133 ++++++++++++++++++
>  include/linux/regmap.h                        |  12 ++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/base/regmap/regmap-indirect-register.c
>
> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> index 159bac6c5046..4ea590604f8d 100644
> --- a/drivers/base/regmap/Kconfig
> +++ b/drivers/base/regmap/Kconfig
> @@ -65,3 +65,6 @@ config REGMAP_I3C
>  config REGMAP_SPI_AVMM
>  	tristate
>  	depends on SPI
> +
> +config REGMAP_INDIRECT_REGISTER
> +	tristate
> diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
> index 11facb32a027..504a32b79d8c 100644
> --- a/drivers/base/regmap/Makefile
> +++ b/drivers/base/regmap/Makefile
> @@ -20,3 +20,4 @@ obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
>  obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o
>  obj-$(CONFIG_REGMAP_SPI_AVMM) += regmap-spi-avmm.o
>  obj-$(CONFIG_REGMAP_MDIO) += regmap-mdio.o
> +obj-$(CONFIG_REGMAP_INDIRECT_REGISTER) += regmap-indirect-register.o
> diff --git a/drivers/base/regmap/regmap-indirect-register.c b/drivers/base/regmap/regmap-indirect-register.c
> new file mode 100644
> index 000000000000..0b748e48bd87
> --- /dev/null
> +++ b/drivers/base/regmap/regmap-indirect-register.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Indirect Register Access.
> +//
> +// Copyright (C) 2022 Intel Corporation, Inc.
> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define INDIRECT_CMD_OFF	0x0
> +#define INDIRECT_CMD_RD	BIT(0)
> +#define INDIRECT_CMD_WR	BIT(1)
> +#define INDIRECT_CMD_ACK	BIT(2)
> +
> +#define INDIRECT_ADDR_OFF	0x4
> +#define INDIRECT_RD_OFF	0x8
> +#define INDIRECT_WR_OFF	0xc
> +
> +#define INDIRECT_INT_US	1
> +#define INDIRECT_TIMEOUT_US	10000
> +
> +struct indirect_ctx {
> +	void __iomem *base;
> +	struct device *dev;
> +};
> +
> +static int indirect_bus_clr_cmd(struct indirect_ctx *ctx)
> +{
> +	unsigned int cmd;
> +	int ret;
> +
> +	writel(0, ctx->base + INDIRECT_CMD_OFF);
> +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> +				 (!cmd), INDIRECT_INT_US, INDIRECT_TIMEOUT_US);
> +	if (ret)
> +		dev_err(ctx->dev, "%s timed out on clearing cmd 0x%xn", __func__, cmd);
The line-return in the above dev_err() is missing a forward slash: \n

- Russ
> +
> +	return ret;
> +}
> +
> +static int indirect_bus_reg_read(void *context, unsigned int reg,
> +				 unsigned int *val)
> +{
> +	struct indirect_ctx *ctx = context;
> +	unsigned int cmd;
> +	int ret;
> +
> +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> +	if (cmd)
> +		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__, cmd);
> +
> +	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
> +	writel(INDIRECT_CMD_RD, ctx->base + INDIRECT_CMD_OFF);
> +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> +				 (cmd & INDIRECT_CMD_ACK), INDIRECT_INT_US,
> +				 INDIRECT_TIMEOUT_US);
> +	if (ret) {
> +		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n", __func__, reg, cmd);
> +		goto out;
> +	}
> +
> +	*val = readl(ctx->base + INDIRECT_RD_OFF);
> +
> +	if (indirect_bus_clr_cmd(ctx))
> +		ret = -ETIMEDOUT;
> +
> +out:
> +	return ret;
> +}
> +
> +static int indirect_bus_reg_write(void *context, unsigned int reg,
> +				  unsigned int val)
> +{
> +	struct indirect_ctx *ctx = context;
> +	unsigned int cmd;
> +	int ret;
> +
> +	cmd = readl(ctx->base + INDIRECT_CMD_OFF);
> +	if (cmd)
> +		dev_warn(ctx->dev, "%s non-zero cmd 0x%x\n", __func__, cmd);
> +
> +	writel(val, ctx->base + INDIRECT_WR_OFF);
> +	writel(reg, ctx->base + INDIRECT_ADDR_OFF);
> +	writel(INDIRECT_CMD_WR, ctx->base + INDIRECT_CMD_OFF);
> +	ret = readl_poll_timeout((ctx->base + INDIRECT_CMD_OFF), cmd,
> +				 (cmd & INDIRECT_CMD_ACK), INDIRECT_INT_US,
> +				 INDIRECT_TIMEOUT_US);
> +	if (ret) {
> +		dev_err(ctx->dev, "%s timed out on reg 0x%x cmd 0x%x\n", __func__, reg, cmd);
> +		goto out;
> +	}
> +
> +	if (indirect_bus_clr_cmd(ctx))
> +		ret = -ETIMEDOUT;
> +
> +out:
> +	return ret;
> +}
> +
> +static const struct regmap_bus indirect_bus = {
> +	.reg_write = indirect_bus_reg_write,
> +	.reg_read =  indirect_bus_reg_read,
> +};
> +
> +/**
> + * devm_regmap_init_indirect_register - create a regmap for indirect register access
> + * @dev: device creating the regmap
> + * @base: __iomem point to base of memory with mailbox
> + * @cfg: regmap_config describing interface
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +struct regmap *devm_regmap_init_indirect_register(struct device *dev,
> +						  void __iomem *base,
> +						  struct regmap_config *cfg)
> +{
> +	struct indirect_ctx *ctx;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return NULL;
> +
> +	ctx->base = base;
> +	ctx->dev = dev;
> +
> +	return devm_regmap_init(dev, &indirect_bus, ctx, cfg);
> +}
> +EXPORT_SYMBOL_GPL(devm_regmap_init_indirect_register);
> +
> +MODULE_DESCRIPTION("Indirect Register Access");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index de81a94d7b30..72eb38883e88 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -670,6 +670,18 @@ struct regmap *__devm_regmap_init_spi_avmm(struct spi_device *spi,
>  					   const struct regmap_config *config,
>  					   struct lock_class_key *lock_key,
>  					   const char *lock_name);
> +/**
> + * devm_regmap_init_indirect_register - create a regmap for indirect register access
> + * @dev: device creating the regmap
> + * @base: __iomem point to base of memory with mailbox
> + * @cfg: regmap_config describing interface
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +struct regmap *devm_regmap_init_indirect_register(struct device *dev,
> +						  void __iomem *base,
> +						  struct regmap_config *cfg);
> +
>  /*
>   * Wrapper for regmap_init macros to include a unique lockdep key and name
>   * for each call. No-op if CONFIG_LOCKDEP is not set.