* [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 @ 2016-08-19 9:40 ` Shawn Lin 2016-08-19 9:40 ` [PATCH 2/2] mmc: dw_mmc: avoid race condition of cpu and IDMAC Shawn Lin 2016-08-31 6:55 ` [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 Jaehoon Chung 0 siblings, 2 replies; 6+ messages in thread From: Shawn Lin @ 2016-08-19 9:40 UTC (permalink / raw) To: Jaehoon Chung Cc: Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson, Brian Norris, Heiko Stuebner, linux-rockchip, Shawn Lin We intend to add more check for descriptors when preparing desc. Let's spilt out the separate body to make the dw_mci_translate_sglist not so lengthy. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/host/dw_mmc.c | 148 +++++++++++++++++++++++++--------------------- 1 file changed, 81 insertions(+), 67 deletions(-) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 32380d5..0a5a49f 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -467,103 +467,117 @@ static void dw_mci_dmac_complete_dma(void *arg) } } -static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, - unsigned int sg_len) +static inline void dw_mci_prepare_desc64(struct dw_mci *host, + struct mmc_data *data, + unsigned int sg_len) { unsigned int desc_len; + struct idmac_desc_64addr *desc_first, *desc_last, *desc; int i; - if (host->dma_64bit_address == 1) { - struct idmac_desc_64addr *desc_first, *desc_last, *desc; - - desc_first = desc_last = desc = host->sg_cpu; + desc_first = desc_last = desc = host->sg_cpu; - for (i = 0; i < sg_len; i++) { - unsigned int length = sg_dma_len(&data->sg[i]); + 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]); + u64 mem_addr = sg_dma_address(&data->sg[i]); - for ( ; length ; desc++) { - desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? - length : DW_MCI_DESC_DATA_LENGTH; + for ( ; length ; desc++) { + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? + length : DW_MCI_DESC_DATA_LENGTH; - length -= desc_len; + length -= desc_len; - /* - * Set the OWN bit and disable interrupts - * for this descriptor - */ - desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | - IDMAC_DES0_CH; + /* + * 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); + /* 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; + /* 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; + /* Update physical address for the next desc */ + mem_addr += desc_len; - /* Save pointer to the last descriptor */ - desc_last = desc; - } + /* Save pointer to the last descriptor */ + desc_last = desc; } + } - /* Set first descriptor */ - desc_first->des0 |= IDMAC_DES0_FD; + /* Set first descriptor */ + desc_first->des0 |= IDMAC_DES0_FD; - /* Set last descriptor */ - desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); - desc_last->des0 |= IDMAC_DES0_LD; + /* Set last descriptor */ + desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); + desc_last->des0 |= IDMAC_DES0_LD; +} - } else { - struct idmac_desc *desc_first, *desc_last, *desc; - desc_first = desc_last = desc = host->sg_cpu; +static inline void dw_mci_prepare_desc32(struct dw_mci *host, + struct mmc_data *data, + unsigned int sg_len) +{ + unsigned int desc_len; + struct idmac_desc *desc_first, *desc_last, *desc; + int i; - for (i = 0; i < sg_len; i++) { - unsigned int length = sg_dma_len(&data->sg[i]); + desc_first = desc_last = desc = host->sg_cpu; - u32 mem_addr = sg_dma_address(&data->sg[i]); + for (i = 0; i < sg_len; i++) { + unsigned int length = sg_dma_len(&data->sg[i]); - for ( ; length ; desc++) { - desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? - length : DW_MCI_DESC_DATA_LENGTH; + u32 mem_addr = sg_dma_address(&data->sg[i]); - length -= desc_len; + for ( ; length ; desc++) { + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? + length : DW_MCI_DESC_DATA_LENGTH; - /* - * Set the OWN bit and disable interrupts - * for this descriptor - */ - desc->des0 = cpu_to_le32(IDMAC_DES0_OWN | - IDMAC_DES0_DIC | - IDMAC_DES0_CH); + length -= desc_len; - /* Buffer length */ - IDMAC_SET_BUFFER1_SIZE(desc, 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); - /* Physical address to DMA to/from */ - desc->des2 = cpu_to_le32(mem_addr); + /* Buffer length */ + IDMAC_SET_BUFFER1_SIZE(desc, desc_len); - /* Update physical address for the next desc */ - mem_addr += desc_len; + /* Physical address to DMA to/from */ + desc->des2 = cpu_to_le32(mem_addr); - /* Save pointer to the last descriptor */ - desc_last = desc; - } + /* Update physical address for the next desc */ + mem_addr += desc_len; + + /* Save pointer to the last descriptor */ + desc_last = desc; } + } - /* Set first descriptor */ - desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD); + /* Set first descriptor */ + desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD); - /* Set last descriptor */ - desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | - IDMAC_DES0_DIC)); - desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); - } + /* Set last descriptor */ + desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | + IDMAC_DES0_DIC)); + desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); +} + +static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, + unsigned int sg_len) +{ + if (host->dma_64bit_address == 1) + dw_mci_prepare_desc64(host, data, sg_len); + else + dw_mci_prepare_desc32(host, data, sg_len); wmb(); /* drain writebuffer */ } -- 2.3.7 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] mmc: dw_mmc: avoid race condition of cpu and IDMAC 2016-08-19 9:40 ` [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 Shawn Lin @ 2016-08-19 9:40 ` Shawn Lin 2016-08-31 6:58 ` Jaehoon Chung 2016-08-31 6:55 ` [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 Jaehoon Chung 1 sibling, 1 reply; 6+ messages in thread From: Shawn Lin @ 2016-08-19 9:40 UTC (permalink / raw) To: Jaehoon Chung Cc: Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson, Brian Norris, Heiko Stuebner, linux-rockchip, Shawn Lin We could see an obvious race condition by test that the former write operation by IDMAC aiming to clear OWN bit reach right after the later configuration of the same desc, which makes the IDMAC be in SUSPEND state as the OWN bit was cleared by the asynchronous write operation of IDMAC. The bug can be very easy reproduced on RK3288 or similar when we reduce the running rate of system buses and keep the CPU running faster. So as two separate masters, IDMAC and cpu write the same descriptor stored on the same address, and this should be protected by adding check of OWN bit before preparing new descriptors. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/host/dw_mmc.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 0a5a49f..e640f83 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -473,6 +473,7 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, { unsigned int desc_len; struct idmac_desc_64addr *desc_first, *desc_last, *desc; + unsigned long timeout = jiffies + msecs_to_jiffies(100); int i; desc_first = desc_last = desc = host->sg_cpu; @@ -489,6 +490,20 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, length -= desc_len; /* + * Wait for the former clear OWN bit operation + * of IDMAC to make sure that this descriptor + * isn't still owned by IDMAC as IDMAC's write + * ops and CPU's read ops are asynchronous. + */ + while (readl(&desc->des0) & IDMAC_DES0_OWN) { + if (time_after(jiffies, timeout)) { + dev_err(host->dev, "DESC is still owned by IDMAC.\n"); + break; + } + udelay(10); + } + + /* * Set the OWN bit and disable interrupts * for this descriptor */ @@ -525,6 +540,7 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, { unsigned int desc_len; struct idmac_desc *desc_first, *desc_last, *desc; + unsigned long timeout = jiffies + msecs_to_jiffies(100); int i; desc_first = desc_last = desc = host->sg_cpu; @@ -541,6 +557,20 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, length -= desc_len; /* + * Wait for the former clear OWN bit operation + * of IDMAC to make sure that this descriptor + * isn't still owned by IDMAC as IDMAC's write + * ops and CPU's read ops are asynchronous. + */ + while (readl(&desc->des0) & IDMAC_DES0_OWN) { + if (time_after(jiffies, timeout)) { + dev_err(host->dev, "DESC is still owned by IDMAC.\n"); + break; + } + udelay(10); + } + + /* * Set the OWN bit and disable interrupts * for this descriptor */ -- 2.3.7 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mmc: dw_mmc: avoid race condition of cpu and IDMAC 2016-08-19 9:40 ` [PATCH 2/2] mmc: dw_mmc: avoid race condition of cpu and IDMAC Shawn Lin @ 2016-08-31 6:58 ` Jaehoon Chung 2016-08-31 7:36 ` Shawn Lin 0 siblings, 1 reply; 6+ messages in thread From: Jaehoon Chung @ 2016-08-31 6:58 UTC (permalink / raw) To: Shawn Lin Cc: Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson, Brian Norris, Heiko Stuebner, linux-rockchip Hi Shawn, On 08/19/2016 06:40 PM, Shawn Lin wrote: > We could see an obvious race condition by test that > the former write operation by IDMAC aiming to clear > OWN bit reach right after the later configuration of > the same desc, which makes the IDMAC be in SUSPEND > state as the OWN bit was cleared by the asynchronous > write operation of IDMAC. The bug can be very easy > reproduced on RK3288 or similar when we reduce the > running rate of system buses and keep the CPU running > faster. So as two separate masters, IDMAC and cpu > write the same descriptor stored on the same address, > and this should be protected by adding check of OWN > bit before preparing new descriptors. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/host/dw_mmc.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 0a5a49f..e640f83 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -473,6 +473,7 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, > { > unsigned int desc_len; > struct idmac_desc_64addr *desc_first, *desc_last, *desc; > + unsigned long timeout = jiffies + msecs_to_jiffies(100); > int i; > > desc_first = desc_last = desc = host->sg_cpu; > @@ -489,6 +490,20 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, > length -= desc_len; > > /* > + * Wait for the former clear OWN bit operation > + * of IDMAC to make sure that this descriptor > + * isn't still owned by IDMAC as IDMAC's write > + * ops and CPU's read ops are asynchronous. > + */ > + while (readl(&desc->des0) & IDMAC_DES0_OWN) { > + if (time_after(jiffies, timeout)) { > + dev_err(host->dev, "DESC is still owned by IDMAC.\n"); > + break; Doesn't it need the error handling? Just display the message? Best Regards, Jaehoon Chung > + } > + udelay(10); > + } > + > + /* > * Set the OWN bit and disable interrupts > * for this descriptor > */ > @@ -525,6 +540,7 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, > { > unsigned int desc_len; > struct idmac_desc *desc_first, *desc_last, *desc; > + unsigned long timeout = jiffies + msecs_to_jiffies(100); > int i; > > desc_first = desc_last = desc = host->sg_cpu; > @@ -541,6 +557,20 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, > length -= desc_len; > > /* > + * Wait for the former clear OWN bit operation > + * of IDMAC to make sure that this descriptor > + * isn't still owned by IDMAC as IDMAC's write > + * ops and CPU's read ops are asynchronous. > + */ > + while (readl(&desc->des0) & IDMAC_DES0_OWN) { > + if (time_after(jiffies, timeout)) { > + dev_err(host->dev, "DESC is still owned by IDMAC.\n"); > + break; > + } > + udelay(10); > + } > + > + /* > * Set the OWN bit and disable interrupts > * for this descriptor > */ > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mmc: dw_mmc: avoid race condition of cpu and IDMAC 2016-08-31 6:58 ` Jaehoon Chung @ 2016-08-31 7:36 ` Shawn Lin 0 siblings, 0 replies; 6+ messages in thread From: Shawn Lin @ 2016-08-31 7:36 UTC (permalink / raw) To: Jaehoon Chung Cc: shawn.lin, Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson, Brian Norris, Heiko Stuebner, linux-rockchip On 2016/8/31 14:58, Jaehoon Chung wrote: > Hi Shawn, > > On 08/19/2016 06:40 PM, Shawn Lin wrote: >> We could see an obvious race condition by test that >> the former write operation by IDMAC aiming to clear >> OWN bit reach right after the later configuration of >> the same desc, which makes the IDMAC be in SUSPEND >> state as the OWN bit was cleared by the asynchronous >> write operation of IDMAC. The bug can be very easy >> reproduced on RK3288 or similar when we reduce the >> running rate of system buses and keep the CPU running >> faster. So as two separate masters, IDMAC and cpu >> write the same descriptor stored on the same address, >> and this should be protected by adding check of OWN >> bit before preparing new descriptors. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/mmc/host/dw_mmc.c | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 0a5a49f..e640f83 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -473,6 +473,7 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, >> { >> unsigned int desc_len; >> struct idmac_desc_64addr *desc_first, *desc_last, *desc; >> + unsigned long timeout = jiffies + msecs_to_jiffies(100); >> int i; >> >> desc_first = desc_last = desc = host->sg_cpu; >> @@ -489,6 +490,20 @@ static inline void dw_mci_prepare_desc64(struct dw_mci *host, >> length -= desc_len; >> >> /* >> + * Wait for the former clear OWN bit operation >> + * of IDMAC to make sure that this descriptor >> + * isn't still owned by IDMAC as IDMAC's write >> + * ops and CPU's read ops are asynchronous. >> + */ >> + while (readl(&desc->des0) & IDMAC_DES0_OWN) { >> + if (time_after(jiffies, timeout)) { >> + dev_err(host->dev, "DESC is still owned by IDMAC.\n"); >> + break; > > Doesn't it need the error handling? Just display the message? One reason for why I didn't add error handling is that maybe we could add a very large timeout value for this, and the system could be broken anyway if it does need such a long time for a peripheral IP to write memory. But you are right maybe, it is not a good idea to do that. I will propgate a error and let it fall back to PIO mode. Thanks. > > Best Regards, > Jaehoon Chung > >> + } >> + udelay(10); >> + } >> + >> + /* >> * Set the OWN bit and disable interrupts >> * for this descriptor >> */ >> @@ -525,6 +540,7 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, >> { >> unsigned int desc_len; >> struct idmac_desc *desc_first, *desc_last, *desc; >> + unsigned long timeout = jiffies + msecs_to_jiffies(100); >> int i; >> >> desc_first = desc_last = desc = host->sg_cpu; >> @@ -541,6 +557,20 @@ static inline void dw_mci_prepare_desc32(struct dw_mci *host, >> length -= desc_len; >> >> /* >> + * Wait for the former clear OWN bit operation >> + * of IDMAC to make sure that this descriptor >> + * isn't still owned by IDMAC as IDMAC's write >> + * ops and CPU's read ops are asynchronous. >> + */ >> + while (readl(&desc->des0) & IDMAC_DES0_OWN) { >> + if (time_after(jiffies, timeout)) { >> + dev_err(host->dev, "DESC is still owned by IDMAC.\n"); >> + break; >> + } >> + udelay(10); >> + } >> + >> + /* >> * Set the OWN bit and disable interrupts >> * for this descriptor >> */ >> > > > > -- Best Regards Shawn Lin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 2016-08-19 9:40 ` [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 Shawn Lin 2016-08-19 9:40 ` [PATCH 2/2] mmc: dw_mmc: avoid race condition of cpu and IDMAC Shawn Lin @ 2016-08-31 6:55 ` Jaehoon Chung 2016-08-31 7:17 ` Shawn Lin 1 sibling, 1 reply; 6+ messages in thread From: Jaehoon Chung @ 2016-08-31 6:55 UTC (permalink / raw) To: Shawn Lin Cc: Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson, Brian Norris, Heiko Stuebner, linux-rockchip Hi Shawn, On 08/19/2016 06:40 PM, Shawn Lin wrote: > We intend to add more check for descriptors when > preparing desc. Let's spilt out the separate body > to make the dw_mci_translate_sglist not so lengthy. Sorry for reviewing late. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/host/dw_mmc.c | 148 +++++++++++++++++++++++++--------------------- > 1 file changed, 81 insertions(+), 67 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 32380d5..0a5a49f 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -467,103 +467,117 @@ static void dw_mci_dmac_complete_dma(void *arg) > } > } > > -static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, > - unsigned int sg_len) > +static inline void dw_mci_prepare_desc64(struct dw_mci *host, > + struct mmc_data *data, > + unsigned int sg_len) > { > unsigned int desc_len; > + struct idmac_desc_64addr *desc_first, *desc_last, *desc; > int i; > > - if (host->dma_64bit_address == 1) { > - struct idmac_desc_64addr *desc_first, *desc_last, *desc; > - > - desc_first = desc_last = desc = host->sg_cpu; > + desc_first = desc_last = desc = host->sg_cpu; > > - for (i = 0; i < sg_len; i++) { > - unsigned int length = sg_dma_len(&data->sg[i]); > + 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]); > + u64 mem_addr = sg_dma_address(&data->sg[i]); > > - for ( ; length ; desc++) { > - desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? > - length : DW_MCI_DESC_DATA_LENGTH; > + for ( ; length ; desc++) { > + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? > + length : DW_MCI_DESC_DATA_LENGTH; > > - length -= desc_len; > + length -= desc_len; > > - /* > - * Set the OWN bit and disable interrupts > - * for this descriptor > - */ > - desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | > - IDMAC_DES0_CH; > + /* > + * 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); > + /* 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; > + /* 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; > + /* Update physical address for the next desc */ > + mem_addr += desc_len; > > - /* Save pointer to the last descriptor */ > - desc_last = desc; > - } > + /* Save pointer to the last descriptor */ > + desc_last = desc; > } > + } > > - /* Set first descriptor */ > - desc_first->des0 |= IDMAC_DES0_FD; > + /* Set first descriptor */ > + desc_first->des0 |= IDMAC_DES0_FD; > > - /* Set last descriptor */ > - desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); > - desc_last->des0 |= IDMAC_DES0_LD; > + /* Set last descriptor */ > + desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); > + desc_last->des0 |= IDMAC_DES0_LD; > +} > > - } else { > - struct idmac_desc *desc_first, *desc_last, *desc; > > - desc_first = desc_last = desc = host->sg_cpu; > +static inline void dw_mci_prepare_desc32(struct dw_mci *host, > + struct mmc_data *data, > + unsigned int sg_len) > +{ > + unsigned int desc_len; > + struct idmac_desc *desc_first, *desc_last, *desc; > + int i; > > - for (i = 0; i < sg_len; i++) { > - unsigned int length = sg_dma_len(&data->sg[i]); > + desc_first = desc_last = desc = host->sg_cpu; > > - u32 mem_addr = sg_dma_address(&data->sg[i]); > + for (i = 0; i < sg_len; i++) { > + unsigned int length = sg_dma_len(&data->sg[i]); > > - for ( ; length ; desc++) { > - desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? > - length : DW_MCI_DESC_DATA_LENGTH; > + u32 mem_addr = sg_dma_address(&data->sg[i]); > > - length -= desc_len; > + for ( ; length ; desc++) { > + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? > + length : DW_MCI_DESC_DATA_LENGTH; > > - /* > - * Set the OWN bit and disable interrupts > - * for this descriptor > - */ > - desc->des0 = cpu_to_le32(IDMAC_DES0_OWN | > - IDMAC_DES0_DIC | > - IDMAC_DES0_CH); > + length -= desc_len; > > - /* Buffer length */ > - IDMAC_SET_BUFFER1_SIZE(desc, 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); > > - /* Physical address to DMA to/from */ > - desc->des2 = cpu_to_le32(mem_addr); > + /* Buffer length */ > + IDMAC_SET_BUFFER1_SIZE(desc, desc_len); > > - /* Update physical address for the next desc */ > - mem_addr += desc_len; > + /* Physical address to DMA to/from */ > + desc->des2 = cpu_to_le32(mem_addr); > > - /* Save pointer to the last descriptor */ > - desc_last = desc; > - } > + /* Update physical address for the next desc */ > + mem_addr += desc_len; > + > + /* Save pointer to the last descriptor */ > + desc_last = desc; > } > + } > > - /* Set first descriptor */ > - desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD); > + /* Set first descriptor */ > + desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD); > > - /* Set last descriptor */ > - desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | > - IDMAC_DES0_DIC)); > - desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); > - } > + /* Set last descriptor */ > + desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | > + IDMAC_DES0_DIC)); > + desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); > +} > + > +static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, > + unsigned int sg_len) > +{ > + if (host->dma_64bit_address == 1) > + dw_mci_prepare_desc64(host, data, sg_len); > + else > + dw_mci_prepare_desc32(host, data, sg_len); I think dw_mci_translate_sglist can be removed. Instead, these functions should be called in dw_mci_idamc_start_dma(). How about? Best Regards, Jaehoon Chung > > wmb(); /* drain writebuffer */ > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 2016-08-31 6:55 ` [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 Jaehoon Chung @ 2016-08-31 7:17 ` Shawn Lin 0 siblings, 0 replies; 6+ messages in thread From: Shawn Lin @ 2016-08-31 7:17 UTC (permalink / raw) To: Jaehoon Chung Cc: shawn.lin, Ulf Hansson, linux-mmc, linux-kernel, Doug Anderson, Brian Norris, Heiko Stuebner, linux-rockchip Hi Jaehoon, On 2016/8/31 14:55, Jaehoon Chung wrote: > Hi Shawn, > > On 08/19/2016 06:40 PM, Shawn Lin wrote: >> We intend to add more check for descriptors when >> preparing desc. Let's spilt out the separate body >> to make the dw_mci_translate_sglist not so lengthy. > > Sorry for reviewing late. > >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/mmc/host/dw_mmc.c | 148 +++++++++++++++++++++++++--------------------- >> 1 file changed, 81 insertions(+), 67 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 32380d5..0a5a49f 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -467,103 +467,117 @@ static void dw_mci_dmac_complete_dma(void *arg) >> } >> } >> >> -static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, >> - unsigned int sg_len) >> +static inline void dw_mci_prepare_desc64(struct dw_mci *host, >> + struct mmc_data *data, >> + unsigned int sg_len) >> { >> unsigned int desc_len; >> + struct idmac_desc_64addr *desc_first, *desc_last, *desc; >> int i; >> >> - if (host->dma_64bit_address == 1) { >> - struct idmac_desc_64addr *desc_first, *desc_last, *desc; >> - >> - desc_first = desc_last = desc = host->sg_cpu; >> + desc_first = desc_last = desc = host->sg_cpu; >> >> - for (i = 0; i < sg_len; i++) { >> - unsigned int length = sg_dma_len(&data->sg[i]); >> + 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]); >> + u64 mem_addr = sg_dma_address(&data->sg[i]); >> >> - for ( ; length ; desc++) { >> - desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? >> - length : DW_MCI_DESC_DATA_LENGTH; >> + for ( ; length ; desc++) { >> + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? >> + length : DW_MCI_DESC_DATA_LENGTH; >> >> - length -= desc_len; >> + length -= desc_len; >> >> - /* >> - * Set the OWN bit and disable interrupts >> - * for this descriptor >> - */ >> - desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | >> - IDMAC_DES0_CH; >> + /* >> + * 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); >> + /* 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; >> + /* 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; >> + /* Update physical address for the next desc */ >> + mem_addr += desc_len; >> >> - /* Save pointer to the last descriptor */ >> - desc_last = desc; >> - } >> + /* Save pointer to the last descriptor */ >> + desc_last = desc; >> } >> + } >> >> - /* Set first descriptor */ >> - desc_first->des0 |= IDMAC_DES0_FD; >> + /* Set first descriptor */ >> + desc_first->des0 |= IDMAC_DES0_FD; >> >> - /* Set last descriptor */ >> - desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); >> - desc_last->des0 |= IDMAC_DES0_LD; >> + /* Set last descriptor */ >> + desc_last->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC); >> + desc_last->des0 |= IDMAC_DES0_LD; >> +} >> >> - } else { >> - struct idmac_desc *desc_first, *desc_last, *desc; >> >> - desc_first = desc_last = desc = host->sg_cpu; >> +static inline void dw_mci_prepare_desc32(struct dw_mci *host, >> + struct mmc_data *data, >> + unsigned int sg_len) >> +{ >> + unsigned int desc_len; >> + struct idmac_desc *desc_first, *desc_last, *desc; >> + int i; >> >> - for (i = 0; i < sg_len; i++) { >> - unsigned int length = sg_dma_len(&data->sg[i]); >> + desc_first = desc_last = desc = host->sg_cpu; >> >> - u32 mem_addr = sg_dma_address(&data->sg[i]); >> + for (i = 0; i < sg_len; i++) { >> + unsigned int length = sg_dma_len(&data->sg[i]); >> >> - for ( ; length ; desc++) { >> - desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? >> - length : DW_MCI_DESC_DATA_LENGTH; >> + u32 mem_addr = sg_dma_address(&data->sg[i]); >> >> - length -= desc_len; >> + for ( ; length ; desc++) { >> + desc_len = (length <= DW_MCI_DESC_DATA_LENGTH) ? >> + length : DW_MCI_DESC_DATA_LENGTH; >> >> - /* >> - * Set the OWN bit and disable interrupts >> - * for this descriptor >> - */ >> - desc->des0 = cpu_to_le32(IDMAC_DES0_OWN | >> - IDMAC_DES0_DIC | >> - IDMAC_DES0_CH); >> + length -= desc_len; >> >> - /* Buffer length */ >> - IDMAC_SET_BUFFER1_SIZE(desc, 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); >> >> - /* Physical address to DMA to/from */ >> - desc->des2 = cpu_to_le32(mem_addr); >> + /* Buffer length */ >> + IDMAC_SET_BUFFER1_SIZE(desc, desc_len); >> >> - /* Update physical address for the next desc */ >> - mem_addr += desc_len; >> + /* Physical address to DMA to/from */ >> + desc->des2 = cpu_to_le32(mem_addr); >> >> - /* Save pointer to the last descriptor */ >> - desc_last = desc; >> - } >> + /* Update physical address for the next desc */ >> + mem_addr += desc_len; >> + >> + /* Save pointer to the last descriptor */ >> + desc_last = desc; >> } >> + } >> >> - /* Set first descriptor */ >> - desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD); >> + /* Set first descriptor */ >> + desc_first->des0 |= cpu_to_le32(IDMAC_DES0_FD); >> >> - /* Set last descriptor */ >> - desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | >> - IDMAC_DES0_DIC)); >> - desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); >> - } >> + /* Set last descriptor */ >> + desc_last->des0 &= cpu_to_le32(~(IDMAC_DES0_CH | >> + IDMAC_DES0_DIC)); >> + desc_last->des0 |= cpu_to_le32(IDMAC_DES0_LD); >> +} >> + >> +static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, >> + unsigned int sg_len) >> +{ >> + if (host->dma_64bit_address == 1) >> + dw_mci_prepare_desc64(host, data, sg_len); >> + else >> + dw_mci_prepare_desc32(host, data, sg_len); > > I think dw_mci_translate_sglist can be removed. > Instead, these functions should be called in dw_mci_idamc_start_dma(). > How about? > Sounds fine, I will do it. > Best Regards, > Jaehoon Chung > >> >> wmb(); /* drain writebuffer */ >> } >> > > > > -- Best Regards Shawn Lin ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-31 7:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20160819095620epcas1p28ffdabf62af8d6e3c01941fde8c8b96b@epcas1p2.samsung.com>
2016-08-19 9:40 ` [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 Shawn Lin
2016-08-19 9:40 ` [PATCH 2/2] mmc: dw_mmc: avoid race condition of cpu and IDMAC Shawn Lin
2016-08-31 6:58 ` Jaehoon Chung
2016-08-31 7:36 ` Shawn Lin
2016-08-31 6:55 ` [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64 Jaehoon Chung
2016-08-31 7:17 ` Shawn Lin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®