Re: [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask

[Michael Walle]

Am 2021-08-26 14:19, schrieb Russell King (Oracle):
> On Thu, Aug 26, 2021 at 02:10:06PM +0200, Michael Walle wrote:
> > -	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40);
> > -	pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> > +	/*
> > +	 * PTA and MTLB can have 40 bit base addresses, but
> > +	 * unfortunately, an entry in the MTLB can only point to a
> > +	 * 32 bit base address of a STLB. Moreover, to initialize the
> > +	 * MMU we need a command buffer with a 32 bit address because
> > +	 * without an MMU there is only an indentity mapping between
> > +	 * the internal 32 bit addresses and the bus addresses.
> > +	 *
> > +	 * To make things easy, we set the dma_coherent_mask to 32
> > +	 * bit to make sure we are allocating the command buffers and
> > +	 * TLBs in the lower 4 GiB address space.
> > +	 */
> > +	if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(40)) ||
> > +	    dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32))) {
> > +		dev_dbg(&pdev->dev, "No suitable DMA available\n");
> > +		return -ENODEV;
> > +	}
>
> This makes no sense. In the previous patch, you initialised
> pdev->dev.dma_mask ot point at the coherent mask, implying that
> it wasn't already set - for which dma_coerce_mask_and_coherent()
> should be used. Now you're just calling dma_set_mask(), which will
> fail if pdev->dev.dma_mask hasn't already been set to point at
> something.
>
> If it's already been initialised to point at something, then you
> shouldn't be overwriting it in the driver, and you should've used
> dma_set_mask_and_coherent() in your previous patch.
>
> Confused.

Mh, I see that moving these two lines was a bad idea. See commit
message in patch 2/3:
> Also move the dma mask assignemnts to probe() to keep all DMA related
> settings together.

The actual fix in patch 2/3 is the move of the of_dma_configure()
not the dma_mask assignments.

-michael