From: Boris Brezillon <boris.brezillon@bootlin.com>
To: Yixun Lan <yixun.lan@amlogic.com>
Cc: Rob Herring <robh@kernel.org>,
Neil Armstrong <narmstrong@baylibre.com>,
Richard Weinberger <richard@nod.at>,
Miquel Raynal <miquel.raynal@bootlin.com>,
<linux-kernel@vger.kernel.org>,
Marek Vasut <marek.vasut@gmail.com>,
Jian Hu <jian.hu@amlogic.com>,
Liang Yang <liang.yang@amlogic.com>,
<linux-mtd@lists.infradead.org>,
Kevin Hilman <khilman@baylibre.com>,
Carlo Caione <carlo@caione.org>,
<linux-amlogic@lists.infradead.org>,
Brian Norris <computersforpeace@gmail.com>,
David Woodhouse <dwmw2@infradead.org>,
<linux-arm-kernel@lists.infradead.org>,
Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller
Date: Thu, 19 Jul 2018 10:39:53 +0200 [thread overview]
Message-ID: <20180719103953.26164eb6@bbrezillon> (raw)
In-Reply-To: <45c1a96c-0d14-dece-37cf-ac428bb98621@amlogic.com>
Hi Yixun,
On Thu, 19 Jul 2018 16:13:47 +0800
Yixun Lan <yixun.lan@amlogic.com> wrote:
> >>> You're doing DMA on those buffers, and devm_kzalloc() is not
> >>> DMA-friendly (returned buffers are not aligned on a cache line). Also,
> >>> you don't have to allocate your own buffers because the core already
> >>> allocate them (chip->data_buf, chip->oob_poi). All you need to do is
> >>> set the NAND_USE_BOUNCE_BUFFER flag in chip->options to make sure
> >>> you're always passed a DMA-able buffer.
> >>>
> >>
> >> thanks for the suggestion, we've migrated to use the
> >> dmam_alloc_coherent() API
> >
> > kzalloc() should be just fine, no need to alloc a DMA coherent region.
> >
>
> we're a little bit confused here, isn't devm_kzalloc (previously we are
> using) a variant of kzalloc? and since the NAND controller is doing DMA
> here, using DMA coherent API is more proper way?
Well, making buffers DMA coherent might be expensive, especially if you
access them a lot (unless you have a coherency unit and the cache is
kept enabled).
Regarding the "why is devm_kzalloc() is not DMA-safe?" question, I'd
recommend that you read this discussion [1].
> >>>> + mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
> >>>> + "%s:nand", dev_name(dev));
> >>>> + if (!mtd->name) {
> >>>> + dev_err(nfc->dev, "Failed to allocate mtd->name\n");
> >>>> + return -ENOMEM;
> >>>> + }
> >>>
> >>> You set the name after nand_scan_ident() and make it conditional (only
> >>> if ->name == NULL) so that the label property defined in the DT takes
> >>> precedence over the default name.
> >>
> for setting mtd->name conditional, do you mean doing something like this?
>
> if (!mtd->name)
> mtd->name = devm_kasprintf(..)
Yes, that's what I meant.
>
> but we found mtd->name = "ffe07800.nfc" after function
> nand_scan_ident(), which is same value as dev_name(dev)..
> and there is no cs information encoded there.
Hm, that shouldn't be the case. Maybe you can add traces to find out
who is setting mtd->name to this value.
>
> >>
> >>> Also, I recommend suffixing this name
> >>> with the CS id, just in case you ever need to support connecting several
> >>> chips to the same controller.
> >>>
> >>
> >> we actually didn't get the point here, cs is about chip selection with
> >> multiple nand chip? and how to get this information?
> >
> > Well, you currently seem to only support one chip per controller, but I
> > guess the IP can handle several CS lines. So my recommendation is about
> > choosing a name so that you can later easily add support for multiple
> > chips without breaking setups where mtdparts is used.
> >
> > To sum-up, assuming your NAND chip is always connected to CS0 (on the
> > controller side), I'd suggest doing:
> >
> yes, this is exactly how the hardware connected.
> > mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
> > "%s:nand.%d", dev_name(dev), cs_id);
> >
> > where cs_id is the value you extracted from the reg property of the
> > NAND node.
> >
> Ok, you right.
> current, the NAND chip is only use one CS (which CE0) for now, what's in
> the DT is
>
> nand@0 {
> reg = < 0 >;
> ..
> };
>
> so for the multiple chips it would something like this in DT?
>
> nand@0 {
> reg = < 0 >;
> };
>
> nand@1 {
> reg = < 1 >;
> };
Yep, that's for 2 single-die chips.
>
> or even
> nand@0 {
> reg = < 0 2 >;
> };
>
> nand@1 {
nand@3 {
> reg = < 3 4 >;
> };
And this is describing 2 dual-die chips.
>
> do we need to encode all the cs information here? not sure if we
> understand this correctly, but could send out the patch for review..
Yes, reg should contain an array of controller-side CS lines used to
select the chip (or a specific die in a chip, the index in the reg
table being the id of the die).
Regards,
Boris
[1]http://linux-arm-kernel.infradead.narkive.com/vyJqy0RQ/question-devm-kmalloc-for-dma
next prev parent reply other threads:[~2018-07-19 8:40 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-13 16:13 [PATCH 0/2] mtd: rawnand: meson: add Amlogic NAND driver support Yixun Lan
2018-06-13 16:13 ` [PATCH 1/2] dt-bindings: nand: meson: add Amlogic NAND controller driver Yixun Lan
2018-06-23 22:46 ` Martin Blumenstingl
2018-06-26 18:30 ` Rob Herring
2018-06-27 23:40 ` Kevin Hilman
2018-06-24 13:57 ` Boris Brezillon
2018-06-13 16:13 ` [PATCH 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller Yixun Lan
2018-06-13 9:07 ` kbuild test robot
2018-06-13 9:33 ` kbuild test robot
2018-06-24 19:38 ` Boris Brezillon
2018-06-27 23:33 ` Kevin Hilman
2018-06-28 7:00 ` Miquel Raynal
2018-06-28 23:45 ` Kevin Hilman
2018-06-29 7:14 ` Neil Armstrong
2018-07-02 7:17 ` Yixun Lan
2018-07-18 9:38 ` Yixun Lan
2018-07-18 19:08 ` Boris Brezillon
2018-07-19 8:13 ` Yixun Lan
2018-07-19 8:39 ` Boris Brezillon [this message]
2018-07-19 9:53 ` Yixun Lan
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=20180719103953.26164eb6@bbrezillon \
--to=boris.brezillon@bootlin.com \
--cc=carlo@caione.org \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=jbrunet@baylibre.com \
--cc=jian.hu@amlogic.com \
--cc=khilman@baylibre.com \
--cc=liang.yang@amlogic.com \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=miquel.raynal@bootlin.com \
--cc=narmstrong@baylibre.com \
--cc=richard@nod.at \
--cc=robh@kernel.org \
--cc=yixun.lan@amlogic.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).