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

[Vasant Hegde]

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?


---
  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