Re: [PATCH V2] i2c: tegra: disable irq in tegra_i2c_xfer_msg

[Jon Hunter]

On 18/06/2019 19:26, Dmitry Osipenko wrote:
> 18.06.2019 11:42, Bitan Biswas ÐÐÑÐÑ:
>> tegra_i2c_xfer_msg initiates the I2C transfer in DMA
>> or PIO mode. It involves steps that need FIFO register
>> access, DMA API calls like dma_sync_single_for_device, etc.
>> Tegra I2C ISR has calls to tegra_i2c_empty_rx_fifo in PIO mode
>> and in DMA/PIO mode writes different I2C registers including
>> I2C interrupt status. ISR cannot start processing
>> before the preparation step at tegra_i2c_xfer_msg is complete.
>> Hence, a synchronization between ISR and tegra_i2c_xfer_msg
>> is in place today using spinlock.
> 
> Please use full 75 chars per-line, this should make commit message to look better.
> 
>> Spinlock busy waits and can add avoidable delays.
>>
>> In this patch needed synchronization is achieved by disabling
>> I2C interrupt during preparation step and enabling interrupt
>> once preparation is over and spinlock is no longer needed.
>>
>> Signed-off-by: Bitan Biswas <bbiswas@xxxxxxxxxx>
>> ---
>>  drivers/i2c/busses/i2c-tegra.c | 17 ++++++++---------
>>  1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index 6fb545e..ccc7fae 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -240,7 +240,6 @@ struct tegra_i2c_hw_feature {
>>   * @bus_clk_rate: current I2C bus clock rate
>>   * @clk_divisor_non_hs_mode: clock divider for non-high-speed modes
>>   * @is_multimaster_mode: track if I2C controller is in multi-master mode
>> - * @xfer_lock: lock to serialize transfer submission and processing
>>   * @tx_dma_chan: DMA transmit channel
>>   * @rx_dma_chan: DMA receive channel
>>   * @dma_phys: handle to DMA resources
>> @@ -270,8 +269,6 @@ struct tegra_i2c_dev {
>>  	u32 bus_clk_rate;
>>  	u16 clk_divisor_non_hs_mode;
>>  	bool is_multimaster_mode;
>> -	/* xfer_lock: lock to serialize transfer submission and processing */
>> -	spinlock_t xfer_lock;
>>  	struct dma_chan *tx_dma_chan;
>>  	struct dma_chan *rx_dma_chan;
>>  	dma_addr_t dma_phys;
>> @@ -835,7 +832,6 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>>  
>>  	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
>>  
>> -	spin_lock(&i2c_dev->xfer_lock);
>>  	if (status == 0) {
>>  		dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
>>  			 i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
>> @@ -935,7 +931,6 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>>  
>>  	complete(&i2c_dev->msg_complete);
>>  done:
>> -	spin_unlock(&i2c_dev->xfer_lock);
>>  	return IRQ_HANDLED;
>>  }
>>  
>> @@ -1054,7 +1049,6 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>>  	u32 packet_header;
>>  	u32 int_mask;
>>  	unsigned long time_left;
>> -	unsigned long flags;
>>  	size_t xfer_size;
>>  	u32 *buffer = NULL;
>>  	int err = 0;
>> @@ -1085,7 +1079,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>>  	 */
>>  	xfer_time += DIV_ROUND_CLOSEST(((xfer_size * 9) + 2) * MSEC_PER_SEC,
>>  					i2c_dev->bus_clk_rate);
>> -	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
>> +	if (!i2c_dev->irq_disabled) {
>> +		disable_irq_nosync(i2c_dev->irq);
>> +		i2c_dev->irq_disabled = true;
>> +	}
> 
> 1) Peter correctly pointed out in the other email that the disabling should be synced.
> But see more below in 3.
> 
> 2) i2c_dev->irq_disabled == true can't ever be the case here because tegra_i2c_init()
> re-enables interrupt in a case of error condition. Hence interrupt always enabled at
> the beginning of the transfer.
> 
> 3) In my previous answer I was suggesting to request IRQ in a disabled state, this
> will allow to remove i2c_dev->irq_disabled completely.
> 
> Then the tegra_i2c_xfer_msg() will have to enable IRQ after completion of the
> transfer-preparation process and disable IRQ once transfer is done (both success and
> failure cases). This is actually not a bad additional motivation for this patch, to
> keep CPU's interrupt disabled while idling and not to only rely on interrupt masking
> of the I2C hardware.
> 
> 4) ISR should simply return IRQ_NONE when interrupt status is 0 and allow kernel core
> to disable the faulty interrupt itself. There will be "unhandled interrupt" error
> message in KMSG log, following the disabling.
> 
> 5) In order to request IRQ in a disabled state, the IRQ_NOAUTOEN flag need to be set
> before the requesting, like this:
> 
>     irq_set_status_flags(irq, IRQ_NOAUTOEN);
> 
>     devm_request_irq(&pdev->dev, irq...);
> 
> In a result of combining 3-5, both i2c_dev->irq_disabled and i2c_dev->irq variables
> become obsolete and could be removed in addition to xfer_lock. That all is a good
> cleanup in my opinion.

I see, so basically you are simplifying the code by waiting to enable
the interrupt until the transfer is ready and hence you can eliminate
the need for the spinlock. OK, that would make sense. This really needs
to be describe better in the changelog.

Also what about the tegra_i2c_unmask/mask_irq? Can these be eliminated?

Finally, what about tegra_i2c_issue_bus_clear()? This requires
interrupts as well.

Cheers
Jon

-- 
nvpublic