[v7,2/2] Add SBE driver support

SBE (Self Boot Engine) on P9 has two different jobs:
  - Boot the chip up to the point the core is functional
  - Provide various services like timer, scom, stash MPIPL, etc., at runtime

OPAL can communicate to SBE via a set of data and control registers provided
by the PSU block in P9 chip.
 - Four 8 byte registers for Host to send command packets to SBE
 - Four 8 byte registers for SBE to send response packets to Host
 - Two doorbell registers (1 on each side) to alert either party
   when data is placed in above mentioned data register

Protocol constraints:
  Only one command is accepted in the command buffer until the response for the
  command is enqueued in the response buffer by SBE.

Usage:
  We will use SBE for various purposes like timer, MPIPL, etc.

This patch implements the SBE MBOX spec for OPAL to communicate with
SBE.

Design consideration:
  - Each chip has SBE. We need to track SBE messages per chip. Hence added
    per chip sbe structure and list of messages to that chip
  - SBE accepts only one command at a time. Hence serialized MBOX commands.
  - OPAL gets interrupted once SBE sets doorbell register
  - OPAL has to clear doorbell register after reading response
  - Every command class has timeout option. Timed out messages are discarded
  - SBE MBOX commands can be classified into four types :
    - Those that must be sent to the master only (ex: sending MDST/MDDT info)
    - Those that must be sent to slaves only (ex: continue MPIPL)
    - Those that must be sent to all chips (ex: close insecure window)
    - Those that can be sent to any chip (ex: timer)

Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
---
Ben,
  Note that I've changed default timeout value from 100us to 500us as we
  have some issues with 100us. Once that's fixed I will change that to
  100us.

-Vasant

 core/init.c      |   7 +
 hw/psi.c         |   2 +-
 hw/sbe-p9.c      | 658 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 include/chip.h   |   4 +
 include/sbe-p9.h | 206 ++++++++++++++++-
 5 files changed, 856 insertions(+), 21 deletions(-)

On Mon, 2018-04-16 at 17:17 +0530, Vasant Hegde wrote:
> 
> +static inline bool p9_sbe_mbox_busy(struct p9_sbe *sbe)
> +{
> +	return sbe->state != sbe_mbox_idle ? true : false;
> +}

What's wrong with just return sbe->state != sbe_mbox_idle ?

> +static inline void p9_sbe_mbox_update_state(struct p9_sbe *sbe,
> +					    enum p9_sbe_mbox_state state)
> +{
> +	sbe->state = state;
> +}

What's the point of the above wrapper ? I don't find it makes
anything clearer... 

> +static inline bool p9_sbe_msg_busy(struct p9_sbe_msg *msg)
> +{
> +	switch (msg->state) {
> +	case sbe_msg_queued:
> +	/* fall through */
> +	case sbe_msg_sent:
> +	case sbe_msg_wresp:
> +		return true;
> +	default:	/* + sbe_msg_unused, sbe_msg_done,
> +			     sbe_msg_timeout, sbe_msg_error */
> +		break;
> +	}
> +	return false;
> +}
> +
> +static inline struct p9_sbe *p9_sbe_get_sbe(u32 chip_id)
> +{
>  	struct proc_chip *chip;
>  
> -	chip = get_chip(chip_id);
> -	if (chip == NULL)
> +	/* Default to SBE on master chip */
> +	if (chip_id == -1) {
> +		if (sbe_default_chip_id == -1)
> +			return NULL;
> +
> +		chip = get_chip(sbe_default_chip_id);
> +	} else {
> +		chip = get_chip(chip_id);
> +	}
> +	if (chip == NULL || chip->sbe == NULL)
> +		return NULL;
> +
> +	return chip->sbe;
> +}
> +
> +static int p9_sbe_msg_send(u32 chip_id, struct p9_sbe_msg *msg)
> +{
> +	int rc, i;
> +	u64 addr, *data;
> +
> +	addr = PSU_HOST_SBE_MBOX_REG0;
> +	data = &msg->reg[0];
> +
> +	for (i = 0; i < NR_HOST_SBE_MBOX_REG; i++) {
> +		rc = xscom_write(chip_id, addr, *data);
> +		if (rc)
> +			return rc;
> +
> +		addr++;
> +		data++;
> +	}
> +
> +	rc = xscom_write(chip_id, PSU_SBE_DOORBELL_REG_OR,
> +			 HOST_SBE_MSG_WAITING);
> +	return rc;
> +}
> +
> +static int p9_sbe_msg_receive(u32 chip_id, struct p9_sbe_msg *resp)
> +{
> +	int i;
> +	int rc = OPAL_SUCCESS;
> +	u64 addr, *data;
> +
> +	addr = PSU_HOST_SBE_MBOX_REG4;
> +	data = &resp->reg[0];
> +
> +	for (i = 0; i < NR_HOST_SBE_MBOX_REG; i++) {
> +		rc = xscom_read(chip_id, addr, data);
> +		if (rc)
> +			return rc;
> +
> +		addr++;
> +		data++;
> +	}
> +	return rc;
> +}
> +
> +/* WARNING: This will drop sbe->lock */
> +static void p9_sbe_msg_complete(struct p9_sbe *sbe, struct p9_sbe_msg *msg)
> +{
> +	prlog(PR_TRACE, "Completing msg [chip id = %x], reg0 : 0x%llx\n",
> +	      sbe->chip_id, msg->reg[0]);
> +
> +	list_del(&msg->link);
> +	if (msg->complete) {
> +		unlock(&sbe->lock);
> +		msg->complete(msg);
> +		lock(&sbe->lock);
> +	}
> +}
> +
> +/* WARNING: This will drop sbe->lock */
> +static void p9_sbe_send_complete(struct p9_sbe *sbe)
> +{
> +	struct p9_sbe_msg *msg;
> +
> +	if (list_empty(&sbe->msg_list))
>  		return;
>  
> +	msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
> +	/* Need response */
> +	if (msg->response) {
> +		msg->state = sbe_msg_wresp;
> +	} else {
> +		msg->state = sbe_msg_done;
> +		p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
> +		p9_sbe_msg_complete(sbe, msg);
> +	}
> +}

Subtle race: if somebody is polling on msg->state waiting for it to be
done without holding the SBE lock (which could happen), then there's a
risk that the message gets freed right after msg->state is updated.

msg->state should be set to "done" *inside* p9_sbe_msg_complete, and
the latter should make sure it snapshots msg->complete before it
changes msg->state (with a nice fat sync in between). It also needs to
take it out of the list before updating the state as well.

> +/* WARNING: This will drop sbe->lock */
> +static void p9_sbe_process_queue(struct p9_sbe *sbe)
> +{
> +	int rc, retry_cnt = 0;
> +	struct p9_sbe_msg *msg = NULL;
> +
> +	if (p9_sbe_mbox_busy(sbe))
> +		return;
> +
> +	while (!list_empty(&sbe->msg_list)) {
> +		msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
> +		/* Send message */
> +		rc = p9_sbe_msg_send(sbe->chip_id, msg);
> +		if (rc == OPAL_SUCCESS)
> +			break;
> +
> +		prlog(PR_ERR, "Failed to send message to SBE [chip id = %x]\n",
> +		      sbe->chip_id);
> +		if (msg->resp) {
> +			p9_sbe_set_primary_rc(msg->resp,
> +					      SBE_STATUS_PRI_GENERIC_ERR);
> +		}
> +		msg->state = sbe_msg_error;
> +		p9_sbe_msg_complete(sbe, msg);
> +
> +		/*
> +		 * Repeatedly failed to send message to SBE. Lets stop
> +		 * sending message.
> +		 */
> +		if (retry_cnt++ >= 3) {
> +			prlog(PR_ERR, "Temporarily stopped sending "
> +			      "message to SBE\n");
> +			return;
> +		}
> +	}
> +
> +	if (list_empty(&sbe->msg_list))
> +		return;
> +
> +	prlog(PR_TRACE, "Message queued [chip id = 0x%x]:\n\t Reg0 : %016llx"
> +	      "\n\t Reg1 : %016llx\n\t Reg2 : %016llx\n\t Reg3 : %016llx\n",
> +	      sbe->chip_id, msg->reg[0], msg->reg[1], msg->reg[2], msg->reg[3]);
> +
> +	msg->timeout = mftb() + msecs_to_tb(SBE_CMD_TIMEOUT_MAX);
> +	p9_sbe_mbox_update_state(sbe, sbe_mbox_send);
> +	msg->state = sbe_msg_sent;

I would personally have the 2 above inside p9_sbe_msg_send(), any
reason why not ?

> +}
> +
> +/*
> + * WARNING:
> + *         Only one command is accepted in the command buffer until response
> + *         to the command is enqueued in the response buffer by SBE.
> + *
> + *         Head of msg_list contains in-flight message. Hence we should always
> + *         add new message to tail of the list.
> + */
> +int p9_sbe_queue_msg(u32 chip_id, struct p9_sbe_msg *msg,
> +		     void (*comp)(struct p9_sbe_msg *msg))
> +{
> +	struct p9_sbe *sbe;
> +
> +	if (!msg)
> +		return OPAL_PARAMETER;
> +
> +	sbe = p9_sbe_get_sbe(chip_id);
> +	if (!sbe)
> +		return OPAL_HARDWARE;
> +
> +	lock(&sbe->lock);
> +	/* Set completion and update sequence number */
> +	msg->complete = comp;
> +	msg->state = sbe_msg_queued;
> +	msg->reg[0] = msg->reg[0] | ((u64)sbe->cur_seq << 16);
> +	sbe->cur_seq++;
> +
> +	/* Reset sequence number */
> +	if (sbe->cur_seq == 0xffff)
> +		sbe->cur_seq = 1;
> +
> +	/* Add message to queue */
> +	list_add_tail(&sbe->msg_list, &msg->link);
> +	p9_sbe_process_queue(sbe);
> +	unlock(&sbe->lock);
> +
> +	return OPAL_SUCCESS;
> +}
> +
> +int p9_sbe_sync_msg(u32 chip_id, struct p9_sbe_msg *msg, bool autofree)
> +{
> +	int rc;
> +	struct p9_sbe *sbe;
> +
> +	rc = p9_sbe_queue_msg(chip_id, msg, NULL);
> +	if (rc)
> +		goto free_msg;
> +
> +	sbe = p9_sbe_get_sbe(chip_id);
> +	if (!sbe) {
> +		rc = OPAL_HARDWARE;
> +		goto free_msg;
> +	}
> +
> +	while (p9_sbe_msg_busy(msg)) {
> +		cpu_relax();
> +		p9_sbe_timeout_poll_one(sbe);
> +	}
> +
> +	if (msg->state == sbe_msg_done)
> +		rc = SBE_STATUS_PRI_SUCCESS;
> +	else
> +		rc = SBE_STATUS_PRI_GENERIC_ERR;
> +
> +	if (msg->response && msg->resp)
> +		rc = p9_sbe_get_primary_rc(msg->resp);
> +
> +free_msg:
> +	if (autofree)
> +		p9_sbe_freemsg(msg);
> +
> +	return rc;
> +}
> +
> +/* Remove SBE message from queue. It will not remove inflight message */
> +int p9_sbe_cancelmsg(u32 chip_id, struct p9_sbe_msg *msg)
> +{
> +	struct p9_sbe *sbe;
> +
> +	sbe = p9_sbe_get_sbe(chip_id);
> +	if (!sbe)
> +		return OPAL_PARAMETER;
> +
> +	lock(&sbe->lock);
> +	if (msg->state != sbe_msg_queued) {
> +		unlock(&sbe->lock);
> +		return OPAL_BUSY;
> +	}
> +
> +	list_del(&msg->link);
> +	msg->state = sbe_msg_done;
> +	unlock(&sbe->lock);
> +	return OPAL_SUCCESS;
> +}
> +
> +static void p9_sbe_handle_response(u32 chip_id, struct p9_sbe_msg *msg)
> +{
> +	u16 send_seq, resp_seq;
> +	int rc;
> +
> +	if (msg == NULL || msg->resp == NULL)
> +		return;
> +
> +	rc = p9_sbe_msg_receive(chip_id, msg->resp);
> +	if (rc != OPAL_SUCCESS) {
> +		prlog(PR_ERR, "Failed to read response message "
> +		      "[chip id = %x]\n", chip_id);
> +		p9_sbe_set_primary_rc(msg->resp, SBE_STATUS_PRI_GENERIC_ERR);
> +		return;
> +	}
> +
> +	/* Validate sequence number */
> +	send_seq = (msg->reg[0] >> 16) & 0xffff;
> +	resp_seq = (msg->resp->reg[0] >> 16) & 0xffff;
> +	if (send_seq != resp_seq) {
> +		/*
> +		 * XXX Handle SBE R/R.
> +		 *     Lets send sequence error to caller until SBE reset works.
> +		 */
> +		prlog(PR_ERR, "Invalid sequence id [chip id = %x]\n", chip_id);
> +		p9_sbe_set_primary_rc(msg->resp, SBE_STATUS_PRI_SEQ_ERR);
> +		return;
> +	}
> +}
> +
> +static void __p9_sbe_interrupt(struct p9_sbe *sbe)
> +{
> +	bool has_response = false;
> +	int rc;
> +	u64 data = 0, val;
> +	struct p9_sbe_msg *msg = NULL;
> +
>  	/* Read doorbell register */
> -	rc = xscom_read(chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
> +	rc = xscom_read(sbe->chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
>  	if (rc) {
>  		prlog(PR_ERR, "Failed to read SBE to Host doorbell register "
> -		      "[chip id = %x]\n", chip_id);
> -		goto clr_interrupt;
> +		      "[chip id = %x]\n", sbe->chip_id);
> +		p9_sbe_reg_dump(sbe->chip_id);
> +		return;
>  	}
>  
> -	/* SBE passtrhough command, call prd handler */
> -	if (data & SBE_HOST_PASSTHROUGH) {
> -		prd_sbe_passthrough(chip_id);
> +	/* Called from poller path, nothing to process */
> +	if (!data)
> +		return;
> +
> +	/* Read SBE response before clearing doorbell register */
> +	if (data & SBE_HOST_RESPONSE_WAITING) {
> +		if (!list_empty(&sbe->msg_list)) {
> +			msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
> +			p9_sbe_handle_response(sbe->chip_id, msg);
> +			msg->state = sbe_msg_done;
> +			has_response = true;
> +		}
> +
> +		/* Reset SBE MBOX state */
> +		p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
>  	}

Why do you do that one before the doorbell bit clearing ?

Also you can hit other cases further down such as the "return"
in the xscom_write error case or the SBE_HOST_RESET which will
make you "lose" the content of "msg" and overwrite its state.

You may want to use a separate variable:

struct p9_sbe_msg *resp.

Also you have another possible issue. If you send a message that takes
a response, and you get both the ack and the resp bits, you'll end
up doing the above, and *then* hit send_complete, is that what you
really want ?

You should also double check when you get the response bit that
the message at the head of the list is indeed expecting a response.

If you don't want to worry too much about the lock dropping, what
you can do is a loop, clearing only the bit you processed and reading
the doorbell again, though that's a bit slower.


> -clr_interrupt:
> -	/* Clears all the bits */
> -	rc = xscom_write(chip_id, PSU_HOST_DOORBELL_REG_AND,
> -			 SBE_HOST_RESPONSE_CLEAR);
> +	/* Clear doorbell register */
> +	val = SBE_HOST_RESPONSE_MASK & ~data;
> +	rc = xscom_write(sbe->chip_id, PSU_HOST_DOORBELL_REG_AND, val);
>  	if (rc) {
>  		prlog(PR_ERR, "Failed to clear SBE to Host doorbell "
> -		      "register [chip id = %x]\n", chip_id);
> +		      "interrupt [chip id = %x]\n", sbe->chip_id);
> +		return;
>  	}
> +
> +	/* SBE came back from reset */
> +	if (data & SBE_HOST_RESET) {
> +		prlog(PR_NOTICE, "Back from reset [chip id = %x]\n", sbe->chip_id);
> +		/* Reset SBE MBOX state */
> +		p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
> +
> +		/* Reset message state */
> +		if (!list_empty(&sbe->msg_list)) {
> +			msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
> +			msg->state = sbe_msg_queued;
> +		}
> +		goto next_msg;
> +	}
> +
> +	/* SBE passthrough command, call prd handler */
> +	if (data & SBE_HOST_PASSTHROUGH) {
> +		prd_sbe_passthrough(sbe->chip_id);
> +	}
> +
> +	/* Got message ack from SBE */
> +	if (data & SBE_HOST_MSG_READ) {
> +		p9_sbe_send_complete(sbe);
> +	}
> +
> +	/* Got response */
> +	if (has_response) {
> +		p9_sbe_msg_complete(sbe, msg);
> +	}
> +
> +next_msg:
> +	p9_sbe_process_queue(sbe);
> +}
> +
> +void p9_sbe_interrupt(uint32_t chip_id)
> +{
> +	struct proc_chip *chip;
> +	struct p9_sbe *sbe;
> +
> +	chip = get_chip(chip_id);
> +	if (chip == NULL || chip->sbe == NULL)
> +		return;
> +
> +	sbe = chip->sbe;
> +	lock(&sbe->lock);
> +	__p9_sbe_interrupt(sbe);
> +	unlock(&sbe->lock);
> +}
> +
> +static void p9_sbe_timeout_poll_one(struct p9_sbe *sbe)
> +{
> +	struct p9_sbe_msg *msg;
> +
> +	if (list_empty_nocheck(&sbe->msg_list))
> +		return;
> +
> +	lock(&sbe->lock);
> +
> +	/*
> +	 * For some reason OPAL didn't sent message to SBE.
> +	 * Lets try to send message again.
> +	 */
> +	if (!p9_sbe_mbox_busy(sbe)) {
> +		p9_sbe_process_queue(sbe);
> +		goto out;
> +	}
> +
> +	/*
> +	 * In some cases there will be a delay in calling OPAL interrupt
> +	 * handler routine (opal_handle_interrupt). In such cases its
> +	 * possible that SBE has responded, but OPAL didn't act on that.
> +	 * Hence check for SBE response.
> +	 */
> +	__p9_sbe_interrupt(sbe);
> +
> +	if (list_empty(&sbe->msg_list))
> +		goto out;
> +
> +	msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
> +	if (tb_compare(mftb(), msg->timeout) != TB_AAFTERB)
> +		goto out;
> +
> +	/* Message timeout */
> +	prlog(PR_ERR, "Message timeout [chip id = %x], cmd = %llx, "
> +	      "subcmd = %llx\n", sbe->chip_id,
> +	      (msg->reg[0] >> 8) & 0xff, msg->reg[0] & 0xff);
> +	p9_sbe_reg_dump(sbe->chip_id);
> +	if (msg->resp) {
> +		p9_sbe_set_primary_rc(msg->resp,
> +				      SBE_STATUS_PRI_GENERIC_ERR);
> +	}
> +
> +	msg->state = sbe_msg_timeout;
> +	/* XXX Handle SBE R/R. Reset SBE state until SBE R/R works. */
> +	p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
> +	p9_sbe_msg_complete(sbe, msg);
> +	p9_sbe_process_queue(sbe);
> +
> +out:
> +	unlock(&sbe->lock);
> +}
> +
> +static void p9_sbe_timeout_poll(void *user_data __unused)
> +{
> +	struct p9_sbe *sbe;
> +	struct proc_chip *chip;
> +
> +	for_each_chip(chip) {
> +		if (chip->sbe == NULL)
> +			continue;
> +		sbe = chip->sbe;
> +		p9_sbe_timeout_poll_one(sbe);
> +	}
> +}
> +
> +void p9_sbe_init(void)
> +{
> +	struct dt_node *xn;
> +	struct proc_chip *chip;
> +	struct p9_sbe *sbe;
> +
> +	if (proc_gen < proc_gen_p9)
> +		return;
> +
> +	dt_for_each_compatible(dt_root, xn, "ibm,xscom") {
> +		sbe = zalloc(sizeof(struct p9_sbe));
> +		assert(sbe);
> +		sbe->chip_id = dt_get_chip_id(xn);
> +		sbe->cur_seq = 1;
> +		sbe->state = sbe_mbox_idle;
> +		list_head_init(&sbe->msg_list);
> +		init_lock(&sbe->lock);
> +
> +		chip = get_chip(sbe->chip_id);
> +		assert(chip);
> +		chip->sbe = sbe;
> +
> +		if (dt_has_node_property(xn, "primary", NULL)) {
> +			sbe_default_chip_id = sbe->chip_id;
> +			prlog(PR_DEBUG, "Master chip id : %x\n", sbe->chip_id);
> +		}
> +	}
> +
> +	if (sbe_default_chip_id == -1) {
> +		prlog(PR_ERR, "Master chip ID not found.\n");
> +		return;
> +	}
> +
> +	/* Initiate SBE timeout poller */
> +	opal_add_poller(p9_sbe_timeout_poll, NULL);
>  }
> diff --git a/include/chip.h b/include/chip.h
> index 43b5ea55b..059033e27 100644
> --- a/include/chip.h
> +++ b/include/chip.h
> @@ -113,6 +113,7 @@ struct mfsi;
>  struct xive;
>  struct lpcm;
>  struct vas;
> +struct p9_sbe;
>  
>  /* Chip type */
>  enum proc_chip_type {
> @@ -218,6 +219,9 @@ struct proc_chip {
>  
>  	/* location code of this chip */
>  	const uint8_t		*loc_code;
> +
> +	/* Used by hw/sbe-p9.c */
> +	struct p9_sbe		*sbe;
>  };
>  
>  extern uint32_t pir_to_chip_id(uint32_t pir);
> diff --git a/include/sbe-p9.h b/include/sbe-p9.h
> index cca210024..329afff80 100644
> --- a/include/sbe-p9.h
> +++ b/include/sbe-p9.h
> @@ -1,4 +1,4 @@
> -/* Copyright 2017 IBM Corp.
> +/* Copyright 2017-2018 IBM Corp.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -21,14 +21,214 @@
>  #include <ccan/list/list.h>
>  #include <ccan/short_types/short_types.h>
>  
> +/* Worst case command timeout value (90 sec) */
> +#define SBE_CMD_TIMEOUT_MAX			(90 * 1000)
> +
> +/* Primary response status code */
> +#define SBE_STATUS_PRI_SUCCESS			0x00
> +#define SBE_STATUS_PRI_INVALID_CMD		0x01
> +#define SBE_STATUS_PRI_INVALID_DATA		0x02
> +#define SBE_STATUS_PRI_SEQ_ERR			0x03
> +#define SBE_STATUS_PRI_INTERNAL_ERR		0x04
> +#define SBE_STATUS_PRI_UNSECURE_ACCESS		0x05
> +#define SBE_STATUS_PRI_GENERIC_ERR		0xFE
> +
> +/* Secondary response status code */
> +#define SBE_STATUS_SEC_SUCCESS			0x00
> +#define SBE_STATUS_SEC_CMD_CLASS_UNSUPPORTED	0x01
> +#define SBE_STATUS_SEC_CMD_UNSUPPORTED		0x02
> +#define SBE_STATUS_SEC_INV_ADDR			0x03
> +#define SBE_STATUS_SEC_INV_TARGET_TYPE		0x04
> +#define SBE_STATUS_SEC_INV_CHIPLET_ID		0x05
> +#define SBE_STATUS_SEC_TARGET_NOT_PRESENT	0x06
> +#define SBE_STATUS_SEC_TARGET_NOT_FUNC		0x07
> +#define SBE_STATUS_SEC_CMD_NOT_ALLOW		0x08
> +#define SBE_STATUS_SEC_FUNC_NOT_SUPPORTED	0x09
> +#define SBE_STATUS_SEC_GENERIC_ERR		0x0A
> +#define SBE_STATUS_SEC_BLACKLIST_REG		0x0B
> +#define SBE_STATUS_SEC_OS_FAILURE		0x0C
> +#define SBE_STATUS_SEC_MBX_REG_FAILURE		0x0D
> +#define SBE_STATUS_SEC_INSUFFICIENT_DATA	0x0E
> +#define SBE_STATUS_SEC_EXCESS_DATA		0x0F
> +#define SBE_STATUS_SEC_HW_TIMEOUT		0x10
> +#define SBE_STATUS_SEC_PCBPIB_ERR		0x11
> +#define SBE_STATUS_SEC_FIFO_PARITY_ERR		0x12
> +#define SBE_STATUS_SEC_TIMER_EXPIRED		0x13
> +#define SBE_STATUS_SEC_BLACKLISTED_MEM		0x14
> +#define SBE_STATUS_SEC_UNSEC_REGION_NOT_FOUND	0x15
> +#define SBE_STATUS_SEC_UNSEC_REGION_EXCEEDED	0x16
> +#define SBE_STATUS_SEC_UNSEC_REGION_AMEND	0x17
> +#define SBE_STATUS_SEC_INPUT_BUF_OVERFLOW	0x18
> +#define SBE_STATUS_SEC_INVALID_PARAMS		0x19
> +#define SBE_STATUS_SEC_BLACKLISTED_CMD		0x20
> +
> +/* Number of MBOX register on each side */
> +#define NR_HOST_SBE_MBOX_REG		0x04
> +
> +/*
> + * SBE MBOX register address
> + *   Reg 0 - 3 : Host to send command packets to SBE
> + *   Reg 4 - 7 : SBE to send response packets to Host
> + */
> +#define PSU_HOST_SBE_MBOX_REG0		0x000D0050
> +#define PSU_HOST_SBE_MBOX_REG1		0x000D0051
> +#define PSU_HOST_SBE_MBOX_REG2		0x000D0052
> +#define PSU_HOST_SBE_MBOX_REG3		0x000D0053
> +#define PSU_HOST_SBE_MBOX_REG4		0x000D0054
> +#define PSU_HOST_SBE_MBOX_REG5		0x000D0055
> +#define PSU_HOST_SBE_MBOX_REG6		0x000D0056
> +#define PSU_HOST_SBE_MBOX_REG7		0x000D0057
> +#define PSU_SBE_DOORBELL_REG_RW		0x000D0060
> +#define PSU_SBE_DOORBELL_REG_AND	0x000D0061
> +#define PSU_SBE_DOORBELL_REG_OR		0x000D0062
>  #define PSU_HOST_DOORBELL_REG_RW	0x000D0063
>  #define PSU_HOST_DOORBELL_REG_AND	0x000D0064
>  #define PSU_HOST_DOORBELL_REG_OR	0x000D0065
>  
> +/*
> + * Doorbell register to trigger SBE interrupt. Set by OPAL to inform
> + * the SBE about a waiting message in the Host/SBE mailbox registers
> + */
> +#define HOST_SBE_MSG_WAITING		PPC_BIT(0)
> +
> +/*
> + * Doorbell register for host bridge interrupt. Set by the SBE to inform
> + * host about a response message in the Host/SBE mailbox registers
> + */
> +#define SBE_HOST_RESPONSE_WAITING	PPC_BIT(0)
> +#define SBE_HOST_MSG_READ		PPC_BIT(1)
> +#define SBE_HOST_STOP15_EXIT		PPC_BIT(2)
> +#define SBE_HOST_RESET			PPC_BIT(3)
>  #define SBE_HOST_PASSTHROUGH		PPC_BIT(4)
> -#define SBE_HOST_RESPONSE_CLEAR		0x00
> +#define SBE_HOST_TIMER_EXPIRY		PPC_BIT(14)
> +#define SBE_HOST_RESPONSE_MASK		(PPC_BITMASK(0, 4) | SBE_HOST_TIMER_EXPIRY)
> +
> +/* SBE Target Type */
> +#define SBE_TARGET_TYPE_PROC		0x00
> +#define SBE_TARGET_TYPE_EX		0x01
> +#define SBE_TARGET_TYPE_PERV		0x02
> +#define SBE_TARGET_TYPE_MCS		0x03
> +#define SBE_TARGET_TYPE_EQ		0x04
> +#define SBE_TARGET_TYPE_CORE		0x05
> +
> +/* SBE MBOX command class */
> +#define SBE_MCLASS_FIRST		0xD1
> +#define SBE_MCLASS_CORE_STATE		0xD1
> +#define SBE_MCLASS_SCOM			0xD2
> +#define SBE_MCLASS_RING			0xD3
> +#define SBE_MCLASS_TIMER		0xD4
> +#define SBE_MCLASS_MPIPL		0xD5
> +#define SBE_MCLASS_SECURITY		0xD6
> +#define SBE_MCLASS_GENERIC		0xD7
> +#define SBE_MCLASS_LAST			0xD7
> +
> +/*
> + * Commands are provided in xxyy form where:
> + *   - xx : command class
> + *   - yy : command
> + *
> + * Both request and response message uses same seq ID,
> + * command class and command.
> + */
> +#define SBE_CMD_CTRL_DEADMAN_LOOP	0xD101
> +#define SBE_CMD_MULTI_SCOM		0xD201
> +#define SBE_CMD_PUT_RING_FORM_IMAGE	0xD301
> +#define SBE_CMD_CONTROL_TIMER		0xD401
> +#define SBE_CMD_GET_ARCHITECTED_REG	0xD501
> +#define SBE_CMD_CLR_ARCHITECTED_REG	0xD502
> +#define SBE_CMD_SET_UNSEC_MEM_WINDOW	0xD601
> +#define SBE_CMD_GET_SBE_FFDC		0xD701
> +#define SBE_CMD_GET_CAPABILITY		0xD702
> +#define SBE_CMD_READ_SBE_SEEPROM	0xD703
> +#define SBE_CMD_SET_FFDC_ADDR		0xD704
> +#define SBE_CMD_QUIESCE_SBE		0xD705
> +#define SBE_CMD_SET_FABRIC_ID_MAP	0xD706
> +#define SBE_CMD_STASH_MPIPL_CONFIG	0xD707
> +
> +/* SBE MBOX control flags */
> +
> +/* Generic flags */
> +#define SBE_CMD_CTRL_RESP_REQ		0x0100
> +#define SBE_CMD_CTRL_ACK_REQ		0x0200
> +
> +/* Deadman loop */
> +#define CTRL_DEADMAN_LOOP_START		0x0001
> +#define CTRL_DEADMAN_LOOP_STOP		0x0002
> +
> +/* Control timer */
> +#define CONTROL_TIMER_START		0x0001
> +#define CONTROL_TIMER_STOP		0x0002
> +
> +/* SBE message state */
> +enum p9_sbe_msg_state {
> +	sbe_msg_unused = 0,	/* Free */
> +	sbe_msg_queued,		/* Queued to SBE list */
> +	sbe_msg_sent,		/* Sent to SBE */
> +	sbe_msg_wresp,		/* Waiting for response */
> +	sbe_msg_done,		/* Complete */
> +	sbe_msg_timeout,	/* Message timeout */
> +	sbe_msg_error,		/* Failed to send message to SBE */
> +};
> +
> +/* SBE message */
> +struct p9_sbe_msg {
> +	/*
> +	 * Reg[0] :
> +	 *   word0 :
> +	 *     direct cmd  : reserved << 16 | ctrl flag
> +	 *     indirect cmd: mem_addr_size_dword
> +	 *     response    : primary status << 16 | secondary status
> +	 *
> +	 *   word1 : seq id << 16 | cmd class << 8 | cmd
> +	 *
> +	 * WARNING:
> +	 *   - Don't populate reg[0].seq (byte 4,5). This will be populated by
> +	 *     p9_sbe_queue_msg().
> +	 */
> +	u64	reg[4];
> +
> +	/* cmd timout : mftb() + msecs_to_tb(SBE_CMD_TIMEOUT_MAX) */
> +	u64	timeout;
> +
> +	/* Completion function */
> +	void (*complete)(struct p9_sbe_msg *msg);
> +	void *user_data;
> +
> +	/* Current msg state */
> +	enum p9_sbe_msg_state	state;
> +
> +	/* Set if the message expects a response */
> +	bool			response;
> +
> +	/* Response will be filled by driver when response received */
> +	struct p9_sbe_msg	*resp;
> +
> +	/* Internal queuing */
> +	struct list_node	link;
> +} __packed;
> +
> +
> +/* Allocate and populate p9_sbe_msg structure */
> +extern struct p9_sbe_msg *p9_sbe_mkmsg(u16 cmd, u16 ctrl_flag, u64 reg1,
> +				       u64 reg2, u64 reg3) __warn_unused_result;
> +
> +/* Free p9_sbe_msg structure */
> +extern void p9_sbe_freemsg(struct p9_sbe_msg *msg);
> +
> +/* Add new message to sbe queue */
> +extern int p9_sbe_queue_msg(uint32_t chip_id, struct p9_sbe_msg *msg,
> +		void (*comp)(struct p9_sbe_msg *msg)) __warn_unused_result;
> +
> +/* Synchronously send message to SBE */
> +extern int p9_sbe_sync_msg(u32 chip_id, struct p9_sbe_msg *msg, bool autofree);
> +
> +/* Remove message from SBE queue, it will not remove inflight message */
> +extern int p9_sbe_cancelmsg(u32 chip_id, struct p9_sbe_msg *msg);
> +
> +/* Initialize the SBE mailbox driver */
> +extern void p9_sbe_init(void);
>  
>  /* SBE interrupt */
> -extern void sbe_interrupt(uint32_t chip_id);
> +extern void p9_sbe_interrupt(uint32_t chip_id);
>  
>  #endif	/* __SBE_P9_H */

On 04/17/2018 09:38 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2018-04-16 at 17:17 +0530, Vasant Hegde wrote:
>>
>> +static inline bool p9_sbe_mbox_busy(struct p9_sbe *sbe)
>> +{
>> +	return sbe->state != sbe_mbox_idle ? true : false;
>> +}
> 
> What's wrong with just return sbe->state != sbe_mbox_idle ?
> 

That should work fine. Will fix.

>> +static inline void p9_sbe_mbox_update_state(struct p9_sbe *sbe,
>> +					    enum p9_sbe_mbox_state state)
>> +{
>> +	sbe->state = state;
>> +}
> 
> What's the point of the above wrapper ? I don't find it makes
> anything clearer...

Ok. Will remove.


.../...

>> +}
>> +
>> +/* WARNING: This will drop sbe->lock */
>> +static void p9_sbe_send_complete(struct p9_sbe *sbe)
>> +{
>> +	struct p9_sbe_msg *msg;
>> +
>> +	if (list_empty(&sbe->msg_list))
>>   		return;
>>   
>> +	msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> +	/* Need response */
>> +	if (msg->response) {
>> +		msg->state = sbe_msg_wresp;
>> +	} else {
>> +		msg->state = sbe_msg_done;
>> +		p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
>> +		p9_sbe_msg_complete(sbe, msg);
>> +	}
>> +}
> 
> Subtle race: if somebody is polling on msg->state waiting for it to be
> done without holding the SBE lock (which could happen), then there's a
> risk that the message gets freed right after msg->state is updated.
> 

Oh yes. If someone is polling without lock yes we have trouble.
Will fix.

> msg->state should be set to "done" *inside* p9_sbe_msg_complete, and
> the latter should make sure it snapshots msg->complete before it
> changes msg->state (with a nice fat sync in between). It also needs to
> take it out of the list before updating the state as well.

Yeah. I can do something like bleow in p9_sbe_msg_complete()

static void p9_sbe_msg_complete(struct p9_sbe *sbe, struct p9_sbe_msg *msg)
{
	void (*comp)(struct p9_sbe_msg *msg);

	comp = msg->complete;
	list_del(&msg);

	msg->state = sbe_msg_done;
	if (comp) {
		unlock(&sbe->lock);
		comp(msg);
		lock(&sbe->lock);
	}
}

But now we always endup setting msg->state = done. How do we pass error state to 
caller?
May be allocate resp structure for all messages?

> 
>> +/* WARNING: This will drop sbe->lock */
>> +static void p9_sbe_process_queue(struct p9_sbe *sbe)
>> +{
>> +	int rc, retry_cnt = 0;
>> +	struct p9_sbe_msg *msg = NULL;
>> +
>> +	if (p9_sbe_mbox_busy(sbe))
>> +		return;
>> +
>> +	while (!list_empty(&sbe->msg_list)) {
>> +		msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> +		/* Send message */
>> +		rc = p9_sbe_msg_send(sbe->chip_id, msg);
>> +		if (rc == OPAL_SUCCESS)
>> +			break;
>> +
>> +		prlog(PR_ERR, "Failed to send message to SBE [chip id = %x]\n",
>> +		      sbe->chip_id);
>> +		if (msg->resp) {
>> +			p9_sbe_set_primary_rc(msg->resp,
>> +					      SBE_STATUS_PRI_GENERIC_ERR);
>> +		}
>> +		msg->state = sbe_msg_error;
>> +		p9_sbe_msg_complete(sbe, msg);
>> +
>> +		/*
>> +		 * Repeatedly failed to send message to SBE. Lets stop
>> +		 * sending message.
>> +		 */
>> +		if (retry_cnt++ >= 3) {
>> +			prlog(PR_ERR, "Temporarily stopped sending "
>> +			      "message to SBE\n");
>> +			return;
>> +		}
>> +	}
>> +
>> +	if (list_empty(&sbe->msg_list))
>> +		return;
>> +
>> +	prlog(PR_TRACE, "Message queued [chip id = 0x%x]:\n\t Reg0 : %016llx"
>> +	      "\n\t Reg1 : %016llx\n\t Reg2 : %016llx\n\t Reg3 : %016llx\n",
>> +	      sbe->chip_id, msg->reg[0], msg->reg[1], msg->reg[2], msg->reg[3]);
>> +
>> +	msg->timeout = mftb() + msecs_to_tb(SBE_CMD_TIMEOUT_MAX);
>> +	p9_sbe_mbox_update_state(sbe, sbe_mbox_send);
>> +	msg->state = sbe_msg_sent;
> 
> I would personally have the 2 above inside p9_sbe_msg_send(), any
> reason why not ?

I don't have strong reason for that. I just wanted sbe_msg_send to just send 
message without dealing with message state etc (like I do in sbe_msg_recv). I 
can move above two statement inside msg_send().


.../...

>> +
>> +static void __p9_sbe_interrupt(struct p9_sbe *sbe)
>> +{
>> +	bool has_response = false;
>> +	int rc;
>> +	u64 data = 0, val;
>> +	struct p9_sbe_msg *msg = NULL;
>> +
>>   	/* Read doorbell register */
>> -	rc = xscom_read(chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
>> +	rc = xscom_read(sbe->chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
>>   	if (rc) {
>>   		prlog(PR_ERR, "Failed to read SBE to Host doorbell register "
>> -		      "[chip id = %x]\n", chip_id);
>> -		goto clr_interrupt;
>> +		      "[chip id = %x]\n", sbe->chip_id);
>> +		p9_sbe_reg_dump(sbe->chip_id);
>> +		return;
>>   	}
>>   
>> -	/* SBE passtrhough command, call prd handler */
>> -	if (data & SBE_HOST_PASSTHROUGH) {
>> -		prd_sbe_passthrough(chip_id);
>> +	/* Called from poller path, nothing to process */
>> +	if (!data)
>> +		return;
>> +
>> +	/* Read SBE response before clearing doorbell register */
>> +	if (data & SBE_HOST_RESPONSE_WAITING) {
>> +		if (!list_empty(&sbe->msg_list)) {
>> +			msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>> +			p9_sbe_handle_response(sbe->chip_id, msg);
>> +			msg->state = sbe_msg_done;
>> +			has_response = true;
>> +		}
>> +
>> +		/* Reset SBE MBOX state */
>> +		p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
>>   	}
> 
> Why do you do that one before the doorbell bit clearing ?
> 
> Also you can hit other cases further down such as the "return"
> in the xscom_write error case or the SBE_HOST_RESET which will
> make you "lose" the content of "msg" and overwrite its state.

Yeah. May be I will have separate local "resp" message, copy response to that 
and then if everything is fine (like writing xscom , handling reset), I will 
call msg_complete() with this new resp (as in I will copy "resp" to msg->resp).

> 
> You may want to use a separate variable:
>
> struct p9_sbe_msg *resp.
>

Yeah. Separate message is cleaner.


> Also you have another possible issue. If you send a message that takes
> a response, and you get both the ack and the resp bits, you'll end
> up doing the above, and *then* hit send_complete, is that what you
> really want ?

Actually this is fine. I'm processing like below
	Holding lock
	process ACK -> if message expects response then send_complete just sets message 
state
					and returns
	process response -> At this stage if we have response then it calls msg->complete()


> 
> You should also double check when you get the response bit that
> the message at the head of the list is indeed expecting a response.

SBE message is serialized. Until we get response to first message we are not 
sending second one. So top of the message is the one which is waiting for 
response. Worst case we may
get both ACK and response bit .. which we are handling.

> 
> If you don't want to worry too much about the lock dropping, what
> you can do is a loop, clearing only the bit you processed and reading
> the doorbell again, though that's a bit slower.

I think current code is fine with above modification. We should be able to 
handle with single xscom_read.


Thanks!
-Vasant

On Tue, 2018-04-17 at 11:34 +0530, Vasant Hegde wrote:
> On 04/17/2018 09:38 AM, Benjamin Herrenschmidt wrote:
> > On Mon, 2018-04-16 at 17:17 +0530, Vasant Hegde wrote:
> > > 
> > > +static inline bool p9_sbe_mbox_busy(struct p9_sbe *sbe)
> > > +{
> > > +	return sbe->state != sbe_mbox_idle ? true : false;
> > > +}
> > 
> > What's wrong with just return sbe->state != sbe_mbox_idle ?
> > 
> 
> That should work fine. Will fix.
> 
> > > +static inline void p9_sbe_mbox_update_state(struct p9_sbe *sbe,
> > > +					    enum p9_sbe_mbox_state state)
> > > +{
> > > +	sbe->state = state;
> > > +}
> > 
> > What's the point of the above wrapper ? I don't find it makes
> > anything clearer...
> 
> Ok. Will remove.
> 
> 
> .../...
> 
> > > +}
> > > +
> > > +/* WARNING: This will drop sbe->lock */
> > > +static void p9_sbe_send_complete(struct p9_sbe *sbe)
> > > +{
> > > +	struct p9_sbe_msg *msg;
> > > +
> > > +	if (list_empty(&sbe->msg_list))
> > >   		return;
> > >   
> > > +	msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
> > > +	/* Need response */
> > > +	if (msg->response) {
> > > +		msg->state = sbe_msg_wresp;
> > > +	} else {
> > > +		msg->state = sbe_msg_done;
> > > +		p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
> > > +		p9_sbe_msg_complete(sbe, msg);
> > > +	}
> > > +}
> > 
> > Subtle race: if somebody is polling on msg->state waiting for it to be
> > done without holding the SBE lock (which could happen), then there's a
> > risk that the message gets freed right after msg->state is updated.
> > 
> 
> Oh yes. If someone is polling without lock yes we have trouble.
> Will fix.
> 
> > msg->state should be set to "done" *inside* p9_sbe_msg_complete, and
> > the latter should make sure it snapshots msg->complete before it
> > changes msg->state (with a nice fat sync in between). It also needs to
> > take it out of the list before updating the state as well.
> 
> Yeah. I can do something like bleow in p9_sbe_msg_complete()
> 
> static void p9_sbe_msg_complete(struct p9_sbe *sbe, struct p9_sbe_msg *msg)
> {
> 	void (*comp)(struct p9_sbe_msg *msg);
> 
> 	comp = msg->complete;
> 	list_del(&msg);

	sync(); <---- important

> 	msg->state = sbe_msg_done;
> 	if (comp) {
> 		unlock(&sbe->lock);
> 		comp(msg);
> 		lock(&sbe->lock);
> 	}
> }
> 
> But now we always endup setting msg->state = done. How do we pass error state to 
> caller?

Pass it as an argument to msg_complete ?

> May be allocate resp structure for all messages?
> 
> > 
> > > +/* WARNING: This will drop sbe->lock */
> > > +static void p9_sbe_process_queue(struct p9_sbe *sbe)
> > > +{
> > > +	int rc, retry_cnt = 0;
> > > +	struct p9_sbe_msg *msg = NULL;
> > > +
> > > +	if (p9_sbe_mbox_busy(sbe))
> > > +		return;
> > > +
> > > +	while (!list_empty(&sbe->msg_list)) {
> > > +		msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
> > > +		/* Send message */
> > > +		rc = p9_sbe_msg_send(sbe->chip_id, msg);
> > > +		if (rc == OPAL_SUCCESS)
> > > +			break;
> > > +
> > > +		prlog(PR_ERR, "Failed to send message to SBE [chip id = %x]\n",
> > > +		      sbe->chip_id);
> > > +		if (msg->resp) {
> > > +			p9_sbe_set_primary_rc(msg->resp,
> > > +					      SBE_STATUS_PRI_GENERIC_ERR);
> > > +		}
> > > +		msg->state = sbe_msg_error;
> > > +		p9_sbe_msg_complete(sbe, msg);
> > > +
> > > +		/*
> > > +		 * Repeatedly failed to send message to SBE. Lets stop
> > > +		 * sending message.
> > > +		 */
> > > +		if (retry_cnt++ >= 3) {
> > > +			prlog(PR_ERR, "Temporarily stopped sending "
> > > +			      "message to SBE\n");
> > > +			return;
> > > +		}
> > > +	}
> > > +
> > > +	if (list_empty(&sbe->msg_list))
> > > +		return;
> > > +
> > > +	prlog(PR_TRACE, "Message queued [chip id = 0x%x]:\n\t Reg0 : %016llx"
> > > +	      "\n\t Reg1 : %016llx\n\t Reg2 : %016llx\n\t Reg3 : %016llx\n",
> > > +	      sbe->chip_id, msg->reg[0], msg->reg[1], msg->reg[2], msg->reg[3]);
> > > +
> > > +	msg->timeout = mftb() + msecs_to_tb(SBE_CMD_TIMEOUT_MAX);
> > > +	p9_sbe_mbox_update_state(sbe, sbe_mbox_send);
> > > +	msg->state = sbe_msg_sent;
> > 
> > I would personally have the 2 above inside p9_sbe_msg_send(), any
> > reason why not ?
> 
> I don't have strong reason for that. I just wanted sbe_msg_send to just send 
> message without dealing with message state etc (like I do in sbe_msg_recv). I 
> can move above two statement inside msg_send().

it's not that much better to have that stuff opencoded in the general
queue processing either ... not biggie tho.
> 
> 
> .../...
> 
> > > +
> > > +static void __p9_sbe_interrupt(struct p9_sbe *sbe)
> > > +{
> > > +	bool has_response = false;
> > > +	int rc;
> > > +	u64 data = 0, val;
> > > +	struct p9_sbe_msg *msg = NULL;
> > > +
> > >   	/* Read doorbell register */
> > > -	rc = xscom_read(chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
> > > +	rc = xscom_read(sbe->chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
> > >   	if (rc) {
> > >   		prlog(PR_ERR, "Failed to read SBE to Host doorbell register "
> > > -		      "[chip id = %x]\n", chip_id);
> > > -		goto clr_interrupt;
> > > +		      "[chip id = %x]\n", sbe->chip_id);
> > > +		p9_sbe_reg_dump(sbe->chip_id);
> > > +		return;
> > >   	}
> > >   
> > > -	/* SBE passtrhough command, call prd handler */
> > > -	if (data & SBE_HOST_PASSTHROUGH) {
> > > -		prd_sbe_passthrough(chip_id);
> > > +	/* Called from poller path, nothing to process */
> > > +	if (!data)
> > > +		return;
> > > +
> > > +	/* Read SBE response before clearing doorbell register */
> > > +	if (data & SBE_HOST_RESPONSE_WAITING) {
> > > +		if (!list_empty(&sbe->msg_list)) {
> > > +			msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
> > > +			p9_sbe_handle_response(sbe->chip_id, msg);
> > > +			msg->state = sbe_msg_done;
> > > +			has_response = true;
> > > +		}
> > > +
> > > +		/* Reset SBE MBOX state */
> > > +		p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
> > >   	}
> > 
> > Why do you do that one before the doorbell bit clearing ?
> > 
> > Also you can hit other cases further down such as the "return"
> > in the xscom_write error case or the SBE_HOST_RESET which will
> > make you "lose" the content of "msg" and overwrite its state.
> 
> Yeah. May be I will have separate local "resp" message, copy response to that 
> and then if everything is fine (like writing xscom , handling reset), I will 
> call msg_complete() with this new resp (as in I will copy "resp" to msg->resp).

Why copy ? You don't need to copy anything ...

> > 
> > You may want to use a separate variable:
> > 
> > struct p9_sbe_msg *resp.
> > 
> 
> Yeah. Separate message is cleaner.

Separate variable, same message... or not. Up to you.
> 
> > Also you have another possible issue. If you send a message that takes
> > a response, and you get both the ack and the resp bits, you'll end
> > up doing the above, and *then* hit send_complete, is that what you
> > really want ?
> 
> Actually this is fine. I'm processing like below
> 	Holding lock
> 	process ACK -> if message expects response then send_complete just sets message 
> state
> 					and returns
> 	process response -> At this stage if we have response then it calls msg->complete()
> 
> 
> > 
> > You should also double check when you get the response bit that
> > the message at the head of the list is indeed expecting a response.
> 
> SBE message is serialized. Until we get response to first message we are not 
> sending second one. So top of the message is the one which is waiting for 
> response. Worst case we may
> get both ACK and response bit .. which we are handling.

You should still look at ACK first, before you look at response.
> > 
> > If you don't want to worry too much about the lock dropping, what
> > you can do is a loop, clearing only the bit you processed and reading
> > the doorbell again, though that's a bit slower.
> 
> I think current code is fine with above modification. We should be able to 
> handle with single xscom_read.

I'll review again when you do the changes and see if I can convince
myself that it is.

If you look at the FSP code, when I complete a message via
fsp_complete_send() in __fsp_poll() I do re-fetch the interrupt causes
because the lock drop means somebody else could have called poll in the
meantime.

That's probably the best way for you too.

Rather than clear all the bits, do something like

again:
	bits = read_bits();

	if (bit & bit_one) {
		clear_bit(one);
		do_actions_for_bit_1();
		if (completed_message)
			goto again;
	}

	if (bit & bit_two)
	etc...

> Thanks!
> -Vasant

On 04/17/2018 12:10 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2018-04-17 at 11:34 +0530, Vasant Hegde wrote:
>> On 04/17/2018 09:38 AM, Benjamin Herrenschmidt wrote:
>>> On Mon, 2018-04-16 at 17:17 +0530, Vasant Hegde wrote:

.../...

>>
>> Oh yes. If someone is polling without lock yes we have trouble.
>> Will fix.
>>
>>> msg->state should be set to "done" *inside* p9_sbe_msg_complete, and
>>> the latter should make sure it snapshots msg->complete before it
>>> changes msg->state (with a nice fat sync in between). It also needs to
>>> take it out of the list before updating the state as well.
>>
>> Yeah. I can do something like bleow in p9_sbe_msg_complete()
>>
>> static void p9_sbe_msg_complete(struct p9_sbe *sbe, struct p9_sbe_msg *msg)
>> {
>> 	void (*comp)(struct p9_sbe_msg *msg);
>>
>> 	comp = msg->complete;
>> 	list_del(&msg);
> 
> 	sync(); <---- important

Yes. Fixed.

> 
>> 	msg->state = sbe_msg_done;
>> 	if (comp) {
>> 		unlock(&sbe->lock);
>> 		comp(msg);
>> 		lock(&sbe->lock);
>> 	}
>> }
>>
>> But now we always endup setting msg->state = done. How do we pass error state to
>> caller?
> 
> Pass it as an argument to msg_complete ?

Yes. That should work fine.

> 
>> May be allocate resp structure for all messages?
>>
>>>
>>>> +/* WARNING: This will drop sbe->lock */
>>>> +static void p9_sbe_process_queue(struct p9_sbe *sbe)
>>>> +{
>>>> +	int rc, retry_cnt = 0;
>>>> +	struct p9_sbe_msg *msg = NULL;
>>>> +
>>>> +	if (p9_sbe_mbox_busy(sbe))
>>>> +		return;
>>>> +
>>>> +	while (!list_empty(&sbe->msg_list)) {
>>>> +		msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>>>> +		/* Send message */
>>>> +		rc = p9_sbe_msg_send(sbe->chip_id, msg);
>>>> +		if (rc == OPAL_SUCCESS)
>>>> +			break;
>>>> +
>>>> +		prlog(PR_ERR, "Failed to send message to SBE [chip id = %x]\n",
>>>> +		      sbe->chip_id);
>>>> +		if (msg->resp) {
>>>> +			p9_sbe_set_primary_rc(msg->resp,
>>>> +					      SBE_STATUS_PRI_GENERIC_ERR);
>>>> +		}
>>>> +		msg->state = sbe_msg_error;
>>>> +		p9_sbe_msg_complete(sbe, msg);
>>>> +
>>>> +		/*
>>>> +		 * Repeatedly failed to send message to SBE. Lets stop
>>>> +		 * sending message.
>>>> +		 */
>>>> +		if (retry_cnt++ >= 3) {
>>>> +			prlog(PR_ERR, "Temporarily stopped sending "
>>>> +			      "message to SBE\n");
>>>> +			return;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (list_empty(&sbe->msg_list))
>>>> +		return;
>>>> +
>>>> +	prlog(PR_TRACE, "Message queued [chip id = 0x%x]:\n\t Reg0 : %016llx"
>>>> +	      "\n\t Reg1 : %016llx\n\t Reg2 : %016llx\n\t Reg3 : %016llx\n",
>>>> +	      sbe->chip_id, msg->reg[0], msg->reg[1], msg->reg[2], msg->reg[3]);
>>>> +
>>>> +	msg->timeout = mftb() + msecs_to_tb(SBE_CMD_TIMEOUT_MAX);
>>>> +	p9_sbe_mbox_update_state(sbe, sbe_mbox_send);
>>>> +	msg->state = sbe_msg_sent;
>>>
>>> I would personally have the 2 above inside p9_sbe_msg_send(), any
>>> reason why not ?
>>
>> I don't have strong reason for that. I just wanted sbe_msg_send to just send
>> message without dealing with message state etc (like I do in sbe_msg_recv). I
>> can move above two statement inside msg_send().
> 
> it's not that much better to have that stuff opencoded in the general
> queue processing either ... not biggie tho.

Actually if I move above hunk to send_msg() I can remove list_empty() check as well.

>>
>>
>> .../...
>>
>>>> +
>>>> +static void __p9_sbe_interrupt(struct p9_sbe *sbe)
>>>> +{
>>>> +	bool has_response = false;
>>>> +	int rc;
>>>> +	u64 data = 0, val;
>>>> +	struct p9_sbe_msg *msg = NULL;
>>>> +
>>>>    	/* Read doorbell register */
>>>> -	rc = xscom_read(chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
>>>> +	rc = xscom_read(sbe->chip_id, PSU_HOST_DOORBELL_REG_RW, &data);
>>>>    	if (rc) {
>>>>    		prlog(PR_ERR, "Failed to read SBE to Host doorbell register "
>>>> -		      "[chip id = %x]\n", chip_id);
>>>> -		goto clr_interrupt;
>>>> +		      "[chip id = %x]\n", sbe->chip_id);
>>>> +		p9_sbe_reg_dump(sbe->chip_id);
>>>> +		return;
>>>>    	}
>>>>    
>>>> -	/* SBE passtrhough command, call prd handler */
>>>> -	if (data & SBE_HOST_PASSTHROUGH) {
>>>> -		prd_sbe_passthrough(chip_id);
>>>> +	/* Called from poller path, nothing to process */
>>>> +	if (!data)
>>>> +		return;
>>>> +
>>>> +	/* Read SBE response before clearing doorbell register */
>>>> +	if (data & SBE_HOST_RESPONSE_WAITING) {
>>>> +		if (!list_empty(&sbe->msg_list)) {
>>>> +			msg = list_top(&sbe->msg_list, struct p9_sbe_msg, link);
>>>> +			p9_sbe_handle_response(sbe->chip_id, msg);
>>>> +			msg->state = sbe_msg_done;
>>>> +			has_response = true;
>>>> +		}
>>>> +
>>>> +		/* Reset SBE MBOX state */
>>>> +		p9_sbe_mbox_update_state(sbe, sbe_mbox_idle);
>>>>    	}
>>>
>>> Why do you do that one before the doorbell bit clearing ?
>>>
>>> Also you can hit other cases further down such as the "return"
>>> in the xscom_write error case or the SBE_HOST_RESET which will
>>> make you "lose" the content of "msg" and overwrite its state.
>>
>> Yeah. May be I will have separate local "resp" message, copy response to that
>> and then if everything is fine (like writing xscom , handling reset), I will
>> call msg_complete() with this new resp (as in I will copy "resp" to msg->resp).
> 
> Why copy ? You don't need to copy anything ...
> 
>>>
>>> You may want to use a separate variable:
>>>
>>> struct p9_sbe_msg *resp.
>>>
>>
>> Yeah. Separate message is cleaner.
> 
> Separate variable, same message... or not. Up to you.
>>
>>> Also you have another possible issue. If you send a message that takes
>>> a response, and you get both the ack and the resp bits, you'll end
>>> up doing the above, and *then* hit send_complete, is that what you
>>> really want ?
>>
>> Actually this is fine. I'm processing like below
>> 	Holding lock
>> 	process ACK -> if message expects response then send_complete just sets message
>> state
>> 					and returns
>> 	process response -> At this stage if we have response then it calls msg->complete()
>>
>>
>>>
>>> You should also double check when you get the response bit that
>>> the message at the head of the list is indeed expecting a response.
>>
>> SBE message is serialized. Until we get response to first message we are not
>> sending second one. So top of the message is the one which is waiting for
>> response. Worst case we may
>> get both ACK and response bit .. which we are handling.
> 
> You should still look at ACK first, before you look at response.
>>>
>>> If you don't want to worry too much about the lock dropping, what
>>> you can do is a loop, clearing only the bit you processed and reading
>>> the doorbell again, though that's a bit slower.
>>
>> I think current code is fine with above modification. We should be able to
>> handle with single xscom_read.
> 
> I'll review again when you do the changes and see if I can convince
> myself that it is.

Sure.

-Vasant