[4/4] mmc: sdhci: Add DMA memory boundary workaround

Message ID 20191202144104.5069-5-jun.nie@linaro.org
State New
Headers show
Series
  • mmc: Add sdhci workaround stability enhencement
Related show

Commit Message

Jun Nie Dec. 2, 2019, 2:41 p.m.
DMA memory cannot cross specific boundary for some SDHCI controller,
such as DesignWare SDHCI controller. Add DMA memory boundary dt
property and workaround the limitation.

Signed-off-by: Jun Nie <jun.nie@linaro.org>

---
 drivers/mmc/host/sdhci.c | 20 +++++++++++++++++++-
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Jisheng Zhang Dec. 3, 2019, 2:47 a.m. | #1
On Mon,  2 Dec 2019 22:41:04 +0800 Jun Nie wrote:


> 

> 

> DMA memory cannot cross specific boundary for some SDHCI controller,

> such as DesignWare SDHCI controller. Add DMA memory boundary dt

> property and workaround the limitation.


IMHO, the workaround could be implemented in each SDHCI host driver.

eg. drivers/mmc/host/sdhci-of-dwcmshc.c

thanks

> 

> Signed-off-by: Jun Nie <jun.nie@linaro.org>

> ---

>  drivers/mmc/host/sdhci.c | 20 +++++++++++++++++++-

>  drivers/mmc/host/sdhci.h |  1 +

>  2 files changed, 20 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c

> index d8a6c1c91448..56c53fbadd9d 100644

> --- a/drivers/mmc/host/sdhci.c

> +++ b/drivers/mmc/host/sdhci.c

> @@ -763,9 +763,25 @@ static void sdhci_adma_table_pre(struct sdhci_host *host,

>                 BUG_ON(len > 65536);

> 

>                 /* tran, valid */

> -               if (len)

> +               if (len) {

> +                       unsigned int boundary = host->dma_mem_boundary;

> +                       /*

> +                        * work around for buffer across mem boundary, split

> +                        * the buffer.

> +                        */

> +                       if (boundary &&

> +                           ((addr & (boundary - 1)) + len) > boundary) {

> +                               offset = boundary - (addr & (boundary - 1));

> +                               __sdhci_adma_write_desc(host, &desc,

> +                                                       addr, offset,

> +                                                       ADMA2_TRAN_VALID);

> +                               addr += offset;

> +                               len -= offset;

> +                       }

> +

>                         __sdhci_adma_write_desc(host, &desc, addr, len,

>                                                 ADMA2_TRAN_VALID);

> +               }

> 

>                 /*

>                  * If this triggers then we have a calculation bug

> @@ -3634,6 +3650,8 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver,

>                              "sdhci-caps-mask", &dt_caps_mask);

>         of_property_read_u64(mmc_dev(host->mmc)->of_node,

>                              "sdhci-caps", &dt_caps);

> +       of_property_read_u32(mmc_dev(host->mmc)->of_node,

> +                            "sdhci-dma-mem-boundary", &host->dma_mem_boundary);

> 

>         if (of_property_read_u32(mmc_dev(host->mmc)->of_node,

>                                  "sdhci-ctrl-hs400", &host->sdhci_ctrl_hs400))

> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h

> index cac4d819f62c..954ac08c4fb0 100644

> --- a/drivers/mmc/host/sdhci.h

> +++ b/drivers/mmc/host/sdhci.h

> @@ -608,6 +608,7 @@ struct sdhci_host {

> 

>         /* SDHCI_CTRL_HS400 value */

>         u32                     sdhci_ctrl_hs400;

> +       u32                     dma_mem_boundary;

> 

>         unsigned long private[0] ____cacheline_aligned;

>  };

> --

> 2.17.1

>
Jun Nie Dec. 3, 2019, 3:29 a.m. | #2
Christoph Hellwig <hch@infradead.org> 于2019年12月3日周二 上午1:52写道:
>

> On Mon, Dec 02, 2019 at 10:41:04PM +0800, Jun Nie wrote:

> > DMA memory cannot cross specific boundary for some SDHCI controller,

> > such as DesignWare SDHCI controller. Add DMA memory boundary dt

> > property and workaround the limitation.

>

> If you use blk_queue_segment_boundary to tell the block layer the

> segment boundary restrictions it won't ever send you segments

> that don't fit.  With just the workaround in this patch you'll run into

> max_segments accounting issues, don't you?


Thanks for the reminder! So I need to parse the segment_boundary from
device tree and use below code to set it, right?
For the max_segments accounting error, I did not see it so far though I
believe it is true in theory. Maybe it is due to segment boundary value is
very large.

+++ b/drivers/mmc/core/queue.c
@@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,
struct mmc_card *card)
                WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
                                                        mmc_dev(host)),
                     "merging was advertised but not possible");
+       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);
        blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
Jisheng Zhang Dec. 3, 2019, 9:05 a.m. | #3
+ Christoph

On Tue, 3 Dec 2019 11:33:15 +0800 Jun Nie wrote:

> 

> 

> Jisheng Zhang <Jisheng.Zhang@synaptics.com> 于2019年12月3日周二 上午10:47写道:

> >

> > On Mon,  2 Dec 2019 22:41:04 +0800 Jun Nie wrote:

> >

> >  

> > >

> > >

> > > DMA memory cannot cross specific boundary for some SDHCI controller,

> > > such as DesignWare SDHCI controller. Add DMA memory boundary dt

> > > property and workaround the limitation.  

> >

> > IMHO, the workaround could be implemented in each SDHCI host driver.

> >

> > eg. drivers/mmc/host/sdhci-of-dwcmshc.c

> >  

> Thanks for the suggestion! Christoph's suggestion can prevent the the issue

> from the block layer, thus the code can be shared across all


To be honest, I did consider similar solution from block layer, I.E set
the seg_boundary_mask, when submitting the workaround last year, but per
my understanding, SDHCI limitation is the physical DMA addr can't span one
specific boundary, so setting seg_boundary_mask w/ blk_queue_segment_boundary
can't work. I'm not sure I understand blk_queue_segment_boundary() properly.
May Christoph help to clarify?

From another side, drivers/ata/libata-sff.c also workaround the 64K phy DMA
boundary limitation itself rather than from block layer.

Thanks

> controllers. I prefer

> his suggestions.

> 

> Jun
Jisheng Zhang Dec. 3, 2019, 9:49 a.m. | #4
On Tue, 3 Dec 2019 09:05:23 +0000 Jisheng Zhang wrote:


> 

> + Christoph

> 

> On Tue, 3 Dec 2019 11:33:15 +0800 Jun Nie wrote:

> 

> >

> >

> > Jisheng Zhang <Jisheng.Zhang@synaptics.com> 于2019年12月3日周二 上午10:47写道:  

> > >

> > > On Mon,  2 Dec 2019 22:41:04 +0800 Jun Nie wrote:

> > >

> > >  

> > > >

> > > >

> > > > DMA memory cannot cross specific boundary for some SDHCI controller,

> > > > such as DesignWare SDHCI controller. Add DMA memory boundary dt

> > > > property and workaround the limitation.  

> > >

> > > IMHO, the workaround could be implemented in each SDHCI host driver.

> > >

> > > eg. drivers/mmc/host/sdhci-of-dwcmshc.c

> > >  

> > Thanks for the suggestion! Christoph's suggestion can prevent the the issue

> > from the block layer, thus the code can be shared across all  

> 

> To be honest, I did consider similar solution from block layer, I.E set

> the seg_boundary_mask, when submitting the workaround last year, but per

> my understanding, SDHCI limitation is the physical DMA addr can't span one

> specific boundary, so setting seg_boundary_mask w/ blk_queue_segment_boundary

> can't work. I'm not sure I understand blk_queue_segment_boundary() properly.

> May Christoph help to clarify?


what's more, not all scatterlist in mmc are from block layer, for example, 
__mmc_blk_ioctl_cmd(), mmc_test.c etc..

How do we ensure the boundary is fine in these cases?

> 

> From another side, drivers/ata/libata-sff.c also workaround the 64K phy DMA

> boundary limitation itself rather than from block layer.


> Thanks

> 

> > controllers. I prefer

> > his suggestions.

> >

> > Jun  

>
Christoph Hellwig Dec. 3, 2019, 1:04 p.m. | #5
On Tue, Dec 03, 2019 at 09:49:33AM +0000, Jisheng Zhang wrote:
> > can't work. I'm not sure I understand blk_queue_segment_boundary() properly.

> > May Christoph help to clarify?

> 

> what's more, not all scatterlist in mmc are from block layer, for example, 

> __mmc_blk_ioctl_cmd(), mmc_test.c etc..

> 

> How do we ensure the boundary is fine in these cases?


By using block layer passthrough requests (REQ_OP_DRV*).  Otherwise
any other I/O limit imposed by the driver is ignored as well.
Jun Nie Dec. 4, 2019, 6 a.m. | #6
Christoph Hellwig <hch@infradead.org> 于2019年12月3日周二 下午3:36写道:
>

> On Tue, Dec 03, 2019 at 11:29:15AM +0800, Jun Nie wrote:

> > Thanks for the reminder! So I need to parse the segment_boundary from

> > device tree and use below code to set it, right?

> > For the max_segments accounting error, I did not see it so far though I

> > believe it is true in theory. Maybe it is due to segment boundary value is

> > very large.

> >

> > +++ b/drivers/mmc/core/queue.c

> > @@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,

> > struct mmc_card *card)

> >                 WARN(!blk_queue_can_use_dma_map_merging(mq->queue,

> >                                                         mmc_dev(host)),

> >                      "merging was advertised but not possible");

> > +       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);

> >         blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));

>

> Yes, I think should do it.  Maybe modulo a check if the low-level

> driver actually sets a segment boundary.


For the block device, such as SD card, it is right solution. But I
have concern on SDIO case. Maybe we should add workaround together
with block layer segment boundary restriction. How do you think about
it?

Jun
Jisheng Zhang Dec. 4, 2019, 7:11 a.m. | #7
Hi Christoph

On Tue, 3 Dec 2019 05:06:09 -0800 Christoph Hellwig wrote:


> 

> On Tue, Dec 03, 2019 at 09:49:49AM +0000, Jisheng Zhang wrote:

> > > As in exactly one boundary and not an alignment?  Where the one

> > > boundary is not a power of two and thus can't be expressed?  

> >

> > Take drivers/mmc/host/sdhci-of-dwcmshc.c for example, target physical DMA addr

> > can't span 128MB, 256MB, 128*3MB, ...128*nMB

> >

> > I'm not sure whether blk_queue_segment_boundary could solve this limitation.  

> 

> That is exaxtly the kind of limitation blk_queue_segment_boundary is

> intended for.


Until after dma_map_sg(), we can't know the physical DMA address range, so
how does block layer know and check the DMA range beforehand? I'm a newbie on
block layer, could you please teach me? At the same time
I'm reading the code as well.

Thanks in advance
Jisheng Zhang Dec. 4, 2019, 7:14 a.m. | #8
On Wed, 4 Dec 2019 14:00:08 +0800 Jun Nie wrote:

> 

> 

> Christoph Hellwig <hch@infradead.org> 于2019年12月3日周二 下午3:36写道:

> >

> > On Tue, Dec 03, 2019 at 11:29:15AM +0800, Jun Nie wrote:  

> > > Thanks for the reminder! So I need to parse the segment_boundary from

> > > device tree and use below code to set it, right?

> > > For the max_segments accounting error, I did not see it so far though I

> > > believe it is true in theory. Maybe it is due to segment boundary value is

> > > very large.

> > >

> > > +++ b/drivers/mmc/core/queue.c

> > > @@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,

> > > struct mmc_card *card)

> > >                 WARN(!blk_queue_can_use_dma_map_merging(mq->queue,

> > >                                                         mmc_dev(host)),

> > >                      "merging was advertised but not possible");

> > > +       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);

> > >         blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));  

> >

> > Yes, I think should do it.  Maybe modulo a check if the low-level

> > driver actually sets a segment boundary.  

> 

> For the block device, such as SD card, it is right solution. But I

> have concern on SDIO case. Maybe we should add workaround together

> with block layer segment boundary restriction. How do you think about

> it?

> 


Another trouble is how to workaround if the sg is constructed by mmc and
no block layer interactions at all. e.g __mmc_blk_ioctl_cmd(), and all
those sgs in mmc_test.c

Thanks
Ulf Hansson Dec. 10, 2019, 9:36 a.m. | #9
On Wed, 4 Dec 2019 at 08:14, Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote:
>

> On Wed, 4 Dec 2019 14:00:08 +0800 Jun Nie wrote:

>

> >

> >

> > Christoph Hellwig <hch@infradead.org> 于2019年12月3日周二 下午3:36写道:

> > >

> > > On Tue, Dec 03, 2019 at 11:29:15AM +0800, Jun Nie wrote:

> > > > Thanks for the reminder! So I need to parse the segment_boundary from

> > > > device tree and use below code to set it, right?

> > > > For the max_segments accounting error, I did not see it so far though I

> > > > believe it is true in theory. Maybe it is due to segment boundary value is

> > > > very large.

> > > >

> > > > +++ b/drivers/mmc/core/queue.c

> > > > @@ -374,6 +374,7 @@ static void mmc_setup_queue(struct mmc_queue *mq,

> > > > struct mmc_card *card)

> > > >                 WARN(!blk_queue_can_use_dma_map_merging(mq->queue,

> > > >                                                         mmc_dev(host)),

> > > >                      "merging was advertised but not possible");

> > > > +       blk_queue_segment_boundary(mq->queue, mmc->segment_boundary);

> > > >         blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));

> > >

> > > Yes, I think should do it.  Maybe modulo a check if the low-level

> > > driver actually sets a segment boundary.

> >

> > For the block device, such as SD card, it is right solution. But I

> > have concern on SDIO case. Maybe we should add workaround together

> > with block layer segment boundary restriction. How do you think about

> > it?

> >


Yes, buffers for SDIO are a consern. Especially since those buffers
are allocated by SDIO func drivers or even from upper layers, such as
the network stacks, for example.

I think SDIO func drivers simply need to respect the constraints the
host has set via the .segment_boundary, .max_seg_size, .max_segs, etc.
We should export SDIO func APIs to make the information available for
the SDIO func drivers.

>

> Another trouble is how to workaround if the sg is constructed by mmc and

> no block layer interactions at all. e.g __mmc_blk_ioctl_cmd(), and all

> those sgs in mmc_test.c


Those should be easier to fix, as the buffer/sg allocation can be
fixed internally by mmc core. Just post some patches. :-)

I am more worried about SDIO, as those buffers are not that easy to control.

Note that, there have been suggestions on adding an SDIO interface
where an sg can be passed [1]. Unfurtunate those patches got stuck and
didn't make it.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/patch/10123143/

Patch

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d8a6c1c91448..56c53fbadd9d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -763,9 +763,25 @@  static void sdhci_adma_table_pre(struct sdhci_host *host,
 		BUG_ON(len > 65536);
 
 		/* tran, valid */
-		if (len)
+		if (len) {
+			unsigned int boundary = host->dma_mem_boundary;
+			/*
+			 * work around for buffer across mem boundary, split
+			 * the buffer.
+			 */
+			if (boundary &&
+			    ((addr & (boundary - 1)) + len) > boundary) {
+				offset = boundary - (addr & (boundary - 1));
+				__sdhci_adma_write_desc(host, &desc,
+							addr, offset,
+							ADMA2_TRAN_VALID);
+				addr += offset;
+				len -= offset;
+			}
+
 			__sdhci_adma_write_desc(host, &desc, addr, len,
 						ADMA2_TRAN_VALID);
+		}
 
 		/*
 		 * If this triggers then we have a calculation bug
@@ -3634,6 +3650,8 @@  void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver,
 			     "sdhci-caps-mask", &dt_caps_mask);
 	of_property_read_u64(mmc_dev(host->mmc)->of_node,
 			     "sdhci-caps", &dt_caps);
+	of_property_read_u32(mmc_dev(host->mmc)->of_node,
+			     "sdhci-dma-mem-boundary", &host->dma_mem_boundary);
 
 	if (of_property_read_u32(mmc_dev(host->mmc)->of_node,
 				 "sdhci-ctrl-hs400", &host->sdhci_ctrl_hs400))
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index cac4d819f62c..954ac08c4fb0 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -608,6 +608,7 @@  struct sdhci_host {
 
 	/* SDHCI_CTRL_HS400 value */
 	u32			sdhci_ctrl_hs400;
+	u32			dma_mem_boundary;
 
 	unsigned long private[0] ____cacheline_aligned;
 };