[U-Boot] [PATCH] env: add flash_read function

[Horatiu Vultur]

Hi Wolfgang,

The 12/03/2018 17:08, Wolfgang Denk wrote:
> Dear Horatiu,
> 
> In message <1543678222-15837-1-git-send-email-horatiu.vultur at microchip.com> you wrote:
> > The flash_read function is a wrapper over spi_flash_read, which enables
> > the env to read multiple flash page size from flash until '\0\0' is read
> > or the end of env partition is reached. Instead of reading the entire env
> > size. When it reads '\0\0', it stops reading further the env and assumes
> > that the rest of env is '\0'.
> > 
> > This is an optimization for large environments that contain few bytes of
> > environment variables. In this case it doesn't need to read the entire
> > environment and only few pages.
> > 
> > Signed-off-by: Horatiu Vultur <horatiu.vultur at microchip.com>
> > ---
> >  env/sf.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 43 insertions(+), 11 deletions(-)
> > 
> > diff --git a/env/sf.c b/env/sf.c
> > index 2e3c600..e1e1a94 100644
> > --- a/env/sf.c
> > +++ b/env/sf.c
> > @@ -81,6 +81,39 @@ static int setup_flash_device(void)
> >  	return 0;
> >  }
> >  
> > +static int is_end(char *addr, int size)
> 
> That should be
> 
> 	(const char *addr, size_t size)
> 
> instead.

I will change this in the next version.
> 
> > +static int flash_read(struct spi_flash *flash, u32 offset, int size, void *buf)
> 
> I think this should be
> 
> 	(const struct spi_flash *flash, u32 offset, size_t len, void *buf)
> 
> [Yes, for reasons of consistency with spi_flash_read() I would also
> rename "size" into "len".]
> 

The same here.

> 
> Also I recommend to chose another name for the function instead of
> "flash_read".  At first glance this looks like a generalization over
> spi_flash_read() which supports other flash types than SPI flash as
> well - but it does not.  Instead, it is a specialized version of
> spi_flash_read() tailored for environment reading only.
> 
> Maybe spi_flash_read_env() would be a more reasonable name?
> 

Yes spi_flash_read_env is a better name. I will update it in the next
version.

> 
> > +static int flash_read(struct spi_flash *flash, u32 offset, int size, void *buf)
> > +{
> > +	u32 addr = 0;
> > +	u32 page_size = flash->page_size;
> > +
> > +	memset(buf, 0x0, size);
> > +	for (int i = 0; i < size / page_size; ++i) {
> > +		int ret = spi_flash_read(flash, offset, page_size,
> > +					 &((char *)buf)[addr]);
> > +
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (is_end(&((char *)buf)[addr], page_size))
> > +			return 0;
> > +
> > +		addr += page_size;
> > +		offset += page_size;
> > +	}
> > +	return 0;
> 
> Is this correct?  Is the read() function not supposed to return the
> number of read bytes?

It is true that C read function returns the number of bytes read, but
the intention of the function flash_read was to mimic the function
spi_flash_read, which returns 0 if OK otherwise an error code. And
that's the reason why the function flash_read returns 0 and not number
of bytes read.

It would not be a problem for me to change the function flash_read to
return the number of bytes read.

> 
> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> The optimum committee has no members.
>                                                    - Norman Augustine

-- 
Thanks,
/Horatiu