Re: [RFC PATCH 2/3] mx6: ddr: Wait before issuing the first MRS cmd
On 4/5/22 11:09, Francesco Dolcini wrote: On Mon, Apr 04, 2022 at 09:56:50PM +0200, Marek Vasut wrote: On 4/4/22 16:53, Francesco Dolcini wrote: On Mon, Apr 04, 2022 at 03:39:35PM +0200, Marek Vasut wrote: --- a/arch/arm/mach-imx/mx6/ddr.c +++ b/arch/arm/mach-imx/mx6/ddr.c @@ -1526,6 +1526,8 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo *sysinfo, ((sysinfo->ncs == 2) ? 1 : 0) << 30; /* SDE_1 for CS1 */ /* Step 8: Write Mode Registers to Init DDR3 devices */ + mdelay(1); /* Wait before issuing the first MRS command + (tXPR / 500us CKE delay after reset deassertion) */ Should we infer this delay from tXPR instead ? I could just delay(tXPR + 500us) and do the exact worst case delay. However I wonder if it is worth doing it, the 1ms delay works in practice, it is big enough to be correct in any case, but small enough not to be a concern on the boot time. Please note that I do not know which timing is violated here (tXPR, the 500us after reset de-assertion or both of them). Can the tXPR ever be larger than 500us ? No, it can't. Max value for 8GB density is 360ns, min value 120ns for 1GB density (see JEDEC standard, but also mx6/ddr.c). Would be fine for you to improve the commit message and code comment to make this discussion we just had transparent, while keeping the 1ms delay? Yeah, 1ms is fine. If you do V2, add my: Reviewed-by: Marek Vasut
Re: [RFC PATCH 2/3] mx6: ddr: Wait before issuing the first MRS cmd
On Mon, Apr 04, 2022 at 09:56:50PM +0200, Marek Vasut wrote: > On 4/4/22 16:53, Francesco Dolcini wrote: > > On Mon, Apr 04, 2022 at 03:39:35PM +0200, Marek Vasut wrote: > > > > --- a/arch/arm/mach-imx/mx6/ddr.c > > > > +++ b/arch/arm/mach-imx/mx6/ddr.c > > > > @@ -1526,6 +1526,8 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo > > > > *sysinfo, > > > > ((sysinfo->ncs == 2) ? 1 : 0) << 30; /* SDE_1 > > > > for CS1 */ > > > > /* Step 8: Write Mode Registers to Init DDR3 devices */ > > > > + mdelay(1); /* Wait before issuing the first MRS command > > > > + (tXPR / 500us CKE delay after reset deassertion) > > > > */ > > > > > > Should we infer this delay from tXPR instead ? > > > > I could just delay(tXPR + 500us) and do the exact worst case delay. > > > > However I wonder if it is worth doing it, the 1ms delay works in > > practice, it is big enough to be correct in any case, but small enough > > not to be a concern on the boot time. > > > > Please note that I do not know which timing is violated here > > (tXPR, the 500us after reset de-assertion or both of them). > > Can the tXPR ever be larger than 500us ? No, it can't. Max value for 8GB density is 360ns, min value 120ns for 1GB density (see JEDEC standard, but also mx6/ddr.c). Would be fine for you to improve the commit message and code comment to make this discussion we just had transparent, while keeping the 1ms delay? Francesco
Re: [RFC PATCH 2/3] mx6: ddr: Wait before issuing the first MRS cmd
On 4/4/22 16:53, Francesco Dolcini wrote:
Hello Marek,
thanks for your review.
On Mon, Apr 04, 2022 at 03:39:35PM +0200, Marek Vasut wrote:
On 4/4/22 10:51, Francesco Dolcini wrote:
Wait 1ms before issuing the first MRS command to write DDR3 Mode
registers.
There is a requirement to wait minimum of Reset CKE Exit time, tXPR,
with tXPR = max(tXS, 5tCK) and to wait 500 useconds after reset is
de-asserted. It seems that for some reason this is not enforced by the
MMDC controller, despite MMDCx_MDOR RST_to_CKE and tXPR being correctly
configured.
Without this change we experienced random memory initialization failures
with about 2% boot failure rate on specific problematic boards, after
this change we were able to do more than 10.000 power-cycle without a
single failure.
Fixes: fe0f7f7842e1 ("mx6: add mmdc configuration for MX6Q/MX6DL")
Signed-off-by: Francesco Dolcini
---
arch/arm/mach-imx/mx6/ddr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/mach-imx/mx6/ddr.c b/arch/arm/mach-imx/mx6/ddr.c
index 08e2f0f130a6..7b3d620094c4 100644
--- a/arch/arm/mach-imx/mx6/ddr.c
+++ b/arch/arm/mach-imx/mx6/ddr.c
@@ -1526,6 +1526,8 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo *sysinfo,
((sysinfo->ncs == 2) ? 1 : 0) << 30; /* SDE_1 for CS1 */
/* Step 8: Write Mode Registers to Init DDR3 devices */
+ mdelay(1); /* Wait before issuing the first MRS command
+ (tXPR / 500us CKE delay after reset deassertion) */
Should we infer this delay from tXPR instead ?
I could just delay(tXPR + 500us) and do the exact worst case delay.
However I wonder if it is worth doing it, the 1ms delay works in
practice, it is big enough to be correct in any case, but small enough
not to be a concern on the boot time.
Please note that I do not know which timing is violated here
(tXPR, the 500us after reset de-assertion or both of them).
Can the tXPR ever be larger than 500us ?
Re: [RFC PATCH 2/3] mx6: ddr: Wait before issuing the first MRS cmd
Hello Marek,
thanks for your review.
On Mon, Apr 04, 2022 at 03:39:35PM +0200, Marek Vasut wrote:
> On 4/4/22 10:51, Francesco Dolcini wrote:
> > Wait 1ms before issuing the first MRS command to write DDR3 Mode
> > registers.
> >
> > There is a requirement to wait minimum of Reset CKE Exit time, tXPR,
> > with tXPR = max(tXS, 5tCK) and to wait 500 useconds after reset is
> > de-asserted. It seems that for some reason this is not enforced by the
> > MMDC controller, despite MMDCx_MDOR RST_to_CKE and tXPR being correctly
> > configured.
> >
> > Without this change we experienced random memory initialization failures
> > with about 2% boot failure rate on specific problematic boards, after
> > this change we were able to do more than 10.000 power-cycle without a
> > single failure.
> >
> > Fixes: fe0f7f7842e1 ("mx6: add mmdc configuration for MX6Q/MX6DL")
> > Signed-off-by: Francesco Dolcini
> > ---
> > arch/arm/mach-imx/mx6/ddr.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/arm/mach-imx/mx6/ddr.c b/arch/arm/mach-imx/mx6/ddr.c
> > index 08e2f0f130a6..7b3d620094c4 100644
> > --- a/arch/arm/mach-imx/mx6/ddr.c
> > +++ b/arch/arm/mach-imx/mx6/ddr.c
> > @@ -1526,6 +1526,8 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo
> > *sysinfo,
> > ((sysinfo->ncs == 2) ? 1 : 0) << 30; /* SDE_1 for CS1 */
> > /* Step 8: Write Mode Registers to Init DDR3 devices */
> > + mdelay(1); /* Wait before issuing the first MRS command
> > + (tXPR / 500us CKE delay after reset deassertion) */
>
> Should we infer this delay from tXPR instead ?
I could just delay(tXPR + 500us) and do the exact worst case delay.
However I wonder if it is worth doing it, the 1ms delay works in
practice, it is big enough to be correct in any case, but small enough
not to be a concern on the boot time.
Please note that I do not know which timing is violated here
(tXPR, the 500us after reset de-assertion or both of them).
Francesco
Re: [RFC PATCH 2/3] mx6: ddr: Wait before issuing the first MRS cmd
On 4/4/22 10:51, Francesco Dolcini wrote:
Wait 1ms before issuing the first MRS command to write DDR3 Mode
registers.
There is a requirement to wait minimum of Reset CKE Exit time, tXPR,
with tXPR = max(tXS, 5tCK) and to wait 500 useconds after reset is
de-asserted. It seems that for some reason this is not enforced by the
MMDC controller, despite MMDCx_MDOR RST_to_CKE and tXPR being correctly
configured.
Without this change we experienced random memory initialization failures
with about 2% boot failure rate on specific problematic boards, after
this change we were able to do more than 10.000 power-cycle without a
single failure.
Fixes: fe0f7f7842e1 ("mx6: add mmdc configuration for MX6Q/MX6DL")
Signed-off-by: Francesco Dolcini
---
arch/arm/mach-imx/mx6/ddr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/mach-imx/mx6/ddr.c b/arch/arm/mach-imx/mx6/ddr.c
index 08e2f0f130a6..7b3d620094c4 100644
--- a/arch/arm/mach-imx/mx6/ddr.c
+++ b/arch/arm/mach-imx/mx6/ddr.c
@@ -1526,6 +1526,8 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo *sysinfo,
((sysinfo->ncs == 2) ? 1 : 0) << 30; /* SDE_1 for CS1 */
/* Step 8: Write Mode Registers to Init DDR3 devices */
+ mdelay(1); /* Wait before issuing the first MRS command
+ (tXPR / 500us CKE delay after reset deassertion) */
Should we infer this delay from tXPR instead ?
