| Message ID | 1354714297-11568-2-git-send-email-amarendra.xt@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
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 >
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
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 >
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 >
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.
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 >
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.
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
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(-)