[PATCH 0/7] usb: bdc: Updates and fixes to the USB BDC driver

[PATCH 0/7] usb: bdc: Updates and fixes to the USB BDC driver

[Al Cooper]

Updates and fixes to the Broadcom USB BDC driver.

Al Cooper (4):
  dt-bindings: usb: bdc: Update compatible strings
  usb: bdc: Add compatible string for new style USB DT nodes
  usb: bdc: Adb shows offline after resuming from S2
  usb: bdc: driver runs out of buffer descriptors on large ADB transfers

Danesh Petigara (1):
  usb: bdc: Halt controller on suspend

Florian Fainelli (1):
  usb: bdc: Use devm_clk_get_optional()

Sasi Kumar (1):
  bdc: Fix bug causing crash after multiple disconnects

 .../devicetree/bindings/usb/brcm,bdc.txt      |  4 +--
 drivers/usb/gadget/udc/bdc/bdc.h              |  2 +-
 drivers/usb/gadget/udc/bdc/bdc_core.c         | 26 ++++++++++++++-----
 drivers/usb/gadget/udc/bdc/bdc_ep.c           | 16 +++++++-----
 4 files changed, 32 insertions(+), 16 deletions(-)

-- 
2.17.1

[PATCH 1/7] dt-bindings: usb: bdc: Update compatible strings

[Al Cooper]

Remove "brcm,bdc-v0.16" because it was never used on any system.
Add "brcm,bdc-udc-v3.1" which exists for any STB system with BDC.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 Documentation/devicetree/bindings/usb/brcm,bdc.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/brcm,bdc.txt b/Documentation/devicetree/bindings/usb/brcm,bdc.txt
index 63e63af3bf59..597f3902d857 100644
--- a/Documentation/devicetree/bindings/usb/brcm,bdc.txt
+++ b/Documentation/devicetree/bindings/usb/brcm,bdc.txt
@@ -4,7 +4,7 @@ Broadcom USB Device Controller (BDC)
 Required properties:
 
 - compatible: must be one of:
-                "brcm,bdc-v0.16"
+                "brcm,bdc-udc-v3.1"
                 "brcm,bdc"
 - reg: the base register address and length
 - interrupts: the interrupt line for this controller
@@ -21,7 +21,7 @@ On Broadcom STB platforms, these properties are required:
 Example:
 
         bdc@f0b02000 {
-                compatible = "brcm,bdc-v0.16";
+                compatible = "brcm,bdc-udc-v3.1";
                 reg = <0xf0b02000 0xfc4>;
                 interrupts = <0x0 0x60 0x0>;
                 phys = <&usbphy_0 0x0>;
-- 
2.17.1

[PATCH 2/7] usb: bdc: Add compatible string for new style USB DT nodes

[Al Cooper]

Add compatible string for some newer boards that only have this
as there match sting.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/usb/gadget/udc/bdc/bdc_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c
index 02a3a774670b..436a734ea0bc 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_core.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
@@ -629,6 +629,7 @@ static SIMPLE_DEV_PM_OPS(bdc_pm_ops, bdc_suspend,
 		bdc_resume);
 
 static const struct of_device_id bdc_of_match[] = {
+	{ .compatible = "brcm,bdc-udc-v2" },
 	{ .compatible = "brcm,bdc-v0.16" },
 	{ .compatible = "brcm,bdc" },
 	{ /* sentinel */ }
-- 
2.17.1

[PATCH 3/7] bdc: Fix bug causing crash after multiple disconnects

[Al Cooper]

From: Sasi Kumar <sasi.kumar@broadcom.com>

Multiple connects/disconnects can cause a crash on the second
disconnect. The driver had a problem where it would try to send
endpoint commands after it was disconnected which is not allowed
by the hardware. The fix is to only allow the endpoint commands
when the endpoint is connected. This will also fix issues that
showed up when using configfs to create gadgets.

refs #SWLINUX-5477

Signed-off-by: Sasi Kumar <sasi.kumar@broadcom.com>
Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/usb/gadget/udc/bdc/bdc_core.c |  4 ++++
 drivers/usb/gadget/udc/bdc/bdc_ep.c   | 16 ++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c
index 436a734ea0bc..be833d9e9db2 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_core.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
@@ -282,6 +282,7 @@ static void bdc_mem_init(struct bdc *bdc, bool reinit)
 	 * in that case reinit is passed as 1
 	 */
 	if (reinit) {
+		int i;
 		/* Enable interrupts */
 		temp = bdc_readl(bdc->regs, BDC_BDCSC);
 		temp |= BDC_GIE;
@@ -291,6 +292,9 @@ static void bdc_mem_init(struct bdc *bdc, bool reinit)
 		/* Initialize SRR to 0 */
 		memset(bdc->srr.sr_bds, 0,
 					NUM_SR_ENTRIES * sizeof(struct bdc_bd));
+		/* clear ep flags to avoid post disconnect stops/deconfigs */
+		for (i = 1; i < bdc->num_eps; ++i)
+			bdc->bdc_ep_array[i]->flags = 0;
 	} else {
 		/* One time initiaization only */
 		/* Enable status report function pointers */
diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c b/drivers/usb/gadget/udc/bdc/bdc_ep.c
index d49c6dc1082d..9ddc0b4e92c9 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_ep.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c
@@ -615,7 +615,6 @@ int bdc_ep_enable(struct bdc_ep *ep)
 	}
 	bdc_dbg_bd_list(bdc, ep);
 	/* only for ep0: config ep is called for ep0 from connect event */
-	ep->flags |= BDC_EP_ENABLED;
 	if (ep->ep_num == 1)
 		return ret;
 
@@ -759,10 +758,13 @@ static int ep_dequeue(struct bdc_ep *ep, struct bdc_req *req)
 					__func__, ep->name, start_bdi, end_bdi);
 	dev_dbg(bdc->dev, "ep_dequeue ep=%p ep->desc=%p\n",
 						ep, (void *)ep->usb_ep.desc);
-	/* Stop the ep to see where the HW is ? */
-	ret = bdc_stop_ep(bdc, ep->ep_num);
-	/* if there is an issue with stopping ep, then no need to go further */
-	if (ret)
+	/* if still connected, stop the ep to see where the HW is ? */
+	if (!(bdc_readl(bdc->regs, BDC_USPC) & BDC_PST_MASK)) {
+		ret = bdc_stop_ep(bdc, ep->ep_num);
+		/* if there is an issue, then no need to go further */
+		if (ret)
+			return 0;
+	} else
 		return 0;
 
 	/*
@@ -1911,7 +1913,9 @@ static int bdc_gadget_ep_disable(struct usb_ep *_ep)
 		__func__, ep->name, ep->flags);
 
 	if (!(ep->flags & BDC_EP_ENABLED)) {
-		dev_warn(bdc->dev, "%s is already disabled\n", ep->name);
+		if (bdc->gadget.speed != USB_SPEED_UNKNOWN)
+			dev_warn(bdc->dev, "%s is already disabled\n",
+				 ep->name);
 		return 0;
 	}
 	spin_lock_irqsave(&bdc->lock, flags);
-- 
2.17.1

[PATCH 4/7] usb: bdc: Adb shows offline after resuming from S2

[Al Cooper]

On Android systems, After temporarily putting device to S2 by
short pressing the power button on the remote, the display turns
off. Then press the power button to turn the display back up. Adb
devices would show the devices is offline. It needs a physical
disconnect of the usb cable or power cycle to bring the device
back online. The device is operational otherwise.

The problem is that during S2 resume, the ADB gadget driver could
not link back with the BDC driver because the endpoint flags were
cleared. The fix is to clear the endpoint flags for the disconnect
case only and not for S2 exit.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/usb/gadget/udc/bdc/bdc_core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c
index be833d9e9db2..47bcc9f6afbd 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_core.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
@@ -292,9 +292,13 @@ static void bdc_mem_init(struct bdc *bdc, bool reinit)
 		/* Initialize SRR to 0 */
 		memset(bdc->srr.sr_bds, 0,
 					NUM_SR_ENTRIES * sizeof(struct bdc_bd));
-		/* clear ep flags to avoid post disconnect stops/deconfigs */
-		for (i = 1; i < bdc->num_eps; ++i)
-			bdc->bdc_ep_array[i]->flags = 0;
+		/*
+		 * clear ep flags to avoid post disconnect stops/deconfigs but
+		 * not during S2 exit
+		 */
+		if (!bdc->gadget.speed)
+			for (i = 1; i < bdc->num_eps; ++i)
+				bdc->bdc_ep_array[i]->flags = 0;
 	} else {
 		/* One time initiaization only */
 		/* Enable status report function pointers */
-- 
2.17.1

[PATCH 5/7] usb: bdc: driver runs out of buffer descriptors on large ADB transfers

[Al Cooper]

Version v1.0.40 of the Android host ADB software increased maximum
transfer sizes from 256K to 1M. Since the STB ADB gadget driver
requests only 16K at a time, the BDC driver ran out of buffer
descriptors (BDs) if the queuing happens faster than the incoming
16K transfers. This issue is fixed by doubling the number of BDs
that can be queued so that the entire 1M request can be queued
without running out of buffers.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/usb/gadget/udc/bdc/bdc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/bdc/bdc.h b/drivers/usb/gadget/udc/bdc/bdc.h
index 6e1e881dc51e..ac75e25c3b6a 100644
--- a/drivers/usb/gadget/udc/bdc/bdc.h
+++ b/drivers/usb/gadget/udc/bdc/bdc.h
@@ -44,7 +44,7 @@
 #define NUM_SR_ENTRIES	64
 
 /* Num of bds per table */
-#define NUM_BDS_PER_TABLE	32
+#define NUM_BDS_PER_TABLE	64
 
 /* Num of tables in bd list for control,bulk and Int ep */
 #define NUM_TABLES	2
-- 
2.17.1

[PATCH 6/7] usb: bdc: Halt controller on suspend

[Al Cooper]

From: Danesh Petigara <danesh.petigara@broadcom.com>

GISB bus error kernel panics have been observed during S2 transition
tests on the 7271t platform. The errors are a result of the BDC
interrupt handler trying to access BDC register space after the
system's suspend callbacks have completed.

Adding a suspend hook to the BDC driver that halts the controller before
S2 entry thus preventing unwanted access to the BDC register space during
this transition.

Signed-off-by: Danesh Petigara <danesh.petigara@broadcom.com>
Signed-off-by: Al Cooper <alcooperx@gmail.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/usb/gadget/udc/bdc/bdc_core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c
index 47bcc9f6afbd..ac989adefe9d 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_core.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
@@ -607,9 +607,14 @@ static int bdc_remove(struct platform_device *pdev)
 static int bdc_suspend(struct device *dev)
 {
 	struct bdc *bdc = dev_get_drvdata(dev);
+	int ret;
 
-	clk_disable_unprepare(bdc->clk);
-	return 0;
+	/* Halt the controller */
+	ret = bdc_stop(bdc);
+	if (!ret)
+		clk_disable_unprepare(bdc->clk);
+
+	return ret;
 }
 
 static int bdc_resume(struct device *dev)
-- 
2.17.1

[PATCH 7/7] usb: bdc: Use devm_clk_get_optional()

[Al Cooper]

From: Florian Fainelli <florian.fainelli@broadcom.com>

The BDC clock is optional and we may get an -EPROBE_DEFER error code
which would not be propagated correctly, fix this by using
devm_clk_get_optional().

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/usb/gadget/udc/bdc/bdc_core.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/udc/bdc/bdc_core.c b/drivers/usb/gadget/udc/bdc/bdc_core.c
index ac989adefe9d..5d8bd9129365 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_core.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_core.c
@@ -497,11 +497,9 @@ static int bdc_probe(struct platform_device *pdev)
 
 	dev_dbg(dev, "%s()\n", __func__);
 
-	clk = devm_clk_get(dev, "sw_usbd");
-	if (IS_ERR(clk)) {
-		dev_info(dev, "Clock not found in Device Tree\n");
-		clk = NULL;
-	}
+	clk = devm_clk_get_optional(dev, "sw_usbd");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	ret = clk_prepare_enable(clk);
 	if (ret) {
-- 
2.17.1

Re: [PATCH 2/7] usb: bdc: Add compatible string for new style USB DT nodes

[Florian Fainelli]

On 7/17/2020 8:23 AM, Al Cooper wrote:
> Add compatible string for some newer boards that only have this
> as there match sting.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>

This looks good to me, however you should also include it in the binding
document.
-- 
Florian

Re: [PATCH 3/7] bdc: Fix bug causing crash after multiple disconnects

[Florian Fainelli]

On 7/17/2020 8:23 AM, Al Cooper wrote:
> From: Sasi Kumar <sasi.kumar@broadcom.com>
> 
> Multiple connects/disconnects can cause a crash on the second
> disconnect. The driver had a problem where it would try to send
> endpoint commands after it was disconnected which is not allowed
> by the hardware. The fix is to only allow the endpoint commands
> when the endpoint is connected. This will also fix issues that
> showed up when using configfs to create gadgets.
> 
> refs #SWLINUX-5477

This internal reference should be removed and maybe we should be
providing a Fixes tag as well since this is a bug fix?
-- 
Florian

Re: [PATCH 7/7] usb: bdc: Use devm_clk_get_optional()

[Florian Fainelli]

On 7/17/2020 8:23 AM, Al Cooper wrote:
> From: Florian Fainelli <florian.fainelli@broadcom.com>
> 
> The BDC clock is optional and we may get an -EPROBE_DEFER error code
> which would not be propagated correctly, fix this by using
> devm_clk_get_optional().
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

Since you are carrying this patch on my behalf your Signed-off-by should
also be present, if you don't mind, I would like my gmail.com address to
be used here for consistency with all submissions done throughout my
tenure at broadcom, thanks!
-- 
Florian

Re: [PATCH 5/7] usb: bdc: driver runs out of buffer descriptors on large ADB transfers

[Florian Fainelli]

On 7/17/2020 8:23 AM, Al Cooper wrote:
> Version v1.0.40 of the Android host ADB software increased maximum
> transfer sizes from 256K to 1M. Since the STB ADB gadget driver
> requests only 16K at a time, the BDC driver ran out of buffer
> descriptors (BDs) if the queuing happens faster than the incoming
> 16K transfers. This issue is fixed by doubling the number of BDs
> that can be queued so that the entire 1M request can be queued
> without running out of buffers.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

You can certainly remove my SoB here, since you are carrying your own
patch, thanks!
-- 
Florian

Re: [PATCH 0/7] usb: bdc: Updates and fixes to the USB BDC driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 713 bytes --]


Hi,

Al Cooper <alcooperx@gmail.com> writes:
> Updates and fixes to the Broadcom USB BDC driver.
>
> Al Cooper (4):
>   dt-bindings: usb: bdc: Update compatible strings
>   usb: bdc: Add compatible string for new style USB DT nodes
>   usb: bdc: Adb shows offline after resuming from S2
>   usb: bdc: driver runs out of buffer descriptors on large ADB transfers
>
> Danesh Petigara (1):
>   usb: bdc: Halt controller on suspend
>
> Florian Fainelli (1):
>   usb: bdc: Use devm_clk_get_optional()
>
> Sasi Kumar (1):
>   bdc: Fix bug causing crash after multiple disconnects

What should we do here? There are few comments which seem
unresolved. Are we getting a new version?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 0/7] usb: bdc: Updates and fixes to the USB BDC driver

[Alan Cooper]

On Tue, Jul 21, 2020 at 5:33 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> Al Cooper <alcooperx@gmail.com> writes:
> > Updates and fixes to the Broadcom USB BDC driver.
> >
> > Al Cooper (4):
> >   dt-bindings: usb: bdc: Update compatible strings
> >   usb: bdc: Add compatible string for new style USB DT nodes
> >   usb: bdc: Adb shows offline after resuming from S2
> >   usb: bdc: driver runs out of buffer descriptors on large ADB transfers
> >
> > Danesh Petigara (1):
> >   usb: bdc: Halt controller on suspend
> >
> > Florian Fainelli (1):
> >   usb: bdc: Use devm_clk_get_optional()
> >
> > Sasi Kumar (1):
> >   bdc: Fix bug causing crash after multiple disconnects
>
> What should we do here? There are few comments which seem
> unresolved. Are we getting a new version?

I'm resolving the comments and submitting v2 today.

Thanks
Al

>
> --
> balbi