Re: [PATCH] PCI: designware: bail out if host_init failed
Dear Bjorn,
On Wed, 25 Nov 2015 14:01:03 -0600
Bjorn Helgaas wrote:
> Hi Jisheng,
>
> On Thu, Nov 12, 2015 at 09:48:45PM +0800, Jisheng Zhang wrote:
> > There's no reason to continue the initialization such as configure
> > register, scan root bus etc. if customized host_init() failed. This
> > patch tries to check the host_init result, bail out if failed.
>
> This patch changes the (*host_init) return type and adds "return 0" to
> the host_init() implementations of ten different drivers, all to
> support a possible error in one driver.
>
> Is there any way to detect and handle the error in
> ls1021_pcie_host_init() earlier, maybe by doing the syscon_regmap
> lookup in ls_pcie_probe() instead of in the host_init() function?
>
> That would be even better because you wouldn't have to touch any of
> the other drivers, and you'd detect the error even earlier, before
> we've done any of the designware setup.
Sorry for being late. Got your point. The reason I made this patch is that:
I want to clk gate the pcie host to save power if the customized host_init()
report link training failure etc. For example: there's no pcie device in the
slot at all. But today, I have a different idea: the PCIE support hotplug,
so link training failure doesn't mean we should bail out, in fact we should
continue the initialization as current code does. So I think Jingoo made the
correct decision when implement the pcie designware interface. I want to drop
this patch.
Only one problem need your suggestions: how to save power when there's no device
, I.E clk gate the host, shutdown the pcie phy etc.
Any suggestions are appreciated!
Thanks in advance,
Jisheng
>
> Bjorn
>
> > Signed-off-by: Jisheng Zhang
> > ---
> > drivers/pci/host/pci-dra7xx.c | 4 +++-
> > drivers/pci/host/pci-exynos.c | 4 +++-
> > drivers/pci/host/pci-imx6.c| 4 +++-
> > drivers/pci/host/pci-keystone.c| 4 +++-
> > drivers/pci/host/pci-layerscape.c | 25 -
> > drivers/pci/host/pcie-designware.c | 7 +--
> > drivers/pci/host/pcie-designware.h | 2 +-
> > drivers/pci/host/pcie-spear13xx.c | 4 +++-
> > 8 files changed, 37 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> > index 8c36880..b3160a1 100644
> > --- a/drivers/pci/host/pci-dra7xx.c
> > +++ b/drivers/pci/host/pci-dra7xx.c
> > @@ -149,7 +149,7 @@ static void dra7xx_pcie_enable_interrupts(struct
> > pcie_port *pp)
> >LEG_EP_INTERRUPTS);
> > }
> >
> > -static void dra7xx_pcie_host_init(struct pcie_port *pp)
> > +static int dra7xx_pcie_host_init(struct pcie_port *pp)
> > {
> > dw_pcie_setup_rc(pp);
> >
> > @@ -162,6 +162,8 @@ static void dra7xx_pcie_host_init(struct pcie_port *pp)
> > if (IS_ENABLED(CONFIG_PCI_MSI))
> > dw_pcie_msi_init(pp);
> > dra7xx_pcie_enable_interrupts(pp);
> > +
> > + return 0;
> > }
> >
> > static struct pcie_host_ops dra7xx_pcie_host_ops = {
> > diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> > index 01095e1..57f370b 100644
> > --- a/drivers/pci/host/pci-exynos.c
> > +++ b/drivers/pci/host/pci-exynos.c
> > @@ -481,10 +481,12 @@ static int exynos_pcie_link_up(struct pcie_port *pp)
> > return 0;
> > }
> >
> > -static void exynos_pcie_host_init(struct pcie_port *pp)
> > +static int exynos_pcie_host_init(struct pcie_port *pp)
> > {
> > exynos_pcie_establish_link(pp);
> > exynos_pcie_enable_interrupts(pp);
> > +
> > + return 0;
> > }
> >
> > static struct pcie_host_ops exynos_pcie_host_ops = {
> > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> > index 22e8224..9a46680 100644
> > --- a/drivers/pci/host/pci-imx6.c
> > +++ b/drivers/pci/host/pci-imx6.c
> > @@ -425,7 +425,7 @@ static int imx6_pcie_establish_link(struct pcie_port
> > *pp)
> > return 0;
> > }
> >
> > -static void imx6_pcie_host_init(struct pcie_port *pp)
> > +static int imx6_pcie_host_init(struct pcie_port *pp)
> > {
> > imx6_pcie_assert_core_reset(pp);
> >
> > @@ -439,6 +439,8 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
> >
> > if (IS_ENABLED(CONFIG_PCI_MSI))
> > dw_pcie_msi_init(pp);
> > +
> > + return 0;
> > }
> >
> > static void imx6_pcie_reset_phy(struct pcie_port *pp)
> > diff --git a/drivers/pci/host/pci-keystone.c
> > b/drivers/pci/host/pci-keystone.c
> > index 0aa81bd..2604571 100644
> > --- a/drivers/pci/host/pci-keystone.c
> > +++ b/drivers/pci/host/pci-keystone.c
> > @@ -250,7 +250,7 @@ static int keystone_pcie_fault(unsigned long addr,
> > unsigned int fsr,
> > return 0;
> > }
> >
> > -static void __init ks_pcie_host_init(struct pcie_port *pp)
> > +static int __init ks_pcie_host_init(struct pcie_port *pp)
> > {
> > struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
> > u32 val;
> > @@ -277,6 +277,8 @@ static void __init ks_pcie_host_init(struct pcie_port
> > *p
Re: [PATCH] PCI: designware: bail out if host_init failed
Hi Jisheng,
On Thu, Nov 12, 2015 at 09:48:45PM +0800, Jisheng Zhang wrote:
> There's no reason to continue the initialization such as configure
> register, scan root bus etc. if customized host_init() failed. This
> patch tries to check the host_init result, bail out if failed.
This patch changes the (*host_init) return type and adds "return 0" to
the host_init() implementations of ten different drivers, all to
support a possible error in one driver.
Is there any way to detect and handle the error in
ls1021_pcie_host_init() earlier, maybe by doing the syscon_regmap
lookup in ls_pcie_probe() instead of in the host_init() function?
That would be even better because you wouldn't have to touch any of
the other drivers, and you'd detect the error even earlier, before
we've done any of the designware setup.
Bjorn
> Signed-off-by: Jisheng Zhang
> ---
> drivers/pci/host/pci-dra7xx.c | 4 +++-
> drivers/pci/host/pci-exynos.c | 4 +++-
> drivers/pci/host/pci-imx6.c| 4 +++-
> drivers/pci/host/pci-keystone.c| 4 +++-
> drivers/pci/host/pci-layerscape.c | 25 -
> drivers/pci/host/pcie-designware.c | 7 +--
> drivers/pci/host/pcie-designware.h | 2 +-
> drivers/pci/host/pcie-spear13xx.c | 4 +++-
> 8 files changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> index 8c36880..b3160a1 100644
> --- a/drivers/pci/host/pci-dra7xx.c
> +++ b/drivers/pci/host/pci-dra7xx.c
> @@ -149,7 +149,7 @@ static void dra7xx_pcie_enable_interrupts(struct
> pcie_port *pp)
> LEG_EP_INTERRUPTS);
> }
>
> -static void dra7xx_pcie_host_init(struct pcie_port *pp)
> +static int dra7xx_pcie_host_init(struct pcie_port *pp)
> {
> dw_pcie_setup_rc(pp);
>
> @@ -162,6 +162,8 @@ static void dra7xx_pcie_host_init(struct pcie_port *pp)
> if (IS_ENABLED(CONFIG_PCI_MSI))
> dw_pcie_msi_init(pp);
> dra7xx_pcie_enable_interrupts(pp);
> +
> + return 0;
> }
>
> static struct pcie_host_ops dra7xx_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> index 01095e1..57f370b 100644
> --- a/drivers/pci/host/pci-exynos.c
> +++ b/drivers/pci/host/pci-exynos.c
> @@ -481,10 +481,12 @@ static int exynos_pcie_link_up(struct pcie_port *pp)
> return 0;
> }
>
> -static void exynos_pcie_host_init(struct pcie_port *pp)
> +static int exynos_pcie_host_init(struct pcie_port *pp)
> {
> exynos_pcie_establish_link(pp);
> exynos_pcie_enable_interrupts(pp);
> +
> + return 0;
> }
>
> static struct pcie_host_ops exynos_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 22e8224..9a46680 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -425,7 +425,7 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
> return 0;
> }
>
> -static void imx6_pcie_host_init(struct pcie_port *pp)
> +static int imx6_pcie_host_init(struct pcie_port *pp)
> {
> imx6_pcie_assert_core_reset(pp);
>
> @@ -439,6 +439,8 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
>
> if (IS_ENABLED(CONFIG_PCI_MSI))
> dw_pcie_msi_init(pp);
> +
> + return 0;
> }
>
> static void imx6_pcie_reset_phy(struct pcie_port *pp)
> diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c
> index 0aa81bd..2604571 100644
> --- a/drivers/pci/host/pci-keystone.c
> +++ b/drivers/pci/host/pci-keystone.c
> @@ -250,7 +250,7 @@ static int keystone_pcie_fault(unsigned long addr,
> unsigned int fsr,
> return 0;
> }
>
> -static void __init ks_pcie_host_init(struct pcie_port *pp)
> +static int __init ks_pcie_host_init(struct pcie_port *pp)
> {
> struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
> u32 val;
> @@ -277,6 +277,8 @@ static void __init ks_pcie_host_init(struct pcie_port *pp)
>*/
> hook_fault_code(17, keystone_pcie_fault, SIGBUS, 0,
> "Asynchronous external abort");
> +
> + return 0;
> }
>
> static struct pcie_host_ops keystone_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-layerscape.c
> b/drivers/pci/host/pci-layerscape.c
> index 3923bed..a6e9070 100644
> --- a/drivers/pci/host/pci-layerscape.c
> +++ b/drivers/pci/host/pci-layerscape.c
> @@ -94,8 +94,9 @@ static int ls1021_pcie_link_up(struct pcie_port *pp)
> return 1;
> }
>
> -static void ls1021_pcie_host_init(struct pcie_port *pp)
> +static int ls1021_pcie_host_init(struct pcie_port *pp)
> {
> + int ret;
> struct ls_pcie *pcie = to_ls_pcie(pp);
> u32 val, index[2];
>
> @@ -103,15 +104,14 @@ static void ls1021_pcie_host_init(struct pcie_port *pp)
>"fsl,pcie-scfg");
> if (IS_ERR(pcie->scfg)) {
> dev_err(pp->dev, "No syscfg phandle specified\n");
> - pcie->scfg
[PATCH] PCI: designware: bail out if host_init failed
There's no reason to continue the initialization such as configure
register, scan root bus etc. if customized host_init() failed. This
patch tries to check the host_init result, bail out if failed.
Signed-off-by: Jisheng Zhang
---
drivers/pci/host/pci-dra7xx.c | 4 +++-
drivers/pci/host/pci-exynos.c | 4 +++-
drivers/pci/host/pci-imx6.c| 4 +++-
drivers/pci/host/pci-keystone.c| 4 +++-
drivers/pci/host/pci-layerscape.c | 25 -
drivers/pci/host/pcie-designware.c | 7 +--
drivers/pci/host/pcie-designware.h | 2 +-
drivers/pci/host/pcie-spear13xx.c | 4 +++-
8 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 8c36880..b3160a1 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -149,7 +149,7 @@ static void dra7xx_pcie_enable_interrupts(struct pcie_port
*pp)
LEG_EP_INTERRUPTS);
}
-static void dra7xx_pcie_host_init(struct pcie_port *pp)
+static int dra7xx_pcie_host_init(struct pcie_port *pp)
{
dw_pcie_setup_rc(pp);
@@ -162,6 +162,8 @@ static void dra7xx_pcie_host_init(struct pcie_port *pp)
if (IS_ENABLED(CONFIG_PCI_MSI))
dw_pcie_msi_init(pp);
dra7xx_pcie_enable_interrupts(pp);
+
+ return 0;
}
static struct pcie_host_ops dra7xx_pcie_host_ops = {
diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
index 01095e1..57f370b 100644
--- a/drivers/pci/host/pci-exynos.c
+++ b/drivers/pci/host/pci-exynos.c
@@ -481,10 +481,12 @@ static int exynos_pcie_link_up(struct pcie_port *pp)
return 0;
}
-static void exynos_pcie_host_init(struct pcie_port *pp)
+static int exynos_pcie_host_init(struct pcie_port *pp)
{
exynos_pcie_establish_link(pp);
exynos_pcie_enable_interrupts(pp);
+
+ return 0;
}
static struct pcie_host_ops exynos_pcie_host_ops = {
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 22e8224..9a46680 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -425,7 +425,7 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
return 0;
}
-static void imx6_pcie_host_init(struct pcie_port *pp)
+static int imx6_pcie_host_init(struct pcie_port *pp)
{
imx6_pcie_assert_core_reset(pp);
@@ -439,6 +439,8 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
if (IS_ENABLED(CONFIG_PCI_MSI))
dw_pcie_msi_init(pp);
+
+ return 0;
}
static void imx6_pcie_reset_phy(struct pcie_port *pp)
diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c
index 0aa81bd..2604571 100644
--- a/drivers/pci/host/pci-keystone.c
+++ b/drivers/pci/host/pci-keystone.c
@@ -250,7 +250,7 @@ static int keystone_pcie_fault(unsigned long addr, unsigned
int fsr,
return 0;
}
-static void __init ks_pcie_host_init(struct pcie_port *pp)
+static int __init ks_pcie_host_init(struct pcie_port *pp)
{
struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
u32 val;
@@ -277,6 +277,8 @@ static void __init ks_pcie_host_init(struct pcie_port *pp)
*/
hook_fault_code(17, keystone_pcie_fault, SIGBUS, 0,
"Asynchronous external abort");
+
+ return 0;
}
static struct pcie_host_ops keystone_pcie_host_ops = {
diff --git a/drivers/pci/host/pci-layerscape.c
b/drivers/pci/host/pci-layerscape.c
index 3923bed..a6e9070 100644
--- a/drivers/pci/host/pci-layerscape.c
+++ b/drivers/pci/host/pci-layerscape.c
@@ -94,8 +94,9 @@ static int ls1021_pcie_link_up(struct pcie_port *pp)
return 1;
}
-static void ls1021_pcie_host_init(struct pcie_port *pp)
+static int ls1021_pcie_host_init(struct pcie_port *pp)
{
+ int ret;
struct ls_pcie *pcie = to_ls_pcie(pp);
u32 val, index[2];
@@ -103,15 +104,14 @@ static void ls1021_pcie_host_init(struct pcie_port *pp)
"fsl,pcie-scfg");
if (IS_ERR(pcie->scfg)) {
dev_err(pp->dev, "No syscfg phandle specified\n");
- pcie->scfg = NULL;
- return;
+ ret = PTR_ERR(pcie->scfg);
+ goto err;
}
- if (of_property_read_u32_array(pp->dev->of_node,
- "fsl,pcie-scfg", index, 2)) {
- pcie->scfg = NULL;
- return;
- }
+ ret = of_property_read_u32_array(pp->dev->of_node,
+"fsl,pcie-scfg", index, 2);
+ if (ret)
+ goto err;
pcie->index = index[1];
dw_pcie_setup_rc(pp);
@@ -123,6 +123,11 @@ static void ls1021_pcie_host_init(struct pcie_port *pp)
val = ioread32(pcie->dbi + PCIE_STRFMR1);
val &= 0x;
iowrite32(val, pcie->dbi + PCIE_STRFMR1);
+
+ return 0;
+err:
+ pcie->scfg = NULL;
+
