Hi Stefan, On Fri, 25 May 2018 00:56:23 +0200, Stefan Agner <[email protected]> wrote:
> On 24.05.2018 14:41, Boris Brezillon wrote: > > On Thu, 24 May 2018 14:23:56 +0200 > > Boris Brezillon <[email protected]> wrote: > > > >> On Thu, 24 May 2018 13:09:53 +0200 > >> Stefan Agner <[email protected]> wrote: > >> > >> > On 24.05.2018 10:56, Boris Brezillon wrote: > >> > > On Thu, 24 May 2018 10:46:27 +0200 > >> > > Stefan Agner <[email protected]> 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 <[email protected]> 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? I don't think there is a test in mtd-utils that does train both implementations with oob_required set, but you should be able to test it manually with nanddump/nandwrite --oob. Thanks, Miquèl
