19.06.2019 12:02, 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.
Ah, actually looks like i2c_dev->irq won't be obsoleted, but not a big deal.
Also, please note that it won't be the least to change the type of "irq" to unsigned int for
consistency. I know that Thierry is picky about it.
