PCI fixes for Renesas RCar

PCI fixes for Renesas RCar

[Ben Dooks]

This is a small set of patches that fix a couple of issues with
the rcar-gen2 pci driver and add error interrupt support for
debugging.

Fixes from v1:
	- remove debugging code
	- fix patch title on bridge config accesses

[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

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

[Valentine]

On 01/28/2014 02:06 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.
>

If you have really experienced this error your platform data is incorrect.

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Acked-by: Valentine Barshak <valentine.barshak@cogentembedded.com>

> ---
> 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);
>   }
>
>

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

[PATCH 3/3] PCI: rcar: fix bridge logic configuration accesses

[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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
index 01ba069..42f0105 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;
 
-- 
1.8.5.2

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

[Valentine]

On 01/28/2014 02:06 PM, 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:

I don't think this is a critical error.
The bridge works fine after an attempt to access the unsupported PCIe/PCI-X 2 area.

If you want to prevent the access, I'm OK with it.
But I think it's better to do "if (where <= 0x100)"
and drop the slot check since all other slots do not support
access beyond 0x100 as well.

The PCI code attempts to access beyond 0x100 only once when probing PCI bridges
to see if they support PCIe/PCI-X 2 area which is 4K.
The area beyond 0x40 is never accessed because the bridge does not expose any PCI capabilities.

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

Did you experience any problems other than this message
printed by the IRQ handler introduced by the previous patch?

>
> 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 | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
> index 01ba069..42f0105 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;
>
>

Thanks,
Val.

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

[Simon Horman]

On Tue, Jan 28, 2014 at 11:41:29PM +0400, Valentine wrote:
> On 01/28/2014 02:06 PM, 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:
> 
> I don't think this is a critical error.
> The bridge works fine after an attempt to access the unsupported PCIe/PCI-X 2 area.
> 
> If you want to prevent the access, I'm OK with it.
> But I think it's better to do "if (where <= 0x100)"
> and drop the slot check since all other slots do not support
> access beyond 0x100 as well.
> 
> The PCI code attempts to access beyond 0x100 only once when probing PCI bridges
> to see if they support PCIe/PCI-X 2 area which is 4K.
> The area beyond 0x40 is never accessed because the bridge does not expose any PCI capabilities.
> 
> >
> >pci-rcar-gen2 ee0d0000.pci: error irq: status 00000014
> 
> Did you experience any problems other than this message
> printed by the IRQ handler introduced by the previous patch?

I am wondering if the documentation for the r8a7790 and r8a7791 are
the same with regards to the <= 0x100 limit.

> 
> >
> >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 | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
> >index 01ba069..42f0105 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;
> >
> >
> 
> Thanks,
> Val.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Valentine]

On 01/29/2014 10:42 AM, Simon Horman wrote:
> On Tue, Jan 28, 2014 at 11:41:29PM +0400, Valentine wrote:
>> On 01/28/2014 02:06 PM, 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:
>>
>> I don't think this is a critical error.
>> The bridge works fine after an attempt to access the unsupported PCIe/PCI-X 2 area.
>>
>> If you want to prevent the access, I'm OK with it.
>> But I think it's better to do "if (where <= 0x100)"
>> and drop the slot check since all other slots do not support
>> access beyond 0x100 as well.
>>
>> The PCI code attempts to access beyond 0x100 only once when probing PCI bridges
>> to see if they support PCIe/PCI-X 2 area which is 4K.
>> The area beyond 0x40 is never accessed because the bridge does not expose any PCI capabilities.
>>
>>>
>>> pci-rcar-gen2 ee0d0000.pci: error irq: status 00000014
>>
>> Did you experience any problems other than this message
>> printed by the IRQ handler introduced by the previous patch?
>
> I am wondering if the documentation for the r8a7790 and r8a7791 are
> the same with regards to the <= 0x100 limit.
>

It is the same since it is neither a PCIe nor a PCI-X 2 device.

Thanks,
Val.

>>
>>>
>>> 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 | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/pci/host/pci-rcar-gen2.c b/drivers/pci/host/pci-rcar-gen2.c
>>> index 01ba069..42f0105 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;
>>>
>>>
>>
>> Thanks,
>> Val.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

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

[Ben Dooks]

On 28/01/14 19:41, Valentine wrote:
> On 01/28/2014 02:06 PM, 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:
>
> I don't think this is a critical error.
> The bridge works fine after an attempt to access the unsupported
> PCIe/PCI-X 2 area.
>
> If you want to prevent the access, I'm OK with it.
> But I think it's better to do "if (where <= 0x100)"
> and drop the slot check since all other slots do not support
> access beyond 0x100 as well.

The bridge specifically says that the registers end at 0x40 so
that really should stay.

We're seeing external aborts being generated from somewhere, and
it seems that either the bridge or the USB drivers are responsible
for generating them.

> The PCI code attempts to access beyond 0x100 only once when probing PCI
> bridges
> to see if they support PCIe/PCI-X 2 area which is 4K.
> The area beyond 0x40 is never accessed because the bridge does not
> expose any PCI capabilities.
>
>>
>> pci-rcar-gen2 ee0d0000.pci: error irq: status 00000014
>
> Did you experience any problems other than this message
> printed by the IRQ handler introduced by the previous patch?
>


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

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

[Valentine]

On 01/29/2014 01:13 PM, Ben Dooks wrote:
> On 28/01/14 19:41, Valentine wrote:
>> On 01/28/2014 02:06 PM, 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:
>>
>> I don't think this is a critical error.
>> The bridge works fine after an attempt to access the unsupported
>> PCIe/PCI-X 2 area.
>>
>> If you want to prevent the access, I'm OK with it.
>> But I think it's better to do "if (where <= 0x100)"
>> and drop the slot check since all other slots do not support
>> access beyond 0x100 as well.
>
> The bridge specifically says that the registers end at 0x40 so
> that really should stay.

AFAIU, nothing is going to access the area beyond 0x40 if the device does not
expose any PCI capabilities. Otherwise we would probably need to fix the
generic PCI subsytem driver.

>
> We're seeing external aborts being generated from somewhere, and
> it seems that either the bridge or the USB drivers are responsible
> for generating them.

How often do you see them?
Do you see them if you use the approach I've proposed?

>
>> The PCI code attempts to access beyond 0x100 only once when probing PCI
>> bridges
>> to see if they support PCIe/PCI-X 2 area which is 4K.
>> The area beyond 0x40 is never accessed because the bridge does not
>> expose any PCI capabilities.
>>
>>>
>>> pci-rcar-gen2 ee0d0000.pci: error irq: status 00000014
>>
>> Did you experience any problems other than this message
>> printed by the IRQ handler introduced by the previous patch?
>>
>
>

Thanks,
Val.

[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

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