| Message ID | 20180607103845.13515-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 7, 2018 at 1:38 PM, Hans de Goede <hdegoede@redhat.com> wrote: > On some Bay Trail (BYT) systems the firmware does not enable the > ULPI Refclk. > > This commit adds a helper which checks and if necessary enabled the Refclk > and calls this helper for BYT machines. > +static void dwc3_pci_enable_ulpi_refclock(struct pci_dev *pci) > +{ > + void __iomem *reg; > + struct resource res; > + struct device *dev = &pci->dev; > + u32 value; > + > + res.start = pci_resource_start(pci, 1); > + res.end = pci_resource_end(pci, 1); > + res.name = "dwc_usb3_bar1"; > + res.flags = IORESOURCE_MEM; > + > + reg = devm_ioremap_resource(dev, &res); > + if (IS_ERR(reg)) { > + dev_err(dev, "cannot check GP_RWREG1 to assert ulpi refclock\n"); > + return; > + } I'm not sure I understand what's wrong with simple pci_iomap() & Co (perhaps pcim_iomap() / pcim_iomap_regions() and others) > + > + value = readl(reg + GP_RWREG1); > + if (!(value & GP_RWREG1_ULPI_REFCLK_DISABLE)) > + return; /* ULPI refclk already enabled */ > + > + dev_warn(dev, "ULPI refclock is disabled from the BIOS, enabling it\n"); > + value &= ~GP_RWREG1_ULPI_REFCLK_DISABLE; > + writel(value, reg + GP_RWREG1); > + msleep(100); This has to be explained. > +}
Hi, On 07-06-18 15:30, Andy Shevchenko wrote: > On Thu, Jun 7, 2018 at 1:38 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> On some Bay Trail (BYT) systems the firmware does not enable the >> ULPI Refclk. >> >> This commit adds a helper which checks and if necessary enabled the Refclk >> and calls this helper for BYT machines. > > >> +static void dwc3_pci_enable_ulpi_refclock(struct pci_dev *pci) >> +{ >> + void __iomem *reg; >> + struct resource res; >> + struct device *dev = &pci->dev; >> + u32 value; >> + > >> + res.start = pci_resource_start(pci, 1); >> + res.end = pci_resource_end(pci, 1); >> + res.name = "dwc_usb3_bar1"; >> + res.flags = IORESOURCE_MEM; >> + >> + reg = devm_ioremap_resource(dev, &res); >> + if (IS_ERR(reg)) { >> + dev_err(dev, "cannot check GP_RWREG1 to assert ulpi refclock\n"); >> + return; >> + } > > I'm not sure I understand what's wrong with simple > pci_iomap() & Co (perhaps pcim_iomap() / pcim_iomap_regions() and others) Good point, I took this from the crufty Android X86 patched which Intel maintains here: https://github.com/01org/ProductionKernelQuilts And did not realize I could simplify this, will fix for v2. >> + >> + value = readl(reg + GP_RWREG1); >> + if (!(value & GP_RWREG1_ULPI_REFCLK_DISABLE)) >> + return; /* ULPI refclk already enabled */ >> + >> + dev_warn(dev, "ULPI refclock is disabled from the BIOS, enabling it\n"); >> + value &= ~GP_RWREG1_ULPI_REFCLK_DISABLE; >> + writel(value, reg + GP_RWREG1); > >> + msleep(100); > > This has to be explained. Erm, this comes 1:1 from Intels Android X86 patches I've no idea why it is there, I believe it is better to leave this uncommented then making something up. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello! On 06/07/2018 01:38 PM, Hans de Goede wrote: > On some Bay Trail (BYT) systems the firmware does not enable the > ULPI Refclk. > > This commit adds a helper which checks and if necessary enabled the Refclk > and calls this helper for BYT machines. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/usb/dwc3/dwc3-pci.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c > index 34b84d3bc7cf..65bc110388f3 100644 > --- a/drivers/usb/dwc3/dwc3-pci.c > +++ b/drivers/usb/dwc3/dwc3-pci.c [...] > @@ -78,6 +81,34 @@ static struct gpiod_lookup_table platform_bytcr_gpios = { > }, > }; > > +static void dwc3_pci_enable_ulpi_refclock(struct pci_dev *pci) > +{ > + void __iomem *reg; > + struct resource res; > + struct device *dev = &pci->dev; > + u32 value; > + > + res.start = pci_resource_start(pci, 1); > + res.end = pci_resource_end(pci, 1); > + res.name = "dwc_usb3_bar1"; > + res.flags = IORESOURCE_MEM; > + > + reg = devm_ioremap_resource(dev, &res); > + if (IS_ERR(reg)) { > + dev_err(dev, "cannot check GP_RWREG1 to assert ulpi refclock\n"); Eh? BTW, devm_ioremap_resource() prints the error cause already... > + return; > + } > + > + value = readl(reg + GP_RWREG1); > + if (!(value & GP_RWREG1_ULPI_REFCLK_DISABLE)) I guess that dev_err() belongs here... > + return; /* ULPI refclk already enabled */ > + > + dev_warn(dev, "ULPI refclock is disabled from the BIOS, enabling it\n"); > + value &= ~GP_RWREG1_ULPI_REFCLK_DISABLE; > + writel(value, reg + GP_RWREG1); > + msleep(100); > +} > + > static int dwc3_pci_quirks(struct dwc3_pci *dwc) > { > struct platform_device *dwc3 = dwc->dwc3; [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 07-06-18 18:43, Sergei Shtylyov wrote: > Hello! > > On 06/07/2018 01:38 PM, Hans de Goede wrote: > >> On some Bay Trail (BYT) systems the firmware does not enable the >> ULPI Refclk. >> >> This commit adds a helper which checks and if necessary enabled the Refclk >> and calls this helper for BYT machines. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/usb/dwc3/dwc3-pci.c | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c >> index 34b84d3bc7cf..65bc110388f3 100644 >> --- a/drivers/usb/dwc3/dwc3-pci.c >> +++ b/drivers/usb/dwc3/dwc3-pci.c > [...] >> @@ -78,6 +81,34 @@ static struct gpiod_lookup_table platform_bytcr_gpios = { >> }, >> }; >> >> +static void dwc3_pci_enable_ulpi_refclock(struct pci_dev *pci) >> +{ >> + void __iomem *reg; >> + struct resource res; >> + struct device *dev = &pci->dev; >> + u32 value; >> + >> + res.start = pci_resource_start(pci, 1); >> + res.end = pci_resource_end(pci, 1); >> + res.name = "dwc_usb3_bar1"; >> + res.flags = IORESOURCE_MEM; >> + >> + reg = devm_ioremap_resource(dev, &res); >> + if (IS_ERR(reg)) { >> + dev_err(dev, "cannot check GP_RWREG1 to assert ulpi refclock\n"); > > Eh? > BTW, devm_ioremap_resource() prints the error cause already... > >> + return; >> + } >> + >> + value = readl(reg + GP_RWREG1); >> + if (!(value & GP_RWREG1_ULPI_REFCLK_DISABLE)) > > I guess that dev_err() belongs here... Nope the error is that GP_RWREG1 cannot be read because the remap fails. I agree that this is a bit silly and I will fix for v2, but returning here without printing anything is correct. Regards, Hans > >> + return; /* ULPI refclk already enabled */ >> + >> + dev_warn(dev, "ULPI refclock is disabled from the BIOS, enabling it\n"); >> + value &= ~GP_RWREG1_ULPI_REFCLK_DISABLE; >> + writel(value, reg + GP_RWREG1); >> + msleep(100); >> +} >> + >> static int dwc3_pci_quirks(struct dwc3_pci *dwc) >> { >> struct platform_device *dwc3 = dwc->dwc3; > [...] > > MBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 7, 2018 at 4:42 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>> + msleep(100); >> >> >> This has to be explained. > > > Erm, this comes 1:1 from Intels Android X86 patches I've no > idea why it is there, I believe it is better to leave this > uncommented then making something up. The above would be a good candidate /* This comes from the Intel Android x86 tree w/o any explanation */
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c index 34b84d3bc7cf..65bc110388f3 100644 --- a/drivers/usb/dwc3/dwc3-pci.c +++ b/drivers/usb/dwc3/dwc3-pci.c @@ -42,6 +42,9 @@ #define PCI_INTEL_BXT_STATE_D0 0 #define PCI_INTEL_BXT_STATE_D3 3 +#define GP_RWREG1 0xa0 +#define GP_RWREG1_ULPI_REFCLK_DISABLE (1 << 17) + /** * struct dwc3_pci - Driver private structure * @dwc3: child dwc3 platform_device @@ -78,6 +81,34 @@ static struct gpiod_lookup_table platform_bytcr_gpios = { }, }; +static void dwc3_pci_enable_ulpi_refclock(struct pci_dev *pci) +{ + void __iomem *reg; + struct resource res; + struct device *dev = &pci->dev; + u32 value; + + res.start = pci_resource_start(pci, 1); + res.end = pci_resource_end(pci, 1); + res.name = "dwc_usb3_bar1"; + res.flags = IORESOURCE_MEM; + + reg = devm_ioremap_resource(dev, &res); + if (IS_ERR(reg)) { + dev_err(dev, "cannot check GP_RWREG1 to assert ulpi refclock\n"); + return; + } + + value = readl(reg + GP_RWREG1); + if (!(value & GP_RWREG1_ULPI_REFCLK_DISABLE)) + return; /* ULPI refclk already enabled */ + + dev_warn(dev, "ULPI refclock is disabled from the BIOS, enabling it\n"); + value &= ~GP_RWREG1_ULPI_REFCLK_DISABLE; + writel(value, reg + GP_RWREG1); + msleep(100); +} + static int dwc3_pci_quirks(struct dwc3_pci *dwc) { struct platform_device *dwc3 = dwc->dwc3; @@ -134,6 +165,9 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc) struct gpio_desc *gpio; const char *vendor; + /* On BYT the FW does not always enable the refclock */ + dwc3_pci_enable_ulpi_refclock(pdev); + ret = devm_acpi_dev_add_driver_gpios(&pdev->dev, acpi_dwc3_byt_gpios); if (ret)
On some Bay Trail (BYT) systems the firmware does not enable the ULPI Refclk. This commit adds a helper which checks and if necessary enabled the Refclk and calls this helper for BYT machines. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/usb/dwc3/dwc3-pci.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)