[PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

- The legacy status register value for particular INTx becomes low
only after DEASSERT_INTx is received.
- Few End Points take time for sending DEASSERT_INTx, checking
legacy status register in while loop causes invoking of EP
handler continuosly until DEASSERT_INTx is received.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 43eaa4a..c8b5a33 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc)
 
 	chained_irq_enter(chip, desc);
 	pcie = irq_desc_get_handler_data(desc);
+	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
+				  MSGF_LEG_SR_MASKALL;
 
-	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
-				MSGF_LEG_SR_MASKALL) != 0) {
+	if (status != 0) {
 		for_each_set_bit(bit, &status, INTX_NUM) {
 			virq = irq_find_mapping(pcie->legacy_irq_domain,
 						bit + 1);
-- 
1.7.1

[PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

- The legacy status register value for particular INTx becomes low
only after DEASSERT_INTx is received.
- Few End Points take time for sending DEASSERT_INTx, checking
legacy status register in while loop causes invoking of EP
handler continuosly until DEASSERT_INTx is received.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 43eaa4a..c8b5a33 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc)
 
 	chained_irq_enter(chip, desc);
 	pcie = irq_desc_get_handler_data(desc);
+	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
+				  MSGF_LEG_SR_MASKALL;
 
-	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
-				MSGF_LEG_SR_MASKALL) != 0) {
+	if (status != 0) {
 		for_each_set_bit(bit, &status, INTX_NUM) {
 			virq = irq_find_mapping(pcie->legacy_irq_domain,
 						bit + 1);
-- 
1.7.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

- The legacy status register value for particular INTx becomes low
only after DEASSERT_INTx is received.
- Few End Points take time for sending DEASSERT_INTx, checking
legacy status register in while loop causes invoking of EP
handler continuosly until DEASSERT_INTx is received.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 43eaa4a..c8b5a33 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc)
 
 	chained_irq_enter(chip, desc);
 	pcie = irq_desc_get_handler_data(desc);
+	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
+				  MSGF_LEG_SR_MASKALL;
 
-	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
-				MSGF_LEG_SR_MASKALL) != 0) {
+	if (status != 0) {
 		for_each_set_bit(bit, &status, INTX_NUM) {
 			virq = irq_find_mapping(pcie->legacy_irq_domain,
 						bit + 1);
-- 
1.7.1

[PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

- 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.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index c8b5a33..e1809f9 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -396,10 +396,44 @@ 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 int mask = 0;
+
+	pcie = irq_desc_get_chip_data(desc);
+	mask = 1 << (data->hwirq - 1);
+	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
+			  MSGF_LEG_MASK);
+
+}
+
+static void nwl_unmask_leg_irq(struct irq_data *data)
+{
+	struct irq_desc *desc = irq_to_desc(data->irq);
+	struct nwl_pcie *pcie;
+	unsigned int mask = 0;
+
+	pcie = irq_desc_get_chip_data(desc);
+	mask = 1 << (data->hwirq - 1);
+	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
+			  MSGF_LEG_MASK);
+
+}
+
+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_simple_irq);
 	irq_set_chip_data(irq, domain->host_data);
 
 	return 0;
-- 
1.7.1

[PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

- 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.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index c8b5a33..e1809f9 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -396,10 +396,44 @@ 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 int mask = 0;
+
+	pcie = irq_desc_get_chip_data(desc);
+	mask = 1 << (data->hwirq - 1);
+	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
+			  MSGF_LEG_MASK);
+
+}
+
+static void nwl_unmask_leg_irq(struct irq_data *data)
+{
+	struct irq_desc *desc = irq_to_desc(data->irq);
+	struct nwl_pcie *pcie;
+	unsigned int mask = 0;
+
+	pcie = irq_desc_get_chip_data(desc);
+	mask = 1 << (data->hwirq - 1);
+	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
+			  MSGF_LEG_MASK);
+
+}
+
+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_simple_irq);
 	irq_set_chip_data(irq, domain->host_data);
 
 	return 0;
-- 
1.7.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

- 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.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |   36 +++++++++++++++++++++++++++++++++++-
 1 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index c8b5a33..e1809f9 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -396,10 +396,44 @@ 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 int mask = 0;
+
+	pcie = irq_desc_get_chip_data(desc);
+	mask = 1 << (data->hwirq - 1);
+	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
+			  MSGF_LEG_MASK);
+
+}
+
+static void nwl_unmask_leg_irq(struct irq_data *data)
+{
+	struct irq_desc *desc = irq_to_desc(data->irq);
+	struct nwl_pcie *pcie;
+	unsigned int mask = 0;
+
+	pcie = irq_desc_get_chip_data(desc);
+	mask = 1 << (data->hwirq - 1);
+	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
+			  MSGF_LEG_MASK);
+
+}
+
+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_simple_irq);
 	irq_set_chip_data(irq, domain->host_data);
 
 	return 0;
-- 
1.7.1

[PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Bharat Kumar Gogada]

Legacy interrupts are level sensitive, so using handle_level_irq
is more approprate as it is masks interrupts until End point handles
interrupts and unmasks interrutps after End point handler is executed.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index e1809f9..50f9c0d 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data *data)
 static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
 			  irq_hw_number_t hwirq)
 {
-	irq_set_chip_and_handler(irq, &nwl_leg_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);
 
 	return 0;
-- 
1.7.1

[PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Bharat Kumar Gogada]

Legacy interrupts are level sensitive, so using handle_level_irq
is more approprate as it is masks interrupts until End point handles
interrupts and unmasks interrutps after End point handler is executed.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index e1809f9..50f9c0d 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data *data)
 static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
 			  irq_hw_number_t hwirq)
 {
-	irq_set_chip_and_handler(irq, &nwl_leg_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);
 
 	return 0;
-- 
1.7.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Bharat Kumar Gogada]

Legacy interrupts are level sensitive, so using handle_level_irq
is more approprate as it is masks interrupts until End point handles
interrupts and unmasks interrutps after End point handler is executed.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index e1809f9..50f9c0d 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data *data)
 static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
 			  irq_hw_number_t hwirq)
 {
-	irq_set_chip_and_handler(irq, &nwl_leg_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);
 
 	return 0;
-- 
1.7.1

[PATCH 4/4] PCI: Xilinx NWL: Fix, proc interrupts for legacy virtual irq shown as edge

[Bharat Kumar Gogada]

- Legacy interrupts are level triggered, virtual irq line of End
Point shows as edge.
- 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 |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 50f9c0d..b7aa6f8 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -435,6 +435,7 @@ static int nwl_legacy_map(struct irq_domain *domain, unsigned int 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;
 }
-- 
1.7.1

[PATCH 4/4] PCI: Xilinx NWL: Fix, proc interrupts for legacy virtual irq shown as edge

[Bharat Kumar Gogada]

- Legacy interrupts are level triggered, virtual irq line of End
Point shows as edge.
- 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 |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 50f9c0d..b7aa6f8 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -435,6 +435,7 @@ static int nwl_legacy_map(struct irq_domain *domain, unsigned int 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;
 }
-- 
1.7.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] PCI: Xilinx NWL: Fix, proc interrupts for legacy virtual irq shown as edge

[Bharat Kumar Gogada]

- Legacy interrupts are level triggered, virtual irq line of End
Point shows as edge.
- 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 |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 50f9c0d..b7aa6f8 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -435,6 +435,7 @@ static int nwl_legacy_map(struct irq_domain *domain, unsigned int 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;
 }
-- 
1.7.1

Re: [PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> - The legacy status register value for particular INTx becomes low
> only after DEASSERT_INTx is received.
> - Few End Points take time for sending DEASSERT_INTx, checking
> legacy status register in while loop causes invoking of EP
> handler continuosly until DEASSERT_INTx is received.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index 43eaa4a..c8b5a33 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc)
>  
>  	chained_irq_enter(chip, desc);
>  	pcie = irq_desc_get_handler_data(desc);
> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> +				  MSGF_LEG_SR_MASKALL;
>  
> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> -				MSGF_LEG_SR_MASKALL) != 0) {
> +	if (status != 0) {
>  		for_each_set_bit(bit, &status, INTX_NUM) {
>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
>  						bit + 1);
> 

But even if you only handle the interrupt once, it is still asserted,
right? You exit the low-level exception handler, only to take the
interrupt immediately again. So what is the gain here?

As an aside, please add a cover letter to your patch series. It is
immensely useful as a summary of what is being done, as well as an
anchor for the patches themselves.

Thanks,

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

Re: [PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> - The legacy status register value for particular INTx becomes low
> only after DEASSERT_INTx is received.
> - Few End Points take time for sending DEASSERT_INTx, checking
> legacy status register in while loop causes invoking of EP
> handler continuosly until DEASSERT_INTx is received.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index 43eaa4a..c8b5a33 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc)
>  
>  	chained_irq_enter(chip, desc);
>  	pcie = irq_desc_get_handler_data(desc);
> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> +				  MSGF_LEG_SR_MASKALL;
>  
> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> -				MSGF_LEG_SR_MASKALL) != 0) {
> +	if (status != 0) {
>  		for_each_set_bit(bit, &status, INTX_NUM) {
>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
>  						bit + 1);
> 

But even if you only handle the interrupt once, it is still asserted,
right? You exit the low-level exception handler, only to take the
interrupt immediately again. So what is the gain here?

As an aside, please add a cover letter to your patch series. It is
immensely useful as a summary of what is being done, as well as an
anchor for the patches themselves.

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> - The legacy status register value for particular INTx becomes low
> only after DEASSERT_INTx is received.
> - Few End Points take time for sending DEASSERT_INTx, checking
> legacy status register in while loop causes invoking of EP
> handler continuosly until DEASSERT_INTx is received.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index 43eaa4a..c8b5a33 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc *desc)
>  
>  	chained_irq_enter(chip, desc);
>  	pcie = irq_desc_get_handler_data(desc);
> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> +				  MSGF_LEG_SR_MASKALL;
>  
> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> -				MSGF_LEG_SR_MASKALL) != 0) {
> +	if (status != 0) {
>  		for_each_set_bit(bit, &status, INTX_NUM) {
>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
>  						bit + 1);
> 

But even if you only handle the interrupt once, it is still asserted,
right? You exit the low-level exception handler, only to take the
interrupt immediately again. So what is the gain here?

As an aside, please add a cover letter to your patch series. It is
immensely useful as a summary of what is being done, as well as an
anchor for the patches themselves.

Thanks,

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

Re: [PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> - 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.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |   36 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 35 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index c8b5a33..e1809f9 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -396,10 +396,44 @@ 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 int mask = 0;

No need for this initialization. And if the function you're passing that
to takes a u32, why isn't that a u32 too?

> +
> +	pcie = irq_desc_get_chip_data(desc);
> +	mask = 1 << (data->hwirq - 1);
> +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
> +			  MSGF_LEG_MASK);

Erm. This looks completely bogus. Let's say I mask INTA:

	mask = 1 << 0;
	nwl_bridge_writel(pcie, INTD|INTC|INTB, ...);

Now, in a separate context, I decide to mask INTB:

	mask = 1 << 1;
	nwl_bridge_writel(pcie, INTD|INTC|INTA, ...);

unmasking INTA in the process. Probably not what you intended.

> +
> +}
> +
> +static void nwl_unmask_leg_irq(struct irq_data *data)
> +{
> +	struct irq_desc *desc = irq_to_desc(data->irq);
> +	struct nwl_pcie *pcie;
> +	unsigned int mask = 0;
> +
> +	pcie = irq_desc_get_chip_data(desc);
> +	mask = 1 << (data->hwirq - 1);
> +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
> +			  MSGF_LEG_MASK);

Same issue.

> +
> +}
> +
> +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_simple_irq);
>  	irq_set_chip_data(irq, domain->host_data);
>  
>  	return 0;
> 

Thanks,

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

Re: [PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> - 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.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |   36 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 35 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index c8b5a33..e1809f9 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -396,10 +396,44 @@ 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 int mask = 0;

No need for this initialization. And if the function you're passing that
to takes a u32, why isn't that a u32 too?

> +
> +	pcie = irq_desc_get_chip_data(desc);
> +	mask = 1 << (data->hwirq - 1);
> +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
> +			  MSGF_LEG_MASK);

Erm. This looks completely bogus. Let's say I mask INTA:

	mask = 1 << 0;
	nwl_bridge_writel(pcie, INTD|INTC|INTB, ...);

Now, in a separate context, I decide to mask INTB:

	mask = 1 << 1;
	nwl_bridge_writel(pcie, INTD|INTC|INTA, ...);

unmasking INTA in the process. Probably not what you intended.

> +
> +}
> +
> +static void nwl_unmask_leg_irq(struct irq_data *data)
> +{
> +	struct irq_desc *desc = irq_to_desc(data->irq);
> +	struct nwl_pcie *pcie;
> +	unsigned int mask = 0;
> +
> +	pcie = irq_desc_get_chip_data(desc);
> +	mask = 1 << (data->hwirq - 1);
> +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
> +			  MSGF_LEG_MASK);

Same issue.

> +
> +}
> +
> +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_simple_irq);
>  	irq_set_chip_data(irq, domain->host_data);
>  
>  	return 0;
> 

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> - 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.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |   36 +++++++++++++++++++++++++++++++++++-
>  1 files changed, 35 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index c8b5a33..e1809f9 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -396,10 +396,44 @@ 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 int mask = 0;

No need for this initialization. And if the function you're passing that
to takes a u32, why isn't that a u32 too?

> +
> +	pcie = irq_desc_get_chip_data(desc);
> +	mask = 1 << (data->hwirq - 1);
> +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
> +			  MSGF_LEG_MASK);

Erm. This looks completely bogus. Let's say I mask INTA:

	mask = 1 << 0;
	nwl_bridge_writel(pcie, INTD|INTC|INTB, ...);

Now, in a separate context, I decide to mask INTB:

	mask = 1 << 1;
	nwl_bridge_writel(pcie, INTD|INTC|INTA, ...);

unmasking INTA in the process. Probably not what you intended.

> +
> +}
> +
> +static void nwl_unmask_leg_irq(struct irq_data *data)
> +{
> +	struct irq_desc *desc = irq_to_desc(data->irq);
> +	struct nwl_pcie *pcie;
> +	unsigned int mask = 0;
> +
> +	pcie = irq_desc_get_chip_data(desc);
> +	mask = 1 << (data->hwirq - 1);
> +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
> +			  MSGF_LEG_MASK);

Same issue.

> +
> +}
> +
> +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_simple_irq);
>  	irq_set_chip_data(irq, domain->host_data);
>  
>  	return 0;
> 

Thanks,

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

Re: [PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> Legacy interrupts are level sensitive, so using handle_level_irq
> is more approprate as it is masks interrupts until End point handles
> interrupts and unmasks interrutps after End point handler is executed.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index e1809f9..50f9c0d 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data *data)
>  static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
>  			  irq_hw_number_t hwirq)
>  {
> -	irq_set_chip_and_handler(irq, &nwl_leg_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);
>  
>  	return 0;
> 

Please merge this patch and the following one in patch #2. There is no
need for going through equally broken intermediate steps.

Thanks,

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

Re: [PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> Legacy interrupts are level sensitive, so using handle_level_irq
> is more approprate as it is masks interrupts until End point handles
> interrupts and unmasks interrutps after End point handler is executed.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index e1809f9..50f9c0d 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data *data)
>  static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
>  			  irq_hw_number_t hwirq)
>  {
> -	irq_set_chip_and_handler(irq, &nwl_leg_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);
>  
>  	return 0;
> 

Please merge this patch and the following one in patch #2. There is no
need for going through equally broken intermediate steps.

Thanks,

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Marc Zyngier]

On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> Legacy interrupts are level sensitive, so using handle_level_irq
> is more approprate as it is masks interrupts until End point handles
> interrupts and unmasks interrutps after End point handler is executed.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> ---
>  drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
> index e1809f9..50f9c0d 100644
> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> @@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data *data)
>  static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
>  			  irq_hw_number_t hwirq)
>  {
> -	irq_set_chip_and_handler(irq, &nwl_leg_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);
>  
>  	return 0;
> 

Please merge this patch and the following one in patch #2. There is no
need for going through equally broken intermediate steps.

Thanks,

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

RE: [PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > - The legacy status register value for particular INTx becomes low
> > only after DEASSERT_INTx is received.
> > - Few End Points take time for sending DEASSERT_INTx, checking legacy
> > status register in while loop causes invoking of EP handler
> > continuosly until DEASSERT_INTx is received.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > index 43eaa4a..c8b5a33 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc
> > *desc)
> >
> >  	chained_irq_enter(chip, desc);
> >  	pcie = irq_desc_get_handler_data(desc);
> > +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> > +				  MSGF_LEG_SR_MASKALL;
> >
> > -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> > -				MSGF_LEG_SR_MASKALL) != 0) {
> > +	if (status != 0) {
> >  		for_each_set_bit(bit, &status, INTX_NUM) {
> >  			virq = irq_find_mapping(pcie->legacy_irq_domain,
> >  						bit + 1);
> >
> 
> But even if you only handle the interrupt once, it is still asserted, right? You exit
> the low-level exception handler, only to take the interrupt immediately again. So
> what is the gain here?
> 
Whenever masking of INTx happens (like as in my next patch[PATCH 2/4]), the irq line goes low,
but  status bit will be set until DEASSERT_INTx comes.
In this scenario if it is always in while loop, even though line went low it will not be seen 
until DEASSERT_INTx, unnecessarily invoking EP driver. so instead of while loop  
using if condition so that this change in line is noticed.
> As an aside, please add a cover letter to your patch series. It is immensely useful
> as a summary of what is being done, as well as an anchor for the patches
> themselves.
> 
Agreed, will do next time. 

Thanks & Regards,
Bharat

RE: [PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > - The legacy status register value for particular INTx becomes low
> > only after DEASSERT_INTx is received.
> > - Few End Points take time for sending DEASSERT_INTx, checking legacy
> > status register in while loop causes invoking of EP handler
> > continuosly until DEASSERT_INTx is received.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > index 43eaa4a..c8b5a33 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc
> > *desc)
> >
> >  	chained_irq_enter(chip, desc);
> >  	pcie =3D irq_desc_get_handler_data(desc);
> > +	status =3D nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> > +				  MSGF_LEG_SR_MASKALL;
> >
> > -	while ((status =3D nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> > -				MSGF_LEG_SR_MASKALL) !=3D 0) {
> > +	if (status !=3D 0) {
> >  		for_each_set_bit(bit, &status, INTX_NUM) {
> >  			virq =3D irq_find_mapping(pcie->legacy_irq_domain,
> >  						bit + 1);
> >
>=20
> But even if you only handle the interrupt once, it is still asserted, rig=
ht? You exit
> the low-level exception handler, only to take the interrupt immediately a=
gain. So
> what is the gain here?
>=20
Whenever masking of INTx happens (like as in my next patch[PATCH 2/4]), the=
 irq line goes low,
but  status bit will be set until DEASSERT_INTx comes.
In this scenario if it is always in while loop, even though line went low i=
t will not be seen=20
until DEASSERT_INTx, unnecessarily invoking EP driver. so instead of while =
loop =20
using if condition so that this change in line is noticed.
> As an aside, please add a cover letter to your patch series. It is immens=
ely useful
> as a summary of what is being done, as well as an anchor for the patches
> themselves.
>=20
Agreed, will do next time.=20

Thanks & Regards,
Bharat

[PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > - The legacy status register value for particular INTx becomes low
> > only after DEASSERT_INTx is received.
> > - Few End Points take time for sending DEASSERT_INTx, checking legacy
> > status register in while loop causes invoking of EP handler
> > continuosly until DEASSERT_INTx is received.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > index 43eaa4a..c8b5a33 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc
> > *desc)
> >
> >  	chained_irq_enter(chip, desc);
> >  	pcie = irq_desc_get_handler_data(desc);
> > +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> > +				  MSGF_LEG_SR_MASKALL;
> >
> > -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> > -				MSGF_LEG_SR_MASKALL) != 0) {
> > +	if (status != 0) {
> >  		for_each_set_bit(bit, &status, INTX_NUM) {
> >  			virq = irq_find_mapping(pcie->legacy_irq_domain,
> >  						bit + 1);
> >
> 
> But even if you only handle the interrupt once, it is still asserted, right? You exit
> the low-level exception handler, only to take the interrupt immediately again. So
> what is the gain here?
> 
Whenever masking of INTx happens (like as in my next patch[PATCH 2/4]), the irq line goes low,
but  status bit will be set until DEASSERT_INTx comes.
In this scenario if it is always in while loop, even though line went low it will not be seen 
until DEASSERT_INTx, unnecessarily invoking EP driver. so instead of while loop  
using if condition so that this change in line is noticed.
> As an aside, please add a cover letter to your patch series. It is immensely useful
> as a summary of what is being done, as well as an anchor for the patches
> themselves.
> 
Agreed, will do next time. 

Thanks & Regards,
Bharat

RE: [PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

 > On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > - 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.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |   36
> +++++++++++++++++++++++++++++++++++-
> >  1 files changed, 35 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-
> nwl.c
> > index c8b5a33..e1809f9 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -396,10 +396,44 @@ 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 int mask = 0;
> 
> No need for this initialization. And if the function you're passing that
> to takes a u32, why isn't that a u32 too?
> 
> > +
> > +	pcie = irq_desc_get_chip_data(desc);
> > +	mask = 1 << (data->hwirq - 1);
> > +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
> > +			  MSGF_LEG_MASK);
> 
> Erm. This looks completely bogus. Let's say I mask INTA:
> 
> 	mask = 1 << 0;
> 	nwl_bridge_writel(pcie, INTD|INTC|INTB, ...);
> 
> Now, in a separate context, I decide to mask INTB:
> 
> 	mask = 1 << 1;
> 	nwl_bridge_writel(pcie, INTD|INTC|INTA, ...);
> 
> unmasking INTA in the process. Probably not what you intended.
> 
Agreed, will do it in next patch.
> > +
> > +}
> > +
> > +static void nwl_unmask_leg_irq(struct irq_data *data)
> > +{
> > +	struct irq_desc *desc = irq_to_desc(data->irq);
> > +	struct nwl_pcie *pcie;
> > +	unsigned int mask = 0;
> > +
> > +	pcie = irq_desc_get_chip_data(desc);
> > +	mask = 1 << (data->hwirq - 1);
> > +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
> > +			  MSGF_LEG_MASK);
> 
> Same issue.
> 
Agreed, will do it in next patch.

Thanks & Regards,
Bharat

RE: [PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

 > On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > - 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.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |   36
> +++++++++++++++++++++++++++++++++++-
> >  1 files changed, 35 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie=
-xilinx-
> nwl.c
> > index c8b5a33..e1809f9 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -396,10 +396,44 @@ 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 =3D irq_to_desc(data->irq);
> > +	struct nwl_pcie *pcie;
> > +	unsigned int mask =3D 0;
>=20
> No need for this initialization. And if the function you're passing that
> to takes a u32, why isn't that a u32 too?
>=20
> > +
> > +	pcie =3D irq_desc_get_chip_data(desc);
> > +	mask =3D 1 << (data->hwirq - 1);
> > +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
> > +			  MSGF_LEG_MASK);
>=20
> Erm. This looks completely bogus. Let's say I mask INTA:
>=20
> 	mask =3D 1 << 0;
> 	nwl_bridge_writel(pcie, INTD|INTC|INTB, ...);
>=20
> Now, in a separate context, I decide to mask INTB:
>=20
> 	mask =3D 1 << 1;
> 	nwl_bridge_writel(pcie, INTD|INTC|INTA, ...);
>=20
> unmasking INTA in the process. Probably not what you intended.
>=20
Agreed, will do it in next patch.
> > +
> > +}
> > +
> > +static void nwl_unmask_leg_irq(struct irq_data *data)
> > +{
> > +	struct irq_desc *desc =3D irq_to_desc(data->irq);
> > +	struct nwl_pcie *pcie;
> > +	unsigned int mask =3D 0;
> > +
> > +	pcie =3D irq_desc_get_chip_data(desc);
> > +	mask =3D 1 << (data->hwirq - 1);
> > +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
> > +			  MSGF_LEG_MASK);
>=20
> Same issue.
>=20
Agreed, will do it in next patch.

Thanks & Regards,
Bharat

[PATCH 2/4] PCI: Xilinx NWL: Modifying irq chip for legacy interrupts

[Bharat Kumar Gogada]

 > On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > - 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.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |   36
> +++++++++++++++++++++++++++++++++++-
> >  1 files changed, 35 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-
> nwl.c
> > index c8b5a33..e1809f9 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -396,10 +396,44 @@ 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 int mask = 0;
> 
> No need for this initialization. And if the function you're passing that
> to takes a u32, why isn't that a u32 too?
> 
> > +
> > +	pcie = irq_desc_get_chip_data(desc);
> > +	mask = 1 << (data->hwirq - 1);
> > +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL & (~mask)),
> > +			  MSGF_LEG_MASK);
> 
> Erm. This looks completely bogus. Let's say I mask INTA:
> 
> 	mask = 1 << 0;
> 	nwl_bridge_writel(pcie, INTD|INTC|INTB, ...);
> 
> Now, in a separate context, I decide to mask INTB:
> 
> 	mask = 1 << 1;
> 	nwl_bridge_writel(pcie, INTD|INTC|INTA, ...);
> 
> unmasking INTA in the process. Probably not what you intended.
> 
Agreed, will do it in next patch.
> > +
> > +}
> > +
> > +static void nwl_unmask_leg_irq(struct irq_data *data)
> > +{
> > +	struct irq_desc *desc = irq_to_desc(data->irq);
> > +	struct nwl_pcie *pcie;
> > +	unsigned int mask = 0;
> > +
> > +	pcie = irq_desc_get_chip_data(desc);
> > +	mask = 1 << (data->hwirq - 1);
> > +	nwl_bridge_writel(pcie, ((u32)MSGF_LEG_SR_MASKALL | mask),
> > +			  MSGF_LEG_MASK);
> 
> Same issue.
> 
Agreed, will do it in next patch.

Thanks & Regards,
Bharat

RE: [PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Bharat Kumar Gogada]

> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > Legacy interrupts are level sensitive, so using handle_level_irq is
> > more approprate as it is masks interrupts until End point handles
> > interrupts and unmasks interrutps after End point handler is executed.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > index e1809f9..50f9c0d 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data
> > *data)  static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
> >  			  irq_hw_number_t hwirq)
> >  {
> > -	irq_set_chip_and_handler(irq, &nwl_leg_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);
> >
> >  	return 0;
> >
> 
> Please merge this patch and the following one in patch #2. There is no need for
> going through equally broken intermediate steps.
> 
Agreed, will merge it with second patch. 

Thanks & Regards,
Bharat

RE: [PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Bharat Kumar Gogada]

> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > Legacy interrupts are level sensitive, so using handle_level_irq is
> > more approprate as it is masks interrupts until End point handles
> > interrupts and unmasks interrutps after End point handler is executed.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > index e1809f9..50f9c0d 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data
> > *data)  static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
> >  			  irq_hw_number_t hwirq)
> >  {
> > -	irq_set_chip_and_handler(irq, &nwl_leg_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);
> >
> >  	return 0;
> >
> 
> Please merge this patch and the following one in patch #2. There is no need for
> going through equally broken intermediate steps.
> 
Agreed, will merge it with second patch. 

Thanks & Regards,
Bharat

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] PCI: Xilinx NWL: Modifying flow handler for legacy interrupts

[Bharat Kumar Gogada]

> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> > Legacy interrupts are level sensitive, so using handle_level_irq is
> > more approprate as it is masks interrupts until End point handles
> > interrupts and unmasks interrutps after End point handler is executed.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > ---
> >  drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> > b/drivers/pci/host/pcie-xilinx-nwl.c
> > index e1809f9..50f9c0d 100644
> > --- a/drivers/pci/host/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> > @@ -433,7 +433,7 @@ static void nwl_unmask_leg_irq(struct irq_data
> > *data)  static int nwl_legacy_map(struct irq_domain *domain, unsigned int irq,
> >  			  irq_hw_number_t hwirq)
> >  {
> > -	irq_set_chip_and_handler(irq, &nwl_leg_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);
> >
> >  	return 0;
> >
> 
> Please merge this patch and the following one in patch #2. There is no need for
> going through equally broken intermediate steps.
> 
Agreed, will merge it with second patch. 

Thanks & Regards,
Bharat

Re: [PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Marc Zyngier]

On 24/01/17 10:15, Bharat Kumar Gogada wrote:
>> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
>>> - The legacy status register value for particular INTx becomes low
>>> only after DEASSERT_INTx is received.
>>> - Few End Points take time for sending DEASSERT_INTx, checking legacy
>>> status register in while loop causes invoking of EP handler
>>> continuosly until DEASSERT_INTx is received.
>>>
>>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>>> ---
>>>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
>>> b/drivers/pci/host/pcie-xilinx-nwl.c
>>> index 43eaa4a..c8b5a33 100644
>>> --- a/drivers/pci/host/pcie-xilinx-nwl.c
>>> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
>>> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc
>>> *desc)
>>>
>>>  	chained_irq_enter(chip, desc);
>>>  	pcie = irq_desc_get_handler_data(desc);
>>> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
>>> +				  MSGF_LEG_SR_MASKALL;
>>>
>>> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
>>> -				MSGF_LEG_SR_MASKALL) != 0) {
>>> +	if (status != 0) {
>>>  		for_each_set_bit(bit, &status, INTX_NUM) {
>>>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
>>>  						bit + 1);
>>>
>>
>> But even if you only handle the interrupt once, it is still asserted, right? You exit
>> the low-level exception handler, only to take the interrupt immediately again. So
>> what is the gain here?
>>
> Whenever masking of INTx happens (like as in my next patch[PATCH 2/4]), the irq line goes low,
> but  status bit will be set until DEASSERT_INTx comes.
> In this scenario if it is always in while loop, even though line went low it will not be seen 
> until DEASSERT_INTx, unnecessarily invoking EP driver. so instead of while loop  
> using if condition so that this change in line is noticed.

But what guarantees that you will observe this DEASSERT if you return to
the interrupted context early? From what I understand, your interrupt
flow is the following:


	read status
	mask
	handler
	unmask

If the EP takes time to send a deassert after the handler has poked it
and the interrupt is still active, then the only thing that this patch
buys you is that by returning to the interrupted context, you waste a
lot of cycles, giving the device a chance to send its deassert. But
that's just luck. Plug this on a fast CPU, and the same issue will
resurface.

It could also just hide a driver bug where the write acknowledging the
interrupt has been posted, but has not taken effect yet.

Thanks,

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

Re: [PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Marc Zyngier]

On 24/01/17 10:15, Bharat Kumar Gogada wrote:
>> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
>>> - The legacy status register value for particular INTx becomes low
>>> only after DEASSERT_INTx is received.
>>> - Few End Points take time for sending DEASSERT_INTx, checking legacy
>>> status register in while loop causes invoking of EP handler
>>> continuosly until DEASSERT_INTx is received.
>>>
>>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>>> ---
>>>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
>>> b/drivers/pci/host/pcie-xilinx-nwl.c
>>> index 43eaa4a..c8b5a33 100644
>>> --- a/drivers/pci/host/pcie-xilinx-nwl.c
>>> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
>>> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc
>>> *desc)
>>>
>>>  	chained_irq_enter(chip, desc);
>>>  	pcie = irq_desc_get_handler_data(desc);
>>> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
>>> +				  MSGF_LEG_SR_MASKALL;
>>>
>>> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
>>> -				MSGF_LEG_SR_MASKALL) != 0) {
>>> +	if (status != 0) {
>>>  		for_each_set_bit(bit, &status, INTX_NUM) {
>>>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
>>>  						bit + 1);
>>>
>>
>> But even if you only handle the interrupt once, it is still asserted, right? You exit
>> the low-level exception handler, only to take the interrupt immediately again. So
>> what is the gain here?
>>
> Whenever masking of INTx happens (like as in my next patch[PATCH 2/4]), the irq line goes low,
> but  status bit will be set until DEASSERT_INTx comes.
> In this scenario if it is always in while loop, even though line went low it will not be seen 
> until DEASSERT_INTx, unnecessarily invoking EP driver. so instead of while loop  
> using if condition so that this change in line is noticed.

But what guarantees that you will observe this DEASSERT if you return to
the interrupted context early? From what I understand, your interrupt
flow is the following:


	read status
	mask
	handler
	unmask

If the EP takes time to send a deassert after the handler has poked it
and the interrupt is still active, then the only thing that this patch
buys you is that by returning to the interrupted context, you waste a
lot of cycles, giving the device a chance to send its deassert. But
that's just luck. Plug this on a fast CPU, and the same issue will
resurface.

It could also just hide a driver bug where the write acknowledging the
interrupt has been posted, but has not taken effect yet.

Thanks,

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

[PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Marc Zyngier]

On 24/01/17 10:15, Bharat Kumar Gogada wrote:
>> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
>>> - The legacy status register value for particular INTx becomes low
>>> only after DEASSERT_INTx is received.
>>> - Few End Points take time for sending DEASSERT_INTx, checking legacy
>>> status register in while loop causes invoking of EP handler
>>> continuosly until DEASSERT_INTx is received.
>>>
>>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>>> ---
>>>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
>>> b/drivers/pci/host/pcie-xilinx-nwl.c
>>> index 43eaa4a..c8b5a33 100644
>>> --- a/drivers/pci/host/pcie-xilinx-nwl.c
>>> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
>>> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct irq_desc
>>> *desc)
>>>
>>>  	chained_irq_enter(chip, desc);
>>>  	pcie = irq_desc_get_handler_data(desc);
>>> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
>>> +				  MSGF_LEG_SR_MASKALL;
>>>
>>> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
>>> -				MSGF_LEG_SR_MASKALL) != 0) {
>>> +	if (status != 0) {
>>>  		for_each_set_bit(bit, &status, INTX_NUM) {
>>>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
>>>  						bit + 1);
>>>
>>
>> But even if you only handle the interrupt once, it is still asserted, right? You exit
>> the low-level exception handler, only to take the interrupt immediately again. So
>> what is the gain here?
>>
> Whenever masking of INTx happens (like as in my next patch[PATCH 2/4]), the irq line goes low,
> but  status bit will be set until DEASSERT_INTx comes.
> In this scenario if it is always in while loop, even though line went low it will not be seen 
> until DEASSERT_INTx, unnecessarily invoking EP driver. so instead of while loop  
> using if condition so that this change in line is noticed.

But what guarantees that you will observe this DEASSERT if you return to
the interrupted context early? From what I understand, your interrupt
flow is the following:


	read status
	mask
	handler
	unmask

If the EP takes time to send a deassert after the handler has poked it
and the interrupt is still active, then the only thing that this patch
buys you is that by returning to the interrupted context, you waste a
lot of cycles, giving the device a chance to send its deassert. But
that's just luck. Plug this on a fast CPU, and the same issue will
resurface.

It could also just hide a driver bug where the write acknowledging the
interrupt has been posted, but has not taken effect yet.

Thanks,

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

RE: [PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

> On 24/01/17 10:15, Bharat Kumar Gogada wrote:
> >> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> >>> - The legacy status register value for particular INTx becomes low
> >>> only after DEASSERT_INTx is received.
> >>> - Few End Points take time for sending DEASSERT_INTx, checking
> >>> legacy status register in while loop causes invoking of EP handler
> >>> continuosly until DEASSERT_INTx is received.
> >>>
> >>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> >>> ---
> >>>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
> >>>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> >>> b/drivers/pci/host/pcie-xilinx-nwl.c
> >>> index 43eaa4a..c8b5a33 100644
> >>> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> >>> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> >>> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct
> >>> irq_desc
> >>> *desc)
> >>>
> >>>  	chained_irq_enter(chip, desc);
> >>>  	pcie = irq_desc_get_handler_data(desc);
> >>> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> >>> +				  MSGF_LEG_SR_MASKALL;
> >>>
> >>> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> >>> -				MSGF_LEG_SR_MASKALL) != 0) {
> >>> +	if (status != 0) {
> >>>  		for_each_set_bit(bit, &status, INTX_NUM) {
> >>>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
> >>>  						bit + 1);
> >>>
> >>
> >> But even if you only handle the interrupt once, it is still asserted,
> >> right? You exit the low-level exception handler, only to take the
> >> interrupt immediately again. So what is the gain here?
> >>
> > Whenever masking of INTx happens (like as in my next patch[PATCH
> > 2/4]), the irq line goes low, but  status bit will be set until DEASSERT_INTx
> comes.
> > In this scenario if it is always in while loop, even though line went
> > low it will not be seen until DEASSERT_INTx, unnecessarily invoking EP
> > driver. so instead of while loop using if condition so that this change in line is
> noticed.
> 
> But what guarantees that you will observe this DEASSERT if you return to the
> interrupted context early? From what I understand, your interrupt flow is the
> following:
> 
> 
> 	read status
> 	mask
> 	handler
> 	unmask
> 
> If the EP takes time to send a deassert after the handler has poked it and the
> interrupt is still active, then the only thing that this patch buys you is that by
> returning to the interrupted context, you waste a lot of cycles, giving the device
> a chance to send its deassert. But that's just luck. Plug this on a fast CPU, and
> the same issue will resurface.
> 

Agreed, will leave it as it is.

Thanks & Regards,
Bharat


 

RE: [PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

> On 24/01/17 10:15, Bharat Kumar Gogada wrote:
> >> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> >>> - The legacy status register value for particular INTx becomes low
> >>> only after DEASSERT_INTx is received.
> >>> - Few End Points take time for sending DEASSERT_INTx, checking
> >>> legacy status register in while loop causes invoking of EP handler
> >>> continuosly until DEASSERT_INTx is received.
> >>>
> >>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> >>> ---
> >>>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
> >>>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> >>> b/drivers/pci/host/pcie-xilinx-nwl.c
> >>> index 43eaa4a..c8b5a33 100644
> >>> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> >>> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> >>> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct
> >>> irq_desc
> >>> *desc)
> >>>
> >>>  	chained_irq_enter(chip, desc);
> >>>  	pcie = irq_desc_get_handler_data(desc);
> >>> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> >>> +				  MSGF_LEG_SR_MASKALL;
> >>>
> >>> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> >>> -				MSGF_LEG_SR_MASKALL) != 0) {
> >>> +	if (status != 0) {
> >>>  		for_each_set_bit(bit, &status, INTX_NUM) {
> >>>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
> >>>  						bit + 1);
> >>>
> >>
> >> But even if you only handle the interrupt once, it is still asserted,
> >> right? You exit the low-level exception handler, only to take the
> >> interrupt immediately again. So what is the gain here?
> >>
> > Whenever masking of INTx happens (like as in my next patch[PATCH
> > 2/4]), the irq line goes low, but  status bit will be set until DEASSERT_INTx
> comes.
> > In this scenario if it is always in while loop, even though line went
> > low it will not be seen until DEASSERT_INTx, unnecessarily invoking EP
> > driver. so instead of while loop using if condition so that this change in line is
> noticed.
> 
> But what guarantees that you will observe this DEASSERT if you return to the
> interrupted context early? From what I understand, your interrupt flow is the
> following:
> 
> 
> 	read status
> 	mask
> 	handler
> 	unmask
> 
> If the EP takes time to send a deassert after the handler has poked it and the
> interrupt is still active, then the only thing that this patch buys you is that by
> returning to the interrupted context, you waste a lot of cycles, giving the device
> a chance to send its deassert. But that's just luck. Plug this on a fast CPU, and
> the same issue will resurface.
> 

Agreed, will leave it as it is.

Thanks & Regards,
Bharat


 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] PCI: Xilinx NWL: Fix, do not check for legacy status in while loop

[Bharat Kumar Gogada]

> On 24/01/17 10:15, Bharat Kumar Gogada wrote:
> >> On 21/01/17 11:11, Bharat Kumar Gogada wrote:
> >>> - The legacy status register value for particular INTx becomes low
> >>> only after DEASSERT_INTx is received.
> >>> - Few End Points take time for sending DEASSERT_INTx, checking
> >>> legacy status register in while loop causes invoking of EP handler
> >>> continuosly until DEASSERT_INTx is received.
> >>>
> >>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> >>> ---
> >>>  drivers/pci/host/pcie-xilinx-nwl.c |    5 +++--
> >>>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/host/pcie-xilinx-nwl.c
> >>> b/drivers/pci/host/pcie-xilinx-nwl.c
> >>> index 43eaa4a..c8b5a33 100644
> >>> --- a/drivers/pci/host/pcie-xilinx-nwl.c
> >>> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
> >>> @@ -342,9 +342,10 @@ static void nwl_pcie_leg_handler(struct
> >>> irq_desc
> >>> *desc)
> >>>
> >>>  	chained_irq_enter(chip, desc);
> >>>  	pcie = irq_desc_get_handler_data(desc);
> >>> +	status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> >>> +				  MSGF_LEG_SR_MASKALL;
> >>>
> >>> -	while ((status = nwl_bridge_readl(pcie, MSGF_LEG_STATUS) &
> >>> -				MSGF_LEG_SR_MASKALL) != 0) {
> >>> +	if (status != 0) {
> >>>  		for_each_set_bit(bit, &status, INTX_NUM) {
> >>>  			virq = irq_find_mapping(pcie->legacy_irq_domain,
> >>>  						bit + 1);
> >>>
> >>
> >> But even if you only handle the interrupt once, it is still asserted,
> >> right? You exit the low-level exception handler, only to take the
> >> interrupt immediately again. So what is the gain here?
> >>
> > Whenever masking of INTx happens (like as in my next patch[PATCH
> > 2/4]), the irq line goes low, but  status bit will be set until DEASSERT_INTx
> comes.
> > In this scenario if it is always in while loop, even though line went
> > low it will not be seen until DEASSERT_INTx, unnecessarily invoking EP
> > driver. so instead of while loop using if condition so that this change in line is
> noticed.
> 
> But what guarantees that you will observe this DEASSERT if you return to the
> interrupted context early? From what I understand, your interrupt flow is the
> following:
> 
> 
> 	read status
> 	mask
> 	handler
> 	unmask
> 
> If the EP takes time to send a deassert after the handler has poked it and the
> interrupt is still active, then the only thing that this patch buys you is that by
> returning to the interrupted context, you waste a lot of cycles, giving the device
> a chance to send its deassert. But that's just luck. Plug this on a fast CPU, and
> the same issue will resurface.
> 

Agreed, will leave it as it is.

Thanks & Regards,
Bharat