| Message ID | 20180831142026.49401-5-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: A few more improvements for Intel CHT | expand |
On Fri, Aug 31, 2018 at 5:21 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Only create thre Intel role mux device if the platform has > USB peripheral controller PCI device. > > While here, enable the role mux on Apollo Lake platforms. > +static int xhci_pci_board_has_udc(void) > +{ > + struct pci_dev *udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL); > + > + if (udc) { > + pci_dev_put(udc); > + return true; > + } > + return false; > +} Looks like a code duplication with patch 3. Does it make sense to have this in some header (usb.h?)?
On Mon, Sep 03, 2018 at 09:01:47AM +0300, Andy Shevchenko wrote: > On Fri, Aug 31, 2018 at 5:21 PM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > Only create thre Intel role mux device if the platform has > > USB peripheral controller PCI device. > > > > While here, enable the role mux on Apollo Lake platforms. > > > +static int xhci_pci_board_has_udc(void) > > +{ > > + struct pci_dev *udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL); > > + > > + if (udc) { > > + pci_dev_put(udc); > > + return true; > > + } > > + return false; > > +} > > Looks like a code duplication with patch 3. Does it make sense to have > this in some header (usb.h?)? I don't know. The check is very PCI specific. I'm not sure ush.h would be appropriate place for it. I don't know where should it go? Right now the check is only needed on Intel CHT (in both patches), so I figured that it's better wait for an other user before introducing a helper for it. Would that make sense? Thanks,
Hi, On 31-08-18 16:20, Heikki Krogerus wrote: > Only create thre Intel role mux device if the platform has > USB peripheral controller PCI device. > > While here, enable the role mux on Apollo Lake platforms. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Mathias Nyman <mathias.nyman@linux.intel.com> > --- > drivers/usb/host/xhci-pci.c | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index 6372edf339d9..ea651c2adcfd 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -75,6 +75,17 @@ static int xhci_pci_reinit(struct xhci_hcd *xhci, struct pci_dev *pdev) > return 0; > } > > +static int xhci_pci_board_has_udc(void) > +{ > + struct pci_dev *udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL); > + > + if (udc) { > + pci_dev_put(udc); > + return true; > + } > + return false; > +} > + > static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > { > struct pci_dev *pdev = to_pci_dev(dev); > @@ -179,15 +190,18 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) > xhci->quirks |= XHCI_PME_STUCK_QUIRK; > } > if (pdev->vendor == PCI_VENDOR_ID_INTEL && > - pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) { > + pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) > xhci->quirks |= XHCI_SSIC_PORT_UNUSED; > - xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; > - } > if (pdev->vendor == PCI_VENDOR_ID_INTEL && > (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || > pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI || > pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI)) > xhci->quirks |= XHCI_MISSING_CAS; > + if (pdev->vendor == PCI_VENDOR_ID_INTEL && > + (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || > + pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI) && > + xhci_pci_board_has_udc()) > + xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; > > if (pdev->vendor == PCI_VENDOR_ID_ETRON && > pdev->device == PCI_DEVICE_ID_EJ168) { > This patch and "[PATCH 3/8] plarform: x86: intel_cht_int33fe: Use the USB role switch conditionally" both assume that the mux will be in host mode when Linux boots, so we do not need to touch it. I'm not sure that is a valid assumption. More over the USB DP- / DP+ lines should be floated when in device mode, otherwise USB BC1.2 charger detection will not work, some PMIC-s have their mux for the data lines and will decouple them from the SoCs usb data lines when doing charger detection, but others simply piggy-back on the datalines and are hardwired to the DP- / DP+ lines of the SoC, if we then do not float the datalines the PMICs BC detection logic sees a USB host and assumes its a SDP (standard downstream port) and will limit the charging to 500mA. This is at least true for all devices with an AXP288 PMIC, of which there are a lot (all recent cheap CHT designs use this PMIC). So I believe it is best to drop patches 3 and 4 from this patch-set. Regards, Hans
On Mon, Sep 03, 2018 at 10:01:01AM +0200, Hans de Goede wrote: > This patch and "[PATCH 3/8] plarform: x86: intel_cht_int33fe: Use the USB role switch conditionally" > both assume that the mux will be in host mode when Linux boots, so we do not need to > touch it. I'm not sure that is a valid assumption. > > More over the USB DP- / DP+ lines should be floated when in device > mode, otherwise USB BC1.2 charger detection will not work, some > PMIC-s have their mux for the data lines and will decouple them > from the SoCs usb data lines when doing charger detection, but > others simply piggy-back on the datalines and are hardwired to > the DP- / DP+ lines of the SoC, if we then do not float the datalines > the PMICs BC detection logic sees a USB host and assumes its a SDP > (standard downstream port) and will limit the charging to 500mA. > > This is at least true for all devices with an AXP288 PMIC, of which > there are a lot (all recent cheap CHT designs use this PMIC). > > So I believe it is best to drop patches 3 and 4 from this patch-set. OK, fair enough. I'll drop those. Thanks,
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index 6372edf339d9..ea651c2adcfd 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -75,6 +75,17 @@ static int xhci_pci_reinit(struct xhci_hcd *xhci, struct pci_dev *pdev) return 0; } +static int xhci_pci_board_has_udc(void) +{ + struct pci_dev *udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL); + + if (udc) { + pci_dev_put(udc); + return true; + } + return false; +} + static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) { struct pci_dev *pdev = to_pci_dev(dev); @@ -179,15 +190,18 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) xhci->quirks |= XHCI_PME_STUCK_QUIRK; } if (pdev->vendor == PCI_VENDOR_ID_INTEL && - pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) { + pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) xhci->quirks |= XHCI_SSIC_PORT_UNUSED; - xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; - } if (pdev->vendor == PCI_VENDOR_ID_INTEL && (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI)) xhci->quirks |= XHCI_MISSING_CAS; + if (pdev->vendor == PCI_VENDOR_ID_INTEL && + (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || + pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI) && + xhci_pci_board_has_udc()) + xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; if (pdev->vendor == PCI_VENDOR_ID_ETRON && pdev->device == PCI_DEVICE_ID_EJ168) {
Only create thre Intel role mux device if the platform has USB peripheral controller PCI device. While here, enable the role mux on Apollo Lake platforms. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: Mathias Nyman <mathias.nyman@linux.intel.com> --- drivers/usb/host/xhci-pci.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)