[usb] add support for APM X-Gene to xhci-platform

[usb] add support for APM X-Gene to xhci-platform

[Mark Langsdorf]

Applied Micro's X-Gene platform uses ARM64 processors and a
standard, XHCI compatible piece of silicon for USB connectivity.
It only supports 64 bit DMA. Modify the xhci platform driver to
use either 32 bit or 64 bit DMA, as appropriate, and then
modify the platform driver to recognize the ACPI signature of
the silicon.

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

[Mark Langsdorf]

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.

Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
---
 drivers/usb/host/xhci-plat.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 1a0cf9f..3045e77 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -147,14 +147,17 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	/* 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 the coherent_dma_mask to 64 bits, then try 32 bits */
+	if (sizeof(dma_addr_t) < 8 ||
+		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
+		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));
+		dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
 
 	hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
 	if (!hcd)
-- 
1.9.3

[PATCH 2/2] [usb] add support for APM X-Gene to xhci-platform

[Mark Langsdorf]

Applied Micro's X-Gene platform uses the xhci-platform for USB.
Add the glue to decode it from ACPI and change the Kconfig files
so the xhci-plat.o file gets built.

Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
---
 arch/arm64/Kconfig           |  1 +
 drivers/usb/host/Kconfig     |  8 ++++++++
 drivers/usb/host/xhci-plat.c | 11 +++++++++++
 3 files changed, 20 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 042f785..07549ec 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -149,6 +149,7 @@ config ARCH_XGENE
 	bool "AppliedMicro X-Gene SOC Family"
 	select MFD_SYSCON
 	select POWER_RESET_SYSCON
+	select USB_XHCI_XGENE
 	help
 	  This enables support for AppliedMicro X-Gene SOC Family
 
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 82800a7..9fd52db 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -45,6 +45,14 @@ config USB_XHCI_RCAR
 	  Say 'Y' to enable the support for the xHCI host controller
 	  found in Renesas R-Car ARM SoCs.
 
+config USB_XHCI_XGENE
+	tristate "xHCI support for Applied Micro X-Gene SoCs"
+	select USB_XHCI_PLATFORM
+	depends on ARCH_XGENE || COMPILE_TEST
+	---help---
+	  Say 'Y' to enable the support for the xHCI host controller
+	   found in Applied Micro X-Gene ARM SoCs.
+
 endif # USB_XHCI_HCD
 
 config USB_EHCI_HCD
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 3045e77..5012c68 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/acpi.h>
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
@@ -290,6 +291,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,
@@ -297,6 +307,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.3

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

[Arnd Bergmann]

On Thursday 30 October 2014 13:16:28 Mark Langsdorf wrote:
> -       /* 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 the coherent_dma_mask to 64 bits, then try 32 bits */
> +       if (sizeof(dma_addr_t) < 8 ||
> +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> +               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));
> +               dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
>  
>         hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>         if (!hcd)
> 

The logic here seems wrong: if dma_set_mask is successful, you
can rely on on dma_set_coherent_mask suceeding as well, but
not the other way round.

Also, we should no longer need to worry about the case where
pdev->dev.dma_mask is NULL, as this now gets initialized from
the DT setup.

	Arnd

[PATCH 2/2] [usb] add support for APM X-Gene to xhci-platform

[Arnd Bergmann]

On Thursday 30 October 2014 13:16:29 Mark Langsdorf wrote:
> Applied Micro's X-Gene platform uses the xhci-platform for USB.
> Add the glue to decode it from ACPI and change the Kconfig files
> so the xhci-plat.o file gets built.
> 
> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>

I don't see anything x-gene specific in this patch, the PNP0D10
ID should work on any platform.

> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 042f785..07549ec 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -149,6 +149,7 @@ config ARCH_XGENE
>  	bool "AppliedMicro X-Gene SOC Family"
>  	select MFD_SYSCON
>  	select POWER_RESET_SYSCON
> +	select USB_XHCI_XGENE
>  	help
>  	  This enables support for AppliedMicro X-Gene SOC Family
>  
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 82800a7..9fd52db 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -45,6 +45,14 @@ config USB_XHCI_RCAR
>  	  Say 'Y' to enable the support for the xHCI host controller
>  	  found in Renesas R-Car ARM SoCs.
>  
> +config USB_XHCI_XGENE
> +	tristate "xHCI support for Applied Micro X-Gene SoCs"
> +	select USB_XHCI_PLATFORM
> +	depends on ARCH_XGENE || COMPILE_TEST
> +	---help---
> +	  Say 'Y' to enable the support for the xHCI host controller
> +	   found in Applied Micro X-Gene ARM SoCs.
> +

So just remove this symbol and let the user select USB_XHCI_PLATFORM

> @@ -290,6 +291,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,
> @@ -297,6 +307,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");

This change looks good, just the comment is a bit misleading.

	Arnd

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

[Mark Langsdorf]

On 10/30/2014 02:05 PM, Arnd Bergmann wrote:
> On Thursday 30 October 2014 13:16:28 Mark Langsdorf wrote:
>> -       /* 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 the coherent_dma_mask to 64 bits, then try 32 bits */
>> +       if (sizeof(dma_addr_t) < 8 ||
>> +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
>> +               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));
>> +               dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
>>
>>          hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>>          if (!hcd)
>>
>
> The logic here seems wrong: if dma_set_mask is successful, you
> can rely on on dma_set_coherent_mask suceeding as well, but
> not the other way round.

That's the order in the existing driver. Would you prefer I
switch it to:
	if (sizeof(dma_addr_t) < 8 ||
		dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
			ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
			if (ret)
				return ret;
	}
	dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);

or based on the comment below:
	ret = dma_set_mask(&pdev->dev, pdev->dev.dma_mask);
	if (ret)
		return ret;
	dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);

I prefer this version but I don't know if it would work.

> Also, we should no longer need to worry about the case where
> pdev->dev.dma_mask is NULL, as this now gets initialized from
> the DT setup.

I'm running this on a system with ACPI enabled and no DT. Does
that make a difference?

--Mark Langsdorf

[PATCH 2/2] [usb] add support for APM X-Gene to xhci-platform

[Mark Langsdorf]

On 10/30/2014 02:07 PM, Arnd Bergmann wrote:
> On Thursday 30 October 2014 13:16:29 Mark Langsdorf wrote:
>> Applied Micro's X-Gene platform uses the xhci-platform for USB.
>> Add the glue to decode it from ACPI and change the Kconfig files
>> so the xhci-plat.o file gets built.
>>
>> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>
> I don't see anything x-gene specific in this patch, the PNP0D10
> ID should work on any platform.

Okay.

>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 042f785..07549ec 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -149,6 +149,7 @@ config ARCH_XGENE
>>   	bool "AppliedMicro X-Gene SOC Family"
>>   	select MFD_SYSCON
>>   	select POWER_RESET_SYSCON
>> +	select USB_XHCI_XGENE
>>   	help
>>   	  This enables support for AppliedMicro X-Gene SOC Family
>>
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index 82800a7..9fd52db 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -45,6 +45,14 @@ config USB_XHCI_RCAR
>>   	  Say 'Y' to enable the support for the xHCI host controller
>>   	  found in Renesas R-Car ARM SoCs.
>>
>> +config USB_XHCI_XGENE
>> +	tristate "xHCI support for Applied Micro X-Gene SoCs"
>> +	select USB_XHCI_PLATFORM
>> +	depends on ARCH_XGENE || COMPILE_TEST
>> +	---help---
>> +	  Say 'Y' to enable the support for the xHCI host controller
>> +	   found in Applied Micro X-Gene ARM SoCs.
>> +
>
> So just remove this symbol and let the user select USB_XHCI_PLATFORM

USB_XHCI_PLATFORM isn't currently user selectable since it doesn't
have a string after the tristate. It gets automatically selected by
USB_XHCI_RCAR and USB_XHCI_MVEBU and I was continuing the pattern.
Do you want me to make it user selectable instead?

>> @@ -290,6 +291,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,
>> @@ -297,6 +307,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");
>
> This change looks good, just the comment is a bit misleading.

I'll change the comment in the resubmit.

--Mark Langsdorf

[PATCH 2/2] [usb] add support for APM X-Gene to xhci-platform

[Arnd Bergmann]

On Thursday 30 October 2014 15:12:37 Mark Langsdorf wrote:
> > So just remove this symbol and let the user select USB_XHCI_PLATFORM
> 
> USB_XHCI_PLATFORM isn't currently user selectable since it doesn't
> have a string after the tristate. It gets automatically selected by
> USB_XHCI_RCAR and USB_XHCI_MVEBU and I was continuing the pattern.
> Do you want me to make it user selectable instead?

Yes, please do.

The difference is that the two you mentioned have separate driver files
that you may or may not want to have built into your kernel. The APM
support apparently doesn't need any of that and just works with the
common parts of the driver. I expect to see more of those in the
future.

	Arnd

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

[Arnd Bergmann]

On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
> On 10/30/2014 02:05 PM, Arnd Bergmann wrote:
> > On Thursday 30 October 2014 13:16:28 Mark Langsdorf wrote:
> >> -       /* 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 the coherent_dma_mask to 64 bits, then try 32 bits */
> >> +       if (sizeof(dma_addr_t) < 8 ||
> >> +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> >> +               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));
> >> +               dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
> >>
> >>          hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
> >>          if (!hcd)
> >>
> >
> > The logic here seems wrong: if dma_set_mask is successful, you
> > can rely on on dma_set_coherent_mask suceeding as well, but
> > not the other way round.
> 
> That's the order in the existing driver. Would you prefer I
> switch it to:
> 	if (sizeof(dma_addr_t) < 8 ||
> 		dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
> 			ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> 			if (ret)
> 				return ret;
> 	}
> 	dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
> 
> or based on the comment below:
> 	ret = dma_set_mask(&pdev->dev, pdev->dev.dma_mask);
> 	if (ret)
> 		return ret;
> 	dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
> 
> I prefer this version but I don't know if it would work.

You should not access pdev->dev.dma_mask here, that gets set
by the platform code. You should be able to just use
dma_set_mask_and_coherent to set both.

> > Also, we should no longer need to worry about the case where
> > pdev->dev.dma_mask is NULL, as this now gets initialized from
> > the DT setup.
> 
> I'm running this on a system with ACPI enabled and no DT. Does
> that make a difference?

I don't know how the DMA mask gets initialized on ACPI, I assume it
doesn't at the moment, but that is something that should be fixed
in the ACPI code, not worked around in the driver.

You should definitely make sure that this also works with DT, as
I don't think it's possible to support X-Gene with ACPI. I know
that Al Stone has experimented with it in the past, but he never
came back with any results, so I assume the experiment failed.

Note that the discussions about merging ACPI support on ARM64
are based on the assumption that we'd only ever support SBSA-like
platforms, not something like X-Gene that looks more like an
embedded SoC. Your XHCI patches still obviously make sense for
other platforms, so that's not a show-stopper.

	Arnd

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

[Mark Langsdorf]

On 10/30/2014 04:05 PM, Arnd Bergmann wrote:
> On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
>> On 10/30/2014 02:05 PM, Arnd Bergmann wrote:
>>> On Thursday 30 October 2014 13:16:28 Mark Langsdorf wrote:
>>>> -       /* 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 the coherent_dma_mask to 64 bits, then try 32 bits */
>>>> +       if (sizeof(dma_addr_t) < 8 ||
>>>> +               dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64))) {
>>>> +               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));
>>>> +               dma_set_mask(&pdev->dev, pdev->dev.coherent_dma_mask);
>>>>
>>>>           hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>>>>           if (!hcd)
>>>>
>>>
>>> The logic here seems wrong: if dma_set_mask is successful, you
>>> can rely on on dma_set_coherent_mask suceeding as well, but
>>> not the other way round.
>>
>> That's the order in the existing driver. Would you prefer I
>> switch it to:
>> 	if (sizeof(dma_addr_t) < 8 ||
>> 		dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) {
>> 			ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>> 			if (ret)
>> 				return ret;
>> 	}
>> 	dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
>>
>> or based on the comment below:
>> 	ret = dma_set_mask(&pdev->dev, pdev->dev.dma_mask);
>> 	if (ret)
>> 		return ret;
>> 	dma_set_coherent_mask(&pdev->dev, pdev->dev.dma_mask);
>>
>> I prefer this version but I don't know if it would work.
>
> You should not access pdev->dev.dma_mask here, that gets set
> by the platform code. You should be able to just use
> dma_set_mask_and_coherent to set both.

So:

  	if (sizeof(dma_addr_t) < 8 ||
  		dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
  		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
  		if (ret)
  			return ret;
  	}

This doesn't actually work for me. I experimented a bit on the
hardware and I always fail if I don't set the coherent mask
first.

>>> Also, we should no longer need to worry about the case where
>>> pdev->dev.dma_mask is NULL, as this now gets initialized from
>>> the DT setup.
>>
>> I'm running this on a system with ACPI enabled and no DT. Does
>> that make a difference?
>
> I don't know how the DMA mask gets initialized on ACPI, I assume it
> doesn't at the moment, but that is something that should be fixed
> in the ACPI code, not worked around in the driver.
>
> You should definitely make sure that this also works with DT, as
> I don't think it's possible to support X-Gene with ACPI. I know
> that Al Stone has experimented with it in the past, but he never
> came back with any results, so I assume the experiment failed.

I'm running my test code on an X-Gene with ACPI. Al Stone, Mark
Salter, and I got it working.

--Mark Langsdorf

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

[Arnd Bergmann]

On Friday 31 October 2014 09:22:26 Mark Langsdorf wrote:
> On 10/30/2014 04:05 PM, Arnd Bergmann wrote:
> > On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
> >
> > You should not access pdev->dev.dma_mask here, that gets set
> > by the platform code. You should be able to just use
> > dma_set_mask_and_coherent to set both.
> 
> So:
> 
>         if (sizeof(dma_addr_t) < 8 ||
>                 dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
>                 ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>                 if (ret)
>                         return ret;
>         }
> 
> This doesn't actually work for me. I experimented a bit on the
> hardware and I always fail if I don't set the coherent mask
> first.

Very strange, the code looks right to me. What is the initial value
of dev->dma_mask?

> >>> Also, we should no longer need to worry about the case where
> >>> pdev->dev.dma_mask is NULL, as this now gets initialized from
> >>> the DT setup.
> >>
> >> I'm running this on a system with ACPI enabled and no DT. Does
> >> that make a difference?
> >
> > I don't know how the DMA mask gets initialized on ACPI, I assume it
> > doesn't at the moment, but that is something that should be fixed
> > in the ACPI code, not worked around in the driver.
> >
> > You should definitely make sure that this also works with DT, as
> > I don't think it's possible to support X-Gene with ACPI. I know
> > that Al Stone has experimented with it in the past, but he never
> > came back with any results, so I assume the experiment failed.
> 
> I'm running my test code on an X-Gene with ACPI. Al Stone, Mark
> Salter, and I got it working.

The question is whether that is in a form that we could merge upstream.
I haven't seen any patches being posted, so I can't know for sure, but
from all I know about the hardware it doesn't seem likely, unless
you leave out all the interesting bits such as power mangement, PCI
and networking.

	Arnd

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

[Mark Langsdorf]

On 10/31/2014 10:49 AM, Arnd Bergmann wrote:
> On Friday 31 October 2014 09:22:26 Mark Langsdorf wrote:
>> On 10/30/2014 04:05 PM, Arnd Bergmann wrote:
>>> On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
>>>
>>> You should not access pdev->dev.dma_mask here, that gets set
>>> by the platform code. You should be able to just use
>>> dma_set_mask_and_coherent to set both.
>>
>> So:
>>
>>          if (sizeof(dma_addr_t) < 8 ||
>>                  dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
>>                  ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>>                  if (ret)
>>                          return ret;
>>          }
>>
>> This doesn't actually work for me. I experimented a bit on the
>> hardware and I always fail if I don't set the coherent mask
>> first.
>
> Very strange, the code looks right to me. What is the initial value
> of dev->dma_mask?

Did you mean &pdev->dev.dma_mask? It's 0xdc759df8.

--Mark Langsdorf

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

[Arnd Bergmann]

On Friday 31 October 2014 12:32:47 Mark Langsdorf wrote:
> On 10/31/2014 10:49 AM, Arnd Bergmann wrote:
> > On Friday 31 October 2014 09:22:26 Mark Langsdorf wrote:
> >> On 10/30/2014 04:05 PM, Arnd Bergmann wrote:
> >>> On Thursday 30 October 2014 15:09:33 Mark Langsdorf wrote:
> >>>
> >>> You should not access pdev->dev.dma_mask here, that gets set
> >>> by the platform code. You should be able to just use
> >>> dma_set_mask_and_coherent to set both.
> >>
> >> So:
> >>
> >>          if (sizeof(dma_addr_t) < 8 ||
> >>                  dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64))) {
> >>                  ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> >>                  if (ret)
> >>                          return ret;
> >>          }
> >>
> >> This doesn't actually work for me. I experimented a bit on the
> >> hardware and I always fail if I don't set the coherent mask
> >> first.
> >
> > Very strange, the code looks right to me. What is the initial value
> > of dev->dma_mask?
> 
> Did you mean &pdev->dev.dma_mask? It's 0xdc759df8.

No, that would be the pointer to the pointer to the dma mask ;-)

I meant the DMA mask that was set by the platform code in
*pdev->dev.dma_mask. It would also be interesting to know where
pdev->dev.dma_mask points to. Does ACPI allocate memory for that,
or does it point to the coherent mask?

	Arnd

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

[Mark Salter]

On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote:
> You should definitely make sure that this also works with DT, as
> I don't think it's possible to support X-Gene with ACPI. I know
> that Al Stone has experimented with it in the past, but he never
> came back with any results, so I assume the experiment failed.
> 
> Note that the discussions about merging ACPI support on ARM64
> are based on the assumption that we'd only ever support SBSA-like
> platforms, not something like X-Gene that looks more like an
> embedded SoC. Your XHCI patches still obviously make sense for
> other platforms, so that's not a show-stopper.

But for some misconfiguration, the arm64 kernels in fedora arm
koji would boot using ACPI on Mustang, the Foundation model,
and AMD Seattle platforms. All very much a work in progress,
but the tree from which the fedora patches are taken is the
devel branch of:

  git.fedorahosted.org/git/kernel-arm64.git

The configuration will be fixed this week and then you can
just grab an arm64 fedora kernel and boot with acpi=force.

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

[Arnd Bergmann]

On Monday 03 November 2014 09:15:51 Mark Salter wrote:
> On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote:
> > You should definitely make sure that this also works with DT, as
> > I don't think it's possible to support X-Gene with ACPI. I know
> > that Al Stone has experimented with it in the past, but he never
> > came back with any results, so I assume the experiment failed.
> > 
> > Note that the discussions about merging ACPI support on ARM64
> > are based on the assumption that we'd only ever support SBSA-like
> > platforms, not something like X-Gene that looks more like an
> > embedded SoC. Your XHCI patches still obviously make sense for
> > other platforms, so that's not a show-stopper.
> 
> But for some misconfiguration, the arm64 kernels in fedora arm
> koji would boot using ACPI on Mustang, the Foundation model,
> and AMD Seattle platforms. All very much a work in progress,
> but the tree from which the fedora patches are taken is the
> devel branch of:
> 
>   git.fedorahosted.org/git/kernel-arm64.git
> 
> The configuration will be fixed this week and then you can
> just grab an arm64 fedora kernel and boot with acpi=force.

It's not as bad as I had feared, but it still does a lot of
the things that in previous discussions were said that ACPI
would avoid doing, and that were used as arguments against
just using DT on all arm64 servers:

- The use of the device properties API that was introduced for
  Intel's embedded devices for on-chip components directly
  contradicts the "standardized firmware interfaces" argument
  that certain people keep making. This certainly explains how
  you plan to use the networking side, but I suspect it's not
  what the ACPI proponents were looking for, or what the base
  patch set is being advertised as.

- Using custom drivers for hardware that is required by SBSA
  (ahci, pci, ...) means you are contradicting the 
  Documentation/arm64/arm-acpi.txt document that is merged as
  one of your early patches and that keeps getting posted as
  the base of discussion for the inclusion of the base ACPI
  support. I don't think you can have it both ways, saying that
  SBSA is required and supporting hardware that doesn't do any
  of it won't work.

- Adding a generalized method to support arbitrary PCI host
  controllers indicates that you expect this practice to not
  just be a special exception for APM but that others would
  require the same hack to work around firmware or hardware that
  is not compliant with ACPI. At the same time you introduce a
  significant of driver code in arch/arm64/kernel/pci.c. Some
  of that code is probably just an oversight and can be moved into
  the PCI core later, but it doesn't even seem you tried that
  hard.

All of the above are probably things we can discuss, but by working on
this in secret until now, you have really made it hard for the Linaro
team whose work you are basing on. They have tried hard for a long
time to get basic ACPI support into a mergeable state, and have been
working on incorrect base assumptions the whole time because they
didn't have a platform implementation to show and to verify their
assumptions.

	Arnd

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

[Mark Salter]

On Mon, 2014-11-03 at 18:14 +0100, Arnd Bergmann wrote:
> On Monday 03 November 2014 09:15:51 Mark Salter wrote:
> > On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote:
> > > You should definitely make sure that this also works with DT, as
> > > I don't think it's possible to support X-Gene with ACPI. I know
> > > that Al Stone has experimented with it in the past, but he never
> > > came back with any results, so I assume the experiment failed.
> > > 
> > > Note that the discussions about merging ACPI support on ARM64
> > > are based on the assumption that we'd only ever support SBSA-like
> > > platforms, not something like X-Gene that looks more like an
> > > embedded SoC. Your XHCI patches still obviously make sense for
> > > other platforms, so that's not a show-stopper.
> > 
> > But for some misconfiguration, the arm64 kernels in fedora arm
> > koji would boot using ACPI on Mustang, the Foundation model,
> > and AMD Seattle platforms. All very much a work in progress,
> > but the tree from which the fedora patches are taken is the
> > devel branch of:
> > 
> >   git.fedorahosted.org/git/kernel-arm64.git
> > 
> > The configuration will be fixed this week and then you can
> > just grab an arm64 fedora kernel and boot with acpi=force.
> 
> It's not as bad as I had feared, but it still does a lot of
> the things that in previous discussions were said that ACPI
> would avoid doing, and that were used as arguments against
> just using DT on all arm64 servers:

As I said, this is very much a work in progress. The point of the
fedorahosted tree is to have *something* that will boot with ACPI
and have working disk and network as a minimum for the few existing
early platforms. It is certainly not an example of what an ACPI/SBSA
server should look like. At least not yet. And it isn't a big secret.
Linaro, Arm, and other folk are aware of that fedorahosted tree. I
wouldn't read too much into any given patch in that tree as it stands
right now.

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

[Mark Langsdorf]

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.

Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
Tested-by: Mark Salter <msalter@redhat.com>
---
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 1a0cf9f..91c7557 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -147,14 +147,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	/* 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.3

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Mark Langsdorf]

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.

Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
---
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 82800a7..060a2361 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -27,7 +27,12 @@ config USB_XHCI_HCD
 if USB_XHCI_HCD
 
 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 91c7557..3db47ea 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/acpi.h>
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
@@ -287,6 +288,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,
@@ -294,6 +304,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.3

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Greg KH]

On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf 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.
> 
> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
> ---
> 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 82800a7..060a2361 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -27,7 +27,12 @@ config USB_XHCI_HCD
>  if USB_XHCI_HCD
>  
>  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'.

You really want a 'default Y' response here?

That's not good at all, what happens if I select this on a system
without such hardware?


>  
>  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 91c7557..3db47ea 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -18,6 +18,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/usb/xhci_pdriver.h>
> +#include <linux/acpi.h>
>  
>  #include "xhci.h"
>  #include "xhci-mvebu.h"
> @@ -287,6 +288,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

That looks like a very "generic" PNP value, are you sure it is assigned
only to this specific device?

> +
>  static struct platform_driver usb_xhci_driver = {
>  	.probe	= xhci_plat_probe,
>  	.remove	= xhci_plat_remove,
> @@ -294,6 +304,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),

Shouldn't the reworked driver core code handle this differently with the
ability to handle either OF or ACPI in the same driver?

thanks,

greg k-h

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

[Al Stone]

On 11/03/2014 10:14 AM, Arnd Bergmann wrote:
> On Monday 03 November 2014 09:15:51 Mark Salter wrote:
>> On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote:
>>> You should definitely make sure that this also works with DT, as
>>> I don't think it's possible to support X-Gene with ACPI. I know
>>> that Al Stone has experimented with it in the past, but he never
>>> came back with any results, so I assume the experiment failed.

My apologies; I am still remiss in making more patches public.  Life
interferes sometimes with my kernel development.  I make no excuses
for being farther behind in this than I have wanted.

<diversion type=whinge>

The experiment has not failed.  The assumption is completely wrong.
The git tree listed works on the X-Gene platform and it works on AMD
Seattle; I have both running on my desk.  Both DT and ACPI work, with
the proper versions of the DT or the ACPI tables.  I will post RFCs as
soon as I can, but my work in progress is basically the same as the
git tree mentioned below, based on slightly different starting points.

>>> Note that the discussions about merging ACPI support on ARM64
>>> are based on the assumption that we'd only ever support SBSA-like
>>> platforms, not something like X-Gene that looks more like an
>>> embedded SoC. Your XHCI patches still obviously make sense for
>>> other platforms, so that's not a show-stopper.
>>
>> But for some misconfiguration, the arm64 kernels in fedora arm
>> koji would boot using ACPI on Mustang, the Foundation model,
>> and AMD Seattle platforms. All very much a work in progress,
>> but the tree from which the fedora patches are taken is the
>> devel branch of:
>>
>>   git.fedorahosted.org/git/kernel-arm64.git
>>
>> The configuration will be fixed this week and then you can
>> just grab an arm64 fedora kernel and boot with acpi=force.

Please note that I work directly with Mark Salter and that I have
personally handed out this particular URL many times at either Linaro
Connect and/or to individuals directly.  It is not now nor has it ever
been secret, at any time.

> It's not as bad as I had feared, but it still does a lot of
> the things that in previous discussions were said that ACPI
> would avoid doing, and that were used as arguments against
> just using DT on all arm64 servers:
> 
> - The use of the device properties API that was introduced for
>   Intel's embedded devices for on-chip components directly
>   contradicts the "standardized firmware interfaces" argument
>   that certain people keep making. This certainly explains how
>   you plan to use the networking side, but I suspect it's not
>   what the ACPI proponents were looking for, or what the base
>   patch set is being advertised as.

I do not understand this statement at all.  One of the things
that was added to the ACPI 5.1 specification was the _DSD method
-- Device Specific Properties -- specifically so that device
properties could be standardized.  The API mentioned relies on the
existence of _DSD for ACPI.  Further, there are people from Linaro
(who happen to work in the kernel and ACPI), the Linux community,
as well as from Intel, ARM and even Microsoft working together to
figure out ways to standardize the device properties being used in
the ACPI Spec Working Group (ASWG) of the UEFI Forum; several of us
are of course paying attention to, participating in, and incorporating
the discussions on this kernel list and others.

So what is not being standardized?  From where I sit, as part of ASWG,
Linaro, Red Hat, and some-time contributor to the Linux kernel, this
whole device properties API was driven by the desire to have a
standardized firmware interface.  It even seems to be going a step
further and trying to standardize how the kernel interacts with any
firmware.

> - Using custom drivers for hardware that is required by SBSA
>   (ahci, pci, ...) means you are contradicting the 
>   Documentation/arm64/arm-acpi.txt document that is merged as
>   one of your early patches and that keeps getting posted as
>   the base of discussion for the inclusion of the base ACPI
>   support. I don't think you can have it both ways, saying that
>   SBSA is required and supporting hardware that doesn't do any
>   of it won't work.

This is where I start the serious whinging....

On the one hand, I'm told "show the patches."  Fine.  Someone other
than me happens to show patches for work in progress.  Or, perhaps I
show patches that work but are still proof of concept.  It doesn't
seem to matter which.  The response then looks to me like I'm being
told, "don't show patches that are not the absolute, final, irrefutable
and irrevocable version."  So which is it?

The git tree mentioned is a work in progress.  No one has ever claimed
it was otherwise.  This is exactly the same case as with the Linaro
ACPI and kernel development trees.

What I find incredibly frustrating is that I feel I am being told to
submit the final form of patches for ACPI drivers, but those can ONLY
be a work a progress until the ACPI core patches that we have been
trying to get into the kernel are accepted.  Until those core patches
are in the kernel, all I can really do is experiment with drivers and
show work in progress; there's no foundation to rely on.

I get further frustrated when I fold in basic, practical considerations.
Regardless of whether one thinks the APM or AMD hardware is an actual
arm64 server or not, it is what we have.  I cannot change that.  Some
of the hardware is not server-like; some of it just does strange and
goofy things.  These are first generation boxes; I always expect such
things to be weird.  It's just in their nature.

So, I write (or ask someone else to write, or bodge something borrowed
from someone else) a driver to experiment with, that tries to get odd
hardware working.  Or, in the case of the SBSA UART, what's available
doesn't actually work on some hardware.  Or, in some cases, the driver
based on DT is not right.  Or, the firmware (DT *or* ACPI) is not yet
complete.  My goal is just to see if I can get *something* working so
I can see what's really going on with the hardware.  Then, the patches
that work, even if ugly, are shared as work in progress.  The result
of that sharing seems to be more "go away until absolute final perfect
versions are ready."

If what I am really being told is that ACPI for arm64 will never be
accepted in the Linux kernel, then just say so.  I have other things
to do with my life than go in circles like this.

If instead there is some constructive criticism that helps improve
either the ACPI core patches or the driver patches, or better yet
allows us to make forward progress, that would be wonderful.  As far
as I know, everyone is doing the best they can with the resources
available, and making as much progress as they can without everything
being settled.

> - Adding a generalized method to support arbitrary PCI host
>   controllers indicates that you expect this practice to not
>   just be a special exception for APM but that others would
>   require the same hack to work around firmware or hardware that
>   is not compliant with ACPI. At the same time you introduce a
>   significant of driver code in arch/arm64/kernel/pci.c. Some
>   of that code is probably just an oversight and can be moved into
>   the PCI core later, but it doesn't even seem you tried that
>   hard.

APM *is* a special case because of what they do in their hardware;
perhaps that should have been stated explicitly.  By the same token,
writing kernel code that is flexible in the face of unknown future
hardware seemed reasonable at face value, but perhaps we should not
do that.  But again, this is a work in progress.  The fact that we
are having to fix x86-specific code and refactor it so that it works
on multiple architectures is quite blindingly obvious and is indeed
something that is already being worked on; perhaps that should also
have been said explicitly.  But again, we chose to share code that we
know is an early version instead of waiting until a final refactored
version is available, or instead of keeping it "secret."  Assuming we
haven't even tried or are completely unaware of the problem without
even asking I find kind of insulting.

>From where I sit, I feel I'm being told that I'm doing all the work in
secret and getting dinged for that.  So, work in progress is shared,
even though it's early and we explicitly says it's work in progress,
and now I'm getting dinged for that.

What it looks like to me is that I'm damned if I do and I'm damned if
I don't.  And in the meantime, I need to fix all the x86-isms that are
present, that probably shouldn't be there, and I didn't actually create
in the first place.

> All of the above are probably things we can discuss, but by working on
> this in secret until now, you have really made it hard for the Linaro
> team whose work you are basing on. They have tried hard for a long
> time to get basic ACPI support into a mergeable state, and have been
> working on incorrect base assumptions the whole time because they
> didn't have a platform implementation to show and to verify their
> assumptions.

The git tree that Mark mentions is one that has been public for a
few months (I don't recall how many -- well before the last Linaro
Connect).  This has never been secret; I personally have shared the
URL with people at ARM, APM, AMD, Cavium and Linaro.  This git tree
is also being used as a basis for Fedora on ARMv8, and I really don't
think of Fedora as being secretive.

And as the de facto leader of the Linaro ACPI team, I can honestly say
that there is absolutely nothing in this tree that damages any of the
work the ACPI team at Linaro has done.  Nothing.  It is the current
iteration towards a full implementation, that is all.  What's more, it
is work that is shared with Linaro through me.  I just don't find it
necessary to broadcast everything I do on a daily basis.

The primary focus of the Linaro ACPI team is on getting the ACPI core
work upstream; everything else is experimentation and finding the
problem areas -- in the kernel, in the spec, in the firmware, in the
hardware, in the development process -- so we can try to fix them
before we submit drivers that build on that core, and so we can make
progress towards a long term, supportable solution.

</diversion>

So, back to the original topic: are these USB patches ack'd or not?
I can verify that they behave on Mustang; they have been tested with
both DT and ACPI.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3 at redhat.com
-----------------------------------

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Mark Langsdorf]

On 11/04/2014 11:12 AM, Greg KH wrote:
> On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf 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.
>>
>> Signed-off-by: Mark Langsdorf <mlangsdo@redhat.com>
>> ---
>> 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 82800a7..060a2361 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -27,7 +27,12 @@ config USB_XHCI_HCD
>>   if USB_XHCI_HCD
>>
>>   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'.
>
> You really want a 'default Y' response here?
>
> That's not good at all, what happens if I select this on a system
> without such hardware?

Based on testing with my 2 x86 systems, nothing bad, but I'll make
it 'M' because that's correct.

>>   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 91c7557..3db47ea 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   #include <linux/usb/xhci_pdriver.h>
>> +#include <linux/acpi.h>
>>
>>   #include "xhci.h"
>>   #include "xhci-mvebu.h"
>> @@ -287,6 +288,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
>
> That looks like a very "generic" PNP value, are you sure it is assigned
> only to this specific device?

I'll adjust the comment. It is a generic PNP value and a lot of
other SoCs will use this controller.

>> +
>>   static struct platform_driver usb_xhci_driver = {
>>   	.probe	= xhci_plat_probe,
>>   	.remove	= xhci_plat_remove,
>> @@ -294,6 +304,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),
>
> Shouldn't the reworked driver core code handle this differently with the
> ability to handle either OF or ACPI in the same driver?

I'm not sure I understand the question. With these changes, the driver
handles both ACPI and DTB/OF. It's the same style of code as used
in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF.
Why do you think this code isn't correct?

--Mark Langsdorf

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Greg KH]

On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote:
> >>  static struct platform_driver usb_xhci_driver = {
> >>  	.probe	= xhci_plat_probe,
> >>  	.remove	= xhci_plat_remove,
> >>@@ -294,6 +304,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),
> >
> >Shouldn't the reworked driver core code handle this differently with the
> >ability to handle either OF or ACPI in the same driver?
> 
> I'm not sure I understand the question. With these changes, the driver
> handles both ACPI and DTB/OF. It's the same style of code as used
> in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF.
> Why do you think this code isn't correct?

There is a new framework in the kernel that keeps a driver from having
to query both of and acpi to get the needed resources, it just does one
query and depending on the platform, everything "just works".  Shouldn't
that be used here as well?

thanks,

greg k-h

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Mark Langsdorf]

On 11/05/2014 01:11 PM, Greg KH wrote:
> On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote:
>>>>   static struct platform_driver usb_xhci_driver = {
>>>>   	.probe	= xhci_plat_probe,
>>>>   	.remove	= xhci_plat_remove,
>>>> @@ -294,6 +304,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),
>>>
>>> Shouldn't the reworked driver core code handle this differently with the
>>> ability to handle either OF or ACPI in the same driver?
>>
>> I'm not sure I understand the question. With these changes, the driver
>> handles both ACPI and DTB/OF. It's the same style of code as used
>> in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF.
>> Why do you think this code isn't correct?
>
> There is a new framework in the kernel that keeps a driver from having
> to query both of and acpi to get the needed resources, it just does one
> query and depending on the platform, everything "just works".  Shouldn't
> that be used here as well?

Would you send me a pointer to a driver that's using this new
framework? I can't find any references to it and all the other
drivers that support ACPI and OF are doing it the way I'm doing
it.

--Mark Langsdorf

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Greg KH]

On Wed, Nov 05, 2014 at 01:44:43PM -0600, Mark Langsdorf wrote:
> On 11/05/2014 01:11 PM, Greg KH wrote:
> >On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote:
> >>>>  static struct platform_driver usb_xhci_driver = {
> >>>>  	.probe	= xhci_plat_probe,
> >>>>  	.remove	= xhci_plat_remove,
> >>>>@@ -294,6 +304,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),
> >>>
> >>>Shouldn't the reworked driver core code handle this differently with the
> >>>ability to handle either OF or ACPI in the same driver?
> >>
> >>I'm not sure I understand the question. With these changes, the driver
> >>handles both ACPI and DTB/OF. It's the same style of code as used
> >>in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF.
> >>Why do you think this code isn't correct?
> >
> >There is a new framework in the kernel that keeps a driver from having
> >to query both of and acpi to get the needed resources, it just does one
> >query and depending on the platform, everything "just works".  Shouldn't
> >that be used here as well?
> 
> Would you send me a pointer to a driver that's using this new
> framework? I can't find any references to it and all the other
> drivers that support ACPI and OF are doing it the way I'm doing
> it.

See the email on lkml:
 Subject: [PATCH v6 00/12] Add ACPI _DSD and unified device properties support

for the latest patch series.

thanks,

greg k-h

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Arnd Bergmann]

On Wednesday 05 November 2014 11:55:07 Greg KH wrote:
> On Wed, Nov 05, 2014 at 01:44:43PM -0600, Mark Langsdorf wrote:
> > On 11/05/2014 01:11 PM, Greg KH wrote:
> > >On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote:
> > >>>>  static struct platform_driver usb_xhci_driver = {
> > >>>>          .probe  = xhci_plat_probe,
> > >>>>          .remove = xhci_plat_remove,
> > >>>>@@ -294,6 +304,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),
> > >>>
> > >>>Shouldn't the reworked driver core code handle this differently with the
> > >>>ability to handle either OF or ACPI in the same driver?
> > >>
> > >>I'm not sure I understand the question. With these changes, the driver
> > >>handles both ACPI and DTB/OF. It's the same style of code as used
> > >>in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF.
> > >>Why do you think this code isn't correct?
> > >
> > >There is a new framework in the kernel that keeps a driver from having
> > >to query both of and acpi to get the needed resources, it just does one
> > >query and depending on the platform, everything "just works".  Shouldn't
> > >that be used here as well?
> > 
> > Would you send me a pointer to a driver that's using this new
> > framework? I can't find any references to it and all the other
> > drivers that support ACPI and OF are doing it the way I'm doing
> > it.
> 
> See the email on lkml:
>  Subject: [PATCH v6 00/12] Add ACPI _DSD and unified device properties support
> 
> for the latest patch series.
> 

The _DSD approach is for devices that do not follow the ACPI specification
but do have a DT binding. Those will work without the .acpi_match_table
entry when the firmware uses the compatible value in the new properties.

In this case, the device does have an official ACPI ID "PNP0D10", so we should
use that for compatibility with other operating systems and with BIOS
versions that provide the standard IDs.

	Arnd

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Greg KH]

On Wed, Nov 05, 2014 at 09:41:23PM +0100, Arnd Bergmann wrote:
> On Wednesday 05 November 2014 11:55:07 Greg KH wrote:
> > On Wed, Nov 05, 2014 at 01:44:43PM -0600, Mark Langsdorf wrote:
> > > On 11/05/2014 01:11 PM, Greg KH wrote:
> > > >On Wed, Nov 05, 2014 at 07:59:32AM -0600, Mark Langsdorf wrote:
> > > >>>>  static struct platform_driver usb_xhci_driver = {
> > > >>>>          .probe  = xhci_plat_probe,
> > > >>>>          .remove = xhci_plat_remove,
> > > >>>>@@ -294,6 +304,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),
> > > >>>
> > > >>>Shouldn't the reworked driver core code handle this differently with the
> > > >>>ability to handle either OF or ACPI in the same driver?
> > > >>
> > > >>I'm not sure I understand the question. With these changes, the driver
> > > >>handles both ACPI and DTB/OF. It's the same style of code as used
> > > >>in drivers/ata/plat-xgene.c, which also handles both ACPI and DTB/OF.
> > > >>Why do you think this code isn't correct?
> > > >
> > > >There is a new framework in the kernel that keeps a driver from having
> > > >to query both of and acpi to get the needed resources, it just does one
> > > >query and depending on the platform, everything "just works".  Shouldn't
> > > >that be used here as well?
> > > 
> > > Would you send me a pointer to a driver that's using this new
> > > framework? I can't find any references to it and all the other
> > > drivers that support ACPI and OF are doing it the way I'm doing
> > > it.
> > 
> > See the email on lkml:
> >  Subject: [PATCH v6 00/12] Add ACPI _DSD and unified device properties support
> > 
> > for the latest patch series.
> > 
> 
> The _DSD approach is for devices that do not follow the ACPI specification
> but do have a DT binding. Those will work without the .acpi_match_table
> entry when the firmware uses the compatible value in the new properties.
> 
> In this case, the device does have an official ACPI ID "PNP0D10", so we should
> use that for compatibility with other operating systems and with BIOS
> versions that provide the standard IDs.

Ah, ok, nevermind then, sorry for the noise, I wasn't aware of this.

greg k-h

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

[Arnd Bergmann]

On Tuesday 04 November 2014 10:33:18 Al Stone wrote:
> On 11/03/2014 10:14 AM, Arnd Bergmann wrote:
> > On Monday 03 November 2014 09:15:51 Mark Salter wrote:
> >> On Thu, 2014-10-30 at 22:05 +0100, Arnd Bergmann wrote:
> >>> Note that the discussions about merging ACPI support on ARM64
> >>> are based on the assumption that we'd only ever support SBSA-like
> >>> platforms, not something like X-Gene that looks more like an
> >>> embedded SoC. Your XHCI patches still obviously make sense for
> >>> other platforms, so that's not a show-stopper.
> >>
> >> But for some misconfiguration, the arm64 kernels in fedora arm
> >> koji would boot using ACPI on Mustang, the Foundation model,
> >> and AMD Seattle platforms. All very much a work in progress,
> >> but the tree from which the fedora patches are taken is the
> >> devel branch of:
> >>
> >>   git.fedorahosted.org/git/kernel-arm64.git
> >>
> >> The configuration will be fixed this week and then you can
> >> just grab an arm64 fedora kernel and boot with acpi=force.
> 
> Please note that I work directly with Mark Salter and that I have
> personally handed out this particular URL many times at either Linaro
> Connect and/or to individuals directly.  It is not now nor has it ever
> been secret, at any time.

What I meant was that the patches haven't been circulated on the
usual mailing lists when this is exactly the work that needs to
be discussed before the base patches that are being sent out can
be merged.

I was also under the impression (based on the URL) that this
was the official Fedora kernel for ARM64. Apparently that is not
true, and I probably overreacted based on that. I certainly don't
want to see this kind of patches being put into Fedora before
there is basic consensus about whether or not they can eventaully
get merged upstream.

> > It's not as bad as I had feared, but it still does a lot of
> > the things that in previous discussions were said that ACPI
> > would avoid doing, and that were used as arguments against
> > just using DT on all arm64 servers:
> > 
> > - The use of the device properties API that was introduced for
> >   Intel's embedded devices for on-chip components directly
> >   contradicts the "standardized firmware interfaces" argument
> >   that certain people keep making. This certainly explains how
> >   you plan to use the networking side, but I suspect it's not
> >   what the ACPI proponents were looking for, or what the base
> >   patch set is being advertised as.
> 
> I do not understand this statement at all.  One of the things
> that was added to the ACPI 5.1 specification was the _DSD method
> -- Device Specific Properties -- specifically so that device
> properties could be standardized.  The API mentioned relies on the
> existence of _DSD for ACPI.  Further, there are people from Linaro
> (who happen to work in the kernel and ACPI), the Linux community,
> as well as from Intel, ARM and even Microsoft working together to
> figure out ways to standardize the device properties being used in
> the ACPI Spec Working Group (ASWG) of the UEFI Forum; several of us
> are of course paying attention to, participating in, and incorporating
> the discussions on this kernel list and others.
> 
> So what is not being standardized?  From where I sit, as part of ASWG,
> Linaro, Red Hat, and some-time contributor to the Linux kernel, this
> whole device properties API was driven by the desire to have a
> standardized firmware interface.  It even seems to be going a step
> further and trying to standardize how the kernel interacts with any
> firmware.

The point of _DSD is to allow a more generalized format for describing
devices that are not standardized. We need this for (x86) embedded
systems, because the standardization process is not scalable for
coming up with an official description for every single device, so
_DSD with the DT properties extension is a great workaround.

In particular, this allows us to have a common stable binding for
devices between ACPI and DT, which is also great because we already
have DT bindings for hundreds or thousands of peripherals that are
used on embedded systems. Grant has also started conversations with
a number of parties about creating an official standard for the
subsystem specific DT bindings that are not part of IEEE1275 or
ePAPR, but the individual device bindings are not standardized in
the sense of having a standardization body oversee the addition
of every single device property in the form that the UEFI forum
oversees the things that go into the ACPI specification.

We still need to discuss about what this means for ARM servers, and
when we raised this topic during the kernel summit, it was far from
clear whether we would apply the _DSD device properties to ARM64
servers or not. I think there are good reasons on both sides of
the argument.

> > - Using custom drivers for hardware that is required by SBSA
> >   (ahci, pci, ...) means you are contradicting the 
> >   Documentation/arm64/arm-acpi.txt document that is merged as
> >   one of your early patches and that keeps getting posted as
> >   the base of discussion for the inclusion of the base ACPI
> >   support. I don't think you can have it both ways, saying that
> >   SBSA is required and supporting hardware that doesn't do any
> >   of it won't work.
> 
> This is where I start the serious whinging....
> 
> On the one hand, I'm told "show the patches."  Fine.  Someone other
> than me happens to show patches for work in progress.  Or, perhaps I
> show patches that work but are still proof of concept.  It doesn't
> seem to matter which.  The response then looks to me like I'm being
> told, "don't show patches that are not the absolute, final, irrefutable
> and irrevocable version."  So which is it?
> 
> The git tree mentioned is a work in progress.  No one has ever claimed
> it was otherwise.  This is exactly the same case as with the Linaro
> ACPI and kernel development trees.

I definitely want to see work-in-progress patches. It's very important
that we figure out what kind of platforms we can and cannot support
upstream. Your patches can do exactly that: If we find that the issues
are fundamental based on hardware problems that you can only discover
by trying things out and we can't ever merge them, then you should
find out as early as possible so you can stop wasting your time on
them. If on the other hand you can show that all remaining problems
are shallow and that you have a plan to fix them in software in an
acceptable way, then you can start announcing that it works and that
we will be able to support that hardware with ACPI upstream in the
future, as well as merge the base patches in a reasonable time frame.

> What I find incredibly frustrating is that I feel I am being told to
> submit the final form of patches for ACPI drivers, but those can ONLY
> be a work a progress until the ACPI core patches that we have been
> trying to get into the kernel are accepted.

That is bullshit, who is saying that?

You should definitely submit driver patches for review, it is the
absence of these patches that has made the core ACPI support impossible
to review properly, because we just don't know what is coming.

Submitting for inclusion is a bit different: I don't want to clutter
the kernel with support for an interface that we probably won't
ever support. The PCI support for X-Gene shows exactly that, it isn't
compliant with either ACPI-5.x or SBSA, and supporting it with ACPI
requires extremely ugly hacks. If we know we can't support X-Gene PCIe,
what good does it do to put any of the other X-Gene patches in?

>  Until those core patches
> are in the kernel, all I can really do is experiment with drivers and
> show work in progress; there's no foundation to rely on.
> 
> I get further frustrated when I fold in basic, practical considerations.
> Regardless of whether one thinks the APM or AMD hardware is an actual
> arm64 server or not, it is what we have.  I cannot change that.  Some
> of the hardware is not server-like; some of it just does strange and
> goofy things.  These are first generation boxes; I always expect such
> things to be weird.  It's just in their nature.

Sure, and we will have even stranger servers, and more broken ones,
as soon as we get more of the embedded SoCs using ARM64 cores and
people start putting them into server boxes. We can support them all
with DT, and we likely will, but there are limits to what we can and
should do with ACPI, and we have 

> So, I write (or ask someone else to write, or bodge something borrowed
> from someone else) a driver to experiment with, that tries to get odd
> hardware working.  Or, in the case of the SBSA UART, what's available
> doesn't actually work on some hardware.  Or, in some cases, the driver
> based on DT is not right.  Or, the firmware (DT *or* ACPI) is not yet
> complete.  My goal is just to see if I can get *something* working so
> I can see what's really going on with the hardware.  Then, the patches
> that work, even if ugly, are shared as work in progress.  The result
> of that sharing seems to be more "go away until absolute final perfect
> versions are ready."

Not at all, we never require code to be final or perfect before it
can get merged. The requirement is that we can reasonably assume that
we don't have to make incompatible interface changes (user space or
firmware interfaces, not in-kernel), and that we can see how bad it
would get to complete the work.

> If what I am really being told is that ACPI for arm64 will never be
> accepted in the Linux kernel, then just say so.  I have other things
> to do with my life than go in circles like this.
> 
> If instead there is some constructive criticism that helps improve
> either the ACPI core patches or the driver patches, or better yet
> allows us to make forward progress, that would be wonderful.  As far
> as I know, everyone is doing the best they can with the resources
> available, and making as much progress as they can without everything
> being settled.

I can keep saying what I have said many times before: the core patches
are not a big issue, stop trying to polish them more and focus on
the important parts:

- Show what a reasonably complete platform would look like. A link
  to a git tree in the description of the base patches would be
  a good start, but it should also mention the things that are
  still missing.

- write a patch series introduction that explains why you think
  we should merge it rather than what it is that the patches do.
  The v5 "Introduce ACPI for ARM64 based on ACPI 5.1" series still
  completely fails to do this, I would never dream of sending this
  upstream to Linus Torvalds without a really good justification.
  
- Limit the scope of ACPI support to the smallest useful subset
  of machines that are required to achieve the goals of ACPI
  support. Insisting on including X-Gene in the mix is just making
  this very painful and slow, and I can't imagine any technical
  reason for doing this.

> > - Adding a generalized method to support arbitrary PCI host
> >   controllers indicates that you expect this practice to not
> >   just be a special exception for APM but that others would
> >   require the same hack to work around firmware or hardware that
> >   is not compliant with ACPI. At the same time you introduce a
> >   significant of driver code in arch/arm64/kernel/pci.c. Some
> >   of that code is probably just an oversight and can be moved into
> >   the PCI core later, but it doesn't even seem you tried that
> >   hard.
> 
> APM *is* a special case because of what they do in their hardware;
> perhaps that should have been stated explicitly.  By the same token,
> writing kernel code that is flexible in the face of unknown future
> hardware seemed reasonable at face value, but perhaps we should not
> do that.

Correct: Don't write kernel code more generic than you need to.
Make it as simple as you can to cover the cases you know about,
and refactor it when you have to. In general, adding lots of
generic infrastructure to handle one special case is not worth
it, it will just make your entire code look unnecessarily complex,
which is a problem for merging.

>  But again, this is a work in progress.  The fact that we
> are having to fix x86-specific code and refactor it so that it works
> on multiple architectures is quite blindingly obvious and is indeed
> something that is already being worked on; perhaps that should also
> have been said explicitly.  But again, we chose to share code that we
> know is an early version instead of waiting until a final refactored
> version is available, or instead of keeping it "secret."  Assuming we
> haven't even tried or are completely unaware of the problem without
> even asking I find kind of insulting.

Sorry for the bad wording here, I didn't mean to be insulting.
 
> From where I sit, I feel I'm being told that I'm doing all the work in
> secret and getting dinged for that.  So, work in progress is shared,
> even though it's early and we explicitly says it's work in progress,
> and now I'm getting dinged for that.

Some patches very clearly talk about work in progress in the changelog,
and I wasn't in any way referring to those. Some other patches
like "arm64/acpi/pci: add support for parsing MCFG table" are written
like you expect them to get merged, so it seems I was confused by
the somewhat lacking changelog if this really just meant as a prototype.

> </diversion>
> 
> So, back to the original topic: are these USB patches ack'd or not?
> I can verify that they behave on Mustang; they have been tested with
> both DT and ACPI.

The USB patches are fine in the latest version, they are not X-Gene
specific and are needed anyway.

	Arnd

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Mark Langsdorf]

On 11/04/2014 11:12 AM, Greg KH wrote:
> On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote:
   #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
>
> That looks like a very "generic" PNP value, are you sure it is assigned
> only to this specific device?

Although this is a generic PNP device, the specific implementation
I'm testing has issues with USB3. Is there a flag or function
call that will disable the USB3 host while keeping the USB2
host?? My naive attempts in finding one mostly hung the machine.

--Mark Langsdorf

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Greg KH]

On Thu, Nov 13, 2014 at 12:36:09PM -0600, Mark Langsdorf wrote:
> On 11/04/2014 11:12 AM, Greg KH wrote:
> >On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote:
>   #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
> >
> >That looks like a very "generic" PNP value, are you sure it is assigned
> >only to this specific device?
> 
> Although this is a generic PNP device, the specific implementation
> I'm testing has issues with USB3. Is there a flag or function
> call that will disable the USB3 host while keeping the USB2
> host?? My naive attempts in finding one mostly hung the machine.

If this is an xhci chip, there is no stand-along USB 2 controller
anymore, sorry.

greg k-h

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Feng Kan]

On Thu, Nov 13, 2014 at 10:36 AM, Mark Langsdorf <mlangsdo@redhat.com> wrote:
> On 11/04/2014 11:12 AM, Greg KH wrote:
>>
>> On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote:
>
>   #endif
>>>
>>>
>>> +#ifdef CONFIG_ACPI
>>> +static const struct acpi_device_id usb_xhci_acpi_match[] = {
>>> +       /* APM X-Gene USB Controller */
>>> +       { "PNP0D10", },

Mark, would it be better to use PRP0001 instead as in this patch?
https://lkml.org/lkml/2014/9/16/230

>>> +       { }
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, usb_xhci_acpi_match);
>>> +#endif
>>
>>
>> That looks like a very "generic" PNP value, are you sure it is assigned
>> only to this specific device?
>
>
> Although this is a generic PNP device, the specific implementation
> I'm testing has issues with USB3. Is there a flag or function
> call that will disable the USB3 host while keeping the USB2
> host?? My naive attempts in finding one mostly hung the machine.
>
> --Mark Langsdorf
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Mark Langsdorf]

On 11/18/2014 02:05 PM, Feng Kan wrote:
> On Thu, Nov 13, 2014 at 10:36 AM, Mark Langsdorf <mlangsdo@redhat.com> wrote:
>> On 11/04/2014 11:12 AM, Greg KH wrote:
>>>
>>> On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote:
>>
>>    #endif
>>>>
>>>>
>>>> +#ifdef CONFIG_ACPI
>>>> +static const struct acpi_device_id usb_xhci_acpi_match[] = {
>>>> +       /* APM X-Gene USB Controller */
>>>> +       { "PNP0D10", },
>
> Mark, would it be better to use PRP0001 instead as in this patch?
> https://lkml.org/lkml/2014/9/16/230

Quoting Arnd,
"In this case, the device does have an official ACPI ID "PNP0D10",
so we should use that for compatibility with other operating
systems and with BIOS versions that provide the standard IDs."

--Mark Langsdorf

[PATCH v2 2/2] [usb] add support for ACPI identification to xhci-platform

[Feng Kan]

On Tue, Nov 18, 2014 at 12:33 PM, Mark Langsdorf <mlangsdo@redhat.com> wrote:
> On 11/18/2014 02:05 PM, Feng Kan wrote:
>>
>> On Thu, Nov 13, 2014 at 10:36 AM, Mark Langsdorf <mlangsdo@redhat.com>
>> wrote:
>>>
>>> On 11/04/2014 11:12 AM, Greg KH wrote:
>>>>
>>>>
>>>> On Tue, Nov 04, 2014 at 10:50:33AM -0600, Mark Langsdorf wrote:
>>>
>>>
>>>    #endif
>>>>>
>>>>>
>>>>>
>>>>> +#ifdef CONFIG_ACPI
>>>>> +static const struct acpi_device_id usb_xhci_acpi_match[] = {
>>>>> +       /* APM X-Gene USB Controller */
>>>>> +       { "PNP0D10", },
>>
>>
>> Mark, would it be better to use PRP0001 instead as in this patch?
>> https://lkml.org/lkml/2014/9/16/230
>
>
> Quoting Arnd,
>
> "In this case, the device does have an official ACPI ID "PNP0D10",
> so we should use that for compatibility with other operating
> systems and with BIOS versions that provide the standard IDs."
>
Yes, thanks. I missed that part, sorry for the spam then.
> --Mark Langsdorf