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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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/[email protected]/
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> 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
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/[email protected]/
-michael
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
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
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
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
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/[email protected]/
>
> -michael
--
/Horatiu