mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 0/8] USB ehci/chipidea fixes
@ 2018-10-26  9:33 Sascha Hauer
  2018-10-26  9:33 ` [PATCH 1/8] usb: gadget: fsl_udc: Drop using global variable Sascha Hauer
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

This series fixes messages seen on i.MX when multiple chipidea
instances are registered:

ERROR: imx-usb 53f80000.usb: gadget not registered.

While I originally thought this is a simple one it turned out there
are several things wrong in the ehci/chipidea (un)registration code.
Most notably the ehci controllers were often not properly quiesced when
leaving barebox.

Sascha

Sascha Hauer (8):
  usb: gadget: fsl_udc: Drop using global variable
  usb: host: ehci: rename ehci_priv to ehci_host
  usb: Add usb_unregister_host()
  usb: host: ehci: add ehci_unregister()
  usb: host: ehci: do not use dev->priv
  usb: host: ehci-atmel: unregister host on device remove
  usb: imx: unregister ehci controller on device removal
  usb: gadget: fsl_udc: pass controller instance to unregister

 drivers/usb/core/usb.c         |  5 +++
 drivers/usb/gadget/fsl_udc.c   | 69 +++++++++++++++++------------
 drivers/usb/host/ehci-atmel.c  | 24 +++++++++-
 drivers/usb/host/ehci-hcd.c    | 80 +++++++++++++++++++++-------------
 drivers/usb/imx/chipidea-imx.c | 39 ++++++++++++++---
 include/usb/ehci.h             | 20 +++++++--
 include/usb/fsl_usb2.h         |  6 ++-
 include/usb/usb.h              |  1 +
 8 files changed, 174 insertions(+), 70 deletions(-)

-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/8] usb: gadget: fsl_udc: Drop using global variable
  2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
@ 2018-10-26  9:33 ` Sascha Hauer
  2018-10-26  9:33 ` [PATCH 2/8] usb: host: ehci: rename ehci_priv to ehci_host Sascha Hauer
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

No need to use the global udc_controller variable when we are provided
an usb_gadget.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/gadget/fsl_udc.c | 39 ++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/gadget/fsl_udc.c b/drivers/usb/gadget/fsl_udc.c
index 9b59669773..7782d4bdd9 100644
--- a/drivers/usb/gadget/fsl_udc.c
+++ b/drivers/usb/gadget/fsl_udc.c
@@ -450,6 +450,11 @@ struct fsl_udc {
 	u8 device_address;	/* Device USB address */
 };
 
+static inline struct fsl_udc *to_fsl_udc(struct usb_gadget *gadget)
+{
+	return container_of(gadget, struct fsl_udc, gadget);
+}
+
 /*-------------------------------------------------------------------------*/
 
 #ifdef DEBUG
@@ -1943,12 +1948,9 @@ static void dtd_complete_irq(struct fsl_udc *udc)
  */
 static void fsl_udc_gadget_poll(struct usb_gadget *gadget)
 {
-	struct fsl_udc *udc = udc_controller;
+	struct fsl_udc *udc = to_fsl_udc(gadget);
 	u32 irq_src;
 
-	if (!udc)
-		return;
-
 	/* Disable ISR for OTG host mode */
 	if (udc->stopped)
 		return;
@@ -1981,7 +1983,7 @@ static void fsl_udc_gadget_poll(struct usb_gadget *gadget)
 
 	/* Sleep Enable (Suspend) */
 	if (irq_src & USB_STS_SUSPEND)
-		udc->driver->disconnect(&udc_controller->gadget);
+		udc->driver->disconnect(gadget);
 
 	if (irq_src & (USB_STS_ERR | USB_STS_SYS_ERR))
 		printf("Error IRQ %x\n", irq_src);
@@ -1993,6 +1995,8 @@ static void fsl_udc_gadget_poll(struct usb_gadget *gadget)
 *----------------------------------------------------------------*/
 static int fsl_udc_start(struct usb_gadget *gadget, struct usb_gadget_driver *driver)
 {
+	struct fsl_udc *udc = to_fsl_udc(gadget);
+
 	/*
 	 * We currently have PHY no driver which could call vbus_connect,
 	 * so when the USB gadget core calls usb_gadget_connect() the
@@ -2002,13 +2006,13 @@ static int fsl_udc_start(struct usb_gadget *gadget, struct usb_gadget_driver *dr
 	usb_gadget_vbus_connect(gadget);
 
 	/* hook up the driver */
-	udc_controller->driver = driver;
+	udc->driver = driver;
 
 	/* Enable DR IRQ reg and Set usbcmd reg  Run bit */
-	dr_controller_run(udc_controller);
-	udc_controller->usb_state = USB_STATE_ATTACHED;
-	udc_controller->ep0_state = WAIT_FOR_SETUP;
-	udc_controller->ep0_dir = 0;
+	dr_controller_run(udc);
+	udc->usb_state = USB_STATE_ATTACHED;
+	udc->ep0_state = WAIT_FOR_SETUP;
+	udc->ep0_dir = 0;
 
 	return 0;
 }
@@ -2016,20 +2020,21 @@ static int fsl_udc_start(struct usb_gadget *gadget, struct usb_gadget_driver *dr
 /* Disconnect from gadget driver */
 static int fsl_udc_stop(struct usb_gadget *gadget, struct usb_gadget_driver *driver)
 {
+	struct fsl_udc *udc = to_fsl_udc(gadget);
 	struct fsl_ep *loop_ep;
 
 	/* stop DR, disable intr */
-	dr_controller_stop(udc_controller);
+	dr_controller_stop(udc);
 
 	/* in fact, no needed */
-	udc_controller->usb_state = USB_STATE_ATTACHED;
-	udc_controller->ep0_state = WAIT_FOR_SETUP;
-	udc_controller->ep0_dir = 0;
+	udc->usb_state = USB_STATE_ATTACHED;
+	udc->ep0_state = WAIT_FOR_SETUP;
+	udc->ep0_dir = 0;
 
 	/* stand operation */
-	udc_controller->gadget.speed = USB_SPEED_UNKNOWN;
-	nuke(&udc_controller->eps[0], -ESHUTDOWN);
-	list_for_each_entry(loop_ep, &udc_controller->gadget.ep_list,
+	udc->gadget.speed = USB_SPEED_UNKNOWN;
+	nuke(&udc->eps[0], -ESHUTDOWN);
+	list_for_each_entry(loop_ep, &udc->gadget.ep_list,
 			ep.ep_list)
 		nuke(loop_ep, -ESHUTDOWN);
 
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/8] usb: host: ehci: rename ehci_priv to ehci_host
  2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
  2018-10-26  9:33 ` [PATCH 1/8] usb: gadget: fsl_udc: Drop using global variable Sascha Hauer
@ 2018-10-26  9:33 ` Sascha Hauer
  2018-10-26  9:33 ` [PATCH 3/8] usb: Add usb_unregister_host() Sascha Hauer
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

As we are going to export the structure as a cookie to others
rename it to ehci_host.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/host/ehci-hcd.c | 42 ++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 9bbdda365c..c20d4392d2 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -34,7 +34,7 @@
 
 #include "ehci.h"
 
-struct ehci_priv {
+struct ehci_host {
 	int rootdev;
 	struct device_d *dev;
 	struct ehci_hccr *hccr;
@@ -63,7 +63,7 @@ struct int_queue {
 	struct qTD *tds;
 };
 
-#define to_ehci(ptr) container_of(ptr, struct ehci_priv, host)
+#define to_ehci(ptr) container_of(ptr, struct ehci_host, host)
 
 #define NUM_QH	2
 #define NUM_TD	3
@@ -155,7 +155,7 @@ static int handshake(uint32_t *ptr, uint32_t mask, uint32_t done, int usec)
 	}
 }
 
-static int ehci_reset(struct ehci_priv *ehci)
+static int ehci_reset(struct ehci_host *ehci)
 {
 	uint32_t cmd;
 	uint32_t tmp;
@@ -218,7 +218,7 @@ ehci_submit_async(struct usb_device *dev, unsigned long pipe, void *buffer,
 		   int length, struct devrequest *req, int timeout_ms)
 {
 	struct usb_host *host = dev->host;
-	struct ehci_priv *ehci = to_ehci(host);
+	struct ehci_host *ehci = to_ehci(host);
 	struct QH *qh;
 	struct qTD *td;
 	volatile struct qTD *vtd;
@@ -460,7 +460,7 @@ fail:
  * boards.
  * See http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/037341.html
  */
-static void ehci_powerup_fixup(struct ehci_priv *ehci)
+static void ehci_powerup_fixup(struct ehci_host *ehci)
 {
 	void *viewport = (void *)ehci->hcor + 0x30;
 
@@ -471,12 +471,12 @@ static void ehci_powerup_fixup(struct ehci_priv *ehci)
 			viewport);
 }
 #else
-static inline void ehci_powerup_fixup(struct ehci_priv *ehci)
+static inline void ehci_powerup_fixup(struct ehci_host *ehci)
 {
 }
 #endif
 
-static void pass_to_companion(struct ehci_priv *ehci, int port)
+static void pass_to_companion(struct ehci_host *ehci, int port)
 {
 	uint32_t *status_reg = (uint32_t *)&ehci->hcor->or_portsc[port - 1];
 	uint32_t reg = ehci_readl(status_reg);
@@ -493,7 +493,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
 		 int length, struct devrequest *req)
 {
 	struct usb_host *host = dev->host;
-	struct ehci_priv *ehci = to_ehci(host);
+	struct ehci_host *ehci = to_ehci(host);
 	uint8_t tmpbuf[4];
 	u16 typeReq;
 	void *srcptr = NULL;
@@ -772,7 +772,7 @@ unknown:
 }
 
 /* force HC to halt state from unknown (EHCI spec section 2.3) */
-static int ehci_halt(struct ehci_priv *ehci)
+static int ehci_halt(struct ehci_host *ehci)
 {
 	u32	temp = ehci_readl(&ehci->hcor->or_usbsts);
 
@@ -792,7 +792,7 @@ static int ehci_halt(struct ehci_priv *ehci)
 
 static int ehci_init(struct usb_host *host)
 {
-	struct ehci_priv *ehci = to_ehci(host);
+	struct ehci_host *ehci = to_ehci(host);
 	uint32_t reg;
 	uint32_t cmd;
 	int ret = 0;
@@ -902,7 +902,7 @@ submit_bulk_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 		int length, int timeout)
 {
 	struct usb_host *host = dev->host;
-	struct ehci_priv *ehci = to_ehci(host);
+	struct ehci_host *ehci = to_ehci(host);
 
 	if (usb_pipetype(pipe) != PIPE_BULK) {
 		dev_dbg(ehci->dev, "non-bulk pipe (type=%lu)", usb_pipetype(pipe));
@@ -916,7 +916,7 @@ submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 		   int length, struct devrequest *setup, int timeout)
 {
 	struct usb_host *host = dev->host;
-	struct ehci_priv *ehci = to_ehci(host);
+	struct ehci_host *ehci = to_ehci(host);
 
 	if (usb_pipetype(pipe) != PIPE_CONTROL) {
 		dev_dbg(ehci->dev, "non-control pipe (type=%lu)", usb_pipetype(pipe));
@@ -932,7 +932,7 @@ submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 }
 
 static int
-disable_periodic(struct ehci_priv *ehci)
+disable_periodic(struct ehci_host *ehci)
 {
 	uint32_t cmd;
 	struct ehci_hcor *hcor = ehci->hcor;
@@ -954,7 +954,7 @@ disable_periodic(struct ehci_priv *ehci)
 #define NEXT_QH(qh) (struct QH *)((unsigned long)hc32_to_cpu((qh)->qh_link) & ~0x1f)
 
 static int
-enable_periodic(struct ehci_priv *ehci)
+enable_periodic(struct ehci_host *ehci)
 {
 	uint32_t cmd;
 	struct ehci_hcor *hcor = ehci->hcor;
@@ -1018,7 +1018,7 @@ static struct int_queue *ehci_create_int_queue(struct usb_device *dev,
 			void *buffer, int interval)
 {
 	struct usb_host *host = dev->host;
-	struct ehci_priv *ehci = to_ehci(host);
+	struct ehci_host *ehci = to_ehci(host);
 	struct int_queue *result = NULL;
 	uint32_t i;
 	struct QH *list = ehci->periodic_queue;
@@ -1187,7 +1187,7 @@ static int ehci_destroy_int_queue(struct usb_device *dev,
 {
 	int result = -EINVAL;
 	struct usb_host *host = dev->host;
-	struct ehci_priv *ehci = to_ehci(host);
+	struct ehci_host *ehci = to_ehci(host);
 	struct QH *cur = ehci->periodic_queue;
 
 	if (disable_periodic(ehci) < 0) {
@@ -1230,7 +1230,7 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 	       int length, int interval)
 {
 	struct usb_host *host = dev->host;
-	struct ehci_priv *ehci = to_ehci(host);
+	struct ehci_host *ehci = to_ehci(host);
 	struct int_queue *queue;
 	uint64_t start;
 	void *backbuffer;
@@ -1275,7 +1275,7 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 
 static int ehci_detect(struct device_d *dev)
 {
-	struct ehci_priv *ehci = dev->priv;
+	struct ehci_host *ehci = dev->priv;
 
 	return usb_host_detect(&ehci->host);
 }
@@ -1283,10 +1283,10 @@ static int ehci_detect(struct device_d *dev)
 int ehci_register(struct device_d *dev, struct ehci_data *data)
 {
 	struct usb_host *host;
-	struct ehci_priv *ehci;
+	struct ehci_host *ehci;
 	uint32_t reg;
 
-	ehci = xzalloc(sizeof(struct ehci_priv));
+	ehci = xzalloc(sizeof(struct ehci_host));
 	host = &ehci->host;
 	dev->priv = ehci;
 	ehci->flags = data->flags;
@@ -1369,7 +1369,7 @@ static int ehci_probe(struct device_d *dev)
 
 static void ehci_remove(struct device_d *dev)
 {
-	struct ehci_priv *ehci = dev->priv;
+	struct ehci_host *ehci = dev->priv;
 	ehci_halt(ehci);
 }
 
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/8] usb: Add usb_unregister_host()
  2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
  2018-10-26  9:33 ` [PATCH 1/8] usb: gadget: fsl_udc: Drop using global variable Sascha Hauer
  2018-10-26  9:33 ` [PATCH 2/8] usb: host: ehci: rename ehci_priv to ehci_host Sascha Hauer
@ 2018-10-26  9:33 ` Sascha Hauer
  2018-10-26  9:33 ` [PATCH 4/8] usb: host: ehci: add ehci_unregister() Sascha Hauer
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

We have usb_register_host() which puts a new host on the list
of hosts we should also have the opposite which removes the
host from the list again.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/core/usb.c | 5 +++++
 include/usb/usb.h      | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 70ded6ded1..1e48c1d0a8 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -102,6 +102,11 @@ int usb_register_host(struct usb_host *host)
 	return 0;
 }
 
+void usb_unregister_host(struct usb_host *host)
+{
+	list_del(&host->list);
+}
+
 /**
  * set configuration number to configuration
  */
diff --git a/include/usb/usb.h b/include/usb/usb.h
index 9aab06c87c..eb2eeb8db3 100644
--- a/include/usb/usb.h
+++ b/include/usb/usb.h
@@ -157,6 +157,7 @@ struct usb_host {
 };
 
 int usb_register_host(struct usb_host *);
+void usb_unregister_host(struct usb_host *host);
 
 int usb_host_detect(struct usb_host *host);
 
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/8] usb: host: ehci: add ehci_unregister()
  2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
                   ` (2 preceding siblings ...)
  2018-10-26  9:33 ` [PATCH 3/8] usb: Add usb_unregister_host() Sascha Hauer
@ 2018-10-26  9:33 ` Sascha Hauer
  2018-10-26  9:33 ` [PATCH 5/8] usb: host: ehci: do not use dev->priv Sascha Hauer
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

ehci_register() allocates data and registers a ehci host. Add
ehci_unregister() to properly halt the controller and to free the memory
again.. To do so, change ehci_register() to return the ehci host rather
than an error code.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/host/ehci-atmel.c  | 10 +++++++++-
 drivers/usb/host/ehci-hcd.c    | 23 +++++++++++++++++++----
 drivers/usb/imx/chipidea-imx.c | 11 ++++++++---
 include/usb/ehci.h             | 14 +++++++++++---
 4 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
index 1132879c9b..bfa235d954 100644
--- a/drivers/usb/host/ehci-atmel.c
+++ b/drivers/usb/host/ehci-atmel.c
@@ -30,6 +30,7 @@
 #include "ehci.h"
 
 struct atmel_ehci_priv {
+	struct ehci_host *ehci;
 	struct device_d *dev;
 	struct clk *iclk;
 	struct clk *uclk;
@@ -66,6 +67,7 @@ static int atmel_ehci_probe(struct device_d *dev)
 	struct ehci_data data;
 	struct atmel_ehci_priv *atehci;
 	const char *uclk_name;
+	struct ehci_host *ehci;
 
 	uclk_name = (dev->device_node) ? "usb_clk" : "uhpck";
 
@@ -99,7 +101,13 @@ static int atmel_ehci_probe(struct device_d *dev)
 		return PTR_ERR(iores);
 	data.hccr = IOMEM(iores->start);
 
-	return ehci_register(dev, &data);
+	ehci = ehci_register(dev, &data);
+	if (IS_ERR(ehci))
+		return PTR_ERR(ehci);
+
+	atehci->ehci = ehci;
+
+	return 0;
 }
 
 static void atmel_ehci_remove(struct device_d *dev)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index c20d4392d2..59268bf5ec 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1280,7 +1280,7 @@ static int ehci_detect(struct device_d *dev)
 	return usb_host_detect(&ehci->host);
 }
 
-int ehci_register(struct device_d *dev, struct ehci_data *data)
+struct ehci_host *ehci_register(struct device_d *dev, struct ehci_data *data)
 {
 	struct usb_host *host;
 	struct ehci_host *ehci;
@@ -1328,7 +1328,16 @@ int ehci_register(struct device_d *dev, struct ehci_data *data)
 	reg = HC_VERSION(ehci_readl(&ehci->hccr->cr_capbase));
 	dev_info(dev, "USB EHCI %x.%02x\n", reg >> 8, reg & 0xff);
 
-	return 0;
+	return ehci;
+}
+
+void ehci_unregister(struct ehci_host *ehci)
+{
+	ehci_halt(ehci);
+
+	usb_unregister_host(&ehci->host);
+
+	free(ehci);
 }
 
 static int ehci_probe(struct device_d *dev)
@@ -1337,6 +1346,7 @@ static int ehci_probe(struct device_d *dev)
 	struct ehci_data data = {};
 	struct ehci_platform_data *pdata = dev->platform_data;
 	struct device_node *dn = dev->device_node;
+	struct ehci_host *ehci;
 
 	if (pdata)
 		data.flags = pdata->flags;
@@ -1364,13 +1374,18 @@ static int ehci_probe(struct device_d *dev)
 	else
 		data.hcor = NULL;
 
-	return ehci_register(dev, &data);
+	ehci = ehci_register(dev, &data);
+	if (IS_ERR(ehci))
+		return PTR_ERR(ehci);
+
+	return 0;
 }
 
 static void ehci_remove(struct device_d *dev)
 {
 	struct ehci_host *ehci = dev->priv;
-	ehci_halt(ehci);
+
+	ehci_unregister(ehci);
 }
 
 static __maybe_unused struct of_device_id ehci_platform_dt_ids[] = {
diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
index 3e3e6a365f..a8914e25b6 100644
--- a/drivers/usb/imx/chipidea-imx.c
+++ b/drivers/usb/imx/chipidea-imx.c
@@ -46,6 +46,7 @@ struct imx_chipidea {
 	struct phy *phy;
 	struct usb_phy *usbphy;
 	struct clk *clk;
+	struct ehci_host *ehci;
 };
 
 static int imx_chipidea_port_init(void *drvdata)
@@ -184,14 +185,18 @@ static int ci_register_role(struct imx_chipidea *ci)
 
 	if (ci->mode == IMX_USB_MODE_HOST) {
 		if (IS_ENABLED(CONFIG_USB_EHCI)) {
+			struct ehci_host *ehci;
+
 			ci->role_registered = IMX_USB_MODE_HOST;
 			ret = regulator_enable(ci->vbus);
 			if (ret)
 				return ret;
 
-			ret = ehci_register(ci->dev, &ci->data);
-			if (!ret)
-				return 0;
+			ehci = ehci_register(ci->dev, &ci->data);
+			if (IS_ERR(ehci))
+				return PTR_ERR(ehci);
+
+			ci->ehci = ehci;
 
 			regulator_disable(ci->vbus);
 		} else {
diff --git a/include/usb/ehci.h b/include/usb/ehci.h
index 1008e92f02..5bce3ab110 100644
--- a/include/usb/ehci.h
+++ b/include/usb/ehci.h
@@ -19,12 +19,20 @@ struct ehci_data {
 	void *drvdata;
 };
 
+struct ehci_host;
+
 #ifdef CONFIG_USB_EHCI
-int ehci_register(struct device_d *dev, struct ehci_data *data);
+struct ehci_host *ehci_register(struct device_d *dev, struct ehci_data *data);
+void ehci_unregister(struct ehci_host *);
 #else
-static inline int ehci_register(struct device_d *dev, struct ehci_data *data)
+static inline struct ehci_host *ehci_register(struct device_d *dev,
+					      struct ehci_data *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline void ehci_unregister(struct ehci_host *)
 {
-	return -ENOSYS;
 }
 #endif
 
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 5/8] usb: host: ehci: do not use dev->priv
  2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
                   ` (3 preceding siblings ...)
  2018-10-26  9:33 ` [PATCH 4/8] usb: host: ehci: add ehci_unregister() Sascha Hauer
@ 2018-10-26  9:33 ` Sascha Hauer
  2018-10-26  9:33 ` [PATCH 6/8] usb: host: ehci-atmel: unregister host on device remove Sascha Hauer
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

An ehci can be registered with ehci_register which is passed a struct
device_d *. In that case the priv pointer may already be used by the
caller, so we must not use it in the ehci code. At least for the Atmel
ehci driver this fixes a bug, here dev->priv is set two times to
different values.
Since we need dev->priv in the ehci code to get the controller in
ehci_detect() we can no longer implement that without the help of the
caller, hence we eport ehci_detect() and epect it to be called by
the code which registers a ehci host.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/host/ehci-atmel.c  |  8 ++++++++
 drivers/usb/host/ehci-hcd.c    | 17 +++++++++++------
 drivers/usb/imx/chipidea-imx.c | 10 ++++++++++
 include/usb/ehci.h             |  6 ++++++
 4 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
index bfa235d954..36c9166e74 100644
--- a/drivers/usb/host/ehci-atmel.c
+++ b/drivers/usb/host/ehci-atmel.c
@@ -60,6 +60,13 @@ static void atmel_stop_clock(struct atmel_ehci_priv *atehci)
 	clk_disable(atehci->uclk);
 }
 
+static int atmel_ehci_detect(struct device_d *dev)
+{
+	struct atmel_ehci_priv *atehci = dev->priv;
+
+	return ehci_detect(atehci->ehci);
+}
+
 static int atmel_ehci_probe(struct device_d *dev)
 {
 	int ret;
@@ -74,6 +81,7 @@ static int atmel_ehci_probe(struct device_d *dev)
 	atehci = xzalloc(sizeof(*atehci));
 	atehci->dev = dev;
 	dev->priv = atehci;
+	dev->detect = atmel_ehci_detect;
 
 	atehci->iclk = clk_get(dev, "ehci_clk");
 	if (IS_ERR(atehci->iclk)) {
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 59268bf5ec..87af95d2ed 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1273,10 +1273,8 @@ submit_int_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
 	return result;
 }
 
-static int ehci_detect(struct device_d *dev)
+int ehci_detect(struct ehci_host *ehci)
 {
-	struct ehci_host *ehci = dev->priv;
-
 	return usb_host_detect(&ehci->host);
 }
 
@@ -1288,7 +1286,6 @@ struct ehci_host *ehci_register(struct device_d *dev, struct ehci_data *data)
 
 	ehci = xzalloc(sizeof(struct ehci_host));
 	host = &ehci->host;
-	dev->priv = ehci;
 	ehci->flags = data->flags;
 	ehci->hccr = data->hccr;
 	ehci->dev = dev;
@@ -1321,8 +1318,6 @@ struct ehci_host *ehci_register(struct device_d *dev, struct ehci_data *data)
 		ehci_reset(ehci);
 	}
 
-	dev->detect = ehci_detect;
-
 	usb_register_host(host);
 
 	reg = HC_VERSION(ehci_readl(&ehci->hccr->cr_capbase));
@@ -1340,6 +1335,13 @@ void ehci_unregister(struct ehci_host *ehci)
 	free(ehci);
 }
 
+static int ehci_dev_detect(struct device_d *dev)
+{
+	struct ehci_host *ehci = dev->priv;
+
+	return ehci_detect(ehci);
+}
+
 static int ehci_probe(struct device_d *dev)
 {
 	struct resource *iores;
@@ -1378,6 +1380,9 @@ static int ehci_probe(struct device_d *dev)
 	if (IS_ERR(ehci))
 		return PTR_ERR(ehci);
 
+	dev->priv = ehci;
+	dev->detect = ehci_dev_detect;
+
 	return 0;
 }
 
diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
index a8914e25b6..321a8ada3f 100644
--- a/drivers/usb/imx/chipidea-imx.c
+++ b/drivers/usb/imx/chipidea-imx.c
@@ -176,6 +176,13 @@ static int imx_chipidea_probe_dt(struct imx_chipidea *ci)
 	return 0;
 }
 
+static int ci_ehci_detect(struct device_d *dev)
+{
+	struct imx_chipidea *ci = dev->priv;
+
+	return ehci_detect(ci->ehci);
+}
+
 static int ci_register_role(struct imx_chipidea *ci)
 {
 	int ret;
@@ -198,6 +205,8 @@ static int ci_register_role(struct imx_chipidea *ci)
 
 			ci->ehci = ehci;
 
+			ci->dev->detect = ci_ehci_detect;
+
 			regulator_disable(ci->vbus);
 		} else {
 			dev_err(ci->dev, "Host support not available\n");
@@ -271,6 +280,7 @@ static int imx_chipidea_probe(struct device_d *dev)
 
 	ci = xzalloc(sizeof(*ci));
 	ci->dev = dev;
+	dev->priv = ci;
 	ci->role_registered = IMX_USB_MODE_OTG;
 
 	if (IS_ENABLED(CONFIG_OFDEVICE) && dev->device_node) {
diff --git a/include/usb/ehci.h b/include/usb/ehci.h
index 5bce3ab110..f9bf88c497 100644
--- a/include/usb/ehci.h
+++ b/include/usb/ehci.h
@@ -24,6 +24,7 @@ struct ehci_host;
 #ifdef CONFIG_USB_EHCI
 struct ehci_host *ehci_register(struct device_d *dev, struct ehci_data *data);
 void ehci_unregister(struct ehci_host *);
+int ehci_detect(struct ehci_host *ehci);
 #else
 static inline struct ehci_host *ehci_register(struct device_d *dev,
 					      struct ehci_data *data)
@@ -34,6 +35,11 @@ static inline struct ehci_host *ehci_register(struct device_d *dev,
 static inline void ehci_unregister(struct ehci_host *)
 {
 }
+
+static inline int ehci_detect(struct ehci_host *ehci)
+{
+	return 0;
+}
 #endif
 
 #endif  /* __USB_EHCI_H */
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 6/8] usb: host: ehci-atmel: unregister host on device remove
  2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
                   ` (4 preceding siblings ...)
  2018-10-26  9:33 ` [PATCH 5/8] usb: host: ehci: do not use dev->priv Sascha Hauer
@ 2018-10-26  9:33 ` Sascha Hauer
  2018-10-26  9:33 ` [PATCH 7/8] usb: imx: unregister ehci controller on device removal Sascha Hauer
  2018-10-26  9:33 ` [PATCH 8/8] usb: gadget: fsl_udc: pass controller instance to unregister Sascha Hauer
  7 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

The ehci code does DMA and really must be properly stopped when we
leave barebox, so call ehci_unregister on device removal.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/host/ehci-atmel.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
index 36c9166e74..6c88d646c9 100644
--- a/drivers/usb/host/ehci-atmel.c
+++ b/drivers/usb/host/ehci-atmel.c
@@ -120,10 +120,14 @@ static int atmel_ehci_probe(struct device_d *dev)
 
 static void atmel_ehci_remove(struct device_d *dev)
 {
+	struct atmel_ehci_priv *atehci = dev->priv;
+
+	ehci_unregister(atehci->ehci);
+
 	/*
 	 * Stop the USB clocks.
 	 */
-	atmel_stop_clock(dev->priv);
+	atmel_stop_clock(atehci);
 }
 
 static const struct of_device_id atmel_ehci_dt_ids[] = {
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 7/8] usb: imx: unregister ehci controller on device removal
  2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
                   ` (5 preceding siblings ...)
  2018-10-26  9:33 ` [PATCH 6/8] usb: host: ehci-atmel: unregister host on device remove Sascha Hauer
@ 2018-10-26  9:33 ` Sascha Hauer
  2018-10-26  9:33 ` [PATCH 8/8] usb: gadget: fsl_udc: pass controller instance to unregister Sascha Hauer
  7 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

ehci does DMA and hence must be properly quiesced before we leave
barebox. Call ehci_unregister() on device removal when we previously
registered a ehci controller.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/imx/chipidea-imx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
index 321a8ada3f..7bf2ef76c5 100644
--- a/drivers/usb/imx/chipidea-imx.c
+++ b/drivers/usb/imx/chipidea-imx.c
@@ -364,6 +364,11 @@ static int imx_chipidea_probe(struct device_d *dev)
 
 static void imx_chipidea_remove(struct device_d *dev)
 {
+	struct imx_chipidea *ci = dev->priv;
+
+	if (ci->ehci)
+		ehci_unregister(ci->ehci);
+
 	if (IS_ENABLED(CONFIG_USB_GADGET_DRIVER_ARC))
 		ci_udc_unregister();
 }
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 8/8] usb: gadget: fsl_udc: pass controller instance to unregister
  2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
                   ` (6 preceding siblings ...)
  2018-10-26  9:33 ` [PATCH 7/8] usb: imx: unregister ehci controller on device removal Sascha Hauer
@ 2018-10-26  9:33 ` Sascha Hauer
  7 siblings, 0 replies; 9+ messages in thread
From: Sascha Hauer @ 2018-10-26  9:33 UTC (permalink / raw)
  To: Barebox List

ci_udc_unregister() used to unregister "the controller". Since we
may register multiple chipidea devices we called ci_udc_unregister()
for each of them. This led to messages like:

ERROR: imx-usb 53f80000.usb: gadget not registered.

Fix this by returning the registered controller. This allows us to call
ci_udc_unregister() only when we actually registered one before.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/usb/gadget/fsl_udc.c   | 30 ++++++++++++++++++++----------
 drivers/usb/imx/chipidea-imx.c | 13 ++++++++++---
 include/usb/fsl_usb2.h         |  6 ++++--
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/gadget/fsl_udc.c b/drivers/usb/gadget/fsl_udc.c
index 7782d4bdd9..061ee5185f 100644
--- a/drivers/usb/gadget/fsl_udc.c
+++ b/drivers/usb/gadget/fsl_udc.c
@@ -528,7 +528,6 @@ static void dump_msg(const char *label, const u8 * buf, unsigned int length)
 #define get_pipe_by_ep(EP)	(ep_index(EP) * 2 + ep_is_in(EP))
 
 static struct usb_dr_device __iomem *dr_regs;
-static struct fsl_udc *udc_controller = NULL;
 
 static const struct usb_endpoint_descriptor
 fsl_ep0_desc = {
@@ -2226,8 +2225,9 @@ static int __init struct_ep_setup(struct fsl_udc *udc, unsigned char index,
 	return 0;
 }
 
-int ci_udc_register(struct device_d *dev, void __iomem *regs)
+struct fsl_udc *ci_udc_register(struct device_d *dev, void __iomem *regs)
 {
+	struct fsl_udc *udc_controller;
 	int ret, i;
 	u32 dccparams;
 
@@ -2293,31 +2293,41 @@ int ci_udc_register(struct device_d *dev, void __iomem *regs)
 	if (ret)
 		goto err_out;
 
-	return 0;
+	return udc_controller;
 err_out:
-	return ret;
+	free(udc_controller);
+
+	return ERR_PTR(ret);
 }
 
-void ci_udc_unregister(void)
+void ci_udc_unregister(struct fsl_udc *udc)
 {
-	if (udc_controller)
-		usb_del_gadget_udc(&udc_controller->gadget);
-
+	usb_del_gadget_udc(&udc->gadget);
+	free(udc);
 }
 
 static int fsl_udc_probe(struct device_d *dev)
 {
+	struct fsl_udc *udc;
 	void __iomem *regs = dev_request_mem_region(dev, 0);
 
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
-	return ci_udc_register(dev, regs);
+	udc = ci_udc_register(dev, regs);
+	if (IS_ERR(udc))
+		return PTR_ERR(udc);
+
+	dev->priv = udc;
+
+	return 0;
 }
 
 static void fsl_udc_remove(struct device_d *dev)
 {
-	ci_udc_unregister();
+	struct fsl_udc *udc = dev->priv;
+
+	ci_udc_unregister(udc);
 }
 
 static struct driver_d fsl_udc_driver = {
diff --git a/drivers/usb/imx/chipidea-imx.c b/drivers/usb/imx/chipidea-imx.c
index 7bf2ef76c5..8792217706 100644
--- a/drivers/usb/imx/chipidea-imx.c
+++ b/drivers/usb/imx/chipidea-imx.c
@@ -47,6 +47,7 @@ struct imx_chipidea {
 	struct usb_phy *usbphy;
 	struct clk *clk;
 	struct ehci_host *ehci;
+	struct fsl_udc *udc;
 };
 
 static int imx_chipidea_port_init(void *drvdata)
@@ -216,8 +217,14 @@ static int ci_register_role(struct imx_chipidea *ci)
 
 	if (ci->mode == IMX_USB_MODE_DEVICE) {
 		if (IS_ENABLED(CONFIG_USB_GADGET_DRIVER_ARC)) {
+			struct fsl_udc *udc;
 			ci->role_registered = IMX_USB_MODE_DEVICE;
-			return ci_udc_register(ci->dev, ci->base);
+
+			udc = ci_udc_register(ci->dev, ci->base);
+			if (IS_ERR(udc))
+				return PTR_ERR(udc);
+
+			ci->udc = udc;
 		} else {
 			dev_err(ci->dev, "USB device support not available\n");
 			return -ENODEV;
@@ -369,8 +376,8 @@ static void imx_chipidea_remove(struct device_d *dev)
 	if (ci->ehci)
 		ehci_unregister(ci->ehci);
 
-	if (IS_ENABLED(CONFIG_USB_GADGET_DRIVER_ARC))
-		ci_udc_unregister();
+	if (IS_ENABLED(CONFIG_USB_GADGET_DRIVER_ARC) && ci->udc)
+		ci_udc_unregister(ci->udc);
 }
 
 static __maybe_unused struct of_device_id imx_chipidea_dt_ids[] = {
diff --git a/include/usb/fsl_usb2.h b/include/usb/fsl_usb2.h
index 881a5d4fdf..39757f71ad 100644
--- a/include/usb/fsl_usb2.h
+++ b/include/usb/fsl_usb2.h
@@ -23,7 +23,9 @@ struct fsl_usb2_platform_data {
 	unsigned int			port_enables;
 };
 
-int ci_udc_register(struct device_d *dev, void __iomem *regs);
-void ci_udc_unregister(void);
+struct fsl_udc;
+
+struct fsl_udc *ci_udc_register(struct device_d *dev, void __iomem *regs);
+void ci_udc_unregister(struct fsl_udc *);
 
 #endif /* __USB_FSL_USB2_H */
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-10-26  9:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-26  9:33 [PATCH 0/8] USB ehci/chipidea fixes Sascha Hauer
2018-10-26  9:33 ` [PATCH 1/8] usb: gadget: fsl_udc: Drop using global variable Sascha Hauer
2018-10-26  9:33 ` [PATCH 2/8] usb: host: ehci: rename ehci_priv to ehci_host Sascha Hauer
2018-10-26  9:33 ` [PATCH 3/8] usb: Add usb_unregister_host() Sascha Hauer
2018-10-26  9:33 ` [PATCH 4/8] usb: host: ehci: add ehci_unregister() Sascha Hauer
2018-10-26  9:33 ` [PATCH 5/8] usb: host: ehci: do not use dev->priv Sascha Hauer
2018-10-26  9:33 ` [PATCH 6/8] usb: host: ehci-atmel: unregister host on device remove Sascha Hauer
2018-10-26  9:33 ` [PATCH 7/8] usb: imx: unregister ehci controller on device removal Sascha Hauer
2018-10-26  9:33 ` [PATCH 8/8] usb: gadget: fsl_udc: pass controller instance to unregister Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox