Re: [PATCH 2/3] mtd: mediatek: driver for MTK Smart Device Gen1 NAND

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 15 Mar 2016 09:21:35 -0400
Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:

> On 03/15/2016 08:59 AM, Boris Brezillon wrote:
> > On Tue, 15 Mar 2016 08:28:37 -0400
> > Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> wrote:
> >
> >> On 03/08/2016 11:24 AM, Boris Brezillon wrote:
> >>>> +static int mtk_nfc_write_page(struct mtd_info *mtd,
> >>>>> +			struct nand_chip *chip, const uint8_t *buf,
> >>>>> +			int oob_on, int page, int raw)
> >>>>> +{
> >>>>> +
> >>>>> +	struct mtk_nfc_host *host = nand_get_controller_data(chip);
> >>>>> +	struct completion *nfi = &host->nfi.complete;
> >>>>> +	struct device *dev = host->dev;
> >>>>> +	const bool use_ecc = !raw;
> >>>>> +	void *q = (void *) buf;
> >>>>> +	dma_addr_t dma_addr;
> >>>>> +	size_t dmasize;
> >>>>> +	u32 reg;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	dmasize = mtd->writesize + (raw ? mtd->oobsize : 0);
> >>>>> +
> >>>>> +	dma_addr = dma_map_single(dev, q, dmasize, DMA_TO_DEVICE);
> >>> buf is not guaranteed to be physically contiguous, so you can't just
> >>> use it with DMA without doing a few more verifications.
> >>>
> >>> In case you're interested in using a generic approach to do this
> >>> verification, you can have a look at this series [2].
> >>>
> >> unfortunately the internal dma controller does not support scatter gather
> >> operations (we need to DMA in/out of memory in a single shot)
> >> If we enable NAND_USE_BOUNCE_BUFFER, I think this guarantees that the buffer
> >> will be contiguous (since they are allocated with kmalloc)
> >> ...although maybe I should have the bounce buffers in the driver and allocate
> >> them with devm_get_free_pages instead
> >>
> >> would either of this would be acceptable?
> > Or you could test if the buffer is contiguous, and fallback to using
> > a bounce buffer (either an internal one or the generic one) if that's
> > not the case. Note that the proposed API can be improved to reject
> > non-contiguous buffers...
> 
> wouldn't that check be the same than the one done in the nand interface when
> NAND_USE_BOUNCE_BUFFER is enabled?
> IIRC virt_addr_valid() guarantees that the buffer is contiguous.

Correct, I forgot we had that test, and thought we were always using
the bounce buffer when NAND_USE_BOUNCE_BUFFER was set.
Still, in some cases, you might have vmallocated buffers that are
physically contiguous (say you have NAND page < PAGE_SIZE).

Anyway, I guess setting NAND_USE_BOUNCE_BUFFER and assuming the buf
passed to ->read/write_page() is always pointing to a physically
contiguous region is an acceptable option until we have this
mtd_map_buf() API ready.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com