From: Boris Brezillon <boris.brezillon@collabora.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
devicetree@vger.kernel.org, Michal Simek <monstr@monstr.eu>,
Vignesh Raghavendra <vigneshr@ti.com>,
Tudor Ambarus <Tudor.Ambarus@microchip.com>,
Richard Weinberger <richard@nod.at>,
Rob Herring <robh+dt@kernel.org>,
linux-mtd@lists.infradead.org,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
Naga Sureshkumar Relli <nagasure@xilinx.com>
Subject: Re: [PATCH v4 7/8] mtd: rawnand: arasan: Add new Arasan NAND controller
Date: Mon, 11 May 2020 17:59:32 +0200 [thread overview]
Message-ID: <20200511175932.0864490d@collabora.com> (raw)
In-Reply-To: <20200511174614.38058ec7@xps13>
On Mon, 11 May 2020 17:46:14 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> Hi Boris,
>
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 11 May
> 2020 17:32:35 +0200:
>
> > On Mon, 11 May 2020 17:07:29 +0200
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > > Hi Boris,
> > >
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 10 May
> > > 2020 09:03:14 +0200:
> > >
> > > > On Fri, 8 May 2020 19:13:38 +0200
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >
> > > > > +static int anfc_exec_op(struct nand_chip *chip,
> > > > > + const struct nand_operation *op,
> > > > > + bool check_only)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + if (check_only)
> > > > > + return nand_op_parser_exec_op(chip, &anfc_op_parser, op,
> > > > > + check_only);
> > > >
> > > > You should also check the DATA_IN/OUT size here ^.
> > >
> > > Here is my proposal:
> > >
> > > ---8<---
> > >
> > > +static int anfc_check_op(struct nand_chip *chip,
> > > + const struct nand_operation *op)
> > > +{
> > > + int op_id;
> > > +
> > > + /*
> > > + * The controller abstracts all the NAND operations and do not support
> > > + * data only operations.
> >
> > * FIXME: The nand_op_parser framework should be extended to
> > * support custom checks on DATA instructions.
>
> Oh you really want to extend the core for that? I thought having a
> "check_op" helper like this was enough, as it gives enough freedom to
> the controller driver to return all the corner cases that are not very
> generic. See below for more details.
Widespread enough to have at least 2 controllers needing that :P. Any
code sharing is good to take IMO.
>
> >
> > > + */
> >
> > You also didn't mention the fact that the number of data cycles should
> > be aligned on 4 bytes, and that the controller might read/write more
> > than requested when that's not the case. But maybe you have that
> > comment elsewhere in the code (where you do the round_up(4)?).
>
> Precisely, for me the previous check is not a problem from the core
> perspective (hence not deserving a FIXME) because the driver do not lie
> at any moment. Conversely, the driver limitations of what is supported
> and what is not is clear and accurate.
True, you can turn that into a TODO. That's still something I'd like to
see shared. That's not to say we have to handle all weird cases in the
core, we can simply add a ->check() hook to the pattern_elem so drivers
can have custom per-element checks.
>
> However for this round_up() operation you are talking about, this is an
> issue as we have currently no mean to say to the core that something
> different than ordered was actually requested by the driver, so there
> is lying involved and this deserves a FIXME.
> >
> > /*
> > * Number of DATA cycles must be aligned on 4, that means the
> > * controller might read/write more than requested This is
> > * harmless most of the time as extra DATA are discarded in
> > * the write path and read pointer adjusted in the read path.
> > * FIXME: The core should mark operations where reading/writing
> > * more is allowed so the exec_op() implementation can take
> > * the right decision when the alignment constraint is not met:
> > * adjust the number of DATA cycles when it's allowed, and
> > * reject the operation otherwise.
> > */
>
> I want to put this comment where the round_up takes place.
Sounds good.
>
> >
> > > + for (op_id = 0; op_id < op->ninstrs; op_id++) {
> > > + instr = &op->instrs[op_id];
> > > +
> > > + switch (instr->type) {
> > > + case NAND_OP_ADDR_INSTR:
> > > + if (instr->ctx.addr.naddrs > ANFC_MAX_ADDR_CYC)
> > > + return -ENOTSUPP;
> > > + break;
> > > + case NAND_OP_DATA_IN_INSTR:
> > > + case NAND_OP_DATA_OUT_INSTR:
> > > + if (instr->ctx.data.len > ANFC_MAX_CHUNK_SIZE)
> > > + return -ENOTSUPP;
> > > + break;
> > > + default:
> > > + }
> > > + }
> > > +
> > > + /*
> > > + * The controller does not allow to proceed with a CMD+DATA_IN cycle
> > > + * manually on the bus by reading data from the data register. Instead,
> > > + * the controller abstract a status read operation with its own status
> > > + * register after ordering a read status operation. Hence, we cannot
> > > + * support any CMD+DATA_IN operation other than a READ STATUS.
> >
> > * FIXME: The nand_op_parser() framework should be extended to
> > * describe fixed patterns instead of open-coding this check
> > * here.
>
> For this one, I am not against a FIXME as this is something that might
> be useful for other drivers too.
Maybe TODO more than FIXME, you're right.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-05-11 16:00 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-08 17:13 [PATCH v4 0/8] New Arasan NAND controller driver Miquel Raynal
2020-05-08 17:13 ` [PATCH v4 1/8] lib/bch: Rework a little bit the exported function names Miquel Raynal
2020-05-08 17:13 ` [PATCH v4 2/8] lib/bch: Allow easy bit swapping Miquel Raynal
2020-05-08 17:13 ` [PATCH v4 3/8] mtd: rawnand: Ensure the number of bitflips is consistent Miquel Raynal
2020-05-08 17:13 ` [PATCH v4 4/8] mtd: rawnand: Add nand_extract_bits() Miquel Raynal
2020-05-08 17:13 ` [PATCH v4 5/8] MAINTAINERS: Add Arasan NAND controller and bindings Miquel Raynal
2020-05-08 17:13 ` [PATCH v4 6/8] dt-bindings: mtd: Document ARASAN NAND bindings Miquel Raynal
2020-05-11 14:10 ` Michal Simek
2020-05-18 18:12 ` Rob Herring
2020-05-08 17:13 ` [PATCH v4 7/8] mtd: rawnand: arasan: Add new Arasan NAND controller Miquel Raynal
2020-05-10 6:51 ` Boris Brezillon
2020-05-10 6:52 ` Boris Brezillon
2020-05-10 8:33 ` Miquel Raynal
2020-05-10 7:02 ` Boris Brezillon
2020-05-10 8:35 ` Miquel Raynal
2020-05-10 8:41 ` Boris Brezillon
2020-05-10 8:53 ` Miquel Raynal
2020-05-11 16:14 ` Miquel Raynal
2020-05-10 7:03 ` Boris Brezillon
2020-05-11 15:07 ` Miquel Raynal
2020-05-11 15:32 ` Boris Brezillon
2020-05-11 15:46 ` Miquel Raynal
2020-05-11 15:50 ` Miquel Raynal
2020-05-11 15:59 ` Boris Brezillon [this message]
2020-05-08 17:13 ` [PATCH v4 8/8] mtd: rawnand: arasan: Support the hardware BCH ECC engine Miquel Raynal
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=20200511175932.0864490d@collabora.com \
--to=boris.brezillon@collabora.com \
--cc=Tudor.Ambarus@microchip.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=miquel.raynal@bootlin.com \
--cc=monstr@monstr.eu \
--cc=nagasure@xilinx.com \
--cc=richard@nod.at \
--cc=robh+dt@kernel.org \
--cc=thomas.petazzoni@bootlin.com \
--cc=vigneshr@ti.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).