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