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);
Also, you might want to set the NAND_USE_BOUNCE_BUFFER flag in
chip->options, so that the core always pass DMA-able buffers to your
->read/write_page[_raw] function.
[1]https://elixir.bootlin.com/linux/v4.17-rc6/source/include/linux/mtd/rawnand.h#L202
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
