| Message ID | 1560847368-16069-1-git-send-email-bbiswas@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [V2] i2c: tegra: disable irq in tegra_i2c_xfer_msg | expand |
On 18/06/2019 09:42, Bitan Biswas wrote: > 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. > > 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. Sorry but I still don't understand the problem you are trying to solve. Yes spinlocks are busy waits but is this busy wait an actual problem? If so what is the problem with this? It appears that the spinlock was added to prevent error interrupts occurring until the transfer has started. If this is for error cases, then probably it is not often that the CPU is stuck busy waiting on the spinlock. Furthermore, in addition to the spinlock we also have calls to tegra_i2c_unmask_irq/tegra_i2c_mask_irq. Therefore, if we are going to change this it would seem like a good idea to consolidate the masking/unmasking of IRQs and the spinlock, if possible. Finally, I still see that we have a spinlock in the downstream kernels we are shipping and so I would prefer to see such a change also be tested in the downstream kernels we are releasing. Cheers Jon
On 2019-06-18 10:42, Bitan Biswas wrote: > 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. > > 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@nvidia.com> > --- > 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); Why do you use ..._nosync in this context? Do you not care about exactly when the irq is actually disabled? Maybe you must use _nosync, I don't know, but I get the feeling that you just copied the code from the ISR. Anyway, the question is if this is still adequate synchronization? Everything might very well be safe and in order, but this is not clear to me from neither the code, the (nonexisting) comments nor the patch description. I'm no IRQ expert, I'm just asking questions... Cheers, Peter > + i2c_dev->irq_disabled = true; > + } > > int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST; > tegra_i2c_unmask_irq(i2c_dev, int_mask); > @@ -1180,7 +1177,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, > i2c_readl(i2c_dev, I2C_INT_MASK)); > > unlock: > - spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags); > + if (i2c_dev->irq_disabled) { > + i2c_dev->irq_disabled = false; > + enable_irq(i2c_dev->irq); > + } > > if (dma) { > if (err) > @@ -1576,7 +1576,6 @@ static int tegra_i2c_probe(struct platform_device *pdev) > I2C_PACKET_HEADER_SIZE; > init_completion(&i2c_dev->msg_complete); > init_completion(&i2c_dev->dma_complete); > - spin_lock_init(&i2c_dev->xfer_lock); > > if (!i2c_dev->hw->has_single_clk_source) { > fast_clk = devm_clk_get(&pdev->dev, "fast-clk"); >
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@nvidia.com> > --- > 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. [snip]
18.06.2019 12:13, Jon Hunter пишет: > > On 18/06/2019 09:42, Bitan Biswas wrote: >> 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. >> >> 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. > Sorry but I still don't understand the problem you are trying to solve. > Yes spinlocks are busy waits but is this busy wait an actual problem? If > so what is the problem with this? There is no real problem here, the intent is to clean up the code more and that should be mentioned in the commit message for clarity. This was actually my suggestion that Bitan is now started to implement, which is very nice! It should be a good result in the end. > It appears that the spinlock was added to prevent error interrupts > occurring until the transfer has started. If this is for error cases, > then probably it is not often that the CPU is stuck busy waiting on the > spinlock. > > Furthermore, in addition to the spinlock we also have calls to > tegra_i2c_unmask_irq/tegra_i2c_mask_irq. Therefore, if we are going to > change this it would seem like a good idea to consolidate the > masking/unmasking of IRQs and the spinlock, if possible. > > Finally, I still see that we have a spinlock in the downstream kernels > we are shipping and so I would prefer to see such a change also be > tested in the downstream kernels we are releasing.
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@nvidia.com> >> --- >> 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
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@nvidia.com> >>> --- >>> 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.
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@nvidia.com> >>>> --- >>>> 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.
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; + } int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST; tegra_i2c_unmask_irq(i2c_dev, int_mask); @@ -1180,7 +1177,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, i2c_readl(i2c_dev, I2C_INT_MASK)); unlock: - spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags); + if (i2c_dev->irq_disabled) { + i2c_dev->irq_disabled = false; + enable_irq(i2c_dev->irq); + } if (dma) { if (err) @@ -1576,7 +1576,6 @@ static int tegra_i2c_probe(struct platform_device *pdev) I2C_PACKET_HEADER_SIZE; init_completion(&i2c_dev->msg_complete); init_completion(&i2c_dev->dma_complete); - spin_lock_init(&i2c_dev->xfer_lock); if (!i2c_dev->hw->has_single_clk_source) { fast_clk = devm_clk_get(&pdev->dev, "fast-clk");
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. 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@nvidia.com> --- drivers/i2c/busses/i2c-tegra.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)