[RFC] libflash/libffs: decouple libflash and libffs.

Currently libffs uses libflash to read data. This does works well if the
data is directly on flash hardware that libflash is designed to read from.
Problems arise when the data is not on an actual flash hardware but libffs is
needed to parse ffs partition data.

This patch begins the work to decouple libffs and libflash as well as starting
the work to provide an abstraction so that libffs may eventually be able to
read its data from essentially any source.

---
V2: Added Alistairs feedback.
    Modified core/flash.c to use the new 'highlevel' interface
    Modified pflash to use the new 'highlevel' interface
V3: Changed everything to add the container_of stuff. Looks good!
V4: Very minor modifications. Made the tests work.

Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 core/flash.c                 | 34 ++++++++++--------
 external/pflash/Makefile     |  2 +-
 external/pflash/ast.h        |  1 +
 external/pflash/pflash.c     | 33 +++++++++---------
 libflash/highlevel.c         | 51 +++++++++++++++++++++++++++
 libflash/highlevel.h         | 38 ++++++++++++++++++++
 libflash/libffs.c            | 51 +++++++++++----------------
 libflash/libffs.h            |  7 ++--
 libflash/libflash.c          | 83 +++++++++++++++++++++++++++-----------------
 libflash/libflash.h          | 25 +++++--------
 libflash/test/Makefile.check |  1 +
 libflash/test/test-flash.c   | 20 +++++------
 platforms/astbmc/pnor.c      | 12 ++++---
 platforms/rhesus/rhesus.c    | 10 +++---
 14 files changed, 235 insertions(+), 133 deletions(-)
 create mode 100644 libflash/highlevel.c
 create mode 100644 libflash/highlevel.h

I've had thoughts and to get the ball rolling...

On Wed, 2015-04-08 at 11:19 +1000, Cyril Bur wrote:
> Currently libffs uses libflash to read data. This does works well if the
> data is directly on flash hardware that libflash is designed to read from.
> Problems arise when the data is not on an actual flash hardware but libffs is
> needed to parse ffs partition data.
> 
> This patch begins the work to decouple libffs and libflash as well as starting
> the work to provide an abstraction so that libffs may eventually be able to
> read its data from essentially any source.
> 
> ---
> V2: Added Alistairs feedback.
>     Modified core/flash.c to use the new 'highlevel' interface
>     Modified pflash to use the new 'highlevel' interface
> V3: Changed everything to add the container_of stuff. Looks good!
> V4: Very minor modifications. Made the tests work.
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  core/flash.c                 | 34 ++++++++++--------
>  external/pflash/Makefile     |  2 +-
>  external/pflash/ast.h        |  1 +
>  external/pflash/pflash.c     | 33 +++++++++---------
>  libflash/highlevel.c         | 51 +++++++++++++++++++++++++++
>  libflash/highlevel.h         | 38 ++++++++++++++++++++
>  libflash/libffs.c            | 51 +++++++++++----------------
>  libflash/libffs.h            |  7 ++--
>  libflash/libflash.c          | 83 +++++++++++++++++++++++++++-----------------
>  libflash/libflash.h          | 25 +++++--------
>  libflash/test/Makefile.check |  1 +
>  libflash/test/test-flash.c   | 20 +++++------
>  platforms/astbmc/pnor.c      | 12 ++++---
>  platforms/rhesus/rhesus.c    | 10 +++---
>  14 files changed, 235 insertions(+), 133 deletions(-)
>  create mode 100644 libflash/highlevel.c
>  create mode 100644 libflash/highlevel.h
> 
> diff --git a/core/flash.c b/core/flash.c
> index 7d32c8d..3dc0a0d 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -21,12 +21,13 @@
>  #include <device.h>
>  #include <libflash/libflash.h>
>  #include <libflash/libffs.h>
> +#include <libflash/highlevel.h>
>  #include <ecc.h>
>  
>  struct flash {
>  	bool			registered;
>  	bool			busy;
> -	struct flash_chip	*chip;
> +	struct highlevel_ops *ops;
>  	uint32_t		size;
>  	uint32_t		block_size;
>  };
> @@ -107,7 +108,7 @@ static int flash_nvram_start_read(void *dst, uint32_t src, uint32_t len)
>  		goto out;
>  	}
>  
> -	rc = flash_read(nvram_flash->chip, nvram_offset + src, dst, len);
> +	rc = highlevel_read(nvram_flash->ops, nvram_offset + src, len, dst);
>  
>  out:
>  	unlock(&flash_lock);
> @@ -136,7 +137,7 @@ static int flash_nvram_write(uint32_t dst, void *src, uint32_t len)
>  		rc = OPAL_PARAMETER;
>  		goto out;
>  	}
> -	rc = flash_smart_write(nvram_flash->chip, nvram_offset + dst, src, len);
> +	rc = highlevel_write(nvram_flash->ops, nvram_offset + dst, len, src);
>  
>  out:
>  	unlock(&flash_lock);
> @@ -249,7 +250,7 @@ static void setup_system_flash(struct flash *flash, struct dt_node *node,
>  	flash_nvram_probe(flash, ffs);
>  }
>  
> -int flash_register(struct flash_chip *chip, bool is_system_flash)
> +int flash_register(struct highlevel_ops *hl, bool is_system_flash)
>  {
>  	uint32_t size, block_size;
>  	struct ffs_handle *ffs;
> @@ -259,7 +260,7 @@ int flash_register(struct flash_chip *chip, bool is_system_flash)
>  	unsigned int i;
>  	int rc;
>  
> -	rc = flash_get_info(chip, &name, &size, &block_size);
> +	rc = highlevel_get_info(hl, &name, &size, &block_size);
>  	if (rc)
>  		return rc;
>  
> @@ -275,7 +276,7 @@ int flash_register(struct flash_chip *chip, bool is_system_flash)
>  		flash = &flashes[i];
>  		flash->registered = true;
>  		flash->busy = false;
> -		flash->chip = chip;
> +		flash->funcs = hl;
>  		flash->size = size;
>  		flash->block_size = block_size;
>  		break;
> @@ -287,7 +288,7 @@ int flash_register(struct flash_chip *chip, bool is_system_flash)
>  		return OPAL_RESOURCE;
>  	}
>  
> -	rc = ffs_open_flash(chip, 0, flash->size, &ffs);
> +	rc = ffs_init(0, flash->size, hl, &ffs);
>  	if (rc) {
>  		prlog(PR_WARNING, "FLASH: No ffs info; "
>  				"using raw device only\n");
> @@ -345,13 +346,17 @@ static int64_t opal_flash_op(enum flash_op op, uint64_t id, uint64_t offset,
>  
>  	switch (op) {
>  	case FLASH_OP_READ:
> -		rc = flash_read(flash->chip, offset, (void *)buf, size);
> +		rc = highlevel_read(flash->ops, offset, size, (void *)buf);
>  		break;
>  	case FLASH_OP_WRITE:
> -		rc = flash_write(flash->chip, offset, (void *)buf, size, false);
> +		/*
> +		 * Note: highlevel_write() uses flash_smart_write(), this call used to
> +		 * be flash_write()
> +		 */
> +		rc = highlevel_write(flash->ops, offset, size, (void *)buf);
>  		break;
>  	case FLASH_OP_ERASE:
> -		rc = flash_erase(flash->chip, offset, size);
> +		rc = highlevel_erase(flash->ops, offset, size);
>  		break;
>  	default:
>  		assert(0);
> @@ -424,10 +429,11 @@ struct flash_hostboot_header {
>  };
>  
>  /* start and total size include ECC */
> -static int flash_find_subpartition(struct flash_chip *chip, uint32_t subid,
> +static int flash_find_subpartition(struct highlevel_ops *funcs, uint32_t subid,
>  				   uint32_t *start, uint32_t *total_size,
>  				   bool *ecc)
>  {
> +	struct flash_chip *chip;
>  	struct flash_hostboot_header *header;
>  	char eyecatcher[5];
>  	uint32_t i, partsize;
> @@ -563,7 +569,7 @@ int flash_load_resource(enum resource_id id, uint32_t subid,
>  		goto out_unlock;
>  	}
>  
> -	rc = ffs_open_flash(flash->chip, 0, flash->size, &ffs);
> +	rc = ffs_init(0, flash->size, flash->funcs, &ffs);
>  	if (rc) {
>  		prerror("FLASH: Can't open ffs handle\n");
>  		goto out_unlock;
> @@ -590,7 +596,7 @@ int flash_load_resource(enum resource_id id, uint32_t subid,
>  
>  	/* Find the sub partition if required */
>  	if (subid != RESOURCE_SUBID_NONE) {
> -		rc = flash_find_subpartition(flash->chip, subid, &part_start,
> +		rc = flash_find_subpartition(flash->funcs, subid, &part_start,
>  					     &part_size, &ecc);
>  		if (rc)
>  			goto out_free_ffs;
> @@ -613,7 +619,7 @@ int flash_load_resource(enum resource_id id, uint32_t subid,
>  		goto out_free_ffs;
>  	}
>  
> -	rc = flash_read_corrected(flash->chip, part_start, buf, size, ecc);
> +	rc = flash_read_corrected(flash->funcs, part_start, buf, size, ecc);
>  	if (rc) {
>  		prerror("FLASH: failed to read %s partition\n", name);
>  		goto out_free_ffs;
> diff --git a/external/pflash/Makefile b/external/pflash/Makefile
> index c28e359..f806b34 100644
> --- a/external/pflash/Makefile
> +++ b/external/pflash/Makefile
> @@ -14,7 +14,7 @@ endif
>  CFLAGS  = -O2 -Wall -I.
>  LDFLAGS	= -lrt
>  OBJS    = pflash.o progress.o ast-sf-ctrl.o
> -OBJS	+= libflash/libflash.o libflash/libffs.o libflash/ecc.o
> +OBJS	+= libflash/libflash.o libflash/libffs.o libflash/ecc.o libflash/highlevel.o
>  OBJS	+= $(ARCH_OBJS)
>  EXE     = pflash
>  
> diff --git a/external/pflash/ast.h b/external/pflash/ast.h
> index 92cafa4..3789b39 100644
> --- a/external/pflash/ast.h
> +++ b/external/pflash/ast.h
> @@ -57,6 +57,7 @@ void ast_io_init(void);
>   */
>  #define AST_SF_TYPE_PNOR	0
>  #define AST_SF_TYPE_BMC		1
> +#define AST_SF_TYPE_MEM		2
>  
This hunk should probably be a separate patch... its not really related
to what I'm doing, I think I'll post it separately.

>  struct spi_flash_ctrl;
>  int ast_sf_open(uint8_t type, struct spi_flash_ctrl **ctrl);
> diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c
> index fb783a2..2eb3164 100644
> --- a/external/pflash/pflash.c
> +++ b/external/pflash/pflash.c
> @@ -16,6 +16,7 @@
>  
>  #include <libflash/libflash.h>
>  #include <libflash/libffs.h>
> +#include <libflash/highlevel.h>
>  #include "progress.h"
>  #include "io.h"
>  #include "ast.h"
> @@ -23,7 +24,7 @@
>  
>  #define __aligned(x)			__attribute__((aligned(x)))
>  
> -#define PFLASH_VERSION	"0.8.6"
> +#define PFLASH_VERSION	"0.8.7"
>  
Probably should get some consensus here...

>  static bool must_confirm = true;
>  static bool dummy_run;
> @@ -36,8 +37,8 @@ static bool using_sfc;
>  #define FILE_BUF_SIZE	0x10000
>  static uint8_t file_buf[FILE_BUF_SIZE] __aligned(0x1000);
>  
> +static struct highlevel_funcs *hl;
>  static struct spi_flash_ctrl	*fl_ctrl;
> -static struct flash_chip	*fl_chip;
>  static struct ffs_handle	*ffsh;
>  static uint32_t			fl_total_size, fl_erase_granule;
>  static const char		*fl_name;
> @@ -84,7 +85,7 @@ static void print_flash_info(void)
>  		return;
>  
>  	if (!ffsh) {
> -		rc = ffs_open_flash(fl_chip, 0, 0, &ffsh);
> +		rc = ffs_init(0, fl_total_size, hl, &ffsh);
>  		if (rc) {
>  			fprintf(stderr, "Error %d opening ffs !\n", rc);
>  			ffsh = NULL;
> @@ -122,7 +123,7 @@ static void lookup_partition(const char *name)
>  
>  	/* Open libffs if needed */
>  	if (!ffsh) {
> -		rc = ffs_open_flash(fl_chip, 0, 0, &ffsh);
> +		rc = ffs_init(0, fl_total_size, hl, &ffsh);
>  		if (rc) {
>  			fprintf(stderr, "Error %d opening ffs !\n", rc);
>  			exit(1);
> @@ -158,7 +159,7 @@ static void erase_chip(void)
>  		return;
>  	}
>  
> -	rc = flash_erase_chip(fl_chip);
> +	rc = flash_erase_chip(hl);
>  	if (rc) {
>  		fprintf(stderr, "Error %d erasing chip\n", rc);
>  		exit(1);
> @@ -185,7 +186,7 @@ static void erase_range(uint32_t start, uint32_t size, bool will_program)
>  	while(size) {
>  		/* If aligned to 64k and at least 64k, use 64k erase */
>  		if ((start & 0xffff) == 0 && size >= 0x10000) {
> -			rc = flash_erase(fl_chip, start, 0x10000);
> +			rc = highlevel_erase(hl, start, 0x10000);
>  			if (rc) {
>  				fprintf(stderr, "Error %d erasing 0x%08x\n",
>  					rc, start);
> @@ -195,7 +196,7 @@ static void erase_range(uint32_t start, uint32_t size, bool will_program)
>  			size -= 0x10000;
>  			done += 0x10000;
>  		} else {
> -			rc = flash_erase(fl_chip, start, 0x1000);
> +			rc = highlevel_erase(hl, start, 0x1000);
>  			if (rc) {
>  				fprintf(stderr, "Error %d erasing 0x%08x\n",
>  					rc, start);
> @@ -252,7 +253,7 @@ static void program_file(const char *file, uint32_t start, uint32_t size)
>  			len = size;
>  		size -= len;
>  		actual_size += len;
> -		rc = flash_write(fl_chip, start, file_buf, len, true);
> +		rc = highlevel_write(hl, start, len, file_buf);
>  		if (rc) {
>  			if (rc == FLASH_ERR_VERIFY_FAILURE)
>  				fprintf(stderr, "Verification failed for"
> @@ -292,7 +293,7 @@ static void do_read_file(const char *file, uint32_t start, uint32_t size)
>  	progress_init(size >> 8);
>  	while(size) {
>  		len = size > FILE_BUF_SIZE ? FILE_BUF_SIZE : size;
> -		rc = flash_read(fl_chip, start, file_buf, len);
> +		rc = highlevel_read(hl, start, len, file_buf);
>  		if (rc) {
>  			fprintf(stderr, "Flash read error %d for"
>  				" chunk at 0x%08x\n", rc, start);
> @@ -318,7 +319,7 @@ static void enable_4B_addresses(void)
>  
>  	printf("Switching to 4-bytes address mode\n");
>  
> -	rc = flash_force_4b_mode(fl_chip, true);
> +	rc = flash_force_4b_mode(hl, true);
>  	if (rc) {
>  		fprintf(stderr, "Error %d enabling 4b mode\n", rc);
>  		exit(1);
> @@ -331,7 +332,7 @@ static void disable_4B_addresses(void)
>  
>  	printf("Switching to 3-bytes address mode\n");
>  
> -	rc = flash_force_4b_mode(fl_chip, false);
> +	rc = flash_force_4b_mode(hl, false);
>  	if (rc) {
>  		fprintf(stderr, "Error %d disabling 4b mode\n", rc);
>  		exit(1);
> @@ -342,7 +343,7 @@ static void flash_access_cleanup_bmc(void)
>  {
>  	if (ffsh)
>  		ffs_close(ffsh);
> -	flash_exit(fl_chip);
> +	flash_exit(hl);
>  	ast_sf_close(fl_ctrl);
>  	close_devs();
>  }
> @@ -362,7 +363,7 @@ static void flash_access_setup_bmc(bool use_lpc, bool need_write)
>  	}
>  
>  	/* Open flash chip */
> -	rc = flash_init(fl_ctrl, &fl_chip);
> +	rc = flash_init(fl_ctrl, &hl);
>  	if (rc) {
>  		fprintf(stderr, "Failed to open flash chip\n");
>  		exit(1);
> @@ -380,7 +381,7 @@ static void flash_access_cleanup_pnor(void)
>  
>  	if (ffsh)
>  		ffs_close(ffsh);
> -	flash_exit(fl_chip);
> +	flash_exit(hl);
>  #ifdef __powerpc__
>  	if (using_sfc)
>  		sfc_close(fl_ctrl);
> @@ -421,7 +422,7 @@ static void flash_access_setup_pnor(bool use_lpc, bool use_sfc, bool need_write)
>  #endif
>  
>  	/* Open flash chip */
> -	rc = flash_init(fl_ctrl, &fl_chip);
> +	rc = flash_init(fl_ctrl, &hl);
>  	if (rc) {
>  		fprintf(stderr, "Failed to open flash chip\n");
>  		exit(1);
> @@ -698,7 +699,7 @@ int main(int argc, char *argv[])
>  		flash_access_setup_pnor(use_lpc, has_sfc, erase || program);
>  	}
>  
> -	rc = flash_get_info(fl_chip, &fl_name,
> +	rc = flash_get_info(hl, &fl_name,
>  			    &fl_total_size, &fl_erase_granule);
>  	if (rc) {
>  		fprintf(stderr, "Error %d getting flash info\n", rc);
> diff --git a/libflash/highlevel.c b/libflash/highlevel.c
> new file mode 100644
> index 0000000..e29020f
> --- /dev/null
> +++ b/libflash/highlevel.c
> @@ -0,0 +1,51 @@
> +/* Copyright 2013-2014 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + * 	http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include "highlevel.h"
> +
> +int highlevel_read(struct highlevel_funcs *funcs, uint32_t pos, uint32_t len, void *buf)
> +{
> +	if (!funcs || !funcs->read || !buf)
> +		return -1;
> +
> +	return funcs->read(funcs, pos, len, buf);
> +}
> +
> +int highlevel_write(struct highlevel_funcs *funcs, uint32_t pos, uint32_t len, const void *buf)
> +{
> +	if (!funcs || !funcs->write || !buf)
> +		return -1;
> +
> +	return funcs->write(funcs, pos, len, buf);
> +}
> +
> +int highlevel_erase(struct highlevel_funcs *funcs, uint32_t pos, uint32_t len)
> +{
> +	if (!funcs || !funcs->erase)
> +		return -1;
> +
> +	return funcs->erase(funcs, pos, len);
> +}
> +
> +int highlevel_get_info(struct highlevel_funcs *funcs, const char **name, uint32_t *total_size, uint32_t *erase_granule)
> +{
> +	if (!funcs || !funcs->get_info)
> +		return -1;
> +
> +	return funcs->get_info(funcs, name, total_size, erase_granule);
> +}
> diff --git a/libflash/highlevel.h b/libflash/highlevel.h
> new file mode 100644
> index 0000000..177ef4e
> --- /dev/null
> +++ b/libflash/highlevel.h
> @@ -0,0 +1,38 @@
> +/* Copyright 2013-2014 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + * 	http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +#ifndef __LIBFLASH_HIGHLEVEL_H
> +#define __LIBFLASH_HIGHLEVEL_H
> +
> +#include <stdint.h>
> +
> +/*
> + * libffs may be used with different backends, all should provide these for
> + * libflash to get the information it needs
> + */
> +struct highlevel_ops {
> +	void *priv;
> +	int (*read)(struct highlevel_ops *hl, uint32_t pos, uint32_t len, void *buf);
> +	int (*write)(struct highlevel_ops *hl, uint32_t pos, uint32_t len, const void *buf);
> +	int (*erase)(struct highlevel_ops *hl, uint32_t pos, uint32_t len);
> +	int (*get_info)(struct highlevel_ops *hl, const char **name, uint32_t *total_size, uint32_t *erase_granule);
> +};
> +
> +int highlevel_read(struct highlevel_ops *funcs, uint32_t pos, uint32_t len, void *buf);
> +int highlevel_write(struct highlevel_ops *funcs, uint32_t pos, uint32_t len, const void *buf);
> +int highlevel_erase(struct highlevel_ops *funcs, uint32_t pos, uint32_t len);
> +int highlevel_get_info(struct highlevel_ops *funcs, const char **name, uint32_t *total_size, uint32_t *erase_granule);
> +
> +#endif /* __LIBFLASH_HIGHLEVEL_H */
> diff --git a/libflash/libffs.c b/libflash/libffs.c
> index 2d05cc9..4d8cbf9 100644
> --- a/libflash/libffs.c
> +++ b/libflash/libffs.c
> @@ -41,6 +41,7 @@ struct ffs_handle {
>  	uint32_t		max_size;
>  	void			*cache;
>  	uint32_t		cached_size;
> +	struct highlevel_ops *ops;
>  };
>  
>  static uint32_t ffs_checksum(void* data, size_t size)
> @@ -71,31 +72,22 @@ static int ffs_check_convert_header(struct ffs_hdr *dst, struct ffs_hdr *src)
>  	return 0;
>  }
>  
> -int ffs_open_flash(struct flash_chip *chip, uint32_t offset,
> -		   uint32_t max_size, struct ffs_handle **ffs)
> +int ffs_init(uint32_t offset, uint32_t max_size, struct highlevel_ops *hl,
> +		struct ffs_handle **ffs)
>  {
>  	struct ffs_hdr hdr;
>  	struct ffs_handle *f;
> -	uint32_t fl_size, erase_size;
>  	int rc;
>  
> -	if (!ffs)
> +	if (!ffs || !funcs || !funcs->read || !funcs->write || !funcs->erase)
>  		return FLASH_ERR_PARM_ERROR;
>  	*ffs = NULL;
>  
> -	/* Grab some info about our flash chip */
> -	rc = flash_get_info(chip, NULL, &fl_size, &erase_size);
> -	if (rc) {
> -		FL_ERR("FFS: Error %d retrieving flash info\n", rc);
> -		return rc;
> -	}
> -	if ((offset + max_size) < offset)
> -		return FLASH_ERR_PARM_ERROR;
> -	if ((offset + max_size) > fl_size)
> +	if ((offset + max_size) < offset || (offset + max_size) > funcs->total_size)
>  		return FLASH_ERR_PARM_ERROR;
>  
>  	/* Read flash header */
> -	rc = flash_read(chip, offset, &hdr, sizeof(hdr));
> +	rc = highlevel_read(hl, offset, sizeof(hdr), &hdr);
>  	if (rc) {
>  		FL_ERR("FFS: Error %d reading flash header\n", rc);
>  		return rc;
> @@ -106,17 +98,15 @@ int ffs_open_flash(struct flash_chip *chip, uint32_t offset,
>  	if (!f)
>  		return FLASH_ERR_MALLOC_FAILED;
>  	memset(f, 0, sizeof(*f));
> -	f->type = ffs_type_flash;
>  	f->flash_offset = offset;
> -	f->max_size = max_size ? max_size : (fl_size - offset);
> -	f->chip = chip;
> +	f->max_size = max_size;
> +	f->funcs = funcs;
>  
>  	/* Convert and check flash header */
>  	rc = ffs_check_convert_header(&f->hdr, &hdr);
>  	if (rc) {
>  		FL_ERR("FFS: Error %d checking flash header\n", rc);
> -		free(f);
> -		return rc;
> +		goto out;
>  	}
>  
>  	/*
> @@ -126,26 +116,24 @@ int ffs_open_flash(struct flash_chip *chip, uint32_t offset,
>  	f->cached_size = f->hdr.block_size * f->hdr.size;
>  	FL_DBG("FFS: Partition map size: 0x%x\n", f->cached_size);
>  
> -	/* Align to erase size */
> -	f->cached_size |= (erase_size - 1);
> -	f->cached_size &= ~(erase_size - 1);
> -	FL_DBG("FFS:         Aligned to: 0x%x\n", f->cached_size);
> -
>  	/* Allocate cache */
>  	f->cache = malloc(f->cached_size);
>  	if (!f->cache) {
> -		free(f);
> -		return FLASH_ERR_MALLOC_FAILED;
> +		rc = FLASH_ERR_MALLOC_FAILED;
> +		goto out;
>  	}
>  
>  	/* Read the cached map */
> -	rc = flash_read(chip, offset, f->cache, f->cached_size);
> -	if (rc) {
> +	rc = highlevel_read(hl, offset, f->cached_size, f->cache);
> +	if (rc)
>  		FL_ERR("FFS: Error %d reading flash partition map\n", rc);
> -		free(f);
> -	}
> +
> +out:
>  	if (rc == 0)
>  		*ffs = f;
> +	else
> +		free(f);
> +
>  	return rc;
>  }
>  
> @@ -361,6 +349,7 @@ int ffs_update_act_size(struct ffs_handle *ffs, uint32_t part_idx,
>  	ent->checksum = ffs_checksum(ent, FFS_ENTRY_SIZE_CSUM);
>  	if (!ffs->chip)
>  		return 0;
> -	return flash_smart_write(ffs->chip, offset, ent, FFS_ENTRY_SIZE);
> +
> +	return highlevel_write(ffs->ops, offset, FFS_ENTRY_SIZE, ent);
>  }
>  
> diff --git a/libflash/libffs.h b/libflash/libffs.h
> index dd58d28..0fabe7f 100644
> --- a/libflash/libffs.h
> +++ b/libflash/libffs.h
> @@ -18,6 +18,7 @@
>  
>  #include <libflash/libflash.h>
>  #include <libflash/ffs.h>
> +#include <libflash/highlevel.h>
>  
>  /* FFS handle, opaque */
>  struct ffs_handle;
> @@ -34,8 +35,10 @@ struct ffs_handle;
>  #define FFS_ERR_PART_NOT_FOUND	103
>  #define FFS_ERR_BAD_ECC		104
>  
> -int ffs_open_flash(struct flash_chip *chip, uint32_t offset,
> -		   uint32_t max_size, struct ffs_handle **ffs);
> +/* Init */
> +
> +int ffs_init(uint32_t offset, uint32_t max_size,
> +		struct highlevel_funcs *ops, struct ffs_handle **ffs);
>  
>  /* ffs_open_image is Linux only as it uses lseek, which skiboot does not
>   * implement */
> diff --git a/libflash/libflash.c b/libflash/libflash.c
> index a81ff4d..3303bb4 100644
> --- a/libflash/libflash.c
> +++ b/libflash/libflash.c
> @@ -20,6 +20,7 @@
>  #include "libflash.h"
>  #include "libflash-priv.h"
>  #include "ecc.h"
> +#include "highlevel.h"
>  
>  #ifndef MIN
>  #define MIN(a, b)	((a) < (b) ? (a) : (b))
> @@ -45,6 +46,7 @@ struct flash_chip {
>  	bool			mode_4b;	/* Flash currently in 4b mode */
>  	struct flash_req	*cur_req;	/* Current request */
>  	void			*smart_buf;	/* Buffer for smart writes */
> +	struct highlevel_ops ops;
>  };
>  
>  #ifndef __SKIBOOT__
> @@ -116,8 +118,9 @@ int fl_wren(struct spi_flash_ctrl *ct)
>  	return FLASH_ERR_WREN_TIMEOUT;
>  }
>  
> -int flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len)
> +static int flash_read(struct highlevel_ops *hl, uint32_t pos, uint32_t len, void *buf)
>  {
> +	struct flash_chip *c = container_of(hl, struct flash_chip, ops);
>  	struct spi_flash_ctrl *ct = c->ctrl;
>  
>  	/* XXX Add sanity/bound checking */
> @@ -142,7 +145,7 @@ int flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len)
>   * This provides a wrapper around flash_read on ECCed data
>   * len is length of data without ECC attached
>   */
> -int flash_read_corrected(struct flash_chip *c, uint32_t pos, void *buf,
> +int flash_read_corrected(struct highlevel_ops *hl, uint32_t pos, void *buf,
>  		uint32_t len, bool ecc)
>  {
>  	struct ecc64 *bufecc;
> @@ -151,7 +154,7 @@ int flash_read_corrected(struct flash_chip *c, uint32_t pos, void *buf,
>  	uint8_t ret;
>  
>  	if (!ecc)
> -		return flash_read(c, pos, buf, len);
> +		return flash_read(hl, pos, len, buf);
>  
>  	/* Copy the buffer in chunks */
>  	bufecc = malloc(ECC_BUFFER_SIZE(COPY_BUFFER_LENGTH));
> @@ -163,7 +166,7 @@ int flash_read_corrected(struct flash_chip *c, uint32_t pos, void *buf,
>  		copylen = MIN(len, COPY_BUFFER_LENGTH);
>  
>  		/* Read ECCed data from flash */
> -		rc = flash_read(c, pos, bufecc, ECC_BUFFER_SIZE(copylen));
> +		rc = flash_read(hl, pos, ECC_BUFFER_SIZE(copylen), bufecc);
>  		if (rc)
>  			goto err;
>  
> @@ -219,8 +222,9 @@ static void fl_get_best_erase(struct flash_chip *c, uint32_t dst, uint32_t size,
>  	*cmd = CMD_BE;
>  }
>  
> -int flash_erase(struct flash_chip *c, uint32_t dst, uint32_t size)
> +static int flash_erase(struct highlevel_ops *hl, uint32_t dst, uint32_t size)
>  {
> +	struct flash_chip *c = container_of(hl, struct flash_chip, ops);
>  	struct spi_flash_ctrl *ct = c->ctrl;
>  	uint32_t chunk;
>  	uint8_t cmd;
> @@ -266,8 +270,9 @@ int flash_erase(struct flash_chip *c, uint32_t dst, uint32_t size)
>  	return 0;
>  }
>  
> -int flash_erase_chip(struct flash_chip *c)
> +int flash_erase_chip(struct highlevel_ops *hl)
>  {
> +	struct flash_chip *c = container_of(hl, struct flash_chip, ops);
>  	struct spi_flash_ctrl *ct = c->ctrl;
>  	int rc;
>  
> @@ -315,9 +320,10 @@ static int fl_wpage(struct flash_chip *c, uint32_t dst, const void *src,
>  	return fl_sync_wait_idle(ct);
>  }
>  
> -int flash_write(struct flash_chip *c, uint32_t dst, const void *src,
> +static int flash_write(struct highlevel_ops *hl, uint32_t dst, const void *src,
>  		uint32_t size, bool verify)
>  {
> +	struct flash_chip *c = container_of(hl, struct flash_chip, ops);
>  	struct spi_flash_ctrl *ct = c->ctrl;
>  	uint32_t todo = size;
>  	uint32_t d = dst;
> @@ -378,7 +384,7 @@ int flash_write(struct flash_chip *c, uint32_t dst, const void *src,
>  		chunk = sizeof(vbuf);
>  		if (chunk > size)
>  			chunk = size;
> -		rc = flash_read(c, dst, vbuf, chunk);
> +		rc = flash_read(hl, dst, chunk, vbuf);
>  		if (rc) return rc;
>  		if (memcmp(vbuf, src, chunk)) {
>  			FL_ERR("LIBFLASH: Miscompare at 0x%08x\n", dst);
> @@ -391,7 +397,7 @@ int flash_write(struct flash_chip *c, uint32_t dst, const void *src,
>  	return 0;
>  }
>  
> -int flash_write_corrected(struct flash_chip *c, uint32_t pos, const void *buf,
> +int flash_write_corrected(struct highlevel_ops *hl, uint32_t pos, const void *buf,
>  		uint32_t len, bool verify, bool ecc)
>  {
>  	struct ecc64 *bufecc;
> @@ -400,7 +406,7 @@ int flash_write_corrected(struct flash_chip *c, uint32_t pos, const void *buf,
>  	uint8_t ret;
>  
>  	if (!ecc)
> -		return flash_write(c, pos, buf, len, verify);
> +		return flash_write(hl, pos, buf, len, verify);
>  
>  	/* Copy the buffer in chunks */
>  	bufecc = malloc(ECC_BUFFER_SIZE(COPY_BUFFER_LENGTH));
> @@ -419,7 +425,7 @@ int flash_write_corrected(struct flash_chip *c, uint32_t pos, const void *buf,
>  		}
>  
>  		/* Write ECCed data to the flash */
> -		rc = flash_write(c, pos, bufecc, copylen, verify);
> +		rc = flash_write(hl, pos, bufecc, copylen, verify);
>  		if (rc)
>  			goto err;
>  
> @@ -467,12 +473,12 @@ static enum sm_comp_res flash_smart_comp(struct flash_chip *c,
>  	return is_same ? sm_no_change : sm_need_write;
>  }
>  
> -int flash_smart_write(struct flash_chip *c, uint32_t dst, const void *src,
> -		      uint32_t size)
> +static int flash_smart_write(struct highlevel_ops *hl, uint32_t dst, uint32_t size, const void *src)
>  {
> +	struct flash_chip *c = container_of(hl, struct flash_chip, ops);
>  	uint32_t er_size = c->min_erase_mask + 1;
>  	uint32_t end = dst + size;
> -	int rc;	
> +	int rc;
>  
>  	/* Some sanity checking */
>  	if (end <= dst || !size || end > c->tsize) {
> @@ -493,7 +499,7 @@ int flash_smart_write(struct flash_chip *c, uint32_t dst, const void *src,
>  		off = dst & c->min_erase_mask;
>  		FL_DBG("LIBFLASH:   reading page 0x%08x..0x%08x...",
>  		       page, page + er_size);
> -		rc = flash_read(c, page, c->smart_buf, er_size);
> +		rc = flash_read(hl, page, er_size, c->smart_buf);
>  		if (rc) {
>  			FL_DBG(" error %d!\n", rc);
>  			return rc;
> @@ -514,7 +520,7 @@ int flash_smart_write(struct flash_chip *c, uint32_t dst, const void *src,
>  		case sm_need_write:
>  			/* Just needs writing over */
>  			FL_DBG(" need write !\n");
> -			rc = flash_write(c, dst, src, chunk, true);
> +			rc = flash_write(hl, dst, src, chunk, true);
>  			if (rc) {
>  				FL_DBG("LIBFLASH: Write error %d !\n", rc);
>  				return rc;
> @@ -522,14 +528,14 @@ int flash_smart_write(struct flash_chip *c, uint32_t dst, const void *src,
>  			break;
>  		case sm_need_erase:
>  			FL_DBG(" need erase !\n");
> -			rc = flash_erase(c, page, er_size);
> +			rc = flash_erase(hl, page, er_size);
>  			if (rc) {
>  				FL_DBG("LIBFLASH: erase error %d !\n", rc);
>  				return rc;
>  			}
>  			/* Then update the portion of the buffer and write the block */
>  			memcpy(c->smart_buf + off, src, chunk);
> -			rc = flash_write(c, page, c->smart_buf, er_size, true);
> +			rc = flash_write(hl, page, c->smart_buf, er_size, true);
>  			if (rc) {
>  				FL_DBG("LIBFLASH: write error %d !\n", rc);
>  				return rc;
> @@ -543,14 +549,14 @@ int flash_smart_write(struct flash_chip *c, uint32_t dst, const void *src,
>  	return 0;
>  }
>  
> -int flash_smart_write_corrected(struct flash_chip *c, uint32_t dst, const void *src,
> +int flash_smart_write_corrected(struct highlevel_ops *hl, uint32_t dst, const void *src,
>  		      uint32_t size, bool ecc)
>  {
>  	struct ecc64 *buf;
>  	int rc;
>  
>  	if (!ecc)
> -		return flash_smart_write(c, dst, src, size);
> +		return flash_smart_write(hl, dst, size, src);
>  
>  	buf = malloc(ECC_BUFFER_SIZE(size));
>  	if (!buf)
> @@ -562,7 +568,7 @@ int flash_smart_write_corrected(struct flash_chip *c, uint32_t dst, const void *
>  		goto out;
>  	}
>  
> -	rc = flash_smart_write(c, dst, buf, ECC_BUFFER_SIZE(size));
> +	rc = flash_smart_write(hl, dst, ECC_BUFFER_SIZE(size), buf);
>  
>  out:
>  	free(buf);
> @@ -690,8 +696,9 @@ static int flash_set_4b(struct flash_chip *c, bool enable)
>  	return ct->cmd_wr(ct, enable ? CMD_EN4B : CMD_EX4B, false, 0, NULL, 0);
>  }
>  
> -int flash_force_4b_mode(struct flash_chip *c, bool enable_4b)
> +int flash_force_4b_mode(struct highlevel_ops *hl, bool enable_4b)
>  {
> +	struct flash_chip *c = container_of(hl, struct flash_chip, ops);
>  	struct spi_flash_ctrl *ct = c->ctrl;
>  	int rc;
>  
> @@ -782,24 +789,29 @@ static int flash_configure(struct flash_chip *c)
>  	return 0;
>  }
>  
> -int flash_get_info(struct flash_chip *chip, const char **name,
> +static int flash_get_info(struct highlevel_ops *hl, const char **name,
>  		   uint32_t *total_size, uint32_t *erase_granule)
>  {
> +	struct flash_chip *c = container_of(hl, struct flash_chip, ops);
>  	if (name)
> -		*name = chip->info.name;
> +		*name = c->info.name;
>  	if (total_size)
> -		*total_size = chip->tsize;
> +		*total_size = c->tsize;
>  	if (erase_granule)
> -		*erase_granule = chip->min_erase_mask + 1;
> +		*erase_granule = c->min_erase_mask + 1;
>  	return 0;
>  }
>  
> -int flash_init(struct spi_flash_ctrl *ctrl, struct flash_chip **flash)
> +int flash_init(struct spi_flash_ctrl *ctrl, struct highlevel_ops **hl)
>  {
>  	struct flash_chip *c;
>  	int rc;
>  
> -	*flash = NULL;
> +	if (!hl)
> +		return FLASH_ERR_PARM_ERROR;
> +
> +	*hl = NULL;
> +
>  	c = malloc(sizeof(struct flash_chip));
>  	if (!c)
>  		return FLASH_ERR_MALLOC_FAILED;
> @@ -820,18 +832,25 @@ int flash_init(struct spi_flash_ctrl *ctrl, struct flash_chip **flash)
>  	rc = flash_configure(c);
>  	if (rc)
>  		FL_ERR("LIBFLASH: Flash configuration failed\n");
> - bail:
> +bail:
>  	if (rc) {
>  		free(c);
>  		return rc;
>  	}
> -	*flash = c;
> +
> +	c->ops.read = &flash_read;
> +	c->ops.write = &flash_smart_write;
> +	c->ops.erase = &flash_erase;
> +	c->ops.get_info = &flash_get_info;
> +
> +	*hl = &(c->ops);
>  	return 0;
>  }
>  
> -void flash_exit(struct flash_chip *chip)
> +void flash_exit(struct highlevel_ops *hl)
>  {
>  	/* XXX Make sure we are idle etc... */
> -	free(chip);
> +	if (hl)
> +		free(container_of(hl, struct flash_chip, ops));
>  }
>  
> diff --git a/libflash/libflash.h b/libflash/libflash.h
> index 7ed65d0..3d81813 100644
> --- a/libflash/libflash.h
> +++ b/libflash/libflash.h
> @@ -18,6 +18,7 @@
>  
>  #include <stdint.h>
>  #include <stdbool.h>
> +#include <libflash/highlevel.h>
>  
>  #ifdef __SKIBOOT__
>  #include <skiboot.h>
> @@ -59,18 +60,13 @@ extern bool libflash_debug;
>  struct flash_chip;
>  struct spi_flash_ctrl;
>  
> -int flash_init(struct spi_flash_ctrl *ctrl, struct flash_chip **flash);
> -void flash_exit(struct flash_chip *chip);
> -
> -int flash_get_info(struct flash_chip *chip, const char **name,
> -		   uint32_t *total_size, uint32_t *erase_granule);
> +int flash_init(struct spi_flash_ctrl *ctrl, struct highlevel_ops **hl);
> +void flash_exit(struct highlevel_ops *hl);
>  
>  /* libflash sets the 4b mode automatically based on the flash
>   * size and controller capabilities but it can be overriden
>   */
> -int flash_force_4b_mode(struct flash_chip *chip, bool enable_4b);
> -
> -int flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len);
> +int flash_force_4b_mode(struct highlevel_ops *hl, bool enable_4b);
>  
>  /*
>   * This provides a wapper around flash_read() on ECCed data. All params are
> @@ -80,11 +76,8 @@ int flash_read(struct flash_chip *c, uint32_t pos, void *buf, uint32_t len);
>   * len is length of data without ecc attached therefore this will read beyond
>   * pos + len.
>   */
> -int flash_read_corrected(struct flash_chip *c, uint32_t pos, void *buf,
> +int flash_read_corrected(struct highlevel_ops *hl, uint32_t pos, void *buf,
>  		uint32_t len, bool ecc);
> -int flash_erase(struct flash_chip *c, uint32_t dst, uint32_t size);
> -int flash_write(struct flash_chip *c, uint32_t dst, const void *src,
> -		uint32_t size, bool verify);
>  
>  /*
>   * This provides a wrapper around flash_write() on ECCed data. All params are
> @@ -94,10 +87,8 @@ int flash_write(struct flash_chip *c, uint32_t dst, const void *src,
>   * size is length of data without ECC attached therefore this will write beyond
>   * dst + size.
>   */
> -int flash_write_corrected(struct flash_chip *c, uint32_t dst, const void *src,
> +int flash_write_corrected(struct highlevel_ops *hl, uint32_t dst, const void *src,
>  		uint32_t size, bool verify, bool ecc);
> -int flash_smart_write(struct flash_chip *c, uint32_t dst, const void *src,
> -		      uint32_t size);
>  
>  /*
>   * This provides a wrapper around flash_smart_write() on ECCed data. All
> @@ -107,12 +98,12 @@ int flash_smart_write(struct flash_chip *c, uint32_t dst, const void *src,
>   * size is length of data without ECC attached therefore this will write beyond
>   * dst + size.
>   */
> -int flash_smart_write_corrected(struct flash_chip *c, uint32_t dst, const void *src,
> +int flash_smart_write_corrected(struct highlevel_ops *hl, uint32_t dst, const void *src,
>  		      uint32_t size, bool ecc);
>  
>  /* chip erase may not be supported by all chips/controllers, get ready
>   * for FLASH_ERR_CHIP_ER_NOT_SUPPORTED
>   */
> -int flash_erase_chip(struct flash_chip *c);
> +int flash_erase_chip(struct highlevel_ops *hl);
>  
>  #endif /* __LIBFLASH_H */
> diff --git a/libflash/test/Makefile.check b/libflash/test/Makefile.check
> index 276de0c..7e765a8 100644
> --- a/libflash/test/Makefile.check
> +++ b/libflash/test/Makefile.check
> @@ -30,4 +30,5 @@ clean: libflash-test-clean
>  
>  libflash-test-clean:
>  	$(RM) libflash/test/*.o $(LIBFLASH_TEST)
> +	$(RM) libflash/test/*.d
>  	$(RM) libflash/test/*-gcov

Also a hunk that isn't all that related to what I'm doing and could
really be applied at any time. I'll send a standalone

> diff --git a/libflash/test/test-flash.c b/libflash/test/test-flash.c
> index 4479b5a..98205de 100644
> --- a/libflash/test/test-flash.c
> +++ b/libflash/test/test-flash.c
> @@ -366,7 +366,7 @@ struct spi_flash_ctrl sim_ctrl = {
>  
>  int main(void)
>  {
> -	struct flash_chip *fl;
> +	struct highlevel_ops *hl;
>  	uint32_t total_size, erase_granule;
>  	const char *name;
>  	uint16_t *test;
> @@ -378,12 +378,12 @@ int main(void)
>  	memset(sim_image, 0xff, sim_image_sz);
>  	test = malloc(0x10000 * 2);
>  
> -	rc = flash_init(&sim_ctrl, &fl);
> +	rc = flash_init(&sim_ctrl, &hl);
>  	if (rc) {
>  		ERR("flash_init failed with err %d\n", rc);
>  		exit(1);
>  	}
> -	rc = flash_get_info(fl, &name, &total_size, &erase_granule);
> +	rc = flash_get_info(hl, &name, &total_size, &erase_granule);
>  	if (rc) {
>  		ERR("flash_get_info failed with err %d\n", rc);
>  		exit(1);
> @@ -395,13 +395,13 @@ int main(void)
>  
>  	/* Write 64k of stuff at 0 and at 128k */
>  	printf("Writing test patterns...\n");
> -	flash_smart_write(fl, 0, test, 0x10000);
> -	flash_smart_write(fl, 0x20000, test, 0x10000);
> +	flash_smart_write(hl, 0, 0x10000, test);
> +	flash_smart_write(hl, 0x20000, 0x10000, test);
>  
>  	/* Write "Hello world" straddling the 64k boundary */
>  #define HW "Hello World"
>  	printf("Writing test string...\n");
> -	flash_smart_write(fl, 0xfffc, HW, sizeof(HW));
> +	flash_smart_write(hl, 0xfffc, sizeof(HW), HW);
>  
>  	/* Check result */
>  	if (memcmp(sim_image + 0xfffc, HW, sizeof(HW))) {
> @@ -416,7 +416,7 @@ int main(void)
>  	printf("Test pattern pass\n");
>  
>  	printf("Test ECC interfaces\n");
> -	flash_smart_write_corrected(fl, 0, test, 0x10000, 1);
> +	flash_smart_write_corrected(hl, 0, test, 0x10000, 1);
>  	ecc_test = (struct ecc64 *)sim_image;
>  	test64 = (uint64_t *)test;
>  	for (i = 0; i < 0x10000 / sizeof(*ecc_test); i++) {
> @@ -433,7 +433,7 @@ int main(void)
>  	printf("Test ECC interface pass\n");
>  
>  	printf("Test ECC erase\n");
> -	if (flash_erase(fl, 0, 0x10000) != 0) {
> +	if (flash_erase(hl, 0, 0x10000) != 0) {
>  		ERR("flash_erase didn't return 0\n");
>  		exit(1);
>  	}
> @@ -444,7 +444,7 @@ int main(void)
>  			ERR("Data not properly cleared at %d\n", i);
>  			exit(1);
>  		}
> -		rc = flash_write(fl, i * sizeof(*ecc_test) + 8, &zero, 1, 0);
> +		rc = flash_write(hl, i * sizeof(*ecc_test) + 8, &zero, 1, 0);
>  		if (rc || ecc_test[i].ecc != 0) {
>  			ERR("Cleared data not correctly ECCed: 0x%02x (0x%016lx) expecting 0 at %d\n", ecc_test[i].ecc, ecc_test[i].data, i);
>  			exit(1);
> @@ -452,7 +452,7 @@ int main(void)
>  	}
>  	printf("Test ECC erase pass\n");
>  
> -	flash_exit(fl);
> +	flash_exit(hl);
>  
>  	return 0;
>  }
> diff --git a/platforms/astbmc/pnor.c b/platforms/astbmc/pnor.c
> index 8e88edd..c30322e 100644
> --- a/platforms/astbmc/pnor.c
> +++ b/platforms/astbmc/pnor.c
> @@ -20,6 +20,7 @@
>  #include <opal.h>
>  #include <libflash/libflash.h>
>  #include <libflash/libffs.h>
> +#include <libflash/highlevel.h>
>  #include <ast.h>
>  
>  #include "astbmc.h"
> @@ -27,7 +28,7 @@
>  int pnor_init(void)
>  {
>  	struct spi_flash_ctrl *pnor_ctrl;
> -	struct flash_chip *pnor_chip;
> +	struct highlevel_funcs *funcs;
>  	int rc;
>  
>  	/* Open controller and flash. If the LPC->AHB doesn't point to
> @@ -44,19 +45,20 @@ int pnor_init(void)
>  		prerror("PLAT: Failed to open PNOR flash controller\n");
>  		goto fail;
>  	}
> -	rc = flash_init(pnor_ctrl, &pnor_chip);
> +
> +	rc = flash_init(pnor_ctrl, &funcs);
>  	if (rc) {
>  		prerror("PLAT: Failed to open init PNOR driver\n");
>  		goto fail;
>  	}
>  
> -	rc = flash_register(pnor_chip, true);
> +	rc = flash_register(funcs, true);
>  	if (!rc)
>  		return 0;
>  
>   fail:
> -	if (pnor_chip)
> -		flash_exit(pnor_chip);
> +	if (funcs)
> +		flash_exit(funcs);
>  	if (pnor_ctrl)
>  		ast_sf_close(pnor_ctrl);
>  
> diff --git a/platforms/rhesus/rhesus.c b/platforms/rhesus/rhesus.c
> index 50769cf..6182315 100644
> --- a/platforms/rhesus/rhesus.c
> +++ b/platforms/rhesus/rhesus.c
> @@ -118,7 +118,7 @@ static int64_t rhesus_power_down(uint64_t request __unused)
>  static int rhesus_pnor_init(void)
>  {
>  	struct spi_flash_ctrl *pnor_ctrl;
> -	struct flash_chip *pnor_chip;
> +	struct highlevel_funcs *funcs;
>  	int rc;
>  
>  	/* Open controller, flash and ffs */
> @@ -127,19 +127,19 @@ static int rhesus_pnor_init(void)
>  		prerror("PLAT: Failed to open PNOR flash controller\n");
>  		goto fail;
>  	}
> -	rc = flash_init(pnor_ctrl, &pnor_chip);
> +	rc = flash_init(pnor_ctrl, &funcs);
>  	if (rc) {
>  		prerror("PLAT: Failed to open init PNOR driver\n");
>  		goto fail;
>  	}
>  
> -	rc = flash_register(pnor_chip, true);
> +	rc = flash_register(funcs, true);
>  	if (!rc)
>  		return 0;
>  
>   fail:
> -	if (pnor_chip)
> -		flash_exit(pnor_chip);
> +	if (funcs)
> +		flash_exit(funcs);
>  	if (pnor_ctrl)
>  		sfc_close(pnor_ctrl);
>

Cyril,

A few minor comments inline below. Generally looks ok to me, although I still don't 
like the name/terminology  :)

Maybe it should just be s/highlevel/blocklevel/ and s/highlevel_(ops|
funcs)/blocklevel_device/ ? That seems like a better global description of what we're 
doing.

Also it's a fairly long patch. It might be better to split it up into two - one that adds the 
blocklevel functions/datastructures and one that switches everything over to use 
them.

For those who don't know the background the patch started out as a way of 
implementing "golden image" support by allowing read-only areas to be enforced at a 
global level, however it also makes it much easier to add other backends (eg. 
memboot) and more straight forward usage of libffs for userspace tools not requiring 
libflash, such as a more advanced memboot command that "merges" images.

Regards,

Alistair

On Wed, 8 Apr 2015 11:19:07 Cyril Bur wrote:
> Currently libffs uses libflash to read data. This does works well if the
> data is directly on flash hardware that libflash is designed to read from.
> Problems arise when the data is not on an actual flash hardware but libffs
> is needed to parse ffs partition data.
> 
> This patch begins the work to decouple libffs and libflash as well as
> starting the work to provide an abstraction so that libffs may eventually
> be able to read its data from essentially any source.
> 
> ---
> V2: Added Alistairs feedback.
>     Modified core/flash.c to use the new 'highlevel' interface
>     Modified pflash to use the new 'highlevel' interface
> V3: Changed everything to add the container_of stuff. Looks good!
> V4: Very minor modifications. Made the tests work.
> 
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  core/flash.c                 | 34 ++++++++++--------
>  external/pflash/Makefile     |  2 +-
>  external/pflash/ast.h        |  1 +
>  external/pflash/pflash.c     | 33 +++++++++---------
>  libflash/highlevel.c         | 51 +++++++++++++++++++++++++++
>  libflash/highlevel.h         | 38 ++++++++++++++++++++
>  libflash/libffs.c            | 51 +++++++++++----------------
>  libflash/libffs.h            |  7 ++--
>  libflash/libflash.c          | 83
> +++++++++++++++++++++++++++----------------- libflash/libflash.h          |
> 25 +++++--------
>  libflash/test/Makefile.check |  1 +
>  libflash/test/test-flash.c   | 20 +++++------
>  platforms/astbmc/pnor.c      | 12 ++++---
>  platforms/rhesus/rhesus.c    | 10 +++---
>  14 files changed, 235 insertions(+), 133 deletions(-)
>  create mode 100644 libflash/highlevel.c
>  create mode 100644 libflash/highlevel.h
> 
> diff --git a/core/flash.c b/core/flash.c
> index 7d32c8d..3dc0a0d 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -21,12 +21,13 @@
>  #include <device.h>
>  #include <libflash/libflash.h>
>  #include <libflash/libffs.h>
> +#include <libflash/highlevel.h>
>  #include <ecc.h>
> 
>  struct flash {
>  	bool			registered;
>  	bool			busy;
> -	struct flash_chip	*chip;
> +	struct highlevel_ops *ops;
>  	uint32_t		size;
>  	uint32_t		block_size;
>  };
> @@ -107,7 +108,7 @@ static int flash_nvram_start_read(void *dst, uint32_t
> src, uint32_t len) goto out;
>  	}
> 
> -	rc = flash_read(nvram_flash->chip, nvram_offset + src, dst, len);
> +	rc = highlevel_read(nvram_flash->ops, nvram_offset + src, len, dst);
> 
>  out:
>  	unlock(&flash_lock);
> @@ -136,7 +137,7 @@ static int flash_nvram_write(uint32_t dst, void *src,
> uint32_t len) rc = OPAL_PARAMETER;
>  		goto out;
>  	}
> -	rc = flash_smart_write(nvram_flash->chip, nvram_offset + dst, src, len);
> +	rc = highlevel_write(nvram_flash->ops, nvram_offset + dst, len, src);
> 
>  out:
>  	unlock(&flash_lock);
> @@ -249,7 +250,7 @@ static void setup_system_flash(struct flash *flash,
> struct dt_node *node, flash_nvram_probe(flash, ffs);
>  }
> 
> -int flash_register(struct flash_chip *chip, bool is_system_flash)
> +int flash_register(struct highlevel_ops *hl, bool is_system_flash)
>  {
>  	uint32_t size, block_size;
>  	struct ffs_handle *ffs;
> @@ -259,7 +260,7 @@ int flash_register(struct flash_chip *chip, bool
> is_system_flash) unsigned int i;
>  	int rc;
> 
> -	rc = flash_get_info(chip, &name, &size, &block_size);
> +	rc = highlevel_get_info(hl, &name, &size, &block_size);
>  	if (rc)
>  		return rc;
> 
> @@ -275,7 +276,7 @@ int flash_register(struct flash_chip *chip, bool
> is_system_flash) flash = &flashes[i];
>  		flash->registered = true;
>  		flash->busy = false;
> -		flash->chip = chip;
> +		flash->funcs = hl;
>  		flash->size = size;
>  		flash->block_size = block_size;
>  		break;
> @@ -287,7 +288,7 @@ int flash_register(struct flash_chip *chip, bool
> is_system_flash) return OPAL_RESOURCE;
>  	}
> 
> -	rc = ffs_open_flash(chip, 0, flash->size, &ffs);
> +	rc = ffs_init(0, flash->size, hl, &ffs);
>  	if (rc) {
>  		prlog(PR_WARNING, "FLASH: No ffs info; "
>  				"using raw device only\n");