Messages in this thread | | | Date | Tue, 15 Nov 2022 00:08:12 -0600 | From | Samuel Holland <> | Subject | Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer |
| |
On 11/14/22 15:00, Jernej Škrabec wrote: > Hi Samuel, > > Dne ponedeljek, 14. november 2022 ob 02:57:49 CET je Samuel Holland > napisal(a): >> Currently, the driver clears the interrupt status bits after anything >> could have set them. However, this requires duplicating the same logic >> in several places. >> >> Instead of clearing the status flags in the interrupt handler, disable >> all further interrupts by clearing the RSB_CTRL_GLOBAL_INT_ENB bit. > > where is this bit cleared?
It is cleared by any write to RSB_CTRL that does not include it. I noted it below with the "disable interrupts" comments.
>> Then we can delay the status register write until the start of the next >> transfer, so it only has to be done in one place. >> >> Signed-off-by: Samuel Holland <samuel@sholland.org> >> --- >> >> Changes in v3: >> - Add a patch refactoring how the status bits are cleared >> >> drivers/bus/sunxi-rsb.c | 20 +++++--------------- >> 1 file changed, 5 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c >> index 3aa91aed3bf7..cb622e60897b 100644 >> --- a/drivers/bus/sunxi-rsb.c >> +++ b/drivers/bus/sunxi-rsb.c >> @@ -279,6 +279,7 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) >> >> int_mask = RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | RSB_INTS_TRANS_OVER; >> writel(int_mask, rsb->regs + RSB_INTE); >> + writel(int_mask, rsb->regs + RSB_INTS); > > Wouldn't be better to clear status before enabling interrupts? Unless global > interrupt flag is cleared beforehand, but I don't see that anywhere.
Indeed the intention was that the global interrupt flag is cleared beforehand, and only enabled on the next line below. However, I realize I missed disabling it for the new atomic case.
I'm not so sure anymore that this patch is an improvement. What do you think? I can send a v4 with a fix, or I am fine with skipping this patch. I would at least like the other two to be merged for -fixes.
Regards, Samuel
>> writel(RSB_CTRL_START_TRANS | RSB_CTRL_GLOBAL_INT_ENB, >> rsb->regs + RSB_CTRL); >> >> @@ -286,7 +287,6 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) >> timeout = readl_poll_timeout_atomic(rsb->regs + RSB_INTS, >> status, (status & int_mask), >> 10, 100000); >> - writel(status, rsb->regs + RSB_INTS); >> } else { >> timeout = !wait_for_completion_io_timeout(&rsb- >> complete, >> msecs_to_jiffies(100)); >> @@ -296,12 +296,9 @@ static int _sunxi_rsb_run_xfer(struct sunxi_rsb *rsb) >> if (timeout) { >> 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 */ >> - writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS); >> - >> return -ETIMEDOUT; >> } >> >> @@ -503,15 +500,11 @@ EXPORT_SYMBOL_GPL(__devm_regmap_init_sunxi_rsb); >> static irqreturn_t sunxi_rsb_irq(int irq, void *dev_id) >> { >> struct sunxi_rsb *rsb = dev_id; >> - u32 status; >> >> - status = readl(rsb->regs + RSB_INTS); >> - rsb->status = status; >> + /* disable interrupts */ >> + writel(0, rsb->regs + RSB_CTRL); >> >> - /* Clear interrupts */ >> - status &= (RSB_INTS_LOAD_BSY | RSB_INTS_TRANS_ERR | >> - RSB_INTS_TRANS_OVER); >> - writel(status, rsb->regs + RSB_INTS); >> + rsb->status = readl(rsb->regs + RSB_INTS); >> >> complete(&rsb->complete); >> >> @@ -532,9 +525,6 @@ static int sunxi_rsb_init_device_mode(struct sunxi_rsb >> *rsb) if (reg & RSB_DMCR_DEVICE_START) >> ret = -ETIMEDOUT; >> >> - /* clear interrupt status bits */ >> - writel(readl(rsb->regs + RSB_INTS), rsb->regs + RSB_INTS); >> - >> return ret; >> }
| |