[PATCH v4 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[PATCH v4 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

The xhci platform driver needs to work on systems that
either only support 64-bit DMA or only support 32-bit DMA. 
Attempt to set a coherent dma mask for 64-bit DMA, and 
attempt again with 32-bit DMA if that fails.

[dhdang: regenerate the patch over 4.2-rc5]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Tested-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
Changes from v3:
	Re-generate the patch over 4.2-rc5
	No code change.

Changes from v2:
	None

Changes from v1:
        Consolidated to use dma_set_mask_and_coherent
        Got rid of the check against sizeof(dma_addr_t)

 drivers/usb/host/xhci-plat.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 890ad9d..5d03f8b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -93,14 +93,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENODEV;
 
-	/* Initialize dma_mask and coherent_dma_mask to 32-bits */
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
-	else
-		dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+	/* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (ret) {
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+		if (ret)
+			return ret;
+	}
+
 
 	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
 	if (!hcd)
-- 
1.9.1

[PATCH v4 2/2] usb: Add support for ACPI identification to xhci-platform

[Duc Dang]

Provide the methods to let ACPI identify the need to use
xhci-platform. Change the Kconfig files so the
xhci-plat.o file is selectable during kernel config.

This has been tested on an ARM64 machine with platform XHCI, an
x86_64 machine with XHCI, and an x86_64 machine without XHCI.
There were no regressions or error messages on the machines
without platform XHCI.

[dhdang: regenerate the patch over 4.2-rc5]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
Changes from v3:
	Regenerate the patch over 4.2-rc5
	No code change

Changes from v2
	Replaced tristate with a boolean as the driver doesn't
		compile as a module
	Correct --help-- to ---help---

Changes from v1
	Renamed from "add support for APM X-Gene to xhci-platform"
	Removed changes to arm64/Kconfig
	Made CONFIG_USB_XHCI_PLATFORM a user selectable config option

 drivers/usb/host/Kconfig     |  7 ++++++-
 drivers/usb/host/xhci-plat.c | 11 +++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 8afc3c1..96231ee 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -32,7 +32,12 @@ config USB_XHCI_PCI
        default y
 
 config USB_XHCI_PLATFORM
-	tristate
+	tristate "xHCI platform driver support"
+	---help---
+	  Say 'Y' to enable the support for the xHCI host controller
+	  as a platform device. Many ARM SoCs provide USB this way.
+
+	  If unsure, say 'Y'.
 
 config USB_XHCI_MVEBU
 	tristate "xHCI support for Marvell Armada 375/38x"
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 5d03f8b..14b40d2 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -19,6 +19,7 @@
 #include <linux/usb/phy.h>
 #include <linux/slab.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/acpi.h>
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
@@ -262,6 +263,15 @@ static const struct of_device_id usb_xhci_of_match[] = {
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 #endif
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id usb_xhci_acpi_match[] = {
+	/* APM X-Gene USB Controller */
+	{ "PNP0D10", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match);
+#endif
+
 static struct platform_driver usb_xhci_driver = {
 	.probe	= xhci_plat_probe,
 	.remove	= xhci_plat_remove,
@@ -269,6 +279,7 @@ static struct platform_driver usb_xhci_driver = {
 		.name = "xhci-hcd",
 		.pm = DEV_PM_OPS,
 		.of_match_table = of_match_ptr(usb_xhci_of_match),
+		.acpi_match_table = ACPI_PTR(usb_xhci_acpi_match),
 	},
 };
 MODULE_ALIAS("platform:xhci-hcd");
-- 
1.9.1

[PATCH v4 2/2] usb: Add support for ACPI identification to xhci-platform

[Greg KH]

On Fri, Aug 07, 2015 at 06:03:36PM -0700, Duc Dang wrote:
> Provide the methods to let ACPI identify the need to use
> xhci-platform. Change the Kconfig files so the
> xhci-plat.o file is selectable during kernel config.
> 
> This has been tested on an ARM64 machine with platform XHCI, an
> x86_64 machine with XHCI, and an x86_64 machine without XHCI.
> There were no regressions or error messages on the machines
> without platform XHCI.
> 
> [dhdang: regenerate the patch over 4.2-rc5]
> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
> Signed-off-by: Duc Dang <dhdang@apm.com>
> 
> ---
> Changes from v3:
> 	Regenerate the patch over 4.2-rc5
> 	No code change
> 
> Changes from v2
> 	Replaced tristate with a boolean as the driver doesn't
> 		compile as a module
> 	Correct --help-- to ---help---
> 
> Changes from v1
> 	Renamed from "add support for APM X-Gene to xhci-platform"
> 	Removed changes to arm64/Kconfig
> 	Made CONFIG_USB_XHCI_PLATFORM a user selectable config option
> 
>  drivers/usb/host/Kconfig     |  7 ++++++-
>  drivers/usb/host/xhci-plat.c | 11 +++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 8afc3c1..96231ee 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -32,7 +32,12 @@ config USB_XHCI_PCI
>         default y
>  
>  config USB_XHCI_PLATFORM
> -	tristate
> +	tristate "xHCI platform driver support"
> +	---help---
> +	  Say 'Y' to enable the support for the xHCI host controller
> +	  as a platform device. Many ARM SoCs provide USB this way.
> +
> +	  If unsure, say 'Y'.
>  
>  config USB_XHCI_MVEBU
>  	tristate "xHCI support for Marvell Armada 375/38x"
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 5d03f8b..14b40d2 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -19,6 +19,7 @@
>  #include <linux/usb/phy.h>
>  #include <linux/slab.h>
>  #include <linux/usb/xhci_pdriver.h>
> +#include <linux/acpi.h>
>  
>  #include "xhci.h"
>  #include "xhci-mvebu.h"
> @@ -262,6 +263,15 @@ static const struct of_device_id usb_xhci_of_match[] = {
>  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
>  #endif
>  
> +#ifdef CONFIG_ACPI

You shoudn't need this #ifdef, right?

thanks,

greg k-h

[PATCH v4 2/2] usb: Add support for ACPI identification to xhci-platform

[Duc Dang]

On Fri, Aug 7, 2015 at 6:29 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Aug 07, 2015 at 06:03:36PM -0700, Duc Dang wrote:
>> Provide the methods to let ACPI identify the need to use
>> xhci-platform. Change the Kconfig files so the
>> xhci-plat.o file is selectable during kernel config.
>>
>> This has been tested on an ARM64 machine with platform XHCI, an
>> x86_64 machine with XHCI, and an x86_64 machine without XHCI.
>> There were no regressions or error messages on the machines
>> without platform XHCI.
>>
>> [dhdang: regenerate the patch over 4.2-rc5]
>> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>>
>> ---
>> Changes from v3:
>>       Regenerate the patch over 4.2-rc5
>>       No code change
>>
>> Changes from v2
>>       Replaced tristate with a boolean as the driver doesn't
>>               compile as a module
>>       Correct --help-- to ---help---
>>
>> Changes from v1
>>       Renamed from "add support for APM X-Gene to xhci-platform"
>>       Removed changes to arm64/Kconfig
>>       Made CONFIG_USB_XHCI_PLATFORM a user selectable config option
>>
>>  drivers/usb/host/Kconfig     |  7 ++++++-
>>  drivers/usb/host/xhci-plat.c | 11 +++++++++++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index 8afc3c1..96231ee 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -32,7 +32,12 @@ config USB_XHCI_PCI
>>         default y
>>
>>  config USB_XHCI_PLATFORM
>> -     tristate
>> +     tristate "xHCI platform driver support"
>> +     ---help---
>> +       Say 'Y' to enable the support for the xHCI host controller
>> +       as a platform device. Many ARM SoCs provide USB this way.
>> +
>> +       If unsure, say 'Y'.
>>
>>  config USB_XHCI_MVEBU
>>       tristate "xHCI support for Marvell Armada 375/38x"
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 5d03f8b..14b40d2 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/usb/phy.h>
>>  #include <linux/slab.h>
>>  #include <linux/usb/xhci_pdriver.h>
>> +#include <linux/acpi.h>
>>
>>  #include "xhci.h"
>>  #include "xhci-mvebu.h"
>> @@ -262,6 +263,15 @@ static const struct of_device_id usb_xhci_of_match[] = {
>>  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
>>  #endif
>>
>> +#ifdef CONFIG_ACPI
>
> You shoudn't need this #ifdef, right?

You are correct, Greg.

I will post a new version that remove this #ifdef CONFIG_ACPI shortly

>
> thanks,
>
> greg k-h



-- 
Regards,
Duc Dang.

[PATCH v5 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

The xhci platform driver needs to work on systems that
either only support 64-bit DMA or only support 32-bit DMA.
Attempt to set a coherent dma mask for 64-bit DMA, and
attempt again with 32-bit DMA if that fails.

[dhdang: Regenerate the patch over 4.2-rc5]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Tested-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
Changes from v4:
	None

Changes from v3:
        Re-generate the patch over 4.2-rc5
        No code change.

Changes from v2:
        None

Changes from v1:
        Consolidated to use dma_set_mask_and_coherent
        Got rid of the check against sizeof(dma_addr_t)

 drivers/usb/host/xhci-plat.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 890ad9d..5d03f8b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -93,14 +93,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENODEV;
 
-	/* Initialize dma_mask and coherent_dma_mask to 32-bits */
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
-	else
-		dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+	/* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (ret) {
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+		if (ret)
+			return ret;
+	}
+
 
 	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
 	if (!hcd)
-- 
1.9.1

[PATCH v5 2/2] usb: Add support for ACPI identification to xhci-platform

[Duc Dang]

Provide the methods to let ACPI identify the need to use
xhci-platform. Change the Kconfig files so the
xhci-plat.o file is selectable during kernel config.

This has been tested on an ARM64 machine with platform XHCI, an
x86_64 machine with XHCI, and an x86_64 machine without XHCI.
There were no regressions or error messages on the machines
without platform XHCI.

Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
Changes from v4:
	Remove #ifdef CONFIG_ACPI

Changes from v3:
        Regenerate the patch over 4.2-rc5
        No code change

Changes from v2
        Replaced tristate with a boolean as the driver doesn't
                compile as a module
        Correct --help-- to ---help---

Changes from v1
        Renamed from "add support for APM X-Gene to xhci-platform"
        Removed changes to arm64/Kconfig
        Made CONFIG_USB_XHCI_PLATFORM a user selectable config option

 drivers/usb/host/Kconfig     | 7 ++++++-
 drivers/usb/host/xhci-plat.c | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 8afc3c1..96231ee 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -32,7 +32,12 @@ config USB_XHCI_PCI
        default y
 
 config USB_XHCI_PLATFORM
-	tristate
+	tristate "xHCI platform driver support"
+	---help---
+	  Say 'Y' to enable the support for the xHCI host controller
+	  as a platform device. Many ARM SoCs provide USB this way.
+
+	  If unsure, say 'Y'.
 
 config USB_XHCI_MVEBU
 	tristate "xHCI support for Marvell Armada 375/38x"
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 5d03f8b..bd282cd 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -19,6 +19,7 @@
 #include <linux/usb/phy.h>
 #include <linux/slab.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/acpi.h>
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
@@ -262,6 +263,13 @@ static const struct of_device_id usb_xhci_of_match[] = {
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 #endif
 
+static const struct acpi_device_id usb_xhci_acpi_match[] = {
+	/* APM X-Gene USB Controller */
+	{ "PNP0D10", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match);
+
 static struct platform_driver usb_xhci_driver = {
 	.probe	= xhci_plat_probe,
 	.remove	= xhci_plat_remove,
@@ -269,6 +277,7 @@ static struct platform_driver usb_xhci_driver = {
 		.name = "xhci-hcd",
 		.pm = DEV_PM_OPS,
 		.of_match_table = of_match_ptr(usb_xhci_of_match),
+		.acpi_match_table = ACPI_PTR(usb_xhci_acpi_match),
 	},
 };
 MODULE_ALIAS("platform:xhci-hcd");
-- 
1.9.1

[PATCH v5 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 08:18:48PM -0700, Duc Dang wrote:
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 890ad9d..5d03f8b 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -93,14 +93,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return -ENODEV;
>  
> -	/* Initialize dma_mask and coherent_dma_mask to 32-bits */
> -	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> -	if (ret)
> -		return ret;
> -	if (!pdev->dev.dma_mask)
> -		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> -	else
> -		dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	/* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> +	if (ret) {
> +		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +		if (ret)
> +			return ret;
> +	}

Note that dma_set_mask_and_coherent() and the original code are not
equivalent because of this:

	if (!pdev->dev.dma_mask)
		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;

If we know that pdev->dev.dma_mask will always be initialised at this
point, then the above change is fine.  If not, it's introducing a
regression - dma_set_mask_and_coherent() will fail if pdev->dev.dma_mask
is NULL (depending on the architectures implementation of dma_set_mask()).

Prefixing the above change with the two lines I mention above would
ensure equivalent behaviour.  Even if we do want to get rid of this,
I'd advise to do it as a separate patch after this change, which can
be independently reverted if there's problems with its removal.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v5 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

On Sat, Aug 8, 2015 at 2:22 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Aug 07, 2015 at 08:18:48PM -0700, Duc Dang wrote:
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 890ad9d..5d03f8b 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -93,14 +93,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>       if (irq < 0)
>>               return -ENODEV;
>>
>> -     /* Initialize dma_mask and coherent_dma_mask to 32-bits */
>> -     ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
>> -     if (ret)
>> -             return ret;
>> -     if (!pdev->dev.dma_mask)
>> -             pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>> -     else
>> -             dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>> +     /* Try setting the coherent_dma_mask to 64 bits, then try 32 bits */
>> +     ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +     if (ret) {
>> +             ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> +             if (ret)
>> +                     return ret;
>> +     }
>
> Note that dma_set_mask_and_coherent() and the original code are not
> equivalent because of this:
>
>         if (!pdev->dev.dma_mask)
>                 pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>
> If we know that pdev->dev.dma_mask will always be initialised at this
> point, then the above change is fine.  If not, it's introducing a
> regression - dma_set_mask_and_coherent() will fail if pdev->dev.dma_mask
> is NULL (depending on the architectures implementation of dma_set_mask()).
>
> Prefixing the above change with the two lines I mention above would
> ensure equivalent behaviour.  Even if we do want to get rid of this,
> I'd advise to do it as a separate patch after this change, which can
> be independently reverted if there's problems with its removal.
>
Hi Russell,

I will add the 2 lines you mentioned back to next version of the
patch. It is safer to do it that way as I do not see
pdev->dev.dma_mask gets initialized before the call
dma_set_mask_and_coherent  inside this xhci_plat.c file.

> --
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.

-- 
Regards,
Duc Dang.

[PATCH v5 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Arnd Bergmann]

On Saturday 08 August 2015 13:31:02 Duc Dang wrote:
> >
> > If we know that pdev->dev.dma_mask will always be initialised at this
> > point, then the above change is fine.  If not, it's introducing a
> > regression - dma_set_mask_and_coherent() will fail if pdev->dev.dma_mask
> > is NULL (depending on the architectures implementation of dma_set_mask()).
> >
> > Prefixing the above change with the two lines I mention above would
> > ensure equivalent behaviour.  Even if we do want to get rid of this,
> > I'd advise to do it as a separate patch after this change, which can
> > be independently reverted if there's problems with its removal.
> >
> Hi Russell,
> 
> I will add the 2 lines you mentioned back to next version of the
> patch. It is safer to do it that way as I do not see
> pdev->dev.dma_mask gets initialized before the call
> dma_set_mask_and_coherent  inside this xhci_plat.c file.

It would be good to add a WARN_ON() to the case where dma_mask
is a NULL pointer at the least. That way, we will at least
find out if there are some broken platforms that do not correctly
initialize the mask pointer.

	Arnd

[PATCH v5 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

On Sat, Aug 15, 2015 at 1:05 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Saturday 08 August 2015 13:31:02 Duc Dang wrote:
> > >
> > > If we know that pdev->dev.dma_mask will always be initialised at this
> > > point, then the above change is fine.  If not, it's introducing a
> > > regression - dma_set_mask_and_coherent() will fail if pdev->dev.dma_mask
> > > is NULL (depending on the architectures implementation of dma_set_mask()).
> > >
> > > Prefixing the above change with the two lines I mention above would
> > > ensure equivalent behaviour.  Even if we do want to get rid of this,
> > > I'd advise to do it as a separate patch after this change, which can
> > > be independently reverted if there's problems with its removal.
> > >
> > Hi Russell,
> >
> > I will add the 2 lines you mentioned back to next version of the
> > patch. It is safer to do it that way as I do not see
> > pdev->dev.dma_mask gets initialized before the call
> > dma_set_mask_and_coherent  inside this xhci_plat.c file.
>
> It would be good to add a WARN_ON() to the case where dma_mask
> is a NULL pointer at the least. That way, we will at least
> find out if there are some broken platforms that do not correctly
> initialize the mask pointer.

Hi Arnd,

So the check will look like this, please let me know what do you think:
        if (!pdev->dev.dma_mask) {
                WARN_ON(1);
                /* Initialize dma_mask if the broken platform code has
not done so */
                pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
        }

>
>         Arnd




-- 
Regards,
Duc Dang.

[PATCH v5 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Arnd Bergmann]

On Wednesday 19 August 2015 14:28:33 Duc Dang wrote:
> 
> Hi Arnd,
> 
> So the check will look like this, please let me know what do you think:
>         if (!pdev->dev.dma_mask) {
>                 WARN_ON(1);
>                 /* Initialize dma_mask if the broken platform code has
> not done so */
>                 pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>         }

The condition can be written as 

	if (WARN_ON(!pdev->dev.dma_mask))

and I'd use dma_coerce_mask_and_coherent() instead of manually setting the
pointer, as an annotation for the fact that we are knowingly violating the
API here.

Those two points are just cosmetic though, aside from them, your code
above is what I had in mind.

Thanks,

	Arnd

[PATCH v7 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

The xhci platform driver needs to work on systems that
either only support 64-bit DMA or only support 32-bit DMA.
Attempt to set a coherent dma mask for 64-bit DMA, and
attempt again with 32-bit DMA if that fails.

[dhdang: regenerate the patch over 4.2-rc5 and address new comments]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Tested-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
Changes from v6:
	-Add WARN_ON if dma_mask is NULL
	-Use dma_coerce_mask_and_coherent to assign
	dma_mask and coherent_dma_mask

Changes from v5:
        -Change comment
        -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
        to make sure dma_set_mask_and_coherent does not fail prematurely.

Changes from v4:
        -None

Changes from v3:
        -Re-generate the patch over 4.2-rc5
        -No code change.

Changes from v2:
        -None

Changes from v1:
        -Consolidated to use dma_set_mask_and_coherent
        -Got rid of the check against sizeof(dma_addr_t)

 drivers/usb/host/xhci-plat.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 890ad9d..e4c7f9d 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -93,14 +93,19 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENODEV;
 
-	/* Initialize dma_mask and coherent_dma_mask to 32-bits */
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
-	else
-		dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+	/* Throw a waring if broken platform code didn't initialize dma_mask */
+	WARN_ON(!pdev->dev.dma_mask);
+	/*
+	 * Try setting dma_mask and coherent_dma_mask to 64 bits,
+	 * then try 32 bits
+	 */
+	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (ret) {
+		ret = dma_coerce_mask_and_coherent(&pdev->dev,
+						   DMA_BIT_MASK(32));
+		if (ret)
+			return ret;
+	}
 
 	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
 	if (!hcd)
-- 
1.9.1

[PATCH v7 2/2] usb: Add support for ACPI identification to xhci-platform

[Duc Dang]

Provide the methods to let ACPI identify the need to use
xhci-platform. Change the Kconfig files so the
xhci-plat.o file is selectable during kernel config.

This has been tested on an ARM64 machine with platform XHCI, an
x86_64 machine with XHCI, and an x86_64 machine without XHCI.
There were no regressions or error messages on the machines
without platform XHCI.

[dhdang: regenerate the patch over 4.2-rc5 and address new comments]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
Changes from v6:
	-None

Change from v5:
        -Change comment to "XHCI-compliant USB Controller" as
        "PNP0D10" ID is not X-Gene specific

Changes from v4:
        -Remove #ifdef CONFIG_ACPI

Changes from v3:
        -Regenerate the patch over 4.2-rc5
        -No code change

Changes from v2
        -Replaced tristate with a boolean as the driver doesn't
                compile as a module
        -Correct --help-- to ---help---

Changes from v1
        -Renamed from "add support for APM X-Gene to xhci-platform"
        -Removed changes to arm64/Kconfig
        -Made CONFIG_USB_XHCI_PLATFORM a user selectable config option

 drivers/usb/host/Kconfig     | 7 ++++++-
 drivers/usb/host/xhci-plat.c | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 8afc3c1..96231ee 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -32,7 +32,12 @@ config USB_XHCI_PCI
        default y
 
 config USB_XHCI_PLATFORM
-	tristate
+	tristate "xHCI platform driver support"
+	---help---
+	  Say 'Y' to enable the support for the xHCI host controller
+	  as a platform device. Many ARM SoCs provide USB this way.
+
+	  If unsure, say 'Y'.
 
 config USB_XHCI_MVEBU
 	tristate "xHCI support for Marvell Armada 375/38x"
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index e4c7f9d..6c03e1c 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -19,6 +19,7 @@
 #include <linux/usb/phy.h>
 #include <linux/slab.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/acpi.h>
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
@@ -267,6 +268,13 @@ static const struct of_device_id usb_xhci_of_match[] = {
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 #endif
 
+static const struct acpi_device_id usb_xhci_acpi_match[] = {
+	/* XHCI-compliant USB Controller */
+	{ "PNP0D10", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match);
+
 static struct platform_driver usb_xhci_driver = {
 	.probe	= xhci_plat_probe,
 	.remove	= xhci_plat_remove,
@@ -274,6 +282,7 @@ static struct platform_driver usb_xhci_driver = {
 		.name = "xhci-hcd",
 		.pm = DEV_PM_OPS,
 		.of_match_table = of_match_ptr(usb_xhci_of_match),
+		.acpi_match_table = ACPI_PTR(usb_xhci_acpi_match),
 	},
 };
 MODULE_ALIAS("platform:xhci-hcd");
-- 
1.9.1

[PATCH v7 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

On Thu, Aug 20, 2015 at 12:38 PM, Duc Dang <dhdang@apm.com> wrote:
> The xhci platform driver needs to work on systems that
> either only support 64-bit DMA or only support 32-bit DMA.
> Attempt to set a coherent dma mask for 64-bit DMA, and
> attempt again with 32-bit DMA if that fails.
>
> [dhdang: regenerate the patch over 4.2-rc5 and address new comments]
> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
> Tested-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Duc Dang <dhdang@apm.com>
>
> ---
> Changes from v6:
>         -Add WARN_ON if dma_mask is NULL
>         -Use dma_coerce_mask_and_coherent to assign
>         dma_mask and coherent_dma_mask
>
> Changes from v5:
>         -Change comment
>         -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
>         to make sure dma_set_mask_and_coherent does not fail prematurely.
>
> Changes from v4:
>         -None
>
> Changes from v3:
>         -Re-generate the patch over 4.2-rc5
>         -No code change.
>
> Changes from v2:
>         -None
>
> Changes from v1:
>         -Consolidated to use dma_set_mask_and_coherent
>         -Got rid of the check against sizeof(dma_addr_t)
>
>  drivers/usb/host/xhci-plat.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 890ad9d..e4c7f9d 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -93,14 +93,19 @@ static int xhci_plat_probe(struct platform_device *pdev)
>         if (irq < 0)
>                 return -ENODEV;
>
> -       /* Initialize dma_mask and coherent_dma_mask to 32-bits */
> -       ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> -       if (ret)
> -               return ret;
> -       if (!pdev->dev.dma_mask)
> -               pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> -       else
> -               dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> +       /* Throw a waring if broken platform code didn't initialize dma_mask */
> +       WARN_ON(!pdev->dev.dma_mask);
> +       /*
> +        * Try setting dma_mask and coherent_dma_mask to 64 bits,
> +        * then try 32 bits
> +        */
> +       ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> +       if (ret) {
> +               ret = dma_coerce_mask_and_coherent(&pdev->dev,
> +                                                  DMA_BIT_MASK(32));
> +               if (ret)
> +                       return ret;
> +       }
>
>         hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>         if (!hcd)
> --
> 1.9.1
>

Hi Greg, Arnd, Russell,

Do you have any more comment about this patch set?

-- 
Duc Dang.

[PATCH v7 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Mathias Nyman]

On 31.08.2015 21:58, Duc Dang wrote:
> On Thu, Aug 20, 2015 at 12:38 PM, Duc Dang <dhdang@apm.com> wrote:
>> The xhci platform driver needs to work on systems that
>> either only support 64-bit DMA or only support 32-bit DMA.
>> Attempt to set a coherent dma mask for 64-bit DMA, and
>> attempt again with 32-bit DMA if that fails.
>>
>> [dhdang: regenerate the patch over 4.2-rc5 and address new comments]
>> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>> Tested-by: Mark Salter <msalter@redhat.com>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>>
>> ---
>> Changes from v6:
>>          -Add WARN_ON if dma_mask is NULL
>>          -Use dma_coerce_mask_and_coherent to assign
>>          dma_mask and coherent_dma_mask
>>
>> Changes from v5:
>>          -Change comment
>>          -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
>>          to make sure dma_set_mask_and_coherent does not fail prematurely.
>>
>> Changes from v4:
>>          -None
>>
>> Changes from v3:
>>          -Re-generate the patch over 4.2-rc5
>>          -No code change.
>>
>> Changes from v2:
>>          -None
>>
>> Changes from v1:
>>          -Consolidated to use dma_set_mask_and_coherent
>>          -Got rid of the check against sizeof(dma_addr_t)
>>
>>   drivers/usb/host/xhci-plat.c | 21 +++++++++++++--------
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 890ad9d..e4c7f9d 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -93,14 +93,19 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>          if (irq < 0)
>>                  return -ENODEV;
>>
>> -       /* Initialize dma_mask and coherent_dma_mask to 32-bits */
>> -       ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
>> -       if (ret)
>> -               return ret;
>> -       if (!pdev->dev.dma_mask)
>> -               pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>> -       else
>> -               dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>> +       /* Throw a waring if broken platform code didn't initialize dma_mask */
>> +       WARN_ON(!pdev->dev.dma_mask);
>> +       /*
>> +        * Try setting dma_mask and coherent_dma_mask to 64 bits,
>> +        * then try 32 bits
>> +        */
>> +       ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> +       if (ret) {
>> +               ret = dma_coerce_mask_and_coherent(&pdev->dev,
>> +                                                  DMA_BIT_MASK(32));
>> +               if (ret)
>> +                       return ret;
>> +       }
>>
>>          hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>>          if (!hcd)
>> --
>> 1.9.1
>>
>
> Hi Greg, Arnd, Russell,
>
> Do you have any more comment about this patch set?
>

I'm not sure I fully understand why we need to try the 64 bit DMA mask in platform probe.

As I understood it we just want to have some DMA mask set before calling usb_create_hcd()
to make sure USB core gets the "uses_dma" flag set and dma_set_mask() won't fail because of
missing dev->dma_mask.

The correct DMA mask is set later in xhci_gen_setup()

We also need to make sure the controller supports 64bit addressing capability before setting a 64 bit DMA mask.
(bit 0 in HCCPARAMS)

So for platform devices it goes look go this:

xhci_plat_probe()
   usb_create_hcd()
     usb_create_shared_hcd()
       hcd->self.uses_dma = (dev->dma_mask != NULL);
   usb_add_hcd()
     hcd->driver->reset()  (.reset = xhci_plat_setup)
       xhci_plat_setup()
         xhci_gen_setup()
           if (HCC_64BIT_ADDR(xhci->hcc_params) && !dma_set_mask(dev, DMA_BIT_MASK(64))) {
             xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
             dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
           }

or do we end up with dev->dma_mask = NULL on 64-bit DMA only after trying to set it to 32 in xhci_plat_probe(),
and thus also failing the dma_set_mask() in xhci_gen_setup()?

-Mathias

[PATCH v7 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Russell King - ARM Linux]

On Tue, Sep 01, 2015 at 02:54:17PM +0300, Mathias Nyman wrote:
> On 31.08.2015 21:58, Duc Dang wrote:
> >On Thu, Aug 20, 2015 at 12:38 PM, Duc Dang <dhdang@apm.com> wrote:
> >>The xhci platform driver needs to work on systems that
> >>either only support 64-bit DMA or only support 32-bit DMA.
> >>Attempt to set a coherent dma mask for 64-bit DMA, and
> >>attempt again with 32-bit DMA if that fails.
> >>
> >>[dhdang: regenerate the patch over 4.2-rc5 and address new comments]
> >>Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
> >>Tested-by: Mark Salter <msalter@redhat.com>
> >>Signed-off-by: Duc Dang <dhdang@apm.com>
> >>
> >>---
> >>Changes from v6:
> >>         -Add WARN_ON if dma_mask is NULL
> >>         -Use dma_coerce_mask_and_coherent to assign
> >>         dma_mask and coherent_dma_mask
> >>
> >>Changes from v5:
> >>         -Change comment
> >>         -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
> >>         to make sure dma_set_mask_and_coherent does not fail prematurely.
> >>
> >>Changes from v4:
> >>         -None
> >>
> >>Changes from v3:
> >>         -Re-generate the patch over 4.2-rc5
> >>         -No code change.
> >>
> >>Changes from v2:
> >>         -None
> >>
> >>Changes from v1:
> >>         -Consolidated to use dma_set_mask_and_coherent
> >>         -Got rid of the check against sizeof(dma_addr_t)
> >>
> >>  drivers/usb/host/xhci-plat.c | 21 +++++++++++++--------
> >>  1 file changed, 13 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> >>index 890ad9d..e4c7f9d 100644
> >>--- a/drivers/usb/host/xhci-plat.c
> >>+++ b/drivers/usb/host/xhci-plat.c
> >>@@ -93,14 +93,19 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >>         if (irq < 0)
> >>                 return -ENODEV;
> >>
> >>-       /* Initialize dma_mask and coherent_dma_mask to 32-bits */
> >>-       ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> >>-       if (ret)
> >>-               return ret;
> >>-       if (!pdev->dev.dma_mask)
> >>-               pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> >>-       else
> >>-               dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> >>+       /* Throw a waring if broken platform code didn't initialize dma_mask */
> >>+       WARN_ON(!pdev->dev.dma_mask);
> >>+       /*
> >>+        * Try setting dma_mask and coherent_dma_mask to 64 bits,
> >>+        * then try 32 bits
> >>+        */
> >>+       ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> >>+       if (ret) {
> >>+               ret = dma_coerce_mask_and_coherent(&pdev->dev,
> >>+                                                  DMA_BIT_MASK(32));
> >>+               if (ret)
> >>+                       return ret;
> >>+       }

This isn't very good.  If dev.dma_mask is already set,
dma_coerce_mask_and_coherent() will always overwrite it.  There's also
no need to call it twice.  This, imho, is much better:

	/* Try to set a 64-bit DMA mask first */
	if (WARN_ON(!pdev->dev.dma_mask)) {
		/* Eek, platform didn't initialise the streaming DMA mask */
		ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
	} else {
		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
	}

	/* If that failed, fall back to a 32-bit DMA mask */
	if (ret) {
		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
		if (ret)
			return ret;
	}

since it preserves the dev.dma_mask pointer if it was properly setup

Really, drivers shouldn't be messing around with that pointer - especially
if it's already been correctly setup.  A platform may require separate
streaming and coherent masks, and we should respect that.

(The whole dma_mask being a pointer thing is a left-over from the PCI
layer which has never been cleaned up through fear of breaking something.)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v7 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

On Tue, Sep 1, 2015 at 4:54 AM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> On 31.08.2015 21:58, Duc Dang wrote:
>>
>> On Thu, Aug 20, 2015 at 12:38 PM, Duc Dang <dhdang@apm.com> wrote:
>>>
>>> The xhci platform driver needs to work on systems that
>>> either only support 64-bit DMA or only support 32-bit DMA.
>>> Attempt to set a coherent dma mask for 64-bit DMA, and
>>> attempt again with 32-bit DMA if that fails.
>>>
>>> [dhdang: regenerate the patch over 4.2-rc5 and address new comments]
>>> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>>> Tested-by: Mark Salter <msalter@redhat.com>
>>> Signed-off-by: Duc Dang <dhdang@apm.com>
>>>
>>> ---
>>> Changes from v6:
>>>          -Add WARN_ON if dma_mask is NULL
>>>          -Use dma_coerce_mask_and_coherent to assign
>>>          dma_mask and coherent_dma_mask
>>>
>>> Changes from v5:
>>>          -Change comment
>>>          -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
>>>          to make sure dma_set_mask_and_coherent does not fail
>>> prematurely.
>>>
>>> Changes from v4:
>>>          -None
>>>
>>> Changes from v3:
>>>          -Re-generate the patch over 4.2-rc5
>>>          -No code change.
>>>
>>> Changes from v2:
>>>          -None
>>>
>>> Changes from v1:
>>>          -Consolidated to use dma_set_mask_and_coherent
>>>          -Got rid of the check against sizeof(dma_addr_t)
>>>
>>>   drivers/usb/host/xhci-plat.c | 21 +++++++++++++--------
>>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>> index 890ad9d..e4c7f9d 100644
>>> --- a/drivers/usb/host/xhci-plat.c
>>> +++ b/drivers/usb/host/xhci-plat.c
>>> @@ -93,14 +93,19 @@ static int xhci_plat_probe(struct platform_device
>>> *pdev)
>>>          if (irq < 0)
>>>                  return -ENODEV;
>>>
>>> -       /* Initialize dma_mask and coherent_dma_mask to 32-bits */
>>> -       ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
>>> -       if (ret)
>>> -               return ret;
>>> -       if (!pdev->dev.dma_mask)
>>> -               pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
>>> -       else
>>> -               dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>>> +       /* Throw a waring if broken platform code didn't initialize
>>> dma_mask */
>>> +       WARN_ON(!pdev->dev.dma_mask);
>>> +       /*
>>> +        * Try setting dma_mask and coherent_dma_mask to 64 bits,
>>> +        * then try 32 bits
>>> +        */
>>> +       ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>>> +       if (ret) {
>>> +               ret = dma_coerce_mask_and_coherent(&pdev->dev,
>>> +                                                  DMA_BIT_MASK(32));
>>> +               if (ret)
>>> +                       return ret;
>>> +       }
>>>
>>>          hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>>>          if (!hcd)
>>> --
>>> 1.9.1
>>>
>>
>> Hi Greg, Arnd, Russell,
>>
>> Do you have any more comment about this patch set?
>>
>
> I'm not sure I fully understand why we need to try the 64 bit DMA mask in
> platform probe.
>
> As I understood it we just want to have some DMA mask set before calling
> usb_create_hcd()
> to make sure USB core gets the "uses_dma" flag set and dma_set_mask() won't
> fail because of
> missing dev->dma_mask.
>
> The correct DMA mask is set later in xhci_gen_setup()
>
> We also need to make sure the controller supports 64bit addressing
> capability before setting a 64 bit DMA mask.
> (bit 0 in HCCPARAMS)
>
> So for platform devices it goes look go this:
>
> xhci_plat_probe()
>   usb_create_hcd()
>     usb_create_shared_hcd()
>       hcd->self.uses_dma = (dev->dma_mask != NULL);
>   usb_add_hcd()
>     hcd->driver->reset()  (.reset = xhci_plat_setup)
>       xhci_plat_setup()
>         xhci_gen_setup()
>           if (HCC_64BIT_ADDR(xhci->hcc_params) && !dma_set_mask(dev,
> DMA_BIT_MASK(64))) {
>             xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
>             dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>           }
>
> or do we end up with dev->dma_mask = NULL on 64-bit DMA only after trying to
> set it to 32 in xhci_plat_probe(),
> and thus also failing the dma_set_mask() in xhci_gen_setup()?
>

Hi Mathias,

dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32)) in xhci_plat_probe
fails on our Arm64 X-Gene platform as we do not have DMA memory below 4GB. As a
consequence, xhci_plat_probe exits very early without creating any hcd.

The xhci_gen_setup code assumes that the dma_mask and
coherent_dma_mask were already
set to DMA_BIT_MASK(32) and only check to switch to DMA_BIT_MASK(64)
if the controller
supports 64-bit DMA. So I also see possible regression if a 32-bit
controller is used on 64-bit
system with my patch.

I will integrate Russell's suggestion and add changes to address the
case of 32-bit controller on 64-bit system
in my next version of this patch set.

> -Mathias
>
>
>
>
>
>
>
>
>
>


Regards,
-- 
Duc Dang.

[PATCH v8 0/3] usb: xhci-platform: Configure 64-bit DMA mask if the platform is capable

[Duc Dang]

The xhci platform driver does not work with system that only supports
64-bit DMA as it requests 32-bit DMA mask during driver initialization.
This patch set addresses this issue and also adds XHCI-compliant USB
Controller ACPI identification into xhci-platform driver.

Changes from v7:
        - Only use dma_coerce_mask_and_coherent when
        dma_mask is NULL
        - Check the controller DMA capability and configure
        32-bit dma_mask if it only supports 32-bit DMA
        - Patches is generated over v4.3-rc1

Changes from v6:
        -Add WARN_ON if dma_mask is NULL
        -Use dma_coerce_mask_and_coherent to assign
        dma_mask and coherent_dma_mask

Change from v5:
        -Change comment to "XHCI-compliant USB Controller" as
        "PNP0D10" ID is not X-Gene specific
        -Change comment
        -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
        to make sure dma_set_mask_and_coherent does not fail prematurely.

Changes from v4:
        -Remove #ifdef CONFIG_ACPI
        -Change comment
        -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
        to make sure dma_set_mask_and_coherent does not fail prematurely.

Changes from v3:
        -Regenerate the patch over 4.2-rc5
        -No code change

Changes from v2
        -Replaced tristate with a boolean as the driver doesn't
                compile as a module
        -Correct --help-- to ---help---

Changes from v1
        -Consolidated to use dma_set_mask_and_coherent
        -Got rid of the check against sizeof(dma_addr_t)
        -Renamed from "add support for APM X-Gene to xhci-platform"
        -Removed changes to arm64/Kconfig
        -Made CONFIG_USB_XHCI_PLATFORM a user selectable config option

 drivers/usb/host/xhci-plat.c | 29 ++++++++++++++++++++++-------
 drivers/usb/host/xhci.c      | 10 ++++++++++
 2 files changed, 32 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH v8 1/3] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

The xhci platform driver needs to work on systems that
either only support 64-bit DMA or only support 32-bit DMA.
Attempt to set a coherent dma mask for 64-bit DMA, and
attempt again with 32-bit DMA if that fails.

[dhdang: regenerate the patch over v4.3-rc1 and address new comments]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Tested-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
 drivers/usb/host/xhci-plat.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 890ad9d..34c93e8 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -93,14 +93,20 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENODEV;
 
-	/* Initialize dma_mask and coherent_dma_mask to 32-bits */
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-	if (!pdev->dev.dma_mask)
-		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
+	/* Try to set 64-bit DMA first */
+	if (WARN_ON(!pdev->dev.dma_mask))
+		/* Platform did not initialize dma_mask */
+		ret = dma_coerce_mask_and_coherent(&pdev->dev,
+						   DMA_BIT_MASK(64));
 	else
-		dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+
+	/* If seting 64-bit DMA mask fails, fall back to 32-bit DMA mask */
+	if (ret) {
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+		if (ret)
+			return ret;
+	}
 
 	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
 	if (!hcd)
-- 
1.9.1

[PATCH v8 1/3] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Arnd Bergmann]

On Thursday 17 September 2015 11:19:46 Duc Dang wrote:
> The xhci platform driver needs to work on systems that
> either only support 64-bit DMA or only support 32-bit DMA.
> Attempt to set a coherent dma mask for 64-bit DMA, and
> attempt again with 32-bit DMA if that fails.
> 
> [dhdang: regenerate the patch over v4.3-rc1 and address new comments]
> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
> Tested-by: Mark Salter <msalter@redhat.com>
> Signed-off-by: Duc Dang <dhdang@apm.com>
> 

Acked-by: Arnd Bergmann <arnd@arndb.de>

[PATCH v8 1/3] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

On Thu, Sep 17, 2015 at 12:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 17 September 2015 11:19:46 Duc Dang wrote:
>> The xhci platform driver needs to work on systems that
>> either only support 64-bit DMA or only support 32-bit DMA.
>> Attempt to set a coherent dma mask for 64-bit DMA, and
>> attempt again with 32-bit DMA if that fails.
>>
>> [dhdang: regenerate the patch over v4.3-rc1 and address new comments]
>> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>> Tested-by: Mark Salter <msalter@redhat.com>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>>
>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Thanks, Arnd.

-- 
Regards,
Duc Dang.

[PATCH v8 2/3] usb: Add support for ACPI identification to xhci-platform

[Duc Dang]

Provide the methods to let ACPI identify the need to use
xhci-platform. Change the Kconfig files so the
xhci-plat.o file is selectable during kernel config.

This has been tested on an ARM64 machine with platform XHCI, an
x86_64 machine with XHCI, and an x86_64 machine without XHCI.
There were no regressions or error messages on the machines
without platform XHCI.

[dhdang: regenerate the patch over v4.3-rc1 and address new comments]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
 drivers/usb/host/xhci-plat.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 34c93e8..05647e6 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -19,6 +19,7 @@
 #include <linux/usb/phy.h>
 #include <linux/slab.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/acpi.h>
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
@@ -268,6 +269,13 @@ static const struct of_device_id usb_xhci_of_match[] = {
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 #endif
 
+static const struct acpi_device_id usb_xhci_acpi_match[] = {
+	/* XHCI-compliant USB Controller */
+	{ "PNP0D10", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match);
+
 static struct platform_driver usb_xhci_driver = {
 	.probe	= xhci_plat_probe,
 	.remove	= xhci_plat_remove,
@@ -275,6 +283,7 @@ static struct platform_driver usb_xhci_driver = {
 		.name = "xhci-hcd",
 		.pm = DEV_PM_OPS,
 		.of_match_table = of_match_ptr(usb_xhci_of_match),
+		.acpi_match_table = ACPI_PTR(usb_xhci_acpi_match),
 	},
 };
 MODULE_ALIAS("platform:xhci-hcd");
-- 
1.9.1

[PATCH v8 3/3] usb: xhci: configure 32-bit DMA if the controller does not support 64-bit DMA

[Duc Dang]

This change avoids DMA error in the cases where dma_mask and
coherent_dma_mask of a 32-bit controller get configured as
DMA_BIT_MASK(64) when running on a 64-bit system.

Signed-off-by: Duc Dang <dhdang@apm.com>
---
 drivers/usb/host/xhci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6b0f4a4..8cb8f39 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4908,6 +4908,16 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 			!dma_set_mask(dev, DMA_BIT_MASK(64))) {
 		xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
 		dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
+	} else {
+		/*
+		 * This is to avoid error in cases where a 32-bit USB
+		 * controller is used on a 64-bit capable system.
+		 */
+		retval = dma_set_mask(dev, DMA_BIT_MASK(32));
+		if (retval)
+			return retval;
+		xhci_dbg(xhci, "Enabling 32-bit DMA addresses.\n");
+		dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
 	}
 
 	xhci_dbg(xhci, "Calling HCD init\n");
-- 
1.9.1

[PATCH v8 0/3] usb: xhci-platform: Configure 64-bit DMA mask if the platform is capable

[Duc Dang]

On Thu, Sep 17, 2015 at 11:19 AM, Duc Dang <dhdang@apm.com> wrote:
> The xhci platform driver does not work with system that only supports
> 64-bit DMA as it requests 32-bit DMA mask during driver initialization.
> This patch set addresses this issue and also adds XHCI-compliant USB
> Controller ACPI identification into xhci-platform driver.

Hi Greg, Mathias,

Arnd already ack-ed the first patch, please let me know if you have
more comment on this set?

>
> Changes from v7:
>         - Only use dma_coerce_mask_and_coherent when
>         dma_mask is NULL
>         - Check the controller DMA capability and configure
>         32-bit dma_mask if it only supports 32-bit DMA
>         - Patches is generated over v4.3-rc1
>
> Changes from v6:
>         -Add WARN_ON if dma_mask is NULL
>         -Use dma_coerce_mask_and_coherent to assign
>         dma_mask and coherent_dma_mask
>
> Change from v5:
>         -Change comment to "XHCI-compliant USB Controller" as
>         "PNP0D10" ID is not X-Gene specific
>         -Change comment
>         -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
>         to make sure dma_set_mask_and_coherent does not fail prematurely.
>
> Changes from v4:
>         -Remove #ifdef CONFIG_ACPI
>         -Change comment
>         -Assign dma_mask to coherent_dma_mask if dma_mask is NULL
>         to make sure dma_set_mask_and_coherent does not fail prematurely.
>
> Changes from v3:
>         -Regenerate the patch over 4.2-rc5
>         -No code change
>
> Changes from v2
>         -Replaced tristate with a boolean as the driver doesn't
>                 compile as a module
>         -Correct --help-- to ---help---
>
> Changes from v1
>         -Consolidated to use dma_set_mask_and_coherent
>         -Got rid of the check against sizeof(dma_addr_t)
>         -Renamed from "add support for APM X-Gene to xhci-platform"
>         -Removed changes to arm64/Kconfig
>         -Made CONFIG_USB_XHCI_PLATFORM a user selectable config option
>
>  drivers/usb/host/xhci-plat.c | 29 ++++++++++++++++++++++-------
>  drivers/usb/host/xhci.c      | 10 ++++++++++
>  2 files changed, 32 insertions(+), 7 deletions(-)
>
> --
> 1.9.1
>

Regards,
Duc Dang.

[PATCH v8 0/3] usb: xhci-platform: Configure 64-bit DMA mask if the platform is capable

[Greg KH]

On Wed, Sep 30, 2015 at 02:24:30PM -0700, Duc Dang wrote:
> On Thu, Sep 17, 2015 at 11:19 AM, Duc Dang <dhdang@apm.com> wrote:
> > The xhci platform driver does not work with system that only supports
> > 64-bit DMA as it requests 32-bit DMA mask during driver initialization.
> > This patch set addresses this issue and also adds XHCI-compliant USB
> > Controller ACPI identification into xhci-platform driver.
> 
> Hi Greg, Mathias,
> 
> Arnd already ack-ed the first patch, please let me know if you have
> more comment on this set?

I'm waiting for Mathias to forward this on to me as he's the xhci
maintainer.

thanks,

greg k-h

[PATCH v8 0/3] usb: xhci-platform: Configure 64-bit DMA mask if the platform is capable

[Mathias Nyman]

On 04.10.2015 12:10, Greg KH wrote:
> On Wed, Sep 30, 2015 at 02:24:30PM -0700, Duc Dang wrote:
>> On Thu, Sep 17, 2015 at 11:19 AM, Duc Dang <dhdang@apm.com> wrote:
>>> The xhci platform driver does not work with system that only supports
>>> 64-bit DMA as it requests 32-bit DMA mask during driver initialization.
>>> This patch set addresses this issue and also adds XHCI-compliant USB
>>> Controller ACPI identification into xhci-platform driver.
>>
>> Hi Greg, Mathias,
>>
>> Arnd already ack-ed the first patch, please let me know if you have
>> more comment on this set?
>
> I'm waiting for Mathias to forward this on to me as he's the xhci
> maintainer.
>

I got them waiting in my queue, just need to sort out some other unrelated patches
before passing everything forward.

-Mathias

[PATCH v6 1/2] usb: make xhci platform driver use 64 bit or 32 bit DMA

[Duc Dang]

The xhci platform driver needs to work on systems that
either only support 64-bit DMA or only support 32-bit DMA.
Attempt to set a coherent dma mask for 64-bit DMA, and
attempt again with 32-bit DMA if that fails.

[dhdang: regenerate the patch over 4.2-rc5 and address new comments]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Tested-by: Mark Salter <msalter@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
Changes from v5:
	-Change comment
	-Assign dma_mask to coherent_dma_mask if dma_mask is NULL
	to make sure dma_set_mask_and_coherent does not	fail prematurely.

Changes from v4:
        -None

Changes from v3:
        -Re-generate the patch over 4.2-rc5
        -No code change.

Changes from v2:
        -None

Changes from v1:
        -Consolidated to use dma_set_mask_and_coherent
        -Got rid of the check against sizeof(dma_addr_t)

 drivers/usb/host/xhci-plat.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 890ad9d..5d1b84b 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -93,14 +93,18 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENODEV;
 
-	/* Initialize dma_mask and coherent_dma_mask to 32-bits */
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
+	/*
+	 * Try setting dma_mask and coherent_dma_mask to 64 bits,
+	 * then try 32 bits
+	 */
 	if (!pdev->dev.dma_mask)
 		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
-	else
-		dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (ret) {
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+		if (ret)
+			return ret;
+	}
 
 	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
 	if (!hcd)
-- 
1.9.1

[PATCH v6 2/2] usb: Add support for ACPI identification to xhci-platform

[Duc Dang]

Provide the methods to let ACPI identify the need to use
xhci-platform. Change the Kconfig files so the
xhci-plat.o file is selectable during kernel config.

This has been tested on an ARM64 machine with platform XHCI, an
x86_64 machine with XHCI, and an x86_64 machine without XHCI.
There were no regressions or error messages on the machines
without platform XHCI.

[dhdang: regenerate the patch over 4.2-rc5 and address new comments]
Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Signed-off-by: Duc Dang <dhdang@apm.com>

---
Change from v5:
	Change comment to "XHCI-compliant USB Controller" as
	"PNP0D10" ID is not X-Gene specific

Changes from v4:
        Remove #ifdef CONFIG_ACPI

Changes from v3:
        Regenerate the patch over 4.2-rc5
        No code change

Changes from v2
        Replaced tristate with a boolean as the driver doesn't
                compile as a module
        Correct --help-- to ---help---

Changes from v1
        Renamed from "add support for APM X-Gene to xhci-platform"
        Removed changes to arm64/Kconfig
        Made CONFIG_USB_XHCI_PLATFORM a user selectable config option

 drivers/usb/host/Kconfig     | 7 ++++++-
 drivers/usb/host/xhci-plat.c | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 8afc3c1..96231ee 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -32,7 +32,12 @@ config USB_XHCI_PCI
        default y
 
 config USB_XHCI_PLATFORM
-	tristate
+	tristate "xHCI platform driver support"
+	---help---
+	  Say 'Y' to enable the support for the xHCI host controller
+	  as a platform device. Many ARM SoCs provide USB this way.
+
+	  If unsure, say 'Y'.
 
 config USB_XHCI_MVEBU
 	tristate "xHCI support for Marvell Armada 375/38x"
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 5d1b84b..7ec2d31 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -19,6 +19,7 @@
 #include <linux/usb/phy.h>
 #include <linux/slab.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/acpi.h>
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
@@ -266,6 +267,13 @@ static const struct of_device_id usb_xhci_of_match[] = {
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
 #endif
 
+static const struct acpi_device_id usb_xhci_acpi_match[] = {
+	/* XHCI-compliant USB Controller */
+	{ "PNP0D10", },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match);
+
 static struct platform_driver usb_xhci_driver = {
 	.probe	= xhci_plat_probe,
 	.remove	= xhci_plat_remove,
@@ -273,6 +281,7 @@ static struct platform_driver usb_xhci_driver = {
 		.name = "xhci-hcd",
 		.pm = DEV_PM_OPS,
 		.of_match_table = of_match_ptr(usb_xhci_of_match),
+		.acpi_match_table = ACPI_PTR(usb_xhci_acpi_match),
 	},
 };
 MODULE_ALIAS("platform:xhci-hcd");
-- 
1.9.1

[PATCH v4 2/2] usb: Add support for ACPI identification to xhci-platform

[Javier Martinez Canillas]

Hello Greg,

On Sat, Aug 8, 2015 at 3:29 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Aug 07, 2015 at 06:03:36PM -0700, Duc Dang wrote:
>> Provide the methods to let ACPI identify the need to use
>> xhci-platform. Change the Kconfig files so the
>> xhci-plat.o file is selectable during kernel config.
>>
>> This has been tested on an ARM64 machine with platform XHCI, an
>> x86_64 machine with XHCI, and an x86_64 machine without XHCI.
>> There were no regressions or error messages on the machines
>> without platform XHCI.
>>
>> [dhdang: regenerate the patch over 4.2-rc5]
>> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>> Signed-off-by: Duc Dang <dhdang@apm.com>
>>
>> ---
>> Changes from v3:
>>       Regenerate the patch over 4.2-rc5
>>       No code change
>>
>> Changes from v2
>>       Replaced tristate with a boolean as the driver doesn't
>>               compile as a module
>>       Correct --help-- to ---help---
>>
>> Changes from v1
>>       Renamed from "add support for APM X-Gene to xhci-platform"
>>       Removed changes to arm64/Kconfig
>>       Made CONFIG_USB_XHCI_PLATFORM a user selectable config option
>>
>>  drivers/usb/host/Kconfig     |  7 ++++++-
>>  drivers/usb/host/xhci-plat.c | 11 +++++++++++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index 8afc3c1..96231ee 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -32,7 +32,12 @@ config USB_XHCI_PCI
>>         default y
>>
>>  config USB_XHCI_PLATFORM
>> -     tristate
>> +     tristate "xHCI platform driver support"
>> +     ---help---
>> +       Say 'Y' to enable the support for the xHCI host controller
>> +       as a platform device. Many ARM SoCs provide USB this way.
>> +
>> +       If unsure, say 'Y'.
>>
>>  config USB_XHCI_MVEBU
>>       tristate "xHCI support for Marvell Armada 375/38x"
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 5d03f8b..14b40d2 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/usb/phy.h>
>>  #include <linux/slab.h>
>>  #include <linux/usb/xhci_pdriver.h>
>> +#include <linux/acpi.h>
>>
>>  #include "xhci.h"
>>  #include "xhci-mvebu.h"
>> @@ -262,6 +263,15 @@ static const struct of_device_id usb_xhci_of_match[] = {
>>  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
>>  #endif
>>
>> +#ifdef CONFIG_ACPI
>
> You shoudn't need this #ifdef, right?
>

Why it is not needed?

The driver does .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match) and
ACPI_PTR() is NULL if CONFIG_ACPI is not enabled. Which can happen
AFAIU since the driver also supports OF. So without the #ifdef guards,
.acpi_match_table = NULL and the struct acpi_device_id
usb_xhci_acpi_match[] will be built but not used.

Or am I missing something?

> thanks,
>
> greg k-h
>

Best regards,
Javier

[PATCH v4 2/2] usb: Add support for ACPI identification to xhci-platform

[Greg KH]

On Sat, Aug 08, 2015 at 07:43:40AM +0200, Javier Martinez Canillas wrote:
> Hello Greg,
> 
> On Sat, Aug 8, 2015 at 3:29 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Aug 07, 2015 at 06:03:36PM -0700, Duc Dang wrote:
> >> Provide the methods to let ACPI identify the need to use
> >> xhci-platform. Change the Kconfig files so the
> >> xhci-plat.o file is selectable during kernel config.
> >>
> >> This has been tested on an ARM64 machine with platform XHCI, an
> >> x86_64 machine with XHCI, and an x86_64 machine without XHCI.
> >> There were no regressions or error messages on the machines
> >> without platform XHCI.
> >>
> >> [dhdang: regenerate the patch over 4.2-rc5]
> >> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
> >> Signed-off-by: Duc Dang <dhdang@apm.com>
> >>
> >> ---
> >> Changes from v3:
> >>       Regenerate the patch over 4.2-rc5
> >>       No code change
> >>
> >> Changes from v2
> >>       Replaced tristate with a boolean as the driver doesn't
> >>               compile as a module
> >>       Correct --help-- to ---help---
> >>
> >> Changes from v1
> >>       Renamed from "add support for APM X-Gene to xhci-platform"
> >>       Removed changes to arm64/Kconfig
> >>       Made CONFIG_USB_XHCI_PLATFORM a user selectable config option
> >>
> >>  drivers/usb/host/Kconfig     |  7 ++++++-
> >>  drivers/usb/host/xhci-plat.c | 11 +++++++++++
> >>  2 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> >> index 8afc3c1..96231ee 100644
> >> --- a/drivers/usb/host/Kconfig
> >> +++ b/drivers/usb/host/Kconfig
> >> @@ -32,7 +32,12 @@ config USB_XHCI_PCI
> >>         default y
> >>
> >>  config USB_XHCI_PLATFORM
> >> -     tristate
> >> +     tristate "xHCI platform driver support"
> >> +     ---help---
> >> +       Say 'Y' to enable the support for the xHCI host controller
> >> +       as a platform device. Many ARM SoCs provide USB this way.
> >> +
> >> +       If unsure, say 'Y'.
> >>
> >>  config USB_XHCI_MVEBU
> >>       tristate "xHCI support for Marvell Armada 375/38x"
> >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> >> index 5d03f8b..14b40d2 100644
> >> --- a/drivers/usb/host/xhci-plat.c
> >> +++ b/drivers/usb/host/xhci-plat.c
> >> @@ -19,6 +19,7 @@
> >>  #include <linux/usb/phy.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/usb/xhci_pdriver.h>
> >> +#include <linux/acpi.h>
> >>
> >>  #include "xhci.h"
> >>  #include "xhci-mvebu.h"
> >> @@ -262,6 +263,15 @@ static const struct of_device_id usb_xhci_of_match[] = {
> >>  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
> >>  #endif
> >>
> >> +#ifdef CONFIG_ACPI
> >
> > You shoudn't need this #ifdef, right?
> >
> 
> Why it is not needed?

Why is it needed?

> The driver does .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match) and
> ACPI_PTR() is NULL if CONFIG_ACPI is not enabled. Which can happen
> AFAIU since the driver also supports OF. So without the #ifdef guards,
> .acpi_match_table = NULL and the struct acpi_device_id
> usb_xhci_acpi_match[] will be built but not used.

Which is just fine, right?

> Or am I missing something?

Don't put #ifdef in .c files if at all possible is the kernel style
rules.

thanks,

greg k-h

[PATCH v4 2/2] usb: Add support for ACPI identification to xhci-platform

[Duc Dang]

On Sat, Aug 8, 2015 at 8:37 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Aug 08, 2015 at 07:43:40AM +0200, Javier Martinez Canillas wrote:
>> Hello Greg,
>>
>> On Sat, Aug 8, 2015 at 3:29 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Aug 07, 2015 at 06:03:36PM -0700, Duc Dang wrote:
>> >> Provide the methods to let ACPI identify the need to use
>> >> xhci-platform. Change the Kconfig files so the
>> >> xhci-plat.o file is selectable during kernel config.
>> >>
>> >> This has been tested on an ARM64 machine with platform XHCI, an
>> >> x86_64 machine with XHCI, and an x86_64 machine without XHCI.
>> >> There were no regressions or error messages on the machines
>> >> without platform XHCI.
>> >>
>> >> [dhdang: regenerate the patch over 4.2-rc5]
>> >> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>> >> Signed-off-by: Duc Dang <dhdang@apm.com>
>> >>
>> >> ---
>> >> Changes from v3:
>> >>       Regenerate the patch over 4.2-rc5
>> >>       No code change
>> >>
>> >> Changes from v2
>> >>       Replaced tristate with a boolean as the driver doesn't
>> >>               compile as a module
>> >>       Correct --help-- to ---help---
>> >>
>> >> Changes from v1
>> >>       Renamed from "add support for APM X-Gene to xhci-platform"
>> >>       Removed changes to arm64/Kconfig
>> >>       Made CONFIG_USB_XHCI_PLATFORM a user selectable config option
>> >>
>> >>  drivers/usb/host/Kconfig     |  7 ++++++-
>> >>  drivers/usb/host/xhci-plat.c | 11 +++++++++++
>> >>  2 files changed, 17 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> >> index 8afc3c1..96231ee 100644
>> >> --- a/drivers/usb/host/Kconfig
>> >> +++ b/drivers/usb/host/Kconfig
>> >> @@ -32,7 +32,12 @@ config USB_XHCI_PCI
>> >>         default y
>> >>
>> >>  config USB_XHCI_PLATFORM
>> >> -     tristate
>> >> +     tristate "xHCI platform driver support"
>> >> +     ---help---
>> >> +       Say 'Y' to enable the support for the xHCI host controller
>> >> +       as a platform device. Many ARM SoCs provide USB this way.
>> >> +
>> >> +       If unsure, say 'Y'.
>> >>
>> >>  config USB_XHCI_MVEBU
>> >>       tristate "xHCI support for Marvell Armada 375/38x"
>> >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> >> index 5d03f8b..14b40d2 100644
>> >> --- a/drivers/usb/host/xhci-plat.c
>> >> +++ b/drivers/usb/host/xhci-plat.c
>> >> @@ -19,6 +19,7 @@
>> >>  #include <linux/usb/phy.h>
>> >>  #include <linux/slab.h>
>> >>  #include <linux/usb/xhci_pdriver.h>
>> >> +#include <linux/acpi.h>
>> >>
>> >>  #include "xhci.h"
>> >>  #include "xhci-mvebu.h"
>> >> @@ -262,6 +263,15 @@ static const struct of_device_id usb_xhci_of_match[] = {
>> >>  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
>> >>  #endif
>> >>
>> >> +#ifdef CONFIG_ACPI
>> >
>> > You shoudn't need this #ifdef, right?
>> >
>>
>> Why it is not needed?
>
> Why is it needed?
>
>> The driver does .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match) and
>> ACPI_PTR() is NULL if CONFIG_ACPI is not enabled. Which can happen
>> AFAIU since the driver also supports OF. So without the #ifdef guards,
>> .acpi_match_table = NULL and the struct acpi_device_id
>> usb_xhci_acpi_match[] will be built but not used.
>
> Which is just fine, right?
>
>> Or am I missing something?
>
> Don't put #ifdef in .c files if at all possible is the kernel style
> rules.

I tested booting with both device tree and ACPI with the new code that
has #ifdef CONFIG_ACPI removed and USB works fine with my X-Gene Arm64
platform.

>
> thanks,
>
> greg k-h



-- 
Regards,
Duc Dang.

[PATCH v4 2/2] usb: Add support for ACPI identification to xhci-platform

[Javier Martinez Canillas]

Hello Greg,

On Sat, Aug 8, 2015 at 5:37 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sat, Aug 08, 2015 at 07:43:40AM +0200, Javier Martinez Canillas wrote:
>> Hello Greg,
>>
>> On Sat, Aug 8, 2015 at 3:29 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Aug 07, 2015 at 06:03:36PM -0700, Duc Dang wrote:
>> >> Provide the methods to let ACPI identify the need to use
>> >> xhci-platform. Change the Kconfig files so the
>> >> xhci-plat.o file is selectable during kernel config.
>> >>
>> >> This has been tested on an ARM64 machine with platform XHCI, an
>> >> x86_64 machine with XHCI, and an x86_64 machine without XHCI.
>> >> There were no regressions or error messages on the machines
>> >> without platform XHCI.
>> >>
>> >> [dhdang: regenerate the patch over 4.2-rc5]
>> >> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>> >> Signed-off-by: Duc Dang <dhdang@apm.com>
>> >>
>> >> ---
>> >> Changes from v3:
>> >>       Regenerate the patch over 4.2-rc5
>> >>       No code change
>> >>
>> >> Changes from v2
>> >>       Replaced tristate with a boolean as the driver doesn't
>> >>               compile as a module
>> >>       Correct --help-- to ---help---
>> >>
>> >> Changes from v1
>> >>       Renamed from "add support for APM X-Gene to xhci-platform"
>> >>       Removed changes to arm64/Kconfig
>> >>       Made CONFIG_USB_XHCI_PLATFORM a user selectable config option
>> >>
>> >>  drivers/usb/host/Kconfig     |  7 ++++++-
>> >>  drivers/usb/host/xhci-plat.c | 11 +++++++++++
>> >>  2 files changed, 17 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> >> index 8afc3c1..96231ee 100644
>> >> --- a/drivers/usb/host/Kconfig
>> >> +++ b/drivers/usb/host/Kconfig
>> >> @@ -32,7 +32,12 @@ config USB_XHCI_PCI
>> >>         default y
>> >>
>> >>  config USB_XHCI_PLATFORM
>> >> -     tristate
>> >> +     tristate "xHCI platform driver support"
>> >> +     ---help---
>> >> +       Say 'Y' to enable the support for the xHCI host controller
>> >> +       as a platform device. Many ARM SoCs provide USB this way.
>> >> +
>> >> +       If unsure, say 'Y'.
>> >>
>> >>  config USB_XHCI_MVEBU
>> >>       tristate "xHCI support for Marvell Armada 375/38x"
>> >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> >> index 5d03f8b..14b40d2 100644
>> >> --- a/drivers/usb/host/xhci-plat.c
>> >> +++ b/drivers/usb/host/xhci-plat.c
>> >> @@ -19,6 +19,7 @@
>> >>  #include <linux/usb/phy.h>
>> >>  #include <linux/slab.h>
>> >>  #include <linux/usb/xhci_pdriver.h>
>> >> +#include <linux/acpi.h>
>> >>
>> >>  #include "xhci.h"
>> >>  #include "xhci-mvebu.h"
>> >> @@ -262,6 +263,15 @@ static const struct of_device_id usb_xhci_of_match[] = {
>> >>  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
>> >>  #endif
>> >>
>> >> +#ifdef CONFIG_ACPI
>> >
>> > You shoudn't need this #ifdef, right?
>> >
>>
>> Why it is not needed?
>
> Why is it needed?
>

As explained, to have avoid having an unused variable.

>> The driver does .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match) and
>> ACPI_PTR() is NULL if CONFIG_ACPI is not enabled. Which can happen
>> AFAIU since the driver also supports OF. So without the #ifdef guards,
>> .acpi_match_table = NULL and the struct acpi_device_id
>> usb_xhci_acpi_match[] will be built but not used.
>
> Which is just fine, right?
>

I've seen people having different opinions about this specific case
(using #ifdef guards for ACPI, OF, etc match tables definition),
that's why I asked.

>> Or am I missing something?
>
> Don't put #ifdef in .c files if at all possible is the kernel style
> rules.
>

I know but as you said the rule is to not have #ifdef if possible. But
I understand now that for you this case doesn't justify the #ifdefery.

> thanks,
>
> greg k-h

Best regards,
Javier