| Message ID | 1542535751-16079-9-git-send-email-pawell@cadence.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduced new Cadence USBSS DRD Driver | expand |
On 18/11/18 12:09, Pawel Laszczak wrote: > Patch adds implementation callback function defined in > usb_gadget_ops object. > > Signed-off-by: Pawel Laszczak <pawell@cadence.com> > --- > drivers/usb/cdns3/gadget.c | 249 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 247 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c > index 376b68b13d1b..702a05faa664 100644 > --- a/drivers/usb/cdns3/gadget.c > +++ b/drivers/usb/cdns3/gadget.c > @@ -17,6 +17,36 @@ > #include "gadget-export.h" > #include "gadget.h" > > +/** > + * cdns3_handshake - spin reading until handshake completes or fails > + * @ptr: address of device controller register to be read > + * @mask: bits to look at in result of read > + * @done: value of those bits when handshake succeeds > + * @usec: timeout in microseconds > + * > + * Returns negative errno, or zero on success > + * > + * Success happens when the "mask" bits have the specified value (hardware > + * handshake done). There are two failure modes: "usec" have passed (major > + * hardware flakeout), or the register reads as all-ones (hardware removed). > + */ > +int cdns3_handshake(void __iomem *ptr, u32 mask, u32 done, int usec) > +{ > + u32 result; > + > + do { > + result = readl(ptr); > + if (result == ~(u32)0) /* card removed */ > + return -ENODEV; Is this applicable to all registers? What is meant by card removed? We're not connected to host? how does EP reset behave when there is no USB connection? > + result &= mask; > + if (result == done) > + return 0; > + udelay(1); > + usec--; > + } while (usec > 0); > + return -ETIMEDOUT; > +} > + > /** > * cdns3_set_register_bit - set bit in given register. > * @ptr: address of device controller register to be read and changed > @@ -43,6 +73,25 @@ void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep) > writel(ep, &priv_dev->regs->ep_sel); > } > > +static void cdns3_free_trb_pool(struct cdns3_endpoint *priv_ep) > +{ > + struct cdns3_device *priv_dev = priv_ep->cdns3_dev; > + > + if (priv_ep->trb_pool) { > + dma_free_coherent(priv_dev->sysdev, > + TRB_RIGN_SIZE, > + priv_ep->trb_pool, priv_ep->trb_pool_dma); > + priv_ep->trb_pool = NULL; > + } > + > + if (priv_ep->aligned_buff) { > + dma_free_coherent(priv_dev->sysdev, CDNS3_UNALIGNED_BUF_SIZE, > + priv_ep->aligned_buff, > + priv_ep->aligned_dma_addr); > + priv_ep->aligned_buff = NULL; > + } > +} > + > /** > * cdns3_irq_handler - irq line interrupt handler > * @cdns: cdns3 instance > @@ -58,6 +107,114 @@ static irqreturn_t cdns3_irq_handler_thread(struct cdns3 *cdns) > return ret; > } > > +/* Find correct direction for HW endpoint according to description */ > +static int cdns3_ep_dir_is_correct(struct usb_endpoint_descriptor *desc, > + struct cdns3_endpoint *priv_ep) > +{ > + return (priv_ep->endpoint.caps.dir_in && usb_endpoint_dir_in(desc)) || > + (priv_ep->endpoint.caps.dir_out && usb_endpoint_dir_out(desc)); > +} > + > +static struct cdns3_endpoint *cdns3_find_available_ss_ep(struct cdns3_device *priv_dev, > + struct usb_endpoint_descriptor *desc) why is this function called ss_ep? This doesn't seem like only for superspeed endpoints. > +{ > + struct usb_ep *ep; > + struct cdns3_endpoint *priv_ep; > + > + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) { > + unsigned long num; > + int ret; > + /* ep name pattern likes epXin or epXout */ > + char c[2] = {ep->name[2], '\0'}; > + > + ret = kstrtoul(c, 10, &num); > + if (ret) > + return ERR_PTR(ret); > + > + priv_ep = ep_to_cdns3_ep(ep); > + if (cdns3_ep_dir_is_correct(desc, priv_ep)) { > + if (!(priv_ep->flags & EP_USED)) { > + priv_ep->num = num; > + priv_ep->flags |= EP_USED; > + return priv_ep; > + } > + } > + } > + return ERR_PTR(-ENOENT); > +} > + > +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, > + struct usb_endpoint_descriptor *desc, > + struct usb_ss_ep_comp_descriptor *comp_desc) > +{ > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > + struct cdns3_endpoint *priv_ep; > + unsigned long flags; > + > + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); > + if (IS_ERR(priv_ep)) { > + dev_err(&priv_dev->dev, "no available ep\n"); > + return NULL; > + } > + > + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); > + > + spin_lock_irqsave(&priv_dev->lock, flags); > + priv_ep->endpoint.desc = desc; > + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; > + priv_ep->type = usb_endpoint_type(desc); > + > + list_add_tail(&priv_ep->ep_match_pending_list, > + &priv_dev->ep_match_list); > + spin_unlock_irqrestore(&priv_dev->lock, flags); > + return &priv_ep->endpoint; > +} Why do you need a custom match_ep? doesn't usb_ep_autoconfig suffice? You can check if EP is claimed or not by checking the ep->claimed flag. > + > +/** > + * cdns3_gadget_get_frame Returns number of actual ITP frame > + * @gadget: gadget object > + * > + * Returns number of actual ITP frame > + */ > +static int cdns3_gadget_get_frame(struct usb_gadget *gadget) > +{ > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > + > + return readl(&priv_dev->regs->usb_iptn); > +} > + > +static int cdns3_gadget_wakeup(struct usb_gadget *gadget) > +{ > + return 0; > +} > + > +static int cdns3_gadget_set_selfpowered(struct usb_gadget *gadget, > + int is_selfpowered) > +{ > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > + unsigned long flags; > + > + spin_lock_irqsave(&priv_dev->lock, flags); > + gadget->is_selfpowered = !!is_selfpowered; > + spin_unlock_irqrestore(&priv_dev->lock, flags); > + return 0; > +} > + > +static int cdns3_gadget_pullup(struct usb_gadget *gadget, int is_on) > +{ > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > + > + if (!priv_dev->start_gadget) > + return 0; > + > + if (is_on) > + writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf); > + else > + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); > + > + return 0; > +} > + > static void cdns3_gadget_config(struct cdns3_device *priv_dev) > { > struct cdns3_usb_regs __iomem *regs = priv_dev->regs; > @@ -74,6 +231,95 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev) > writel(USB_CONF_DEVEN, ®s->usb_conf); > } > > +/** > + * cdns3_gadget_udc_start Gadget start > + * @gadget: gadget object > + * @driver: driver which operates on this gadget > + * > + * Returns 0 on success, error code elsewhere > + */ > +static int cdns3_gadget_udc_start(struct usb_gadget *gadget, > + struct usb_gadget_driver *driver) > +{ > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > + unsigned long flags; > + > + if (priv_dev->gadget_driver) { > + dev_err(&priv_dev->dev, "%s is already bound to %s\n", > + priv_dev->gadget.name, > + priv_dev->gadget_driver->driver.name); > + return -EBUSY; > + } Not sure if this check is required. UDC core should be doing that. > + > + spin_lock_irqsave(&priv_dev->lock, flags); > + priv_dev->gadget_driver = driver; > + if (!priv_dev->start_gadget) > + goto unlock; > + > + cdns3_gadget_config(priv_dev); > +unlock: > + spin_unlock_irqrestore(&priv_dev->lock, flags); > + return 0; > +} > + > +/** > + * cdns3_gadget_udc_stop Stops gadget > + * @gadget: gadget object > + * > + * Returns 0 > + */ > +static int cdns3_gadget_udc_stop(struct usb_gadget *gadget) > +{ > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > + struct cdns3_endpoint *priv_ep, *temp_ep; > + u32 bEndpointAddress; > + struct usb_ep *ep; > + int ret = 0; > + int i; > + > + priv_dev->gadget_driver = NULL; > + list_for_each_entry_safe(priv_ep, temp_ep, &priv_dev->ep_match_list, > + ep_match_pending_list) { > + list_del(&priv_ep->ep_match_pending_list); > + priv_ep->flags &= ~EP_USED; > + } > + > + priv_dev->onchip_mem_allocated_size = 0; > + priv_dev->out_mem_is_allocated = 0; > + priv_dev->gadget.speed = USB_SPEED_UNKNOWN; > + > + for (i = 0; i < priv_dev->ep_nums ; i++) > + cdns3_free_trb_pool(priv_dev->eps[i]); > + > + if (!priv_dev->start_gadget) > + return 0; This looks tricky. Why do we need this flag? > + > + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) { > + priv_ep = ep_to_cdns3_ep(ep); > + bEndpointAddress = priv_ep->num | priv_ep->dir; > + cdns3_select_ep(priv_dev, bEndpointAddress); > + writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd); > + ret = cdns3_handshake(&priv_dev->regs->ep_cmd, > + EP_CMD_EPRST, 0, 100); > + } > + > + /* disable interrupt for device */ > + writel(0, &priv_dev->regs->usb_ien); > + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); where are you requesting the interrupt? Looks like it should be done in udc_start() no? > + > + return ret; > +} Can we combine cdns3_gadget_udc_start() and cdns3_gadget_udc_start() with cdns3_gadget_start() and cdns3_gadget_stop() respectively so that cdns3_gadget_config() and cleanup() is done at one place. > + > +static const struct usb_gadget_ops cdns3_gadget_ops = { > + .get_frame = cdns3_gadget_get_frame, > + .wakeup = cdns3_gadget_wakeup, > + .set_selfpowered = cdns3_gadget_set_selfpowered, > + .pullup = cdns3_gadget_pullup, > + .udc_start = cdns3_gadget_udc_start, > + .udc_stop = cdns3_gadget_udc_stop, > + .match_ep = cdns3_gadget_match_ep, > +}; > + > /** > * cdns3_init_ep Initializes software endpoints of gadget > * @cdns3: extended gadget object > @@ -184,8 +430,7 @@ static int __cdns3_gadget_init(struct cdns3 *cdns) > /* fill gadget fields */ > priv_dev->gadget.max_speed = USB_SPEED_SUPER; > priv_dev->gadget.speed = USB_SPEED_UNKNOWN; > - //TODO: Add implementation of cdns3_gadget_ops > - //priv_dev->gadget.ops = &cdns3_gadget_ops; > + priv_dev->gadget.ops = &cdns3_gadget_ops; > priv_dev->gadget.name = "usb-ss-gadget"; > priv_dev->gadget.sg_supported = 1; > priv_dev->is_connected = 0; > cheers, -roger
Hi, >> Patch adds implementation callback function defined in >> usb_gadget_ops object. >> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com> >> --- >> drivers/usb/cdns3/gadget.c | 249 ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 247 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c >> index 376b68b13d1b..702a05faa664 100644 >> --- a/drivers/usb/cdns3/gadget.c >> +++ b/drivers/usb/cdns3/gadget.c >> @@ -17,6 +17,36 @@ >> #include "gadget-export.h" >> #include "gadget.h" >> >> +/** >> + * cdns3_handshake - spin reading until handshake completes or fails >> + * @ptr: address of device controller register to be read >> + * @mask: bits to look at in result of read >> + * @done: value of those bits when handshake succeeds >> + * @usec: timeout in microseconds >> + * >> + * Returns negative errno, or zero on success >> + * >> + * Success happens when the "mask" bits have the specified value (hardware >> + * handshake done). There are two failure modes: "usec" have passed (major >> + * hardware flakeout), or the register reads as all-ones (hardware removed). >> + */ >> +int cdns3_handshake(void __iomem *ptr, u32 mask, u32 done, int usec) >> +{ >> + u32 result; >> + >> + do { >> + result = readl(ptr); >> + if (result == ~(u32)0) /* card removed */ >> + return -ENODEV; > >Is this applicable to all registers? >What is meant by card removed? We're not connected to host? > >how does EP reset behave when there is no USB connection? > Function was adopted from XHCI driver (xhci_handshake). I think it's about PCI card. If this happens driver should read 0xffffffff from any register. I need to confirm it with hardware team or test it. I remember that I had such situation some time ago. My testing platform consist of separate FPGA board and PC. Both has PCI adapter and are connected by means of special PCI cable. >> + result &= mask; >> + if (result == done) >> + return 0; >> + udelay(1); >> + usec--; >> + } while (usec > 0); >> + return -ETIMEDOUT; >> +} >> + >> /** >> * cdns3_set_register_bit - set bit in given register. >> * @ptr: address of device controller register to be read and changed >> @@ -43,6 +73,25 @@ void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep) >> writel(ep, &priv_dev->regs->ep_sel); >> } >> >> +static void cdns3_free_trb_pool(struct cdns3_endpoint *priv_ep) >> +{ >> + struct cdns3_device *priv_dev = priv_ep->cdns3_dev; >> + >> + if (priv_ep->trb_pool) { >> + dma_free_coherent(priv_dev->sysdev, >> + TRB_RIGN_SIZE, >> + priv_ep->trb_pool, priv_ep->trb_pool_dma); >> + priv_ep->trb_pool = NULL; >> + } >> + >> + if (priv_ep->aligned_buff) { >> + dma_free_coherent(priv_dev->sysdev, CDNS3_UNALIGNED_BUF_SIZE, >> + priv_ep->aligned_buff, >> + priv_ep->aligned_dma_addr); >> + priv_ep->aligned_buff = NULL; >> + } >> +} >> + >> /** >> * cdns3_irq_handler - irq line interrupt handler >> * @cdns: cdns3 instance >> @@ -58,6 +107,114 @@ static irqreturn_t cdns3_irq_handler_thread(struct cdns3 *cdns) >> return ret; >> } >> >> +/* Find correct direction for HW endpoint according to description */ >> +static int cdns3_ep_dir_is_correct(struct usb_endpoint_descriptor *desc, >> + struct cdns3_endpoint *priv_ep) >> +{ >> + return (priv_ep->endpoint.caps.dir_in && usb_endpoint_dir_in(desc)) || >> + (priv_ep->endpoint.caps.dir_out && usb_endpoint_dir_out(desc)); >> +} >> + >> +static struct cdns3_endpoint *cdns3_find_available_ss_ep(struct cdns3_device *priv_dev, >> + struct usb_endpoint_descriptor *desc) > >why is this function called ss_ep? This doesn't seem like only for superspeed endpoints. I will remove _ss_. > >> +{ >> + struct usb_ep *ep; >> + struct cdns3_endpoint *priv_ep; >> + >> + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) { >> + unsigned long num; >> + int ret; >> + /* ep name pattern likes epXin or epXout */ >> + char c[2] = {ep->name[2], '\0'}; >> + >> + ret = kstrtoul(c, 10, &num); >> + if (ret) >> + return ERR_PTR(ret); >> + >> + priv_ep = ep_to_cdns3_ep(ep); >> + if (cdns3_ep_dir_is_correct(desc, priv_ep)) { >> + if (!(priv_ep->flags & EP_USED)) { >> + priv_ep->num = num; >> + priv_ep->flags |= EP_USED; >> + return priv_ep; >> + } >> + } >> + } >> + return ERR_PTR(-ENOENT); >> +} >> + >> +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, >> + struct usb_endpoint_descriptor *desc, >> + struct usb_ss_ep_comp_descriptor *comp_desc) >> +{ >> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >> + struct cdns3_endpoint *priv_ep; >> + unsigned long flags; >> + >> + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); >> + if (IS_ERR(priv_ep)) { >> + dev_err(&priv_dev->dev, "no available ep\n"); >> + return NULL; >> + } >> + >> + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); >> + >> + spin_lock_irqsave(&priv_dev->lock, flags); >> + priv_ep->endpoint.desc = desc; >> + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; >> + priv_ep->type = usb_endpoint_type(desc); >> + >> + list_add_tail(&priv_ep->ep_match_pending_list, >> + &priv_dev->ep_match_list); >> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> + return &priv_ep->endpoint; >> +} > >Why do you need a custom match_ep? >doesn't usb_ep_autoconfig suffice? I need to test it but at first glance it looks like usb_ep_autoconfig suffice. > >You can check if EP is claimed or not by checking the ep->claimed flag. > >> + >> +/** >> + * cdns3_gadget_get_frame Returns number of actual ITP frame >> + * @gadget: gadget object >> + * >> + * Returns number of actual ITP frame >> + */ >> +static int cdns3_gadget_get_frame(struct usb_gadget *gadget) >> +{ >> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >> + >> + return readl(&priv_dev->regs->usb_iptn); >> +} >> + >> +static int cdns3_gadget_wakeup(struct usb_gadget *gadget) >> +{ >> + return 0; >> +} >> + >> +static int cdns3_gadget_set_selfpowered(struct usb_gadget *gadget, >> + int is_selfpowered) >> +{ >> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&priv_dev->lock, flags); >> + gadget->is_selfpowered = !!is_selfpowered; >> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> + return 0; >> +} >> + >> +static int cdns3_gadget_pullup(struct usb_gadget *gadget, int is_on) >> +{ >> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >> + >> + if (!priv_dev->start_gadget) >> + return 0; >> + >> + if (is_on) >> + writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf); >> + else >> + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); >> + >> + return 0; >> +} >> + >> static void cdns3_gadget_config(struct cdns3_device *priv_dev) >> { >> struct cdns3_usb_regs __iomem *regs = priv_dev->regs; >> @@ -74,6 +231,95 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev) >> writel(USB_CONF_DEVEN, ®s->usb_conf); >> } >> >> +/** >> + * cdns3_gadget_udc_start Gadget start >> + * @gadget: gadget object >> + * @driver: driver which operates on this gadget >> + * >> + * Returns 0 on success, error code elsewhere >> + */ >> +static int cdns3_gadget_udc_start(struct usb_gadget *gadget, >> + struct usb_gadget_driver *driver) >> +{ >> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >> + unsigned long flags; >> + >> + if (priv_dev->gadget_driver) { >> + dev_err(&priv_dev->dev, "%s is already bound to %s\n", >> + priv_dev->gadget.name, >> + priv_dev->gadget_driver->driver.name); >> + return -EBUSY; >> + } > >Not sure if this check is required. UDC core should be doing that. > >> + >> + spin_lock_irqsave(&priv_dev->lock, flags); >> + priv_dev->gadget_driver = driver; >> + if (!priv_dev->start_gadget) >> + goto unlock; >> + >> + cdns3_gadget_config(priv_dev); >> +unlock: >> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> + return 0; >> +} >> + >> +/** >> + * cdns3_gadget_udc_stop Stops gadget >> + * @gadget: gadget object >> + * >> + * Returns 0 >> + */ >> +static int cdns3_gadget_udc_stop(struct usb_gadget *gadget) >> +{ >> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >> + struct cdns3_endpoint *priv_ep, *temp_ep; >> + u32 bEndpointAddress; >> + struct usb_ep *ep; >> + int ret = 0; >> + int i; >> + >> + priv_dev->gadget_driver = NULL; >> + list_for_each_entry_safe(priv_ep, temp_ep, &priv_dev->ep_match_list, >> + ep_match_pending_list) { >> + list_del(&priv_ep->ep_match_pending_list); >> + priv_ep->flags &= ~EP_USED; >> + } >> + >> + priv_dev->onchip_mem_allocated_size = 0; >> + priv_dev->out_mem_is_allocated = 0; >> + priv_dev->gadget.speed = USB_SPEED_UNKNOWN; >> + >> + for (i = 0; i < priv_dev->ep_nums ; i++) >> + cdns3_free_trb_pool(priv_dev->eps[i]); >> + >> + if (!priv_dev->start_gadget) >> + return 0; > >This looks tricky. Why do we need this flag? It will be removed in next version. It's no longer needed. > >> + >> + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) { >> + priv_ep = ep_to_cdns3_ep(ep); >> + bEndpointAddress = priv_ep->num | priv_ep->dir; >> + cdns3_select_ep(priv_dev, bEndpointAddress); >> + writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd); >> + ret = cdns3_handshake(&priv_dev->regs->ep_cmd, >> + EP_CMD_EPRST, 0, 100); >> + } >> + >> + /* disable interrupt for device */ >> + writel(0, &priv_dev->regs->usb_ien); >> + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); > >where are you requesting the interrupt? Looks like it should be done in >udc_start() no? Currently it is in initialization when the role is switched on. Interrupt is freeing during switching off role. So now the concept is the same as for host role. Whole device part is loaded and unloaded during changing role.. > >> + >> + return ret; >> +} > >Can we combine cdns3_gadget_udc_start() and cdns3_gadget_udc_start() >with cdns3_gadget_start() and cdns3_gadget_stop() respectively so that >cdns3_gadget_config() and cleanup() is done at one place. > Ok, probably it can be invoked only in cdns3_gadget_udc_start() and cdns3_gadget_udc_start(), but I have to test it. >> + >> +static const struct usb_gadget_ops cdns3_gadget_ops = { >> + .get_frame = cdns3_gadget_get_frame, >> + .wakeup = cdns3_gadget_wakeup, >> + .set_selfpowered = cdns3_gadget_set_selfpowered, >> + .pullup = cdns3_gadget_pullup, >> + .udc_start = cdns3_gadget_udc_start, >> + .udc_stop = cdns3_gadget_udc_stop, >> + .match_ep = cdns3_gadget_match_ep, >> +}; >> + >> /** >> * cdns3_init_ep Initializes software endpoints of gadget >> * @cdns3: extended gadget object >> @@ -184,8 +430,7 @@ static int __cdns3_gadget_init(struct cdns3 *cdns) >> /* fill gadget fields */ >> priv_dev->gadget.max_speed = USB_SPEED_SUPER; >> priv_dev->gadget.speed = USB_SPEED_UNKNOWN; >> - //TODO: Add implementation of cdns3_gadget_ops >> - //priv_dev->gadget.ops = &cdns3_gadget_ops; >> + priv_dev->gadget.ops = &cdns3_gadget_ops; >> priv_dev->gadget.name = "usb-ss-gadget"; >> priv_dev->gadget.sg_supported = 1; >> priv_dev->is_connected = 0; >> > Cheers Pawel
Hi >>> + >>> +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, >>> + struct usb_endpoint_descriptor *desc, >>> + struct usb_ss_ep_comp_descriptor *comp_desc) >>> +{ >>> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >>> + struct cdns3_endpoint *priv_ep; >>> + unsigned long flags; >>> + >>> + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); >>> + if (IS_ERR(priv_ep)) { >>> + dev_err(&priv_dev->dev, "no available ep\n"); >>> + return NULL; >>> + } >>> + >>> + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); >>> + >>> + spin_lock_irqsave(&priv_dev->lock, flags); >>> + priv_ep->endpoint.desc = desc; >>> + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; >>> + priv_ep->type = usb_endpoint_type(desc); >>> + >>> + list_add_tail(&priv_ep->ep_match_pending_list, >>> + &priv_dev->ep_match_list); >>> + spin_unlock_irqrestore(&priv_dev->lock, flags); >>> + return &priv_ep->endpoint; >>> +} >> >>Why do you need a custom match_ep? >>doesn't usb_ep_autoconfig suffice? > >I need to test it but at first glance it looks like usb_ep_autoconfig suffice. > >> >>You can check if EP is claimed or not by checking the ep->claimed flag. >> I checked it and did not work correct. The flag claimed is set in usb_ep_autoconf, but a little bit later during USB RESET this flag is cleared. So, I can't base on this flag. I think that it's incorrect behavior in gadget core driver or in function drivers. Maybe we should have additional flag in usb_ep object used during resetting, or usb_ep_autoconf function should be called after each USB RESET. I notice that I can based on ep->address it is set also in usb_ep_autoconf and probably is not cleared anywhere. It's little tricky, but It looks like it works correct. I add some comment for this checking. Maybe in feature I will be able to replace it with claimed flag. Felipe what's your opinion about claimed flag. It's should be fixed or not ? Cheers Pawel
> > +static struct cdns3_endpoint *cdns3_find_available_ss_ep(struct cdns3_device *priv_dev, > > + struct usb_endpoint_descriptor *desc) > > why is this function called ss_ep? This doesn't seem like only for superspeed endpoints. > Yes, it is for all speed. > > +{ > > + struct usb_ep *ep; > > + struct cdns3_endpoint *priv_ep; > > + > > + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) { > > + unsigned long num; > > + int ret; > > + /* ep name pattern likes epXin or epXout */ > > + char c[2] = {ep->name[2], '\0'}; > > + > > + ret = kstrtoul(c, 10, &num); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + priv_ep = ep_to_cdns3_ep(ep); > > + if (cdns3_ep_dir_is_correct(desc, priv_ep)) { > > + if (!(priv_ep->flags & EP_USED)) { > > + priv_ep->num = num; > > + priv_ep->flags |= EP_USED; > > + return priv_ep; > > + } > > + } > > + } > > + return ERR_PTR(-ENOENT); > > +} > > + > > +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, > > + struct usb_endpoint_descriptor *desc, > > + struct usb_ss_ep_comp_descriptor *comp_desc) > > +{ > > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > > + struct cdns3_endpoint *priv_ep; > > + unsigned long flags; > > + > > + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); > > + if (IS_ERR(priv_ep)) { > > + dev_err(&priv_dev->dev, "no available ep\n"); > > + return NULL; > > + } > > + > > + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); > > + > > + spin_lock_irqsave(&priv_dev->lock, flags); > > + priv_ep->endpoint.desc = desc; > > + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; > > + priv_ep->type = usb_endpoint_type(desc); > > + > > + list_add_tail(&priv_ep->ep_match_pending_list, > > + &priv_dev->ep_match_list); > > + spin_unlock_irqrestore(&priv_dev->lock, flags); > > + return &priv_ep->endpoint; > > +} > > Why do you need a custom match_ep? > doesn't usb_ep_autoconfig suffice? > > You can check if EP is claimed or not by checking the ep->claimed flag. > It is a special requirement for this IP, the EP type and MaxPacketSize changing can't be done at runtime, eg, at ep->enable. See below commit for detail. usb: cdns3: gadget: configure all endpoints before set configuration Cadence IP has one limitation that all endpoints must be configured (Type & MaxPacketSize) before setting configuration through hardware register, it means we can't change endpoints configuration after set_configuration. In this patch, we add non-control endpoint through usb_ss->ep_match_list, which is added when the gadget driver uses usb_ep_autoconfig to configure specific endpoint; When the udc driver receives set_configurion request, it goes through usb_ss->ep_match_list, and configure all endpoints accordingly. At usb_ep_ops.enable/disable, we only enable and disable endpoint through ep_cfg register which can be changed after set_configuration, and do some software operation accordingly. > > + > > +/** > > + * cdns3_gadget_get_frame Returns number of actual ITP frame > > + * @gadget: gadget object > > + * > > + * Returns number of actual ITP frame > > + */ > > +static int cdns3_gadget_get_frame(struct usb_gadget *gadget) > > +{ > > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > > + > > + return readl(&priv_dev->regs->usb_iptn); > > +} > > + > > +static int cdns3_gadget_wakeup(struct usb_gadget *gadget) > > +{ > > + return 0; > > +} > > + > > +static int cdns3_gadget_set_selfpowered(struct usb_gadget *gadget, > > + int is_selfpowered) > > +{ > > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > > + unsigned long flags; > > + > > + spin_lock_irqsave(&priv_dev->lock, flags); > > + gadget->is_selfpowered = !!is_selfpowered; > > + spin_unlock_irqrestore(&priv_dev->lock, flags); > > + return 0; > > +} > > + > > +static int cdns3_gadget_pullup(struct usb_gadget *gadget, int is_on) > > +{ > > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > > + > > + if (!priv_dev->start_gadget) > > + return 0; > > + > > + if (is_on) > > + writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf); > > + else > > + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); > > + > > + return 0; > > +} > > + > > static void cdns3_gadget_config(struct cdns3_device *priv_dev) > > { > > struct cdns3_usb_regs __iomem *regs = priv_dev->regs; > > @@ -74,6 +231,95 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev) > > writel(USB_CONF_DEVEN, ®s->usb_conf); > > } > > > > +/** > > + * cdns3_gadget_udc_start Gadget start > > + * @gadget: gadget object > > + * @driver: driver which operates on this gadget > > + * > > + * Returns 0 on success, error code elsewhere > > + */ > > +static int cdns3_gadget_udc_start(struct usb_gadget *gadget, > > + struct usb_gadget_driver *driver) > > +{ > > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > > + unsigned long flags; > > + > > + if (priv_dev->gadget_driver) { > > + dev_err(&priv_dev->dev, "%s is already bound to %s\n", > > + priv_dev->gadget.name, > > + priv_dev->gadget_driver->driver.name); > > + return -EBUSY; > > + } > > Not sure if this check is required. UDC core should be doing that. > Yes, UDC core has done this. > > + > > + spin_lock_irqsave(&priv_dev->lock, flags); > > + priv_dev->gadget_driver = driver; > > + if (!priv_dev->start_gadget) > > + goto unlock; > > + > > + cdns3_gadget_config(priv_dev); > > +unlock: > > + spin_unlock_irqrestore(&priv_dev->lock, flags); > > + return 0; > > +} > > + > > +/** > > + * cdns3_gadget_udc_stop Stops gadget > > + * @gadget: gadget object > > + * > > + * Returns 0 > > + */ > > +static int cdns3_gadget_udc_stop(struct usb_gadget *gadget) > > +{ > > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > > + struct cdns3_endpoint *priv_ep, *temp_ep; > > + u32 bEndpointAddress; > > + struct usb_ep *ep; > > + int ret = 0; > > + int i; > > + > > + priv_dev->gadget_driver = NULL; > > + list_for_each_entry_safe(priv_ep, temp_ep, &priv_dev->ep_match_list, > > + ep_match_pending_list) { > > + list_del(&priv_ep->ep_match_pending_list); > > + priv_ep->flags &= ~EP_USED; > > + } > > + > > + priv_dev->onchip_mem_allocated_size = 0; > > + priv_dev->out_mem_is_allocated = 0; > > + priv_dev->gadget.speed = USB_SPEED_UNKNOWN; > > + > > + for (i = 0; i < priv_dev->ep_nums ; i++) > > + cdns3_free_trb_pool(priv_dev->eps[i]); > > + > > + if (!priv_dev->start_gadget) > > + return 0; > > This looks tricky. Why do we need this flag? We only start the gadget (enable clocks) when the VBUS is on, so if we load class driver without VBUS, we only do software .bind but without hardware operation. > > > + > > + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) { > > + priv_ep = ep_to_cdns3_ep(ep); > > + bEndpointAddress = priv_ep->num | priv_ep->dir; > > + cdns3_select_ep(priv_dev, bEndpointAddress); > > + writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd); > > + ret = cdns3_handshake(&priv_dev->regs->ep_cmd, > > + EP_CMD_EPRST, 0, 100); > > + } > > + > > + /* disable interrupt for device */ > > + writel(0, &priv_dev->regs->usb_ien); > > + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); > > where are you requesting the interrupt? Looks like it should be done in > udc_start() no? > Yes, it is at udc_start. Peter > > + > > + return ret; > > +} > > Can we combine cdns3_gadget_udc_start() and cdns3_gadget_udc_start() > with cdns3_gadget_start() and cdns3_gadget_stop() respectively so that > cdns3_gadget_config() and cleanup() is done at one place. > > > + > > +static const struct usb_gadget_ops cdns3_gadget_ops = { > > + .get_frame = cdns3_gadget_get_frame, > > + .wakeup = cdns3_gadget_wakeup, > > + .set_selfpowered = cdns3_gadget_set_selfpowered, > > + .pullup = cdns3_gadget_pullup, > > + .udc_start = cdns3_gadget_udc_start, > > + .udc_stop = cdns3_gadget_udc_stop, > > + .match_ep = cdns3_gadget_match_ep, > > +}; > > + > > /** > > * cdns3_init_ep Initializes software endpoints of gadget > > * @cdns3: extended gadget object > > @@ -184,8 +430,7 @@ static int __cdns3_gadget_init(struct cdns3 *cdns) > > /* fill gadget fields */ > > priv_dev->gadget.max_speed = USB_SPEED_SUPER; > > priv_dev->gadget.speed = USB_SPEED_UNKNOWN; > > - //TODO: Add implementation of cdns3_gadget_ops > > - //priv_dev->gadget.ops = &cdns3_gadget_ops; > > + priv_dev->gadget.ops = &cdns3_gadget_ops; > > priv_dev->gadget.name = "usb-ss-gadget"; > > priv_dev->gadget.sg_supported = 1; > > priv_dev->is_connected = 0; > > > > cheers, > -roger > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 10/12/18 7:42 AM, Peter Chen wrote: >>> +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, >>> + struct usb_endpoint_descriptor *desc, >>> + struct usb_ss_ep_comp_descriptor *comp_desc) >>> +{ >>> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >>> + struct cdns3_endpoint *priv_ep; >>> + unsigned long flags; >>> + >>> + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); >>> + if (IS_ERR(priv_ep)) { >>> + dev_err(&priv_dev->dev, "no available ep\n"); >>> + return NULL; >>> + } >>> + >>> + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); >>> + >>> + spin_lock_irqsave(&priv_dev->lock, flags); >>> + priv_ep->endpoint.desc = desc; >>> + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; >>> + priv_ep->type = usb_endpoint_type(desc); >>> + >>> + list_add_tail(&priv_ep->ep_match_pending_list, >>> + &priv_dev->ep_match_list); >>> + spin_unlock_irqrestore(&priv_dev->lock, flags); >>> + return &priv_ep->endpoint; >>> +} >> Why do you need a custom match_ep? >> doesn't usb_ep_autoconfig suffice? >> >> You can check if EP is claimed or not by checking the ep->claimed flag. >> > It is a special requirement for this IP, the EP type and MaxPacketSize > changing can't be done at runtime, eg, at ep->enable. See below commit > for detail. > > usb: cdns3: gadget: configure all endpoints before set configuration > > Cadence IP has one limitation that all endpoints must be configured > (Type & MaxPacketSize) before setting configuration through hardware > register, it means we can't change endpoints configuration after > set_configuration. > > In this patch, we add non-control endpoint through usb_ss->ep_match_list, > which is added when the gadget driver uses usb_ep_autoconfig to configure > specific endpoint; When the udc driver receives set_configurion request, > it goes through usb_ss->ep_match_list, and configure all endpoints > accordingly. > > At usb_ep_ops.enable/disable, we only enable and disable endpoint through > ep_cfg register which can be changed after set_configuration, and do > some software operation accordingly. All this should be part of comments in code along with information about controller versions which suffer from the errata. Is there a version of controller available which does not have the defect? Is there a future plan to fix this? If any of that is yes, you probably want to handle this with runtime detection of version (like done with DWC3_REVISION_XXX macros). Sometimes the hardware-read versions themselves are incorrect, so its better to introduce a version specific compatible too like "cdns,usb-1.0.0" (as hinted to by Rob Herring as well). Thanks, Sekhar
Hi, >On 10/12/18 7:42 AM, Peter Chen wrote: >>>> +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, >>>> + struct usb_endpoint_descriptor *desc, >>>> + struct usb_ss_ep_comp_descriptor *comp_desc) >>>> +{ >>>> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >>>> + struct cdns3_endpoint *priv_ep; >>>> + unsigned long flags; >>>> + >>>> + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); >>>> + if (IS_ERR(priv_ep)) { >>>> + dev_err(&priv_dev->dev, "no available ep\n"); >>>> + return NULL; >>>> + } >>>> + >>>> + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); >>>> + >>>> + spin_lock_irqsave(&priv_dev->lock, flags); >>>> + priv_ep->endpoint.desc = desc; >>>> + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; >>>> + priv_ep->type = usb_endpoint_type(desc); >>>> + >>>> + list_add_tail(&priv_ep->ep_match_pending_list, >>>> + &priv_dev->ep_match_list); >>>> + spin_unlock_irqrestore(&priv_dev->lock, flags); >>>> + return &priv_ep->endpoint; >>>> +} >>> Why do you need a custom match_ep? >>> doesn't usb_ep_autoconfig suffice? >>> >>> You can check if EP is claimed or not by checking the ep->claimed flag. >>> >> It is a special requirement for this IP, the EP type and MaxPacketSize >> changing can't be done at runtime, eg, at ep->enable. See below commit >> for detail. >> >> usb: cdns3: gadget: configure all endpoints before set configuration >> >> Cadence IP has one limitation that all endpoints must be configured >> (Type & MaxPacketSize) before setting configuration through hardware >> register, it means we can't change endpoints configuration after >> set_configuration. >> >> In this patch, we add non-control endpoint through usb_ss->ep_match_list, >> which is added when the gadget driver uses usb_ep_autoconfig to configure >> specific endpoint; When the udc driver receives set_configurion request, >> it goes through usb_ss->ep_match_list, and configure all endpoints >> accordingly. >> >> At usb_ep_ops.enable/disable, we only enable and disable endpoint through >> ep_cfg register which can be changed after set_configuration, and do >> some software operation accordingly. > >All this should be part of comments in code along with information about >controller versions which suffer from the errata. > >Is there a version of controller available which does not have the >defect? Is there a future plan to fix this? > >If any of that is yes, you probably want to handle this with runtime >detection of version (like done with DWC3_REVISION_XXX macros). >Sometimes the hardware-read versions themselves are incorrect, so its >better to introduce a version specific compatible too like >"cdns,usb-1.0.0" (as hinted to by Rob Herring as well). > custom match_ep is used and works with all versions of the gen1 controller. Future (gen2) releases of the controller won’t have such limitation but there is no plan to change current (gen1) functionality of the controller. I will add comment before cdns3_gadget_match_ep function. Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional cdns,usb3-1.0.1 compatible. cdns,usb3-1.0.1 will be for current version of controller which I use. cdns,usb3-1.0.0 will be for older version - Peter Chan platform. I now that I have some changes in controller, and one of them require some changes in DRD driver. It will be safer to add two separate version in compatibles. Thanks Pawel
On Wed, Dec 12, 2018 at 3:49 AM Pawel Laszczak <pawell@cadence.com> wrote: > > Hi, > > >On 10/12/18 7:42 AM, Peter Chen wrote: > >>>> +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, > >>>> + struct usb_endpoint_descriptor *desc, > >>>> + struct usb_ss_ep_comp_descriptor *comp_desc) > >>>> +{ > >>>> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); > >>>> + struct cdns3_endpoint *priv_ep; > >>>> + unsigned long flags; > >>>> + > >>>> + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); > >>>> + if (IS_ERR(priv_ep)) { > >>>> + dev_err(&priv_dev->dev, "no available ep\n"); > >>>> + return NULL; > >>>> + } > >>>> + > >>>> + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); > >>>> + > >>>> + spin_lock_irqsave(&priv_dev->lock, flags); > >>>> + priv_ep->endpoint.desc = desc; > >>>> + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; > >>>> + priv_ep->type = usb_endpoint_type(desc); > >>>> + > >>>> + list_add_tail(&priv_ep->ep_match_pending_list, > >>>> + &priv_dev->ep_match_list); > >>>> + spin_unlock_irqrestore(&priv_dev->lock, flags); > >>>> + return &priv_ep->endpoint; > >>>> +} > >>> Why do you need a custom match_ep? > >>> doesn't usb_ep_autoconfig suffice? > >>> > >>> You can check if EP is claimed or not by checking the ep->claimed flag. > >>> > >> It is a special requirement for this IP, the EP type and MaxPacketSize > >> changing can't be done at runtime, eg, at ep->enable. See below commit > >> for detail. > >> > >> usb: cdns3: gadget: configure all endpoints before set configuration > >> > >> Cadence IP has one limitation that all endpoints must be configured > >> (Type & MaxPacketSize) before setting configuration through hardware > >> register, it means we can't change endpoints configuration after > >> set_configuration. > >> > >> In this patch, we add non-control endpoint through usb_ss->ep_match_list, > >> which is added when the gadget driver uses usb_ep_autoconfig to configure > >> specific endpoint; When the udc driver receives set_configurion request, > >> it goes through usb_ss->ep_match_list, and configure all endpoints > >> accordingly. > >> > >> At usb_ep_ops.enable/disable, we only enable and disable endpoint through > >> ep_cfg register which can be changed after set_configuration, and do > >> some software operation accordingly. > > > >All this should be part of comments in code along with information about > >controller versions which suffer from the errata. > > > >Is there a version of controller available which does not have the > >defect? Is there a future plan to fix this? > > > >If any of that is yes, you probably want to handle this with runtime > >detection of version (like done with DWC3_REVISION_XXX macros). > >Sometimes the hardware-read versions themselves are incorrect, so its > >better to introduce a version specific compatible too like > >"cdns,usb-1.0.0" (as hinted to by Rob Herring as well). > > > > custom match_ep is used and works with all versions of the gen1 > controller. Future (gen2) releases of the controller won’t have such > limitation but there is no plan to change current (gen1) functionality > of the controller. > > I will add comment before cdns3_gadget_match_ep function. > Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional > cdns,usb3-1.0.1 compatible. > > cdns,usb3-1.0.1 will be for current version of controller which I use. > cdns,usb3-1.0.0 will be for older version - Peter Chan platform. > I now that I have some changes in controller, and one of them require > some changes in DRD driver. It will be safer to add two separate > version in compatibles. > Pawel, could we have correct register to show controller version? It is better we could version judgement at runtime instead of static compatible. Peter
Hi, > >On Wed, Dec 12, 2018 at 3:49 AM Pawel Laszczak <pawell@cadence.com> wrote: >> >> Hi, >> >> >On 10/12/18 7:42 AM, Peter Chen wrote: >> >>>> +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, >> >>>> + struct usb_endpoint_descriptor *desc, >> >>>> + struct usb_ss_ep_comp_descriptor *comp_desc) >> >>>> +{ >> >>>> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >> >>>> + struct cdns3_endpoint *priv_ep; >> >>>> + unsigned long flags; >> >>>> + >> >>>> + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); >> >>>> + if (IS_ERR(priv_ep)) { >> >>>> + dev_err(&priv_dev->dev, "no available ep\n"); >> >>>> + return NULL; >> >>>> + } >> >>>> + >> >>>> + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); >> >>>> + >> >>>> + spin_lock_irqsave(&priv_dev->lock, flags); >> >>>> + priv_ep->endpoint.desc = desc; >> >>>> + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; >> >>>> + priv_ep->type = usb_endpoint_type(desc); >> >>>> + >> >>>> + list_add_tail(&priv_ep->ep_match_pending_list, >> >>>> + &priv_dev->ep_match_list); >> >>>> + spin_unlock_irqrestore(&priv_dev->lock, flags); >> >>>> + return &priv_ep->endpoint; >> >>>> +} >> >>> Why do you need a custom match_ep? >> >>> doesn't usb_ep_autoconfig suffice? >> >>> >> >>> You can check if EP is claimed or not by checking the ep->claimed flag. >> >>> >> >> It is a special requirement for this IP, the EP type and MaxPacketSize >> >> changing can't be done at runtime, eg, at ep->enable. See below commit >> >> for detail. >> >> >> >> usb: cdns3: gadget: configure all endpoints before set configuration >> >> >> >> Cadence IP has one limitation that all endpoints must be configured >> >> (Type & MaxPacketSize) before setting configuration through hardware >> >> register, it means we can't change endpoints configuration after >> >> set_configuration. >> >> >> >> In this patch, we add non-control endpoint through usb_ss->ep_match_list, >> >> which is added when the gadget driver uses usb_ep_autoconfig to configure >> >> specific endpoint; When the udc driver receives set_configurion request, >> >> it goes through usb_ss->ep_match_list, and configure all endpoints >> >> accordingly. >> >> >> >> At usb_ep_ops.enable/disable, we only enable and disable endpoint through >> >> ep_cfg register which can be changed after set_configuration, and do >> >> some software operation accordingly. >> > >> >All this should be part of comments in code along with information about >> >controller versions which suffer from the errata. >> > >> >Is there a version of controller available which does not have the >> >defect? Is there a future plan to fix this? >> > >> >If any of that is yes, you probably want to handle this with runtime >> >detection of version (like done with DWC3_REVISION_XXX macros). >> >Sometimes the hardware-read versions themselves are incorrect, so its >> >better to introduce a version specific compatible too like >> >"cdns,usb-1.0.0" (as hinted to by Rob Herring as well). >> > >> >> custom match_ep is used and works with all versions of the gen1 >> controller. Future (gen2) releases of the controller won’t have such >> limitation but there is no plan to change current (gen1) functionality >> of the controller. >> >> I will add comment before cdns3_gadget_match_ep function. >> Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional >> cdns,usb3-1.0.1 compatible. >> >> cdns,usb3-1.0.1 will be for current version of controller which I use. >> cdns,usb3-1.0.0 will be for older version - Peter Chan platform. >> I now that I have some changes in controller, and one of them require >> some changes in DRD driver. It will be safer to add two separate >> version in compatibles. >> > >Pawel, could we have correct register to show controller version? It is >better we could version judgement at runtime instead of static compatible. > Ok, I will tray do this in this way. Pawel
On 14/12/18 7:04 AM, Peter Chen wrote: > On Wed, Dec 12, 2018 at 3:49 AM Pawel Laszczak <pawell@cadence.com> wrote: >> >> Hi, >> >>> On 10/12/18 7:42 AM, Peter Chen wrote: >>>>>> +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, >>>>>> + struct usb_endpoint_descriptor *desc, >>>>>> + struct usb_ss_ep_comp_descriptor *comp_desc) >>>>>> +{ >>>>>> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); >>>>>> + struct cdns3_endpoint *priv_ep; >>>>>> + unsigned long flags; >>>>>> + >>>>>> + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); >>>>>> + if (IS_ERR(priv_ep)) { >>>>>> + dev_err(&priv_dev->dev, "no available ep\n"); >>>>>> + return NULL; >>>>>> + } >>>>>> + >>>>>> + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); >>>>>> + >>>>>> + spin_lock_irqsave(&priv_dev->lock, flags); >>>>>> + priv_ep->endpoint.desc = desc; >>>>>> + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; >>>>>> + priv_ep->type = usb_endpoint_type(desc); >>>>>> + >>>>>> + list_add_tail(&priv_ep->ep_match_pending_list, >>>>>> + &priv_dev->ep_match_list); >>>>>> + spin_unlock_irqrestore(&priv_dev->lock, flags); >>>>>> + return &priv_ep->endpoint; >>>>>> +} >>>>> Why do you need a custom match_ep? >>>>> doesn't usb_ep_autoconfig suffice? >>>>> >>>>> You can check if EP is claimed or not by checking the ep->claimed flag. >>>>> >>>> It is a special requirement for this IP, the EP type and MaxPacketSize >>>> changing can't be done at runtime, eg, at ep->enable. See below commit >>>> for detail. >>>> >>>> usb: cdns3: gadget: configure all endpoints before set configuration >>>> >>>> Cadence IP has one limitation that all endpoints must be configured >>>> (Type & MaxPacketSize) before setting configuration through hardware >>>> register, it means we can't change endpoints configuration after >>>> set_configuration. >>>> >>>> In this patch, we add non-control endpoint through usb_ss->ep_match_list, >>>> which is added when the gadget driver uses usb_ep_autoconfig to configure >>>> specific endpoint; When the udc driver receives set_configurion request, >>>> it goes through usb_ss->ep_match_list, and configure all endpoints >>>> accordingly. >>>> >>>> At usb_ep_ops.enable/disable, we only enable and disable endpoint through >>>> ep_cfg register which can be changed after set_configuration, and do >>>> some software operation accordingly. >>> >>> All this should be part of comments in code along with information about >>> controller versions which suffer from the errata. >>> >>> Is there a version of controller available which does not have the >>> defect? Is there a future plan to fix this? >>> >>> If any of that is yes, you probably want to handle this with runtime >>> detection of version (like done with DWC3_REVISION_XXX macros). >>> Sometimes the hardware-read versions themselves are incorrect, so its >>> better to introduce a version specific compatible too like >>> "cdns,usb-1.0.0" (as hinted to by Rob Herring as well). >>> >> >> custom match_ep is used and works with all versions of the gen1 >> controller. Future (gen2) releases of the controller won’t have such >> limitation but there is no plan to change current (gen1) functionality >> of the controller. >> >> I will add comment before cdns3_gadget_match_ep function. >> Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional >> cdns,usb3-1.0.1 compatible. >> >> cdns,usb3-1.0.1 will be for current version of controller which I use. >> cdns,usb3-1.0.0 will be for older version - Peter Chan platform. >> I now that I have some changes in controller, and one of them require >> some changes in DRD driver. It will be safer to add two separate >> version in compatibles. >> > > Pawel, could we have correct register to show controller version? It is > better we could version judgement at runtime instead of static compatible. Agree with detecting IP version at runtime. But please have some indication of version in compatible string too, especially since you already know there is going to be another revision of hardware. It has the advantage that one can easily grep to see which hardware is running current version of controller without having access to the hardware itself. Becomes useful later on when its time to clean-up unused code when boards become obsolete or for requesting testing help. Thanks, Sekhar
Hi, Sekhar Nori <nsekhar@ti.com> writes: <snip> >>>> All this should be part of comments in code along with information about >>>> controller versions which suffer from the errata. >>>> >>>> Is there a version of controller available which does not have the >>>> defect? Is there a future plan to fix this? >>>> >>>> If any of that is yes, you probably want to handle this with runtime >>>> detection of version (like done with DWC3_REVISION_XXX macros). >>>> Sometimes the hardware-read versions themselves are incorrect, so its >>>> better to introduce a version specific compatible too like >>>> "cdns,usb-1.0.0" (as hinted to by Rob Herring as well). >>>> >>> >>> custom match_ep is used and works with all versions of the gen1 >>> controller. Future (gen2) releases of the controller won’t have such >>> limitation but there is no plan to change current (gen1) functionality >>> of the controller. >>> >>> I will add comment before cdns3_gadget_match_ep function. >>> Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional >>> cdns,usb3-1.0.1 compatible. >>> >>> cdns,usb3-1.0.1 will be for current version of controller which I use. >>> cdns,usb3-1.0.0 will be for older version - Peter Chan platform. >>> I now that I have some changes in controller, and one of them require >>> some changes in DRD driver. It will be safer to add two separate >>> version in compatibles. >>> >> >> Pawel, could we have correct register to show controller version? It is >> better we could version judgement at runtime instead of static compatible. > > Agree with detecting IP version at runtime. > > But please have some indication of version in compatible string too, why? Runtime detection by revision register should be the way to go if the HW provides it. Why duplicate the information in compatible string? > especially since you already know there is going to be another revision > of hardware. It has the advantage that one can easily grep to see which > hardware is running current version of controller without having access > to the hardware itself. Becomes useful later on when its time to > clean-up unused code when boards become obsolete or for requesting > testing help. This doesn't sound like a very strong argument, actually. Specially when you consider that, since driver will do revision checking based on revision register, you already have strings to grep. Moreover, we don't usually drop support just like that. Personally, I wouldn't bother. Just like dwc3 never bothered and nothing bad came about because of it. Well, there are quirks which are undetectable and for those we have quirk flags. I much prefer that arrangement.
Hi Felipe, On 14/12/18 4:17 PM, Felipe Balbi wrote: > Hi, > > Sekhar Nori <nsekhar@ti.com> writes: > > <snip> > >>>>> All this should be part of comments in code along with information about >>>>> controller versions which suffer from the errata. >>>>> >>>>> Is there a version of controller available which does not have the >>>>> defect? Is there a future plan to fix this? >>>>> >>>>> If any of that is yes, you probably want to handle this with runtime >>>>> detection of version (like done with DWC3_REVISION_XXX macros). >>>>> Sometimes the hardware-read versions themselves are incorrect, so its >>>>> better to introduce a version specific compatible too like >>>>> "cdns,usb-1.0.0" (as hinted to by Rob Herring as well). >>>>> >>>> >>>> custom match_ep is used and works with all versions of the gen1 >>>> controller. Future (gen2) releases of the controller won’t have such >>>> limitation but there is no plan to change current (gen1) functionality >>>> of the controller. >>>> >>>> I will add comment before cdns3_gadget_match_ep function. >>>> Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional >>>> cdns,usb3-1.0.1 compatible. >>>> >>>> cdns,usb3-1.0.1 will be for current version of controller which I use. >>>> cdns,usb3-1.0.0 will be for older version - Peter Chan platform. >>>> I now that I have some changes in controller, and one of them require >>>> some changes in DRD driver. It will be safer to add two separate >>>> version in compatibles. >>>> >>> >>> Pawel, could we have correct register to show controller version? It is >>> better we could version judgement at runtime instead of static compatible. >> >> Agree with detecting IP version at runtime. >> >> But please have some indication of version in compatible string too, > > why? Runtime detection by revision register should be the way to go if > the HW provides it. Why duplicate the information in compatible string? > >> especially since you already know there is going to be another revision >> of hardware. It has the advantage that one can easily grep to see which >> hardware is running current version of controller without having access >> to the hardware itself. Becomes useful later on when its time to >> clean-up unused code when boards become obsolete or for requesting >> testing help. > > This doesn't sound like a very strong argument, actually. Specially when > you consider that, since driver will do revision checking based on > revision register, you already have strings to grep. Moreover, we don't > usually drop support just like that. AFAICS, it is impossible to know just by grep'ing if there is any hardware still supported in kernel and using DWC3_REVISION_194A, for example. If we are never going to drop support for any revision, this does not matter much. Also, once you have the controller supported behind PCI, then I guess you are pretty much tied to having to read hardware revision at runtime. > Personally, I wouldn't bother. Just like dwc3 never bothered and nothing > bad came about because of it. Well, there are quirks which are > undetectable and for those we have quirk flags. I much prefer that > arrangement. Yes, quirk flags will work too for undetectable quirks. Thanks, Sekhar
Hi, Sekhar Nori <nsekhar@ti.com> writes: >>>>>> All this should be part of comments in code along with information about >>>>>> controller versions which suffer from the errata. >>>>>> >>>>>> Is there a version of controller available which does not have the >>>>>> defect? Is there a future plan to fix this? >>>>>> >>>>>> If any of that is yes, you probably want to handle this with runtime >>>>>> detection of version (like done with DWC3_REVISION_XXX macros). >>>>>> Sometimes the hardware-read versions themselves are incorrect, so its >>>>>> better to introduce a version specific compatible too like >>>>>> "cdns,usb-1.0.0" (as hinted to by Rob Herring as well). >>>>>> >>>>> >>>>> custom match_ep is used and works with all versions of the gen1 >>>>> controller. Future (gen2) releases of the controller won’t have such >>>>> limitation but there is no plan to change current (gen1) functionality >>>>> of the controller. >>>>> >>>>> I will add comment before cdns3_gadget_match_ep function. >>>>> Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional >>>>> cdns,usb3-1.0.1 compatible. >>>>> >>>>> cdns,usb3-1.0.1 will be for current version of controller which I use. >>>>> cdns,usb3-1.0.0 will be for older version - Peter Chan platform. >>>>> I now that I have some changes in controller, and one of them require >>>>> some changes in DRD driver. It will be safer to add two separate >>>>> version in compatibles. >>>>> >>>> >>>> Pawel, could we have correct register to show controller version? It is >>>> better we could version judgement at runtime instead of static compatible. >>> >>> Agree with detecting IP version at runtime. >>> >>> But please have some indication of version in compatible string too, >> >> why? Runtime detection by revision register should be the way to go if >> the HW provides it. Why duplicate the information in compatible string? >> >>> especially since you already know there is going to be another revision >>> of hardware. It has the advantage that one can easily grep to see which >>> hardware is running current version of controller without having access >>> to the hardware itself. Becomes useful later on when its time to >>> clean-up unused code when boards become obsolete or for requesting >>> testing help. >> >> This doesn't sound like a very strong argument, actually. Specially when >> you consider that, since driver will do revision checking based on >> revision register, you already have strings to grep. Moreover, we don't >> usually drop support just like that. > > AFAICS, it is impossible to know just by grep'ing if there is any > hardware still supported in kernel and using DWC3_REVISION_194A, for > example. but why do you even care? > If we are never going to drop support for any revision, this does not > matter much. > > Also, once you have the controller supported behind PCI, then I guess > you are pretty much tied to having to read hardware revision at runtime. that's another argument *for* using runtime detection, not against it.
On 14/12/18 4:56 PM, Felipe Balbi wrote: > Hi, > > Sekhar Nori <nsekhar@ti.com> writes: >>>>>>> All this should be part of comments in code along with information about >>>>>>> controller versions which suffer from the errata. >>>>>>> >>>>>>> Is there a version of controller available which does not have the >>>>>>> defect? Is there a future plan to fix this? >>>>>>> >>>>>>> If any of that is yes, you probably want to handle this with runtime >>>>>>> detection of version (like done with DWC3_REVISION_XXX macros). >>>>>>> Sometimes the hardware-read versions themselves are incorrect, so its >>>>>>> better to introduce a version specific compatible too like >>>>>>> "cdns,usb-1.0.0" (as hinted to by Rob Herring as well). >>>>>>> >>>>>> >>>>>> custom match_ep is used and works with all versions of the gen1 >>>>>> controller. Future (gen2) releases of the controller won’t have such >>>>>> limitation but there is no plan to change current (gen1) functionality >>>>>> of the controller. >>>>>> >>>>>> I will add comment before cdns3_gadget_match_ep function. >>>>>> Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional >>>>>> cdns,usb3-1.0.1 compatible. >>>>>> >>>>>> cdns,usb3-1.0.1 will be for current version of controller which I use. >>>>>> cdns,usb3-1.0.0 will be for older version - Peter Chan platform. >>>>>> I now that I have some changes in controller, and one of them require >>>>>> some changes in DRD driver. It will be safer to add two separate >>>>>> version in compatibles. >>>>>> >>>>> >>>>> Pawel, could we have correct register to show controller version? It is >>>>> better we could version judgement at runtime instead of static compatible. >>>> >>>> Agree with detecting IP version at runtime. >>>> >>>> But please have some indication of version in compatible string too, >>> >>> why? Runtime detection by revision register should be the way to go if >>> the HW provides it. Why duplicate the information in compatible string? >>> >>>> especially since you already know there is going to be another revision >>>> of hardware. It has the advantage that one can easily grep to see which >>>> hardware is running current version of controller without having access >>>> to the hardware itself. Becomes useful later on when its time to >>>> clean-up unused code when boards become obsolete or for requesting >>>> testing help. >>> >>> This doesn't sound like a very strong argument, actually. Specially when >>> you consider that, since driver will do revision checking based on >>> revision register, you already have strings to grep. Moreover, we don't >>> usually drop support just like that. >> >> AFAICS, it is impossible to know just by grep'ing if there is any >> hardware still supported in kernel and using DWC3_REVISION_194A, for >> example. > > but why do you even care? When, for example, its coming in the way of some clean-up I am attempting to do. > >> If we are never going to drop support for any revision, this does not >> matter much. >> >> Also, once you have the controller supported behind PCI, then I guess >> you are pretty much tied to having to read hardware revision at runtime. > > that's another argument *for* using runtime detection, not against it. I know :). I should have stated that in last e-mail itself, I am okay with just runtime detection. Thanks, Sekhar
Hi, Sekhar Nori <nsekhar@ti.com> writes: >>>>> especially since you already know there is going to be another revision >>>>> of hardware. It has the advantage that one can easily grep to see which >>>>> hardware is running current version of controller without having access >>>>> to the hardware itself. Becomes useful later on when its time to >>>>> clean-up unused code when boards become obsolete or for requesting >>>>> testing help. >>>> >>>> This doesn't sound like a very strong argument, actually. Specially when >>>> you consider that, since driver will do revision checking based on >>>> revision register, you already have strings to grep. Moreover, we don't >>>> usually drop support just like that. >>> >>> AFAICS, it is impossible to know just by grep'ing if there is any >>> hardware still supported in kernel and using DWC3_REVISION_194A, for >>> example. >> >> but why do you even care? > > When, for example, its coming in the way of some clean-up I am > attempting to do. can you share one example when a revision check got in the way of a cleanup? I fail to see a situation when we need to drop support to older platforms just to clean some code up.
Hi > >On 14/12/18 4:56 PM, Felipe Balbi wrote: >> Hi, >> >> Sekhar Nori <nsekhar@ti.com> writes: >>>>>>>> All this should be part of comments in code along with information about >>>>>>>> controller versions which suffer from the errata. >>>>>>>> >>>>>>>> Is there a version of controller available which does not have the >>>>>>>> defect? Is there a future plan to fix this? >>>>>>>> >>>>>>>> If any of that is yes, you probably want to handle this with runtime >>>>>>>> detection of version (like done with DWC3_REVISION_XXX macros). >>>>>>>> Sometimes the hardware-read versions themselves are incorrect, so its >>>>>>>> better to introduce a version specific compatible too like >>>>>>>> "cdns,usb-1.0.0" (as hinted to by Rob Herring as well). >>>>>>>> >>>>>>> >>>>>>> custom match_ep is used and works with all versions of the gen1 >>>>>>> controller. Future (gen2) releases of the controller won’t have such >>>>>>> limitation but there is no plan to change current (gen1) functionality >>>>>>> of the controller. >>>>>>> >>>>>>> I will add comment before cdns3_gadget_match_ep function. >>>>>>> Also I will change cdns,usb3 to cdns,usb3-1.0.0 and add additional >>>>>>> cdns,usb3-1.0.1 compatible. >>>>>>> >>>>>>> cdns,usb3-1.0.1 will be for current version of controller which I use. >>>>>>> cdns,usb3-1.0.0 will be for older version - Peter Chan platform. >>>>>>> I now that I have some changes in controller, and one of them require >>>>>>> some changes in DRD driver. It will be safer to add two separate >>>>>>> version in compatibles. >>>>>>> >>>>>> >>>>>> Pawel, could we have correct register to show controller version? It is >>>>>> better we could version judgement at runtime instead of static compatible. >>>>> >>>>> Agree with detecting IP version at runtime. >>>>> >>>>> But please have some indication of version in compatible string too, >>>> >>>> why? Runtime detection by revision register should be the way to go if >>>> the HW provides it. Why duplicate the information in compatible string? >>>> >>>>> especially since you already know there is going to be another revision >>>>> of hardware. It has the advantage that one can easily grep to see which >>>>> hardware is running current version of controller without having access >>>>> to the hardware itself. Becomes useful later on when its time to >>>>> clean-up unused code when boards become obsolete or for requesting >>>>> testing help. >>>> >>>> This doesn't sound like a very strong argument, actually. Specially when >>>> you consider that, since driver will do revision checking based on >>>> revision register, you already have strings to grep. Moreover, we don't >>>> usually drop support just like that. >>> >>> AFAICS, it is impossible to know just by grep'ing if there is any >>> hardware still supported in kernel and using DWC3_REVISION_194A, for >>> example. >> >> but why do you even care? > >When, for example, its coming in the way of some clean-up I am >attempting to do. > >> >>> If we are never going to drop support for any revision, this does not >>> matter much. >>> >>> Also, once you have the controller supported behind PCI, then I guess >>> you are pretty much tied to having to read hardware revision at runtime. >> >> that's another argument *for* using runtime detection, not against it. > >I know :). I should have stated that in last e-mail itself, I am okay >with just runtime detection. I agree with you. Controller has usb_cap6 register that keep device controller version. It's not a problem doing such detection at runtime. I will do it in this way. But also I will add extra compatible to dt-binding. Even if this will not be used in driver, it informs that there are several versions of controller. Thanks, Pawel
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c index 376b68b13d1b..702a05faa664 100644 --- a/drivers/usb/cdns3/gadget.c +++ b/drivers/usb/cdns3/gadget.c @@ -17,6 +17,36 @@ #include "gadget-export.h" #include "gadget.h" +/** + * cdns3_handshake - spin reading until handshake completes or fails + * @ptr: address of device controller register to be read + * @mask: bits to look at in result of read + * @done: value of those bits when handshake succeeds + * @usec: timeout in microseconds + * + * Returns negative errno, or zero on success + * + * Success happens when the "mask" bits have the specified value (hardware + * handshake done). There are two failure modes: "usec" have passed (major + * hardware flakeout), or the register reads as all-ones (hardware removed). + */ +int cdns3_handshake(void __iomem *ptr, u32 mask, u32 done, int usec) +{ + u32 result; + + do { + result = readl(ptr); + if (result == ~(u32)0) /* card removed */ + return -ENODEV; + result &= mask; + if (result == done) + return 0; + udelay(1); + usec--; + } while (usec > 0); + return -ETIMEDOUT; +} + /** * cdns3_set_register_bit - set bit in given register. * @ptr: address of device controller register to be read and changed @@ -43,6 +73,25 @@ void cdns3_select_ep(struct cdns3_device *priv_dev, u32 ep) writel(ep, &priv_dev->regs->ep_sel); } +static void cdns3_free_trb_pool(struct cdns3_endpoint *priv_ep) +{ + struct cdns3_device *priv_dev = priv_ep->cdns3_dev; + + if (priv_ep->trb_pool) { + dma_free_coherent(priv_dev->sysdev, + TRB_RIGN_SIZE, + priv_ep->trb_pool, priv_ep->trb_pool_dma); + priv_ep->trb_pool = NULL; + } + + if (priv_ep->aligned_buff) { + dma_free_coherent(priv_dev->sysdev, CDNS3_UNALIGNED_BUF_SIZE, + priv_ep->aligned_buff, + priv_ep->aligned_dma_addr); + priv_ep->aligned_buff = NULL; + } +} + /** * cdns3_irq_handler - irq line interrupt handler * @cdns: cdns3 instance @@ -58,6 +107,114 @@ static irqreturn_t cdns3_irq_handler_thread(struct cdns3 *cdns) return ret; } +/* Find correct direction for HW endpoint according to description */ +static int cdns3_ep_dir_is_correct(struct usb_endpoint_descriptor *desc, + struct cdns3_endpoint *priv_ep) +{ + return (priv_ep->endpoint.caps.dir_in && usb_endpoint_dir_in(desc)) || + (priv_ep->endpoint.caps.dir_out && usb_endpoint_dir_out(desc)); +} + +static struct cdns3_endpoint *cdns3_find_available_ss_ep(struct cdns3_device *priv_dev, + struct usb_endpoint_descriptor *desc) +{ + struct usb_ep *ep; + struct cdns3_endpoint *priv_ep; + + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) { + unsigned long num; + int ret; + /* ep name pattern likes epXin or epXout */ + char c[2] = {ep->name[2], '\0'}; + + ret = kstrtoul(c, 10, &num); + if (ret) + return ERR_PTR(ret); + + priv_ep = ep_to_cdns3_ep(ep); + if (cdns3_ep_dir_is_correct(desc, priv_ep)) { + if (!(priv_ep->flags & EP_USED)) { + priv_ep->num = num; + priv_ep->flags |= EP_USED; + return priv_ep; + } + } + } + return ERR_PTR(-ENOENT); +} + +static struct usb_ep *cdns3_gadget_match_ep(struct usb_gadget *gadget, + struct usb_endpoint_descriptor *desc, + struct usb_ss_ep_comp_descriptor *comp_desc) +{ + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); + struct cdns3_endpoint *priv_ep; + unsigned long flags; + + priv_ep = cdns3_find_available_ss_ep(priv_dev, desc); + if (IS_ERR(priv_ep)) { + dev_err(&priv_dev->dev, "no available ep\n"); + return NULL; + } + + dev_dbg(&priv_dev->dev, "match endpoint: %s\n", priv_ep->name); + + spin_lock_irqsave(&priv_dev->lock, flags); + priv_ep->endpoint.desc = desc; + priv_ep->dir = usb_endpoint_dir_in(desc) ? USB_DIR_IN : USB_DIR_OUT; + priv_ep->type = usb_endpoint_type(desc); + + list_add_tail(&priv_ep->ep_match_pending_list, + &priv_dev->ep_match_list); + spin_unlock_irqrestore(&priv_dev->lock, flags); + return &priv_ep->endpoint; +} + +/** + * cdns3_gadget_get_frame Returns number of actual ITP frame + * @gadget: gadget object + * + * Returns number of actual ITP frame + */ +static int cdns3_gadget_get_frame(struct usb_gadget *gadget) +{ + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); + + return readl(&priv_dev->regs->usb_iptn); +} + +static int cdns3_gadget_wakeup(struct usb_gadget *gadget) +{ + return 0; +} + +static int cdns3_gadget_set_selfpowered(struct usb_gadget *gadget, + int is_selfpowered) +{ + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); + unsigned long flags; + + spin_lock_irqsave(&priv_dev->lock, flags); + gadget->is_selfpowered = !!is_selfpowered; + spin_unlock_irqrestore(&priv_dev->lock, flags); + return 0; +} + +static int cdns3_gadget_pullup(struct usb_gadget *gadget, int is_on) +{ + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); + + if (!priv_dev->start_gadget) + return 0; + + if (is_on) + writel(USB_CONF_DEVEN, &priv_dev->regs->usb_conf); + else + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); + + return 0; +} + static void cdns3_gadget_config(struct cdns3_device *priv_dev) { struct cdns3_usb_regs __iomem *regs = priv_dev->regs; @@ -74,6 +231,95 @@ static void cdns3_gadget_config(struct cdns3_device *priv_dev) writel(USB_CONF_DEVEN, ®s->usb_conf); } +/** + * cdns3_gadget_udc_start Gadget start + * @gadget: gadget object + * @driver: driver which operates on this gadget + * + * Returns 0 on success, error code elsewhere + */ +static int cdns3_gadget_udc_start(struct usb_gadget *gadget, + struct usb_gadget_driver *driver) +{ + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); + unsigned long flags; + + if (priv_dev->gadget_driver) { + dev_err(&priv_dev->dev, "%s is already bound to %s\n", + priv_dev->gadget.name, + priv_dev->gadget_driver->driver.name); + return -EBUSY; + } + + spin_lock_irqsave(&priv_dev->lock, flags); + priv_dev->gadget_driver = driver; + if (!priv_dev->start_gadget) + goto unlock; + + cdns3_gadget_config(priv_dev); +unlock: + spin_unlock_irqrestore(&priv_dev->lock, flags); + return 0; +} + +/** + * cdns3_gadget_udc_stop Stops gadget + * @gadget: gadget object + * + * Returns 0 + */ +static int cdns3_gadget_udc_stop(struct usb_gadget *gadget) +{ + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget); + struct cdns3_endpoint *priv_ep, *temp_ep; + u32 bEndpointAddress; + struct usb_ep *ep; + int ret = 0; + int i; + + priv_dev->gadget_driver = NULL; + list_for_each_entry_safe(priv_ep, temp_ep, &priv_dev->ep_match_list, + ep_match_pending_list) { + list_del(&priv_ep->ep_match_pending_list); + priv_ep->flags &= ~EP_USED; + } + + priv_dev->onchip_mem_allocated_size = 0; + priv_dev->out_mem_is_allocated = 0; + priv_dev->gadget.speed = USB_SPEED_UNKNOWN; + + for (i = 0; i < priv_dev->ep_nums ; i++) + cdns3_free_trb_pool(priv_dev->eps[i]); + + if (!priv_dev->start_gadget) + return 0; + + list_for_each_entry(ep, &priv_dev->gadget.ep_list, ep_list) { + priv_ep = ep_to_cdns3_ep(ep); + bEndpointAddress = priv_ep->num | priv_ep->dir; + cdns3_select_ep(priv_dev, bEndpointAddress); + writel(EP_CMD_EPRST, &priv_dev->regs->ep_cmd); + ret = cdns3_handshake(&priv_dev->regs->ep_cmd, + EP_CMD_EPRST, 0, 100); + } + + /* disable interrupt for device */ + writel(0, &priv_dev->regs->usb_ien); + writel(USB_CONF_DEVDS, &priv_dev->regs->usb_conf); + + return ret; +} + +static const struct usb_gadget_ops cdns3_gadget_ops = { + .get_frame = cdns3_gadget_get_frame, + .wakeup = cdns3_gadget_wakeup, + .set_selfpowered = cdns3_gadget_set_selfpowered, + .pullup = cdns3_gadget_pullup, + .udc_start = cdns3_gadget_udc_start, + .udc_stop = cdns3_gadget_udc_stop, + .match_ep = cdns3_gadget_match_ep, +}; + /** * cdns3_init_ep Initializes software endpoints of gadget * @cdns3: extended gadget object @@ -184,8 +430,7 @@ static int __cdns3_gadget_init(struct cdns3 *cdns) /* fill gadget fields */ priv_dev->gadget.max_speed = USB_SPEED_SUPER; priv_dev->gadget.speed = USB_SPEED_UNKNOWN; - //TODO: Add implementation of cdns3_gadget_ops - //priv_dev->gadget.ops = &cdns3_gadget_ops; + priv_dev->gadget.ops = &cdns3_gadget_ops; priv_dev->gadget.name = "usb-ss-gadget"; priv_dev->gadget.sg_supported = 1; priv_dev->is_connected = 0;
Patch adds implementation callback function defined in usb_gadget_ops object. Signed-off-by: Pawel Laszczak <pawell@cadence.com> --- drivers/usb/cdns3/gadget.c | 249 ++++++++++++++++++++++++++++++++++++- 1 file changed, 247 insertions(+), 2 deletions(-)