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

[Dmitry Osipenko]

19.06.2019 11:58, 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?

Probably, yes.

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

Good points! Bitan, please take this all into account.