[4/8] usb: xhci: pci: Only create Intel mux device when it's needed

[4/8] usb: xhci: pci: Only create Intel mux device when it's needed

[Heikki Krogerus]

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) {

[4/8] usb: xhci: pci: Only create Intel mux device when it's needed

[Heikki Krogerus]

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,

[4/8] usb: xhci: pci: Only create Intel mux device when it's needed

[Hans de Goede]

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

[4/8] usb: xhci: pci: Only create Intel mux device when it's needed

[Heikki Krogerus]

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,

[4/8] usb: xhci: pci: Only create Intel mux device when it's needed

[Andy Shevchenko]

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?)?