Re: [PATCH v4 1/5] mtd: rawnand: meson: fix command sequence for read/write

[Miquel Raynal]

Hi Arseniy,

avkrasnov@xxxxxxxxxxxxxx wrote on Tue, 30 May 2023 16:35:59 +0300:

> On 30.05.2023 16:05, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > avkrasnov@xxxxxxxxxxxxxx wrote on Tue, 30 May 2023 14:19:08 +0300:
> >   
> >> On 26.05.2023 20:22, Miquel Raynal wrote:  
> >>> Hi Arseniy,
> >>>
> >>> avkrasnov@xxxxxxxxxxxxxx wrote on Wed, 24 May 2023 12:05:47 +0300:
> >>>     
> >>>> On 23.05.2023 12:12, Arseniy Krasnov wrote:    
> >>>>> Hello Miquel, Liang
> >>>>>
> >>>>> On 22.05.2023 18:05, Miquel Raynal wrote:      
> >>>>>> Hi Arseniy,
> >>>>>>
> >>>>>> AVKrasnov@xxxxxxxxxxxxxx wrote on Mon, 15 May 2023 12:44:35 +0300:
> >>>>>>      
> >>>>>>> This fixes read/write functionality by:
> >>>>>>> 1) Changing NFC_CMD_RB_INT bit value.      
> >>>>>>
> >>>>>> I guess this is a separate fix
> >>>>>>      
> >>>>>
> >>>>> Ok, I'll move it to separate patch
> >>>>>       
> >>>>>>> 2) Adding extra NAND_CMD_STATUS command on each r/w request.      
> >>>>>>
> >>>>>> Is this really needed? Looks like you're delaying the next op only. Is
> >>>>>> using a delay enough? If yes, then it's probably the wrong approach.      
> >>>>
> >>>> Hi Miquel, small update, I found some details from @Liang's message in v1 talks from the last month:
> >>>>
> >>>> *
> >>>> After sending NAND_CMD_READ0, address, NAND_CMD_READSTART and read status(NAND_CMD_STATUS = 0x70) commands, it should send
> >>>> NAND_CMD_READ0 command for exiting the read status mode from the datasheet from NAND device.    
> >>>
> >>> That is true.
> >>>     
> >>>> but previous meson_nfc_queue_rb()
> >>>> only checks the Ready/Busy pin and it doesn't send read status(NAND_CMD_STATUS = 0x70) command.
> >>>> i think there is something wrong with the Ready/Busy pin(please check the hardware whether this
> >>>> Ready/Busy pin is connected with SOC) or the source code. i have the board without Ready/Busy pin and prefer to use the
> >>>> nfc command called RB_IO6. it sends NAND_CMD_STATUS command and checks bit6 of the status register of NAND device from the
> >>>> data bus and generate IRQ if ready.
> >>>> *
> >>>>
> >>>> I guess, that sequence of commands from this patch is described in datasheet (unfortunately I don't have it and relied on the old driver).
> >>>> Yesterday I tried to remove sending of NAND_CMD_STATUS from this patch, but it broke current driver - i had ECC errors, so it looks like
> >>>> "shot in the dark" situation, to understand this logic.    
> >>>
> >>> When an operation on the NAND array happens (eg. read, prog, erase),
> >>> you need to wait "some time" before accessing the internal sram or even
> >>> the chip which is "busy" until it gets "ready" again. You can probe the
> >>> ready/busy pin (that's the hardware way, fast and reliable) or you can
> >>> poll a status with NAND_CMD_STATUS. The chips are designed so they can
> >>> actually process that command while they are doing time consuming tasks
> >>> to update the host. But IIRC every byte read will return the status
> >>> until you send READ0 again, which means "I'm done with the status
> >>> read" somehow.
> >>>
> >>> Please see nand_soft_waitrdy() in order to understand how this is
> >>> supposed to work. You can even use that helper (which is exported)
> >>> instead of open-coding it in your driver. See atmel or sunxi
> >>> implementations for instance.
> >>>
> >>> As using the native RB pin is better, you would need to identify
> >>> whether you have one or not at probe time and then either poll the
> >>> relevant bit of your controller if there is one, or fallback to the
> >>> soft read (which should fallback on exec_op in the end).    
> >>
> >> Thanks for this information! I'll use 'nand_soft_waitrdy()' at least, because i guess that
> >> there is no RB pin on my device.  
> > 
> > Currently there is only support for the physical pin IIRC. This means
> > you cannot just drop it. You need to support both.  
> 
> Yes, i'm not going to drop RB pin support, but as I don't have device to test it(i guess), i'll add
> 'nand_sort_waitrdy()' anyway.

Clear. Then go for it :)

Thanks,
Miquèl