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

[Max Filippov]

On Thu, Aug 4, 2016 at 8:51 PM, Joe Hershberger
 wrote:
> On Tue, Aug 2, 2016 at 6:31 AM, Max Filippov  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 
>
> A few small nits below, but otherwise,
>
> Acked-by: Joe Hershberger 
>
>> 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.

-- 
Thanks.
-- Max
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Joe Hershberger]

On Tue, Aug 2, 2016 at 6:31 AM, Max Filippov  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 

A few small nits below, but otherwise,

Acked-by: Joe Hershberger 

> ---
>  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 
>   * Copyright (C) 2010 Thomas Chou 
> + * 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 
> +#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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, );
> -   if (bd.stat & RX_BD_EMPTY)
> -   break;
> +   entry = priv->num_tx + (priv->cur_rx % priv->num_rx);
> +   ethoc_read_bd(priv, entry, );
> +   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() == 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() == 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, );
> -   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)
> 

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

[Simon Glass]

On 2 August 2016 at 05:31, Max Filippov  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 
> ---
>  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 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Max Filippov]

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 
---
 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 
  * Copyright (C) 2010 Thomas Chou 
+ * 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 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -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)
 {
-   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, );
-   if (bd.stat & RX_BD_EMPTY)
-   break;
+   entry = priv->num_tx + (priv->cur_rx % priv->num_rx);
+   ethoc_read_bd(priv, entry, );
+   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() == 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() == 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)
+{
+   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, );
-   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();
 }
 
-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;
+