> -----Original Message----- > From: york sun > Sent: Wednesday, November 09, 2016 1:04 AM > To: Shengzhou Liu <shengzhou....@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....@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot