[07/18] regmap: mmio: add config option to allow relaxed MMIO accesses
diff mbox series

Message ID 20201012205957.889185-8-adrian.ratiu@collabora.com
State New, archived
Headers show
Series
  • Add Hantro regmap and VC8000 h264 decode support
Related show

Commit Message

Adrian Ratiu Oct. 12, 2020, 8:59 p.m. UTC
On some platforms (eg armv7 due to the CONFIG_ARM_DMA_MEM_BUFFERABLE)
MMIO R/W operations always add memory barriers which can increase load,
decrease battery life or in general reduce performance unnecessarily
on devices which access a lot of configuration registers and where
ordering does not matter (eg. media accelerators like the Verisilicon /
Hantro video decoders).

Drivers used to call the relaxed MMIO variants directly but since they
are now accessing the MMIO registers via regmaps (to compensate for
for different VPU HW reg layouts via regmap fields), there is a need
for a relaxed API / config to preserve their existing behaviour.

Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
 drivers/base/regmap/regmap-mmio.c | 34 +++++++++++++++++++++++++++----
 include/linux/regmap.h            |  5 +++++
 2 files changed, 35 insertions(+), 4 deletions(-)

Comments

Mark Brown Oct. 13, 2020, 10:26 a.m. UTC | #1
On Mon, Oct 12, 2020 at 11:59:46PM +0300, Adrian Ratiu wrote:

> -	writeb(val, ctx->regs + reg);
> +	if (ctx->relaxed_mmio)
> +		writeb_relaxed(val, ctx->regs + reg);
> +	else
> +		writeb(val, ctx->regs + reg);

There is no point in doing a conditional operation on every I/O, it'd be
better to register a different set of ops when doing relaxed I/O.
Adrian Ratiu Oct. 14, 2020, 11:51 a.m. UTC | #2
Hello Mark,

On Tue, 13 Oct 2020, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Oct 12, 2020 at 11:59:46PM +0300, Adrian Ratiu wrote: 
> 
>> -	writeb(val, ctx->regs + reg); +	if (ctx->relaxed_mmio) + 
>> writeb_relaxed(val, ctx->regs + reg); +	else + 
>> writeb(val, ctx->regs + reg); 
> 
> There is no point in doing a conditional operation on every I/O, 
> it'd be better to register a different set of ops when doing 
> relaxed I/O. 

Indeed I have considered adding new functions but went with this 
solution because it's easier for the users to only have to define 
a "relaxed" config then test the regmap ctx as above.

Thinking a bit more about it, yes, it makes more sense to have 
dedicated ops: this way users don't have to be explicit about 
adding membarriers and can combine relaxed and non-relaxed more 
easily, so it's also a better API trade-off in addition to 
avoiding the conditional. Thanks!

Question: Do you want me to split this patch from the series and 
send it separately just for the regmap subsystem to be easier to 
review / apply?

Kind regards,
Adrian
Mark Brown Oct. 14, 2020, 12:12 p.m. UTC | #3
On Wed, Oct 14, 2020 at 02:51:14PM +0300, Adrian Ratiu wrote:
> On Tue, 13 Oct 2020, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Oct 12, 2020 at 11:59:46PM +0300, Adrian Ratiu wrote:

> > > -	writeb(val, ctx->regs + reg); +	if (ctx->relaxed_mmio) +
> > > writeb_relaxed(val, ctx->regs + reg); +	else + writeb(val, ctx->regs
> > > + reg);

> > There is no point in doing a conditional operation on every I/O, it'd be
> > better to register a different set of ops when doing relaxed I/O.

> Indeed I have considered adding new functions but went with this solution
> because it's easier for the users to only have to define a "relaxed" config
> then test the regmap ctx as above.

It seems like you've taken this in a direction other than what  I was
thinking of here - defining separate ops doesn't mean we have to do
anything which has any impact on the interface seen by users.  The
regmap config is supplied at registration time, it's just as available
then as it is when doing I/O.

> Thinking a bit more about it, yes, it makes more sense to have dedicated
> ops: this way users don't have to be explicit about adding membarriers and
> can combine relaxed and non-relaxed more easily, so it's also a better API
> trade-off in addition to avoiding the conditional. Thanks!

I'm not sure what you're proposing here - it does seem useful to be able
to combine relaxed and non-relaxed I/O but that seems like it'd break
down the abstraction for regmap since tht's not really a concept other
buses are going to have?  Unless we provide an operation to switch by
setting flags or somethin possibly and integrate it with the cache
perhaps.  Could you be a bit more specific about what you were thinking
of here please?

> Question: Do you want me to split this patch from the series and send it
> separately just for the regmap subsystem to be easier to review / apply?

Sure.
Adrian Ratiu Oct. 14, 2020, 1 p.m. UTC | #4
On Wed, 14 Oct 2020, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Oct 14, 2020 at 02:51:14PM +0300, Adrian Ratiu wrote: 
>> On Tue, 13 Oct 2020, Mark Brown <broonie@kernel.org> wrote: 
>> > On Mon, Oct 12, 2020 at 11:59:46PM +0300, Adrian Ratiu wrote: 
> 
>> > > -	writeb(val, ctx->regs + reg); +	if 
>> > > (ctx->relaxed_mmio) + writeb_relaxed(val, ctx->regs + reg); 
>> > > +	else + writeb(val, ctx->regs + reg); 
> 
>> > There is no point in doing a conditional operation on every 
>> > I/O, it'd be better to register a different set of ops when 
>> > doing relaxed I/O. 
> 
>> Indeed I have considered adding new functions but went with 
>> this solution because it's easier for the users to only have to 
>> define a "relaxed" config then test the regmap ctx as above. 
> 
> It seems like you've taken this in a direction other than what 
> I was thinking of here - defining separate ops doesn't mean we 
> have to do anything which has any impact on the interface seen 
> by users.  The regmap config is supplied at registration time, 
> it's just as available then as it is when doing I/O.

Right. I got confused by the meaning of ops :) Sorry about that.
 
> 
>> Thinking a bit more about it, yes, it makes more sense to have 
>> dedicated ops: this way users don't have to be explicit about 
>> adding membarriers and can combine relaxed and non-relaxed more 
>> easily, so it's also a better API trade-off in addition to 
>> avoiding the conditional. Thanks! 
> 
> I'm not sure what you're proposing here - it does seem useful to 
> be able to combine relaxed and non-relaxed I/O but that seems 
> like it'd break down the abstraction for regmap since tht's not 
> really a concept other buses are going to have?  Unless we 
> provide an operation to switch by setting flags or somethin 
> possibly and integrate it with the cache perhaps.  Could you be 
> a bit more specific about what you were thinking of here please?

I was thinking about exposing a relaxed API like 
regmap_write_relaxed but now that I know what you meant by ops and 
also that it doesn't make sense for other busses / violates the 
abstraction, I realize that is a bad idea and I will continue 
improving this to avoid the conditional and send a separete 
patch. Thanks again!

>
>> Question: Do you want me to split this patch from the series and send it
>> separately just for the regmap subsystem to be easier to review / apply?
>
> Sure.

Patch
diff mbox series

diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index af967d8f975e..21193ef2a923 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -16,6 +16,7 @@ 
 struct regmap_mmio_context {
 	void __iomem *regs;
 	unsigned val_bytes;
+	bool relaxed_mmio;
 
 	bool attached_clk;
 	struct clk *clk;
@@ -72,14 +73,20 @@  static void regmap_mmio_write8(struct regmap_mmio_context *ctx,
 				unsigned int reg,
 				unsigned int val)
 {
-	writeb(val, ctx->regs + reg);
+	if (ctx->relaxed_mmio)
+		writeb_relaxed(val, ctx->regs + reg);
+	else
+		writeb(val, ctx->regs + reg);
 }
 
 static void regmap_mmio_write16le(struct regmap_mmio_context *ctx,
 				  unsigned int reg,
 				  unsigned int val)
 {
-	writew(val, ctx->regs + reg);
+	if (ctx->relaxed_mmio)
+		writew_relaxed(val, ctx->regs + reg);
+	else
+		writew(val, ctx->regs + reg);
 }
 
 static void regmap_mmio_write16be(struct regmap_mmio_context *ctx,
@@ -93,7 +100,10 @@  static void regmap_mmio_write32le(struct regmap_mmio_context *ctx,
 				  unsigned int reg,
 				  unsigned int val)
 {
-	writel(val, ctx->regs + reg);
+	if (ctx->relaxed_mmio)
+		writel_relaxed(val, ctx->regs + reg);
+	else
+		writel(val, ctx->regs + reg);
 }
 
 static void regmap_mmio_write32be(struct regmap_mmio_context *ctx,
@@ -108,7 +118,10 @@  static void regmap_mmio_write64le(struct regmap_mmio_context *ctx,
 				  unsigned int reg,
 				  unsigned int val)
 {
-	writeq(val, ctx->regs + reg);
+	if (ctx->relaxed_mmio)
+		writeq_relaxed(val, ctx->regs + reg);
+	else
+		writeq(val, ctx->regs + reg);
 }
 #endif
 
@@ -134,12 +147,18 @@  static int regmap_mmio_write(void *context, unsigned int reg, unsigned int val)
 static unsigned int regmap_mmio_read8(struct regmap_mmio_context *ctx,
 				      unsigned int reg)
 {
+	if (ctx->relaxed_mmio)
+		return readb_relaxed(ctx->regs + reg);
+
 	return readb(ctx->regs + reg);
 }
 
 static unsigned int regmap_mmio_read16le(struct regmap_mmio_context *ctx,
 				         unsigned int reg)
 {
+	if (ctx->relaxed_mmio)
+		return readw_relaxed(ctx->regs + reg);
+
 	return readw(ctx->regs + reg);
 }
 
@@ -152,6 +171,9 @@  static unsigned int regmap_mmio_read16be(struct regmap_mmio_context *ctx,
 static unsigned int regmap_mmio_read32le(struct regmap_mmio_context *ctx,
 				         unsigned int reg)
 {
+	if (ctx->relaxed_mmio)
+		return readl_relaxed(ctx->regs + reg);
+
 	return readl(ctx->regs + reg);
 }
 
@@ -165,6 +187,9 @@  static unsigned int regmap_mmio_read32be(struct regmap_mmio_context *ctx,
 static unsigned int regmap_mmio_read64le(struct regmap_mmio_context *ctx,
 				         unsigned int reg)
 {
+	if (ctx->relaxed_mmio)
+		return readq_relaxed(ctx->regs + reg);
+
 	return readq(ctx->regs + reg);
 }
 #endif
@@ -237,6 +262,7 @@  static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 
 	ctx->regs = regs;
 	ctx->val_bytes = config->val_bits / 8;
+	ctx->relaxed_mmio = config->use_relaxed_mmio;
 	ctx->clk = ERR_PTR(-ENODEV);
 
 	switch (regmap_get_val_endian(dev, &regmap_mmio, config)) {
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index e7834d98207f..126fe700d1d8 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -315,6 +315,10 @@  typedef void (*regmap_unlock)(void *);
  *                   masks are used.
  * @zero_flag_mask: If set, read_flag_mask and write_flag_mask are used even
  *                   if they are both empty.
+ * @use_relaxed_mmio: If set, MMIO R/W operations will not use memory barriers.
+ *                    This can avoid load on devices which don't require strict
+ *                    orderings, but drivers should carefully add any explicit
+ *                    memory barriers when they may require them.
  * @use_single_read: If set, converts the bulk read operation into a series of
  *                   single read operations. This is useful for a device that
  *                   does not support  bulk read.
@@ -388,6 +392,7 @@  struct regmap_config {
 
 	bool use_single_read;
 	bool use_single_write;
+	bool use_relaxed_mmio;
 	bool can_multi_write;
 
 	enum regmap_endian reg_format_endian;