* [PATCH net 0/2] net: systemport: misc fixes @ 2014-06-20 20:01 Florian Fainelli 2014-06-20 20:01 ` [PATCH net 1/2] net: systemport: do not clear IFF_MULTICAST flag Florian Fainelli 2014-06-20 20:01 ` [PATCH net 2/2] net: systemport: fix UniMAC reset logic Florian Fainelli 0 siblings, 2 replies; 6+ messages in thread From: Florian Fainelli @ 2014-06-20 20:01 UTC (permalink / raw) To: netdev; +Cc: davem, Florian Fainelli Hi David, Here are two small fixes for SYSTEMPORT targeted at the 'net' tree, thanks! Florian Fainelli (2): net: systemport: do not clear IFF_MULTICAST flag net: systemport: fix UniMAC reset logic drivers/net/ethernet/broadcom/bcmsysport.c | 39 +++++++----------------------- 1 file changed, 9 insertions(+), 30 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 1/2] net: systemport: do not clear IFF_MULTICAST flag 2014-06-20 20:01 [PATCH net 0/2] net: systemport: misc fixes Florian Fainelli @ 2014-06-20 20:01 ` Florian Fainelli 2014-06-20 20:01 ` [PATCH net 2/2] net: systemport: fix UniMAC reset logic Florian Fainelli 1 sibling, 0 replies; 6+ messages in thread From: Florian Fainelli @ 2014-06-20 20:01 UTC (permalink / raw) To: netdev; +Cc: davem, Florian Fainelli The SYSTEMPORT Ethernet MAC supports multicast just fine, it just lacks any sort of Unicast/Broadcast/Multicasting filtering at the Ethernet MAC level since that is handled by the front end Ethernet switch, but that is properly handled by bcm_sysport_set_rx_mode(). Some user-space applications might be relying on the presence of this flag to prevent using multicast sockets, this also prevents that interface from joining the IPv6 all-router mcast group. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/ethernet/broadcom/bcmsysport.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index 141160ef249a..f6bccd847ee6 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -1589,12 +1589,6 @@ static int bcm_sysport_probe(struct platform_device *pdev) BUILD_BUG_ON(sizeof(struct bcm_tsb) != 8); dev->needed_headroom += sizeof(struct bcm_tsb); - /* We are interfaced to a switch which handles the multicast - * filtering for us, so we do not support programming any - * multicast hash table in this Ethernet MAC. - */ - dev->flags &= ~IFF_MULTICAST; - /* libphy will adjust the link state accordingly */ netif_carrier_off(dev); -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net 2/2] net: systemport: fix UniMAC reset logic 2014-06-20 20:01 [PATCH net 0/2] net: systemport: misc fixes Florian Fainelli 2014-06-20 20:01 ` [PATCH net 1/2] net: systemport: do not clear IFF_MULTICAST flag Florian Fainelli @ 2014-06-20 20:01 ` Florian Fainelli 2014-06-20 20:14 ` Sergei Shtylyov 2014-06-24 23:12 ` David Miller 1 sibling, 2 replies; 6+ messages in thread From: Florian Fainelli @ 2014-06-20 20:01 UTC (permalink / raw) To: netdev; +Cc: davem, Florian Fainelli The UniMAC CMD_SW_RESET bit is not a self-clearing bit, so we need to assert it, wait a bit and clear it manually. As a result, umac_reset() is updated not to return any value. By setting CMD_SW_RESET and only that bit, we were also clearing the other 31 bits, overriding the hardware reset defaults which are correctly set on purpose. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/net/ethernet/broadcom/bcmsysport.c | 33 ++++++++---------------------- 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index f6bccd847ee6..d31f7d239064 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -1254,28 +1254,17 @@ static inline void umac_enable_set(struct bcm_sysport_priv *priv, usleep_range(1000, 2000); } -static inline int umac_reset(struct bcm_sysport_priv *priv) +static inline void umac_reset(struct bcm_sysport_priv *priv) { - unsigned int timeout = 0; u32 reg; - int ret = 0; - - umac_writel(priv, 0, UMAC_CMD); - while (timeout++ < 1000) { - reg = umac_readl(priv, UMAC_CMD); - if (!(reg & CMD_SW_RESET)) - break; - - udelay(1); - } - - if (timeout == 1000) { - dev_err(&priv->pdev->dev, - "timeout waiting for MAC to come out of reset\n"); - ret = -ETIMEDOUT; - } - return ret; + reg = umac_readl(priv, UMAC_CMD); + reg |= CMD_SW_RESET; + umac_writel(priv, reg, UMAC_CMD); + udelay(10); + reg = umac_readl(priv, UMAC_CMD); + reg &= ~CMD_SW_RESET; + umac_writel(priv, reg, UMAC_CMD); } static void umac_set_hw_addr(struct bcm_sysport_priv *priv, @@ -1303,11 +1292,7 @@ static int bcm_sysport_open(struct net_device *dev) int ret; /* Reset UniMAC */ - ret = umac_reset(priv); - if (ret) { - netdev_err(dev, "UniMAC reset failed\n"); - return ret; - } + umac_reset(priv); /* Flush TX and RX FIFOs at TOPCTRL level */ topctrl_flush(priv); -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net 2/2] net: systemport: fix UniMAC reset logic 2014-06-20 20:01 ` [PATCH net 2/2] net: systemport: fix UniMAC reset logic Florian Fainelli @ 2014-06-20 20:14 ` Sergei Shtylyov 2014-06-24 23:12 ` David Miller 1 sibling, 0 replies; 6+ messages in thread From: Sergei Shtylyov @ 2014-06-20 20:14 UTC (permalink / raw) To: Florian Fainelli, netdev; +Cc: davem Hello. On 06/21/2014 12:01 AM, Florian Fainelli wrote: > The UniMAC CMD_SW_RESET bit is not a self-clearing bit, so we need to > assert it, wait a bit and clear it manually. As a result, umac_reset() > is updated not to return any value. > By setting CMD_SW_RESET and only that bit, we were also clearing the I wonder where you were setting this bit before -- the only umac_writel() there was wrote 0 to UMAC_CMD... > other 31 bits, overriding the hardware reset defaults which are > correctly set on purpose. > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/net/ethernet/broadcom/bcmsysport.c | 33 ++++++++---------------------- > 1 file changed, 9 insertions(+), 24 deletions(-) > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c > index f6bccd847ee6..d31f7d239064 100644 > --- a/drivers/net/ethernet/broadcom/bcmsysport.c > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c > @@ -1254,28 +1254,17 @@ static inline void umac_enable_set(struct bcm_sysport_priv *priv, > usleep_range(1000, 2000); > } > > -static inline int umac_reset(struct bcm_sysport_priv *priv) > +static inline void umac_reset(struct bcm_sysport_priv *priv) > { > - unsigned int timeout = 0; > u32 reg; > - int ret = 0; > - > - umac_writel(priv, 0, UMAC_CMD); > - while (timeout++ < 1000) { > - reg = umac_readl(priv, UMAC_CMD); > - if (!(reg & CMD_SW_RESET)) > - break; > - > - udelay(1); > - } > - > - if (timeout == 1000) { > - dev_err(&priv->pdev->dev, > - "timeout waiting for MAC to come out of reset\n"); > - ret = -ETIMEDOUT; > - } > > - return ret; > + reg = umac_readl(priv, UMAC_CMD); > + reg |= CMD_SW_RESET; > + umac_writel(priv, reg, UMAC_CMD); > + udelay(10); > + reg = umac_readl(priv, UMAC_CMD); > + reg &= ~CMD_SW_RESET; > + umac_writel(priv, reg, UMAC_CMD); > } > > static void umac_set_hw_addr(struct bcm_sysport_priv *priv, [...] WBR, Sergei ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 2/2] net: systemport: fix UniMAC reset logic 2014-06-20 20:01 ` [PATCH net 2/2] net: systemport: fix UniMAC reset logic Florian Fainelli 2014-06-20 20:14 ` Sergei Shtylyov @ 2014-06-24 23:12 ` David Miller 2014-06-24 23:16 ` Florian Fainelli 1 sibling, 1 reply; 6+ messages in thread From: David Miller @ 2014-06-24 23:12 UTC (permalink / raw) To: f.fainelli; +Cc: netdev From: Florian Fainelli <f.fainelli@gmail.com> Date: Fri, 20 Jun 2014 13:01:40 -0700 > The UniMAC CMD_SW_RESET bit is not a self-clearing bit, so we need to > assert it, wait a bit and clear it manually. As a result, umac_reset() > is updated not to return any value. > > By setting CMD_SW_RESET and only that bit, we were also clearing the > other 31 bits, overriding the hardware reset defaults which are > correctly set on purpose. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> I agree with Sergei, this is really suspicious that the existing code never wrote the sw reset bit into the register at all, it just writes a zero. And you don't mention this at all in this commit message. If I were to guess, I would fathom that the sequence was partially copied from the other broadcom driver, the genet one. There it issues CMD_SW_RESET by first clearing the entire regiser to zero, then writing CMD_SW_RESET, and then polling for that bit to clear. I'm assuming that is being done for a good reason, and I'd like you to make sure that same reason does not apply here. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 2/2] net: systemport: fix UniMAC reset logic 2014-06-24 23:12 ` David Miller @ 2014-06-24 23:16 ` Florian Fainelli 0 siblings, 0 replies; 6+ messages in thread From: Florian Fainelli @ 2014-06-24 23:16 UTC (permalink / raw) To: David Miller; +Cc: netdev 2014-06-24 16:12 GMT-07:00 David Miller <davem@davemloft.net>: > From: Florian Fainelli <f.fainelli@gmail.com> > Date: Fri, 20 Jun 2014 13:01:40 -0700 > >> The UniMAC CMD_SW_RESET bit is not a self-clearing bit, so we need to >> assert it, wait a bit and clear it manually. As a result, umac_reset() >> is updated not to return any value. >> >> By setting CMD_SW_RESET and only that bit, we were also clearing the >> other 31 bits, overriding the hardware reset defaults which are >> correctly set on purpose. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > I agree with Sergei, this is really suspicious that the existing code > never wrote the sw reset bit into the register at all, it just writes > a zero. > > And you don't mention this at all in this commit message. > > If I were to guess, I would fathom that the sequence was partially > copied from the other broadcom driver, the genet one. There it > issues CMD_SW_RESET by first clearing the entire regiser to zero, > then writing CMD_SW_RESET, and then polling for that bit to clear. > > I'm assuming that is being done for a good reason, and I'd like > you to make sure that same reason does not apply here. I'll resubmit with a proper explanation, thanks. -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-24 23:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-20 20:01 [PATCH net 0/2] net: systemport: misc fixes Florian Fainelli 2014-06-20 20:01 ` [PATCH net 1/2] net: systemport: do not clear IFF_MULTICAST flag Florian Fainelli 2014-06-20 20:01 ` [PATCH net 2/2] net: systemport: fix UniMAC reset logic Florian Fainelli 2014-06-20 20:14 ` Sergei Shtylyov 2014-06-24 23:12 ` David Miller 2014-06-24 23:16 ` Florian Fainelli
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).