[PATCH v1 0/5] pinctrl: microchip-sgpio: locking and synchronous output

[PATCH v1 0/5] pinctrl: microchip-sgpio: locking and synchronous output

[Michael Walle]

There are boards which use the output of the SGPIO to drive I2C muxers.
SGPIO right now is broken in a way that when the software sets this bit
there is a rather large delay until that value ends up on the hardware
pin.

While digging into this, I've noticed that there is no locking at all
in this driver. Add locking for all RWM accesses.

Please note, that parts of the modification of the first patch are
removed again in a later patch. This is because the first patch is
intended to be backported to the stable trees.

Michael Walle (5):
  pinctrl: microchip-sgpio: lock RMW access
  pinctrl: microchip-sgpio: don't do RMW for interrupt ack register
  pinctrl: microchip-sgpio: use regmap_update_bits()
  pinctrl: microchip-sgpio: return error in spgio_output_set()
  pinctrl: microchip-sgpio: wait until output is actually set

 drivers/pinctrl/pinctrl-microchip-sgpio.c | 101 ++++++++++++++++++----
 1 file changed, 86 insertions(+), 15 deletions(-)

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 0/5] pinctrl: microchip-sgpio: locking and synchronous output

[Michael Walle]

There are boards which use the output of the SGPIO to drive I2C muxers.
SGPIO right now is broken in a way that when the software sets this bit
there is a rather large delay until that value ends up on the hardware
pin.

While digging into this, I've noticed that there is no locking at all
in this driver. Add locking for all RWM accesses.

Please note, that parts of the modification of the first patch are
removed again in a later patch. This is because the first patch is
intended to be backported to the stable trees.

Michael Walle (5):
  pinctrl: microchip-sgpio: lock RMW access
  pinctrl: microchip-sgpio: don't do RMW for interrupt ack register
  pinctrl: microchip-sgpio: use regmap_update_bits()
  pinctrl: microchip-sgpio: return error in spgio_output_set()
  pinctrl: microchip-sgpio: wait until output is actually set

 drivers/pinctrl/pinctrl-microchip-sgpio.c | 101 ++++++++++++++++++----
 1 file changed, 86 insertions(+), 15 deletions(-)

-- 
2.30.2

[PATCH v1 1/5] pinctrl: microchip-sgpio: lock RMW access

[Michael Walle]

Protect any RMW access to the registers by a spinlock.

Fixes: 7e5ea974e61c ("pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index d371b1e66092..b43302cc188a 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -19,6 +19,7 @@
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
+#include <linux/spinlock.h>
 
 #include "core.h"
 #include "pinconf.h"
@@ -116,6 +117,7 @@ struct sgpio_priv {
 	u32 clock;
 	struct regmap *regs;
 	const struct sgpio_properties *properties;
+	spinlock_t lock;
 };
 
 struct sgpio_port_addr {
@@ -229,6 +231,7 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 			     int value)
 {
 	unsigned int bit = SGPIO_SRC_BITS * addr->bit;
+	unsigned long flags;
 	u32 clr, set;
 
 	switch (priv->properties->arch) {
@@ -247,7 +250,10 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 	default:
 		return;
 	}
+
+	spin_lock_irqsave(&priv->lock, flags);
 	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
+	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 static int sgpio_output_get(struct sgpio_priv *priv,
@@ -575,10 +581,13 @@ static void microchip_sgpio_irq_settype(struct irq_data *data,
 	struct sgpio_bank *bank = gpiochip_get_data(chip);
 	unsigned int gpio = irqd_to_hwirq(data);
 	struct sgpio_port_addr addr;
+	unsigned long flags;
 	u32 ena;
 
 	sgpio_pin_to_addr(bank->priv, gpio, &addr);
 
+	spin_lock_irqsave(&bank->priv->lock, flags);
+
 	/* Disable interrupt while changing type */
 	ena = sgpio_readl(bank->priv, REG_INT_ENABLE, addr.bit);
 	sgpio_writel(bank->priv, ena & ~BIT(addr.port), REG_INT_ENABLE, addr.bit);
@@ -595,6 +604,8 @@ static void microchip_sgpio_irq_settype(struct irq_data *data,
 
 	/* Possibly re-enable interrupts */
 	sgpio_writel(bank->priv, ena, REG_INT_ENABLE, addr.bit);
+
+	spin_unlock_irqrestore(&bank->priv->lock, flags);
 }
 
 static void microchip_sgpio_irq_setreg(struct irq_data *data,
@@ -605,13 +616,16 @@ static void microchip_sgpio_irq_setreg(struct irq_data *data,
 	struct sgpio_bank *bank = gpiochip_get_data(chip);
 	unsigned int gpio = irqd_to_hwirq(data);
 	struct sgpio_port_addr addr;
+	unsigned long flags;
 
 	sgpio_pin_to_addr(bank->priv, gpio, &addr);
 
+	spin_lock_irqsave(&bank->priv->lock, flags);
 	if (clear)
 		sgpio_clrsetbits(bank->priv, reg, addr.bit, BIT(addr.port), 0);
 	else
 		sgpio_clrsetbits(bank->priv, reg, addr.bit, 0, BIT(addr.port));
+	spin_unlock_irqrestore(&bank->priv->lock, flags);
 }
 
 static void microchip_sgpio_irq_mask(struct irq_data *data)
@@ -833,6 +847,7 @@ static int microchip_sgpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->dev = dev;
+	spin_lock_init(&priv->lock);
 
 	reset = devm_reset_control_get_optional_shared(&pdev->dev, "switch");
 	if (IS_ERR(reset))
-- 
2.30.2

[PATCH v1 1/5] pinctrl: microchip-sgpio: lock RMW access

[Michael Walle]

Protect any RMW access to the registers by a spinlock.

Fixes: 7e5ea974e61c ("pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index d371b1e66092..b43302cc188a 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -19,6 +19,7 @@
 #include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
+#include <linux/spinlock.h>
 
 #include "core.h"
 #include "pinconf.h"
@@ -116,6 +117,7 @@ struct sgpio_priv {
 	u32 clock;
 	struct regmap *regs;
 	const struct sgpio_properties *properties;
+	spinlock_t lock;
 };
 
 struct sgpio_port_addr {
@@ -229,6 +231,7 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 			     int value)
 {
 	unsigned int bit = SGPIO_SRC_BITS * addr->bit;
+	unsigned long flags;
 	u32 clr, set;
 
 	switch (priv->properties->arch) {
@@ -247,7 +250,10 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 	default:
 		return;
 	}
+
+	spin_lock_irqsave(&priv->lock, flags);
 	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
+	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 static int sgpio_output_get(struct sgpio_priv *priv,
@@ -575,10 +581,13 @@ static void microchip_sgpio_irq_settype(struct irq_data *data,
 	struct sgpio_bank *bank = gpiochip_get_data(chip);
 	unsigned int gpio = irqd_to_hwirq(data);
 	struct sgpio_port_addr addr;
+	unsigned long flags;
 	u32 ena;
 
 	sgpio_pin_to_addr(bank->priv, gpio, &addr);
 
+	spin_lock_irqsave(&bank->priv->lock, flags);
+
 	/* Disable interrupt while changing type */
 	ena = sgpio_readl(bank->priv, REG_INT_ENABLE, addr.bit);
 	sgpio_writel(bank->priv, ena & ~BIT(addr.port), REG_INT_ENABLE, addr.bit);
@@ -595,6 +604,8 @@ static void microchip_sgpio_irq_settype(struct irq_data *data,
 
 	/* Possibly re-enable interrupts */
 	sgpio_writel(bank->priv, ena, REG_INT_ENABLE, addr.bit);
+
+	spin_unlock_irqrestore(&bank->priv->lock, flags);
 }
 
 static void microchip_sgpio_irq_setreg(struct irq_data *data,
@@ -605,13 +616,16 @@ static void microchip_sgpio_irq_setreg(struct irq_data *data,
 	struct sgpio_bank *bank = gpiochip_get_data(chip);
 	unsigned int gpio = irqd_to_hwirq(data);
 	struct sgpio_port_addr addr;
+	unsigned long flags;
 
 	sgpio_pin_to_addr(bank->priv, gpio, &addr);
 
+	spin_lock_irqsave(&bank->priv->lock, flags);
 	if (clear)
 		sgpio_clrsetbits(bank->priv, reg, addr.bit, BIT(addr.port), 0);
 	else
 		sgpio_clrsetbits(bank->priv, reg, addr.bit, 0, BIT(addr.port));
+	spin_unlock_irqrestore(&bank->priv->lock, flags);
 }
 
 static void microchip_sgpio_irq_mask(struct irq_data *data)
@@ -833,6 +847,7 @@ static int microchip_sgpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->dev = dev;
+	spin_lock_init(&priv->lock);
 
 	reset = devm_reset_control_get_optional_shared(&pdev->dev, "switch");
 	if (IS_ERR(reset))
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/5] pinctrl: microchip-sgpio: don't do RMW for interrupt ack register

[Michael Walle]

The interrupt ack register has the usual "write one to clear" semantics.
No read-modify-write is required here.

This is also a preparation patch to change the sgpio_clrsetbits() to use
regmap_update_bits() which don't write the value if it is not changed.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index b43302cc188a..31c4401f725e 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -640,7 +640,14 @@ static void microchip_sgpio_irq_unmask(struct irq_data *data)
 
 static void microchip_sgpio_irq_ack(struct irq_data *data)
 {
-	microchip_sgpio_irq_setreg(data, REG_INT_ACK, false);
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sgpio_bank *bank = gpiochip_get_data(chip);
+	unsigned int gpio = irqd_to_hwirq(data);
+	struct sgpio_port_addr addr;
+
+	sgpio_pin_to_addr(bank->priv, gpio, &addr);
+
+	sgpio_writel(bank->priv, BIT(addr.port), REG_INT_ACK, addr.bit);
 }
 
 static int microchip_sgpio_irq_set_type(struct irq_data *data, unsigned int type)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/5] pinctrl: microchip-sgpio: don't do RMW for interrupt ack register

[Michael Walle]

The interrupt ack register has the usual "write one to clear" semantics.
No read-modify-write is required here.

This is also a preparation patch to change the sgpio_clrsetbits() to use
regmap_update_bits() which don't write the value if it is not changed.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index b43302cc188a..31c4401f725e 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -640,7 +640,14 @@ static void microchip_sgpio_irq_unmask(struct irq_data *data)
 
 static void microchip_sgpio_irq_ack(struct irq_data *data)
 {
-	microchip_sgpio_irq_setreg(data, REG_INT_ACK, false);
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+	struct sgpio_bank *bank = gpiochip_get_data(chip);
+	unsigned int gpio = irqd_to_hwirq(data);
+	struct sgpio_port_addr addr;
+
+	sgpio_pin_to_addr(bank->priv, gpio, &addr);
+
+	sgpio_writel(bank->priv, BIT(addr.port), REG_INT_ACK, addr.bit);
 }
 
 static int microchip_sgpio_irq_set_type(struct irq_data *data, unsigned int type)
-- 
2.30.2

[PATCH v1 3/5] pinctrl: microchip-sgpio: use regmap_update_bits()

[Michael Walle]

Convert sgpio_clrsetbits() to use regmap_update_bits() and drop the
spinlocks because regmap already takes care of the locking.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index 31c4401f725e..f01ca94943da 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -168,12 +168,11 @@ static void sgpio_writel(struct sgpio_priv *priv,
 static inline void sgpio_clrsetbits(struct sgpio_priv *priv,
 				    u32 rno, u32 off, u32 clear, u32 set)
 {
-	u32 val = sgpio_readl(priv, rno, off);
-
-	val &= ~clear;
-	val |= set;
+	u32 addr = sgpio_get_addr(priv, rno, off);
+	int ret;
 
-	sgpio_writel(priv, val, rno, off);
+	ret = regmap_update_bits(priv->regs, addr, clear | set, set);
+	WARN_ONCE(ret, "error updating sgpio reg %d\n", ret);
 }
 
 static inline void sgpio_configure_bitstream(struct sgpio_priv *priv)
@@ -231,7 +230,6 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 			     int value)
 {
 	unsigned int bit = SGPIO_SRC_BITS * addr->bit;
-	unsigned long flags;
 	u32 clr, set;
 
 	switch (priv->properties->arch) {
@@ -251,9 +249,7 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 		return;
 	}
 
-	spin_lock_irqsave(&priv->lock, flags);
 	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
-	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 static int sgpio_output_get(struct sgpio_priv *priv,
@@ -616,16 +612,13 @@ static void microchip_sgpio_irq_setreg(struct irq_data *data,
 	struct sgpio_bank *bank = gpiochip_get_data(chip);
 	unsigned int gpio = irqd_to_hwirq(data);
 	struct sgpio_port_addr addr;
-	unsigned long flags;
 
 	sgpio_pin_to_addr(bank->priv, gpio, &addr);
 
-	spin_lock_irqsave(&bank->priv->lock, flags);
 	if (clear)
 		sgpio_clrsetbits(bank->priv, reg, addr.bit, BIT(addr.port), 0);
 	else
 		sgpio_clrsetbits(bank->priv, reg, addr.bit, 0, BIT(addr.port));
-	spin_unlock_irqrestore(&bank->priv->lock, flags);
 }
 
 static void microchip_sgpio_irq_mask(struct irq_data *data)
-- 
2.30.2

[PATCH v1 3/5] pinctrl: microchip-sgpio: use regmap_update_bits()

[Michael Walle]

Convert sgpio_clrsetbits() to use regmap_update_bits() and drop the
spinlocks because regmap already takes care of the locking.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index 31c4401f725e..f01ca94943da 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -168,12 +168,11 @@ static void sgpio_writel(struct sgpio_priv *priv,
 static inline void sgpio_clrsetbits(struct sgpio_priv *priv,
 				    u32 rno, u32 off, u32 clear, u32 set)
 {
-	u32 val = sgpio_readl(priv, rno, off);
-
-	val &= ~clear;
-	val |= set;
+	u32 addr = sgpio_get_addr(priv, rno, off);
+	int ret;
 
-	sgpio_writel(priv, val, rno, off);
+	ret = regmap_update_bits(priv->regs, addr, clear | set, set);
+	WARN_ONCE(ret, "error updating sgpio reg %d\n", ret);
 }
 
 static inline void sgpio_configure_bitstream(struct sgpio_priv *priv)
@@ -231,7 +230,6 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 			     int value)
 {
 	unsigned int bit = SGPIO_SRC_BITS * addr->bit;
-	unsigned long flags;
 	u32 clr, set;
 
 	switch (priv->properties->arch) {
@@ -251,9 +249,7 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 		return;
 	}
 
-	spin_lock_irqsave(&priv->lock, flags);
 	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
-	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 static int sgpio_output_get(struct sgpio_priv *priv,
@@ -616,16 +612,13 @@ static void microchip_sgpio_irq_setreg(struct irq_data *data,
 	struct sgpio_bank *bank = gpiochip_get_data(chip);
 	unsigned int gpio = irqd_to_hwirq(data);
 	struct sgpio_port_addr addr;
-	unsigned long flags;
 
 	sgpio_pin_to_addr(bank->priv, gpio, &addr);
 
-	spin_lock_irqsave(&bank->priv->lock, flags);
 	if (clear)
 		sgpio_clrsetbits(bank->priv, reg, addr.bit, BIT(addr.port), 0);
 	else
 		sgpio_clrsetbits(bank->priv, reg, addr.bit, 0, BIT(addr.port));
-	spin_unlock_irqrestore(&bank->priv->lock, flags);
 }
 
 static void microchip_sgpio_irq_mask(struct irq_data *data)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 4/5] pinctrl: microchip-sgpio: return error in spgio_output_set()

[Michael Walle]

Make sgpio_output_set() return an error value. Don't just ignore the
return value of any regmap access but propagate it to our callers. Even
if the accesses never fail, this is a preparation patch to add single
shot mode where we need to poll a bit and thus we might get -ETIMEDOUT.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index f01ca94943da..3f3b8c482f3a 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -225,9 +225,9 @@ static inline void sgpio_configure_clock(struct sgpio_priv *priv, u32 clkfrq)
 	sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0, clr, set);
 }
 
-static void sgpio_output_set(struct sgpio_priv *priv,
-			     struct sgpio_port_addr *addr,
-			     int value)
+static int sgpio_output_set(struct sgpio_priv *priv,
+			    struct sgpio_port_addr *addr,
+			    int value)
 {
 	unsigned int bit = SGPIO_SRC_BITS * addr->bit;
 	u32 clr, set;
@@ -246,10 +246,12 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 		set = FIELD_PREP(SGPIO_SPARX5_BIT_SOURCE, value << bit);
 		break;
 	default:
-		return;
+		return -EINVAL;
 	}
 
 	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
+
+	return 0;
 }
 
 static int sgpio_output_get(struct sgpio_priv *priv,
@@ -335,7 +337,7 @@ static int sgpio_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 		case PIN_CONFIG_OUTPUT:
 			if (bank->is_input)
 				return -EINVAL;
-			sgpio_output_set(priv, &addr, arg);
+			err = sgpio_output_set(priv, &addr, arg);
 			break;
 
 		default:
@@ -475,9 +477,7 @@ static int microchip_sgpio_direction_output(struct gpio_chip *gc,
 
 	sgpio_pin_to_addr(priv, gpio, &addr);
 
-	sgpio_output_set(priv, &addr, value);
-
-	return 0;
+	return sgpio_output_set(priv, &addr, value);
 }
 
 static int microchip_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
-- 
2.30.2

[PATCH v1 4/5] pinctrl: microchip-sgpio: return error in spgio_output_set()

[Michael Walle]

Make sgpio_output_set() return an error value. Don't just ignore the
return value of any regmap access but propagate it to our callers. Even
if the accesses never fail, this is a preparation patch to add single
shot mode where we need to poll a bit and thus we might get -ETIMEDOUT.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index f01ca94943da..3f3b8c482f3a 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -225,9 +225,9 @@ static inline void sgpio_configure_clock(struct sgpio_priv *priv, u32 clkfrq)
 	sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0, clr, set);
 }
 
-static void sgpio_output_set(struct sgpio_priv *priv,
-			     struct sgpio_port_addr *addr,
-			     int value)
+static int sgpio_output_set(struct sgpio_priv *priv,
+			    struct sgpio_port_addr *addr,
+			    int value)
 {
 	unsigned int bit = SGPIO_SRC_BITS * addr->bit;
 	u32 clr, set;
@@ -246,10 +246,12 @@ static void sgpio_output_set(struct sgpio_priv *priv,
 		set = FIELD_PREP(SGPIO_SPARX5_BIT_SOURCE, value << bit);
 		break;
 	default:
-		return;
+		return -EINVAL;
 	}
 
 	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
+
+	return 0;
 }
 
 static int sgpio_output_get(struct sgpio_priv *priv,
@@ -335,7 +337,7 @@ static int sgpio_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 		case PIN_CONFIG_OUTPUT:
 			if (bank->is_input)
 				return -EINVAL;
-			sgpio_output_set(priv, &addr, arg);
+			err = sgpio_output_set(priv, &addr, arg);
 			break;
 
 		default:
@@ -475,9 +477,7 @@ static int microchip_sgpio_direction_output(struct gpio_chip *gc,
 
 	sgpio_pin_to_addr(priv, gpio, &addr);
 
-	sgpio_output_set(priv, &addr, value);
-
-	return 0;
+	return sgpio_output_set(priv, &addr, value);
 }
 
 static int microchip_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Michael Walle]

Right now, when a gpio value is set, the actual hardware pin gets set
asynchronously. When linux write the output register, it takes some time
until it is actually propagated to the output shift registers. If that
output port is connected to an I2C mux for example, the linux driver
assumes the I2C bus is already switched although it is not.

Fortunately, there is a single shot mode with a feedback: you can
trigger the single shot and the hardware will clear that bit once it has
finished the clocking and strobed the load signal of the shift
registers. This can take a considerable amount of time though.
Measuremens have shown that it takes up to a whole burst cycle gap which
is about 50ms on the largest setting. Therefore, we have to mark the
output bank as sleepable. To avoid unnecessary waiting, just trigger the
single shot if the value was actually changed.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 58 ++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index 3f3b8c482f3a..768b69929c99 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -69,6 +69,7 @@ struct sgpio_properties {
 #define SGPIO_OCELOT_BIT_SOURCE  GENMASK(23, 12)
 
 #define SGPIO_SPARX5_AUTO_REPEAT BIT(6)
+#define SGPIO_SPARX5_SINGLE_SHOT BIT(7)
 #define SGPIO_SPARX5_PORT_WIDTH  GENMASK(4, 3)
 #define SGPIO_SPARX5_CLK_FREQ    GENMASK(19, 8)
 #define SGPIO_SPARX5_BIT_SOURCE  GENMASK(23, 12)
@@ -118,6 +119,8 @@ struct sgpio_priv {
 	struct regmap *regs;
 	const struct sgpio_properties *properties;
 	spinlock_t lock;
+	/* protects the config register and single shot mode */
+	struct mutex poll_lock;
 };
 
 struct sgpio_port_addr {
@@ -225,12 +228,54 @@ static inline void sgpio_configure_clock(struct sgpio_priv *priv, u32 clkfrq)
 	sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0, clr, set);
 }
 
+static int sgpio_single_shot(struct sgpio_priv *priv)
+{
+	u32 addr = sgpio_get_addr(priv, REG_SIO_CONFIG, 0);
+	int ret, ret2;
+	u32 ctrl;
+
+	/* Only supported on SparX-5 for now. */
+	if (priv->properties->arch != SGPIO_ARCH_SPARX5)
+		return 0;
+
+	/*
+	 * Trigger immediate burst. This only works when auto repeat is turned
+	 * off. Otherwise, the single shot bit will never be cleared by the
+	 * hardware. Measurements showed that an update might take as long as
+	 * the burst gap. On a LAN9668 this is about 50ms for the largest
+	 * setting.
+	 * After the manual burst, reenable the auto repeat mode again.
+	 */
+	mutex_lock(&priv->poll_lock);
+	ret = regmap_update_bits(priv->regs, addr,
+				 SGPIO_SPARX5_SINGLE_SHOT | SGPIO_SPARX5_AUTO_REPEAT,
+				 SGPIO_SPARX5_SINGLE_SHOT);
+	if (ret)
+		goto out;
+
+	ret = regmap_read_poll_timeout(priv->regs, addr, ctrl,
+				       !(ctrl & SGPIO_SPARX5_SINGLE_SHOT),
+				       100, 60000);
+
+	/* reenable auto repeat mode even if there was an error */
+	ret2 = regmap_update_bits(priv->regs, addr,
+				  SGPIO_SPARX5_AUTO_REPEAT,
+				  SGPIO_SPARX5_AUTO_REPEAT);
+out:
+	mutex_unlock(&priv->poll_lock);
+
+	return ret ?: ret2;
+}
+
 static int sgpio_output_set(struct sgpio_priv *priv,
 			    struct sgpio_port_addr *addr,
 			    int value)
 {
 	unsigned int bit = SGPIO_SRC_BITS * addr->bit;
+	u32 reg = sgpio_get_addr(priv, REG_PORT_CONFIG, addr->port);
+	bool changed;
 	u32 clr, set;
+	int ret;
 
 	switch (priv->properties->arch) {
 	case SGPIO_ARCH_LUTON:
@@ -249,7 +294,16 @@ static int sgpio_output_set(struct sgpio_priv *priv,
 		return -EINVAL;
 	}
 
-	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
+	ret = regmap_update_bits_check(priv->regs, reg, clr | set, set,
+				       &changed);
+	if (ret)
+		return ret;
+
+	if (changed) {
+		ret = sgpio_single_shot(priv);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -788,6 +842,7 @@ static int microchip_sgpio_register_bank(struct device *dev,
 	gc->of_gpio_n_cells     = 3;
 	gc->base		= -1;
 	gc->ngpio		= ngpios;
+	gc->can_sleep		= !bank->is_input;
 
 	if (bank->is_input && priv->properties->flags & SGPIO_FLAGS_HAS_IRQ) {
 		int irq = fwnode_irq_get(fwnode, 0);
@@ -848,6 +903,7 @@ static int microchip_sgpio_probe(struct platform_device *pdev)
 
 	priv->dev = dev;
 	spin_lock_init(&priv->lock);
+	mutex_init(&priv->poll_lock);
 
 	reset = devm_reset_control_get_optional_shared(&pdev->dev, "switch");
 	if (IS_ERR(reset))
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Michael Walle]

Right now, when a gpio value is set, the actual hardware pin gets set
asynchronously. When linux write the output register, it takes some time
until it is actually propagated to the output shift registers. If that
output port is connected to an I2C mux for example, the linux driver
assumes the I2C bus is already switched although it is not.

Fortunately, there is a single shot mode with a feedback: you can
trigger the single shot and the hardware will clear that bit once it has
finished the clocking and strobed the load signal of the shift
registers. This can take a considerable amount of time though.
Measuremens have shown that it takes up to a whole burst cycle gap which
is about 50ms on the largest setting. Therefore, we have to mark the
output bank as sleepable. To avoid unnecessary waiting, just trigger the
single shot if the value was actually changed.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pinctrl/pinctrl-microchip-sgpio.c | 58 ++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
index 3f3b8c482f3a..768b69929c99 100644
--- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
+++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
@@ -69,6 +69,7 @@ struct sgpio_properties {
 #define SGPIO_OCELOT_BIT_SOURCE  GENMASK(23, 12)
 
 #define SGPIO_SPARX5_AUTO_REPEAT BIT(6)
+#define SGPIO_SPARX5_SINGLE_SHOT BIT(7)
 #define SGPIO_SPARX5_PORT_WIDTH  GENMASK(4, 3)
 #define SGPIO_SPARX5_CLK_FREQ    GENMASK(19, 8)
 #define SGPIO_SPARX5_BIT_SOURCE  GENMASK(23, 12)
@@ -118,6 +119,8 @@ struct sgpio_priv {
 	struct regmap *regs;
 	const struct sgpio_properties *properties;
 	spinlock_t lock;
+	/* protects the config register and single shot mode */
+	struct mutex poll_lock;
 };
 
 struct sgpio_port_addr {
@@ -225,12 +228,54 @@ static inline void sgpio_configure_clock(struct sgpio_priv *priv, u32 clkfrq)
 	sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0, clr, set);
 }
 
+static int sgpio_single_shot(struct sgpio_priv *priv)
+{
+	u32 addr = sgpio_get_addr(priv, REG_SIO_CONFIG, 0);
+	int ret, ret2;
+	u32 ctrl;
+
+	/* Only supported on SparX-5 for now. */
+	if (priv->properties->arch != SGPIO_ARCH_SPARX5)
+		return 0;
+
+	/*
+	 * Trigger immediate burst. This only works when auto repeat is turned
+	 * off. Otherwise, the single shot bit will never be cleared by the
+	 * hardware. Measurements showed that an update might take as long as
+	 * the burst gap. On a LAN9668 this is about 50ms for the largest
+	 * setting.
+	 * After the manual burst, reenable the auto repeat mode again.
+	 */
+	mutex_lock(&priv->poll_lock);
+	ret = regmap_update_bits(priv->regs, addr,
+				 SGPIO_SPARX5_SINGLE_SHOT | SGPIO_SPARX5_AUTO_REPEAT,
+				 SGPIO_SPARX5_SINGLE_SHOT);
+	if (ret)
+		goto out;
+
+	ret = regmap_read_poll_timeout(priv->regs, addr, ctrl,
+				       !(ctrl & SGPIO_SPARX5_SINGLE_SHOT),
+				       100, 60000);
+
+	/* reenable auto repeat mode even if there was an error */
+	ret2 = regmap_update_bits(priv->regs, addr,
+				  SGPIO_SPARX5_AUTO_REPEAT,
+				  SGPIO_SPARX5_AUTO_REPEAT);
+out:
+	mutex_unlock(&priv->poll_lock);
+
+	return ret ?: ret2;
+}
+
 static int sgpio_output_set(struct sgpio_priv *priv,
 			    struct sgpio_port_addr *addr,
 			    int value)
 {
 	unsigned int bit = SGPIO_SRC_BITS * addr->bit;
+	u32 reg = sgpio_get_addr(priv, REG_PORT_CONFIG, addr->port);
+	bool changed;
 	u32 clr, set;
+	int ret;
 
 	switch (priv->properties->arch) {
 	case SGPIO_ARCH_LUTON:
@@ -249,7 +294,16 @@ static int sgpio_output_set(struct sgpio_priv *priv,
 		return -EINVAL;
 	}
 
-	sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
+	ret = regmap_update_bits_check(priv->regs, reg, clr | set, set,
+				       &changed);
+	if (ret)
+		return ret;
+
+	if (changed) {
+		ret = sgpio_single_shot(priv);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -788,6 +842,7 @@ static int microchip_sgpio_register_bank(struct device *dev,
 	gc->of_gpio_n_cells     = 3;
 	gc->base		= -1;
 	gc->ngpio		= ngpios;
+	gc->can_sleep		= !bank->is_input;
 
 	if (bank->is_input && priv->properties->flags & SGPIO_FLAGS_HAS_IRQ) {
 		int irq = fwnode_irq_get(fwnode, 0);
@@ -848,6 +903,7 @@ static int microchip_sgpio_probe(struct platform_device *pdev)
 
 	priv->dev = dev;
 	spin_lock_init(&priv->lock);
+	mutex_init(&priv->poll_lock);
 
 	reset = devm_reset_control_get_optional_shared(&pdev->dev, "switch");
 	if (IS_ERR(reset))
-- 
2.30.2

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Horatiu Vultur]

The 02/24/2022 17:10, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi Michael,

> 
> Right now, when a gpio value is set, the actual hardware pin gets set
> asynchronously. When linux write the output register, it takes some time
> until it is actually propagated to the output shift registers. If that
> output port is connected to an I2C mux for example, the linux driver
> assumes the I2C bus is already switched although it is not.
> 
> Fortunately, there is a single shot mode with a feedback: you can
> trigger the single shot and the hardware will clear that bit once it has
> finished the clocking and strobed the load signal of the shift
> registers. This can take a considerable amount of time though.
> Measuremens have shown that it takes up to a whole burst cycle gap which
> is about 50ms on the largest setting. Therefore, we have to mark the
> output bank as sleepable. To avoid unnecessary waiting, just trigger the
> single shot if the value was actually changed.

I have tested this patch series on our lan9668 board and it worked
fine. Thanks!

I just have few questions:
1. What about other boards/chips that have this sgpio, do they have also
   the same issue? Because from what I recall on sparx5 they don't have
   this issue. I have seen it only on lan9668.
2. I remember that I have tried to fix this issue like this [1], but
   unfortunetly that was never accepted. Is this something that is worth
   at looking at?

[1] https://patchwork.ozlabs.org/project/linux-i2c/patch/20211103091839.1665672-3-horatiu.vultur@microchip.com/

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/pinctrl/pinctrl-microchip-sgpio.c | 58 ++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
> index 3f3b8c482f3a..768b69929c99 100644
> --- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
> +++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
> @@ -69,6 +69,7 @@ struct sgpio_properties {
>  #define SGPIO_OCELOT_BIT_SOURCE  GENMASK(23, 12)
> 
>  #define SGPIO_SPARX5_AUTO_REPEAT BIT(6)
> +#define SGPIO_SPARX5_SINGLE_SHOT BIT(7)
>  #define SGPIO_SPARX5_PORT_WIDTH  GENMASK(4, 3)
>  #define SGPIO_SPARX5_CLK_FREQ    GENMASK(19, 8)
>  #define SGPIO_SPARX5_BIT_SOURCE  GENMASK(23, 12)
> @@ -118,6 +119,8 @@ struct sgpio_priv {
>         struct regmap *regs;
>         const struct sgpio_properties *properties;
>         spinlock_t lock;
> +       /* protects the config register and single shot mode */
> +       struct mutex poll_lock;
>  };
> 
>  struct sgpio_port_addr {
> @@ -225,12 +228,54 @@ static inline void sgpio_configure_clock(struct sgpio_priv *priv, u32 clkfrq)
>         sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0, clr, set);
>  }
> 
> +static int sgpio_single_shot(struct sgpio_priv *priv)
> +{
> +       u32 addr = sgpio_get_addr(priv, REG_SIO_CONFIG, 0);
> +       int ret, ret2;
> +       u32 ctrl;
> +
> +       /* Only supported on SparX-5 for now. */
> +       if (priv->properties->arch != SGPIO_ARCH_SPARX5)
> +               return 0;
> +
> +       /*
> +        * Trigger immediate burst. This only works when auto repeat is turned
> +        * off. Otherwise, the single shot bit will never be cleared by the
> +        * hardware. Measurements showed that an update might take as long as
> +        * the burst gap. On a LAN9668 this is about 50ms for the largest
> +        * setting.
> +        * After the manual burst, reenable the auto repeat mode again.
> +        */
> +       mutex_lock(&priv->poll_lock);
> +       ret = regmap_update_bits(priv->regs, addr,
> +                                SGPIO_SPARX5_SINGLE_SHOT | SGPIO_SPARX5_AUTO_REPEAT,
> +                                SGPIO_SPARX5_SINGLE_SHOT);
> +       if (ret)
> +               goto out;
> +
> +       ret = regmap_read_poll_timeout(priv->regs, addr, ctrl,
> +                                      !(ctrl & SGPIO_SPARX5_SINGLE_SHOT),
> +                                      100, 60000);
> +
> +       /* reenable auto repeat mode even if there was an error */
> +       ret2 = regmap_update_bits(priv->regs, addr,
> +                                 SGPIO_SPARX5_AUTO_REPEAT,
> +                                 SGPIO_SPARX5_AUTO_REPEAT);
> +out:
> +       mutex_unlock(&priv->poll_lock);
> +
> +       return ret ?: ret2;
> +}
> +
>  static int sgpio_output_set(struct sgpio_priv *priv,
>                             struct sgpio_port_addr *addr,
>                             int value)
>  {
>         unsigned int bit = SGPIO_SRC_BITS * addr->bit;
> +       u32 reg = sgpio_get_addr(priv, REG_PORT_CONFIG, addr->port);
> +       bool changed;
>         u32 clr, set;
> +       int ret;
> 
>         switch (priv->properties->arch) {
>         case SGPIO_ARCH_LUTON:
> @@ -249,7 +294,16 @@ static int sgpio_output_set(struct sgpio_priv *priv,
>                 return -EINVAL;
>         }
> 
> -       sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
> +       ret = regmap_update_bits_check(priv->regs, reg, clr | set, set,
> +                                      &changed);
> +       if (ret)
> +               return ret;
> +
> +       if (changed) {
> +               ret = sgpio_single_shot(priv);
> +               if (ret)
> +                       return ret;
> +       }
> 
>         return 0;
>  }
> @@ -788,6 +842,7 @@ static int microchip_sgpio_register_bank(struct device *dev,
>         gc->of_gpio_n_cells     = 3;
>         gc->base                = -1;
>         gc->ngpio               = ngpios;
> +       gc->can_sleep           = !bank->is_input;
> 
>         if (bank->is_input && priv->properties->flags & SGPIO_FLAGS_HAS_IRQ) {
>                 int irq = fwnode_irq_get(fwnode, 0);
> @@ -848,6 +903,7 @@ static int microchip_sgpio_probe(struct platform_device *pdev)
> 
>         priv->dev = dev;
>         spin_lock_init(&priv->lock);
> +       mutex_init(&priv->poll_lock);
> 
>         reset = devm_reset_control_get_optional_shared(&pdev->dev, "switch");
>         if (IS_ERR(reset))
> --
> 2.30.2
> 

-- 
/Horatiu

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Horatiu Vultur]

The 02/24/2022 17:10, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi Michael,

> 
> Right now, when a gpio value is set, the actual hardware pin gets set
> asynchronously. When linux write the output register, it takes some time
> until it is actually propagated to the output shift registers. If that
> output port is connected to an I2C mux for example, the linux driver
> assumes the I2C bus is already switched although it is not.
> 
> Fortunately, there is a single shot mode with a feedback: you can
> trigger the single shot and the hardware will clear that bit once it has
> finished the clocking and strobed the load signal of the shift
> registers. This can take a considerable amount of time though.
> Measuremens have shown that it takes up to a whole burst cycle gap which
> is about 50ms on the largest setting. Therefore, we have to mark the
> output bank as sleepable. To avoid unnecessary waiting, just trigger the
> single shot if the value was actually changed.

I have tested this patch series on our lan9668 board and it worked
fine. Thanks!

I just have few questions:
1. What about other boards/chips that have this sgpio, do they have also
   the same issue? Because from what I recall on sparx5 they don't have
   this issue. I have seen it only on lan9668.
2. I remember that I have tried to fix this issue like this [1], but
   unfortunetly that was never accepted. Is this something that is worth
   at looking at?

[1] https://patchwork.ozlabs.org/project/linux-i2c/patch/20211103091839.1665672-3-horatiu.vultur@microchip.com/

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/pinctrl/pinctrl-microchip-sgpio.c | 58 ++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-microchip-sgpio.c b/drivers/pinctrl/pinctrl-microchip-sgpio.c
> index 3f3b8c482f3a..768b69929c99 100644
> --- a/drivers/pinctrl/pinctrl-microchip-sgpio.c
> +++ b/drivers/pinctrl/pinctrl-microchip-sgpio.c
> @@ -69,6 +69,7 @@ struct sgpio_properties {
>  #define SGPIO_OCELOT_BIT_SOURCE  GENMASK(23, 12)
> 
>  #define SGPIO_SPARX5_AUTO_REPEAT BIT(6)
> +#define SGPIO_SPARX5_SINGLE_SHOT BIT(7)
>  #define SGPIO_SPARX5_PORT_WIDTH  GENMASK(4, 3)
>  #define SGPIO_SPARX5_CLK_FREQ    GENMASK(19, 8)
>  #define SGPIO_SPARX5_BIT_SOURCE  GENMASK(23, 12)
> @@ -118,6 +119,8 @@ struct sgpio_priv {
>         struct regmap *regs;
>         const struct sgpio_properties *properties;
>         spinlock_t lock;
> +       /* protects the config register and single shot mode */
> +       struct mutex poll_lock;
>  };
> 
>  struct sgpio_port_addr {
> @@ -225,12 +228,54 @@ static inline void sgpio_configure_clock(struct sgpio_priv *priv, u32 clkfrq)
>         sgpio_clrsetbits(priv, REG_SIO_CLOCK, 0, clr, set);
>  }
> 
> +static int sgpio_single_shot(struct sgpio_priv *priv)
> +{
> +       u32 addr = sgpio_get_addr(priv, REG_SIO_CONFIG, 0);
> +       int ret, ret2;
> +       u32 ctrl;
> +
> +       /* Only supported on SparX-5 for now. */
> +       if (priv->properties->arch != SGPIO_ARCH_SPARX5)
> +               return 0;
> +
> +       /*
> +        * Trigger immediate burst. This only works when auto repeat is turned
> +        * off. Otherwise, the single shot bit will never be cleared by the
> +        * hardware. Measurements showed that an update might take as long as
> +        * the burst gap. On a LAN9668 this is about 50ms for the largest
> +        * setting.
> +        * After the manual burst, reenable the auto repeat mode again.
> +        */
> +       mutex_lock(&priv->poll_lock);
> +       ret = regmap_update_bits(priv->regs, addr,
> +                                SGPIO_SPARX5_SINGLE_SHOT | SGPIO_SPARX5_AUTO_REPEAT,
> +                                SGPIO_SPARX5_SINGLE_SHOT);
> +       if (ret)
> +               goto out;
> +
> +       ret = regmap_read_poll_timeout(priv->regs, addr, ctrl,
> +                                      !(ctrl & SGPIO_SPARX5_SINGLE_SHOT),
> +                                      100, 60000);
> +
> +       /* reenable auto repeat mode even if there was an error */
> +       ret2 = regmap_update_bits(priv->regs, addr,
> +                                 SGPIO_SPARX5_AUTO_REPEAT,
> +                                 SGPIO_SPARX5_AUTO_REPEAT);
> +out:
> +       mutex_unlock(&priv->poll_lock);
> +
> +       return ret ?: ret2;
> +}
> +
>  static int sgpio_output_set(struct sgpio_priv *priv,
>                             struct sgpio_port_addr *addr,
>                             int value)
>  {
>         unsigned int bit = SGPIO_SRC_BITS * addr->bit;
> +       u32 reg = sgpio_get_addr(priv, REG_PORT_CONFIG, addr->port);
> +       bool changed;
>         u32 clr, set;
> +       int ret;
> 
>         switch (priv->properties->arch) {
>         case SGPIO_ARCH_LUTON:
> @@ -249,7 +294,16 @@ static int sgpio_output_set(struct sgpio_priv *priv,
>                 return -EINVAL;
>         }
> 
> -       sgpio_clrsetbits(priv, REG_PORT_CONFIG, addr->port, clr, set);
> +       ret = regmap_update_bits_check(priv->regs, reg, clr | set, set,
> +                                      &changed);
> +       if (ret)
> +               return ret;
> +
> +       if (changed) {
> +               ret = sgpio_single_shot(priv);
> +               if (ret)
> +                       return ret;
> +       }
> 
>         return 0;
>  }
> @@ -788,6 +842,7 @@ static int microchip_sgpio_register_bank(struct device *dev,
>         gc->of_gpio_n_cells     = 3;
>         gc->base                = -1;
>         gc->ngpio               = ngpios;
> +       gc->can_sleep           = !bank->is_input;
> 
>         if (bank->is_input && priv->properties->flags & SGPIO_FLAGS_HAS_IRQ) {
>                 int irq = fwnode_irq_get(fwnode, 0);
> @@ -848,6 +903,7 @@ static int microchip_sgpio_probe(struct platform_device *pdev)
> 
>         priv->dev = dev;
>         spin_lock_init(&priv->lock);
> +       mutex_init(&priv->poll_lock);
> 
>         reset = devm_reset_control_get_optional_shared(&pdev->dev, "switch");
>         if (IS_ERR(reset))
> --
> 2.30.2
> 

-- 
/Horatiu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Michael Walle]

Hi Horatiu,

Am 2022-02-25 10:24, schrieb Horatiu Vultur:
> The 02/24/2022 17:10, Michael Walle wrote:
>> Right now, when a gpio value is set, the actual hardware pin gets set
>> asynchronously. When linux write the output register, it takes some 
>> time
>> until it is actually propagated to the output shift registers. If that
>> output port is connected to an I2C mux for example, the linux driver
>> assumes the I2C bus is already switched although it is not.
>> 
>> Fortunately, there is a single shot mode with a feedback: you can
>> trigger the single shot and the hardware will clear that bit once it 
>> has
>> finished the clocking and strobed the load signal of the shift
>> registers. This can take a considerable amount of time though.
>> Measuremens have shown that it takes up to a whole burst cycle gap 
>> which
>> is about 50ms on the largest setting. Therefore, we have to mark the
>> output bank as sleepable. To avoid unnecessary waiting, just trigger 
>> the
>> single shot if the value was actually changed.
> 
> I have tested this patch series on our lan9668 board and it worked
> fine. Thanks!

Thanks for testing!

> I just have few questions:
> 1. What about other boards/chips that have this sgpio, do they have 
> also
>    the same issue? Because from what I recall on sparx5 they don't have
>    this issue. I have seen it only on lan9668.

Unfortunatly, I don't have any knowledge what IP core is used in
which SoC. I assumed the lan9668 used the same as the sparx5. If
that is not the case, we need a new compatible. Do you know if it
the same?

On the sparx5 are there any peripheral who you would actually
notice that the timing is off?

That being said, I'd assume all the serial gpio controller has
this "flaw". Simply because a register write won't block until the
value is shifted out to the shift register and actualy loaded by
strobing the load signal. It just depends on your burst setting
(even with bursts off, and clocking all the time) on how large
the delay is. So you might or might not notice it on a board.

Could you also have a look at the other supported sgpio block,
the ocelot and the luton? I don't have any register description
of these.

> 2. I remember that I have tried to fix this issue like this [1], but
>    unfortunetly that was never accepted. Is this something that is 
> worth
>    at looking at?

That fix is at the wrong place. You'd need to fix every gpio user, no?
Instead this tries to fix the controller.

> 
> [1] 
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20211103091839.1665672-3-horatiu.vultur@microchip.com/

-michael

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Michael Walle]

Hi Horatiu,

Am 2022-02-25 10:24, schrieb Horatiu Vultur:
> The 02/24/2022 17:10, Michael Walle wrote:
>> Right now, when a gpio value is set, the actual hardware pin gets set
>> asynchronously. When linux write the output register, it takes some 
>> time
>> until it is actually propagated to the output shift registers. If that
>> output port is connected to an I2C mux for example, the linux driver
>> assumes the I2C bus is already switched although it is not.
>> 
>> Fortunately, there is a single shot mode with a feedback: you can
>> trigger the single shot and the hardware will clear that bit once it 
>> has
>> finished the clocking and strobed the load signal of the shift
>> registers. This can take a considerable amount of time though.
>> Measuremens have shown that it takes up to a whole burst cycle gap 
>> which
>> is about 50ms on the largest setting. Therefore, we have to mark the
>> output bank as sleepable. To avoid unnecessary waiting, just trigger 
>> the
>> single shot if the value was actually changed.
> 
> I have tested this patch series on our lan9668 board and it worked
> fine. Thanks!

Thanks for testing!

> I just have few questions:
> 1. What about other boards/chips that have this sgpio, do they have 
> also
>    the same issue? Because from what I recall on sparx5 they don't have
>    this issue. I have seen it only on lan9668.

Unfortunatly, I don't have any knowledge what IP core is used in
which SoC. I assumed the lan9668 used the same as the sparx5. If
that is not the case, we need a new compatible. Do you know if it
the same?

On the sparx5 are there any peripheral who you would actually
notice that the timing is off?

That being said, I'd assume all the serial gpio controller has
this "flaw". Simply because a register write won't block until the
value is shifted out to the shift register and actualy loaded by
strobing the load signal. It just depends on your burst setting
(even with bursts off, and clocking all the time) on how large
the delay is. So you might or might not notice it on a board.

Could you also have a look at the other supported sgpio block,
the ocelot and the luton? I don't have any register description
of these.

> 2. I remember that I have tried to fix this issue like this [1], but
>    unfortunetly that was never accepted. Is this something that is 
> worth
>    at looking at?

That fix is at the wrong place. You'd need to fix every gpio user, no?
Instead this tries to fix the controller.

> 
> [1] 
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20211103091839.1665672-3-horatiu.vultur@microchip.com/

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Colin Foster]

Hi Michael,

On Fri, Feb 25, 2022 at 12:29:50PM +0100, Michael Walle wrote:
> 
> Could you also have a look at the other supported sgpio block,
> the ocelot and the luton? I don't have any register description
> of these.

The current supported Ocelot chips are the VSC7514 (link below) and the
VSC7513. Chapter 6 of this PDF links a second PDF, and you should be
able to find the Serial GPIO definitions in DEVCPU_GCB:SIO_CTRL on page
79 of that PDF.
https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf

I'm working on support for the VSC7511/7512, so I can run a "does it
work" test, but I likely won't have a setup to test the corner
conditions this patch set is addressing with any confidence.


Colin Foster

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Colin Foster]

Hi Michael,

On Fri, Feb 25, 2022 at 12:29:50PM +0100, Michael Walle wrote:
> 
> Could you also have a look at the other supported sgpio block,
> the ocelot and the luton? I don't have any register description
> of these.

The current supported Ocelot chips are the VSC7514 (link below) and the
VSC7513. Chapter 6 of this PDF links a second PDF, and you should be
able to find the Serial GPIO definitions in DEVCPU_GCB:SIO_CTRL on page
79 of that PDF.
https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf

I'm working on support for the VSC7511/7512, so I can run a "does it
work" test, but I likely won't have a setup to test the corner
conditions this patch set is addressing with any confidence.


Colin Foster


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Michael Walle]

Hi Colin,

Am 2022-02-25 17:54, schrieb Colin Foster:
> On Fri, Feb 25, 2022 at 12:29:50PM +0100, Michael Walle wrote:
>> 
>> Could you also have a look at the other supported sgpio block,
>> the ocelot and the luton? I don't have any register description
>> of these.
> 
> The current supported Ocelot chips are the VSC7514 (link below) and the
> VSC7513. Chapter 6 of this PDF links a second PDF, and you should be
> able to find the Serial GPIO definitions in DEVCPU_GCB:SIO_CTRL on page
> 79 of that PDF.
> https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf

Thanks! I've just send a new version with support for these.

> I'm working on support for the VSC7511/7512, so I can run a "does it
> work" test, but I likely won't have a setup to test the corner
> conditions this patch set is addressing with any confidence.

You can time the register polling using ktime_get(), ktime_sub()
and ktime_to_ns(). It should be in the magnitude of the burst gap.
Which will give you at least some confidence. I did the testing
with an oscilloscope and toggling gpios (but also did measure
the timing with ktime_get()).

-michael

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Michael Walle]

Hi Colin,

Am 2022-02-25 17:54, schrieb Colin Foster:
> On Fri, Feb 25, 2022 at 12:29:50PM +0100, Michael Walle wrote:
>> 
>> Could you also have a look at the other supported sgpio block,
>> the ocelot and the luton? I don't have any register description
>> of these.
> 
> The current supported Ocelot chips are the VSC7514 (link below) and the
> VSC7513. Chapter 6 of this PDF links a second PDF, and you should be
> able to find the Serial GPIO definitions in DEVCPU_GCB:SIO_CTRL on page
> 79 of that PDF.
> https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf

Thanks! I've just send a new version with support for these.

> I'm working on support for the VSC7511/7512, so I can run a "does it
> work" test, but I likely won't have a setup to test the corner
> conditions this patch set is addressing with any confidence.

You can time the register polling using ktime_get(), ktime_sub()
and ktime_to_ns(). It should be in the magnitude of the burst gap.
Which will give you at least some confidence. I did the testing
with an oscilloscope and toggling gpios (but also did measure
the timing with ktime_get()).

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Horatiu Vultur]

The 02/25/2022 12:29, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi Michael,

Sorry for late reply.

> 
> Hi Horatiu,
> 
> Am 2022-02-25 10:24, schrieb Horatiu Vultur:
> > The 02/24/2022 17:10, Michael Walle wrote:
> > > Right now, when a gpio value is set, the actual hardware pin gets set
> > > asynchronously. When linux write the output register, it takes some
> > > time
> > > until it is actually propagated to the output shift registers. If that
> > > output port is connected to an I2C mux for example, the linux driver
> > > assumes the I2C bus is already switched although it is not.
> > > 
> > > Fortunately, there is a single shot mode with a feedback: you can
> > > trigger the single shot and the hardware will clear that bit once it
> > > has
> > > finished the clocking and strobed the load signal of the shift
> > > registers. This can take a considerable amount of time though.
> > > Measuremens have shown that it takes up to a whole burst cycle gap
> > > which
> > > is about 50ms on the largest setting. Therefore, we have to mark the
> > > output bank as sleepable. To avoid unnecessary waiting, just trigger
> > > the
> > > single shot if the value was actually changed.
> > 
> > I have tested this patch series on our lan9668 board and it worked
> > fine. Thanks!
> 
> Thanks for testing!
> 
> > I just have few questions:
> > 1. What about other boards/chips that have this sgpio, do they have
> > also
> >    the same issue? Because from what I recall on sparx5 they don't have
> >    this issue. I have seen it only on lan9668.
> 
> Unfortunatly, I don't have any knowledge what IP core is used in
> which SoC. I assumed the lan9668 used the same as the sparx5. If
> that is not the case, we need a new compatible. Do you know if it
> the same?

From what I see, it is the same IP.

> 
> On the sparx5 are there any peripheral who you would actually
> notice that the timing is off?

There are some SFP connected, similar to lan966x. So I don't understand
why that issue is not seen there.

> 
> That being said, I'd assume all the serial gpio controller has
> this "flaw". Simply because a register write won't block until the
> value is shifted out to the shift register and actualy loaded by
> strobing the load signal. It just depends on your burst setting
> (even with bursts off, and clocking all the time) on how large
> the delay is. So you might or might not notice it on a board.
> 
> Could you also have a look at the other supported sgpio block,
> the ocelot and the luton? I don't have any register description
> of these.
> 
> > 2. I remember that I have tried to fix this issue like this [1], but
> >    unfortunetly that was never accepted. Is this something that is
> > worth
> >    at looking at?
> 
> That fix is at the wrong place. You'd need to fix every gpio user, no?
> Instead this tries to fix the controller.

Yes, you are right.

> 
> > 
> > [1]
> > https://patchwork.ozlabs.org/project/linux-i2c/patch/20211103091839.1665672-3-horatiu.vultur@microchip.com/
> 
> -michael

-- 
/Horatiu

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Horatiu Vultur]

The 02/25/2022 12:29, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

Hi Michael,

Sorry for late reply.

> 
> Hi Horatiu,
> 
> Am 2022-02-25 10:24, schrieb Horatiu Vultur:
> > The 02/24/2022 17:10, Michael Walle wrote:
> > > Right now, when a gpio value is set, the actual hardware pin gets set
> > > asynchronously. When linux write the output register, it takes some
> > > time
> > > until it is actually propagated to the output shift registers. If that
> > > output port is connected to an I2C mux for example, the linux driver
> > > assumes the I2C bus is already switched although it is not.
> > > 
> > > Fortunately, there is a single shot mode with a feedback: you can
> > > trigger the single shot and the hardware will clear that bit once it
> > > has
> > > finished the clocking and strobed the load signal of the shift
> > > registers. This can take a considerable amount of time though.
> > > Measuremens have shown that it takes up to a whole burst cycle gap
> > > which
> > > is about 50ms on the largest setting. Therefore, we have to mark the
> > > output bank as sleepable. To avoid unnecessary waiting, just trigger
> > > the
> > > single shot if the value was actually changed.
> > 
> > I have tested this patch series on our lan9668 board and it worked
> > fine. Thanks!
> 
> Thanks for testing!
> 
> > I just have few questions:
> > 1. What about other boards/chips that have this sgpio, do they have
> > also
> >    the same issue? Because from what I recall on sparx5 they don't have
> >    this issue. I have seen it only on lan9668.
> 
> Unfortunatly, I don't have any knowledge what IP core is used in
> which SoC. I assumed the lan9668 used the same as the sparx5. If
> that is not the case, we need a new compatible. Do you know if it
> the same?

From what I see, it is the same IP.

> 
> On the sparx5 are there any peripheral who you would actually
> notice that the timing is off?

There are some SFP connected, similar to lan966x. So I don't understand
why that issue is not seen there.

> 
> That being said, I'd assume all the serial gpio controller has
> this "flaw". Simply because a register write won't block until the
> value is shifted out to the shift register and actualy loaded by
> strobing the load signal. It just depends on your burst setting
> (even with bursts off, and clocking all the time) on how large
> the delay is. So you might or might not notice it on a board.
> 
> Could you also have a look at the other supported sgpio block,
> the ocelot and the luton? I don't have any register description
> of these.
> 
> > 2. I remember that I have tried to fix this issue like this [1], but
> >    unfortunetly that was never accepted. Is this something that is
> > worth
> >    at looking at?
> 
> That fix is at the wrong place. You'd need to fix every gpio user, no?
> Instead this tries to fix the controller.

Yes, you are right.

> 
> > 
> > [1]
> > https://patchwork.ozlabs.org/project/linux-i2c/patch/20211103091839.1665672-3-horatiu.vultur@microchip.com/
> 
> -michael

-- 
/Horatiu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Michael Walle]

Hi Horatiu,

Am 2022-03-04 13:09, schrieb Horatiu Vultur:
> The 02/25/2022 12:29, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Hi Horatiu,
>> 
>> Am 2022-02-25 10:24, schrieb Horatiu Vultur:
>> > The 02/24/2022 17:10, Michael Walle wrote:
>> > > Right now, when a gpio value is set, the actual hardware pin gets set
>> > > asynchronously. When linux write the output register, it takes some
>> > > time
>> > > until it is actually propagated to the output shift registers. If that
>> > > output port is connected to an I2C mux for example, the linux driver
>> > > assumes the I2C bus is already switched although it is not.
>> > >
>> > > Fortunately, there is a single shot mode with a feedback: you can
>> > > trigger the single shot and the hardware will clear that bit once it
>> > > has
>> > > finished the clocking and strobed the load signal of the shift
>> > > registers. This can take a considerable amount of time though.
>> > > Measuremens have shown that it takes up to a whole burst cycle gap
>> > > which
>> > > is about 50ms on the largest setting. Therefore, we have to mark the
>> > > output bank as sleepable. To avoid unnecessary waiting, just trigger
>> > > the
>> > > single shot if the value was actually changed.
>> >
>> > I have tested this patch series on our lan9668 board and it worked
>> > fine. Thanks!
>> 
>> Thanks for testing!
>> 
>> > I just have few questions:
>> > 1. What about other boards/chips that have this sgpio, do they have
>> > also
>> >    the same issue? Because from what I recall on sparx5 they don't have
>> >    this issue. I have seen it only on lan9668.
>> 
>> Unfortunatly, I don't have any knowledge what IP core is used in
>> which SoC. I assumed the lan9668 used the same as the sparx5. If
>> that is not the case, we need a new compatible. Do you know if it
>> the same?
> 
> From what I see, it is the same IP.

Good to know.

>> On the sparx5 are there any peripheral who you would actually
>> notice that the timing is off?
> 
> There are some SFP connected, similar to lan966x. So I don't understand
> why that issue is not seen there.

Is there an I2C mux, too? Or just the SFP signals connected to
the SGPIO? What I was seeing is that during probing of the SFPs
the SFPs EEPROM is read and when the I2C mux is controlled by the
SGPIO it will switch too late - or even worse, in the middle of
a transaction. I would speculate the timing isn't that critical
with signals just connected directly to the SFP.

In any case, I think it is pretty clear that it cannot work the
way it is right now, no? See the very next paragraph...

>> That being said, I'd assume all the serial gpio controller has
>> this "flaw". Simply because a register write won't block until the
>> value is shifted out to the shift register and actualy loaded by
>> strobing the load signal. It just depends on your burst setting
>> (even with bursts off, and clocking all the time) on how large
>> the delay is. So you might or might not notice it on a board.

.. here

-michael

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Michael Walle]

Hi Horatiu,

Am 2022-03-04 13:09, schrieb Horatiu Vultur:
> The 02/25/2022 12:29, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Hi Horatiu,
>> 
>> Am 2022-02-25 10:24, schrieb Horatiu Vultur:
>> > The 02/24/2022 17:10, Michael Walle wrote:
>> > > Right now, when a gpio value is set, the actual hardware pin gets set
>> > > asynchronously. When linux write the output register, it takes some
>> > > time
>> > > until it is actually propagated to the output shift registers. If that
>> > > output port is connected to an I2C mux for example, the linux driver
>> > > assumes the I2C bus is already switched although it is not.
>> > >
>> > > Fortunately, there is a single shot mode with a feedback: you can
>> > > trigger the single shot and the hardware will clear that bit once it
>> > > has
>> > > finished the clocking and strobed the load signal of the shift
>> > > registers. This can take a considerable amount of time though.
>> > > Measuremens have shown that it takes up to a whole burst cycle gap
>> > > which
>> > > is about 50ms on the largest setting. Therefore, we have to mark the
>> > > output bank as sleepable. To avoid unnecessary waiting, just trigger
>> > > the
>> > > single shot if the value was actually changed.
>> >
>> > I have tested this patch series on our lan9668 board and it worked
>> > fine. Thanks!
>> 
>> Thanks for testing!
>> 
>> > I just have few questions:
>> > 1. What about other boards/chips that have this sgpio, do they have
>> > also
>> >    the same issue? Because from what I recall on sparx5 they don't have
>> >    this issue. I have seen it only on lan9668.
>> 
>> Unfortunatly, I don't have any knowledge what IP core is used in
>> which SoC. I assumed the lan9668 used the same as the sparx5. If
>> that is not the case, we need a new compatible. Do you know if it
>> the same?
> 
> From what I see, it is the same IP.

Good to know.

>> On the sparx5 are there any peripheral who you would actually
>> notice that the timing is off?
> 
> There are some SFP connected, similar to lan966x. So I don't understand
> why that issue is not seen there.

Is there an I2C mux, too? Or just the SFP signals connected to
the SGPIO? What I was seeing is that during probing of the SFPs
the SFPs EEPROM is read and when the I2C mux is controlled by the
SGPIO it will switch too late - or even worse, in the middle of
a transaction. I would speculate the timing isn't that critical
with signals just connected directly to the SFP.

In any case, I think it is pretty clear that it cannot work the
way it is right now, no? See the very next paragraph...

>> That being said, I'd assume all the serial gpio controller has
>> this "flaw". Simply because a register write won't block until the
>> value is shifted out to the shift register and actualy loaded by
>> strobing the load signal. It just depends on your burst setting
>> (even with bursts off, and clocking all the time) on how large
>> the delay is. So you might or might not notice it on a board.

.. here

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Horatiu Vultur]

The 03/04/2022 13:46, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Horatiu,
> 
> Am 2022-03-04 13:09, schrieb Horatiu Vultur:
> > The 02/25/2022 12:29, Michael Walle wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > the content is safe
> > > 
> > > Hi Horatiu,
> > > 
> > > Am 2022-02-25 10:24, schrieb Horatiu Vultur:
> > > > The 02/24/2022 17:10, Michael Walle wrote:
> > > > > Right now, when a gpio value is set, the actual hardware pin gets set
> > > > > asynchronously. When linux write the output register, it takes some
> > > > > time
> > > > > until it is actually propagated to the output shift registers. If that
> > > > > output port is connected to an I2C mux for example, the linux driver
> > > > > assumes the I2C bus is already switched although it is not.
> > > > >
> > > > > Fortunately, there is a single shot mode with a feedback: you can
> > > > > trigger the single shot and the hardware will clear that bit once it
> > > > > has
> > > > > finished the clocking and strobed the load signal of the shift
> > > > > registers. This can take a considerable amount of time though.
> > > > > Measuremens have shown that it takes up to a whole burst cycle gap
> > > > > which
> > > > > is about 50ms on the largest setting. Therefore, we have to mark the
> > > > > output bank as sleepable. To avoid unnecessary waiting, just trigger
> > > > > the
> > > > > single shot if the value was actually changed.
> > > >
> > > > I have tested this patch series on our lan9668 board and it worked
> > > > fine. Thanks!
> > > 
> > > Thanks for testing!
> > > 
> > > > I just have few questions:
> > > > 1. What about other boards/chips that have this sgpio, do they have
> > > > also
> > > >    the same issue? Because from what I recall on sparx5 they don't have
> > > >    this issue. I have seen it only on lan9668.
> > > 
> > > Unfortunatly, I don't have any knowledge what IP core is used in
> > > which SoC. I assumed the lan9668 used the same as the sparx5. If
> > > that is not the case, we need a new compatible. Do you know if it
> > > the same?
> > 
> > From what I see, it is the same IP.
> 
> Good to know.
> 
> > > On the sparx5 are there any peripheral who you would actually
> > > notice that the timing is off?
> > 
> > There are some SFP connected, similar to lan966x. So I don't understand
> > why that issue is not seen there.
> 
> Is there an I2C mux, too? 

It looks like also on sparx5 is an i2c mux[1]. The only difference is
that is controlled by pinctrl and not by SGPIO.

> Or just the SFP signals connected to
> the SGPIO? What I was seeing is that during probing of the SFPs
> the SFPs EEPROM is read and when the I2C mux is controlled by the
> SGPIO it will switch too late - or even worse, in the middle of
> a transaction. I would speculate the timing isn't that critical
> with signals just connected directly to the SFP.
> 
> In any case, I think it is pretty clear that it cannot work the
> way it is right now, no? See the very next paragraph...

Yes, I agree, this needs to be fixed.

> 
> > > That being said, I'd assume all the serial gpio controller has
> > > this "flaw". Simply because a register write won't block until the
> > > value is shifted out to the shift register and actualy loaded by
> > > strobing the load signal. It just depends on your burst setting
> > > (even with bursts off, and clocking all the time) on how large
> > > the delay is. So you might or might not notice it on a board.
> 
> .. here
> 
> -michael

[1] https://elixir.bootlin.com/linux/v5.17-rc6/source/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi#L404

-- 
/Horatiu

Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is actually set

[Horatiu Vultur]

The 03/04/2022 13:46, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Horatiu,
> 
> Am 2022-03-04 13:09, schrieb Horatiu Vultur:
> > The 02/25/2022 12:29, Michael Walle wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > the content is safe
> > > 
> > > Hi Horatiu,
> > > 
> > > Am 2022-02-25 10:24, schrieb Horatiu Vultur:
> > > > The 02/24/2022 17:10, Michael Walle wrote:
> > > > > Right now, when a gpio value is set, the actual hardware pin gets set
> > > > > asynchronously. When linux write the output register, it takes some
> > > > > time
> > > > > until it is actually propagated to the output shift registers. If that
> > > > > output port is connected to an I2C mux for example, the linux driver
> > > > > assumes the I2C bus is already switched although it is not.
> > > > >
> > > > > Fortunately, there is a single shot mode with a feedback: you can
> > > > > trigger the single shot and the hardware will clear that bit once it
> > > > > has
> > > > > finished the clocking and strobed the load signal of the shift
> > > > > registers. This can take a considerable amount of time though.
> > > > > Measuremens have shown that it takes up to a whole burst cycle gap
> > > > > which
> > > > > is about 50ms on the largest setting. Therefore, we have to mark the
> > > > > output bank as sleepable. To avoid unnecessary waiting, just trigger
> > > > > the
> > > > > single shot if the value was actually changed.
> > > >
> > > > I have tested this patch series on our lan9668 board and it worked
> > > > fine. Thanks!
> > > 
> > > Thanks for testing!
> > > 
> > > > I just have few questions:
> > > > 1. What about other boards/chips that have this sgpio, do they have
> > > > also
> > > >    the same issue? Because from what I recall on sparx5 they don't have
> > > >    this issue. I have seen it only on lan9668.
> > > 
> > > Unfortunatly, I don't have any knowledge what IP core is used in
> > > which SoC. I assumed the lan9668 used the same as the sparx5. If
> > > that is not the case, we need a new compatible. Do you know if it
> > > the same?
> > 
> > From what I see, it is the same IP.
> 
> Good to know.
> 
> > > On the sparx5 are there any peripheral who you would actually
> > > notice that the timing is off?
> > 
> > There are some SFP connected, similar to lan966x. So I don't understand
> > why that issue is not seen there.
> 
> Is there an I2C mux, too? 

It looks like also on sparx5 is an i2c mux[1]. The only difference is
that is controlled by pinctrl and not by SGPIO.

> Or just the SFP signals connected to
> the SGPIO? What I was seeing is that during probing of the SFPs
> the SFPs EEPROM is read and when the I2C mux is controlled by the
> SGPIO it will switch too late - or even worse, in the middle of
> a transaction. I would speculate the timing isn't that critical
> with signals just connected directly to the SFP.
> 
> In any case, I think it is pretty clear that it cannot work the
> way it is right now, no? See the very next paragraph...

Yes, I agree, this needs to be fixed.

> 
> > > That being said, I'd assume all the serial gpio controller has
> > > this "flaw". Simply because a register write won't block until the
> > > value is shifted out to the shift register and actualy loaded by
> > > strobing the load signal. It just depends on your burst setting
> > > (even with bursts off, and clocking all the time) on how large
> > > the delay is. So you might or might not notice it on a board.
> 
> .. here
> 
> -michael

[1] https://elixir.bootlin.com/linux/v5.17-rc6/source/arch/arm64/boot/dts/microchip/sparx5_pcb134_board.dtsi#L404

-- 
/Horatiu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel