From patchwork Mon Oct 8 21:08:32 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Troy Kisky X-Patchwork-Id: 190124 X-Patchwork-Delegate: sbabic@denx.de Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from theia.denx.de (theia.denx.de [85.214.87.163]) by ozlabs.org (Postfix) with ESMTP id 383E32C0168 for ; Tue, 9 Oct 2012 08:11:03 +1100 (EST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 58105280D6; Mon, 8 Oct 2012 23:11:01 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8Mruzck3-IIr; Mon, 8 Oct 2012 23:11:01 +0200 (CEST) Received: from theia.denx.de (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id BC7D3280DC; Mon, 8 Oct 2012 23:10:34 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by theia.denx.de (Postfix) with ESMTP id 93C02280DC for ; Mon, 8 Oct 2012 23:10:05 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at theia.denx.de Received: from theia.denx.de ([127.0.0.1]) by localhost (theia.denx.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id L6navmRE8Hjf for ; Mon, 8 Oct 2012 23:08:40 +0200 (CEST) X-policyd-weight: NOT_IN_SBL_XBL_SPAMHAUS=-1.5 NOT_IN_SPAMCOP=-1.5 NOT_IN_BL_NJABL=-1.5 (only DNSBL check requested) Received: from mail-pa0-f44.google.com (mail-pa0-f44.google.com [209.85.220.44]) by theia.denx.de (Postfix) with ESMTPS id A1522280D6 for ; Mon, 8 Oct 2012 23:08:35 +0200 (CEST) Received: by mail-pa0-f44.google.com with SMTP id fb11so4475967pad.3 for ; Mon, 08 Oct 2012 14:08:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding :x-gm-message-state; bh=BfykB9QGZVdGPVb3wgQpNKqLoko4VkqUXvDCTbFgc4I=; b=kBM/hZwH4geJmlZ6N2ZyOfuWxlfWQCb+COE5C2MmwgFpf0vcw10vK/TKhWeK/6tEV0 TCQVED90WRj7V0by233Lnfn4LykWWFAJ+6YaJfAne/1ztQuG/YJk99Pa9nDbhX5oeeiC SchlS2jrYoOeamCdfmVLo6h17FofP3dhUoJ0rvohL3ZljXrZtexL9meh/Hk7/ige00ER U2gH5PWn3hk6+gKokW4m9Vjyvncv59dxKoVNZckZfF3LLopWKVfuTCkXYXkfI2JSM+Mh clNiblbO/780ecXcEZsZZ7FMqON0/8CfvN/4OSzpyLlEBROrJ2Wv5selSukiZzu2M6h5 j85w== Received: by 10.68.239.198 with SMTP id vu6mr15719014pbc.109.1349730513030; Mon, 08 Oct 2012 14:08:33 -0700 (PDT) Received: from [29.6.1.4] ([70.96.116.236]) by mx.google.com with ESMTPS id io1sm11222420pbc.43.2012.10.08.14.08.30 (version=SSLv3 cipher=OTHER); Mon, 08 Oct 2012 14:08:31 -0700 (PDT) Message-ID: <507340D0.70909@boundarydevices.com> Date: Mon, 08 Oct 2012 14:08:32 -0700 From: Troy Kisky User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120907 Thunderbird/15.0.1 MIME-Version: 1.0 To: Eric Nelson References: <1348281558-19520-1-git-send-email-troy.kisky@boundarydevices.com> <1349315254-21151-1-git-send-email-troy.kisky@boundarydevices.com> <1349315254-21151-29-git-send-email-troy.kisky@boundarydevices.com> <50731F71.6020805@boundarydevices.com> In-Reply-To: <50731F71.6020805@boundarydevices.com> X-Gm-Message-State: ALoCoQn0N0J0GrR1xka/1t/L3IqoS+YdcJCr1c6LsFIFE/CQcw7UctcN3e4Tqnqc5V+QHCC1ehx3 Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH V3 28/32] mx6q_4x_mt41j128.cfg: add mx6 solo/duallite support X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.11 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: u-boot-bounces@lists.denx.de Errors-To: u-boot-bounces@lists.denx.de On 10/8/2012 11:46 AM, Eric Nelson wrote: > Hi Troy, > > This seems to be the patch where the rubber meets the road in > much of this series. > > On 10/03/2012 06:47 PM, Troy Kisky wrote: >> Signed-off-by: Troy Kisky >> --- >> board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg | 120 >> +++++++++++++++++++------- >> 1 file changed, 87 insertions(+), 33 deletions(-) >> >> diff --git a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg >> b/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg >> index 9e20db0..f45f93e 100644 >> --- a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg >> +++ b/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg >> @@ -49,6 +49,15 @@ BOOT_FROM sd >> * Address absolute address of the register >> * value value to be stored in the register >> */ >> +/* >> + * DDR3 settings >> + * MX6Q ddr is limited to 1066 Mhz, currently 1056 MHz(528 MHz >> clock), >> + * memory bus width: 64 bits, x16/x32/x64 >> + * MX6DL ddr is limited to 800 MHz(400 MHz clock) >> + * memory bus width: 64 bits, x16/x32/x64 >> + * MX6SOLO ddr is limited to 800 MHz(400 MHz clock) >> + * memory bus width: 32 bits, x16/x32 >> + */ > > This comment seems to be critical to understanding some of > what's below, since now three **different** memory configurations > are represented in this file. > > At a minimum, the file name should be changed to get rid of > '6q' and '4x' since those don't necessarily apply. Right, my preference is to put at back in board/freescale/mx6qsabrelite/imximage.cfg since MPDGCTRL0/1 DQS GATE registers and MMDC_MPRDDQBYnDL should be very board specific, but that may be meet by objections from Fabio. > >> WRITE_ENTRY1(IOM_DRAM_SDQS0, 0x00000030) >> WRITE_ENTRY1(IOM_DRAM_SDQS1, 0x00000030) >> WRITE_ENTRY1(IOM_DRAM_SDQS2, 0x00000030) >> @@ -90,6 +99,7 @@ WRITE_ENTRY1(IOM_GRP_B6DS, 0x00000030) >> WRITE_ENTRY1(IOM_GRP_B7DS, 0x00000030) >> >> WRITE_ENTRY1(IOM_GRP_ADDDS, 0x00000030) >> + >> /* (differential input) */ >> WRITE_ENTRY1(IOM_DDRMODE_CTL, 0x00020000) >> /* disable ddr pullups */ >> @@ -119,48 +129,92 @@ WRITE_ENTRY1(MMDC_P0_MDMISC, 0x00081740) >> * MDSCR, con_req >> */ >> WRITE_ENTRY1(MMDC_P0_MDSCR, 0x00008000) >> + >> /* >> - * MDCFG0, tRFC=0x56 clocks, tXS=0x5b clocks >> - * tXP=4 clocks, tXPDLL=13 clocks >> - * tFAW=24 clocks, cas=8 cycles >> + * MDCFG0, >> + * MX6Q: >> + * tRFC=0x56 clocks, tXS=0x5b clocks, tXP=4 clocks, tXPDLL=13 clocks >> + * tFAW=24 clocks, cas=8 cycles >> + * MX6DL/SOLO: >> + * tRFC=0x6a clocks, tXS=0x6e clocks, tXP=3 clocks, tXPDLL=10 clocks >> + * tFAW=19 clocks, cas=6 cycles >> */ >> -WRITE_ENTRY1(MMDC_P0_MDCFG0, 0x555A7975) >> +WRITE_ENTRY2(MMDC_P0_MDCFG0, 0x555A7975, 0x696D5323) >> + > > Here's where I start to get lost in the macro-fu. > > The WRITE_ENTRY1 macros make some sense to me in that they > always write a single value to an offset whose address > changes based on the processor type. > > In order to understand WRITE_ENTRY2, you really have to > know that the dual, solo, and sololite share many > characteristics. RIght, ENTRY2 is used when there are differences between mx6q, and mx6dl/solo. In the case above, the value 0x555A7975 is written if a mx6quad, and the value 0x696D5323 is written if a mx6duallite or mx6solo. > > It's kinda hard to infer that knowledge from the code > and I wonder if this structure will hold up under future > revisions. > >> /* >> - * MDCFG1, tRDC=8, tRP=8, tRC=27,tRAS=20,tRPA=tRP+1,tWR=8 >> - * tMRD=4, tCWL=6 >> + * MDCFG1, >> + * MX6Q: >> + * tRDC=8, tRP=8, tRC=27, tRAS=20, tRPA=tRP+1, tWR=8, tMRD=4, tCWL=6 >> + * MX6DL/SOLO: >> + * tRDC=6, tRP=6, tRC=20, tRAS=15, tRPA=tRP+1, tWR=7, tMRD=4, tCWL=5 >> */ >> -WRITE_ENTRY1(MMDC_P0_MDCFG1, 0xFF538E64) >> +WRITE_ENTRY2(MMDC_P0_MDCFG1, 0xFF538E64, 0xB66E8C63) >> + >> /* >> * MDCFG2,tDLLK=512,tRTP=4,tWTR=4,tRRD=4 >> */ >> WRITE_ENTRY1(MMDC_P0_MDCFG2, 0x01FF00DB) >> WRITE_ENTRY1(MMDC_P0_MDRWD, 0x000026D2) >> - >> WRITE_ENTRY1(MMDC_P0_MDOR, 0x005B0E21) >> -WRITE_ENTRY1(MMDC_P0_MDOTC, 0x09444040) >> -WRITE_ENTRY1(MMDC_P0_MDPDC, 0x00025576) >> >> /* >> - * Mx6Q - 64 bit wide ddr >> + * MMDC_MDOTC, >> + * MX6Q: >> + * tAOFPD=2 cycles, tAONPD=2, tANPD=5, tAXPD=5, tODTLon=5, >> tODT_idle_off=5 >> + * MX6DL/SOLO: >> + * tAOFPD=1 cycles, tAONPD=1, tANPD=4, tAXPD=4, tODTLon=4, >> tODT_idle_off=4 >> + */ >> +WRITE_ENTRY2(MMDC_P0_MDOTC, 0x09444040, 0x00333030) >> + >> +/* >> + * MDPDC - [17:16](2) => CKE pulse width = 3 cycles. >> + * [15:12](5) => PWDT_1 = 256 cycles >> + * [11:8](5) =>PWDR_0 = 256 cycles >> + * MX6Q: [2:0](6) => CKSRE = 6 cycles, [5:3](6) => CKSRX = 6 >> cycles >> + * MX6DL/SOLO: [2:0](5) => CKSRE = 5 cycles, [5:3](5) => CKSRX = 5 >> cycles >> + */ >> +WRITE_ENTRY2(MMDC_P0_MDPDC, 0x00025576, 0x0002556D) >> + >> +/* >> + * MX6Q/DL - 64 bit wide ddr >> * last address is (1<<28 (base) + 1<<30 - 1) / (1<<25) = >> * 1<<3 + 1<<5 - 1 = 8 + 0x20 -1 = 0x27 >> */ >> +/* >> + * MX6SOLO - 32 bit wide ddr >> + * last address is (1<<28 (base) + 1<<29 - 1) / (1<<25) = >> + * 1<<3 + 1<<4 - 1 = 8 + 0x10 -1 = 0x17 >> + */ >> /* MDASP, CS0_END */ >> -WRITE_ENTRY1(MMDC_P0_MDASP, 0x00000027) >> +WRITE_ENTRY3(MMDC_P0_MDASP, 0x00000027, 0x00000027, 0x00000017) > > Is it unreasonable to think there's a use case for 32-bit wide > memory on a dual or quad? Sure, they just would not use this file. > > What happens when we populate 256Mx16 DDR chips? > > Sabre Auto is already doing this. Again, definitely a new file would be needed. > >> /* >> - * MDCTL, CS0 enable, CS1 disabled, row=14, col=10, burst=8, >> width=64/32bit >> - * mx6q : row+col+bank+width=14+10+3+3=30 = 1G >> + * MDCTL, CS0 enable, CS1 disabled, row=14, col=10, burst=8 >> + * MX6Q/DL: width=64bit row+col+bank+width=14+10+3+3=30 = 1G >> + * MX6SOLO: width=32bit row+col+bank+width=14+10+3+2=29 = 512M >> */ >> -WRITE_ENTRY1(MMDC_P0_MDCTL, 0x831A0000) >> +WRITE_ENTRY3(MMDC_P0_MDCTL, 0x831A0000, 0x831A0000, 0x83190000) >> >> -/* MDSCR, con_req, LOAD MR2, CS0, A3,A10 set (CAS Write=6), RZQ/2 */ >> -WRITE_ENTRY1(MMDC_P0_MDSCR, 0x04088032) >> +/* >> + * LOAD MR2: MDSCR, con_req, CS0, A10 set - RZQ/2 >> + * MX6Q: A3 set(CAS Write=6) >> + * MX6DL/SOLO: (CAS Write=5) >> + */ >> +WRITE_ENTRY2(MMDC_P0_MDSCR, 0x04088032, 0x04008032) >> /* LOAD MR3, CS0 */ >> WRITE_ENTRY1(MMDC_P0_MDSCR, 0x00008033) >> -/* LOAD MR1, CS0, A1,A6 set Rtt=RZQ/2, ODI=RZQ/7 */ >> -WRITE_ENTRY1(MMDC_P0_MDSCR, 0x00428031) >> -/* LOAD MR0, CS0, A6,A8,A11 set CAS=8, WR=8, DLL reset */ >> -WRITE_ENTRY1(MMDC_P0_MDSCR, 0x09408030) >> + >> +/* >> + * LOAD MR1, CS0 >> + * MX6Q: A6 set: Rtt=RZQ/2, A1 set: ODI=RZQ/7 >> + * MX6DL/SOLO: A2 set: Rtt=RZQ/4, ODI=RZQ/6 >> + */ >> +WRITE_ENTRY2(MMDC_P0_MDSCR, 0x00428031, 0x00048031) >> + >> +/* LOAD MR0, CS0 A8 set: DLL Reset >> + * MX6Q: A6 set: CAS=8 A11 set: WR=8 >> + * MX6DL/SOLO: A4 set: CAS=5, A9,A10 set: WR=7 >> + */ >> +WRITE_ENTRY2(MMDC_P0_MDSCR, 0x09408030, 0x07208030) >> >> /* ZQ calibrate, CS0 */ >> WRITE_ENTRY1(MMDC_P0_MDSCR, 0x04008040) >> @@ -173,18 +227,18 @@ WRITE_ENTRY1(MMDC_P0_MPODTCTRL, 0x00022227) >> WRITE_ENTRY1(MMDC_P1_MPODTCTRL, 0x00022227) >> >> /* MPDGCTRL0/1 DQS GATE*/ >> -WRITE_ENTRY1(MMDC_P0_MPDGCTRL0, 0x434B0350) >> -WRITE_ENTRY1(MMDC_P0_MPDGCTRL1, 0x034C0359) >> -WRITE_ENTRY1(MMDC_P1_MPDGCTRL0, 0x434B0350) >> -WRITE_ENTRY1(MMDC_P1_MPDGCTRL1, 0x03650348) >> -WRITE_ENTRY1(MMDC_P0_MPRDDLCTL, 0x4436383B) >> -WRITE_ENTRY1(MMDC_P1_MPRDDLCTL, 0x39393341) >> -WRITE_ENTRY1(MMDC_P0_MPWRDLCTL, 0x35373933) >> -WRITE_ENTRY1(MMDC_P1_MPWRDLCTL, 0x48254A36) >> -WRITE_ENTRY1(MMDC_P0_MPWLDECTRL0, 0x001F001F) >> -WRITE_ENTRY1(MMDC_P0_MPWLDECTRL1, 0x001F001F) >> -WRITE_ENTRY1(MMDC_P1_MPWLDECTRL0, 0x00440044) >> -WRITE_ENTRY1(MMDC_P1_MPWLDECTRL1, 0x00440044) >> +WRITE_ENTRY2(MMDC_P0_MPDGCTRL0, 0x434B0350, 0x42350231) >> +WRITE_ENTRY2(MMDC_P0_MPDGCTRL1, 0x034C0359, 0x021A0218) >> +WRITE_ENTRY2(MMDC_P1_MPDGCTRL0, 0x434B0350, 0x42350231) >> +WRITE_ENTRY2(MMDC_P1_MPDGCTRL1, 0x03650348, 0x021A0218) >> +WRITE_ENTRY2(MMDC_P0_MPRDDLCTL, 0x4436383B, 0x4B4B4E49) >> +WRITE_ENTRY2(MMDC_P1_MPRDDLCTL, 0x39393341, 0x4B4B4E49) >> +WRITE_ENTRY2(MMDC_P0_MPWRDLCTL, 0x35373933, 0x3F3F3035) >> +WRITE_ENTRY2(MMDC_P1_MPWRDLCTL, 0x48254A36, 0x3F3F3035) >> +WRITE_ENTRY2(MMDC_P0_MPWLDECTRL0, 0x001F001F, 0x0040003C) >> +WRITE_ENTRY2(MMDC_P0_MPWLDECTRL1, 0x001F001F, 0x0032003E) >> +WRITE_ENTRY2(MMDC_P1_MPWLDECTRL0, 0x00440044, 0x0040003C) >> +WRITE_ENTRY2(MMDC_P1_MPWLDECTRL1, 0x00440044, 0x0032003E) >> >> /* MPMUR0 - Complete calibration by forced measurement */ >> WRITE_ENTRY1(MMDC_P0_MPMUR0, 0x00000800) > > It appears that only the memory configuration registers use > WRITE_ENTRY2 or WRITE_ENTRY3 macros, and if so, I'd much > rather see three separate files expressing the values, i.e.: > > mx6q_4x_mt41j128.cfg > mx6dl_4x_mt41j128.cfg > mx6s_2x_mt41j128.cfg That is definitely a valid option. But you lose the common settings of the iomux registers. Cut-n-pasting the files isn't so bad I guess, but updates such as mx6q_4x_mt41j128.cfg: use ddr3 mode for reset Bits 19-18 of IOMUXC_IOMUXC_SW_PAD_CTL_PAD_DRAM_RESET should be 3 for DDR3 mode. The current value of 0 is reserved in TRM. Signed-off-by: Troy Kisky Would then need to be applied to all three files. > > I'm not certain the processor part of the name is the > right designator though. If I understand correctly, > the differences in values between 6q and 6dl are mostly > based on the memory speed. > > Is that right? Correct... > > It also seems valid that a board would want to use an > i.mx6quad with a lower memory bus speed. Especially if there is a layout problem. > > If the address differences are taken care of by the > preprocessor, it seems that a set like this would > be more appropriate (and processor independent): > > mx6_4x_mt41j128_1066.cfg > mx6_4x_mt41j128_800.cfg > mx6_2x_mt41j128_800.cfg Right, this is definitely a valid option. And if different boards calibrated their DDR to the same settings (DQS and byte delays) it would make more sense. But I still see this file as board specific and not so much memory type specific. > > By splitting up the files, we can still check for > differences between them using 'diff', and it will > be easier to extend the set. > > Note that meaningful diffs will require the use of > symbolic names rather than varying addresses for > registers. > > It's also conceivable that calibration on a given > layout will require per-board tweaks, but that's > not clear at the moment. > > You've clearly been through this in more detail than > I have. Please let me know your thoughts. > > Regards, > > > Eric > diff --git a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg b/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg index b859e2f..9c622c8 100644 --- a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg +++ b/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg @@ -72,7 +72,7 @@ WRITE_ENTRY1(IOM_DRAM_RAS, 0x00020030) WRITE_ENTRY1(IOM_DRAM_SDCLK_0, 0x00020030) WRITE_ENTRY1(IOM_DRAM_SDCLK_1, 0x00020030) -WRITE_ENTRY1(IOM_DRAM_RESET, 0x00020030) +WRITE_ENTRY1(IOM_DRAM_RESET, 0x000e0030) WRITE_ENTRY1(IOM_DRAM_SDCKE0, 0x00003000) WRITE_ENTRY1(IOM_DRAM_SDCKE1, 0x00003000) WRITE_ENTRY1(IOM_DRAM_SDBA2, 0x00000000)