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

[Masahiro Yamada]

On Fri, Jan 10, 2020 at 1:05 PM Marek Vasut <marex at denx.de> wrote:
>
> On 1/10/20 3:45 AM, Masahiro Yamada wrote:
> > On Fri, Jan 10, 2020 at 9:14 AM Marek Vasut <marex at denx.de> wrote:
> >>
> >> While the Denali NAND is initialized by the BootROM in SPL, there
> >> are still a couple of settings which are missing.
> >
> >
> > This statement is wrong.
> >
> > While the Denali NAND is initialized by the BootROM,
> > the SOCFPGA SPL calls socfpga_per_reset_all() to put
> > most of peripherals into the reset state.
> > So, all the register values are lost.
>
> And that is fine, resetting hardware to put it into defined state is
> what must happen, otherwise we would have all kinds of obscure
> semi-defined states.
>
> >> These can trigger
> >> subtle corruption of the data read out of the NAND. Fill these
> >> settings in just like they are filled in by the full Denali NAND
> >> driver in denali_hw_init().
> >>
> >> Signed-off-by: Marek Vasut <marex at denx.de>
> >> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
> >> ---
> >>  drivers/mtd/nand/raw/denali_spl.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/raw/denali_spl.c b/drivers/mtd/nand/raw/denali_spl.c
> >> index dbaba3cab2..b8b29812aa 100644
> >> --- a/drivers/mtd/nand/raw/denali_spl.c
> >> +++ b/drivers/mtd/nand/raw/denali_spl.c
> >> @@ -173,6 +173,13 @@ void nand_init(void)
> >>         page_size = readl(denali_flash_reg + DEVICE_MAIN_AREA_SIZE);
> >>         oob_size = readl(denali_flash_reg + DEVICE_SPARE_AREA_SIZE);
> >>         pages_per_block = readl(denali_flash_reg + PAGES_PER_BLOCK);
> >> +
> >> +       /* Do as denali_hw_init() does. */
> >> +       writel(CONFIG_NAND_DENALI_SPARE_AREA_SKIP_BYTES,
> >> +              denali_flash_reg + SPARE_AREA_SKIP_BYTES);
> >> +       writel(0x0F, denali_flash_reg + RB_PIN_ENABLED);
> >> +       writel(CHIP_EN_DONT_CARE__FLAG, denali_flash_reg + CHIP_ENABLE_DONT_CARE);
> >> +       writel(0xffff, denali_flash_reg + SPARE_AREA_MARKER);
> >
> >
> > You put this code just because you found it worked for your board.
>
> I'm not even sure what to respond to this. Yes, I put this code here
> because it initializes the NAND controller fully and yes, I tested it on
> actual physical SoCFPGA hardware.
>
> > If you reset the NAND controller, not only these four,
> > but all the register values are lost.
> > Especially, page_size, oob_size, etc.
> > https://github.com/u-boot/u-boot/blob/v2020.01/drivers/mtd/nand/raw/denali_spl.c#L170
>
> Nope, those are correct on SoCFPGA.


You are seeing values re-initialized by the controller.

Maybe, you do not know this IP.

So, here is how it works.


When you assert the reset, **all** register values are lost.

When you de-assert the reset again, the controller starts
running without any software control. The controller
issues a RESET command to the chip select 0, and attempts to
read out the chip ID, and further more, ONFI parameters if it
is an ONFI-compliant device. Then, the controller sets up the
relevant registers based on the detected device parameters.

SOCFPGA is working based on this feature.
That is why you thought only 4 registers were lost.



The recent uniphier SoCs tend to not use it
because this hardware feature is known to be super-buggy
for non-ONFi cases.

For recent uniphier SoCs, [2] is applied in the following
commit description:
https://github.com/torvalds/linux/commit/0615e7ad5d52



>
> > 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.


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.


>
> > 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.

Why don't you just use it as-is?


> > 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.



--
Best Regards
Masahiro Yamada