Re: [PATCH] Added set feature command in FSL IFC nand controller driver for ONFI nand

[Boris Brezillon <boris.brezillon@bootlin.com>]

Hi Ronak,

I'm jumping on this discussion to clarify a few things.

On Tue, 24 Apr 2018 12:10:07 -0500
Ronak Desai <ronak.desai@rockwellcollins.com> wrote:

> >> >> >> +
> >> >> >> +     for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
> >> >> >> +             chip->write_byte(mtd, subfeature_param[i]);
> >> >> >> +
> >> >> >> +     chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, 0);  
> >
> > And this implementation does the opposite :)  
> No, this is not opposite, I am just copying the data in the nand
> controller data buffer and not sending on the bus and when the
> NAND_CMD_SET_FEATURES is called, in that I tell nand controller to
> transfer 4 bytes from data buffer after sending the set feature
> command. So, on the bus the sequence will be same as depicted in the
> ONFI specification. I have tested this on two different target
> hardwares with processors having this nand controller.

The ->cmdfunc() semantic has been abused for too long by too many
drivers, let's stop this insanity. ->cmdfunc() has been designed to
send CMD and ADDR cycles on the NAND bus, not to trigger the actual
data transfer. You might think this is a bad design (and I partially
agree that things were not perfect), but that's how it is. This being
said, we now have a clean way for drivers to get all information at
once (CMD, ADDR and DATA cycles) and it's called ->exec_op().

If you don't want to patch the driver to support ->exec_op(), I can
consider accepting a temporary solution where the whole SET_FEATURES
handling is done in fsl_ifc_onfi_set_features() (that is, moving the
code you have in the 'case NAND_CMD_SET_FEATURES' in the
fsl_ifc_onfi_set_features() function so that you don't call ->cmdfunc()
at all), but I'd really prefer to have the driver converted to
->exec_op(), and I think we (Miquel and I) can help with that.

BTW, you implement ->set_features() but ->get_features() is not
supported??!! This sounds like a bad idea to me...

Regards,

Boris