| Message ID | 20190824175013.28840-1-samuel@sholland.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bus: sunxi-rsb: Make interrupt handling more robust | expand |
Hello, On 8/24/19 12:50 PM, Samuel Holland wrote: > The RSB controller has two registers for controlling interrupt inputs: > RSB_INTE, which has bits for each possible interrupt, and the global > interrupt enable bit in RSB_CTRL. > > Currently, we enable the bits in RSB_INTE before each transfer, but this > is unnecessary because we never disable them. Move the initialization of > RSB_INTE so it is done only once. > > We also set the global interrupt enable bit before each transfer. Unlike > other bits in RSB_CTRL, this bit is cleared by writing a zero. Thus, we > clear the bit in the post-timeout cleanup code, so I note that in the > comment. > > However, if we do receive an interrupt, we do not clear the bit. Nor do > we clear interrupt statuses before starting a transfer. Thus, if some > other driver uses the RSB bus while Linux is suspended (as both Trusted > Firmware and SCP firmware do to control the PMIC), we receive spurious > interrupts upon resume. This causes false completion of a transfer, and > the next transfer starts prematurely, causing a LOAD_BSY condition. The > end result is that some transfers at resume fail with -EBUSY. > > With this patch, all transfers reliably succeed during/after resume. > > Signed-off-by: Samuel Holland <samuel@sholland.org> Ping? Any comments? > --- > drivers/bus/sunxi-rsb.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c > index be79d6c6a4e4..b8043b58568a 100644 > --- a/drivers/bus/sunxi-rsb.c > +++ b/drivers/bus/sunxi-rsb.c > @@ -274,7 +274,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) > reinit_completion(&rsb->complete); > > writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER, > - rsb->regs + RSB_INTE); > + rsb->regs + RSB_INTS); > writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB, > rsb->regs + RSB_CTRL); > > @@ -282,7 +282,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) > msecs_to_jiffies(100))) { > dev_dbg(rsb->dev, "RSB timeout\n"); > > - /* abort the transfer */ > + /* abort the transfer and disable interrupts */ > writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL); > > /* clear any interrupt flags */ > @@ -480,6 +480,9 @@ static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id) > status = readl(rsb->regs + RSB_INTS); > rsb->status = status; > > + /* Disable any further interrupts */ > + writel(0, rsb->regs + RSB_CTRL); > + > /* Clear interrupts */ > status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | > RSB_INTS_TRANS_OVER); > @@ -718,6 +721,9 @@ static int sunxi_rsb_probe(struct platform_device *pdev) > goto err_reset_assert; > } > > + writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER, > + rsb->regs + RSB_INTE); > + > /* initialize all devices on the bus into RSB mode */ > ret = sunxi_rsb_init_device_mode(rsb); > if (ret) >
On Sun, Aug 25, 2019 at 1:50 AM Samuel Holland <samuel@sholland.org> wrote: > > The RSB controller has two registers for controlling interrupt inputs: > RSB_INTE, which has bits for each possible interrupt, and the global > interrupt enable bit in RSB_CTRL. > > Currently, we enable the bits in RSB_INTE before each transfer, but this > is unnecessary because we never disable them. Move the initialization of > RSB_INTE so it is done only once. > > We also set the global interrupt enable bit before each transfer. Unlike > other bits in RSB_CTRL, this bit is cleared by writing a zero. Thus, we > clear the bit in the post-timeout cleanup code, so note that in the > comment. > > However, if we do receive an interrupt, we do not clear the bit. Nor do > we clear interrupt statuses before starting a transfer. Thus, if some > other driver uses the RSB bus while Linux is suspended (as both Trusted > Firmware and SCP firmware do to control the PMIC), we receive spurious > interrupts upon resume. This causes false completion of a transfer, and > the next transfer starts prematurely, causing a LOAD_BSY condition. The > end result is that some transfers at resume fail with -EBUSY. If we are expecting the hardware to not be in the state we assume to be or left it in, then maybe we should also keep setting the interrupt enable bits on each transfer? Surely we expect to have exclusive use of the controller most of the time. If it's to handle suspend/resume, shouldn't we be adding power management callbacks instead? That would reset the controller to a known state when the system comes out of suspend, including clearing any pending interrupts. Maxime, anything you want to add? (BTW, Maxime switched email addresses.) ChenYu > With this patch, all transfers reliably succeed during/after resume. > > Signed-off-by: Samuel Holland <samuel@sholland.org> > --- > drivers/bus/sunxi-rsb.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c > index be79d6c6a4e4..b8043b58568a 100644 > --- a/drivers/bus/sunxi-rsb.c > +++ b/drivers/bus/sunxi-rsb.c > @@ -274,7 +274,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) > reinit_completion(&rsb->complete); > > writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER, > - rsb->regs + RSB_INTE); > + rsb->regs + RSB_INTS); > writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB, > rsb->regs + RSB_CTRL); > > @@ -282,7 +282,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) > msecs_to_jiffies(100))) { > dev_dbg(rsb->dev, "RSB timeout\n"); > > - /* abort the transfer */ > + /* abort the transfer and disable interrupts */ > writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL); > > /* clear any interrupt flags */ > @@ -480,6 +480,9 @@ static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id) > status = readl(rsb->regs + RSB_INTS); > rsb->status = status; > > + /* Disable any further interrupts */ > + writel(0, rsb->regs + RSB_CTRL); > + > /* Clear interrupts */ > status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | > RSB_INTS_TRANS_OVER); > @@ -718,6 +721,9 @@ static int sunxi_rsb_probe(struct platform_device *pdev) > goto err_reset_assert; > } > > + writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER, > + rsb->regs + RSB_INTE); > + > /* initialize all devices on the bus into RSB mode */ > ret = sunxi_rsb_init_device_mode(rsb); > if (ret) > -- > 2.21.0 > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190824175013.28840-1-samuel%40sholland.org.
Hi, On Mon, Oct 07, 2019 at 11:19:06PM +0800, Chen-Yu Tsai wrote: > On Sun, Aug 25, 2019 at 1:50 AM Samuel Holland <samuel@sholland.org> wrote: > > The RSB controller has two registers for controlling interrupt inputs: > > RSB_INTE, which has bits for each possible interrupt, and the global > > interrupt enable bit in RSB_CTRL. > > > > Currently, we enable the bits in RSB_INTE before each transfer, but this > > is unnecessary because we never disable them. Move the initialization of > > RSB_INTE so it is done only once. > > > > We also set the global interrupt enable bit before each transfer. Unlike > > other bits in RSB_CTRL, this bit is cleared by writing a zero. Thus, we > > clear the bit in the post-timeout cleanup code, so note that in the > > comment. > > > > However, if we do receive an interrupt, we do not clear the bit. Nor do > > we clear interrupt statuses before starting a transfer. Thus, if some > > other driver uses the RSB bus while Linux is suspended (as both Trusted > > Firmware and SCP firmware do to control the PMIC), we receive spurious > > interrupts upon resume. This causes false completion of a transfer, and > > the next transfer starts prematurely, causing a LOAD_BSY condition. The > > end result is that some transfers at resume fail with -EBUSY. > > If we are expecting the hardware to not be in the state we assume to be > or left it in, then maybe we should also keep setting the interrupt enable > bits on each transfer? > > Surely we expect to have exclusive use of the controller most of the time. > If it's to handle suspend/resume, shouldn't we be adding power management > callbacks instead? That would reset the controller to a known state when > the system comes out of suspend, including clearing any pending interrupts. > > Maxime, anything you want to add? (BTW, Maxime switched email addresses.) The patch looks pretty harmless, so we can merge it, but if we're going to share the RSB between those components, we should probably use the hardware spinlocks as well. Maxime
On 10/7/19 10:19 AM, Chen-Yu Tsai wrote: > On Sun, Aug 25, 2019 at 1:50 AM Samuel Holland <samuel@sholland.org> wrote: >> >> The RSB controller has two registers for controlling interrupt inputs: >> RSB_INTE, which has bits for each possible interrupt, and the global >> interrupt enable bit in RSB_CTRL. >> >> Currently, we enable the bits in RSB_INTE before each transfer, but this >> is unnecessary because we never disable them. Move the initialization of >> RSB_INTE so it is done only once. >> >> We also set the global interrupt enable bit before each transfer. Unlike >> other bits in RSB_CTRL, this bit is cleared by writing a zero. Thus, we >> clear the bit in the post-timeout cleanup code, so note that in the >> comment. >> >> However, if we do receive an interrupt, we do not clear the bit. Nor do >> we clear interrupt statuses before starting a transfer. Thus, if some >> other driver uses the RSB bus while Linux is suspended (as both Trusted >> Firmware and SCP firmware do to control the PMIC), we receive spurious >> interrupts upon resume. This causes false completion of a transfer, and >> the next transfer starts prematurely, causing a LOAD_BSY condition. The >> end result is that some transfers at resume fail with -EBUSY. > > If we are expecting the hardware to not be in the state we assume to be > or left it in, then maybe we should also keep setting the interrupt enable > bits on each transfer? > > Surely we expect to have exclusive use of the controller most of the time. > If it's to handle suspend/resume, shouldn't we be adding power management > callbacks instead? That would reset the controller to a known state when > the system comes out of suspend, including clearing any pending interrupts. Yes, this change is only to handle suspend/resume. You're right, that's a better way to do it. I'll develop a patch using device power management callbacks. Samuel > Maxime, anything you want to add? (BTW, Maxime switched email addresses.) > > ChenYu > >> With this patch, all transfers reliably succeed during/after resume. >> >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> drivers/bus/sunxi-rsb.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c >> index be79d6c6a4e4..b8043b58568a 100644 >> --- a/drivers/bus/sunxi-rsb.c >> +++ b/drivers/bus/sunxi-rsb.c >> @@ -274,7 +274,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) >> reinit_completion(&rsb->complete); >> >> writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER, >> - rsb->regs + RSB_INTE); >> + rsb->regs + RSB_INTS); >> writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB, >> rsb->regs + RSB_CTRL); >> >> @@ -282,7 +282,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) >> msecs_to_jiffies(100))) { >> dev_dbg(rsb->dev, "RSB timeout\n"); >> >> - /* abort the transfer */ >> + /* abort the transfer and disable interrupts */ >> writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL); >> >> /* clear any interrupt flags */ >> @@ -480,6 +480,9 @@ static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id) >> status = readl(rsb->regs + RSB_INTS); >> rsb->status = status; >> >> + /* Disable any further interrupts */ >> + writel(0, rsb->regs + RSB_CTRL); >> + >> /* Clear interrupts */ >> status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | >> RSB_INTS_TRANS_OVER); >> @@ -718,6 +721,9 @@ static int sunxi_rsb_probe(struct platform_device *pdev) >> goto err_reset_assert; >> } >> >> + writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER, >> + rsb->regs + RSB_INTE); >> + >> /* initialize all devices on the bus into RSB mode */ >> ret = sunxi_rsb_init_device_mode(rsb); >> if (ret) >> -- >> 2.21.0 >> >> -- >> You received this message because you are subscribed to the Google Groups "linux-sunxi" group. >> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. >> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190824175013.28840-1-samuel%40sholland.org.
diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c index be79d6c6a4e4..b8043b58568a 100644 --- a/drivers/bus/sunxi-rsb.c +++ b/drivers/bus/sunxi-rsb.c @@ -274,7 +274,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) reinit_completion(&rsb->complete); writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER, - rsb->regs + RSB_INTE); + rsb->regs + RSB_INTS); writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB, rsb->regs + RSB_CTRL); @@ -282,7 +282,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) msecs_to_jiffies(100))) { dev_dbg(rsb->dev, "RSB timeout\n"); - /* abort the transfer */ + /* abort the transfer and disable interrupts */ writel(RSB_CTRL_ABORT_TRANS, rsb->regs + RSB_CTRL); /* clear any interrupt flags */ @@ -480,6 +480,9 @@ static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id) status = readl(rsb->regs + RSB_INTS); rsb->status = status; + /* Disable any further interrupts */ + writel(0, rsb->regs + RSB_CTRL); + /* Clear interrupts */ status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER); @@ -718,6 +721,9 @@ static int sunxi_rsb_probe(struct platform_device *pdev) goto err_reset_assert; } + writel(RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER, + rsb->regs + RSB_INTE); + /* initialize all devices on the bus into RSB mode */ ret = sunxi_rsb_init_device_mode(rsb); if (ret)
The RSB controller has two registers for controlling interrupt inputs: RSB_INTE, which has bits for each possible interrupt, and the global interrupt enable bit in RSB_CTRL. Currently, we enable the bits in RSB_INTE before each transfer, but this is unnecessary because we never disable them. Move the initialization of RSB_INTE so it is done only once. We also set the global interrupt enable bit before each transfer. Unlike other bits in RSB_CTRL, this bit is cleared by writing a zero. Thus, we clear the bit in the post-timeout cleanup code, so note that in the comment. However, if we do receive an interrupt, we do not clear the bit. Nor do we clear interrupt statuses before starting a transfer. Thus, if some other driver uses the RSB bus while Linux is suspended (as both Trusted Firmware and SCP firmware do to control the PMIC), we receive spurious interrupts upon resume. This causes false completion of a transfer, and the next transfer starts prematurely, causing a LOAD_BSY condition. The end result is that some transfers at resume fail with -EBUSY. With this patch, all transfers reliably succeed during/after resume. Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/bus/sunxi-rsb.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)