* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-27 21:21 ` Bjorn Helgaas
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Helgaas @ 2022-01-27 21:21 UTC (permalink / raw)
To: qizhong.cheng
Cc: Marc Zyngier, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
[+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > As an edge-triggered interrupts, its interrupt status should
> > > > be cleared before dispatch to the handler of device.
> > >
> > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > the MSI interrupt status before dispatching the handler because
> > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > convincing because your code will now look like this:
> > >
> > > /* Clear the INTx */
> > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
> > > ...
> > >
> > > /* Clear MSI interrupt status */
> > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > generic_handle_domain_irq(port->inner_domain, bit);
> > >
> > > You clear interrupt status before dispatching the handler for
> > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > interrupts.
> > >
> > > So it doesn't seem that simply being edge-triggered is the
> > > critical factor here.
> >
> > This is the usual problem with these half-baked implementations.
> > The signalling to the primary interrupt controller is level, as
> > they take a multitude of input and (crucially) latch the MSI
> > edges. Effectively, this is an edge-to-level converter, with all
> > the problems that this creates.
> >
> > By clearing the status *after* the handling, you lose edges that
> > have been received and coalesced after the read of the status
> > register. By clearing it *before*, you are acknowledging the
> > interrupts early, and allowing them to be coalesced independently
> > of the ones that have been received earlier.
> >
> > This is however mostly an educated guess. Someone with access to
> > the TRM should verify this.
>
> Yes, as Maz said, we save the edge-interrupt status so that it
> becomes a level-interrupt. This is similar to an edge-to-level
> converter, so we need to clear it *before*. We found this problem
> through a lot of experiments and tested this patch.
I thought there might be other host controllers with similar design,
so I looked at all the other drivers and tried to figure out whether
any others had similar problems.
The ones below look suspicious to me because they all clear some sort
of status register *after* handling an MSI. Can you guys take a look
and make sure they are working correctly?
keembay_pcie_msi_irq_handler
status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
if (status & MSI_CTRL_INT)
dw_handle_msi_irq
generic_handle_domain_irq
writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
spear13xx_pcie_irq_handler
status = readl(&app_reg->int_sts)
if (status & MSI_CTRL_INT)
dw_handle_msi_irq
generic_handle_domain_irq
writel(status, &app_reg->int_clr)
advk_pcie_handle_int
isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
advk_pcie_handle_msi
advk_readl(pcie, PCIE_MSI_STATUS_REG)
advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
generic_handle_irq
advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
mtk_pcie_irq_handler
status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
for_each_set_bit_from(irq_bit, &status, ...)
mtk_pcie_msi_handler
generic_handle_domain_irq
writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-27 21:21 ` Bjorn Helgaas
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Helgaas @ 2022-01-27 21:21 UTC (permalink / raw)
To: qizhong.cheng
Cc: Marc Zyngier, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
[+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > As an edge-triggered interrupts, its interrupt status should
> > > > be cleared before dispatch to the handler of device.
> > >
> > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > the MSI interrupt status before dispatching the handler because
> > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > convincing because your code will now look like this:
> > >
> > > /* Clear the INTx */
> > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
> > > ...
> > >
> > > /* Clear MSI interrupt status */
> > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > generic_handle_domain_irq(port->inner_domain, bit);
> > >
> > > You clear interrupt status before dispatching the handler for
> > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > interrupts.
> > >
> > > So it doesn't seem that simply being edge-triggered is the
> > > critical factor here.
> >
> > This is the usual problem with these half-baked implementations.
> > The signalling to the primary interrupt controller is level, as
> > they take a multitude of input and (crucially) latch the MSI
> > edges. Effectively, this is an edge-to-level converter, with all
> > the problems that this creates.
> >
> > By clearing the status *after* the handling, you lose edges that
> > have been received and coalesced after the read of the status
> > register. By clearing it *before*, you are acknowledging the
> > interrupts early, and allowing them to be coalesced independently
> > of the ones that have been received earlier.
> >
> > This is however mostly an educated guess. Someone with access to
> > the TRM should verify this.
>
> Yes, as Maz said, we save the edge-interrupt status so that it
> becomes a level-interrupt. This is similar to an edge-to-level
> converter, so we need to clear it *before*. We found this problem
> through a lot of experiments and tested this patch.
I thought there might be other host controllers with similar design,
so I looked at all the other drivers and tried to figure out whether
any others had similar problems.
The ones below look suspicious to me because they all clear some sort
of status register *after* handling an MSI. Can you guys take a look
and make sure they are working correctly?
keembay_pcie_msi_irq_handler
status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
if (status & MSI_CTRL_INT)
dw_handle_msi_irq
generic_handle_domain_irq
writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
spear13xx_pcie_irq_handler
status = readl(&app_reg->int_sts)
if (status & MSI_CTRL_INT)
dw_handle_msi_irq
generic_handle_domain_irq
writel(status, &app_reg->int_clr)
advk_pcie_handle_int
isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
advk_pcie_handle_msi
advk_readl(pcie, PCIE_MSI_STATUS_REG)
advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
generic_handle_irq
advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
mtk_pcie_irq_handler
status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
for_each_set_bit_from(irq_bit, &status, ...)
mtk_pcie_msi_handler
generic_handle_domain_irq
writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
Bjorn
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
2022-01-27 21:21 ` Bjorn Helgaas
@ 2022-01-28 7:58 ` Jianjun Wang
-1 siblings, 0 replies; 37+ messages in thread
From: Jianjun Wang @ 2022-01-28 7:58 UTC (permalink / raw)
To: Bjorn Helgaas, qizhong.cheng
Cc: Marc Zyngier, Ryder Lee, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
Hi Bjorn,
On Thu, 2022-01-27 at 15:21 -0600, Bjorn Helgaas wrote:
> [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
>
> On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > > As an edge-triggered interrupts, its interrupt status should
> > > > > be cleared before dispatch to the handler of device.
> > > >
> > > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > > the MSI interrupt status before dispatching the handler because
> > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > convincing because your code will now look like this:
> > > >
> > > > /* Clear the INTx */
> > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->irq_domain, bit -
> > > > INTX_SHIFT);
> > > > ...
> > > >
> > > > /* Clear MSI interrupt status */
> > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > >
> > > > You clear interrupt status before dispatching the handler for
> > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > interrupts.
> > > >
> > > > So it doesn't seem that simply being edge-triggered is the
> > > > critical factor here.
> > >
> > > This is the usual problem with these half-baked implementations.
> > > The signalling to the primary interrupt controller is level, as
> > > they take a multitude of input and (crucially) latch the MSI
> > > edges. Effectively, this is an edge-to-level converter, with all
> > > the problems that this creates.
> > >
> > > By clearing the status *after* the handling, you lose edges that
> > > have been received and coalesced after the read of the status
> > > register. By clearing it *before*, you are acknowledging the
> > > interrupts early, and allowing them to be coalesced independently
> > > of the ones that have been received earlier.
> > >
> > > This is however mostly an educated guess. Someone with access to
> > > the TRM should verify this.
> >
> > Yes, as Maz said, we save the edge-interrupt status so that it
> > becomes a level-interrupt. This is similar to an edge-to-level
> > converter, so we need to clear it *before*. We found this problem
> > through a lot of experiments and tested this patch.
>
> I thought there might be other host controllers with similar design,
> so I looked at all the other drivers and tried to figure out whether
> any others had similar problems.
>
> The ones below look suspicious to me because they all clear some sort
> of status register *after* handling an MSI. Can you guys take a look
> and make sure they are working correctly?
>
> keembay_pcie_msi_irq_handler
> status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
>
> spear13xx_pcie_irq_handler
> status = readl(&app_reg->int_sts)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, &app_reg->int_clr)
>
> advk_pcie_handle_int
> isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> advk_pcie_handle_msi
> advk_readl(pcie, PCIE_MSI_STATUS_REG)
> advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> generic_handle_irq
> advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
>
> mtk_pcie_irq_handler
> status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> for_each_set_bit_from(irq_bit, &status, ...)
> mtk_pcie_msi_handler
> generic_handle_domain_irq
> writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
Thanks for mention that. In the hardware corresponding to pcie-
mediatek-gen3.c, the interrupt status in PCIE_INT_STATUS_REG cannot be
cleared if the MSI status remaining in the register of msi_set, so we
have to clear it after handling the MSI.
I guess the root cause of this patch is the interrupt status can be
cleared even the MSI status still remaining, hence that if there are
some MSIs received while clearing the interrupt status, these MSIs
cannot be serviced.
We will discuss and test internally and update the results later,
thanks for your review.
Thanks.
>
> Bjorn
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-28 7:58 ` Jianjun Wang
0 siblings, 0 replies; 37+ messages in thread
From: Jianjun Wang @ 2022-01-28 7:58 UTC (permalink / raw)
To: Bjorn Helgaas, qizhong.cheng
Cc: Marc Zyngier, Ryder Lee, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
Hi Bjorn,
On Thu, 2022-01-27 at 15:21 -0600, Bjorn Helgaas wrote:
> [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
>
> On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > > As an edge-triggered interrupts, its interrupt status should
> > > > > be cleared before dispatch to the handler of device.
> > > >
> > > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > > the MSI interrupt status before dispatching the handler because
> > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > convincing because your code will now look like this:
> > > >
> > > > /* Clear the INTx */
> > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->irq_domain, bit -
> > > > INTX_SHIFT);
> > > > ...
> > > >
> > > > /* Clear MSI interrupt status */
> > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > >
> > > > You clear interrupt status before dispatching the handler for
> > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > interrupts.
> > > >
> > > > So it doesn't seem that simply being edge-triggered is the
> > > > critical factor here.
> > >
> > > This is the usual problem with these half-baked implementations.
> > > The signalling to the primary interrupt controller is level, as
> > > they take a multitude of input and (crucially) latch the MSI
> > > edges. Effectively, this is an edge-to-level converter, with all
> > > the problems that this creates.
> > >
> > > By clearing the status *after* the handling, you lose edges that
> > > have been received and coalesced after the read of the status
> > > register. By clearing it *before*, you are acknowledging the
> > > interrupts early, and allowing them to be coalesced independently
> > > of the ones that have been received earlier.
> > >
> > > This is however mostly an educated guess. Someone with access to
> > > the TRM should verify this.
> >
> > Yes, as Maz said, we save the edge-interrupt status so that it
> > becomes a level-interrupt. This is similar to an edge-to-level
> > converter, so we need to clear it *before*. We found this problem
> > through a lot of experiments and tested this patch.
>
> I thought there might be other host controllers with similar design,
> so I looked at all the other drivers and tried to figure out whether
> any others had similar problems.
>
> The ones below look suspicious to me because they all clear some sort
> of status register *after* handling an MSI. Can you guys take a look
> and make sure they are working correctly?
>
> keembay_pcie_msi_irq_handler
> status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
>
> spear13xx_pcie_irq_handler
> status = readl(&app_reg->int_sts)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, &app_reg->int_clr)
>
> advk_pcie_handle_int
> isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> advk_pcie_handle_msi
> advk_readl(pcie, PCIE_MSI_STATUS_REG)
> advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> generic_handle_irq
> advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
>
> mtk_pcie_irq_handler
> status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> for_each_set_bit_from(irq_bit, &status, ...)
> mtk_pcie_msi_handler
> generic_handle_domain_irq
> writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
Thanks for mention that. In the hardware corresponding to pcie-
mediatek-gen3.c, the interrupt status in PCIE_INT_STATUS_REG cannot be
cleared if the MSI status remaining in the register of msi_set, so we
have to clear it after handling the MSI.
I guess the root cause of this patch is the interrupt status can be
cleared even the MSI status still remaining, hence that if there are
some MSIs received while clearing the interrupt status, these MSIs
cannot be serviced.
We will discuss and test internally and update the results later,
thanks for your review.
Thanks.
>
> Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
2022-01-28 7:58 ` Jianjun Wang
@ 2022-02-08 7:08 ` qizhong.cheng
-1 siblings, 0 replies; 37+ messages in thread
From: qizhong.cheng @ 2022-02-08 7:08 UTC (permalink / raw)
To: Jianjun Wang, Bjorn Helgaas
Cc: Marc Zyngier, Ryder Lee, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár, qizhong cheng
On Fri, 2022-01-28 at 15:58 +0800, Jianjun Wang wrote:
> Hi Bjorn,
>
> On Thu, 2022-01-27 at 15:21 -0600, Bjorn Helgaas wrote:
> > [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
> >
> > On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng
> > > > > wrote:
> > > > > > As an edge-triggered interrupts, its interrupt status
> > > > > > should
> > > > > > be cleared before dispatch to the handler of device.
> > > > >
> > > > > I'm not an IRQ expert, but the reasoning that "we should
> > > > > clear
> > > > > the MSI interrupt status before dispatching the handler
> > > > > because
> > > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > > convincing because your code will now look like this:
> > > > >
> > > > > /* Clear the INTx */
> > > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->irq_domain, bit -
> > > > > INTX_SHIFT);
> > > > > ...
> > > > >
> > > > > /* Clear MSI interrupt status */
> > > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > > >
> > > > > You clear interrupt status before dispatching the handler for
> > > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > > interrupts.
> > > > >
> > > > > So it doesn't seem that simply being edge-triggered is the
> > > > > critical factor here.
> > > >
> > > > This is the usual problem with these half-baked
> > > > implementations.
> > > > The signalling to the primary interrupt controller is level, as
> > > > they take a multitude of input and (crucially) latch the MSI
> > > > edges. Effectively, this is an edge-to-level converter, with
> > > > all
> > > > the problems that this creates.
> > > >
> > > > By clearing the status *after* the handling, you lose edges
> > > > that
> > > > have been received and coalesced after the read of the status
> > > > register. By clearing it *before*, you are acknowledging the
> > > > interrupts early, and allowing them to be coalesced
> > > > independently
> > > > of the ones that have been received earlier.
> > > >
> > > > This is however mostly an educated guess. Someone with access
> > > > to
> > > > the TRM should verify this.
> > >
> > > Yes, as Maz said, we save the edge-interrupt status so that it
> > > becomes a level-interrupt. This is similar to an edge-to-level
> > > converter, so we need to clear it *before*. We found this problem
> > > through a lot of experiments and tested this patch.
> >
> > I thought there might be other host controllers with similar
> > design,
> > so I looked at all the other drivers and tried to figure out
> > whether
> > any others had similar problems.
> >
> > The ones below look suspicious to me because they all clear some
> > sort
> > of status register *after* handling an MSI. Can you guys take a
> > look
> > and make sure they are working correctly?
> >
> > keembay_pcie_msi_irq_handler
> > status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> >
> > spear13xx_pcie_irq_handler
> > status = readl(&app_reg->int_sts)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, &app_reg->int_clr)
> >
> > advk_pcie_handle_int
> > isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > advk_pcie_handle_msi
> > advk_readl(pcie, PCIE_MSI_STATUS_REG)
> > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> > generic_handle_irq
> > advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
> >
> > mtk_pcie_irq_handler
> > status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> > for_each_set_bit_from(irq_bit, &status, ...)
> > mtk_pcie_msi_handler
> > generic_handle_domain_irq
> > writel_relaxed(BIT(irq_bit), pcie->base +
> > PCIE_INT_STATUS_REG)
>
> Thanks for mention that. In the hardware corresponding to pcie-
> mediatek-gen3.c, the interrupt status in PCIE_INT_STATUS_REG cannot
> be
> cleared if the MSI status remaining in the register of msi_set, so we
> have to clear it after handling the MSI.
>
> I guess the root cause of this patch is the interrupt status can be
> cleared even the MSI status still remaining, hence that if there are
> some MSIs received while clearing the interrupt status, these MSIs
> cannot be serviced.
>
> We will discuss and test internally and update the results later,
> thanks for your review.
>
> Thanks.
>
> >
> > Bjorn
>
>
Sorry for the late reply. Thanks for your comment. I will update
subject and add commit log in the next version.
The interrupt status can be cleared even the MSI status still
remaining, as an edge-triggered interrupts, its interrupt status should
be cleared before dispatching handler to capture the next interrupt.
The design of MSI hardware block diagram is as follows:
+-----+
| GIC |
+-----+
^
|
+-----------------+
| INT_STATUS |
+-----------------+
^
| (edge-triggered)
+-----------------+
| MSI_STATUS |
+-----------------+
^
|
+-----------------+
| EP send MSI |
+-----------------+
Thanks
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-02-08 7:08 ` qizhong.cheng
0 siblings, 0 replies; 37+ messages in thread
From: qizhong.cheng @ 2022-02-08 7:08 UTC (permalink / raw)
To: Jianjun Wang, Bjorn Helgaas
Cc: Marc Zyngier, Ryder Lee, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár, qizhong cheng
On Fri, 2022-01-28 at 15:58 +0800, Jianjun Wang wrote:
> Hi Bjorn,
>
> On Thu, 2022-01-27 at 15:21 -0600, Bjorn Helgaas wrote:
> > [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
> >
> > On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng
> > > > > wrote:
> > > > > > As an edge-triggered interrupts, its interrupt status
> > > > > > should
> > > > > > be cleared before dispatch to the handler of device.
> > > > >
> > > > > I'm not an IRQ expert, but the reasoning that "we should
> > > > > clear
> > > > > the MSI interrupt status before dispatching the handler
> > > > > because
> > > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > > convincing because your code will now look like this:
> > > > >
> > > > > /* Clear the INTx */
> > > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->irq_domain, bit -
> > > > > INTX_SHIFT);
> > > > > ...
> > > > >
> > > > > /* Clear MSI interrupt status */
> > > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > > >
> > > > > You clear interrupt status before dispatching the handler for
> > > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > > interrupts.
> > > > >
> > > > > So it doesn't seem that simply being edge-triggered is the
> > > > > critical factor here.
> > > >
> > > > This is the usual problem with these half-baked
> > > > implementations.
> > > > The signalling to the primary interrupt controller is level, as
> > > > they take a multitude of input and (crucially) latch the MSI
> > > > edges. Effectively, this is an edge-to-level converter, with
> > > > all
> > > > the problems that this creates.
> > > >
> > > > By clearing the status *after* the handling, you lose edges
> > > > that
> > > > have been received and coalesced after the read of the status
> > > > register. By clearing it *before*, you are acknowledging the
> > > > interrupts early, and allowing them to be coalesced
> > > > independently
> > > > of the ones that have been received earlier.
> > > >
> > > > This is however mostly an educated guess. Someone with access
> > > > to
> > > > the TRM should verify this.
> > >
> > > Yes, as Maz said, we save the edge-interrupt status so that it
> > > becomes a level-interrupt. This is similar to an edge-to-level
> > > converter, so we need to clear it *before*. We found this problem
> > > through a lot of experiments and tested this patch.
> >
> > I thought there might be other host controllers with similar
> > design,
> > so I looked at all the other drivers and tried to figure out
> > whether
> > any others had similar problems.
> >
> > The ones below look suspicious to me because they all clear some
> > sort
> > of status register *after* handling an MSI. Can you guys take a
> > look
> > and make sure they are working correctly?
> >
> > keembay_pcie_msi_irq_handler
> > status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> >
> > spear13xx_pcie_irq_handler
> > status = readl(&app_reg->int_sts)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, &app_reg->int_clr)
> >
> > advk_pcie_handle_int
> > isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > advk_pcie_handle_msi
> > advk_readl(pcie, PCIE_MSI_STATUS_REG)
> > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> > generic_handle_irq
> > advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
> >
> > mtk_pcie_irq_handler
> > status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> > for_each_set_bit_from(irq_bit, &status, ...)
> > mtk_pcie_msi_handler
> > generic_handle_domain_irq
> > writel_relaxed(BIT(irq_bit), pcie->base +
> > PCIE_INT_STATUS_REG)
>
> Thanks for mention that. In the hardware corresponding to pcie-
> mediatek-gen3.c, the interrupt status in PCIE_INT_STATUS_REG cannot
> be
> cleared if the MSI status remaining in the register of msi_set, so we
> have to clear it after handling the MSI.
>
> I guess the root cause of this patch is the interrupt status can be
> cleared even the MSI status still remaining, hence that if there are
> some MSIs received while clearing the interrupt status, these MSIs
> cannot be serviced.
>
> We will discuss and test internally and update the results later,
> thanks for your review.
>
> Thanks.
>
> >
> > Bjorn
>
>
Sorry for the late reply. Thanks for your comment. I will update
subject and add commit log in the next version.
The interrupt status can be cleared even the MSI status still
remaining, as an edge-triggered interrupts, its interrupt status should
be cleared before dispatching handler to capture the next interrupt.
The design of MSI hardware block diagram is as follows:
+-----+
| GIC |
+-----+
^
|
+-----------------+
| INT_STATUS |
+-----------------+
^
| (edge-triggered)
+-----------------+
| MSI_STATUS |
+-----------------+
^
|
+-----------------+
| EP send MSI |
+-----------------+
Thanks
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
2022-01-27 21:21 ` Bjorn Helgaas
(?)
@ 2022-01-28 8:57 ` Marc Zyngier
-1 siblings, 0 replies; 37+ messages in thread
From: Marc Zyngier @ 2022-01-28 8:57 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Thu, 27 Jan 2022 21:21:00 +0000,
Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
>
> On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > > As an edge-triggered interrupts, its interrupt status should
> > > > > be cleared before dispatch to the handler of device.
> > > >
> > > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > > the MSI interrupt status before dispatching the handler because
> > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > convincing because your code will now look like this:
> > > >
> > > > /* Clear the INTx */
> > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
> > > > ...
> > > >
> > > > /* Clear MSI interrupt status */
> > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > >
> > > > You clear interrupt status before dispatching the handler for
> > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > interrupts.
> > > >
> > > > So it doesn't seem that simply being edge-triggered is the
> > > > critical factor here.
> > >
> > > This is the usual problem with these half-baked implementations.
> > > The signalling to the primary interrupt controller is level, as
> > > they take a multitude of input and (crucially) latch the MSI
> > > edges. Effectively, this is an edge-to-level converter, with all
> > > the problems that this creates.
> > >
> > > By clearing the status *after* the handling, you lose edges that
> > > have been received and coalesced after the read of the status
> > > register. By clearing it *before*, you are acknowledging the
> > > interrupts early, and allowing them to be coalesced independently
> > > of the ones that have been received earlier.
> > >
> > > This is however mostly an educated guess. Someone with access to
> > > the TRM should verify this.
> >
> > Yes, as Maz said, we save the edge-interrupt status so that it
> > becomes a level-interrupt. This is similar to an edge-to-level
> > converter, so we need to clear it *before*. We found this problem
> > through a lot of experiments and tested this patch.
>
> I thought there might be other host controllers with similar design,
> so I looked at all the other drivers and tried to figure out whether
> any others had similar problems.
>
> The ones below look suspicious to me because they all clear some sort
> of status register *after* handling an MSI. Can you guys take a look
> and make sure they are working correctly?
>
> keembay_pcie_msi_irq_handler
> status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
>
> spear13xx_pcie_irq_handler
> status = readl(&app_reg->int_sts)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, &app_reg->int_clr)
I think these two are fine.
The top level interrupt is only a level signal that the is something
to process. The only thing that is unclear is what the effect of
writing to that status register if MSIs are pending at that point. A
sane implementation would just ignore the write.
The actual processing is done in dw_handle_msi_irq(), reading the
PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack
the interrupt, one bit at a time, as interrupts are handled (see
dw_pci_bottom_ack). Ack taking place before the handling, it makes it
safe for edge delivery.
>
> advk_pcie_handle_int
> isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> advk_pcie_handle_msi
> advk_readl(pcie, PCIE_MSI_STATUS_REG)
> advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> generic_handle_irq
> advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
Same thing, I guess. It is just that the Ack has been open-coded.
>
> mtk_pcie_irq_handler
> status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> for_each_set_bit_from(irq_bit, &status, ...)
> mtk_pcie_msi_handler
> generic_handle_domain_irq
> writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
Similar thing. The PCIE_MSI_SET_STATUS register is read first, and
then written back in the ack callback.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-28 8:57 ` Marc Zyngier
0 siblings, 0 replies; 37+ messages in thread
From: Marc Zyngier @ 2022-01-28 8:57 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Thu, 27 Jan 2022 21:21:00 +0000,
Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
>
> On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > > As an edge-triggered interrupts, its interrupt status should
> > > > > be cleared before dispatch to the handler of device.
> > > >
> > > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > > the MSI interrupt status before dispatching the handler because
> > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > convincing because your code will now look like this:
> > > >
> > > > /* Clear the INTx */
> > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
> > > > ...
> > > >
> > > > /* Clear MSI interrupt status */
> > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > >
> > > > You clear interrupt status before dispatching the handler for
> > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > interrupts.
> > > >
> > > > So it doesn't seem that simply being edge-triggered is the
> > > > critical factor here.
> > >
> > > This is the usual problem with these half-baked implementations.
> > > The signalling to the primary interrupt controller is level, as
> > > they take a multitude of input and (crucially) latch the MSI
> > > edges. Effectively, this is an edge-to-level converter, with all
> > > the problems that this creates.
> > >
> > > By clearing the status *after* the handling, you lose edges that
> > > have been received and coalesced after the read of the status
> > > register. By clearing it *before*, you are acknowledging the
> > > interrupts early, and allowing them to be coalesced independently
> > > of the ones that have been received earlier.
> > >
> > > This is however mostly an educated guess. Someone with access to
> > > the TRM should verify this.
> >
> > Yes, as Maz said, we save the edge-interrupt status so that it
> > becomes a level-interrupt. This is similar to an edge-to-level
> > converter, so we need to clear it *before*. We found this problem
> > through a lot of experiments and tested this patch.
>
> I thought there might be other host controllers with similar design,
> so I looked at all the other drivers and tried to figure out whether
> any others had similar problems.
>
> The ones below look suspicious to me because they all clear some sort
> of status register *after* handling an MSI. Can you guys take a look
> and make sure they are working correctly?
>
> keembay_pcie_msi_irq_handler
> status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
>
> spear13xx_pcie_irq_handler
> status = readl(&app_reg->int_sts)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, &app_reg->int_clr)
I think these two are fine.
The top level interrupt is only a level signal that the is something
to process. The only thing that is unclear is what the effect of
writing to that status register if MSIs are pending at that point. A
sane implementation would just ignore the write.
The actual processing is done in dw_handle_msi_irq(), reading the
PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack
the interrupt, one bit at a time, as interrupts are handled (see
dw_pci_bottom_ack). Ack taking place before the handling, it makes it
safe for edge delivery.
>
> advk_pcie_handle_int
> isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> advk_pcie_handle_msi
> advk_readl(pcie, PCIE_MSI_STATUS_REG)
> advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> generic_handle_irq
> advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
Same thing, I guess. It is just that the Ack has been open-coded.
>
> mtk_pcie_irq_handler
> status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> for_each_set_bit_from(irq_bit, &status, ...)
> mtk_pcie_msi_handler
> generic_handle_domain_irq
> writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
Similar thing. The PCIE_MSI_SET_STATUS register is read first, and
then written back in the ack callback.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-28 8:57 ` Marc Zyngier
0 siblings, 0 replies; 37+ messages in thread
From: Marc Zyngier @ 2022-01-28 8:57 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Thu, 27 Jan 2022 21:21:00 +0000,
Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun]
>
> On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > > As an edge-triggered interrupts, its interrupt status should
> > > > > be cleared before dispatch to the handler of device.
> > > >
> > > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > > the MSI interrupt status before dispatching the handler because
> > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > convincing because your code will now look like this:
> > > >
> > > > /* Clear the INTx */
> > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
> > > > ...
> > > >
> > > > /* Clear MSI interrupt status */
> > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > >
> > > > You clear interrupt status before dispatching the handler for
> > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > interrupts.
> > > >
> > > > So it doesn't seem that simply being edge-triggered is the
> > > > critical factor here.
> > >
> > > This is the usual problem with these half-baked implementations.
> > > The signalling to the primary interrupt controller is level, as
> > > they take a multitude of input and (crucially) latch the MSI
> > > edges. Effectively, this is an edge-to-level converter, with all
> > > the problems that this creates.
> > >
> > > By clearing the status *after* the handling, you lose edges that
> > > have been received and coalesced after the read of the status
> > > register. By clearing it *before*, you are acknowledging the
> > > interrupts early, and allowing them to be coalesced independently
> > > of the ones that have been received earlier.
> > >
> > > This is however mostly an educated guess. Someone with access to
> > > the TRM should verify this.
> >
> > Yes, as Maz said, we save the edge-interrupt status so that it
> > becomes a level-interrupt. This is similar to an edge-to-level
> > converter, so we need to clear it *before*. We found this problem
> > through a lot of experiments and tested this patch.
>
> I thought there might be other host controllers with similar design,
> so I looked at all the other drivers and tried to figure out whether
> any others had similar problems.
>
> The ones below look suspicious to me because they all clear some sort
> of status register *after* handling an MSI. Can you guys take a look
> and make sure they are working correctly?
>
> keembay_pcie_msi_irq_handler
> status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
>
> spear13xx_pcie_irq_handler
> status = readl(&app_reg->int_sts)
> if (status & MSI_CTRL_INT)
> dw_handle_msi_irq
> generic_handle_domain_irq
> writel(status, &app_reg->int_clr)
I think these two are fine.
The top level interrupt is only a level signal that the is something
to process. The only thing that is unclear is what the effect of
writing to that status register if MSIs are pending at that point. A
sane implementation would just ignore the write.
The actual processing is done in dw_handle_msi_irq(), reading the
PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack
the interrupt, one bit at a time, as interrupts are handled (see
dw_pci_bottom_ack). Ack taking place before the handling, it makes it
safe for edge delivery.
>
> advk_pcie_handle_int
> isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> advk_pcie_handle_msi
> advk_readl(pcie, PCIE_MSI_STATUS_REG)
> advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> generic_handle_irq
> advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
Same thing, I guess. It is just that the Ack has been open-coded.
>
> mtk_pcie_irq_handler
> status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> for_each_set_bit_from(irq_bit, &status, ...)
> mtk_pcie_msi_handler
> generic_handle_domain_irq
> writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
Similar thing. The PCIE_MSI_SET_STATUS register is read first, and
then written back in the ack callback.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
2022-01-28 8:57 ` Marc Zyngier
(?)
@ 2022-01-28 13:12 ` Bjorn Helgaas
-1 siblings, 0 replies; 37+ messages in thread
From: Bjorn Helgaas @ 2022-01-28 13:12 UTC (permalink / raw)
To: Marc Zyngier
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Fri, Jan 28, 2022 at 08:57:16AM +0000, Marc Zyngier wrote:
> On Thu, 27 Jan 2022 21:21:00 +0000,
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > > > As an edge-triggered interrupts, its interrupt status should
> > > > > > be cleared before dispatch to the handler of device.
> > > > >
> > > > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > > > the MSI interrupt status before dispatching the handler because
> > > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > > convincing because your code will now look like this:
> > > > >
> > > > > /* Clear the INTx */
> > > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
> > > > > ...
> > > > >
> > > > > /* Clear MSI interrupt status */
> > > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > > >
> > > > > You clear interrupt status before dispatching the handler for
> > > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > > interrupts.
> > > > >
> > > > > So it doesn't seem that simply being edge-triggered is the
> > > > > critical factor here.
> > > >
> > > > This is the usual problem with these half-baked implementations.
> > > > The signalling to the primary interrupt controller is level, as
> > > > they take a multitude of input and (crucially) latch the MSI
> > > > edges. Effectively, this is an edge-to-level converter, with all
> > > > the problems that this creates.
> > > >
> > > > By clearing the status *after* the handling, you lose edges that
> > > > have been received and coalesced after the read of the status
> > > > register. By clearing it *before*, you are acknowledging the
> > > > interrupts early, and allowing them to be coalesced independently
> > > > of the ones that have been received earlier.
> > > >
> > > > This is however mostly an educated guess. Someone with access to
> > > > the TRM should verify this.
> > >
> > > Yes, as Maz said, we save the edge-interrupt status so that it
> > > becomes a level-interrupt. This is similar to an edge-to-level
> > > converter, so we need to clear it *before*. We found this problem
> > > through a lot of experiments and tested this patch.
> >
> > I thought there might be other host controllers with similar design,
> > so I looked at all the other drivers and tried to figure out whether
> > any others had similar problems.
> >
> > The ones below look suspicious to me because they all clear some sort
> > of status register *after* handling an MSI. Can you guys take a look
> > and make sure they are working correctly?
> >
> > keembay_pcie_msi_irq_handler
> > status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> >
> > spear13xx_pcie_irq_handler
> > status = readl(&app_reg->int_sts)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, &app_reg->int_clr)
>
> I think these two are fine.
>
> The top level interrupt is only a level signal that the is something
> to process. The only thing that is unclear is what the effect of
> writing to that status register if MSIs are pending at that point. A
> sane implementation would just ignore the write.
>
> The actual processing is done in dw_handle_msi_irq(), reading the
> PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack
> the interrupt, one bit at a time, as interrupts are handled (see
> dw_pci_bottom_ack). Ack taking place before the handling, it makes it
> safe for edge delivery.
>
> > advk_pcie_handle_int
> > isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > advk_pcie_handle_msi
> > advk_readl(pcie, PCIE_MSI_STATUS_REG)
> > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> > generic_handle_irq
> > advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
>
> Same thing, I guess. It is just that the Ack has been open-coded.
>
> > mtk_pcie_irq_handler
> > status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> > for_each_set_bit_from(irq_bit, &status, ...)
> > mtk_pcie_msi_handler
> > generic_handle_domain_irq
> > writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
>
> Similar thing. The PCIE_MSI_SET_STATUS register is read first, and
> then written back in the ack callback.
Thanks a lot for taking a look at these, Marc! Is there anything we
can do to make all these drivers/pci/controller/* drivers more
consistent and easier to review? I found it very difficult to look
across all of them and find similar design patterns.
Bjorn
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-28 13:12 ` Bjorn Helgaas
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Helgaas @ 2022-01-28 13:12 UTC (permalink / raw)
To: Marc Zyngier
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Fri, Jan 28, 2022 at 08:57:16AM +0000, Marc Zyngier wrote:
> On Thu, 27 Jan 2022 21:21:00 +0000,
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > > > As an edge-triggered interrupts, its interrupt status should
> > > > > > be cleared before dispatch to the handler of device.
> > > > >
> > > > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > > > the MSI interrupt status before dispatching the handler because
> > > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > > convincing because your code will now look like this:
> > > > >
> > > > > /* Clear the INTx */
> > > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
> > > > > ...
> > > > >
> > > > > /* Clear MSI interrupt status */
> > > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > > >
> > > > > You clear interrupt status before dispatching the handler for
> > > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > > interrupts.
> > > > >
> > > > > So it doesn't seem that simply being edge-triggered is the
> > > > > critical factor here.
> > > >
> > > > This is the usual problem with these half-baked implementations.
> > > > The signalling to the primary interrupt controller is level, as
> > > > they take a multitude of input and (crucially) latch the MSI
> > > > edges. Effectively, this is an edge-to-level converter, with all
> > > > the problems that this creates.
> > > >
> > > > By clearing the status *after* the handling, you lose edges that
> > > > have been received and coalesced after the read of the status
> > > > register. By clearing it *before*, you are acknowledging the
> > > > interrupts early, and allowing them to be coalesced independently
> > > > of the ones that have been received earlier.
> > > >
> > > > This is however mostly an educated guess. Someone with access to
> > > > the TRM should verify this.
> > >
> > > Yes, as Maz said, we save the edge-interrupt status so that it
> > > becomes a level-interrupt. This is similar to an edge-to-level
> > > converter, so we need to clear it *before*. We found this problem
> > > through a lot of experiments and tested this patch.
> >
> > I thought there might be other host controllers with similar design,
> > so I looked at all the other drivers and tried to figure out whether
> > any others had similar problems.
> >
> > The ones below look suspicious to me because they all clear some sort
> > of status register *after* handling an MSI. Can you guys take a look
> > and make sure they are working correctly?
> >
> > keembay_pcie_msi_irq_handler
> > status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> >
> > spear13xx_pcie_irq_handler
> > status = readl(&app_reg->int_sts)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, &app_reg->int_clr)
>
> I think these two are fine.
>
> The top level interrupt is only a level signal that the is something
> to process. The only thing that is unclear is what the effect of
> writing to that status register if MSIs are pending at that point. A
> sane implementation would just ignore the write.
>
> The actual processing is done in dw_handle_msi_irq(), reading the
> PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack
> the interrupt, one bit at a time, as interrupts are handled (see
> dw_pci_bottom_ack). Ack taking place before the handling, it makes it
> safe for edge delivery.
>
> > advk_pcie_handle_int
> > isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > advk_pcie_handle_msi
> > advk_readl(pcie, PCIE_MSI_STATUS_REG)
> > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> > generic_handle_irq
> > advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
>
> Same thing, I guess. It is just that the Ack has been open-coded.
>
> > mtk_pcie_irq_handler
> > status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> > for_each_set_bit_from(irq_bit, &status, ...)
> > mtk_pcie_msi_handler
> > generic_handle_domain_irq
> > writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
>
> Similar thing. The PCIE_MSI_SET_STATUS register is read first, and
> then written back in the ack callback.
Thanks a lot for taking a look at these, Marc! Is there anything we
can do to make all these drivers/pci/controller/* drivers more
consistent and easier to review? I found it very difficult to look
across all of them and find similar design patterns.
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-28 13:12 ` Bjorn Helgaas
0 siblings, 0 replies; 37+ messages in thread
From: Bjorn Helgaas @ 2022-01-28 13:12 UTC (permalink / raw)
To: Marc Zyngier
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Fri, Jan 28, 2022 at 08:57:16AM +0000, Marc Zyngier wrote:
> On Thu, 27 Jan 2022 21:21:00 +0000,
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote:
> > > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote:
> > > > On 2022-01-25 16:57, Bjorn Helgaas wrote:
> > > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote:
> > > > > > As an edge-triggered interrupts, its interrupt status should
> > > > > > be cleared before dispatch to the handler of device.
> > > > >
> > > > > I'm not an IRQ expert, but the reasoning that "we should clear
> > > > > the MSI interrupt status before dispatching the handler because
> > > > > MSI is an edge-triggered interrupt" doesn't seem completely
> > > > > convincing because your code will now look like this:
> > > > >
> > > > > /* Clear the INTx */
> > > > > writel(1 << bit, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT);
> > > > > ...
> > > > >
> > > > > /* Clear MSI interrupt status */
> > > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS);
> > > > > generic_handle_domain_irq(port->inner_domain, bit);
> > > > >
> > > > > You clear interrupt status before dispatching the handler for
> > > > > *both* level-triggered INTx interrupts and edge-triggered MSI
> > > > > interrupts.
> > > > >
> > > > > So it doesn't seem that simply being edge-triggered is the
> > > > > critical factor here.
> > > >
> > > > This is the usual problem with these half-baked implementations.
> > > > The signalling to the primary interrupt controller is level, as
> > > > they take a multitude of input and (crucially) latch the MSI
> > > > edges. Effectively, this is an edge-to-level converter, with all
> > > > the problems that this creates.
> > > >
> > > > By clearing the status *after* the handling, you lose edges that
> > > > have been received and coalesced after the read of the status
> > > > register. By clearing it *before*, you are acknowledging the
> > > > interrupts early, and allowing them to be coalesced independently
> > > > of the ones that have been received earlier.
> > > >
> > > > This is however mostly an educated guess. Someone with access to
> > > > the TRM should verify this.
> > >
> > > Yes, as Maz said, we save the edge-interrupt status so that it
> > > becomes a level-interrupt. This is similar to an edge-to-level
> > > converter, so we need to clear it *before*. We found this problem
> > > through a lot of experiments and tested this patch.
> >
> > I thought there might be other host controllers with similar design,
> > so I looked at all the other drivers and tried to figure out whether
> > any others had similar problems.
> >
> > The ones below look suspicious to me because they all clear some sort
> > of status register *after* handling an MSI. Can you guys take a look
> > and make sure they are working correctly?
> >
> > keembay_pcie_msi_irq_handler
> > status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS)
> >
> > spear13xx_pcie_irq_handler
> > status = readl(&app_reg->int_sts)
> > if (status & MSI_CTRL_INT)
> > dw_handle_msi_irq
> > generic_handle_domain_irq
> > writel(status, &app_reg->int_clr)
>
> I think these two are fine.
>
> The top level interrupt is only a level signal that the is something
> to process. The only thing that is unclear is what the effect of
> writing to that status register if MSIs are pending at that point. A
> sane implementation would just ignore the write.
>
> The actual processing is done in dw_handle_msi_irq(), reading the
> PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack
> the interrupt, one bit at a time, as interrupts are handled (see
> dw_pci_bottom_ack). Ack taking place before the handling, it makes it
> safe for edge delivery.
>
> > advk_pcie_handle_int
> > isr0_status = advk_readl(pcie, PCIE_ISR0_REG)
> > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING)
> > advk_pcie_handle_msi
> > advk_readl(pcie, PCIE_MSI_STATUS_REG)
> > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG)
> > generic_handle_irq
> > advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG)
>
> Same thing, I guess. It is just that the Ack has been open-coded.
>
> > mtk_pcie_irq_handler
> > status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG)
> > for_each_set_bit_from(irq_bit, &status, ...)
> > mtk_pcie_msi_handler
> > generic_handle_domain_irq
> > writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG)
>
> Similar thing. The PCIE_MSI_SET_STATUS register is read first, and
> then written back in the ack callback.
Thanks a lot for taking a look at these, Marc! Is there anything we
can do to make all these drivers/pci/controller/* drivers more
consistent and easier to review? I found it very difficult to look
across all of them and find similar design patterns.
Bjorn
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
2022-01-28 13:12 ` Bjorn Helgaas
(?)
@ 2022-01-28 15:09 ` Marc Zyngier
-1 siblings, 0 replies; 37+ messages in thread
From: Marc Zyngier @ 2022-01-28 15:09 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Fri, 28 Jan 2022 13:12:50 +0000,
Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jan 28, 2022 at 08:57:16AM +0000, Marc Zyngier wrote:
> > On Thu, 27 Jan 2022 21:21:00 +0000,
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Thanks a lot for taking a look at these, Marc! Is there anything we
> can do to make all these drivers/pci/controller/* drivers more
> consistent and easier to review? I found it very difficult to look
> across all of them and find similar design patterns.
It looks to me that a number of them are just wrapping the same
underlying IP block, most likely the DW controller (this looks to be
the case for at least the first two).
They probably all use different register and bit offsets, but it
should be possible to write a library abstracting all these details
and have a common handling for most of them. This would certainly go a
long way in making things more solid.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-28 15:09 ` Marc Zyngier
0 siblings, 0 replies; 37+ messages in thread
From: Marc Zyngier @ 2022-01-28 15:09 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Fri, 28 Jan 2022 13:12:50 +0000,
Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jan 28, 2022 at 08:57:16AM +0000, Marc Zyngier wrote:
> > On Thu, 27 Jan 2022 21:21:00 +0000,
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Thanks a lot for taking a look at these, Marc! Is there anything we
> can do to make all these drivers/pci/controller/* drivers more
> consistent and easier to review? I found it very difficult to look
> across all of them and find similar design patterns.
It looks to me that a number of them are just wrapping the same
underlying IP block, most likely the DW controller (this looks to be
the case for at least the first two).
They probably all use different register and bit offsets, but it
should be possible to write a library abstracting all these details
and have a common handling for most of them. This would certainly go a
long way in making things more solid.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence
@ 2022-01-28 15:09 ` Marc Zyngier
0 siblings, 0 replies; 37+ messages in thread
From: Marc Zyngier @ 2022-01-28 15:09 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: qizhong.cheng, Ryder Lee, Jianjun Wang, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, linux-pci,
linux-mediatek, linux-kernel, linux-arm-kernel, chuanjia.liu,
Srikanth Thokala, Pratyush Anand, Thomas Petazzoni,
Pali Rohár
On Fri, 28 Jan 2022 13:12:50 +0000,
Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Jan 28, 2022 at 08:57:16AM +0000, Marc Zyngier wrote:
> > On Thu, 27 Jan 2022 21:21:00 +0000,
> > Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Thanks a lot for taking a look at these, Marc! Is there anything we
> can do to make all these drivers/pci/controller/* drivers more
> consistent and easier to review? I found it very difficult to look
> across all of them and find similar design patterns.
It looks to me that a number of them are just wrapping the same
underlying IP block, most likely the DW controller (this looks to be
the case for at least the first two).
They probably all use different register and bit offsets, but it
should be possible to write a library abstracting all these details
and have a common handling for most of them. This would certainly go a
long way in making things more solid.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 37+ messages in thread