Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bjorn Helgaas]

On Fri, Apr 14, 2017 at 08:34:32PM +0530, 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.
> 
> Signed-off-by: Bharat Kumar Gogada 

Applied to pci/host-xilinx for v4.13.

> ---
>  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(>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(>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(>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(>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, _irq_chip, handle_simple_irq);
> + irq_set_chip_and_handler(irq, _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(>leg_mask_lock);
>   nwl_pcie_init_msi_irq_domain(pcie);
>   return 0;
>  }
> -- 
> 2.1.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bjorn Helgaas]

On Fri, Apr 14, 2017 at 08:34:32PM +0530, 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.
> 
> Signed-off-by: Bharat Kumar Gogada 

Applied to pci/host-xilinx for v4.13.

> ---
>  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(>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(>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(>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(>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, _irq_chip, handle_simple_irq);
> + irq_set_chip_and_handler(irq, _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(>leg_mask_lock);
>   nwl_pcie_init_msi_irq_domain(pcie);
>   return 0;
>  }
> -- 
> 2.1.1
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

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.zyng...@arm.com]
> > > Sent: Thursday, February 09, 2017 9:33 PM
> > > To: Bharat Kumar Gogada <bhara...@xilinx.com>; bhelg...@google.com;
> > > r...@kernel.org; paul.gortma...@windriver.com;
> > > colin.k...@canonical.com; linux-...@vger.kernel.org
> > > Cc: linux-arm-ker...@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; michal.si...@xilinx.com;
> > > a...@arndb.de; Ravikiran Gummaluri <rgum...@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(>intr_tq);
> > > >>> ...
> > > >>> ...
> > > >>> enable_irq(sc->irq);
> > > >>> spin_unlock_bh(>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

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

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.zyng...@arm.com]
> > > Sent: Thursday, February 09, 2017 9:33 PM
> > > To: Bharat Kumar Gogada ; bhelg...@google.com;
> > > r...@kernel.org; paul.gortma...@windriver.com;
> > > colin.k...@canonical.com; linux-...@vger.kernel.org
> > > Cc: linux-arm-ker...@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; michal.si...@xilinx.com;
> > > a...@arndb.de; Ravikiran Gummaluri 
> > > 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(>intr_tq);
> > > >>> ...
> > > >>> ...
> > > >>> enable_irq(sc->irq);
> > > >>> spin_unlock_bh(>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

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

Waiting for Marc's Reply...
 
> > -Original Message-
> > From: Marc Zyngier [mailto:marc.zyng...@arm.com]
> > Sent: Thursday, February 09, 2017 9:33 PM
> > To: Bharat Kumar Gogada <bhara...@xilinx.com>; bhelg...@google.com;
> > r...@kernel.org; paul.gortma...@windriver.com; colin.k...@canonical.com;
> > linux-...@vger.kernel.org
> > Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> > michal.si...@xilinx.com; a...@arndb.de; Ravikiran Gummaluri
> > <rgum...@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(>intr_tq);
> > >>> ...
> > >>> ...
> > >>> enable_irq(sc->irq);
> > >>> spin_unlock_bh(>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

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

Waiting for Marc's Reply...
 
> > -Original Message-
> > From: Marc Zyngier [mailto:marc.zyng...@arm.com]
> > Sent: Thursday, February 09, 2017 9:33 PM
> > To: Bharat Kumar Gogada ; bhelg...@google.com;
> > r...@kernel.org; paul.gortma...@windriver.com; colin.k...@canonical.com;
> > linux-...@vger.kernel.org
> > Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> > michal.si...@xilinx.com; a...@arndb.de; Ravikiran Gummaluri
> > 
> > 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(>intr_tq);
> > >>> ...
> > >>> ...
> > >>> enable_irq(sc->irq);
> > >>> spin_unlock_bh(>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

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

> -Original Message-
> From: Marc Zyngier [mailto:marc.zyng...@arm.com]
> Sent: Thursday, February 09, 2017 9:33 PM
> To: Bharat Kumar Gogada <bhara...@xilinx.com>; bhelg...@google.com;
> r...@kernel.org; paul.gortma...@windriver.com; colin.k...@canonical.com;
> linux-...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> michal.si...@xilinx.com; a...@arndb.de; Ravikiran Gummaluri
> <rgum...@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(>intr_tq);
> >>> ...
> >>> ...
> >>> enable_irq(sc->irq);
> >>> spin_unlock_bh(>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

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

> -Original Message-
> From: Marc Zyngier [mailto:marc.zyng...@arm.com]
> Sent: Thursday, February 09, 2017 9:33 PM
> To: Bharat Kumar Gogada ; bhelg...@google.com;
> r...@kernel.org; paul.gortma...@windriver.com; colin.k...@canonical.com;
> linux-...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> michal.si...@xilinx.com; a...@arndb.de; Ravikiran Gummaluri
> 
> 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(>intr_tq);
> >>> ...
> >>> ...
> >>> enable_irq(sc->irq);
> >>> spin_unlock_bh(>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

Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

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(>intr_tq);
>>> ...
>>> ...
>>> enable_irq(sc->irq);
>>> spin_unlock_bh(>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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

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(>intr_tq);
>>> ...
>>> ...
>>> enable_irq(sc->irq);
>>> spin_unlock_bh(>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.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

> 
> 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(>intr_tq);
> > ...
> > ...
> > enable_irq(sc->irq);
> > spin_unlock_bh(>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

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

> 
> 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(>intr_tq);
> > ...
> > ...
> > enable_irq(sc->irq);
> > spin_unlock_bh(>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

Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

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(>intr_tq);
> ...
> ...
> enable_irq(sc->irq);
> spin_unlock_bh(>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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

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(>intr_tq);
> ...
> ...
> enable_irq(sc->irq);
> spin_unlock_bh(>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.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

> 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 
> > ---
> >  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(>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(>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(>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(>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(>intr_tq);
...
...
enable_irq(sc->irq);
spin_unlock_bh(>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

RE: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

> 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 
> > ---
> >  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(>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(>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(>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(>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(>intr_tq);
...
...
enable_irq(sc->irq);
spin_unlock_bh(>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

Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

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 
> ---
>  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(>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(>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(>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(>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, _irq_chip, handle_simple_irq);
> + irq_set_chip_and_handler(irq, _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(>leg_mask_lock);
>   nwl_pcie_init_msi_irq_domain(pcie);
>   return 0;
>  }
> 

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v5] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

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 
> ---
>  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(>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(>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(>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(>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, _irq_chip, handle_simple_irq);
> + irq_set_chip_and_handler(irq, _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(>leg_mask_lock);
>   nwl_pcie_init_msi_irq_domain(pcie);
>   return 0;
>  }
> 

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...