On Tue, Aug 2, 2016 at 6:31 AM, Max Filippov <jcmvb...@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 <jcmvb...@gmail.com>
A few small nits below, but otherwise, Acked-by: Joe Hershberger <joe.hershber...@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.red...@avionic-design.de> > * Copyright (C) 2010 Thomas Chou <tho...@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 = ðoc_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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot