Re: [PATCH 1/5] ethernet: add sun8i-emac driver

2016-06-12 Thread Florian Fainelli 
Le 09/06/2016 02:44, LABBE Corentin a écrit :
> Hello
> 
> I agree to all your comments, but for some I have additionnal questions
> 
> On Mon, Jun 06, 2016 at 11:25:15AM -0700, Florian Fainelli wrote:
>> On 06/03/2016 02:56 AM, LABBE Corentin wrote:
>>
>> [snip]
>>
>>> +
>>> +/* The datasheet said that each descriptor can transfers up to 4096bytes
>>> + * But latter, a register documentation reduce that value to 2048
>>> + * Anyway using 2048 cause strange behaviours and even BSP driver use 2047
>>> + */
>>> +#define DESC_BUF_MAX 2044
>>> +#if (DESC_BUF_MAX < (ETH_FRAME_LEN + 4))
>>> +#error "DESC_BUF_MAX must be set at minimum to ETH_FRAME_LEN + 4"
>>> +#endif
>>
>> You can probably drop that, it would not make much sense to enable
>> fragments and a buffer size smaller than ETH_FRAME_LEN + ETH_FCS_LEN anyway.
>>
> 
> I has added this test for preventing someone who want to "optimize" 
> DESC_BUF_MAX to doing mistake.
> But I agree that it is of low use.

It's actually dangerous, and if you don't make sure that the value is
properly rounded to whatever the DMA controller's alignment should be,
performance could be terribel too.

> 
>>> +/* Return the number of contiguous free descriptors
>>> + * starting from tx_slot
>>> + */
>>> +static int rb_tx_numfreedesc(struct net_device *ndev)
>>> +{
>>> +   struct sun8i_emac_priv *priv = netdev_priv(ndev);
>>> +
>>> +   if (priv->tx_slot < priv->tx_dirty)
>>> +   return priv->tx_dirty - priv->tx_slot;
>>
>> Does this work with if tx_dirty wraps around?
>>
> 
> The tx_dirty cannot wrap since I always keep an empty slot. (tx_slot cannot 
> go equal or after tx_dirty)

OK, fair enough.

> 
>>> +/* Grab a frame into a skb from descriptor number i */
>>> +static int sun8i_emac_rx_from_ddesc(struct net_device *ndev, int i)
>>> +{
>>> +   struct sk_buff *skb;
>>> +   struct sun8i_emac_priv *priv = netdev_priv(ndev);
>>> +   struct dma_desc *ddesc = priv->dd_rx + i;
>>> +   int frame_len;
>>> +   int crc_checked = 0;
>>> +
>>> +   if (ndev->features & NETIF_F_RXCSUM)
>>> +   crc_checked = 1;
>>
>> Assuming CRC here refers to the Ethernet frame's FCS, then this is
>> absolutely not how NETIF_F_RXCSUM works. NETIF_F_RXCSUM is about your
>> Ethernet adapter supporting L3/L4 checksum offloads, while the Ethernet
>> FCS is pretty much mandatory for the frame to be properly received in
>> the first place. Can you clarify which way it is?
>>
> 
> No CRC here is RXCSUM. I understand the misnaming.
> I will rename the variable to rxcsum_done.

Thanks

> 
>>> +
>>> +   priv->ndev->stats.rx_packets++;
>>> +   priv->ndev->stats.rx_bytes += frame_len;
>>> +   priv->rx_sk[i] = NULL;
>>> +
>>> +   /* this frame is not the last */
>>> +   if ((ddesc->status & BIT(8)) == 0) {
>>> +   dev_warn(priv->dev, "Multi frame not implemented currlen=%d\n",
>>> +frame_len);
>>> +   }
>>> +
>>> +   sun8i_emac_rx_sk(ndev, i);
>>> +
>>> +   netif_rx(skb);
>>
>> netif_receive_skb() at the very least, or if you implement NAPI, like
>> you shoud napi_gro_receive().
>>
> 
> netif_receive_skb documentation say
> "This function may only be called from softirq context and interrupts should 
> be enabled."
> but the calling functions is in hardirq context.

Well, yes, because you are not implementing NAPI, while you should, you
execute in hard interrupt context. Once you move to NAPI, you can and
should use netif_receive_skb().

> 
>>> +   return 0;
>>> +}
>>> +
>>> +/* Cycle over RX DMA descriptors for finding frame to receive
>>> + */
>>> +static int sun8i_emac_receive_all(struct net_device *ndev)
>>> +{
>>> +   struct sun8i_emac_priv *priv = netdev_priv(ndev);
>>> +   struct dma_desc *ddesc;
>>> +
>>> +   ddesc = priv->dd_rx + priv->rx_dirty;
>>> +   while (!(ddesc->status & BIT(31))) {
>>> +   sun8i_emac_rx_from_ddesc(ndev, priv->rx_dirty);
>>> +   rb_inc(>rx_dirty, nbdesc_rx);
>>> +   ddesc = priv->dd_rx + priv->rx_dirty;
>>> +   };
>>
>> So, what if we ping flood your device here, is not there a remote chance
>> that we keep the RX interrupt so busy we can't break out of this loop,
>> and we are executing from hard IRQ context, that's bad.
>>
> 
> I have added a start variable for preventing to do more than a full loop.

Which gets you close to a proper NAPI implementation, good.

> 
>>> +
>>> +   return 0;
>>> +}
>>> +
>>> +/* iterate over dma_desc for finding completed xmit.
>>> + * Called from interrupt context, so no need to spinlock tx
>>
>> Humm, well it depends if what you are doing here may race with
>> ndo_start_xmit(), and usually it does.
>>
> 
> I believe that how it is designed it cannot race each over (access the same 
> descriptor slot) since I keep a free slot between each other.
> 
>> Also, you should consider completing TX packets in NAPI context (soft
>> IRQ) instead of hard IRQs like here.
>>
> 
> I wanted to finish this driver the "old" way (with hard IRQ) and implementing 
> NAPI after as a Kconfig choice.
> Does

Re: [PATCH 1/5] ethernet: add sun8i-emac driver

2016-06-09 Thread LABBE Corentin 
Hello

I agree to all your comments, but for some I have additionnal questions

On Mon, Jun 06, 2016 at 11:25:15AM -0700, Florian Fainelli wrote:
> On 06/03/2016 02:56 AM, LABBE Corentin wrote:
> 
> [snip]
> 
> > +
> > +/* The datasheet said that each descriptor can transfers up to 4096bytes
> > + * But latter, a register documentation reduce that value to 2048
> > + * Anyway using 2048 cause strange behaviours and even BSP driver use 2047
> > + */
> > +#define DESC_BUF_MAX 2044
> > +#if (DESC_BUF_MAX < (ETH_FRAME_LEN + 4))
> > +#error "DESC_BUF_MAX must be set at minimum to ETH_FRAME_LEN + 4"
> > +#endif
> 
> You can probably drop that, it would not make much sense to enable
> fragments and a buffer size smaller than ETH_FRAME_LEN + ETH_FCS_LEN anyway.
> 

I has added this test for preventing someone who want to "optimize" 
DESC_BUF_MAX to doing mistake.
But I agree that it is of low use.

> > +/* Return the number of contiguous free descriptors
> > + * starting from tx_slot
> > + */
> > +static int rb_tx_numfreedesc(struct net_device *ndev)
> > +{
> > +   struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > +
> > +   if (priv->tx_slot < priv->tx_dirty)
> > +   return priv->tx_dirty - priv->tx_slot;
> 
> Does this work with if tx_dirty wraps around?
> 

The tx_dirty cannot wrap since I always keep an empty slot. (tx_slot cannot go 
equal or after tx_dirty)

> > +/* Grab a frame into a skb from descriptor number i */
> > +static int sun8i_emac_rx_from_ddesc(struct net_device *ndev, int i)
> > +{
> > +   struct sk_buff *skb;
> > +   struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > +   struct dma_desc *ddesc = priv->dd_rx + i;
> > +   int frame_len;
> > +   int crc_checked = 0;
> > +
> > +   if (ndev->features & NETIF_F_RXCSUM)
> > +   crc_checked = 1;
> 
> Assuming CRC here refers to the Ethernet frame's FCS, then this is
> absolutely not how NETIF_F_RXCSUM works. NETIF_F_RXCSUM is about your
> Ethernet adapter supporting L3/L4 checksum offloads, while the Ethernet
> FCS is pretty much mandatory for the frame to be properly received in
> the first place. Can you clarify which way it is?
> 

No CRC here is RXCSUM. I understand the misnaming.
I will rename the variable to rxcsum_done.

> > +
> > +   priv->ndev->stats.rx_packets++;
> > +   priv->ndev->stats.rx_bytes += frame_len;
> > +   priv->rx_sk[i] = NULL;
> > +
> > +   /* this frame is not the last */
> > +   if ((ddesc->status & BIT(8)) == 0) {
> > +   dev_warn(priv->dev, "Multi frame not implemented currlen=%d\n",
> > +frame_len);
> > +   }
> > +
> > +   sun8i_emac_rx_sk(ndev, i);
> > +
> > +   netif_rx(skb);
> 
> netif_receive_skb() at the very least, or if you implement NAPI, like
> you shoud napi_gro_receive().
> 

netif_receive_skb documentation say
"This function may only be called from softirq context and interrupts should be 
enabled."
but the calling functions is in hardirq context.

> > +   return 0;
> > +}
> > +
> > +/* Cycle over RX DMA descriptors for finding frame to receive
> > + */
> > +static int sun8i_emac_receive_all(struct net_device *ndev)
> > +{
> > +   struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > +   struct dma_desc *ddesc;
> > +
> > +   ddesc = priv->dd_rx + priv->rx_dirty;
> > +   while (!(ddesc->status & BIT(31))) {
> > +   sun8i_emac_rx_from_ddesc(ndev, priv->rx_dirty);
> > +   rb_inc(>rx_dirty, nbdesc_rx);
> > +   ddesc = priv->dd_rx + priv->rx_dirty;
> > +   };
> 
> So, what if we ping flood your device here, is not there a remote chance
> that we keep the RX interrupt so busy we can't break out of this loop,
> and we are executing from hard IRQ context, that's bad.
> 

I have added a start variable for preventing to do more than a full loop.

> > +
> > +   return 0;
> > +}
> > +
> > +/* iterate over dma_desc for finding completed xmit.
> > + * Called from interrupt context, so no need to spinlock tx
> 
> Humm, well it depends if what you are doing here may race with
> ndo_start_xmit(), and usually it does.
> 

I believe that how it is designed it cannot race each over (access the same 
descriptor slot) since I keep a free slot between each other.

> Also, you should consider completing TX packets in NAPI context (soft
> IRQ) instead of hard IRQs like here.
> 

I wanted to finish this driver the "old" way (with hard IRQ) and implementing 
NAPI after as a Kconfig choice.
Does NAPI is mandatory now ? (or really recommended)
For resuming my understanding, NAPI is good when expecting high traffic. (so my 
Kconfig idea)
If you say that NAPI is really preferable, I will do it.

> > +   /* last descriptor point back to first one */
> > +   ddesc--;
> > +   ddesc->next = (u32)priv->dd_rx_phy;
> 
> So is there a limitation of this hardware that can only do DMA within
> the first 4GB of the system?
> 

Yes, I have added all DMA stuff for handling that after apritzel review.

> > +static int sun8i_emac_check_if_running(struct net_device *ndev)
> > +{
> >

Re: [PATCH 1/5] ethernet: add sun8i-emac driver

2016-06-06 Thread Florian Fainelli 
On 06/03/2016 02:56 AM, 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 
> ---

[snip]

> +
> +struct ethtool_str {
> + char name[ETH_GSTRING_LEN];

You might as well drop the encapsulating structure and just use an array
of strings?

> +};
> +

[snip]

> +
> +/* The datasheet said that each descriptor can transfers up to 4096bytes
> + * But latter, a register documentation reduce that value to 2048
> + * Anyway using 2048 cause strange behaviours and even BSP driver use 2047
> + */
> +#define DESC_BUF_MAX 2044
> +#if (DESC_BUF_MAX < (ETH_FRAME_LEN + 4))
> +#error "DESC_BUF_MAX must be set at minimum to ETH_FRAME_LEN + 4"
> +#endif

You can probably drop that, it would not make much sense to enable
fragments and a buffer size smaller than ETH_FRAME_LEN + ETH_FCS_LEN anyway.

> +
> +/* MAGIC value for knowing if a descriptor is available or not */
> +#define DCLEAN (BIT(16) | BIT(14) | BIT(12) | BIT(10) | BIT(9))
> +
> +/* Structure of DMA descriptor used by the hardware  */
> +struct dma_desc {
> + u32 status; /* status of the descriptor */
> + u32 st; /* Information on the frame */
> + u32 buf_addr; /* physical address of the frame data */
> + u32 next; /* physical address of next dma_desc */
> +} __packed __aligned(4);

This has been noticed in other emails, no need for the __packed here,
they are all naturally aligned.

> +
> +/* Benched on OPIPC with 100M, setting more than 256 does not give any
> + * perf boost
> + */
> +static int nbdesc_tx = 256;
> +module_param(nbdesc_tx, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(nbdesc_tx, "Number of descriptors in the TX list");
> +static int nbdesc_rx = 128;
> +module_param(nbdesc_rx, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(nbdesc_rx, "Number of descriptors in the RX list");

This needs to be statically defined to begin driver operation with, and
then implement the ethtool operations to re-size the rings would you
want that.

[snip]

> +/* Return the number of contiguous free descriptors
> + * starting from tx_slot
> + */
> +static int rb_tx_numfreedesc(struct net_device *ndev)
> +{
> + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> +
> + if (priv->tx_slot < priv->tx_dirty)
> + return priv->tx_dirty - priv->tx_slot;

Does this work with if tx_dirty wraps around?

> +
> + return (nbdesc_tx - priv->tx_slot) + priv->tx_dirty;
> +}
> +
> +/* Allocate a skb in a DMA descriptor
> + *
> + * @i index of slot to fill
> +*/
> +static int sun8i_emac_rx_sk(struct net_device *ndev, int i)
> +{
> + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> + struct dma_desc *ddesc;
> + struct sk_buff *sk;

The networking stack typically refers to "sk" as socket and skb as
socket buffers.

> +
> + ddesc = priv->dd_rx + i;
> +
> + ddesc->st = 0;
> +
> + sk = netdev_alloc_skb_ip_align(ndev, DESC_BUF_MAX);
> + if (!sk)
> + return -ENOMEM;
> +
> + /* should not happen */
> + if (unlikely(priv->rx_sk[i]))
> + dev_warn(priv->dev, "BUG: Leaking a skbuff\n");
> +
> + priv->rx_sk[i] = sk;
> +
> + ddesc->buf_addr = dma_map_single(priv->dev, sk->data,
> +  DESC_BUF_MAX, DMA_FROM_DEVICE);
> + if (dma_mapping_error(priv->dev, ddesc->buf_addr)) {
> + dev_err(priv->dev, "ERROR: Cannot dma_map RX buffer\n");
> + dev_kfree_skb(sk);
> + return -EFAULT;
> + }
> + ddesc->st |= DESC_BUF_MAX;
> + ddesc->status = BIT(31);

You are missing a lightweight barrier here to ensure there is no
re-ordering done by the compiler in how you write to the descriptors in
DRAM, even though they are allocated from dma_alloc_coherent().

[snip]

> +static void sun8i_emac_set_link_mode(struct sun8i_emac_priv *priv)
> +{
> + u32 v;
> +
> + v = readl(priv->base + SUN8I_EMAC_BASIC_CTL0);
> +
> + if (priv->duplex)
> + v |= BIT(0);
> + else
> + v &= ~BIT(0);
> +
> + v &= ~0x0C;
> + switch (priv->speed) {
> + case 1000:
> + break;
> + case 100:
> + v |= BIT(2);
> + v |= BIT(3);
> + break;
> + case 10:
> + v |= BIT(3);
> + break;
> + }

Proper defines for all of these bits and masks?

> +
> + writel(v, priv->base + SUN8I_EMAC_BASIC_CTL0);
> +}
> +
> +static void sun8i_emac_flow_ctrl(struct sun8i_emac_priv *priv, int duplex,
> +  int fc, int pause)
> +{
> + u32 flow = 0;

pause is unused (outside of printing it) here

> +
> + netif_dbg(priv, link, priv->ndev, "%s %d %d %d\n", __func__,
> +   duplex, fc, pause);
> +
> + flow = readl(priv->base +

Re: [linux-sunxi] [PATCH 1/5] ethernet: add sun8i-emac driver

2016-06-06 Thread LABBE Corentin 
On Sun, Jun 05, 2016 at 11:32:11PM +0100, André Przywara wrote:
> On 03/06/16 10:56, LABBE Corentin wrote:
> 
> Hi,
> 
> first: thanks for posting this and the time and work that you spent on
> it. With the respective DT nodes this works for me on the Pine64 and
> turns this board eventually into something useful.
> 
> Some comments below:
> 
> > 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 | 1943 
> > +++
> >  3 files changed, 1957 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..226499d 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"
> 
> nit: w/s error
> 
ok

> > +
> > +#define SUN8I_EMAC_BASIC_CTL0 0x00
> > +#define SUN8I_EMAC_BASIC_CTL1 0x04
> > +
> > +#define SUN8I_EMAC_MDIO_CMD 0x48
> > +#define SUN8I_EMAC_MDIO_DATA 0x4C
> 
> Can you align all these register offsets with tabs, so that the actual
> offset values are below each other?
> Also ordering them by address seems useful to me.
> 

ok

> > +/* Structure of DMA descriptor used by the hardware  */
> > +struct dma_desc {
> > +   u32 status; /* status of the descriptor */
> > +   u32 st; /* Information on the frame */
> > +   u32 buf_addr; /* physical address of the frame data */
> > +   u32 next; /* physical address of next dma_desc */
> > +} __packed __aligned(4);
> 
> I don't think we need this alignment attribute here:
> 1) The structure will be 4-byte aligned anyway, because the first member
> is naturally 4 bytes aligned.
> 2) The alignment requirement is on the physical DMA side. I don't see
> how the compiler should be able to guarantee this. So technically you
> have to tell the DMA allocation code about your alignment requirement.
> But due to 1), I think this is a moot discussion.
> 

ok, I have removed all __aligned

> 
> 
> 
> > +
> > +
> > +   priv->rx_sk = kcalloc(nbdesc_rx, sizeof(struct sk_buff *), GFP_KERNEL);
> > +   if (!priv->rx_sk) {
> > +   err = -ENOMEM;
> > +   goto rx_sk_error;
> > +   }
> > +   priv->tx_sk = kcalloc(nbdesc_tx, sizeof(struct sk_buff *), GFP_KERNEL);
> > +   if (!priv->tx_sk) {
> > +   err = -ENOMEM;
> > +   goto tx_sk_error;
> > +   }
> > +   priv->tx_map = kcalloc(nbdesc_tx, sizeof(int), GFP_KERNEL);
> > +   if (!priv->tx_map) {
> > +   err = -ENOMEM;
> > +   goto tx_map_error;
> > +   }
> > +
> > +   priv->dd_rx = dma_alloc_coherent(priv->dev,
> > +   nbdesc_rx * sizeof(struct dma_desc),
> > +   >dd_rx_phy,
> > +   GFP_KERNEL);
> 
> You need to be sure here to allocate 32-bit DMA memory only, since the
> hardware holds only 32 bits worth of addresses. If I am not mistaken,
> the following snippet (preferrably in the probe function below) should
> take care of this:
> 
>   if (dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32))) {
>   dev_err(>dev, "No suitable DMA available\n");
>   return -ENOMEM;
>   }
> 
> This isn't an issue for the SoCs we know of (they have a 32-bit bus
> anyway), but future SoCs could support more memory (the A80 does
> already!), so you have to allocate it from the low 4GB. This is a
> limitation of that particular _device_ (and not the platform), since it
> does the DMA itself and this engine is limited to 32-bit physical addresses.
> 

ok

> > +   if (!priv->dd_rx) {
> > +   dev_err(priv->dev, "ERROR: cannot DMA RX");
> > +   err = -ENOMEM;
> > +   goto dma_rx_error;
> > +   }
> 
> 
> 
> > +
> > +static const struct of_device_id sun8i_emac_of_match_table[] = {
> > +   { .compatible = "allwinner,sun8i-a83t-emac",
> > + .data = (void *)A83T_EMAC },
> > +   { .compatible = "allwinner,sun8i-h3-emac",
> > + .data = (void *)H3_EMAC },
> > +   { .compatible = "allwinner,sun50i-a64-emac",
> > + .data = (void *)A64_EMAC },
> > +   {}
> > +};
> 
> I am not sure this is the proper way to model the different variants of
> the device. I see two differing features here:
> 1) the H3 has an internal PHY
> 2) the A83T does not support RMII
> 
> So wouldn't it be wiser to put those two options as

Re: [linux-sunxi] [PATCH 1/5] ethernet: add sun8i-emac driver

2016-06-05 Thread Chen-Yu Tsai 
On Mon, Jun 6, 2016 at 6:32 AM, André Przywara  wrote:
> On 03/06/16 10:56, LABBE Corentin wrote:
>
> Hi,
>
> first: thanks for posting this and the time and work that you spent on
> it. With the respective DT nodes this works for me on the Pine64 and
> turns this board eventually into something useful.
>
> Some comments below:
>
>> 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 | 1943 
>> +++
>>  3 files changed, 1957 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..226499d 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"
>
> nit: w/s error
>
>> + 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..179aa61
>> --- /dev/null
>> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c
>> @@ -0,0 +1,1943 @@
>> +/*
>> + * sun8i-emac driver
>> + *
>> + * Copyright (C) 2015-2016 Corentin LABBE 
>> + *
>> + * This is the driver for Allwinner Ethernet MAC found in H3/A83T/A64 SoC
>> + *
>> + * TODO:
>> + * - NAPI
>> + * - 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 
>> +#include 
>> +
>> +#define SUN8I_EMAC_BASIC_CTL0 0x00
>> +#define SUN8I_EMAC_BASIC_CTL1 0x04
>> +
>> +#define SUN8I_EMAC_MDIO_CMD 0x48
>> +#define SUN8I_EMAC_MDIO_DATA 0x4C
>
> Can you align all these register offsets with tabs, so that the actual
> offset values are below each other?
> Also ordering them by address seems useful to me.
>
>> +
>> +#define SUN8I_EMAC_RX_CTL0 0x24
>> +#define SUN8I_EMAC_RX_CTL1 0x28
>> +
>> +#define SUN8I_EMAC_TX_CTL0 0x10
>> +#define SUN8I_EMAC_TX_CTL1 0x14
>> +
>> +#define SUN8I_EMAC_TX_FLOW_CTL 0x1C
>> +
>> +#define SUN8I_EMAC_RX_FRM_FLT 0x38
>> +
>> +#define SUN8I_EMAC_INT_STA 0x08
>> +#define SUN8I_EMAC_INT_EN 0x0C
>> +#define SUN8I_EMAC_RGMII_STA 0xD0
>
>
>> +
>> +#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_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_TX_DO_CRC (BIT(27) | BIT(28))
>> +#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)
>> +
>> +#define FLOW_RX 1
>> +#define FLOW_TX 2
>> +
>> +/* describe how data from skb are DMA mapped */
>> +#define MAP_SINGLE 1
>> +#define MAP_PAGE 2
>> +
>> +enum emac_variant {
>> + A83T_EMAC,
>> + H3_EMAC,
>> + A64_EMAC,
>> +};
>> +
>> +struct ethtool_str {
>> + char name[ETH_GSTRING_LEN];
>> +};
>> +
>> +static const struct

Re: [linux-sunxi] [PATCH 1/5] ethernet: add sun8i-emac driver

2016-06-05 Thread André Przywara 
On 03/06/16 10:56, LABBE Corentin wrote:

Hi,

first: thanks for posting this and the time and work that you spent on
it. With the respective DT nodes this works for me on the Pine64 and
turns this board eventually into something useful.

Some comments below:

> 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 | 1943 
> +++
>  3 files changed, 1957 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..226499d 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"

nit: w/s error

> + 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..179aa61
> --- /dev/null
> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c
> @@ -0,0 +1,1943 @@
> +/*
> + * sun8i-emac driver
> + *
> + * Copyright (C) 2015-2016 Corentin LABBE 
> + *
> + * This is the driver for Allwinner Ethernet MAC found in H3/A83T/A64 SoC
> + *
> + * TODO:
> + * - NAPI
> + * - 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 
> +#include 
> +
> +#define SUN8I_EMAC_BASIC_CTL0 0x00
> +#define SUN8I_EMAC_BASIC_CTL1 0x04
> +
> +#define SUN8I_EMAC_MDIO_CMD 0x48
> +#define SUN8I_EMAC_MDIO_DATA 0x4C

Can you align all these register offsets with tabs, so that the actual
offset values are below each other?
Also ordering them by address seems useful to me.

> +
> +#define SUN8I_EMAC_RX_CTL0 0x24
> +#define SUN8I_EMAC_RX_CTL1 0x28
> +
> +#define SUN8I_EMAC_TX_CTL0 0x10
> +#define SUN8I_EMAC_TX_CTL1 0x14
> +
> +#define SUN8I_EMAC_TX_FLOW_CTL 0x1C
> +
> +#define SUN8I_EMAC_RX_FRM_FLT 0x38
> +
> +#define SUN8I_EMAC_INT_STA 0x08
> +#define SUN8I_EMAC_INT_EN 0x0C
> +#define SUN8I_EMAC_RGMII_STA 0xD0


> +
> +#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_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_TX_DO_CRC (BIT(27) | BIT(28))
> +#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)
> +
> +#define FLOW_RX 1
> +#define FLOW_TX 2
> +
> +/* describe how data from skb are DMA mapped */
> +#define MAP_SINGLE 1
> +#define MAP_PAGE 2
> +
> +enum emac_variant {
> + A83T_EMAC,
> + H3_EMAC,
> + A64_EMAC,
> +};
> +
> +struct ethtool_str {
> + char name[ETH_GSTRING_LEN];
> +};
> +
> +static const struct ethtool_str estats_str[] = {
> + /* errors */
> + { "rx_payload_error" },
> + { "rx_CRC_error" },
> + { "rx_phy_error" },
> + { "rx_length_error" },
> + { "rx_col_error" },
> + { "rx_header_error" },
> + { "rx_overflow_error" },
>

Re: [PATCH 1/5] ethernet: add sun8i-emac driver

2016-06-03 Thread David Miller 
From: LABBE Corentin 
Date: Fri,  3 Jun 2016 11:56:26 +0200

> +static int nbdesc_tx = 256;
> +module_param(nbdesc_tx, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(nbdesc_tx, "Number of descriptors in the TX list");
> +static int nbdesc_rx = 128;
> +module_param(nbdesc_rx, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(nbdesc_rx, "Number of descriptors in the RX list");

Module parameters are not appropriate.

Please use the proper ethtool configuration facilities to control
the size of the RX and TX queues.

Thanks.

[PATCH 1/5] ethernet: add sun8i-emac driver

2016-06-03 Thread LABBE Corentin 
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 | 1943 +++
 3 files changed, 1957 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..226499d 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..179aa61
--- /dev/null
+++ b/drivers/net/ethernet/allwinner/sun8i-emac.c
@@ -0,0 +1,1943 @@
+/*
+ * sun8i-emac driver
+ *
+ * Copyright (C) 2015-2016 Corentin LABBE 
+ *
+ * This is the driver for Allwinner Ethernet MAC found in H3/A83T/A64 SoC
+ *
+ * TODO:
+ * - NAPI
+ * - 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 
+#include 
+
+#define SUN8I_EMAC_BASIC_CTL0 0x00
+#define SUN8I_EMAC_BASIC_CTL1 0x04
+
+#define SUN8I_EMAC_MDIO_CMD 0x48
+#define SUN8I_EMAC_MDIO_DATA 0x4C
+
+#define SUN8I_EMAC_RX_CTL0 0x24
+#define SUN8I_EMAC_RX_CTL1 0x28
+
+#define SUN8I_EMAC_TX_CTL0 0x10
+#define SUN8I_EMAC_TX_CTL1 0x14
+
+#define SUN8I_EMAC_TX_FLOW_CTL 0x1C
+
+#define SUN8I_EMAC_RX_FRM_FLT 0x38
+
+#define SUN8I_EMAC_INT_STA 0x08
+#define SUN8I_EMAC_INT_EN 0x0C
+#define SUN8I_EMAC_RGMII_STA 0xD0
+
+#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_TX_DO_CRC (BIT(27) | BIT(28))
+#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)
+
+#define FLOW_RX 1
+#define FLOW_TX 2
+
+/* describe how data from skb are DMA mapped */
+#define MAP_SINGLE 1
+#define MAP_PAGE 2
+
+enum emac_variant {
+   A83T_EMAC,
+   H3_EMAC,
+   A64_EMAC,
+};
+
+struct ethtool_str {
+   char name[ETH_GSTRING_LEN];
+};
+
+static const struct ethtool_str estats_str[] = {
+   /* errors */
+   { "rx_payload_error" },
+   { "rx_CRC_error" },
+   { "rx_phy_error" },
+   { "rx_length_error" },
+   { "rx_col_error" },
+   { "rx_header_error" },
+   { "rx_overflow_error" },
+   { "rx_saf_error" },
+   { "rx_daf_error" },
+   { "rx_buf_error" },
+   /* misc infos */
+   { "tx_stop_queue" },
+   { "rx_dma_ua" },
+   { "rx_dma_stop" },
+   { "tx_dma_ua" },
+   { "tx_dma_stop" },
+   { "rx_hw_csum" },
+   { "tx_hw_csum" },
+   /* interrupts */
+   { "rx_early_int" },
+   { "tx_early_int" },
+   { "tx_underflow_int" },
+   /* debug */
+   { "tx_used_desc" },
+};
+
+struct sun8i_emac_stats {
+   u64 rx_payload_error;
+   u64 rx_crc_error;
+   u64 rx_phy_error;
+   u64 rx_length_error;
+   u64 rx_col_error;
+   u64 rx_header_error;
+   u64 rx_overflow_error;
+   u64 rx_saf_fail;
+   u64