Re: [PATCH] PCI: designware: bail out if host_init failed

From: Jisheng Zhang
Date: Tue Dec 08 2015 - 01:03:34 EST

Next message: Khiem Nguyen: "Re: [PATCH 5/8 v4] thermal: rcar: enable to use thermal-zone on DT"
Previous message: Andy Lutomirski: "Re: [PSEUDOPATCH] rename is_compat_task"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <jszhang@xxxxxxxxxxx>
> > ---
> > 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 &= 0xffff;
> > iowrite32(val, pcie->dbi + PCIE_STRFMR1);
> > +
> > + return 0;
> > +err:
> > + pcie->scfg = NULL;
> > + return ret;
> > }
> >
> > static int ls_pcie_link_up(struct pcie_port *pp)
> > @@ -140,7 +145,7 @@ static int ls_pcie_link_up(struct pcie_port *pp)
> > return 1;
> > }
> >
> > -static void ls_pcie_host_init(struct pcie_port *pp)
> > +static int ls_pcie_host_init(struct pcie_port *pp)
> > {
> > struct ls_pcie *pcie = to_ls_pcie(pp);
> >
> > @@ -148,6 +153,8 @@ static void ls_pcie_host_init(struct pcie_port *pp)
> > ls_pcie_fix_class(pcie);
> > ls_pcie_clear_multifunction(pcie);
> > iowrite32(0, pcie->dbi + PCIE_DBI_RO_WR_EN);
> > +
> > + return 0;
> > }
> >
> > static int ls_pcie_msi_host_init(struct pcie_port *pp,
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index 540f077..a80a9ba 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -515,8 +515,11 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > }
> > }
> >
> > - if (pp->ops->host_init)
> > - pp->ops->host_init(pp);
> > + if (pp->ops->host_init) {
> > + ret = pp->ops->host_init(pp);
> > + if (ret)
> > + return ret;
> > + }
> >
> > if (!pp->ops->rd_other_conf)
> > dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX1,
> > diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> > index 2356d29..7b5317f 100644
> > --- a/drivers/pci/host/pcie-designware.h
> > +++ b/drivers/pci/host/pcie-designware.h
> > @@ -63,7 +63,7 @@ struct pcie_host_ops {
> > int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
> > unsigned int devfn, int where, int size, u32 val);
> > int (*link_up)(struct pcie_port *pp);
> > - void (*host_init)(struct pcie_port *pp);
> > + int (*host_init)(struct pcie_port *pp);
> > void (*msi_set_irq)(struct pcie_port *pp, int irq);
> > void (*msi_clear_irq)(struct pcie_port *pp, int irq);
> > phys_addr_t (*get_msi_addr)(struct pcie_port *pp);
> > diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c
> > index b95b756..50b3b31 100644
> > --- a/drivers/pci/host/pcie-spear13xx.c
> > +++ b/drivers/pci/host/pcie-spear13xx.c
> > @@ -256,10 +256,12 @@ static int spear13xx_pcie_link_up(struct pcie_port *pp)
> > return 0;
> > }
> >
> > -static void spear13xx_pcie_host_init(struct pcie_port *pp)
> > +static int spear13xx_pcie_host_init(struct pcie_port *pp)
> > {
> > spear13xx_pcie_establish_link(pp);
> > spear13xx_pcie_enable_interrupts(pp);
> > +
> > + return 0;
> > }
> >
> > static struct pcie_host_ops spear13xx_pcie_host_ops = {
> > --
> > 2.6.2
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Khiem Nguyen: "Re: [PATCH 5/8 v4] thermal: rcar: enable to use thermal-zone on DT"
Previous message: Andy Lutomirski: "Re: [PSEUDOPATCH] rename is_compat_task"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]