[Skiboot] [RFC PATCH 1/2] libflash: don't use the low level interface if it doesn't exist

[Cyril Bur]

On Thu, 2015-03-26 at 11:44 +1100, Daniel Axtens wrote:
> On Thu, 2015-03-19 at 18:18 +1100, Cyril Bur wrote:
> > During init libflash calls low level functions without checking.
> > 
> > libflash states to backends that if they implement all the higher level
> > functions the lower level functions are optional (from libflash-priv.h):
> >   If all functions of the high level interface are
> >   implemented then the low level one is optional. A
> >   controller can implement some of the high level one
> >   in which case the missing ones will be handled by
> >   libflash using the low level interface.
> > 
> > Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> 
> 
> I think you've missed fl_wpage.
> 
It's not clear to me if fl_wpage() needs it, it is only called from
flash_write() which has already checked to see if it can use the high
level interface and also done:
    
    if (!ct->cmd_wr)
        return FLASH_ERR_CTRL_CMD_UNSUPPORTED;

before it calls fl_wpage().

> You might also want to change flash_configure: it currently checks
> ct->cmd_wr before calling flash_set_4b, which would be redundant with
> this patch.

Actually better yet, I can remove both the checks and add one to the
start of flash_set_4b() which should cover everything.

Just to explain a bit, I want this patch to be as least invasive as
possible because my backend isn't flash at all (a rewrite of
libflash/libffs is underway to address this problem) and its possible
that there are reasons why these codepathes exist, and 'normal' backends
might want them, I don't want to start simply returning (success or
failure) if the low levels don't exist, it could start masking bigger
issues. I just need libflash to not segfault when it's used with the
file backend :). In a perfect world once there is a better way of using
libffs with files this patch should probably be reverted and the problem
re-examined.

I'll resend updated version.

Thanks for review,

Cyril

> Regards,
> Daniel
> 
> > ---
> >  libflash/libflash.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libflash/libflash.c b/libflash/libflash.c
> > index a229668..ce6a899 100644
> > --- a/libflash/libflash.c
> > +++ b/libflash/libflash.c
> > @@ -91,6 +91,10 @@ int fl_wren(struct spi_flash_ctrl *ct)
> >  	int i, rc;
> >  	uint8_t stat;
> >  
> > +	/* If lower level interface not implmented, just return */
> > +	if (!ct->cmd_wr)
> > +		return 0;
> > +
> >  	/* Some flashes need it to be hammered */
> >  	for (i = 0; i < 1000; i++) {
> >  		rc = ct->cmd_wr(ct, CMD_WREN, false, 0, NULL, 0);
> > @@ -674,6 +678,11 @@ static int flash_set_4b(struct flash_chip *c, bool enable)
> >  		/* Ignore the error & move on (could be wrprotect chip) */
> >  	}
> >  
> > +	/* Don't have low level interface, assume all is well */
> > +	if (!ct->cmd_wr)
> > +		return 0;
> > +
> > +
> >  	/* Ignore error in case chip is write protected */
> >  
> >  	return ct->cmd_wr(ct, enable ? CMD_EN4B : CMD_EX4B, false, 0, NULL, 0);
> > @@ -757,7 +766,6 @@ static int flash_configure(struct flash_chip *c)
> >  				return rc;
> >  			}
> >  		}
> > -
> >  		/* Set controller to 3b mode if mode switch is supported */
> >  		if (ct->set_4b) {
> >  			FL_DBG("LIBFLASH: Disabling controller 4B mode...\n");
>