lkml.org 
[lkml]   [2018]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
    On Thu, 24 May 2018 13:09:53 +0200
    Stefan Agner <stefan@agner.ch> wrote:

    > On 24.05.2018 10:56, Boris Brezillon wrote:
    > > On Thu, 24 May 2018 10:46:27 +0200
    > > Stefan Agner <stefan@agner.ch> 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@agner.ch> 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

    \
     
     \ /
      Last update: 2018-05-24 14:29    [W:8.843 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site