On 11/08/2016 09:03 AM, york.sun@nxp.com wrote:

On 11/08/2016 02:39 AM, Shengzhou Liu wrote:

...

...

-----Original Message----- From: york sun Sent: Tuesday, November 08, 2016 1:04 AM To: Shengzhou Liu shengzhou.liu@nxp.com; u-boot@lists.denx.de Subject: Re: [PATCH 1/3] fsl/ddr: Revise erratum a009942 and clean related erratum

...

York,

This change(moving to ctrl_regs.c) has the same effect as read-modify- write(done in fsl_ddr_gen4.c) before MEM_EN is enabled for DDRC. As I commented in code with "the POR value of debug_29 register is zero" for A009942 workaround when moving it to ctrl_regs.c, Actually only A008378 changes debug_29[8:11] bits to 9 from original POR value 0 before the implementing of A009942, and A009942 overrides debug_29[8:11] set by A008378. So we can set debug_29 in ctrl_regs.c, it doesn't break anything.

Shengzhou,

The presumption of POR value is not safe. It is safe for this case for now. You may have other erratum workaround popping up later using the same register, or other registers. PBI can also change registers. This sets an example to preset those registers out the scope of hardware interaction, regardless which controller is in use, or its state.

York This change(move to common ctrl_regs.c for reuse on DDR4/DDR3) is only for A009942, For other erratum workaround popping up later using the same register, or other registers, we still can implement them in fsl_ddr_gen4.c or in other according to actual specific sequence requirement. I will add reading value of debug_29 register for A009942 in ctrl_regs.c in next version, which will be safe regardless how POR changes.

You added A009942 and A008378. I don't think this is the right way. You break the isolation between calculating the register values and hardware interface. If you follow this path, you will add more and more hardware interaction into ctrl_regs.c file. Until your change you don't have to worry about any hardware condition in this file.

Another thought, if you are concerned about reusing the code for workaround, how about you put the common code into util.c, or even add an errata.c file?

York