Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1664416 > unrolled thread

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

Started byMasahiro Yamada <yamada.masahiro@socionext.com>
First post2017-06-13 07:10 +0200
Last post2017-06-13 11:00 +0200
Articles 5 on this page of 25 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 02/18] mtd: nand: denali: remove unneeded find_valid_banks() Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 08/18] mtd: nand: denali: fix bank reset function to detect the number of chips Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 01/18] mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 16/18] mtd: nand: denali: use non-managed kmalloc() for DMA buffer Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 09/18] mtd: nand: denali: use interrupt instead of polling for bank reset Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 07/18] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 05/18] mtd: nand: denali: fix NAND_CMD_STATUS handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 18/18] mtd: nand: denali: avoid magic numbers and rename for clarification Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 15/18] mtd: nand: denali: skip driver internal bounce buffer when possible Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 03/18] mtd: nand: denali: handle timing parameters by setup_data_interface() Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 06/18] mtd: nand: denali: fix NAND_CMD_PARAM handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 13/18] mtd: nand: denali: fix raw and oob accessors for syndrome page layout Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 17/18] mtd: nand: denali: enable bad block table scan Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 11/18] mtd: nand: denali: merge struct nand_buf into struct denali_nand_info Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 14/18] mtd: nand: denali: support hardware-assisted erased page detection Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:10 +0200
    [PATCH v6 12/18] mtd: nand: denali: use flag instead of register macro for direction Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:20 +0200
    [PATCH v6 04/18] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:20 +0200
    [PATCH v6 10/18] mtd: nand: denali: propagate page to helpers via function argument Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 07:20 +0200
    Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-13 09:10 +0200
      Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 10:10 +0200
        Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-13 10:30 +0200
      Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 10:20 +0200
        Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-13 10:40 +0200
          Re: [PATCH v6 00/18] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 11:00 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1664581

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-13 10:10 +0200
Message-ID<tRP3H-5vi-5@gated-at.bofh.it>
In reply to#1664523
Hi Boris,


2017-06-13 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Le Tue, 13 Jun 2017 14:03:52 +0900,
> Masahiro Yamada <yamada.masahiro@socionext.com> 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 <yamada.masahiro@socionext.com>
> 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

[toc] | [prev] | [next] | [standalone]

#1664589

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-06-13 10:30 +0200
Message-ID<tRPn3-5Bu-1@gated-at.bofh.it>
In reply to#1664581
On Tue, 13 Jun 2017 17:04:11 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 2017-06-13 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > Le Tue, 13 Jun 2017 14:03:52 +0900,
> > Masahiro Yamada <yamada.masahiro@socionext.com> 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 <yamada.masahiro@socionext.com>
> > 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.

[toc] | [prev] | [next] | [standalone]

#1664585

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-13 10:20 +0200
Message-ID<tRPdn-5yt-3@gated-at.bofh.it>
In reply to#1664523
Hi Boris,


2017-06-13 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:

>
> 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, &byte, 1);
        return byte;
}


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




-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1664597

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-06-13 10:40 +0200
Message-ID<tRPwK-5EB-11@gated-at.bofh.it>
In reply to#1664585
On Tue, 13 Jun 2017 17:17:41 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 2017-06-13 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> 
> >
> > 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, &byte, 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.

[toc] | [prev] | [next] | [standalone]

#1664611

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-13 11:00 +0200
Message-ID<tRPQ6-5MF-13@gated-at.bofh.it>
In reply to#1664597
Hi Boris,

2017-06-13 17:30 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Tue, 13 Jun 2017 17:17:41 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>>
>> 2017-06-13 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>>
>> >
>> > 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, &byte, 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

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web