| Message ID | 1435220707-7250-1-git-send-email-abrodkin@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Alexey. Sorry for reviewing late. i missed this patch. I will check your patch and reply at mailing. Thanks a lot for noticing this. Best Regards, Jaehoon Chung On 07/01/2015 07:02 PM, Alexey Brodkin wrote: > Hi Jaehoon, Seungwon, Ulf, > > On Thu, 2015-06-25 at 11:25 +0300, Alexey Brodkin wrote: >> As per DW MobileStorage databook "each descriptor can transfer up to >> 4kB >> of data in chained mode", moreover buffer size that is put in "des1" >> is >> limited to 13 bits, i.e. for example on attempt to >> IDMAC_SET_BUFFER1_SIZE(desc, 8192) size value that's effectively >> written >> will be 0. >> >> On the platform with 8kB PAGE_SIZE I see dw_mmc gets data blocks in >> SG-list of 8kB size and that leads to unpredictable behavior of the >> SD/MMC controller. >> >> In particular on write to FAT partition of SD-card the controller >> will >> stuck in the middle of DMA transaction. >> >> Solution to the problem is simple - we need to pass large (> 4kB) >> data >> buffers to the controller via multiple descriptors. And that's what >> that change does. >> >> What's interesting I did try original driver on same platform but >> configured with 4kB PAGE_SIZE and may confirm that data blocks passed >> in SG-list to dw_mmc never exeed 4kB limit - that explains why nobody >> ever faced a problem I did. >> >> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> >> Cc: Seungwon Jeon <tgih.jun@samsung.com> >> Cc: Jaehoon Chung <jh80.chung@samsung.com> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> Cc: arc-linux-dev@synopsys.com >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/mmc/host/dw_mmc.c | 109 ++++++++++++++++++++++++++++++------ >> ---------- > > I'm wondering if there're any comments on that patch or if it could be > applied? > > It fixes a real problem on systems on 4K PAGE_SIZE so would be good to > have it in upstream. In particular this is the case with ARC AXS board > which made its way in upstream kernel recently. > > Regards, > Alexey-- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 01 July 2015 03:32 PM, Alexey Brodkin wrote: > Hi Jaehoon, Seungwon, Ulf, > > On Thu, 2015-06-25 at 11:25 +0300, Alexey Brodkin wrote: >> > As per DW MobileStorage databook "each descriptor can transfer up to >> > 4kB >> > of data in chained mode", moreover buffer size that is put in "des1" >> > is >> > limited to 13 bits, i.e. for example on attempt to >> > IDMAC_SET_BUFFER1_SIZE(desc, 8192) size value that's effectively >> > written >> > will be 0. >> > >> > On the platform with 8kB PAGE_SIZE I see dw_mmc gets data blocks in >> > SG-list of 8kB size and that leads to unpredictable behavior of the >> > SD/MMC controller. >> > >> > In particular on write to FAT partition of SD-card the controller >> > will >> > stuck in the middle of DMA transaction. >> > >> > Solution to the problem is simple - we need to pass large (> 4kB) >> > data >> > buffers to the controller via multiple descriptors. And that's what >> > that change does. >> > >> > What's interesting I did try original driver on same platform but >> > configured with 4kB PAGE_SIZE and may confirm that data blocks passed >> > in SG-list to dw_mmc never exeed 4kB limit - that explains why nobody >> > ever faced a problem I did. >> > >> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> >> > Cc: Seungwon Jeon <tgih.jun@samsung.com> >> > Cc: Jaehoon Chung <jh80.chung@samsung.com> >> > Cc: Ulf Hansson <ulf.hansson@linaro.org> >> > Cc: arc-linux-dev@synopsys.com >> > Cc: linux-kernel@vger.kernel.org >> > --- >> > drivers/mmc/host/dw_mmc.c | 109 ++++++++++++++++++++++++++++++------ >> > ---------- > I'm wondering if there're any comments on that patch or if it could be > applied? > > It fixes a real problem on systems on 4K PAGE_SIZE so would be good to s/4K/8k Alexey fat-fingered it seems. ARC cores by default have 8k MMU page size and we saw this issue on ARC SDP boards ! -Vineet > have it in upstream. In particular this is the case with ARC AXS board > which made its way in upstream kernel recently. > > Regards, > Alexey-- -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Alexey. On 06/25/2015 05:25 PM, Alexey Brodkin wrote: > As per DW MobileStorage databook "each descriptor can transfer up to 4kB > of data in chained mode", moreover buffer size that is put in "des1" is > limited to 13 bits, i.e. for example on attempt to > IDMAC_SET_BUFFER1_SIZE(desc, 8192) size value that's effectively written > will be 0. > > On the platform with 8kB PAGE_SIZE I see dw_mmc gets data blocks in > SG-list of 8kB size and that leads to unpredictable behavior of the > SD/MMC controller. I didn't see your problem, since i didn't test with 8K PAGE_SIZE. But I think your patch is reasonable. As possible, I want to know in more detail what unpredictable behavior. (Just stuck behavior?) > > In particular on write to FAT partition of SD-card the controller will > stuck in the middle of DMA transaction. > > Solution to the problem is simple - we need to pass large (> 4kB) data > buffers to the controller via multiple descriptors. And that's what > that change does. > > What's interesting I did try original driver on same platform but > configured with 4kB PAGE_SIZE and may confirm that data blocks passed > in SG-list to dw_mmc never exeed 4kB limit - that explains why nobody > ever faced a problem I did. > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Seungwon Jeon <tgih.jun@samsung.com> > Cc: Jaehoon Chung <jh80.chung@samsung.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: arc-linux-dev@synopsys.com > Cc: linux-kernel@vger.kernel.org > --- > drivers/mmc/host/dw_mmc.c | 109 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 71 insertions(+), 38 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 40e9d8e..e41fb74 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -99,6 +99,9 @@ struct idmac_desc { > > __le32 des3; /* buffer 2 physical address */ > }; > + > +/* Each descriptor can transfer up to 4KB of data in chained mode */ > +#define DW_MCI_DESC_DATA_LENGTH 0x1000 > #endif /* CONFIG_MMC_DW_IDMAC */ > > static bool dw_mci_reset(struct dw_mci *host); > @@ -462,66 +465,96 @@ static void dw_mci_idmac_complete_dma(struct dw_mci *host) > static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, > unsigned int sg_len) > { > + unsigned int desc_len; > int i; > if (host->dma_64bit_address == 1) { > - struct idmac_desc_64addr *desc = host->sg_cpu; > + struct idmac_desc_64addr *desc_first, *desc_last, *desc; Why it needs desc_first? > + > + desc_first = desc_last = desc = host->sg_cpu; > > - for (i = 0; i < sg_len; i++, desc++) { > + for (i = 0; i < sg_len; i++) { > unsigned int length = sg_dma_len(&data->sg[i]); > u64 mem_addr = sg_dma_address(&data->sg[i]); > > - /* > - * Set the OWN bit and disable interrupts for this > - * descriptor > - */ > - desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | > - IDMAC_DES0_CH; > - /* Buffer length */ > - IDMAC_64ADDR_SET_BUFFER1_SIZE(desc, length); > - > - /* Physical address to DMA to/from */ > - desc->des4 = mem_addr & 0xffffffff; > - desc->des5 = mem_addr >> 32; > + for ( ; length ; desc++) { Is there no infinite loop case? Best Regards, Jaehoon Chung > + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? > + length : DW_MCI_DESC_DATA_LENGTH; > + > + length -= desc_len; > + > + /* > + * Set the OWN bit and disable interrupts > + * for this descriptor > + */ > + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | > + IDMAC_DES0_CH; > + > + /* Buffer length */ > + IDMAC_64ADDR_SET_BUFFER1_SIZE(desc, desc_len); > + > + /* Physical address to DMA to/from */ > + desc->des4 = mem_addr & 0xffffffff; > + desc->des5 = mem_addr >> 32; > + > + /* Update physical address for the next desc */ > + mem_addr += desc_len; > + > + /* Save pointer to the last descriptor */ > + desc_last = desc; > + } > } > > /* Set first descriptor */ > - desc = host->sg_cpu; > - desc->des0 |= IDMAC_DES0_FD; > + desc_first->des0 |= IDMAC_DES0_FD; > > /* Set last descriptor */ > - desc = host->sg_cpu + (i - 1) * > - sizeof(struct idmac_desc_64addr); > - desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); > - desc->des0 |= IDMAC_DES0_LD; > + desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); > + desc_last->des0 |= IDMAC_DES0_LD; > > } else { > - struct idmac_desc *desc = host->sg_cpu; > + struct idmac_desc *desc_first, *desc_last, *desc; > + > + desc_first = desc_last = desc = host->sg_cpu; > > - for (i = 0; i < sg_len; i++, desc++) { > + for (i = 0; i < sg_len; i++) { > unsigned int length = sg_dma_len(&data->sg[i]); > u32 mem_addr = sg_dma_address(&data->sg[i]); > > - /* > - * Set the OWN bit and disable interrupts for this > - * descriptor > - */ > - desc->des0 = cpu_to_le32(IDMAC_DES0_OWN | > - IDMAC_DES0_DIC | IDMAC_DES0_CH); > - /* Buffer length */ > - IDMAC_SET_BUFFER1_SIZE(desc, length); > + for ( ; length ; desc++) { > + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? > + length : DW_MCI_DESC_DATA_LENGTH; > + > + length -= desc_len; > + > + /* > + * Set the OWN bit and disable interrupts > + * for this descriptor > + */ > + desc->des0 = cpu_to_le32(IDMAC_DES0_OWN | > + IDMAC_DES0_DIC | > + IDMAC_DES0_CH); > + > + /* Buffer length */ > + IDMAC_SET_BUFFER1_SIZE(desc, desc_len); > > - /* Physical address to DMA to/from */ > - desc->des2 = cpu_to_le32(mem_addr); > + /* Physical address to DMA to/from */ > + desc->des2 = cpu_to_le32(mem_addr); > + > + /* Update physical address for the next desc */ > + mem_addr += desc_len; > + > + /* Save pointer to the last descriptor */ > + desc_last = desc; > + } > } > > /* Set first descriptor */ > - desc = host->sg_cpu; > - desc->des0 |= cpu_to_le32(IDMAC_DES0_FD); > + desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD); > > /* Set last descriptor */ > - desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc); > - desc->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | IDMAC_DES0_DIC)); > - desc->des0 |= cpu_to_le32(IDMAC_DES0_LD); > + desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | > + IDMAC_DES0_DIC)); > + desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); > } > > wmb(); > @@ -2394,7 +2427,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) > #ifdef CONFIG_MMC_DW_IDMAC > mmc->max_segs = host->ring_size; > mmc->max_blk_size = 65536; > - mmc->max_seg_size = 0x1000; > + mmc->max_seg_size = DW_MCI_DESC_DATA_LENGTH; > mmc->max_req_size = mmc->max_seg_size * host->ring_size; > mmc->max_blk_count = mmc->max_req_size / 512; > #else > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Alexey. On 07/09/2015 10:04 PM, Alexey Brodkin wrote: > Hi Jaehoon, > > On Wed, 2015-07-08 at 11:45 +0300, Alexey Brodkin wrote: >> >> Let me know if that description makes sense to you. > > I'm wondering if you had a chance to look at my comments and if > those comments make sense or you need more input from me. As my understanding, it makes sense. If page size should be changed bigger than 4K, internal dma should not be working fine. (Especially your case..Page size is 8K.) I will pick this patch at my dw-mmc tree. Thanks for your effort and pointing out! :) Best Regards, Jaehoon Chung > > -Alexey-- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 40e9d8e..e41fb74 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -99,6 +99,9 @@ struct idmac_desc { __le32 des3; /* buffer 2 physical address */ }; + +/* Each descriptor can transfer up to 4KB of data in chained mode */ +#define DW_MCI_DESC_DATA_LENGTH 0x1000 #endif /* CONFIG_MMC_DW_IDMAC */ static bool dw_mci_reset(struct dw_mci *host); @@ -462,66 +465,96 @@ static void dw_mci_idmac_complete_dma(struct dw_mci *host) static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, unsigned int sg_len) { + unsigned int desc_len; int i; if (host->dma_64bit_address == 1) { - struct idmac_desc_64addr *desc = host->sg_cpu; + struct idmac_desc_64addr *desc_first, *desc_last, *desc; + + desc_first = desc_last = desc = host->sg_cpu; - for (i = 0; i < sg_len; i++, desc++) { + for (i = 0; i < sg_len; i++) { unsigned int length = sg_dma_len(&data->sg[i]); u64 mem_addr = sg_dma_address(&data->sg[i]); - /* - * Set the OWN bit and disable interrupts for this - * descriptor - */ - desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | - IDMAC_DES0_CH; - /* Buffer length */ - IDMAC_64ADDR_SET_BUFFER1_SIZE(desc, length); - - /* Physical address to DMA to/from */ - desc->des4 = mem_addr & 0xffffffff; - desc->des5 = mem_addr >> 32; + for ( ; length ; desc++) { + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? + length : DW_MCI_DESC_DATA_LENGTH; + + length -= desc_len; + + /* + * Set the OWN bit and disable interrupts + * for this descriptor + */ + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | + IDMAC_DES0_CH; + + /* Buffer length */ + IDMAC_64ADDR_SET_BUFFER1_SIZE(desc, desc_len); + + /* Physical address to DMA to/from */ + desc->des4 = mem_addr & 0xffffffff; + desc->des5 = mem_addr >> 32; + + /* Update physical address for the next desc */ + mem_addr += desc_len; + + /* Save pointer to the last descriptor */ + desc_last = desc; + } } /* Set first descriptor */ - desc = host->sg_cpu; - desc->des0 |= IDMAC_DES0_FD; + desc_first->des0 |= IDMAC_DES0_FD; /* Set last descriptor */ - desc = host->sg_cpu + (i - 1) * - sizeof(struct idmac_desc_64addr); - desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); - desc->des0 |= IDMAC_DES0_LD; + desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); + desc_last->des0 |= IDMAC_DES0_LD; } else { - struct idmac_desc *desc = host->sg_cpu; + struct idmac_desc *desc_first, *desc_last, *desc; + + desc_first = desc_last = desc = host->sg_cpu; - for (i = 0; i < sg_len; i++, desc++) { + for (i = 0; i < sg_len; i++) { unsigned int length = sg_dma_len(&data->sg[i]); u32 mem_addr = sg_dma_address(&data->sg[i]); - /* - * Set the OWN bit and disable interrupts for this - * descriptor - */ - desc->des0 = cpu_to_le32(IDMAC_DES0_OWN | - IDMAC_DES0_DIC | IDMAC_DES0_CH); - /* Buffer length */ - IDMAC_SET_BUFFER1_SIZE(desc, length); + for ( ; length ; desc++) { + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? + length : DW_MCI_DESC_DATA_LENGTH; + + length -= desc_len; + + /* + * Set the OWN bit and disable interrupts + * for this descriptor + */ + desc->des0 = cpu_to_le32(IDMAC_DES0_OWN | + IDMAC_DES0_DIC | + IDMAC_DES0_CH); + + /* Buffer length */ + IDMAC_SET_BUFFER1_SIZE(desc, desc_len); - /* Physical address to DMA to/from */ - desc->des2 = cpu_to_le32(mem_addr); + /* Physical address to DMA to/from */ + desc->des2 = cpu_to_le32(mem_addr); + + /* Update physical address for the next desc */ + mem_addr += desc_len; + + /* Save pointer to the last descriptor */ + desc_last = desc; + } } /* Set first descriptor */ - desc = host->sg_cpu; - desc->des0 |= cpu_to_le32(IDMAC_DES0_FD); + desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD); /* Set last descriptor */ - desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc); - desc->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | IDMAC_DES0_DIC)); - desc->des0 |= cpu_to_le32(IDMAC_DES0_LD); + desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | + IDMAC_DES0_DIC)); + desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); } wmb(); @@ -2394,7 +2427,7 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id) #ifdef CONFIG_MMC_DW_IDMAC mmc->max_segs = host->ring_size; mmc->max_blk_size = 65536; - mmc->max_seg_size = 0x1000; + mmc->max_seg_size = DW_MCI_DESC_DATA_LENGTH; mmc->max_req_size = mmc->max_seg_size * host->ring_size; mmc->max_blk_count = mmc->max_req_size / 512; #else
As per DW MobileStorage databook "each descriptor can transfer up to 4kB of data in chained mode", moreover buffer size that is put in "des1" is limited to 13 bits, i.e. for example on attempt to IDMAC_SET_BUFFER1_SIZE(desc, 8192) size value that's effectively written will be 0. On the platform with 8kB PAGE_SIZE I see dw_mmc gets data blocks in SG-list of 8kB size and that leads to unpredictable behavior of the SD/MMC controller. In particular on write to FAT partition of SD-card the controller will stuck in the middle of DMA transaction. Solution to the problem is simple - we need to pass large (> 4kB) data buffers to the controller via multiple descriptors. And that's what that change does. What's interesting I did try original driver on same platform but configured with 4kB PAGE_SIZE and may confirm that data blocks passed in SG-list to dw_mmc never exeed 4kB limit - that explains why nobody ever faced a problem I did. Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> Cc: Seungwon Jeon <tgih.jun@samsung.com> Cc: Jaehoon Chung <jh80.chung@samsung.com> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: arc-linux-dev@synopsys.com Cc: linux-kernel@vger.kernel.org --- drivers/mmc/host/dw_mmc.c | 109 ++++++++++++++++++++++++++++++---------------- 1 file changed, 71 insertions(+), 38 deletions(-)