From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Jan Glauber <jan.glauber@caviumnetworks.com>
Cc: linux-mtd@lists.infradead.org,
Richard Weinberger <richard@nod.at>,
David Woodhouse <dwmw2@infradead.org>,
Brian Norris <computersforpeace@gmail.com>,
Marek Vasut <marek.vasut@gmail.com>,
Cyrille Pitchen <cyrille.pitchen@atmel.com>
Subject: Re: [RFC PATCH 2/2] nand: cavium: Nand flash controller for Cavium ARM64 SOCs
Date: Wed, 29 Mar 2017 15:59:33 +0200 [thread overview]
Message-ID: <20170329155933.7494c243@bbrezillon> (raw)
In-Reply-To: <20170329100256.GA3819@hardcore>
On Wed, 29 Mar 2017 12:02:56 +0200
Jan Glauber <jan.glauber@caviumnetworks.com> wrote:
> > > +static void cvm_nand_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
> > > +{
> > > + struct nand_chip *nand = mtd_to_nand(mtd);
> > > + struct cvm_nfc *tn = to_cvm_nfc(nand->controller);
> > > +
> > > + memcpy(tn->buf.dmabuf + tn->buf.data_len, buf, len);
> > > + tn->buf.data_len += len;
> > > +}
> >
> > It seems that cvm_nand_read/write_byte/buf() are returning data that
> > have already been retrieved (problably during the ->cmdfunc() phase).
>
> Yes.
>
> > That's not how it's supposed to work. The core is expecting the data
> > transfer to be done when ->read/write_buf() is called. Doing that in
> > ->cmdfunc() is risky, because when you're there you have no clue about
> > how much bytes the core expect.
>
> It seems to work fine, I've never seen the core trying to do more bytes in
> the read/write_buf() then requested in ->cmdfunc().
We already had problems in the past: when the core evolves to handle
new NAND chips it might decide to read a bit more data than it used to
be, and assuming that your driver will always take the right decision
based on the information passed to ->cmdfunc() is a bit risky.
I still have the plan to provide a better interface allowing drivers to
execute the whole operation sequence (cmd+addr+data cycles), but it's
not there yet (see [1] for more details).
If you're okay to volunteer, I can help you with design this new hook
which should probably make your life easier for the rest of the driver
code (and also help me improve existing drivers ;-)).
Otherwise, you should try to implement ->cmd_ctrl() and try to transfer
data on the bus only when ->read/write_buf() are called (sometime it's
not possible).
> >
> > > +
> > > +static void cvm_nand_cmdfunc(struct mtd_info *mtd, unsigned int command,
> > > + int column, int page_addr)
> >
> > Did you try implementing ->cmd_ctrl() instead of ->cmdfunc(). Your
> > controller seems to be highly configurable and, at first glance, I think
> > you can simplify the driver by implementing ->cmd_ctrl() and relying on
> > the default ->cmdfunc() implementation.
> >
>
> I've looked at the sunxi_nand driver but ->cmd_ctrl() is very different
> from ->cmdfunc() and the later looks like a better match for our controller.
>
> The Cavium controller needs to write the commands (NAND_CMD_READ0, etc.)
> into its pseudo instructions (see ndf_queue_cmd_cle()).
> So how can I do this low-level stuff with ->cmd_ctrl()?
I'd say that it's actually matching pretty well what is passed to
->cmd_ctrl().
For each call to ->cmd_ctrl() you have the information about the type
of access that is made on the bus:
- if the NAND_CLE flag is set in ctrl (the 3rd argument) you have a CLE
cycle
- if NAND_ALE is set in ctrl you have an ALE cycle
- if NAND_CMD_NONE is passed in cmd (2nd argument), you should issue
the whole operation
You can update your cavium command each time NAND_CLE or NAND_ALE is
passed (update the command information after each call), and then issue
the command when NAND_CMD_NONE is passed.
The only missing part in ->cmd_ctrl() are the data transfer cycles
which are handled in ->read/write_buf().
>
> For instance for reading data I have ndf_page_read() that is used for
> both NAND_CMD_READ0 and NAND_CMD_READOOB. Without hacking into ->cmdfunc(),
> how would I differentiate between the two commands in read_buf()?
Do you have to? Can't you just issue a command that is solely doing
data transfer cycles without the CMD and ADDR ones?
[...]
> > > +union ndf_cmd {
> > > + u64 val[2];
> > > + union {
> > > + struct ndf_nop_cmd nop;
> > > + struct ndf_wait_cmd wait;
> > > + struct ndf_bus_cmd bus_acq_rel;
> > > + struct ndf_chip_cmd chip_en_dis;
> > > + struct ndf_cle_cmd cle_cmd;
> > > + struct ndf_rd_cmd rd_cmd;
> > > + struct ndf_wr_cmd wr_cmd;
> > > + struct ndf_set_tm_par_cmd set_tm_par;
> > > + struct ndf_ale_cmd ale_cmd;
> > > + struct ndf_wait_status_cmd wait_status;
> > > + } u;
> > > +};
> >
> > I need some time to process all these information, but your controller
> > seems to be a complex/highly-configurable beast. That's really
> > interesting :-).
> > I'll come up with more comments/question after reviewing more carefully
> > the command creation logic.
>
> Great. I'm afraid out controller is quite different from existing
> hardware, at least I didn't find a driver that does things simalar (like
> the command building and queueing).
Hm, not so different actually, except you seem to have fine grained
control on the sequencing, which is a really good thing because your
driver can evolve with new NAND chip requirements.
>
> I'm happy to help with any more information you need about our hardware.
Thanks,
Boris
[1]http://free-electrons.com/pub/conferences/2016/elc/brezillon-nand-framework/brezillon-nand-framework.pdf
next prev parent reply other threads:[~2017-03-29 14:00 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-27 16:05 [RFC PATCH 0/2] Cavium NAND flash driver Jan Glauber
[not found] ` <20170327160524.29019-1-jglauber-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
2017-03-27 16:05 ` [RFC PATCH 1/2] dt-bindings: mtd: Add Cavium SOCs NAND bindings Jan Glauber
2017-03-27 16:05 ` Jan Glauber
[not found] ` <20170327160524.29019-2-jglauber-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
2017-03-28 20:20 ` Boris Brezillon
2017-03-28 20:20 ` Boris Brezillon
2017-03-28 21:30 ` Jan Glauber
2017-03-28 21:30 ` Jan Glauber
2017-04-03 13:29 ` Rob Herring
2017-04-03 13:29 ` Rob Herring
2017-04-03 14:38 ` Jan Glauber
2017-04-03 14:38 ` Jan Glauber
2017-04-03 14:47 ` Rob Herring
2017-04-03 14:47 ` Rob Herring
[not found] ` <CAL_JsqJ2VgF_Lp-vpdn6VL71K4z6Mu7DWYSaLZ_N0U+jaTuPsQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-04-03 16:18 ` Jan Glauber
2017-04-03 16:18 ` Jan Glauber
2017-03-27 16:05 ` [RFC PATCH 2/2] nand: cavium: Nand flash controller for Cavium ARM64 SOCs Jan Glauber
2017-03-29 9:29 ` Boris Brezillon
2017-03-29 10:02 ` Jan Glauber
2017-03-29 13:59 ` Boris Brezillon [this message]
2017-04-25 11:26 ` Jan Glauber
2017-04-30 13:01 ` Boris Brezillon
2017-05-15 12:33 ` Boris Brezillon
2017-05-15 12:35 ` Boris Brezillon
2017-05-19 7:51 ` Boris Brezillon
2017-05-22 11:35 ` Jan Glauber
2017-05-22 11:53 ` Boris Brezillon
2017-05-22 11:44 ` Boris Brezillon
2017-07-20 20:25 ` [RFC PATCH 0/2] Cavium NAND flash driver Karl Beldan
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=20170329155933.7494c243@bbrezillon \
--to=boris.brezillon@free-electrons.com \
--cc=computersforpeace@gmail.com \
--cc=cyrille.pitchen@atmel.com \
--cc=dwmw2@infradead.org \
--cc=jan.glauber@caviumnetworks.com \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=richard@nod.at \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.