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

Submitted by Amar on Dec. 5, 2012, 1:31 p.m.

Details

Message ID 1354714297-11568-2-git-send-email-amarendra.xt@samsung.com
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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