[PATCH lora-next 00/11] net: lora: sx130x: Fixes for enabling regcache

[PATCH lora-next 00/11] net: lora: sx130x: Fixes for enabling regcache

[Andreas Färber]

Hello,

This series fixes a number of issues that stood in the way of enabling the
regmap cache for SX130x. It goes on to enable REGCACHE_RBTREE.

1) Soft reset needs special treatment.
2) More registers need to be treated as volatile.
3) Some register field writes need to be forced.

This complements Ben Whitten's regmap conversion patches [1] I already squashed,
mainly [2]. I intend to squash these into my driver commit as well.

In my testing it has not regressed with spi-gpio or cdc-acm, nor with spi-sun6i;
given that we seem no closer to a solution for the clk lock-up, we may need to
revert that commit from [1] again.

Have a lot of fun!

Cheers,
Andreas

[1] https://patchwork.ozlabs.org/cover/983168/
[2] https://patchwork.ozlabs.org/patch/983170/

Cc: Ben Whitten <ben.whitten@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-lpwan@lists.infradead.org
Cc: netdev@vger.kernel.org

Andreas Färber (11):
  net: lora: sx125x: Abort on wrong version
  net: lora: sx130x: Fix error codes
  net: lora: sx130x: Revert drop of sx1301_soft_reset()
  net: lora: sx130x: Fix soft reset
  net: lora: sx130x: Fix regmap field toggles
  net: lora: sx130x: Force regmap field write before sleep
  net: lora: sx130x: Force regmap field writes before function
    boundaries
  net: lora: sx130x: Force regmap field write before reading status
  net: lora: sx130x: Mark AGCSTS register as volatile
  net: lora: sx130x: Mark MCU_CTRL register as volatile
  net: lora: sx130x: Enable regmap caching

 drivers/net/lora/sx125x.c |  6 ++++-
 drivers/net/lora/sx130x.c | 65 +++++++++++++++++++++++++++++++++--------------
 2 files changed, 51 insertions(+), 20 deletions(-)

-- 
2.16.4

[PATCH lora-next 01/11] net: lora: sx125x: Abort on wrong version

[Andreas Färber]

This helps detect issues such as the concentrator being in reset.

Enhance error output while at it.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx125x.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/lora/sx125x.c b/drivers/net/lora/sx125x.c
index 90e7cdb9d1a8..0228d1f887a0 100644
--- a/drivers/net/lora/sx125x.c
+++ b/drivers/net/lora/sx125x.c
@@ -195,9 +195,13 @@ static int __maybe_unused sx125x_regmap_probe(struct device *dev, struct regmap
 	if (true) {
 		ret = regmap_read(priv->regmap, SX1255_VERSION, &val);
 		if (ret) {
-			dev_err(dev, "version read failed\n");
+			dev_err(dev, "version read failed (%d)\n", ret);
 			return ret;
 		}
+		if (val != 0x21) {
+			dev_err(dev, "unexpected version: %u\n", val);
+			return -EINVAL;
+		}
 		dev_info(dev, "SX125x version: %02x\n", val);
 	}
 
-- 
2.16.4

[PATCH lora-next 02/11] net: lora: sx130x: Fix error codes

[Andreas Färber]

Use of -ENXIO results in a misleading error message from ip command:

  RTNETLINK answers: No such device or address

Switch to -EIO for more accurate description:

  RTNETLINK answers: Input/output error

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 88d4ce1aabcf..21db0ddb101f 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -353,7 +353,7 @@ static int sx130x_agc_calibrate(struct sx130x_priv *priv)
 	if (val != SX1301_MCU_AGC_CAL_FW_VERSION) {
 		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
 				SX1301_MCU_AGC_CAL_FW_VERSION);
-		return -ENXIO;
+		return -EIO;
 	}
 
 	ret = sx130x_field_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 0);
@@ -380,7 +380,7 @@ static int sx130x_agc_calibrate(struct sx130x_priv *priv)
 	dev_info(priv->dev, "AGC status: %02x\n", (unsigned)val);
 	if ((val & (BIT(7) | BIT(0))) != (BIT(7) | BIT(0))) {
 		dev_err(priv->dev, "AGC calibration failed\n");
-		return -ENXIO;
+		return -EIO;
 	}
 
 	return 0;
@@ -453,7 +453,7 @@ static int sx130x_load_all_firmware(struct sx130x_priv *priv)
 	if (val != SX1301_MCU_AGC_FW_VERSION) {
 		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
 				SX1301_MCU_AGC_FW_VERSION);
-		return -ENXIO;
+		return -EIO;
 	}
 
 	ret = sx130x_arb_ram_read(priv, 0x20, &val);
@@ -467,7 +467,7 @@ static int sx130x_load_all_firmware(struct sx130x_priv *priv)
 	if (val != SX1301_MCU_ARB_FW_VERSION) {
 		dev_err(priv->dev, "unexpected firmware version, expecting %u\n",
 				SX1301_MCU_ARB_FW_VERSION);
-		return -ENXIO;
+		return -EIO;
 	}
 
 	return 0;
@@ -636,7 +636,7 @@ int sx130x_probe(struct device *dev)
 
 	if (ver != SX1301_CHIP_VERSION) {
 		dev_err(dev, "unexpected version: %u\n", ver);
-		return -ENXIO;
+		return -EIO;
 	}
 
 	ret = regmap_write(priv->regmap, SX1301_PAGE, 0);
-- 
2.16.4

[PATCH lora-next 03/11] net: lora: sx130x: Revert drop of sx1301_soft_reset()

[Andreas Färber]

Ben's regmap field conversion patch silently dropped my sx1301_soft_reset() helper.
Soft reset is a special operation, so restore this function as sx130x_soft_reset().

To be squashed.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 21db0ddb101f..7a387d9a75a0 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -180,6 +180,11 @@ static int sx130x_field_write(struct sx130x_priv *priv,
 	return regmap_field_write(priv->regmap_fields[field_id], val);
 }
 
+static int sx130x_soft_reset(struct sx130x_priv *priv)
+{
+	return sx130x_field_write(priv, F_SOFT_RESET, 1);
+}
+
 static int sx130x_agc_ram_read(struct sx130x_priv *priv, u8 addr, unsigned int *val)
 {
 	int ret;
@@ -645,7 +650,7 @@ int sx130x_probe(struct device *dev)
 		return ret;
 	}
 
-	ret = sx130x_field_write(priv, F_SOFT_RESET, 1);
+	ret = sx130x_soft_reset(priv);
 	if (ret) {
 		dev_err(dev, "soft reset failed (%d)\n", ret);
 		return ret;
-- 
2.16.4

[PATCH lora-next 04/11] net: lora: sx130x: Fix soft reset

[Andreas Färber]

The soft reset bit is volatile. As it lives in the frequently accessed
page register, refrain from marking the register as volatile and
instead bypass the cache for this one write.

Mark the cache as dirty afterwards. This does not appear to clear it,
so manually drop the whole cache. If we don't have a cache configured,
this may return -EINVAL, so guard it appropriately to aid in testing.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 7a387d9a75a0..4ba02836a35d 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -182,7 +182,19 @@ static int sx130x_field_write(struct sx130x_priv *priv,
 
 static int sx130x_soft_reset(struct sx130x_priv *priv)
 {
-	return sx130x_field_write(priv, F_SOFT_RESET, 1);
+	int ret;
+
+	regcache_cache_bypass(priv->regmap, true);
+	ret = sx130x_field_write(priv, F_SOFT_RESET, 1);
+	regcache_cache_bypass(priv->regmap, false);
+	if (ret)
+		return ret;
+
+	regcache_mark_dirty(priv->regmap);
+	if (sx130x_regmap_config.cache_type != REGCACHE_NONE)
+		return regcache_drop_region(priv->regmap,
+			0, sx130x_regmap_config.max_register);
+	return 0;
 }
 
 static int sx130x_agc_ram_read(struct sx130x_priv *priv, u8 addr, unsigned int *val)
-- 
2.16.4

[PATCH lora-next 05/11] net: lora: sx130x: Fix regmap field toggles

[Andreas Färber]

When toggling the value of a regmap field, such as for F_RADIO_RST or
F_EMERGENCY_FORCE_HOST_CTRL, we need to ensure it gets written out.

Sadly the timing of the writes got lost in the regmap field conversion.

Introduce an sx130x_field_force_write() helper for this.
While at it, make trivial sx130x_field_write() static inline, too.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 4ba02836a35d..457f2396d9ee 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -174,12 +174,18 @@ const struct regmap_config sx130x_regmap_config = {
 };
 EXPORT_SYMBOL_GPL(sx130x_regmap_config);
 
-static int sx130x_field_write(struct sx130x_priv *priv,
+static inline int sx130x_field_write(struct sx130x_priv *priv,
 		enum sx130x_fields field_id, u8 val)
 {
 	return regmap_field_write(priv->regmap_fields[field_id], val);
 }
 
+static inline int sx130x_field_force_write(struct sx130x_priv *priv,
+		enum sx130x_fields field_id, u8 val)
+{
+	return regmap_field_force_write(priv->regmap_fields[field_id], val);
+}
+
 static int sx130x_soft_reset(struct sx130x_priv *priv)
 {
 	int ret;
@@ -373,7 +379,7 @@ static int sx130x_agc_calibrate(struct sx130x_priv *priv)
 		return -EIO;
 	}
 
-	ret = sx130x_field_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 0);
+	ret = sx130x_field_force_write(priv, F_EMERGENCY_FORCE_HOST_CTRL, 0);
 	if (ret) {
 		dev_err(priv->dev, "emergency force failed\n");
 		return ret;
@@ -694,7 +700,7 @@ int sx130x_probe(struct device *dev)
 
 	msleep(500);
 
-	ret = sx130x_field_write(priv, F_RADIO_RST, 1);
+	ret = sx130x_field_force_write(priv, F_RADIO_RST, 1);
 	if (ret) {
 		dev_err(dev, "radio assert reset failed (%d)\n", ret);
 		return ret;
-- 
2.16.4

[PATCH lora-next 06/11] net: lora: sx130x: Force regmap field write before sleep

[Andreas Färber]

Ensure timing is as expected by forcing a register write before waiting on
its outcome.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 457f2396d9ee..bc2d4b1165d8 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -692,7 +692,7 @@ int sx130x_probe(struct device *dev)
 		return ret;
 	}
 
-	ret = sx130x_field_write(priv, F_RADIO_B_EN, 1);
+	ret = sx130x_field_force_write(priv, F_RADIO_B_EN, 1);
 	if (ret) {
 		dev_err(dev, "radio B enable failed (%d)\n", ret);
 		return ret;
-- 
2.16.4

[PATCH lora-next 07/11] net: lora: sx130x: Force regmap field writes before function boundaries

[Andreas Färber]

As a cautionary step, force writing out registers before calling helpers
or returning.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index bc2d4b1165d8..a0759e277574 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -315,7 +315,7 @@ static int sx130x_load_firmware(struct sx130x_priv *priv, int mcu, const struct
 
 	kfree(buf);
 
-	ret = sx130x_field_write(priv, select_mux, 1);
+	ret = sx130x_field_force_write(priv, select_mux, 1);
 	if (ret) {
 		dev_err(priv->dev, "MCU RAM release mux failed\n");
 		return ret;
@@ -359,7 +359,7 @@ static int sx130x_agc_calibrate(struct sx130x_priv *priv)
 		return ret;
 	}
 
-	ret = sx130x_field_write(priv, F_MCU_RST_1, 0);
+	ret = sx130x_field_force_write(priv, F_MCU_RST_1, 0);
 	if (ret) {
 		dev_err(priv->dev, "MCU 1 reset failed\n");
 		return ret;
@@ -459,7 +459,7 @@ static int sx130x_load_all_firmware(struct sx130x_priv *priv)
 		return ret;
 	}
 
-	ret = sx130x_field_write(priv, F_MCU_RST_1, 0);
+	ret = sx130x_field_force_write(priv, F_MCU_RST_1, 0);
 	if (ret) {
 		dev_err(priv->dev, "MCU 1 release failed\n");
 		return ret;
@@ -542,7 +542,7 @@ static int sx130x_loradev_open(struct net_device *netdev)
 		goto err_reg;
 	}
 
-	ret = sx130x_field_write(priv, F_CLK32M_EN, 1);
+	ret = sx130x_field_force_write(priv, F_CLK32M_EN, 1);
 	if (ret) {
 		dev_err(priv->dev, "enable 32M clock failed (%d)\n", ret);
 		goto err_reg;
@@ -708,7 +708,7 @@ int sx130x_probe(struct device *dev)
 
 	msleep(5);
 
-	ret = sx130x_field_write(priv, F_RADIO_RST, 0);
+	ret = sx130x_field_force_write(priv, F_RADIO_RST, 0);
 	if (ret) {
 		dev_err(dev, "radio deassert reset failed (%d)\n", ret);
 		return ret;
-- 
2.16.4

[PATCH lora-next 08/11] net: lora: sx130x: Force regmap field write before reading status

[Andreas Färber]

Ensure that the F_FORCE_HOST_RADIO_CTRL field gets written before we read
the AGC status register. Otherwise it returns status 01 instead of 87.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index a0759e277574..0a79548a7480 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -343,7 +343,7 @@ static int sx130x_agc_calibrate(struct sx130x_priv *priv)
 		return ret;
 	}
 
-	ret = sx130x_field_write(priv, F_FORCE_HOST_RADIO_CTRL, 0);
+	ret = sx130x_field_force_write(priv, F_FORCE_HOST_RADIO_CTRL, 0);
 	if (ret) {
 		dev_err(priv->dev, "force host control failed\n");
 		return ret;
-- 
2.16.4

[PATCH lora-next 09/11] net: lora: sx130x: Mark AGCSTS register as volatile

[Andreas Färber]

AGC status register reads should not be cached.

Sort the volatile registers by number while at it.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 0a79548a7480..8bdd343a273f 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -123,11 +123,13 @@ static const struct regmap_range_cfg sx130x_regmap_ranges[] = {
 static bool sx130x_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
-	case SX1301_DBG_AGC_MCU_RAM_DATA:
-	case SX1301_DBG_ARB_MCU_RAM_DATA:
 	case SX1301_MPD:
+	case SX1301_AGCSTS:
+
 	case SX1301_RADIO_A_SPI_DATA_RB:
 	case SX1301_RADIO_B_SPI_DATA_RB:
+	case SX1301_DBG_ARB_MCU_RAM_DATA:
+	case SX1301_DBG_AGC_MCU_RAM_DATA:
 		return true;
 	default:
 		return false;
-- 
2.16.4

[PATCH lora-next 10/11] net: lora: sx130x: Mark MCU_CTRL register as volatile

[Andreas Färber]

For reasons as of yet unknown, this is necessary to unbreak ARB firmware
version check.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 8bdd343a273f..76ce072a540a 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -126,6 +126,8 @@ static bool sx130x_volatile_reg(struct device *dev, unsigned int reg)
 	case SX1301_MPD:
 	case SX1301_AGCSTS:
 
+	case SX1301_MCU_CTRL:
+
 	case SX1301_RADIO_A_SPI_DATA_RB:
 	case SX1301_RADIO_B_SPI_DATA_RB:
 	case SX1301_DBG_ARB_MCU_RAM_DATA:
-- 
2.16.4

[PATCH lora-next 11/11] net: lora: sx130x: Enable regmap caching

[Andreas Färber]

To fix performance penalties compared to pre-regmap field code,
enable caching.

Note: This has been tested to not regress on spi-gpio, with no
improvement on spi-sun6i for clk_prepare() locking issue.

Cc: Ben Whitten <ben.whitten@gmail.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 drivers/net/lora/sx130x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 76ce072a540a..2fb18d0c9db5 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -162,7 +162,7 @@ const struct regmap_config sx130x_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 
-	.cache_type = REGCACHE_NONE,
+	.cache_type = REGCACHE_RBTREE,
 	.disable_locking = true,
 
 	.read_flag_mask = 0,
-- 
2.16.4