[U-Boot] [linux-sunxi] Re: [PATCH v3 09/13] sunxi: DRAM: add Allwinner H5 support

Andre Przywara andre.przywara at arm.com
Mon Feb 6 11:12:58 CET 2017


Hi,

Chen-Yu, thanks for your comments.

On 03/02/17 16:36, Chen-Yu Tsai wrote:
> Hi,
> 
> On Fri, Feb 3, 2017 at 11:26 PM, Jagan Teki <jagan at openedev.com> wrote:
>> On Feb 1, 2017 2:37 AM, "Andre Przywara" <andre.przywara at arm.com> wrote:
>>
>> The DRAM controller in the Allwinner H5 SoC is again very similar to
>> the one in the H3 and A64.
>> Based on the existing socid parameter, add support for this controller
>> by reusing the bulk of the code and only deviating where needed.
>> These new bits set or cleared here and there have been mostly found by
>> looking at DRAM register dumps after using the H5 boot0 and comparing
>> them to what we set in the code. So for now it's mostly unclear what
>> those bits actually mean - hence the missing names and comments.
>> Also add the delay line parameters taken from the boot0 and libdram
>> disassembly.
>>
>>
>> Can you split this patch with delay line params as separate patch.
> 
> It looks like the delay lines are for the H5, merely taken from
> different sources. They are and should be part of the same patch
> adding support for DRAM on the H5.

Yeah, I think they really belong into this patch. I don't see how
separating them would help, short of creating bisectability problems.

>>
>> Register setup differences between H5 and H3 are courtesy of Jens Kuske.
>>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> Acked-by: Maxime Ripard <maxime.ripard at free-electrons.com>
>> ---
>>  arch/arm/include/asm/arch-sunxi/cpu.h |  1 +
>>  arch/arm/mach-sunxi/dram_sun8i_h3.c   | 97
>> +++++++++++++++++++++++++++++------
>>  2 files changed, 82 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-sunxi/cpu.h
>> b/arch/arm/include/asm/arch-sunxi/cpu.h
>> index 6f96a97..e8e670e 100644
>> --- a/arch/arm/include/asm/arch-sunxi/cpu.h
>> +++ b/arch/arm/include/asm/arch-sunxi/cpu.h
>> @@ -15,5 +15,6 @@
>>
>>  #define SOCID_A64      0x1689
>>  #define SOCID_H3       0x1680
>> +#define SOCID_H5       0x1718
>>
>>  #endif /* _SUNXI_CPU_H */
>> diff --git a/arch/arm/mach-sunxi/dram_sun8i_h3.c
>> b/arch/arm/mach-sunxi/dram_sun8i_h3.c
>> index 9f7cc7f..d681a9d 100644
>> --- a/arch/arm/mach-sunxi/dram_sun8i_h3.c
>> +++ b/arch/arm/mach-sunxi/dram_sun8i_h3.c
>> @@ -177,6 +177,34 @@ static void mctl_set_master_priority_a64(void)
>>         writel(0x81000004, &mctl_com->mdfs_bwlr[2]);
>>  }
>>
>> +static void mctl_set_master_priority_h5(void)
>> +{
>> +       struct sunxi_mctl_com_reg * const mctl_com =
>> +                       (struct sunxi_mctl_com_reg *)SUNXI_DRAM_COM_BASE;
>> +
>> +       /* enable bandwidth limit windows and set windows size 1us */
>> +       writel(399, &mctl_com->tmr);
>> +       writel((1 << 16), &mctl_com->bwcr);
>>
>>
>> I'm not fond of using direct numerical values make code unhealthy please use
>> macros with bitops. Note that this comment will apply rest of the code where
>> it applies.
> 
> I think you are being unreasonable. The commit message clearly states that
> the added code either comes from register dumps, disassembled blobs, or
> comparison of released code:
> 
> """
> These new bits set or cleared here and there have been mostly found by
> looking at DRAM register dumps after using the H5 boot0 and comparing
> them to what we set in the code. So for now it's mostly unclear what
> those bits actually mean - hence the missing names and comments.
> """
> 
> For this particular instance, see
> 
> https://github.com/BPI-SINOVOIP/BPI-M2U-bsp/blob/master/u-boot-sunxi/arch/arm/cpu/armv7/sun8iw11p1/dram/lib-dram/mctl_hal.c#L197
> 
> which gives the exact same comment, and no named macros. Adding a macro
> for it and calling it H5_DRAM_BW_UNKNOWN_MAGIC is not an improvement.
> Same goes for the next few lines.
> 
> Allwinner has _never_ released documents for the DRAM controllers or DRAM PHYs,
> and only sometimes releases code for DRAM init for some SoCs, sometimes with
> questionable licenses (or lack of), of which some don't match what is actually
> seen in provided blobs. Considering what the community has access to. This
> patch seems to be quite good.

I think we have some sort of rewrite of this code ahead of us anyway,
hopefully we can address some of these points then in a reasonable manner.
But until then I'd like to keep it at "399" instead of using
WINDOW_SIZE_1US or _guessing_ how this is computed from some frequency.

Cheers,
Andre.

>>
>> +
>> +       /* set cpu high priority */
>> +       writel(0x00000001, &mctl_com->mapr);
>> +
>> +       /* Port 2 is reserved per Allwinner's linux-3.10 source, yet
>> +        * they initialise it */
>> +       MBUS_CONF(   CPU, true, HIGHEST, 0,  300,  260,  150);
>> +       MBUS_CONF(   GPU, true, HIGHEST, 0,  600,  400,  200);
>> +       MBUS_CONF(UNUSED, true, HIGHEST, 0,  512,  256,   96);
>> +       MBUS_CONF(   DMA, true, HIGHEST, 0,  256,  128,   32);
>> +       MBUS_CONF(    VE, true, HIGHEST, 0, 1900, 1500, 1000);
>> +       MBUS_CONF(   CSI, true, HIGHEST, 0,  150,  120,  100);
>> +       MBUS_CONF(  NAND, true,    HIGH, 0,  256,  128,   64);
>> +       MBUS_CONF(    SS, true, HIGHEST, 0,  256,  128,   64);
>> +       MBUS_CONF(    TS, true, HIGHEST, 0,  256,  128,   64);
>> +       MBUS_CONF(    DI, true,    HIGH, 0, 1024,  256,   64);
>> +       MBUS_CONF(    DE, true, HIGHEST, 3, 3400, 2400, 1024);
>> +       MBUS_CONF(DE_CFD, true, HIGHEST, 0,  600,  400,  200);
>> +}
>> +
>>  static void mctl_set_master_priority(uint16_t socid)
>>  {
>>         switch (socid) {
>> @@ -186,6 +214,9 @@ static void mctl_set_master_priority(uint16_t socid)
>>         case SOCID_A64:
>>                 mctl_set_master_priority_a64();
>>                 return;
>> +       case SOCID_H5:
>> +               mctl_set_master_priority_h5();
>> +               return;
>>         }
>>  }
>>
>> @@ -256,7 +287,7 @@ static void mctl_set_timing_params(uint16_t socid,
>> struct dram_para *para)
>>
>>         /* set two rank timing */
>>         clrsetbits_le32(&mctl_ctl->dramtmg[8], (0xff << 8) | (0xff << 0),
>> -                       (0x66 << 8) | (0x10 << 0));
>> +                       ((socid == SOCID_H5 ? 0x33 : 0x66) << 8) | (0x10 <<
>> 0));
>>
>>         /* set PHY interface timing, write latency and read latency
>> configure */
>>         writel((0x2 << 24) | (t_rdata_en << 16) | (0x1 << 8) |
>> @@ -391,7 +422,7 @@ static void mctl_sys_init(uint16_t socid, struct
>> dram_para *para)
>>                                 CCM_DRAMCLK_CFG_DIV(1) |
>>                                 CCM_DRAMCLK_CFG_SRC_PLL11 |
>>                                 CCM_DRAMCLK_CFG_UPD);
>> -       } else if (socid == SOCID_H3) {
>> +       } else if (socid == SOCID_H3 || socid == SOCID_H5) {
>>                 clock_set_pll5(CONFIG_DRAM_CLK * 2 * 1000000, false);
>>                 clrsetbits_le32(&ccm->dram_clk_cfg,
>>                                 CCM_DRAMCLK_CFG_DIV_MASK |
>> @@ -410,7 +441,7 @@ static void mctl_sys_init(uint16_t socid, struct
>> dram_para *para)
>>         setbits_le32(&ccm->dram_clk_cfg, CCM_DRAMCLK_CFG_RST);
>>         udelay(10);
>>
>> -       writel(0xc00e, &mctl_ctl->clken);
>> +       writel(socid == SOCID_H5 ? 0x8000 : 0xc00e, &mctl_ctl->clken);
>>         udelay(500);
>>  }
>>
>> @@ -434,7 +465,10 @@ static int mctl_channel_init(uint16_t socid, struct
>> dram_para *para)
>>
>>         /* setting VTC, default disable all VT */
>>         clrbits_le32(&mctl_ctl->pgcr[0], (1 << 30) | 0x3f);
>> -       clrsetbits_le32(&mctl_ctl->pgcr[1], 1 << 24, 1 << 26);
>> +       if (socid == SOCID_H5)
>> +               setbits_le32(&mctl_ctl->pgcr[1], (1 << 24) | (1 << 26));
>> +       else
>> +               clrsetbits_le32(&mctl_ctl->pgcr[1], 1 << 24, 1 << 26);
>>
>>         /* increase DFI_PHY_UPD clock */
>>         writel(PROTECT_MAGIC, &mctl_com->protect);
>> @@ -444,15 +478,22 @@ static int mctl_channel_init(uint16_t socid, struct
>> dram_para *para)
>>         udelay(100);
>>
>>         /* set dramc odt */
>> -       for (i = 0; i < 4; i++)
>> -               clrsetbits_le32(&mctl_ctl->dx[i].gcr, (0x3 << 4) |
>> -                               (0x1 << 1) | (0x3 << 2) | (0x3 << 12) |
>> -                               (0x3 << 14),
>> -                               IS_ENABLED(CONFIG_DRAM_ODT_EN) ?
>> -                                       DX_GCR_ODT_DYNAMIC :
>> DX_GCR_ODT_OFF);
>> +       for (i = 0; i < 4; i++) {
>> +               u32 clearmask = (0x3 << 4) | (0x1 << 1) | (0x3 << 2) |
>> +                               (0x3 << 12) | (0x3 << 14);
>> +               u32 setmask = IS_ENABLED(CONFIG_DRAM_ODT_EN) ?
>> +                               DX_GCR_ODT_DYNAMIC : DX_GCR_ODT_OFF;
>> +
>> +               if (socid == SOCID_H5) {
>> +                       clearmask |= 0x2 << 8;
>> +                       setmask |= 0x4 << 8;
>> +               }
>> +               clrsetbits_le32(&mctl_ctl->dx[i].gcr, clearmask, setmask);
>> +       }
>>
>>         /* AC PDR should always ON */
>> -       setbits_le32(&mctl_ctl->aciocr, 0x1 << 1);
>> +       clrsetbits_le32(&mctl_ctl->aciocr, socid == SOCID_H5 ? (0x1 << 11) :
>> 0,
>> +                       0x1 << 1);
>>
>>         /* set DQS auto gating PD mode */
>>         setbits_le32(&mctl_ctl->pgcr[2], 0x3 << 6);
>> @@ -464,7 +505,7 @@ static int mctl_channel_init(uint16_t socid, struct
>> dram_para *para)
>>                 /* dphy & aphy phase select 270 degree */
>>                 clrsetbits_le32(&mctl_ctl->pgcr[2], (0x3 << 10) | (0x3 <<
>> 8),
>>                                 (0x1 << 10) | (0x2 << 8));
>> -       } else if (socid == SOCID_A64) {
>> +       } else if (socid == SOCID_A64 || socid == SOCID_H5) {
>>                 /* dphy & aphy phase select ? */
>>                 clrsetbits_le32(&mctl_ctl->pgcr[2], (0x3 << 10) | (0x3 <<
>> 8),
>>                                 (0x0 << 10) | (0x3 << 8));
>> @@ -488,11 +529,12 @@ static int mctl_channel_init(uint16_t socid, struct
>> dram_para *para)
>>
>>                 mctl_phy_init(PIR_PLLINIT | PIR_DCAL | PIR_PHYRST |
>>                               PIR_DRAMRST | PIR_DRAMINIT | PIR_QSGATE);
>> -       } else if (socid == SOCID_A64) {
>> +       } else if (socid == SOCID_A64 || socid == SOCID_H5) {
>>                 clrsetbits_le32(&mctl_ctl->zqcr, 0xffffff, CONFIG_DRAM_ZQ);
>>
>>                 mctl_phy_init(PIR_ZCAL | PIR_PLLINIT | PIR_DCAL | PIR_PHYRST
>> |
>>                               PIR_DRAMRST | PIR_DRAMINIT | PIR_QSGATE);
>> +               /* no PIR_QSGATE for H5 ???? */
>>         }
>>
>>         /* detect ranks and bus width */
>> @@ -533,7 +575,7 @@ static int mctl_channel_init(uint16_t socid, struct
>> dram_para *para)
>>         /* set PGCR3, CKE polarity */
>>         if (socid == SOCID_H3)
>>                 writel(0x00aa0060, &mctl_ctl->pgcr[3]);
>> -       else if (socid == SOCID_A64)
>> +       else if (socid == SOCID_A64 || socid == SOCID_H5)
>>                 writel(0xc0aa0060, &mctl_ctl->pgcr[3]);
>>
>>         /* power down zq calibration module for power save */
>> @@ -604,6 +646,22 @@ static void mctl_auto_detect_dram_size(struct dram_para
>> *para)
>>            3,  4,  0,  3,  4,  1,  4,  0,                       \
>>            1,  1,  0,  1, 13,  5,  4      }
>>
>> +#define SUN8I_H5_DX_READ_DELAYS                                        \
>> +       {{ 14, 15, 17, 17, 17, 17, 17, 18, 17,  3,  3 },        \
>> +        { 21, 21, 12, 22, 21, 21, 21, 21, 21,  3,  3 },        \
>> +        { 16, 19, 19, 17, 22, 22, 21, 22, 19,  3,  3 },        \
>> +        { 21, 21, 22, 22, 20, 21, 19, 19, 19,  3,  3 } }
>> +#define SUN8I_H5_DX_WRITE_DELAYS                               \
>> +       {{  1,  2,  3,  4,  3,  4,  4,  4,  6,  6,  6 },        \
>> +        {  6,  6,  6,  5,  5,  5,  5,  5,  6,  6,  6 },        \
>> +        {  0,  2,  4,  2,  6,  5,  5,  5,  6,  6,  6 },        \
>> +        {  3,  3,  3,  2,  2,  1,  1,  1,  4,  4,  4 } }
>> +#define SUN8I_H5_AC_DELAYS                                     \
>> +       {  0,  0,  5,  5,  0,  0,  0,  0,                       \
>> +          0,  0,  0,  0,  3,  3,  3,  3,                       \
>> +          3,  3,  3,  3,  3,  3,  3,  3,                       \
>> +          3,  3,  3,  3,  2,  0,  0      }
>> +
>>  unsigned long sunxi_dram_init(void)
>>  {
>>         struct sunxi_mctl_com_reg * const mctl_com =
>> @@ -625,6 +683,10 @@ unsigned long sunxi_dram_init(void)
>>                 .dx_read_delays  = SUN50I_A64_DX_READ_DELAYS,
>>                 .dx_write_delays = SUN50I_A64_DX_WRITE_DELAYS,
>>                 .ac_delays       = SUN50I_A64_AC_DELAYS,
>> +#elif defined(CONFIG_MACH_SUN50I_H5)
>> +               .dx_read_delays  = SUN8I_H5_DX_READ_DELAYS,
>> +               .dx_write_delays = SUN8I_H5_DX_WRITE_DELAYS,
>> +               .ac_delays       = SUN8I_H5_AC_DELAYS,
>>  #endif
>>         };
>>  /*
>> @@ -636,6 +698,8 @@ unsigned long sunxi_dram_init(void)
>>         uint16_t socid = SOCID_H3;
>>  #elif defined(CONFIG_MACH_SUN50I)
>>         uint16_t socid = SOCID_A64;
>> +#elif defined(CONFIG_MACH_SUN50I_H5)
>> +       uint16_t socid = SOCID_H5;
>>  #endif
>>
>>         mctl_sys_init(socid, &para);
>> @@ -652,8 +716,9 @@ unsigned long sunxi_dram_init(void)
>>         if (socid ==
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "linux-sunxi" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to linux-sunxi+unsubscribe at googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.


More information about the U-Boot mailing list