[U-Boot] [PATCH 1/2] KW: Move the memory register definitions into kirkwood.h

Marek Vasut
2012-06-29T10:51:53Z

Dear Gerlando Falauto,

> Dear Marek Vasut,
>
> On 06/29/2012 12:08 PM, Marek Vasut wrote:
> > Dear Gerlando Falauto,
> >
> >> Dear Maruk,
>
> sorry about my misspelling!
>
> >> I am currently trying to address the exact same issue (SDRAM size
> >> detection and fixup).
> >> My idea was however (as opposed to moving register definition as you
> >> did), to add a fixup function to dram.c, say:
> >>
> >> /*
> >>
> >> * kw_sdram_bs - writes SDRAM Bank size
> >> */
> >>
> >> void kw_sdram_bs_set(enum memory_bank bank, u32 size)
> >> {
> >>
> >> /* Read current register value */
> >> u32 reg = readl(KW_REG_CPUCS_WIN_SZ(bank));
> >>
> >> printf("Current value: %x\n", reg);
> >> /* Clear window size */
> >> reg&= ~KW_REG_CPUCS_WIN_SIZE(0xFF);
> >>
> >> /* Set new window size */
> >> reg |= KW_REG_CPUCS_WIN_SIZE((size - 1)>> 24);
> >>
> >> printf("Writing: %x\n", reg);
> >> writel(reg, KW_REG_CPUCS_WIN_SZ(bank));
> >>
> >> }
> >>
> >> which would then be called to fix the window size according to the total
> >> memory size as reported by get_ram_size().
> >
> > Read up at [1].
> >
> > http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/133991
>
> Did read up, not sure I understand.
> If I get it right, *this* thread and patchset follows the feedback from
> the above thread.
>
> What I am trying to say is: we have a function (available to all
> kirkwood boards) which reads the value of the window size register for a
> given memory bank and returns its size.
> Now we want to do the opposite, write the window size register according
> to the detected SDRAM size.
> Why should we make up our own (board-specific) register manipulation
> tweaks, as opposed to having a function do it for us in dram.c as its
> companion?
> Reasons behind it, are board-specific, so in your case it would turn into:
>
> +int board_early_init_f(void)
> +{
> + unsigned long size = get_ram_size(PHYS_SDRAM_1, PHYS_SDRAM_1_SIZE);
> +
> + /* 256MB module, adjust BAR register */
> + if (size == 256 * 1024 * 1024)
> + kw_sdram_bs_set(0, size);
>
> Which I find cleaner.
> Even the if part could be also removed.
> And would work as-is on any board needing it (ours, for instance).
>
> What do you think?

I'm all right with it. But Prafulla is the maintainer, so you better negotiate
with him :-)

> Thank you for your patience,
> Gerlando

Best regards,
Marek Vasut