Re: [PATCH v6 11/15] nand: spi: add basic operations support

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Peter,

On Wed, 31 May 2017 06:51:34 +0000
Peter Pan 潘栋 (peterpandong) <peterpandong@micron.com> wrote:

> >> +/**
> >> + * spinand_disable_ecc - disable internal ECC
> >> + * @spinand: SPI NAND device structure
> >> + */
> >> +static void spinand_disable_ecc(struct spinand_device *spinand)
> >> +{
> >> +    u8 cfg = 0;
> >> +
> >> +    spinand_get_cfg(spinand, &cfg);
> >> +
> >> +    if ((cfg & CFG_ECC_MASK) == CFG_ECC_ENABLE) {
> >> +        cfg &= ~CFG_ECC_ENABLE;
> >> +        spinand_set_cfg(spinand, cfg);
> >> +    }  
> > 
> > Is this really a generic (manufacturer-agnostic) feature??? I had the
> > feeling that is was only working for Micron. If that's the case, this
> > 'disable-ecc' step should be done in spi/micron.c in a
> > micron_spinand_init() function.  
> 
> As far as I can see, Micron is not the only vendor to disable on die ecc 
> by this way, actually all vendors I know use this .

Hm, ok. I'm a bit skeptical (vendors tend to implement this kind of
feature with vendor specific commands, but maybe it has changed with SPI
NANDs), but I'll trust you on this one. 

> And this series 
> doesn't support on die ecc, so IMO it is necessary to disable it during
> initialization

Actually I was not arguing on the need to disable on-die ECC, just
wasn't sure this should be done in the core. If it's really generic,
then it's fine.

> 
> >   
> >> +}

[...]

> >> 
> >> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> >> index dd9da71..04ad1dd 100644
> >> --- a/include/linux/mtd/spinand.h
> >> +++ b/include/linux/mtd/spinand.h
> >> @@ -103,11 +103,14 @@ struct spinand_controller_ops {
> >>  *          return directly and let others to detect.
> >>  * @init: initialize SPI NAND device.
> >>  * @cleanup: clean SPI NAND device footprint.
> >> + * @prepare_op: prepara read/write operation.  
> > 
> >           ^ prepare
> > 
> > 
> >   
> >>  */
> >> struct spinand_manufacturer_ops {
> >>    bool (*detect)(struct spinand_device *spinand);
> >>    int (*init)(struct spinand_device *spinand);
> >>    void (*cleanup)(struct spinand_device *spinand);
> >> +    void (*prepare_op)(struct spinand_device *spinand,
> >> +               struct spinand_op *op, u32 page, u32 column);  
> > 
> > It seems to be here to prepare read/write page operations, so I'd like
> > to rename this method ->prepare_page_op() if you don't mind.  
> 
> I'm ok with the new name

Actually, in the mean time I asked Cyrille how dummy cycles were
handled in the spi-nor subsystem and how spi flash controllers are
dealing with such dummy cycles. It seems that some controllers are able
to handle those dummy cycles on their own (without any software
intervention to skip bits or move things around in output bufs.

I'll let Cyrille comment on this specific aspect, but I wonder if we
shouldn't just specify the number of dummy cycles and let the
controller handle that.

Regards,

Boris