| Message ID | 20191202144104.5069-1-jun.nie@linaro.org |
|---|---|
| Headers | show |
| Series | mmc: Add sdhci workaround stability enhencement | expand |
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?
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 >
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 controllers. I prefer his suggestions. Jun
On 2/12/19 4:41 pm, Jun Nie wrote: > Add delay after power off to ensure that full power cycle is > successful. Otherwise, some controllers, at lease for Hisilicon > eMMC controller, may not be unstable sometimes for full power > cycle operation. > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > drivers/mmc/host/sdhci.c | 7 +++++++ > drivers/mmc/host/sdhci.h | 2 ++ > 2 files changed, 9 insertions(+) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 3140fe2e5dba..a654f0aeb438 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -1761,6 +1761,13 @@ void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode, > sdhci_writeb(host, 0, SDHCI_POWER_CONTROL); > if (host->quirks2 & SDHCI_QUIRK2_CARD_ON_NEEDS_BUS_ON) > sdhci_runtime_pm_bus_off(host); > + > + /* > + * Some controllers need an extra 100ms delay to secure > + * full power cycle is successful. > + */ > + if (host->quirks2 & SDHCI_QUIRK2_DELAY_AFTER_POWER_OFF) > + msleep(100); Please use the ->set_power() callback and do this in your own driver. > } else { > /* > * Spec says that we should clear the power reg before setting > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index 0ed3e0eaef5f..0e6f97eaa796 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -482,6 +482,8 @@ struct sdhci_host { > * block count. > */ > #define SDHCI_QUIRK2_USE_32BIT_BLK_CNT (1<<18) > +/* Controllers need an extra 100ms delay to make sure power off completely */ > +#define SDHCI_QUIRK2_DELAY_AFTER_POWER_OFF (1<<19) > > int irq; /* Device IRQ */ > void __iomem *ioaddr; /* Mapped address */ >
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.
On 2/12/19 4:41 pm, Jun Nie wrote: > Because ctrl_hs400 value is non-standard, add support to set it > in dts. > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > drivers/mmc/host/sdhci.c | 9 ++++++++- > drivers/mmc/host/sdhci.h | 6 +++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index a654f0aeb438..d8a6c1c91448 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -1899,7 +1899,7 @@ void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing) > (timing == MMC_TIMING_MMC_DDR52)) > ctrl_2 |= SDHCI_CTRL_UHS_DDR50; > else if (timing == MMC_TIMING_MMC_HS400) > - ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */ > + ctrl_2 |= host->sdhci_ctrl_hs400; /* Non-standard */ Let's limit this to the correct bits i.e. ctrl_2 |= host->sdhci_ctrl_hs400 & 7; /* Non-standard */ > sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2); > } > EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling); > @@ -3635,6 +3635,13 @@ void __sdhci_read_caps(struct sdhci_host *host, const u16 *ver, > of_property_read_u64(mmc_dev(host->mmc)->of_node, > "sdhci-caps", &dt_caps); Would you mind changing these APIs from 'of' to 'dev' properties? Needs to be a separate patch. > > + if (of_property_read_u32(mmc_dev(host->mmc)->of_node, > + "sdhci-ctrl-hs400", &host->sdhci_ctrl_hs400)) And then use a 'dev' property API here. > + host->sdhci_ctrl_hs400 = SDHCI_CTRL_HS400; > + else > + WARN_ON(host->sdhci_ctrl_hs400 > 7 > + || host->sdhci_ctrl_hs400 < 5); Please remove this warning. > + > v = ver ? *ver : sdhci_readw(host, SDHCI_HOST_VERSION); > host->version = (v & SDHCI_SPEC_VER_MASK) >> SDHCI_SPEC_VER_SHIFT; > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index 0e6f97eaa796..cac4d819f62c 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -184,7 +184,8 @@ > #define SDHCI_CTRL_UHS_SDR50 0x0002 > #define SDHCI_CTRL_UHS_SDR104 0x0003 > #define SDHCI_CTRL_UHS_DDR50 0x0004 > -#define SDHCI_CTRL_HS400 0x0005 /* Non-standard */ > +/* SDHCI_CTRL_HS400 is non-standard value, can change it in dts */ > +#define SDHCI_CTRL_HS400 0x0005 > #define SDHCI_CTRL_VDD_180 0x0008 > #define SDHCI_CTRL_DRV_TYPE_MASK 0x0030 > #define SDHCI_CTRL_DRV_TYPE_B 0x0000 > @@ -605,6 +606,9 @@ struct sdhci_host { > > u64 data_timeout; > > + /* SDHCI_CTRL_HS400 value */ > + u32 sdhci_ctrl_hs400; > + > unsigned long private[0] ____cacheline_aligned; > }; > >
+ 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, Dec 03, 2019 at 09:05:23AM +0000, Jisheng Zhang wrote: > > > > > > 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, 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? > 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. As far as I can tell that workaround should use the segment boundary setting as well.
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, 3 Dec 2019 01:18:24 -0800 Christoph Hellwig <hch@infradead.org> wrote: > > > On Tue, Dec 03, 2019 at 09:05:23AM +0000, Jisheng Zhang wrote: > > > > > > > > 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, > > 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. > > > > 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. > > As far as I can tell that workaround should use the segment boundary > setting as well.
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.
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.
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/