On 24.05.2018 14:41, Boris Brezillon wrote:
> On Thu, 24 May 2018 14:23:56 +0200
> Boris Brezillon <boris.brezillon@xxxxxxxxxxx> wrote:
>
>> On Thu, 24 May 2018 13:09:53 +0200
>> Stefan Agner <stefan@xxxxxxxx> wrote:
>>
>> > On 24.05.2018 10:56, Boris Brezillon wrote:
>> > > On Thu, 24 May 2018 10:46:27 +0200
>> > > Stefan Agner <stefan@xxxxxxxx> wrote:
>> > >
>> > >> Hi Boris,
>> > >>
>> > >> Thanks for the initial review! One small question below:
>> > >>
>> > >> On 23.05.2018 16:18, Boris Brezillon wrote:
>> > >> > Hi Stefan,
>> > >> >
>> > >> > On Tue, 22 May 2018 14:07:06 +0200
>> > >> > Stefan Agner <stefan@xxxxxxxx> wrote:
>> > >> >> +
>> > >> >> +struct tegra_nand {
>> > >> >> + void __iomem *regs;
>> > >> >> + struct clk *clk;
>> > >> >> + struct gpio_desc *wp_gpio;
>> > >> >> +
>> > >> >> + struct nand_chip chip;
>> > >> >> + struct device *dev;
>> > >> >> +
>> > >> >> + struct completion command_complete;
>> > >> >> + struct completion dma_complete;
>> > >> >> + bool last_read_error;
>> > >> >> +
>> > >> >> + dma_addr_t data_dma;
>> > >> >> + void *data_buf;
>> > >> >> + dma_addr_t oob_dma;
>> > >> >> + void *oob_buf;
>> > >> >> +
>> > >> >> + int cur_chip;
>> > >> >> +};
>> > >> >
>> > >> > This struct should be split in 2 structures: one representing the NAND
>> > >> > controller and one representing the NAND chip:
>> > >> >
>> > >> > struct tegra_nand_controller {
>> > >> > struct nand_hw_control base;
>> > >> > void __iomem *regs;
>> > >> > struct clk *clk;
>> > >> > struct device *dev;
>> > >> > struct completion command_complete;
>> > >> > struct completion dma_complete;
>> > >> > bool last_read_error;
>> > >> > int cur_chip;
>> > >> > };
>> > >> >
>> > >> > struct tegra_nand {
>> > >> > struct nand_chip base;
>> > >> > dma_addr_t data_dma;
>> > >> > void *data_buf;
>> > >> > dma_addr_t oob_dma;
>> > >> > void *oob_buf;
>> > >> > };
>> > >>
>> > >> Is there a particular reason why you would leave DMA buffers in the chip
>> > >> structure? It seems that is more a controller thing...
>> > >
>> > > The size of those buffers is likely to be device dependent, so if you
>> > > have several NANDs connected to the controller, you'll either have to
>> > > have one buffer at the controller level which is max(all-chip-buf-size)
>> > > or a buffer per device.
>> > >
>> > > Also, do you really need these buffers? The core already provide some
>> > > which are suitable for DMA (chip->oob_poi and chip->data_buf).
>> > >
>> >
>> > Good question, I am not sure, that was existing code.
>> >
>> > Are you sure data_buf it is DMA capable?
>> >
>> > nand_scan_tail allocates with kmalloc:
>> >
>> > chip->data_buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
>>
>> Yes, kmalloc() allocates DMA-able buffers, so those are DMA-safe.
>
> Hm, that's not exactly true. It depends on the dma_mask attached to the
> device.
It seems to work (tm).
I am not sure how to deal with the OOB buffer. I now use the given
pointer also for oob (offset writesize). I think mtk_nand does the same
thing.
dma_len = mtd->writesize + (oob_required ? mtd->oobsize : 0);
dma_addr = dma_map_single(ctrl->dev, buf, dma_len, DMA_FROM_DEVICE);
...
Is there a test which allows to test my (read|write)_page implementation
with oob_required set?
--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
