Messages in this thread Patch in this message | | | From | Jernej Škrabec <> | Subject | Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer | Date | Tue, 15 Nov 2022 22:38:29 +0100 |
| |
Dne torek, 15. november 2022 ob 07:08:12 CET je Samuel Holland napisal(a): > 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.
Right.
> > >> 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.
Sure, first two patches will go in regardless. I'm not convinced of value of this patch either. I guess we can skip it.
Best regards, Jernej
> > 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; > >> > >> }
| |