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

[Michael Walle <michael@walle.cc>]

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