Re: [PATCH V5 1/9] ACPI: Honor ACPI _CCA attribute setting

Re: [PATCH V5 1/9] ACPI: Honor ACPI _CCA attribute setting

[Dennis Chen]

On Thu, Oct 29, 2015 at 6:50 AM, Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> wrote:
> From: Jeremy Linton <jeremy.linton@arm.com>
> 
> ACPI configurations can now mark devices as noncoherent,
> support that choice.
> 
> NOTE: This is required to support USB on ARM Juno Development Board.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Will Deacon <will.deacon@arm.com>
> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
> ---
>  include/acpi/acpi_bus.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index d11eff8..0f131d2 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -407,7 +407,7 @@ static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
>  	 * case 1. Do not support and disable DMA.
>  	 * case 2. Support but rely on arch-specific cache maintenance for
>  	 *         non-coherence DMA operations.
> -	 * Currently, we implement case 1 above.
> +	 * Currently, we implement case 2 above.
>  	 *
>  	 * For the case when _CCA is missing (i.e. cca_seen=0) and
>  	 * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
> @@ -415,7 +415,8 @@ static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
>  	 *
>  	 * See acpi_init_coherency() for more info.
>  	 */
> -	if (adev->flags.coherent_dma) {
> +	if (adev->flags.coherent_dma ||
> +	    (adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64))) {
>  		ret = true;
>  		if (coherent)
>  			*coherent = adev->flags.coherent_dma;

Hi Suravee,

The acpi_check_dma function has been removed in patch 6 of this patch set, why it is still be used
here, am I missing something? If the acpi_check_dma will be used in the future, personally I'd like
to use IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) while not CONFIG_ARM64 macro here, or since _CCA attribute
is arch-specific, it's reasonable to leave the _CCA handling policy to the arch-specific code. For example,
with a link weak function like acpi_arch_check_dma() as a default handling if no arch-specific code 
provided, the actual _CCA handling will be implemented in the ARM, Intel or other Arch if required.

Thanks,
Dennis

> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH V5 1/9] ACPI: Honor ACPI _CCA attribute setting

[Hanjun Guo]

Hi Dennis,

On 11/02/2015 12:02 PM, Dennis Chen wrote:
> On Thu, Oct 29, 2015 at 6:50 AM, Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> wrote:
>> From: Jeremy Linton <jeremy.linton@arm.com>
>>
>> ACPI configurations can now mark devices as noncoherent,
>> support that choice.
>>
>> NOTE: This is required to support USB on ARM Juno Development Board.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> CC: Bjorn Helgaas <bhelgaas@google.com>
>> CC: Catalin Marinas <catalin.marinas@arm.com>
>> CC: Rob Herring <robh+dt@kernel.org>
>> CC: Will Deacon <will.deacon@arm.com>
>> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
>> ---
>>   include/acpi/acpi_bus.h | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>> index d11eff8..0f131d2 100644
>> --- a/include/acpi/acpi_bus.h
>> +++ b/include/acpi/acpi_bus.h
>> @@ -407,7 +407,7 @@ static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
>>   	 * case 1. Do not support and disable DMA.
>>   	 * case 2. Support but rely on arch-specific cache maintenance for
>>   	 *         non-coherence DMA operations.
>> -	 * Currently, we implement case 1 above.
>> +	 * Currently, we implement case 2 above.
>>   	 *
>>   	 * For the case when _CCA is missing (i.e. cca_seen=0) and
>>   	 * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
>> @@ -415,7 +415,8 @@ static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
>>   	 *
>>   	 * See acpi_init_coherency() for more info.
>>   	 */
>> -	if (adev->flags.coherent_dma) {
>> +	if (adev->flags.coherent_dma ||
>> +	    (adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64))) {
>>   		ret = true;
>>   		if (coherent)
>>   			*coherent = adev->flags.coherent_dma;
>
> Hi Suravee,
>
> The acpi_check_dma function has been removed in patch 6 of this patch set, why it is still be used
> here, am I missing something? If the acpi_check_dma will be used in the future, personally I'd like

I think this patch just to let people know that there is
case that arch-specific cache maintenance is still needed
for ACPI (such as Juno board), and in the later patches will
cover this case.

acpi_check_dma() will be replaced by acpi_get_dma_attr(),
and in acpi_get_dma_attr() will cover that case and will
be easily understood. (Suravee, correct me if I'm wrong :) )

> to use IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) while not CONFIG_ARM64 macro here, or since _CCA attribute
> is arch-specific, it's reasonable to leave the _CCA handling policy to the arch-specific code. For example,
> with a link weak function like acpi_arch_check_dma() as a default handling if no arch-specific code
> provided, the actual _CCA handling will be implemented in the ARM, Intel or other Arch if required.

Actually Intel platform don't need _CCA and it's coherent
in default, since _CCA is in ACPI spec, I would like it's
handled in ACPI core.

Thanks
Hanjun

Re: [PATCH V5 1/9] ACPI: Honor ACPI _CCA attribute setting

[Suravee Suthikulanit]

Hi Dennis / Hanjun,

On 11/2/2015 5:58 AM, Hanjun Guo wrote:
> Hi Dennis,
>
> On 11/02/2015 12:02 PM, Dennis Chen wrote:
>> On Thu, Oct 29, 2015 at 6:50 AM, Suravee Suthikulpanit
>> <Suravee.Suthikulpanit@amd.com> wrote:
>>> From: Jeremy Linton <jeremy.linton@arm.com>
>>>
>>> ACPI configurations can now mark devices as noncoherent,
>>> support that choice.
>>>
>>> NOTE: This is required to support USB on ARM Juno Development Board.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>> CC: Bjorn Helgaas <bhelgaas@google.com>
>>> CC: Catalin Marinas <catalin.marinas@arm.com>
>>> CC: Rob Herring <robh+dt@kernel.org>
>>> CC: Will Deacon <will.deacon@arm.com>
>>> CC: Rafael J. Wysocki <rjw@rjwysocki.net>
>>> ---
>>>   include/acpi/acpi_bus.h | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
>>> index d11eff8..0f131d2 100644
>>> --- a/include/acpi/acpi_bus.h
>>> +++ b/include/acpi/acpi_bus.h
>>> @@ -407,7 +407,7 @@ static inline bool acpi_check_dma(struct
>>> acpi_device *adev, bool *coherent)
>>>        * case 1. Do not support and disable DMA.
>>>        * case 2. Support but rely on arch-specific cache maintenance for
>>>        *         non-coherence DMA operations.
>>> -     * Currently, we implement case 1 above.
>>> +     * Currently, we implement case 2 above.
>>>        *
>>>        * For the case when _CCA is missing (i.e. cca_seen=0) and
>>>        * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
>>> @@ -415,7 +415,8 @@ static inline bool acpi_check_dma(struct
>>> acpi_device *adev, bool *coherent)
>>>        *
>>>        * See acpi_init_coherency() for more info.
>>>        */
>>> -    if (adev->flags.coherent_dma) {
>>> +    if (adev->flags.coherent_dma ||
>>> +        (adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64))) {
>>>           ret = true;
>>>           if (coherent)
>>>               *coherent = adev->flags.coherent_dma;
>>
>> Hi Suravee,
>>
>> The acpi_check_dma function has been removed in patch 6 of this patch
>> set, why it is still be used
>> here, am I missing something? If the acpi_check_dma will be used in
>> the future, personally I'd like
>
> I think this patch just to let people know that there is
> case that arch-specific cache maintenance is still needed
> for ACPI (such as Juno board), and in the later patches will
> cover this case.
>
> acpi_check_dma() will be replaced by acpi_get_dma_attr(),
> and in acpi_get_dma_attr() will cover that case and will
> be easily understood. (Suravee, correct me if I'm wrong :) )

Thanks Hanjun for filling in the info.

Yes, this is mainly to document the logic changes required by Juno, 
which would be more clear than just merging this change in the later patch.

>> to use IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) while not CONFIG_ARM64
>> macro here,

We could have used CONFIG_ACPI_CCA_REQUIRED here, but this will be 
replaced by logic in patch 5, and removed in patch 6 anyways. So, I 
think it is okay. Eventually, the rest of the logic will be using 
CONFIG_ACPI_CCA_REQUIRED.

or since _CCA attribute
>> is arch-specific, it's reasonable to leave the _CCA handling policy to
>> the arch-specific code. For example,
>> with a link weak function like acpi_arch_check_dma() as a default
>> handling if no arch-specific code
>> provided, the actual _CCA handling will be implemented in the ARM,
>> Intel or other Arch if required.
>
> Actually Intel platform don't need _CCA and it's coherent
> in default, since _CCA is in ACPI spec, I would like it's
> handled in ACPI core.
>
> Thanks
> Hanjun

I also agree with Hanjun that the CCA parsing should be handled by the 
ACPI core driver. Since we are using the CONFIG_ACPI_CCA_REQUIRED, we 
should not need to have arch-specific code. If the ACPI spec gets more 
complicate in the future, we can revisit this. :)

Thanks,
Suravee

Re: [PATCH V5 1/9] ACPI: Honor ACPI _CCA attribute setting

[Dennis Chen]

On Mon, Nov 02, 2015 at 09:51:46AM -0600, Suravee Suthikulanit wrote:
> Hi Dennis / Hanjun,
> 
> On 11/2/2015 5:58 AM, Hanjun Guo wrote:
> >Hi Dennis,
> >
> >On 11/02/2015 12:02 PM, Dennis Chen wrote:
> >>On Thu, Oct 29, 2015 at 6:50 AM, Suravee Suthikulpanit
> >><Suravee.Suthikulpanit@amd.com> wrote:
> >>>From: Jeremy Linton <jeremy.linton@arm.com>
> >>>
> >>>ACPI configurations can now mark devices as noncoherent,
> >>>support that choice.
> >>>
> >>>NOTE: This is required to support USB on ARM Juno Development Board.
> >>>
> >>>Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >>>Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >>>CC: Bjorn Helgaas <bhelgaas@google.com>
> >>>CC: Catalin Marinas <catalin.marinas@arm.com>
> >>>CC: Rob Herring <robh+dt@kernel.org>
> >>>CC: Will Deacon <will.deacon@arm.com>
> >>>CC: Rafael J. Wysocki <rjw@rjwysocki.net>
> >>>---
> >>>  include/acpi/acpi_bus.h | 5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> >>>index d11eff8..0f131d2 100644
> >>>--- a/include/acpi/acpi_bus.h
> >>>+++ b/include/acpi/acpi_bus.h
> >>>@@ -407,7 +407,7 @@ static inline bool acpi_check_dma(struct
> >>>acpi_device *adev, bool *coherent)
> >>>       * case 1. Do not support and disable DMA.
> >>>       * case 2. Support but rely on arch-specific cache maintenance for
> >>>       *         non-coherence DMA operations.
> >>>-     * Currently, we implement case 1 above.
> >>>+     * Currently, we implement case 2 above.
> >>>       *
> >>>       * For the case when _CCA is missing (i.e. cca_seen=0) and
> >>>       * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
> >>>@@ -415,7 +415,8 @@ static inline bool acpi_check_dma(struct
> >>>acpi_device *adev, bool *coherent)
> >>>       *
> >>>       * See acpi_init_coherency() for more info.
> >>>       */
> >>>-    if (adev->flags.coherent_dma) {
> >>>+    if (adev->flags.coherent_dma ||
> >>>+        (adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64))) {
> >>>          ret = true;
> >>>          if (coherent)
> >>>              *coherent = adev->flags.coherent_dma;
> >>
> >>Hi Suravee,
> >>
> >>The acpi_check_dma function has been removed in patch 6 of this patch
> >>set, why it is still be used
> >>here, am I missing something? If the acpi_check_dma will be used in
> >>the future, personally I'd like
> >
> >I think this patch just to let people know that there is
> >case that arch-specific cache maintenance is still needed
> >for ACPI (such as Juno board), and in the later patches will
> >cover this case.
> >
> >acpi_check_dma() will be replaced by acpi_get_dma_attr(),
> >and in acpi_get_dma_attr() will cover that case and will
> >be easily understood. (Suravee, correct me if I'm wrong :) )
> 
> Thanks Hanjun for filling in the info.
> 
> Yes, this is mainly to document the logic changes required by Juno,
> which would be more clear than just merging this change in the later
> patch.
>
Clear.
 
> >>to use IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED) while not CONFIG_ARM64
> >>macro here,
> 
> We could have used CONFIG_ACPI_CCA_REQUIRED here, but this will be
> replaced by logic in patch 5, and removed in patch 6 anyways. So, I
> think it is okay. Eventually, the rest of the logic will be using
> CONFIG_ACPI_CCA_REQUIRED.
> 
> or since _CCA attribute
> >>is arch-specific, it's reasonable to leave the _CCA handling policy to
> >>the arch-specific code. For example,
> >>with a link weak function like acpi_arch_check_dma() as a default
> >>handling if no arch-specific code
> >>provided, the actual _CCA handling will be implemented in the ARM,
> >>Intel or other Arch if required.
> >
> >Actually Intel platform don't need _CCA and it's coherent
> >in default, since _CCA is in ACPI spec, I would like it's
> >handled in ACPI core.
> >
> >Thanks
> >Hanjun
> 
> I also agree with Hanjun that the CCA parsing should be handled by
> the ACPI core driver. Since we are using the
> CONFIG_ACPI_CCA_REQUIRED, we should not need to have arch-specific
> code. If the ACPI spec gets more complicate in the future, we can
> revisit this. :)
> 
Hmm, seems I have no objection currently if we only think about intel and
arm arch. Things maybe a little bit complicated if more Archs becomes 
ACPI awareness, if any. Good to see the patch set upstream soon :) Thank you
Suravee and Hanjun.

> Thanks,
> Suravee
> 

[PATCH V5 0/9] PCI: ACPI: Setting up DMA coherency for PCI device from _CCA attribute

[Suravee Suthikulpanit]

This patch series adds support to setup DMA coherency for PCI device using
the ACPI _CCA attribute. According to the ACPI spec, the _CCA attribute
is required for ARM64. Therefore, this patch is a pre-req for ACPI PCI
support for ARM64 which is currently in development.  Also, this should
not affect other architectures that does not define 
CONFIG_ACPI_CCA_REQUIRED, since the default value is coherent.

In the process, this series also introduces enum dev_dma_attr and a set
of APIs to query device DMA attribute. These APIs replace the obsolete
device_dma_is_coherent(), and acpi_check_dma().

I have also included a patch from Jeremy posted here:
    http://www.spinics.net/lists/linux-usb/msg128582.html

This patch series is now rebased from:
    https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next

This patch series has been tested on AMD Seattle RevB platform.
The git tree containing tested code and pre-req patches is available here:
    http://github.com/ssuthiku/linux.git pci-cca-v5

Changes from V4: (https://lkml.org/lkml/2015/10/21/535)
    * Clean up from Hanjun, Bjorn, and Thomas review comments
    * Rebased on top of Rafael's latest linux-next branch
    * Added patch 7/9 to fix the  pci_get_host_bridge_device leak
    * Added Acked-by from Bjorn
    * Added Reviewed-by from Hanjun

Changes from V3: (https://lkml.org/lkml/2015/8/26/389)
    * Clean up suggested by Bjorn
    * Introduce enum dev_dma_attr
    * Replace device_dma_is_coherent() and acpi_check_dma() with
      new APIs.

Changes from V2: (https://lkml.org/lkml/2015/8/25/549)
    * Return -ENOSUPP instead of -1 (per Rafael's suggestion)
    * Add WARN() when fail to setup DMA for PCI device when booting
      ACPI (per Arnd's suggestion)
    * Added Acked-by from Rob.
    * Minor clean up

Changes from V1: (https://lkml.org/lkml/2015/8/13/182)
    * Include patch 1 from Jeremy to enable support for _CCA=0
    * Clean up acpi_check_dma() per Bjorn suggestions
    * Split the original V1 patch into two patches (patch 3 and 4)

Jeremy Linton (1):
  ACPI: Honor ACPI _CCA attribute setting

Suravee Suthikulpanit (8):
  device property: Introducing enum dev_dma_attr
  ACPI: Adding DMA Attribute APIs for ACPI Device
  device property: Adding DMA Attribute APIs for Generic Devices
  device property: ACPI: Make use of the new DMA Attribute APIs
  device property: ACPI: Remove unused DMA APIs
  of/pci: Fix pci_get_host_bridge_device leak
  PCI: OF: Move of_pci_dma_configure() to pci_dma_configure()
  PCI: ACPI: Add support for PCI device DMA coherency

 drivers/acpi/acpi_platform.c              |  7 +++++-
 drivers/acpi/glue.c                       |  8 +++---
 drivers/acpi/scan.c                       | 42 +++++++++++++++++++++++++++++++
 drivers/base/property.c                   | 32 +++++++++++++++++------
 drivers/crypto/ccp/ccp-platform.c         | 15 ++++++++---
 drivers/net/ethernet/amd/xgbe/xgbe-main.c |  8 +++++-
 drivers/of/of_pci.c                       | 20 ---------------
 drivers/pci/probe.c                       | 33 ++++++++++++++++++++++--
 include/acpi/acpi_bus.h                   | 36 +++-----------------------
 include/linux/acpi.h                      |  7 +++++-
 include/linux/of_pci.h                    |  3 ---
 include/linux/property.h                  | 10 +++++++-
 12 files changed, 144 insertions(+), 77 deletions(-)

-- 
2.1.0

[PATCH V5 1/9] ACPI: Honor ACPI _CCA attribute setting

[Suravee Suthikulpanit]

From: Jeremy Linton <jeremy.linton@arm.com>

ACPI configurations can now mark devices as noncoherent,
support that choice.

NOTE: This is required to support USB on ARM Juno Development Board.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Rob Herring <robh+dt@kernel.org>
CC: Will Deacon <will.deacon@arm.com>
CC: Rafael J. Wysocki <rjw@rjwysocki.net>
---
 include/acpi/acpi_bus.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index d11eff8..0f131d2 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -407,7 +407,7 @@ static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
 	 * case 1. Do not support and disable DMA.
 	 * case 2. Support but rely on arch-specific cache maintenance for
 	 *         non-coherence DMA operations.
-	 * Currently, we implement case 1 above.
+	 * Currently, we implement case 2 above.
 	 *
 	 * For the case when _CCA is missing (i.e. cca_seen=0) and
 	 * platform specifies ACPI_CCA_REQUIRED, we do not support DMA,
@@ -415,7 +415,8 @@ static inline bool acpi_check_dma(struct acpi_device *adev, bool *coherent)
 	 *
 	 * See acpi_init_coherency() for more info.
 	 */
-	if (adev->flags.coherent_dma) {
+	if (adev->flags.coherent_dma ||
+	    (adev->flags.cca_seen && IS_ENABLED(CONFIG_ARM64))) {
 		ret = true;
 		if (coherent)
 			*coherent = adev->flags.coherent_dma;
-- 
2.1.0