[PATCH v6 0/4] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's

[PATCH v6 0/4] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's

[Al Cooper]

v6 - Remove "contains:" from compatible section of
     brcm,bcm7445-ehci.yaml as requested by Rob Herring.

v5 - Use devm_platform_get_and_ioremap_resource() in ehci-brcm.c
     as requested by Andy Shevchenko.
   - Add pm_runtime_set_active() to ehci_resume() in ehci-brcm.c
     as requested by Alan Stern.

v4 - A few more fixes to the brcm,bcm7445-ehci.yaml dt-bindings
     document requested by Rob Herring.
   - Fixed ordering issue in MAINTAINERS as requested by
     Andy Shevchenko.

v3 - Addressed all of Andy Shevchenko's review comments for
     ehci-brcm.c.
   - Fixed the brcm,bcm7445-ehci.yaml dt-bindings document,
     dt_binding_check now passes.
   - Added the XHCI functionality to xhci-plat.c instead of creating
     new brcmstb files, as suggested by Mathias Nyman.

v2 - Addressed Andy Shevchenko's review comments.
   - Fixed dt_binding_check error pointed out by Rob Herring.
   - Removed pr_info message in ehci_brcm_init as suggested by
     Greg Kroah-Hartman.


Al Cooper (4):
  dt-bindings: Add Broadcom STB USB support
  usb: xhci: xhci-plat: Add support for Broadcom STB SoC's
  usb: ehci: Add new EHCI driver for Broadcom STB SoC's
  usb: host: Add ability to build new Broadcom STB USB drivers

 .../bindings/usb/brcm,bcm7445-ehci.yaml       |  59 ++++
 .../devicetree/bindings/usb/usb-xhci.txt      |   1 +
 MAINTAINERS                                   |   8 +
 drivers/usb/host/Kconfig                      |  16 +
 drivers/usb/host/Makefile                     |  16 +-
 drivers/usb/host/ehci-brcm.c                  | 290 ++++++++++++++++++
 drivers/usb/host/xhci-plat.c                  |  10 +
 7 files changed, 394 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml
 create mode 100644 drivers/usb/host/ehci-brcm.c

-- 
2.17.1

[PATCH v6 1/4] dt-bindings: Add Broadcom STB USB support

[Al Cooper]

Add DT bindings for Broadcom STB USB EHCI and XHCI drivers.

NOTE: The OHCI driver is not included because it uses the generic
      platform driver.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/usb/brcm,bcm7445-ehci.yaml       | 59 +++++++++++++++++++
 .../devicetree/bindings/usb/usb-xhci.txt      |  1 +
 2 files changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml

diff --git a/Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml b/Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml
new file mode 100644
index 000000000000..2a9acf2b5a64
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/brcm,bcm7445-ehci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom STB USB EHCI Controller Device Tree Bindings
+
+allOf:
+  - $ref: "usb-hcd.yaml"
+
+maintainers:
+  - Al Cooper <alcooperx@gmail.com>
+
+properties:
+  compatible:
+    const: brcm,bcm7445-ehci
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+    description: Clock specifier for the EHCI clock
+
+  clock-names:
+    const: sw_usb
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: usbphy
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - phys
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    usb@f0b00300 {
+        compatible = "brcm,bcm7445-ehci";
+        reg = <0xf0b00300 0xa8>;
+        interrupts = <0x0 0x5a 0x0>;
+        phys = <&usbphy_0 0x0>;
+        phy-names = "usbphy";
+        clocks = <&usb20>;
+        clock-names = "sw_usb";
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index dc025f126d71..23e89d798b1b 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -24,6 +24,7 @@ Required properties:
       device
     - "renesas,rcar-gen3-xhci" for a generic R-Car Gen3 or RZ/G2 compatible
       device
+    - "brcm,bcm7445-xhci" for Broadcom STB SoCs with XHCI
     - "xhci-platform" (deprecated)
 
     When compatible with the generic version, nodes must list the
-- 
2.17.1

[PATCH v6 2/4] usb: xhci: xhci-plat: Add support for Broadcom STB SoC's

[Al Cooper]

Add support for Broadcom STB SoC's to the xhci platform driver

Signed-off-by: Al Cooper <alcooperx@gmail.com>
Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-plat.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 1d4f6f85f0fe..44406d0eb317 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -112,6 +112,10 @@ static const struct xhci_plat_priv xhci_plat_renesas_rcar_gen3 = {
 	SET_XHCI_PLAT_PRIV_FOR_RCAR(XHCI_RCAR_FIRMWARE_NAME_V3)
 };
 
+static const struct xhci_plat_priv xhci_plat_brcm = {
+	.quirks = XHCI_RESET_ON_RESUME,
+};
+
 static const struct of_device_id usb_xhci_of_match[] = {
 	{
 		.compatible = "generic-xhci",
@@ -147,6 +151,12 @@ static const struct of_device_id usb_xhci_of_match[] = {
 	}, {
 		.compatible = "renesas,rcar-gen3-xhci",
 		.data = &xhci_plat_renesas_rcar_gen3,
+	}, {
+		.compatible = "brcm,xhci-brcm-v2",
+		.data = &xhci_plat_brcm,
+	}, {
+		.compatible = "brcm,bcm7445-xhci",
+		.data = &xhci_plat_brcm,
 	},
 	{},
 };
-- 
2.17.1

[PATCH v6 3/4] usb: ehci: Add new EHCI driver for Broadcom STB SoC's

[Al Cooper]

Add a new EHCI driver for Broadcom STB SoC's. A new EHCI driver
was created instead of adding support to the existing ehci platform
driver because of the code required to workaround bugs in the EHCI
controller.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/usb/host/ehci-brcm.c | 290 +++++++++++++++++++++++++++++++++++
 1 file changed, 290 insertions(+)
 create mode 100644 drivers/usb/host/ehci-brcm.c

diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
new file mode 100644
index 000000000000..381bed5fdab0
--- /dev/null
+++ b/drivers/usb/host/ehci-brcm.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020, Broadcom */
+
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+#include <linux/iopoll.h>
+
+#include "ehci.h"
+
+#define hcd_to_ehci_priv(h) ((struct brcm_priv *)hcd_to_ehci(h)->priv)
+
+struct brcm_priv {
+	struct clk *clk;
+};
+
+static const char brcm_hcd_name[] = "ehci-brcm";
+
+static int (*org_hub_control)(struct usb_hcd *hcd,
+			u16 typeReq, u16 wValue, u16 wIndex,
+			char *buf, u16 wLength);
+
+/*
+ * ehci_brcm_wait_for_sof
+ * Wait for start of next microframe, then wait extra delay microseconds
+ */
+static inline void ehci_brcm_wait_for_sof(struct ehci_hcd *ehci, u32 delay)
+{
+	u32 frame_idx = ehci_readl(ehci, &ehci->regs->frame_index);
+	u32 val;
+	int res;
+
+	/* Wait for next microframe (every 125 usecs) */
+	res = readl_relaxed_poll_timeout(&ehci->regs->frame_index, val,
+					 val != frame_idx, 1, 130);
+	if (res)
+		dev_err(ehci_to_hcd(ehci)->self.controller,
+			"Error waiting for SOF\n");
+	udelay(delay);
+}
+
+/*
+ * ehci_brcm_hub_control
+ * Intercept echi-hcd request to complete RESUME and align it to the start
+ * of the next microframe.
+ * If RESUME is complete too late in the microframe, host controller
+ * detects babble on suspended port and resets the port afterwards.
+ * This s/w workaround allows to avoid this problem.
+ * See SWLINUX-1909 for more details
+ */
+static int ehci_brcm_hub_control(
+	struct usb_hcd	*hcd,
+	u16		typeReq,
+	u16		wValue,
+	u16		wIndex,
+	char		*buf,
+	u16		wLength)
+{
+	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
+	int		ports = HCS_N_PORTS(ehci->hcs_params);
+	u32 __iomem	*status_reg = &ehci->regs->port_status[
+				(wIndex & 0xff) - 1];
+	unsigned long flags;
+	int retval, irq_disabled = 0;
+
+	/*
+	 * RESUME is cleared when GetPortStatus() is called 20ms after start
+	 * of RESUME
+	 */
+	if ((typeReq == GetPortStatus) &&
+	    (wIndex && wIndex <= ports) &&
+	    ehci->reset_done[wIndex-1] &&
+	    time_after_eq(jiffies, ehci->reset_done[wIndex-1]) &&
+	    (ehci_readl(ehci, status_reg) & PORT_RESUME)) {
+
+		/*
+		 * to make sure we are not interrupted until RESUME bit
+		 * is cleared, disable interrupts on current CPU
+		 */
+		ehci_dbg(ehci, "SOF alignment workaround\n");
+		irq_disabled = 1;
+		local_irq_save(flags);
+		ehci_brcm_wait_for_sof(ehci, 5);
+	}
+	retval = (*org_hub_control)(hcd, typeReq, wValue, wIndex, buf, wLength);
+	if (irq_disabled)
+		local_irq_restore(flags);
+	return retval;
+}
+
+static int ehci_brcm_reset(struct usb_hcd *hcd)
+{
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+
+	ehci->big_endian_mmio = 1;
+
+	ehci->caps = (struct ehci_caps *) hcd->regs;
+	ehci->regs = (struct ehci_regs *) (hcd->regs +
+		HC_LENGTH(ehci, ehci_readl(ehci, &ehci->caps->hc_capbase)));
+
+	/* This fixes the lockup during reboot due to prior interrupts */
+	ehci_writel(ehci, CMD_RESET, &ehci->regs->command);
+	mdelay(10);
+
+	/*
+	 * SWLINUX-1705: Avoid OUT packet underflows during high memory
+	 *   bus usage
+	 * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
+	 */
+	ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
+	ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
+
+	return ehci_setup(hcd);
+}
+
+static struct hc_driver __read_mostly ehci_brcm_hc_driver;
+
+static const struct ehci_driver_overrides brcm_overrides __initconst = {
+
+	.reset = ehci_brcm_reset,
+	.extra_priv_size = sizeof(struct brcm_priv),
+};
+
+static int ehci_brcm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res_mem;
+	struct brcm_priv *priv;
+	struct usb_hcd *hcd;
+	int irq;
+	int err;
+
+	if (usb_disabled())
+		return -ENODEV;
+
+	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+	if (err)
+		return err;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	/* Hook the hub control routine to work around a bug */
+	if (!org_hub_control)
+		org_hub_control = ehci_brcm_hc_driver.hub_control;
+	ehci_brcm_hc_driver.hub_control = ehci_brcm_hub_control;
+
+	/* initialize hcd */
+	hcd = usb_create_hcd(&ehci_brcm_hc_driver, dev, dev_name(dev));
+	if (!hcd)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, hcd);
+	priv = hcd_to_ehci_priv(hcd);
+
+	priv->clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		err = PTR_ERR(priv->clk);
+		goto err_hcd;
+	}
+
+	err = clk_prepare_enable(priv->clk);
+	if (err)
+		goto err_hcd;
+
+	hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res_mem);
+	if (IS_ERR(hcd->regs)) {
+		err = PTR_ERR(hcd->regs);
+		goto err_clk;
+	}
+	hcd->rsrc_start = res_mem->start;
+	hcd->rsrc_len = resource_size(res_mem);
+	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
+	if (err)
+		goto err_clk;
+
+	device_wakeup_enable(hcd->self.controller);
+	device_enable_async_suspend(hcd->self.controller);
+	platform_set_drvdata(pdev, hcd);
+
+	return 0;
+
+err_clk:
+	clk_disable_unprepare(priv->clk);
+err_hcd:
+	usb_put_hcd(hcd);
+
+	return err;
+}
+
+static int ehci_brcm_remove(struct platform_device *dev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(dev);
+	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
+
+	usb_remove_hcd(hcd);
+	clk_disable_unprepare(priv->clk);
+	usb_put_hcd(hcd);
+	return 0;
+}
+
+static int __maybe_unused ehci_brcm_suspend(struct device *dev)
+{
+	int ret;
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
+	bool do_wakeup = device_may_wakeup(dev);
+
+	ret = ehci_suspend(hcd, do_wakeup);
+	if (ret)
+		return ret;
+	clk_disable_unprepare(priv->clk);
+	return 0;
+}
+
+static int __maybe_unused ehci_brcm_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
+	int err;
+
+	err = clk_prepare_enable(priv->clk);
+	if (err)
+		return err;
+	/*
+	 * SWLINUX-1705: Avoid OUT packet underflows during high memory
+	 *   bus usage
+	 * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00
+	 * @ 0x90
+	 */
+	ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
+	ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
+
+	ehci_resume(hcd, false);
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(ehci_brcm_pm_ops, ehci_brcm_suspend,
+		ehci_brcm_resume);
+
+static const struct of_device_id brcm_ehci_of_match[] = {
+	{ .compatible = "brcm,ehci-brcm-v2", },
+	{ .compatible = "brcm,bcm7445-ehci", },
+	{}
+};
+
+static struct platform_driver ehci_brcm_driver = {
+	.probe		= ehci_brcm_probe,
+	.remove		= ehci_brcm_remove,
+	.shutdown	= usb_hcd_platform_shutdown,
+	.driver		= {
+		.name	= "ehci-brcm",
+		.pm	= &ehci_brcm_pm_ops,
+		.of_match_table = brcm_ehci_of_match,
+	}
+};
+
+static int __init ehci_brcm_init(void)
+{
+	if (usb_disabled())
+		return -ENODEV;
+
+	ehci_init_driver(&ehci_brcm_hc_driver, &brcm_overrides);
+	return platform_driver_register(&ehci_brcm_driver);
+}
+module_init(ehci_brcm_init);
+
+static void __exit ehci_brcm_exit(void)
+{
+	platform_driver_unregister(&ehci_brcm_driver);
+}
+module_exit(ehci_brcm_exit);
+
+MODULE_ALIAS("platform:ehci-brcm");
+MODULE_DESCRIPTION("EHCI Broadcom STB driver");
+MODULE_AUTHOR("Al Cooper");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v6 3/4] usb: ehci: Add new EHCI driver for Broadcom STB SoC's

[Greg Kroah-Hartman]

On Thu, Apr 30, 2020 at 07:12:57AM -0400, Al Cooper wrote:
> Add a new EHCI driver for Broadcom STB SoC's. A new EHCI driver
> was created instead of adding support to the existing ehci platform
> driver because of the code required to workaround bugs in the EHCI
> controller.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/usb/host/ehci-brcm.c | 290 +++++++++++++++++++++++++++++++++++
>  1 file changed, 290 insertions(+)
>  create mode 100644 drivers/usb/host/ehci-brcm.c

I need an ack from the EHCI maintainer to agree that this needs a whole
new driver file...

> 
> diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
> new file mode 100644
> index 000000000000..381bed5fdab0
> --- /dev/null
> +++ b/drivers/usb/host/ehci-brcm.c
> @@ -0,0 +1,290 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2020, Broadcom */
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +#include <linux/iopoll.h>
> +
> +#include "ehci.h"
> +
> +#define hcd_to_ehci_priv(h) ((struct brcm_priv *)hcd_to_ehci(h)->priv)
> +
> +struct brcm_priv {
> +	struct clk *clk;
> +};
> +
> +static const char brcm_hcd_name[] = "ehci-brcm";

You don't use this anywhere?  Are you sure this didn't cause compile
warnings?

> +
> +static int (*org_hub_control)(struct usb_hcd *hcd,
> +			u16 typeReq, u16 wValue, u16 wIndex,
> +			char *buf, u16 wLength);

So you only support one device per system?  That feels bad...


> +
> +/*
> + * ehci_brcm_wait_for_sof
> + * Wait for start of next microframe, then wait extra delay microseconds
> + */
> +static inline void ehci_brcm_wait_for_sof(struct ehci_hcd *ehci, u32 delay)
> +{
> +	u32 frame_idx = ehci_readl(ehci, &ehci->regs->frame_index);
> +	u32 val;
> +	int res;
> +
> +	/* Wait for next microframe (every 125 usecs) */
> +	res = readl_relaxed_poll_timeout(&ehci->regs->frame_index, val,
> +					 val != frame_idx, 1, 130);
> +	if (res)
> +		dev_err(ehci_to_hcd(ehci)->self.controller,
> +			"Error waiting for SOF\n");
> +	udelay(delay);
> +}
> +
> +/*
> + * ehci_brcm_hub_control
> + * Intercept echi-hcd request to complete RESUME and align it to the start
> + * of the next microframe.
> + * If RESUME is complete too late in the microframe, host controller
> + * detects babble on suspended port and resets the port afterwards.
> + * This s/w workaround allows to avoid this problem.
> + * See SWLINUX-1909 for more details
> + */
> +static int ehci_brcm_hub_control(
> +	struct usb_hcd	*hcd,
> +	u16		typeReq,
> +	u16		wValue,
> +	u16		wIndex,
> +	char		*buf,
> +	u16		wLength)
> +{
> +	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
> +	int		ports = HCS_N_PORTS(ehci->hcs_params);
> +	u32 __iomem	*status_reg = &ehci->regs->port_status[
> +				(wIndex & 0xff) - 1];

Horrid line-wrapping, put this assignment below so it can be read.

And wIndex is little endian?  Or native?

> +	unsigned long flags;
> +	int retval, irq_disabled = 0;
> +
> +	/*
> +	 * RESUME is cleared when GetPortStatus() is called 20ms after start
> +	 * of RESUME
> +	 */
> +	if ((typeReq == GetPortStatus) &&
> +	    (wIndex && wIndex <= ports) &&
> +	    ehci->reset_done[wIndex-1] &&
> +	    time_after_eq(jiffies, ehci->reset_done[wIndex-1]) &&
> +	    (ehci_readl(ehci, status_reg) & PORT_RESUME)) {
> +
> +		/*
> +		 * to make sure we are not interrupted until RESUME bit
> +		 * is cleared, disable interrupts on current CPU
> +		 */
> +		ehci_dbg(ehci, "SOF alignment workaround\n");
> +		irq_disabled = 1;
> +		local_irq_save(flags);
> +		ehci_brcm_wait_for_sof(ehci, 5);
> +	}
> +	retval = (*org_hub_control)(hcd, typeReq, wValue, wIndex, buf, wLength);

But this might not be set, did you just crash?

If it is always set, then why does it need to be a function pointer at
all?

> +	if (irq_disabled)
> +		local_irq_restore(flags);
> +	return retval;
> +}
> +
> +static int ehci_brcm_reset(struct usb_hcd *hcd)
> +{
> +	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> +
> +	ehci->big_endian_mmio = 1;
> +
> +	ehci->caps = (struct ehci_caps *) hcd->regs;
> +	ehci->regs = (struct ehci_regs *) (hcd->regs +

coding style, did you run this through checkpatch.pl?

> +		HC_LENGTH(ehci, ehci_readl(ehci, &ehci->caps->hc_capbase)));
> +
> +	/* This fixes the lockup during reboot due to prior interrupts */
> +	ehci_writel(ehci, CMD_RESET, &ehci->regs->command);
> +	mdelay(10);
> +
> +	/*
> +	 * SWLINUX-1705: Avoid OUT packet underflows during high memory
> +	 *   bus usage
> +	 * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
> +	 */
> +	ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> +	ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
> +
> +	return ehci_setup(hcd);
> +}
> +
> +static struct hc_driver __read_mostly ehci_brcm_hc_driver;
> +
> +static const struct ehci_driver_overrides brcm_overrides __initconst = {
> +

No blank line.

> +	.reset = ehci_brcm_reset,
> +	.extra_priv_size = sizeof(struct brcm_priv),
> +};
> +
> +static int ehci_brcm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res_mem;
> +	struct brcm_priv *priv;
> +	struct usb_hcd *hcd;
> +	int irq;
> +	int err;
> +
> +	if (usb_disabled())
> +		return -ENODEV;
> +
> +	err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +	if (err)
> +		return err;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)

"<=" right?

> +		return irq;
> +
> +	/* Hook the hub control routine to work around a bug */

What bug?  This feels wrong.

> +	if (!org_hub_control)
> +		org_hub_control = ehci_brcm_hc_driver.hub_control;
> +	ehci_brcm_hc_driver.hub_control = ehci_brcm_hub_control;
> +
> +	/* initialize hcd */
> +	hcd = usb_create_hcd(&ehci_brcm_hc_driver, dev, dev_name(dev));
> +	if (!hcd)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, hcd);
> +	priv = hcd_to_ehci_priv(hcd);
> +
> +	priv->clk = devm_clk_get_optional(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		err = PTR_ERR(priv->clk);
> +		goto err_hcd;
> +	}
> +
> +	err = clk_prepare_enable(priv->clk);

But clk was optional, will this break?

> +	if (err)
> +		goto err_hcd;
> +
> +	hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res_mem);
> +	if (IS_ERR(hcd->regs)) {
> +		err = PTR_ERR(hcd->regs);
> +		goto err_clk;
> +	}
> +	hcd->rsrc_start = res_mem->start;
> +	hcd->rsrc_len = resource_size(res_mem);
> +	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
> +	if (err)
> +		goto err_clk;
> +
> +	device_wakeup_enable(hcd->self.controller);
> +	device_enable_async_suspend(hcd->self.controller);
> +	platform_set_drvdata(pdev, hcd);

Shouldn't that be set before you register the hcd?

> +
> +	return 0;
> +
> +err_clk:
> +	clk_disable_unprepare(priv->clk);
> +err_hcd:
> +	usb_put_hcd(hcd);
> +
> +	return err;
> +}
> +
> +static int ehci_brcm_remove(struct platform_device *dev)
> +{
> +	struct usb_hcd *hcd = platform_get_drvdata(dev);
> +	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
> +
> +	usb_remove_hcd(hcd);
> +	clk_disable_unprepare(priv->clk);
> +	usb_put_hcd(hcd);
> +	return 0;
> +}
> +
> +static int __maybe_unused ehci_brcm_suspend(struct device *dev)
> +{
> +	int ret;
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
> +	bool do_wakeup = device_may_wakeup(dev);
> +
> +	ret = ehci_suspend(hcd, do_wakeup);
> +	if (ret)
> +		return ret;
> +	clk_disable_unprepare(priv->clk);
> +	return 0;
> +}
> +
> +static int __maybe_unused ehci_brcm_resume(struct device *dev)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(dev);
> +	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> +	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
> +	int err;
> +
> +	err = clk_prepare_enable(priv->clk);
> +	if (err)
> +		return err;
> +	/*
> +	 * SWLINUX-1705: Avoid OUT packet underflows during high memory
> +	 *   bus usage
> +	 * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00
> +	 * @ 0x90
> +	 */
> +	ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> +	ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
> +
> +	ehci_resume(hcd, false);
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ehci_brcm_pm_ops, ehci_brcm_suspend,
> +		ehci_brcm_resume);
> +
> +static const struct of_device_id brcm_ehci_of_match[] = {
> +	{ .compatible = "brcm,ehci-brcm-v2", },
> +	{ .compatible = "brcm,bcm7445-ehci", },
> +	{}
> +};
> +
> +static struct platform_driver ehci_brcm_driver = {
> +	.probe		= ehci_brcm_probe,
> +	.remove		= ehci_brcm_remove,
> +	.shutdown	= usb_hcd_platform_shutdown,
> +	.driver		= {
> +		.name	= "ehci-brcm",
> +		.pm	= &ehci_brcm_pm_ops,
> +		.of_match_table = brcm_ehci_of_match,
> +	}
> +};
> +
> +static int __init ehci_brcm_init(void)
> +{
> +	if (usb_disabled())
> +		return -ENODEV;

You check this here, so why are you also checking it in the probe
function?

thanks,

greg k-h

Re: [PATCH v6 3/4] usb: ehci: Add new EHCI driver for Broadcom STB SoC's

[Alan Cooper]

On Tue, May 5, 2020 at 7:00 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 30, 2020 at 07:12:57AM -0400, Al Cooper wrote:
> > Add a new EHCI driver for Broadcom STB SoC's. A new EHCI driver
> > was created instead of adding support to the existing ehci platform
> > driver because of the code required to workaround bugs in the EHCI
> > controller.
> >
> > Signed-off-by: Al Cooper <alcooperx@gmail.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > ---
> >  drivers/usb/host/ehci-brcm.c | 290 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 290 insertions(+)
> >  create mode 100644 drivers/usb/host/ehci-brcm.c
>
> I need an ack from the EHCI maintainer to agree that this needs a whole
> new driver file...
>
> >
> > diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
> > new file mode 100644
> > index 000000000000..381bed5fdab0
> > --- /dev/null
> > +++ b/drivers/usb/host/ehci-brcm.c
> > @@ -0,0 +1,290 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2020, Broadcom */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/hcd.h>
> > +#include <linux/iopoll.h>
> > +
> > +#include "ehci.h"
> > +
> > +#define hcd_to_ehci_priv(h) ((struct brcm_priv *)hcd_to_ehci(h)->priv)
> > +
> > +struct brcm_priv {
> > +     struct clk *clk;
> > +};
> > +
> > +static const char brcm_hcd_name[] = "ehci-brcm";
>
> You don't use this anywhere?  Are you sure this didn't cause compile
> warnings?

I'll remove it. I'm not getting a warning because it's a "static
const" which doesn't give unused warnings.

>
> > +
> > +static int (*org_hub_control)(struct usb_hcd *hcd,
> > +                     u16 typeReq, u16 wValue, u16 wIndex,
> > +                     char *buf, u16 wLength);
>
> So you only support one device per system?  That feels bad...

When this driver was originally written, the ehci_hub_control()
function was a static function in ehci-hub.c and couldn't be called
directly. Instead, the function pointer was taken out of  "struct
hc_driver" and since it couldn't change for multiple devices, only one
pointer was needed. The ehci_hub_control function is now global so it
can be called directly and this can be removed. It's nice to get rid
of this hack, thanks.

>
>
> > +
> > +/*
> > + * ehci_brcm_wait_for_sof
> > + * Wait for start of next microframe, then wait extra delay microseconds
> > + */
> > +static inline void ehci_brcm_wait_for_sof(struct ehci_hcd *ehci, u32 delay)
> > +{
> > +     u32 frame_idx = ehci_readl(ehci, &ehci->regs->frame_index);
> > +     u32 val;
> > +     int res;
> > +
> > +     /* Wait for next microframe (every 125 usecs) */
> > +     res = readl_relaxed_poll_timeout(&ehci->regs->frame_index, val,
> > +                                      val != frame_idx, 1, 130);
> > +     if (res)
> > +             dev_err(ehci_to_hcd(ehci)->self.controller,
> > +                     "Error waiting for SOF\n");
> > +     udelay(delay);
> > +}
> > +
> > +/*
> > + * ehci_brcm_hub_control
> > + * Intercept echi-hcd request to complete RESUME and align it to the start
> > + * of the next microframe.
> > + * If RESUME is complete too late in the microframe, host controller
> > + * detects babble on suspended port and resets the port afterwards.
> > + * This s/w workaround allows to avoid this problem.
> > + * See SWLINUX-1909 for more details
> > + */
> > +static int ehci_brcm_hub_control(
> > +     struct usb_hcd  *hcd,
> > +     u16             typeReq,
> > +     u16             wValue,
> > +     u16             wIndex,
> > +     char            *buf,
> > +     u16             wLength)
> > +{
> > +     struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > +     int             ports = HCS_N_PORTS(ehci->hcs_params);
> > +     u32 __iomem     *status_reg = &ehci->regs->port_status[
> > +                             (wIndex & 0xff) - 1];
>
> Horrid line-wrapping, put this assignment below so it can be read.
>
> And wIndex is little endian?  Or native?

native

>
> > +     unsigned long flags;
> > +     int retval, irq_disabled = 0;
> > +
> > +     /*
> > +      * RESUME is cleared when GetPortStatus() is called 20ms after start
> > +      * of RESUME
> > +      */
> > +     if ((typeReq == GetPortStatus) &&
> > +         (wIndex && wIndex <= ports) &&
> > +         ehci->reset_done[wIndex-1] &&
> > +         time_after_eq(jiffies, ehci->reset_done[wIndex-1]) &&
> > +         (ehci_readl(ehci, status_reg) & PORT_RESUME)) {
> > +
> > +             /*
> > +              * to make sure we are not interrupted until RESUME bit
> > +              * is cleared, disable interrupts on current CPU
> > +              */
> > +             ehci_dbg(ehci, "SOF alignment workaround\n");
> > +             irq_disabled = 1;
> > +             local_irq_save(flags);
> > +             ehci_brcm_wait_for_sof(ehci, 5);
> > +     }
> > +     retval = (*org_hub_control)(hcd, typeReq, wValue, wIndex, buf, wLength);
>
> But this might not be set, did you just crash?
>
> If it is always set, then why does it need to be a function pointer at
> all?

Explained above (and removed).

>
> > +     if (irq_disabled)
> > +             local_irq_restore(flags);
> > +     return retval;
> > +}
> > +
> > +static int ehci_brcm_reset(struct usb_hcd *hcd)
> > +{
> > +     struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > +
> > +     ehci->big_endian_mmio = 1;
> > +
> > +     ehci->caps = (struct ehci_caps *) hcd->regs;
> > +     ehci->regs = (struct ehci_regs *) (hcd->regs +
>
> coding style, did you run this through checkpatch.pl?

Checkpatch did not complain, but I'll clean it up.

>
> > +             HC_LENGTH(ehci, ehci_readl(ehci, &ehci->caps->hc_capbase)));
> > +
> > +     /* This fixes the lockup during reboot due to prior interrupts */
> > +     ehci_writel(ehci, CMD_RESET, &ehci->regs->command);
> > +     mdelay(10);
> > +
> > +     /*
> > +      * SWLINUX-1705: Avoid OUT packet underflows during high memory
> > +      *   bus usage
> > +      * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
> > +      */
> > +     ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> > +     ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
> > +
> > +     return ehci_setup(hcd);
> > +}
> > +
> > +static struct hc_driver __read_mostly ehci_brcm_hc_driver;
> > +
> > +static const struct ehci_driver_overrides brcm_overrides __initconst = {
> > +
>
> No blank line.

Fixed.

>
> > +     .reset = ehci_brcm_reset,
> > +     .extra_priv_size = sizeof(struct brcm_priv),
> > +};
> > +
> > +static int ehci_brcm_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct resource *res_mem;
> > +     struct brcm_priv *priv;
> > +     struct usb_hcd *hcd;
> > +     int irq;
> > +     int err;
> > +
> > +     if (usb_disabled())
> > +             return -ENODEV;
> > +
> > +     err = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> > +     if (err)
> > +             return err;
> > +
> > +     irq = platform_get_irq(pdev, 0);
> > +     if (irq < 0)
>
> "<=" right?

The example in the comment above platform_get_irq in platform.c says
"if (irq < 0)"

>
> > +             return irq;
> > +
> > +     /* Hook the hub control routine to work around a bug */
>
> What bug?  This feels wrong.

The bug is explained in a comment above the ehci_brcm_hub_control()
routine as follows:
/*
 * ehci_brcm_hub_control
 * Intercept echi-hcd request to complete RESUME and align it to the start
 * of the next microframe.
 * If RESUME is complete too late in the microframe, host controller
 * detects babble on suspended port and resets the port afterwards.
 * This s/w workaround allows to avoid this problem.
 * See SWLINUX-1909 for more details
 */
I'll remove the internal bug tracking reference from the comment.

>
> > +     if (!org_hub_control)
> > +             org_hub_control = ehci_brcm_hc_driver.hub_control;
> > +     ehci_brcm_hc_driver.hub_control = ehci_brcm_hub_control;
> > +
> > +     /* initialize hcd */
> > +     hcd = usb_create_hcd(&ehci_brcm_hc_driver, dev, dev_name(dev));
> > +     if (!hcd)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, hcd);
> > +     priv = hcd_to_ehci_priv(hcd);
> > +
> > +     priv->clk = devm_clk_get_optional(dev, NULL);
> > +     if (IS_ERR(priv->clk)) {
> > +             err = PTR_ERR(priv->clk);
> > +             goto err_hcd;
> > +     }
> > +
> > +     err = clk_prepare_enable(priv->clk);
>
> But clk was optional, will this break?

devm_clk_get_optional() will set clk to NULL if it's not found and all
the clk_... routines handle a NULL clk by just returning.

>
> > +     if (err)
> > +             goto err_hcd;
> > +
> > +     hcd->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res_mem);
> > +     if (IS_ERR(hcd->regs)) {
> > +             err = PTR_ERR(hcd->regs);
> > +             goto err_clk;
> > +     }
> > +     hcd->rsrc_start = res_mem->start;
> > +     hcd->rsrc_len = resource_size(res_mem);
> > +     err = usb_add_hcd(hcd, irq, IRQF_SHARED);
> > +     if (err)
> > +             goto err_clk;
> > +
> > +     device_wakeup_enable(hcd->self.controller);
> > +     device_enable_async_suspend(hcd->self.controller);
> > +     platform_set_drvdata(pdev, hcd);
>
> Shouldn't that be set before you register the hcd?

Since usb_add_hcd(hcd,...) takes the hcd pointer and not a device
pointer it shouldn't require platform_set_drvdata() to be called
first.
Looking at the code made me notice that platform_set_drvdata() is also
called at the beginning of the function and it's not needed twice so
I'll remove this second call. This driver used the ehci and ohci
platform drivers as a starting point and they also do this call twice
in probe.

>
> > +
> > +     return 0;
> > +
> > +err_clk:
> > +     clk_disable_unprepare(priv->clk);
> > +err_hcd:
> > +     usb_put_hcd(hcd);
> > +
> > +     return err;
> > +}
> > +
> > +static int ehci_brcm_remove(struct platform_device *dev)
> > +{
> > +     struct usb_hcd *hcd = platform_get_drvdata(dev);
> > +     struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
> > +
> > +     usb_remove_hcd(hcd);
> > +     clk_disable_unprepare(priv->clk);
> > +     usb_put_hcd(hcd);
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused ehci_brcm_suspend(struct device *dev)
> > +{
> > +     int ret;
> > +     struct usb_hcd *hcd = dev_get_drvdata(dev);
> > +     struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
> > +     bool do_wakeup = device_may_wakeup(dev);
> > +
> > +     ret = ehci_suspend(hcd, do_wakeup);
> > +     if (ret)
> > +             return ret;
> > +     clk_disable_unprepare(priv->clk);
> > +     return 0;
> > +}
> > +
> > +static int __maybe_unused ehci_brcm_resume(struct device *dev)
> > +{
> > +     struct usb_hcd *hcd = dev_get_drvdata(dev);
> > +     struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> > +     struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
> > +     int err;
> > +
> > +     err = clk_prepare_enable(priv->clk);
> > +     if (err)
> > +             return err;
> > +     /*
> > +      * SWLINUX-1705: Avoid OUT packet underflows during high memory
> > +      *   bus usage
> > +      * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00
> > +      * @ 0x90
> > +      */
> > +     ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
> > +     ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
> > +
> > +     ehci_resume(hcd, false);
> > +
> > +     pm_runtime_disable(dev);
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_enable(dev);
> > +
> > +     return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(ehci_brcm_pm_ops, ehci_brcm_suspend,
> > +             ehci_brcm_resume);
> > +
> > +static const struct of_device_id brcm_ehci_of_match[] = {
> > +     { .compatible = "brcm,ehci-brcm-v2", },
> > +     { .compatible = "brcm,bcm7445-ehci", },
> > +     {}
> > +};
> > +
> > +static struct platform_driver ehci_brcm_driver = {
> > +     .probe          = ehci_brcm_probe,
> > +     .remove         = ehci_brcm_remove,
> > +     .shutdown       = usb_hcd_platform_shutdown,
> > +     .driver         = {
> > +             .name   = "ehci-brcm",
> > +             .pm     = &ehci_brcm_pm_ops,
> > +             .of_match_table = brcm_ehci_of_match,
> > +     }
> > +};
> > +
> > +static int __init ehci_brcm_init(void)
> > +{
> > +     if (usb_disabled())
> > +             return -ENODEV;
>
> You check this here, so why are you also checking it in the probe
> function?

Good point. I'll remove the one in probe. This was another thing that
came from the ehci and ohci platform drivers.  About a third of the
drivers in this directory do this, probably because they started with
the platform drivers. Do you think it's worth me submitting a patch to
clean this up at some point?

>
> thanks,
>
> greg k-h

I'll wait for your response before sending a V7.
Thanks for the review.
Al

Re: [PATCH v6 3/4] usb: ehci: Add new EHCI driver for Broadcom STB SoC's

[Andy Shevchenko]

On Wed, May 6, 2020 at 11:23 PM Alan Cooper <alcooperx@gmail.com> wrote:
> On Tue, May 5, 2020 at 7:00 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, Apr 30, 2020 at 07:12:57AM -0400, Al Cooper wrote:

...

> > > +     /* Hook the hub control routine to work around a bug */
> >
> > What bug?  This feels wrong.
>
> The bug is explained in a comment above the ehci_brcm_hub_control()
> routine as follows:
> /*
>  * ehci_brcm_hub_control
>  * Intercept echi-hcd request to complete RESUME and align it to the start
>  * of the next microframe.
>  * If RESUME is complete too late in the microframe, host controller
>  * detects babble on suspended port and resets the port afterwards.
>  * This s/w workaround allows to avoid this problem.
>  * See SWLINUX-1909 for more details
>  */
> I'll remove the internal bug tracking reference from the comment.

I guess you may leave the internal bug reference. I can tell from my
experience that's hard to understand what was going on in the driver
in years perspective. It will help whoever in this company have a
chance to look after the driver.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/4] usb: ehci: Add new EHCI driver for Broadcom STB SoC's

[Greg Kroah-Hartman]

On Thu, May 07, 2020 at 12:01:16AM +0300, Andy Shevchenko wrote:
> On Wed, May 6, 2020 at 11:23 PM Alan Cooper <alcooperx@gmail.com> wrote:
> > On Tue, May 5, 2020 at 7:00 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Thu, Apr 30, 2020 at 07:12:57AM -0400, Al Cooper wrote:
> 
> ...
> 
> > > > +     /* Hook the hub control routine to work around a bug */
> > >
> > > What bug?  This feels wrong.
> >
> > The bug is explained in a comment above the ehci_brcm_hub_control()
> > routine as follows:
> > /*
> >  * ehci_brcm_hub_control
> >  * Intercept echi-hcd request to complete RESUME and align it to the start
> >  * of the next microframe.
> >  * If RESUME is complete too late in the microframe, host controller
> >  * detects babble on suspended port and resets the port afterwards.
> >  * This s/w workaround allows to avoid this problem.
> >  * See SWLINUX-1909 for more details
> >  */
> > I'll remove the internal bug tracking reference from the comment.
> 
> I guess you may leave the internal bug reference. I can tell from my
> experience that's hard to understand what was going on in the driver
> in years perspective. It will help whoever in this company have a
> chance to look after the driver.

"internal bug references" mean nothing to the 99% of the people that can
see this.  Document the heck out of what this is instead please.

greg k-h

Re: [PATCH v6 3/4] usb: ehci: Add new EHCI driver for Broadcom STB SoC's

[Andy Shevchenko]

On Thu, May 7, 2020 at 9:41 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, May 07, 2020 at 12:01:16AM +0300, Andy Shevchenko wrote:
> > On Wed, May 6, 2020 at 11:23 PM Alan Cooper <alcooperx@gmail.com> wrote:
> > > On Tue, May 5, 2020 at 7:00 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:

> > > > > +     /* Hook the hub control routine to work around a bug */
> > > >
> > > > What bug?  This feels wrong.
> > >
> > > The bug is explained in a comment above the ehci_brcm_hub_control()
> > > routine as follows:
> > > /*
> > >  * ehci_brcm_hub_control
> > >  * Intercept echi-hcd request to complete RESUME and align it to the start
> > >  * of the next microframe.
> > >  * If RESUME is complete too late in the microframe, host controller
> > >  * detects babble on suspended port and resets the port afterwards.
> > >  * This s/w workaround allows to avoid this problem.
> > >  * See SWLINUX-1909 for more details
> > >  */
> > > I'll remove the internal bug tracking reference from the comment.
> >
> > I guess you may leave the internal bug reference. I can tell from my
> > experience that's hard to understand what was going on in the driver
> > in years perspective. It will help whoever in this company have a
> > chance to look after the driver.
>
> "internal bug references" mean nothing to the 99% of the people that can
> see this.  Document the heck out of what this is instead please.

As far as I understand the bug is being described in the same file
somewhere else, that's why internal reference *on top of explanation*
would be valid.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/4] usb: ehci: Add new EHCI driver for Broadcom STB SoC's

[Greg Kroah-Hartman]

On Wed, May 06, 2020 at 04:23:01PM -0400, Alan Cooper wrote:
> > > +     irq = platform_get_irq(pdev, 0);
> > > +     if (irq < 0)
> >
> > "<=" right?
> 
> The example in the comment above platform_get_irq in platform.c says
> "if (irq < 0)"

There is work to fix that up on the mailing lists, 0 is not a valid irq :)

> > > +static int __init ehci_brcm_init(void)
> > > +{
> > > +     if (usb_disabled())
> > > +             return -ENODEV;
> >
> > You check this here, so why are you also checking it in the probe
> > function?
> 
> Good point. I'll remove the one in probe. This was another thing that
> came from the ehci and ohci platform drivers.  About a third of the
> drivers in this directory do this, probably because they started with
> the platform drivers. Do you think it's worth me submitting a patch to
> clean this up at some point?

Yes please.

> 
> >
> > thanks,
> >
> > greg k-h
> 
> I'll wait for your response before sending a V7.

Never wait for someone who you have no idea how much email they get :)

thanks,

greg k-h

[PATCH v6 4/4] usb: host: Add ability to build new Broadcom STB USB drivers

[Al Cooper]

Add the build system changes needed to get the Broadcom STB XHCI,
EHCI and OHCI functionality working. The OHCI support does not
require anything unique to Broadcom so the standard ohci-platform
driver is being used. The link order for XHCI was changed in the
Makefile because of the way STB XHCI, EHCI and OHCI controllers
share a port which requires that the XHCI driver be initialized
first. Also update MAINTAINERS.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 MAINTAINERS               |  8 ++++++++
 drivers/usb/host/Kconfig  | 16 ++++++++++++++++
 drivers/usb/host/Makefile | 16 ++++++++++------
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 26f281d9f32a..6147ed78d212 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3481,6 +3481,14 @@ S:	Supported
 F:	Documentation/devicetree/bindings/i2c/brcm,brcmstb-i2c.yaml
 F:	drivers/i2c/busses/i2c-brcmstb.c
 
+BROADCOM BRCMSTB USB EHCI DRIVER
+M:	Al Cooper <alcooperx@gmail.com>
+L:	linux-usb@vger.kernel.org
+L:	bcm-kernel-feedback-list@broadcom.com
+S:	Maintained
+F:	Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml
+F:	drivers/usb/host/ehci-brcm.*
+
 BROADCOM BRCMSTB USB2 and USB3 PHY DRIVER
 M:	Al Cooper <alcooperx@gmail.com>
 L:	linux-kernel@vger.kernel.org
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 55bdfdf11e4c..7d58fd66e412 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -97,6 +97,22 @@ config USB_XHCI_TEGRA
 
 endif # USB_XHCI_HCD
 
+config USB_EHCI_BRCMSTB
+       tristate
+
+config USB_BRCMSTB
+	tristate "Broadcom STB USB support"
+	depends on (ARCH_BRCMSTB && PHY_BRCM_USB) || COMPILE_TEST
+	select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
+	select USB_EHCI_BRCMSTB if USB_EHCI_HCD
+	select USB_XHCI_PLATFORM if USB_XHCI_HCD
+	help
+	  Say Y to enable support for XHCI, EHCI and OHCI host controllers
+	  found in Broadcom STB SoC's.
+
+	  Disabling this will keep the controllers and corresponding
+	  PHYs powered down.
+
 config USB_EHCI_HCD
 	tristate "EHCI HCD (USB 2.0) support"
 	depends on HAS_DMA && HAS_IOMEM
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index b191361257cc..85fa6ace552a 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -37,6 +37,15 @@ endif
 
 obj-$(CONFIG_USB_PCI)	+= pci-quirks.o
 
+# NOTE: BRCMSTB systems require that xhci driver be linked before the
+# ehci/ohci drivers.
+obj-$(CONFIG_USB_XHCI_HCD)	+= xhci-hcd.o
+obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o
+obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
+obj-$(CONFIG_USB_XHCI_HISTB)	+= xhci-histb.o
+obj-$(CONFIG_USB_XHCI_MTK)	+= xhci-mtk.o
+obj-$(CONFIG_USB_XHCI_TEGRA)	+= xhci-tegra.o
+
 obj-$(CONFIG_USB_EHCI_HCD)	+= ehci-hcd.o
 obj-$(CONFIG_USB_EHCI_PCI)	+= ehci-pci.o
 obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)	+= ehci-platform.o
@@ -49,6 +58,7 @@ obj-$(CONFIG_USB_EHCI_HCD_STI)	+= ehci-st.o
 obj-$(CONFIG_USB_EHCI_EXYNOS)	+= ehci-exynos.o
 obj-$(CONFIG_USB_EHCI_HCD_AT91) += ehci-atmel.o
 obj-$(CONFIG_USB_EHCI_TEGRA)	+= ehci-tegra.o
+obj-$(CONFIG_USB_EHCI_BRCMSTB)	+= ehci-brcm.o
 
 obj-$(CONFIG_USB_OXU210HP_HCD)	+= oxu210hp-hcd.o
 obj-$(CONFIG_USB_ISP116X_HCD)	+= isp116x-hcd.o
@@ -69,12 +79,6 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)	+= ohci-da8xx.o
 
 obj-$(CONFIG_USB_UHCI_HCD)	+= uhci-hcd.o
 obj-$(CONFIG_USB_FHCI_HCD)	+= fhci.o
-obj-$(CONFIG_USB_XHCI_HCD)	+= xhci-hcd.o
-obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o
-obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
-obj-$(CONFIG_USB_XHCI_HISTB)	+= xhci-histb.o
-obj-$(CONFIG_USB_XHCI_MTK)	+= xhci-mtk.o
-obj-$(CONFIG_USB_XHCI_TEGRA)	+= xhci-tegra.o
 obj-$(CONFIG_USB_SL811_HCD)	+= sl811-hcd.o
 obj-$(CONFIG_USB_SL811_CS)	+= sl811_cs.o
 obj-$(CONFIG_USB_U132_HCD)	+= u132-hcd.o
-- 
2.17.1

Re: [PATCH v6 4/4] usb: host: Add ability to build new Broadcom STB USB drivers

[Greg Kroah-Hartman]

On Thu, Apr 30, 2020 at 07:12:58AM -0400, Al Cooper wrote:
> Add the build system changes needed to get the Broadcom STB XHCI,
> EHCI and OHCI functionality working. The OHCI support does not
> require anything unique to Broadcom so the standard ohci-platform
> driver is being used. The link order for XHCI was changed in the
> Makefile because of the way STB XHCI, EHCI and OHCI controllers
> share a port which requires that the XHCI driver be initialized
> first. Also update MAINTAINERS.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---
>  MAINTAINERS               |  8 ++++++++
>  drivers/usb/host/Kconfig  | 16 ++++++++++++++++
>  drivers/usb/host/Makefile | 16 ++++++++++------
>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 26f281d9f32a..6147ed78d212 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3481,6 +3481,14 @@ S:	Supported
>  F:	Documentation/devicetree/bindings/i2c/brcm,brcmstb-i2c.yaml
>  F:	drivers/i2c/busses/i2c-brcmstb.c
>  
> +BROADCOM BRCMSTB USB EHCI DRIVER
> +M:	Al Cooper <alcooperx@gmail.com>
> +L:	linux-usb@vger.kernel.org
> +L:	bcm-kernel-feedback-list@broadcom.com
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml
> +F:	drivers/usb/host/ehci-brcm.*
> +
>  BROADCOM BRCMSTB USB2 and USB3 PHY DRIVER
>  M:	Al Cooper <alcooperx@gmail.com>
>  L:	linux-kernel@vger.kernel.org
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 55bdfdf11e4c..7d58fd66e412 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -97,6 +97,22 @@ config USB_XHCI_TEGRA
>  
>  endif # USB_XHCI_HCD
>  
> +config USB_EHCI_BRCMSTB
> +       tristate
> +
> +config USB_BRCMSTB
> +	tristate "Broadcom STB USB support"
> +	depends on (ARCH_BRCMSTB && PHY_BRCM_USB) || COMPILE_TEST
> +	select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
> +	select USB_EHCI_BRCMSTB if USB_EHCI_HCD
> +	select USB_XHCI_PLATFORM if USB_XHCI_HCD
> +	help
> +	  Say Y to enable support for XHCI, EHCI and OHCI host controllers
> +	  found in Broadcom STB SoC's.
> +
> +	  Disabling this will keep the controllers and corresponding
> +	  PHYs powered down.

Whhat are the module names?

And why 2 config options here?

> +
>  config USB_EHCI_HCD
>  	tristate "EHCI HCD (USB 2.0) support"
>  	depends on HAS_DMA && HAS_IOMEM
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index b191361257cc..85fa6ace552a 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -37,6 +37,15 @@ endif
>  
>  obj-$(CONFIG_USB_PCI)	+= pci-quirks.o
>  
> +# NOTE: BRCMSTB systems require that xhci driver be linked before the
> +# ehci/ohci drivers.

Why?  Please do this as a separate change before your makefile changes.

And what else will this break?  We have relied on this link order for a
very long time, changing it could cause issues...

I don't see how your driver needs this, please explain in great detail.

thanks,

greg k-h

Re: [PATCH v6 4/4] usb: host: Add ability to build new Broadcom STB USB drivers

[Alan Cooper]

On Tue, May 5, 2020 at 6:54 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 30, 2020 at 07:12:58AM -0400, Al Cooper wrote:
> > Add the build system changes needed to get the Broadcom STB XHCI,
> > EHCI and OHCI functionality working. The OHCI support does not
> > require anything unique to Broadcom so the standard ohci-platform
> > driver is being used. The link order for XHCI was changed in the
> > Makefile because of the way STB XHCI, EHCI and OHCI controllers
> > share a port which requires that the XHCI driver be initialized
> > first. Also update MAINTAINERS.
> >
> > Signed-off-by: Al Cooper <alcooperx@gmail.com>
> > ---
> >  MAINTAINERS               |  8 ++++++++
> >  drivers/usb/host/Kconfig  | 16 ++++++++++++++++
> >  drivers/usb/host/Makefile | 16 ++++++++++------
> >  3 files changed, 34 insertions(+), 6 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 26f281d9f32a..6147ed78d212 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3481,6 +3481,14 @@ S:     Supported
> >  F:   Documentation/devicetree/bindings/i2c/brcm,brcmstb-i2c.yaml
> >  F:   drivers/i2c/busses/i2c-brcmstb.c
> >
> > +BROADCOM BRCMSTB USB EHCI DRIVER
> > +M:   Al Cooper <alcooperx@gmail.com>
> > +L:   linux-usb@vger.kernel.org
> > +L:   bcm-kernel-feedback-list@broadcom.com
> > +S:   Maintained
> > +F:   Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.yaml
> > +F:   drivers/usb/host/ehci-brcm.*
> > +
> >  BROADCOM BRCMSTB USB2 and USB3 PHY DRIVER
> >  M:   Al Cooper <alcooperx@gmail.com>
> >  L:   linux-kernel@vger.kernel.org
> > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > index 55bdfdf11e4c..7d58fd66e412 100644
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -97,6 +97,22 @@ config USB_XHCI_TEGRA
> >
> >  endif # USB_XHCI_HCD
> >
> > +config USB_EHCI_BRCMSTB
> > +       tristate
> > +
> > +config USB_BRCMSTB
> > +     tristate "Broadcom STB USB support"
> > +     depends on (ARCH_BRCMSTB && PHY_BRCM_USB) || COMPILE_TEST
> > +     select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
> > +     select USB_EHCI_BRCMSTB if USB_EHCI_HCD
> > +     select USB_XHCI_PLATFORM if USB_XHCI_HCD
> > +     help
> > +       Say Y to enable support for XHCI, EHCI and OHCI host controllers
> > +       found in Broadcom STB SoC's.
> > +
> > +       Disabling this will keep the controllers and corresponding
> > +       PHYs powered down.
>
> Whhat are the module names?

I'll add the module names.

>
> And why 2 config options here?

I'd like the option to build an XHCI only system by not enabling
USB_EHCI_HCD and USB_OHCI_HCD.

>
> > +
> >  config USB_EHCI_HCD
> >       tristate "EHCI HCD (USB 2.0) support"
> >       depends on HAS_DMA && HAS_IOMEM
> > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> > index b191361257cc..85fa6ace552a 100644
> > --- a/drivers/usb/host/Makefile
> > +++ b/drivers/usb/host/Makefile
> > @@ -37,6 +37,15 @@ endif
> >
> >  obj-$(CONFIG_USB_PCI)        += pci-quirks.o
> >
> > +# NOTE: BRCMSTB systems require that xhci driver be linked before the
> > +# ehci/ohci drivers.
>
> Why?  Please do this as a separate change before your makefile changes.

Okay

>
> And what else will this break?  We have relied on this link order for a
> very long time, changing it could cause issues...
>
> I don't see how your driver needs this, please explain in great detail.

For this explaination, I'm going to call our EHCI and OHCI controllers EOHCI.
We have some SoC's that have an XHCI controller and an EOHCI
controller on the same  port, where the XHCI controller handles the
3.0 devices and the EOHCI handles the <= 2.0 devices. If the EOHCI
controller comes up before the XHCI controller, any 3.0 devices
installed on the port will be seen as a 2.0 device. Once the 3.0
controller comes up, most 3.0 USB devices will switch to 3.0, but this
seems to cause problems on some USB devices.
In the past, this wasn't a problem because we had custom XHCI, EHCI
and OHCI drivers that I could order any way necessary. Now that the
standard platform XHCI and OHCI drivers are being used this becomes a
problem.

Thanks for the review
Al

>
> thanks,
>
> greg k-h