| Message ID | 20220224161021.2197263-6-michael@walle.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: microchip-sgpio: locking and synchronous output | expand |
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 >
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
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
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
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
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))
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(-)