[U-Boot] [PATCH] net: usb: r8152: Add DM support

[Stefan Roese <sr@denx.de>]

Hi Simon,

On 26.06.2016 04:53, Simon Glass wrote:
> On 22 June 2016 at 01:18, Stefan Roese <sr@denx.de> wrote:
>> Add support for driver model, so that CONFIG_DM_ETH can be defined and
>> used with this driver.
>>
>> This patch also adds the read_rom_hwaddr() callback so that the ROM MAC
>> address will be used to the DM part of this driver.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Stephen Warren <swarren@nvidia.com>
>> Cc: Ted Chen <tedchen@realtek.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Joe Hershberger <joe.hershberger@ni.com>
>> ---
>>   drivers/usb/eth/r8152.c    | 235 ++++++++++++++++++++++++++++++++++++++++-----
>>   drivers/usb/eth/r8152.h    |   4 +
>>   drivers/usb/eth/r8152_fw.c |   2 +
>>   3 files changed, 219 insertions(+), 22 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> With a few comments below.
> 
>>
>> diff --git a/drivers/usb/eth/r8152.c b/drivers/usb/eth/r8152.c
>> index 325b70c..a148267 100644
>> --- a/drivers/usb/eth/r8152.c
>> +++ b/drivers/usb/eth/r8152.c
>> @@ -3,9 +3,10 @@
>>    *
>>    * SPDX-License-Identifier:    GPL-2.0
>>    *
>> -  */
>> + */
>>
>>   #include <common.h>
>> +#include <dm.h>
>>   #include <errno.h>
>>   #include <malloc.h>
>>   #include <usb.h>
>> @@ -15,6 +16,7 @@
>>   #include "usb_ether.h"
>>   #include "r8152.h"
>>
>> +#ifndef CONFIG_DM_ETH
>>   /* local vars */
>>   static int curr_eth_dev; /* index for name of next device detected */
>>
>> @@ -23,12 +25,6 @@ struct r8152_dongle {
>>          unsigned short product;
>>   };
>>
>> -struct r8152_version {
>> -       unsigned short tcr;
>> -       unsigned short version;
>> -       bool           gmii;
>> -};
>> -
>>   static const struct r8152_dongle const r8152_dongles[] = {
>>          /* Realtek */
>>          { 0x0bda, 0x8050 },
>> @@ -54,6 +50,13 @@ static const struct r8152_dongle const r8152_dongles[] = {
>>          /* Nvidia */
>>          { 0x0955, 0x09ff },
>>   };
>> +#endif
>> +
>> +struct r8152_version {
>> +       unsigned short tcr;
>> +       unsigned short version;
>> +       bool           gmii;
>> +};
>>
>>   static const struct r8152_version const r8152_versions[] = {
>>          { 0x4c00, RTL_VER_01, 0 },
>> @@ -1176,11 +1179,8 @@ static int rtl_ops_init(struct r8152 *tp)
>>          return ret;
>>   }
>>
>> -static int r8152_init(struct eth_device *eth, bd_t *bd)
>> +static int r8152_init_common(struct r8152 *tp)
>>   {
>> -       struct ueth_data *dev = (struct ueth_data *)eth->priv;
>> -       struct r8152 *tp = (struct r8152 *)dev->dev_priv;
>> -
>>          u8 speed;
>>          int timeout = 0;
>>          int link_detected;
>> @@ -1210,14 +1210,11 @@ static int r8152_init(struct eth_device *eth, bd_t *bd)
>>          return 0;
>>   }
>>
>> -static int r8152_send(struct eth_device *eth, void *packet, int length)
>> +static int r8152_send_common(struct ueth_data *ueth, void *packet, int length)
>>   {
>> -       struct ueth_data *dev = (struct ueth_data *)eth->priv;
>> -
>> +       struct usb_device *udev = ueth->pusb_dev;
>>          u32 opts1, opts2 = 0;
>> -
>>          int err;
>> -
>>          int actual_len;
>>          unsigned char msg[PKTSIZE + sizeof(struct tx_desc)];
>>          struct tx_desc *tx_desc = (struct tx_desc *)msg;
>> @@ -1231,18 +1228,31 @@ static int r8152_send(struct eth_device *eth, void *packet, int length)
>>
>>          memcpy(msg + sizeof(struct tx_desc), (void *)packet, length);
>>
>> -       err = usb_bulk_msg(dev->pusb_dev,
>> -                               usb_sndbulkpipe(dev->pusb_dev, dev->ep_out),
>> -                               (void *)msg,
>> -                               length + sizeof(struct tx_desc),
>> -                               &actual_len,
>> -                               USB_BULK_SEND_TIMEOUT);
>> +       err = usb_bulk_msg(udev, usb_sndbulkpipe(udev, ueth->ep_out),
>> +                          (void *)msg, length + sizeof(struct tx_desc),
>> +                          &actual_len, USB_BULK_SEND_TIMEOUT);
>>          debug("Tx: len = %zu, actual = %u, err = %d\n",
>>                length + sizeof(struct tx_desc), actual_len, err);
>>
>>          return err;
>>   }
>>
>> +#ifndef CONFIG_DM_ETH
>> +static int r8152_init(struct eth_device *eth, bd_t *bd)
>> +{
>> +       struct ueth_data *dev = (struct ueth_data *)eth->priv;
>> +       struct r8152 *tp = (struct r8152 *)dev->dev_priv;
>> +
>> +       return r8152_init_common(tp);
>> +}
>> +
>> +static int r8152_send(struct eth_device *eth, void *packet, int length)
>> +{
>> +       struct ueth_data *dev = (struct ueth_data *)eth->priv;
>> +
>> +       return r8152_send_common(dev, packet, length);
>> +}
>> +
>>   static int r8152_recv(struct eth_device *eth)
>>   {
>>          struct ueth_data *dev = (struct ueth_data *)eth->priv;
>> @@ -1454,3 +1464,184 @@ int r8152_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
>>          debug("MAC %pM\n", eth->enetaddr);
>>          return 1;
>>   }
>> +#endif /* !CONFIG_DM_ETH */
>> +
>> +#ifdef CONFIG_DM_ETH
>> +static int r8152_eth_start(struct udevice *dev)
>> +{
>> +       struct r8152 *tp = dev_get_priv(dev);
>> +
>> +       debug("** %s (%d)\n", __func__, __LINE__);
>> +
>> +       return r8152_init_common(tp);
>> +}
>> +
>> +void r8152_eth_stop(struct udevice *dev)
>> +{
>> +       struct r8152 *tp = dev_get_priv(dev);
>> +
>> +       debug("** %s (%d)\n", __func__, __LINE__);
>> +
>> +       tp->rtl_ops.disable(tp);
>> +}
>> +
>> +int r8152_eth_send(struct udevice *dev, void *packet, int length)
>> +{
>> +       struct r8152 *tp = dev_get_priv(dev);
>> +
>> +       return r8152_send_common(&tp->ueth, packet, length);
>> +}
>> +
>> +int r8152_eth_recv(struct udevice *dev, int flags, uchar **packetp)
>> +{
>> +       struct r8152 *tp = dev_get_priv(dev);
>> +       struct ueth_data *ueth = &tp->ueth;
>> +       uint8_t *ptr;
>> +       int ret, len;
>> +       struct rx_desc *rx_desc;
>> +       u16 packet_len;
>> +
>> +       len = usb_ether_get_rx_bytes(ueth, &ptr);
>> +       debug("%s: first try, len=%d\n", __func__, len);
>> +       if (!len) {
>> +               if (!(flags & ETH_RECV_CHECK_DEVICE))
>> +                       return -EAGAIN;
>> +               ret = usb_ether_receive(ueth, RTL8152_AGG_BUF_SZ);
>> +               if (ret == -EAGAIN)
>> +                       return ret;
> 
> What about if ret returns some other error?

Good catch. Will fix in v2.

BTW: I've cloned this part from other drivers (e.g. smsc95xx.c
and asix.c). So they would need some additional error handling here
as well.
 
>> +
>> +               len = usb_ether_get_rx_bytes(ueth, &ptr);
>> +               debug("%s: second try, len=%d\n", __func__, len);
>> +       }
>> +
>> +       rx_desc = (struct rx_desc *)ptr;
>> +       packet_len = le32_to_cpu(rx_desc->opts1) & RX_LEN_MASK;
>> +       packet_len -= CRC_SIZE;
>> +
>> +       if (packet_len > len - (sizeof(struct rx_desc) + CRC_SIZE)) {
>> +               debug("Rx: too large packet: %d\n", packet_len);
>> +               goto err;
>> +       }
>> +
>> +       *packetp = ptr + sizeof(struct rx_desc);
>> +       return packet_len;
>> +
>> +err:
>> +       usb_ether_advance_rxbuf(ueth, -1);
>> +       return -EINVAL;
> 
> -E2BIG?

Hmmm. This does not seem appropriate here:

#define	E2BIG		7		/* Argument list too long */

Or is it commonly used also for non argument list return failure?
 
>> +}
>> +
>> +static int r8152_free_pkt(struct udevice *dev, uchar *packet, int packet_len)
>> +{
>> +       struct r8152 *tp = dev_get_priv(dev);
>> +
>> +       packet_len += sizeof(struct rx_desc) + CRC_SIZE;
>> +       packet_len = ALIGN(packet_len, 8);
>> +       usb_ether_advance_rxbuf(&tp->ueth, packet_len);
>> +
>> +       return 0;
>> +}
>> +
>> +static int r8152_write_hwaddr(struct udevice *dev)
>> +{
>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>> +       struct r8152 *tp = dev_get_priv(dev);
>> +
>> +       unsigned char enetaddr[8] = { 0 };
>> +
>> +       debug("** %s (%d)\n", __func__, __LINE__);
>> +       memcpy(enetaddr, pdata->enetaddr, ETH_ALEN);
>> +
>> +       ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_CONFIG);
>> +       pla_ocp_write(tp, PLA_IDR, BYTE_EN_SIX_BYTES, 8, enetaddr);
>> +       ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CRWECR, CRWECR_NORAML);
>> +
>> +       debug("MAC %pM\n", pdata->enetaddr);
>> +       return 0;
>> +}
>> +
>> +int r8152_read_rom_hwaddr(struct udevice *dev)
>> +{
>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>> +       struct r8152 *tp = dev_get_priv(dev);
>> +
>> +       debug("** %s (%d)\n", __func__, __LINE__);
>> +       r8152_read_mac(tp, pdata->enetaddr);
>> +       return 0;
>> +}
>> +
>> +static int r8152_eth_probe(struct udevice *dev)
>> +{
>> +       struct usb_device *udev = dev_get_parent_priv(dev);
>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>> +       struct r8152 *tp = dev_get_priv(dev);
>> +       struct ueth_data *ueth = &tp->ueth;
>> +
>> +       tp->udev = udev;
>> +       r8152_read_mac(tp, pdata->enetaddr);
>> +
>> +       r8152b_get_version(tp);
>> +
>> +       if (rtl_ops_init(tp))
>> +               return 0;
> 
> Why 0? Isn't this an error?

Right. Will fix in v2.

Thanks for the review,
Stefan