[v2,07/11] regmap: indirect: Add indirect regmap support

Message ID 20221117120515.37807-8-ilpo.jarvinen@linux.intel.com
State New
Headers
Series intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support |

Commit Message

Ilpo Järvinen Nov. 17, 2022, 12:05 p.m. UTC
  Add support for indirect register access via a regmap interface.

Indirect register access is a generic way to access registers indirectly.
One use case is accessing registers on Intel FPGA IPs with e.g. PMCI or
HSSI.

Co-developed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/base/regmap/Kconfig           |   3 +
 drivers/base/regmap/Makefile          |   1 +
 drivers/base/regmap/regmap-indirect.c | 128 ++++++++++++++++++++++++++
 include/linux/regmap.h                |  55 +++++++++++
 4 files changed, 187 insertions(+)
 create mode 100644 drivers/base/regmap/regmap-indirect.c
  

Comments

Mark Brown Nov. 17, 2022, 1:35 p.m. UTC | #1
On Thu, Nov 17, 2022 at 02:05:11PM +0200, Ilpo Järvinen wrote:
> Add support for indirect register access via a regmap interface.
> 
> Indirect register access is a generic way to access registers indirectly.
> One use case is accessing registers on Intel FPGA IPs with e.g. PMCI or
> HSSI.

I can't tell from this changelog what exactly you're trying to
implement here...

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Indirect Register Access.
> + *
> + * Copyright (C) 2020-2022 Intel Corporation, Inc.
> + */
> +#include <linux/debugfs.h>

Please make the entire comment a C++ one so things look more
intentional.

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/seq_file.h>

I can't see what seq_file.h is used for, which is probably good
TBH since the interfaces it offers don't look like things I'd
expect a regmap bus to use.

> +static int indirect_bus_reg_read(void *context, unsigned int reg,
> +				     unsigned int *val)
> +{
> +	struct indirect_ctx *ctx = context;
> +	unsigned int cmd, ack, tmpval;
> +	int ret;
> +
> +	cmd = readl(ctx->base + ctx->indirect_cfg->cmd_offset);
> +	if (cmd != ctx->indirect_cfg->idle_cmd)
> +		dev_warn(ctx->dev, "residual cmd 0x%x on read entry\n", cmd);
> +
> +	writel(reg, ctx->base + ctx->indirect_cfg->addr_offset);
> +	writel(ctx->indirect_cfg->read_cmd, ctx->base + ctx->indirect_cfg->cmd_offset);
> +
> +	ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack,
> +				 (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask,
> +				 ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us);

This all looks very specific to one particular implementation,
requiring a particular set of memory mapped registers and
operations - things like the initial read of the command for
example.  It's not clear to me how much reuse this is likely to
see outside of the one driver you're trying to add - if you want
to implement something device specific you can just provide
the custom operations in the device's regmap configuration rather
than having to provide a bus.  Why add a bus?
  
Ilpo Järvinen Nov. 17, 2022, 2:35 p.m. UTC | #2
On Thu, 17 Nov 2022, Mark Brown wrote:

> On Thu, Nov 17, 2022 at 02:05:11PM +0200, Ilpo Järvinen wrote:
> > Add support for indirect register access via a regmap interface.
> > 
> > Indirect register access is a generic way to access registers indirectly.
> > One use case is accessing registers on Intel FPGA IPs with e.g. PMCI or
> > HSSI.
> 
> I can't tell from this changelog what exactly you're trying to
> implement here...
>
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Indirect Register Access.
> > + *
> > + * Copyright (C) 2020-2022 Intel Corporation, Inc.
> > + */
> > +#include <linux/debugfs.h>
> 
> Please make the entire comment a C++ one so things look more
> intentional.

Eh, all/most SPDX-License-Identifier lines are done like this one?!?

> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/seq_file.h>
> 
> I can't see what seq_file.h is used for, which is probably good
> TBH since the interfaces it offers don't look like things I'd
> expect a regmap bus to use.

Yeah, it seems it was not even the only useless header. I'll clean 
them up.

> > +static int indirect_bus_reg_read(void *context, unsigned int reg,
> > +				     unsigned int *val)
> > +{
> > +	struct indirect_ctx *ctx = context;
> > +	unsigned int cmd, ack, tmpval;
> > +	int ret;
> > +
> > +	cmd = readl(ctx->base + ctx->indirect_cfg->cmd_offset);
> > +	if (cmd != ctx->indirect_cfg->idle_cmd)
> > +		dev_warn(ctx->dev, "residual cmd 0x%x on read entry\n", cmd);
> > +
> > +	writel(reg, ctx->base + ctx->indirect_cfg->addr_offset);
> > +	writel(ctx->indirect_cfg->read_cmd, ctx->base + ctx->indirect_cfg->cmd_offset);
> > +
> > +	ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack,
> > +				 (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask,
> > +				 ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us);
> 
> This all looks very specific to one particular implementation,
> requiring a particular set of memory mapped registers and
> operations - things like the initial read of the command for
> example. It's not clear to me how much reuse this is likely to
> see outside of the one driver you're trying to add - if you want
> to implement something device specific you can just provide
> the custom operations in the device's regmap configuration rather
> than having to provide a bus.  Why add a bus?

The point of not doing it in a particular driver is that the users will 
be spread around more than into a single driver. This is a generic 
mechanism for accessing registers of IPs on Intel FPGA. The point being 
that IPs can use this common mechanism rather than each coming up their 
own way.

Mark Brown objected earlier naming it something related to Intel FPGAs [1]
but I certainly know it still fixes the operations like you note even if 
the offsets and values are now "customizable" (they weren't in the 
earliest versions of this patch).

[1] https://lore.kernel.org/all/YqB9O8HhZV2tXo8g@sirena.org.uk/T/#m75d4abdfd00f05866d837246ddc357a8af53cf99
  
Mark Brown Nov. 17, 2022, 3:29 p.m. UTC | #3
On Thu, Nov 17, 2022 at 04:35:23PM +0200, Ilpo Järvinen wrote:
> On Thu, 17 Nov 2022, Mark Brown wrote:
> > On Thu, Nov 17, 2022 at 02:05:11PM +0200, Ilpo Järvinen wrote:

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Indirect Register Access.
> > > + *
> > > + * Copyright (C) 2020-2022 Intel Corporation, Inc.
> > > + */
> > > +#include <linux/debugfs.h>

> > Please make the entire comment a C++ one so things look more
> > intentional.

> Eh, all/most SPDX-License-Identifier lines are done like this one?!?

The SPDX header has to be a C++ comment, please make the rest of
this a C++ comment.

> > > +	ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack,
> > > +				 (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask,
> > > +				 ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us);

> > This all looks very specific to one particular implementation,
> > requiring a particular set of memory mapped registers and
> > operations - things like the initial read of the command for
> > example. It's not clear to me how much reuse this is likely to
> > see outside of the one driver you're trying to add - if you want
> > to implement something device specific you can just provide
> > the custom operations in the device's regmap configuration rather
> > than having to provide a bus.  Why add a bus?

> The point of not doing it in a particular driver is that the users will 
> be spread around more than into a single driver. This is a generic 
> mechanism for accessing registers of IPs on Intel FPGA. The point being 
> that IPs can use this common mechanism rather than each coming up their 
> own way.

You're saying that this is generic but it's really not looking
very generic at all, like I say there's a bunch of assumptions in
the code that look entirely specific to your implementation here.
Any abstraction and reusability seems extremely unclear, I'm not
seeing what this is doing that is diffrent to the driver using
this providing it's own register read and write operations.

> Mark Brown objected earlier naming it something related to Intel FPGAs [1]
> but I certainly know it still fixes the operations like you note even if 
> the offsets and values are now "customizable" (they weren't in the 
> earliest versions of this patch).

> [1] https://lore.kernel.org/all/YqB9O8HhZV2tXo8g@sirena.org.uk/T/#m75d4abdfd00f05866d837246ddc357a8af53cf99

No, what I'm objecting to there is pretty much the same thing I'm
saying here - this doesn't seem like it's a particularly generic
implementation and I'm really not clear that there'd be anything
meaningful left by the time the implementation assumptions are
removed.
  
Ilpo Järvinen Nov. 18, 2022, 12:49 p.m. UTC | #4
On Thu, 17 Nov 2022, Mark Brown wrote:

> On Thu, Nov 17, 2022 at 04:35:23PM +0200, Ilpo Järvinen wrote:
> > On Thu, 17 Nov 2022, Mark Brown wrote:
> > > On Thu, Nov 17, 2022 at 02:05:11PM +0200, Ilpo Järvinen wrote:

> > > > +	ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack,
> > > > +				 (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask,
> > > > +				 ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us);
> 
> > > This all looks very specific to one particular implementation,
> > > requiring a particular set of memory mapped registers and
> > > operations - things like the initial read of the command for
> > > example. It's not clear to me how much reuse this is likely to
> > > see outside of the one driver you're trying to add - if you want
> > > to implement something device specific you can just provide
> > > the custom operations in the device's regmap configuration rather
> > > than having to provide a bus.  Why add a bus?
> 
> > The point of not doing it in a particular driver is that the users will 
> > be spread around more than into a single driver. This is a generic 
> > mechanism for accessing registers of IPs on Intel FPGA. The point being 
> > that IPs can use this common mechanism rather than each coming up their 
> > own way.
> 
> You're saying that this is generic but it's really not looking
> very generic at all, like I say there's a bunch of assumptions in
> the code that look entirely specific to your implementation here.
> Any abstraction and reusability seems extremely unclear, I'm not
> seeing what this is doing that is diffrent to the driver using
> this providing it's own register read and write operations.
>
> > Mark Brown objected earlier naming it something related to Intel FPGAs [1]
> > but I certainly know it still fixes the operations like you note even if 
> > the offsets and values are now "customizable" (they weren't in the 
> > earliest versions of this patch).
> 
> > [1] https://lore.kernel.org/all/YqB9O8HhZV2tXo8g@sirena.org.uk/T/#m75d4abdfd00f05866d837246ddc357a8af53cf99
> 
> No, what I'm objecting to there is pretty much the same thing I'm
> saying here - this doesn't seem like it's a particularly generic
> implementation and I'm really not clear that there'd be anything
> meaningful left by the time the implementation assumptions are
> removed.

That's probably because it sounds to me you're trying to extend its 
genericness beyond the domain where it's generic. That is, you're looking 
for genericness outside of IPs (that have their own driver each) in Intel 
FPGA domain.

Whether that is "generic" enough to reside in drivers/base/regmap can
of course be debated but lets say I put it into drivers/mfd/ along with 
the code currently using it. By doing that, we'll postpone this discussion 
to the point when the first driver using it outside of drivers/mfd/ comes 
by. At that point, having the indirect code in drivers/mfd/ is shown to 
be a wrong choice.

It's of course nothing that couldn't be fixed by patches moving the code 
around to some more preferred location. And that location likely turns out 
to be drivers/base/regmap, no? Or do you have a better place for it in 
that case?

Please also keep in mind that we're talking about an FPGA device here, a 
device that is capable of implementing other devices that fall under 
various drivers/xx/. Obviously each would have a driver of their own so
there is no as strong only single device/driver mapping here as you might 
be thinking.
  
Mark Brown Nov. 18, 2022, 1:55 p.m. UTC | #5
On Fri, Nov 18, 2022 at 02:49:45PM +0200, Ilpo Järvinen wrote:
> On Thu, 17 Nov 2022, Mark Brown wrote:

> > No, what I'm objecting to there is pretty much the same thing I'm
> > saying here - this doesn't seem like it's a particularly generic
> > implementation and I'm really not clear that there'd be anything
> > meaningful left by the time the implementation assumptions are
> > removed.

> That's probably because it sounds to me you're trying to extend its 
> genericness beyond the domain where it's generic. That is, you're looking 
> for genericness outside of IPs (that have their own driver each) in Intel 
> FPGA domain.

This just says it's adding "indirect regmap support" - there's
nothing here saying that it's some Intel specific thing but it's
quite specific to some IPs.  Perhaps you have some name for this
interface?  You're only adding one user here which isn't helping
make the case that this is something generic.

> Please also keep in mind that we're talking about an FPGA device here, a 
> device that is capable of implementing other devices that fall under 
> various drivers/xx/. Obviously each would have a driver of their own so
> there is no as strong only single device/driver mapping here as you might 
> be thinking.

I can't tell what you're trying to say here.  Are you saying that
this is somehow baked into some FPGA design so that it's memory
mapped with only a few registers showing to the rest of the
system rather than just having a substantial memory mapped
window like is typically used for FPGAs, but someohow this
register window stuff is implemented in the soft IP so people are
just throwng vaugely similar interfaces into a random host mapped
register layout?

Whatever's going on here this clearly isn't a generic
implementation of an indirect register map.
  
Ilpo Järvinen Nov. 21, 2022, 1:37 p.m. UTC | #6
On Fri, 18 Nov 2022, Mark Brown wrote:

> On Fri, Nov 18, 2022 at 02:49:45PM +0200, Ilpo Järvinen wrote:
> > On Thu, 17 Nov 2022, Mark Brown wrote:
> 
> > > No, what I'm objecting to there is pretty much the same thing I'm
> > > saying here - this doesn't seem like it's a particularly generic
> > > implementation and I'm really not clear that there'd be anything
> > > meaningful left by the time the implementation assumptions are
> > > removed.
> 
> > That's probably because it sounds to me you're trying to extend its 
> > genericness beyond the domain where it's generic. That is, you're looking 
> > for genericness outside of IPs (that have their own driver each) in Intel 
> > FPGA domain.
> 
> This just says it's adding "indirect regmap support" - there's
> nothing here saying that it's some Intel specific thing but it's
> quite specific to some IPs. 

Yeah, but it's that way mainly because of your earlier comments. :-)

I tried to make it more "generic" to the extent possible because of your 
concern related to genericness and I therefore intentionally put the Intel 
specific numbers into the other change.

Previously you were against saying it clearly that it's Intel FPGA 
specific when Matthew proposed changing the name to not sound something 
too generic. If you're ok with that now, I'm happy to make such change.

> Perhaps you have some name for this
> interface?  You're only adding one user here which isn't helping
> make the case that this is something generic.
>
> > Please also keep in mind that we're talking about an FPGA device here, a 
> > device that is capable of implementing other devices that fall under 
> > various drivers/xx/. Obviously each would have a driver of their own so
> > there is no as strong only single device/driver mapping here as you might 
> > be thinking.
> 
> I can't tell what you're trying to say here.  Are you saying that
> this is somehow baked into some FPGA design so that it's memory
> mapped with only a few registers showing to the rest of the
> system rather than just having a substantial memory mapped
> window like is typically used for FPGAs, but someohow this
> register window stuff is implemented in the soft IP so people are
> just throwng vaugely similar interfaces into a random host mapped
> register layout?

What I tried to say the users are not expected to be nicely confined into 
drivers/mfd/ (and a single driver in there).

You didn't answer at all my question about where to place the code?
I'm repeating it with the context below since you cut it off:


That's probably because it sounds to me you're trying to extend its 
genericness beyond the domain where it's generic. That is, you're looking 
for genericness outside of IPs (that have their own driver each) in Intel 
FPGA domain.

Whether that is "generic" enough to reside in drivers/base/regmap can
of course be debated but lets say I put it into drivers/mfd/ along with 
the code currently using it. By doing that, we'll postpone this discussion 
to the point when the first driver using it outside of drivers/mfd/ comes 
by. At that point, having the indirect code in drivers/mfd/ is shown to 
be a wrong choice.

It's of course nothing that couldn't be fixed by patches moving the code 
around to some more preferred location. And that location likely turns out 
to be drivers/base/regmap, no? Or do you have a better place for it in 
that case?
  
Mark Brown Nov. 25, 2022, 6:53 p.m. UTC | #7
On Mon, Nov 21, 2022 at 03:37:40PM +0200, Ilpo Järvinen wrote:

> Previously you were against saying it clearly that it's Intel FPGA 
> specific when Matthew proposed changing the name to not sound something 
> too generic. If you're ok with that now, I'm happy to make such change.

Saying it's for some Intel FPGA just makes it sound like it should only
live within the driver for that FPGA.  The issue with there being only
one user:

> > Perhaps you have some name for this
> > interface?  You're only adding one user here which isn't helping
> > make the case that this is something generic.

is part of it, as is the fact that the naming is so very generic.  Even
"Intel FPGA" seems to be heading to the generic side, this is presumably
some specific thing rather than just something that everyone using a
FPGA from Intel is going to need.  The issue with this being overly
specific isn't just the name, it's the code as well.

> > I can't tell what you're trying to say here.  Are you saying that
> > this is somehow baked into some FPGA design so that it's memory
> > mapped with only a few registers showing to the rest of the
> > system rather than just having a substantial memory mapped
> > window like is typically used for FPGAs, but someohow this
> > register window stuff is implemented in the soft IP so people are
> > just throwng vaugely similar interfaces into a random host mapped
> > register layout?
> 
> What I tried to say the users are not expected to be nicely confined into 
> drivers/mfd/ (and a single driver in there).

So this interface is part of the physical IP surrounding the actual
programmable bit of a FPGA or something?  That doesn't seem entirely
right though given the fact that the registers are apparently one of the
things that gets moved around a lot.  I still have no idea what this
hardware actually looks like or what this code is trying to represent,
especially given the very few things that you are trying to
parameterise.  It's really not obvious there's even any point in trying
to share this code at the abstraction level you've gone for.

Do you have any examples of even a second user that isn't this one MFD
which you can share?

> You didn't answer at all my question about where to place the code?
> I'm repeating it with the context below since you cut it off:

I keep telling you to either make this so that it's actually generic or
just have register get/set operations in the regmap for the device using
it.  As things stand with the code you've sent there's a bunch of things
like the way it's doing direct MMIO (which means it only works on top of
memory mapped devices) and the absolute requirement for an idle command
and a wait for completion which clearly look like this is device specific.

> Whether that is "generic" enough to reside in drivers/base/regmap can
> of course be debated but lets say I put it into drivers/mfd/ along with 
> the code currently using it. By doing that, we'll postpone this discussion 
> to the point when the first driver using it outside of drivers/mfd/ comes 
> by. At that point, having the indirect code in drivers/mfd/ is shown to 
> be a wrong choice.

> It's of course nothing that couldn't be fixed by patches moving the code 
> around to some more preferred location. And that location likely turns out 
> to be drivers/base/regmap, no? Or do you have a better place for it in 
> that case?

If you think this should be shared via regmap then make it shareable.
That needs more work than just repainting the name.
  

Patch

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 159bac6c5046..94e5ca5434cf 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
+	tristate
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 11facb32a027..6221a4740806 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) += regmap-indirect.o
diff --git a/drivers/base/regmap/regmap-indirect.c b/drivers/base/regmap/regmap-indirect.c
new file mode 100644
index 000000000000..ac42a36eb907
--- /dev/null
+++ b/drivers/base/regmap/regmap-indirect.c
@@ -0,0 +1,128 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Indirect Register Access.
+ *
+ * Copyright (C) 2020-2022 Intel Corporation, Inc.
+ */
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+
+struct indirect_ctx {
+	void __iomem *base;
+	struct device *dev;
+	const struct regmap_indirect_cfg *indirect_cfg;
+};
+
+static int indirect_bus_idle_cmd(struct indirect_ctx *ctx)
+{
+	unsigned int cmd;
+	int ret;
+
+	writel(ctx->indirect_cfg->idle_cmd, ctx->base + ctx->indirect_cfg->cmd_offset);
+
+	ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->cmd_offset, cmd,
+				 cmd == ctx->indirect_cfg->idle_cmd,
+				 ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us);
+	if (ret)
+		dev_err(ctx->dev, "timed out waiting idle cmd (residual cmd=0x%x)\n", 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, ack, tmpval;
+	int ret;
+
+	cmd = readl(ctx->base + ctx->indirect_cfg->cmd_offset);
+	if (cmd != ctx->indirect_cfg->idle_cmd)
+		dev_warn(ctx->dev, "residual cmd 0x%x on read entry\n", cmd);
+
+	writel(reg, ctx->base + ctx->indirect_cfg->addr_offset);
+	writel(ctx->indirect_cfg->read_cmd, ctx->base + ctx->indirect_cfg->cmd_offset);
+
+	ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack,
+				 (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask,
+				 ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us);
+	if (ret)
+		dev_err(ctx->dev, "read timed out on reg 0x%x ack 0x%x\n", reg, ack);
+	else
+		tmpval = readl(ctx->base + ctx->indirect_cfg->read_offset);
+
+	if (indirect_bus_idle_cmd(ctx)) {
+		if (!ret)
+			ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	*val = tmpval;
+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, ack;
+	int ret;
+
+	cmd = readl(ctx->base + ctx->indirect_cfg->cmd_offset);
+	if (cmd != ctx->indirect_cfg->idle_cmd)
+		dev_warn(ctx->dev, "residual cmd 0x%x on write entry\n", cmd);
+
+	writel(val, ctx->base + ctx->indirect_cfg->write_offset);
+	writel(reg, ctx->base + ctx->indirect_cfg->addr_offset);
+	writel(ctx->indirect_cfg->write_cmd, ctx->base + ctx->indirect_cfg->cmd_offset);
+
+	ret = readl_poll_timeout(ctx->base + ctx->indirect_cfg->ack_offset, ack,
+				 (ack & ctx->indirect_cfg->ack_mask) == ctx->indirect_cfg->ack_mask,
+				 ctx->indirect_cfg->sleep_us, ctx->indirect_cfg->timeout_us);
+	if (ret)
+		dev_err(ctx->dev, "write timed out on reg 0x%x ack 0x%x\n", reg, ack);
+
+	if (indirect_bus_idle_cmd(ctx)) {
+		if (!ret)
+			ret = -ETIMEDOUT;
+	}
+
+	return ret;
+}
+
+static const struct regmap_bus indirect_bus = {
+	.reg_write = indirect_bus_reg_write,
+	.reg_read =  indirect_bus_reg_read,
+};
+
+struct regmap *__devm_regmap_init_indirect(struct device *dev,
+					   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;
+	ctx->indirect_cfg = cfg->indirect_cfg;
+
+	indirect_bus_idle_cmd(ctx);
+
+	return __devm_regmap_init(dev, &indirect_bus, ctx, cfg, lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_indirect);
+
+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 ca3434dca3a0..adaa7bca4f60 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -190,6 +190,41 @@  enum regmap_endian {
 	REGMAP_ENDIAN_NATIVE,
 };
 
+/**
+ * struct regmap_indirect_cfg - A configuration for indirect register access
+ *
+ * @cmd_offset: Command register offset
+ * @idle_cmd: Idle command
+ * @read_cmd: Read command
+ * @write_cmd: Write command
+ *
+ * @ack_offset: Command acknowledgment register offset
+ * @ack_mask: Command acknowledgment bit mask
+ *
+ * @addr_offset: Address register offset
+ * @read_offset: Read register offset
+ * @write_offset: Write register offset
+ *
+ * @sleep_us: Command wait sleep (usecs)
+ * @timeout_us: Command timeout (usecs)
+ */
+struct regmap_indirect_cfg {
+	unsigned int cmd_offset;
+	u32 idle_cmd;
+	u32 read_cmd;
+	u32 write_cmd;
+
+	unsigned int ack_offset;
+	u32 ack_mask;
+
+	unsigned int addr_offset;
+	unsigned int read_offset;
+	unsigned int write_offset;
+
+	unsigned long sleep_us;
+	unsigned long timeout_us;
+};
+
 /**
  * struct regmap_range - A register range, used for access related checks
  *                       (readable/writeable/volatile/precious checks)
@@ -431,6 +466,8 @@  struct regmap_config {
 	const struct regmap_range_cfg *ranges;
 	unsigned int num_ranges;
 
+	const struct regmap_indirect_cfg *indirect_cfg;
+
 	bool use_hwlock;
 	bool use_raw_spinlock;
 	unsigned int hwlock_id;
@@ -693,6 +730,12 @@  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);
+struct regmap *__devm_regmap_init_indirect(struct device *dev,
+					   void __iomem *base,
+					   struct regmap_config *cfg,
+					   struct lock_class_key *lock_key,
+					   const char *lock_name);
+
 /*
  * Wrapper for regmap_init macros to include a unique lockdep key and name
  * for each call. No-op if CONFIG_LOCKDEP is not set.
@@ -1148,6 +1191,18 @@  bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__devm_regmap_init_spi_avmm, #config,	\
 				 spi, config)
 
+/**
+ * devm_regmap_init_indirect - 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.
+ */
+#define devm_regmap_init_indirect(dev, base, config)			\
+	__regmap_lockdep_wrapper(__devm_regmap_init_indirect, #config,	\
+				 dev, base, config)
+
 int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk);
 void regmap_mmio_detach_clk(struct regmap *map);
 void regmap_exit(struct regmap *map);