From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Date: Thu, 16 Apr 2015 09:27:34 +0200 Subject: [U-Boot] [PATCH 06/10] sunxi: Add a33 dram init code In-Reply-To: <1429127780.5660.29.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> Message-ID: <552F6466.7040905@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 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 ? Regards, Hans