Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,

2017-06-13 17:30 GMT+09:00 Boris Brezillon :
> On Tue, 13 Jun 2017 17:17:41 +0900
> Masahiro Yamada  wrote:
>
>> Hi Boris,
>>
>>
>> 2017-06-13 16:02 GMT+09:00 Boris Brezillon 
>> :
>>
>> >
>> > BTW, I also implemented ->read/write_buf_word() since the core may one
>> > day call these functions, and the default implementations used by the
>> > core when these hooks are NULL are not appropriate in your case.
>> >
>>
>> BTW, why doesn't the default hook in the core do like this?
>>
>>
>> static uint8_t nand_read_byte(struct mtd_info *mtd)
>> {
>> struct nand_chip *chip = mtd_to_nand(mtd);
>> uint8_t byte;
>>
>> chip->read_buf(chip, , 1);
>> return byte;
>> }
>>
>>
>> ->read_byte() is a special case of ->read_buf() with length==1,
>> so this should work.
>
> Not sure it works for all implementation. ->read_byte() is expected
> to return the lower 8 bits when interacting with a 16-bits bus. If we do
> what you suggest, and ->read_buf() appears to be caching data in an
> intermediate buffer if the amount of data is not aligned on 2 bytes,
> you might retrieve data you don't care about when ->read_byte() is
> called several times.

You are right.

I missed the case where ->read_byte() is called repeatedly.

Thanks!



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,

2017-06-13 17:30 GMT+09:00 Boris Brezillon :
> On Tue, 13 Jun 2017 17:17:41 +0900
> Masahiro Yamada  wrote:
>
>> Hi Boris,
>>
>>
>> 2017-06-13 16:02 GMT+09:00 Boris Brezillon 
>> :
>>
>> >
>> > BTW, I also implemented ->read/write_buf_word() since the core may one
>> > day call these functions, and the default implementations used by the
>> > core when these hooks are NULL are not appropriate in your case.
>> >
>>
>> BTW, why doesn't the default hook in the core do like this?
>>
>>
>> static uint8_t nand_read_byte(struct mtd_info *mtd)
>> {
>> struct nand_chip *chip = mtd_to_nand(mtd);
>> uint8_t byte;
>>
>> chip->read_buf(chip, , 1);
>> return byte;
>> }
>>
>>
>> ->read_byte() is a special case of ->read_buf() with length==1,
>> so this should work.
>
> Not sure it works for all implementation. ->read_byte() is expected
> to return the lower 8 bits when interacting with a 16-bits bus. If we do
> what you suggest, and ->read_buf() appears to be caching data in an
> intermediate buffer if the amount of data is not aligned on 2 bytes,
> you might retrieve data you don't care about when ->read_byte() is
> called several times.

You are right.

I missed the case where ->read_byte() is called repeatedly.

Thanks!



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon]

On Tue, 13 Jun 2017 17:17:41 +0900
Masahiro Yamada  wrote:

> Hi Boris,
> 
> 
> 2017-06-13 16:02 GMT+09:00 Boris Brezillon 
> :
> 
> >
> > BTW, I also implemented ->read/write_buf_word() since the core may one
> > day call these functions, and the default implementations used by the
> > core when these hooks are NULL are not appropriate in your case.
> >  
> 
> BTW, why doesn't the default hook in the core do like this?
> 
> 
> static uint8_t nand_read_byte(struct mtd_info *mtd)
> {
> struct nand_chip *chip = mtd_to_nand(mtd);
> uint8_t byte;
> 
> chip->read_buf(chip, , 1);
> return byte;
> }
> 
> 
> ->read_byte() is a special case of ->read_buf() with length==1,  
> so this should work.

Not sure it works for all implementation. ->read_byte() is expected
to return the lower 8 bits when interacting with a 16-bits bus. If we do
what you suggest, and ->read_buf() appears to be caching data in an
intermediate buffer if the amount of data is not aligned on 2 bytes,
you might retrieve data you don't care about when ->read_byte() is
called several times.

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon]

On Tue, 13 Jun 2017 17:17:41 +0900
Masahiro Yamada  wrote:

> Hi Boris,
> 
> 
> 2017-06-13 16:02 GMT+09:00 Boris Brezillon 
> :
> 
> >
> > BTW, I also implemented ->read/write_buf_word() since the core may one
> > day call these functions, and the default implementations used by the
> > core when these hooks are NULL are not appropriate in your case.
> >  
> 
> BTW, why doesn't the default hook in the core do like this?
> 
> 
> static uint8_t nand_read_byte(struct mtd_info *mtd)
> {
> struct nand_chip *chip = mtd_to_nand(mtd);
> uint8_t byte;
> 
> chip->read_buf(chip, , 1);
> return byte;
> }
> 
> 
> ->read_byte() is a special case of ->read_buf() with length==1,  
> so this should work.

Not sure it works for all implementation. ->read_byte() is expected
to return the lower 8 bits when interacting with a 16-bits bus. If we do
what you suggest, and ->read_buf() appears to be caching data in an
intermediate buffer if the amount of data is not aligned on 2 bytes,
you might retrieve data you don't care about when ->read_byte() is
called several times.

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon]

On Tue, 13 Jun 2017 17:04:11 +0900
Masahiro Yamada  wrote:

> Hi Boris,
> 
> 
> 2017-06-13 16:02 GMT+09:00 Boris Brezillon 
> :
> > Le Tue, 13 Jun 2017 14:03:52 +0900,
> > Masahiro Yamada  a écrit :
> >  
> >> This patch series intends to solve various problems.
> >>
> >> [1] The driver just retrieves the OOB area as-is
> >> whereas the controller uses syndrome page layout.
> >> [2] ONFi devices are not working
> >> [3] It can not read Bad Block Marker
> >>
> >> Outstanding changes are:
> >>  - Fix raw/oob callbacks for syndrome page layout
> >>  - Implement setup_data_interface() callback
> >>  - Fix/implement more commands for ONFi devices
> >>  - Allow to skip the driver internal bounce buffer
> >>  - Support PIO in case DMA is not supported
> >>  - Switch from ->cmdfunc over to ->cmd_ctrl
> >>
> >> 18 patches were merged by v2.
> >> 11 patches were merged by v3.
> >> 2 patches were merged by v4.
> >> 5 patches were merged by v5.
> >> Here is the rest of the series.
> >>
> >> v1: https://lkml.org/lkml/2016/11/26/144
> >> v2: https://lkml.org/lkml/2017/3/22/804
> >> v3: https://lkml.org/lkml/2017/3/30/90
> >> v4: https://lkml.org/lkml/2017/6/5/1005
> >>
> >> Masahiro Yamada (18):
> >>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
> >>   mtd: nand: denali: remove unneeded find_valid_banks()
> >>   mtd: nand: denali: handle timing parameters by setup_data_interface()
> >>   mtd: nand: denali: rework interrupt handling
> >>   mtd: nand: denali: fix NAND_CMD_STATUS handling
> >>   mtd: nand: denali: fix NAND_CMD_PARAM handling  
> >
> > AFAICT, patch 5 and 6 are unneeded...
> >  
> >>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc  
> >
> > ... because you're anyway switching to ->cmd_ctrl() in patch 7, which
> > fixes the problem you were addressing in patch 5 and 6.
> >
> > Please squash those 3 patches into a single one and adjust your commit
> > message accordingly (explaining that it fixes STATUS and PARAM handling).
> > See below if you need an example.  
> 
> Squashing 3 patches is OK, but I did not get your additional implementation.
> 
> 
> > BTW, I also implemented ->read/write_buf_word() since the core may one
> > day call these functions, and the default implementations used by the
> > core when these hooks are NULL are not appropriate in your case.  
> 
> I implemented
> 
> denali_read_buf()
> denali_write_buf()
> denali_read_buf16()
> denali_write_buf16()
> 
> in 13/18 in a bit different way:
> http://patchwork.ozlabs.org/patch/774961/

Your implementation is better (I didn't know if a single
iowrite32(MODE_11 | BANK(denali->flash_bank) | 2, denali->flash_mem)
was enough or if we needed to call it each time we read/write a
byte/word).

> 
> 
> If you like, I can modify 13/18 so that
> denali_read/write_byte() is implemented based on denali_read/write_buf().

You can. Anyway, I'd prefer to have ->read/write_buf/byte/word()
implemented in patch 7, even if you actually start using
->read/write_buf() in patch 13.

> 
> 
> 
> 
> 
> > --->8---  
> > From 136727ba7b453ca1567c711037230aa6ec0f124a Mon Sep 17 00:00:00 2001
> > From: Masahiro Yamada 
> > Date: Tue, 13 Jun 2017 14:03:57 +0900
> > Subject: [PATCH] mtd: nand: denali: switch over to cmd_ctrl instead of 
> > cmdfunc
> >
> > The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().
> >
> > Besides, we see /* TODO: Read OOB data */ comment line.
> >
> > It would be possible to add more commands along with the current
> > implementation, but having ->cmd_ctrl() seems a better approach from
> > the discussion with Boris [1].
> >
> > Rely on the default ->cmdfunc() from the framework and implement the
> > driver's own ->cmd_ctrl(). We are also implementing  
> > ->read/write_buf/byte/word().  
> >
> > Also add ->write_byte(), which is needed for write direction commands.
> >
> > Then, we can drop nand_onfi_get_set_features_notsupp from this driver.
> >
> > This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
> > NAND_CMD_STATUS was just faked by the implementation, and the only valid
> > bit returned in this case was the WP bit.
> > NAND_CMD_PARAM was completely broken: not only the command sent on the
> > bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM, but the driver was
> > only reading 8 bytes of data, while the parameter page is contains
> > several hundreds of bytes.  
> 
> Probably "... page contains" instead of "... page is contains".

Yep. You can also reword the commit message if you want.

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon]

On Tue, 13 Jun 2017 17:04:11 +0900
Masahiro Yamada  wrote:

> Hi Boris,
> 
> 
> 2017-06-13 16:02 GMT+09:00 Boris Brezillon 
> :
> > Le Tue, 13 Jun 2017 14:03:52 +0900,
> > Masahiro Yamada  a écrit :
> >  
> >> This patch series intends to solve various problems.
> >>
> >> [1] The driver just retrieves the OOB area as-is
> >> whereas the controller uses syndrome page layout.
> >> [2] ONFi devices are not working
> >> [3] It can not read Bad Block Marker
> >>
> >> Outstanding changes are:
> >>  - Fix raw/oob callbacks for syndrome page layout
> >>  - Implement setup_data_interface() callback
> >>  - Fix/implement more commands for ONFi devices
> >>  - Allow to skip the driver internal bounce buffer
> >>  - Support PIO in case DMA is not supported
> >>  - Switch from ->cmdfunc over to ->cmd_ctrl
> >>
> >> 18 patches were merged by v2.
> >> 11 patches were merged by v3.
> >> 2 patches were merged by v4.
> >> 5 patches were merged by v5.
> >> Here is the rest of the series.
> >>
> >> v1: https://lkml.org/lkml/2016/11/26/144
> >> v2: https://lkml.org/lkml/2017/3/22/804
> >> v3: https://lkml.org/lkml/2017/3/30/90
> >> v4: https://lkml.org/lkml/2017/6/5/1005
> >>
> >> Masahiro Yamada (18):
> >>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
> >>   mtd: nand: denali: remove unneeded find_valid_banks()
> >>   mtd: nand: denali: handle timing parameters by setup_data_interface()
> >>   mtd: nand: denali: rework interrupt handling
> >>   mtd: nand: denali: fix NAND_CMD_STATUS handling
> >>   mtd: nand: denali: fix NAND_CMD_PARAM handling  
> >
> > AFAICT, patch 5 and 6 are unneeded...
> >  
> >>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc  
> >
> > ... because you're anyway switching to ->cmd_ctrl() in patch 7, which
> > fixes the problem you were addressing in patch 5 and 6.
> >
> > Please squash those 3 patches into a single one and adjust your commit
> > message accordingly (explaining that it fixes STATUS and PARAM handling).
> > See below if you need an example.  
> 
> Squashing 3 patches is OK, but I did not get your additional implementation.
> 
> 
> > BTW, I also implemented ->read/write_buf_word() since the core may one
> > day call these functions, and the default implementations used by the
> > core when these hooks are NULL are not appropriate in your case.  
> 
> I implemented
> 
> denali_read_buf()
> denali_write_buf()
> denali_read_buf16()
> denali_write_buf16()
> 
> in 13/18 in a bit different way:
> http://patchwork.ozlabs.org/patch/774961/

Your implementation is better (I didn't know if a single
iowrite32(MODE_11 | BANK(denali->flash_bank) | 2, denali->flash_mem)
was enough or if we needed to call it each time we read/write a
byte/word).

> 
> 
> If you like, I can modify 13/18 so that
> denali_read/write_byte() is implemented based on denali_read/write_buf().

You can. Anyway, I'd prefer to have ->read/write_buf/byte/word()
implemented in patch 7, even if you actually start using
->read/write_buf() in patch 13.

> 
> 
> 
> 
> 
> > --->8---  
> > From 136727ba7b453ca1567c711037230aa6ec0f124a Mon Sep 17 00:00:00 2001
> > From: Masahiro Yamada 
> > Date: Tue, 13 Jun 2017 14:03:57 +0900
> > Subject: [PATCH] mtd: nand: denali: switch over to cmd_ctrl instead of 
> > cmdfunc
> >
> > The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().
> >
> > Besides, we see /* TODO: Read OOB data */ comment line.
> >
> > It would be possible to add more commands along with the current
> > implementation, but having ->cmd_ctrl() seems a better approach from
> > the discussion with Boris [1].
> >
> > Rely on the default ->cmdfunc() from the framework and implement the
> > driver's own ->cmd_ctrl(). We are also implementing  
> > ->read/write_buf/byte/word().  
> >
> > Also add ->write_byte(), which is needed for write direction commands.
> >
> > Then, we can drop nand_onfi_get_set_features_notsupp from this driver.
> >
> > This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
> > NAND_CMD_STATUS was just faked by the implementation, and the only valid
> > bit returned in this case was the WP bit.
> > NAND_CMD_PARAM was completely broken: not only the command sent on the
> > bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM, but the driver was
> > only reading 8 bytes of data, while the parameter page is contains
> > several hundreds of bytes.  
> 
> Probably "... page contains" instead of "... page is contains".

Yep. You can also reword the commit message if you want.

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,


2017-06-13 16:02 GMT+09:00 Boris Brezillon :

>
> BTW, I also implemented ->read/write_buf_word() since the core may one
> day call these functions, and the default implementations used by the
> core when these hooks are NULL are not appropriate in your case.
>

BTW, why doesn't the default hook in the core do like this?


static uint8_t nand_read_byte(struct mtd_info *mtd)
{
struct nand_chip *chip = mtd_to_nand(mtd);
uint8_t byte;

chip->read_buf(chip, , 1);
return byte;
}


->read_byte() is a special case of ->read_buf() with length==1,
so this should work.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,


2017-06-13 16:02 GMT+09:00 Boris Brezillon :

>
> BTW, I also implemented ->read/write_buf_word() since the core may one
> day call these functions, and the default implementations used by the
> core when these hooks are NULL are not appropriate in your case.
>

BTW, why doesn't the default hook in the core do like this?


static uint8_t nand_read_byte(struct mtd_info *mtd)
{
struct nand_chip *chip = mtd_to_nand(mtd);
uint8_t byte;

chip->read_buf(chip, , 1);
return byte;
}


->read_byte() is a special case of ->read_buf() with length==1,
so this should work.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,


2017-06-13 16:02 GMT+09:00 Boris Brezillon :
> Le Tue, 13 Jun 2017 14:03:52 +0900,
> Masahiro Yamada  a écrit :
>
>> This patch series intends to solve various problems.
>>
>> [1] The driver just retrieves the OOB area as-is
>> whereas the controller uses syndrome page layout.
>> [2] ONFi devices are not working
>> [3] It can not read Bad Block Marker
>>
>> Outstanding changes are:
>>  - Fix raw/oob callbacks for syndrome page layout
>>  - Implement setup_data_interface() callback
>>  - Fix/implement more commands for ONFi devices
>>  - Allow to skip the driver internal bounce buffer
>>  - Support PIO in case DMA is not supported
>>  - Switch from ->cmdfunc over to ->cmd_ctrl
>>
>> 18 patches were merged by v2.
>> 11 patches were merged by v3.
>> 2 patches were merged by v4.
>> 5 patches were merged by v5.
>> Here is the rest of the series.
>>
>> v1: https://lkml.org/lkml/2016/11/26/144
>> v2: https://lkml.org/lkml/2017/3/22/804
>> v3: https://lkml.org/lkml/2017/3/30/90
>> v4: https://lkml.org/lkml/2017/6/5/1005
>>
>> Masahiro Yamada (18):
>>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
>>   mtd: nand: denali: remove unneeded find_valid_banks()
>>   mtd: nand: denali: handle timing parameters by setup_data_interface()
>>   mtd: nand: denali: rework interrupt handling
>>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>>   mtd: nand: denali: fix NAND_CMD_PARAM handling
>
> AFAICT, patch 5 and 6 are unneeded...
>
>>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>
> ... because you're anyway switching to ->cmd_ctrl() in patch 7, which
> fixes the problem you were addressing in patch 5 and 6.
>
> Please squash those 3 patches into a single one and adjust your commit
> message accordingly (explaining that it fixes STATUS and PARAM handling).
> See below if you need an example.

Squashing 3 patches is OK, but I did not get your additional implementation.


> BTW, I also implemented ->read/write_buf_word() since the core may one
> day call these functions, and the default implementations used by the
> core when these hooks are NULL are not appropriate in your case.

I implemented

denali_read_buf()
denali_write_buf()
denali_read_buf16()
denali_write_buf16()

in 13/18 in a bit different way:
http://patchwork.ozlabs.org/patch/774961/


If you like, I can modify 13/18 so that
denali_read/write_byte() is implemented based on denali_read/write_buf().





> --->8---
> From 136727ba7b453ca1567c711037230aa6ec0f124a Mon Sep 17 00:00:00 2001
> From: Masahiro Yamada 
> Date: Tue, 13 Jun 2017 14:03:57 +0900
> Subject: [PATCH] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>
> The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().
>
> Besides, we see /* TODO: Read OOB data */ comment line.
>
> It would be possible to add more commands along with the current
> implementation, but having ->cmd_ctrl() seems a better approach from
> the discussion with Boris [1].
>
> Rely on the default ->cmdfunc() from the framework and implement the
> driver's own ->cmd_ctrl(). We are also implementing
> ->read/write_buf/byte/word().
>
> Also add ->write_byte(), which is needed for write direction commands.
>
> Then, we can drop nand_onfi_get_set_features_notsupp from this driver.
>
> This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
> NAND_CMD_STATUS was just faked by the implementation, and the only valid
> bit returned in this case was the WP bit.
> NAND_CMD_PARAM was completely broken: not only the command sent on the
> bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM, but the driver was
> only reading 8 bytes of data, while the parameter page is contains
> several hundreds of bytes.

Probably "... page contains" instead of "... page is contains".





-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

Hi Boris,


2017-06-13 16:02 GMT+09:00 Boris Brezillon :
> Le Tue, 13 Jun 2017 14:03:52 +0900,
> Masahiro Yamada  a écrit :
>
>> This patch series intends to solve various problems.
>>
>> [1] The driver just retrieves the OOB area as-is
>> whereas the controller uses syndrome page layout.
>> [2] ONFi devices are not working
>> [3] It can not read Bad Block Marker
>>
>> Outstanding changes are:
>>  - Fix raw/oob callbacks for syndrome page layout
>>  - Implement setup_data_interface() callback
>>  - Fix/implement more commands for ONFi devices
>>  - Allow to skip the driver internal bounce buffer
>>  - Support PIO in case DMA is not supported
>>  - Switch from ->cmdfunc over to ->cmd_ctrl
>>
>> 18 patches were merged by v2.
>> 11 patches were merged by v3.
>> 2 patches were merged by v4.
>> 5 patches were merged by v5.
>> Here is the rest of the series.
>>
>> v1: https://lkml.org/lkml/2016/11/26/144
>> v2: https://lkml.org/lkml/2017/3/22/804
>> v3: https://lkml.org/lkml/2017/3/30/90
>> v4: https://lkml.org/lkml/2017/6/5/1005
>>
>> Masahiro Yamada (18):
>>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
>>   mtd: nand: denali: remove unneeded find_valid_banks()
>>   mtd: nand: denali: handle timing parameters by setup_data_interface()
>>   mtd: nand: denali: rework interrupt handling
>>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>>   mtd: nand: denali: fix NAND_CMD_PARAM handling
>
> AFAICT, patch 5 and 6 are unneeded...
>
>>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>
> ... because you're anyway switching to ->cmd_ctrl() in patch 7, which
> fixes the problem you were addressing in patch 5 and 6.
>
> Please squash those 3 patches into a single one and adjust your commit
> message accordingly (explaining that it fixes STATUS and PARAM handling).
> See below if you need an example.

Squashing 3 patches is OK, but I did not get your additional implementation.


> BTW, I also implemented ->read/write_buf_word() since the core may one
> day call these functions, and the default implementations used by the
> core when these hooks are NULL are not appropriate in your case.

I implemented

denali_read_buf()
denali_write_buf()
denali_read_buf16()
denali_write_buf16()

in 13/18 in a bit different way:
http://patchwork.ozlabs.org/patch/774961/


If you like, I can modify 13/18 so that
denali_read/write_byte() is implemented based on denali_read/write_buf().





> --->8---
> From 136727ba7b453ca1567c711037230aa6ec0f124a Mon Sep 17 00:00:00 2001
> From: Masahiro Yamada 
> Date: Tue, 13 Jun 2017 14:03:57 +0900
> Subject: [PATCH] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
>
> The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().
>
> Besides, we see /* TODO: Read OOB data */ comment line.
>
> It would be possible to add more commands along with the current
> implementation, but having ->cmd_ctrl() seems a better approach from
> the discussion with Boris [1].
>
> Rely on the default ->cmdfunc() from the framework and implement the
> driver's own ->cmd_ctrl(). We are also implementing
> ->read/write_buf/byte/word().
>
> Also add ->write_byte(), which is needed for write direction commands.
>
> Then, we can drop nand_onfi_get_set_features_notsupp from this driver.
>
> This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
> NAND_CMD_STATUS was just faked by the implementation, and the only valid
> bit returned in this case was the WP bit.
> NAND_CMD_PARAM was completely broken: not only the command sent on the
> bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM, but the driver was
> only reading 8 bytes of data, while the parameter page is contains
> several hundreds of bytes.

Probably "... page contains" instead of "... page is contains".





-- 
Best Regards
Masahiro Yamada

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon]

Le Tue, 13 Jun 2017 14:03:52 +0900,
Masahiro Yamada  a écrit :

> This patch series intends to solve various problems.
> 
> [1] The driver just retrieves the OOB area as-is
> whereas the controller uses syndrome page layout.
> [2] ONFi devices are not working
> [3] It can not read Bad Block Marker
> 
> Outstanding changes are:
>  - Fix raw/oob callbacks for syndrome page layout
>  - Implement setup_data_interface() callback
>  - Fix/implement more commands for ONFi devices
>  - Allow to skip the driver internal bounce buffer
>  - Support PIO in case DMA is not supported
>  - Switch from ->cmdfunc over to ->cmd_ctrl
> 
> 18 patches were merged by v2.
> 11 patches were merged by v3.
> 2 patches were merged by v4.
> 5 patches were merged by v5.
> Here is the rest of the series.
> 
> v1: https://lkml.org/lkml/2016/11/26/144
> v2: https://lkml.org/lkml/2017/3/22/804
> v3: https://lkml.org/lkml/2017/3/30/90
> v4: https://lkml.org/lkml/2017/6/5/1005
> 
> Masahiro Yamada (18):
>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
>   mtd: nand: denali: remove unneeded find_valid_banks()
>   mtd: nand: denali: handle timing parameters by setup_data_interface()
>   mtd: nand: denali: rework interrupt handling
>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>   mtd: nand: denali: fix NAND_CMD_PARAM handling

AFAICT, patch 5 and 6 are unneeded...

>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc

... because you're anyway switching to ->cmd_ctrl() in patch 7, which
fixes the problem you were addressing in patch 5 and 6.

Please squash those 3 patches into a single one and adjust your commit
message accordingly (explaining that it fixes STATUS and PARAM handling).
See below if you need an example.

BTW, I also implemented ->read/write_buf_word() since the core may one
day call these functions, and the default implementations used by the
core when these hooks are NULL are not appropriate in your case.

--->8---
>From 136727ba7b453ca1567c711037230aa6ec0f124a Mon Sep 17 00:00:00 2001
From: Masahiro Yamada 
Date: Tue, 13 Jun 2017 14:03:57 +0900
Subject: [PATCH] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc

The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().

Besides, we see /* TODO: Read OOB data */ comment line.

It would be possible to add more commands along with the current
implementation, but having ->cmd_ctrl() seems a better approach from
the discussion with Boris [1].

Rely on the default ->cmdfunc() from the framework and implement the
driver's own ->cmd_ctrl(). We are also implementing
->read/write_buf/byte/word().

Also add ->write_byte(), which is needed for write direction commands.

Then, we can drop nand_onfi_get_set_features_notsupp from this driver.

This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
NAND_CMD_STATUS was just faked by the implementation, and the only valid
bit returned in this case was the WP bit.
NAND_CMD_PARAM was completely broken: not only the command sent on the
bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM, but the driver was
only reading 8 bytes of data, while the parameter page is contains
several hundreds of bytes.

[1] https://lkml.org/lkml/2017/3/15/97

Signed-off-by: Masahiro Yamada 
Signed-off-by: Boris Brezillon 
---
 drivers/mtd/nand/denali.c | 204 +-
 drivers/mtd/nand/denali.h |   2 -
 2 files changed, 95 insertions(+), 111 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index d7e7555a3d73..633faf2da1f4 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -85,43 +85,6 @@ static void index_addr(struct denali_nand_info *denali,
iowrite32(data, denali->flash_mem + 0x10);
 }
 
-/* Perform an indexed read of the device */
-static void index_addr_read_data(struct denali_nand_info *denali,
-uint32_t address, uint32_t *pdata)
-{
-   iowrite32(address, denali->flash_mem);
-   *pdata = ioread32(denali->flash_mem + 0x10);
-}
-
-/*
- * We need to buffer some data for some of the NAND core routines.
- * The operations manage buffering that data.
- */
-static void reset_buf(struct denali_nand_info *denali)
-{
-   denali->buf.head = denali->buf.tail = 0;
-}
-
-static void write_byte_to_buf(struct denali_nand_info *denali, uint8_t byte)
-{
-   denali->buf.buf[denali->buf.tail++] = byte;
-}
-
-/* reads the status of the device */
-static void read_status(struct denali_nand_info *denali)
-{
-   uint32_t cmd;
-
-   /* initialize the data buffer to store status */
-   reset_buf(denali);
-
-   cmd = ioread32(denali->flash_reg + WRITE_PROTECT);
-   if (cmd)
-   write_byte_to_buf(denali, NAND_STATUS_WP);
-   else
-   write_byte_to_buf(denali, 0);
-}
-
 /* Reset the flash controller */
 

Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Boris Brezillon]

Le Tue, 13 Jun 2017 14:03:52 +0900,
Masahiro Yamada  a écrit :

> This patch series intends to solve various problems.
> 
> [1] The driver just retrieves the OOB area as-is
> whereas the controller uses syndrome page layout.
> [2] ONFi devices are not working
> [3] It can not read Bad Block Marker
> 
> Outstanding changes are:
>  - Fix raw/oob callbacks for syndrome page layout
>  - Implement setup_data_interface() callback
>  - Fix/implement more commands for ONFi devices
>  - Allow to skip the driver internal bounce buffer
>  - Support PIO in case DMA is not supported
>  - Switch from ->cmdfunc over to ->cmd_ctrl
> 
> 18 patches were merged by v2.
> 11 patches were merged by v3.
> 2 patches were merged by v4.
> 5 patches were merged by v5.
> Here is the rest of the series.
> 
> v1: https://lkml.org/lkml/2016/11/26/144
> v2: https://lkml.org/lkml/2017/3/22/804
> v3: https://lkml.org/lkml/2017/3/30/90
> v4: https://lkml.org/lkml/2017/6/5/1005
> 
> Masahiro Yamada (18):
>   mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
>   mtd: nand: denali: remove unneeded find_valid_banks()
>   mtd: nand: denali: handle timing parameters by setup_data_interface()
>   mtd: nand: denali: rework interrupt handling
>   mtd: nand: denali: fix NAND_CMD_STATUS handling
>   mtd: nand: denali: fix NAND_CMD_PARAM handling

AFAICT, patch 5 and 6 are unneeded...

>   mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc

... because you're anyway switching to ->cmd_ctrl() in patch 7, which
fixes the problem you were addressing in patch 5 and 6.

Please squash those 3 patches into a single one and adjust your commit
message accordingly (explaining that it fixes STATUS and PARAM handling).
See below if you need an example.

BTW, I also implemented ->read/write_buf_word() since the core may one
day call these functions, and the default implementations used by the
core when these hooks are NULL are not appropriate in your case.

--->8---
>From 136727ba7b453ca1567c711037230aa6ec0f124a Mon Sep 17 00:00:00 2001
From: Masahiro Yamada 
Date: Tue, 13 Jun 2017 14:03:57 +0900
Subject: [PATCH] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc

The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().

Besides, we see /* TODO: Read OOB data */ comment line.

It would be possible to add more commands along with the current
implementation, but having ->cmd_ctrl() seems a better approach from
the discussion with Boris [1].

Rely on the default ->cmdfunc() from the framework and implement the
driver's own ->cmd_ctrl(). We are also implementing
->read/write_buf/byte/word().

Also add ->write_byte(), which is needed for write direction commands.

Then, we can drop nand_onfi_get_set_features_notsupp from this driver.

This transition also fixes NAND_CMD_STATUS and NAND_CMD_PARAM handling.
NAND_CMD_STATUS was just faked by the implementation, and the only valid
bit returned in this case was the WP bit.
NAND_CMD_PARAM was completely broken: not only the command sent on the
bus was NAND_CMD_STATUS instead of NAND_CMD_PARAM, but the driver was
only reading 8 bytes of data, while the parameter page is contains
several hundreds of bytes.

[1] https://lkml.org/lkml/2017/3/15/97

Signed-off-by: Masahiro Yamada 
Signed-off-by: Boris Brezillon 
---
 drivers/mtd/nand/denali.c | 204 +-
 drivers/mtd/nand/denali.h |   2 -
 2 files changed, 95 insertions(+), 111 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index d7e7555a3d73..633faf2da1f4 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -85,43 +85,6 @@ static void index_addr(struct denali_nand_info *denali,
iowrite32(data, denali->flash_mem + 0x10);
 }
 
-/* Perform an indexed read of the device */
-static void index_addr_read_data(struct denali_nand_info *denali,
-uint32_t address, uint32_t *pdata)
-{
-   iowrite32(address, denali->flash_mem);
-   *pdata = ioread32(denali->flash_mem + 0x10);
-}
-
-/*
- * We need to buffer some data for some of the NAND core routines.
- * The operations manage buffering that data.
- */
-static void reset_buf(struct denali_nand_info *denali)
-{
-   denali->buf.head = denali->buf.tail = 0;
-}
-
-static void write_byte_to_buf(struct denali_nand_info *denali, uint8_t byte)
-{
-   denali->buf.buf[denali->buf.tail++] = byte;
-}
-
-/* reads the status of the device */
-static void read_status(struct denali_nand_info *denali)
-{
-   uint32_t cmd;
-
-   /* initialize the data buffer to store status */
-   reset_buf(denali);
-
-   cmd = ioread32(denali->flash_reg + WRITE_PROTECT);
-   if (cmd)
-   write_byte_to_buf(denali, NAND_STATUS_WP);
-   else
-   write_byte_to_buf(denali, 0);
-}
-
 /* Reset the flash controller */
 static uint16_t denali_nand_reset(struct denali_nand_info *denali)
 {
@@ -267,20 +230,16 @@ static uint32_t 

[PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

This patch series intends to solve various problems.

[1] The driver just retrieves the OOB area as-is
whereas the controller uses syndrome page layout.
[2] ONFi devices are not working
[3] It can not read Bad Block Marker

Outstanding changes are:
 - Fix raw/oob callbacks for syndrome page layout
 - Implement setup_data_interface() callback
 - Fix/implement more commands for ONFi devices
 - Allow to skip the driver internal bounce buffer
 - Support PIO in case DMA is not supported
 - Switch from ->cmdfunc over to ->cmd_ctrl

18 patches were merged by v2.
11 patches were merged by v3.
2 patches were merged by v4.
5 patches were merged by v5.
Here is the rest of the series.

v1: https://lkml.org/lkml/2016/11/26/144
v2: https://lkml.org/lkml/2017/3/22/804
v3: https://lkml.org/lkml/2017/3/30/90
v4: https://lkml.org/lkml/2017/6/5/1005

Masahiro Yamada (18):
  mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
  mtd: nand: denali: remove unneeded find_valid_banks()
  mtd: nand: denali: handle timing parameters by setup_data_interface()
  mtd: nand: denali: rework interrupt handling
  mtd: nand: denali: fix NAND_CMD_STATUS handling
  mtd: nand: denali: fix NAND_CMD_PARAM handling
  mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
  mtd: nand: denali: fix bank reset function to detect the number of
chips
  mtd: nand: denali: use interrupt instead of polling for bank reset
  mtd: nand: denali: propagate page to helpers via function argument
  mtd: nand: denali: merge struct nand_buf into struct denali_nand_info
  mtd: nand: denali: use flag instead of register macro for direction
  mtd: nand: denali: fix raw and oob accessors for syndrome page layout
  mtd: nand: denali: support hardware-assisted erased page detection
  mtd: nand: denali: skip driver internal bounce buffer when possible
  mtd: nand: denali: use non-managed kmalloc() for DMA buffer
  mtd: nand: denali: enable bad block table scan
  mtd: nand: denali: avoid magic numbers and rename for clarification

 drivers/mtd/nand/denali.c | 1715 ++---
 drivers/mtd/nand/denali.h |   63 +-
 drivers/mtd/nand/denali_dt.c  |   15 +-
 drivers/mtd/nand/denali_pci.c |   22 +-
 4 files changed, 800 insertions(+), 1015 deletions(-)

-- 
2.7.4

[PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb

[Masahiro Yamada]

This patch series intends to solve various problems.

[1] The driver just retrieves the OOB area as-is
whereas the controller uses syndrome page layout.
[2] ONFi devices are not working
[3] It can not read Bad Block Marker

Outstanding changes are:
 - Fix raw/oob callbacks for syndrome page layout
 - Implement setup_data_interface() callback
 - Fix/implement more commands for ONFi devices
 - Allow to skip the driver internal bounce buffer
 - Support PIO in case DMA is not supported
 - Switch from ->cmdfunc over to ->cmd_ctrl

18 patches were merged by v2.
11 patches were merged by v3.
2 patches were merged by v4.
5 patches were merged by v5.
Here is the rest of the series.

v1: https://lkml.org/lkml/2016/11/26/144
v2: https://lkml.org/lkml/2017/3/22/804
v3: https://lkml.org/lkml/2017/3/30/90
v4: https://lkml.org/lkml/2017/6/5/1005

Masahiro Yamada (18):
  mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
  mtd: nand: denali: remove unneeded find_valid_banks()
  mtd: nand: denali: handle timing parameters by setup_data_interface()
  mtd: nand: denali: rework interrupt handling
  mtd: nand: denali: fix NAND_CMD_STATUS handling
  mtd: nand: denali: fix NAND_CMD_PARAM handling
  mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
  mtd: nand: denali: fix bank reset function to detect the number of
chips
  mtd: nand: denali: use interrupt instead of polling for bank reset
  mtd: nand: denali: propagate page to helpers via function argument
  mtd: nand: denali: merge struct nand_buf into struct denali_nand_info
  mtd: nand: denali: use flag instead of register macro for direction
  mtd: nand: denali: fix raw and oob accessors for syndrome page layout
  mtd: nand: denali: support hardware-assisted erased page detection
  mtd: nand: denali: skip driver internal bounce buffer when possible
  mtd: nand: denali: use non-managed kmalloc() for DMA buffer
  mtd: nand: denali: enable bad block table scan
  mtd: nand: denali: avoid magic numbers and rename for clarification

 drivers/mtd/nand/denali.c | 1715 ++---
 drivers/mtd/nand/denali.h |   63 +-
 drivers/mtd/nand/denali_dt.c  |   15 +-
 drivers/mtd/nand/denali_pci.c |   22 +-
 4 files changed, 800 insertions(+), 1015 deletions(-)

-- 
2.7.4