Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1473071 > unrolled thread

Re: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64

Started byJaehoon Chung <jh80.chung@samsung.com>
First post2016-08-31 09:00 +0200
Last post2016-08-31 09:30 +0200
Articles 2 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32  and IDMAC64 Jaehoon Chung <jh80.chung@samsung.com> - 2016-08-31 09:00 +0200
    Re: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for  IDMAC32 and IDMAC64 Shawn Lin <shawn.lin@rock-chips.com> - 2016-08-31 09:30 +0200

#1473071 — Re: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64

FromJaehoon Chung <jh80.chung@samsung.com>
Date2016-08-31 09:00 +0200
SubjectRe: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64
Message-ID<sc7F8-7q3-23@gated-at.bofh.it>
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 */
>  }
>

[toc] | [next] | [standalone]

#1473091 — Re: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64

FromShawn Lin <shawn.lin@rock-chips.com>
Date2016-08-31 09:30 +0200
SubjectRe: [PATCH 1/2] mmc: dw_mmc: split out preparation of desc for IDMAC32 and IDMAC64
Message-ID<sc88a-7Pb-27@gated-at.bofh.it>
In reply to#1473071
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web