On Wednesday 30 November 2016 05:24 AM, Joe Hershberger wrote: > On Thu, Nov 17, 2016 at 11:39 PM, Mugunthan V N <mugunthan...@ti.com> wrote: >> Adopt usb ether gadget and rndis driver to adopt driver model >> >> Signed-off-by: Mugunthan V N <mugunthan...@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@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot