[U-Boot,3/7] net/ethoc: add CONFIG_DM_ETH support

Extract reusable parts from ethoc_init, ethoc_set_mac_address,
ethoc_send and ethoc_receive, move the rest under #ifdef CONFIG_DM_ETH.
Add U_BOOT_DRIVER, eth_ops structure and implement required methods.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 drivers/net/ethoc.c                  | 221 +++++++++++++++++++++++++++--------
 include/dm/platform_data/net_ethoc.h |  20 ++++
 2 files changed, 194 insertions(+), 47 deletions(-)
 create mode 100644 include/dm/platform_data/net_ethoc.h

On 2 August 2016 at 05:31, Max Filippov <jcmvbkbc@gmail.com> wrote:
> Extract reusable parts from ethoc_init, ethoc_set_mac_address,
> ethoc_send and ethoc_receive, move the rest under #ifdef CONFIG_DM_ETH.
> Add U_BOOT_DRIVER, eth_ops structure and implement required methods.
>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  drivers/net/ethoc.c                  | 221 +++++++++++++++++++++++++++--------
>  include/dm/platform_data/net_ethoc.h |  20 ++++
>  2 files changed, 194 insertions(+), 47 deletions(-)
>  create mode 100644 include/dm/platform_data/net_ethoc.h

Reviewed-by: Simon Glass <sjg@chromium.org>

On Tue, Aug 2, 2016 at 6:31 AM, Max Filippov <jcmvbkbc@gmail.com> wrote:
> Extract reusable parts from ethoc_init, ethoc_set_mac_address,
> ethoc_send and ethoc_receive, move the rest under #ifdef CONFIG_DM_ETH.
> Add U_BOOT_DRIVER, eth_ops structure and implement required methods.
>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>

A few small nits below, but otherwise,

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

> ---
>  drivers/net/ethoc.c                  | 221 +++++++++++++++++++++++++++--------
>  include/dm/platform_data/net_ethoc.h |  20 ++++
>  2 files changed, 194 insertions(+), 47 deletions(-)
>  create mode 100644 include/dm/platform_data/net_ethoc.h
>
> diff --git a/drivers/net/ethoc.c b/drivers/net/ethoc.c
> index f5bd1ab..0225595 100644
> --- a/drivers/net/ethoc.c
> +++ b/drivers/net/ethoc.c
> @@ -5,13 +5,14 @@
>   * Copyright (C) 2008-2009 Avionic Design GmbH
>   *   Thierry Reding <thierry.reding@avionic-design.de>
>   * Copyright (C) 2010 Thomas Chou <thomas@wytron.com.tw>
> + * Copyright (C) 2016 Cadence Design Systems Inc.
>   *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> + * SPDX-License-Identifier:    GPL-2.0
>   */
>
>  #include <common.h>
> +#include <dm/device.h>
> +#include <dm/platform_data/net_ethoc.h>
>  #include <linux/io.h>
>  #include <malloc.h>
>  #include <net.h>
> @@ -216,11 +217,8 @@ static inline void ethoc_write_bd(struct ethoc *priv, int index,
>         ethoc_write(priv, offset + 4, bd->addr);
>  }
>
> -static int ethoc_set_mac_address(struct eth_device *dev)
> +static int ethoc_set_mac_address(struct ethoc *priv, u8 *mac)

Please use ethoc_write_hwaddr_common() instead.

>  {
> -       struct ethoc *priv = (struct ethoc *)dev->priv;
> -       u8 *mac = dev->enetaddr;
> -
>         ethoc_write(priv, MAC_ADDR0, (mac[2] << 24) | (mac[3] << 16) |
>                     (mac[4] << 8) | (mac[5] << 0));
>         ethoc_write(priv, MAC_ADDR1, (mac[0] << 8) | (mac[1] << 0));
> @@ -305,11 +303,8 @@ static int ethoc_reset(struct ethoc *priv)
>         return 0;
>  }
>
> -static int ethoc_init(struct eth_device *dev, bd_t * bd)
> +static int ethoc_init_common(struct ethoc *priv)
>  {
> -       struct ethoc *priv = (struct ethoc *)dev->priv;
> -       printf("ethoc\n");
> -
>         priv->num_tx = 1;
>         priv->num_rx = PKTBUFSRX;
>         ethoc_write(priv, TX_BD_NUM, priv->num_tx);
> @@ -354,36 +349,43 @@ static int ethoc_update_rx_stats(struct ethoc_bd *bd)
>         return ret;
>  }
>
> -static int ethoc_rx(struct ethoc *priv, int limit)
> +static int ethoc_rx_common(struct ethoc *priv, uchar **packetp)
>  {
> -       int count;
> -
> -       for (count = 0; count < limit; ++count) {
> -               u32 entry;
> -               struct ethoc_bd bd;
> +       u32 entry;
> +       struct ethoc_bd bd;
>
> -               entry = priv->num_tx + (priv->cur_rx % priv->num_rx);
> -               ethoc_read_bd(priv, entry, &bd);
> -               if (bd.stat & RX_BD_EMPTY)
> -                       break;
> +       entry = priv->num_tx + (priv->cur_rx % priv->num_rx);
> +       ethoc_read_bd(priv, entry, &bd);
> +       if (bd.stat & RX_BD_EMPTY)
> +               return -EAGAIN;
> +
> +       debug("%s(): RX buffer %d, %x received\n",
> +             __func__, priv->cur_rx, bd.stat);
> +       if (ethoc_update_rx_stats(&bd) == 0) {
> +               int size = bd.stat >> 16;
> +
> +               size -= 4;      /* strip the CRC */
> +               *packetp = (void *)bd.addr;
> +               return size;
> +       } else {
> +               return 0;
> +       }
> +}
>
> -               debug("%s(): RX buffer %d, %x received\n",
> -                     __func__, priv->cur_rx, bd.stat);
> -               if (ethoc_update_rx_stats(&bd) == 0) {
> -                       int size = bd.stat >> 16;
> -                       size -= 4;      /* strip the CRC */
> -                       net_process_received_packet((void *)bd.addr, size);
> -               }
> +static int ethoc_recv_common(struct ethoc *priv)

Please use a better name for this, something like ethoc_is_rx_pkt_rdy().

> +{
> +       u32 pending;
>
> -               /* clear the buffer descriptor so it can be reused */
> -               flush_dcache_range(bd.addr, bd.addr + PKTSIZE_ALIGN);
> -               bd.stat &= ~RX_BD_STATS;
> -               bd.stat |= RX_BD_EMPTY;
> -               ethoc_write_bd(priv, entry, &bd);
> -               priv->cur_rx++;
> +       pending = ethoc_read(priv, INT_SOURCE);
> +       ethoc_ack_irq(priv, pending);
> +       if (pending & INT_MASK_BUSY)
> +               debug("%s(): packet dropped\n", __func__);
> +       if (pending & INT_MASK_RX) {
> +               debug("%s(): rx irq\n", __func__);
> +               return 1;
>         }
>
> -       return count;
> +       return 0;
>  }
>
>  static int ethoc_update_tx_stats(struct ethoc_bd *bd)
> @@ -413,9 +415,8 @@ static void ethoc_tx(struct ethoc *priv)
>                 (void)ethoc_update_tx_stats(&bd);
>  }
>
> -static int ethoc_send(struct eth_device *dev, void *packet, int length)
> +static int ethoc_send_common(struct ethoc *priv, void *packet, int length)
>  {
> -       struct ethoc *priv = (struct ethoc *)dev->priv;
>         struct ethoc_bd bd;
>         u32 entry;
>         u32 pending;
> @@ -460,6 +461,47 @@ static int ethoc_send(struct eth_device *dev, void *packet, int length)
>         return 0;
>  }
>
> +static int ethoc_free_pkt_common(struct ethoc *priv)
> +{
> +       u32 entry;
> +       struct ethoc_bd bd;
> +
> +       entry = priv->num_tx + (priv->cur_rx % priv->num_rx);
> +       ethoc_read_bd(priv, entry, &bd);
> +
> +       /* clear the buffer descriptor so it can be reused */
> +       flush_dcache_range(bd.addr, bd.addr + PKTSIZE_ALIGN);
> +       bd.stat &= ~RX_BD_STATS;
> +       bd.stat |= RX_BD_EMPTY;
> +       ethoc_write_bd(priv, entry, &bd);
> +       priv->cur_rx++;
> +
> +       return 0;
> +}
> +
> +#ifndef CONFIG_DM_ETH

Please use positive logic and reverse the order of these code snippets.

#ifdef CONFIG_DM_ETH
...
#else
...
#endif

> +
> +static int ethoc_init(struct eth_device *dev, bd_t *bd)
> +{
> +       struct ethoc *priv = (struct ethoc *)dev->priv;
> +
> +       priv->iobase = ioremap(dev->iobase, ETHOC_IOSIZE);
> +       return ethoc_init_common(priv);
> +}
> +
> +static int ethoc_write_hwaddr(struct eth_device *dev)
> +{
> +       struct ethoc *priv = (struct ethoc *)dev->priv;
> +       u8 *mac = dev->enetaddr;
> +
> +       return ethoc_set_mac_address(priv, mac);
> +}
> +
> +static int ethoc_send(struct eth_device *dev, void *packet, int length)
> +{
> +       return ethoc_send_common(dev->priv, packet, length);
> +}
> +
>  static void ethoc_halt(struct eth_device *dev)
>  {
>         ethoc_disable_rx_and_tx(dev->priv);
> @@ -468,17 +510,21 @@ static void ethoc_halt(struct eth_device *dev)
>  static int ethoc_recv(struct eth_device *dev)
>  {
>         struct ethoc *priv = (struct ethoc *)dev->priv;
> -       u32 pending;
> +       int count;
>
> -       pending = ethoc_read(priv, INT_SOURCE);
> -       ethoc_ack_irq(priv, pending);
> -       if (pending & INT_MASK_BUSY)
> -               debug("%s(): packet dropped\n", __func__);
> -       if (pending & INT_MASK_RX) {
> -               debug("%s(): rx irq\n", __func__);
> -               ethoc_rx(priv, PKTBUFSRX);
> -       }
> +       if (!ethoc_recv_common(priv))
> +               return 0;
>
> +       for (count = 0; count < PKTBUFSRX; ++count) {
> +               uchar *packetp;
> +               int size = ethoc_rx_common(priv, &packetp);
> +
> +               if (size < 0)
> +                       break;
> +               if (size > 0)
> +                       net_process_received_packet(packetp, size);
> +               ethoc_free_pkt_common(priv);
> +       }
>         return 0;
>  }
>
> @@ -503,10 +549,91 @@ int ethoc_initialize(u8 dev_num, int base_addr)
>         dev->halt = ethoc_halt;
>         dev->send = ethoc_send;
>         dev->recv = ethoc_recv;
> -       dev->write_hwaddr = ethoc_set_mac_address;
> +       dev->write_hwaddr = ethoc_write_hwaddr;
>         sprintf(dev->name, "%s-%hu", "ETHOC", dev_num);
>         priv->iobase = ioremap(dev->iobase, ETHOC_IOSIZE);
>
>         eth_register(dev);
>         return 1;
>  }
> +
> +#else
> +
> +static int ethoc_write_hwaddr(struct udevice *dev)
> +{
> +       struct ethoc_eth_pdata *pdata = dev_get_platdata(dev);
> +       struct ethoc *priv = dev_get_priv(dev);
> +       u8 *mac = pdata->eth_pdata.enetaddr;
> +
> +       return ethoc_set_mac_address(priv, mac);
> +}
> +
> +static int ethoc_send(struct udevice *dev, void *packet, int length)
> +{
> +       return ethoc_send_common(dev_get_priv(dev), packet, length);
> +}
> +
> +static int ethoc_free_pkt(struct udevice *dev, uchar *packet, int length)
> +{
> +       return ethoc_free_pkt_common(dev_get_priv(dev));
> +}
> +
> +static int ethoc_recv(struct udevice *dev, int flags, uchar **packetp)
> +{
> +       struct ethoc *priv = dev_get_priv(dev);
> +
> +       if (flags & ETH_RECV_CHECK_DEVICE)
> +               ethoc_recv_common(priv);

Kinda seems like you should skip the next call (especially when this
is renamed to be clearer). The code flow seems OK, though. Up to you.

> +
> +       return ethoc_rx_common(priv, packetp);
> +}
> +
> +static int ethoc_start(struct udevice *dev)
> +{
> +       return ethoc_init_common(dev_get_priv(dev));
> +}
> +
> +static void ethoc_stop(struct udevice *dev)
> +{
> +       struct ethoc *priv = dev_get_priv(dev);
> +
> +       ethoc_disable_rx_and_tx(priv);
> +}
> +
> +static int ethoc_probe(struct udevice *dev)
> +{
> +       struct ethoc_eth_pdata *pdata = dev_get_platdata(dev);
> +       struct ethoc *priv = dev_get_priv(dev);
> +
> +       priv->iobase = ioremap(pdata->eth_pdata.iobase, ETHOC_IOSIZE);
> +       return 0;
> +}
> +
> +static int ethoc_remove(struct udevice *dev)
> +{
> +       struct ethoc *priv = dev_get_priv(dev);
> +
> +       iounmap(priv->iobase);
> +       return 0;
> +}
> +
> +static const struct eth_ops ethoc_ops = {
> +       .start          = ethoc_start,
> +       .stop           = ethoc_stop,
> +       .send           = ethoc_send,
> +       .recv           = ethoc_recv,
> +       .free_pkt       = ethoc_free_pkt,
> +       .write_hwaddr   = ethoc_write_hwaddr,
> +};
> +
> +U_BOOT_DRIVER(ethoc) = {
> +       .name                           = "ethoc",
> +       .id                             = UCLASS_ETH,
> +       .probe                          = ethoc_probe,
> +       .remove                         = ethoc_remove,
> +       .ops                            = &ethoc_ops,
> +       .priv_auto_alloc_size           = sizeof(struct ethoc),
> +       .platdata_auto_alloc_size       = sizeof(struct ethoc_eth_pdata),
> +};
> +
> +#endif
> diff --git a/include/dm/platform_data/net_ethoc.h b/include/dm/platform_data/net_ethoc.h
> new file mode 100644
> index 0000000..1d8c73c
> --- /dev/null
> +++ b/include/dm/platform_data/net_ethoc.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (C) 2016 Cadence Design Systems Inc.
> + *
> + * SPDX-License-Identifier:    GPL-2.0
> + */
> +
> +#ifndef _ETHOC_H
> +#define _ETHOC_H
> +
> +#include <net.h>
> +
> +#ifdef CONFIG_DM_ETH
> +
> +struct ethoc_eth_pdata {
> +       struct eth_pdata eth_pdata;
> +};
> +
> +#endif
> +
> +#endif /* _ETHOC_H */

Thanks,
-Joe

On Thu, Aug 4, 2016 at 8:51 PM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> On Tue, Aug 2, 2016 at 6:31 AM, Max Filippov <jcmvbkbc@gmail.com> wrote:
>> Extract reusable parts from ethoc_init, ethoc_set_mac_address,
>> ethoc_send and ethoc_receive, move the rest under #ifdef CONFIG_DM_ETH.
>> Add U_BOOT_DRIVER, eth_ops structure and implement required methods.
>>
>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>
> A few small nits below, but otherwise,
>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>
>> diff --git a/drivers/net/ethoc.c b/drivers/net/ethoc.c
>> index f5bd1ab..0225595 100644
>> --- a/drivers/net/ethoc.c
>> +++ b/drivers/net/ethoc.c

[...]

>> @@ -216,11 +217,8 @@ static inline void ethoc_write_bd(struct ethoc *priv, int index,
>>         ethoc_write(priv, offset + 4, bd->addr);
>>  }
>>
>> -static int ethoc_set_mac_address(struct eth_device *dev)
>> +static int ethoc_set_mac_address(struct ethoc *priv, u8 *mac)
>
> Please use ethoc_write_hwaddr_common() instead.

Ok.

[...]

>> +static int ethoc_recv_common(struct ethoc *priv)
>
> Please use a better name for this, something like ethoc_is_rx_pkt_rdy().

It's a bit more complex than that, it would only return
1 if new packets arrived since its last invocation. When
it returns 0 there still may be received packets in the
RX queue. I'll call it ethoc_is_new_packet_received.

[...]

>> +#ifndef CONFIG_DM_ETH
>
> Please use positive logic and reverse the order of these code snippets.
>
> #ifdef CONFIG_DM_ETH
> ...
> #else
> ...
> #endif

Ok.

[...]

>> +static int ethoc_recv(struct udevice *dev, int flags, uchar **packetp)
>> +{
>> +       struct ethoc *priv = dev_get_priv(dev);
>> +
>> +       if (flags & ETH_RECV_CHECK_DEVICE)
>> +               ethoc_recv_common(priv);
>
> Kinda seems like you should skip the next call (especially when this
> is renamed to be clearer). The code flow seems OK, though. Up to you.

Ok.