[Skiboot] [PATCH 1/2] Change load_resource() API to be all about preloading.

[Joel Stanley]

On Tue, 2015-03-24 at 14:31 +1100, Stewart Smith wrote:
> No functional changes in what happens, just have two calls, one for
> queueing preload the other for waiting until it has loaded.
> 
> future patches will introduce platform specific queueing.

I started to implement these calls for async loading on the BMC
platforms and had a few questions come up.

> Signed-off-by: Stewart Smith <stewart at linux.vnet.ibm.com>

> ---
>  core/flash.c                |    4 ++--
>  core/init.c                 |   26 +++++++++++++++++++-------
>  core/platform.c             |   33 ++++++++++++++++++++++++++++-----
>  hw/fsp/fsp.c                |   14 +++++++-------
>  hw/phb3.c                   |   10 +++++++++-
>  include/fsp.h               |    4 ++--
>  include/platform.h          |   29 +++++++++++++++++++++++------
>  include/skiboot.h           |    4 ++--
>  platforms/astbmc/habanero.c |    2 +-
>  platforms/astbmc/palmetto.c |    2 +-
>  platforms/ibm-fsp/apollo.c  |    2 +-
>  platforms/ibm-fsp/firenze.c |    2 +-
>  12 files changed, 96 insertions(+), 36 deletions(-)
> 
> diff --git a/core/flash.c b/core/flash.c
> index f2d7501..270e6fc 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -522,8 +522,8 @@ end:
>   * load a resource from FLASH
>   * buf and len shouldn't account for ECC even if partition is ECCed.
>   */
> -bool flash_load_resource(enum resource_id id, uint32_t subid,
> -		void *buf, size_t *len)
> +int flash_start_preload_resource(enum resource_id id, uint32_t subid,
> +				 void *buf, size_t *len)

Not convinced it's worth changing from bool to the opal error codes as
the callers only check for success. Up to you though.

>  {
>  	int i, rc, part_num, part_size, part_start, size;
>  	struct ffs_handle *ffs;

> diff --git a/core/platform.c b/core/platform.c
> index 7cf9727..5369092 100644
> --- a/core/platform.c
> +++ b/core/platform.c

> +
> +int wait_for_resource_loaded(enum resource_id id, uint32_t idx)
> +{
> +	int r = resource_loaded(id, idx);
> +
> +	while(r == OPAL_BUSY) {
> +		opal_run_pollers();
> +		time_wait_nopoll(msecs_to_tb(5));

time_wait_ms_nopoll(5);

> +		cpu_relax();

time_wait calls cpu_relax while it loops.

> +		r = resource_loaded(id, idx);
> +	}
>  
> +	return r;
>  }

> diff --git a/include/platform.h b/include/platform.h
> index bdf8bdf..2900b4e 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -129,11 +129,24 @@ struct platform {
>  	int		(*elog_commit)(struct errorlog *buf);
>  
>  	/*
> -	 * Load an external resource (eg, kernel payload) into a preallocated
> -	 * buffer. Returns true on success.
> +	 * Initiate loading an external resource (e.g. kernel payload, OCC)
> +	 * into a preallocated buffer.
> +	 * This is designed to asynchronously load external resources.

Not sure that the last line is true. It could say "The implementation
should asynchronously load external resources.", but I'm not sure if
that adds much.

> +	 * Returns OPAL_SUCCESS or error.
>  	 */
> -	bool		(*load_resource)(enum resource_id id, uint32_t idx,
> -					 void *buf, size_t *len);
> +	int		(*start_preload_resource)(enum resource_id id,
> +						  uint32_t idx,
> +						  void *buf, size_t *len);

With this scheme we need to know the final size in the synch path. On
the BMC platforms this would mean searching the TOC for the partition
we're after. We could hide more of the work if the size was returned
asynchronously.

> +
> diff --git a/include/skiboot.h b/include/skiboot.h
> index 0fe50e9..6cb9b2d 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -199,8 +199,8 @@ extern void occ_fsp_init(void);
>  /* flash support */
>  struct flash_chip;
>  extern int flash_register(struct flash_chip *chip, bool is_system_flash);
> -extern bool flash_load_resource(enum resource_id id, uint32_t subid,
> -		void *buf, size_t *len);
> +extern int flash_start_preload_resource(enum resource_id id, uint32_t subid,
> +					void *buf, size_t *len);
>  extern bool flash_reserve(void);
>  extern void flash_release(void);
>  
> diff --git a/platforms/astbmc/habanero.c b/platforms/astbmc/habanero.c
> index c4875ef..d56e451 100644
> --- a/platforms/astbmc/habanero.c
> +++ b/platforms/astbmc/habanero.c
> @@ -53,6 +53,6 @@ DECLARE_PLATFORM(habanero) = {
>  	.cec_power_down         = astbmc_ipmi_power_down,
>  	.cec_reboot             = astbmc_ipmi_reboot,
>  	.elog_commit		= ipmi_elog_commit,
> -	.load_resource		= flash_load_resource,
> +	.start_preload_resource	= flash_start_preload_resource,

I think we should leave the name of the flash functions as they were as
they do not (yet) do any preloading.

>  	.exit			= ipmi_wdt_final_reset,
>  };