[PATCH 1/3] PCI: rcar: check platform_get_irq() return code

[PATCH 1/3] PCI: rcar: check platform_get_irq() return code

[Ben Dooks]

The current code does not check the return from platform_get_irq()
so add an error check and return if this call does fail.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
Cc: Simon Horman <horms@verge.net.au>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-sh@vger.kernel.org
---
 drivers/pci/host/pci-rcar-gen2.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
index ea65bac..674f7fe 100644
--- a/drivers/pci/host/pci-rcar-gen2.c
+++ b/drivers/pci/host/pci-rcar-gen2.c
@@ -308,6 +308,11 @@ static int __init rcar_pci_probe(struct platform_device *pdev)
 	priv->reg = reg;
 	priv->dev = &pdev->dev;
 
+	if (priv->irq < 0) {
+		dev_err(&pdev->dev, "no valid irq found\n");
+		return priv->irq;
+	}
+
 	return rcar_pci_add_controller(priv);
 }
 
-- 
1.8.5.2

[PATCH 2/3] PCI: rcar: add error interrupt handling

[Ben Dooks]

Add option to enable interrupts to report any errors from
the AHB-PCI bridge to help find any issues with the bridge
when in use.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
Cc: Simon Horman <horms@verge.net.au>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-sh@vger.kernel.org
---
 drivers/pci/host/Kconfig         |  9 ++++++
 drivers/pci/host/pci-rcar-gen2.c | 60 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 47d46c6..6d4c46e 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -33,4 +33,13 @@ config PCI_RCAR_GEN2
 	  There are 3 internal PCI controllers available with a single
 	  built-in EHCI/OHCI host controller present on each one.
 
+config PCI_RCAR_GEN2_ERRIRQ
+	bool "Enable error reporting interrupt support"
+	depends on PCI_RCAR_GEN2
+	help
+	  Say Y here to enable support for the bus-error interrupts from
+	  the PCI controller bridge.
+
+	  This is here for aiding in debugging issues with the hardware.
+
 endmenu
diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
index 674f7fe..01ba069 100644
--- a/drivers/pci/host/pci-rcar-gen2.c
+++ b/drivers/pci/host/pci-rcar-gen2.c
@@ -39,9 +39,26 @@
 
 #define RCAR_PCI_INT_ENABLE_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x20)
 #define RCAR_PCI_INT_STATUS_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x24)
+#define RCAR_PCI_INT_SIGTABORT		(1 << 0)
+#define RCAR_PCI_INT_SIGRETABORT	(1 << 1)
+#define RCAR_PCI_INT_REMABORT		(1 << 2)
+#define RCAR_PCI_INT_PERR		(1 << 3)
+#define RCAR_PCI_INT_SIGSERR		(1 << 4)
+#define RCAR_PCI_INT_RESERR		(1 << 5)
+#define RCAR_PCI_INT_WIN1ERR		(1 << 12)
+#define RCAR_PCI_INT_WIN2ERR		(1 << 13)
 #define RCAR_PCI_INT_A			(1 << 16)
 #define RCAR_PCI_INT_B			(1 << 17)
 #define RCAR_PCI_INT_PME		(1 << 19)
+#define RCAR_PCI_INT_ALLERRORS (RCAR_PCI_INT_SIGTABORT		| \
+				RCAR_PCI_INT_SIGRETABORT	| \
+				RCAR_PCI_INT_SIGRETABORT	| \
+				RCAR_PCI_INT_REMABORT		| \
+				RCAR_PCI_INT_PERR		| \
+				RCAR_PCI_INT_SIGSERR		| \
+				RCAR_PCI_INT_RESERR		| \
+				RCAR_PCI_INT_WIN1ERR		| \
+				RCAR_PCI_INT_WIN2ERR)
 
 #define RCAR_AHB_BUS_CTR_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x30)
 #define RCAR_AHB_BUS_MMODE_HTRANS	(1 << 0)
@@ -164,6 +181,47 @@ static int __init rcar_pci_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 	return priv->irq;
 }
 
+#ifdef CONFIG_PCI_RCAR_GEN2_ERRIRQ
+static irqreturn_t rcar_pci_err_irq(int irq, void *pw)
+{
+	struct rcar_pci_priv *priv = pw;
+	u32 status = ioread32(priv->reg + RCAR_PCI_INT_STATUS_REG);
+
+	if (status & RCAR_PCI_INT_ALLERRORS) {
+		dev_err(priv->dev, "error irq: status %08x\n", status);
+
+		/* clear the error(s) */
+		iowrite32(status & RCAR_PCI_INT_ALLERRORS,
+			  priv->reg + RCAR_PCI_INT_STATUS_REG);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static void rcar_pci_setup_errirq(struct rcar_pci_priv *priv)
+{
+	int ret;
+	u32 val;
+
+	ret = devm_request_irq(priv->dev, priv->irq, rcar_pci_err_irq,
+			       IRQF_SHARED, "error irq", priv);
+	if (ret) {
+		dev_err(priv->dev, "cannot claim IRQ for error handling\n");
+		return;
+	}
+
+	val = ioread32(priv->reg + RCAR_PCI_INT_ENABLE_REG);
+	val |= RCAR_PCI_INT_ALLERRORS;
+	iowrite32(val, priv->reg + RCAR_PCI_INT_ENABLE_REG);
+
+	dev_info(priv->dev, "irq mask now %08x\n", val);
+}
+#else
+static inline void rcar_pci_setup_errirq(struct rcar_pci_priv *priv) { }
+
+#endif
+
 /* PCI host controller setup */
 static int __init rcar_pci_setup(int nr, struct pci_sys_data *sys)
 {
@@ -224,6 +282,8 @@ static int __init rcar_pci_setup(int nr, struct pci_sys_data *sys)
 	iowrite32(RCAR_PCI_INT_A | RCAR_PCI_INT_B | RCAR_PCI_INT_PME,
 		  reg + RCAR_PCI_INT_ENABLE_REG);
 
+	rcar_pci_setup_errirq(priv);
+
 	/* Add PCI resources */
 	pci_add_resource(&sys->resources, &priv->io_res);
 	pci_add_resource(&sys->resources, &priv->mem_res);
-- 
1.8.5.2

[PATCH 3/3] PCI: rcar: fix bridge logic abort

[Ben Dooks]

The bridge logic at slot 0 only supports reads up to 0x40 and the
rest of the PCI configuration space for this slot is marked as
reserved in the manual.

Trying a read from offset 0x100 is producing an error from the
bridge. With error interrupts enabled, the following is printed:

pci-rcar-gen2 ee0d0000.pci: error irq: status 00000014

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
Cc: Simon Horman <horms@verge.net.au>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-sh@vger.kernel.org
---
 drivers/pci/host/pci-rcar-gen2.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
index 01ba069..17ad857 100644
--- a/drivers/pci/host/pci-rcar-gen2.c
+++ b/drivers/pci/host/pci-rcar-gen2.c
@@ -119,6 +119,10 @@ static void __iomem *rcar_pci_cfg_base(struct pci_bus *bus, unsigned int devfn,
 	if (slot > 2)
 		return NULL;
 
+	/* bridge logic only has registers to 0x40 */
+	if (slot == 0x0 && where >= 0x40)
+		return NULL;
+
 	val = slot ? RCAR_AHBPCI_WIN1_DEVICE | RCAR_AHBPCI_WIN_CTR_CFG :
 		     RCAR_AHBPCI_WIN1_HOST | RCAR_AHBPCI_WIN_CTR_CFG;
 
@@ -134,6 +138,8 @@ static int rcar_pci_read_config(struct pci_bus *bus, unsigned int devfn,
 	if (!reg)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
+	dev_info(&bus->dev, "fn %08x, read %08x, size %d\n", devfn, where, size);
+
 	switch (size) {
 	case 1:
 		*val = ioread8(reg);
@@ -157,6 +163,8 @@ static int rcar_pci_write_config(struct pci_bus *bus, unsigned int devfn,
 	if (!reg)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
+	dev_info(&bus->dev, "devfn %08x: write %08x to %08x, size %d\n", devfn, val, where, size);
+
 	switch (size) {
 	case 1:
 		iowrite8(val, reg);
-- 
1.8.5.2

Re: [PATCH 3/3] PCI: rcar: fix bridge logic abort

[Ben Dooks]

On 28/01/14 09:47, Ben Dooks wrote:
> The bridge logic at slot 0 only supports reads up to 0x40 and the
> rest of the PCI configuration space for this slot is marked as
> reserved in the manual.
>
> Trying a read from offset 0x100 is producing an error from the
> bridge. With error interrupts enabled, the following is printed:
>
> pci-rcar-gen2 ee0d0000.pci: error irq: status 00000014
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

apologies, did not remove the debug code from this.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 2/3] PCI: rcar: add error interrupt handling

[Ben Dooks]

Add option to enable interrupts to report any errors from
the AHB-PCI bridge to help find any issues with the bridge
when in use.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
Cc: Simon Horman <horms@verge.net.au>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-sh@vger.kernel.org
---
 drivers/pci/host/Kconfig         |  9 ++++++
 drivers/pci/host/pci-rcar-gen2.c | 60 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 47d46c6..6d4c46e 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -33,4 +33,13 @@ config PCI_RCAR_GEN2
 	  There are 3 internal PCI controllers available with a single
 	  built-in EHCI/OHCI host controller present on each one.
 
+config PCI_RCAR_GEN2_ERRIRQ
+	bool "Enable error reporting interrupt support"
+	depends on PCI_RCAR_GEN2
+	help
+	  Say Y here to enable support for the bus-error interrupts from
+	  the PCI controller bridge.
+
+	  This is here for aiding in debugging issues with the hardware.
+
 endmenu
diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
index 674f7fe..01ba069 100644
--- a/drivers/pci/host/pci-rcar-gen2.c
+++ b/drivers/pci/host/pci-rcar-gen2.c
@@ -39,9 +39,26 @@
 
 #define RCAR_PCI_INT_ENABLE_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x20)
 #define RCAR_PCI_INT_STATUS_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x24)
+#define RCAR_PCI_INT_SIGTABORT		(1 << 0)
+#define RCAR_PCI_INT_SIGRETABORT	(1 << 1)
+#define RCAR_PCI_INT_REMABORT		(1 << 2)
+#define RCAR_PCI_INT_PERR		(1 << 3)
+#define RCAR_PCI_INT_SIGSERR		(1 << 4)
+#define RCAR_PCI_INT_RESERR		(1 << 5)
+#define RCAR_PCI_INT_WIN1ERR		(1 << 12)
+#define RCAR_PCI_INT_WIN2ERR		(1 << 13)
 #define RCAR_PCI_INT_A			(1 << 16)
 #define RCAR_PCI_INT_B			(1 << 17)
 #define RCAR_PCI_INT_PME		(1 << 19)
+#define RCAR_PCI_INT_ALLERRORS (RCAR_PCI_INT_SIGTABORT		| \
+				RCAR_PCI_INT_SIGRETABORT	| \
+				RCAR_PCI_INT_SIGRETABORT	| \
+				RCAR_PCI_INT_REMABORT		| \
+				RCAR_PCI_INT_PERR		| \
+				RCAR_PCI_INT_SIGSERR		| \
+				RCAR_PCI_INT_RESERR		| \
+				RCAR_PCI_INT_WIN1ERR		| \
+				RCAR_PCI_INT_WIN2ERR)
 
 #define RCAR_AHB_BUS_CTR_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x30)
 #define RCAR_AHB_BUS_MMODE_HTRANS	(1 << 0)
@@ -164,6 +181,47 @@ static int __init rcar_pci_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 	return priv->irq;
 }
 
+#ifdef CONFIG_PCI_RCAR_GEN2_ERRIRQ
+static irqreturn_t rcar_pci_err_irq(int irq, void *pw)
+{
+	struct rcar_pci_priv *priv = pw;
+	u32 status = ioread32(priv->reg + RCAR_PCI_INT_STATUS_REG);
+
+	if (status & RCAR_PCI_INT_ALLERRORS) {
+		dev_err(priv->dev, "error irq: status %08x\n", status);
+
+		/* clear the error(s) */
+		iowrite32(status & RCAR_PCI_INT_ALLERRORS,
+			  priv->reg + RCAR_PCI_INT_STATUS_REG);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static void rcar_pci_setup_errirq(struct rcar_pci_priv *priv)
+{
+	int ret;
+	u32 val;
+
+	ret = devm_request_irq(priv->dev, priv->irq, rcar_pci_err_irq,
+			       IRQF_SHARED, "error irq", priv);
+	if (ret) {
+		dev_err(priv->dev, "cannot claim IRQ for error handling\n");
+		return;
+	}
+
+	val = ioread32(priv->reg + RCAR_PCI_INT_ENABLE_REG);
+	val |= RCAR_PCI_INT_ALLERRORS;
+	iowrite32(val, priv->reg + RCAR_PCI_INT_ENABLE_REG);
+
+	dev_info(priv->dev, "irq mask now %08x\n", val);
+}
+#else
+static inline void rcar_pci_setup_errirq(struct rcar_pci_priv *priv) { }
+
+#endif
+
 /* PCI host controller setup */
 static int __init rcar_pci_setup(int nr, struct pci_sys_data *sys)
 {
@@ -224,6 +282,8 @@ static int __init rcar_pci_setup(int nr, struct pci_sys_data *sys)
 	iowrite32(RCAR_PCI_INT_A | RCAR_PCI_INT_B | RCAR_PCI_INT_PME,
 		  reg + RCAR_PCI_INT_ENABLE_REG);
 
+	rcar_pci_setup_errirq(priv);
+
 	/* Add PCI resources */
 	pci_add_resource(&sys->resources, &priv->io_res);
 	pci_add_resource(&sys->resources, &priv->mem_res);
-- 
1.8.5.2

Re: [PATCH 1/3] PCI: rcar: check platform_get_irq() return code

[Ben Dooks]

On 28/01/14 16:11, Sergei Shtylyov wrote:
> Hello.
>
> On 01/28/2014 12:47 PM, Ben Dooks wrote:
>
>> The current code does not check the return from platform_get_irq()
>> so add an error check and return if this call does fail.
>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
>> Cc: Simon Horman <horms@verge.net.au>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: linux-pci@vger.kernel.org
>> Cc: linux-sh@vger.kernel.org
>> ---
>>   drivers/pci/host/pci-rcar-gen2.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>
>> diff --git a/drivers/pci/host/pci-rcar-gen2.c
>> b/drivers/pci/host/pci-rcar-gen2.c
>> index ea65bac..674f7fe 100644
>> --- a/drivers/pci/host/pci-rcar-gen2.c
>> +++ b/drivers/pci/host/pci-rcar-gen2.c
>> @@ -308,6 +308,11 @@ static int __init rcar_pci_probe(struct
>> platform_device *pdev)
>>       priv->reg = reg;
>>       priv->dev = &pdev->dev;
>>
>> +    if (priv->irq < 0) {
>
>     <= actually, as IRQ0 has been declared invalid by Linus.
>
>> +        dev_err(&pdev->dev, "no valid irq found\n");
>> +        return priv->irq;
>> +    }
>> +

IIRC, 0 is /no irq/ not an error.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH 1/3] PCI: rcar: check platform_get_irq() return code

[Sergei Shtylyov]

Hello.

On 01/28/2014 12:47 PM, Ben Dooks wrote:

> The current code does not check the return from platform_get_irq()
> so add an error check and return if this call does fail.

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> ---
>   drivers/pci/host/pci-rcar-gen2.c | 5 +++++
>   1 file changed, 5 insertions(+)

> diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
> index ea65bac..674f7fe 100644
> --- a/drivers/pci/host/pci-rcar-gen2.c
> +++ b/drivers/pci/host/pci-rcar-gen2.c
> @@ -308,6 +308,11 @@ static int __init rcar_pci_probe(struct platform_device *pdev)
>   	priv->reg = reg;
>   	priv->dev = &pdev->dev;
>
> +	if (priv->irq < 0) {

    <= actually, as IRQ0 has been declared invalid by Linus.

> +		dev_err(&pdev->dev, "no valid irq found\n");
> +		return priv->irq;
> +	}
> +

WBR, Sergei

Re: [PATCH 2/3] PCI: rcar: add error interrupt handling

[Valentine]

On 01/28/2014 02:06 PM, Ben Dooks wrote:
> Add option to enable interrupts to report any errors from
> the AHB-PCI bridge to help find any issues with the bridge
> when in use.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> ---
>   drivers/pci/host/Kconfig         |  9 ++++++
>   drivers/pci/host/pci-rcar-gen2.c | 60 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 69 insertions(+)
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 47d46c6..6d4c46e 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -33,4 +33,13 @@ config PCI_RCAR_GEN2
>   	  There are 3 internal PCI controllers available with a single
>   	  built-in EHCI/OHCI host controller present on each one.
>
> +config PCI_RCAR_GEN2_ERRIRQ

I don't think we need to introduce another option.
Please, see my comments below.

> +	bool "Enable error reporting interrupt support"
> +	depends on PCI_RCAR_GEN2
> +	help
> +	  Say Y here to enable support for the bus-error interrupts from
> +	  the PCI controller bridge.
> +
> +	  This is here for aiding in debugging issues with the hardware.
> +
>   endmenu
> diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
> index 674f7fe..01ba069 100644
> --- a/drivers/pci/host/pci-rcar-gen2.c
> +++ b/drivers/pci/host/pci-rcar-gen2.c
> @@ -39,9 +39,26 @@
>
>   #define RCAR_PCI_INT_ENABLE_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x20)
>   #define RCAR_PCI_INT_STATUS_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x24)
> +#define RCAR_PCI_INT_SIGTABORT		(1 << 0)
> +#define RCAR_PCI_INT_SIGRETABORT	(1 << 1)
> +#define RCAR_PCI_INT_REMABORT		(1 << 2)
> +#define RCAR_PCI_INT_PERR		(1 << 3)
> +#define RCAR_PCI_INT_SIGSERR		(1 << 4)
> +#define RCAR_PCI_INT_RESERR		(1 << 5)
> +#define RCAR_PCI_INT_WIN1ERR		(1 << 12)
> +#define RCAR_PCI_INT_WIN2ERR		(1 << 13)
>   #define RCAR_PCI_INT_A			(1 << 16)
>   #define RCAR_PCI_INT_B			(1 << 17)
>   #define RCAR_PCI_INT_PME		(1 << 19)
> +#define RCAR_PCI_INT_ALLERRORS (RCAR_PCI_INT_SIGTABORT		| \
> +				RCAR_PCI_INT_SIGRETABORT	| \
> +				RCAR_PCI_INT_SIGRETABORT	| \
> +				RCAR_PCI_INT_REMABORT		| \
> +				RCAR_PCI_INT_PERR		| \
> +				RCAR_PCI_INT_SIGSERR		| \
> +				RCAR_PCI_INT_RESERR		| \
> +				RCAR_PCI_INT_WIN1ERR		| \
> +				RCAR_PCI_INT_WIN2ERR)

If you don't mind me nitpicking, please, follow the style of #define RCAR_AHB_BUS_MODE

>
>   #define RCAR_AHB_BUS_CTR_REG		(RCAR_AHBPCI_PCICOM_OFFSET + 0x30)
>   #define RCAR_AHB_BUS_MMODE_HTRANS	(1 << 0)
> @@ -164,6 +181,47 @@ static int __init rcar_pci_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>   	return priv->irq;
>   }
>
> +#ifdef CONFIG_PCI_RCAR_GEN2_ERRIRQ

I think we could get away with #if IS_ENABLED(CONFIG_PCI_DEBUG) instead of
introducing another option here.

> +static irqreturn_t rcar_pci_err_irq(int irq, void *pw)
> +{
> +	struct rcar_pci_priv *priv = pw;
> +	u32 status = ioread32(priv->reg + RCAR_PCI_INT_STATUS_REG);

I'd do "& RCAR_PCI_INT_ALLERRORS" here once instead of doing
"status & RCAR_PCI_INT_ALLERRORS" twice below.

> +
> +	if (status & RCAR_PCI_INT_ALLERRORS) {
> +		dev_err(priv->dev, "error irq: status %08x\n", status);

Some errors are not critical. For example, master abort may happen during probing.
You may want to use dev_dbg since this is all about debugging

> +
> +		/* clear the error(s) */
> +		iowrite32(status & RCAR_PCI_INT_ALLERRORS,
> +			  priv->reg + RCAR_PCI_INT_STATUS_REG);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static void rcar_pci_setup_errirq(struct rcar_pci_priv *priv)
> +{
> +	int ret;
> +	u32 val;
> +
> +	ret = devm_request_irq(priv->dev, priv->irq, rcar_pci_err_irq,
> +			       IRQF_SHARED, "error irq", priv);
> +	if (ret) {
> +		dev_err(priv->dev, "cannot claim IRQ for error handling\n");
> +		return;
> +	}
> +
> +	val = ioread32(priv->reg + RCAR_PCI_INT_ENABLE_REG);
> +	val |= RCAR_PCI_INT_ALLERRORS;
> +	iowrite32(val, priv->reg + RCAR_PCI_INT_ENABLE_REG);
> +
> +	dev_info(priv->dev, "irq mask now %08x\n", val);
> +}
> +#else
> +static inline void rcar_pci_setup_errirq(struct rcar_pci_priv *priv) { }
> +
> +#endif
> +
>   /* PCI host controller setup */
>   static int __init rcar_pci_setup(int nr, struct pci_sys_data *sys)
>   {
> @@ -224,6 +282,8 @@ static int __init rcar_pci_setup(int nr, struct pci_sys_data *sys)

>   	iowrite32(RCAR_PCI_INT_A | RCAR_PCI_INT_B | RCAR_PCI_INT_PME,
>   		  reg + RCAR_PCI_INT_ENABLE_REG);
>
> +	rcar_pci_setup_errirq(priv);
> +

Since priv->irq == 0 is valid and means no interrupt, you may want to
call rcar_pci_setup_errirq only if (irq > 0)

Besides, I'd probably setup the IRQ handler directly before enabling the interrupts
-- under #if IS_ENABLED(CONFIG_PCI_DEBUG) if you like -- instead of introducing
the rcar_pci_setup_errirq function and reading/modifying/writing the RCAR_PCI_INT_ENABLE_REG there.

>   	/* Add PCI resources */
>   	pci_add_resource(&sys->resources, &priv->io_res);
>   	pci_add_resource(&sys->resources, &priv->mem_res);
>

Thanks,
Val.

Re: [PATCH 2/3] PCI: rcar: add error interrupt handling

[Magnus Damm]

On Tue, Jan 28, 2014 at 6:47 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> Add option to enable interrupts to report any errors from
> the AHB-PCI bridge to help find any issues with the bridge
> when in use.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> ---
>  drivers/pci/host/Kconfig         |  9 ++++++
>  drivers/pci/host/pci-rcar-gen2.c | 60 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 47d46c6..6d4c46e 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -33,4 +33,13 @@ config PCI_RCAR_GEN2
>           There are 3 internal PCI controllers available with a single
>           built-in EHCI/OHCI host controller present on each one.
>
> +config PCI_RCAR_GEN2_ERRIRQ
> +       bool "Enable error reporting interrupt support"
> +       depends on PCI_RCAR_GEN2
> +       help
> +         Say Y here to enable support for the bus-error interrupts from
> +         the PCI controller bridge.
> +
> +         This is here for aiding in debugging issues with the hardware.
> +

Thanks for your patches. Handling errors sounds good. About this
particular one, I'm not so keen on introducing Kconfig options for
each driver. Instead I'd prefer to handle this during runtime or as
Valentine suggested using CONFIG_PCI_DEBUG.

Cheers,

/ magnus

Re: [PATCH 2/3] PCI: rcar: add error interrupt handling

[Ben Dooks]

On 05/02/14 08:50, Magnus Damm wrote:
> On Tue, Jan 28, 2014 at 6:47 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> Add option to enable interrupts to report any errors from
>> the AHB-PCI bridge to help find any issues with the bridge
>> when in use.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

>
> Thanks for your patches. Handling errors sounds good. About this
> particular one, I'm not so keen on introducing Kconfig options for
> each driver. Instead I'd prefer to handle this during runtime or as
> Valentine suggested using CONFIG_PCI_DEBUG.

Ok, I will change to using that.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH 2/3] PCI: rcar: add error interrupt handling

[Andrew Murray]

On 28 January 2014 09:47, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> Add option to enable interrupts to report any errors from
> the AHB-PCI bridge to help find any issues with the bridge
> when in use.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: Valentine Barshak <valentine.barshak@cogentembedded.com>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linux-sh@vger.kernel.org
> ---
>  drivers/pci/host/Kconfig         |  9 ++++++
>  drivers/pci/host/pci-rcar-gen2.c | 60 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 69 insertions(+)
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 47d46c6..6d4c46e 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -33,4 +33,13 @@ config PCI_RCAR_GEN2
>           There are 3 internal PCI controllers available with a single
>           built-in EHCI/OHCI host controller present on each one.
>
> +config PCI_RCAR_GEN2_ERRIRQ
> +       bool "Enable error reporting interrupt support"
> +       depends on PCI_RCAR_GEN2
> +       help
> +         Say Y here to enable support for the bus-error interrupts from
> +         the PCI controller bridge.
> +
> +         This is here for aiding in debugging issues with the hardware.
> +
>  endmenu
> diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
> index 674f7fe..01ba069 100644
> --- a/drivers/pci/host/pci-rcar-gen2.c
> +++ b/drivers/pci/host/pci-rcar-gen2.c
> @@ -39,9 +39,26 @@
>
>  #define RCAR_PCI_INT_ENABLE_REG                (RCAR_AHBPCI_PCICOM_OFFSET + 0x20)
>  #define RCAR_PCI_INT_STATUS_REG                (RCAR_AHBPCI_PCICOM_OFFSET + 0x24)
> +#define RCAR_PCI_INT_SIGTABORT         (1 << 0)
> +#define RCAR_PCI_INT_SIGRETABORT       (1 << 1)
> +#define RCAR_PCI_INT_REMABORT          (1 << 2)
> +#define RCAR_PCI_INT_PERR              (1 << 3)
> +#define RCAR_PCI_INT_SIGSERR           (1 << 4)
> +#define RCAR_PCI_INT_RESERR            (1 << 5)
> +#define RCAR_PCI_INT_WIN1ERR           (1 << 12)
> +#define RCAR_PCI_INT_WIN2ERR           (1 << 13)
>  #define RCAR_PCI_INT_A                 (1 << 16)
>  #define RCAR_PCI_INT_B                 (1 << 17)
>  #define RCAR_PCI_INT_PME               (1 << 19)
> +#define RCAR_PCI_INT_ALLERRORS (RCAR_PCI_INT_SIGTABORT         | \
> +                               RCAR_PCI_INT_SIGRETABORT        | \
> +                               RCAR_PCI_INT_SIGRETABORT        | \
> +                               RCAR_PCI_INT_REMABORT           | \
> +                               RCAR_PCI_INT_PERR               | \
> +                               RCAR_PCI_INT_SIGSERR            | \
> +                               RCAR_PCI_INT_RESERR             | \
> +                               RCAR_PCI_INT_WIN1ERR            | \
> +                               RCAR_PCI_INT_WIN2ERR)

Is there an opportunity here to instead report some of these errors
via 'pcibios_report_status'? The status would then be cleared at
source and be printed to the console.

This only seems to be used for arm32, but there is probably no reason
this needs to be arch specific.

Andrew Murray

>
>  #define RCAR_AHB_BUS_CTR_REG           (RCAR_AHBPCI_PCICOM_OFFSET + 0x30)
>  #define RCAR_AHB_BUS_MMODE_HTRANS      (1 << 0)
> @@ -164,6 +181,47 @@ static int __init rcar_pci_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>         return priv->irq;
>  }
>
> +#ifdef CONFIG_PCI_RCAR_GEN2_ERRIRQ
> +static irqreturn_t rcar_pci_err_irq(int irq, void *pw)
> +{
> +       struct rcar_pci_priv *priv = pw;
> +       u32 status = ioread32(priv->reg + RCAR_PCI_INT_STATUS_REG);
> +
> +       if (status & RCAR_PCI_INT_ALLERRORS) {
> +               dev_err(priv->dev, "error irq: status %08x\n", status);
> +
> +               /* clear the error(s) */
> +               iowrite32(status & RCAR_PCI_INT_ALLERRORS,
> +                         priv->reg + RCAR_PCI_INT_STATUS_REG);
> +               return IRQ_HANDLED;
> +       }
> +
> +       return IRQ_NONE;
> +}
> +
> +static void rcar_pci_setup_errirq(struct rcar_pci_priv *priv)
> +{
> +       int ret;
> +       u32 val;
> +
> +       ret = devm_request_irq(priv->dev, priv->irq, rcar_pci_err_irq,
> +                              IRQF_SHARED, "error irq", priv);
> +       if (ret) {
> +               dev_err(priv->dev, "cannot claim IRQ for error handling\n");
> +               return;
> +       }
> +
> +       val = ioread32(priv->reg + RCAR_PCI_INT_ENABLE_REG);
> +       val |= RCAR_PCI_INT_ALLERRORS;
> +       iowrite32(val, priv->reg + RCAR_PCI_INT_ENABLE_REG);
> +
> +       dev_info(priv->dev, "irq mask now %08x\n", val);
> +}
> +#else
> +static inline void rcar_pci_setup_errirq(struct rcar_pci_priv *priv) { }
> +
> +#endif
> +
>  /* PCI host controller setup */
>  static int __init rcar_pci_setup(int nr, struct pci_sys_data *sys)
>  {
> @@ -224,6 +282,8 @@ static int __init rcar_pci_setup(int nr, struct pci_sys_data *sys)
>         iowrite32(RCAR_PCI_INT_A | RCAR_PCI_INT_B | RCAR_PCI_INT_PME,
>                   reg + RCAR_PCI_INT_ENABLE_REG);
>
> +       rcar_pci_setup_errirq(priv);
> +
>         /* Add PCI resources */
>         pci_add_resource(&sys->resources, &priv->io_res);
>         pci_add_resource(&sys->resources, &priv->mem_res);
> --
> 1.8.5.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Andrew Murray, Director
Embedded Bits Limited
www.embedded-bits.co.uk

Embedded Bits Limited is a company registered in England and Wales
with company number 08178608 and VAT number 140658911. Registered
office: Embedded Bits Limited c/o InTouch Accounting Ltd. Bristol and
West House Post Office Road Bournemouth Dorset BH1 1BL