Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932608AbbEHU26 (ORCPT ); Fri, 8 May 2015 16:28:58 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:59339 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S932261AbbEHU2z (ORCPT ); Fri, 8 May 2015 16:28:55 -0400 From: "Rafael J. Wysocki" To: Suravee Suthikulpanit Cc: lenb@kernel.org, catalin.marinas@arm.com, will.deacon@arm.com, thomas.lendacky@amd.com, herbert@gondor.apana.org.au, davem@davemloft.net, arnd@arndb.de, msalter@redhat.com, hanjun.guo@linaro.org, al.stone@linaro.org, grant.likely@linaro.org, leo.duran@amd.com, linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linaro-acpi@lists.linaro.org, netdev@vger.kernel.org, linux-crypto@vger.kernel.org Subject: Re: [V3 PATCH 1/5] ACPI / scan: Parse _CCA and setup device coherency Date: Fri, 08 May 2015 22:53:59 +0200 Message-ID: <3684853.vaQGhc4jR5@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.0.0+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <1431045436-8690-2-git-send-email-Suravee.Suthikulpanit@amd.com> References: <1431045436-8690-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1431045436-8690-2-git-send-email-Suravee.Suthikulpanit@amd.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7587 Lines: 233 On Thursday, May 07, 2015 07:37:12 PM Suravee Suthikulpanit wrote: > This patch implements support for ACPI _CCA object, which is introduced in > ACPIv5.1, can be used for specifying device DMA coherency attribute. > > The parsing logic traverses device namespace to parse coherency > information, and stores it in acpi_device_flags. Then uses it to call > arch_setup_dma_ops() when creating each device enumerated in DSDT > during ACPI scan. > > This patch also introduces acpi_dma_is_coherent(), which provides > an interface for device drivers to check the coherency information > similarly to the of_dma_is_coherent(). > > Signed-off-by: Mark Salter > Signed-off-by: Suravee Suthikulpanit > --- > drivers/acpi/Kconfig | 6 ++++++ > drivers/acpi/acpi_platform.c | 10 +++++++--- > drivers/acpi/scan.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 32 +++++++++++++++++++++++++++++++- > include/linux/acpi.h | 10 ++++++++++ > 5 files changed, 96 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index ab2cbb5..7822149 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -54,6 +54,12 @@ config ACPI_GENERIC_GSI > config ACPI_SYSTEM_POWER_STATES_SUPPORT > bool > > +config ACPI_CCA_REQUIRED > + bool > + > +config ARM64_SUPPORT_ACPI_CCA_ZERO Hmm. I guess the Arnd's idea what to simply use CONFIG_ARM64 directly instead of adding this new option. > + bool > + > config ACPI_SLEEP > bool > depends on SUSPEND || HIBERNATION > diff --git a/drivers/acpi/acpi_platform.c b/drivers/acpi/acpi_platform.c > index 4bf7559..a084ea0 100644 > --- a/drivers/acpi/acpi_platform.c > +++ b/drivers/acpi/acpi_platform.c > @@ -103,14 +103,18 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *adev) > pdevinfo.res = resources; > pdevinfo.num_res = count; > pdevinfo.fwnode = acpi_fwnode_handle(adev); > - pdevinfo.dma_mask = DMA_BIT_MASK(32); > + pdevinfo.dma_mask = acpi_dma_is_supported(adev)? DMA_BIT_MASK(32): 0; Spaces before the "?" and ":", please. > pdev = platform_device_register_full(&pdevinfo); > - if (IS_ERR(pdev)) > + if (IS_ERR(pdev)) { > dev_err(&adev->dev, "platform device creation failed: %ld\n", > PTR_ERR(pdev)); > - else > + } else { > + if (acpi_dma_is_supported(adev)) > + arch_setup_dma_ops(&pdev->dev, 0, 0, NULL, > + acpi_dma_is_coherent(adev)); OK, so I understand why this is needed, but -> > dev_dbg(&adev->dev, "created platform device %s\n", > dev_name(&pdev->dev)); > + } > > kfree(resources); > return pdev; > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index 849b699..0976dc2 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -11,6 +11,7 @@ > #include > #include > #include > +#include > > #include > > @@ -2137,6 +2138,46 @@ void acpi_free_pnp_ids(struct acpi_device_pnp *pnp) > kfree(pnp->unique_id); > } > > +static void acpi_init_coherency(struct acpi_device *adev) > +{ > + unsigned long long cca = 0; > + acpi_status status; > + struct acpi_device *parent = adev->parent; > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + > + if (parent && parent->flags.cca_seen) { > + /* > + * From ACPI spec, OSPM will ignore _CCA if an ancestor > + * already saw one. > + */ > + adev->flags.cca_seen = 1; > + cca = acpi_dma_is_coherent(parent); > + } else { > + status = acpi_evaluate_integer(adev->handle, "_CCA", > + NULL, &cca); > + if (ACPI_SUCCESS(status)) { > + adev->flags.cca_seen = 1; > + } else if (!IS_ENABLED(CONFIG_ACPI_CCA_REQUIRED)) { > + /* > + * If architecture does not specify that _CCA is > + * required for DMA-able devices (e.g. x86), > + * we default to _CCA=1. > + */ > + cca = 1; > + } else { > + acpi_get_name(adev->handle, ACPI_FULL_PATHNAME, &buffer); > + pr_debug("ACPI device %s is missing _CCA.\n", > + (char *) buffer.pointer); > + kfree(buffer.pointer); > + } > + } > + > + adev->flags.is_coherent = cca; > + if (acpi_dma_is_supported(adev)) > + arch_setup_dma_ops(&adev->dev, 0, 0, NULL, > + acpi_dma_is_coherent(adev)); Why do we need this one? adev->dev is not a device, it is an ACPI namespace node representation. Why do you want to set up DMA ops for it? Would you set up DMA ops for a struct device_node? > +} > + > void acpi_init_device_object(struct acpi_device *device, acpi_handle handle, > int type, unsigned long long sta) > { > @@ -2155,6 +2196,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle, > device->flags.visited = false; > device_initialize(&device->dev); > dev_set_uevent_suppress(&device->dev, true); > + acpi_init_coherency(device); > } > > void acpi_device_add_finalize(struct acpi_device *device) > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 8de4fa9..17fb630 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -208,7 +208,9 @@ struct acpi_device_flags { > u32 visited:1; > u32 hotplug_notify:1; > u32 is_dock_station:1; > - u32 reserved:23; > + u32 is_coherent:1; > + u32 cca_seen:1; > + u32 reserved:21; > }; > > /* File System */ > @@ -380,6 +382,34 @@ struct acpi_device { > void (*remove)(struct acpi_device *); > }; > > +static inline bool acpi_dma_is_supported(struct acpi_device *adev) > +{ > + /** > + * Currently, we mainly support _CCA=1 (i.e. is_coherent=1) > + * This should be equivalent to specifyig dma-coherent for > + * a device in OF. > + * > + * For the case when _CCA=0 (i.e. is_coherent=0 && cca_seen=1), > + * we would rely on arch-specific cache maintenance for > + * non-coherence DMA operations if architecture specifies > + * _XXX_SUPPORT_CCA_ZERO. Otherwise, we do not support > + * DMA on this device and fallback to arch-specific default > + * handling. > + * > + * For the case when _CCA is missing (i.e. cca_seen=0) but > + * platform specifies ACPI_CCA_REQUIRED, we do not support DMA, > + * and fallback to arch-specific default handling. > + */ > + return adev && (adev->flags.is_coherent || > + (adev->flags.cca_seen && > + IS_ENABLED(CONFIG_ARM64_SUPPORT_ACPI_CCA_ZERO))); So what exactly would be wrong with using IS_ENABLED(CONFIG_ARM64) here? > +} > + > +static inline bool acpi_dma_is_coherent(struct acpi_device *adev) > +{ > + return adev && adev->flags.is_coherent; > +} > + > static inline bool is_acpi_node(struct fwnode_handle *fwnode) > { > return fwnode && fwnode->type == FWNODE_ACPI; > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index b10c4a6..baccf3b 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -583,6 +583,16 @@ static inline int acpi_device_modalias(struct device *dev, > return -ENODEV; > } > > +static inline bool acpi_dma_is_supported(struct acpi_device *adev) > +{ > + return false; > +} > + > +static inline bool acpi_dma_is_coherent(struct acpi_device *adev) > +{ > + return false; > +} > + > #define ACPI_PTR(_ptr) (NULL) > > #endif /* !CONFIG_ACPI */ > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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/