[v2] net: sfp: workaround GPIO input signals bounce

Commit Message

Baruch Siach Sept. 14, 2022, 5:36 a.m. UTC

From: Baruch Siach <[e-mail]>

Add a trivial debounce to avoid miss of state changes when there is no
proper hardware debounce on the input signals. Otherwise a GPIO might
randomly indicate high level when the signal is actually going down,
and vice versa.

This fixes observed miss of link up event when LOS signal goes down on
Armada 8040 based system with an optical SFP module.

Signed-off-by: Baruch Siach <[e-mail]>
---
v2:
  Skip delay in the polling case (RMK)
---
 drivers/net/phy/sfp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Jakub Kicinski Sept. 20, 2022, 3:19 p.m. UTC | #1

On Wed, 14 Sep 2022 08:36:35 +0300 Baruch Siach wrote:
> From: Baruch Siach <[e-mail]>
> 
> Add a trivial debounce to avoid miss of state changes when there is no
> proper hardware debounce on the input signals. Otherwise a GPIO might
> randomly indicate high level when the signal is actually going down,
> and vice versa.
> 
> This fixes observed miss of link up event when LOS signal goes down on
> Armada 8040 based system with an optical SFP module.
> 
> Signed-off-by: Baruch Siach <[e-mail]>
> ---
> v2:
>   Skip delay in the polling case (RMK)

Is this acceptable now, Russell?

> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 63f90fe9a4d2..b0ba144c9633 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -313,7 +313,9 @@ static unsigned long poll_jiffies;
>  static unsigned int sfp_gpio_get_state(struct sfp *sfp)
>  {
>  	unsigned int i, state, v;
> +	int repeat = 10;
>  
> +again:
>  	for (i = state = 0; i < GPIO_MAX; i++) {
>  		if (gpio_flags[i] != GPIOD_IN || !sfp->gpio[i])
>  			continue;
> @@ -323,6 +325,16 @@ static unsigned int sfp_gpio_get_state(struct sfp *sfp)
>  			state |= BIT(i);
>  	}
>  
> +	/* Trivial debounce for the interrupt case. When no state change is
> +	 * detected, wait for up to a limited bound time interval for the
> +	 * signal state to settle.
> +	 */
> +	if (state == sfp->state && !sfp->need_poll && repeat > 0) {
> +		udelay(10);
> +		repeat--;
> +		goto again;
> +	}
> +
>  	return state;
>  }
>

Russell King (Oracle) Sept. 20, 2022, 5:21 p.m. UTC | #2

On Tue, Sep 20, 2022 at 08:19:11AM -0700, Jakub Kicinski wrote:
> On Wed, 14 Sep 2022 08:36:35 +0300 Baruch Siach wrote:
> > From: Baruch Siach <[e-mail]>
> > 
> > Add a trivial debounce to avoid miss of state changes when there is no
> > proper hardware debounce on the input signals. Otherwise a GPIO might
> > randomly indicate high level when the signal is actually going down,
> > and vice versa.
> > 
> > This fixes observed miss of link up event when LOS signal goes down on
> > Armada 8040 based system with an optical SFP module.
> > 
> > Signed-off-by: Baruch Siach <[e-mail]>
> > ---
> > v2:
> >   Skip delay in the polling case (RMK)
> 
> Is this acceptable now, Russell?

I don't think so. The decision to poll is not just sfp->need_poll,
we also do it when need_poll is false, but we need to use the soft
state as well:

        if (sfp->state_soft_mask & (SFP_F_LOS | SFP_F_TX_FAULT) &&
            !sfp->need_poll)
                mod_delayed_work(system_wq, &sfp->poll, poll_jiffies);

I think, if we're going to use this "simple" debounce, we need to
add a flag to sfp_gpio_get_state() that indicates whether it's been
called from an interrupt.

However, even that isn't ideal, because if we poll, we get no
debouncing.

The unfortunate thing is, on the Macchiatobin (which I suspect is
the platform that Baruch is addressing here) there are no pull-up
resistors on the lines as required by the SFP MSA, so they tend to
float around when not being actively driven. Debouncing will help
to avoid some of the problems stemming from that, but ultimately
some will still get through. The only true real is a hardware one
which isn't going to happen.

Baruch Siach Sept. 21, 2022, 4:57 a.m. UTC | #3

Hi Russell,

On Tue, Sep 20 2022, Russell King (Oracle) wrote:
> On Tue, Sep 20, 2022 at 08:19:11AM -0700, Jakub Kicinski wrote:
>> On Wed, 14 Sep 2022 08:36:35 +0300 Baruch Siach wrote:
>> > From: Baruch Siach <[e-mail]>
>> > 
>> > Add a trivial debounce to avoid miss of state changes when there is no
>> > proper hardware debounce on the input signals. Otherwise a GPIO might
>> > randomly indicate high level when the signal is actually going down,
>> > and vice versa.
>> > 
>> > This fixes observed miss of link up event when LOS signal goes down on
>> > Armada 8040 based system with an optical SFP module.
>> > 
>> > Signed-off-by: Baruch Siach <[e-mail]>
>> > ---
>> > v2:
>> >   Skip delay in the polling case (RMK)
>> 
>> Is this acceptable now, Russell?
>
> I don't think so. The decision to poll is not just sfp->need_poll,
> we also do it when need_poll is false, but we need to use the soft
> state as well:
>
>         if (sfp->state_soft_mask & (SFP_F_LOS | SFP_F_TX_FAULT) &&
>             !sfp->need_poll)
>                 mod_delayed_work(system_wq, &sfp->poll, poll_jiffies);
>
> I think, if we're going to use this "simple" debounce, we need to
> add a flag to sfp_gpio_get_state() that indicates whether it's been
> called from an interrupt.
>
> However, even that isn't ideal, because if we poll, we get no
> debouncing.

Why would you need debouncing in the poll case? The next poll will give
you the updated state, isn't it?

> The unfortunate thing is, on the Macchiatobin (which I suspect is
> the platform that Baruch is addressing here) there are no pull-up
> resistors on the lines as required by the SFP MSA, so they tend to
> float around when not being actively driven. Debouncing will help
> to avoid some of the problems stemming from that, but ultimately
> some will still get through. The only true real is a hardware one
> which isn't going to happen.

The design of the hardware I am dealing with is based on the
Macchiatobin.  The pull-ups are indeed missing which caused us other
trouble as well (see the hack below). Though I would not expect pull-up
absence to affect the LOS signal high to low transition (link up).

commit 2e76b75d8623b016390126b54b4d4047b13dc085
Author: Baruch Siach <[e-mail]>
Date:   Mon Apr 5 16:40:27 2021 +0300

    net: sfp: workaround missing Tx disable pull-up
    
    When Tx disable pull-up is missing the SFP module might not sense the
    transition from disable to enable. The signal just stays low.
    
    As a workaround assert Tx disable on probe.
    
    This only works when the SFP is plugged in when the sfp module probe.
    Hot plug of SFP module might not work.
    
    Signed-off-by: Baruch Siach <[e-mail]>

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 73c2969f11a4..d41bbd617123 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -2327,6 +2327,9 @@ static int sfp_probe(struct platform_device *pdev)
 	 * since the network interface will not be up.
 	 */
 	sfp->state = sfp_get_state(sfp) | SFP_F_TX_DISABLE;
+	/* Siklu workaround: missing Tx disable pull-up. Force disable. */
+	if ((sfp->state & SFP_F_PRESENT) && sfp->gpio[GPIO_TX_DISABLE])
+		gpiod_direction_output(sfp->gpio[GPIO_TX_DISABLE], 1);
 
 	if (sfp->gpio[GPIO_RATE_SELECT] &&
 	    gpiod_get_value_cansleep(sfp->gpio[GPIO_RATE_SELECT]))

baruch

Patch

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 63f90fe9a4d2..b0ba144c9633 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -313,7 +313,9 @@  static unsigned long poll_jiffies;
 static unsigned int sfp_gpio_get_state(struct sfp *sfp)
 {
 	unsigned int i, state, v;
+	int repeat = 10;
 
+again:
 	for (i = state = 0; i < GPIO_MAX; i++) {
 		if (gpio_flags[i] != GPIOD_IN || !sfp->gpio[i])
 			continue;
@@ -323,6 +325,16 @@  static unsigned int sfp_gpio_get_state(struct sfp *sfp)
 			state |= BIT(i);
 	}
 
+	/* Trivial debounce for the interrupt case. When no state change is
+	 * detected, wait for up to a limited bound time interval for the
+	 * signal state to settle.
+	 */
+	if (state == sfp->state && !sfp->need_poll && repeat > 0) {
+		udelay(10);
+		repeat--;
+		goto again;
+	}
+
 	return state;
 }