Messages in this thread | | | Date | Mon, 5 Feb 2024 18:03:53 +0000 | Subject | Re: [PATCH 05/12] bcm2835-dma: Derive slave DMA addresses correctly | From | Robin Murphy <> |
| |
On 2024-02-04 6:59 am, Andrea della Porta wrote: > From: Phil Elwell <phil@raspberrypi.com> > > Slave addresses for DMA are meant to be supplied as physical addresses > (contrary to what struct snd_dmaengine_dai_dma_data does). It is up to > the DMA controller driver to perform the translation based on its own > view of the world, as described in Device Tree. > > Now that the Pi Device Trees have the correct peripheral mappings, > replace the hacky address munging with phys_to_dma(). > > Signed-off-by: Phil Elwell <phil@raspberrypi.com> > --- > drivers/dma/bcm2835-dma.c | 23 +++++------------------ > 1 file changed, 5 insertions(+), 18 deletions(-) > > diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c > index 237dcdb8d726..077812eda609 100644 > --- a/drivers/dma/bcm2835-dma.c > +++ b/drivers/dma/bcm2835-dma.c > @@ -18,6 +18,7 @@ > * Copyright 2012 Marvell International Ltd. > */ > #include <linux/dmaengine.h> > +#include <linux/dma-direct.h>
Please read the comment at the top of that file; this driver is definitely not a DMA API implementation, and should not be including it.
> #include <linux/dma-mapping.h> > #include <linux/dmapool.h> > #include <linux/err.h> > @@ -980,22 +981,12 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_slave_sg( > if (direction == DMA_DEV_TO_MEM) { > if (c->cfg.src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) > return NULL; > - src = c->cfg.src_addr; > - /* > - * One would think it ought to be possible to get the physical > - * to dma address mapping information from the dma-ranges DT > - * property, but I've not found a way yet that doesn't involve > - * open-coding the whole thing. > - */ > - if (c->is_40bit_channel) > - src |= 0x400000000ull; > + src = phys_to_dma(chan->device->dev, c->cfg.src_addr);
FWIW I'd argue that abusing DMA API internals like this is even more hacky than bypassing it entirely. The appropriate public API for setting up the device end of a transfer is dma_map_resource(). Now, it *is* the case currently that the dma-direct implementation of that does not take dma_range_map into account, but that's already an open question:
https://lore.kernel.org/linux-iommu/20220610080802.11147-1-Sergey.Semin@baikalelectronics.ru/
Thanks, Robin.
> info |= BCM2835_DMA_S_DREQ | BCM2835_DMA_D_INC; > } else { > if (c->cfg.dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) > return NULL; > - dst = c->cfg.dst_addr; > - if (c->is_40bit_channel) > - dst |= 0x400000000ull; > + dst = phys_to_dma(chan->device->dev, c->cfg.dst_addr); > info |= BCM2835_DMA_D_DREQ | BCM2835_DMA_S_INC; > } > > @@ -1064,17 +1055,13 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic( > if (direction == DMA_DEV_TO_MEM) { > if (c->cfg.src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) > return NULL; > - src = c->cfg.src_addr; > - if (c->is_40bit_channel) > - src |= 0x400000000ull; > + src = phys_to_dma(chan->device->dev, c->cfg.src_addr); > dst = buf_addr; > info |= BCM2835_DMA_S_DREQ | BCM2835_DMA_D_INC; > } else { > if (c->cfg.dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES) > return NULL; > - dst = c->cfg.dst_addr; > - if (c->is_40bit_channel) > - dst |= 0x400000000ull; > + dst = phys_to_dma(chan->device->dev, c->cfg.dst_addr); > src = buf_addr; > info |= BCM2835_DMA_D_DREQ | BCM2835_DMA_S_INC; >
| |