[U-Boot] [PATCH 07/10] net: axi_emac: Move driver to DM

[Michal Simek <michal.simek@xilinx.com>]

On 15.12.2015 23:34, Simon Glass wrote:
> Hi Joe,
> 
> On 15 December 2015 at 13:52, Joe Hershberger <joe.hershberger@gmail.com> wrote:
>>
>> Hi Simon,
>>
>> On Tue, Dec 15, 2015 at 12:57 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Michal,
>>>
>>> On 11 December 2015 at 04:59, Michal Simek <michal.simek@xilinx.com> wrote:
>>>> Move driver to DM.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>> ---
>>>>
>>>>  .../xilinx/microblaze-generic/microblaze-generic.c |   5 -
>>>>  board/xilinx/zynq/board.c                          |   4 -
>>>>  drivers/net/xilinx_axi_emac.c                      | 190 +++++++++++++--------
>>>>  include/netdev.h                                   |   2 -
>>>>  4 files changed, 122 insertions(+), 79 deletions(-)
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>
>>> See a few things below.
>>>
>>>>
>>>> diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c b/board/xilinx/microblaze-generic/microblaze-generic.c
>>>> index dfa629322223..a3122da9acaa 100644
>>>> --- a/board/xilinx/microblaze-generic/microblaze-generic.c
>>>> +++ b/board/xilinx/microblaze-generic/microblaze-generic.c
>>>> @@ -105,11 +105,6 @@ int board_eth_init(bd_t *bis)
>>>>  {
>>>>         int ret = 0;
>>>>
>>>> -#ifdef CONFIG_XILINX_AXIEMAC
>>>> -       ret |= xilinx_axiemac_initialize(bis, XILINX_AXIEMAC_BASEADDR,
>>>> -                                               XILINX_AXIDMA_BASEADDR);
>>>> -#endif
>>>> -
>>>>  #if defined(CONFIG_XILINX_EMACLITE) && defined(XILINX_EMACLITE_BASEADDR)
>>>>         u32 txpp = 0;
>>>>         u32 rxpp = 0;
>>>> diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c
>>>> index 414f5302a066..427e75485deb 100644
>>>> --- a/board/xilinx/zynq/board.c
>>>> +++ b/board/xilinx/zynq/board.c
>>>> @@ -103,10 +103,6 @@ int board_eth_init(bd_t *bis)
>>>>  {
>>>>         u32 ret = 0;
>>>>
>>>> -#ifdef CONFIG_XILINX_AXIEMAC
>>>> -       ret |= xilinx_axiemac_initialize(bis, XILINX_AXIEMAC_BASEADDR,
>>>> -                                               XILINX_AXIDMA_BASEADDR);
>>>> -#endif
>>>>  #ifdef CONFIG_XILINX_EMACLITE
>>>>         u32 txpp = 0;
>>>>         u32 rxpp = 0;
>>>> diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c
>>>> index 77b1869dc9dc..c03f8f730d3a 100644
>>>> --- a/drivers/net/xilinx_axi_emac.c
>>>> +++ b/drivers/net/xilinx_axi_emac.c
>>>> @@ -8,12 +8,15 @@
>>>>
>>>>  #include <config.h>
>>>>  #include <common.h>
>>>> +#include <dm.h>
>>>>  #include <net.h>
>>>>  #include <malloc.h>
>>>>  #include <asm/io.h>
>>>>  #include <phy.h>
>>>>  #include <miiphy.h>
>>>>
>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>> +
>>>>  #if !defined(CONFIG_PHYLIB)
>>>>  # error AXI_ETHERNET requires PHYLIB
>>>>  #endif
>>>> @@ -87,6 +90,7 @@ struct axidma_priv {
>>>>         struct axidma_reg *dmarx;
>>>>         int phyaddr;
>>>>         struct axi_regs *iobase;
>>>> +       phy_interface_t interface;
>>>>         struct phy_device *phydev;
>>>>         struct mii_dev *bus;
>>>>  };
>>>> @@ -218,11 +222,11 @@ static u32 phywrite(struct axidma_priv *priv, u32 phyaddress, u32 registernum,
>>>>  }
>>>>
>>>>  /* Setting axi emac and phy to proper setting */
>>>> -static int setup_phy(struct eth_device *dev)
>>>> +static int setup_phy(struct udevice *dev)
>>>>  {
>>>>         u16 phyreg;
>>>>         u32 i, speed, emmc_reg, ret;
>>>> -       struct axidma_priv *priv = dev->priv;
>>>> +       struct axidma_priv *priv = dev_get_priv(dev);
>>>>         struct axi_regs *regs = priv->iobase;
>>>>         struct phy_device *phydev;
>>>>
>>>> @@ -298,9 +302,9 @@ static int setup_phy(struct eth_device *dev)
>>>>  }
>>>>
>>>>  /* STOP DMA transfers */
>>>> -static void axiemac_halt(struct eth_device *dev)
>>>> +static void axiemac_halt(struct udevice *dev)
>>>>  {
>>>> -       struct axidma_priv *priv = dev->priv;
>>>> +       struct axidma_priv *priv = dev_get_priv(dev);
>>>>         u32 temp;
>>>>
>>>>         /* Stop the hardware */
>>>> @@ -358,16 +362,18 @@ static int axi_ethernet_init(struct axidma_priv *priv)
>>>>         return 0;
>>>>  }
>>>>
>>>> -static int axiemac_setup_mac(struct eth_device *dev)
>>>> +static int axiemac_setup_mac(struct udevice *dev)
>>>>  {
>>>> -       struct axi_regs *regs = (struct axi_regs *)dev->iobase;
>>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>>> +       struct axidma_priv *priv = dev_get_priv(dev);
>>>> +       struct axi_regs *regs = priv->iobase;
>>>>
>>>>         /* Set the MAC address */
>>>> -       int val = ((dev->enetaddr[3] << 24) | (dev->enetaddr[2] << 16) |
>>>> -               (dev->enetaddr[1] << 8) | (dev->enetaddr[0]));
>>>> +       int val = ((pdata->enetaddr[3] << 24) | (pdata->enetaddr[2] << 16) |
>>>> +               (pdata->enetaddr[1] << 8) | (pdata->enetaddr[0]));
>>>>         out_be32(&regs->uaw0, val);
>>>>
>>>> -       val = (dev->enetaddr[5] << 8) | dev->enetaddr[4] ;
>>>> +       val = (pdata->enetaddr[5] << 8) | pdata->enetaddr[4];
>>>>         val |= in_be32(&regs->uaw1) & ~XAE_UAW1_UNICASTADDR_MASK;
>>>>         out_be32(&regs->uaw1, val);
>>>>         return 0;
>>>> @@ -396,10 +402,10 @@ static void axi_dma_init(struct axidma_priv *priv)
>>>>                 printf("%s: Timeout\n", __func__);
>>>>  }
>>>>
>>>> -static int axiemac_init(struct eth_device *dev, bd_t * bis)
>>>> +static int axiemac_init(struct udevice *dev)
>>>>  {
>>>> -       struct axidma_priv *priv = dev->priv;
>>>> -       struct axi_regs *regs = (struct axi_regs *)dev->iobase;
>>>> +       struct axidma_priv *priv = dev_get_priv(dev);
>>>> +       struct axi_regs *regs = priv->iobase;
>>>>         u32 temp;
>>>>
>>>>         debug("axiemac: Init started\n");
>>>> @@ -458,9 +464,9 @@ static int axiemac_init(struct eth_device *dev, bd_t * bis)
>>>>         return 0;
>>>>  }
>>>>
>>>> -static int axiemac_send(struct eth_device *dev, void *ptr, int len)
>>>> +static int axiemac_send(struct udevice *dev, void *ptr, int len)
>>>>  {
>>>> -       struct axidma_priv *priv = dev->priv;
>>>> +       struct axidma_priv *priv = dev_get_priv(dev);
>>>>         u32 timeout;
>>>>
>>>>         if (len > PKTSIZE_ALIGN)
>>>> @@ -530,15 +536,15 @@ static int isrxready(struct axidma_priv *priv)
>>>>         return 0;
>>>>  }
>>>>
>>>> -static int axiemac_recv(struct eth_device *dev)
>>>> +static int axiemac_recv(struct udevice *dev, int flags, uchar **packetp)
>>>>  {
>>>>         u32 length;
>>>> -       struct axidma_priv *priv = dev->priv;
>>>> +       struct axidma_priv *priv = dev_get_priv(dev);
>>>>         u32 temp;
>>>>
>>>>         /* Wait for an incoming packet */
>>>>         if (!isrxready(priv))
>>>> -               return 0;
>>>> +               return -1;
>>>
>>> I suggest -EAGAIN
>>>
>>>>
>>>>         debug("axiemac: RX data ready\n");
>>>>
>>>> @@ -578,77 +584,125 @@ static int axiemac_recv(struct eth_device *dev)
>>>>
>>>>         debug("axiemac: RX completed, framelength = %d\n", length);
>>>>
>>>> -       return length;
>>>> +       return 0;
>>>
>>> You do need to return the length here. You could update net.h to make
>>> this clearer.
>>
>> I think I figured out what Michal is doing here. What's not clear from
>> this patch is that he doesn't want net_process_received_packet() to
>> run on the packet since this function already calls it directly above.
>>
>> He has a follow-on patch that fixes that and resumes returning the
>> length from recv().
> 
> OK I see, thanks.

That's what I meant by word "already" below.
"recv part is calling  net_process_received_packet(rxframe, length);
already and then the core is checking for returning value. which should
be 0 or less to quit this loop."

I wanted to do separation by two patches to make it clear but it looks
like I have confused you a little bit. :-)

Does that mean that I can add Simon's Reviewed-by tag?
Joe: What about you?

Thanks,
Michal