| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220225092427.jjilv3qo52crsmuw@soft-dev3-1.localhost>
Date: Fri, 25 Feb 2022 10:24:27 +0100
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Michael Walle <michael@...le.cc>
CC: Lars Povlsen <lars.povlsen@...rochip.com>,
Steen Hegelund <Steen.Hegelund@...rochip.com>,
Linus Walleij <linus.walleij@...aro.org>,
<UNGLinuxDriver@...rochip.com>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-gpio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
Colin Foster <colin.foster@...advantage.com>
Subject: Re: [PATCH v1 5/5] pinctrl: microchip-sgpio: wait until output is
actually set
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@...le.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
Powered by blists - more mailing lists