[PATCH v7 3/3] usb: gadget: pxa27x_udc: fix clock prepare and enable

[Robert Jarzmik]

As the udc clock controls both the output signals and the internal IP,
it must be enabled before any UDC register is touched.

The bug is revealed when the clock framework disables the clock for a
couple of milliseconds during the boot sequence, and the endpoint
configuration is lost. The bug is hidden when clock framework is not
used, because no "unused clocks disable" occurs.

This patch fixes the wrong behaviour by ensuring that :
- whenever a UDC register is read or written, the clock is enabled
- reworks the endpoints programming to have it done under running clock
- reworks suspend/resume to ensure the same thing

Signed-off-by: Robert Jarzmik <robert.jarzmik-***@public.gmane.org>

---
Since v6: resubmit in 3 patches serie, the first 3 already being in
Felipe's testing/next.
---
drivers/usb/gadget/udc/pxa27x_udc.c | 26 +++++---------------------
1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c b/drivers/usb/gadget/udc/pxa27x_udc.c
index c7deac4..dd6890d 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -1698,10 +1698,10 @@ static void udc_disable(struct pxa_udc *udc)
udc_writel(udc, UDCICR1, 0);

udc_clear_mask_UDCCR(udc, UDCCR_UDE);
- clk_disable(udc->clk);

ep0_idle(udc);
udc->gadget.speed = USB_SPEED_UNKNOWN;
+ clk_disable(udc->clk);

udc->enabled = 0;
}
@@ -1754,16 +1754,16 @@ static void udc_enable(struct pxa_udc *udc)
if (udc->enabled)
return;

+ clk_enable(udc->clk);
udc_writel(udc, UDCICR0, 0);
udc_writel(udc, UDCICR1, 0);
udc_clear_mask_UDCCR(udc, UDCCR_UDE);

- clk_enable(udc->clk);
-
ep0_idle(udc);
udc->gadget.speed = USB_SPEED_FULL;
memset(&udc->stats, 0, sizeof(udc->stats));

+ pxa_eps_setup(udc);
udc_set_mask_UDCCR(udc, UDCCR_UDE);
ep_write_UDCCSR(&udc->pxa_ep[0], UDCCSR0_ACM);
udelay(2);
@@ -2469,7 +2469,6 @@ static int pxa_udc_probe(struct platform_device *pdev)
the_controller = udc;
platform_set_drvdata(pdev, udc);
udc_init_data(udc);
- pxa_eps_setup(udc);

/* irq setup after old hardware state is cleaned up */
retval = devm_request_irq(&pdev->dev, udc->irq, pxa_udc_irq,
@@ -2485,7 +2484,8 @@ static int pxa_udc_probe(struct platform_device *pdev)
goto err;

pxa_init_debugfs(udc);
-
+ if (should_enable_udc(udc))
+ udc_enable(udc);
return 0;
err:
clk_unprepare(udc->clk);
@@ -2538,19 +2538,11 @@ extern void pxa27x_clear_otgph(void);
*/
static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state)
{
- int i;
struct pxa_udc *udc = platform_get_drvdata(_dev);
struct pxa_ep *ep;

ep = &udc->pxa_ep[0];
udc->udccsr0 = udc_ep_readl(ep, UDCCSR);
- for (i = 1; i < NR_PXA_ENDPOINTS; i++) {
- ep = &udc->pxa_ep[i];
- ep->udccsr_value = udc_ep_readl(ep, UDCCSR);
- ep->udccr_value = udc_ep_readl(ep, UDCCR);
- ep_dbg(ep, "udccsr:0x%03x, udccr:0x%x\n",
- ep->udccsr_value, ep->udccr_value);
- }

udc_disable(udc);
udc->pullup_resume = udc->pullup_on;
@@ -2568,19 +2560,11 @@ static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state)
*/
static int pxa_udc_resume(struct platform_device *_dev)
{
- int i;
struct pxa_udc *udc = platform_get_drvdata(_dev);
struct pxa_ep *ep;

ep = &udc->pxa_ep[0];
udc_ep_writel(ep, UDCCSR, udc->udccsr0 & (UDCCSR0_FST | UDCCSR0_DME));
- for (i = 1; i < NR_PXA_ENDPOINTS; i++) {
- ep = &udc->pxa_ep[i];
- udc_ep_writel(ep, UDCCSR, ep->udccsr_value);
- udc_ep_writel(ep, UDCCR, ep->udccr_value);
- ep_dbg(ep, "udccsr:0x%03x, udccr:0x%x\n",
- ep->udccsr_value, ep->udccr_value);
- }

dplus_pullup(udc, udc->pullup_resume);
if (should_enable_udc(udc))

[Robert Jarzmik]

Add documentation for device-tree binding of arm PXA 27x udc (usb
device) driver.

Signed-off-by: Robert Jarzmik <robert.jarzmik-***@public.gmane.org>
Cc: devicetree-***@public.gmane.org
Acked-by: Arnd Bergmann <arnd-***@public.gmane.org>

---
Since V1: change OF id mrvl,pxa27x_udc -> marvell,pxa27x-udc
This is a consequence of other DT reviews on the marvell
namings.
Since V2: Mark's review
- described standard properties
- use standard gpio bindings for pullup gpio
Since V3: Arnd's review
- removed clock-names
---
Documentation/devicetree/bindings/usb/pxa-usb.txt | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/pxa-usb.txt b/Documentation/devicetree/bindings/usb/pxa-usb.txt
index 79729a9..9c33179 100644
--- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
+++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
@@ -29,3 +29,25 @@ Example:
marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
};

+UDC
+
+Required properties:
+ - compatible: Should be "marvell,pxa270-udc" for USB controllers
+ used in device mode.
+ - reg: usb device MMIO address space
+ - interrupts: single interrupt generated by the UDC IP
+ - clocks: input clock of the UDC IP (see clock-bindings.txt)
+
+Optional properties:
+ - gpios:
+ - gpio activated to control the USB D+ pullup (see gpio.txt)
+
+Example:
+
+ pxa27x_udc: ***@40600000 {
+ compatible = "marvell,pxa270-udc";
+ reg = <0x40600000 0x10000>;
+ interrupts = <11>;
+ clocks = <&pxa2xx_clks 11>;
+ gpios = <&gpio 22 GPIO_ACTIVE_LOW>;
+ };

[Robert Jarzmik]

Use devm_* helpers in the probe function to simplify the error path and
the remove path.

Signed-off-by: Robert Jarzmik <robert.jarzmik-***@public.gmane.org>
Cc: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+***@public.gmane.org>

---
Since V1: Addressed Sergei's comments
Since V2: Addressed Sergei's comments on includes
Since V4: Addressed Sergei's comment on clk_uprepare in probe() function
---
drivers/usb/gadget/udc/pxa27x_udc.c | 45 +++++++++++--------------------------
1 file changed, 13 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c b/drivers/usb/gadget/udc/pxa27x_udc.c
index 17a0193..c7deac4 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -2438,8 +2438,9 @@ static int pxa_udc_probe(struct platform_device *pdev)
}

regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!regs)
- return -ENXIO;
+ udc->regs = devm_ioremap_resource(&pdev->dev, regs);
+ if (IS_ERR(udc->regs))
+ return PTR_ERR(udc->regs);
udc->irq = platform_get_irq(pdev, 0);
if (udc->irq < 0)
return udc->irq;
@@ -2455,21 +2456,13 @@ static int pxa_udc_probe(struct platform_device *pdev)
if (udc->gpiod)
gpiod_direction_output(udc->gpiod, 0);

- udc->clk = clk_get(&pdev->dev, NULL);
- if (IS_ERR(udc->clk)) {
- retval = PTR_ERR(udc->clk);
- goto err_clk;
- }
+ udc->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(udc->clk))
+ return PTR_ERR(udc->clk);
+
retval = clk_prepare(udc->clk);
if (retval)
- goto err_clk_prepare;
-
- retval = -ENOMEM;
- udc->regs = ioremap(regs->start, resource_size(regs));
- if (!udc->regs) {
- dev_err(&pdev->dev, "Unable to map UDC I/O memory\n");
- goto err_map;
- }
+ return retval;

udc->vbus_sensed = 0;

@@ -2479,32 +2472,23 @@ static int pxa_udc_probe(struct platform_device *pdev)
pxa_eps_setup(udc);

/* irq setup after old hardware state is cleaned up */
- retval = request_irq(udc->irq, pxa_udc_irq,
- IRQF_SHARED, driver_name, udc);
+ retval = devm_request_irq(&pdev->dev, udc->irq, pxa_udc_irq,
+ IRQF_SHARED, driver_name, udc);
if (retval != 0) {
dev_err(udc->dev, "%s: can't get irq %i, err %d\n",
driver_name, udc->irq, retval);
- goto err_irq;
+ goto err;
}

retval = usb_add_gadget_udc(&pdev->dev, &udc->gadget);
if (retval)
- goto err_add_udc;
+ goto err;

pxa_init_debugfs(udc);

return 0;
-
-err_add_udc:
- free_irq(udc->irq, udc);
-err_irq:
- iounmap(udc->regs);
-err_map:
+err:
clk_unprepare(udc->clk);
-err_clk_prepare:
- clk_put(udc->clk);
- udc->clk = NULL;
-err_clk:
return retval;
}

@@ -2518,7 +2502,6 @@ static int pxa_udc_remove(struct platform_device *_dev)

usb_del_gadget_udc(&udc->gadget);
usb_gadget_unregister_driver(udc->driver);
- free_irq(udc->irq, udc);
pxa_cleanup_debugfs(udc);

usb_put_phy(udc->transceiver);
@@ -2526,8 +2509,6 @@ static int pxa_udc_remove(struct platform_device *_dev)
udc->transceiver = NULL;
the_controller = NULL;
clk_unprepare(udc->clk);
- clk_put(udc->clk);
- iounmap(udc->regs);

return 0;
}