Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

[Roytburd, Benjamin]

Hello,

Recently, when working with the AM65x_GP_EVM development board, I found that the U-boot source code (specifically the SPL), does not properly initialize the DDRSS ram drivers for the AM6548. There are missing register writes that are required from DDR to function with ECC on.

I validated this by comparing what U-boot source code does to Texas Instrument DDR test scripts (known as GEL scripts), and these GEL scripts could properly initialize DDR with ECC while U-boot could not, this is due to the missing register writes. The changes are shown here in this pull request: https://github.com/u-boot/u-boot/pull/289 (made just to check CI).

Should I submit a patch for this? I have not contributed to U-boot before and would like to know if such a change would be accepted, or even reviewed, as a submission.

Thanks,
Ben

Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On Thu, May 04, 2023 at 12:47:29PM +0000, Roytburd, Benjamin wrote:
o
> Hello,
> 
> Recently, when working with the AM65x_GP_EVM development board, I found that the U-boot source code (specifically the SPL), does not properly initialize the DDRSS ram drivers for the AM6548. There are missing register writes that are required from DDR to function with ECC on.
> 
> I validated this by comparing what U-boot source code does to Texas Instrument DDR test scripts (known as GEL scripts), and these GEL scripts could properly initialize DDR with ECC while U-boot could not, this is due to the missing register writes. The changes are shown here in this pull request: https://github.com/u-boot/u-boot/pull/289 (made just to check CI).
> 
> Should I submit a patch for this? I have not contributed to U-boot before and would like to know if such a change would be accepted, or even reviewed, as a submission.

Yes, please submit a patch for it and cc the people that the
scripts/get_maintainer.pl script suggests, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

[Nishanth Menon]

Benjamin,

On 21:59-20230504, Tom Rini wrote:
> On Thu, May 04, 2023 at 12:47:29PM +0000, Roytburd, Benjamin wrote:
> o
> > Hello,
> > 
> > Recently, when working with the AM65x_GP_EVM development board, I found that the U-boot source code (specifically the SPL), does not properly initialize the DDRSS ram drivers for the AM6548. There are missing register writes that are required from DDR to function with ECC on.
> > 
> > I validated this by comparing what U-boot source code does to Texas Instrument DDR test scripts (known as GEL scripts), and these GEL scripts could properly initialize DDR with ECC while U-boot could not, this is due to the missing register writes. The changes are shown here in this pull request: https://github.com/u-boot/u-boot/pull/289 (made just to check CI).
> > 
> > Should I submit a patch for this? I have not contributed to U-boot before and would like to know if such a change would be accepted, or even reviewed, as a submission.
> 
> Yes, please submit a patch for it and cc the people that the
> scripts/get_maintainer.pl script suggests, thanks!

While you follow the formal submission process, I happened to glance
at your patch and could'nt help but comment, because we have enabled
ECC in our platforms (off tree) - Time taken to do ECC priming is the
hardest topic.. memset (essentially what you did) is probably the most
easiest way to attempt, BUT, it is very very expensive to boot time.
That is not acceptable for real products. We have struggled with the
same with various options proposed.

Please note that when you do submit the patches, you want to keep
in mind that it is not just one board that the DDR driver supports
- there are lots of boards that support this + an ecosystem of
tools such as the DDR configuration tool that spits out the DDR
configuration.

All of these should be factored into your patches when you post.
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

[Roytburd, Benjamin]

Nishanth,

Agreed, this is very expensive for boot time, I could probably add a comment to address this. But I believe it is better to have this option than not, as I struggled to figure out that I even needed to prime ECC when I enabled it.

What is the strategy for TODOs with U-boot? DMA would probably be the best option here instead of memset / for loop, but I do not have that implemented yet. I could submit it in a future patch.

Thanks,
Ben

________________________________
From: Nishanth Menon <nm@ti.com>
Sent: Friday, May 5, 2023, 8:51 AM
To: Tom Rini <trini@konsulko.com>
Cc: Roytburd, Benjamin <roytburd@msu.edu>; u-boot@lists.denx.de <u-boot@lists.denx.de>
Subject: Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

Benjamin,

On 21:59-20230504, Tom Rini wrote:
> On Thu, May 04, 2023 at 12:47:29PM +0000, Roytburd, Benjamin wrote:
> o
> > Hello,
> >
> > Recently, when working with the AM65x_GP_EVM development board, I found that the U-boot source code (specifically the SPL), does not properly initialize the DDRSS ram drivers for the AM6548. There are missing register writes that are required from DDR to function with ECC on.
> >
> > I validated this by comparing what U-boot source code does to Texas Instrument DDR test scripts (known as GEL scripts), and these GEL scripts could properly initialize DDR with ECC while U-boot could not, this is due to the missing register writes. The changes are shown here in this pull request: https://urldefense.com/v3/__https://github.com/u-boot/u-boot/pull/289__;!!HXCxUKc!1GAn_RygP2YfrkD6PwS6XTNm0d0yft7cgoHX9LO-jxrIJcpcUjzuaMlV38MkwMU22dIsmw$  (made just to check CI).
> >
> > Should I submit a patch for this? I have not contributed to U-boot before and would like to know if such a change would be accepted, or even reviewed, as a submission.
>
> Yes, please submit a patch for it and cc the people that the
> scripts/get_maintainer.pl script suggests, thanks!

While you follow the formal submission process, I happened to glance
at your patch and could'nt help but comment, because we have enabled
ECC in our platforms (off tree) - Time taken to do ECC priming is the
hardest topic.. memset (essentially what you did) is probably the most
easiest way to attempt, BUT, it is very very expensive to boot time.
That is not acceptable for real products. We have struggled with the
same with various options proposed.

Please note that when you do submit the patches, you want to keep
in mind that it is not just one board that the DDR driver supports
- there are lots of boards that support this + an ecosystem of
tools such as the DDR configuration tool that spits out the DDR
configuration.

All of these should be factored into your patches when you post.
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

[Nishanth Menon]

On 14:23-20230505, Roytburd, Benjamin wrote:
> Nishanth,
> 

Gentle reminder: Please do not top post  - email etiquette in upstream mailing
as well as please do not use flowed formatting. See [1] (I use neomutt
personally with 70 char line break)

> Agreed, this is very expensive for boot time, I could probably add a comment
> to address this. But I believe it is better to have this option than not, as
> I struggled to figure out that I even needed to prime ECC when I enabled it.
> 
> What is the strategy for TODOs with U-boot? DMA would probably be the best
> option here instead of memset / for loop, but I do not have that implemented
> yet. I could submit it in a future patch.

Remember what I mentioned in my response: there are quite a few other
folks also using the SoC support and evm. They would rather not want
to see the increase in boot time if we merge such a change. I don't see
a reasonable alternative without using DMA to prime effectively.

Also, is blindly enabling ECC across the DDR effective or creating
other problems? Access latencies are increased (since incoming and
outgoing bursts need to be checked against checksum) or should we
scheme something with a range?

[...]

[1] https://www.kernel.org/doc/html/v6.3/process/email-clients.html
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

[Roytburd, Benjamin]

Enabling ECC has not given me any problems in running my applications, I have even performed poison tests with SECDED that have been effective. No problems besides the increased boot time due to priming are occurring.

But agreed, for a patch we should definitely use DMA as opposed to a for loop. I have not implemented DMA yet, but when I do I wil formally submit a patch with it.
________________________________
From: Nishanth Menon <nm@ti.com>
Sent: Wednesday, May 17, 2023 11:30 AM
To: Roytburd, Benjamin <roytburd@msu.edu>
Cc: Tom Rini <trini@konsulko.com>; u-boot@lists.denx.de <u-boot@lists.denx.de>
Subject: Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

On 14:23-20230505, Roytburd, Benjamin wrote:
> Nishanth,
>

Gentle reminder: Please do not top post  - email etiquette in upstream mailing
as well as please do not use flowed formatting. See [1] (I use neomutt
personally with 70 char line break)

> Agreed, this is very expensive for boot time, I could probably add a comment
> to address this. But I believe it is better to have this option than not, as
> I struggled to figure out that I even needed to prime ECC when I enabled it.
>
> What is the strategy for TODOs with U-boot? DMA would probably be the best
> option here instead of memset / for loop, but I do not have that implemented
> yet. I could submit it in a future patch.

Remember what I mentioned in my response: there are quite a few other
folks also using the SoC support and evm. They would rather not want
to see the increase in boot time if we merge such a change. I don't see
a reasonable alternative without using DMA to prime effectively.

Also, is blindly enabling ECC across the DDR effective or creating
other problems? Access latencies are increased (since incoming and
outgoing bursts need to be checked against checksum) or should we
scheme something with a range?

[...]

[1] https://urldefense.com/v3/__https://www.kernel.org/doc/html/v6.3/process/email-clients.html__;!!HXCxUKc!wabHWUgkSClHiAAP16BJSW3kCDVa_Vt0h8v97UjrkTHHYEiIMNsggLpyACyvo0XtCa-GuQ$
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

[Roytburd, Benjamin]

Hello all, I submitted a patch recently under  [PATCH 1/1] am65x_gp_evm: Support for ECC DDR with DMA priming<https://lists.denx.de/pipermail/u-boot/2023-June/519615.html>. It is with a different email but it is a formal patch for fixing the DDR ECC issue, as well as priming with DMA.

Thanks,
Ben
________________________________
From: Roytburd, Benjamin <roytburd@msu.edu>
Sent: Wednesday, May 17, 2023 11:41 AM
To: Nishanth Menon <nm@ti.com>
Cc: Tom Rini <trini@konsulko.com>; u-boot@lists.denx.de <u-boot@lists.denx.de>
Subject: Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

Enabling ECC has not given me any problems in running my applications, I have even performed poison tests with SECDED that have been effective. No problems besides the increased boot time due to priming are occurring.

But agreed, for a patch we should definitely use DMA as opposed to a for loop. I have not implemented DMA yet, but when I do I wil formally submit a patch with it.
________________________________
From: Nishanth Menon <nm@ti.com>
Sent: Wednesday, May 17, 2023 11:30 AM
To: Roytburd, Benjamin <roytburd@msu.edu>
Cc: Tom Rini <trini@konsulko.com>; u-boot@lists.denx.de <u-boot@lists.denx.de>
Subject: Re: Pull Request / Patch: Enable Usage of ECC with DDR on AM654x

On 14:23-20230505, Roytburd, Benjamin wrote:
> Nishanth,
>

Gentle reminder: Please do not top post  - email etiquette in upstream mailing
as well as please do not use flowed formatting. See [1] (I use neomutt
personally with 70 char line break)

> Agreed, this is very expensive for boot time, I could probably add a comment
> to address this. But I believe it is better to have this option than not, as
> I struggled to figure out that I even needed to prime ECC when I enabled it.
>
> What is the strategy for TODOs with U-boot? DMA would probably be the best
> option here instead of memset / for loop, but I do not have that implemented
> yet. I could submit it in a future patch.

Remember what I mentioned in my response: there are quite a few other
folks also using the SoC support and evm. They would rather not want
to see the increase in boot time if we merge such a change. I don't see
a reasonable alternative without using DMA to prime effectively.

Also, is blindly enabling ECC across the DDR effective or creating
other problems? Access latencies are increased (since incoming and
outgoing bursts need to be checked against checksum) or should we
scheme something with a range?

[...]

[1] https://urldefense.com/v3/__https://www.kernel.org/doc/html/v6.3/process/email-clients.html__;!!HXCxUKc!wabHWUgkSClHiAAP16BJSW3kCDVa_Vt0h8v97UjrkTHHYEiIMNsggLpyACyvo0XtCa-GuQ$
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D