-----Original Message----- From: york sun Sent: Wednesday, November 09, 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
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.
If we keep A009942 workaround in fsl_ddr_gen4.c, 1) we have to duplicate 3 same implement of A009942 separately in mpc85xx_ddr_gen3.c, arm_ddr_gen3.c and fsl_ddr_gen4.c, that is not a good idea. 2) we have to modify more code struct to introduce memctl_options_t *popts pointer in mpc85xx_ddr_gen3.c, arm_ddr_gen3.c and fsl_ddr_gen4.c to configure new option popts->cpo_sample. You had added ERRATUM_A004508 in ctrl_regs.c instead of in hardware interface, so as a special A009942, we can do it as well. Actually I had carefully thought of what you mentioned concerns before I decided to move A009942 and A008378(only for debug[28]) to common ctrl_regs.c. for future erratum and other registers except debug[28], we still keep them in files of hardware interface.
-Shengzhou