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

[york sun]

On 11/08/2016 09:03 AM, york.sun at 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 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.
>

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