Re: [PATCH RFC 09/12] of: populate new device_d::dma_coherent attribute

[Sascha Hauer <sha@pengutronix.de>]

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@pengutronix.de>
> ---
>  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 |