| Message ID | 1486364596-19699-1-git-send-email-bharatku@xilinx.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 06/02/17 07:03, Bharat Kumar Gogada wrote: > - Adding spinlock for protecting legacy mask register > - Few wifi end points which only support legacy interrupts, > performs hardware reset functionalities after disabling interrupts > by invoking disable_irq and then re-enable using enable_irq, they > enable hardware interrupts first and then virtual irq line later. > - The legacy irq line goes low only after DEASSERT_INTx is > received.As the legacy irq line is high immediately after hardware > interrupts are enabled but virq of EP is still in disabled state > and EP handler is never executed resulting no DEASSERT_INTx.If dummy > irq chip is used, interrutps are not masked and system is > hanging with CPU stall. > - Adding irq chip functions instead of dummy irq chip for legacy > interrupts. > - Legacy interrupts are level sensitive, so using handle_level_irq > is more appropriate as it is masks interrupts until End point handles > interrupts and unmasks interrutps after End point handler is executed. > - Legacy interrupts are level triggered, virtual irq line of End > Point shows as edge in /proc/interrupts. > - Setting irq flags of virtual irq line of EP to level triggered > at the time of mapping. I'm now OK with the code (modulo the small nit below), but the commit message is a complete mess. How about something like: The current handling of legacy interrupts in the Xilinx NWL driver is completely dysfunctional: Interrupts are handled as edge instead of level, and there is no way to mask an interrupt, leading to drivers misbehaving. Let's address this by making it a full blown irqchip, implement mask/unmask methods, and use handle_level_irq as the flow handler. > > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com> > --- > drivers/pci/host/pcie-xilinx-nwl.c | 45 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c > index 43eaa4a..36f4fb4 100644 > --- a/drivers/pci/host/pcie-xilinx-nwl.c > +++ b/drivers/pci/host/pcie-xilinx-nwl.c > @@ -184,6 +184,7 @@ struct nwl_pcie { > u8 root_busno; > struct nwl_msi msi; > struct irq_domain *legacy_irq_domain; > + raw_spinlock_t leg_mask_lock; > }; > > static inline u32 nwl_bridge_readl(struct nwl_pcie *pcie, u32 off) > @@ -395,11 +396,52 @@ static void nwl_pcie_msi_handler_low(struct irq_desc *desc) > chained_irq_exit(chip, desc); > } > > +static void nwl_mask_leg_irq(struct irq_data *data) > +{ > + struct irq_desc *desc = irq_to_desc(data->irq); > + struct nwl_pcie *pcie; > + unsigned long flags; > + u32 mask; > + u32 val; > + > + pcie = irq_desc_get_chip_data(desc); > + mask = 1 << (data->hwirq - 1); > + raw_spin_lock_irqsave(&pcie->leg_mask_lock, flags); > + val = nwl_bridge_readl(pcie, MSGF_LEG_MASK); > + nwl_bridge_writel(pcie, (val & (~mask)), MSGF_LEG_MASK); > + raw_spin_unlock_irqrestore(&pcie->leg_mask_lock, flags); > +} > + > +static void nwl_unmask_leg_irq(struct irq_data *data) > +{ > + struct irq_desc *desc = irq_to_desc(data->irq); > + struct nwl_pcie *pcie; > + unsigned long flags; > + u32 mask; > + u32 val; > + > + pcie = irq_desc_get_chip_data(desc); > + mask = 1 << (data->hwirq - 1); > + raw_spin_lock_irqsave(&pcie->leg_mask_lock, flags); > + val = nwl_bridge_readl(pcie, MSGF_LEG_MASK); > + nwl_bridge_writel(pcie, (val | mask), MSGF_LEG_MASK); > + raw_spin_unlock_irqrestore(&pcie->leg_mask_lock, flags); > +} > + > +static struct irq_chip nwl_leg_irq_chip = { > + .name = "nwl_pcie:legacy", > + .irq_enable = nwl_unmask_leg_irq, > + .irq_disable = nwl_mask_leg_irq, You don't need these two if they are implemented in terms of mask/unmask. > + .irq_mask = nwl_mask_leg_irq, > + .irq_unmask = nwl_unmask_leg_irq, > +}; > + > static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq, > irq_hw_number_t hwirq) > { > - irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); > + irq_set_chip_and_handler(irq, &nwl_leg_irq_chip, handle_level_irq); > irq_set_chip_data(irq, domain->host_data); > + irq_set_status_flags(irq, IRQ_LEVEL); > > return 0; > } > @@ -538,6 +580,7 @@ static int nwl_pcie_init_irq_domain(struct nwl_pcie *pcie) > return -ENOMEM; > } > > + raw_spin_lock_init(&pcie->leg_mask_lock); > nwl_pcie_init_msi_irq_domain(pcie); > return 0; > } > Thanks, M.
> On 06/02/17 07:03, Bharat Kumar Gogada wrote: > > - Adding spinlock for protecting legacy mask register > > - Few wifi end points which only support legacy interrupts, > > performs hardware reset functionalities after disabling interrupts > > by invoking disable_irq and then re-enable using enable_irq, they > > enable hardware interrupts first and then virtual irq line later. > > - The legacy irq line goes low only after DEASSERT_INTx is > > received.As the legacy irq line is high immediately after hardware > > interrupts are enabled but virq of EP is still in disabled state > > and EP handler is never executed resulting no DEASSERT_INTx.If dummy > > irq chip is used, interrutps are not masked and system is > > hanging with CPU stall. > > - Adding irq chip functions instead of dummy irq chip for legacy > > interrupts. > > - Legacy interrupts are level sensitive, so using handle_level_irq > > is more appropriate as it is masks interrupts until End point handles > > interrupts and unmasks interrutps after End point handler is executed. > > - Legacy interrupts are level triggered, virtual irq line of End > > Point shows as edge in /proc/interrupts. > > - Setting irq flags of virtual irq line of EP to level triggered > > at the time of mapping. > > I'm now OK with the code (modulo the small nit below), but the commit > message is a complete mess. How about something like: > > The current handling of legacy interrupts in the Xilinx NWL driver is > completely dysfunctional: Interrupts are handled as edge instead of > level, and there is no way to mask an interrupt, leading to drivers > misbehaving. > > Let's address this by making it a full blown irqchip, implement > mask/unmask methods, and use handle_level_irq as the flow handler. Yes, will change the commit message. > > > > > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com> > > --- > > drivers/pci/host/pcie-xilinx-nwl.c | 45 > +++++++++++++++++++++++++++++++++++++- > > 1 file changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx- > nwl.c > > index 43eaa4a..36f4fb4 100644 > > --- a/drivers/pci/host/pcie-xilinx-nwl.c > > +++ b/drivers/pci/host/pcie-xilinx-nwl.c > > @@ -184,6 +184,7 @@ struct nwl_pcie { > > u8 root_busno; > > struct nwl_msi msi; > > struct irq_domain *legacy_irq_domain; > > + raw_spinlock_t leg_mask_lock; > > }; > > > > static inline u32 nwl_bridge_readl(struct nwl_pcie *pcie, u32 off) > > @@ -395,11 +396,52 @@ static void nwl_pcie_msi_handler_low(struct > irq_desc *desc) > > chained_irq_exit(chip, desc); > > } > > > > +static void nwl_mask_leg_irq(struct irq_data *data) > > +{ > > + struct irq_desc *desc = irq_to_desc(data->irq); > > + struct nwl_pcie *pcie; > > + unsigned long flags; > > + u32 mask; > > + u32 val; > > + > > + pcie = irq_desc_get_chip_data(desc); > > + mask = 1 << (data->hwirq - 1); > > + raw_spin_lock_irqsave(&pcie->leg_mask_lock, flags); > > + val = nwl_bridge_readl(pcie, MSGF_LEG_MASK); > > + nwl_bridge_writel(pcie, (val & (~mask)), MSGF_LEG_MASK); > > + raw_spin_unlock_irqrestore(&pcie->leg_mask_lock, flags); > > +} > > + > > +static void nwl_unmask_leg_irq(struct irq_data *data) > > +{ > > + struct irq_desc *desc = irq_to_desc(data->irq); > > + struct nwl_pcie *pcie; > > + unsigned long flags; > > + u32 mask; > > + u32 val; > > + > > + pcie = irq_desc_get_chip_data(desc); > > + mask = 1 << (data->hwirq - 1); > > + raw_spin_lock_irqsave(&pcie->leg_mask_lock, flags); > > + val = nwl_bridge_readl(pcie, MSGF_LEG_MASK); > > + nwl_bridge_writel(pcie, (val | mask), MSGF_LEG_MASK); > > + raw_spin_unlock_irqrestore(&pcie->leg_mask_lock, flags); > > +} > > + > > +static struct irq_chip nwl_leg_irq_chip = { > > + .name = "nwl_pcie:legacy", > > + .irq_enable = nwl_unmask_leg_irq, > > + .irq_disable = nwl_mask_leg_irq, > > You don't need these two if they are implemented in terms of mask/unmask. These are being invoked by some drivers other than interrupt flow. Ex: drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan) { .... disable_irq(sc->irq); tasklet_disable(&sc->intr_tq); ... ... enable_irq(sc->irq); spin_unlock_bh(&sc->sc_pcu_lock); } For us masking/unmasking is the way to enable/disable interrupts. > > > + .irq_mask = nwl_mask_leg_irq, > > + .irq_unmask = nwl_unmask_leg_irq, > > +}; > > + Thanks & Regards, Bharat
On 09/02/17 12:01, Bharat Kumar Gogada wrote: >> On 06/02/17 07:03, Bharat Kumar Gogada wrote: >>> +static struct irq_chip nwl_leg_irq_chip = { >>> + .name = "nwl_pcie:legacy", >>> + .irq_enable = nwl_unmask_leg_irq, >>> + .irq_disable = nwl_mask_leg_irq, >> >> You don't need these two if they are implemented in terms of mask/unmask. > > These are being invoked by some drivers other than interrupt flow. > Ex: drivers/net/wireless/ath/ath9k/main.c > static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan) > { > .... > disable_irq(sc->irq); > tasklet_disable(&sc->intr_tq); > ... > ... > enable_irq(sc->irq); > spin_unlock_bh(&sc->sc_pcu_lock); > } > For us masking/unmasking is the way to enable/disable interrupts. And if you looked at the way disable_irq is implemented, you would have found out that it falls back to masking if there is no disable method, preserving the semantic you expect. M.
> > On 09/02/17 12:01, Bharat Kumar Gogada wrote: > >> On 06/02/17 07:03, Bharat Kumar Gogada wrote: > >>> +static struct irq_chip nwl_leg_irq_chip = { > >>> + .name = "nwl_pcie:legacy", > >>> + .irq_enable = nwl_unmask_leg_irq, > >>> + .irq_disable = nwl_mask_leg_irq, > >> > >> You don't need these two if they are implemented in terms of mask/unmask. > > > > These are being invoked by some drivers other than interrupt flow. > > Ex: drivers/net/wireless/ath/ath9k/main.c > > static int ath_reset_internal(struct ath_softc *sc, struct > > ath9k_channel *hchan) { > > .... > > disable_irq(sc->irq); > > tasklet_disable(&sc->intr_tq); > > ... > > ... > > enable_irq(sc->irq); > > spin_unlock_bh(&sc->sc_pcu_lock); } For us masking/unmasking > > is the way to enable/disable interrupts. > > And if you looked at the way disable_irq is implemented, you would have found > out that it falls back to masking if there is no disable method, preserving the > semantic you expect. > Yes I did see, but this fall back requires extra "IRQ_DISABLE_UNLAZY" flag to be set to each virq. Bharat
On 09/02/17 15:16, Bharat Kumar Gogada wrote: >> >> On 09/02/17 12:01, Bharat Kumar Gogada wrote: >>>> On 06/02/17 07:03, Bharat Kumar Gogada wrote: >>>>> +static struct irq_chip nwl_leg_irq_chip = { >>>>> + .name = "nwl_pcie:legacy", >>>>> + .irq_enable = nwl_unmask_leg_irq, >>>>> + .irq_disable = nwl_mask_leg_irq, >>>> >>>> You don't need these two if they are implemented in terms of mask/unmask. >>> >>> These are being invoked by some drivers other than interrupt flow. >>> Ex: drivers/net/wireless/ath/ath9k/main.c >>> static int ath_reset_internal(struct ath_softc *sc, struct >>> ath9k_channel *hchan) { >>> .... >>> disable_irq(sc->irq); >>> tasklet_disable(&sc->intr_tq); >>> ... >>> ... >>> enable_irq(sc->irq); >>> spin_unlock_bh(&sc->sc_pcu_lock); } For us masking/unmasking >>> is the way to enable/disable interrupts. >> >> And if you looked at the way disable_irq is implemented, you would have found >> out that it falls back to masking if there is no disable method, preserving the >> semantic you expect. >> > Yes I did see, but this fall back requires extra "IRQ_DISABLE_UNLAZY" flag to be set to each virq. No it doesn't. If you do a disable_irq(), the interrupt is flagged as disabled, but nothing gets done. If an interrupt actually fires, then the interrupts gets masked, and the handler is not called. So just drop these two methods, because if this doesn't work, you have bigger issues. M.
> -----Original Message----- > From: Marc Zyngier [mailto:marc.zyngier@arm.com] > Sent: Thursday, February 09, 2017 9:33 PM > To: Bharat Kumar Gogada <bharatku@xilinx.com>; bhelgaas@google.com; > robh@kernel.org; paul.gortmaker@windriver.com; colin.king@canonical.com; > linux-pci@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > michal.simek@xilinx.com; arnd@arndb.de; Ravikiran Gummaluri > <rgummal@xilinx.com> > Subject: Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts > > On 09/02/17 15:16, Bharat Kumar Gogada wrote: > >> > >> On 09/02/17 12:01, Bharat Kumar Gogada wrote: > >>>> On 06/02/17 07:03, Bharat Kumar Gogada wrote: > >>>>> +static struct irq_chip nwl_leg_irq_chip = { > >>>>> + .name = "nwl_pcie:legacy", > >>>>> + .irq_enable = nwl_unmask_leg_irq, > >>>>> + .irq_disable = nwl_mask_leg_irq, > >>>> > >>>> You don't need these two if they are implemented in terms of > mask/unmask. > >>> > >>> These are being invoked by some drivers other than interrupt flow. > >>> Ex: drivers/net/wireless/ath/ath9k/main.c > >>> static int ath_reset_internal(struct ath_softc *sc, struct > >>> ath9k_channel *hchan) { > >>> .... > >>> disable_irq(sc->irq); > >>> tasklet_disable(&sc->intr_tq); > >>> ... > >>> ... > >>> enable_irq(sc->irq); > >>> spin_unlock_bh(&sc->sc_pcu_lock); } For us masking/unmasking > >>> is the way to enable/disable interrupts. > >> > >> And if you looked at the way disable_irq is implemented, you would > >> have found out that it falls back to masking if there is no disable > >> method, preserving the semantic you expect. > >> > > Yes I did see, but this fall back requires extra "IRQ_DISABLE_UNLAZY" flag to > be set to each virq. > > No it doesn't. If you do a disable_irq(), the interrupt is flagged as disabled, but > nothing gets done. If an interrupt actually fires, then the interrupts gets masked, > and the handler is not called. Yes agreed, this is where the problem comes for us. Here is the scenario Ex:drivers/net/wireless/ath/ath9k/main.c static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan) { .... ath9k_hw_set_interrupts(ah); ath9k_hw_enable_interrupts(ah); ... enable_irq(sc->irq); ... } If you observe this they enable hardware interrupts first and then call enable_irq, at this point of time virq is in disabled state. So, if interrupt is raised in this period of time the handler is never invoked and DEASEERT_INTx will not be seen. As I mentioned in my subject the irq line between bridge and GIC goes low only after it sees DEASSERT_INTx. But since DEASSERT_INTx is never seen line is always high causing cpu stall. So for this kind of EP's we need those two methods. Bharat
Waiting for Marc's Reply... > > -----Original Message----- > > From: Marc Zyngier [mailto:marc.zyngier@arm.com] > > Sent: Thursday, February 09, 2017 9:33 PM > > To: Bharat Kumar Gogada <bharatku@xilinx.com>; bhelgaas@google.com; > > robh@kernel.org; paul.gortmaker@windriver.com; colin.king@canonical.com; > > linux-pci@vger.kernel.org > > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > > michal.simek@xilinx.com; arnd@arndb.de; Ravikiran Gummaluri > > <rgummal@xilinx.com> > > Subject: Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy > interrupts > > > > On 09/02/17 15:16, Bharat Kumar Gogada wrote: > > >> > > >> On 09/02/17 12:01, Bharat Kumar Gogada wrote: > > >>>> On 06/02/17 07:03, Bharat Kumar Gogada wrote: > > >>>>> +static struct irq_chip nwl_leg_irq_chip = { > > >>>>> + .name = "nwl_pcie:legacy", > > >>>>> + .irq_enable = nwl_unmask_leg_irq, > > >>>>> + .irq_disable = nwl_mask_leg_irq, > > >>>> > > >>>> You don't need these two if they are implemented in terms of > > mask/unmask. > > >>> > > >>> These are being invoked by some drivers other than interrupt flow. > > >>> Ex: drivers/net/wireless/ath/ath9k/main.c > > >>> static int ath_reset_internal(struct ath_softc *sc, struct > > >>> ath9k_channel *hchan) { > > >>> .... > > >>> disable_irq(sc->irq); > > >>> tasklet_disable(&sc->intr_tq); > > >>> ... > > >>> ... > > >>> enable_irq(sc->irq); > > >>> spin_unlock_bh(&sc->sc_pcu_lock); } For us masking/unmasking > > >>> is the way to enable/disable interrupts. > > >> > > >> And if you looked at the way disable_irq is implemented, you would > > >> have found out that it falls back to masking if there is no disable > > >> method, preserving the semantic you expect. > > >> > > > Yes I did see, but this fall back requires extra "IRQ_DISABLE_UNLAZY" flag to > > be set to each virq. > > > > No it doesn't. If you do a disable_irq(), the interrupt is flagged as disabled, but > > nothing gets done. If an interrupt actually fires, then the interrupts gets > masked, > > and the handler is not called. > Yes agreed, this is where the problem comes for us. Here is the scenario > Ex:drivers/net/wireless/ath/ath9k/main.c > static int ath_reset_internal(struct ath_softc *sc, struct ath9k_channel *hchan) > { > .... > ath9k_hw_set_interrupts(ah); > ath9k_hw_enable_interrupts(ah); > ... > enable_irq(sc->irq); > ... > } > If you observe this they enable hardware interrupts first and then call enable_irq, > at this point of time > virq is in disabled state. So, if interrupt is raised in this period of time the handler > is never invoked > and DEASEERT_INTx will not be seen. As I mentioned in my subject the irq line > between bridge and > GIC goes low only after it sees DEASSERT_INTx. But since DEASSERT_INTx is > never seen line is always high > causing cpu stall. > So for this kind of EP's we need those two methods. > > Bharat
Hi Marc, can you please look into my last comments ? Regards, Bharat > Subject: RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts > > Waiting for Marc's Reply... > > > > -----Original Message----- > > > From: Marc Zyngier [mailto:marc.zyngier@arm.com] > > > Sent: Thursday, February 09, 2017 9:33 PM > > > To: Bharat Kumar Gogada <bharatku@xilinx.com>; bhelgaas@google.com; > > > robh@kernel.org; paul.gortmaker@windriver.com; > > > colin.king@canonical.com; linux-pci@vger.kernel.org > > > Cc: linux-arm-kernel@lists.infradead.org; > > > linux-kernel@vger.kernel.org; michal.simek@xilinx.com; > > > arnd@arndb.de; Ravikiran Gummaluri <rgummal@xilinx.com> > > > Subject: Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for > > > legacy > > interrupts > > > > > > On 09/02/17 15:16, Bharat Kumar Gogada wrote: > > > >> > > > >> On 09/02/17 12:01, Bharat Kumar Gogada wrote: > > > >>>> On 06/02/17 07:03, Bharat Kumar Gogada wrote: > > > >>>>> +static struct irq_chip nwl_leg_irq_chip = { > > > >>>>> + .name = "nwl_pcie:legacy", > > > >>>>> + .irq_enable = nwl_unmask_leg_irq, > > > >>>>> + .irq_disable = nwl_mask_leg_irq, > > > >>>> > > > >>>> You don't need these two if they are implemented in terms of > > > mask/unmask. > > > >>> > > > >>> These are being invoked by some drivers other than interrupt flow. > > > >>> Ex: drivers/net/wireless/ath/ath9k/main.c > > > >>> static int ath_reset_internal(struct ath_softc *sc, struct > > > >>> ath9k_channel *hchan) { > > > >>> .... > > > >>> disable_irq(sc->irq); > > > >>> tasklet_disable(&sc->intr_tq); > > > >>> ... > > > >>> ... > > > >>> enable_irq(sc->irq); > > > >>> spin_unlock_bh(&sc->sc_pcu_lock); } For us > > > >>> masking/unmasking is the way to enable/disable interrupts. > > > >> > > > >> And if you looked at the way disable_irq is implemented, you > > > >> would have found out that it falls back to masking if there is no > > > >> disable method, preserving the semantic you expect. > > > >> > > > > Yes I did see, but this fall back requires extra > > > > "IRQ_DISABLE_UNLAZY" flag to > > > be set to each virq. > > > > > > No it doesn't. If you do a disable_irq(), the interrupt is flagged > > > as disabled, but nothing gets done. If an interrupt actually fires, > > > then the interrupts gets > > masked, > > > and the handler is not called. > > Yes agreed, this is where the problem comes for us. Here is the > > scenario Ex:drivers/net/wireless/ath/ath9k/main.c > > static int ath_reset_internal(struct ath_softc *sc, struct > > ath9k_channel *hchan) { > > .... > > ath9k_hw_set_interrupts(ah); > > ath9k_hw_enable_interrupts(ah); > > ... > > enable_irq(sc->irq); > > ... > > } > > If you observe this they enable hardware interrupts first and then > > call enable_irq, at this point of time virq is in disabled state. So, > > if interrupt is raised in this period of time the handler is never > > invoked and DEASEERT_INTx will not be seen. As I mentioned in my > > subject the irq line between bridge and GIC goes low only after it > > sees DEASSERT_INTx. But since DEASSERT_INTx is never seen line is > > always high causing cpu stall. > > So for this kind of EP's we need those two methods. > > > > Bharat
diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c index 43eaa4a..36f4fb4 100644 --- a/drivers/pci/host/pcie-xilinx-nwl.c +++ b/drivers/pci/host/pcie-xilinx-nwl.c @@ -184,6 +184,7 @@ struct nwl_pcie { u8 root_busno; struct nwl_msi msi; struct irq_domain *legacy_irq_domain; + raw_spinlock_t leg_mask_lock; }; static inline u32 nwl_bridge_readl(struct nwl_pcie *pcie, u32 off) @@ -395,11 +396,52 @@ static void nwl_pcie_msi_handler_low(struct irq_desc *desc) chained_irq_exit(chip, desc); } +static void nwl_mask_leg_irq(struct irq_data *data) +{ + struct irq_desc *desc = irq_to_desc(data->irq); + struct nwl_pcie *pcie; + unsigned long flags; + u32 mask; + u32 val; + + pcie = irq_desc_get_chip_data(desc); + mask = 1 << (data->hwirq - 1); + raw_spin_lock_irqsave(&pcie->leg_mask_lock, flags); + val = nwl_bridge_readl(pcie, MSGF_LEG_MASK); + nwl_bridge_writel(pcie, (val & (~mask)), MSGF_LEG_MASK); + raw_spin_unlock_irqrestore(&pcie->leg_mask_lock, flags); +} + +static void nwl_unmask_leg_irq(struct irq_data *data) +{ + struct irq_desc *desc = irq_to_desc(data->irq); + struct nwl_pcie *pcie; + unsigned long flags; + u32 mask; + u32 val; + + pcie = irq_desc_get_chip_data(desc); + mask = 1 << (data->hwirq - 1); + raw_spin_lock_irqsave(&pcie->leg_mask_lock, flags); + val = nwl_bridge_readl(pcie, MSGF_LEG_MASK); + nwl_bridge_writel(pcie, (val | mask), MSGF_LEG_MASK); + raw_spin_unlock_irqrestore(&pcie->leg_mask_lock, flags); +} + +static struct irq_chip nwl_leg_irq_chip = { + .name = "nwl_pcie:legacy", + .irq_enable = nwl_unmask_leg_irq, + .irq_disable = nwl_mask_leg_irq, + .irq_mask = nwl_mask_leg_irq, + .irq_unmask = nwl_unmask_leg_irq, +}; + static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq, irq_hw_number_t hwirq) { - irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); + irq_set_chip_and_handler(irq, &nwl_leg_irq_chip, handle_level_irq); irq_set_chip_data(irq, domain->host_data); + irq_set_status_flags(irq, IRQ_LEVEL); return 0; } @@ -538,6 +580,7 @@ static int nwl_pcie_init_irq_domain(struct nwl_pcie *pcie) return -ENOMEM; } + raw_spin_lock_init(&pcie->leg_mask_lock); nwl_pcie_init_msi_irq_domain(pcie); return 0; }
- Adding spinlock for protecting legacy mask register - Few wifi end points which only support legacy interrupts, performs hardware reset functionalities after disabling interrupts by invoking disable_irq and then re-enable using enable_irq, they enable hardware interrupts first and then virtual irq line later. - The legacy irq line goes low only after DEASSERT_INTx is received.As the legacy irq line is high immediately after hardware interrupts are enabled but virq of EP is still in disabled state and EP handler is never executed resulting no DEASSERT_INTx.If dummy irq chip is used, interrutps are not masked and system is hanging with CPU stall. - Adding irq chip functions instead of dummy irq chip for legacy interrupts. - Legacy interrupts are level sensitive, so using handle_level_irq is more appropriate as it is masks interrupts until End point handles interrupts and unmasks interrutps after End point handler is executed. - Legacy interrupts are level triggered, virtual irq line of End Point shows as edge in /proc/interrupts. - Setting irq flags of virtual irq line of EP to level triggered at the time of mapping. Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com> --- drivers/pci/host/pcie-xilinx-nwl.c | 45 +++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-)