From: Andre Przywara <andre.przywara@arm.com>
To: patrick9876@free.fr
Cc: u-boot@lists.denx.de,
"linux-sunxi@lists.linux.dev" <linux-sunxi@lists.linux.dev>,
Jernej Skrabec <jernej.skrabec@gmail.com>
Subject: Re: [PATCH] sunxi: dram: h6: fix the unreliability related to the DDR3 sequence
Date: Thu, 1 Feb 2024 18:17:35 +0000 [thread overview]
Message-ID: <20240201181735.1b5408c9@donnerap.manchester.arm.com> (raw)
In-Reply-To: <20240122211531.4272-1-patrick9876@free.fr>
On Mon, 22 Jan 2024 22:15:30 +0100
patrick9876@free.fr wrote:
Hi Patrick,
> From: Patrick Lerda <patrick9876@free.fr>
>
> Indeed, the DDR3 has a non-zero probability to not be properly
> initialized. This could be the PLL that is not locked or anything else.
> When this happens and the code tests the correct board configuration,
> the proper board configuration is not set. The board could end with
> half the expected memory size, or u-boot could stall.
>
> This change adds a loop to execute the DDR3 sequence again until the
> stable state is reached.
>
> My H6 TX6 board was prone to this issue. Once fixed with this change,
> the same board can now handle 10000+ consecutive reboots properly.
That's certainly an interesting approach, though I feel like it papers
over something. So for instance if the PLL is not locked, we should rather
fix that than going the Windows way (retry, reboot, reinstall) ;-)
But indeed there are some reports about instability of the DRAM init,
reporting twice the size being a common issue.
For a start, can you try to apply this series?
https://patchwork.ozlabs.org/project/uboot/list/?series=378679
Also there is this patch:
https://lore.kernel.org/u-boot/20231001161336.31140-2-viraniac@gmail.com/
And while I am still a bit sceptical about this solution, this looks
better than just retrying to me.
I would be grateful if you could test these patches and report back.
Cheers,
Andre
> Fixes: ec9cdaaa13d ("sunxi: dram: h6: Improve DDR3 config detection")
> Signed-off-by: Patrick Lerda <patrick9876@free.fr>
> ---
> arch/arm/mach-sunxi/dram_sun50i_h6.c | 207 ++++++++++++++-------------
> 1 file changed, 111 insertions(+), 96 deletions(-)
>
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h6.c b/arch/arm/mach-sunxi/dram_sun50i_h6.c
> index 62bc2a0231..462adb1c9e 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_h6.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_h6.c
> @@ -420,116 +420,131 @@ static bool mctl_channel_init(struct dram_para *para)
> (struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
> struct sunxi_mctl_phy_reg * const mctl_phy =
> (struct sunxi_mctl_phy_reg *)SUNXI_DRAM_PHY0_BASE;
> - int i;
> + int i, j = 0;
> u32 val;
>
> - setbits_le32(&mctl_ctl->dfiupd[0], BIT(31) | BIT(30));
> - setbits_le32(&mctl_ctl->zqctl[0], BIT(31) | BIT(30));
> - writel(0x2f05, &mctl_ctl->sched[0]);
> - setbits_le32(&mctl_ctl->rfshctl3, BIT(0));
> - setbits_le32(&mctl_ctl->dfimisc, BIT(0));
> - setbits_le32(&mctl_ctl->unk_0x00c, BIT(8));
> - clrsetbits_le32(&mctl_phy->pgcr[1], 0x180, 0xc0);
> - /* TODO: non-LPDDR3 types */
> - clrsetbits_le32(&mctl_phy->pgcr[2], GENMASK(17, 0), ns_to_t(7800));
> - clrbits_le32(&mctl_phy->pgcr[6], BIT(0));
> - clrsetbits_le32(&mctl_phy->dxccr, 0xee0, 0x220);
> - /* TODO: VT compensation */
> - clrsetbits_le32(&mctl_phy->dsgcr, BIT(0), 0x440060);
> - clrbits_le32(&mctl_phy->vtcr[1], BIT(1));
> -
> - for (i = 0; i < 4; i++)
> - clrsetbits_le32(&mctl_phy->dx[i].gcr[0], 0xe00, 0x800);
> - for (i = 0; i < 4; i++)
> - clrsetbits_le32(&mctl_phy->dx[i].gcr[2], 0xffff, 0x5555);
> - for (i = 0; i < 4; i++)
> - clrsetbits_le32(&mctl_phy->dx[i].gcr[3], 0x3030, 0x1010);
> -
> - udelay(100);
> + do {
> + setbits_le32(&mctl_ctl->dfiupd[0], BIT(31) | BIT(30));
> + setbits_le32(&mctl_ctl->zqctl[0], BIT(31) | BIT(30));
> + writel(0x2f05, &mctl_ctl->sched[0]);
> + setbits_le32(&mctl_ctl->rfshctl3, BIT(0));
> + setbits_le32(&mctl_ctl->dfimisc, BIT(0));
> + setbits_le32(&mctl_ctl->unk_0x00c, BIT(8));
> + clrsetbits_le32(&mctl_phy->pgcr[1], 0x180, 0xc0);
> + /* TODO: non-LPDDR3 types */
> + clrsetbits_le32(&mctl_phy->pgcr[2], GENMASK(17, 0),
> + ns_to_t(7800));
> + clrbits_le32(&mctl_phy->pgcr[6], BIT(0));
> + clrsetbits_le32(&mctl_phy->dxccr, 0xee0, 0x220);
> + /* TODO: VT compensation */
> + clrsetbits_le32(&mctl_phy->dsgcr, BIT(0), 0x440060);
> + clrbits_le32(&mctl_phy->vtcr[1], BIT(1));
>
> - if (para->ranks == 2)
> - setbits_le32(&mctl_phy->dtcr[1], 0x30000);
> - else
> - clrsetbits_le32(&mctl_phy->dtcr[1], 0x30000, 0x10000);
> + for (i = 0; i < 4; i++)
> + clrsetbits_le32(&mctl_phy->dx[i].gcr[0], 0xe00, 0x800);
> + for (i = 0; i < 4; i++)
> + clrsetbits_le32(&mctl_phy->dx[i].gcr[2], 0xffff,
> + 0x5555);
> + for (i = 0; i < 4; i++)
> + clrsetbits_le32(&mctl_phy->dx[i].gcr[3], 0x3030,
> + 0x1010);
>
> - if (sunxi_dram_is_lpddr(para->type))
> - clrbits_le32(&mctl_phy->dtcr[1], BIT(1));
> - if (para->ranks == 2) {
> - writel(0x00010001, &mctl_phy->rankidr);
> - writel(0x20000, &mctl_phy->odtcr);
> - } else {
> - writel(0x0, &mctl_phy->rankidr);
> - writel(0x10000, &mctl_phy->odtcr);
> - }
> + udelay(100);
>
> - /* set bits [3:0] to 1? 0 not valid in ZynqMP d/s */
> - if (para->type == SUNXI_DRAM_TYPE_LPDDR3)
> - clrsetbits_le32(&mctl_phy->dtcr[0], 0xF0000000, 0x10000040);
> - else
> - clrsetbits_le32(&mctl_phy->dtcr[0], 0xF0000000, 0x10000000);
> - if (para->clk <= 792) {
> - if (para->clk <= 672) {
> - if (para->clk <= 600)
> - val = 0x300;
> - else
> - val = 0x400;
> + if (para->ranks == 2)
> + setbits_le32(&mctl_phy->dtcr[1], 0x30000);
> + else
> + clrsetbits_le32(&mctl_phy->dtcr[1], 0x30000, 0x10000);
> +
> + if (sunxi_dram_is_lpddr(para->type))
> + clrbits_le32(&mctl_phy->dtcr[1], BIT(1));
> + if (para->ranks == 2) {
> + writel(0x00010001, &mctl_phy->rankidr);
> + writel(0x20000, &mctl_phy->odtcr);
> } else {
> - val = 0x500;
> + writel(0x0, &mctl_phy->rankidr);
> + writel(0x10000, &mctl_phy->odtcr);
> }
> - } else {
> - val = 0x600;
> - }
> - /* FIXME: NOT REVIEWED YET */
> - clrsetbits_le32(&mctl_phy->zq[0].zqcr, 0x700, val);
> - clrsetbits_le32(&mctl_phy->zq[0].zqpr[0], 0xff,
> - CONFIG_DRAM_ZQ & 0xff);
> - clrbits_le32(&mctl_phy->zq[0].zqor[0], 0xfffff);
> - setbits_le32(&mctl_phy->zq[0].zqor[0], (CONFIG_DRAM_ZQ >> 8) & 0xff);
> - setbits_le32(&mctl_phy->zq[0].zqor[0], (CONFIG_DRAM_ZQ & 0xf00) - 0x100);
> - setbits_le32(&mctl_phy->zq[0].zqor[0], (CONFIG_DRAM_ZQ & 0xff00) << 4);
> - clrbits_le32(&mctl_phy->zq[1].zqpr[0], 0xfffff);
> - setbits_le32(&mctl_phy->zq[1].zqpr[0], (CONFIG_DRAM_ZQ >> 16) & 0xff);
> - setbits_le32(&mctl_phy->zq[1].zqpr[0], ((CONFIG_DRAM_ZQ >> 8) & 0xf00) - 0x100);
> - setbits_le32(&mctl_phy->zq[1].zqpr[0], (CONFIG_DRAM_ZQ & 0xff0000) >> 4);
> - if (para->type == SUNXI_DRAM_TYPE_LPDDR3) {
> - for (i = 1; i < 14; i++)
> - writel(0x06060606, &mctl_phy->acbdlr[i]);
> - }
>
> - val = PIR_ZCAL | PIR_DCAL | PIR_PHYRST | PIR_DRAMINIT | PIR_QSGATE |
> - PIR_RDDSKW | PIR_WRDSKW | PIR_RDEYE | PIR_WREYE;
> - if (para->type == SUNXI_DRAM_TYPE_DDR3)
> - val |= PIR_DRAMRST | PIR_WL;
> - mctl_phy_pir_init(val);
> + /* set bits [3:0] to 1? 0 not valid in ZynqMP d/s */
> + if (para->type == SUNXI_DRAM_TYPE_LPDDR3)
> + clrsetbits_le32(&mctl_phy->dtcr[0], 0xF0000000,
> + 0x10000040);
> + else
> + clrsetbits_le32(&mctl_phy->dtcr[0], 0xF0000000,
> + 0x10000000);
> + if (para->clk <= 792) {
> + if (para->clk <= 672) {
> + if (para->clk <= 600)
> + val = 0x300;
> + else
> + val = 0x400;
> + } else {
> + val = 0x500;
> + }
> + } else {
> + val = 0x600;
> + }
> + /* FIXME: NOT REVIEWED YET */
> + clrsetbits_le32(&mctl_phy->zq[0].zqcr, 0x700, val);
> + clrsetbits_le32(&mctl_phy->zq[0].zqpr[0], 0xff,
> + CONFIG_DRAM_ZQ & 0xff);
> + clrbits_le32(&mctl_phy->zq[0].zqor[0], 0xfffff);
> + setbits_le32(&mctl_phy->zq[0].zqor[0],
> + (CONFIG_DRAM_ZQ >> 8) & 0xff);
> + setbits_le32(&mctl_phy->zq[0].zqor[0],
> + (CONFIG_DRAM_ZQ & 0xf00) - 0x100);
> + setbits_le32(&mctl_phy->zq[0].zqor[0], (CONFIG_DRAM_ZQ & 0xff00)
> + << 4);
> + clrbits_le32(&mctl_phy->zq[1].zqpr[0], 0xfffff);
> + setbits_le32(&mctl_phy->zq[1].zqpr[0],
> + (CONFIG_DRAM_ZQ >> 16) & 0xff);
> + setbits_le32(&mctl_phy->zq[1].zqpr[0],
> + ((CONFIG_DRAM_ZQ >> 8) & 0xf00) - 0x100);
> + setbits_le32(&mctl_phy->zq[1].zqpr[0],
> + (CONFIG_DRAM_ZQ & 0xff0000) >> 4);
> + if (para->type == SUNXI_DRAM_TYPE_LPDDR3) {
> + for (i = 1; i < 14; i++)
> + writel(0x06060606, &mctl_phy->acbdlr[i]);
> + }
>
> - /* TODO: DDR4 types ? */
> - for (i = 0; i < 4; i++)
> - writel(0x00000909, &mctl_phy->dx[i].gcr[5]);
> + val = PIR_ZCAL | PIR_DCAL | PIR_PHYRST | PIR_DRAMINIT |
> + PIR_QSGATE | PIR_RDDSKW | PIR_WRDSKW | PIR_RDEYE |
> + PIR_WREYE;
> + if (para->type == SUNXI_DRAM_TYPE_DDR3)
> + val |= PIR_DRAMRST | PIR_WL;
> + mctl_phy_pir_init(val);
>
> - for (i = 0; i < 4; i++) {
> - if (IS_ENABLED(CONFIG_DRAM_ODT_EN))
> - val = 0x0;
> - else
> - val = 0xaaaa;
> - clrsetbits_le32(&mctl_phy->dx[i].gcr[2], 0xffff, val);
> + /* TODO: DDR4 types ? */
> + for (i = 0; i < 4; i++)
> + writel(0x00000909, &mctl_phy->dx[i].gcr[5]);
>
> - if (IS_ENABLED(CONFIG_DRAM_ODT_EN))
> - val = 0x0;
> - else
> - val = 0x2020;
> - clrsetbits_le32(&mctl_phy->dx[i].gcr[3], 0x3030, val);
> - }
> + for (i = 0; i < 4; i++) {
> + if (IS_ENABLED(CONFIG_DRAM_ODT_EN))
> + val = 0x0;
> + else
> + val = 0xaaaa;
> + clrsetbits_le32(&mctl_phy->dx[i].gcr[2], 0xffff, val);
>
> - mctl_bit_delay_set(para);
> - udelay(1);
> + if (IS_ENABLED(CONFIG_DRAM_ODT_EN))
> + val = 0x0;
> + else
> + val = 0x2020;
> + clrsetbits_le32(&mctl_phy->dx[i].gcr[3], 0x3030, val);
> + }
>
> - setbits_le32(&mctl_phy->pgcr[6], BIT(0));
> - clrbits_le32(&mctl_phy->pgcr[6], 0xfff8);
> - for (i = 0; i < 4; i++)
> - clrbits_le32(&mctl_phy->dx[i].gcr[3], ~0x3ffff);
> - udelay(10);
> + mctl_bit_delay_set(para);
> + udelay(1);
> +
> + setbits_le32(&mctl_phy->pgcr[6], BIT(0));
> + clrbits_le32(&mctl_phy->pgcr[6], 0xfff8);
> + for (i = 0; i < 4; i++)
> + clrbits_le32(&mctl_phy->dx[i].gcr[3], ~0x3ffff);
> + udelay(10);
> + val = readl(&mctl_phy->pgsr[0]) & 0xff00000;
> + } while (val && j++ < 64);
>
> - if (readl(&mctl_phy->pgsr[0]) & 0xff00000) {
> + if (val) {
> /* Oops! There's something wrong! */
> debug("PLL = %x\n", readl(0x3001010));
> debug("DRAM PHY PGSR0 = %x\n", readl(&mctl_phy->pgsr[0]));
parent reply other threads:[~2024-02-01 18:17 UTC|newest]
Thread overview: expand[flat|nested] mbox.gz Atom feed
[parent not found: <20240122211531.4272-1-patrick9876@free.fr>]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240201181735.1b5408c9@donnerap.manchester.arm.com \
--to=andre.przywara@arm.com \
--cc=jernej.skrabec@gmail.com \
--cc=linux-sunxi@lists.linux.dev \
--cc=patrick9876@free.fr \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox