Hi,

On 04/16/2015 11:09 AM, Ian Campbell wrote:
On Thu, 2015-04-16 at 09:27 +0200, Hans de Goede wrote:
Hi,

On 15-04-15 21:56, Ian Campbell wrote:
On Tue, 2015-04-14 at 18:06 +0200, Hans de Goede wrote:
From: Vishnu Patekar <vishnupatekar0...@gmail.com>

Based on Allwinner dram init code from the a33 bsp:
https://github.com/allwinner-zh/bootloader/blob/master/basic_loader/bsp/bsp_for_a33/init_dram/mctl_hal.c

Initial u-boot port by Vishnu Patekar, major cleanup / rewrite by
Hans de Goede.

Signed-off-by: Vishnu Patekar <vishnupatekar0...@gmail.com>
Signed-off-by: Hans de Goede <hdego...@redhat.com>

+       /* Set dram timing */
+       reg_val = (twtp << 24) | (tfaw << 16) | (trasmax << 8) | (tras << 0);
+       writel(reg_val, &mctl_ctl->dramtmg0);
+       reg_val = (txp << 16) | (trtp << 8) | (trc << 0);
+       writel(reg_val, &mctl_ctl->dramtmg1);
+       reg_val = (tcwl << 24) | (tcl << 16) | (trd2wr << 8) | (twr2rd << 0);
+       writel(reg_val, &mctl_ctl->dramtmg2);
+       reg_val = (tmrw << 16) | (tmrd << 12) | (tmod << 0);
+       writel(reg_val, &mctl_ctl->dramtmg3);
+       reg_val = (trcd << 24) | (tccd << 16) | (trrd << 8) | (trp << 0);
+       writel(reg_val, &mctl_ctl->dramtmg4);
+       reg_val = (tcksrx << 24) | (tcksre << 16) | (tckesr << 8) | (tcke << 0);
+       writel(reg_val, &mctl_ctl->dramtmg5);

There's a lot of magic numbers here (and in the following code),
although in this particular context (with the named var) unless they are
the same elsewhere I'm not sure #defines would improve things much, but
I think some of the other stuff likely would.

Assuming you have any idea what the bits are, I suppose that per usual
we don't really know because -ENODOC?

Right the problem here is -ENODOC, the magic values come from the allwinnner
code and in the cases where we do not have named variables (as we do in
the above blurb) we've no clue what we're doing really, so I think adding
defines there will only obfuscate things.

Can you give a short description of the cases where you believe that
adding defines would be a good idea ?

Anywhere where we know the meanings ;-) If that's nowhere then the magic
numbers are fine (which is what my final paragraph was trying to say,
but not very clearly).

Ok, so I've gone over the entire dram init code another time, and I've
not been able to find a single place where I can add a define with a
meaningful name to replace the magic values.

So no v2 for this patch, can I get an ack for v1 ?

Regards,

Hans
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to