Re: [PATCH V4] PCI: rcar: Use runtime PM to control controller clock
On Sun, Apr 01, 2018 at 05:02:07PM +0200, Marek Vasut wrote: > On 03/19/2018 11:34 AM, Simon Horman wrote: > > On Mon, Mar 19, 2018 at 10:54:47AM +0100, Marek Vasut wrote: > >> On 03/19/2018 09:44 AM, Simon Horman wrote: > >>> On Sun, Mar 18, 2018 at 11:52:09AM +0100, Marek Vasut wrote: > From: Dien Pham> > The controller clock can be switched off during suspend/resume, > let runtime PM take care of that. > > Signed-off-by: Dien Pham > Signed-off-by: Hien Dang > Signed-off-by: Marek Vasut > Cc: Geert Uytterhoeven > Cc: Lorenzo Pieralisi > Cc: Phil Edworthy > Cc: Simon Horman > Cc: Wolfram Sang > Cc: linux-renesas-soc@vger.kernel.org > To: linux-...@vger.kernel.org > --- > V2: - Reorder the fail path in rcar_pcie_probe() to cater for the > reordering of function calls in probe > - Dispose of fail_clk in rcar_pcie_get_resources() > V3: - Fix up the failpath in probe function > V4: - Rebase on recent linux-next > --- > drivers/pci/host/pcie-rcar.c | 40 > > 1 file changed, 12 insertions(+), 28 deletions(-) > > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > index b4c4aad2cf66..93d59f15c589 100644 > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -142,7 +142,6 @@ struct rcar_pcie { > void __iomem*base; > struct list_headresources; > int root_bus_nr; > -struct clk *clk; > struct clk *bus_clk; > struct rcar_msi msi; > }; > @@ -914,24 +913,14 @@ static int rcar_pcie_get_resources(struct > rcar_pcie *pcie) > if (IS_ERR(pcie->base)) > return PTR_ERR(pcie->base); > > -pcie->clk = devm_clk_get(dev, "pcie"); > -if (IS_ERR(pcie->clk)) { > -dev_err(dev, "cannot get platform clock\n"); > -return PTR_ERR(pcie->clk); > -} > -err = clk_prepare_enable(pcie->clk); > -if (err) > -return err; > - > pcie->bus_clk = devm_clk_get(dev, "pcie_bus"); > if (IS_ERR(pcie->bus_clk)) { > dev_err(dev, "cannot get pcie bus clock\n"); > -err = PTR_ERR(pcie->bus_clk); > -goto fail_clk; > +return PTR_ERR(pcie->bus_clk); > } > err = clk_prepare_enable(pcie->bus_clk); > if (err) > -goto fail_clk; > +return err; > > i = irq_of_parse_and_map(dev->of_node, 0); > if (!i) { > @@ -953,8 +942,6 @@ static int rcar_pcie_get_resources(struct rcar_pcie > *pcie) > > err_map_reg: > clk_disable_unprepare(pcie->bus_clk); > -fail_clk: > -clk_disable_unprepare(pcie->clk); > > return err; > } > @@ -1124,22 +,22 @@ static int rcar_pcie_probe(struct > platform_device *pdev) > if (err) > goto err_free_bridge; > >>> > >>> This error path now calls pci_free_resource_list() and in this case > >>> the path is taken if rcar_pcie_parse_request_of_pci_ranges() fails. > >>> Is that ok? > >> > >> I think so, or did I miss something obvious? > > > > I feel that I am the one missing something obvious. > > But here is what I am worried about: > > > > 1. rcar_pcie_probe() calls rcar_pcie_parse_request_of_pci_ranges() > > 2. rcar_pcie_parse_request_of_pci_ranges() calls > >devm_request_pci_bus_resources() but that fails so > >pci_free_resource_list() is called and an error is returned. > > 3. rcar_pcie_probe() branches to err_free_bridge and calls > >pci_free_resource_list() again. > > I see what you mean now, thanks for the details explanation. > > So the change needed here is to move > pci_free_resource_list(>resources); above err_free_bridge, so it's > not called twice, right ? Yes, I think so.
Re: [PATCH V4] PCI: rcar: Use runtime PM to control controller clock
On 03/19/2018 11:34 AM, Simon Horman wrote: > On Mon, Mar 19, 2018 at 10:54:47AM +0100, Marek Vasut wrote: >> On 03/19/2018 09:44 AM, Simon Horman wrote: >>> On Sun, Mar 18, 2018 at 11:52:09AM +0100, Marek Vasut wrote: From: Dien PhamThe controller clock can be switched off during suspend/resume, let runtime PM take care of that. Signed-off-by: Dien Pham Signed-off-by: Hien Dang Signed-off-by: Marek Vasut Cc: Geert Uytterhoeven Cc: Lorenzo Pieralisi Cc: Phil Edworthy Cc: Simon Horman Cc: Wolfram Sang Cc: linux-renesas-soc@vger.kernel.org To: linux-...@vger.kernel.org --- V2: - Reorder the fail path in rcar_pcie_probe() to cater for the reordering of function calls in probe - Dispose of fail_clk in rcar_pcie_get_resources() V3: - Fix up the failpath in probe function V4: - Rebase on recent linux-next --- drivers/pci/host/pcie-rcar.c | 40 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index b4c4aad2cf66..93d59f15c589 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -142,7 +142,6 @@ struct rcar_pcie { void __iomem*base; struct list_headresources; int root_bus_nr; - struct clk *clk; struct clk *bus_clk; struct rcar_msi msi; }; @@ -914,24 +913,14 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie) if (IS_ERR(pcie->base)) return PTR_ERR(pcie->base); - pcie->clk = devm_clk_get(dev, "pcie"); - if (IS_ERR(pcie->clk)) { - dev_err(dev, "cannot get platform clock\n"); - return PTR_ERR(pcie->clk); - } - err = clk_prepare_enable(pcie->clk); - if (err) - return err; - pcie->bus_clk = devm_clk_get(dev, "pcie_bus"); if (IS_ERR(pcie->bus_clk)) { dev_err(dev, "cannot get pcie bus clock\n"); - err = PTR_ERR(pcie->bus_clk); - goto fail_clk; + return PTR_ERR(pcie->bus_clk); } err = clk_prepare_enable(pcie->bus_clk); if (err) - goto fail_clk; + return err; i = irq_of_parse_and_map(dev->of_node, 0); if (!i) { @@ -953,8 +942,6 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie) err_map_reg: clk_disable_unprepare(pcie->bus_clk); -fail_clk: - clk_disable_unprepare(pcie->clk); return err; } @@ -1124,22 +,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) if (err) goto err_free_bridge; >>> >>> This error path now calls pci_free_resource_list() and in this case >>> the path is taken if rcar_pcie_parse_request_of_pci_ranges() fails. >>> Is that ok? >> >> I think so, or did I miss something obvious? > > I feel that I am the one missing something obvious. > But here is what I am worried about: > > 1. rcar_pcie_probe() calls rcar_pcie_parse_request_of_pci_ranges() > 2. rcar_pcie_parse_request_of_pci_ranges() calls >devm_request_pci_bus_resources() but that fails so >pci_free_resource_list() is called and an error is returned. > 3. rcar_pcie_probe() branches to err_free_bridge and calls >pci_free_resource_list() again. I see what you mean now, thanks for the details explanation. So the change needed here is to move pci_free_resource_list(>resources); above err_free_bridge, so it's not called twice, right ? -- Best regards, Marek Vasut
Re: [PATCH V4] PCI: rcar: Use runtime PM to control controller clock
On Mon, Mar 19, 2018 at 10:54:47AM +0100, Marek Vasut wrote: > On 03/19/2018 09:44 AM, Simon Horman wrote: > > On Sun, Mar 18, 2018 at 11:52:09AM +0100, Marek Vasut wrote: > >> From: Dien Pham> >> > >> The controller clock can be switched off during suspend/resume, > >> let runtime PM take care of that. > >> > >> Signed-off-by: Dien Pham > >> Signed-off-by: Hien Dang > >> Signed-off-by: Marek Vasut > >> Cc: Geert Uytterhoeven > >> Cc: Lorenzo Pieralisi > >> Cc: Phil Edworthy > >> Cc: Simon Horman > >> Cc: Wolfram Sang > >> Cc: linux-renesas-soc@vger.kernel.org > >> To: linux-...@vger.kernel.org > >> --- > >> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the > >> reordering of function calls in probe > >> - Dispose of fail_clk in rcar_pcie_get_resources() > >> V3: - Fix up the failpath in probe function > >> V4: - Rebase on recent linux-next > >> --- > >> drivers/pci/host/pcie-rcar.c | 40 > >> 1 file changed, 12 insertions(+), 28 deletions(-) > >> > >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > >> index b4c4aad2cf66..93d59f15c589 100644 > >> --- a/drivers/pci/host/pcie-rcar.c > >> +++ b/drivers/pci/host/pcie-rcar.c > >> @@ -142,7 +142,6 @@ struct rcar_pcie { > >>void __iomem*base; > >>struct list_headresources; > >>int root_bus_nr; > >> - struct clk *clk; > >>struct clk *bus_clk; > >>struct rcar_msi msi; > >> }; > >> @@ -914,24 +913,14 @@ static int rcar_pcie_get_resources(struct rcar_pcie > >> *pcie) > >>if (IS_ERR(pcie->base)) > >>return PTR_ERR(pcie->base); > >> > >> - pcie->clk = devm_clk_get(dev, "pcie"); > >> - if (IS_ERR(pcie->clk)) { > >> - dev_err(dev, "cannot get platform clock\n"); > >> - return PTR_ERR(pcie->clk); > >> - } > >> - err = clk_prepare_enable(pcie->clk); > >> - if (err) > >> - return err; > >> - > >>pcie->bus_clk = devm_clk_get(dev, "pcie_bus"); > >>if (IS_ERR(pcie->bus_clk)) { > >>dev_err(dev, "cannot get pcie bus clock\n"); > >> - err = PTR_ERR(pcie->bus_clk); > >> - goto fail_clk; > >> + return PTR_ERR(pcie->bus_clk); > >>} > >>err = clk_prepare_enable(pcie->bus_clk); > >>if (err) > >> - goto fail_clk; > >> + return err; > >> > >>i = irq_of_parse_and_map(dev->of_node, 0); > >>if (!i) { > >> @@ -953,8 +942,6 @@ static int rcar_pcie_get_resources(struct rcar_pcie > >> *pcie) > >> > >> err_map_reg: > >>clk_disable_unprepare(pcie->bus_clk); > >> -fail_clk: > >> - clk_disable_unprepare(pcie->clk); > >> > >>return err; > >> } > >> @@ -1124,22 +,22 @@ static int rcar_pcie_probe(struct platform_device > >> *pdev) > >>if (err) > >>goto err_free_bridge; > > > > This error path now calls pci_free_resource_list() and in this case > > the path is taken if rcar_pcie_parse_request_of_pci_ranges() fails. > > Is that ok? > > I think so, or did I miss something obvious? I feel that I am the one missing something obvious. But here is what I am worried about: 1. rcar_pcie_probe() calls rcar_pcie_parse_request_of_pci_ranges() 2. rcar_pcie_parse_request_of_pci_ranges() calls devm_request_pci_bus_resources() but that fails so pci_free_resource_list() is called and an error is returned. 3. rcar_pcie_probe() branches to err_free_bridge and calls pci_free_resource_list() again. > > -- > Best regards, > Marek Vasut >
Re: [PATCH V4] PCI: rcar: Use runtime PM to control controller clock
On 03/19/2018 09:44 AM, Simon Horman wrote: > On Sun, Mar 18, 2018 at 11:52:09AM +0100, Marek Vasut wrote: >> From: Dien Pham>> >> The controller clock can be switched off during suspend/resume, >> let runtime PM take care of that. >> >> Signed-off-by: Dien Pham >> Signed-off-by: Hien Dang >> Signed-off-by: Marek Vasut >> Cc: Geert Uytterhoeven >> Cc: Lorenzo Pieralisi >> Cc: Phil Edworthy >> Cc: Simon Horman >> Cc: Wolfram Sang >> Cc: linux-renesas-soc@vger.kernel.org >> To: linux-...@vger.kernel.org >> --- >> V2: - Reorder the fail path in rcar_pcie_probe() to cater for the >> reordering of function calls in probe >> - Dispose of fail_clk in rcar_pcie_get_resources() >> V3: - Fix up the failpath in probe function >> V4: - Rebase on recent linux-next >> --- >> drivers/pci/host/pcie-rcar.c | 40 >> 1 file changed, 12 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c >> index b4c4aad2cf66..93d59f15c589 100644 >> --- a/drivers/pci/host/pcie-rcar.c >> +++ b/drivers/pci/host/pcie-rcar.c >> @@ -142,7 +142,6 @@ struct rcar_pcie { >> void __iomem*base; >> struct list_headresources; >> int root_bus_nr; >> -struct clk *clk; >> struct clk *bus_clk; >> struct rcar_msi msi; >> }; >> @@ -914,24 +913,14 @@ static int rcar_pcie_get_resources(struct rcar_pcie >> *pcie) >> if (IS_ERR(pcie->base)) >> return PTR_ERR(pcie->base); >> >> -pcie->clk = devm_clk_get(dev, "pcie"); >> -if (IS_ERR(pcie->clk)) { >> -dev_err(dev, "cannot get platform clock\n"); >> -return PTR_ERR(pcie->clk); >> -} >> -err = clk_prepare_enable(pcie->clk); >> -if (err) >> -return err; >> - >> pcie->bus_clk = devm_clk_get(dev, "pcie_bus"); >> if (IS_ERR(pcie->bus_clk)) { >> dev_err(dev, "cannot get pcie bus clock\n"); >> -err = PTR_ERR(pcie->bus_clk); >> -goto fail_clk; >> +return PTR_ERR(pcie->bus_clk); >> } >> err = clk_prepare_enable(pcie->bus_clk); >> if (err) >> -goto fail_clk; >> +return err; >> >> i = irq_of_parse_and_map(dev->of_node, 0); >> if (!i) { >> @@ -953,8 +942,6 @@ static int rcar_pcie_get_resources(struct rcar_pcie >> *pcie) >> >> err_map_reg: >> clk_disable_unprepare(pcie->bus_clk); >> -fail_clk: >> -clk_disable_unprepare(pcie->clk); >> >> return err; >> } >> @@ -1124,22 +,22 @@ static int rcar_pcie_probe(struct platform_device >> *pdev) >> if (err) >> goto err_free_bridge; > > This error path now calls pci_free_resource_list() and in this case > the path is taken if rcar_pcie_parse_request_of_pci_ranges() fails. > Is that ok? I think so, or did I miss something obvious? -- Best regards, Marek Vasut
Re: [PATCH V4] PCI: rcar: Use runtime PM to control controller clock
On Sun, Mar 18, 2018 at 11:52:09AM +0100, Marek Vasut wrote: > From: Dien Pham> > The controller clock can be switched off during suspend/resume, > let runtime PM take care of that. > > Signed-off-by: Dien Pham > Signed-off-by: Hien Dang > Signed-off-by: Marek Vasut > Cc: Geert Uytterhoeven > Cc: Lorenzo Pieralisi > Cc: Phil Edworthy > Cc: Simon Horman > Cc: Wolfram Sang > Cc: linux-renesas-soc@vger.kernel.org > To: linux-...@vger.kernel.org > --- > V2: - Reorder the fail path in rcar_pcie_probe() to cater for the > reordering of function calls in probe > - Dispose of fail_clk in rcar_pcie_get_resources() > V3: - Fix up the failpath in probe function > V4: - Rebase on recent linux-next > --- > drivers/pci/host/pcie-rcar.c | 40 > 1 file changed, 12 insertions(+), 28 deletions(-) > > diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c > index b4c4aad2cf66..93d59f15c589 100644 > --- a/drivers/pci/host/pcie-rcar.c > +++ b/drivers/pci/host/pcie-rcar.c > @@ -142,7 +142,6 @@ struct rcar_pcie { > void __iomem*base; > struct list_headresources; > int root_bus_nr; > - struct clk *clk; > struct clk *bus_clk; > struct rcar_msi msi; > }; > @@ -914,24 +913,14 @@ static int rcar_pcie_get_resources(struct rcar_pcie > *pcie) > if (IS_ERR(pcie->base)) > return PTR_ERR(pcie->base); > > - pcie->clk = devm_clk_get(dev, "pcie"); > - if (IS_ERR(pcie->clk)) { > - dev_err(dev, "cannot get platform clock\n"); > - return PTR_ERR(pcie->clk); > - } > - err = clk_prepare_enable(pcie->clk); > - if (err) > - return err; > - > pcie->bus_clk = devm_clk_get(dev, "pcie_bus"); > if (IS_ERR(pcie->bus_clk)) { > dev_err(dev, "cannot get pcie bus clock\n"); > - err = PTR_ERR(pcie->bus_clk); > - goto fail_clk; > + return PTR_ERR(pcie->bus_clk); > } > err = clk_prepare_enable(pcie->bus_clk); > if (err) > - goto fail_clk; > + return err; > > i = irq_of_parse_and_map(dev->of_node, 0); > if (!i) { > @@ -953,8 +942,6 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie) > > err_map_reg: > clk_disable_unprepare(pcie->bus_clk); > -fail_clk: > - clk_disable_unprepare(pcie->clk); > > return err; > } > @@ -1124,22 +,22 @@ static int rcar_pcie_probe(struct platform_device > *pdev) > if (err) > goto err_free_bridge; This error path now calls pci_free_resource_list() and in this case the path is taken if rcar_pcie_parse_request_of_pci_ranges() fails. Is that ok? > > + pm_runtime_enable(pcie->dev); > + err = pm_runtime_get_sync(pcie->dev); > + if (err < 0) { > + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); > + goto err_pm_disable; > + } > + > err = rcar_pcie_get_resources(pcie); > if (err < 0) { > dev_err(dev, "failed to request resources: %d\n", err); > - goto err_free_resource_list; > + goto err_pm_put; > } > > err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); > if (err) > - goto err_free_resource_list; > - > - pm_runtime_enable(dev); > - err = pm_runtime_get_sync(dev); > - if (err < 0) { > - dev_err(dev, "pm_runtime_get_sync failed\n"); > - goto err_pm_disable; > - } > + goto err_pm_put; > > /* Failure to get a link might just be that no cards are inserted */ > hw_init_fn = of_device_get_match_data(dev); > @@ -1171,13 +1158,10 @@ static int rcar_pcie_probe(struct platform_device > *pdev) > > err_pm_put: > pm_runtime_put(dev); > - > err_pm_disable: > pm_runtime_disable(dev); > - > -err_free_resource_list: > - pci_free_resource_list(>resources); > err_free_bridge: > + pci_free_resource_list(>resources); > pci_free_host_bridge(bridge); > > return err; > -- > 2.16.2 >
[PATCH V4] PCI: rcar: Use runtime PM to control controller clock
From: Dien PhamThe controller clock can be switched off during suspend/resume, let runtime PM take care of that. Signed-off-by: Dien Pham Signed-off-by: Hien Dang Signed-off-by: Marek Vasut Cc: Geert Uytterhoeven Cc: Lorenzo Pieralisi Cc: Phil Edworthy Cc: Simon Horman Cc: Wolfram Sang Cc: linux-renesas-soc@vger.kernel.org To: linux-...@vger.kernel.org --- V2: - Reorder the fail path in rcar_pcie_probe() to cater for the reordering of function calls in probe - Dispose of fail_clk in rcar_pcie_get_resources() V3: - Fix up the failpath in probe function V4: - Rebase on recent linux-next --- drivers/pci/host/pcie-rcar.c | 40 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index b4c4aad2cf66..93d59f15c589 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -142,7 +142,6 @@ struct rcar_pcie { void __iomem*base; struct list_headresources; int root_bus_nr; - struct clk *clk; struct clk *bus_clk; struct rcar_msi msi; }; @@ -914,24 +913,14 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie) if (IS_ERR(pcie->base)) return PTR_ERR(pcie->base); - pcie->clk = devm_clk_get(dev, "pcie"); - if (IS_ERR(pcie->clk)) { - dev_err(dev, "cannot get platform clock\n"); - return PTR_ERR(pcie->clk); - } - err = clk_prepare_enable(pcie->clk); - if (err) - return err; - pcie->bus_clk = devm_clk_get(dev, "pcie_bus"); if (IS_ERR(pcie->bus_clk)) { dev_err(dev, "cannot get pcie bus clock\n"); - err = PTR_ERR(pcie->bus_clk); - goto fail_clk; + return PTR_ERR(pcie->bus_clk); } err = clk_prepare_enable(pcie->bus_clk); if (err) - goto fail_clk; + return err; i = irq_of_parse_and_map(dev->of_node, 0); if (!i) { @@ -953,8 +942,6 @@ static int rcar_pcie_get_resources(struct rcar_pcie *pcie) err_map_reg: clk_disable_unprepare(pcie->bus_clk); -fail_clk: - clk_disable_unprepare(pcie->clk); return err; } @@ -1124,22 +,22 @@ static int rcar_pcie_probe(struct platform_device *pdev) if (err) goto err_free_bridge; + pm_runtime_enable(pcie->dev); + err = pm_runtime_get_sync(pcie->dev); + if (err < 0) { + dev_err(pcie->dev, "pm_runtime_get_sync failed\n"); + goto err_pm_disable; + } + err = rcar_pcie_get_resources(pcie); if (err < 0) { dev_err(dev, "failed to request resources: %d\n", err); - goto err_free_resource_list; + goto err_pm_put; } err = rcar_pcie_parse_map_dma_ranges(pcie, dev->of_node); if (err) - goto err_free_resource_list; - - pm_runtime_enable(dev); - err = pm_runtime_get_sync(dev); - if (err < 0) { - dev_err(dev, "pm_runtime_get_sync failed\n"); - goto err_pm_disable; - } + goto err_pm_put; /* Failure to get a link might just be that no cards are inserted */ hw_init_fn = of_device_get_match_data(dev); @@ -1171,13 +1158,10 @@ static int rcar_pcie_probe(struct platform_device *pdev) err_pm_put: pm_runtime_put(dev); - err_pm_disable: pm_runtime_disable(dev); - -err_free_resource_list: - pci_free_resource_list(>resources); err_free_bridge: + pci_free_resource_list(>resources); pci_free_host_bridge(bridge); return err; -- 2.16.2