nand_base - kill chip->oob_poi?

nand_base - kill chip->oob_poi?

[Brian Norris]

Hi,

I'm looking to support a new NAND hardware controller, and it doesn't
support OOB read/write for its fastest modes of operation, since most
normal activity (i.e., UBI(FS)) doesn't need OOB. So I'm trying to
cleanly separate data and OOB functionality so that read and write
driver functions can address data, OOB, or both based on the MTD API
request.

Unfortunately, the more I study nand_base.c, the more I see that
{read,write}_page functionality is mostly tied together with OOB.
Specifically, there is a lot of usage of a dirty, backend
"nand_chip.oob_poi" pointer for transferring OOB data between
functions. And some driver functions (the "syndrome" class of
functions) expect that they can *always* read/write to oob_poi, even
if the high-level API call only needed to read from the in-band data.
Thus, chip->oob_poi is required but not always used, and there is no
way for a driver to know if OOB was actually requested in the first
place.

So the question is: can anybody provide tips for allowing OOB
read/write to be conditional at the driver level, during page
read/write? I have a plan (below), but it conflicts somewhat with some
current {read,write}_page implementations as seen above in the
"syndrome" functions.

My plan looked something like the following:
- avoid using chip->oob_poi explicitly if at all possible
- pass both buf and oob pointers to the various {read,write}_page
interfaces (in nand_chip and in nand_ecc_ctrl)
- allow oob to be NULL, which would imply that the API call only
needed the in-band data

So the nand_ecc_ctrl interfaces would look like:

void (*write_page)(struct mtd_info *mtd, struct nand_chip *chip, const
uint8_t *buf, const uint8_t *oob);
int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip, uint8_t
*buf, uint8_t *oob, int page);
...

And similarly for nand_chip:

int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
                        const uint8_t *buf, const uint8_t *oob, int page,
                        int cached, int raw);

Any recommendations, tips, questions, or criticisms are welcome. If I
don't see any other great ideas, I may post a preliminary patch series
soon.

Brian

P.S. I see that there may be another way to solve my problem (besides
my plan above): just add more interfaces in the nand_ecc_ctrl
structure. But I think this is an ugly solution, and the NAND
infrastructure needs to be cleaner, not uglier.

Re: nand_base - kill chip->oob_poi?

[Thomas Gleixner]

On Tue, 27 Mar 2012, Brian Norris wrote:

> Hi,
> 
> I'm looking to support a new NAND hardware controller, and it doesn't
> support OOB read/write for its fastest modes of operation, since most
> normal activity (i.e., UBI(FS)) doesn't need OOB. So I'm trying to

And how is ECC working for that "normal" activity ?

Using NAND w/o ECC is doomed for fail.

Aside of that improvements to the NAND code are always welcome,
modifications which are solely done to support insane usage of NAND
FLASH not so much.

Thanks,

	tglx

Re: nand_base - kill chip->oob_poi?

[Woodhouse, David]

[-- Attachment #1: Type: text/plain, Size: 1693 bytes --]

On Wed, 2012-03-28 at 11:51 +0200, Thomas Gleixner wrote:
> On Tue, 27 Mar 2012, Brian Norris wrote:
> 
> > Hi,
> > 
> > I'm looking to support a new NAND hardware controller, and it doesn't
> > support OOB read/write for its fastest modes of operation, since most
> > normal activity (i.e., UBI(FS)) doesn't need OOB. So I'm trying to
> 
> And how is ECC working for that "normal" activity ?
> 
> Using NAND w/o ECC is doomed for fail.

I was assuming that the controller *used* the OOB area for hardware ECC,
but just didn't support letting the *host* access the OOB area "in its
fastest modes of operation".

> > My plan looked something like the following:
> > - avoid using chip->oob_poi explicitly if at all possible
> > - pass both buf and oob pointers to the various {read,write}_page
> >   interfaces (in nand_chip and in nand_ecc_ctrl)
> > - allow oob to be NULL, which would imply that the API call only
> >   needed the in-band data

And presumably the swecc/hwecc cases would still use either oob_poi or
their own buffer, since they have to put the syndrome *somewhere* after
calculating it... but those are only for those controllers which *use*
the swecc/hwecc support.

I don't think it matters that your plan "conflicts" with the ECC
implementations that you wouldn't be able to to use on this hardware
anyway.

I agree that adding more interfaces would probably not be a good idea;
this code is tangled enough already.


-- 
                   Sent with MeeGo's ActiveSync support.

David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 4370 bytes --]

Re: nand_base - kill chip->oob_poi?

[Brian Norris]

On Wed, Mar 28, 2012 at 3:14 AM, Woodhouse, David
<david.woodhouse@intel.com> wrote:
> On Wed, 2012-03-28 at 11:51 +0200, Thomas Gleixner wrote:
>> On Tue, 27 Mar 2012, Brian Norris wrote:
>> > I'm looking to support a new NAND hardware controller, and it doesn't
>> > support OOB read/write for its fastest modes of operation, since most
>> > normal activity (i.e., UBI(FS)) doesn't need OOB. So I'm trying to
>>
>> And how is ECC working for that "normal" activity ?
>>
>> Using NAND w/o ECC is doomed for fail.
>
> I was assuming that the controller *used* the OOB area for hardware ECC,
> but just didn't support letting the *host* access the OOB area "in its
> fastest modes of operation".

Right, HW ECC is working with a HW internal buffer that isn't exposed
to the host in its DMA modes. I can access it via PIO, but that's not
ideal.

>> > My plan looked something like the following:
>> > - avoid using chip->oob_poi explicitly if at all possible
>> > - pass both buf and oob pointers to the various {read,write}_page
>> >   interfaces (in nand_chip and in nand_ecc_ctrl)
>> > - allow oob to be NULL, which would imply that the API call only
>> >   needed the in-band data
>
> And presumably the swecc/hwecc cases would still use either oob_poi or
> their own buffer, since they have to put the syndrome *somewhere* after
> calculating it... but those are only for those controllers which *use*
> the swecc/hwecc support.

Yes, I guess they still need it. Maybe in places where they would have
used chip->oob_poi, I can replace it with a buffer passed as an
argument, then when that buffer is NULL we fall back to chip->oob_poi,
like the following:

static int nand_read_page_raw_syndrome(... uint8_t *buf, uint8_t *oobbuf ...)
{
    ...
    uint8_t *oob = oobbuf ? oobbuf : chip->oob_poi;
    ...
    chip->read_buf(mtd, oob, ...);
    ...
}

> I don't think it matters that your plan "conflicts" with the ECC
> implementations that you wouldn't be able to to use on this hardware
> anyway.

Right. I'm just concerned with modifying the nand_chip / nand_ecc_ctrl
interfaces enough to work with this hardware while avoiding breaking
existing stuff and keeping the interfaces sane. I have to at least
break the assumption that the {read,write}_page interfaces *always*
will fill/use chip->oob_poi.

Brian