Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
Hi, On Wed, Aug 24, 2016 at 02:02:21PM +0200, LABBE Corentin wrote: > > > +/* Set Management Data Clock, must be call after device reset */ > > > +static void sun8i_emac_set_mdc(struct net_device *ndev) > > > +{ > > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > > > + unsigned long rate; > > > + u32 reg; > > > + > > > + rate = clk_get_rate(priv->ahb_clk); > > > + if (rate > 16000) > > > + reg = 0x3 << 20; /* AHB / 128 */ > > > + else if (rate > 8000) > > > + reg = 0x2 << 20; /* AHB / 64 */ > > > + else if (rate > 4000) > > > + reg = 0x1 << 20; /* AHB / 32 */ > > > + else > > > + reg = 0x0 << 20; /* AHB / 16 */ > > > + netif_dbg(priv, link, ndev, "MDC auto : %x\n", reg); > > > + writel(reg, priv->base + SUN8I_EMAC_MDIO_CMD); > > > > You could also expose that as a clock. > > > > For which purpose ? > No ethernet driver expose the MDC as clock and I dont see any interest: > - I dont think that tuning it give any gain > - Knowing it's value is of little interest You don't have to implement anything, you can just register a clk_div driver, and everything works, and you would use the proper clock APIs (ie. clk_set_rate, and that's it). That would be exposed just like any other clock, including in debugfs, which would remove the need for the debug call. But this really was just a suggestion. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
> > +/* Set Management Data Clock, must be call after device reset */ > > +static void sun8i_emac_set_mdc(struct net_device *ndev) > > +{ > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > > + unsigned long rate; > > + u32 reg; > > + > > + rate = clk_get_rate(priv->ahb_clk); > > + if (rate > 16000) > > + reg = 0x3 << 20; /* AHB / 128 */ > > + else if (rate > 8000) > > + reg = 0x2 << 20; /* AHB / 64 */ > > + else if (rate > 4000) > > + reg = 0x1 << 20; /* AHB / 32 */ > > + else > > + reg = 0x0 << 20; /* AHB / 16 */ > > + netif_dbg(priv, link, ndev, "MDC auto : %x\n", reg); > > + writel(reg, priv->base + SUN8I_EMAC_MDIO_CMD); > > You could also expose that as a clock. > For which purpose ? No ethernet driver expose the MDC as clock and I dont see any interest: - I dont think that tuning it give any gain - Knowing it's value is of little interest Regards
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
On Sat, Jul 30, 2016 at 09:30:01AM +0800, Chen-Yu Tsai wrote: > >> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev) > >> > > +{ > >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > >> > > + u32 reg = 0; > >> > > + > >> > > + if (priv->variant == H3_EMAC) > >> > > + reg = H3_EPHY_DEFAULT_VALUE; > >> > > >> > Why do you need that? > >> > > >> For resetting the syscon to the factory default. > > > > Yes, but does it matter? Does it have any side effect? Is that > > register shared with another device? > > > > Otherwise, either it won't be used anymore, and you don't care, or you > > will reload the driver later, and the driver should work whatever > > state is programmed in there. In both cases, you don't need to reset > > that value. > > The "default" setting also disables and powers down the internal PHY. > I think that's a good thing? The naming could be better. Ah, it might, and that would obviously be the right thing to do. Using a define for those enable and power down bits would be better though. > >> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id) > >> > > +{ > >> > > + struct net_device *ndev = dev_id; > >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > >> > > + u32 v, u; > >> > > + > >> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA); > >> > > + > >> > > + /* When this bit is asserted, a frame transmission is completed. */ > >> > > + if (v & BIT(0)) { > >> > > + priv->estats.tx_int++; > >> > > + writel(0, priv->base + SUN8I_EMAC_INT_EN); > >> > > + napi_schedule(>napi); > >> > > + } > >> > > + > >> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */ > >> > > + if (v & BIT(1)) > >> > > + priv->estats.tx_dma_stop++; > >> > > + > >> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor > >> > > + * and TX DMA FSM is suspended. > >> > > + */ > >> > > + if (v & BIT(2)) > >> > > + priv->estats.tx_dma_ua++; > >> > > + > >> > > + if (v & BIT(3)) > >> > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX > >> > > TIMEOUT\n"); > >> > > >> > Why do you enable that interrupt if you can't handle it? > >> > >> Some interrupt fire even when not enabled (like > >> RX_BUF_UA_INT/TX_BUF_UA_INT) > > > > So the bits 9 and 2, respectively, in the interrupt enable register > > are useless? > > Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just > the interrupt state showing an event? IIRC some other hardware blocks have > this > behavior, such as the timer. That's quite easy to implement, you can do a bitwise and on the status and enable registers. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
On Sat, Jul 30, 2016 at 1:25 AM, Maxime Ripardwrote: > On Thu, Jul 28, 2016 at 04:57:34PM +0200, LABBE Corentin wrote: >> > > +static int sun8i_mdio_write(struct mii_bus *bus, int phy_addr, int >> > > phy_reg, >> > > + u16 data) >> > > +{ >> > > + struct net_device *ndev = bus->priv; >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); >> > > + u32 reg; >> > > + int err; >> > > + >> > > + err = readl_poll_timeout(priv->base + SUN8I_EMAC_MDIO_CMD, reg, >> > > + !(reg & MDIO_CMD_MII_BUSY), 100, 1); >> > > + if (err) { >> > > + dev_err(priv->dev, "%s timeout %x\n", __func__, reg); >> > > + return err; >> > > + } >> > >> > Why the poll_timeout variant? >> > >> Because, in case of bad clock/reset/regulator setting, the value >> expected to come could never be set. > > Ah, I missed that it was for a busy bit, my bad. However, you seem to > be using that on several occasions, maybe you could turn that into a > function? > >> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev) >> > > +{ >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); >> > > + u32 reg = 0; >> > > + >> > > + if (priv->variant == H3_EMAC) >> > > + reg = H3_EPHY_DEFAULT_VALUE; >> > >> > Why do you need that? >> > >> For resetting the syscon to the factory default. > > Yes, but does it matter? Does it have any side effect? Is that > register shared with another device? > > Otherwise, either it won't be used anymore, and you don't care, or you > will reload the driver later, and the driver should work whatever > state is programmed in there. In both cases, you don't need to reset > that value. The "default" setting also disables and powers down the internal PHY. I think that's a good thing? The naming could be better. >> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id) >> > > +{ >> > > + struct net_device *ndev = dev_id; >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); >> > > + u32 v, u; >> > > + >> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA); >> > > + >> > > + /* When this bit is asserted, a frame transmission is completed. */ >> > > + if (v & BIT(0)) { >> > > + priv->estats.tx_int++; >> > > + writel(0, priv->base + SUN8I_EMAC_INT_EN); >> > > + napi_schedule(>napi); >> > > + } >> > > + >> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */ >> > > + if (v & BIT(1)) >> > > + priv->estats.tx_dma_stop++; >> > > + >> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor >> > > + * and TX DMA FSM is suspended. >> > > + */ >> > > + if (v & BIT(2)) >> > > + priv->estats.tx_dma_ua++; >> > > + >> > > + if (v & BIT(3)) >> > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX >> > > TIMEOUT\n"); >> > >> > Why do you enable that interrupt if you can't handle it? >> >> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT) > > So the bits 9 and 2, respectively, in the interrupt enable register > are useless? Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just the interrupt state showing an event? IIRC some other hardware blocks have this behavior, such as the timer. ChenYu >> > And printing in the interrupt handler is a very bad idea. >> >> There are printed only when DEBUG is set, so not a problem ? > > It's always a problem, this adds a very significant latency and will > fill the kernel log buffer at an insane rate, flushing out actual > important messages, for no particular reason. >> > > + >> > > + return IRQ_HANDLED; >> > >> > The lack of spinlocks in there is quite worrying. >> > >> >> The interrupt handler just do nothing harmfull if it race with itself. >> Just stats, enabling NAPI etc.. >> Anyway, It miss a comment for that non-locking strategy > > The interrupt handler cannot race with itself. The interrupts will be > masked on the local CPU and the interrupt can only be delivered to a > single CPU (so, the one that the handler is currently running from). > >> > > +} >> > > + >> > > +static int sun8i_emac_probe(struct platform_device *pdev) >> > > +{ >> > > + struct device_node *node = pdev->dev.of_node; >> > > + struct sun8i_emac_priv *priv; >> > > + struct net_device *ndev; >> > > + struct resource *res; >> > > + int ret; >> > > + >> > > + ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32)); >> > > + if (ret) { >> > > + dev_err(>dev, "No suitable DMA available\n"); >> > > + return ret; >> > > + } >> > >> > Isn't that the default? >> > >> No, it is necessary on arm64 as apritzel requested. > > http://lxr.free-electrons.com/source/drivers/of/device.c#L93 > > It seems to be shared between the two. > > Thanks! > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
On Thu, Jul 28, 2016 at 04:57:34PM +0200, LABBE Corentin wrote: > > > +static int sun8i_mdio_write(struct mii_bus *bus, int phy_addr, int > > > phy_reg, > > > + u16 data) > > > +{ > > > + struct net_device *ndev = bus->priv; > > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > > > + u32 reg; > > > + int err; > > > + > > > + err = readl_poll_timeout(priv->base + SUN8I_EMAC_MDIO_CMD, reg, > > > + !(reg & MDIO_CMD_MII_BUSY), 100, 1); > > > + if (err) { > > > + dev_err(priv->dev, "%s timeout %x\n", __func__, reg); > > > + return err; > > > + } > > > > Why the poll_timeout variant? > > > Because, in case of bad clock/reset/regulator setting, the value > expected to come could never be set. Ah, I missed that it was for a busy bit, my bad. However, you seem to be using that on several occasions, maybe you could turn that into a function? > > > +static void sun8i_emac_unset_syscon(struct net_device *ndev) > > > +{ > > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > > > + u32 reg = 0; > > > + > > > + if (priv->variant == H3_EMAC) > > > + reg = H3_EPHY_DEFAULT_VALUE; > > > > Why do you need that? > > > For resetting the syscon to the factory default. Yes, but does it matter? Does it have any side effect? Is that register shared with another device? Otherwise, either it won't be used anymore, and you don't care, or you will reload the driver later, and the driver should work whatever state is programmed in there. In both cases, you don't need to reset that value. > > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id) > > > +{ > > > + struct net_device *ndev = dev_id; > > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > > > + u32 v, u; > > > + > > > + v = readl(priv->base + SUN8I_EMAC_INT_STA); > > > + > > > + /* When this bit is asserted, a frame transmission is completed. */ > > > + if (v & BIT(0)) { > > > + priv->estats.tx_int++; > > > + writel(0, priv->base + SUN8I_EMAC_INT_EN); > > > + napi_schedule(>napi); > > > + } > > > + > > > + /* When this bit is asserted, the TX DMA FSM is stopped. */ > > > + if (v & BIT(1)) > > > + priv->estats.tx_dma_stop++; > > > + > > > + /* When this asserted, the TX DMA can not acquire next TX descriptor > > > + * and TX DMA FSM is suspended. > > > + */ > > > + if (v & BIT(2)) > > > + priv->estats.tx_dma_ua++; > > > + > > > + if (v & BIT(3)) > > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIMEOUT\n"); > > > > Why do you enable that interrupt if you can't handle it? > > Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT) So the bits 9 and 2, respectively, in the interrupt enable register are useless? > > And printing in the interrupt handler is a very bad idea. > > There are printed only when DEBUG is set, so not a problem ? It's always a problem, this adds a very significant latency and will fill the kernel log buffer at an insane rate, flushing out actual important messages, for no particular reason. > > > + > > > + return IRQ_HANDLED; > > > > The lack of spinlocks in there is quite worrying. > > > > The interrupt handler just do nothing harmfull if it race with itself. > Just stats, enabling NAPI etc.. > Anyway, It miss a comment for that non-locking strategy The interrupt handler cannot race with itself. The interrupts will be masked on the local CPU and the interrupt can only be delivered to a single CPU (so, the one that the handler is currently running from). > > > +} > > > + > > > +static int sun8i_emac_probe(struct platform_device *pdev) > > > +{ > > > + struct device_node *node = pdev->dev.of_node; > > > + struct sun8i_emac_priv *priv; > > > + struct net_device *ndev; > > > + struct resource *res; > > > + int ret; > > > + > > > + ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32)); > > > + if (ret) { > > > + dev_err(>dev, "No suitable DMA available\n"); > > > + return ret; > > > + } > > > > Isn't that the default? > > > No, it is necessary on arm64 as apritzel requested. http://lxr.free-electrons.com/source/drivers/of/device.c#L93 It seems to be shared between the two. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com signature.asc Description: PGP signature
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
Hi, On 25/07/16 20:54, Maxime Ripard wrote: > On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote: >> This patch add support for sun8i-emac ethernet MAC hardware. >> It could be found in Allwinner H3/A83T/A64 SoCs. >> >> It supports 10/100/1000 Mbit/s speed with half/full duplex. >> It can use an internal PHY (MII 10/100) or an external PHY >> via RGMII/RMII. >> >> Signed-off-by: LABBE Corentin>> --- >> drivers/net/ethernet/allwinner/Kconfig | 13 + >> drivers/net/ethernet/allwinner/Makefile |1 + >> drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 >> +++ >> 3 files changed, 2143 insertions(+) >> create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c ... >> diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c >> b/drivers/net/ethernet/allwinner/sun8i-emac.c >> new file mode 100644 >> index 000..fc0c1dd >> --- /dev/null >> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c ... >> + >> +/* struct dma_desc - Structure of DMA descriptor used by the hardware >> + * @status: Status of the frame written by HW, so RO for the >> + * driver (except for BIT(31) which is R/W) >> + * @ctl: Information on the frame written by the driver (INT, len,...) >> + * @buf_addr: physical address of the frame data >> + * @next: physical address of next dma_desc >> + */ >> +struct dma_desc { >> +u32 status; >> +u32 ctl; >> +u32 buf_addr; >> +u32 next; >> +}; > > You should use the endian-aware variants here. For the records: just doing the sparse annotation with __le32 here will of course not be sufficient to make it work on BE kernels. I added proper endianness conversion to all accesses to the descriptors and got it to work with an arm64 big-endian kernel on the Pine64. I put a patch here: https://gist.github.com/apritzel/bc792c4dbbd8789f5f18aef538e8c440 This particular version is untested (though it compiles), since I just adapted the working patch against the newer driver code and couldn't test it yet. I am not really an endianness expert, so don't know if there are smarter ways to tackle this, if we should for instance provide access wrappers to the DMA descriptor fields. I will try to test this later today, if that works, feel free to merge those changes into your driver. Cheers, Andre.
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
On Thursday, July 28, 2016 3:18:26 PM CEST LABBE Corentin wrote: > > I will reworked locking and it seems that no locking is necessary. > I have added the following comment about the locking strategy: > > /* Locking strategy: > * RX queue does not need any lock since only sun8i_emac_poll() access it. > * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and so > sun8i_emac_poll()) > * TX queue is handled by sun8i_emac_xmit(), sun8i_emac_complete_xmit() and > sun8i_emac_tx_timeout() > * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and stop queue) > * > * sun8i_emac_xmit() could fire only once (netif_tx_lock) > * sun8i_emac_complete_xmit() could fire only once (called from NAPI) > * sun8i_emac_tx_timeout() could fire only once (netif_tx_lock) and couldnt > * race with sun8i_emac_xmit (due to netif_tx_lock) and with > sun8i_emac_complete_xmit which disable NAPI. > * > * So only sun8i_emac_xmit and sun8i_emac_complete_xmit could fire at the > same time. > * But they never could modify the same descriptors: > * - sun8i_emac_complete_xmit() will modify only descriptors with empty status > * - sun8i_emac_xmit() will modify only descriptors set to DCLEAN > * Proper memory barriers ensure that descriptor set to DCLEAN could not be > * modified latter by sun8i_emac_complete_xmit(). > * */ Sounds good, the comment is certainly very helpful here. Arnd
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
On Mon, Jul 25, 2016 at 09:54:55PM +0200, Maxime Ripard wrote: > On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote: > > This patch add support for sun8i-emac ethernet MAC hardware. > > It could be found in Allwinner H3/A83T/A64 SoCs. > > > > It supports 10/100/1000 Mbit/s speed with half/full duplex. > > It can use an internal PHY (MII 10/100) or an external PHY > > via RGMII/RMII. > > > > Signed-off-by: LABBE Corentin> > --- > > drivers/net/ethernet/allwinner/Kconfig | 13 + > > drivers/net/ethernet/allwinner/Makefile |1 + > > drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 > > +++ > > 3 files changed, 2143 insertions(+) > > create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c > > > > diff --git a/drivers/net/ethernet/allwinner/Kconfig > > b/drivers/net/ethernet/allwinner/Kconfig > > index 47da7e7..060569c 100644 > > --- a/drivers/net/ethernet/allwinner/Kconfig > > +++ b/drivers/net/ethernet/allwinner/Kconfig > > @@ -33,4 +33,17 @@ config SUN4I_EMAC > >To compile this driver as a module, choose M here. The module > >will be called sun4i-emac. > > > > +config SUN8I_EMAC > > + tristate "Allwinner sun8i EMAC support" > > + depends on ARCH_SUNXI || COMPILE_TEST > > + depends on OF > > + select MII > > + select PHYLIB > > +---help--- > > + This driver support the sun8i EMAC ethernet driver present on > > + H3/A83T/A64 Allwinner SoCs. > > + > > + To compile this driver as a module, choose M here. The module > > + will be called sun8i-emac. > > + > > endif # NET_VENDOR_ALLWINNER > > diff --git a/drivers/net/ethernet/allwinner/Makefile > > b/drivers/net/ethernet/allwinner/Makefile > > index 03129f7..8bd1693c 100644 > > --- a/drivers/net/ethernet/allwinner/Makefile > > +++ b/drivers/net/ethernet/allwinner/Makefile > > @@ -3,3 +3,4 @@ > > # > > > > obj-$(CONFIG_SUN4I_EMAC) += sun4i-emac.o > > +obj-$(CONFIG_SUN8I_EMAC) += sun8i-emac.o > > diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c > > b/drivers/net/ethernet/allwinner/sun8i-emac.c > > new file mode 100644 > > index 000..fc0c1dd > > --- /dev/null > > +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c > > @@ -0,0 +1,2129 @@ > > +/* > > + * sun8i-emac driver > > + * > > + * Copyright (C) 2015-2016 Corentin LABBE > > + * > > + * This is the driver for Allwinner Ethernet MAC found in H3/A83T/A64 SoC > > + * > > + * TODO: > > + * - MAC filtering > > + * - Jumbo frame > > + * - features rx-all (NETIF_F_RXALL_BIT) > > + */ > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define SUN8I_EMAC_BASIC_CTL0 0x00 > > +#define SUN8I_EMAC_BASIC_CTL1 0x04 > > +#define SUN8I_EMAC_INT_STA 0x08 > > +#define SUN8I_EMAC_INT_EN 0x0C > > +#define SUN8I_EMAC_TX_CTL0 0x10 > > +#define SUN8I_EMAC_TX_CTL1 0x14 > > +#define SUN8I_EMAC_TX_FLOW_CTL 0x1C > > +#define SUN8I_EMAC_RX_CTL0 0x24 > > +#define SUN8I_EMAC_RX_CTL1 0x28 > > +#define SUN8I_EMAC_RX_FRM_FLT 0x38 > > +#define SUN8I_EMAC_MDIO_CMD0x48 > > +#define SUN8I_EMAC_MDIO_DATA 0x4C > > +#define SUN8I_EMAC_TX_DMA_STA 0xB0 > > +#define SUN8I_EMAC_TX_CUR_DESC 0xB4 > > +#define SUN8I_EMAC_TX_CUR_BUF 0xB8 > > +#define SUN8I_EMAC_RX_DMA_STA 0xC0 > > + > > +#define MDIO_CMD_MII_BUSY BIT(0) > > +#define MDIO_CMD_MII_WRITE BIT(1) > > +#define MDIO_CMD_MII_PHY_REG_ADDR_MASK GENMASK(8, 4) > > +#define MDIO_CMD_MII_PHY_REG_ADDR_SHIFT4 > > +#define MDIO_CMD_MII_PHY_ADDR_MASK GENMASK(16, 12) > > +#define MDIO_CMD_MII_PHY_ADDR_SHIFT12 > > + > > +#define SUN8I_EMAC_MACADDR_HI 0x50 > > +#define SUN8I_EMAC_MACADDR_LO 0x54 > > + > > +#define SUN8I_EMAC_RX_DESC_LIST 0x34 > > +#define SUN8I_EMAC_TX_DESC_LIST 0x20 > > + > > +#define SUN8I_EMAC_RX_DO_CRC BIT(27) > > +#define SUN8I_EMAC_RX_STRIP_FCS BIT(28) > > + > > +#define SUN8I_COULD_BE_USED_BY_DMA BIT(31) > > + > > +/* Used in RX_CTL1*/ > > +#define RX_DMA_EN BIT(30) > > +#define RX_DMA_START BIT(31) > > +/* Used in TX_CTL1*/ > > +#define TX_DMA_EN BIT(30) > > +#define TX_DMA_START BIT(31) > > + > > +/* Used in RX_CTL0 */ > > +#define RX_RECEIVER_EN BIT(31) > > +/* Used in TX_CTL0 */ > > +#define TX_TRANSMITTER_EN BIT(31) > > + > > +/* Basic CTL0 */ > > +#define BCTL0_FD BIT(0) > > +#define BCTL0_SPEED_10 2 > > +#define BCTL0_SPEED_1003 > > +#define BCTL0_SPEED_MASK GENMASK(3, 2) > > +#define BCTL0_SPEED_SHIFT 2 > > + > > +#define FLOW_RX 1 > > +#define FLOW_TX 2 > > + > > +#define RX_INT BIT(8) > > +#define TX_INT BIT(0) > > + > > +/* Bits used in frame RX status */ > > +#define
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
On Wed, Jul 20, 2016 at 11:56:12AM +0200, Arnd Bergmann wrote: > On Wednesday, July 20, 2016 10:03:16 AM CEST LABBE Corentin wrote: > > + > > + /* Benched on OPIPC with 100M, setting more than 256 does not give > > any > > +* perf boost > > +*/ > > + priv->nbdesc_rx = 128; > > + priv->nbdesc_tx = 256; > > + > > > > 256 tx descriptors can introduce a significant latency. Can you add > support for BQL (netdev_sent_queue/netdev_completed_queue) to limit > the queue size to the minimum? Done, since setting below 256 give lower performance with iperf. > > I also noticed that your tx_lock() prevents you from concurrently > running sun8i_emac_complete_xmit() and sun8i_emac_xmit(). Is that > necessary? I'd think that you can find a way to make them work > concurrently. > > Arnd I will reworked locking and it seems that no locking is necessary. I have added the following comment about the locking strategy: /* Locking strategy: * RX queue does not need any lock since only sun8i_emac_poll() access it. * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and so sun8i_emac_poll()) * TX queue is handled by sun8i_emac_xmit(), sun8i_emac_complete_xmit() and sun8i_emac_tx_timeout() * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and stop queue) * * sun8i_emac_xmit() could fire only once (netif_tx_lock) * sun8i_emac_complete_xmit() could fire only once (called from NAPI) * sun8i_emac_tx_timeout() could fire only once (netif_tx_lock) and couldnt * race with sun8i_emac_xmit (due to netif_tx_lock) and with sun8i_emac_complete_xmit which disable NAPI. * * So only sun8i_emac_xmit and sun8i_emac_complete_xmit could fire at the same time. * But they never could modify the same descriptors: * - sun8i_emac_complete_xmit() will modify only descriptors with empty status * - sun8i_emac_xmit() will modify only descriptors set to DCLEAN * Proper memory barriers ensure that descriptor set to DCLEAN could not be * modified latter by sun8i_emac_complete_xmit(). * */ Does I am right ? Thanks for your review. Regards LABBE Corentin
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote: > This patch add support for sun8i-emac ethernet MAC hardware. > It could be found in Allwinner H3/A83T/A64 SoCs. > > It supports 10/100/1000 Mbit/s speed with half/full duplex. > It can use an internal PHY (MII 10/100) or an external PHY > via RGMII/RMII. > > Signed-off-by: LABBE Corentin> --- > drivers/net/ethernet/allwinner/Kconfig | 13 + > drivers/net/ethernet/allwinner/Makefile |1 + > drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 > +++ > 3 files changed, 2143 insertions(+) > create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c > > diff --git a/drivers/net/ethernet/allwinner/Kconfig > b/drivers/net/ethernet/allwinner/Kconfig > index 47da7e7..060569c 100644 > --- a/drivers/net/ethernet/allwinner/Kconfig > +++ b/drivers/net/ethernet/allwinner/Kconfig > @@ -33,4 +33,17 @@ config SUN4I_EMAC >To compile this driver as a module, choose M here. The module >will be called sun4i-emac. > > +config SUN8I_EMAC > + tristate "Allwinner sun8i EMAC support" > + depends on ARCH_SUNXI || COMPILE_TEST > + depends on OF > + select MII > + select PHYLIB > +---help--- > + This driver support the sun8i EMAC ethernet driver present on > + H3/A83T/A64 Allwinner SoCs. > + > + To compile this driver as a module, choose M here. The module > + will be called sun8i-emac. > + > endif # NET_VENDOR_ALLWINNER > diff --git a/drivers/net/ethernet/allwinner/Makefile > b/drivers/net/ethernet/allwinner/Makefile > index 03129f7..8bd1693c 100644 > --- a/drivers/net/ethernet/allwinner/Makefile > +++ b/drivers/net/ethernet/allwinner/Makefile > @@ -3,3 +3,4 @@ > # > > obj-$(CONFIG_SUN4I_EMAC) += sun4i-emac.o > +obj-$(CONFIG_SUN8I_EMAC) += sun8i-emac.o > diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c > b/drivers/net/ethernet/allwinner/sun8i-emac.c > new file mode 100644 > index 000..fc0c1dd > --- /dev/null > +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c > @@ -0,0 +1,2129 @@ > +/* > + * sun8i-emac driver > + * > + * Copyright (C) 2015-2016 Corentin LABBE > + * > + * This is the driver for Allwinner Ethernet MAC found in H3/A83T/A64 SoC > + * > + * TODO: > + * - MAC filtering > + * - Jumbo frame > + * - features rx-all (NETIF_F_RXALL_BIT) > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define SUN8I_EMAC_BASIC_CTL00x00 > +#define SUN8I_EMAC_BASIC_CTL10x04 > +#define SUN8I_EMAC_INT_STA 0x08 > +#define SUN8I_EMAC_INT_EN0x0C > +#define SUN8I_EMAC_TX_CTL0 0x10 > +#define SUN8I_EMAC_TX_CTL1 0x14 > +#define SUN8I_EMAC_TX_FLOW_CTL 0x1C > +#define SUN8I_EMAC_RX_CTL0 0x24 > +#define SUN8I_EMAC_RX_CTL1 0x28 > +#define SUN8I_EMAC_RX_FRM_FLT0x38 > +#define SUN8I_EMAC_MDIO_CMD 0x48 > +#define SUN8I_EMAC_MDIO_DATA 0x4C > +#define SUN8I_EMAC_TX_DMA_STA0xB0 > +#define SUN8I_EMAC_TX_CUR_DESC 0xB4 > +#define SUN8I_EMAC_TX_CUR_BUF0xB8 > +#define SUN8I_EMAC_RX_DMA_STA0xC0 > + > +#define MDIO_CMD_MII_BUSYBIT(0) > +#define MDIO_CMD_MII_WRITE BIT(1) > +#define MDIO_CMD_MII_PHY_REG_ADDR_MASK GENMASK(8, 4) > +#define MDIO_CMD_MII_PHY_REG_ADDR_SHIFT 4 > +#define MDIO_CMD_MII_PHY_ADDR_MASK GENMASK(16, 12) > +#define MDIO_CMD_MII_PHY_ADDR_SHIFT 12 > + > +#define SUN8I_EMAC_MACADDR_HI0x50 > +#define SUN8I_EMAC_MACADDR_LO0x54 > + > +#define SUN8I_EMAC_RX_DESC_LIST 0x34 > +#define SUN8I_EMAC_TX_DESC_LIST 0x20 > + > +#define SUN8I_EMAC_RX_DO_CRC BIT(27) > +#define SUN8I_EMAC_RX_STRIP_FCS BIT(28) > + > +#define SUN8I_COULD_BE_USED_BY_DMA BIT(31) > + > +/* Used in RX_CTL1*/ > +#define RX_DMA_ENBIT(30) > +#define RX_DMA_START BIT(31) > +/* Used in TX_CTL1*/ > +#define TX_DMA_ENBIT(30) > +#define TX_DMA_START BIT(31) > + > +/* Used in RX_CTL0 */ > +#define RX_RECEIVER_EN BIT(31) > +/* Used in TX_CTL0 */ > +#define TX_TRANSMITTER_ENBIT(31) > + > +/* Basic CTL0 */ > +#define BCTL0_FD BIT(0) > +#define BCTL0_SPEED_10 2 > +#define BCTL0_SPEED_100 3 > +#define BCTL0_SPEED_MASK GENMASK(3, 2) > +#define BCTL0_SPEED_SHIFT2 > + > +#define FLOW_RX 1 > +#define FLOW_TX 2 > + > +#define RX_INT BIT(8) > +#define TX_INT BIT(0) > + > +/* Bits used in frame RX status */ > +#define DSC_RX_FIRST BIT(9) > +#define DSC_RX_LAST BIT(8) > + > +/* Bits used in frame TX ctl */ > +#define SUN8I_EMAC_MAGIC_TX_BIT BIT(24) > +#define SUN8I_EMAC_TX_DO_CRC (BIT(27) | BIT(28)) > +#define DSC_TX_FIRST BIT(29) > +#define DSC_TX_LAST BIT(30) > +#define SUN8I_EMAC_WANT_INT
Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver
On Wednesday, July 20, 2016 10:03:16 AM CEST LABBE Corentin wrote: > + > + /* Benched on OPIPC with 100M, setting more than 256 does not give any > +* perf boost > +*/ > + priv->nbdesc_rx = 128; > + priv->nbdesc_tx = 256; > + > 256 tx descriptors can introduce a significant latency. Can you add support for BQL (netdev_sent_queue/netdev_completed_queue) to limit the queue size to the minimum? I also noticed that your tx_lock() prevents you from concurrently running sun8i_emac_complete_xmit() and sun8i_emac_xmit(). Is that necessary? I'd think that you can find a way to make them work concurrently. Arnd
[PATCH v2 1/5] ethernet: add sun8i-emac driver
This patch add support for sun8i-emac ethernet MAC hardware. It could be found in Allwinner H3/A83T/A64 SoCs. It supports 10/100/1000 Mbit/s speed with half/full duplex. It can use an internal PHY (MII 10/100) or an external PHY via RGMII/RMII. Signed-off-by: LABBE Corentin--- drivers/net/ethernet/allwinner/Kconfig | 13 + drivers/net/ethernet/allwinner/Makefile |1 + drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 +++ 3 files changed, 2143 insertions(+) create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c diff --git a/drivers/net/ethernet/allwinner/Kconfig b/drivers/net/ethernet/allwinner/Kconfig index 47da7e7..060569c 100644 --- a/drivers/net/ethernet/allwinner/Kconfig +++ b/drivers/net/ethernet/allwinner/Kconfig @@ -33,4 +33,17 @@ config SUN4I_EMAC To compile this driver as a module, choose M here. The module will be called sun4i-emac. +config SUN8I_EMAC + tristate "Allwinner sun8i EMAC support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on OF + select MII + select PHYLIB +---help--- + This driver support the sun8i EMAC ethernet driver present on + H3/A83T/A64 Allwinner SoCs. + + To compile this driver as a module, choose M here. The module + will be called sun8i-emac. + endif # NET_VENDOR_ALLWINNER diff --git a/drivers/net/ethernet/allwinner/Makefile b/drivers/net/ethernet/allwinner/Makefile index 03129f7..8bd1693c 100644 --- a/drivers/net/ethernet/allwinner/Makefile +++ b/drivers/net/ethernet/allwinner/Makefile @@ -3,3 +3,4 @@ # obj-$(CONFIG_SUN4I_EMAC) += sun4i-emac.o +obj-$(CONFIG_SUN8I_EMAC) += sun8i-emac.o diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c b/drivers/net/ethernet/allwinner/sun8i-emac.c new file mode 100644 index 000..fc0c1dd --- /dev/null +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c @@ -0,0 +1,2129 @@ +/* + * sun8i-emac driver + * + * Copyright (C) 2015-2016 Corentin LABBE + * + * This is the driver for Allwinner Ethernet MAC found in H3/A83T/A64 SoC + * + * TODO: + * - MAC filtering + * - Jumbo frame + * - features rx-all (NETIF_F_RXALL_BIT) + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define SUN8I_EMAC_BASIC_CTL0 0x00 +#define SUN8I_EMAC_BASIC_CTL1 0x04 +#define SUN8I_EMAC_INT_STA 0x08 +#define SUN8I_EMAC_INT_EN 0x0C +#define SUN8I_EMAC_TX_CTL0 0x10 +#define SUN8I_EMAC_TX_CTL1 0x14 +#define SUN8I_EMAC_TX_FLOW_CTL 0x1C +#define SUN8I_EMAC_RX_CTL0 0x24 +#define SUN8I_EMAC_RX_CTL1 0x28 +#define SUN8I_EMAC_RX_FRM_FLT 0x38 +#define SUN8I_EMAC_MDIO_CMD0x48 +#define SUN8I_EMAC_MDIO_DATA 0x4C +#define SUN8I_EMAC_TX_DMA_STA 0xB0 +#define SUN8I_EMAC_TX_CUR_DESC 0xB4 +#define SUN8I_EMAC_TX_CUR_BUF 0xB8 +#define SUN8I_EMAC_RX_DMA_STA 0xC0 + +#define MDIO_CMD_MII_BUSY BIT(0) +#define MDIO_CMD_MII_WRITE BIT(1) +#define MDIO_CMD_MII_PHY_REG_ADDR_MASK GENMASK(8, 4) +#define MDIO_CMD_MII_PHY_REG_ADDR_SHIFT4 +#define MDIO_CMD_MII_PHY_ADDR_MASK GENMASK(16, 12) +#define MDIO_CMD_MII_PHY_ADDR_SHIFT12 + +#define SUN8I_EMAC_MACADDR_HI 0x50 +#define SUN8I_EMAC_MACADDR_LO 0x54 + +#define SUN8I_EMAC_RX_DESC_LIST 0x34 +#define SUN8I_EMAC_TX_DESC_LIST 0x20 + +#define SUN8I_EMAC_RX_DO_CRC BIT(27) +#define SUN8I_EMAC_RX_STRIP_FCS BIT(28) + +#define SUN8I_COULD_BE_USED_BY_DMA BIT(31) + +/* Used in RX_CTL1*/ +#define RX_DMA_EN BIT(30) +#define RX_DMA_START BIT(31) +/* Used in TX_CTL1*/ +#define TX_DMA_EN BIT(30) +#define TX_DMA_START BIT(31) + +/* Used in RX_CTL0 */ +#define RX_RECEIVER_EN BIT(31) +/* Used in TX_CTL0 */ +#define TX_TRANSMITTER_EN BIT(31) + +/* Basic CTL0 */ +#define BCTL0_FD BIT(0) +#define BCTL0_SPEED_10 2 +#define BCTL0_SPEED_1003 +#define BCTL0_SPEED_MASK GENMASK(3, 2) +#define BCTL0_SPEED_SHIFT 2 + +#define FLOW_RX 1 +#define FLOW_TX 2 + +#define RX_INT BIT(8) +#define TX_INT BIT(0) + +/* Bits used in frame RX status */ +#define DSC_RX_FIRST BIT(9) +#define DSC_RX_LASTBIT(8) + +/* Bits used in frame TX ctl */ +#define SUN8I_EMAC_MAGIC_TX_BITBIT(24) +#define SUN8I_EMAC_TX_DO_CRC (BIT(27) | BIT(28)) +#define DSC_TX_FIRST BIT(29) +#define DSC_TX_LASTBIT(30) +#define SUN8I_EMAC_WANT_INTBIT(31) + +enum emac_variant { + NONE_EMAC,/* for be sure that variant is non-0 if set */ + A83T_EMAC, + H3_EMAC, + A64_EMAC, +}; + +static const char const estats_str[][ETH_GSTRING_LEN] = { + /* errors */ + "rx_payload_error", + "rx_CRC_error", + "rx_phy_error", + "rx_length_error", + "rx_col_error", + "rx_header_error", +