diff mbox series

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

Message ID 20200430111258.6091-4-alcooperx@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add XHCI, EHCI and OHCI support for Broadcom STB SoS's | expand

Commit Message

Alan Cooper April 30, 2020, 11:12 a.m. UTC
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

Comments

Greg Kroah-Hartman May 5, 2020, 11 a.m. UTC | #1
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
Alan Cooper May 6, 2020, 8:23 p.m. UTC | #2
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
Andy Shevchenko May 6, 2020, 9:01 p.m. UTC | #3
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.
Greg Kroah-Hartman May 7, 2020, 6:41 a.m. UTC | #4
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
Greg Kroah-Hartman May 7, 2020, 6:43 a.m. UTC | #5
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
Andy Shevchenko May 7, 2020, 9:06 a.m. UTC | #6
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.
diff mbox series

Patch

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");