ONFI timing mode with onfi_set_features unsupported

[Sascha Hauer]

On Tue, Nov 22, 2016 at 12:10:47PM +0100, Boris Brezillon wrote:
> On Tue, 22 Nov 2016 12:02:27 +0100
> Sascha Hauer <s.hauer at pengutronix.de> wrote:
> 
> > On Mon, Nov 21, 2016 at 02:51:04PM +0100, Boris Brezillon wrote:
> > > Hi Sascha,
> > > 
> > > On Mon, 21 Nov 2016 14:23:27 +0100
> > > Sascha Hauer <s.hauer at pengutronix.de> wrote:
> > >   
> > > > Hi all,
> > > > 
> > > > I have a i.MX6ul here with a Cypress S34ML04G2 connected. This Nand chip
> > > > has ONFI support, but onfi_set_features is unsupported (opt_cmd is
> > > > 0x3b). Currently the gpmi Nand driver calls nand_onfi_set_features(),
> > > > gets -EINVAL as error and continues with a very slow default timing.
> > > > 
> > > > I assume the nand_onfi_set_features() call is just unnecessary for this
> > > > chip, if I skip it, the chip works with the fast timing.
> > > > 
> > > > Any idea how to cope with this situation? I attached the most obvious
> > > > patch, but it looks a bit hackish. Any suggestions or is the patch fine
> > > > as is?  
> > > 
> > > It looks good to me. Why do you find this code hackish?
> > > Of course, it would be even better to implement the  
> > > ->setup_data_interface() method.  
> > 
> > Indeed, but my current project doesn't allow to spend that much time at
> > the moment.
> 
> Understood. Let's wait for Han's review.
> 
> > 
> > > 
> > > BTW, can you patch the core to only send the ->set_feature() command
> > > (to change the timings mode) when the chip supports it?  
> > 
> > With hackish I mean that I think the problem should be solved in the
> > core. How about returning -EOPNOTSUPP from onfi_set_features() when the
> > operation is not supported? The caller could then decide what to do
> > without testing for bits in the onfi param page.
> 
> The problem is, ->onfi_set_feature() is a method that can be overloaded
> by NAND controller drivers. Of course, we could add a wrapper around
> ->onfi_set_feature() (nand_set_feature()?),

Such a wrapper would have the advantage that we wouldn't have to repeat
the

	if (!chip->onfi_version ||
		    !(le16_to_cpu(chip->onfi_params.opt_cmd)
		    	      & ONFI_OPT_CMD_SET_GET_FEATURES))

test in each driver with a special onfi_get_features() implementation.

> but then, the meaning of
> -ENOTSUPP is not clear. It could be returned if the
> ->onfi_set_feature() is NULL or if the requested feature is not
> supported

->onfi_set_feature() will never be NULL as it's set to nand_onfi_set_features
if it's NULL in during registration. Also I would suggest -ENOSYS if the
driver is lacking support for an operation.

> 
> Another solution would be to add an extra helper to check if a specific
> feature is supported:
> 
> bool nand_feature_supported(nand, feature_id);

Yes, that would work aswell. Up to you to decide ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |