| Message ID | cover.1616368101.git.cristian.ciocaltea@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for Actions Semi Owl Ethernet MAC | expand |
Hi Christian, On 3/21/2021 4:29 PM, Cristian Ciocaltea wrote: > Add new driver for the Ethernet MAC used on the Actions Semi Owl > family of SoCs. > > Currently this has been tested only on the Actions Semi S500 SoC > variant. > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> [snip] Do you know the story behind this Ethernet controller? The various receive/transmit descriptor definitions are 99% those defined in drivers/net/ethernet/stmmicro/stmmac/descs.h for the normal descriptor. The register layout of the MAC looks completely different from a dwmac100 or dwmac1000 however. -- Florian
Hi Florian, On Sun, Mar 21, 2021 at 08:30:07PM -0700, Florian Fainelli wrote: > Hi Christian, > > On 3/21/2021 4:29 PM, Cristian Ciocaltea wrote: > > Add new driver for the Ethernet MAC used on the Actions Semi Owl > > family of SoCs. > > > > Currently this has been tested only on the Actions Semi S500 SoC > > variant. > > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com> > > [snip] > > Do you know the story behind this Ethernet controller? I just happened to get a board based on the S500 SoC, so I took this opportunity to help improving the mainline kernel support, but other than that I do not really know much about the hardware history. > The various > receive/transmit descriptor definitions are 99% those defined in > drivers/net/ethernet/stmmicro/stmmac/descs.h for the normal descriptor. That's an interesting observation. I could only assume the vendor did not want to reinvent the wheel here, but I cannot say if this is a common design scheme or is something specific to STMicroelectronics only. > The register layout of the MAC looks completely different from a > dwmac100 or dwmac1000 however. Most probably this is Actions specific.. > -- > Florian
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Mon, 22 Mar 2021 01:29:42 +0200 you wrote: > This patch series adds support for the Ethernet MAC found on the Actions > Semi Owl family of SoCs. > > For the moment I have only tested the driver on RoseapplePi SBC, which is > based on the S500 SoC variant. It might work on S900 as well, but I cannot > tell for sure since the S900 datasheet I currently have doesn't provide > any information regarding the MAC registers - so I couldn't check the > compatibility with S500. > > [...] Here is the summary with links: - [v3,1/3] dt-bindings: net: Add Actions Semi Owl Ethernet MAC binding https://git.kernel.org/netdev/net-next/c/fd42327f31bb - [v3,2/3] net: ethernet: actions: Add Actions Semi Owl Ethernet MAC driver https://git.kernel.org/netdev/net-next/c/de6e0b198239 - [v3,3/3] MAINTAINERS: Add entries for Actions Semi Owl Ethernet MAC https://git.kernel.org/netdev/net-next/c/b31f51832acf You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
> +static void owl_emac_set_multicast(struct net_device *netdev, int count) > +{ > + struct owl_emac_priv *priv = netdev_priv(netdev); > + struct netdev_hw_addr *ha; > + int index = 0; > + > + if (count <= 0) { > + priv->mcaddr_list.count = 0; > + return; > + } > + > + netdev_for_each_mc_addr(ha, netdev) { > + if (!is_multicast_ether_addr(ha->addr)) > + continue; Is this possible? > + > + WARN_ON(index >= OWL_EMAC_MAX_MULTICAST_ADDRS); > + ether_addr_copy(priv->mcaddr_list.addrs[index++], ha->addr); > + } > + > + priv->mcaddr_list.count = index; > + > + owl_emac_setup_frame_xmit(priv); > +} > + > +static void owl_emac_ndo_set_rx_mode(struct net_device *netdev) > +{ > + struct owl_emac_priv *priv = netdev_priv(netdev); > + u32 status, val = 0; > + int mcast_count = 0; > + > + if (netdev->flags & IFF_PROMISC) { > + val = OWL_EMAC_BIT_MAC_CSR6_PR; > + } else if (netdev->flags & IFF_ALLMULTI) { > + val = OWL_EMAC_BIT_MAC_CSR6_PM; > + } else if (netdev->flags & IFF_MULTICAST) { > + mcast_count = netdev_mc_count(netdev); > + > + if (mcast_count > OWL_EMAC_MAX_MULTICAST_ADDRS) { > + val = OWL_EMAC_BIT_MAC_CSR6_PM; > + mcast_count = 0; > + } > + } > + > + spin_lock_bh(&priv->lock); > + > + /* Temporarily stop DMA TX & RX. */ > + status = owl_emac_dma_cmd_stop(priv); > + > + /* Update operation modes. */ > + owl_emac_reg_update(priv, OWL_EMAC_REG_MAC_CSR6, > + OWL_EMAC_BIT_MAC_CSR6_PR | OWL_EMAC_BIT_MAC_CSR6_PM, > + val); > + > + /* Restore DMA TX & RX status. */ > + owl_emac_dma_cmd_set(priv, status); > + > + spin_unlock_bh(&priv->lock); > + > + /* Set/reset multicast addr list. */ > + owl_emac_set_multicast(netdev, mcast_count); > +} I think this can be simplified. At least, you had me going around in circles a while trying to see if WARN_ON() could be triggered from user space. If you have more than OWL_EMAC_MAX_MULTICAST_ADDRS MC addresses, you go into promisc mode. Can you then skip calling owl_emac_set_multicast(), which appears not to do too much useful when passed 0? Andrew
On Tue, Mar 23, 2021 at 12:38:10AM +0100, Andrew Lunn wrote: > > +static void owl_emac_set_multicast(struct net_device *netdev, int count) > > +{ > > + struct owl_emac_priv *priv = netdev_priv(netdev); > > + struct netdev_hw_addr *ha; > > + int index = 0; > > + > > + if (count <= 0) { > > + priv->mcaddr_list.count = 0; > > + return; > > + } > > + > > + netdev_for_each_mc_addr(ha, netdev) { > > + if (!is_multicast_ether_addr(ha->addr)) > > + continue; > > Is this possible? I remember I've seen this in one of the drivers I have studied, but I'm not really sure it is actually necessary. I added it to be on the safe side.. > > + > > + WARN_ON(index >= OWL_EMAC_MAX_MULTICAST_ADDRS); > > + ether_addr_copy(priv->mcaddr_list.addrs[index++], ha->addr); > > + } > > + > > + priv->mcaddr_list.count = index; > > + > > + owl_emac_setup_frame_xmit(priv); > > +} > > + > > +static void owl_emac_ndo_set_rx_mode(struct net_device *netdev) > > +{ > > + struct owl_emac_priv *priv = netdev_priv(netdev); > > + u32 status, val = 0; > > + int mcast_count = 0; > > + > > + if (netdev->flags & IFF_PROMISC) { > > + val = OWL_EMAC_BIT_MAC_CSR6_PR; > > + } else if (netdev->flags & IFF_ALLMULTI) { > > + val = OWL_EMAC_BIT_MAC_CSR6_PM; > > + } else if (netdev->flags & IFF_MULTICAST) { > > + mcast_count = netdev_mc_count(netdev); > > + > > + if (mcast_count > OWL_EMAC_MAX_MULTICAST_ADDRS) { > > + val = OWL_EMAC_BIT_MAC_CSR6_PM; > > + mcast_count = 0; > > + } > > + } > > + > > + spin_lock_bh(&priv->lock); > > + > > + /* Temporarily stop DMA TX & RX. */ > > + status = owl_emac_dma_cmd_stop(priv); > > + > > + /* Update operation modes. */ > > + owl_emac_reg_update(priv, OWL_EMAC_REG_MAC_CSR6, > > + OWL_EMAC_BIT_MAC_CSR6_PR | OWL_EMAC_BIT_MAC_CSR6_PM, > > + val); > > + > > + /* Restore DMA TX & RX status. */ > > + owl_emac_dma_cmd_set(priv, status); > > + > > + spin_unlock_bh(&priv->lock); > > + > > + /* Set/reset multicast addr list. */ > > + owl_emac_set_multicast(netdev, mcast_count); > > +} > > I think this can be simplified. At least, you had me going around in > circles a while trying to see if WARN_ON() could be triggered from > user space. > > If you have more than OWL_EMAC_MAX_MULTICAST_ADDRS MC addresses, you > go into promisc mode. Can you then skip calling > owl_emac_set_multicast(), which appears not to do too much useful when > passed 0? The main purpose of always calling owl_emac_set_multicast() is to ensure the size of the mcaddr_list is correctly updated (either set or reset). This prevents owl_emac_setup_frame_xmit() using obsolete data, when invoked from different contexts (i.e. MAC address changed). A conditional call involves splitting the mcaddr_list management logic (i.e. moving the 'reset' operation from the callee to the caller), which IMO would make the usage of a separate function less justified. Thanks, Cristi > > Andrew
Hi, > > Do you know the story behind this Ethernet controller? > > I just happened to get a board based on the S500 SoC, so I took this > opportunity to help improving the mainline kernel support, but other > than that I do not really know much about the hardware history. > > > The various > > receive/transmit descriptor definitions are 99% those defined in > > drivers/net/ethernet/stmmicro/stmmac/descs.h for the normal descriptor. > > That's an interesting observation. I could only assume the vendor did > not want to reinvent the wheel here, but I cannot say if this is a > common design scheme or is something specific to STMicroelectronics > only. I am not entirely sure about it but it looks like it *may* only need to have a glue driver to connect to DWMAC. For instance, on the U-boot[1] side (S700 is one of 64bit OWL SoC from actions), we kind of re-uses already existing DWMAC and provide a glue code, and on the Linux side as well have some similar implementation (locally). Thanks -Amit. [1]: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/dwmac_s700.c
Hi Amit, On Mon, Jun 28, 2021 at 01:55:40PM +0530, Amit Tomer wrote: > Hi, > > > > Do you know the story behind this Ethernet controller? > > > > I just happened to get a board based on the S500 SoC, so I took this > > opportunity to help improving the mainline kernel support, but other > > than that I do not really know much about the hardware history. > > > > > The various > > > receive/transmit descriptor definitions are 99% those defined in > > > drivers/net/ethernet/stmmicro/stmmac/descs.h for the normal descriptor. > > > > That's an interesting observation. I could only assume the vendor did > > not want to reinvent the wheel here, but I cannot say if this is a > > common design scheme or is something specific to STMicroelectronics > > only. > > I am not entirely sure about it but it looks like it *may* only need > to have a glue driver to > connect to DWMAC. From the RX/TX descriptors perspective, this looks like a Synopsys IP, but the MAC register layout is not similar at all. Thanks to Mani, a request for clarification has been also sent to Actions, but they could not confirm. Hence, at the moment, we do not have clear evidences that it is based on Designware. > For instance, on the U-boot[1] side (S700 is one of 64bit OWL SoC from > actions), we kind of re-uses already > existing DWMAC and provide a glue code, and on the Linux side as well > have some similar implementation (locally). The S700 SoC provides Gigabit ethernet capabilities and I assume the controller is quite different from the 10/100 variant present on S500. As a matter of fact, Actions has confirmed that in the case of S700, the licensing was obtained from a third party IP company, although they were not certain if the provider had previous agreements with Synopsys. Regards, Cristi > Thanks > -Amit. > > [1]: https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/dwmac_s700.c