[PATCH 0/3] gpio: pcie-idio-24: Fix IRQ handling

[PATCH 0/3] gpio: pcie-idio-24: Fix IRQ handling

[Arnaud de Turckheim]

This patch set fixes the irq_mask/irq_unmask functions and enables the
PLX PEX8311 local interrupts.

With the current version of the driver, gpiomon (from libgpiod) or a
poll(2) on the sysfs value are not working. According to
/proc/interrupts, there is no interruption triggered.
The main issue is that the local interrupts are not forwarded by the
PEX8311 to the PCI.

There is also two bugs:
    - The IRQ mask is not correctly updated when unmasking an
      interruption.
    - The COS Enable Register value is not correctly updated when
      masking/unmasking an interruption.

It seems this problems exist since the initial commit.

Arnaud de Turckheim (3):
  gpio: pcie-idio-24: Fix irq mask when masking
  gpio: pcie-idio-24: Fix IRQ Enable Register value
  gpio: pcie-idio-24: Enable PEX8311 interrupts

 drivers/gpio/gpio-pcie-idio-24.c | 62 ++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/3] gpio: pcie-idio-24: Fix irq mask when masking

[Arnaud de Turckheim]

Fix the bitwise operation to remove only the corresponding bit from the
mask.

Fixes: 585562046628 ("gpio: Add GPIO support for the ACCES PCIe-IDIO-24 family")
Signed-off-by: Arnaud de Turckheim <quarium@gmail.com>
---
 drivers/gpio/gpio-pcie-idio-24.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pcie-idio-24.c b/drivers/gpio/gpio-pcie-idio-24.c
index a68941d19ac6..5ea517416366 100644
--- a/drivers/gpio/gpio-pcie-idio-24.c
+++ b/drivers/gpio/gpio-pcie-idio-24.c
@@ -339,7 +339,7 @@ static void idio_24_irq_mask(struct irq_data *data)
 
 	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
 
-	idio24gpio->irq_mask &= BIT(bit_offset);
+	idio24gpio->irq_mask &= ~BIT(bit_offset);
 	new_irq_mask = idio24gpio->irq_mask >> bank_offset;
 
 	if (!new_irq_mask) {
-- 
2.25.1

Re: [PATCH 1/3] gpio: pcie-idio-24: Fix irq mask when masking

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

On Wed, Nov 04, 2020 at 04:24:53PM +0100, Arnaud de Turckheim wrote:
> Fix the bitwise operation to remove only the corresponding bit from the
> mask.
> 
> Fixes: 585562046628 ("gpio: Add GPIO support for the ACCES PCIe-IDIO-24 family")
> Signed-off-by: Arnaud de Turckheim <quarium@gmail.com>

Reviewed-by: William Breathitt Gray <vilhelm.gray@gmail.com>

> ---
>  drivers/gpio/gpio-pcie-idio-24.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-pcie-idio-24.c b/drivers/gpio/gpio-pcie-idio-24.c
> index a68941d19ac6..5ea517416366 100644
> --- a/drivers/gpio/gpio-pcie-idio-24.c
> +++ b/drivers/gpio/gpio-pcie-idio-24.c
> @@ -339,7 +339,7 @@ static void idio_24_irq_mask(struct irq_data *data)
>  
>  	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
>  
> -	idio24gpio->irq_mask &= BIT(bit_offset);
> +	idio24gpio->irq_mask &= ~BIT(bit_offset);
>  	new_irq_mask = idio24gpio->irq_mask >> bank_offset;
>  
>  	if (!new_irq_mask) {
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] gpio: pcie-idio-24: Fix IRQ Enable Register value

[Arnaud de Turckheim]

This fixes the COS Enable Register value for enabling/disabling the
corresponding IRQs bank.

Fixes: 585562046628 ("gpio: Add GPIO support for the ACCES PCIe-IDIO-24 family")
Signed-off-by: Arnaud de Turckheim <quarium@gmail.com>
---
 drivers/gpio/gpio-pcie-idio-24.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-pcie-idio-24.c b/drivers/gpio/gpio-pcie-idio-24.c
index 5ea517416366..a61de14d09b6 100644
--- a/drivers/gpio/gpio-pcie-idio-24.c
+++ b/drivers/gpio/gpio-pcie-idio-24.c
@@ -334,13 +334,13 @@ static void idio_24_irq_mask(struct irq_data *data)
 	unsigned long flags;
 	const unsigned long bit_offset = irqd_to_hwirq(data) - 24;
 	unsigned char new_irq_mask;
-	const unsigned long bank_offset = bit_offset/8 * 8;
+	const unsigned long bank_offset = bit_offset / 8;
 	unsigned char cos_enable_state;
 
 	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
 
 	idio24gpio->irq_mask &= ~BIT(bit_offset);
-	new_irq_mask = idio24gpio->irq_mask >> bank_offset;
+	new_irq_mask = idio24gpio->irq_mask >> bank_offset * 8;
 
 	if (!new_irq_mask) {
 		cos_enable_state = ioread8(&idio24gpio->reg->cos_enable);
@@ -363,12 +363,12 @@ static void idio_24_irq_unmask(struct irq_data *data)
 	unsigned long flags;
 	unsigned char prev_irq_mask;
 	const unsigned long bit_offset = irqd_to_hwirq(data) - 24;
-	const unsigned long bank_offset = bit_offset/8 * 8;
+	const unsigned long bank_offset = bit_offset / 8;
 	unsigned char cos_enable_state;
 
 	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
 
-	prev_irq_mask = idio24gpio->irq_mask >> bank_offset;
+	prev_irq_mask = idio24gpio->irq_mask >> bank_offset * 8;
 	idio24gpio->irq_mask |= BIT(bit_offset);
 
 	if (!prev_irq_mask) {
-- 
2.25.1

Re: [PATCH 2/3] gpio: pcie-idio-24: Fix IRQ Enable Register value

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

On Wed, Nov 04, 2020 at 04:24:54PM +0100, Arnaud de Turckheim wrote:
> This fixes the COS Enable Register value for enabling/disabling the
> corresponding IRQs bank.
> 
> Fixes: 585562046628 ("gpio: Add GPIO support for the ACCES PCIe-IDIO-24 family")
> Signed-off-by: Arnaud de Turckheim <quarium@gmail.com>

Reviewed-by: William Breathitt Gray <vilhelm.gray@gmail.com>

> ---
>  drivers/gpio/gpio-pcie-idio-24.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pcie-idio-24.c b/drivers/gpio/gpio-pcie-idio-24.c
> index 5ea517416366..a61de14d09b6 100644
> --- a/drivers/gpio/gpio-pcie-idio-24.c
> +++ b/drivers/gpio/gpio-pcie-idio-24.c
> @@ -334,13 +334,13 @@ static void idio_24_irq_mask(struct irq_data *data)
>  	unsigned long flags;
>  	const unsigned long bit_offset = irqd_to_hwirq(data) - 24;
>  	unsigned char new_irq_mask;
> -	const unsigned long bank_offset = bit_offset/8 * 8;
> +	const unsigned long bank_offset = bit_offset / 8;
>  	unsigned char cos_enable_state;
>  
>  	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
>  
>  	idio24gpio->irq_mask &= ~BIT(bit_offset);
> -	new_irq_mask = idio24gpio->irq_mask >> bank_offset;
> +	new_irq_mask = idio24gpio->irq_mask >> bank_offset * 8;
>  
>  	if (!new_irq_mask) {
>  		cos_enable_state = ioread8(&idio24gpio->reg->cos_enable);
> @@ -363,12 +363,12 @@ static void idio_24_irq_unmask(struct irq_data *data)
>  	unsigned long flags;
>  	unsigned char prev_irq_mask;
>  	const unsigned long bit_offset = irqd_to_hwirq(data) - 24;
> -	const unsigned long bank_offset = bit_offset/8 * 8;
> +	const unsigned long bank_offset = bit_offset / 8;
>  	unsigned char cos_enable_state;
>  
>  	raw_spin_lock_irqsave(&idio24gpio->lock, flags);
>  
> -	prev_irq_mask = idio24gpio->irq_mask >> bank_offset;
> +	prev_irq_mask = idio24gpio->irq_mask >> bank_offset * 8;
>  	idio24gpio->irq_mask |= BIT(bit_offset);
>  
>  	if (!prev_irq_mask) {
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] gpio: pcie-idio-24: Enable PEX8311 interrupts

[Arnaud de Turckheim]

This enables the PEX8311 internal PCI wire interrupt and the PEX8311
local interrupt input so the local interrupts are forwarded to the PCI.

Fixes: 585562046628 ("gpio: Add GPIO support for the ACCES PCIe-IDIO-24 family")
Signed-off-by: Arnaud de Turckheim <quarium@gmail.com>
---
 drivers/gpio/gpio-pcie-idio-24.c | 52 +++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-pcie-idio-24.c b/drivers/gpio/gpio-pcie-idio-24.c
index a61de14d09b6..2a07fd96707e 100644
--- a/drivers/gpio/gpio-pcie-idio-24.c
+++ b/drivers/gpio/gpio-pcie-idio-24.c
@@ -28,6 +28,47 @@
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
+/*
+ * PLX PEX8311 PCI LCS_INTCSR Interrupt Control/Status
+ *
+ * Bit: Description
+ *   0: Enable Interrupt Sources (Bit 0)
+ *   1: Enable Interrupt Sources (Bit 1)
+ *   2: Generate Internal PCI Bus Internal SERR# Interrupt
+ *   3: Mailbox Interrupt Enable
+ *   4: Power Management Interrupt Enable
+ *   5: Power Management Interrupt
+ *   6: Slave Read Local Data Parity Check Error Enable
+ *   7: Slave Read Local Data Parity Check Error Status
+ *   8: Internal PCI Wire Interrupt Enable
+ *   9: PCI Express Doorbell Interrupt Enable
+ *  10: PCI Abort Interrupt Enable
+ *  11: Local Interrupt Input Enable
+ *  12: Retry Abort Enable
+ *  13: PCI Express Doorbell Interrupt Active
+ *  14: PCI Abort Interrupt Active
+ *  15: Local Interrupt Input Active
+ *  16: Local Interrupt Output Enable
+ *  17: Local Doorbell Interrupt Enable
+ *  18: DMA Channel 0 Interrupt Enable
+ *  19: DMA Channel 1 Interrupt Enable
+ *  20: Local Doorbell Interrupt Active
+ *  21: DMA Channel 0 Interrupt Active
+ *  22: DMA Channel 1 Interrupt Active
+ *  23: Built-In Self-Test (BIST) Interrupt Active
+ *  24: Direct Master was the Bus Master during a Master or Target Abort
+ *  25: DMA Channel 0 was the Bus Master during a Master or Target Abort
+ *  26: DMA Channel 1 was the Bus Master during a Master or Target Abort
+ *  27: Target Abort after internal 256 consecutive Master Retrys
+ *  28: PCI Bus wrote data to LCS_MBOX0
+ *  29: PCI Bus wrote data to LCS_MBOX1
+ *  30: PCI Bus wrote data to LCS_MBOX2
+ *  31: PCI Bus wrote data to LCS_MBOX3
+ */
+#define PLX_PEX8311_PCI_LCS_INTCSR  0x68
+#define INTCSR_INTERNAL_PCI_WIRE    BIT(8)
+#define INTCSR_LOCAL_INPUT          BIT(11)
+
 /**
  * struct idio_24_gpio_reg - GPIO device registers structure
  * @out0_7:	Read: FET Outputs 0-7
@@ -92,6 +133,7 @@ struct idio_24_gpio_reg {
 struct idio_24_gpio {
 	struct gpio_chip chip;
 	raw_spinlock_t lock;
+	__u8 __iomem *plx;
 	struct idio_24_gpio_reg __iomem *reg;
 	unsigned long irq_mask;
 };
@@ -455,6 +497,7 @@ static int idio_24_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct device *const dev = &pdev->dev;
 	struct idio_24_gpio *idio24gpio;
 	int err;
+	const size_t pci_plx_bar_index = 1;
 	const size_t pci_bar_index = 2;
 	const char *const name = pci_name(pdev);
 	struct gpio_irq_chip *girq;
@@ -469,12 +512,13 @@ static int idio_24_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		return err;
 	}
 
-	err = pcim_iomap_regions(pdev, BIT(pci_bar_index), name);
+	err = pcim_iomap_regions(pdev, BIT(pci_plx_bar_index) | BIT(pci_bar_index), name);
 	if (err) {
 		dev_err(dev, "Unable to map PCI I/O addresses (%d)\n", err);
 		return err;
 	}
 
+	idio24gpio->plx = pcim_iomap_table(pdev)[pci_plx_bar_index];
 	idio24gpio->reg = pcim_iomap_table(pdev)[pci_bar_index];
 
 	idio24gpio->chip.label = name;
@@ -504,6 +548,12 @@ static int idio_24_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	/* Software board reset */
 	iowrite8(0, &idio24gpio->reg->soft_reset);
+	/*
+	 * enable PLX PEX8311 internal PCI wire interrupt and local interrupt
+	 * input
+	 */
+	iowrite8((INTCSR_INTERNAL_PCI_WIRE | INTCSR_LOCAL_INPUT) >> 8,
+		 idio24gpio->plx + PLX_PEX8311_PCI_LCS_INTCSR + 1);
 
 	err = devm_gpiochip_add_data(dev, &idio24gpio->chip, idio24gpio);
 	if (err) {
-- 
2.25.1

Re: [PATCH 3/3] gpio: pcie-idio-24: Enable PEX8311 interrupts

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 4480 bytes --]

On Wed, Nov 04, 2020 at 04:24:55PM +0100, Arnaud de Turckheim wrote:
> This enables the PEX8311 internal PCI wire interrupt and the PEX8311
> local interrupt input so the local interrupts are forwarded to the PCI.
> 
> Fixes: 585562046628 ("gpio: Add GPIO support for the ACCES PCIe-IDIO-24 family")
> Signed-off-by: Arnaud de Turckheim <quarium@gmail.com>

Reviewed-by: William Breathitt Gray <vilhelm.gray@gmail.com>

> ---
>  drivers/gpio/gpio-pcie-idio-24.c | 52 +++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-pcie-idio-24.c b/drivers/gpio/gpio-pcie-idio-24.c
> index a61de14d09b6..2a07fd96707e 100644
> --- a/drivers/gpio/gpio-pcie-idio-24.c
> +++ b/drivers/gpio/gpio-pcie-idio-24.c
> @@ -28,6 +28,47 @@
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
>  
> +/*
> + * PLX PEX8311 PCI LCS_INTCSR Interrupt Control/Status
> + *
> + * Bit: Description
> + *   0: Enable Interrupt Sources (Bit 0)
> + *   1: Enable Interrupt Sources (Bit 1)
> + *   2: Generate Internal PCI Bus Internal SERR# Interrupt
> + *   3: Mailbox Interrupt Enable
> + *   4: Power Management Interrupt Enable
> + *   5: Power Management Interrupt
> + *   6: Slave Read Local Data Parity Check Error Enable
> + *   7: Slave Read Local Data Parity Check Error Status
> + *   8: Internal PCI Wire Interrupt Enable
> + *   9: PCI Express Doorbell Interrupt Enable
> + *  10: PCI Abort Interrupt Enable
> + *  11: Local Interrupt Input Enable
> + *  12: Retry Abort Enable
> + *  13: PCI Express Doorbell Interrupt Active
> + *  14: PCI Abort Interrupt Active
> + *  15: Local Interrupt Input Active
> + *  16: Local Interrupt Output Enable
> + *  17: Local Doorbell Interrupt Enable
> + *  18: DMA Channel 0 Interrupt Enable
> + *  19: DMA Channel 1 Interrupt Enable
> + *  20: Local Doorbell Interrupt Active
> + *  21: DMA Channel 0 Interrupt Active
> + *  22: DMA Channel 1 Interrupt Active
> + *  23: Built-In Self-Test (BIST) Interrupt Active
> + *  24: Direct Master was the Bus Master during a Master or Target Abort
> + *  25: DMA Channel 0 was the Bus Master during a Master or Target Abort
> + *  26: DMA Channel 1 was the Bus Master during a Master or Target Abort
> + *  27: Target Abort after internal 256 consecutive Master Retrys
> + *  28: PCI Bus wrote data to LCS_MBOX0
> + *  29: PCI Bus wrote data to LCS_MBOX1
> + *  30: PCI Bus wrote data to LCS_MBOX2
> + *  31: PCI Bus wrote data to LCS_MBOX3
> + */
> +#define PLX_PEX8311_PCI_LCS_INTCSR  0x68
> +#define INTCSR_INTERNAL_PCI_WIRE    BIT(8)
> +#define INTCSR_LOCAL_INPUT          BIT(11)
> +
>  /**
>   * struct idio_24_gpio_reg - GPIO device registers structure
>   * @out0_7:	Read: FET Outputs 0-7
> @@ -92,6 +133,7 @@ struct idio_24_gpio_reg {
>  struct idio_24_gpio {
>  	struct gpio_chip chip;
>  	raw_spinlock_t lock;
> +	__u8 __iomem *plx;
>  	struct idio_24_gpio_reg __iomem *reg;
>  	unsigned long irq_mask;
>  };
> @@ -455,6 +497,7 @@ static int idio_24_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	struct device *const dev = &pdev->dev;
>  	struct idio_24_gpio *idio24gpio;
>  	int err;
> +	const size_t pci_plx_bar_index = 1;
>  	const size_t pci_bar_index = 2;
>  	const char *const name = pci_name(pdev);
>  	struct gpio_irq_chip *girq;
> @@ -469,12 +512,13 @@ static int idio_24_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		return err;
>  	}
>  
> -	err = pcim_iomap_regions(pdev, BIT(pci_bar_index), name);
> +	err = pcim_iomap_regions(pdev, BIT(pci_plx_bar_index) | BIT(pci_bar_index), name);
>  	if (err) {
>  		dev_err(dev, "Unable to map PCI I/O addresses (%d)\n", err);
>  		return err;
>  	}
>  
> +	idio24gpio->plx = pcim_iomap_table(pdev)[pci_plx_bar_index];
>  	idio24gpio->reg = pcim_iomap_table(pdev)[pci_bar_index];
>  
>  	idio24gpio->chip.label = name;
> @@ -504,6 +548,12 @@ static int idio_24_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	/* Software board reset */
>  	iowrite8(0, &idio24gpio->reg->soft_reset);
> +	/*
> +	 * enable PLX PEX8311 internal PCI wire interrupt and local interrupt
> +	 * input
> +	 */
> +	iowrite8((INTCSR_INTERNAL_PCI_WIRE | INTCSR_LOCAL_INPUT) >> 8,
> +		 idio24gpio->plx + PLX_PEX8311_PCI_LCS_INTCSR + 1);
>  
>  	err = devm_gpiochip_add_data(dev, &idio24gpio->chip, idio24gpio);
>  	if (err) {
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] gpio: pcie-idio-24: Fix IRQ handling

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

On Wed, Nov 04, 2020 at 04:24:52PM +0100, Arnaud de Turckheim wrote:
> This patch set fixes the irq_mask/irq_unmask functions and enables the
> PLX PEX8311 local interrupts.
> 
> With the current version of the driver, gpiomon (from libgpiod) or a
> poll(2) on the sysfs value are not working. According to
> /proc/interrupts, there is no interruption triggered.
> The main issue is that the local interrupts are not forwarded by the
> PEX8311 to the PCI.
> 
> There is also two bugs:
>     - The IRQ mask is not correctly updated when unmasking an
>       interruption.
>     - The COS Enable Register value is not correctly updated when
>       masking/unmasking an interruption.
> 
> It seems this problems exist since the initial commit.
> 
> Arnaud de Turckheim (3):
>   gpio: pcie-idio-24: Fix irq mask when masking
>   gpio: pcie-idio-24: Fix IRQ Enable Register value
>   gpio: pcie-idio-24: Enable PEX8311 interrupts
> 
>  drivers/gpio/gpio-pcie-idio-24.c | 62 ++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 6 deletions(-)
> 
> -- 
> 2.25.1
> 

This patchset looks good to me and fixes several fundamental bugs. I'm
going to reply to each patch individually to give my Reviewed-by tags
for the benefit of those picking up the patches.

Thanks!

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] gpio: pcie-idio-24: Fix IRQ handling

[Bartosz Golaszewski]

On Wed, Nov 4, 2020 at 4:25 PM Arnaud de Turckheim <quarium@gmail.com> wrote:
>
> This patch set fixes the irq_mask/irq_unmask functions and enables the
> PLX PEX8311 local interrupts.
>
> With the current version of the driver, gpiomon (from libgpiod) or a
> poll(2) on the sysfs value are not working. According to
> /proc/interrupts, there is no interruption triggered.
> The main issue is that the local interrupts are not forwarded by the
> PEX8311 to the PCI.
>
> There is also two bugs:
>     - The IRQ mask is not correctly updated when unmasking an
>       interruption.
>     - The COS Enable Register value is not correctly updated when
>       masking/unmasking an interruption.
>
> It seems this problems exist since the initial commit.
>
> Arnaud de Turckheim (3):
>   gpio: pcie-idio-24: Fix irq mask when masking
>   gpio: pcie-idio-24: Fix IRQ Enable Register value
>   gpio: pcie-idio-24: Enable PEX8311 interrupts
>
>  drivers/gpio/gpio-pcie-idio-24.c | 62 ++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 6 deletions(-)
>
> --
> 2.25.1
>

Series queued for fixes, thanks!

Bartosz