[U-Boot] [PATCH resend v3 6/6] drivers: usb: gadget: ether/rndis: convert driver to adopt device driver model
Mugunthan V N
mugunthanvnm at ti.com
Wed Nov 30 06:31:11 CET 2016
On Wednesday 30 November 2016 05:24 AM, Joe Hershberger wrote:
> On Thu, Nov 17, 2016 at 11:39 PM, Mugunthan V N <mugunthanvnm at ti.com> wrote:
>> Adopt usb ether gadget and rndis driver to adopt driver model
>>
>> Signed-off-by: Mugunthan V N <mugunthanvnm at ti.com>
>> ---
>> drivers/usb/gadget/Kconfig | 4 ++
>> drivers/usb/gadget/ether.c | 153 ++++++++++++++++++++++++++++++++++++++++++---
>> drivers/usb/gadget/rndis.c | 13 +++-
>> drivers/usb/gadget/rndis.h | 19 ++++--
>> include/net.h | 7 +++
>> 5 files changed, 181 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 40839d89e9..261ed128ac 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -112,6 +112,10 @@ config G_DNL_VENDOR_NUM
>> config G_DNL_PRODUCT_NUM
>> hex "Product ID of USB device"
>>
>> +config USBNET_DEVADDR
>> + string "USB Gadget Ethernet device mac address"
>> + default "de:ad:be:ef:00:01"
>
> Please don't do this. We don't have "default" MAC addresses. They are
> either from the env, from ROM, or randomly generated.
>
Okay will remove this and use either env MAC or random MAC.
>> +
>> endif # USB_GADGET_DOWNLOAD
>>
>> endif # USB_GADGET
>> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
>> index 046ad8ca2b..8c3c3fd9ab 100644
>> --- a/drivers/usb/gadget/ether.c
>> +++ b/drivers/usb/gadget/ether.c
>> @@ -25,6 +25,7 @@
>> #include "rndis.h"
>>
>> #include <dm.h>
>> +#include <dm/lists.h>
>> #include <dm/uclass-internal.h>
>> #include <dm/device-internal.h>
>>
>> @@ -116,7 +117,11 @@ struct eth_dev {
>>
>> struct usb_request *tx_req, *rx_req;
>>
>> +#ifndef CONFIG_DM_ETH
>
> Please use positive logic.
Okay, will fix in next version.
>
>> struct eth_device *net;
>> +#else
>> + struct udevice *net;
>> +#endif
>> struct net_device_stats stats;
>> unsigned int tx_qlen;
>>
>> @@ -143,7 +148,11 @@ struct eth_dev {
>> /*-------------------------------------------------------------------------*/
>> struct ether_priv {
>> struct eth_dev ethdev;
>> +#ifndef CONFIG_DM_ETH
>
> Please use positive logic
>
>> struct eth_device netdev;
>> +#else
>> + struct udevice *netdev;
>
> Did you really intend to have a pointer here when the other is an
> inline structure?
Yes, udevice is the device allocated by probe and passed in probe, but
in non-dm case netdev has to be allocated by driver, in this case it
declared as inline structure.
>
>> +#endif
>> struct usb_gadget_driver eth_driver;
>> };
>>
>> @@ -1851,7 +1860,11 @@ static void rndis_control_ack_complete(struct usb_ep *ep,
>>
>> static char rndis_resp_buf[8] __attribute__((aligned(sizeof(__le32))));
>>
>> +#ifndef CONFIG_DM_ETH
>
> Please use positive logic.
>
>> static int rndis_control_ack(struct eth_device *net)
>> +#else
>> +static int rndis_control_ack(struct udevice *net)
>> +#endif
>> {
>> struct ether_priv *priv = (struct ether_priv *)net->priv;
>> struct eth_dev *dev = &priv->ethdev;
>> @@ -2001,6 +2014,9 @@ static int eth_bind(struct usb_gadget *gadget)
>> int status = -ENOMEM;
>> int gcnum;
>> u8 tmp[7];
>> +#ifdef CONFIG_DM_ETH
>> + struct eth_pdata *pdata = dev_get_platdata(l_priv->netdev);
>> +#endif
>>
>> /* these flags are only ever cleared; compiler take note */
>> #ifndef CONFIG_USB_ETH_CDC
>> @@ -2188,7 +2204,11 @@ autoconf_fail:
>>
>>
>> /* network device setup */
>> +#ifndef CONFIG_DM_ETH
>> dev->net = &l_priv->netdev;
>
> You wouldn't need this difference if the priv also used a ptr in the
> non-dm case.
>
> Also, if you are opposed to cleaning this up (preferably by adding a
> preceding patch to make it a pointer), at least use positive logic
> (#ifdef CONFIG_DM_ETH). Same applies elsewhere.
Okay, will add a patch to convert netdev to pointer
>
>> +#else
>> + dev->net = l_priv->netdev;
>> +#endif
>>
>> dev->cdc = cdc;
>> dev->zlp = zlp;
>> @@ -2197,6 +2217,7 @@ autoconf_fail:
>> dev->out_ep = out_ep;
>> dev->status_ep = status_ep;
>>
>> + memset(tmp, 0, sizeof(tmp));
>> /*
>> * Module params for these addresses should come from ID proms.
>> * The host side address is used with CDC and RNDIS, and commonly
>> @@ -2204,10 +2225,13 @@ autoconf_fail:
>> * host side code for the SAFE thing cares -- its original BLAN
>> * thing didn't, Sharp never assigned those addresses on Zaurii.
>> */
>> +#ifndef CONFIG_DM_ETH
>> get_ether_addr(dev_addr, dev->net->enetaddr);
>> -
>> - memset(tmp, 0, sizeof(tmp));
>> memcpy(tmp, dev->net->enetaddr, sizeof(dev->net->enetaddr));
>> +#else
>> + get_ether_addr(dev_addr, pdata->enetaddr);
>> + memcpy(tmp, pdata->enetaddr, sizeof(pdata->enetaddr));
>> +#endif
>>
>> get_ether_addr(host_addr, dev->host_mac);
>>
>> @@ -2268,10 +2292,11 @@ autoconf_fail:
>> status_ep ? " STATUS " : "",
>> status_ep ? status_ep->name : ""
>> );
>> - printf("MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
>> - dev->net->enetaddr[0], dev->net->enetaddr[1],
>> - dev->net->enetaddr[2], dev->net->enetaddr[3],
>> - dev->net->enetaddr[4], dev->net->enetaddr[5]);
>> +#ifndef CONFIG_DM_ETH
>> + printf("MAC %pM\n", dev->net->enetaddr);
>> +#else
>> + printf("MAC %pM\n", pdata->enetaddr);
>> +#endif
>>
>> if (cdc || rndis)
>> printf("HOST MAC %02x:%02x:%02x:%02x:%02x:%02x\n",
>> @@ -2520,13 +2545,12 @@ void _usb_eth_halt(struct ether_priv *priv)
>> }
>>
>> usb_gadget_unregister_driver(&priv->eth_driver);
>> -#ifdef CONFIG_DM_USB
>> - device_remove(dev->usb_udev);
>
> Why is this not needed anymore?
Oops, thought of moveing device remove to .stop, but missed. Now
realizing that this change is not needed as it will break !DM_ETH and
DM_USB configuration.
>
>> -#else
>> +#ifndef CONFIG_DM_USB
>> board_usb_cleanup(0, USB_INIT_DEVICE);
>> #endif
>> }
>>
>> +#ifndef CONFIG_DM_ETH
>> static int usb_eth_init(struct eth_device *netdev, bd_t *bd)
>> {
>> struct ether_priv *priv = (struct ether_priv *)netdev->priv;
>> @@ -2593,3 +2617,114 @@ int usb_eth_initialize(bd_t *bi)
>> eth_register(netdev);
>> return 0;
>> }
>> +#else
>> +static int usb_eth_start(struct udevice *dev)
>> +{
>> + struct ether_priv *priv = dev_get_priv(dev);
>> +
>> + return _usb_eth_init(priv);
>> +}
>> +
>> +static int usb_eth_send(struct udevice *dev, void *packet, int length)
>> +{
>> + struct ether_priv *priv = dev_get_priv(dev);
>> +
>> + return _usb_eth_send(priv, packet, length);
>> +}
>> +
>> +static int usb_eth_recv(struct udevice *dev, int flags, uchar **packetp)
>> +{
>> + struct ether_priv *priv = dev_get_priv(dev);
>> + struct eth_dev *ethdev = &priv->ethdev;
>> + int ret;
>> +
>> + ret = _usb_eth_recv(priv);
>> + if (ret) {
>> + error("error packet receive\n");
>> + return ret;
>> + }
>> +
>> + if (packet_received) {
>> + if (ethdev->rx_req) {
>> + *packetp = (uchar *)net_rx_packets[0];
>> + return ethdev->rx_req->length;
>> + } else {
>> + error("dev->rx_req invalid");
>> + return -EFAULT;
>> + }
>> + }
>> +
>> + return -EAGAIN;
>> +}
>> +
>> +static int usb_eth_free_pkt(struct udevice *dev, uchar *packet,
>> + int length)
>> +{
>> + struct ether_priv *priv = dev_get_priv(dev);
>> + struct eth_dev *ethdev = &priv->ethdev;
>> +
>> + packet_received = 0;
>> +
>> + return rx_submit(ethdev, ethdev->rx_req, 0);
>> +}
>> +
>> +static void usb_eth_stop(struct udevice *dev)
>> +{
>> + struct ether_priv *priv = dev_get_priv(dev);
>> +
>> + _usb_eth_halt(priv);
>> +}
>> +
>> +static int usb_eth_probe(struct udevice *dev)
>> +{
>> + struct ether_priv *priv = dev_get_priv(dev);
>> + struct eth_pdata *pdata = dev_get_platdata(dev);
>> +
>> + priv->netdev = dev;
>> + l_priv = priv;
>> +
>> + get_ether_addr(CONFIG_USBNET_DEVADDR, pdata->enetaddr);
>
> This function it just a separately implemented copy of other functions
> the net stack already provides. Assuming you should be manipulating
> the MAC addresses from some string at all, at least it should be using
> eth_parse_enetaddr().
>
>> + eth_setenv_enetaddr("usbnet_devaddr", pdata->enetaddr);
>
> This means that it will always clobber user's env. That's no good.
> Also, UCLASS_ETH already deals with mac address names for USB NICs as
> just another ethaddr, not a special usbnet name. Why not let that
> work?
Sure, will verify this and remove this section. Let network stack handle it.
>
>> +
>> + return 0;
>> +}
>> +
>> +static const struct eth_ops usb_eth_ops = {
>> + .start = usb_eth_start,
>> + .send = usb_eth_send,
>> + .recv = usb_eth_recv,
>> + .free_pkt = usb_eth_free_pkt,
>> + .stop = usb_eth_stop,
>> +};
>> +
>> +int usb_ether_init(void)
>> +{
>> + struct udevice *dev;
>> + struct udevice *usb_dev;
>> + int ret;
>> +
>> + ret = uclass_first_device(UCLASS_USB_DEV_GENERIC, &usb_dev);
>> + if (!usb_dev || ret) {
>> + error("No USB device found\n");
>> + return ret;
>> + }
>> +
>> + ret = device_bind_driver(usb_dev, "usb_ether", "usb_ether", &dev);
>> + if (!dev || ret) {
>> + error("usb - not able to bind usb_ether device\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +U_BOOT_DRIVER(eth_usb) = {
>> + .name = "usb_ether",
>> + .id = UCLASS_ETH,
>> + .probe = usb_eth_probe,
>> + .ops = &usb_eth_ops,
>> + .priv_auto_alloc_size = sizeof(struct ether_priv),
>> + .platdata_auto_alloc_size = sizeof(struct eth_pdata),
>> + .flags = DM_FLAG_ALLOC_PRIV_DMA,
>> +};
>> +#endif /* CONFIG_DM_ETH */
>
> It seems that this driver now supports 3 configurations.
>
> DM_ETH + DM_USB
> !DM_ETH + DM_USB
> !DM_ETH + !DM_USB
>
> Since it doesn't support DM_ETH + !DM_USB, you should probably
> explicitly error in that case by checking at the top of the file.
>
Will add an error at the top of the file.
Regards
Mugunthan V N
>> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
>> index 844a0c7236..5ad481302b 100644
>> --- a/drivers/usb/gadget/rndis.c
>> +++ b/drivers/usb/gadget/rndis.c
>> @@ -1121,7 +1121,11 @@ int rndis_msg_parser(u8 configNr, u8 *buf)
>> return -ENOTSUPP;
>> }
>>
>> +#ifndef CONFIG_DM_ETH
>> int rndis_register(int (*rndis_control_ack)(struct eth_device *))
>> +#else
>> +int rndis_register(int (*rndis_control_ack)(struct udevice *))
>> +#endif
>> {
>> u8 i;
>>
>> @@ -1149,8 +1153,13 @@ void rndis_deregister(int configNr)
>> return;
>> }
>>
>> -int rndis_set_param_dev(u8 configNr, struct eth_device *dev, int mtu,
>> - struct net_device_stats *stats, u16 *cdc_filter)
>> +#ifndef CONFIG_DM_ETH
>> +int rndis_set_param_dev(u8 configNr, struct eth_device *dev, int mtu,
>> + struct net_device_stats *stats, u16 *cdc_filter)
>> +#else
>> +int rndis_set_param_dev(u8 configNr, struct udevice *dev, int mtu,
>> + struct net_device_stats *stats, u16 *cdc_filter)
>> +#endif
>> {
>> debug("%s: configNr = %d\n", __func__, configNr);
>> if (!dev || !stats)
>> diff --git a/drivers/usb/gadget/rndis.h b/drivers/usb/gadget/rndis.h
>> index 7a389a580a..084af8541c 100644
>> --- a/drivers/usb/gadget/rndis.h
>> +++ b/drivers/usb/gadget/rndis.h
>> @@ -222,23 +222,34 @@ typedef struct rndis_params {
>>
>> const u8 *host_mac;
>> u16 *filter;
>> - struct eth_device *dev;
>> struct net_device_stats *stats;
>> int mtu;
>>
>> u32 vendorID;
>> const char *vendorDescr;
>> - int (*ack)(struct eth_device *);
>> +#ifndef CONFIG_DM_ETH
>> + struct eth_device *dev;
>> + int (*ack)(struct eth_device *);
>> +#else
>> + struct udevice *dev;
>> + int (*ack)(struct udevice *);
>> +#endif
>> struct list_head resp_queue;
>> } rndis_params;
>>
>> /* RNDIS Message parser and other useless functions */
>> int rndis_msg_parser(u8 configNr, u8 *buf);
>> enum rndis_state rndis_get_state(int configNr);
>> -int rndis_register(int (*rndis_control_ack)(struct eth_device *));
>> void rndis_deregister(int configNr);
>> +#ifndef CONFIG_DM_ETH
>> +int rndis_register(int (*rndis_control_ack)(struct eth_device *));
>> int rndis_set_param_dev(u8 configNr, struct eth_device *dev, int mtu,
>> - struct net_device_stats *stats, u16 *cdc_filter);
>> + struct net_device_stats *stats, u16 *cdc_filter);
>> +#else
>> +int rndis_register(int (*rndis_control_ack)(struct udevice *));
>> +int rndis_set_param_dev(u8 configNr, struct udevice *dev, int mtu,
>> + struct net_device_stats *stats, u16 *cdc_filter);
>> +#endif
>> int rndis_set_param_vendor(u8 configNr, u32 vendorID,
>> const char *vendorDescr);
>> int rndis_set_param_medium(u8 configNr, u32 medium, u32 speed);
>> diff --git a/include/net.h b/include/net.h
>> index 06320c6514..1f4d947350 100644
>> --- a/include/net.h
>> +++ b/include/net.h
>> @@ -255,6 +255,13 @@ int eth_setenv_enetaddr_by_index(const char *base_name, int index,
>>
>>
>> /*
>> + * Initialize USB ethernet device with CONFIG_DM_ETH
>> + * Returns:
>> + * 0 is success, non-zero is error status.
>> + */
>> +int usb_ether_init(void);
>> +
>> +/*
>> * Get the hardware address for an ethernet interface .
>> * Args:
>> * base_name - base name for device (normally "eth")
>> --
>> 2.11.0.rc2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list