[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, ¶);
>> @@ -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