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

Groups > linux.kernel > #1597109 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP  patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-03-10 12:10 +0100
    Re: [PATCH 00/39] mtd: nand: denali: 2nd round of Denali NAND IP  patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-03-13 12:40 +0100

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

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

[toc] | [next] | [standalone]

#1599235

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web