Patchwork [U-Boot,1/4] MMC: DWMMC: Modified fifo size computation

login
register
mail settings
Submitter Amar
Date Dec. 5, 2012, 1:31 p.m.
Message ID <1354714297-11568-2-git-send-email-amarendra.xt@samsung.com>
Download mbox | patch
Permalink /patch/203869/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Amar - Dec. 5, 2012, 1:31 p.m.
The current implementation of fifo size computation was giving improper
values for eMMC channel. Modified the computation as per user manual.

Signed-off-by: Amarendra Reddy <amarendra.xt@samsung.com>
---
 drivers/mmc/dw_mmc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Jaehoon Chung - Dec. 6, 2012, 3:59 a.m.
It looks good to me.
Added minor comment.

Acked-by: Jaehoon Chung <jh80.chung@samsung.com>

On 12/05/2012 10:31 PM, Amar wrote:
> The current implementation of fifo size computation was giving improper
> values for eMMC channel. Modified the computation as per user manual.
> 
> Signed-off-by: Amarendra Reddy <amarendra.xt@samsung.com>
> ---
>  drivers/mmc/dw_mmc.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> index 4070d4e..62dc152 100644
> --- a/drivers/mmc/dw_mmc.c
> +++ b/drivers/mmc/dw_mmc.c
> @@ -332,6 +332,8 @@ static int dwmci_init(struct mmc *mmc)
>  	dwmci_writel(host, DWMCI_BMOD, 1);
>  
>  	fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
> +	fifo_size = ((fifo_size & RX_WMARK(0xFFF)) >> 16) + 1;
How about using like FIFO_SIZE_MASK?
> +
>  	if (host->fifoth_val)
>  		fifoth_val = host->fifoth_val;
>  	else
>
Simon Glass - Dec. 8, 2012, 7:49 p.m.
Hi Jaehoon,

On Wed, Dec 5, 2012 at 7:59 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> It looks good to me.
> Added minor comment.
>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
>
> On 12/05/2012 10:31 PM, Amar wrote:
>> The current implementation of fifo size computation was giving improper
>> values for eMMC channel. Modified the computation as per user manual.
>>
>> Signed-off-by: Amarendra Reddy <amarendra.xt@samsung.com>
>> ---
>>  drivers/mmc/dw_mmc.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>> index 4070d4e..62dc152 100644
>> --- a/drivers/mmc/dw_mmc.c
>> +++ b/drivers/mmc/dw_mmc.c
>> @@ -332,6 +332,8 @@ static int dwmci_init(struct mmc *mmc)
>>       dwmci_writel(host, DWMCI_BMOD, 1);
>>
>>       fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
>> +     fifo_size = ((fifo_size & RX_WMARK(0xFFF)) >> 16) + 1;
> How about using like FIFO_SIZE_MASK?

It might be better to avoid macros in header files which shift and
mask x, since they obscure the operation, and just define the amount
of shift and mask in the header file. Also if you have a #define for
the mask you should probably also have one for the shift, otherwise
you have the information in two places. So maybe:

#define RX_WMARK_SHIFT 16
#define RX_WMARK_MASK (0xfff << RX_WMARK_SHIFT)

fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;

>> +
>>       if (host->fifoth_val)
>>               fifoth_val = host->fifoth_val;
>>       else
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon
Jaehoon Chung - Dec. 10, 2012, 2:21 a.m.
Hi Simon,

On 12/09/2012 04:49 AM, Simon Glass wrote:
> Hi Jaehoon,
> 
> On Wed, Dec 5, 2012 at 7:59 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> It looks good to me.
>> Added minor comment.
>>
>> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
>>
>> On 12/05/2012 10:31 PM, Amar wrote:
>>> The current implementation of fifo size computation was giving improper
>>> values for eMMC channel. Modified the computation as per user manual.
>>>
>>> Signed-off-by: Amarendra Reddy <amarendra.xt@samsung.com>
>>> ---
>>>  drivers/mmc/dw_mmc.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>>> index 4070d4e..62dc152 100644
>>> --- a/drivers/mmc/dw_mmc.c
>>> +++ b/drivers/mmc/dw_mmc.c
>>> @@ -332,6 +332,8 @@ static int dwmci_init(struct mmc *mmc)
>>>       dwmci_writel(host, DWMCI_BMOD, 1);
>>>
>>>       fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
>>> +     fifo_size = ((fifo_size & RX_WMARK(0xFFF)) >> 16) + 1;
>> How about using like FIFO_SIZE_MASK?
> 
> It might be better to avoid macros in header files which shift and
> mask x, since they obscure the operation, and just define the amount
> of shift and mask in the header file. Also if you have a #define for
> the mask you should probably also have one for the shift, otherwise
> you have the information in two places. So maybe:
I want to add the macro into header file like your suggestion.
RX_WMARK() is used to set with user input or pdata.
If Amarendra will change this patch, it will looks great to me.

Best Regards,
Jaehoon Chung
> 
> #define RX_WMARK_SHIFT 16
> #define RX_WMARK_MASK (0xfff << RX_WMARK_SHIFT)
> 
> fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;
> 
>>> +
>>>       if (host->fifoth_val)
>>>               fifoth_val = host->fifoth_val;
>>>       else
>>>
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Amarendra Reddy - Dec. 10, 2012, 6:09 a.m.
Hi Jaehoon Chung,

I will update the patches as per your review comments.

As suggested by Mr.Simon, I will create macros similar to the below macros
in header file.

 #define RX_WMARK_SHIFT 16
 #define RX_WMARK_MASK (0xfff << RX_WMARK_SHIFT)


Thanks & Regards
Amarendra

On 10 December 2012 07:51, Jaehoon Chung <jh80.chung@samsung.com> wrote:

> Hi Simon,
>
> On 12/09/2012 04:49 AM, Simon Glass wrote:
> > Hi Jaehoon,
> >
> > On Wed, Dec 5, 2012 at 7:59 PM, Jaehoon Chung <jh80.chung@samsung.com>
> wrote:
> >> It looks good to me.
> >> Added minor comment.
> >>
> >> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
> >>
> >> On 12/05/2012 10:31 PM, Amar wrote:
> >>> The current implementation of fifo size computation was giving improper
> >>> values for eMMC channel. Modified the computation as per user manual.
> >>>
> >>> Signed-off-by: Amarendra Reddy <amarendra.xt@samsung.com>
> >>> ---
> >>>  drivers/mmc/dw_mmc.c |    2 ++
> >>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> >>> index 4070d4e..62dc152 100644
> >>> --- a/drivers/mmc/dw_mmc.c
> >>> +++ b/drivers/mmc/dw_mmc.c
> >>> @@ -332,6 +332,8 @@ static int dwmci_init(struct mmc *mmc)
> >>>       dwmci_writel(host, DWMCI_BMOD, 1);
> >>>
> >>>       fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
> >>> +     fifo_size = ((fifo_size & RX_WMARK(0xFFF)) >> 16) + 1;
> >> How about using like FIFO_SIZE_MASK?
> >
> > It might be better to avoid macros in header files which shift and
> > mask x, since they obscure the operation, and just define the amount
> > of shift and mask in the header file. Also if you have a #define for
> > the mask you should probably also have one for the shift, otherwise
> > you have the information in two places. So maybe:
> I want to add the macro into header file like your suggestion.
> RX_WMARK() is used to set with user input or pdata.
> If Amarendra will change this patch, it will looks great to me.
>
> Best Regards,
> Jaehoon Chung
>  >
> > #define RX_WMARK_SHIFT 16
> > #define RX_WMARK_MASK (0xfff << RX_WMARK_SHIFT)
> >
> > fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;
> >
> >>> +
> >>>       if (host->fifoth_val)
> >>>               fifoth_val = host->fifoth_val;
> >>>       else
> >>>
> >>
> >> _______________________________________________
> >> U-Boot mailing list
> >> U-Boot@lists.denx.de
> >> http://lists.denx.de/mailman/listinfo/u-boot
> >
> > Regards,
> > Simon
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Minkyu Kang - March 27, 2013, 5:03 a.m.
Dear Amar,

On 05/12/12 22:31, Amar wrote:
> The current implementation of fifo size computation was giving improper
> values for eMMC channel. Modified the computation as per user manual.
> 
> Signed-off-by: Amarendra Reddy <amarendra.xt@samsung.com>
> ---
>  drivers/mmc/dw_mmc.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> index 4070d4e..62dc152 100644
> --- a/drivers/mmc/dw_mmc.c
> +++ b/drivers/mmc/dw_mmc.c
> @@ -332,6 +332,8 @@ static int dwmci_init(struct mmc *mmc)
>  	dwmci_writel(host, DWMCI_BMOD, 1);
>  
>  	fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
> +	fifo_size = ((fifo_size & RX_WMARK(0xFFF)) >> 16) + 1;

The definition of RX_WMARK is missing.

> +
>  	if (host->fifoth_val)
>  		fifoth_val = host->fifoth_val;
>  	else
> 

Thanks,
Minkyu Kang.
Amarendra Reddy - March 27, 2013, 5:18 a.m.
Dear Minkyu,

The definition of "RX_WMARK" is present in "[PATCH V7 04/10] EXYNOS5:
DWMMC: Added FDT support for DWMMC".

Here is the URL,
http://www.mail-archive.com/u-boot@lists.denx.de/msg107515.html

Thanks & Regards
Amarendra Reddy

On 27 March 2013 10:33, Minkyu Kang <mk7.kang@samsung.com> wrote:

> Dear Amar,
>
> On 05/12/12 22:31, Amar wrote:
> > The current implementation of fifo size computation was giving improper
> > values for eMMC channel. Modified the computation as per user manual.
> >
> > Signed-off-by: Amarendra Reddy <amarendra.xt@samsung.com>
> > ---
> >  drivers/mmc/dw_mmc.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> > index 4070d4e..62dc152 100644
> > --- a/drivers/mmc/dw_mmc.c
> > +++ b/drivers/mmc/dw_mmc.c
> > @@ -332,6 +332,8 @@ static int dwmci_init(struct mmc *mmc)
> >       dwmci_writel(host, DWMCI_BMOD, 1);
> >
> >       fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
> > +     fifo_size = ((fifo_size & RX_WMARK(0xFFF)) >> 16) + 1;
>
> The definition of RX_WMARK is missing.
>
> > +
> >       if (host->fifoth_val)
> >               fifoth_val = host->fifoth_val;
> >       else
> >
>
> Thanks,
> Minkyu Kang.
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Minkyu Kang - March 27, 2013, 5:39 a.m.
Dear Amar,

On 27/03/13 14:18, Amarendra Reddy wrote:
> Dear Minkyu,
> 

please don't top posting.

> The definition of "RX_WMARK" is present in "[PATCH V7 04/10] EXYNOS5: DWMMC: Added FDT support for DWMMC".
> 
> Here is the URL, http://www.mail-archive.com/u-boot@lists.denx.de/msg107515.html

Then please move it to this patch.

> 
> Thanks & Regards
> Amarendra Reddy
> 
> On 27 March 2013 10:33, Minkyu Kang <mk7.kang@samsung.com <mailto:mk7.kang@samsung.com>> wrote:
> 
>     Dear Amar,
> 
>     On 05/12/12 22:31, Amar wrote:
>     > The current implementation of fifo size computation was giving improper
>     > values for eMMC channel. Modified the computation as per user manual.
>     >
>     > Signed-off-by: Amarendra Reddy <amarendra.xt@samsung.com <mailto:amarendra.xt@samsung.com>>
>     > ---
>     >  drivers/mmc/dw_mmc.c |    2 ++
>     >  1 files changed, 2 insertions(+), 0 deletions(-)
>     >
>     > diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>     > index 4070d4e..62dc152 100644
>     > --- a/drivers/mmc/dw_mmc.c
>     > +++ b/drivers/mmc/dw_mmc.c
>     > @@ -332,6 +332,8 @@ static int dwmci_init(struct mmc *mmc)
>     >       dwmci_writel(host, DWMCI_BMOD, 1);
>     >
>     >       fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
>     > +     fifo_size = ((fifo_size & RX_WMARK(0xFFF)) >> 16) + 1;
> 
>     The definition of RX_WMARK is missing.
> 
>     > +
>     >       if (host->fifoth_val)
>     >               fifoth_val = host->fifoth_val;
>     >       else
>     >
> 
>     Thanks,
>     Minkyu Kang.
>     _______________________________________________
>     U-Boot mailing list
>     U-Boot@lists.denx.de <mailto:U-Boot@lists.denx.de>
>     http://lists.denx.de/mailman/listinfo/u-boot
> 
> 

Thanks,
Minkyu Kang.

Patch

diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
index 4070d4e..62dc152 100644
--- a/drivers/mmc/dw_mmc.c
+++ b/drivers/mmc/dw_mmc.c
@@ -332,6 +332,8 @@  static int dwmci_init(struct mmc *mmc)
 	dwmci_writel(host, DWMCI_BMOD, 1);
 
 	fifo_size = dwmci_readl(host, DWMCI_FIFOTH);
+	fifo_size = ((fifo_size & RX_WMARK(0xFFF)) >> 16) + 1;
+
 	if (host->fifoth_val)
 		fifoth_val = host->fifoth_val;
 	else