From: Stefan Agner <stefan@agner.ch>
To: Jens Axboe <axboe@kernel.dk>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>,
Dmitry Osipenko <digetx@gmail.com>,
dwmw2@infradead.org, computersforpeace@gmail.com,
marek.vasut@gmail.com, robh+dt@kernel.org, mark.rutland@arm.com,
thierry.reding@gmail.com, dev@lynxeye.de,
miquel.raynal@bootlin.com, richard@nod.at, marcel@ziswiler.com,
krzk@kernel.org, benjamin.lindqvist@endian.se,
jonathanh@nvidia.com, pdeschrijver@nvidia.com,
pgaikwad@nvidia.com, mirza.krak@gmail.com, gaireg@gaireg.de,
linux-mtd@lists.infradead.org, linux-tegra@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 4/6] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver
Date: Tue, 12 Jun 2018 22:20:58 +0200 [thread overview]
Message-ID: <bb2b5e0de922dd928ad923c185bb5da8@agner.ch> (raw)
In-Reply-To: <1fcde0fa-d1e8-1cc3-8c3d-0e8c4097cb91@kernel.dk>
On 12.06.2018 17:24, Jens Axboe wrote:
> On 6/12/18 3:17 AM, Stefan Agner wrote:
>> [also added Jens Axboe]
>>
>> On 12.06.2018 10:27, Boris Brezillon wrote:
>>> On Tue, 12 Jun 2018 10:06:42 +0200
>>> Stefan Agner <stefan@agner.ch> wrote:
>>>
>>>> On 12.06.2018 02:03, Dmitry Osipenko wrote:
>>>>> On Monday, 11 June 2018 23:52:22 MSK Stefan Agner wrote:
>>>>>> Add support for the NAND flash controller found on NVIDIA
>>>>>> Tegra 2 SoCs. This implementation does not make use of the
>>>>>> command queue feature. Regular operations/data transfers are
>>>>>> done in PIO mode. Page read/writes with hardware ECC make
>>>>>> use of the DMA for data transfer.
>>>>>>
>>>>>> Signed-off-by: Lucas Stach <dev@lynxeye.de>
>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>> ---
>>>>>> MAINTAINERS | 7 +
>>>>>> drivers/mtd/nand/raw/Kconfig | 6 +
>>>>>> drivers/mtd/nand/raw/Makefile | 1 +
>>>>>> drivers/mtd/nand/raw/tegra_nand.c | 1248 +++++++++++++++++++++++++++++
>>>>>> 4 files changed, 1262 insertions(+)
>>>>>> create mode 100644 drivers/mtd/nand/raw/tegra_nand.c
>>>>>>
>>>> [snip]
>>>>>> +static int tegra_nand_cmd(struct nand_chip *chip,
>>>>>> + const struct nand_subop *subop)
>>>>>> +{
>>>>>> + const struct nand_op_instr *instr;
>>>>>> + const struct nand_op_instr *instr_data_in = NULL;
>>>>>> + struct tegra_nand_controller *ctrl = to_tegra_ctrl(chip->controller);
>>>>>> + unsigned int op_id, size = 0, offset = 0;
>>>>>> + bool first_cmd = true;
>>>>>> + u32 reg, cmd = 0;
>>>>>> + int ret;
>>>>>> +
>>>>>> + for (op_id = 0; op_id < subop->ninstrs; op_id++) {
>>>>>> + unsigned int naddrs, i;
>>>>>> + const u8 *addrs;
>>>>>> + u32 addr1 = 0, addr2 = 0;
>>>>>> +
>>>>>> + instr = &subop->instrs[op_id];
>>>>>> +
>>>>>> + switch (instr->type) {
>>>>>> + case NAND_OP_CMD_INSTR:
>>>>>> + if (first_cmd) {
>>>>>> + cmd |= COMMAND_CLE;
>>>>>> + writel_relaxed(instr->ctx.cmd.opcode,
>>>>>> + ctrl->regs + CMD_REG1);
>>>>>> + } else {
>>>>>> + cmd |= COMMAND_SEC_CMD;
>>>>>> + writel_relaxed(instr->ctx.cmd.opcode,
>>>>>> + ctrl->regs + CMD_REG2);
>>>>>> + }
>>>>>> + first_cmd = false;
>>>>>> + break;
>>>>>> + case NAND_OP_ADDR_INSTR:
>>>>>> + offset = nand_subop_get_addr_start_off(subop, op_id);
>>>>>> + naddrs = nand_subop_get_num_addr_cyc(subop, op_id);
>>>>>> + addrs = &instr->ctx.addr.addrs[offset];
>>>>>> +
>>>>>> + cmd |= COMMAND_ALE | COMMAND_ALE_SIZE(naddrs);
>>>>>> + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++)
>>>>>> + addr1 |= *addrs++ << (BITS_PER_BYTE * i);
>>>>>> + naddrs -= i;
>>>>>> + for (i = 0; i < min_t(unsigned int, 4, naddrs); i++)
>>>>>> + addr2 |= *addrs++ << (BITS_PER_BYTE * i);
>>>>>> + writel_relaxed(addr1, ctrl->regs + ADDR_REG1);
>>>>>> + writel_relaxed(addr2, ctrl->regs + ADDR_REG2);
>>>>>> + break;
>>>>>> +
>>>>>> + case NAND_OP_DATA_IN_INSTR:
>>>>>> + size = nand_subop_get_data_len(subop, op_id);
>>>>>> + offset = nand_subop_get_data_start_off(subop, op_id);
>>>>>> +
>>>>>> + cmd |= COMMAND_TRANS_SIZE(size) | COMMAND_PIO |
>>>>>> + COMMAND_RX | COMMAND_A_VALID;
>>>>>> +
>>>>>> + instr_data_in = instr;
>>>>>> + break;
>>>>>> +
>>>>>> + case NAND_OP_DATA_OUT_INSTR:
>>>>>> + size = nand_subop_get_data_len(subop, op_id);
>>>>>> + offset = nand_subop_get_data_start_off(subop, op_id);
>>>>>> +
>>>>>> + cmd |= COMMAND_TRANS_SIZE(size) | COMMAND_PIO |
>>>>>> + COMMAND_TX | COMMAND_A_VALID;
>>>>>> +
>>>>>> + memcpy(®, instr->ctx.data.buf.out + offset, size);
>>>>>> + writel_relaxed(reg, ctrl->regs + RESP);
>>>>>> +
>>>>>> + break;
>>>>>> + case NAND_OP_WAITRDY_INSTR:
>>>>>> + cmd |= COMMAND_RBSY_CHK;
>>>>>> + break;
>>>>>> +
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + cmd |= COMMAND_GO | COMMAND_CE(ctrl->cur_cs);
>>>>>> + writel_relaxed(cmd, ctrl->regs + COMMAND);
>>>>>> + ret = wait_for_completion_io_timeout(&ctrl->command_complete,
>>>>>> + msecs_to_jiffies(500));
>>>>>
>>>>> It's not obvious to me whether _io_ variant is appropriate to use here, would
>>>>> be nice if somebody could clarify that. Maybe block/ already does the IO
>>>>> accounting itself and hence the IO time would be counted twice in that case.
>>>>
>>>> Good that you bring this up.
>>>>
>>>> I don't think that there is any higher layer which could take care of
>>>> accounting. Usually, with raw nand there is no block layer involved
>>>> anyway.
>>>>
>>>> In a quick test it seems that only when using wait_for_completion_io I/O
>>>> is properly accounted in the "wait" section of top.
>>>>
>>>> So far only a single driver (omap2) used the _io variant, but I think it
>>>> is the right thing to do! After all, it is I/O...
>>>>
>>>> Boris or any other MTD maintainer, any comment on this?
>>>
>>> Given this definition of io_schedule_timeout() [1] (which is used when
>>> you call wait_for_completion_io_timeout()), I'd say it's not useful to
>>> use the _io_ version, simply because MTD devs are not exposed as blk
>>> devices, and thus don't need the blk_schedule_flush_plug() that is done
>>> is io_schedule_prepare(). But that also means MTD I/Os are not
>>> accounted as I/Os :-(.
>>
>> Documentation of wait_for_completion_io says:
>> "The caller is accounted as waiting for IO (which traditionally means
>> blkio only)."
>>
>> Which sounds as if it using _io is only an accounting thing...
>
> Yes, you should only use it for waiting for IO off a system call
> read path. So block IO, or file system IO. Don't use it for internal
> IO that isn't related to that.
I guess that would be the case here, since MTD page read/writes are
typically file system IOs (e.g. UBIFS).
The problem is just that is not block related at all since it uses the
MTD subsystem... And it seems that the _io variants besides accounting,
also take a role in the block subsystems device plugging mechanism. What
is unclear to me if using the _io variant from the MTD subsystem
potentially disturbs the plugging mechanism...
--
Stefan
next prev parent reply other threads:[~2018-06-12 20:21 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-11 20:52 [PATCH v4 0/6] mtd: rawnand: add NVIDIA Tegra NAND flash support Stefan Agner
2018-06-11 20:52 ` [PATCH v4 1/6] mtd: rawnand: add Reed-Solomon error correction algorithm Stefan Agner
2018-06-12 18:37 ` Rob Herring
2018-06-11 20:52 ` [PATCH v4 2/6] mtd: rawnand: add an option to specify NAND chip as a boot device Stefan Agner
2018-06-11 20:52 ` [PATCH v4 3/6] mtd: rawnand: tegra: add devicetree binding Stefan Agner
2018-06-11 20:52 ` [PATCH v4 4/6] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver Stefan Agner
2018-06-11 23:08 ` kbuild test robot
2018-06-11 23:32 ` Dmitry Osipenko
2018-06-12 8:02 ` Stefan Agner
2018-06-12 8:13 ` Boris Brezillon
2018-06-17 16:57 ` Stefan Agner
2018-06-12 0:03 ` Dmitry Osipenko
2018-06-12 8:06 ` Stefan Agner
2018-06-12 8:27 ` Boris Brezillon
2018-06-12 9:17 ` Stefan Agner
2018-06-12 9:34 ` Boris Brezillon
2018-06-12 15:24 ` Jens Axboe
2018-06-12 20:20 ` Stefan Agner [this message]
2018-06-12 21:20 ` Boris Brezillon
2018-06-12 21:24 ` Jens Axboe
2018-06-12 21:30 ` Boris Brezillon
2018-06-12 12:17 ` Dmitry Osipenko
2018-06-11 20:52 ` [PATCH v4 5/6] ARM: dts: tegra: add Tegra20 NAND flash controller node Stefan Agner
2018-06-11 20:52 ` [PATCH v4 6/6] ARM: dts: tegra: enable NAND flash on Colibri T20 Stefan Agner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bb2b5e0de922dd928ad923c185bb5da8@agner.ch \
--to=stefan@agner.ch \
--cc=axboe@kernel.dk \
--cc=benjamin.lindqvist@endian.se \
--cc=boris.brezillon@bootlin.com \
--cc=computersforpeace@gmail.com \
--cc=dev@lynxeye.de \
--cc=devicetree@vger.kernel.org \
--cc=digetx@gmail.com \
--cc=dwmw2@infradead.org \
--cc=gaireg@gaireg.de \
--cc=jonathanh@nvidia.com \
--cc=krzk@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-tegra@vger.kernel.org \
--cc=marcel@ziswiler.com \
--cc=marek.vasut@gmail.com \
--cc=mark.rutland@arm.com \
--cc=miquel.raynal@bootlin.com \
--cc=mirza.krak@gmail.com \
--cc=pdeschrijver@nvidia.com \
--cc=pgaikwad@nvidia.com \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).