Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jernej =?utf-8?B?xaBrcmFiZWM=?= <jernej.skrabec@gmail.com>
To:     Chen-Yu Tsai <wens@csie.org>, Samuel Holland <samuel@sholland.org>
Cc:     Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-sunxi@lists.linux.dev,
        Andre Przywara <andre.przywara@arm.com>,
        Samuel Holland <samuel@sholland.org>
Subject: Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each transfer
Date:   Mon, 14 Nov 2022 22:00:06 +0100
Message-ID: <2170633.72vocr9iq0@jernej-laptop>
In-Reply-To: <20221114015749.28490-4-samuel@sholland.org>
References: <20221114015749.28490-1-samuel@sholland.org> <20221114015749.28490-4-samuel@sholland.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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?

> 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.

Best regards,
Jernej

>  	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;
>  }