Message-ID: <faf23c39-af14-40ae-ad22-849aea87a4bd@linaro.org>
Date: Thu, 26 Jun 2025 11:04:50 +0100
From: James Clark <james.clark@...aro.org>
To: Arnd Bergmann <arnd@...db.de>, Frank Li <Frank.li@....com>,
 Vladimir Oltean <olteanv@...il.com>
Cc: Mark Brown <broonie@...nel.org>, Vladimir Oltean
 <vladimir.oltean@....com>, Larisa Grigore <larisa.grigore@....com>,
 Christoph Hellwig <hch@....de>, linux-spi@...r.kernel.org,
 imx@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions



On 25/06/2025 11:54 am, Arnd Bergmann wrote:
> On Wed, Jun 25, 2025, at 12:19, James Clark wrote:
>> On 25/06/2025 11:00 am, Arnd Bergmann wrote:
>>> On Wed, Jun 25, 2025, at 11:19, James Clark wrote:
>>>> On 24/06/2025 6:16 pm, Arnd Bergmann wrote:
>>
>> Wouldn't that allow someone to break it by disabling (or not enabling)
>> that option? The driver won't fall back to the other modes if DMA isn't
>> configured, it just won't work. In this case it seems like it's better
>> to depend directly on DMA_ENGINE because that fixes the randconfig
>> issues, which is the whole reason for the discussion.
> 
> It would be the same as disabling DMA_ENGINE today when running on an
> SoC that supports DMA mode in DSPI. Ideally that should fall back
> to non-accelerated mode. I see a lot of checks for
> trans_mode == DSPI_DMA_MODE, and I it's probably best to change
> them to a function call like
> 
> static inline bool dsp_dma_mode(struct fsl_dspi *dspi)
> {
>       if (!IS_ENABLED(CONFIG_DMA_ENGINE)) // or CONFIG_FSL_DSPI_USE_DMA
>            return false;
> 
>       return dspi->devtype_data->trans_mode == DSPI_DMA_MODE;
> }
> 

I think we'd have to check with Vladimir on that. He said that he didn't 
change some devices that use DMA to use XSPI because he couldn't verify 
that they would still work [1]. So by adding this fallback we would be 
making that decision now, which I'm not sure we can do. It would 
probably be better to add the stubs that make the probe fail rather than 
register a driver with a fallback that we don't know works.

>> Would someone really want an option to disable compilation of two
>> functions if their DSPI device is one that doesn't use DMA? Seems like
>> this option would always be on anyway.
> 
> It's probably mainly relevant if they want to completely turn off
> CONFIG_DMA_ENGINE, which is substantially bigger. Using a check
> for that symbol in the driver is certainly simpler for the user,
> as they can't accidentally turn it off the custom symbol.

That was my thinking.

> 
> In theory you may also want to turn off DMA mode for testing,
> which is supported by at least the DW_DMA driver.
> 
> I see that SPI_TEGRA114 has a dependency on TEGRA20_APB_DMA,
> which is yet another variation. This is clearly done for
> usability purposes since that SPI driver only ever works with
> the specific DMA driver in practice, but it seems worse
> conceptually.
> 
>       Arnd

I can add it, I'm just questioning the usefulness of adding a new 
config option that's unlikely to be used in reality. It only achieves 
the same thing as doing #if CONFIG_DMA_ENGINE in one place in the code.

Ultimately we're just trying to fix some randconfig error, if we start 
adding new knobs for people to turn just do that, there's a risk they 
won't get used and we'll make configuring more complicated for people.

Testing might be a partial justification, but that config wouldn't allow 
you to force DMA mode where XSPI is used. You would almost certainly be 
doing that too if you are doing that kind of testing, and you'd have to 
hack the driver anyway to do that.

[1]: https://lkml.org/lkml/2025/6/12/1385

Powered by blists - more mailing lists