From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Sun, 26 Apr 2015 20:31:15 +0200 Subject: [U-Boot] [PATCH 06/10] sunxi: Add a33 dram init code In-Reply-To: <1429175350.25195.43.camel@hellion.org.uk> References: <1429027621-19252-1-git-send-email-hdegoede@redhat.com> <1429027621-19252-6-git-send-email-hdegoede@redhat.com> <1429127780.5660.29.camel@hellion.org.uk> <552F6466.7040905@redhat.com> <1429175350.25195.43.camel@hellion.org.uk> Message-ID: <553D2EF3.2090609@redhat.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>> >>>> 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 >>>> Signed-off-by: Hans de Goede >>> >>>> + /* 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