[PATCH 1/3] mtd: rawnand: denali-spl: Add missing hardware init

[Marek Vasut]

On 1/14/20 7:55 AM, Masahiro Yamada wrote:

Hi,

[...]

>> I only know what is written in the publicly available Altera
>> documentation and how NAND works on Altera SoCs.
> 
> Right.
> Altera's SOCFPGA does not describe all the features of this IP.
> 
> To understand this IP fully, you need to read the IP datasheet,
> which is not publicly available, unfortunately.
> 
> Socionext (and also Altera/Intel) has access to the IP datasheet.

Great, that's not really helpful from the silicon vendor side :(

>> [...]
>>
>>>>> It is working on your board
>>>>> because SOCFPGA enables the reset sequencer,
>>>>> which enumerates the nand chip by hardware.
>>>>> It is not necessarily true for other platforms,
>>>>> like the UniPhier platform.
>>>>
>>>> So Uniphier has a different behavior, fine, shall I put ifdef around
>>>> this then ? Or what is it that you want ?
>>>
>>>
>>> I do not want to see this patch in upstream.
>>
>> I see, but then that does not permit my hardware to work, so we need to
>> find some solution.
> 
> OK, see below.

OK

>>> Currently, CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES
>>> is only used for U-Boot proper.
>>>
>>> I'd like to back-port
>>> http://patchwork.ozlabs.org/patch/1214018/
>>> when it is accepted in Linux,
>>> then delete CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES
>>> entirely.
>>>
>>> This patch is adding it to SPL.
>>>
>>> CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES will
>>> stay if this patch gets in.
>>
>> Not necessarily, you can just program the skip bytes register with
>> default value depending on the platform, just like Linux does.
>>
>> register = is_socfpga() ? 2 : 8;
>>
>>>>> The reliable way for full initialization is to
>>>>> call nand_scan_ident(), which is too big for SPL.
>>>>
>>>> Right, so not an option.
>>>>
>>>>> To sum up, this is not a proper approach.
>>>>> Do not reset the NAND controller in SPL.
>>>>
>>>> This is not a proper approach, not resetting hardware would mean you end
>>>> up with hardware in undefined state.
>>>
>>>
>>> It's working in the state defined by the boot ROM.
>>> The boot ROM can read pages. So, SPL will be
>>> able to do it in the same manner.
>>
>> This is where you are wrong.
>>
>> The SoCFPGA has different reset states -- cold and warm -- warm is the
>> default when resetting out of U-Boot, Linux, etc. Cold reset is
>> generally not used.
>>
>> In Cold reset / Power-on reset, the NAND IP gets reset, along with other
>> IPs. The BootROM then loads the SPL into SRAM from NAND and starts it.
>>
>> In Warm reset, the BootROM checks if the SPL in SRAM is valid and if so,
>> jumps directly to it. The boot media is not even accessed. If the boot
>> media is in some inconsistent state, then the first software which tries
>> to access it fails in some obscure way.
> 
> 
> Until I read this email, I did not notice the fact
> you were talking about the warm reset, which
> completely skips the NAND controller access.

Good, so now we're back on track.

However, note that the SPL always resets the NAND IP, both for warm
reset and cold reset condition. Hence the registers must also always be
reprogrammed.

> Your commit description (both v1 and v2) starts like this:
> 
> "While the Denali NAND is initialized by the BootROM in SPL, ..."
>                           ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> 
> Now I finally understood what issue you want to solve.
> 
> Could you rephrase the commit description please?
> I could not read the warm boot stuff from the commit log.
> 
> 
> 
> FWIW, the following is what I understood so far.
> (Please modify as needed, or you can write it up from scratch.)

I will reword it.

> "
> Currently, denali_spl does not set up the registers because
> it expects the controller has been initialized by the Boot ROM.
> 
> It is true for the ARM-v7 generation SoCs of UniPhier platform,
> which only supports the cold boot. So, UniPhier SPL uses
> the NAND controller without resetting it.
> 
> For SOCFPGA, we need to support not only the cold boot,
> but also the warm boot, where Boot ROM may entirely skip
> the boot media access.
> 
> To avoid all kinds of obscure failures caused by undefined
> hardware state, SOCFPGA SPL puts most of peripherals into the
> reset state by calling socfpga_per_reset(), and then de-asserts
> the reset of NAND controller if the system is booting up
> from the NAND device.
> 
> Most of registers are self-initialized by the NAND controller
> because the Denali NAND IP supports the HW-controlled bootstrap.
> However, some registers must be explicitly set up by software.
> Currently, this part is missing in nand_spl.
> "

Yes

>> Hence, not resetting the NAND in the SPL (and in fact, boot media in
>> general) leads to all kinds of obscure failures. And so, we need to
>> reset the boot media on SoCFPGA, not doing so is not an option.
>>
>> Note that switching to Cold reset is also not an option, as that has
>> other implications and drawbacks.
>>
>>> Why don't you just use it as-is?
>>
>> See above.
>>
>>>>> Keep the working state that has already been set up
>>>>> by the boot ROM.
>>>>
>>>> The state in which the controller is is NOT defined. Resetting it puts
>>>> it into defined state.
>>>>
>>>> I am fine with putting an ifdef(SOCFPGA) around this code to avoid
>>>> triggering it on uniphier if that's fine with you.
>>>
>>>
>>>
>>> I am not fine with ugly #ifdef in drivers
>>> as you did in the previous 2/3, 3/3.
>>
>> Do you have a better option for an non-DM SPL driver ?
> 
> 
> Now I am convinced that this change is needed, but
> I'd like the commit log to describe the motivation
> of this change.
I will do that.

-- 
Best regards,
Marek Vasut