Hi Simon,
Thanks for the response.
Please find my response below.
Thanks & Regards
Amarendra
On 9 February 2013 22:24, Simon Glass <s...@chromium.org> wrote:
> Hi Amarendra,
>
> On Mon, Jan 28, 2013 at 1:31 AM, Amarendra Reddy
> <amar.lavan...@gmail.com> wrote:
> > Hi Simon,
> >
> > Please find the responses below.
> >
> > Thanks & Regards
> > Amarendra
> >
> > On 27 January 2013 01:38, Simon Glass <s...@chromium.org> wrote:
> >>
> >> Hi,
> >>
> >> On Tue, Jan 22, 2013 at 12:43 AM, Amar <amarendra...@samsung.com>
> wrote:
> >> > This patch adds FDT support for DWMMC, by reading the DWMMC node data
> >> > from the device tree and initialising DWMMC channels as per data
> >> > obtained from the node.
> >> >
> >> > Signed-off-by: Vivek Gautam <gautam.vi...@samsung.com>
> >> > Signed-off-by: Amar <amarendra...@samsung.com>
> >>
> >> Acked-by: Simon Glass <s...@chromium.org>
> >>
> >> See below for nit/question.
> >>
> >> > ---
> >> > Changes since V1:
> >> > 1)Updated code to have same signature for the function
> >> > exynos_dwmci_init() for both FDT and non-FDT versions.
> >> > 2)Updated code to pass device_id parameter to the function
> >> > exynos5_mmc_set_clk_div() instead of index.
> >> > 3)Updated code to decode the value of "samsung,width" from
> FDT.
> >> > 4)Channel index is computed instead of getting from FDT.
> >> >
> >> > Changes since V2:
> >> > 1)Updation of commit message and resubmition of proper patch
> >> > set.
> >> >
> >> > Changes since V3:
> >> > 1)Replaced the new function exynos5_mmc_set_clk_div() with the
> >> > existing function set_mmc_clk(). set_mmc_clk() will do the
> >> > purpose.
> >> > 2)Computation of FSYS block clock divisor (pre-ratio) is
> added.
> >> >
> >> > Changes since V4:
> >> > 1)Replaced "unsigned int exynos_dwmmc_init(int index, int
> >> > bus_width)" with
> >> > int exynos_dwmci_add_port(int, u32, inth, u32)
> >> > i)exynos_dwmmc_add_port() will be used by non-FDT
> >> > boards.
> >> > ii)In FDT case, exynos_dwmmc_init(const void *blob)
> will
> >> > use
> >> > exynos_dwmmc_add_port() for every channel enabled in
> >> > device node.
> >> > 2)Changed the computation method of mmc clock divisor.
> >> > 3)Updated exynos_dwmmc_init() to compute the 'clksel_val'
> within
> >> > the function.
> >> >
> >> > arch/arm/include/asm/arch-exynos/dwmmc.h | 11 +--
> >> > drivers/mmc/exynos_dw_mmc.c | 127
> >> > ++++++++++++++++++++++++++++---
> >> > include/dwmmc.h | 3 +
> >> > 3 files changed, 124 insertions(+), 17 deletions(-)
> >> >
> >> > diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
> >> > b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >> > index 8acdf9b..3b147b8 100644
> >> > --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
> >> > +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >> > @@ -27,10 +27,7 @@
> >> > #define DWMCI_SET_DRV_CLK(x) ((x) << 16)
> >> > #define DWMCI_SET_DIV_RATIO(x) ((x) << 24)
> >> >
> >> > -int exynos_dwmci_init(u32 regbase, int bus_width, int index);
> >> > -
> >> > -static inline unsigned int exynos_dwmmc_init(int index, int
> bus_width)
> >> > -{
> >> > - unsigned int base = samsung_get_base_mmc() + (0x10000 *
> index);
> >> > - return exynos_dwmci_init(base, bus_width, index);
> >> > -}
> >> > +#ifdef CONFIG_OF_CONTROL
> >> > +int exynos_dwmmc_init(const void *blob);
> >> > +#endif
> >> > +int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32
> >> > clksel);
> >> > diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
> >> > index 72a31b7..1d09e56 100644
> >> > --- a/drivers/mmc/exynos_dw_mmc.c
> >> > +++ b/drivers/mmc/exynos_dw_mmc.c
> >> > @@ -19,39 +19,146 @@
> >> > */
> >> >
> >> > #include <common.h>
> >> > -#include <malloc.h>
> >> > #include <dwmmc.h>
> >> > +#include <fdtdec.h>
> >> > +#include <libfdt.h>
> >> > +#include <malloc.h>
> >> > #include <asm/arch/dwmmc.h>
> >> > #include <asm/arch/clk.h>
> >> > +#include <asm/arch/pinmux.h>
> >> >
> >> > -static char *EXYNOS_NAME = "EXYNOS DWMMC";
> >> > +#define DWMMC_MAX_CH_NUM 4
> >> > +#define DWMMC_MAX_FREQ 52000000
> >> > +#define DWMMC_MIN_FREQ 400000
> >> > +#define DWMMC_MMC0_CLKSEL_VAL 0x03030001
> >> > +#define DWMMC_MMC2_CLKSEL_VAL 0x03020001
> >> >
> >> > +/*
> >> > + * Function used as callback function to initialise the
> >> > + * CLKSEL register for every mmc channel.
> >> > + */
> >> > static void exynos_dwmci_clksel(struct dwmci_host *host)
> >> > {
> >> > - u32 val;
> >> > - val = DWMCI_SET_SAMPLE_CLK(DWMCI_SHIFT_0) |
> >> > - DWMCI_SET_DRV_CLK(DWMCI_SHIFT_0) |
> >> > DWMCI_SET_DIV_RATIO(0);
> >> > + dwmci_writel(host, DWMCI_CLKSEL, host->clksel_val);
> >> > +}
> >> >
> >> > - dwmci_writel(host, DWMCI_CLKSEL, val);
> >> > +unsigned int exynos_dwmci_get_clk(int dev_index)
> >> > +{
> >> > + return get_mmc_clk(dev_index);
> >> > }
> >> >
> >> > -int exynos_dwmci_init(u32 regbase, int bus_width, int index)
> >> > +/*
> >> > + * This function adds the mmc channel to be registered with mmc core.
> >> > + * index - mmc channel number.
> >> > + * regbase - register base address of mmc channel specified in
> >> > 'index'.
> >> > + * bus_width - operating bus width of mmc channel specified in
> 'index'.
> >> > + * clksel - value to be written into CLKSEL register in case of
> FDT.
> >> > + * NULL in case od non-FDT.
> >> > + */
> >> > +int exynos_dwmci_add_port(int index, u32 regbase, int bus_width, u32
> >> > clksel)
> >> > {
> >> > struct dwmci_host *host = NULL;
> >> > + unsigned int div;
> >> > + unsigned long freq, sclk;
> >> > host = malloc(sizeof(struct dwmci_host));
> >> > if (!host) {
> >> > printf("dwmci_host malloc fail!\n");
> >> > return 1;
> >> > }
> >> > + /* request mmc clock vlaue of 50MHz. */
> >> > + freq = 50000000;
> >>
> >> Should this be 52MHz?
> >
> > Ok shall change it to 52MHz.
> >>
> >>
> >> > + sclk = get_mmc_clk(index);
> >> > + div = DIV_ROUND_UP(sclk, freq);
> >> > + /* set the clock divisor for mmc */
> >> > + set_mmc_clk(index, div);
> >> >
> >> > - host->name = EXYNOS_NAME;
> >> > + host->name = "EXYNOS DWMMC";
> >> > host->ioaddr = (void *)regbase;
> >> > host->buswidth = bus_width;
> >> > +
> >> > + if (clksel)
> >>
> >> {} around if() part since you have it for else.
> >>
> >
> > Ok will put {} around if() part.
> >
> >>
> >> > + host->clksel_val = clksel;
> >> > + else {
> >> > + if (0 == index)
> >> > + host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
> >> > + if (2 == index)
> >> > + host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
> >> > + }
> >> > +
> >> > host->clksel = exynos_dwmci_clksel;
> >> > host->dev_index = index;
> >> > + host->mmc_clk = exynos_dwmci_get_clk;
> >> > + /* Add the mmc channel to be registered with mmc core */
> >> > + if (add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ)) {
> >> > + printf("dwmmc%d registration failed\n", index);
> >>
> >> Should this be debug()?
> >
> > I think it should be printf(), because if the mmc channel registration
> > fails, then this failure information needs to be conveyed to the user.
> > This will happen if printf() is used.
> > If debug() is used, then the statement will be printed only when the
> DEBUG
> > is defined.
>
> It's a tricky point, and it depends on what else you have in terns of
> error reporting. This is not a user-facing function, but something
> called by the internals of your code. I think as a general principle
> errors should be reported by the commands that started the operation.
>
> In this case exynos_dwmci_add_port() already has an error return code,
> and its caller can presumable report the error, although it would not
> include the 'dwmmc' text.
>
> The problem with putting printf()s in drivers is that it can make it
> impossible to silently do something. For example if I want to write a
> command to check for a valid partition on an MMC device, and all of
> the MMC, partition and filesystem code prints out errors along the
> way, it can make for very confusing output, and can make it impossible
> to write a higher-level function which silently checks for the valid
> partition.
>
> So in general I think we should try to keep printf() out of drivers.
> We have talked on the list about some sort of dynamic verbosity, and
> someone may look at that at some point, but at the moment we just have
> printf() and debug().
>
> Regards,
> Simon
>
>
Ok, will replace the printf() with debug() in the driver file
drivers/mmc/exynos_dw_mmc.c.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
