On Fri, 2018-03-09 at 11:20 +0200, Felipe Balbi wrote:
>
> > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > new file mode 100644
> > index 000000000000..31ed2b6e241b
> > --- /dev/null
> > +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> > @@ -0,0 +1,429 @@
>
> missing SPDX license identifier (all files)
Ah yup, the driver predates me knowing about them, I'll fix.
> > +static bool force_usb1 = false;
> > +static bool no_dma_desc = false;
> > +
> > +module_param_named(force_usb1, force_usb1, bool, 0644);
> > +MODULE_PARM_DESC(force_usb1, "Set to true to force to USB1 speed");
> > +module_param_named(no_dma_desc, no_dma_desc, bool, 0644);
> > +MODULE_PARM_DESC(no_dma_desc, "Set to true to disable use of DMA descriptors");
>
> module parameters? Sorry, but no.
Why ? Any suggestion then for the two above ? They are mostly meant as
diagnostic/debug features if something doesn't work (for example, the
board has some sub-standard wiring making USB2 unreliable, or a driver
bug with DMA descriptors).
I can just take them out completely but it feels like module parameters
are the right thing to do in that specific case.
> > +void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req *req,
> > + int status)
> > +{
> > + bool internal = req->internal;
> > +
> > + EPVDBG(ep, "completing request @%p, status %d\n", req, status);
> > +
> > + list_del_init(&req->queue);
> > +
> > + if (req->req.status == -EINPROGRESS)
> > + req->req.status = status;
> > +
> > + if (req->req.dma) {
> > + if (!WARN_ON(!ep->dev))
> > + usb_gadget_unmap_request(&ep->dev->gadget,
> > + &req->req, ep->epn.is_in);
> > + req->req.dma = 0;
> > + }
> > +
> > + /*
> > + * If this isn't an internal EP0 request, call the core
> > + * to call the gadget completion.
> > + */
> > + if (!internal) {
> > + spin_unlock(&ep->vhub->lock);
> > + usb_gadget_giveback_request(&ep->ep, &req->req);
> > + spin_lock(&ep->vhub->lock);
> > + }
> > +}
> > +
> > +void ast_vhub_nuke(struct ast_vhub_ep *ep, int status)
> > +{
> > + struct ast_vhub_req *req;
> > +
> > + EPDBG(ep, "Nuking\n");
> > +
> > + /* terminate any request in the queue */
> > + if (list_empty(&ep->queue))
> > + return;
>
> unnecessary test.
Right.
>
> > +
> > + /* Beware, lock will be dropped & req-acquired by done() */
> > + while (!list_empty(&ep->queue)) {
> > + req = list_first_entry(&ep->queue, struct ast_vhub_req, queue);
>
> list_for_each_entry_safe() ??
No, list_for_each_entry_safe is about the current entry being removed
by the loop. In this case, we drop the lock in done(), so any entry
including the next one could go away concurrently. This is why this
construct is necessary.
> > + ast_vhub_done(ep, req, status);
> > + }
> > +}
> > +
> > +struct usb_request *ast_vhub_alloc_request(struct usb_ep *u_ep,
> > + gfp_t gfp_flags)
> > +{
> > + struct ast_vhub_req *req;
> > +
> > + req = kzalloc(sizeof(*req), gfp_flags);
> > + if (!req)
> > + return NULL;
>
> Don't you wanna keep a reference to the endpoint that owns $this
> request?
What for ? I haven't had needed one so far...
> > + INIT_LIST_HEAD(&req->queue);
>
> unnecessary list initialization
Ah right, I wanted to poison it actually but just leaving it NULL is
probably fine.
> > + return &req->req;
> > +}
> > +
> > +void ast_vhub_free_request(struct usb_ep *u_ep, struct usb_request *u_req)
> > +{
> > + struct ast_vhub_req *req = to_ast_req(u_req);
> > +
> > + kfree(req);
> > +}
> > +
> > +static irqreturn_t ast_vhub_irq (int irq, void *data)
>
> ^
> unnecessary space
Ok.
> > +{
> > + struct ast_vhub *vhub = data;
> > + irqreturn_t iret = IRQ_NONE;
> > + u32 istat;
> > +
> > + /* Stale interrupt while tearing down */
> > + if (!vhub->ep0_bufs)
> > + return IRQ_NONE;
> > +
> > + spin_lock(&vhub->lock);
>
> this is your hardirq handler right? Do you really need this lock here?
Yup, pretty much everything it calls needs to be protected, it also
protects vs a concurrent remove.
> > +
> > + /* Read and ACK interrupts */
> > + istat = readl(vhub->regs + AST_VHUB_ISR);
> > + if (!istat)
> > + goto bail;
> > + writel(istat, vhub->regs + AST_VHUB_ISR);
> > + iret = IRQ_HANDLED;
> > +
> > + UDCVDBG(vhub, "irq status=%08x, ep_acks=%08x ep_nacks=%08x\n",
> > + istat,
> > + readl(vhub->regs + AST_VHUB_EP_ACK_ISR),
> > + readl(vhub->regs + AST_VHUB_EP_NACK_ISR));
> > +
> > + /* Handle generic EPs first */
> > + if (istat & VHUB_IRQ_EP_POOL_ACK_STALL) {
> > + u32 i, ep_acks = readl(vhub->regs + AST_VHUB_EP_ACK_ISR);
> > + writel(ep_acks, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > +
> > + /* Check if we have a useful ffs on arm ... */
> > + for (i = 0; ep_acks && i < AST_VHUB_NUM_GEN_EPs; i++) {
> > + u32 mask = 1 << i;
> > + if (ep_acks & mask) {
> > + ast_vhub_epn_ack_irq(&vhub->epns[i]);
> > + ep_acks &= ~mask;
> > + }
> > + }
> > + }
> > +
> > + /* Handle device interrupts */
> > + if (istat & (VHUB_IRQ_DEVICE1 |
> > + VHUB_IRQ_DEVICE2 |
> > + VHUB_IRQ_DEVICE3 |
> > + VHUB_IRQ_DEVICE4 |
> > + VHUB_IRQ_DEVICE5)) {
> > + if (istat & VHUB_IRQ_DEVICE1)
> > + ast_vhub_dev_irq(&vhub->ports[0].dev);
> > + if (istat & VHUB_IRQ_DEVICE2)
> > + ast_vhub_dev_irq(&vhub->ports[1].dev);
> > + if (istat & VHUB_IRQ_DEVICE3)
> > + ast_vhub_dev_irq(&vhub->ports[2].dev);
> > + if (istat & VHUB_IRQ_DEVICE4)
> > + ast_vhub_dev_irq(&vhub->ports[3].dev);
> > + if (istat & VHUB_IRQ_DEVICE5)
> > + ast_vhub_dev_irq(&vhub->ports[4].dev);
> > + }
> > +
> > + /* Handle top-level vHub EP0 interrupts */
> > + if (istat & (VHUB_IRQ_HUB_EP0_OUT_ACK_STALL |
> > + VHUB_IRQ_HUB_EP0_IN_ACK_STALL |
> > + VHUB_IRQ_HUB_EP0_SETUP)) {
> > + if (istat & VHUB_IRQ_HUB_EP0_IN_ACK_STALL)
> > + ast_vhub_ep0_handle_ack(&vhub->ep0, true);
> > + if (istat & VHUB_IRQ_HUB_EP0_OUT_ACK_STALL)
> > + ast_vhub_ep0_handle_ack(&vhub->ep0, false);
> > + if (istat & VHUB_IRQ_HUB_EP0_SETUP)
> > + ast_vhub_ep0_handle_setup(&vhub->ep0);
> > + }
> > +
> > + /* Various top level bus events */
> > + if (istat & (VHUB_IRQ_BUS_RESUME |
> > + VHUB_IRQ_BUS_SUSPEND |
> > + VHUB_IRQ_BUS_RESET)) {
> > + if (istat & VHUB_IRQ_BUS_RESUME)
> > + ast_vhub_hub_resume(vhub);
> > + if (istat & VHUB_IRQ_BUS_SUSPEND)
> > + ast_vhub_hub_suspend(vhub);
> > + if (istat & VHUB_IRQ_BUS_RESET)
> > + ast_vhub_hub_reset(vhub);
> > + }
> > +
> > + bail:
> > + spin_unlock(&vhub->lock);
> > + return iret;
> > +}
> > +
> > +void ast_vhub_init_hw(struct ast_vhub *vhub)
> > +{
> > + u32 ctrl;
> > +
> > + UDCDBG(vhub,"(Re)Starting HW ...\n");
> > +
> > + /* Enable PHY */
> > + ctrl = VHUB_CTRL_PHY_CLK |
> > + VHUB_CTRL_PHY_RESET_DIS;
> > +
> > +#if 0 /*
> > + * This causes registers to become inaccessible during
> > + * suspend. Need to figure out how to bring the controller
> > + * back into life to issue a wakeup.
> > + */
> > + ctrl |= VHUB_CTRL_CLK_STOP_SUSPEND;
> > +#endif
>
> no commented code.
Why ? This documents a HW issue ... ie, one would normally want to set
this bit but you can't because in practice you can't bring the HW back
short of doing a full reset.
> > + /*
> > + * Set some ISO & split control bits according to Aspeed
> > + * recommendation
> > + *
> > + * VHUB_CTRL_ISO_RSP_CTRL: When set tells the HW to respond
> > + * with 0 bytes data packet to ISO IN endpoints when no data
> > + * is available.
> > + *
> > + * VHUB_CTRL_SPLIT_IN: This makes a SOF complete a split IN
> > + * transaction.
> > + */
> > + ctrl |= VHUB_CTRL_ISO_RSP_CTRL | VHUB_CTRL_SPLIT_IN;
> > + writel(ctrl, vhub->regs + AST_VHUB_CTRL);
> > + udelay(1);
> > +
> > + /* Set descriptor ring size */
> > +#if AST_VHUB_DESCS_COUNT == 256
> > + ctrl |= VHUB_CTRL_LONG_DESC;
> > + writel(ctrl, vhub->regs + AST_VHUB_CTRL);
> > +#elif AST_VHUB_DESCS_COUNT != 32
> > + #error Invalid AST_VHUB_DESCS_COUNT
> > +#endif
>
> find a better way for this. No ifdefs
Ugh ? What's that rule ? I could do a module parameter but you hate
that too and honestly keeping that an ifdef makes things easier. It's
not meant to be changed unless a hardware or performance issues shows
up, I wanted to keep both mode of operations available.
> > +
> > + /* Reset all devices */
> > + writel(0x33f, vhub->regs + AST_VHUB_SW_REST);
>
> no magic constants...
>
> > + udelay(1);
> > + writel(0, vhub->regs + AST_VHUB_SW_REST);
> > +
> > + /* Disable and cleanup EP ACK/NACK interrupts */
> > + writel(0, vhub->regs + AST_VHUB_EP_ACK_IER);
> > + writel(0, vhub->regs + AST_VHUB_EP_NACK_IER);
> > + writel(0xffffffff, vhub->regs + AST_VHUB_EP_ACK_ISR);
> > + writel(0xffffffff, vhub->regs + AST_VHUB_EP_NACK_ISR);
>
> ... of any kind
This is beyond crazy. This is bloody clear we are just clearing all
bits here. Those are just a bit per EP so making them constants makes
no sense.
> > + /* Default settings for EP0, enable HW hub EP1 */
> > + writel(0, vhub->regs + AST_VHUB_EP0_CTRL);
> > + writel(VHUB_EP1_CTRL_RESET_TOGGLE |
> > + VHUB_EP1_CTRL_ENABLE,
> > + vhub->regs + AST_VHUB_EP1_CTRL);
> > + writel(0, vhub->regs + AST_VHUB_EP1_STS_CHG);
> > +
> > + /* Configure EP0 DMA buffer */
> > + writel(vhub->ep0.buf_dma, vhub->regs + AST_VHUB_EP0_DATA);
> > +
> > + /* Clear address */
> > + writel(0, vhub->regs + AST_VHUB_CONF);
> > +
> > + /* Pullup hub (activate on host) */
> > + if (vhub->force_usb1)
> > + ctrl |= VHUB_CTRL_FULL_SPEED_ONLY;
> > + ctrl |= VHUB_CTRL_UPSTREAM_CONNECT;
> > + writel(ctrl, vhub->regs + AST_VHUB_CTRL);
> > +
> > + /* Enable some interrupts */
> > + writel(VHUB_IRQ_HUB_EP0_IN_ACK_STALL |
> > + VHUB_IRQ_HUB_EP0_OUT_ACK_STALL |
> > + VHUB_IRQ_HUB_EP0_SETUP |
> > + VHUB_IRQ_EP_POOL_ACK_STALL |
> > + VHUB_IRQ_BUS_RESUME |
> > + VHUB_IRQ_BUS_SUSPEND |
> > + VHUB_IRQ_BUS_RESET,
> > + vhub->regs + AST_VHUB_IER);
> > +}
> > +
> > +static int ast_vhub_remove(struct platform_device *pdev)
> > +{
> > + struct ast_vhub *vhub = platform_get_drvdata(pdev);
> > + unsigned long flags;
> > + int i;
> > +
> > + if (!vhub || !vhub->regs)
> > + return 0;
> > +
> > + /* Remove devices */
> > + for (i = 0; i < AST_VHUB_NUM_PORTS; i++)
> > + ast_vhub_del_dev(&vhub->ports[i].dev);
> > +
> > + spin_lock_irqsave(&vhub->lock, flags);
> > +
> > + /* Mask & ack all interrupts */
> > + writel(0, vhub->regs + AST_VHUB_IER);
> > + writel(VHUB_IRQ_ACK_ALL, vhub->regs + AST_VHUB_ISR);
> > +
> > + /* Pull device, leave PHY enabled */
> > + writel(VHUB_CTRL_PHY_CLK |
> > + VHUB_CTRL_PHY_RESET_DIS,
> > + vhub->regs + AST_VHUB_CTRL);
> > +
> > + if (vhub->clk)
> > + clk_disable_unprepare(vhub->clk);
> > +
> > + spin_unlock_irqrestore(&vhub->lock, flags);
> > +
> > + if (vhub->ep0_bufs)
> > + dma_free_coherent(&pdev->dev,
> > + AST_VHUB_EP0_MAX_PACKET *
> > + (AST_VHUB_NUM_PORTS + 1),
> > + vhub->ep0_bufs,
> > + vhub->ep0_bufs_dma);
> > + vhub->ep0_bufs = NULL;
> > +
> > + return 0;
> > +}
> > +
> > +static int ast_vhub_probe(struct platform_device *pdev)
> > +{
> > + struct ast_vhub *vhub;
> > + struct resource *res;
> > + int i, rc = 0;
> > +
> > + vhub = devm_kzalloc(&pdev->dev, sizeof(*vhub), GFP_KERNEL);
> > + if (!vhub)
> > + return -ENOMEM;
> > +
> > + spin_lock_init(&vhub->lock);
> > + vhub->pdev = pdev;
> > + vhub->force_usb1 = force_usb1;
> > + vhub->no_dma_desc = no_dma_desc;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + vhub->regs = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(vhub->regs)) {
> > + dev_err(&pdev->dev, "Failed to map resources\n");
> > + return PTR_ERR(vhub->regs);
> > + }
> > + UDCDBG(vhub, "vHub@%pR mapped @%p\n", res, vhub->regs);
> > +
> > + platform_set_drvdata(pdev, vhub);
> > +
> > + vhub->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(vhub->clk)) {
> > + rc = PTR_ERR(vhub->clk);
> > + goto err;
> > + }
> > + rc = clk_prepare_enable(vhub->clk);
> > + if (rc) {
> > + dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", rc);
> > + goto err;
> > + }
> > +
> > + /* Mask & ack all interrupts before installing the handler */
> > + writel(0, vhub->regs + AST_VHUB_IER);
> > + writel(VHUB_IRQ_ACK_ALL, vhub->regs + AST_VHUB_ISR);
> > +
> > + /* Find interrupt and install handler */
> > + vhub->irq = platform_get_irq(pdev, 0);
> > + if (vhub->irq < 0) {
> > + dev_err(&pdev->dev, "Failed to get interrupt\n");
> > + rc = vhub->irq;
> > + goto err;
> > + }
> > + rc = devm_request_irq(&pdev->dev, vhub->irq, ast_vhub_irq, 0,
> > + KBUILD_MODNAME, vhub);
>
> we usually rely on the device name, but okay.
Either works, I can change it.
Ben.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
