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

[Alistair Popple]

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 at 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");
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20150508/ad005349/attachment-0001.html>