Linux-sh Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
@ 2020-09-01 15:04 Ulf Hansson
  2020-09-01 15:06 ` Christoph Hellwig
  0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2020-09-01 15:04 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson, Rich Felker, Christoph Hellwig
  Cc: Mark Brown, linux-sh, linux-kernel

The commit cd57d07b1e4e ("sh: don't allow non-coherent DMA for NOMMU") made
CONFIG_NO_DMA to be set for some platforms, for good reasons.
Consequentially, CONFIG_HAS_DMA doesn't get set, which makes the DMA
mapping interface to be built as stub functions, but also prevent the
mmc_spi driver from being built as it depends on CONFIG_HAS_DMA.

It turns out that for some odd cases, the driver still relied on the DMA
mapping interface, even if the DMA was not actively being used.

To fixup the behaviour, let's drop the build dependency for CONFIG_HAS_DMA.
Moreover, as to allow the driver to succeed probing, let's move the DMA
initializations behind "#ifdef CONFIG_HAS_DMA".

Fixes: cd57d07b1e4e ("sh: don't allow non-coherent DMA for NOMMU")
Reported-by: Rich Felker <dalias@libc.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Drop build dependency to CONFIG_HAS_DMA.
	- Rephrase commit message and its header, to reflect the updated change.

---
 drivers/mmc/host/Kconfig   |  2 +-
 drivers/mmc/host/mmc_spi.c | 86 +++++++++++++++++++++++---------------
 2 files changed, 53 insertions(+), 35 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9c89a5b780e8..9a34c827c96e 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -602,7 +602,7 @@ config MMC_GOLDFISH
 
 config MMC_SPI
 	tristate "MMC/SD/SDIO over SPI"
-	depends on SPI_MASTER && HAS_DMA
+	depends on SPI_MASTER
 	select CRC7
 	select CRC_ITU_T
 	help
diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 39bb1e30c2d7..5055a7eb134a 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -1278,6 +1278,52 @@ mmc_spi_detect_irq(int irq, void *mmc)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_HAS_DMA
+static int mmc_spi_dma_alloc(struct mmc_spi_host *host)
+{
+	struct spi_device *spi = host->spi;
+	struct device *dev;
+
+	if (!spi->master->dev.parent->dma_mask)
+		return 0;
+
+	dev = spi->master->dev.parent;
+
+	host->ones_dma = dma_map_single(dev, host->ones, MMC_SPI_BLOCKSIZE,
+					DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, host->ones_dma))
+		return -ENOMEM;
+
+	host->data_dma = dma_map_single(dev, host->data, sizeof(*host->data),
+					DMA_BIDIRECTIONAL);
+	if (dma_mapping_error(dev, host->data_dma)) {
+		dma_unmap_single(dev, host->ones_dma, MMC_SPI_BLOCKSIZE,
+				 DMA_TO_DEVICE);
+		return -ENOMEM;
+	}
+
+	dma_sync_single_for_cpu(dev, host->data_dma, sizeof(*host->data),
+				DMA_BIDIRECTIONAL);
+
+	host->dma_dev = dev;
+	return 0;
+}
+
+static void mmc_spi_dma_free(struct mmc_spi_host *host)
+{
+	if (!host->dma_dev)
+		return;
+
+	dma_unmap_single(host->dma_dev, host->ones_dma, MMC_SPI_BLOCKSIZE,
+			 DMA_TO_DEVICE);
+	dma_unmap_single(host->dma_dev, host->data_dma,	sizeof(*host->data),
+			 DMA_BIDIRECTIONAL);
+}
+#else
+static inline mmc_spi_dma_alloc(struct mmc_spi_host *host) { return 0; }
+static inline void mmc_spi_dma_free(struct mmc_spi_host *host) {}
+#endif
+
 static int mmc_spi_probe(struct spi_device *spi)
 {
 	void			*ones;
@@ -1374,23 +1420,9 @@ static int mmc_spi_probe(struct spi_device *spi)
 	if (!host->data)
 		goto fail_nobuf1;
 
-	if (spi->master->dev.parent->dma_mask) {
-		struct device	*dev = spi->master->dev.parent;
-
-		host->dma_dev = dev;
-		host->ones_dma = dma_map_single(dev, ones,
-				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
-		if (dma_mapping_error(dev, host->ones_dma))
-			goto fail_ones_dma;
-		host->data_dma = dma_map_single(dev, host->data,
-				sizeof(*host->data), DMA_BIDIRECTIONAL);
-		if (dma_mapping_error(dev, host->data_dma))
-			goto fail_data_dma;
-
-		dma_sync_single_for_cpu(host->dma_dev,
-				host->data_dma, sizeof(*host->data),
-				DMA_BIDIRECTIONAL);
-	}
+	status = mmc_spi_dma_alloc(host);
+	if (status)
+		goto fail_dma;
 
 	/* setup message for status/busy readback */
 	spi_message_init(&host->readback);
@@ -1458,20 +1490,12 @@ static int mmc_spi_probe(struct spi_device *spi)
 fail_add_host:
 	mmc_remove_host(mmc);
 fail_glue_init:
-	if (host->dma_dev)
-		dma_unmap_single(host->dma_dev, host->data_dma,
-				sizeof(*host->data), DMA_BIDIRECTIONAL);
-fail_data_dma:
-	if (host->dma_dev)
-		dma_unmap_single(host->dma_dev, host->ones_dma,
-				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
-fail_ones_dma:
+	mmc_spi_dma_free(host);
+fail_dma:
 	kfree(host->data);
-
 fail_nobuf1:
 	mmc_free_host(mmc);
 	mmc_spi_put_pdata(spi);
-
 nomem:
 	kfree(ones);
 	return status;
@@ -1489,13 +1513,7 @@ static int mmc_spi_remove(struct spi_device *spi)
 
 	mmc_remove_host(mmc);
 
-	if (host->dma_dev) {
-		dma_unmap_single(host->dma_dev, host->ones_dma,
-			MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
-		dma_unmap_single(host->dma_dev, host->data_dma,
-			sizeof(*host->data), DMA_BIDIRECTIONAL);
-	}
-
+	mmc_spi_dma_free(host);
 	kfree(host->data);
 	kfree(host->ones);
 
-- 
2.25.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-01 15:04 [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset Ulf Hansson
@ 2020-09-01 15:06 ` Christoph Hellwig
  2020-09-01 15:36   ` Ulf Hansson
  0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2020-09-01 15:06 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, Rich Felker, Christoph Hellwig, Mark Brown, linux-sh,
	linux-kernel

On Tue, Sep 01, 2020 at 05:04:38PM +0200, Ulf Hansson wrote:
> +#ifdef CONFIG_HAS_DMA
> +static int mmc_spi_dma_alloc(struct mmc_spi_host *host)
> +{
> +	struct spi_device *spi = host->spi;
> +	struct device *dev;
> +
> +	if (!spi->master->dev.parent->dma_mask)
> +		return 0;

I still don't think this makes sense, as the dma_mask should always
be non-NULL here.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-01 15:06 ` Christoph Hellwig
@ 2020-09-01 15:36   ` Ulf Hansson
  2020-09-01 15:40     ` Christoph Hellwig
  0 siblings, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2020-09-01 15:36 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-mmc, Rich Felker, Mark Brown, Linux-sh list,
	Linux Kernel Mailing List

On Tue, 1 Sep 2020 at 17:06, Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Sep 01, 2020 at 05:04:38PM +0200, Ulf Hansson wrote:
> > +#ifdef CONFIG_HAS_DMA
> > +static int mmc_spi_dma_alloc(struct mmc_spi_host *host)
> > +{
> > +     struct spi_device *spi = host->spi;
> > +     struct device *dev;
> > +
> > +     if (!spi->master->dev.parent->dma_mask)
> > +             return 0;
>
> I still don't think this makes sense, as the dma_mask should always
> be non-NULL here.

If that is the case, I wonder how the driver could even have worked without DMA.

Because in the existing code, host->dma_dev gets assigned to
spi->master->dev.parent->dma_mask - which seems to turn on the DMA
usage in the driver.

What am I missing?

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-01 15:36   ` Ulf Hansson
@ 2020-09-01 15:40     ` Christoph Hellwig
  2020-09-02  8:31       ` Ulf Hansson
  2020-09-02 13:34       ` Rich Felker
  0 siblings, 2 replies; 10+ messages in thread
From: Christoph Hellwig @ 2020-09-01 15:40 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Christoph Hellwig, linux-mmc, Rich Felker, Mark Brown,
	Linux-sh list, Linux Kernel Mailing List

On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > I still don't think this makes sense, as the dma_mask should always
> > be non-NULL here.
> 
> If that is the case, I wonder how the driver could even have worked without DMA.
> 
> Because in the existing code, host->dma_dev gets assigned to
> spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> usage in the driver.
> 
> What am I missing?

Do you know of other non-DMA users?  For SH nommu it probably worked
because SH nommu used to provide a DMA implementation that worked
fine for streaming maps, but was completely broken for coherent
allocation.  And this driver appears to only use the former.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-01 15:40     ` Christoph Hellwig
@ 2020-09-02  8:31       ` Ulf Hansson
  2020-09-02 13:44         ` Rich Felker
  2020-09-02 13:34       ` Rich Felker
  1 sibling, 1 reply; 10+ messages in thread
From: Ulf Hansson @ 2020-09-02  8:31 UTC (permalink / raw)
  To: Christoph Hellwig, Rich Felker
  Cc: linux-mmc, Mark Brown, Linux-sh list, Linux Kernel Mailing List

On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > I still don't think this makes sense, as the dma_mask should always
> > > be non-NULL here.
> >
> > If that is the case, I wonder how the driver could even have worked without DMA.
> >
> > Because in the existing code, host->dma_dev gets assigned to
> > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > usage in the driver.
> >
> > What am I missing?
>
> Do you know of other non-DMA users?  For SH nommu it probably worked

I don't know of other non-DMA users. As I said, I wish someone could
step in and take better care of mmc_spi - as I know it's being used a
lot.

> because SH nommu used to provide a DMA implementation that worked
> fine for streaming maps, but was completely broken for coherent
> allocation.  And this driver appears to only use the former.

Alright, so you are saying the DMA support may potentially never have
been optional to this driver. In any case, I can remove the check in
$subject patch, as it shouldn't matter.

Anyway, let's see what Rich thinks of this. I am curious to see if the
patch works on his SH boards - as I haven't been able to test it.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-01 15:40     ` Christoph Hellwig
  2020-09-02  8:31       ` Ulf Hansson
@ 2020-09-02 13:34       ` Rich Felker
  1 sibling, 0 replies; 10+ messages in thread
From: Rich Felker @ 2020-09-02 13:34 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ulf Hansson, linux-mmc, Mark Brown, Linux-sh list,
	Linux Kernel Mailing List

On Tue, Sep 01, 2020 at 05:40:49PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > I still don't think this makes sense, as the dma_mask should always
> > > be non-NULL here.
> > 
> > If that is the case, I wonder how the driver could even have worked without DMA.
> > 
> > Because in the existing code, host->dma_dev gets assigned to
> > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > usage in the driver.
> > 
> > What am I missing?
> 
> Do you know of other non-DMA users?  For SH nommu it probably worked
> because SH nommu used to provide a DMA implementation that worked
> fine for streaming maps, but was completely broken for coherent
> allocation.  And this driver appears to only use the former.

On the system we're using it on, there's no DMA whatsoever. It just
used to allow memory allocations suitable for DMA (which any
allocation vacuuously is when there's no DMA) but now it doesn't, due
to your change.

Just below the if block in question in this thread is:

	host->readback.is_dma_mapped = (host->dma_dev != NULL);

and similar conditions appear elsewhere all over the file in the form
of if (host->dma_dev). Of course doing DMA requires a link to a DMA
controller device, and plenty SPI devices (most obviously bit-banged
ones) lack DMA.

The right condition for the block in question here is probably just
checking dma_dev instead of dma_mask or CONFIG_HAS_DMA, but it seems
useful to optimize out the code if CONFIG_HAS_DMA is false, anyway.

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-02  8:31       ` Ulf Hansson
@ 2020-09-02 13:44         ` Rich Felker
  2020-09-02 15:51           ` Geert Uytterhoeven
  0 siblings, 1 reply; 10+ messages in thread
From: Rich Felker @ 2020-09-02 13:44 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Christoph Hellwig, linux-mmc, Mark Brown, Linux-sh list,
	Linux Kernel Mailing List

On Wed, Sep 02, 2020 at 10:31:47AM +0200, Ulf Hansson wrote:
> On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > > I still don't think this makes sense, as the dma_mask should always
> > > > be non-NULL here.
> > >
> > > If that is the case, I wonder how the driver could even have worked without DMA.
> > >
> > > Because in the existing code, host->dma_dev gets assigned to
> > > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > > usage in the driver.
> > >
> > > What am I missing?
> >
> > Do you know of other non-DMA users?  For SH nommu it probably worked
> 
> I don't know of other non-DMA users. As I said, I wish someone could
> step in and take better care of mmc_spi - as I know it's being used a
> lot.
> 
> > because SH nommu used to provide a DMA implementation that worked
> > fine for streaming maps, but was completely broken for coherent
> > allocation.  And this driver appears to only use the former.
> 
> Alright, so you are saying the DMA support may potentially never have
> been optional to this driver. In any case, I can remove the check in
> $subject patch, as it shouldn't matter.

DMA support was always optional, because even on systems where DMA is
present, it doesn't necessarily mean the SPI controller uses DMA. In
particular, pure bit-banged SPI via GPIOs doesn't have DMA, but has
always worked. See my previous reply to Christoph about host->dma_dev
for my current-best understanding of what's going on here.

> Anyway, let's see what Rich thinks of this. I am curious to see if the
> patch works on his SH boards - as I haven't been able to test it.

I'll rebuild and retest just to confirm, but I already tested a
functionally equivalent patch that just did the #ifdef inline (rather
than moving the logic out to separate functions) and it worked fine.

Rich

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-02 13:44         ` Rich Felker
@ 2020-09-02 15:51           ` Geert Uytterhoeven
  2020-09-03  0:41             ` Rich Felker
  0 siblings, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2020-09-02 15:51 UTC (permalink / raw)
  To: Rich Felker
  Cc: Ulf Hansson, Christoph Hellwig, linux-mmc, Mark Brown,
	Linux-sh list, Linux Kernel Mailing List

Hi Rich,

On Wed, Sep 2, 2020 at 5:43 PM Rich Felker <dalias@libc.org> wrote:
> On Wed, Sep 02, 2020 at 10:31:47AM +0200, Ulf Hansson wrote:
> > On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
> > > On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > > > I still don't think this makes sense, as the dma_mask should always
> > > > > be non-NULL here.
> > > >
> > > > If that is the case, I wonder how the driver could even have worked without DMA.
> > > >
> > > > Because in the existing code, host->dma_dev gets assigned to
> > > > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > > > usage in the driver.
> > > >
> > > > What am I missing?
> > >
> > > Do you know of other non-DMA users?  For SH nommu it probably worked
> >
> > I don't know of other non-DMA users. As I said, I wish someone could
> > step in and take better care of mmc_spi - as I know it's being used a
> > lot.
> >
> > > because SH nommu used to provide a DMA implementation that worked
> > > fine for streaming maps, but was completely broken for coherent
> > > allocation.  And this driver appears to only use the former.
> >
> > Alright, so you are saying the DMA support may potentially never have
> > been optional to this driver. In any case, I can remove the check in
> > $subject patch, as it shouldn't matter.
>
> DMA support was always optional, because even on systems where DMA is
> present, it doesn't necessarily mean the SPI controller uses DMA. In
> particular, pure bit-banged SPI via GPIOs doesn't have DMA, but has
> always worked. See my previous reply to Christoph about host->dma_dev
> for my current-best understanding of what's going on here.
>
> > Anyway, let's see what Rich thinks of this. I am curious to see if the
> > patch works on his SH boards - as I haven't been able to test it.
>
> I'll rebuild and retest just to confirm, but I already tested a
> functionally equivalent patch that just did the #ifdef inline (rather
> than moving the logic out to separate functions) and it worked fine.

Hence, Tested-by? ;-)

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-02 15:51           ` Geert Uytterhoeven
@ 2020-09-03  0:41             ` Rich Felker
  2020-09-03  8:10               ` Ulf Hansson
  0 siblings, 1 reply; 10+ messages in thread
From: Rich Felker @ 2020-09-03  0:41 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Ulf Hansson, Christoph Hellwig, linux-mmc, Mark Brown,
	Linux-sh list, Linux Kernel Mailing List

On Wed, Sep 02, 2020 at 05:51:16PM +0200, Geert Uytterhoeven wrote:
> Hi Rich,
> 
> On Wed, Sep 2, 2020 at 5:43 PM Rich Felker <dalias@libc.org> wrote:
> > On Wed, Sep 02, 2020 at 10:31:47AM +0200, Ulf Hansson wrote:
> > > On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
> > > > On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > > > > I still don't think this makes sense, as the dma_mask should always
> > > > > > be non-NULL here.
> > > > >
> > > > > If that is the case, I wonder how the driver could even have worked without DMA.
> > > > >
> > > > > Because in the existing code, host->dma_dev gets assigned to
> > > > > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > > > > usage in the driver.
> > > > >
> > > > > What am I missing?
> > > >
> > > > Do you know of other non-DMA users?  For SH nommu it probably worked
> > >
> > > I don't know of other non-DMA users. As I said, I wish someone could
> > > step in and take better care of mmc_spi - as I know it's being used a
> > > lot.
> > >
> > > > because SH nommu used to provide a DMA implementation that worked
> > > > fine for streaming maps, but was completely broken for coherent
> > > > allocation.  And this driver appears to only use the former.
> > >
> > > Alright, so you are saying the DMA support may potentially never have
> > > been optional to this driver. In any case, I can remove the check in
> > > $subject patch, as it shouldn't matter.
> >
> > DMA support was always optional, because even on systems where DMA is
> > present, it doesn't necessarily mean the SPI controller uses DMA. In
> > particular, pure bit-banged SPI via GPIOs doesn't have DMA, but has
> > always worked. See my previous reply to Christoph about host->dma_dev
> > for my current-best understanding of what's going on here.
> >
> > > Anyway, let's see what Rich thinks of this. I am curious to see if the
> > > patch works on his SH boards - as I haven't been able to test it.
> >
> > I'll rebuild and retest just to confirm, but I already tested a
> > functionally equivalent patch that just did the #ifdef inline (rather
> > than moving the logic out to separate functions) and it worked fine.
> 
> Hence, Tested-by? ;-)

Confirmed that this version of the patch works too. Thus,

Tested-by: Rich Felker <dalias@libc.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset
  2020-09-03  0:41             ` Rich Felker
@ 2020-09-03  8:10               ` Ulf Hansson
  0 siblings, 0 replies; 10+ messages in thread
From: Ulf Hansson @ 2020-09-03  8:10 UTC (permalink / raw)
  To: Rich Felker, Christoph Hellwig
  Cc: Geert Uytterhoeven, linux-mmc, Mark Brown, Linux-sh list,
	Linux Kernel Mailing List

On Thu, 3 Sep 2020 at 02:41, Rich Felker <dalias@libc.org> wrote:
>
> On Wed, Sep 02, 2020 at 05:51:16PM +0200, Geert Uytterhoeven wrote:
> > Hi Rich,
> >
> > On Wed, Sep 2, 2020 at 5:43 PM Rich Felker <dalias@libc.org> wrote:
> > > On Wed, Sep 02, 2020 at 10:31:47AM +0200, Ulf Hansson wrote:
> > > > On Tue, 1 Sep 2020 at 17:40, Christoph Hellwig <hch@lst.de> wrote:
> > > > > On Tue, Sep 01, 2020 at 05:36:17PM +0200, Ulf Hansson wrote:
> > > > > > > I still don't think this makes sense, as the dma_mask should always
> > > > > > > be non-NULL here.
> > > > > >
> > > > > > If that is the case, I wonder how the driver could even have worked without DMA.
> > > > > >
> > > > > > Because in the existing code, host->dma_dev gets assigned to
> > > > > > spi->master->dev.parent->dma_mask - which seems to turn on the DMA
> > > > > > usage in the driver.
> > > > > >
> > > > > > What am I missing?
> > > > >
> > > > > Do you know of other non-DMA users?  For SH nommu it probably worked
> > > >
> > > > I don't know of other non-DMA users. As I said, I wish someone could
> > > > step in and take better care of mmc_spi - as I know it's being used a
> > > > lot.
> > > >
> > > > > because SH nommu used to provide a DMA implementation that worked
> > > > > fine for streaming maps, but was completely broken for coherent
> > > > > allocation.  And this driver appears to only use the former.
> > > >
> > > > Alright, so you are saying the DMA support may potentially never have
> > > > been optional to this driver. In any case, I can remove the check in
> > > > $subject patch, as it shouldn't matter.
> > >
> > > DMA support was always optional, because even on systems where DMA is
> > > present, it doesn't necessarily mean the SPI controller uses DMA. In
> > > particular, pure bit-banged SPI via GPIOs doesn't have DMA, but has
> > > always worked. See my previous reply to Christoph about host->dma_dev
> > > for my current-best understanding of what's going on here.
> > >
> > > > Anyway, let's see what Rich thinks of this. I am curious to see if the
> > > > patch works on his SH boards - as I haven't been able to test it.
> > >
> > > I'll rebuild and retest just to confirm, but I already tested a
> > > functionally equivalent patch that just did the #ifdef inline (rather
> > > than moving the logic out to separate functions) and it worked fine.
> >
> > Hence, Tested-by? ;-)
>
> Confirmed that this version of the patch works too. Thus,
>
> Tested-by: Rich Felker <dalias@libc.org>

I have applied the patch for fixes, thanks for testing!

Christoph, when it comes to the check of
"spi->master->dev.parent->dma_mask", I am keeping it for now. I am
simply not sure that all spi masters assign the pointer (even if most
are platform drivers). I think it's better that we remove that check
in a separate patch - to get it tested.

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 15:04 [PATCH v2] mmc: mmc_spi: Allow the driver to be built when CONFIG_HAS_DMA is unset Ulf Hansson
2020-09-01 15:06 ` Christoph Hellwig
2020-09-01 15:36   ` Ulf Hansson
2020-09-01 15:40     ` Christoph Hellwig
2020-09-02  8:31       ` Ulf Hansson
2020-09-02 13:44         ` Rich Felker
2020-09-02 15:51           ` Geert Uytterhoeven
2020-09-03  0:41             ` Rich Felker
2020-09-03  8:10               ` Ulf Hansson
2020-09-02 13:34       ` Rich Felker

Linux-sh Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sh/0 linux-sh/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sh linux-sh/ https://lore.kernel.org/linux-sh \
		linux-sh@vger.kernel.org
	public-inbox-index linux-sh

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sh


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git