[Skiboot] [PATCH V2 2/4] hw/lpc-mbox: Use message registers for interrupts

Andrew Jeffery andrew at aj.id.au
Thu May 25 16:28:21 AEST 2017


On Wed, 2017-05-24 at 16:37 +1000, Suraj Jitindar Singh wrote:
> > From: Cyril Bur <cyril.bur at au1.ibm.com>
> 
> Currently the BMC raises the interrupt using the BMC control register.
> It does so on all accesses to the 16 'data' registers meaning that when
> the BMC only wants to set the ATTN (on which we have interrupts enabled)
> bit we will also get a control register based interrupt.
> 
> The solution here is to mask that interrupt permanantly and enable
> interrupts on the protocol defined 'response' data byte.
> 
> > Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh at gmail.com>
> 
> ---
> 
> V1 -> V2:
> 
> - Reword comments
> ---
>  hw/lpc-mbox.c      | 61 +++++++++++++++++++++++++++++++++++++++++-------------
>  include/lpc-mbox.h |  4 ++--
>  2 files changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c
> index 21e6eee..c296a8a 100644
> --- a/hw/lpc-mbox.c
> +++ b/hw/lpc-mbox.c
> @@ -35,12 +35,14 @@
>  
>  #define MBOX_FLAG_REG 0x0f
>  #define MBOX_STATUS_0 0x10
> -#define   MBOX_STATUS_ATTN (1 << 7)
>  #define MBOX_STATUS_1 0x11
> +#define   MBOX_STATUS_1_ATTN (1 << 7)
> +#define   MBOX_STATUS_1_RESP (1 << 5)
>  #define MBOX_BMC_CTRL 0x12
>  #define   MBOX_CTRL_INT_STATUS (1 << 7)
>  #define   MBOX_CTRL_INT_MASK (1 << 1)
> -#define   MBOX_CTRL_INT_SEND (1 << 0)
> +#define   MBOX_CTRL_INT_PING (1 << 0)
> +#define   MBOX_CTRL_INT_SEND (MBOX_CTRL_INT_PING | MBOX_CTRL_INT_MASK)
>  #define MBOX_HOST_CTRL 0x13
>  #define MBOX_BMC_INT_EN_0 0x14
>  #define MBOX_BMC_INT_EN_1 0x15
> @@ -85,7 +87,7 @@ static void bmc_mbox_recv_message(struct bmc_mbox_msg *msg)
> >  	uint8_t *msg_data = (uint8_t *)msg;
> >  	int i;
>  
> > -	for (i = 0; i < BMC_MBOX_DATA_REGS; i++)
> > +	for (i = 0; i < BMC_MBOX_READ_REGS; i++)
> >  		msg_data[i] = bmc_mbox_inb(i);
>  }
>  
> @@ -98,9 +100,16 @@ static void bmc_mbox_send_message(struct bmc_mbox_msg *msg)
> >  	if (!lpc_ok())
> >  		/* We're going to have to handle this better */
> >  		prlog(PR_ERR, "LPC isn't ok\n");
> > -	for (i = 0; i < BMC_MBOX_DATA_REGS; i++)
> +
> > +	for (i = 0; i < BMC_MBOX_WRITE_REGS; i++)
> >  		bmc_mbox_outb(msg_data[i], i);
>  
> > +	/*
> > +	 * Don't touch the response byte - it's setup to generate an interrupt
> > +	 * to the host (us) when written to, or the host status reg - we don't
> > +	 * currently use it, or the BMC status reg - we're not allowed to.
> > +	 */
> +
> >  	/* Ping */
> >  	prlog(PR_DEBUG, "Sending BMC interrupt\n");
> >  	bmc_mbox_outb(MBOX_CTRL_INT_SEND, MBOX_HOST_CTRL);
> @@ -136,10 +145,14 @@ static void mbox_poll(struct timer *t __unused, void *data __unused,
>  {
> >  	struct bmc_mbox_msg *msg;
>  
> > -	/* This is a 'registered' the message you just sent me */
> > -	if (bmc_mbox_inb(MBOX_HOST_CTRL) & MBOX_CTRL_INT_STATUS) {
> > +	/*
> > +	 * This status bit being high means that someone touched the
> > +	 * response byte (byte 13).
> > +	 * There is probably a response for the previously sent commant
> > +	 */
> > +	if (bmc_mbox_inb(MBOX_STATUS_1) & MBOX_STATUS_1_RESP) {
> >  		/* W1C on that reg */
> > -		bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL);
> > +		bmc_mbox_outb(MBOX_STATUS_1_RESP, MBOX_STATUS_1);
>  
> >  		prlog(PR_INSANE, "Got a regular interrupt\n");
> >  		/*
> @@ -148,7 +161,7 @@ static void mbox_poll(struct timer *t __unused, void *data __unused,
> >  		msg = mbox.in_flight;
> >  		if (msg == NULL) {
> >  			prlog(PR_CRIT, "Couldn't find the message!!\n");
> > -			return;
> > +			goto out_response;
> >  		}
> >  		bmc_mbox_recv_message(msg);
> >  		if (mbox.callback)
> @@ -162,19 +175,29 @@ static void mbox_poll(struct timer *t __unused, void *data __unused,
> >  		unlock(&mbox.lock);
> >  	}
>  
> > -	/* This is to indicate that the BMC has information to tell us */
> > -	if (bmc_mbox_inb(MBOX_STATUS_1) & MBOX_STATUS_ATTN) {
> +out_response:
> +
> > +	/*
> > +	 * The BMC has touched byte 15 to get our attention as it has
> > +	 * something to tell us.
> > +	 */
> > +	if (bmc_mbox_inb(MBOX_STATUS_1) & MBOX_STATUS_1_ATTN) {
> >  		uint8_t action;
>  
> >  		/* W1C on that reg */
> > -		bmc_mbox_outb(MBOX_STATUS_ATTN, MBOX_STATUS_1);
> > +		bmc_mbox_outb(MBOX_STATUS_1_ATTN, MBOX_STATUS_1);
>  
> >  		action = bmc_mbox_inb(MBOX_FLAG_REG);
> > -		prlog(PR_INSANE, "Got a status register interrupt with action 0x%02x\n",
> > +		prlog(PR_TRACE, "Got a status register interrupt with action 0x%02x\n",
> >  				action);
>  
> >  		if (action & BMC_RESET) {
> > -			/* TODO Freak */
> > +			/*
> > +			 * It's unlikely that something needs to be done at the
> > +			 * driver level. Let libflash deal with it.
> > +			 * Print something just in case, it is quite a signficant
> > +			 * event.
> > +			 */
> >  			prlog(PR_WARNING, "BMC reset detected\n");
> >  			action &= ~BMC_RESET;
> >  		}
> @@ -202,7 +225,7 @@ static bool mbox_init_hw(void)
>  {
> >  	/* Disable all status interrupts except attentions */
> >  	bmc_mbox_outb(0x00, MBOX_HOST_INT_EN_0);
> > -	bmc_mbox_outb(MBOX_STATUS_ATTN, MBOX_HOST_INT_EN_1);
> +	bmc_mbox_outb(MBOX_STATUS_1_ATTN, MBOX_HOST_INT_EN_1);

The datasheet really doesn't help much with its descriptions of the
mailbox registers.

>  
> >  	/* Cleanup host interrupt and status */
> >  	bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL);
> @@ -274,6 +297,13 @@ void mbox_init(void)
> >  		return;
> >  	}
>  
> > +	/* Disable the standard interrupt we don't care */
> > +	bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_HOST_CTRL);
> +
> > +	/* Clear the status reg bits that we intend to use for interrupts */
> > +	/* W1C */
> > +	bmc_mbox_outb(MBOX_STATUS_1_RESP | MBOX_STATUS_1_ATTN, MBOX_STATUS_1);
> +
> >  	mbox.queue_len = 0;
> >  	mbox.in_flight = NULL;
> >  	mbox.callback = NULL;
> @@ -286,6 +316,9 @@ void mbox_init(void)
> >  	mbox_lpc_client.interrupts = LPC_IRQ(irq);
> >  	lpc_register_client(chip_id, &mbox_lpc_client, IRQ_ATTR_TARGET_OPAL);
>  
> > +	/* Enable interrupts */
> > +	bmc_mbox_outb(MBOX_STATUS_1_ATTN | MBOX_STATUS_1_RESP, MBOX_HOST_INT_EN_1);
> +
> >  	prlog(PR_DEBUG, "Enabled on chip %d, IO port 0x%x, IRQ %d\n",
> >  	      chip_id, mbox.base, irq);
>  }
> diff --git a/include/lpc-mbox.h b/include/lpc-mbox.h
> index c67efee..14e38cf 100644
> --- a/include/lpc-mbox.h
> +++ b/include/lpc-mbox.h
> @@ -20,9 +20,9 @@
>  #include <opal.h>
>  #include <ccan/endian/endian.h>
>  
> -/* Not 16 because the last two are interrupt based status regs */
> -#define BMC_MBOX_DATA_REGS 14
>  #define BMC_MBOX_ARGS_REGS 11
> +#define BMC_MBOX_READ_REGS 16
> +#define BMC_MBOX_WRITE_REGS 13
>  
>  #define MBOX_C_RESET_STATE 0x01
>  #define MBOX_C_GET_MBOX_INFO 0x02


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

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20170525/a18ddde5/attachment.sig>


More information about the Skiboot mailing list