| Message ID | 20191202144104.5069-5-jun.nie@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: Add sdhci workaround stability enhencement | expand |
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 >
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));
+ 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
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 >
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.
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
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
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
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/
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; };
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