[U-Boot] [PATCH 1/3] fsl/ddr: Revise erratum a009942 and clean related erratum

[Shengzhou Liu]

> -----Original Message-----
> From: york sun
> Sent: Wednesday, November 09, 2016 1:04 AM
> To: Shengzhou Liu <shengzhou.liu at nxp.com>; u-boot at 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 at nxp.com>; u-boot at 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