Mailing List Archive: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

[PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb

Nov 26, 2016, 10:20 AM

Post #1 of 9 (175 views)

As I said in the 1st round series, I am tackling on this driver
to use it for my SoCs.

The previous series was just cosmetic things, but this series
includes *real* changes.

After some more cleanups, I will start to add changes that
are really necessary.
One of the biggest problems I want to solve is a bunch of
hard-coded parameters that prevent me from using this driver for
my SoCs.

I will introduce capability flags that are associated with DT
compatible and make platform-dependent parameters overridable.

I still have lots of reworks to get done (so probably 3rd round
series will come), but I hope it is getting better and
I am showing a big picture now.

Masahiro Yamada (39):
mtd: nand: allow to set only one of ECC size and ECC strength from DT
mtd: nand: denali: remove unused CONFIG option and macros
mtd: nand: denali: remove redundant define of BANK(x)
mtd: nand: denali: remove more unused struct members
mtd: nand: denali: fix comment of denali_nand_info::flash_mem
mtd: nand: denali: fix write_oob_data() function
mtd: nand: denali: transfer OOB only when oob_required is set
mtd: nand: denali: introduce capability flag
mtd: nand: denali: fix erased page check code
mtd: nand: denali: remove redundant if conditional of erased_check
mtd: nand: denali: increment ecc_stats.failed by one per error
mtd: nand: denali: return 0 for uncorrectable ECC error
mtd: nand: denali: increment ecc_stats->corrected
mtd: nand: denali: replace uint{8/16/32}_t with u{8/16/32}
mtd: nand: denali: improve readability of handle_ecc()
mtd: nand: denali: rename handle_ecc() to denali_sw_ecc_fixup()
mtd: nand: denali: support HW_ECC_FIXUP capability
mtd: nand: denali: move denali_read_page_raw() above
denali_read_page()
mtd: nand: denali: perform erased check against raw transferred page
mtd: nand: denali_dt: enable HW_ECC_FIXUP capability for DT platform
mtd: nand: denali: support 64bit capable DMA engine
mtd: nand: denali_dt: remove dma-mask DT property
mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
mtd: nand: denali: add NEW_N_BANKS_FORMAT capability
mtd: nand: denali: use nand_chip to hold frequently accessed data
mtd: nand: denali: call nand_set_flash_node() to set DT node
mtd: nand: denali: do not set mtd->name
mtd: nand: denali: move multi NAND fixup code to a helper function
mtd: nand: denali: refactor multi NAND fixup code in more generic way
mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
mtd: nand: denali: remove meaningless writes to read-only registers
mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
mtd: nand: denali: support 1024 byte ECC step size
mtd: nand: denali: fix the condition for 15 bit ECC strength
mtd: nand: denali: calculate ecc.strength and ecc.bytes generically
mtd: nand: denali: allow to use SoC-specific ECC strength
mtd: nand: denali: support "nand-ecc-strength" DT property
mtd: nand: denali: remove Toshiba, Hynix specific fixup code
mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants

.../devicetree/bindings/mtd/denali-nand.txt | 19 +-
drivers/mtd/nand/Kconfig | 11 -
drivers/mtd/nand/denali.c | 740 ++++++++++++---------
drivers/mtd/nand/denali.h | 84 +--
drivers/mtd/nand/denali_dt.c | 95 ++-
drivers/mtd/nand/denali_pci.c | 2 +
drivers/mtd/nand/nand_base.c | 6 -
7 files changed, 515 insertions(+), 442 deletions(-)

--
2.7.4

Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb [ In reply to ]

boris.brezillon at free-electrons

Nov 27, 2016, 8:20 AM

Post #2 of 9 (170 views)

Permalink

+Andy

Hi Masahiro,

On Sun, 27 Nov 2016 03:05:46 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> As I said in the 1st round series, I am tackling on this driver
> to use it for my SoCs.
>
> The previous series was just cosmetic things, but this series
> includes *real* changes.
>
> After some more cleanups, I will start to add changes that
> are really necessary.
> One of the biggest problems I want to solve is a bunch of
> hard-coded parameters that prevent me from using this driver for
> my SoCs.
>
> I will introduce capability flags that are associated with DT
> compatible and make platform-dependent parameters overridable.
>
> I still have lots of reworks to get done (so probably 3rd round
> series will come), but I hope it is getting better and
> I am showing a big picture now.
>

Thanks for posting this 2nd round of patches, I know have a clearer
view of what you're trying to achieve.
Could you be a bit more specific about the remaining rework (your 3rd
round)?

Also, if you don't mind, I'd like to have reviews and testing from intel
users before applying the series. Can you Cc Andy (and possibly other
intel maintainers) for the next round.

Thanks,

Boris

>
>
> Masahiro Yamada (39):
> mtd: nand: allow to set only one of ECC size and ECC strength from DT
> mtd: nand: denali: remove unused CONFIG option and macros
> mtd: nand: denali: remove redundant define of BANK(x)
> mtd: nand: denali: remove more unused struct members
> mtd: nand: denali: fix comment of denali_nand_info::flash_mem
> mtd: nand: denali: fix write_oob_data() function
> mtd: nand: denali: transfer OOB only when oob_required is set
> mtd: nand: denali: introduce capability flag
> mtd: nand: denali: fix erased page check code
> mtd: nand: denali: remove redundant if conditional of erased_check
> mtd: nand: denali: increment ecc_stats.failed by one per error
> mtd: nand: denali: return 0 for uncorrectable ECC error
> mtd: nand: denali: increment ecc_stats->corrected
> mtd: nand: denali: replace uint{8/16/32}_t with u{8/16/32}
> mtd: nand: denali: improve readability of handle_ecc()
> mtd: nand: denali: rename handle_ecc() to denali_sw_ecc_fixup()
> mtd: nand: denali: support HW_ECC_FIXUP capability
> mtd: nand: denali: move denali_read_page_raw() above
> denali_read_page()
> mtd: nand: denali: perform erased check against raw transferred page
> mtd: nand: denali_dt: enable HW_ECC_FIXUP capability for DT platform
> mtd: nand: denali: support 64bit capable DMA engine
> mtd: nand: denali_dt: remove dma-mask DT property
> mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
> mtd: nand: denali: add NEW_N_BANKS_FORMAT capability
> mtd: nand: denali: use nand_chip to hold frequently accessed data
> mtd: nand: denali: call nand_set_flash_node() to set DT node
> mtd: nand: denali: do not set mtd->name
> mtd: nand: denali: move multi NAND fixup code to a helper function
> mtd: nand: denali: refactor multi NAND fixup code in more generic way
> mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
> mtd: nand: denali: remove meaningless writes to read-only registers
> mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
> mtd: nand: denali: support 1024 byte ECC step size
> mtd: nand: denali: fix the condition for 15 bit ECC strength
> mtd: nand: denali: calculate ecc.strength and ecc.bytes generically
> mtd: nand: denali: allow to use SoC-specific ECC strength
> mtd: nand: denali: support "nand-ecc-strength" DT property
> mtd: nand: denali: remove Toshiba, Hynix specific fixup code
> mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
>
> .../devicetree/bindings/mtd/denali-nand.txt | 19 +-
> drivers/mtd/nand/Kconfig | 11 -
> drivers/mtd/nand/denali.c | 740 ++++++++++++---------
> drivers/mtd/nand/denali.h | 84 +--
> drivers/mtd/nand/denali_dt.c | 95 ++-
> drivers/mtd/nand/denali_pci.c | 2 +
> drivers/mtd/nand/nand_base.c | 6 -
> 7 files changed, 515 insertions(+), 442 deletions(-)
>

Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb [ In reply to ]

boris.brezillon at free-electrons

Nov 27, 2016, 10:20 AM

Post #3 of 9 (169 views)

Permalink

On Sun, 27 Nov 2016 03:05:46 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> As I said in the 1st round series, I am tackling on this driver
> to use it for my SoCs.
>
> The previous series was just cosmetic things, but this series
> includes *real* changes.
>
> After some more cleanups, I will start to add changes that
> are really necessary.
> One of the biggest problems I want to solve is a bunch of
> hard-coded parameters that prevent me from using this driver for
> my SoCs.
>
> I will introduce capability flags that are associated with DT
> compatible and make platform-dependent parameters overridable.
>
> I still have lots of reworks to get done (so probably 3rd round
> series will come), but I hope it is getting better and
> I am showing a big picture now.
>

I still need to carefully review some of those patches, but I must
admit I like some of the cleanups/rework you're doing here.

Thanks for all your work.

Boris

>
>
> Masahiro Yamada (39):
> mtd: nand: allow to set only one of ECC size and ECC strength from DT
> mtd: nand: denali: remove unused CONFIG option and macros
> mtd: nand: denali: remove redundant define of BANK(x)
> mtd: nand: denali: remove more unused struct members
> mtd: nand: denali: fix comment of denali_nand_info::flash_mem
> mtd: nand: denali: fix write_oob_data() function
> mtd: nand: denali: transfer OOB only when oob_required is set
> mtd: nand: denali: introduce capability flag
> mtd: nand: denali: fix erased page check code
> mtd: nand: denali: remove redundant if conditional of erased_check
> mtd: nand: denali: increment ecc_stats.failed by one per error
> mtd: nand: denali: return 0 for uncorrectable ECC error
> mtd: nand: denali: increment ecc_stats->corrected
> mtd: nand: denali: replace uint{8/16/32}_t with u{8/16/32}
> mtd: nand: denali: improve readability of handle_ecc()
> mtd: nand: denali: rename handle_ecc() to denali_sw_ecc_fixup()
> mtd: nand: denali: support HW_ECC_FIXUP capability
> mtd: nand: denali: move denali_read_page_raw() above
> denali_read_page()
> mtd: nand: denali: perform erased check against raw transferred page
> mtd: nand: denali_dt: enable HW_ECC_FIXUP capability for DT platform
> mtd: nand: denali: support 64bit capable DMA engine
> mtd: nand: denali_dt: remove dma-mask DT property
> mtd: nand: denali_dt: use pdev instead of ofdev for platform_device
> mtd: nand: denali: add NEW_N_BANKS_FORMAT capability
> mtd: nand: denali: use nand_chip to hold frequently accessed data
> mtd: nand: denali: call nand_set_flash_node() to set DT node
> mtd: nand: denali: do not set mtd->name
> mtd: nand: denali: move multi NAND fixup code to a helper function
> mtd: nand: denali: refactor multi NAND fixup code in more generic way
> mtd: nand: denali: set DEVICES_CONNECTED 1 if not set
> mtd: nand: denali: remove meaningless writes to read-only registers
> mtd: nand: denali: remove unnecessary writes to ECC_CORRECTION
> mtd: nand: denali: support 1024 byte ECC step size
> mtd: nand: denali: fix the condition for 15 bit ECC strength
> mtd: nand: denali: calculate ecc.strength and ecc.bytes generically
> mtd: nand: denali: allow to use SoC-specific ECC strength
> mtd: nand: denali: support "nand-ecc-strength" DT property
> mtd: nand: denali: remove Toshiba, Hynix specific fixup code
> mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
>
> .../devicetree/bindings/mtd/denali-nand.txt | 19 +-
> drivers/mtd/nand/Kconfig | 11 -
> drivers/mtd/nand/denali.c | 740 ++++++++++++---------
> drivers/mtd/nand/denali.h | 84 +--
> drivers/mtd/nand/denali_dt.c | 95 ++-
> drivers/mtd/nand/denali_pci.c | 2 +
> drivers/mtd/nand/nand_base.c | 6 -
> 7 files changed, 515 insertions(+), 442 deletions(-)
>

Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb [ In reply to ]

yamada.masahiro at socionext

Nov 30, 2016, 12:21 AM

Post #4 of 9 (164 views)

Permalink

Hi.

2016-11-28 0:04 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> +Andy
>
> Hi Masahiro,
>
> On Sun, 27 Nov 2016 03:05:46 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> As I said in the 1st round series, I am tackling on this driver
>> to use it for my SoCs.
>>
>> The previous series was just cosmetic things, but this series
>> includes *real* changes.
>>
>> After some more cleanups, I will start to add changes that
>> are really necessary.
>> One of the biggest problems I want to solve is a bunch of
>> hard-coded parameters that prevent me from using this driver for
>> my SoCs.
>>
>> I will introduce capability flags that are associated with DT
>> compatible and make platform-dependent parameters overridable.
>>
>> I still have lots of reworks to get done (so probably 3rd round
>> series will come), but I hope it is getting better and
>> I am showing a big picture now.
>>
>
> Thanks for posting this 2nd round of patches, I know have a clearer
> view of what you're trying to achieve.
> Could you be a bit more specific about the remaining rework (your 3rd
> round)?

[1]
I want to remove
get_samsung_nand_para()
get_onfi_nand_para()

The driver should not hard-code timing parameters of Samsung specific
chips. For ONFI, it is duplicating effort of the core framework.

I am thinking if it would be possible to implement
chip->setup_data_interface() in order to set up
timings in a generic way.

[2]
Remove driver-internal bounce buffer.
The current Denali driver allocate DMA_BIDIRECTIONAL buffer
to use it as a driver-internal bounce buffer.

The hardware transfer page data into the bounce buffer,
then CPU copies from the bounce buffer to a given buf (and oob_poi).
This is not efficient.

So, I want to set NAND_USE_BOUNCE_BUFFER flag
and do dma_map_single directly for a given buffer.

[3]
Fix raw and oob callbacks.

I asked in another thread,
the current driver just puts the physically accessed OOB data
into oob_poi, which is not a collection of ECC data.
Raw write/read() are wrong as well.

After fixing those, enable BBT scan by removing the following flag:
/* skip the scan for now until we have OOB read and write support */
chip->options |= NAND_SKIP_BBTSCAN;

> Also, if you don't mind, I'd like to have reviews and testing from intel
> users before applying the series. Can you Cc Andy (and possibly other
> intel maintainers) for the next round.

Sure.

Anyway, this series already missed the pull-req for 4.10-rc1,
we have plenty of time until 4.11-rc1.

Review/test from Intel engineers are very appreciated
because I have no access to their boards.

--
Best Regards
Masahiro Yamada

Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb [ In reply to ]

yamada.masahiro at socionext

Nov 30, 2016, 12:21 AM

Post #5 of 9 (164 views)

Permalink

2016-11-28 0:04 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> +Andy
>
> Hi Masahiro,
>
> On Sun, 27 Nov 2016 03:05:46 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> As I said in the 1st round series, I am tackling on this driver
>> to use it for my SoCs.
>>
>> The previous series was just cosmetic things, but this series
>> includes *real* changes.
>>
>> After some more cleanups, I will start to add changes that
>> are really necessary.
>> One of the biggest problems I want to solve is a bunch of
>> hard-coded parameters that prevent me from using this driver for
>> my SoCs.
>>
>> I will introduce capability flags that are associated with DT
>> compatible and make platform-dependent parameters overridable.
>>
>> I still have lots of reworks to get done (so probably 3rd round
>> series will come), but I hope it is getting better and
>> I am showing a big picture now.
>>
>
> Thanks for posting this 2nd round of patches, I know have a clearer
> view of what you're trying to achieve.
> Could you be a bit more specific about the remaining rework (your 3rd
> round)?

We still have plenty of time, and no reason to hurry now.
So, you do not have to apply this series
until you see an even bigger picture.

I will try my best to include as many of my plans as possible
in this round.

I may end up with dropping my on-going work to the ML occasionally
because I want early feedback in case I am doing something wrong.

--
Best Regards
Masahiro Yamada

Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb [ In reply to ]

boris.brezillon at free-electrons

Nov 30, 2016, 12:21 AM

Post #6 of 9 (164 views)

Permalink

On Wed, 30 Nov 2016 17:02:16 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi.
>
> 2016-11-28 0:04 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > +Andy
> >
> > Hi Masahiro,
> >
> > On Sun, 27 Nov 2016 03:05:46 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> As I said in the 1st round series, I am tackling on this driver
> >> to use it for my SoCs.
> >>
> >> The previous series was just cosmetic things, but this series
> >> includes *real* changes.
> >>
> >> After some more cleanups, I will start to add changes that
> >> are really necessary.
> >> One of the biggest problems I want to solve is a bunch of
> >> hard-coded parameters that prevent me from using this driver for
> >> my SoCs.
> >>
> >> I will introduce capability flags that are associated with DT
> >> compatible and make platform-dependent parameters overridable.
> >>
> >> I still have lots of reworks to get done (so probably 3rd round
> >> series will come), but I hope it is getting better and
> >> I am showing a big picture now.
> >>
> >
> > Thanks for posting this 2nd round of patches, I know have a clearer
> > view of what you're trying to achieve.
> > Could you be a bit more specific about the remaining rework (your 3rd
> > round)?
>
>
> [1]
> I want to remove
> get_samsung_nand_para()
> get_onfi_nand_para()
>
> The driver should not hard-code timing parameters of Samsung specific
> chips. For ONFI, it is duplicating effort of the core framework.

Definitely.

>
> I am thinking if it would be possible to implement
> chip->setup_data_interface() in order to set up
> timings in a generic way.

Indeed, and that'd be really cool to have this driver converted to this
new interface.

>
> [2]
> Remove driver-internal bounce buffer.
> The current Denali driver allocate DMA_BIDIRECTIONAL buffer
> to use it as a driver-internal bounce buffer.
>
> The hardware transfer page data into the bounce buffer,
> then CPU copies from the bounce buffer to a given buf (and oob_poi).
> This is not efficient.
>
> So, I want to set NAND_USE_BOUNCE_BUFFER flag
> and do dma_map_single directly for a given buffer.

Sounds good. Be careful though, when you use the generic bounce buffer
interface you might have to clear the page cache info (->pagebuf = -1).

>
> [3]
> Fix raw and oob callbacks.
>
> I asked in another thread,
> the current driver just puts the physically accessed OOB data
> into oob_poi, which is not a collection of ECC data.
> Raw write/read() are wrong as well.

That's all good things too.

>
> After fixing those, enable BBT scan by removing the following flag:
> /* skip the scan for now until we have OOB read and write support */
> chip->options |= NAND_SKIP_BBTSCAN;
>

Hm, here you have a problem. The layout you described replaces BBMs by
payload data, thus preventing the BBM scan approach (or at least, it
won't work with factory BBMs).

Some drivers/controllers have an extra 'switch BBM/data bytes' step to
restore the BBM at the correct place before flushing the data to the
NAND or after reading a page, but I'm not sure this is the case here.

>
>
> > Also, if you don't mind, I'd like to have reviews and testing from intel
> > users before applying the series. Can you Cc Andy (and possibly other
> > intel maintainers) for the next round.
>
> Sure.
>
> Anyway, this series already missed the pull-req for 4.10-rc1,
> we have plenty of time until 4.11-rc1.
>
> Review/test from Intel engineers are very appreciated
> because I have no access to their boards.
>
>

Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb [ In reply to ]

yamada.masahiro at socionext

Dec 1, 2016, 2:22 AM

Post #7 of 9 (159 views)

Permalink

Hi Boris,

2016-11-30 17:17 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:

>> [3]
>> Fix raw and oob callbacks.
>>
>> I asked in another thread,
>> the current driver just puts the physically accessed OOB data
>> into oob_poi, which is not a collection of ECC data.
>> Raw write/read() are wrong as well.
>
> That's all good things too.
>
>>
>> After fixing those, enable BBT scan by removing the following flag:
>> /* skip the scan for now until we have OOB read and write support */
>> chip->options |= NAND_SKIP_BBTSCAN;
>>
>
> Hm, here you have a problem. The layout you described replaces BBMs by
> payload data, thus preventing the BBM scan approach (or at least, it
> won't work with factory BBMs).

As I answered in another mail, the Denali IP expects BBMs
at the beginning of each OOB area (standard location).
They are protected from the ECC engine.

I just did not mention the BBM-reserved area
to make the story simpler.

So, after fixing oob read/write functions,
the driver will be able to enable BBT-scanning.

--
Best Regards
Masahiro Yamada

Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb [ In reply to ]

yamada.masahiro at socionext

Mar 10, 2017, 4:20 AM

Post #8 of 9 (141 views)

Permalink

Hi Boris,

I am almost getting v2 done,
and now I am testing it.

I am having one problem. Please teach me.

2016-11-30 17:17 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> [2]
>> Remove driver-internal bounce buffer.
>> The current Denali driver allocate DMA_BIDIRECTIONAL buffer
>> to use it as a driver-internal bounce buffer.
>>
>> The hardware transfer page data into the bounce buffer,
>> then CPU copies from the bounce buffer to a given buf (and oob_poi).
>> This is not efficient.
>>
>> So, I want to set NAND_USE_BOUNCE_BUFFER flag
>> and do dma_map_single directly for a given buffer.
>
> Sounds good. Be careful though, when you use the generic bounce buffer
> interface you might have to clear the page cache info (->pagebuf = -1).

Instead of memcpy() of the whole page,
I am trying to use dma_map_single() in ecc->read_page() / ecc->write_page().
This will allow direct transfer between the buffer and the device by DMA.

But, this does not work for Denali if use_bufpoi is set in nand_do_read_ops().

In the following code in nand_scan_tail(),

if (!(chip->options & NAND_OWN_BUFFERS)) {
nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
+ mtd->oobsize * 3, GFP_KERNEL);
if (!nbuf)
return -ENOMEM;
nbuf->ecccalc = (uint8_t *)(nbuf + 1);
nbuf->ecccode = nbuf->ecccalc + mtd->oobsize;
nbuf->databuf = nbuf->ecccode + mtd->oobsize;

chip->buffers = nbuf;

chip->buffers->databuf has no guarantee for DMA'able alignment.
(actually it has unwanted offset 0xc because sizeof(*nbuf) == 0xc on
32bit systems)

If we could change the code as follows,

nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
nbuf->ecccode = kmalloc(mtd->oobsize, GFP_KERNEL);
nbuf->databuf = kmalloc(mtd->writesize + mtd->oobsize,
GFP_KERNEL);

chip->buffers->databuf would have DMA'able alignment in most cases
without NAND_OWN_BUFFERS. (but, I am not sure if this is a good idea)

So, the idea of NAND_OWN_BUFFERS is that
drivers should allocate own buffers if they need to perform DMA-mapping
in read_page(), write_page(), right?

However, "git grep NAND_OWN_BUFFERS" shows
cafe_nand.c is the only driver that does so.

On the other hand, "git grep dma_map_single" has more hits,
i.e. some drivers perform dma_map_single() for read/write without
NAND_OWN_BUFFERS.

I have no idea how they are working.

--
Best Regards
Masahiro Yamada

Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP patch bomb [ In reply to ]

boris.brezillon at free-electrons

Mar 13, 2017, 6:20 AM

Post #9 of 9 (141 views)

Permalink

Hi Masahiro,

On Fri, 10 Mar 2017 20:00:03 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
>
> I am almost getting v2 done,
> and now I am testing it.
>
> I am having one problem. Please teach me.
>
>
> 2016-11-30 17:17 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> [2]
> >> Remove driver-internal bounce buffer.
> >> The current Denali driver allocate DMA_BIDIRECTIONAL buffer
> >> to use it as a driver-internal bounce buffer.
> >>
> >> The hardware transfer page data into the bounce buffer,
> >> then CPU copies from the bounce buffer to a given buf (and oob_poi).
> >> This is not efficient.
> >>
> >> So, I want to set NAND_USE_BOUNCE_BUFFER flag
> >> and do dma_map_single directly for a given buffer.
> >
> > Sounds good. Be careful though, when you use the generic bounce buffer
> > interface you might have to clear the page cache info (->pagebuf = -1).
>
>
> Instead of memcpy() of the whole page,
> I am trying to use dma_map_single() in ecc->read_page() / ecc->write_page().
> This will allow direct transfer between the buffer and the device by DMA.
>
> But, this does not work for Denali if use_bufpoi is set in nand_do_read_ops().
>
>
> In the following code in nand_scan_tail(),
>
> if (!(chip->options & NAND_OWN_BUFFERS)) {
> nbuf = kzalloc(sizeof(*nbuf) + mtd->writesize
> + mtd->oobsize * 3, GFP_KERNEL);
> if (!nbuf)
> return -ENOMEM;
> nbuf->ecccalc = (uint8_t *)(nbuf + 1);
> nbuf->ecccode = nbuf->ecccalc + mtd->oobsize;
> nbuf->databuf = nbuf->ecccode + mtd->oobsize;
>
> chip->buffers = nbuf;
>
>
> chip->buffers->databuf has no guarantee for DMA'able alignment.
> (actually it has unwanted offset 0xc because sizeof(*nbuf) == 0xc on
> 32bit systems)

Well, I think the DMA alignment requirement is a platform/controller
specific (some controllers are fine with this 32bits alignment), but I
get your point.

>
> If we could change the code as follows,
>
> nbuf->ecccalc = kmalloc(mtd->oobsize, GFP_KERNEL);
> nbuf->ecccode = kmalloc(mtd->oobsize, GFP_KERNEL);
> nbuf->databuf = kmalloc(mtd->writesize + mtd->oobsize,
> GFP_KERNEL);
>
> chip->buffers->databuf would have DMA'able alignment in most cases
> without NAND_OWN_BUFFERS. (but, I am not sure if this is a good idea)

I'm fine with this change. I don't know what are the guarantees in term
of alignment when you use kmalloc, but I guess the size you're
allocating (writesize + oobsize) kind of guarantees that the alignment
is rather big (because the SLAB caches are organized by power-of-2
chunk sizes, and for allocations >PAGE_SIZE the page allocator will be
used).

>
>
> So, the idea of NAND_OWN_BUFFERS is that
> drivers should allocate own buffers if they need to perform DMA-mapping
> in read_page(), write_page(), right?

Right.

>
>
> However, "git grep NAND_OWN_BUFFERS" shows
> cafe_nand.c is the only driver that does so.
>
> On the other hand, "git grep dma_map_single" has more hits,
> i.e. some drivers perform dma_map_single() for read/write without
> NAND_OWN_BUFFERS.
>
> I have no idea how they are working.

Probably because the controllers and/or DMA engines have no alignment
constraints.

Anyway, the change you're proposing is rather simple, so go ahead.

Regards,

Boris