Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Tuesday 19 August 2014 07:42 AM, Brian Norris wrote:
On Wed, Aug 06, 2014 at 02:32:18AM +0530, pe...@pek-sem.com wrote:
On Tuesday 05 August 2014 07:53 PM, Lee Jones wrote:
On Thu, 03 Jul 2014, Gupta, Pekon wrote:
+ /* Load last page of block */
+ offs = (loff_t)block << chip->phys_erase_shift;
+ offs += mtd->erasesize - mtd->writesize;
+ if (bch_read_page(nandi, offs, buf) < 0) {
*Important*: You shouldn't call internal functions directly here..
Instead use chip->_read() OR mtd-read() that will:
I'm not sure exactly what you're saying in the above line (chip->_read()
is not a function, and and mtd-read() is syntactically incorrect),
but...
You most certainly do *not* want to call mtd->_read() directly (or any
of the callbacks prefixed with underscores). That is one of the main
purposes of:
commit 3c3c10bba1e4ccb75b41442e45c1a072f6cded19
Author: Artem Bityutskiy
Date: Mon Jan 30 14:58:32 2012 +0200
mtd: add leading underscore to all mtd functions
Makes sense. Thanks for pointing this.
[...]
I think somewhere in earlier comments, Brian also supported
to use high-level function like mtd_read(). Also, nand_bbt.c
itself uses 'mtd_read(), mtd_read_oob() and mtd_write_oob()
at many places. So I'll let Brain decide here.
But having low-level function will add redundant code for
re-checking and aligning the addresses boundaries to block
and page/sector sizes.
Not all checks are redundant. It's redundant to have the direct
descendant doing the same checks that the parent does, like:
mtd_read() (checks boundaries)
|_ mtd->_read() (e.g., nand_read())
So nand_read() and nand_do_read_ops() don't need the checking.
But for a BBT implementation, it can help to make sure that its
page/block/etc. arithmetic fits the right bounds when it ends up
deciding to scan a block.
Now, it still may be easy to prove that the checks are
redundant/unnecessary for correctly-written code, but if the layering
makes sense, then it still may be a better choice to simply use the
high-level, self-checking API than to try to dig deeper. For instance,
I'm pretty sure UBI does some checks to make sure it's not reading off
the end of an MTD, but it still calls mtd_read() because it's The Right
Thing (TM).
Brian
Agree.. re-using mtd_*() API for non-critical paths is justified.
with regards, pekon
Powered by BigRock.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Tuesday 19 August 2014 07:42 AM, Brian Norris wrote:
On Wed, Aug 06, 2014 at 02:32:18AM +0530, pe...@pek-sem.com wrote:
On Tuesday 05 August 2014 07:53 PM, Lee Jones wrote:
On Thu, 03 Jul 2014, Gupta, Pekon wrote:
+ /* Load last page of block */
+ offs = (loff_t)block chip-phys_erase_shift;
+ offs += mtd-erasesize - mtd-writesize;
+ if (bch_read_page(nandi, offs, buf) 0) {
*Important*: You shouldn't call internal functions directly here..
Instead use chip-_read() OR mtd-read() that will:
I'm not sure exactly what you're saying in the above line (chip-_read()
is not a function, and and mtd-read() is syntactically incorrect),
but...
You most certainly do *not* want to call mtd-_read() directly (or any
of the callbacks prefixed with underscores). That is one of the main
purposes of:
commit 3c3c10bba1e4ccb75b41442e45c1a072f6cded19
Author: Artem Bityutskiy artem.bityuts...@linux.intel.com
Date: Mon Jan 30 14:58:32 2012 +0200
mtd: add leading underscore to all mtd functions
Makes sense. Thanks for pointing this.
[...]
I think somewhere in earlier comments, Brian also supported
to use high-level function like mtd_read(). Also, nand_bbt.c
itself uses 'mtd_read(), mtd_read_oob() and mtd_write_oob()
at many places. So I'll let Brain decide here.
But having low-level function will add redundant code for
re-checking and aligning the addresses boundaries to block
and page/sector sizes.
Not all checks are redundant. It's redundant to have the direct
descendant doing the same checks that the parent does, like:
mtd_read() (checks boundaries)
|_ mtd-_read() (e.g., nand_read())
So nand_read() and nand_do_read_ops() don't need the checking.
But for a BBT implementation, it can help to make sure that its
page/block/etc. arithmetic fits the right bounds when it ends up
deciding to scan a block.
Now, it still may be easy to prove that the checks are
redundant/unnecessary for correctly-written code, but if the layering
makes sense, then it still may be a better choice to simply use the
high-level, self-checking API than to try to dig deeper. For instance,
I'm pretty sure UBI does some checks to make sure it's not reading off
the end of an MTD, but it still calls mtd_read() because it's The Right
Thing (TM).
Brian
Agree.. re-using mtd_*() API for non-critical paths is justified.
with regards, pekon
Powered by BigRock.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Fri, Aug 01, 2014 at 10:27:25AM +0100, Lee Jones wrote:
> On Thu, 31 Jul 2014, Brian Norris wrote:
> > On Thu, Jul 31, 2014 at 05:47:09PM +0100, Lee Jones wrote:
> > > > > arch/arm/boot/dts/stih41x-b2020.dtsi | 40 +
> > > >
> > > > This will need refreshed and sent as a separate patch, to go through the
> > > > appropriate ARM tree.
> > >
> > > As above.
> >
> > OK, if you're really planning to send a final git pull request, you'll
> > need to have this in a separate branch for them to take, then.
>
> Of course. Same goes with the DTS(I) changes.
PSA: make sure your patches will bisect if you're going to send so many.
I don't want to have to copy-and-paste my build results again.
> > > > (Related: Coverity caught a whole bunch of these type of bugs in the MTD
> > > > test modules. I have fixes queued up that I meant to test and submit
> > > > soon.)
> > >
> > > I will attempt to play around with Coverity.
> >
> > Good luck :) Coverity isn't always the easiest to get running
> > individually. You might have better luck (once your stuff is merged)
>
> Sorry merged where?
To Linus's tree. Presumably that's what you're sending your patches to
me for? :)
Dave Jones runs a Coverity instance against mainline.
> > checking out the free service offered for open source projects through
> > github. There's a project instance set up for mainline:
> >
> > https://scan.coverity.com/projects/128?tab=overview
>
> Thanks. I'll have a play.
>
> > > > > +/*
> > > > > + * Detect an erased page, tolerating and correcting up to a
> > > > > specified number of
> > > > > + * bits at '0'. (For many devices, it is now deemed within spec for
> > > > > an erased
> > > > > + * page to include a number of bits at '0', either as a result of
> > > > > read-disturb
> > > > > + * behaviour or 'stuck-at-zero' failures.) Returns the number of
> > > > > corrected
> > > > > + * bits, or a '-1' if we have exceeded the maximum number of bits at
> > > > > '0' (likely
> > > > > + * to be a genuine uncorrectable ECC error). In the latter case,
> > > > > the data must
> > > > > + * be returned unmodified, in accordance with the MTD API.
> > > > > + */
> > > > > +static int check_erased_page(uint8_t *data, uint32_t page_size, int
> > > > > max_zeros)
> > > >
> > > > Another "check erased page" implementation? Sigh... it would be nice if
> > > > we could agree on a common implementation to share. My last attempt was
> > > > unsuccessful, since some drivers have some very odd requirements.
> > > >
> > > > > +{
> > > > > + uint8_t *b = data;
> > > > > + int zeros = 0;
> > > > > + int i;
> > > > > +
> > > > > + for (i = 0; i < page_size; i++) {
> > > > > + zeros += hweight8(~*b++);
> > > > > + if (zeros > max_zeros)
> > > > > + return -1;
> > > > > + }
> > > > > +
> > > > > + if (zeros)
> > > > > + memset(data, 0xff, page_size);
> > > >
> > > > It seems like you're not considering the spare area at all. What if this
> > > > is a mostly-blank page, with ECC data, but most of the bitflips are in
> > > > the spare area? Then you will "correct" this page to all 0xFF, not
> > > > noticing that this was really not a blank page at all.
> > >
> > > I could really use Angus' advice on this one. Although, I fear he may
> > > have disappeared into the cosmos. If I remember correctly (I'm
> > > getting rusty on this now), this controller doesn't allow access to
> > > the spare area easily. I think it's all handled automatically
> > > i.e. without intervention.
> >
> > That's tough. It's pretty hard to support NAND without *any* access to
> > spare area.
>
> I think we _can_ do it, via the second (Flex) interface, but it appears
> to be readonly and we only do so when initially scanning/building the
> BBTs.
Does your driver pass the MTD tests? (drivers/mtd/tests/)
> I'm not sure I follow your example precisely. When you say that most
> of the bitflips are in the spare area, do you mean that there's usable
> data in there?
What I mean is that because ALL [*] data (in- and out-of-band) is used
by the error correction algorithm, then you need to use all of that data
when determining that a page is erased, not just the data that you see
in-band.
Consider this: you program a page with all 0xFF data, except for a
single byte of data. Then suppose your page experiences too many
bitflips in the spare area. When you try to read this page back, you
will experience an ECC error, and fall back to "checking for an erased
page." But if this algorithm only looks at the in-band data, it may see
mostly-0xFF, with fewer than 8 bits that are low. Because it ignored all
of the ECC parity data stored OOB, then your algorithm might falsely
declare your page "erased," when in fact it held data and is instead
truly uncorrectable.
This kind of subtle error might lead to silent corruption.
I haven't exactly seen this in practice, since most MTD
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Wed, Aug 06, 2014 at 02:32:18AM +0530, pe...@pek-sem.com wrote:
> On Tuesday 05 August 2014 07:53 PM, Lee Jones wrote:
> >On Thu, 03 Jul 2014, Gupta, Pekon wrote:
> + /* Load last page of block */
> + offs = (loff_t)block << chip->phys_erase_shift;
> + offs += mtd->erasesize - mtd->writesize;
> + if (bch_read_page(nandi, offs, buf) < 0) {
> >>
> >>*Important*: You shouldn't call internal functions directly here..
> >>Instead use chip->_read() OR mtd-read() that will:
I'm not sure exactly what you're saying in the above line (chip->_read()
is not a function, and and mtd-read() is syntactically incorrect),
but...
You most certainly do *not* want to call mtd->_read() directly (or any
of the callbacks prefixed with underscores). That is one of the main
purposes of:
commit 3c3c10bba1e4ccb75b41442e45c1a072f6cded19
Author: Artem Bityutskiy
Date: Mon Jan 30 14:58:32 2012 +0200
mtd: add leading underscore to all mtd functions
> >>- keep this code independent of ECC modes added in your driver
> >>in future.
> >>- implicitly handle updating mtd->ecc_stat (like ecc_failed,
> >>bits_corrected).
> >>- implicitly take care of address alignments checks and offset
> >>calculations.
> >>
> >> >>bch_read_page())
> >
> >Yourself and Brian seem to disagree on this point. Which is correct?
I think you've worked out a decent solution in your latest series, but
a few of the guiding principles:
* BBT code can rely on generic NAND implementation details (e.g.,
chip->options), but it can also act as an MTD user (i.e., use the
mtd_*() APIs)
* It should not rely on details of the particular NAND driver (e.g.,
calling your ST bch_read_page())
* Look at similar code and try to follow its pattern where it makes
sense. So while nand_bbt.c is not a shining example in all regards, I
think it does OK in terms of some of the key points of layering.
> + /* Is block already considered bad? (will also catch
> invalid offsets) */
> + ret = mtd_block_isbad(mtd, offs);
> >>>
> >>>You're violating some of the layering here; the low-level driver
> >>>probably shouldn't be calling the high-level mtd_*() APIs. On
> >>>a similar
> >>>note, I'm not 100% confident that the nand_base/nand_bbt
> >>>separation is
> >>>written cleanly enough for easy maintenance of your nand_base
> >>>+ ST BBT
> >>>code in parallel. I might need a second look at that, and I think
> >>>modularizing your BBT code to a separate file could help ease
> >>>this a
> >>>little.
> >
> >... here is the converse argument.
I think I clarified this one; I misspoke about the mtd_*() APIs.
> I think somewhere in earlier comments, Brian also supported
> to use high-level function like mtd_read(). Also, nand_bbt.c
> itself uses 'mtd_read(), mtd_read_oob() and mtd_write_oob()
> at many places. So I'll let Brain decide here.
> But having low-level function will add redundant code for
> re-checking and aligning the addresses boundaries to block
> and page/sector sizes.
Not all checks are redundant. It's redundant to have the direct
descendant doing the same checks that the parent does, like:
mtd_read() (checks boundaries)
|_ mtd->_read() (e.g., nand_read())
So nand_read() and nand_do_read_ops() don't need the checking.
But for a BBT implementation, it can help to make sure that its
page/block/etc. arithmetic fits the right bounds when it ends up
deciding to scan a block.
Now, it still may be easy to prove that the checks are
redundant/unnecessary for correctly-written code, but if the layering
makes sense, then it still may be a better choice to simply use the
high-level, self-checking API than to try to dig deeper. For instance,
I'm pretty sure UBI does some checks to make sure it's not reading off
the end of an MTD, but it still calls mtd_read() because it's The Right
Thing (TM).
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Wed, Aug 06, 2014 at 02:32:18AM +0530, pe...@pek-sem.com wrote:
On Tuesday 05 August 2014 07:53 PM, Lee Jones wrote:
On Thu, 03 Jul 2014, Gupta, Pekon wrote:
+ /* Load last page of block */
+ offs = (loff_t)block chip-phys_erase_shift;
+ offs += mtd-erasesize - mtd-writesize;
+ if (bch_read_page(nandi, offs, buf) 0) {
*Important*: You shouldn't call internal functions directly here..
Instead use chip-_read() OR mtd-read() that will:
I'm not sure exactly what you're saying in the above line (chip-_read()
is not a function, and and mtd-read() is syntactically incorrect),
but...
You most certainly do *not* want to call mtd-_read() directly (or any
of the callbacks prefixed with underscores). That is one of the main
purposes of:
commit 3c3c10bba1e4ccb75b41442e45c1a072f6cded19
Author: Artem Bityutskiy artem.bityuts...@linux.intel.com
Date: Mon Jan 30 14:58:32 2012 +0200
mtd: add leading underscore to all mtd functions
- keep this code independent of ECC modes added in your driver
in future.
- implicitly handle updating mtd-ecc_stat (like ecc_failed,
bits_corrected).
- implicitly take care of address alignments checks and offset
calculations.
Same applies to other places where you have directly used
bch_read_page())
Yourself and Brian seem to disagree on this point. Which is correct?
I think you've worked out a decent solution in your latest series, but
a few of the guiding principles:
* BBT code can rely on generic NAND implementation details (e.g.,
chip-options), but it can also act as an MTD user (i.e., use the
mtd_*() APIs)
* It should not rely on details of the particular NAND driver (e.g.,
calling your ST bch_read_page())
* Look at similar code and try to follow its pattern where it makes
sense. So while nand_bbt.c is not a shining example in all regards, I
think it does OK in terms of some of the key points of layering.
+ /* Is block already considered bad? (will also catch
invalid offsets) */
+ ret = mtd_block_isbad(mtd, offs);
You're violating some of the layering here; the low-level driver
probably shouldn't be calling the high-level mtd_*() APIs. On
a similar
note, I'm not 100% confident that the nand_base/nand_bbt
separation is
written cleanly enough for easy maintenance of your nand_base
+ ST BBT
code in parallel. I might need a second look at that, and I think
modularizing your BBT code to a separate file could help ease
this a
little.
... here is the converse argument.
I think I clarified this one; I misspoke about the mtd_*() APIs.
I think somewhere in earlier comments, Brian also supported
to use high-level function like mtd_read(). Also, nand_bbt.c
itself uses 'mtd_read(), mtd_read_oob() and mtd_write_oob()
at many places. So I'll let Brain decide here.
But having low-level function will add redundant code for
re-checking and aligning the addresses boundaries to block
and page/sector sizes.
Not all checks are redundant. It's redundant to have the direct
descendant doing the same checks that the parent does, like:
mtd_read() (checks boundaries)
|_ mtd-_read() (e.g., nand_read())
So nand_read() and nand_do_read_ops() don't need the checking.
But for a BBT implementation, it can help to make sure that its
page/block/etc. arithmetic fits the right bounds when it ends up
deciding to scan a block.
Now, it still may be easy to prove that the checks are
redundant/unnecessary for correctly-written code, but if the layering
makes sense, then it still may be a better choice to simply use the
high-level, self-checking API than to try to dig deeper. For instance,
I'm pretty sure UBI does some checks to make sure it's not reading off
the end of an MTD, but it still calls mtd_read() because it's The Right
Thing (TM).
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Fri, Aug 01, 2014 at 10:27:25AM +0100, Lee Jones wrote:
On Thu, 31 Jul 2014, Brian Norris wrote:
On Thu, Jul 31, 2014 at 05:47:09PM +0100, Lee Jones wrote:
arch/arm/boot/dts/stih41x-b2020.dtsi | 40 +
This will need refreshed and sent as a separate patch, to go through the
appropriate ARM tree.
As above.
OK, if you're really planning to send a final git pull request, you'll
need to have this in a separate branch for them to take, then.
Of course. Same goes with the DTS(I) changes.
PSA: make sure your patches will bisect if you're going to send so many.
I don't want to have to copy-and-paste my build results again.
(Related: Coverity caught a whole bunch of these type of bugs in the MTD
test modules. I have fixes queued up that I meant to test and submit
soon.)
I will attempt to play around with Coverity.
Good luck :) Coverity isn't always the easiest to get running
individually. You might have better luck (once your stuff is merged)
Sorry merged where?
To Linus's tree. Presumably that's what you're sending your patches to
me for? :)
Dave Jones runs a Coverity instance against mainline.
checking out the free service offered for open source projects through
github. There's a project instance set up for mainline:
https://scan.coverity.com/projects/128?tab=overview
Thanks. I'll have a play.
+/*
+ * Detect an erased page, tolerating and correcting up to a
specified number of
+ * bits at '0'. (For many devices, it is now deemed within spec for
an erased
+ * page to include a number of bits at '0', either as a result of
read-disturb
+ * behaviour or 'stuck-at-zero' failures.) Returns the number of
corrected
+ * bits, or a '-1' if we have exceeded the maximum number of bits at
'0' (likely
+ * to be a genuine uncorrectable ECC error). In the latter case,
the data must
+ * be returned unmodified, in accordance with the MTD API.
+ */
+static int check_erased_page(uint8_t *data, uint32_t page_size, int
max_zeros)
Another check erased page implementation? Sigh... it would be nice if
we could agree on a common implementation to share. My last attempt was
unsuccessful, since some drivers have some very odd requirements.
+{
+ uint8_t *b = data;
+ int zeros = 0;
+ int i;
+
+ for (i = 0; i page_size; i++) {
+ zeros += hweight8(~*b++);
+ if (zeros max_zeros)
+ return -1;
+ }
+
+ if (zeros)
+ memset(data, 0xff, page_size);
It seems like you're not considering the spare area at all. What if this
is a mostly-blank page, with ECC data, but most of the bitflips are in
the spare area? Then you will correct this page to all 0xFF, not
noticing that this was really not a blank page at all.
I could really use Angus' advice on this one. Although, I fear he may
have disappeared into the cosmos. If I remember correctly (I'm
getting rusty on this now), this controller doesn't allow access to
the spare area easily. I think it's all handled automatically
i.e. without intervention.
That's tough. It's pretty hard to support NAND without *any* access to
spare area.
I think we _can_ do it, via the second (Flex) interface, but it appears
to be readonly and we only do so when initially scanning/building the
BBTs.
Does your driver pass the MTD tests? (drivers/mtd/tests/)
I'm not sure I follow your example precisely. When you say that most
of the bitflips are in the spare area, do you mean that there's usable
data in there?
What I mean is that because ALL [*] data (in- and out-of-band) is used
by the error correction algorithm, then you need to use all of that data
when determining that a page is erased, not just the data that you see
in-band.
Consider this: you program a page with all 0xFF data, except for a
single byte of data. Then suppose your page experiences too many
bitflips in the spare area. When you try to read this page back, you
will experience an ECC error, and fall back to checking for an erased
page. But if this algorithm only looks at the in-band data, it may see
mostly-0xFF, with fewer than 8 bits that are low. Because it ignored all
of the ECC parity data stored OOB, then your algorithm might falsely
declare your page erased, when in fact it held data and is instead
truly uncorrectable.
This kind of subtle error might lead to silent corruption.
I haven't exactly seen this in practice, since most MTD users will
be programming significant amounts of non-0xFF data to pages, but it's
really not a guarantee. And power cuts, for instance, provide a big
source of uncertainty, where you can't expect that an unsound algorithm
like this will work properly.
Note on [*]: some ECC algorithms may ignore parts of
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Thu, 31 Jul 2014, Lee Jones wrote: > On Wed, 02 Jul 2014, Brian Norris wrote: > > On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: [...] > > > + if (IS_ERR(pdata)) > > > + return PTR_ERR(pdata); > > > + > > > + ppdata.of_node = stm_of_get_partitions_node(np, 0); > > > > So you only support a single bank, at chip-select 0? Is this a > > hardware limitation, or simply software? You might consider whether this > > needs to be noted in the DT binding too -- it currently suggests that > > this can be plural. > > Yes, only one bank. Chip select is not possible. Sorry, not only one bank. We can have multiple banks, but only one chip. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Wed, 02 Jul 2014, Brian Norris wrote: > On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: [...] > > + if (((unsigned int)buf & (NANDI_BCH_DMA_ALIGNMENT - 1)) || > > + (!virt_addr_valid(buf))) /* vmalloc'd buffer! */ > > + bounce = true; > > + > > + p = bounce ? nandi->page_buf : buf; > > It looks like you're reimplementing NAND_USE_BOUNCE_BUFFER. Can you try > using that flag? (You may need to extend it to account for your DMA > alignment, too.) NAND_USE_BOUNCE_BUFFER won't work for us unless we can guarantee that the bounce buffer will always be 64 Byte aligned, which I don't think we can. Another way to do it would be to assign all of our own buffers, but I'm really not comfortable fiddling with those as there are a lot of controller specific intricacies which a) I'm not familiar with and b) no longer have Angus to fire questions off to and/or review. So if you don't mind, I'd really rather use Angus' implementation. It's only an extra couple of lines and Angus has already tested it to a high level. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Wed, 02 Jul 2014, Brian Norris wrote: On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: [...] + if (((unsigned int)buf (NANDI_BCH_DMA_ALIGNMENT - 1)) || + (!virt_addr_valid(buf))) /* vmalloc'd buffer! */ + bounce = true; + + p = bounce ? nandi-page_buf : buf; It looks like you're reimplementing NAND_USE_BOUNCE_BUFFER. Can you try using that flag? (You may need to extend it to account for your DMA alignment, too.) NAND_USE_BOUNCE_BUFFER won't work for us unless we can guarantee that the bounce buffer will always be 64 Byte aligned, which I don't think we can. Another way to do it would be to assign all of our own buffers, but I'm really not comfortable fiddling with those as there are a lot of controller specific intricacies which a) I'm not familiar with and b) no longer have Angus to fire questions off to and/or review. So if you don't mind, I'd really rather use Angus' implementation. It's only an extra couple of lines and Angus has already tested it to a high level. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Thu, 31 Jul 2014, Lee Jones wrote: On Wed, 02 Jul 2014, Brian Norris wrote: On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: [...] + if (IS_ERR(pdata)) + return PTR_ERR(pdata); + + ppdata.of_node = stm_of_get_partitions_node(np, 0); So you only support a single bank, at chip-select 0? Is this a hardware limitation, or simply software? You might consider whether this needs to be noted in the DT binding too -- it currently suggests that this can be plural. Yes, only one bank. Chip select is not possible. Sorry, not only one bank. We can have multiple banks, but only one chip. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hello,
On Tuesday 05 August 2014 07:53 PM, Lee Jones wrote:
On Thu, 03 Jul 2014, Gupta, Pekon wrote:
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
[...]
+static void bch_wait_seq(struct nandi_controller *nandi)
+{
+ int ret;
+
+ ret = wait_for_completion_timeout(>seq_completed, HZ/2);
Are you sure you want to use same timeout value for all operations
like ERASE, READ, WRITE ? because
(1) There is wide variance between:
- BLOCK_ERASE: max(tBER) = 10ms) for MT29F4G08 Micron NAND
- PAGE_ERASE: max(tPROG) = 600usec for same Micron part.
(2) The value of HZ varies across kernel versions and hardware
platforms.
I suggest you pass the timeout value in call to bch_wait_seq().
And this timeout value should be like 2x of typical value of which
you found
during ONFI parsing, or from DT
How do you obtain these timings? I don't see tBER or tPROG being
used
anywhere in MTD.
These are not from NAND framework, these are terms used in data-sheet.
However, you can read all signal timing from ONFI page while probing.
But, as Brian suggested that timeout is a erroneous and rare condition
anyway, so these relaxed timeout values are acceptable. So you may
ignore my previous comment.
[...]
+{
+ uint8_t *b = data;
+ int zeros = 0;
+ int i;
+
+ for (i = 0; i < page_size; i++) {
+ zeros += hweight8(~*b++);
Are you sure you want to use hweight8 ?
hweight32 or something should give better performance, plz check.
because this piece of code (check_xx_bitflips) sometimes becomes
bottle neck for READ path.
I'm not sure, no. If I change it, how will I know if it's still
doing
the correct thing or otherwise?
hweight/hweight16/hweight32 are all macros of same family
just operating on different data-widths, so you may continue
for now. But once the driver is merged, you might like to
re-analyze performance gain (especially on MLC NAND) when
using hweight32() instead of hweight8().
[...]
+ /* Load last page of block */
+ offs = (loff_t)block << chip->phys_erase_shift;
+ offs += mtd->erasesize - mtd->writesize;
+ if (bch_read_page(nandi, offs, buf) < 0) {
*Important*: You shouldn't call internal functions directly here..
Instead use chip->_read() OR mtd-read() that will:
- keep this code independent of ECC modes added in your driver in
future.
- implicitly handle updating mtd->ecc_stat (like ecc_failed,
bits_corrected).
- implicitly take care of address alignments checks and offset
calculations.
bch_read_page())
Yourself and Brian seem to disagree on this point. Which is correct?
+ /* Is block already considered bad? (will also catch invalid
offsets) */
+ ret = mtd_block_isbad(mtd, offs);
You're violating some of the layering here; the low-level driver
probably shouldn't be calling the high-level mtd_*() APIs. On a
similar
note, I'm not 100% confident that the nand_base/nand_bbt separation
is
written cleanly enough for easy maintenance of your nand_base + ST
BBT
code in parallel. I might need a second look at that, and I think
modularizing your BBT code to a separate file could help ease this
a
little.
... here is the converse argument.
I think somewhere in earlier comments, Brian also supported
to use high-level function like mtd_read(). Also, nand_bbt.c
itself uses 'mtd_read(), mtd_read_oob() and mtd_write_oob()
at many places. So I'll let Brain decide here.
But having low-level function will add redundant code for
re-checking and aligning the addresses boundaries to block
and page/sector sizes.
Brian ??
with regards, pekon
Powered by BigRock.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Thu, 03 Jul 2014, Gupta, Pekon wrote:
> >From: Brian Norris [mailto:computersforpe...@gmail.com]
> >On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
[...]
> >> +/*
> >> + * NANDi Interrupts (shared by Hamming and BCH controllers)
> >> + */
> >> +static irqreturn_t nandi_irq_handler(int irq, void *dev)
> >> +{
> >> + struct nandi_controller *nandi = dev;
> >> + unsigned int status;
> >> +
> >> + status = readl(nandi->base + NANDBCH_INT_STA);
> >> +
> >> + if (status & NANDBCH_INT_SEQNODESOVER) {
> >> + /* BCH */
> >> + writel(NANDBCH_INT_CLR_SEQNODESOVER,
> >> + nandi->base + NANDBCH_INT_CLR);
> >> + complete(>seq_completed);
> >> + }
> >> + if (status & NAND_INT_RBN) {
> >> + /* Hamming */
> >> + writel(NAND_INT_CLR_RBN, nandi->base + NANDHAM_INT_CLR);
> >> + complete(>rbn_completed);
> >> + }
> >> +
> >> + return IRQ_HANDLED;
>
> Did you miss following comments ?
> [RFC 05/47] mtd: nand: stm_nand_bch: IRQ support for ST's BCH NAND Controller
> driver
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052916.html
>
> Shouldn't IRQ_NONE be returned if no valid irq_status was found ?
I don't recall seeing these comments, so yes, I think they were
missed. Will fix.
> >> +/*
> >> + * BCH Operations
> >> + */
> >> +static inline void bch_load_prog_cpu(struct nandi_controller *nandi,
> >> + struct bch_prog *prog)
> >> +{
> >> + uint32_t *src = (uint32_t *)prog;
> >> + uint32_t *dst = (uint32_t *)(nandi->base + NANDBCH_ADDRESS_REG_1);
> >> + int i;
> >> +
> >> + for (i = 0; i < 16; i++) {
> >> + /* Skip registers marked as "reserved" */
> >> + if (i != 11 && i != 14)
>
> Using macros for 11, 14, and 16 would make it more readable.
I think with the comment there, it's perfectly clear what's happening.
> >> + writel(*src, dst);
> >> + dst++;
> >> + src++;
> >> + }
> >> +}
> >> +
> >> +static void bch_wait_seq(struct nandi_controller *nandi)
> >> +{
> >> + int ret;
> >> +
> >> + ret = wait_for_completion_timeout(>seq_completed, HZ/2);
>
> Are you sure you want to use same timeout value for all operations
> like ERASE, READ, WRITE ? because
> (1) There is wide variance between:
> - BLOCK_ERASE: max(tBER) = 10ms) for MT29F4G08 Micron NAND
> - PAGE_ERASE: max(tPROG) = 600usec for same Micron part.
>
> (2) The value of HZ varies across kernel versions and hardware platforms.
>
> I suggest you pass the timeout value in call to bch_wait_seq().
> And this timeout value should be like 2x of typical value of which you found
> during ONFI parsing, or from DT
How do you obtain these timings? I don't see tBER or tPROG being used
anywhere in MTD.
[...]
> >> +{
> >> + uint8_t *b = data;
> >> + int zeros = 0;
> >> + int i;
> >> +
> >> + for (i = 0; i < page_size; i++) {
> >> + zeros += hweight8(~*b++);
> Are you sure you want to use hweight8 ?
> hweight32 or something should give better performance, plz check.
> because this piece of code (check_xx_bitflips) sometimes becomes
> bottle neck for READ path.
I'm not sure, no. If I change it, how will I know if it's still doing
the correct thing or otherwise?
[...]
> >> + /* Load last page of block */
> >> + offs = (loff_t)block << chip->phys_erase_shift;
> >> + offs += mtd->erasesize - mtd->writesize;
> >> + if (bch_read_page(nandi, offs, buf) < 0) {
>
> *Important*: You shouldn't call internal functions directly here..
> Instead use chip->_read() OR mtd-read() that will:
> - keep this code independent of ECC modes added in your driver in future.
> - implicitly handle updating mtd->ecc_stat (like ecc_failed, bits_corrected).
> - implicitly take care of address alignments checks and offset calculations.
>
> >> + /* Is block already considered bad? (will also catch invalid offsets) */
> >> + ret = mtd_block_isbad(mtd, offs);
> >
> >You're violating some of the layering here; the low-level driver
> >probably shouldn't be calling the high-level mtd_*() APIs. On a similar
> >note, I'm not 100% confident that the nand_base/nand_bbt separation is
> >written cleanly enough for easy maintenance of your nand_base + ST BBT
> >code in parallel. I might need a second look at that, and I think
> >modularizing your BBT code to a separate file could help ease this a
> >little.
... here is the converse argument.
[...]
> >Are you actually looking for driver data, not platform data? (e.g.,
> >dev_set_drvdata() / platform_get_drvdata()) The two are a little
> >different, but your usage sounds like this more of a driver-private
> >description.
On closer inspection it appears that drvdata was already in use. I've
not encompassed the NANDi information and the old pdata into one ddata
container.
[...]
> Sorry, this review got delayed from my part..
> But I hope I have covered most of the review in this and earlier responses.
> if you are able to clean most of these then
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Thu, 03 Jul 2014, Gupta, Pekon wrote:
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
[...]
+/*
+ * NANDi Interrupts (shared by Hamming and BCH controllers)
+ */
+static irqreturn_t nandi_irq_handler(int irq, void *dev)
+{
+ struct nandi_controller *nandi = dev;
+ unsigned int status;
+
+ status = readl(nandi-base + NANDBCH_INT_STA);
+
+ if (status NANDBCH_INT_SEQNODESOVER) {
+ /* BCH */
+ writel(NANDBCH_INT_CLR_SEQNODESOVER,
+ nandi-base + NANDBCH_INT_CLR);
+ complete(nandi-seq_completed);
+ }
+ if (status NAND_INT_RBN) {
+ /* Hamming */
+ writel(NAND_INT_CLR_RBN, nandi-base + NANDHAM_INT_CLR);
+ complete(nandi-rbn_completed);
+ }
+
+ return IRQ_HANDLED;
Did you miss following comments ?
[RFC 05/47] mtd: nand: stm_nand_bch: IRQ support for ST's BCH NAND Controller
driver
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052916.html
Shouldn't IRQ_NONE be returned if no valid irq_status was found ?
I don't recall seeing these comments, so yes, I think they were
missed. Will fix.
+/*
+ * BCH Operations
+ */
+static inline void bch_load_prog_cpu(struct nandi_controller *nandi,
+ struct bch_prog *prog)
+{
+ uint32_t *src = (uint32_t *)prog;
+ uint32_t *dst = (uint32_t *)(nandi-base + NANDBCH_ADDRESS_REG_1);
+ int i;
+
+ for (i = 0; i 16; i++) {
+ /* Skip registers marked as reserved */
+ if (i != 11 i != 14)
Using macros for 11, 14, and 16 would make it more readable.
I think with the comment there, it's perfectly clear what's happening.
+ writel(*src, dst);
+ dst++;
+ src++;
+ }
+}
+
+static void bch_wait_seq(struct nandi_controller *nandi)
+{
+ int ret;
+
+ ret = wait_for_completion_timeout(nandi-seq_completed, HZ/2);
Are you sure you want to use same timeout value for all operations
like ERASE, READ, WRITE ? because
(1) There is wide variance between:
- BLOCK_ERASE: max(tBER) = 10ms) for MT29F4G08 Micron NAND
- PAGE_ERASE: max(tPROG) = 600usec for same Micron part.
(2) The value of HZ varies across kernel versions and hardware platforms.
I suggest you pass the timeout value in call to bch_wait_seq().
And this timeout value should be like 2x of typical value of which you found
during ONFI parsing, or from DT
How do you obtain these timings? I don't see tBER or tPROG being used
anywhere in MTD.
[...]
+{
+ uint8_t *b = data;
+ int zeros = 0;
+ int i;
+
+ for (i = 0; i page_size; i++) {
+ zeros += hweight8(~*b++);
Are you sure you want to use hweight8 ?
hweight32 or something should give better performance, plz check.
because this piece of code (check_xx_bitflips) sometimes becomes
bottle neck for READ path.
I'm not sure, no. If I change it, how will I know if it's still doing
the correct thing or otherwise?
[...]
+ /* Load last page of block */
+ offs = (loff_t)block chip-phys_erase_shift;
+ offs += mtd-erasesize - mtd-writesize;
+ if (bch_read_page(nandi, offs, buf) 0) {
*Important*: You shouldn't call internal functions directly here..
Instead use chip-_read() OR mtd-read() that will:
- keep this code independent of ECC modes added in your driver in future.
- implicitly handle updating mtd-ecc_stat (like ecc_failed, bits_corrected).
- implicitly take care of address alignments checks and offset calculations.
Same applies to other places where you have directly used bch_read_page())
Yourself and Brian seem to disagree on this point. Which is correct?
+ /* Is block already considered bad? (will also catch invalid offsets) */
+ ret = mtd_block_isbad(mtd, offs);
You're violating some of the layering here; the low-level driver
probably shouldn't be calling the high-level mtd_*() APIs. On a similar
note, I'm not 100% confident that the nand_base/nand_bbt separation is
written cleanly enough for easy maintenance of your nand_base + ST BBT
code in parallel. I might need a second look at that, and I think
modularizing your BBT code to a separate file could help ease this a
little.
... here is the converse argument.
[...]
Are you actually looking for driver data, not platform data? (e.g.,
dev_set_drvdata() / platform_get_drvdata()) The two are a little
different, but your usage sounds like this more of a driver-private
description.
On closer inspection it appears that drvdata was already in use. I've
not encompassed the NANDi information and the old pdata into one ddata
container.
[...]
Sorry, this review got delayed from my part..
But I hope I have covered most of the review in this and earlier responses.
if you are able to clean most of these then should be ready for 3.17+.
It's a bit late for v3.17 now, but let's see what we can do about
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hello,
On Tuesday 05 August 2014 07:53 PM, Lee Jones wrote:
On Thu, 03 Jul 2014, Gupta, Pekon wrote:
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
[...]
+static void bch_wait_seq(struct nandi_controller *nandi)
+{
+ int ret;
+
+ ret = wait_for_completion_timeout(nandi-seq_completed, HZ/2);
Are you sure you want to use same timeout value for all operations
like ERASE, READ, WRITE ? because
(1) There is wide variance between:
- BLOCK_ERASE: max(tBER) = 10ms) for MT29F4G08 Micron NAND
- PAGE_ERASE: max(tPROG) = 600usec for same Micron part.
(2) The value of HZ varies across kernel versions and hardware
platforms.
I suggest you pass the timeout value in call to bch_wait_seq().
And this timeout value should be like 2x of typical value of which
you found
during ONFI parsing, or from DT
How do you obtain these timings? I don't see tBER or tPROG being
used
anywhere in MTD.
These are not from NAND framework, these are terms used in data-sheet.
However, you can read all signal timing from ONFI page while probing.
But, as Brian suggested that timeout is a erroneous and rare condition
anyway, so these relaxed timeout values are acceptable. So you may
ignore my previous comment.
[...]
+{
+ uint8_t *b = data;
+ int zeros = 0;
+ int i;
+
+ for (i = 0; i page_size; i++) {
+ zeros += hweight8(~*b++);
Are you sure you want to use hweight8 ?
hweight32 or something should give better performance, plz check.
because this piece of code (check_xx_bitflips) sometimes becomes
bottle neck for READ path.
I'm not sure, no. If I change it, how will I know if it's still
doing
the correct thing or otherwise?
hweight/hweight16/hweight32 are all macros of same family
just operating on different data-widths, so you may continue
for now. But once the driver is merged, you might like to
re-analyze performance gain (especially on MLC NAND) when
using hweight32() instead of hweight8().
[...]
+ /* Load last page of block */
+ offs = (loff_t)block chip-phys_erase_shift;
+ offs += mtd-erasesize - mtd-writesize;
+ if (bch_read_page(nandi, offs, buf) 0) {
*Important*: You shouldn't call internal functions directly here..
Instead use chip-_read() OR mtd-read() that will:
- keep this code independent of ECC modes added in your driver in
future.
- implicitly handle updating mtd-ecc_stat (like ecc_failed,
bits_corrected).
- implicitly take care of address alignments checks and offset
calculations.
Same applies to other places where you have directly used
bch_read_page())
Yourself and Brian seem to disagree on this point. Which is correct?
+ /* Is block already considered bad? (will also catch invalid
offsets) */
+ ret = mtd_block_isbad(mtd, offs);
You're violating some of the layering here; the low-level driver
probably shouldn't be calling the high-level mtd_*() APIs. On a
similar
note, I'm not 100% confident that the nand_base/nand_bbt separation
is
written cleanly enough for easy maintenance of your nand_base + ST
BBT
code in parallel. I might need a second look at that, and I think
modularizing your BBT code to a separate file could help ease this
a
little.
... here is the converse argument.
I think somewhere in earlier comments, Brian also supported
to use high-level function like mtd_read(). Also, nand_bbt.c
itself uses 'mtd_read(), mtd_read_oob() and mtd_write_oob()
at many places. So I'll let Brain decide here.
But having low-level function will add redundant code for
re-checking and aligning the addresses boundaries to block
and page/sector sizes.
Brian ??
with regards, pekon
Powered by BigRock.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Thu, 31 Jul 2014, Brian Norris wrote: > On Thu, Jul 31, 2014 at 05:47:09PM +0100, Lee Jones wrote: > > Finally getting round to this ... > > Sounds like me :) :) > > On Wed, 02 Jul 2014, Brian Norris wrote: > > > On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: > > > > This is a squashed version of the submission to avoid re-sending the > > > > entire set over and over, essentially clogging up the MLs. > > > > > > Thanks. I think I'd prefer to accept your driver in a form like this > > > too. > > > > You mean squashed? I'd _really_ like _not_ to do that. > > Any particular reason? Just to bump your patch count? ;) A half-finished > driver is not really bisectable. I'd be lying if I said that it wasn't a consideration. I have put more hours into this one driver than I would have 10 others. The bumping of the patch count, as you call it would closer reflect the effort put in. But actually I initially split this patch-set up after a review comment you provided during the upstreaming process of the FSM NOR driver. You mentioned that breaking up the driver made it easier to review as I guess you could review a couple of patches, then take a break without the fear of duplicated work. Admittedly, I went a little overboard with this submission - it appears that I underestimated the size of the driver and subsequently misjudged the function split. > > We've spoken > > about this in reasonable depth before. I agreed to squash it for this > > review to keep the submissions nice and light, but we did have an > > agreement that once accepted the final delivery could be via > > pull-request. I'd really like it if you honoured that. > > I don't recall the part about final delivery, but that's what happens > via informal and unlogged communication like IRC, I guess. As I recall, > I just mentioned that you could link people to your git history if you > felt it was still useful. > > Anyway, as long as the review process is sane (1 new driver = small > number of patches), then I don't particularly mind how the final > delivery comes. > > > I can pull the vendor specific (rather than > > legacy :) ) BBT handling out and protect it with a CONFIG option. > > A CONFIG option perhaps; but you also might need to specify what > 'nand-on-flash-bbt' means in your DT. Personally, I'd prefer that > 'nand-on-flash-bbt' refers only to one of the BBT's found in nand_bbt.c. > Maybe you could extend nand-on-flash-bbt to have a value, like: > > nand-on-flash-bbt = "ST"; > > Just a thought. I don't know if this is overdesign, or proper DT safety > (the whole "DT as ABI" thing is tricky, especially when dealing with > softer things like this). I'm not sure it means anything to us specifically. I think we're only specifying it to keep the framework quiet. If using the vendor specific BBT, then the factory set BBT is always scanned for and the resultant BBT it's always stored in-band (i.e. in flash). If using the framework's version of BBT, then NAND_BBT_USE_FLASH will mean the same thing as it does everywhere else. > > > > Documentation/devicetree/bindings/mtd/stm-nand.txt | 87 + > > > > > > See: > > > > > > Documentation/devicetree/bindings/submitting-patches.txt > > > > This file doesn't exist. What are you referencing? > > commit 2a9330010bea5982a5c6593824bc036bf62d67b7 > Author: Jason Cooper > Date: Tue Dec 17 16:59:40 2013 + > > dt/bindings: submitting patches and ABI documents Ah, looks like I need to rebase this branch. Jeez, this (NAND BCH) patch-set has been around since forever! > > > You're missing devicet...@vger.kernel.org on CC, and the binding doc > > > needs a separate patch. (Sorry if I confused this one earlier.) > > > > The binding document will certainly be a separate patch. This patch > > is the 'everything squashed' version which was requested. > > I was only referring to the driver. For the DT guys' sake, you should > still follow their rules for patch review. So yes, I am fully aware of the rules about submitting drivers containing DT bindings. As mentioned before, I have no intention of sending this whole series as onebigpatch (TM). > > > > arch/arm/boot/dts/stih41x-b2020.dtsi | 40 + > > > > > > This will need refreshed and sent as a separate patch, to go through the > > > appropriate ARM tree. > > > > As above. > > OK, if you're really planning to send a final git pull request, you'll > need to have this in a separate branch for them to take, then. Of course. Same goes with the DTS(I) changes. > > > > diff --git a/Documentation/devicetree/bindings/mtd/stm-nand.txt > > > > b/Documentation/devicetree/bindings/mtd/stm-nand.txt > > > > new file mode 100644 > > > > index 000..d957f49 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/mtd/stm-nand.txt > > > > @@ -0,0 +1,87 @@ > > > > +STM BCH NAND Support > > > > + > > > > + > > > > +Required properties: > > > > + > > > > +-
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Thu, 31 Jul 2014, Brian Norris wrote: On Thu, Jul 31, 2014 at 05:47:09PM +0100, Lee Jones wrote: Finally getting round to this ... Sounds like me :) :) On Wed, 02 Jul 2014, Brian Norris wrote: On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: This is a squashed version of the submission to avoid re-sending the entire set over and over, essentially clogging up the MLs. Thanks. I think I'd prefer to accept your driver in a form like this too. You mean squashed? I'd _really_ like _not_ to do that. Any particular reason? Just to bump your patch count? ;) A half-finished driver is not really bisectable. I'd be lying if I said that it wasn't a consideration. I have put more hours into this one driver than I would have 10 others. The bumping of the patch count, as you call it would closer reflect the effort put in. But actually I initially split this patch-set up after a review comment you provided during the upstreaming process of the FSM NOR driver. You mentioned that breaking up the driver made it easier to review as I guess you could review a couple of patches, then take a break without the fear of duplicated work. Admittedly, I went a little overboard with this submission - it appears that I underestimated the size of the driver and subsequently misjudged the function split. We've spoken about this in reasonable depth before. I agreed to squash it for this review to keep the submissions nice and light, but we did have an agreement that once accepted the final delivery could be via pull-request. I'd really like it if you honoured that. I don't recall the part about final delivery, but that's what happens via informal and unlogged communication like IRC, I guess. As I recall, I just mentioned that you could link people to your git history if you felt it was still useful. Anyway, as long as the review process is sane (1 new driver = small number of patches), then I don't particularly mind how the final delivery comes. I can pull the vendor specific (rather than legacy :) ) BBT handling out and protect it with a CONFIG option. A CONFIG option perhaps; but you also might need to specify what 'nand-on-flash-bbt' means in your DT. Personally, I'd prefer that 'nand-on-flash-bbt' refers only to one of the BBT's found in nand_bbt.c. Maybe you could extend nand-on-flash-bbt to have a value, like: nand-on-flash-bbt = ST; Just a thought. I don't know if this is overdesign, or proper DT safety (the whole DT as ABI thing is tricky, especially when dealing with softer things like this). I'm not sure it means anything to us specifically. I think we're only specifying it to keep the framework quiet. If using the vendor specific BBT, then the factory set BBT is always scanned for and the resultant BBT it's always stored in-band (i.e. in flash). If using the framework's version of BBT, then NAND_BBT_USE_FLASH will mean the same thing as it does everywhere else. Documentation/devicetree/bindings/mtd/stm-nand.txt | 87 + See: Documentation/devicetree/bindings/submitting-patches.txt This file doesn't exist. What are you referencing? commit 2a9330010bea5982a5c6593824bc036bf62d67b7 Author: Jason Cooper ja...@lakedaemon.net Date: Tue Dec 17 16:59:40 2013 + dt/bindings: submitting patches and ABI documents Ah, looks like I need to rebase this branch. Jeez, this (NAND BCH) patch-set has been around since forever! You're missing devicet...@vger.kernel.org on CC, and the binding doc needs a separate patch. (Sorry if I confused this one earlier.) The binding document will certainly be a separate patch. This patch is the 'everything squashed' version which was requested. I was only referring to the driver. For the DT guys' sake, you should still follow their rules for patch review. So yes, I am fully aware of the rules about submitting drivers containing DT bindings. As mentioned before, I have no intention of sending this whole series as onebigpatch (TM). arch/arm/boot/dts/stih41x-b2020.dtsi | 40 + This will need refreshed and sent as a separate patch, to go through the appropriate ARM tree. As above. OK, if you're really planning to send a final git pull request, you'll need to have this in a separate branch for them to take, then. Of course. Same goes with the DTS(I) changes. diff --git a/Documentation/devicetree/bindings/mtd/stm-nand.txt b/Documentation/devicetree/bindings/mtd/stm-nand.txt new file mode 100644 index 000..d957f49 --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/stm-nand.txt @@ -0,0 +1,87 @@ +STM BCH NAND Support + + +Required properties: + +- compatible : Should be st,nand-bch +- reg : Should contain register's location and length +- reg-names: nand_mem - NAND
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Thu, Jul 31, 2014 at 05:47:09PM +0100, Lee Jones wrote:
> Finally getting round to this ...
Sounds like me :)
> On Wed, 02 Jul 2014, Brian Norris wrote:
> > On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
> > > This is a squashed version of the submission to avoid re-sending the
> > > entire set over and over, essentially clogging up the MLs.
> >
> > Thanks. I think I'd prefer to accept your driver in a form like this
> > too.
>
> You mean squashed? I'd _really_ like _not_ to do that.
Any particular reason? Just to bump your patch count? ;) A half-finished
driver is not really bisectable.
> We've spoken
> about this in reasonable depth before. I agreed to squash it for this
> review to keep the submissions nice and light, but we did have an
> agreement that once accepted the final delivery could be via
> pull-request. I'd really like it if you honoured that.
I don't recall the part about final delivery, but that's what happens
via informal and unlogged communication like IRC, I guess. As I recall,
I just mentioned that you could link people to your git history if you
felt it was still useful.
Anyway, as long as the review process is sane (1 new driver = small
number of patches), then I don't particularly mind how the final
delivery comes.
> I can pull the vendor specific (rather than
> legacy :) ) BBT handling out and protect it with a CONFIG option.
A CONFIG option perhaps; but you also might need to specify what
'nand-on-flash-bbt' means in your DT. Personally, I'd prefer that
'nand-on-flash-bbt' refers only to one of the BBT's found in nand_bbt.c.
Maybe you could extend nand-on-flash-bbt to have a value, like:
nand-on-flash-bbt = "ST";
Just a thought. I don't know if this is overdesign, or proper DT safety
(the whole "DT as ABI" thing is tricky, especially when dealing with
softer things like this).
> > > Documentation/devicetree/bindings/mtd/stm-nand.txt | 87 +
> >
> > See:
> >
> > Documentation/devicetree/bindings/submitting-patches.txt
>
> This file doesn't exist. What are you referencing?
commit 2a9330010bea5982a5c6593824bc036bf62d67b7
Author: Jason Cooper
Date: Tue Dec 17 16:59:40 2013 +
dt/bindings: submitting patches and ABI documents
> > You're missing devicet...@vger.kernel.org on CC, and the binding doc
> > needs a separate patch. (Sorry if I confused this one earlier.)
>
> The binding document will certainly be a separate patch. This patch
> is the 'everything squashed' version which was requested.
I was only referring to the driver. For the DT guys' sake, you should
still follow their rules for patch review.
> > > arch/arm/boot/dts/stih41x-b2020.dtsi | 40 +
> >
> > This will need refreshed and sent as a separate patch, to go through the
> > appropriate ARM tree.
>
> As above.
OK, if you're really planning to send a final git pull request, you'll
need to have this in a separate branch for them to take, then.
...
> > > diff --git a/Documentation/devicetree/bindings/mtd/stm-nand.txt
> > > b/Documentation/devicetree/bindings/mtd/stm-nand.txt
> > > new file mode 100644
> > > index 000..d957f49
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mtd/stm-nand.txt
> > > @@ -0,0 +1,87 @@
> > > +STM BCH NAND Support
> > > +
> > > +
> > > +Required properties:
> > > +
> > > +- compatible : Should be "st,nand-bch"
> > > +- reg: Should contain register's location and length
> > > +- reg-names : "nand_mem" - NAND Controller register map
> > > + "nand_dma" - BCH Controller DMA configuration map
> > > +- interrupts : Interrupt number
> > > +- interrupt-names: "nand_irq" - NAND Controller IRQ
> > > +- st,nand-banks : Subnode representing one or more "banks" of
> > > NAND
> > > + Flash, connected to an STM NAND Controller (see
> > > + description below).
> > > +- nand-ecc-strength : Generic NAND property (See mtd/nand.txt)
> > > + Options are; 0, 18, 30 or 0xFF (AUTO)
> >
> > What is 0xFF (AUTO)?
Could you answer this one? IIRC, 0xff (AUTO) could easily be replaced by
language that "If nand-ecc-strength is not present", software is free to
auto-select the ECC strength. That removes this non-intuitive special
value.
Also, nand-ecc-strength should probably be paired with
nand-ecc-step-size, otherwise it doesn't really have any meaning.
...
> > > +Example:
> > > +
...
> > > + nand_banks: nand-banks {
> > > + bank0 {
...
> > > + partitions {
...
> > > + partition@0{
> > > + label = "NAND Flash 1";
> >
> > Do you really want spaces in your partition names?
>
> No clue to be honest. What are the known ramifications?
I thought it might give issues with the /proc/mtd columns, but actually,
the format there is quoted, so the columns will still be unambiguous.
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Finally getting round to this ...
On Wed, 02 Jul 2014, Brian Norris wrote:
> On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
> > This is a squashed version of the submission to avoid re-sending the
> > entire set over and over, essentially clogging up the MLs.
>
> Thanks. I think I'd prefer to accept your driver in a form like this
> too.
You mean squashed? I'd _really_ like _not_ to do that. We've spoken
about this in reasonable depth before. I agreed to squash it for this
review to keep the submissions nice and light, but we did have an
agreement that once accepted the final delivery could be via
pull-request. I'd really like it if you honoured that.
> A few comments below.
> And I'll get one big comment out of the way here: can you abstract your
> ST BBT code into its own self-contained portion, preferably in a
> separate source file, a la nand_bbt.c? Then, provide a way to optionally
> use either your ST BBT or the existing BBT -- perhaps a NAND_BBT_ST flag
> for chip->bbt_options, and a matching device tree property. That way,
> even though you require a legacy format for bootloader interoperability,
> someone can theoretically utilize more mainstream (albeit, not
> necessarily better...) BBT support from nand_bbt.c. I think this will
> provide the best balance between your existing product support and
> upstream-friendly modularity/flexibility. I'm open to other suggestions,
> of course.
Sounds reasonable. I can pull the vendor specific (rather than
legacy :) ) BBT handling out and protect it with a CONFIG option.
> > Cc: computersforpe...@gmail.com
> > Cc: Gupta, Pekon"
> > Cc: Ezequiel Garcia
> > Cc: linux-...@lists.infradead.org
> > Signed-off-by: Lee Jones
> > ---
>
> Please add versioning to your next patch(es), and describe changes here.
Certainly.
> > Documentation/devicetree/bindings/mtd/stm-nand.txt | 87 +
>
> See:
>
> Documentation/devicetree/bindings/submitting-patches.txt
This file doesn't exist. What are you referencing?
> You're missing devicet...@vger.kernel.org on CC, and the binding doc
> needs a separate patch. (Sorry if I confused this one earlier.)
The binding document will certainly be a separate patch. This patch
is the 'everything squashed' version which was requested.
> > arch/arm/boot/dts/stih41x-b2020.dtsi | 40 +
>
> This will need refreshed and sent as a separate patch, to go through the
> appropriate ARM tree.
As above.
> > drivers/mtd/nand/Kconfig | 14 +
> > drivers/mtd/nand/Makefile |2 +
> > drivers/mtd/nand/stm_nand_bch.c| 2415
> >
> > drivers/mtd/nand/stm_nand_dt.c | 116 +
> > drivers/mtd/nand/stm_nand_dt.h | 39 +
> > drivers/mtd/nand/stm_nand_regs.h | 302 +++
> > include/linux/mtd/stm_nand.h | 104 +
> > 9 files changed, 3119 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mtd/stm-nand.txt
> > create mode 100644 drivers/mtd/nand/stm_nand_bch.c
> > create mode 100644 drivers/mtd/nand/stm_nand_dt.c
> > create mode 100644 drivers/mtd/nand/stm_nand_dt.h
> > create mode 100644 drivers/mtd/nand/stm_nand_regs.h
> > create mode 100644 include/linux/mtd/stm_nand.h
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/stm-nand.txt
> > b/Documentation/devicetree/bindings/mtd/stm-nand.txt
> > new file mode 100644
> > index 000..d957f49
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/stm-nand.txt
> > @@ -0,0 +1,87 @@
> > +STM BCH NAND Support
> > +
> > +
> > +Required properties:
> > +
> > +- compatible : Should be "st,nand-bch"
> > +- reg : Should contain register's location and length
> > +- reg-names: "nand_mem" - NAND Controller register map
> > + "nand_dma" - BCH Controller DMA configuration map
> > +- interrupts : Interrupt number
> > +- interrupt-names : "nand_irq" - NAND Controller IRQ
> > +- st,nand-banks: Subnode representing one or more "banks" of
> > NAND
> > + Flash, connected to an STM NAND Controller (see
> > + description below).
> > +- nand-ecc-strength: Generic NAND property (See mtd/nand.txt)
> > + Options are; 0, 18, 30 or 0xFF (AUTO)
>
> What is 0xFF (AUTO)?
>
> > +
> > +Properties describing Bank of NAND Flash ("st,nand-banks"):
> > +
> > +- st,nand-csn : Chip select associated with the Bank.
> > +
> > +- st,nand-timing-relax : [Optional] Number of IP clock cycles by which
> > to
> > + "relax" timing configuration. Required on some boards
> > + to accommodate board-level limitations. Applies to
> > + ONFI timing mode configuration.
> > +
> > +- nand-on-flash-bbt: Generic NAND
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Finally getting round to this ... On Wed, 02 Jul 2014, Brian Norris wrote: On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: This is a squashed version of the submission to avoid re-sending the entire set over and over, essentially clogging up the MLs. Thanks. I think I'd prefer to accept your driver in a form like this too. You mean squashed? I'd _really_ like _not_ to do that. We've spoken about this in reasonable depth before. I agreed to squash it for this review to keep the submissions nice and light, but we did have an agreement that once accepted the final delivery could be via pull-request. I'd really like it if you honoured that. A few comments below. And I'll get one big comment out of the way here: can you abstract your ST BBT code into its own self-contained portion, preferably in a separate source file, a la nand_bbt.c? Then, provide a way to optionally use either your ST BBT or the existing BBT -- perhaps a NAND_BBT_ST flag for chip-bbt_options, and a matching device tree property. That way, even though you require a legacy format for bootloader interoperability, someone can theoretically utilize more mainstream (albeit, not necessarily better...) BBT support from nand_bbt.c. I think this will provide the best balance between your existing product support and upstream-friendly modularity/flexibility. I'm open to other suggestions, of course. Sounds reasonable. I can pull the vendor specific (rather than legacy :) ) BBT handling out and protect it with a CONFIG option. Cc: computersforpe...@gmail.com Cc: Gupta, Pekon pe...@ti.com Cc: Ezequiel Garcia ezequiel.gar...@free-electrons.com Cc: linux-...@lists.infradead.org Signed-off-by: Lee Jones lee.jo...@linaro.org --- Please add versioning to your next patch(es), and describe changes here. Certainly. Documentation/devicetree/bindings/mtd/stm-nand.txt | 87 + See: Documentation/devicetree/bindings/submitting-patches.txt This file doesn't exist. What are you referencing? You're missing devicet...@vger.kernel.org on CC, and the binding doc needs a separate patch. (Sorry if I confused this one earlier.) The binding document will certainly be a separate patch. This patch is the 'everything squashed' version which was requested. arch/arm/boot/dts/stih41x-b2020.dtsi | 40 + This will need refreshed and sent as a separate patch, to go through the appropriate ARM tree. As above. drivers/mtd/nand/Kconfig | 14 + drivers/mtd/nand/Makefile |2 + drivers/mtd/nand/stm_nand_bch.c| 2415 drivers/mtd/nand/stm_nand_dt.c | 116 + drivers/mtd/nand/stm_nand_dt.h | 39 + drivers/mtd/nand/stm_nand_regs.h | 302 +++ include/linux/mtd/stm_nand.h | 104 + 9 files changed, 3119 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/stm-nand.txt create mode 100644 drivers/mtd/nand/stm_nand_bch.c create mode 100644 drivers/mtd/nand/stm_nand_dt.c create mode 100644 drivers/mtd/nand/stm_nand_dt.h create mode 100644 drivers/mtd/nand/stm_nand_regs.h create mode 100644 include/linux/mtd/stm_nand.h diff --git a/Documentation/devicetree/bindings/mtd/stm-nand.txt b/Documentation/devicetree/bindings/mtd/stm-nand.txt new file mode 100644 index 000..d957f49 --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/stm-nand.txt @@ -0,0 +1,87 @@ +STM BCH NAND Support + + +Required properties: + +- compatible : Should be st,nand-bch +- reg : Should contain register's location and length +- reg-names: nand_mem - NAND Controller register map + nand_dma - BCH Controller DMA configuration map +- interrupts : Interrupt number +- interrupt-names : nand_irq - NAND Controller IRQ +- st,nand-banks: Subnode representing one or more banks of NAND + Flash, connected to an STM NAND Controller (see + description below). +- nand-ecc-strength: Generic NAND property (See mtd/nand.txt) + Options are; 0, 18, 30 or 0xFF (AUTO) What is 0xFF (AUTO)? + +Properties describing Bank of NAND Flash (st,nand-banks): + +- st,nand-csn : Chip select associated with the Bank. + +- st,nand-timing-relax : [Optional] Number of IP clock cycles by which to + relax timing configuration. Required on some boards + to accommodate board-level limitations. Applies to + ONFI timing mode configuration. + +- nand-on-flash-bbt: Generic NAND property (See mtd/nand.txt) + +- partitions : [Optional] Subnode describing MTD partition
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
On Thu, Jul 31, 2014 at 05:47:09PM +0100, Lee Jones wrote:
Finally getting round to this ...
Sounds like me :)
On Wed, 02 Jul 2014, Brian Norris wrote:
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
This is a squashed version of the submission to avoid re-sending the
entire set over and over, essentially clogging up the MLs.
Thanks. I think I'd prefer to accept your driver in a form like this
too.
You mean squashed? I'd _really_ like _not_ to do that.
Any particular reason? Just to bump your patch count? ;) A half-finished
driver is not really bisectable.
We've spoken
about this in reasonable depth before. I agreed to squash it for this
review to keep the submissions nice and light, but we did have an
agreement that once accepted the final delivery could be via
pull-request. I'd really like it if you honoured that.
I don't recall the part about final delivery, but that's what happens
via informal and unlogged communication like IRC, I guess. As I recall,
I just mentioned that you could link people to your git history if you
felt it was still useful.
Anyway, as long as the review process is sane (1 new driver = small
number of patches), then I don't particularly mind how the final
delivery comes.
I can pull the vendor specific (rather than
legacy :) ) BBT handling out and protect it with a CONFIG option.
A CONFIG option perhaps; but you also might need to specify what
'nand-on-flash-bbt' means in your DT. Personally, I'd prefer that
'nand-on-flash-bbt' refers only to one of the BBT's found in nand_bbt.c.
Maybe you could extend nand-on-flash-bbt to have a value, like:
nand-on-flash-bbt = ST;
Just a thought. I don't know if this is overdesign, or proper DT safety
(the whole DT as ABI thing is tricky, especially when dealing with
softer things like this).
Documentation/devicetree/bindings/mtd/stm-nand.txt | 87 +
See:
Documentation/devicetree/bindings/submitting-patches.txt
This file doesn't exist. What are you referencing?
commit 2a9330010bea5982a5c6593824bc036bf62d67b7
Author: Jason Cooper ja...@lakedaemon.net
Date: Tue Dec 17 16:59:40 2013 +
dt/bindings: submitting patches and ABI documents
You're missing devicet...@vger.kernel.org on CC, and the binding doc
needs a separate patch. (Sorry if I confused this one earlier.)
The binding document will certainly be a separate patch. This patch
is the 'everything squashed' version which was requested.
I was only referring to the driver. For the DT guys' sake, you should
still follow their rules for patch review.
arch/arm/boot/dts/stih41x-b2020.dtsi | 40 +
This will need refreshed and sent as a separate patch, to go through the
appropriate ARM tree.
As above.
OK, if you're really planning to send a final git pull request, you'll
need to have this in a separate branch for them to take, then.
...
diff --git a/Documentation/devicetree/bindings/mtd/stm-nand.txt
b/Documentation/devicetree/bindings/mtd/stm-nand.txt
new file mode 100644
index 000..d957f49
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/stm-nand.txt
@@ -0,0 +1,87 @@
+STM BCH NAND Support
+
+
+Required properties:
+
+- compatible : Should be st,nand-bch
+- reg: Should contain register's location and length
+- reg-names : nand_mem - NAND Controller register map
+ nand_dma - BCH Controller DMA configuration map
+- interrupts : Interrupt number
+- interrupt-names: nand_irq - NAND Controller IRQ
+- st,nand-banks : Subnode representing one or more banks of
NAND
+ Flash, connected to an STM NAND Controller (see
+ description below).
+- nand-ecc-strength : Generic NAND property (See mtd/nand.txt)
+ Options are; 0, 18, 30 or 0xFF (AUTO)
What is 0xFF (AUTO)?
Could you answer this one? IIRC, 0xff (AUTO) could easily be replaced by
language that If nand-ecc-strength is not present, software is free to
auto-select the ECC strength. That removes this non-intuitive special
value.
Also, nand-ecc-strength should probably be paired with
nand-ecc-step-size, otherwise it doesn't really have any meaning.
...
+Example:
+
...
+ nand_banks: nand-banks {
+ bank0 {
...
+ partitions {
...
+ partition@0{
+ label = NAND Flash 1;
Do you really want spaces in your partition names?
No clue to be honest. What are the known ramifications?
I thought it might give issues with the /proc/mtd columns, but actually,
the format there is quoted, so the columns will still be unambiguous.
It also could make things a little more difficult with boot cmdline
parameters, for instance if you want to specify a partition for UBI to
attach to:
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Boris, On Tue, Jul 08, 2014 at 09:58:55AM +0200, Boris BREZILLON wrote: > On Mon, 7 Jul 2014 16:52:22 -0700 Brian Norris > wrote: > > On Thu, Jul 03, 2014 at 10:05:22AM +0200, Boris BREZILLON wrote: > > > On Wed, 2 Jul 2014 17:22:37 -0700 Brian Norris > > > wrote: > > > > > > AFAIR, the NAND timing representation for non-ONFI chips question was > > > left unanswered: > > > > > > https://lkml.org/lkml/2014/5/20/581 > > > > > > I can definitely respin my NAND timings series, but I'd like to be sure > > > this is how you want it done before doing so. > > > > Can we start by supporting ONFI-only (or ONFI-only, plus entries in > > nand_flash_ids[]), and have nand_base provide the translation so drivers > > can retrieve the info? Then we can begin supporting new drivers like > > Lee's, and worry about the DT question separately. > > So, basically, I just send a new series with patch 1 and 2 from my sunxi > NAND series [1] (including the fixes you suggested, of course), right ? > > [1] http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/7977 Yes, please send 1 and 2 on their own. I think that would be a good start for supporting these drivers. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Boris, On Tue, Jul 08, 2014 at 09:58:55AM +0200, Boris BREZILLON wrote: On Mon, 7 Jul 2014 16:52:22 -0700 Brian Norris computersforpe...@gmail.com wrote: On Thu, Jul 03, 2014 at 10:05:22AM +0200, Boris BREZILLON wrote: On Wed, 2 Jul 2014 17:22:37 -0700 Brian Norris computersforpe...@gmail.com wrote: AFAIR, the NAND timing representation for non-ONFI chips question was left unanswered: https://lkml.org/lkml/2014/5/20/581 I can definitely respin my NAND timings series, but I'd like to be sure this is how you want it done before doing so. Can we start by supporting ONFI-only (or ONFI-only, plus entries in nand_flash_ids[]), and have nand_base provide the translation so drivers can retrieve the info? Then we can begin supporting new drivers like Lee's, and worry about the DT question separately. So, basically, I just send a new series with patch 1 and 2 from my sunxi NAND series [1] (including the fixes you suggested, of course), right ? [1] http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/7977 Yes, please send 1 and 2 on their own. I think that would be a good start for supporting these drivers. Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Brian,
On Mon, 7 Jul 2014 16:52:22 -0700
Brian Norris wrote:
> Hi Boris,
>
> On Thu, Jul 03, 2014 at 10:05:22AM +0200, Boris BREZILLON wrote:
> > On Wed, 2 Jul 2014 17:22:37 -0700 Brian Norris
> > wrote:
> > > On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
> > > > +
> > > > + nand_timing0: nand-timing {
> > > > + sig-setup = <10>;
> > > > + sig-hold= <10>;
> > > > + CE-deassert = <0>;
> > > > + WE-to-RBn = <100>;
> > > > + wr-on = <10>;
> > > > + wr-off = <30>;
> > > > + rd-on = <10>;
> > > > + rd-off = <30>;
> > > > + chip-delay = <30>; /* delay in us */
> > > > + };
> > >
> > > You didn't document any of this node. And I don't think we want to
> > > specify every single timing parameter in DT; it may make sense to use
> > > Boris Brezillon's approach (I note this further down, in the driver
> > > code) for mapping non-ONFI NAND timings into a compatible ONFI timing
> > > mode. This will greatly simplify the bindings needed, since it's
> > > standardized and auto-detectable in many cases.
> >
> >
> > AFAIR, the NAND timing representation for non-ONFI chips question was
> > left unanswered:
> >
> > https://lkml.org/lkml/2014/5/20/581
> >
> > I can definitely respin my NAND timings series, but I'd like to be sure
> > this is how you want it done before doing so.
>
> Can we start by supporting ONFI-only (or ONFI-only, plus entries in
> nand_flash_ids[]), and have nand_base provide the translation so drivers
> can retrieve the info? Then we can begin supporting new drivers like
> Lee's, and worry about the DT question separately.
So, basically, I just send a new series with patch 1 and 2 from my sunxi
NAND series [1] (including the fixes you suggested, of course), right ?
[1] http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/7977
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Brian,
On Mon, 7 Jul 2014 16:52:22 -0700
Brian Norris computersforpe...@gmail.com wrote:
Hi Boris,
On Thu, Jul 03, 2014 at 10:05:22AM +0200, Boris BREZILLON wrote:
On Wed, 2 Jul 2014 17:22:37 -0700 Brian Norris
computersforpe...@gmail.com wrote:
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
+
+ nand_timing0: nand-timing {
+ sig-setup = 10;
+ sig-hold= 10;
+ CE-deassert = 0;
+ WE-to-RBn = 100;
+ wr-on = 10;
+ wr-off = 30;
+ rd-on = 10;
+ rd-off = 30;
+ chip-delay = 30; /* delay in us */
+ };
You didn't document any of this node. And I don't think we want to
specify every single timing parameter in DT; it may make sense to use
Boris Brezillon's approach (I note this further down, in the driver
code) for mapping non-ONFI NAND timings into a compatible ONFI timing
mode. This will greatly simplify the bindings needed, since it's
standardized and auto-detectable in many cases.
AFAIR, the NAND timing representation for non-ONFI chips question was
left unanswered:
https://lkml.org/lkml/2014/5/20/581
I can definitely respin my NAND timings series, but I'd like to be sure
this is how you want it done before doing so.
Can we start by supporting ONFI-only (or ONFI-only, plus entries in
nand_flash_ids[]), and have nand_base provide the translation so drivers
can retrieve the info? Then we can begin supporting new drivers like
Lee's, and worry about the DT question separately.
So, basically, I just send a new series with patch 1 and 2 from my sunxi
NAND series [1] (including the fixes you suggested, of course), right ?
[1] http://thread.gmane.org/gmane.comp.hardware.netbook.arm.sunxi/7977
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Lee, Pekon,
A few additions/corrections to Pekon's comments.
On Thu, Jul 03, 2014 at 09:09:27AM +, Pekon Gupta wrote:
> >From: Brian Norris [mailto:computersforpe...@gmail.com]
> >On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
> >> diff --git a/arch/arm/boot/dts/stih41x-b2020.dtsi
> >> b/arch/arm/boot/dts/stih41x-b2020.dtsi
> >> index bc5818d..7a6a6e8 100644
> >> --- a/arch/arm/boot/dts/stih41x-b2020.dtsi
> >> +++ b/arch/arm/boot/dts/stih41x-b2020.dtsi
> >> @@ -52,5 +52,45 @@
> >>pinctrl-0 = <_rgmii1>;
> >>};
> >>
> >> + nandbch: nand-bch {
> >
> >Shouldn't this be named 'nand@', 'flash@', or similar?
> >
> >> + compatible = "st,nand-bch";
> >> + reg = <0xfe901000 0x1000>, <0xfef00800 0x0800>;
> >> + reg-names = "nand_mem", "nand_dma";
> >> + interrupts = <0 139 0x0>;
> >> + interrupt-names = "nand_irq";
> >> + st,nand-banks = <_banks>;
> >> + nand-ecc-strength = <0xFF>;
>
> nit-pick to save yourself from another patch re-spin, Please use lower-case
> hex :-)
Or in this case, drop the property altogether. I believe
nand-ecc-strength = 255 is a misuse of the binding, as Lee intended it
to mean "automatic". (It makes more sense to say that a missing
"nand-ecc-strength" property means Linux can 'automatically' choose.)
> >> +
> >> + status = "okay";
> >> + };
> >> +
...
> >> +static void bch_wait_seq(struct nandi_controller *nandi)
> >> +{
> >> + int ret;
> >> +
> >> + ret = wait_for_completion_timeout(>seq_completed, HZ/2);
>
> Are you sure you want to use same timeout value for all operations
> like ERASE, READ, WRITE ? because
> (1) There is wide variance between:
> - BLOCK_ERASE: max(tBER) = 10ms) for MT29F4G08 Micron NAND
> - PAGE_ERASE: max(tPROG) = 600usec for same Micron part.
>
> (2) The value of HZ varies across kernel versions and hardware platforms.
Lee's use of HZ is correct. HZ represents one second, and it is useful
for giving an (approximate) timeout that is independent of timer tick
rate. It is equivalent to msecs_to_jiffies(1000), for example.
> I suggest you pass the timeout value in call to bch_wait_seq().
> And this timeout value should be like 2x of typical value of which you found
> during ONFI parsing, or from DT
I'm not sure it's very important to get a precise timeout value. It's
especially not worth a DT binding. Timeouts should be the exception, not
the rule, and should not be relied on (for example) for good performance.
> >> + if (!ret)
> >> + dev_err(nandi->dev, "BCH Seq timeout\n");
> >> +}
> >> +
...
> >> + /* Load last page of block */
> >> + offs = (loff_t)block << chip->phys_erase_shift;
> >> + offs += mtd->erasesize - mtd->writesize;
> >> + if (bch_read_page(nandi, offs, buf) < 0) {
>
> *Important*: You shouldn't call internal functions directly here..
> Instead use chip->_read() OR mtd-read() that will:
Probably looking for mtd_read()?
> - keep this code independent of ECC modes added in your driver in future.
> - implicitly handle updating mtd->ecc_stat (like ecc_failed, bits_corrected).
> - implicitly take care of address alignments checks and offset calculations.
>
> >> +struct stm_plat_nand_bch_data {
> >> + struct stm_nand_bank_data *bank;
> >> + enum stm_nand_bch_ecc_config bch_ecc_cfg;
> >> +
> >> + /* The threshold at which the number of corrected bit-flips per sector
> >> + * is deemed to have reached an excessive level (triggers '-EUCLEAN' to
> >> + * be returned to the caller). The value should be in the range 1 to
> >> + * where is 18 or 30, depending on the BCH
> >> + * ECC mode in operation. A value of 0 is interpreted by the driver as
> >> + * .
> >> + */
> >> + unsigned int bch_bitflip_threshold;
> >
> >This field is never set or used...
> >
> >> + bool flashss;
> >
> >Ditto.
> >
> Probably answered in [2]
OK, then mark it with a FIXME or TODO comment. Don't let every reader
come across this driver with the same questions.
> Please re-send the next version in similar squashed format, as its easy for
> review.
Yes.
> And once Brian is okay, may be then you can re-send individual patches.
I don't see any need to send individual sub-patches for the driver (you
do, however, need to split out the DTS and Documentation/ portions as
separate patches). If you want to include the splitting, just link to a
git tree where patches can be found.
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Boris,
On Thu, Jul 03, 2014 at 10:05:22AM +0200, Boris BREZILLON wrote:
> On Wed, 2 Jul 2014 17:22:37 -0700 Brian Norris
> wrote:
> > On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
> > > +
> > > + nand_timing0: nand-timing {
> > > + sig-setup = <10>;
> > > + sig-hold= <10>;
> > > + CE-deassert = <0>;
> > > + WE-to-RBn = <100>;
> > > + wr-on = <10>;
> > > + wr-off = <30>;
> > > + rd-on = <10>;
> > > + rd-off = <30>;
> > > + chip-delay = <30>; /* delay in us */
> > > + };
> >
> > You didn't document any of this node. And I don't think we want to
> > specify every single timing parameter in DT; it may make sense to use
> > Boris Brezillon's approach (I note this further down, in the driver
> > code) for mapping non-ONFI NAND timings into a compatible ONFI timing
> > mode. This will greatly simplify the bindings needed, since it's
> > standardized and auto-detectable in many cases.
>
>
> AFAIR, the NAND timing representation for non-ONFI chips question was
> left unanswered:
>
> https://lkml.org/lkml/2014/5/20/581
>
> I can definitely respin my NAND timings series, but I'd like to be sure
> this is how you want it done before doing so.
Can we start by supporting ONFI-only (or ONFI-only, plus entries in
nand_flash_ids[]), and have nand_base provide the translation so drivers
can retrieve the info? Then we can begin supporting new drivers like
Lee's, and worry about the DT question separately.
BTW, Lee: you're completely missing the definitions for
'struct nand_sdr_timings' in this patch, so it doesn't compile.
> Just as a reminder, you and Jason thought NAND timings for non-ONFI
> chips could be auto detected thanks to READID informations (by storing
> some sort of "NANDID <-> timings" association table).
Yes, thanks for the reminder. I knew there was more than one reason I
was wary of Lee's patch (first, that it was duplication of another patch
set; and second, than I'm not sure it belongs in DT at all).
I think we should attempt to solve this without any need for DT
bindings, as most other parameters are auto-detectable in some sense
(even if we have to store some NANDID <-> timings tables). I think going
forward, we can expect that new NAND will use a JEDEC or ONFI spec, and
that we shouldn't have to scale nand_flash_ids[] to include too many
flash chip timings.
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Boris,
On Thu, Jul 03, 2014 at 10:05:22AM +0200, Boris BREZILLON wrote:
On Wed, 2 Jul 2014 17:22:37 -0700 Brian Norris computersforpe...@gmail.com
wrote:
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
+
+ nand_timing0: nand-timing {
+ sig-setup = 10;
+ sig-hold= 10;
+ CE-deassert = 0;
+ WE-to-RBn = 100;
+ wr-on = 10;
+ wr-off = 30;
+ rd-on = 10;
+ rd-off = 30;
+ chip-delay = 30; /* delay in us */
+ };
You didn't document any of this node. And I don't think we want to
specify every single timing parameter in DT; it may make sense to use
Boris Brezillon's approach (I note this further down, in the driver
code) for mapping non-ONFI NAND timings into a compatible ONFI timing
mode. This will greatly simplify the bindings needed, since it's
standardized and auto-detectable in many cases.
AFAIR, the NAND timing representation for non-ONFI chips question was
left unanswered:
https://lkml.org/lkml/2014/5/20/581
I can definitely respin my NAND timings series, but I'd like to be sure
this is how you want it done before doing so.
Can we start by supporting ONFI-only (or ONFI-only, plus entries in
nand_flash_ids[]), and have nand_base provide the translation so drivers
can retrieve the info? Then we can begin supporting new drivers like
Lee's, and worry about the DT question separately.
BTW, Lee: you're completely missing the definitions for
'struct nand_sdr_timings' in this patch, so it doesn't compile.
Just as a reminder, you and Jason thought NAND timings for non-ONFI
chips could be auto detected thanks to READID informations (by storing
some sort of NANDID - timings association table).
Yes, thanks for the reminder. I knew there was more than one reason I
was wary of Lee's patch (first, that it was duplication of another patch
set; and second, than I'm not sure it belongs in DT at all).
I think we should attempt to solve this without any need for DT
bindings, as most other parameters are auto-detectable in some sense
(even if we have to store some NANDID - timings tables). I think going
forward, we can expect that new NAND will use a JEDEC or ONFI spec, and
that we shouldn't have to scale nand_flash_ids[] to include too many
flash chip timings.
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Lee, Pekon,
A few additions/corrections to Pekon's comments.
On Thu, Jul 03, 2014 at 09:09:27AM +, Pekon Gupta wrote:
From: Brian Norris [mailto:computersforpe...@gmail.com]
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
diff --git a/arch/arm/boot/dts/stih41x-b2020.dtsi
b/arch/arm/boot/dts/stih41x-b2020.dtsi
index bc5818d..7a6a6e8 100644
--- a/arch/arm/boot/dts/stih41x-b2020.dtsi
+++ b/arch/arm/boot/dts/stih41x-b2020.dtsi
@@ -52,5 +52,45 @@
pinctrl-0 = pinctrl_rgmii1;
};
+ nandbch: nand-bch {
Shouldn't this be named 'nand@', 'flash@', or similar?
+ compatible = st,nand-bch;
+ reg = 0xfe901000 0x1000, 0xfef00800 0x0800;
+ reg-names = nand_mem, nand_dma;
+ interrupts = 0 139 0x0;
+ interrupt-names = nand_irq;
+ st,nand-banks = nand_banks;
+ nand-ecc-strength = 0xFF;
nit-pick to save yourself from another patch re-spin, Please use lower-case
hex :-)
Or in this case, drop the property altogether. I believe
nand-ecc-strength = 255 is a misuse of the binding, as Lee intended it
to mean automatic. (It makes more sense to say that a missing
nand-ecc-strength property means Linux can 'automatically' choose.)
+
+ status = okay;
+ };
+
...
+static void bch_wait_seq(struct nandi_controller *nandi)
+{
+ int ret;
+
+ ret = wait_for_completion_timeout(nandi-seq_completed, HZ/2);
Are you sure you want to use same timeout value for all operations
like ERASE, READ, WRITE ? because
(1) There is wide variance between:
- BLOCK_ERASE: max(tBER) = 10ms) for MT29F4G08 Micron NAND
- PAGE_ERASE: max(tPROG) = 600usec for same Micron part.
(2) The value of HZ varies across kernel versions and hardware platforms.
Lee's use of HZ is correct. HZ represents one second, and it is useful
for giving an (approximate) timeout that is independent of timer tick
rate. It is equivalent to msecs_to_jiffies(1000), for example.
I suggest you pass the timeout value in call to bch_wait_seq().
And this timeout value should be like 2x of typical value of which you found
during ONFI parsing, or from DT
I'm not sure it's very important to get a precise timeout value. It's
especially not worth a DT binding. Timeouts should be the exception, not
the rule, and should not be relied on (for example) for good performance.
+ if (!ret)
+ dev_err(nandi-dev, BCH Seq timeout\n);
+}
+
...
+ /* Load last page of block */
+ offs = (loff_t)block chip-phys_erase_shift;
+ offs += mtd-erasesize - mtd-writesize;
+ if (bch_read_page(nandi, offs, buf) 0) {
*Important*: You shouldn't call internal functions directly here..
Instead use chip-_read() OR mtd-read() that will:
Probably looking for mtd_read()?
- keep this code independent of ECC modes added in your driver in future.
- implicitly handle updating mtd-ecc_stat (like ecc_failed, bits_corrected).
- implicitly take care of address alignments checks and offset calculations.
Same applies to other places where you have directly used bch_read_page())
I agree. And this would help with my request that you separate your ST
BBT code from the rest of your driver.
+struct stm_plat_nand_bch_data {
+ struct stm_nand_bank_data *bank;
+ enum stm_nand_bch_ecc_config bch_ecc_cfg;
+
+ /* The threshold at which the number of corrected bit-flips per sector
+ * is deemed to have reached an excessive level (triggers '-EUCLEAN' to
+ * be returned to the caller). The value should be in the range 1 to
+ * ecc-strength where ecc-strength is 18 or 30, depending on the BCH
+ * ECC mode in operation. A value of 0 is interpreted by the driver as
+ * ecc-strength.
+ */
+ unsigned int bch_bitflip_threshold;
This field is never set or used...
+ bool flashss;
Ditto.
Probably answered in [2]
OK, then mark it with a FIXME or TODO comment. Don't let every reader
come across this driver with the same questions.
Please re-send the next version in similar squashed format, as its easy for
review.
Yes.
And once Brian is okay, may be then you can re-send individual patches.
I don't see any need to send individual sub-patches for the driver (you
do, however, need to split out the DTS and Documentation/ portions as
separate patches). If you want to include the splitting, just link to a
git tree where patches can be found.
Brian
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Brian,
On Wed, 2 Jul 2014 17:22:37 -0700
Brian Norris wrote:
> Hi Lee,
>
> On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
> > This is a squashed version of the submission to avoid re-sending the
> > entire set over and over, essentially clogging up the MLs.
>
> Thanks. I think I'd prefer to accept your driver in a form like this
> too. A few comments below.
>
> And I'll get one big comment out of the way here: can you abstract your
> ST BBT code into its own self-contained portion, preferably in a
> separate source file, a la nand_bbt.c? Then, provide a way to optionally
> use either your ST BBT or the existing BBT -- perhaps a NAND_BBT_ST flag
> for chip->bbt_options, and a matching device tree property. That way,
> even though you require a legacy format for bootloader interoperability,
> someone can theoretically utilize more mainstream (albeit, not
> necessarily better...) BBT support from nand_bbt.c. I think this will
> provide the best balance between your existing product support and
> upstream-friendly modularity/flexibility. I'm open to other suggestions,
> of course.
>
> > Cc: computersforpe...@gmail.com
> > Cc: Gupta, Pekon"
> > Cc: Ezequiel Garcia
> > Cc: linux-...@lists.infradead.org
> > Signed-off-by: Lee Jones
> > ---
>
> Please add versioning to your next patch(es), and describe changes here.
>
> > Documentation/devicetree/bindings/mtd/stm-nand.txt | 87 +
>
> See:
>
> Documentation/devicetree/bindings/submitting-patches.txt
>
[...]
> > +
> > + nand_timing0: nand-timing {
> > + sig-setup = <10>;
> > + sig-hold= <10>;
> > + CE-deassert = <0>;
> > + WE-to-RBn = <100>;
> > + wr-on = <10>;
> > + wr-off = <30>;
> > + rd-on = <10>;
> > + rd-off = <30>;
> > + chip-delay = <30>; /* delay in us */
> > + };
>
> You didn't document any of this node. And I don't think we want to
> specify every single timing parameter in DT; it may make sense to use
> Boris Brezillon's approach (I note this further down, in the driver
> code) for mapping non-ONFI NAND timings into a compatible ONFI timing
> mode. This will greatly simplify the bindings needed, since it's
> standardized and auto-detectable in many cases.
AFAIR, the NAND timing representation for non-ONFI chips question was
left unanswered:
https://lkml.org/lkml/2014/5/20/581
I can definitely respin my NAND timings series, but I'd like to be sure
this is how you want it done before doing so.
Just as a reminder, you and Jason thought NAND timings for non-ONFI
chips could be auto detected thanks to READID informations (by storing
some sort of "NANDID <-> timings" association table).
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
Hi Brian,
On Wed, 2 Jul 2014 17:22:37 -0700
Brian Norris computersforpe...@gmail.com wrote:
Hi Lee,
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:
This is a squashed version of the submission to avoid re-sending the
entire set over and over, essentially clogging up the MLs.
Thanks. I think I'd prefer to accept your driver in a form like this
too. A few comments below.
And I'll get one big comment out of the way here: can you abstract your
ST BBT code into its own self-contained portion, preferably in a
separate source file, a la nand_bbt.c? Then, provide a way to optionally
use either your ST BBT or the existing BBT -- perhaps a NAND_BBT_ST flag
for chip-bbt_options, and a matching device tree property. That way,
even though you require a legacy format for bootloader interoperability,
someone can theoretically utilize more mainstream (albeit, not
necessarily better...) BBT support from nand_bbt.c. I think this will
provide the best balance between your existing product support and
upstream-friendly modularity/flexibility. I'm open to other suggestions,
of course.
Cc: computersforpe...@gmail.com
Cc: Gupta, Pekon pe...@ti.com
Cc: Ezequiel Garcia ezequiel.gar...@free-electrons.com
Cc: linux-...@lists.infradead.org
Signed-off-by: Lee Jones lee.jo...@linaro.org
---
Please add versioning to your next patch(es), and describe changes here.
Documentation/devicetree/bindings/mtd/stm-nand.txt | 87 +
See:
Documentation/devicetree/bindings/submitting-patches.txt
[...]
+
+ nand_timing0: nand-timing {
+ sig-setup = 10;
+ sig-hold= 10;
+ CE-deassert = 0;
+ WE-to-RBn = 100;
+ wr-on = 10;
+ wr-off = 30;
+ rd-on = 10;
+ rd-off = 30;
+ chip-delay = 30; /* delay in us */
+ };
You didn't document any of this node. And I don't think we want to
specify every single timing parameter in DT; it may make sense to use
Boris Brezillon's approach (I note this further down, in the driver
code) for mapping non-ONFI NAND timings into a compatible ONFI timing
mode. This will greatly simplify the bindings needed, since it's
standardized and auto-detectable in many cases.
AFAIR, the NAND timing representation for non-ONFI chips question was
left unanswered:
https://lkml.org/lkml/2014/5/20/581
I can definitely respin my NAND timings series, but I'd like to be sure
this is how you want it done before doing so.
Just as a reminder, you and Jason thought NAND timings for non-ONFI
chips could be auto detected thanks to READID informations (by storing
some sort of NANDID - timings association table).
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
BTW, there are a few checkpatch.pl complaints. Please fix these for the next version (v2 or v3?) On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: > This is a squashed version of the submission to avoid re-sending the > entire set over and over, essentially clogging up the MLs. > > Cc: computersforpe...@gmail.com > Cc: Gupta, Pekon" Stray/unbalanced quote character? > Cc: Ezequiel Garcia > Cc: linux-...@lists.infradead.org > Signed-off-by: Lee Jones [...] Brian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] mtd: nand: stm_nand_bch: add new driver
BTW, there are a few checkpatch.pl complaints. Please fix these for the next version (v2 or v3?) On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote: This is a squashed version of the submission to avoid re-sending the entire set over and over, essentially clogging up the MLs. Cc: computersforpe...@gmail.com Cc: Gupta, Pekon pe...@ti.com Stray/unbalanced quote character? Cc: Ezequiel Garcia ezequiel.gar...@free-electrons.com Cc: linux-...@lists.infradead.org Signed-off-by: Lee Jones lee.jo...@linaro.org [...] Brian -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
