On Tue, Feb 21, 2023 at 09:05:21AM +0100, Ahmad Fatoum wrote:
> So far, whether device DMA is coherent was a one-time global decision.
> This is insufficient, because some platforms:
>
> - are cache coherent, while the architecture isn't in general, e.g. ARM
> with CONFIG_MMU=y assumes non-coherent DMA, but LS1046A is coherent
>
> - have a mix of devices that snoop caches and devices that don't
> (StarFive JH7100).
>
> To enable dev_dma_(map|unmap)_single to take the correct device-specific
> action with regards to cache maintenance, provide dev_is_dma_coherent()
> with semantics similar to what Linux provides.
>
> This admittedly looks a bit ugly, but we will refrain from making
> dma_coherent defined independent of ifdef CONFIG_ARCH_HAS_NONCOHERENT_DMA:
> On platforms that are cache-coherent, we will want dev->dma_coherent to
> be true. Yet not all code allocating devices uses dev_alloc(), so adding
> a global toggle is a bit too much refactoring effort for now.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> ---
> commands/devinfo.c | 5 +++++
> drivers/of/platform.c | 3 +++
> include/driver.h | 28 ++++++++++++++++++++++++++++
> 3 files changed, 36 insertions(+)
>
> diff --git a/commands/devinfo.c b/commands/devinfo.c
> index 2487786c7101..be28d02028c3 100644
> --- a/commands/devinfo.c
> +++ b/commands/devinfo.c
> @@ -50,6 +50,7 @@ static int do_devinfo(int argc, char *argv[])
> struct param_d *param;
> int i;
> int first;
> + int coherent;
> struct resource *res;
>
> if (argc == 1) {
> @@ -82,6 +83,10 @@ static int do_devinfo(int argc, char *argv[])
> if (dev->bus)
> printf("Bus: %s\n", dev->bus->name);
>
> + coherent = dev_is_dma_coherent(dev);
> + if (coherent >= 0)
> + printf("DMA Coherent: %s\n", coherent ? "true" : "false");
'>=' doesn't seem to be quite right here.
> +
> if (dev->info)
> dev->info(dev);
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 0982873446a6..1bd5d41ba226 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -128,6 +128,9 @@ static void of_dma_configure(struct device *dev, struct device_node *np)
> }
>
> dev->dma_offset = offset;
> +#ifdef CONFIG_OF_DMA_COHERENCY
> + dev->dma_coherent = of_dma_is_coherent(np);
> +#endif
> }
>
> /**
> diff --git a/include/driver.h b/include/driver.h
> index 7e25c060280b..f6301d954bb5 100644
> --- a/include/driver.h
> +++ b/include/driver.h
> @@ -8,6 +8,7 @@
>
> #include <linux/list.h>
> #include <linux/ioport.h>
> +#include <linux/bitops.h>
> #include <of.h>
> #include <filetype.h>
>
> @@ -43,6 +44,13 @@ struct device {
> * something like eth0 or nor0. */
> int id;
>
> + /*! This particular device is dma coherent, even if the
> + * architecture supports non-coherent devices.
> + */
> +#ifdef CONFIG_OF_DMA_COHERENCY
> + bool dma_coherent:1;
> +#endif
I think this patch could be easier when we add the dma_coherent field
unconditionally.
> +
> struct resource *resource;
> int num_resources;
>
> @@ -652,6 +660,26 @@ static inline struct device_node *dev_of_node(struct device *dev)
> return IS_ENABLED(CONFIG_OFDEVICE) ? dev->of_node : NULL;
> }
>
> +#ifdef CONFIG_OF_DMA_COHERENCY
> +static inline int __dev_is_dma_coherent(struct device *dev)
> +{
> + return dev->dma_coherent;
> +}
> +#else
> +static inline bool __dev_is_dma_coherent(struct device *dev)
> +{
> + return IS_ENABLED(CONFIG_OF_DMA_DEFAULT_COHERENT);
> +}
> +#endif
> +
> +static inline int dev_is_dma_coherent(struct device *dev)
> +{
> + if (!dev_of_node(dev))
> + return -EINVAL;
> +
> + return __dev_is_dma_coherent(dev);
> +}
This should return bool. Either a device is DMA coherent or not.
You seem to assume that a device is not cache coherent when you don't
know:
- dma_sync_single_for_device(addr, size, dir);
+ if (dev_is_dma_coherent(dev) <= 0)
+ dma_sync_single_for_device(addr, size, dir);
This assumption seems to make sense as an unnecessary cache synchronisation
shouldn't hurt, right?
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
