Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 26 Aug 2021 14:25:13 +0200
From:   Michael Walle <michael@walle.cc>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     etnaviv@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org,
        "Lukas F . Hartmann" <lukas@mntre.com>,
        Marek Vasut <marek.vasut@gmail.com>,
        Lucas Stach <l.stach@pengutronix.de>,
        Christian Gmeiner <christian.gmeiner@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH 3/3] drm/etnaviv: use a 32 bit mask as coherent DMA mask
In-Reply-To: <20210826121917.GS22278@shell.armlinux.org.uk>
References: <20210826121006.685257-1-michael@walle.cc>
 <20210826121006.685257-4-michael@walle.cc>
 <20210826121917.GS22278@shell.armlinux.org.uk>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <0f67c2a847963ee13ad02762866c94bf@walle.cc>
Precedence: bulk

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