Hi Jaehoon,
Thanks for your comments.
Please find the response below.
Thanks & Regards
Amarendra
On 21 December 2012 14:48, Jaehoon Chung <jh80.ch...@samsung.com> wrote:
> On 12/21/2012 02:16 PM, Amarendra Reddy wrote:
> > Hi Simon,
> >
> > Thanks for your review comments.
> > Please find the responses below.
> >
> > Thanks & Regards
> > Amarendra Reddy
> >
> > On 20 December 2012 07:53, Simon Glass <s...@chromium.org> wrote:
> >
> >> Hi Amar,
> >>
> >> On Mon, Dec 17, 2012 at 3:19 AM, Amar <amarendra...@samsung.com> wrote:
> >>
> >>> Signed-off-by: Amar <amarendra...@samsung.com>
> >>>
> >>
> >> Good to see this patch! Please can you add a short commit message?
> >> Oops.. sorry. I will put the commit msg.
> >>
> >>> ---
> >>
> >> arch/arm/include/asm/arch-exynos/dwmmc.h | 4 +
> >>> drivers/mmc/exynos_dw_mmc.c | 117
> >>> ++++++++++++++++++++++++++++--
> >>> include/dwmmc.h | 4 +
> >>> 3 files changed, 119 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> index 8acdf9b..92352df 100644
> >>> --- a/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> +++ b/arch/arm/include/asm/arch-exynos/dwmmc.h
> >>> @@ -27,6 +27,9 @@
> >>> #define DWMCI_SET_DRV_CLK(x) ((x) << 16)
> >>> #define DWMCI_SET_DIV_RATIO(x) ((x) << 24)
> >>>
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +unsigned int exynos_dwmmc_init(const void *blob);
> >>> +#else
> >>> int exynos_dwmci_init(u32 regbase, int bus_width, int index);
> >>>
> >>> static inline unsigned int exynos_dwmmc_init(int index, int bus_width)
> >>> @@ -34,3 +37,4 @@ 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);
> >>> }
> >>> +#endif
> >>> diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c
> >>> index 72a31b7..3b3e3e5 100644
> >>> --- a/drivers/mmc/exynos_dw_mmc.c
> >>> +++ b/drivers/mmc/exynos_dw_mmc.c
> >>> @@ -19,23 +19,38 @@
> >>> */
> >>>
> >>> #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>
> >>> +
> >>> +#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
> >>>
> >>
> >> What do these last two values mean? I think clock setting should be
> done in
> >> clock.c, not here.
> >>
> >> In case of non-dt support, these two values are written into respective
> > clock selection register-CLKSEL, to select the clock for MMC-channel0 and
> > MMC-channel2.
> > Will move clock setting into clock.c and the #defines will be moved into
> > include/asm/arch/clk.h.
> I didn't want to move into clk.h. Why do you move there?
> It is related with CLKSEL register into DesignWare IP.
>
> Ok, now I understand that all register settings corresponding to
DesignWare IP should be present in exynos_dw_mmc.c / dw_mmc.c.
Fine, I will not move CLKSEL register setting into clock.c / clk.h.
> >
> >>
> >>>
> >>> static char *EXYNOS_NAME = "EXYNOS DWMMC";
> >>>
> >>> 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);
> >>> }
> >>>
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +static int exynos_dwmci_init(u32 regbase, int bus_width,
> >>> + int index, u32 *timing)
> >>> +#else
> >>> int exynos_dwmci_init(u32 regbase, int bus_width, int index)
> >>> +#endif
> >>>
> >>
> >> I'm really not keen on having the same function with different
> signatures.
> >> My first question is whether we need to support operation without
> >> CONFIG_OF_CONTROL. If so, I suggest having an init routine that takes an
> >> FDT blob as a parameter, and a separate add_port function which can be
> >> called by non-FDT-enabled board files. The init routine will call the
> >> add_port function for each port it finds in the FDT.
> >>
> >> I think we need support operation without CONFIG_OF_CONTROL atleast
> untill
> > the entire FDT is formed.
> > Hence I will implement it as per your suggestion.
> >
> >
> >> Also please can you briefly comment non-trivial functions?
> >> OK.
> >>
> >>> {
> >>> struct dwmci_host *host = NULL;
> >>> host = malloc(sizeof(struct dwmci_host));
> >>> @@ -44,14 +59,104 @@ int exynos_dwmci_init(u32 regbase, int bus_width,
> >> int
> >>> index)
> >>> return 1;
> >>> }
> >>>
> >>> + /* set the clock divisor - clk_div_fsys for mmc */
> >>> + if (exynos5_mmc_set_clk_div(index)) {
> >>> + debug("mmc clock div set failed\n");
> >>> + return -1;
> >>> + }
> >>> +
> >>> host->name = EXYNOS_NAME;
> >>> host->ioaddr = (void *)regbase;
> >>> host->buswidth = bus_width;
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> + host->clksel_val = (DWMCI_SET_SAMPLE_CLK(timing[0]) |
> >>> + DWMCI_SET_DRV_CLK(timing[1]) |
> >>> + DWMCI_SET_DIV_RATIO(timing[2]));
> >>> +#else
> >>> + if (0 == index)
> >>> + host->clksel_val = DWMMC_MMC0_CLKSEL_VAL;
> >>> + if (2 == index)
> >>> + host->clksel_val = DWMMC_MMC2_CLKSEL_VAL;
> >>> +#endif
> >>> host->clksel = exynos_dwmci_clksel;
> >>> host->dev_index = index;
> >>> + host->mmc_clk = exynos_dwmci_get_clk;
> >>>
> >>> - add_dwmci(host, 52000000, 400000);
> >>> + add_dwmci(host, DWMMC_MAX_FREQ, DWMMC_MIN_FREQ);
> >>>
> >>> return 0;
> >>> }
> >>>
> >>> +#ifdef CONFIG_OF_CONTROL
> >>> +unsigned int exynos_dwmmc_init(const void *blob)
> >>> +{
> >>> + u32 base;
> >>> + int index, bus_width;
> >>> + int node_list[DWMMC_MAX_CH_NUM];
> >>> + int err = 0;
> >>> + int dev_id, flag;
> >>> + u32 timing[3];
> >>> + int count, i;
> >>> +
> >>> + count = fdtdec_find_aliases_for_id(blob, "dwmmc",
> >>> + COMPAT_SAMSUNG_EXYNOS5_DWMMC,
> node_list,
> >>> + DWMMC_MAX_CH_NUM);
> >>> +
> >>> + for (i = 0; i < count; i++) {
> >>> + int node = node_list[i];
> >>> +
> >>> + if (node <= 0)
> >>> + continue;
> >>> +
> >>> + /* config pinmux for each mmc channel */
> >>> + dev_id = pinmux_decode_periph_id(blob, node);
> >>> + if (dev_id == PERIPH_ID_SDMMC0)
> >>> + flag = PINMUX_FLAG_8BIT_MODE;
> >>> + else
> >>> + flag = PINMUX_FLAG_NONE;
> >>>
> >>
> >> This should be decoded from the FDT - e.g. fdtdec_get_int(blob, node,
> >> "samsung,width") == 8.
> >>
> >> OK.
> >>
> >
> >
> >>> +
> >>> + err = exynos_pinmux_config(dev_id, flag);
> >>> + if (err) {
> >>> + debug("DWMMC not configured\n");
> >>> + return err;
> >>> + }
> >>> +
> >>> + /* Get the base address from the device node */
> >>> + base = fdtdec_get_addr(blob, node, "reg");
> >>> + if (!base) {
> >>> + debug("DWMMC: Can't get base address\n");
> >>> + return -1;
> >>> + }
> >>> +
> >>> + /* Get the channel index from the device node */
> >>> + index = fdtdec_get_int(blob, node, "index", 0);
> >>> + if (index < 0) {
> >>> + debug("DWMMC: Can't get channel index\n");
> >>> + return -1;
> >>> + }
> >>>
> >>
> >> What is this needed for? Doesn't the reg address tell you everything you
> >> need here?
> >> Ok will change accordingly.
> >>
> >>> +
> >>> + /* Get the bus width from the device node */
> >>> + bus_width = fdtdec_get_int(blob, node, "bus-width", 0);
> >>> + if (bus_width < 0) {
> >>> + debug("DWMMC: Can't get bus-width\n");
> >>> + return -1;
> >>> + }
> >>
> >> +
> >>> + err = fdtdec_get_int_array(blob, node, "timing",
> >>> + timing, 3);
> >>> + if (err) {
> >>> + debug("Can't get sdr-timings for divider\n");
> >>> + return -1;
> >>> + }
> >>> +
> >>> + err = exynos_dwmci_init(base, bus_width,
> >>> + index, timing);
> >>> + if (err) {
> >>> + debug("Can't do dwmci init\n");
> >>> + return -1;
> >>> + }
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +#endif
> >>> diff --git a/include/dwmmc.h b/include/dwmmc.h
> >>> index c8b1d40..4a42849 100644
> >>> --- a/include/dwmmc.h
> >>> +++ b/include/dwmmc.h
> >>> @@ -123,6 +123,9 @@
> >>> #define MSIZE(x) ((x) << 28)
> >>> #define RX_WMARK(x) ((x) << 16)
> >>> #define TX_WMARK(x) (x)
> >>> +#define RX_WMARK_SHIFT 16
> >>> +#define RX_WMARK_MASK (0xfff << RX_WMARK_SHIFT)
> >>> +
> >>>
> >>
> >> Extra blank line here?
> >> Will remove it.
> >>
> >>>
> >>> #define DWMCI_IDMAC_OWN (1 << 31)
> >>> #define DWMCI_IDMAC_CH (1 << 4)
> >>> @@ -144,6 +147,7 @@ struct dwmci_host {
> >>> unsigned int bus_hz;
> >>> int dev_index;
> >>> int buswidth;
> >>> + u32 clksel_val;
> >>> u32 fifoth_val;
> >>> struct mmc *mmc;
> >>>
> >>> --
> >>> 1.7.0.4
> >>>
> >>>
> >> 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
> >
>
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
