[Skiboot] [PATCH v3 18/18] mbox: Reset bmc mbox in MPIPL path

[Andrew Jeffery]

Hi Vasant,

I'm going to mix things up a bit here and re-order the diff hunks so my flow of comments makes more sense. Starting with:

> --- a/libflash/blocklevel.h
> +++ b/libflash/blocklevel.h
> @@ -47,6 +47,7 @@ struct blocklevel_device {
>  	int (*erase)(struct blocklevel_device *bl, uint64_t pos, uint64_t len);
>  	int (*get_info)(struct blocklevel_device *bl, const char **name, 
> uint64_t *total_size,
>  			uint32_t *erase_granule);
> +	int (*reset)(struct blocklevel_device *bl);

I feel that adding a reset operation to blocklevel_device is wrong - we're adding a function that should be generic but are doing so for a specific backend (mbox). What does it mean to reset() a flash device in general? I don't think there's a good answer to that, so lets try to avoid it and remove this member.

So in an effort to do that:

> diff --git a/core/flash.c b/core/flash.c
> index e3be57613..474f36f0f 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -76,6 +76,14 @@ void flash_release(void)
>  	unlock(&flash_lock);
>  }
>  
> +void flash_reset(void)

Lets call this flash_unregister() instead.

> +{
> +	struct blocklevel_device *bl = system_flash->bl;
> +
> +	if (dt_find_compatible_node(dt_root, NULL, "mbox"))
> +		bl->reset(bl);

Lets change this to a call to mbox_flash_exit(bl).

> +}
> +
>  static int flash_nvram_info(uint32_t *total_size)
>  {
>  	int rc;
> diff --git a/hw/sbe-p9.c b/hw/sbe-p9.c
> index 627ef428a..c6df40fa2 100644
> --- a/hw/sbe-p9.c
> +++ b/hw/sbe-p9.c
> @@ -941,6 +941,9 @@ void __attribute__((noreturn)) 
> p9_sbe_terminate(const char *msg)
>  	int rc, waited = 0;
>  	struct dt_node *xn;
>  
> +	/* Reset BMC MBOX driver */
> +	flash_reset();

becomes flash_unregister();

> +
>  	dt_for_each_compatible(dt_root, xn, "ibm,xscom") {
>  		chip_id = dt_get_chip_id(xn);
>  
> diff --git a/include/skiboot.h b/include/skiboot.h
> index b4bdf3779..badbd9ebf 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -255,6 +255,7 @@ extern int flash_start_preload_resource(enum 
> resource_id id, uint32_t subid,
>  extern int flash_resource_loaded(enum resource_id id, uint32_t idx);
>  extern bool flash_reserve(void);
>  extern void flash_release(void);
> +extern void flash_reset(void);

And again here.

>  
>  	/*
>  	 * Keep the erase mask so that blocklevel_erase() can do sanity 
> checking
> diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c
> index 6742d215c..dd5ac2f88 100644
> --- a/libflash/mbox-flash.c
> +++ b/libflash/mbox-flash.c
> @@ -818,6 +818,27 @@ static int mbox_flash_read(struct blocklevel_device 
> *bl, uint64_t pos,
>  	return rc;
>  }
>  
> +static int mbox_flash_reset(struct blocklevel_device *bl)
> +{
> +	int rc;
> +	struct mbox_flash_data *mbox_flash;
> +	struct bmc_mbox_msg msg = MSG_CREATE(MBOX_C_RESET_STATE);
> +
> +	mbox_flash = container_of(bl, struct mbox_flash_data, bl);
> +
> +	rc = msg_send(mbox_flash, &msg, mbox_flash->timeout);
> +	if (rc) {
> +		prlog(PR_ERR, "Failed to enqueue/send BMC MBOX RESET msg\n");
> +		return rc;
> +	}
> +	if (wait_for_bmc(mbox_flash, mbox_flash->timeout)) {
> +		prlog(PR_ERR, "Error waiting for BMC\n");
> +		return rc;
> +	}
> +
> +	return OPAL_SUCCESS;
> +}
> +
>  static int mbox_flash_get_info(struct blocklevel_device *bl, const char **name,
>  		uint64_t *total_size, uint32_t *erase_granule)
>  {
> @@ -1147,6 +1168,7 @@ int mbox_flash_init(struct blocklevel_device **bl)
>  	mbox_flash->bl.write = &mbox_flash_write;
>  	mbox_flash->bl.erase = &mbox_flash_erase_v2;
>  	mbox_flash->bl.get_info = &mbox_flash_get_info;
> +	mbox_flash->bl.reset = &mbox_flash_reset;

As per the comment at the top of my mail, this callback goes away so we need to do something else. Instead we should call mbox_flash_reset() in mbox_flash_exit() (which we now call in flash_unregister() under the condition that we're using the mbox backend).

Thoughts?

Andrew

>  
>  	if (bmc_mbox_get_attn_reg() & MBOX_ATTN_BMC_REBOOT)
>  		rc = handle_reboot(mbox_flash);
> -- 
> 2.14.3
>