[PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Andy Shevchenko]

The commit eb4a219d19fd ("regmap: Skip clk_put for attached clocks when
freeing context") oexcluded clk_put() call on regmap freeing. But the
same is needed for clk_unprepare() since the regmap MMIO users should
call regmap_mmio_detach_clk() which does unprepare the clock. Update
the code accordingly, so neither clk_put() nor clk_unprepare() would
be called for the attached clock.

Fixes: eb4a219d19fd ("regmap: Skip clk_put for attached clocks when freeing context")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/regmap/regmap-mmio.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index 71f16be7e717..e83a2c3ba95a 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -245,10 +245,9 @@ static void regmap_mmio_free_context(void *context)
 {
 	struct regmap_mmio_context *ctx = context;
 
-	if (!IS_ERR(ctx->clk)) {
+	if (!IS_ERR(ctx->clk) && !ctx->attached_clk) {
 		clk_unprepare(ctx->clk);
-		if (!ctx->attached_clk)
-			clk_put(ctx->clk);
+		clk_put(ctx->clk);
 	}
 	kfree(context);
 }
-- 
2.35.1

[PATCH v1 2/5] regmap: mmio: Drop unneeded and duplicative checks around CLK calls

[Andy Shevchenko]

The commit 6b8e090ecc3d ("regmap: use IS_ERR() to check clk_get()
results") assumes that CLK calls return the error pointer when clock
is not found. However in the current code the described situation
is simply impossible, because the regmap won't be created with
missed clock if requested. The only way when it can be the case is
what the above mentioned commit introduced by itself, when clock is
not provided.

Taking above into consideration, effectively revert the commit
6b8e090ecc3d and while at it, drop unneeded NULL checks since CLK
calls are NULL-aware.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/regmap/regmap-mmio.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index e83a2c3ba95a..e1923506a89a 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -146,16 +146,13 @@ static int regmap_mmio_write(void *context, unsigned int reg, unsigned int val)
 	struct regmap_mmio_context *ctx = context;
 	int ret;
 
-	if (!IS_ERR(ctx->clk)) {
-		ret = clk_enable(ctx->clk);
-		if (ret < 0)
-			return ret;
-	}
+	ret = clk_enable(ctx->clk);
+	if (ret < 0)
+		return ret;
 
 	ctx->reg_write(ctx, reg, val);
 
-	if (!IS_ERR(ctx->clk))
-		clk_disable(ctx->clk);
+	clk_disable(ctx->clk);
 
 	return 0;
 }
@@ -227,16 +224,13 @@ static int regmap_mmio_read(void *context, unsigned int reg, unsigned int *val)
 	struct regmap_mmio_context *ctx = context;
 	int ret;
 
-	if (!IS_ERR(ctx->clk)) {
-		ret = clk_enable(ctx->clk);
-		if (ret < 0)
-			return ret;
-	}
+	ret = clk_enable(ctx->clk);
+	if (ret < 0)
+		return ret;
 
 	*val = ctx->reg_read(ctx, reg);
 
-	if (!IS_ERR(ctx->clk))
-		clk_disable(ctx->clk);
+	clk_disable(ctx->clk);
 
 	return 0;
 }
@@ -245,7 +239,7 @@ static void regmap_mmio_free_context(void *context)
 {
 	struct regmap_mmio_context *ctx = context;
 
-	if (!IS_ERR(ctx->clk) && !ctx->attached_clk) {
+	if (!ctx->attached_clk) {
 		clk_unprepare(ctx->clk);
 		clk_put(ctx->clk);
 	}
@@ -290,7 +284,6 @@ 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)) {
 	case REGMAP_ENDIAN_DEFAULT:
-- 
2.35.1

Re: [PATCH v1 2/5] regmap: mmio: Drop unneeded and duplicative checks around CLK calls

[Mark Brown]

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

On Fri, Aug 05, 2022 at 11:53:18PM +0300, Andy Shevchenko wrote:

> The commit 6b8e090ecc3d ("regmap: use IS_ERR() to check clk_get()
> results") assumes that CLK calls return the error pointer when clock
> is not found. However in the current code the described situation
> is simply impossible, because the regmap won't be created with
> missed clock if requested. The only way when it can be the case is
> what the above mentioned commit introduced by itself, when clock is
> not provided.

> Taking above into consideration, effectively revert the commit
> 6b8e090ecc3d and while at it, drop unneeded NULL checks since CLK
> calls are NULL-aware.

I don't understand the supposed benefit of this.  Yes, the clk API does
currently accept NULL as a valid clock and returns it as a dummy but
explicitly taking advantage of that in the way that this does just feels
more sloppy than the current behaviour.

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

Re: [PATCH v1 2/5] regmap: mmio: Drop unneeded and duplicative checks around CLK calls

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 3:28 PM Mark Brown <broonie@kernel.org> wrote:
> On Fri, Aug 05, 2022 at 11:53:18PM +0300, Andy Shevchenko wrote:
>
> > The commit 6b8e090ecc3d ("regmap: use IS_ERR() to check clk_get()
> > results") assumes that CLK calls return the error pointer when clock
> > is not found. However in the current code the described situation
> > is simply impossible, because the regmap won't be created with
> > missed clock if requested. The only way when it can be the case is
> > what the above mentioned commit introduced by itself, when clock is
> > not provided.
>
> > Taking above into consideration, effectively revert the commit
> > 6b8e090ecc3d and while at it, drop unneeded NULL checks since CLK
> > calls are NULL-aware.
>
> I don't understand the supposed benefit of this.  Yes, the clk API does
> currently accept NULL as a valid clock and returns it as a dummy but
> explicitly taking advantage of that in the way that this does just feels
> more sloppy than the current behaviour.

How? The clock is still checked by NULL by the clock framework. The
above mentioned patch is simply wrong (taking into account modern
behaviour of CCF APIs) and reverting it clears things.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/5] regmap: mmio: Remove mmio_relaxed member from context

[Andy Shevchenko]

There is no need to keep mmio_relaxed member in the context, it's
onetime used during generation of the context. Remove it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/regmap/regmap-mmio.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index e1923506a89a..c2b18973144b 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -16,7 +16,6 @@
 struct regmap_mmio_context {
 	void __iomem *regs;
 	unsigned int val_bytes;
-	bool relaxed_mmio;
 
 	bool attached_clk;
 	struct clk *clk;
@@ -283,7 +282,6 @@ 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;
 
 	switch (regmap_get_val_endian(dev, &regmap_mmio, config)) {
 	case REGMAP_ENDIAN_DEFAULT:
@@ -293,7 +291,7 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 #endif
 		switch (config->val_bits) {
 		case 8:
-			if (ctx->relaxed_mmio) {
+			if (config->use_relaxed_mmio) {
 				ctx->reg_read = regmap_mmio_read8_relaxed;
 				ctx->reg_write = regmap_mmio_write8_relaxed;
 			} else {
@@ -302,7 +300,7 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 			}
 			break;
 		case 16:
-			if (ctx->relaxed_mmio) {
+			if (config->use_relaxed_mmio) {
 				ctx->reg_read = regmap_mmio_read16le_relaxed;
 				ctx->reg_write = regmap_mmio_write16le_relaxed;
 			} else {
@@ -311,7 +309,7 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 			}
 			break;
 		case 32:
-			if (ctx->relaxed_mmio) {
+			if (config->use_relaxed_mmio) {
 				ctx->reg_read = regmap_mmio_read32le_relaxed;
 				ctx->reg_write = regmap_mmio_write32le_relaxed;
 			} else {
@@ -321,7 +319,7 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 			break;
 #ifdef CONFIG_64BIT
 		case 64:
-			if (ctx->relaxed_mmio) {
+			if (config->use_relaxed_mmio) {
 				ctx->reg_read = regmap_mmio_read64le_relaxed;
 				ctx->reg_write = regmap_mmio_write64le_relaxed;
 			} else {
-- 
2.35.1

[PATCH v1 4/5] regmap: mmio: Get rid of broken 64-bit IO

[Andy Shevchenko]

The current implementation, besides having no active users, is broken
by design of regmap. For 64-bit IO we need to supply 64-bit value,
otherwise there is no way to handle upper 32 bits in 64-bit register.

Hence, remove the broken IO accessors for good and wait for real user
that can fix entire regmap API for that.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/regmap/regmap-mmio.c | 49 -------------------------------
 1 file changed, 49 deletions(-)

diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index c2b18973144b..698295a8f5a6 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -32,9 +32,6 @@ static int regmap_mmio_regbits_check(size_t reg_bits)
 	case 8:
 	case 16:
 	case 32:
-#ifdef CONFIG_64BIT
-	case 64:
-#endif
 		return 0;
 	default:
 		return -EINVAL;
@@ -56,11 +53,6 @@ static int regmap_mmio_get_min_stride(size_t val_bits)
 	case 32:
 		min_stride = 4;
 		break;
-#ifdef CONFIG_64BIT
-	case 64:
-		min_stride = 8;
-		break;
-#endif
 	default:
 		return -EINVAL;
 	}
@@ -124,22 +116,6 @@ static void regmap_mmio_write32be(struct regmap_mmio_context *ctx,
 	iowrite32be(val, ctx->regs + reg);
 }
 
-#ifdef CONFIG_64BIT
-static void regmap_mmio_write64le(struct regmap_mmio_context *ctx,
-				  unsigned int reg,
-				  unsigned int val)
-{
-	writeq(val, ctx->regs + reg);
-}
-
-static void regmap_mmio_write64le_relaxed(struct regmap_mmio_context *ctx,
-				  unsigned int reg,
-				  unsigned int val)
-{
-	writeq_relaxed(val, ctx->regs + reg);
-}
-#endif
-
 static int regmap_mmio_write(void *context, unsigned int reg, unsigned int val)
 {
 	struct regmap_mmio_context *ctx = context;
@@ -204,20 +180,6 @@ static unsigned int regmap_mmio_read32be(struct regmap_mmio_context *ctx,
 	return ioread32be(ctx->regs + reg);
 }
 
-#ifdef CONFIG_64BIT
-static unsigned int regmap_mmio_read64le(struct regmap_mmio_context *ctx,
-				         unsigned int reg)
-{
-	return readq(ctx->regs + reg);
-}
-
-static unsigned int regmap_mmio_read64le_relaxed(struct regmap_mmio_context *ctx,
-						 unsigned int reg)
-{
-	return readq_relaxed(ctx->regs + reg);
-}
-#endif
-
 static int regmap_mmio_read(void *context, unsigned int reg, unsigned int *val)
 {
 	struct regmap_mmio_context *ctx = context;
@@ -317,17 +279,6 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 				ctx->reg_write = regmap_mmio_write32le;
 			}
 			break;
-#ifdef CONFIG_64BIT
-		case 64:
-			if (config->use_relaxed_mmio) {
-				ctx->reg_read = regmap_mmio_read64le_relaxed;
-				ctx->reg_write = regmap_mmio_write64le_relaxed;
-			} else {
-				ctx->reg_read = regmap_mmio_read64le;
-				ctx->reg_write = regmap_mmio_write64le;
-			}
-			break;
-#endif
 		default:
 			ret = -EINVAL;
 			goto err_free;
-- 
2.35.1

[PATCH v1 5/5] regmap: mmio: Introduce IO accessors that can talk to IO port

[Andy Shevchenko]

Currently regmap MMIO is inconsistent with IO accessors. I.e.
the Big Endian counterparts are using ioreadXXbe() / iowriteXXbe()
which are not clean implementations of readXXbe(). Besides that
some users may use regmap MMIO for IO ports, and this can be done
by assigning ioreadXX()/iowriteXX() and their Big Endian counterparts
to the regmap context.

That said, reimplement current Big Endian MMIO accessors by replacing
ioread()/iowrite() with respective read()/write() and swab() calls.
While at it, add IO port support with a corresponding flag added.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/regmap/regmap-mmio.c | 102 +++++++++++++++++++++++++++---
 include/linux/regmap.h            |   3 +
 2 files changed, 96 insertions(+), 9 deletions(-)

diff --git a/drivers/base/regmap/regmap-mmio.c b/drivers/base/regmap/regmap-mmio.c
index 698295a8f5a6..6a0d370ac83b 100644
--- a/drivers/base/regmap/regmap-mmio.c
+++ b/drivers/base/regmap/regmap-mmio.c
@@ -74,6 +74,12 @@ static void regmap_mmio_write8_relaxed(struct regmap_mmio_context *ctx,
 	writeb_relaxed(val, ctx->regs + reg);
 }
 
+static void regmap_mmio_iowrite8(struct regmap_mmio_context *ctx,
+				 unsigned int reg, unsigned int val)
+{
+	iowrite8(val, ctx->regs + reg);
+}
+
 static void regmap_mmio_write16le(struct regmap_mmio_context *ctx,
 				  unsigned int reg,
 				  unsigned int val)
@@ -88,9 +94,21 @@ static void regmap_mmio_write16le_relaxed(struct regmap_mmio_context *ctx,
 	writew_relaxed(val, ctx->regs + reg);
 }
 
+static void regmap_mmio_iowrite16le(struct regmap_mmio_context *ctx,
+				    unsigned int reg, unsigned int val)
+{
+	iowrite16(val, ctx->regs + reg);
+}
+
 static void regmap_mmio_write16be(struct regmap_mmio_context *ctx,
 				  unsigned int reg,
 				  unsigned int val)
+{
+	writew(swab16(val), ctx->regs + reg);
+}
+
+static void regmap_mmio_iowrite16be(struct regmap_mmio_context *ctx,
+				    unsigned int reg, unsigned int val)
 {
 	iowrite16be(val, ctx->regs + reg);
 }
@@ -109,9 +127,21 @@ static void regmap_mmio_write32le_relaxed(struct regmap_mmio_context *ctx,
 	writel_relaxed(val, ctx->regs + reg);
 }
 
+static void regmap_mmio_iowrite32le(struct regmap_mmio_context *ctx,
+				    unsigned int reg, unsigned int val)
+{
+	iowrite32(val, ctx->regs + reg);
+}
+
 static void regmap_mmio_write32be(struct regmap_mmio_context *ctx,
 				  unsigned int reg,
 				  unsigned int val)
+{
+	writel(swab32(val), ctx->regs + reg);
+}
+
+static void regmap_mmio_iowrite32be(struct regmap_mmio_context *ctx,
+				    unsigned int reg, unsigned int val)
 {
 	iowrite32be(val, ctx->regs + reg);
 }
@@ -144,6 +174,12 @@ static unsigned int regmap_mmio_read8_relaxed(struct regmap_mmio_context *ctx,
 	return readb_relaxed(ctx->regs + reg);
 }
 
+static unsigned int regmap_mmio_ioread8(struct regmap_mmio_context *ctx,
+					unsigned int reg)
+{
+	return ioread8(ctx->regs + reg);
+}
+
 static unsigned int regmap_mmio_read16le(struct regmap_mmio_context *ctx,
 				         unsigned int reg)
 {
@@ -156,8 +192,20 @@ static unsigned int regmap_mmio_read16le_relaxed(struct regmap_mmio_context *ctx
 	return readw_relaxed(ctx->regs + reg);
 }
 
+static unsigned int regmap_mmio_ioread16le(struct regmap_mmio_context *ctx,
+					   unsigned int reg)
+{
+	return ioread16(ctx->regs + reg);
+}
+
 static unsigned int regmap_mmio_read16be(struct regmap_mmio_context *ctx,
 				         unsigned int reg)
+{
+	return swab16(readw(ctx->regs + reg));
+}
+
+static unsigned int regmap_mmio_ioread16be(struct regmap_mmio_context *ctx,
+					   unsigned int reg)
 {
 	return ioread16be(ctx->regs + reg);
 }
@@ -174,8 +222,20 @@ static unsigned int regmap_mmio_read32le_relaxed(struct regmap_mmio_context *ctx
 	return readl_relaxed(ctx->regs + reg);
 }
 
+static unsigned int regmap_mmio_ioread32le(struct regmap_mmio_context *ctx,
+					   unsigned int reg)
+{
+	return ioread32(ctx->regs + reg);
+}
+
 static unsigned int regmap_mmio_read32be(struct regmap_mmio_context *ctx,
 				         unsigned int reg)
+{
+	return swab32(readl(ctx->regs + reg));
+}
+
+static unsigned int regmap_mmio_ioread32be(struct regmap_mmio_context *ctx,
+					   unsigned int reg)
 {
 	return ioread32be(ctx->regs + reg);
 }
@@ -253,7 +313,10 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 #endif
 		switch (config->val_bits) {
 		case 8:
-			if (config->use_relaxed_mmio) {
+			if (config->io_port) {
+				ctx->reg_read = regmap_mmio_ioread8;
+				ctx->reg_write = regmap_mmio_iowrite8;
+			} else if (config->use_relaxed_mmio) {
 				ctx->reg_read = regmap_mmio_read8_relaxed;
 				ctx->reg_write = regmap_mmio_write8_relaxed;
 			} else {
@@ -262,7 +325,10 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 			}
 			break;
 		case 16:
-			if (config->use_relaxed_mmio) {
+			if (config->io_port) {
+				ctx->reg_read = regmap_mmio_ioread16le;
+				ctx->reg_write = regmap_mmio_iowrite16le;
+			} else if (config->use_relaxed_mmio) {
 				ctx->reg_read = regmap_mmio_read16le_relaxed;
 				ctx->reg_write = regmap_mmio_write16le_relaxed;
 			} else {
@@ -271,7 +337,10 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 			}
 			break;
 		case 32:
-			if (config->use_relaxed_mmio) {
+			if (config->io_port) {
+				ctx->reg_read = regmap_mmio_ioread32le;
+				ctx->reg_write = regmap_mmio_iowrite32le;
+			} else if (config->use_relaxed_mmio) {
 				ctx->reg_read = regmap_mmio_read32le_relaxed;
 				ctx->reg_write = regmap_mmio_write32le_relaxed;
 			} else {
@@ -290,16 +359,31 @@ static struct regmap_mmio_context *regmap_mmio_gen_context(struct device *dev,
 #endif
 		switch (config->val_bits) {
 		case 8:
-			ctx->reg_read = regmap_mmio_read8;
-			ctx->reg_write = regmap_mmio_write8;
+			if (config->io_port) {
+				ctx->reg_read = regmap_mmio_ioread8;
+				ctx->reg_write = regmap_mmio_iowrite8;
+			} else {
+				ctx->reg_read = regmap_mmio_read8;
+				ctx->reg_write = regmap_mmio_write8;
+			}
 			break;
 		case 16:
-			ctx->reg_read = regmap_mmio_read16be;
-			ctx->reg_write = regmap_mmio_write16be;
+			if (config->io_port) {
+				ctx->reg_read = regmap_mmio_ioread16be;
+				ctx->reg_write = regmap_mmio_iowrite16be;
+			} else {
+				ctx->reg_read = regmap_mmio_read16be;
+				ctx->reg_write = regmap_mmio_write16be;
+			}
 			break;
 		case 32:
-			ctx->reg_read = regmap_mmio_read32be;
-			ctx->reg_write = regmap_mmio_write32be;
+			if (config->io_port) {
+				ctx->reg_read = regmap_mmio_ioread32be;
+				ctx->reg_write = regmap_mmio_iowrite32be;
+			} else {
+				ctx->reg_read = regmap_mmio_read32be;
+				ctx->reg_write = regmap_mmio_write32be;
+			}
 			break;
 		default:
 			ret = -EINVAL;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 7cf2157134ac..8cccc247cd37 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -311,6 +311,8 @@ typedef void (*regmap_unlock)(void *);
  *		  This field is a duplicate of a similar file in
  *		  'struct regmap_bus' and serves exact same purpose.
  *		   Use it only for "no-bus" cases.
+ * @io_port:	  Support IO port accessors. Makes sense only when MMIO vs. IO port
+ *		  access can be distinguished.
  * @max_register: Optional, specifies the maximum valid register address.
  * @wr_table:     Optional, points to a struct regmap_access_table specifying
  *                valid ranges for write access.
@@ -399,6 +401,7 @@ struct regmap_config {
 	size_t max_raw_write;
 
 	bool fast_io;
+	bool io_port;
 
 	unsigned int max_register;
 	const struct regmap_access_table *wr_table;
-- 
2.35.1

Re: [PATCH v1 5/5] regmap: mmio: Introduce IO accessors that can talk to IO port

[Andy Shevchenko]

On Fri, Aug 5, 2022 at 11:14 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Currently regmap MMIO is inconsistent with IO accessors. I.e.
> the Big Endian counterparts are using ioreadXXbe() / iowriteXXbe()
> which are not clean implementations of readXXbe(). Besides that
> some users may use regmap MMIO for IO ports, and this can be done
> by assigning ioreadXX()/iowriteXX() and their Big Endian counterparts
> to the regmap context.
>
> That said, reimplement current Big Endian MMIO accessors by replacing
> ioread()/iowrite() with respective read()/write() and swab() calls.
> While at it, add IO port support with a corresponding flag added.

William, I believe this series allows you to switch PC104 drivers to use regmap.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] regmap: mmio: Introduce IO accessors that can talk to IO port

[Mark Brown]

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

On Fri, Aug 05, 2022 at 11:53:21PM +0300, Andy Shevchenko wrote:

> Currently regmap MMIO is inconsistent with IO accessors. I.e.
> the Big Endian counterparts are using ioreadXXbe() / iowriteXXbe()
> which are not clean implementations of readXXbe(). Besides that
> some users may use regmap MMIO for IO ports, and this can be done
> by assigning ioreadXX()/iowriteXX() and their Big Endian counterparts
> to the regmap context.

Have you validated that nothing is relying on whatever the problem is
with using the io versions?

> That said, reimplement current Big Endian MMIO accessors by replacing
> ioread()/iowrite() with respective read()/write() and swab() calls.
> While at it, add IO port support with a corresponding flag added.

This should be a separate patch.

> +			if (config->io_port) {
> +				ctx->reg_read = regmap_mmio_ioread8;
> +				ctx->reg_write = regmap_mmio_iowrite8;
> +			} else if (config->use_relaxed_mmio) {

If these options are mutually exclusive we should validate that they are
not simultaneously set.

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

Re: [PATCH v1 5/5] regmap: mmio: Introduce IO accessors that can talk to IO port

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 3:31 PM Mark Brown <broonie@kernel.org> wrote:
> On Fri, Aug 05, 2022 at 11:53:21PM +0300, Andy Shevchenko wrote:
>
> > Currently regmap MMIO is inconsistent with IO accessors. I.e.
> > the Big Endian counterparts are using ioreadXXbe() / iowriteXXbe()
> > which are not clean implementations of readXXbe(). Besides that
> > some users may use regmap MMIO for IO ports, and this can be done
> > by assigning ioreadXX()/iowriteXX() and their Big Endian counterparts
> > to the regmap context.
>
> Have you validated that nothing is relying on whatever the problem is
> with using the io versions?

I have cross-checked 1) the architectures that are BE and have IO port
capability, and 2) the drivers that are using regmap MMIO with a
big-endian setting. I found no driver is mapping IO ports and uses
regmap MMIO at the same time. The architecture wise the x86 and ia64
are not in question, I think. And alpha is more academical nowadays.
Did I miss anything?

That said, I'm 99.999% sure there is no problem with that.

...

> > That said, reimplement current Big Endian MMIO accessors by replacing
> > ioread()/iowrite() with respective read()/write() and swab() calls.
> > While at it, add IO port support with a corresponding flag added.
>
> This should be a separate patch.

OK! Then we remove some code and (re-)add it later. Do we need this churn?
Another way is to add IO port accessors and then fix the MMIO.

...

> > +                     if (config->io_port) {
> > +                             ctx->reg_read = regmap_mmio_ioread8;
> > +                             ctx->reg_write = regmap_mmio_iowrite8;
> > +                     } else if (config->use_relaxed_mmio) {
>
> If these options are mutually exclusive we should validate that they are
> not simultaneously set.

Yes, the validation is missed. I will add it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] regmap: mmio: Introduce IO accessors that can talk to IO port

[Mark Brown]

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

On Mon, Aug 08, 2022 at 04:40:26PM +0200, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 3:31 PM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Aug 05, 2022 at 11:53:21PM +0300, Andy Shevchenko wrote:

> > > Currently regmap MMIO is inconsistent with IO accessors. I.e.
> > > the Big Endian counterparts are using ioreadXXbe() / iowriteXXbe()
> > > which are not clean implementations of readXXbe(). Besides that
> > > some users may use regmap MMIO for IO ports, and this can be done
> > > by assigning ioreadXX()/iowriteXX() and their Big Endian counterparts
> > > to the regmap context.

> > Have you validated that nothing is relying on whatever the problem is
> > with using the io versions?

> I have cross-checked 1) the architectures that are BE and have IO port
> capability, and 2) the drivers that are using regmap MMIO with a
> big-endian setting. I found no driver is mapping IO ports and uses
> regmap MMIO at the same time. The architecture wise the x86 and ia64
> are not in question, I think. And alpha is more academical nowadays.
> Did I miss anything?

> That said, I'm 99.999% sure there is no problem with that.

The issue is the potential that something that is currently using the
ioport accessors might be relying on whatever it is that they do that
causes a problem.

> > > That said, reimplement current Big Endian MMIO accessors by replacing
> > > ioread()/iowrite() with respective read()/write() and swab() calls.
> > > While at it, add IO port support with a corresponding flag added.

> > This should be a separate patch.

> OK! Then we remove some code and (re-)add it later. Do we need this churn?
> Another way is to add IO port accessors and then fix the MMIO.

Add and then fix seems sensible,

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

Re: [PATCH v1 5/5] regmap: mmio: Introduce IO accessors that can talk to IO port

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 6:05 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Aug 08, 2022 at 04:40:26PM +0200, Andy Shevchenko wrote:
> > On Mon, Aug 8, 2022 at 3:31 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Fri, Aug 05, 2022 at 11:53:21PM +0300, Andy Shevchenko wrote:

...

> > > > That said, reimplement current Big Endian MMIO accessors by replacing
> > > > ioread()/iowrite() with respective read()/write() and swab() calls.
> > > > While at it, add IO port support with a corresponding flag added.
>
> > > This should be a separate patch.
>
> > OK! Then we remove some code and (re-)add it later. Do we need this churn?
> > Another way is to add IO port accessors and then fix the MMIO.
>
> Add and then fix seems sensible,

Got it, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Mark Brown]

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

On Fri, Aug 05, 2022 at 11:53:17PM +0300, Andy Shevchenko wrote:
> The commit eb4a219d19fd ("regmap: Skip clk_put for attached clocks when
> freeing context") oexcluded clk_put() call on regmap freeing. But the
> same is needed for clk_unprepare() since the regmap MMIO users should
> call regmap_mmio_detach_clk() which does unprepare the clock. Update
> the code accordingly, so neither clk_put() nor clk_unprepare() would
> be called for the attached clock.

regmap_mmio_attach_clk() prepares the clock that's passed in, we should
undo that when detaching otherwise we're leaking a prepare (as we do in
the explicit detach).

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

Re: [PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 3:19 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Aug 05, 2022 at 11:53:17PM +0300, Andy Shevchenko wrote:
> > The commit eb4a219d19fd ("regmap: Skip clk_put for attached clocks when
> > freeing context") oexcluded clk_put() call on regmap freeing. But the
> > same is needed for clk_unprepare() since the regmap MMIO users should
> > call regmap_mmio_detach_clk() which does unprepare the clock. Update
> > the code accordingly, so neither clk_put() nor clk_unprepare() would
> > be called for the attached clock.
>
> regmap_mmio_attach_clk() prepares the clock that's passed in, we should
> undo that when detaching otherwise we're leaking a prepare (as we do in
> the explicit detach).

Why do we allow the user to avoid explicit detach? What is the point
of having that API in the case we take care of it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Mark Brown]

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

On Mon, Aug 08, 2022 at 03:41:48PM +0200, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 3:19 PM Mark Brown <broonie@kernel.org> wrote:

> > regmap_mmio_attach_clk() prepares the clock that's passed in, we should
> > undo that when detaching otherwise we're leaking a prepare (as we do in
> > the explicit detach).

> Why do we allow the user to avoid explicit detach? What is the point
> of having that API in the case we take care of it?

I think just for symmetry so it's obvious that error handling is
happening if people want it to be.

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

Re: [PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 3:48 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Aug 08, 2022 at 03:41:48PM +0200, Andy Shevchenko wrote:
> > On Mon, Aug 8, 2022 at 3:19 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > regmap_mmio_attach_clk() prepares the clock that's passed in, we should
> > > undo that when detaching otherwise we're leaking a prepare (as we do in
> > > the explicit detach).
>
> > Why do we allow the user to avoid explicit detach? What is the point
> > of having that API in the case we take care of it?
>
> I think just for symmetry so it's obvious that error handling is
> happening if people want it to be.

So, the only user of that API calls it explicitly. Should I rewrite a
commit message somehow?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Mark Brown]

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

On Mon, Aug 08, 2022 at 04:42:28PM +0200, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 3:48 PM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Aug 08, 2022 at 03:41:48PM +0200, Andy Shevchenko wrote:
> > > On Mon, Aug 8, 2022 at 3:19 PM Mark Brown <broonie@kernel.org> wrote:

> > > > regmap_mmio_attach_clk() prepares the clock that's passed in, we should
> > > > undo that when detaching otherwise we're leaking a prepare (as we do in
> > > > the explicit detach).

> > > Why do we allow the user to avoid explicit detach? What is the point
> > > of having that API in the case we take care of it?

> > I think just for symmetry so it's obvious that error handling is
> > happening if people want it to be.

> So, the only user of that API calls it explicitly. Should I rewrite a
> commit message somehow?

No.  Your commit would just introduce a bug.

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

Re: [PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Andy Shevchenko]

On Mon, Aug 8, 2022 at 5:52 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Aug 08, 2022 at 04:42:28PM +0200, Andy Shevchenko wrote:
> > On Mon, Aug 8, 2022 at 3:48 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Mon, Aug 08, 2022 at 03:41:48PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Aug 8, 2022 at 3:19 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > > > regmap_mmio_attach_clk() prepares the clock that's passed in, we should
> > > > > undo that when detaching otherwise we're leaking a prepare (as we do in
> > > > > the explicit detach).
>
> > > > Why do we allow the user to avoid explicit detach? What is the point
> > > > of having that API in the case we take care of it?
>
> > > I think just for symmetry so it's obvious that error handling is
> > > happening if people want it to be.
>
> > So, the only user of that API calls it explicitly. Should I rewrite a
> > commit message somehow?
>
> No.  Your commit would just introduce a bug.

There is no bug with the existing codebase after this commit. Are you
talking about out-of-tree modules? Or maybe there is documentation
that says that regmap clears all additionally attached resources?

Okay I may admit that Fixes tag might be wrong due to potential users
in the past. However, in the current state of affairs either we can
proceed with a patch, or amend documentation (if not yet done) to
clarify this aspect of regmap MMIO. From the above I may assume that
you would rather expect the latter to be done (if not yet).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Mark Brown]

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

On Mon, Aug 08, 2022 at 08:23:28PM +0200, Andy Shevchenko wrote:
> On Mon, Aug 8, 2022 at 5:52 PM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Aug 08, 2022 at 04:42:28PM +0200, Andy Shevchenko wrote:

> > > > I think just for symmetry so it's obvious that error handling is
> > > > happening if people want it to be.

> > > So, the only user of that API calls it explicitly. Should I rewrite a
> > > commit message somehow?

> > No.  Your commit would just introduce a bug.

> There is no bug with the existing codebase after this commit. Are you
> talking about out-of-tree modules? Or maybe there is documentation
> that says that regmap clears all additionally attached resources?

If the regmap code prepared a clock it should unprepare it otherwise we
leaked a reference - the caller shouldn't have to worry what enables
are done by regmap inside the implementation (and conversely regmap is
making sure the clock is prepared and enabled when it's needed).

> Okay I may admit that Fixes tag might be wrong due to potential users
> in the past. However, in the current state of affairs either we can
> proceed with a patch, or amend documentation (if not yet done) to
> clarify this aspect of regmap MMIO. From the above I may assume that
> you would rather expect the latter to be done (if not yet).

I don't understand why you think we need any change at all here.  The
only effect I can see is to make the code less robust in the case where
the user doesn't explicitly detach the clock which is not something we
document as mandatory and doesn't strike me as something where there's a
pressing need for it to be mandatory.

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

Re: (subset) [PATCH v1 1/5] regmap: mmio: Don't unprepare attached clock

[Mark Brown]

On Fri, 5 Aug 2022 23:53:17 +0300, Andy Shevchenko wrote:
> The commit eb4a219d19fd ("regmap: Skip clk_put for attached clocks when
> freeing context") oexcluded clk_put() call on regmap freeing. But the
> same is needed for clk_unprepare() since the regmap MMIO users should
> call regmap_mmio_detach_clk() which does unprepare the clock. Update
> the code accordingly, so neither clk_put() nor clk_unprepare() would
> be called for the attached clock.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[3/5] regmap: mmio: Remove mmio_relaxed member from context
      commit: ada79bca380009a85d1e643e5a4da0c079f28225

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark