[permalink] [raw]

On 5/24/2024 8:26 PM, Dave Stevenson wrote:
> Now the driver looks for the common dma-channel-mask property
> rather than the vendor-specific brcm,dma-channel-mask, update
> the dt files to follow suit.
>
> Signed-off-by: Dave Stevenson <[email protected]>

Acked-by: Florian Fainelli <[email protected]>
--
Florian

Attachments:

smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-06-05 13:25:00

On 5/24/2024 8:26 PM, Dave Stevenson wrote:
> Hi All
>
> This series initially cleans up the BCM2835 DMA driver in preparation for
> supporting the 40bit version. It then fixes up the incorrect mapping behaviour
> we've had to date.
>
> The cleanups are based on Stefan Wahren's RFC [1], with a couple of minor bugs
> fixed, but stopping before actually adding the 40bit support. If we can sort
> the mapping issue, it avoids having to have workarounds in the 40bit support.
>
> The mapping issues were discussed in [2].
> Up until this point all DMA users have been passing in dma addresses rather than
> CPU physical addresses, and the DMA driver has been using those directly rather
> than using dma_map_resource() to map them.
> The DT has also been missing some of the required mappings in "dma-ranges", but
> they have been present in "ranges". I've therefore duplicated the minimum amount
> of of_dma_get_range and translate_phys_to_dma to be able to use "ranges" as
> discussed in that thread. I'm assuming that sort of code is not desirable in the
> core code as it shouldn't be necessary, so keeping it contained within a driver
> is the better solution.
>
> When Andrea posted our downstream patches in [3], Robin Murphy stated that
> dma_map_resource is the correct API, but as it currently doesn't check the
> dma_range_map we need Sergey Semin's patch [4].
> There seemed to be no follow up over the implications of it. I've therefore
> included it in the series at least for discussion. If it's not acceptable then
> I'm not sure of the route forward in fixing this mapping issue.
>
> I'm expecting there to be some discussion, but also acknowledge that merging this
> will need to be phased with the patches 1-13 needing to be merged before any of
> 14-17, and then 18 merged last to remove the workaround. I suspect that's the
> least of my worries though.
>
>
> I will apologise in advance if I don't respond immediately to comments - I'm
> out of the office for the next week, but do appreciate any feedback.

Those patches should be routed via the dmaengine tree, including the DTS
files to minimize the possibility of introducing regressions if people
happen to bisect changes. I don't expect conflicts when these changes
reach linux-next.
--
Florian

Attachments:

smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-06-05 18:15:09

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 09/18] dmaengine: bcm2835: Add function to handle DMA mapping

On Fri, May 24, 2024 at 07:26:53PM +0100, Dave Stevenson wrote:
> The code handling DMA mapping is currently incorrect and
> needs a sequence of fixups.

Can you descript what incorrect here?

> Move the mapping out into a separate function and structure
> to allow for those fixes to be applied more cleanly.
>
> Signed-off-by: Dave Stevenson <[email protected]>
> ---
> drivers/dma/bcm2835-dma.c | 46 ++++++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index aefaa1f01d7f..ef1d95bae84e 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -65,6 +65,10 @@ struct bcm2835_cb_entry {
> dma_addr_t paddr;
> };
>
> +struct bcm2835_dma_chan_map {
> + dma_addr_t addr;
> +};
> +
> struct bcm2835_chan {
> struct virt_dma_chan vc;
>
> @@ -74,6 +78,7 @@ struct bcm2835_chan {
> int ch;
> struct bcm2835_desc *desc;
> struct dma_pool *cb_pool;
> + struct bcm2835_dma_chan_map map;

I suppose map should in bcm2835_desc. if put in chan, how about client
driver create two desc by bcm2835_dma_prep_slave_sg()?

prep_slave_sg()
submit()
prep_savle_sg()
submit()
issue_pending()

Frank

>
> void __iomem *chan_base;
> int irq_number;
> @@ -268,6 +273,19 @@ static inline bool need_dst_incr(enum dma_transfer_direction direction)
> }
>
> return false;
> +};
> +
> +static int bcm2835_dma_map_slave_addr(struct dma_chan *chan,
> + phys_addr_t dev_addr,
> + size_t dev_size,
> + enum dma_data_direction dev_dir)
> +{
> + struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> + struct bcm2835_dma_chan_map *map = &c->map;
> +
> + map->addr = dev_addr;
> +
> + return 0;
> }
>
> static void bcm2835_dma_free_cb_chain(struct bcm2835_desc *desc)
> @@ -734,13 +752,19 @@ 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)
> + if (bcm2835_dma_map_slave_addr(chan, c->cfg.src_addr,
> + c->cfg.src_addr_width,
> + DMA_TO_DEVICE))
> return NULL;
> - src = c->cfg.src_addr;
> +
> + src = c->map.addr;
> } else {
> - if (c->cfg.dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
> + if (bcm2835_dma_map_slave_addr(chan, c->cfg.dst_addr,
> + c->cfg.dst_addr_width,
> + DMA_FROM_DEVICE))
> return NULL;
> - dst = c->cfg.dst_addr;
> +
> + dst = c->map.addr;
> }
>
> /* count frames in sg list */
> @@ -795,14 +819,20 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
> __func__, buf_len, period_len);
>
> if (direction == DMA_DEV_TO_MEM) {
> - if (c->cfg.src_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
> + if (bcm2835_dma_map_slave_addr(chan, c->cfg.src_addr,
> + c->cfg.src_addr_width,
> + DMA_TO_DEVICE))
> return NULL;
> - src = c->cfg.src_addr;
> +
> + src = c->map.addr;
> dst = buf_addr;
> } else {
> - if (c->cfg.dst_addr_width != DMA_SLAVE_BUSWIDTH_4_BYTES)
> + if (bcm2835_dma_map_slave_addr(chan, c->cfg.dst_addr,
> + c->cfg.dst_addr_width,
> + DMA_FROM_DEVICE))
> return NULL;
> - dst = c->cfg.dst_addr;
> +
> + dst = c->map.addr;
> src = buf_addr;
> }
>
> --
> 2.34.1
>

2024-06-05 18:17:02

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 08/18] dmaengine: bcm2835: pass dma_chan to generic functions

On Fri, May 24, 2024 at 07:26:52PM +0100, Dave Stevenson wrote:
> From: Stefan Wahren <[email protected]>
>
> In preparation to support more platforms pass the dma_chan to the
> generic functions. This provides access to the DMA device and possible
> platform specific data.

why need this change? you can easy convert between dma_chan and
bcm2835_chan.

>
> Signed-off-by: Stefan Wahren <[email protected]>
> Signed-off-by: Dave Stevenson <[email protected]>
> ---
> drivers/dma/bcm2835-dma.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index e2f9c8692e6b..aefaa1f01d7f 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -288,12 +288,13 @@ static void bcm2835_dma_desc_free(struct virt_dma_desc *vd)
> }
>
> static bool
> -bcm2835_dma_create_cb_set_length(struct bcm2835_chan *chan,
> +bcm2835_dma_create_cb_set_length(struct dma_chan *chan,
> struct bcm2835_dma_cb *control_block,
> size_t len, size_t period_len,
> size_t *total_len)
> {
> - size_t max_len = bcm2835_dma_max_frame_length(chan);
> + struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> + size_t max_len = bcm2835_dma_max_frame_length(c);
>
> /* set the length taking lite-channel limitations into account */
> control_block->length = min_t(u32, len, max_len);
> @@ -417,7 +418,7 @@ static struct bcm2835_desc *bcm2835_dma_create_cb_chain(
> /* set up length in control_block if requested */
> if (buf_len) {
> /* calculate length honoring period_length */
> - if (bcm2835_dma_create_cb_set_length(c, control_block,
> + if (bcm2835_dma_create_cb_set_length(chan, control_block,
> len, period_len,
> &total_len)) {
> /* add extrainfo bits in info */
> @@ -485,8 +486,9 @@ static void bcm2835_dma_fill_cb_chain_with_sg(
> }
> }
>
> -static void bcm2835_dma_abort(struct bcm2835_chan *c)
> +static void bcm2835_dma_abort(struct dma_chan *chan)
> {
> + struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> void __iomem *chan_base = c->chan_base;
> long int timeout = 10000;
>
> @@ -513,8 +515,9 @@ static void bcm2835_dma_abort(struct bcm2835_chan *c)
> writel(BCM2835_DMA_RESET, chan_base + BCM2835_DMA_CS);
> }
>
> -static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
> +static void bcm2835_dma_start_desc(struct dma_chan *chan)
> {
> + struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> struct virt_dma_desc *vd = vchan_next_desc(&c->vc);
> struct bcm2835_desc *d;
>
> @@ -533,7 +536,8 @@ static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
>
> static irqreturn_t bcm2835_dma_callback(int irq, void *data)
> {
> - struct bcm2835_chan *c = data;
> + struct dma_chan *chan = data;
> + struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
> struct bcm2835_desc *d;
> unsigned long flags;
>
> @@ -566,7 +570,7 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
> vchan_cyclic_callback(&d->vd);
> } else if (!readl(c->chan_base + BCM2835_DMA_ADDR)) {
> vchan_cookie_complete(&c->desc->vd);
> - bcm2835_dma_start_desc(c);
> + bcm2835_dma_start_desc(chan);
> }
> }
>
> @@ -594,7 +598,7 @@ static int bcm2835_dma_alloc_chan_resources(struct dma_chan *chan)
> }
>
> return request_irq(c->irq_number, bcm2835_dma_callback,
> - c->irq_flags, "DMA IRQ", c);
> + c->irq_flags, "DMA IRQ", chan);
> }
>
> static void bcm2835_dma_free_chan_resources(struct dma_chan *chan)
> @@ -682,7 +686,7 @@ static void bcm2835_dma_issue_pending(struct dma_chan *chan)
>
> spin_lock_irqsave(&c->vc.lock, flags);
> if (vchan_issue_pending(&c->vc) && !c->desc)
> - bcm2835_dma_start_desc(c);
> + bcm2835_dma_start_desc(chan);
>
> spin_unlock_irqrestore(&c->vc.lock, flags);
> }
> @@ -846,7 +850,7 @@ static int bcm2835_dma_terminate_all(struct dma_chan *chan)
> if (c->desc) {
> vchan_terminate_vdesc(&c->desc->vd);
> c->desc = NULL;
> - bcm2835_dma_abort(c);
> + bcm2835_dma_abort(chan);
> }
>
> vchan_get_all_descriptors(&c->vc, &head);
> --
> 2.34.1
>

2024-06-05 18:23:18

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 11/18] dmaengine: bcm2835: Use dma_map_resource to map addresses

On Fri, May 24, 2024 at 07:26:55PM +0100, Dave Stevenson wrote:
> There is a need to account for dma-ranges and iommus in the
> dma mapping process, and the public API for handling that is
> dma_map_resource.

what's means?

>
> Add support for mapping addresses to the DMA driver.
>
> Signed-off-by: Dave Stevenson <[email protected]>
> ---
> drivers/dma/bcm2835-dma.c | 46 ++++++++++++++++++++++++++++++++++-----
> 1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> index 9531c0b82071..e48008b06716 100644
> --- a/drivers/dma/bcm2835-dma.c
> +++ b/drivers/dma/bcm2835-dma.c
> @@ -67,6 +67,10 @@ struct bcm2835_cb_entry {
>
> struct bcm2835_dma_chan_map {
> dma_addr_t addr;
> + enum dma_data_direction dir;
> +
> + phys_addr_t slave_addr;
> + unsigned int xfer_size;
> };
>
> struct bcm2835_chan {
> @@ -294,12 +298,44 @@ static int bcm2835_dma_map_slave_addr(struct dma_chan *chan,
> return 0;
> }

where umap function? I suppose you should unmap all when terminate chan
or free chan by client driver.

>
> - /*
> - * This path will be updated to handle new clients, but currently should
> - * never be used.
> - */
> + if (dev_size != DMA_SLAVE_BUSWIDTH_4_BYTES)
> + return -EIO;
> +
> + /* Reuse current map if possible. */
> + if (dev_addr == map->slave_addr &&
> + dev_size == map->xfer_size &&
> + dev_dir == map->dir)
> + return 0;
> +
> + /* Remove old mapping if present. */
> + if (map->xfer_size) {
> + dev_dbg(chan->device->dev, "chan: unmap %zx@%pap to %pad dir: %s\n",
> + dev_size, &dev_addr, &map->addr,
> + dev_dir == DMA_TO_DEVICE ? "DMA_TO_DEVICE" : "DMA_FROM_DEVICE");
> + dma_unmap_resource(chan->device->dev, map->addr,
> + map->xfer_size, map->dir, 0);
> + }
> + map->xfer_size = 0;
>
> - return -EINVAL;
> + /* Create new slave address map. */
> + map->addr = dma_map_resource(chan->device->dev, dev_addr, dev_size,
> + dev_dir, 0);
> +
> + if (dma_mapping_error(chan->device->dev, map->addr)) {
> + dev_err(chan->device->dev, "chan: failed to map %zx@%pap",
> + dev_size, &dev_addr);
> + return -EIO;
> + }
> +
> + dev_dbg(chan->device->dev, "chan: map %zx@%pap to %pad dir: %s\n",
> + dev_size, &dev_addr, &map->addr,
> + dev_dir == DMA_TO_DEVICE ? "DMA_TO_DEVICE" : "DMA_FROM_DEVICE");
> +
> + map->slave_addr = dev_addr;
> + map->xfer_size = dev_size;
> + map->dir = dev_dir;
> +
> + return 0;
> }
>
> static void bcm2835_dma_free_cb_chain(struct bcm2835_desc *desc)
> --
> 2.34.1
>

2024-06-05 18:29:08

by Frank Li

[permalink] [raw]

Subject: Re: [PATCH 16/18] drm/vc4: Use phys addresses for slave DMA config

On Fri, May 24, 2024 at 07:27:00PM +0100, Dave Stevenson wrote:
> From: Phil Elwell <[email protected]>
>
> Slave addresses for DMA are meant to be supplied as physical addresses
> (contrary to what struct snd_dmaengine_dai_dma_data does).

Can you use the same content for patch 14-17?

Frank

>
> Signed-off-by: Phil Elwell <[email protected]>
> Signed-off-by: Dave Stevenson <[email protected]>
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index d30f8e8e8967..c2afd72bd96e 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -2696,7 +2696,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> struct snd_soc_card *card = &vc4_hdmi->audio.card;
> struct device *dev = &vc4_hdmi->pdev->dev;
> struct platform_device *codec_pdev;
> - const __be32 *addr;
> + struct resource *iomem;
> int index, len;
> int ret;
>
> @@ -2732,22 +2732,15 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
> }
>
> /*
> - * Get the physical address of VC4_HD_MAI_DATA. We need to retrieve
> - * the bus address specified in the DT, because the physical address
> - * (the one returned by platform_get_resource()) is not appropriate
> - * for DMA transfers.
> - * This VC/MMU should probably be exposed to avoid this kind of hacks.
> + * Get the physical address of VC4_HD_MAI_DATA.
> */
> index = of_property_match_string(dev->of_node, "reg-names", "hd");
> /* Before BCM2711, we don't have a named register range */
> if (index < 0)
> index = 1;
> + iomem = platform_get_resource(vc4_hdmi->pdev, IORESOURCE_MEM, index);
>
> - addr = of_get_address(dev->of_node, index, NULL, NULL);
> - if (!addr)
> - return -EINVAL;
> -
> - vc4_hdmi->audio.dma_data.addr = be32_to_cpup(addr) + mai_data->offset;
> + vc4_hdmi->audio.dma_data.addr = iomem->start + mai_data->offset;
> vc4_hdmi->audio.dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> vc4_hdmi->audio.dma_data.maxburst = 2;
>
> --
> 2.34.1
>