[U-Boot] [PATCH resend v3 6/6] drivers: usb: gadget: ether/rndis: convert driver to adopt device driver model
Joe Hershberger
joe.hershberger at gmail.com
Wed Nov 30 00:54:53 CET 2016
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.
> +
> 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.
> 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?
> +#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.
> +#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?
> -#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?
> +
> + 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.
> 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