[v1] mtd: rawnand: meson: fix ready/busy command

  Fix the ready/busy command value.

Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
Cc: stable@vger.kernel.org
Suggested-by: Liang Yang <liang.yang@amlogic.com>
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Hello Miquel!

I exclude this patch from the recent Meson patchset, as it is fix and not related to another patches.
Also I think that I can split Meson patchset (from links below) in the following way:
1) Patch/patchset for OOB layout
2) Patchset for "nand-rb" logic (meson_nand.c + DT bindings)
These two can also go independently:
3) https://lore.kernel.org/linux-mtd/20230601061850.3907800-6-AVKrasnov@sberdevices.ru/
4) https://lore.kernel.org/linux-mtd/20230601061850.3907800-7-AVKrasnov@sberdevices.ru/

What do You think?

Thanks, Arseniy

On 05.06.2023 09:10, Arseniy Krasnov wrote:
> Fix the ready/busy command value.
> 
> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
> Cc: stable@vger.kernel.org
> Suggested-by: Liang Yang <liang.yang@amlogic.com>
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  drivers/mtd/nand/raw/meson_nand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index 074e14225c06..9dd4a676497b 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -37,7 +37,7 @@
>  #define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
>  #define NFC_CMD_SCRAMBLER_DISABLE	0
>  #define NFC_CMD_SHORTMODE_DISABLE	0
> -#define NFC_CMD_RB_INT		BIT(14)
> +#define NFC_CMD_RB_INT		((0xb << 10) | BIT(18) | BIT(16))
>  
>  #define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
>
  

On Mon, Jun 05, 2023 at 09:10:48AM +0300, Arseniy Krasnov wrote:
> Fix the ready/busy command value.

"Fix" it in what way?  Please provide more information, this does not
explain what is happening here at all.

greg k-h
  

Seems this fix is not full, pls ignore it

Thanks, Arseniy

On 05.06.2023 10:05, Greg KH wrote:
> On Mon, Jun 05, 2023 at 09:10:48AM +0300, Arseniy Krasnov wrote:
>> Fix the ready/busy command value.
> 
> "Fix" it in what way?  Please provide more information, this does not
> explain what is happening here at all.
> 
> greg k-h
  

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 09:39:40 +0300:

> Hello Miquel!
> 
> I exclude this patch from the recent Meson patchset, as it is fix and not related to another patches.
> Also I think that I can split Meson patchset (from links below) in the following way:
> 1) Patch/patchset for OOB layout
> 2) Patchset for "nand-rb" logic (meson_nand.c + DT bindings)
> These two can also go independently:
> 3) https://lore.kernel.org/linux-mtd/20230601061850.3907800-6-AVKrasnov@sberdevices.ru/
> 4) https://lore.kernel.org/linux-mtd/20230601061850.3907800-7-AVKrasnov@sberdevices.ru/

LGTM.

> 
> What do You think?
> 
> Thanks, Arseniy
> 
> On 05.06.2023 09:10, Arseniy Krasnov wrote:
> > Fix the ready/busy command value.
> > 
> > Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
> > Cc: stable@vger.kernel.org
> > Suggested-by: Liang Yang <liang.yang@amlogic.com>
> > Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> > ---
> >  drivers/mtd/nand/raw/meson_nand.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> > index 074e14225c06..9dd4a676497b 100644
> > --- a/drivers/mtd/nand/raw/meson_nand.c
> > +++ b/drivers/mtd/nand/raw/meson_nand.c
> > @@ -37,7 +37,7 @@
> >  #define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
> >  #define NFC_CMD_SCRAMBLER_DISABLE	0
> >  #define NFC_CMD_SHORTMODE_DISABLE	0
> > -#define NFC_CMD_RB_INT		BIT(14)
> > +#define NFC_CMD_RB_INT		((0xb << 10) | BIT(18) | BIT(16))
> >  
> >  #define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
> >    


Thanks,
Miquèl
  

On 05.06.2023 11:17, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 09:39:40 +0300:
> 
>> Hello Miquel!
>>
>> I exclude this patch from the recent Meson patchset, as it is fix and not related to another patches.
>> Also I think that I can split Meson patchset (from links below) in the following way:
>> 1) Patch/patchset for OOB layout
>> 2) Patchset for "nand-rb" logic (meson_nand.c + DT bindings)
>> These two can also go independently:
>> 3) https://lore.kernel.org/linux-mtd/20230601061850.3907800-6-AVKrasnov@sberdevices.ru/
>> 4) https://lore.kernel.org/linux-mtd/20230601061850.3907800-7-AVKrasnov@sberdevices.ru/
> 
> LGTM.

I think we don't need this patch with new RB_INT value. I dive into conversations with Liang Yang - author of this driver. He said,
that this define is "special" polling mode. Now I'm a little bit more experienced in NAND area, so IIUC we have two basic waiting modes:
1) Software, by sending status command, then polling reply from controller, and then finally send READ0 to exit status reading mode (nand_soft_waitrdy()).
2) By RB pin and interrupt - in this mode we send vendor specific command to the controller and it triggers interrupt on RB pin update.
   In this case RB pin must be connected to the controller. This mode is already implemented by Liang Yang.

Now, command with this new define works in the following way: we send status command, by instead of reading reply from it by software,
controller itself checks RB pin and status ready bit in IO bus. Driver just waits on completion. After waiting we need to send
READ0 command again to make controller leave status reading mode. I think this is like "intermediate" mode between software polling
and hardware interrupt - there is no need to spin in loop, waiting for status ready bit, but still need to leave status mode by sending
READ0 command. I'm not sure that we need this as third mode of waiting for command is done. May be at least not in this work on Meson driver,
so I guess to drop this patch at  this moment and add 'nand_soft_waitrdy()' support.

Thanks, Arseniy

> 
>>
>> What do You think?
>>
>> Thanks, Arseniy
>>
>> On 05.06.2023 09:10, Arseniy Krasnov wrote:
>>> Fix the ready/busy command value.
>>>
>>> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
>>> Cc: stable@vger.kernel.org
>>> Suggested-by: Liang Yang <liang.yang@amlogic.com>
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>> ---
>>>  drivers/mtd/nand/raw/meson_nand.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>> index 074e14225c06..9dd4a676497b 100644
>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>> @@ -37,7 +37,7 @@
>>>  #define NFC_CMD_SCRAMBLER_ENABLE	BIT(19)
>>>  #define NFC_CMD_SCRAMBLER_DISABLE	0
>>>  #define NFC_CMD_SHORTMODE_DISABLE	0
>>> -#define NFC_CMD_RB_INT		BIT(14)
>>> +#define NFC_CMD_RB_INT		((0xb << 10) | BIT(18) | BIT(16))
>>>  
>>>  #define NFC_CMD_GET_SIZE(x)	(((x) >> 22) & GENMASK(4, 0))
>>>    
> 
> 
> Thanks,
> Miquèl
  

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 11:27:34 +0300:

> On 05.06.2023 11:17, Miquel Raynal wrote:
> > Hi Arseniy,
> > 
> > avkrasnov@sberdevices.ru wrote on Mon, 5 Jun 2023 09:39:40 +0300:
> >   
> >> Hello Miquel!
> >>
> >> I exclude this patch from the recent Meson patchset, as it is fix and not related to another patches.
> >> Also I think that I can split Meson patchset (from links below) in the following way:
> >> 1) Patch/patchset for OOB layout
> >> 2) Patchset for "nand-rb" logic (meson_nand.c + DT bindings)
> >> These two can also go independently:
> >> 3) https://lore.kernel.org/linux-mtd/20230601061850.3907800-6-AVKrasnov@sberdevices.ru/
> >> 4) https://lore.kernel.org/linux-mtd/20230601061850.3907800-7-AVKrasnov@sberdevices.ru/  
> > 
> > LGTM.  
> 
> I think we don't need this patch with new RB_INT value. I dive into conversations with Liang Yang - author of this driver. He said,
> that this define is "special" polling mode. Now I'm a little bit more experienced in NAND area, so IIUC we have two basic waiting modes:
> 1) Software, by sending status command, then polling reply from controller, and then finally send READ0 to exit status reading mode (nand_soft_waitrdy()).

This is useful...

> 2) By RB pin and interrupt - in this mode we send vendor specific command to the controller and it triggers interrupt on RB pin update.
>    In this case RB pin must be connected to the controller. This mode is already implemented by Liang Yang.

...this is useful...

> 
> Now, command with this new define works in the following way: we send status command, by instead of reading reply from it by software,
> controller itself checks RB pin and status ready bit in IO bus. Driver just waits on completion. After waiting we need to send
> READ0 command again to make controller leave status reading mode. I think this is like "intermediate" mode between software polling
> and hardware interrupt - there is no need to spin in loop, waiting for status ready bit, but still need to leave status mode by sending
> READ0 command. I'm not sure that we need this as third mode of waiting for command is done. May be at least not in this work on Meson driver,
> so I guess to drop this patch at  this moment and add 'nand_soft_waitrdy()' support.

...I agree we can skip this one.

Thanks,
Miquèl