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

[Ronak Desai]

Hey Borris, Miquel,

On Tue, Apr 24, 2018 at 12:37 PM, Boris Brezillon
<boris.brezillon at bootlin.com> wrote:
> 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 at 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...
>
I understand your concern and thank you for the feedback, I am
planning to update the patch to accommodate temporary solution as
Borris suggested as converting the driver would take time but I am
also looking forward to it.
> Regards,
>
> Boris



-- 
Ronak A Desai
Sr. Software Engineer
Airborne Information Solutions / RC Linux Platform Software
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
Ronak.Desai at rockwellcollins.com
https://www.rockwellcollins.com/