* [PATCH v2] net: sfp: workaround GPIO input signals bounce @ 2022-09-14 5:36 Baruch Siach 2022-09-20 15:19 ` Jakub Kicinski 0 siblings, 1 reply; 4+ messages in thread From: Baruch Siach @ 2022-09-14 5:36 UTC (permalink / raw) To: Russell King; +Cc: netdev, Andrew Lunn, Baruch Siach From: Baruch Siach <baruch.siach@siklu.com> 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 <baruch.siach@siklu.com> --- v2: Skip delay in the polling case (RMK) --- drivers/net/phy/sfp.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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; } -- 2.35.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] net: sfp: workaround GPIO input signals bounce 2022-09-14 5:36 [PATCH v2] net: sfp: workaround GPIO input signals bounce Baruch Siach @ 2022-09-20 15:19 ` Jakub Kicinski 2022-09-20 17:21 ` Russell King (Oracle) 0 siblings, 1 reply; 4+ messages in thread From: Jakub Kicinski @ 2022-09-20 15:19 UTC (permalink / raw) To: Russell King; +Cc: Baruch Siach, netdev, Andrew Lunn, Baruch Siach On Wed, 14 Sep 2022 08:36:35 +0300 Baruch Siach wrote: > From: Baruch Siach <baruch.siach@siklu.com> > > 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 <baruch.siach@siklu.com> > --- > 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; > } > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] net: sfp: workaround GPIO input signals bounce 2022-09-20 15:19 ` Jakub Kicinski @ 2022-09-20 17:21 ` Russell King (Oracle) 2022-09-21 4:57 ` Baruch Siach 0 siblings, 1 reply; 4+ messages in thread From: Russell King (Oracle) @ 2022-09-20 17:21 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Baruch Siach, netdev, Andrew Lunn, Baruch Siach 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 <baruch.siach@siklu.com> > > > > 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 <baruch.siach@siklu.com> > > --- > > 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. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] net: sfp: workaround GPIO input signals bounce 2022-09-20 17:21 ` Russell King (Oracle) @ 2022-09-21 4:57 ` Baruch Siach 0 siblings, 0 replies; 4+ messages in thread From: Baruch Siach @ 2022-09-21 4:57 UTC (permalink / raw) To: Russell King (Oracle); +Cc: Jakub Kicinski, netdev, Andrew Lunn 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 <baruch.siach@siklu.com> >> > >> > 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 <baruch.siach@siklu.com> >> > --- >> > 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 <baruch@tkos.co.il> 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 <baruch@tkos.co.il> 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 -- ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-21 5:31 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-14 5:36 [PATCH v2] net: sfp: workaround GPIO input signals bounce Baruch Siach 2022-09-20 15:19 ` Jakub Kicinski 2022-09-20 17:21 ` Russell King (Oracle) 2022-09-21 4:57 ` Baruch Siach
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).