[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7b3cc3991fb054130fd54c6fdfec5097@agner.ch>
Date: Thu, 24 May 2018 13:09:53 +0200
From: Stefan Agner <stefan@...er.ch>
To: Boris Brezillon <boris.brezillon@...tlin.com>
Cc: dwmw2@...radead.org, computersforpeace@...il.com,
marek.vasut@...il.com, robh+dt@...nel.org, mark.rutland@....com,
thierry.reding@...il.com, mturquette@...libre.com,
sboyd@...nel.org, dev@...xeye.de, miquel.raynal@...tlin.com,
richard@....at, marcel@...wiler.com, krzk@...nel.org,
digetx@...il.com, benjamin.lindqvist@...ian.se,
jonathanh@...dia.com, pdeschrijver@...dia.com, pgaikwad@...dia.com,
mirza.krak@...il.com, linux-mtd@...ts.infradead.org,
linux-tegra@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash
controller driver
On 24.05.2018 10:56, Boris Brezillon wrote:
> On Thu, 24 May 2018 10:46:27 +0200
> Stefan Agner <stefan@...er.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@...er.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);
--
Stefan
>>
>> If I move them, then struct tegra_nand would be basically empty. Can I
>> just use struct nand_chip and have no driver specific chip abstraction?
>
> Sure.
Powered by blists - more mailing lists