diff mbox series

[U-Boot,v1,3/4] ARM: vf610: ddrmc: fix initialization completion detection

Message ID 20181204101021.22817-4-stefan@agner.ch
State Accepted
Commit 52c2c97e7c5b3ba326bae53a7940e27878efd405
Delegated to: Stefano Babic
Headers show
Series ddr: vybrid: various fixes | expand

Commit Message

Stefan Agner Dec. 4, 2018, 10:10 a.m. UTC 
From: Stefan Agner <stefan.agner@toradex.com>

The CR80 register has multiple interrupt bits, the code is supposed
to check bit 8 but instead uses a logical and. In most cases this
probably did not affect real operations since at that stage typically
none of the other bits are set.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---

 arch/arm/include/asm/arch-vf610/imx-regs.h | 3 ++-
 arch/arm/mach-imx/ddrmc-vf610.c            | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Lukasz Majewski Dec. 4, 2018, 10:59 a.m. UTC | #1 
Hi Stefan,

> From: Stefan Agner <stefan.agner@toradex.com>
> 
> The CR80 register has multiple interrupt bits, the code is supposed
> to check bit 8 but instead uses a logical and. In most cases this
> probably did not affect real operations since at that stage typically
> none of the other bits are set.

It can exit the loop when any error bit is set (but anyway with broken
DDR initialization we hang latter anyway).

> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> ---
> 
>  arch/arm/include/asm/arch-vf610/imx-regs.h | 3 ++-
>  arch/arm/mach-imx/ddrmc-vf610.c            | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-vf610/imx-regs.h
> b/arch/arm/include/asm/arch-vf610/imx-regs.h index
> b7374bfb8f..f71fbf4e73 100644 ---
> a/arch/arm/include/asm/arch-vf610/imx-regs.h +++
> b/arch/arm/include/asm/arch-vf610/imx-regs.h @@ -200,7 +200,8 @@
>  #define DDRMC_CR78_Q_FULLNESS(v)			(((v) & 0x7)
> << 24) #define DDRMC_CR78_BUR_ON_FLY_BIT(v)
> ((v) & 0xf) #define DDRMC_CR79_CTLUPD_AREF(v)
> (((v) & 0x1) << 24) -#define
> DDRMC_CR82_INT_MASK				0x10000000
> +#define DDRMC_CR80_MC_INIT_COMPLETE			(1 << 8)
> +#define DDRMC_CR82_INT_MASK				(1 << 28)

When I was working on my code - the checkpatch.pl was complaining about
(1 << x) code. It was recommended to use BIT(x) instead.

> #define DDRMC_CR87_ODT_WR_MAPCS0(v)			((v) <<
> 24) #define DDRMC_CR87_ODT_RD_MAPCS0(v)			((v)
> << 16) #define DDRMC_CR88_TODTL_CMD(v)
> (((v) & 0x1f) << 16) diff --git a/arch/arm/mach-imx/ddrmc-vf610.c
> b/arch/arm/mach-imx/ddrmc-vf610.c index 3d7da1c25e..d121a53898 100644
> --- a/arch/arm/mach-imx/ddrmc-vf610.c +++
> b/arch/arm/mach-imx/ddrmc-vf610.c @@ -231,6 +231,7 @@ void
> ddrmc_ctrl_init_ddr3(struct ddr3_jedec_timings const *timings, /* all
> inits done, start the DDR controller */
> writel(DDRMC_CR00_DRAM_CLASS_DDR3 | DDRMC_CR00_START, &ddrmr->cr[0]); 
> -	while (!(readl(&ddrmr->cr[80]) && 0x100))
> +	while (!(readl(&ddrmr->cr[80]) &
> DDRMC_CR80_MC_INIT_COMPLETE)) udelay(10);
> +	writel(DDRMC_CR80_MC_INIT_COMPLETE, &ddrmr->cr[81]);
>  }




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Stefan Agner Dec. 4, 2018, 11:20 a.m. UTC | #2 
On 04.12.2018 11:59, Lukasz Majewski wrote:
> Hi Stefan,
> 
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> The CR80 register has multiple interrupt bits, the code is supposed
>> to check bit 8 but instead uses a logical and. In most cases this
>> probably did not affect real operations since at that stage typically
>> none of the other bits are set.
> 
> It can exit the loop when any error bit is set (but anyway with broken
> DDR initialization we hang latter anyway).
> 
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>> ---
>>
>>  arch/arm/include/asm/arch-vf610/imx-regs.h | 3 ++-
>>  arch/arm/mach-imx/ddrmc-vf610.c            | 3 ++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-vf610/imx-regs.h
>> b/arch/arm/include/asm/arch-vf610/imx-regs.h index
>> b7374bfb8f..f71fbf4e73 100644 ---
>> a/arch/arm/include/asm/arch-vf610/imx-regs.h +++
>> b/arch/arm/include/asm/arch-vf610/imx-regs.h @@ -200,7 +200,8 @@
>>  #define DDRMC_CR78_Q_FULLNESS(v)			(((v) & 0x7)
>> << 24) #define DDRMC_CR78_BUR_ON_FLY_BIT(v)
>> ((v) & 0xf) #define DDRMC_CR79_CTLUPD_AREF(v)
>> (((v) & 0x1) << 24) -#define
>> DDRMC_CR82_INT_MASK				0x10000000
>> +#define DDRMC_CR80_MC_INIT_COMPLETE			(1 << 8)
>> +#define DDRMC_CR82_INT_MASK				(1 << 28)
> 
> When I was working on my code - the checkpatch.pl was complaining about
> (1 << x) code. It was recommended to use BIT(x) instead.
> 

Yeah I noticed that too, but arch/arm/include/asm/arch-vf610/imx-regs.h
does use bit shifting so far. I rated consistency higher than the new
recommendation :-)

--
Stefan

>> #define DDRMC_CR87_ODT_WR_MAPCS0(v)			((v) <<
>> 24) #define DDRMC_CR87_ODT_RD_MAPCS0(v)			((v)
>> << 16) #define DDRMC_CR88_TODTL_CMD(v)
>> (((v) & 0x1f) << 16) diff --git a/arch/arm/mach-imx/ddrmc-vf610.c
>> b/arch/arm/mach-imx/ddrmc-vf610.c index 3d7da1c25e..d121a53898 100644
>> --- a/arch/arm/mach-imx/ddrmc-vf610.c +++
>> b/arch/arm/mach-imx/ddrmc-vf610.c @@ -231,6 +231,7 @@ void
>> ddrmc_ctrl_init_ddr3(struct ddr3_jedec_timings const *timings, /* all
>> inits done, start the DDR controller */
>> writel(DDRMC_CR00_DRAM_CLASS_DDR3 | DDRMC_CR00_START, &ddrmr->cr[0]);
>> -	while (!(readl(&ddrmr->cr[80]) && 0x100))
>> +	while (!(readl(&ddrmr->cr[80]) &
>> DDRMC_CR80_MC_INIT_COMPLETE)) udelay(10);
>> +	writel(DDRMC_CR80_MC_INIT_COMPLETE, &ddrmr->cr[81]);
>>  }
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Stefan Agner Dec. 14, 2018, 1:22 p.m. UTC | #3 
Lukasz, Stefano,

On 04.12.2018 12:20, Stefan Agner wrote:
> On 04.12.2018 11:59, Lukasz Majewski wrote:
>> Hi Stefan,
>>
>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>
>>> The CR80 register has multiple interrupt bits, the code is supposed
>>> to check bit 8 but instead uses a logical and. In most cases this
>>> probably did not affect real operations since at that stage typically
>>> none of the other bits are set.
>>
>> It can exit the loop when any error bit is set (but anyway with broken
>> DDR initialization we hang latter anyway).
>>
>>>
>>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>>> Acked-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>>> ---
>>>
>>>  arch/arm/include/asm/arch-vf610/imx-regs.h | 3 ++-
>>>  arch/arm/mach-imx/ddrmc-vf610.c            | 3 ++-
>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/arch-vf610/imx-regs.h
>>> b/arch/arm/include/asm/arch-vf610/imx-regs.h index
>>> b7374bfb8f..f71fbf4e73 100644 ---
>>> a/arch/arm/include/asm/arch-vf610/imx-regs.h +++
>>> b/arch/arm/include/asm/arch-vf610/imx-regs.h @@ -200,7 +200,8 @@
>>>  #define DDRMC_CR78_Q_FULLNESS(v)			(((v) & 0x7)
>>> << 24) #define DDRMC_CR78_BUR_ON_FLY_BIT(v)
>>> ((v) & 0xf) #define DDRMC_CR79_CTLUPD_AREF(v)
>>> (((v) & 0x1) << 24) -#define
>>> DDRMC_CR82_INT_MASK				0x10000000
>>> +#define DDRMC_CR80_MC_INIT_COMPLETE			(1 << 8)
>>> +#define DDRMC_CR82_INT_MASK				(1 << 28)
>>
>> When I was working on my code - the checkpatch.pl was complaining about
>> (1 << x) code. It was recommended to use BIT(x) instead.
>>
> 
> Yeah I noticed that too, but arch/arm/include/asm/arch-vf610/imx-regs.h
> does use bit shifting so far. I rated consistency higher than the new
> recommendation :-)

Do you want me to change the whole file to BIT here? I'd rather prefer
to keep it for now.

But I think we otherwise are ok with this patch set right? I guess
technically patch 4 would not be your realm Stefano, but maybe you can
also take it through your tree?

--
Stefan

> 
> --
> Stefan
> 
>>> #define DDRMC_CR87_ODT_WR_MAPCS0(v)			((v) <<
>>> 24) #define DDRMC_CR87_ODT_RD_MAPCS0(v)			((v)
>>> << 16) #define DDRMC_CR88_TODTL_CMD(v)
>>> (((v) & 0x1f) << 16) diff --git a/arch/arm/mach-imx/ddrmc-vf610.c
>>> b/arch/arm/mach-imx/ddrmc-vf610.c index 3d7da1c25e..d121a53898 100644
>>> --- a/arch/arm/mach-imx/ddrmc-vf610.c +++
>>> b/arch/arm/mach-imx/ddrmc-vf610.c @@ -231,6 +231,7 @@ void
>>> ddrmc_ctrl_init_ddr3(struct ddr3_jedec_timings const *timings, /* all
>>> inits done, start the DDR controller */
>>> writel(DDRMC_CR00_DRAM_CLASS_DDR3 | DDRMC_CR00_START, &ddrmr->cr[0]);
>>> -	while (!(readl(&ddrmr->cr[80]) && 0x100))
>>> +	while (!(readl(&ddrmr->cr[80]) &
>>> DDRMC_CR80_MC_INIT_COMPLETE)) udelay(10);
>>> +	writel(DDRMC_CR80_MC_INIT_COMPLETE, &ddrmr->cr[81]);
>>>  }
>>
>>
>>
>>
>> Best regards,
>>
>> Lukasz Majewski
>>
>> --
>>
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff mbox series

Patch

diff --git a/arch/arm/include/asm/arch-vf610/imx-regs.h b/arch/arm/include/asm/arch-vf610/imx-regs.h
index b7374bfb8f..f71fbf4e73 100644
--- a/arch/arm/include/asm/arch-vf610/imx-regs.h
+++ b/arch/arm/include/asm/arch-vf610/imx-regs.h
@@ -200,7 +200,8 @@ 
 #define DDRMC_CR78_Q_FULLNESS(v)			(((v) & 0x7) << 24)
 #define DDRMC_CR78_BUR_ON_FLY_BIT(v)			((v) & 0xf)
 #define DDRMC_CR79_CTLUPD_AREF(v)			(((v) & 0x1) << 24)
-#define DDRMC_CR82_INT_MASK				0x10000000
+#define DDRMC_CR80_MC_INIT_COMPLETE			(1 << 8)
+#define DDRMC_CR82_INT_MASK				(1 << 28)
 #define DDRMC_CR87_ODT_WR_MAPCS0(v)			((v) << 24)
 #define DDRMC_CR87_ODT_RD_MAPCS0(v)			((v) << 16)
 #define DDRMC_CR88_TODTL_CMD(v)				(((v) & 0x1f) << 16)
diff --git a/arch/arm/mach-imx/ddrmc-vf610.c b/arch/arm/mach-imx/ddrmc-vf610.c
index 3d7da1c25e..d121a53898 100644
--- a/arch/arm/mach-imx/ddrmc-vf610.c
+++ b/arch/arm/mach-imx/ddrmc-vf610.c
@@ -231,6 +231,7 @@  void ddrmc_ctrl_init_ddr3(struct ddr3_jedec_timings const *timings,
 	/* all inits done, start the DDR controller */
 	writel(DDRMC_CR00_DRAM_CLASS_DDR3 | DDRMC_CR00_START, &ddrmr->cr[0]);
 
-	while (!(readl(&ddrmr->cr[80]) && 0x100))
+	while (!(readl(&ddrmr->cr[80]) & DDRMC_CR80_MC_INIT_COMPLETE))
 		udelay(10);
+	writel(DDRMC_CR80_MC_INIT_COMPLETE, &ddrmr->cr[81]);
 }