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

[Andrew Jeffery]

On Thu, 7 Jun 2018, at 15:34, Vasant Hegde wrote:
> On 06/07/2018 06:55 AM, Andrew Jeffery wrote:
> > Hi Vasant,
> 
> Hi Andrew,
> 
> > 
> > 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.
> 
> Makes sense. Lets 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.
> 
> Done.
> 
> > 
> >> +{
> >> +	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).
> 
> Done.
> .../...
> 
> >>   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).
> 
> So we end up calling mbox_flash_reset in all mbox_flash_exit() scenarios.. May 
> be that's fine as
> it will try to bring MBOX back to the state where we can reboot.
> 
> Does below changes looks good?

Yeah, the patch below looks good to me.

Reviewed-by: Andrew Jeffery <andrew at aj.id.au>

> 
> 
> ---
>   core/flash.c          |  9 +++++++++
>   hw/sbe-p9.c           |  3 +++
>   include/skiboot.h     |  1 +
>   libflash/mbox-flash.c | 23 +++++++++++++++++++++++
>   4 files changed, 36 insertions(+)
> 
> diff --git a/core/flash.c b/core/flash.c
> index e3be57613..4a9014d87 100644
> --- a/core/flash.c
> +++ b/core/flash.c
> @@ -23,6 +23,7 @@
>   #include <device.h>
>   #include <libflash/libflash.h>
>   #include <libflash/libffs.h>
> +#include <libflash/mbox-flash.h>
>   #include <libflash/blocklevel.h>
>   #include <libflash/ecc.h>
>   #include <libstb/secureboot.h>
> @@ -76,6 +77,14 @@ void flash_release(void)
>   	unlock(&flash_lock);
>   }
> 
> +void flash_unregister(void)
> +{
> +	struct blocklevel_device *bl = system_flash->bl;
> +
> +	if (dt_find_compatible_node(dt_root, NULL, "mbox"))
> +		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..56f666b5c 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;
> 
> +	/* Unregister flash. It will request BMC MBOX reset */
> +	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..c9d2b23fe 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_unregister(void);
>   #define FLASH_SUBPART_ALIGNMENT 0x1000
>   #define FLASH_SUBPART_HEADER_SIZE FLASH_SUBPART_ALIGNMENT
>   extern int flash_subpart_info(void *part_header, uint32_t header_len,
> diff --git a/libflash/mbox-flash.c b/libflash/mbox-flash.c
> index 6742d215c..037fdd2b9 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)
>   {
> @@ -1167,6 +1188,8 @@ void mbox_flash_exit(struct blocklevel_device *bl)
>   {
>   	struct mbox_flash_data *mbox_flash;
>   	if (bl) {
> +		mbox_flash_reset(bl);
> +
>   		mbox_flash = container_of(bl, struct mbox_flash_data, bl);
>   		free(mbox_flash);
>   	}
> -- 
> 2.14.3
> 
> 
> 
> -Vasant
>