This will allow the build to succeed with !CONFIG_HAS_DMA, either due to
a randconfig build test or when the target only uses one of the non-DMA
transfer modes which this driver supports.
Signed-off-by: James Clark <james.clark@linaro.org>
---
drivers/spi/spi-fsl-dspi.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index feb29bb92a77..8212c4193536 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -373,6 +373,8 @@ struct fsl_dspi {
void (*dev_to_host)(struct fsl_dspi *dspi, u32 rxdata);
};
+static void dspi_setup_accel(struct fsl_dspi *dspi);
+
static bool is_s32g_dspi(struct fsl_dspi *data)
{
return data->devtype_data == &devtype_data[S32G] ||
@@ -489,6 +491,7 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
dspi->dev_to_host(dspi, rxdata);
}
+#if IS_ENABLED(CONFIG_HAS_DMA)
static void dspi_tx_dma_callback(void *arg)
{
struct fsl_dspi *dspi = arg;
@@ -589,8 +592,6 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
return 0;
}
-static void dspi_setup_accel(struct fsl_dspi *dspi);
-
static void dspi_dma_xfer(struct fsl_dspi *dspi)
{
struct spi_message *message = dspi->cur_msg;
@@ -722,6 +723,18 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
dma_release_channel(dma->chan_rx);
}
}
+#else
+static void dspi_dma_xfer(struct fsl_dspi *dspi)
+{
+ sdpi->cur_msg->status = -EINVAL;
+}
+static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
+{
+ dev_err(&dspi->pdev->dev, "DMA support not enabled in kernel\n");
+ return -EINVAL;
+}
+static void dspi_release_dma(struct fsl_dspi *dspi) {}
+#endif
static void hz_to_spi_baud(char *pbr, char *br, int speed_hz,
unsigned long clkrate, bool mtf_enabled)
--
2.34.1Hi James,
kernel test robot noticed the following build errors:
[auto build test ERROR on 4f326fa6236787ca516ea6eab8e5e9dc5c236f03]
url: https://github.com/intel-lab-lkp/linux/commits/James-Clark/spi-spi-fsl-dspi-Clear-completion-counter-before-initiating-transfer/20250624-183952
base: 4f326fa6236787ca516ea6eab8e5e9dc5c236f03
patch link: https://lore.kernel.org/r/20250624-james-nxp-spi-dma-v3-3-e7d574f5f62c%40linaro.org
patch subject: [PATCH v3 3/6] spi: spi-fsl-dspi: Stub out DMA functions
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20250625/202506251332.thYB4ced-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250625/202506251332.thYB4ced-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506251332.thYB4ced-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
drivers/spi/spi-fsl-dspi.c: In function 'dspi_dma_xfer':
>> drivers/spi/spi-fsl-dspi.c:729:9: error: 'sdpi' undeclared (first use in this function); did you mean 'dspi'?
729 | sdpi->cur_msg->status = -EINVAL;
| ^~~~
| dspi
drivers/spi/spi-fsl-dspi.c:729:9: note: each undeclared identifier is reported only once for each function it appears in
drivers/spi/spi-fsl-dspi.c: At top level:
>> drivers/spi/spi-fsl-dspi.c:474:12: warning: 'dspi_pop_tx_pushr' defined but not used [-Wunused-function]
474 | static u32 dspi_pop_tx_pushr(struct fsl_dspi *dspi)
| ^~~~~~~~~~~~~~~~~
vim +729 drivers/spi/spi-fsl-dspi.c
705
706 static void dspi_release_dma(struct fsl_dspi *dspi)
707 {
708 int dma_bufsize = dspi->devtype_data->fifo_size * 2;
709 struct fsl_dspi_dma *dma = dspi->dma;
710
711 if (!dma)
712 return;
713
714 if (dma->chan_tx) {
715 dma_free_coherent(dma->chan_tx->device->dev, dma_bufsize,
716 dma->tx_dma_buf, dma->tx_dma_phys);
717 dma_release_channel(dma->chan_tx);
718 }
719
720 if (dma->chan_rx) {
721 dma_free_coherent(dma->chan_rx->device->dev, dma_bufsize,
722 dma->rx_dma_buf, dma->rx_dma_phys);
723 dma_release_channel(dma->chan_rx);
724 }
725 }
726 #else
727 static void dspi_dma_xfer(struct fsl_dspi *dspi)
728 {
> 729 sdpi->cur_msg->status = -EINVAL;
730 }
731 static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
732 {
733 dev_err(&dspi->pdev->dev, "DMA support not enabled in kernel\n");
734 return -EINVAL;
735 }
736 static void dspi_release_dma(struct fsl_dspi *dspi) {}
737 #endif
738
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Tue, Jun 24, 2025 at 11:35:33AM +0100, James Clark wrote:
> This will allow the build to succeed with !CONFIG_HAS_DMA, either due to
> a randconfig build test or when the target only uses one of the non-DMA
I supposed you met kbuild error. If yes, can you add kbuild build report
tags.
Frank
> transfer modes which this driver supports.
>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
> drivers/spi/spi-fsl-dspi.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index feb29bb92a77..8212c4193536 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -373,6 +373,8 @@ struct fsl_dspi {
> void (*dev_to_host)(struct fsl_dspi *dspi, u32 rxdata);
> };
>
> +static void dspi_setup_accel(struct fsl_dspi *dspi);
> +
> static bool is_s32g_dspi(struct fsl_dspi *data)
> {
> return data->devtype_data == &devtype_data[S32G] ||
> @@ -489,6 +491,7 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
> dspi->dev_to_host(dspi, rxdata);
> }
>
> +#if IS_ENABLED(CONFIG_HAS_DMA)
> static void dspi_tx_dma_callback(void *arg)
> {
> struct fsl_dspi *dspi = arg;
> @@ -589,8 +592,6 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
> return 0;
> }
>
> -static void dspi_setup_accel(struct fsl_dspi *dspi);
> -
> static void dspi_dma_xfer(struct fsl_dspi *dspi)
> {
> struct spi_message *message = dspi->cur_msg;
> @@ -722,6 +723,18 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
> dma_release_channel(dma->chan_rx);
> }
> }
> +#else
> +static void dspi_dma_xfer(struct fsl_dspi *dspi)
> +{
> + sdpi->cur_msg->status = -EINVAL;
> +}
> +static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
> +{
> + dev_err(&dspi->pdev->dev, "DMA support not enabled in kernel\n");
> + return -EINVAL;
> +}
> +static void dspi_release_dma(struct fsl_dspi *dspi) {}
> +#endif
>
> static void hz_to_spi_baud(char *pbr, char *br, int speed_hz,
> unsigned long clkrate, bool mtf_enabled)
>
> --
> 2.34.1
>
On Tue, Jun 24, 2025, at 18:29, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:33AM +0100, James Clark wrote:
>> This will allow the build to succeed with !CONFIG_HAS_DMA, either due to
>> a randconfig build test or when the target only uses one of the non-DMA
>
> I supposed you met kbuild error. If yes, can you add kbuild build report
> tags.
Actually I would suggest making it a dependency on CONFIG_DMA_ENGINE
instead of CONFIG_HAS_DMA, since that is the more relevant symbol.
It would also be simpler to enforce this in Kconfig if we only
care about users that use the DMA support.
Arnd
On 24/06/2025 6:16 pm, Arnd Bergmann wrote: > On Tue, Jun 24, 2025, at 18:29, Frank Li wrote: >> On Tue, Jun 24, 2025 at 11:35:33AM +0100, James Clark wrote: >>> This will allow the build to succeed with !CONFIG_HAS_DMA, either due to >>> a randconfig build test or when the target only uses one of the non-DMA >> >> I supposed you met kbuild error. If yes, can you add kbuild build report >> tags. > > Actually I would suggest making it a dependency on CONFIG_DMA_ENGINE > instead of CONFIG_HAS_DMA, since that is the more relevant symbol. > Makes sense. > It would also be simpler to enforce this in Kconfig if we only > care about users that use the DMA support. > > Arnd But most of the devices supported by the driver don't do any DMA. That was the reason to stub them out rather than add the Kconfig depends.
On Wed, Jun 25, 2025, at 11:19, James Clark wrote:
> On 24/06/2025 6:16 pm, Arnd Bergmann wrote:
>> On Tue, Jun 24, 2025, at 18:29, Frank Li wrote:
>
>> It would also be simpler to enforce this in Kconfig if we only
>> care about users that use the DMA support.
>
> But most of the devices supported by the driver don't do any DMA. That
> was the reason to stub them out rather than add the Kconfig depends.
Ah right. So even when running on SoCs that have a DMA engine,
you can end up not using it?
In this case you could have an extra Kconfig symbol to configure
DMA support for this driver and use that in the source code:
config SPI_FSL_DSPI_DMA
bool "Use DMA engine for offloading Freescale DSPI transfers"
depends on SPI_FSL_DSPI && DMA_ENGINE
help
....
Arnd
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: >>> On Tue, Jun 24, 2025, at 18:29, Frank Li wrote: >> >>> It would also be simpler to enforce this in Kconfig if we only >>> care about users that use the DMA support. >> >> But most of the devices supported by the driver don't do any DMA. That >> was the reason to stub them out rather than add the Kconfig depends. > > Ah right. So even when running on SoCs that have a DMA engine, > you can end up not using it? > Yes > In this case you could have an extra Kconfig symbol to configure > DMA support for this driver and use that in the source code: > > config SPI_FSL_DSPI_DMA > bool "Use DMA engine for offloading Freescale DSPI transfers" > depends on SPI_FSL_DSPI && DMA_ENGINE > help > .... > > > Arnd 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. 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.
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;
}
> 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.
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
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
On Thu, Jun 26, 2025 at 11:04:50AM +0100, James Clark wrote: > On 25/06/2025 11:54 am, Arnd Bergmann wrote: > > 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. > 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. Please just keep things simple, as you say nobody is likely to be actually trying to run such a configuration practically.
© 2016 - 2025 Red Hat, Inc.