| Message ID | 1471599615-6249-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */ > } > -- 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 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 */ >> } >> > > > >
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 */ }
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(-)