| Message ID | 1492729190.25766.155.camel@kernel.crashing.org |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, 2017-04-21 at 08:59 +1000, Benjamin Herrenschmidt wrote: > If some status interrupts are left unmasked by a previous > firmware run (either HostBoot or some other version of skiboot), > we fail to clear them and end up with a runaway SerIRQ. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > > Let's merge that now. It will be obsoleted by the v2 protocol > implementation but we want a fix out ASAP. I think this is still important even after v2 implementation gets merged. > > hw/lpc-mbox.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c > index fd107fb..21e6eee 100644 > --- a/hw/lpc-mbox.c > +++ b/hw/lpc-mbox.c > @@ -200,11 +200,18 @@ static struct lpc_client mbox_lpc_client = { > > static bool mbox_init_hw(void) > { > - /* > - * Turns out there isn't anything to do. > - * It might be a good idea to santise the registers though. > - * TODO > + /* 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); > + Would it be wise to also (you do it for the control register interrupt): /* * Cleanup host interrupt status bits except attention, these * bits are write 1 to clear. */ bmc_mbox_outb(0xff, MBOX_STATUS_0); bmc_mbox_outb(~MBOX_STATUS_ATTN, MBOX_STATUS_1); > + /* Cleanup host interrupt and status */ > + bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL); > + > + /* Disable host control interrupt for now (will be > + * re-enabled when needed). Clear BMC interrupts > */ > + bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_BMC_CTRL); > + > return true; > } > > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Fri, 2017-04-21 at 09:51 +1000, Cyril Bur wrote: > On Fri, 2017-04-21 at 08:59 +1000, Benjamin Herrenschmidt wrote: > > If some status interrupts are left unmasked by a previous > > firmware run (either HostBoot or some other version of skiboot), > > we fail to clear them and end up with a runaway SerIRQ. > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > --- > > > > Let's merge that now. It will be obsoleted by the v2 protocol > > implementation but we want a fix out ASAP. > > I think this is still important even after v2 implementation gets > merged. Don't we use a different interrupt with v2 anyway ? > > > > hw/lpc-mbox.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c > > index fd107fb..21e6eee 100644 > > --- a/hw/lpc-mbox.c > > +++ b/hw/lpc-mbox.c > > @@ -200,11 +200,18 @@ static struct lpc_client mbox_lpc_client = { > > > > static bool mbox_init_hw(void) > > { > > - /* > > - * Turns out there isn't anything to do. > > - * It might be a good idea to santise the registers > > though. > > - * TODO > > + /* 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); > > + > > Would it be wise to also (you do it for the control register > interrupt): > > /* > * Cleanup host interrupt status bits except attention, these > * bits are write 1 to clear. > */ > bmc_mbox_outb(0xff, MBOX_STATUS_0); > bmc_mbox_outb(~MBOX_STATUS_ATTN, MBOX_STATUS_1); Not as important since we don't enable those... it won't hurt to clear them but it won't gain anything either. Once we enable them, the story is different. So let's do that later with the v2 update. Another subsequent fix we will need when we start using status interrupts for message exchanges: Remember how the HW trigger the interrupt by our own writes to the register ? IE not only when the BMC writes the response but also when we write the command. So if/when we switch to using the status interrupt instead of the host interrupt, we need to keep it disabled until after we have written the command (but not sent it to BMC yet) and cleared the status bit. Only then it is safe to enable the interrupt. > > + /* Cleanup host interrupt and status */ > > + bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL); > > + > > + /* Disable host control interrupt for now (will be > > + * re-enabled when needed). Clear BMC interrupts > > */ > > + bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_BMC_CTRL); > > + > > return true; > > } > > > > > > _______________________________________________ > > Skiboot mailing list > > Skiboot@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/skiboot
This is upstream as of 50e1921f98. On Fri, 2017-04-21 at 08:59 +1000, Benjamin Herrenschmidt wrote: > If some status interrupts are left unmasked by a previous > firmware run (either HostBoot or some other version of skiboot), > we fail to clear them and end up with a runaway SerIRQ. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > > Let's merge that now. It will be obsoleted by the v2 protocol > implementation but we want a fix out ASAP. > > hw/lpc-mbox.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c > index fd107fb..21e6eee 100644 > --- a/hw/lpc-mbox.c > +++ b/hw/lpc-mbox.c > @@ -200,11 +200,18 @@ static struct lpc_client mbox_lpc_client = { > > static bool mbox_init_hw(void) > { > - /* > - * Turns out there isn't anything to do. > - * It might be a good idea to santise the registers though. > - * TODO > + /* 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); > + > + /* Cleanup host interrupt and status */ > + bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL); > + > + /* Disable host control interrupt for now (will be > + * re-enabled when needed). Clear BMC interrupts > */ > + bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_BMC_CTRL); > + > return true; > } > > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c index fd107fb..21e6eee 100644 --- a/hw/lpc-mbox.c +++ b/hw/lpc-mbox.c @@ -200,11 +200,18 @@ static struct lpc_client mbox_lpc_client = { static bool mbox_init_hw(void) { - /* - * Turns out there isn't anything to do. - * It might be a good idea to santise the registers though. - * TODO + /* 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); + + /* Cleanup host interrupt and status */ + bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL); + + /* Disable host control interrupt for now (will be + * re-enabled when needed). Clear BMC interrupts */ + bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_BMC_CTRL); + return true; }
If some status interrupts are left unmasked by a previous firmware run (either HostBoot or some other version of skiboot), we fail to clear them and end up with a runaway SerIRQ. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- Let's merge that now. It will be obsoleted by the v2 protocol implementation but we want a fix out ASAP. hw/lpc-mbox.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)