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;
Feedback-ID: i0ad843c9:Fastmail
Message-ID: <735349b0-6738-8ace-a7d3-aafae2a62501@sholland.org>
Date:   Tue, 15 Nov 2022 00:08:12 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux ppc64le; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Content-Language: en-US
To:     =?UTF-8?Q?Jernej_=c5=a0krabec?= <jernej.skrabec@gmail.com>,
        Chen-Yu Tsai <wens@csie.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>
References: <20221114015749.28490-1-samuel@sholland.org>
 <20221114015749.28490-4-samuel@sholland.org>
 <2170633.72vocr9iq0@jernej-laptop>
From:   Samuel Holland <samuel@sholland.org>
Subject: Re: [PATCH v3 3/3] bus: sunxi-rsb: Clear interrupt status before each
 transfer
In-Reply-To: <2170633.72vocr9iq0@jernej-laptop>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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