Subject: Re: [PATCH V5 1/9] ACPI: Honor ACPI _CCA attribute setting
To: Hanjun Guo <hanjun.guo@linaro.org>, Dennis Chen <dennis.chen@arm.com>,
        <bhelgaas@google.com>, <rjw@rjwysocki.net>, <lenb@kernel.org>,
        <catalin.marinas@arm.com>, <will.deacon@arm.com>
References: <20151102040224.GA16979@arm.org> <56374FF7.6050008@linaro.org>
CC: <thomas.lendacky@amd.com>, <herbert@gondor.apana.org.au>,
        <davem@davemloft.net>, <linux-acpi@vger.kernel.org>,
        <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <jeremy.linton@arm.com>,
        <robh+dt@kernel.org>
From: Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
Message-ID: <56378692.9060208@amd.com>
Date: Mon, 2 Nov 2015 09:51:46 -0600
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101
 Thunderbird/38.2.0
MIME-Version: 1.0
In-Reply-To: <56374FF7.6050008@linaro.org>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:23
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2015 15:52:00.9211
 (UTC)
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.221];Helo=[atltwp01.amd.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR12MB0859
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4221
Lines: 109

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

--
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/