| Message ID | 20240731222831.14895-4-james.quinlan@broadcom.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcnstb: Enable STB 7712 SOC | expand |
On 7/31/24 15:28, Jim Quinlan wrote: > o Move the clk_prepare_enable() below the resource allocations. > o Move the clk_prepare_enable() out of __brcm_pcie_remove() but > add it to the end of brcm_pcie_remove(). > o Add a jump target (clk_disable_unprepare) so that a bit of exception > handling can be better reused at the end of this function implementation. > o Use dev_err_probe() where it makes sense. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
On Wed, Jul 31, 2024 at 06:28:17PM -0400, Jim Quinlan wrote: > o Move the clk_prepare_enable() below the resource allocations. > o Move the clk_prepare_enable() out of __brcm_pcie_remove() but > add it to the end of brcm_pcie_remove(). > o Add a jump target (clk_disable_unprepare) so that a bit of exception > handling can be better reused at the end of this function implementation. > o Use dev_err_probe() where it makes sense. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++--------------- > 1 file changed, 15 insertions(+), 19 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index c08683febdd4..7595e7009192 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) > dev_err(pcie->dev, "Could not stop phy\n"); > if (reset_control_rearm(pcie->rescal)) > dev_err(pcie->dev, "Could not rearm rescal reset\n"); > - clk_disable_unprepare(pcie->clk); > } > > static void brcm_pcie_remove(struct platform_device *pdev) > @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev) > pci_stop_root_bus(bridge->bus); > pci_remove_root_bus(bridge->bus); > __brcm_pcie_remove(pcie); > + clk_disable_unprepare(pcie->clk); > } > > static const int pcie_offsets[] = { > @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc"); > > - ret = clk_prepare_enable(pcie->clk); > - if (ret) { > - dev_err(&pdev->dev, "could not enable clock\n"); > - return ret; > - } > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); > - if (IS_ERR(pcie->rescal)) { > - clk_disable_unprepare(pcie->clk); > + if (IS_ERR(pcie->rescal)) > return PTR_ERR(pcie->rescal); > - } > + > pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst"); > - if (IS_ERR(pcie->perst_reset)) { > - clk_disable_unprepare(pcie->clk); > + if (IS_ERR(pcie->perst_reset)) > return PTR_ERR(pcie->perst_reset); > - } > > - ret = reset_control_reset(pcie->rescal); > + ret = clk_prepare_enable(pcie->clk); > if (ret) > - dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); > + return dev_err_probe(&pdev->dev, ret, "could not enable clock\n"); > + > + ret = reset_control_reset(pcie->rescal); > + if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) > + goto clk_disable_unprepare; > > ret = brcm_phy_start(pcie); > if (ret) { > reset_control_rearm(pcie->rescal); > - clk_disable_unprepare(pcie->clk); > - return ret; > + goto clk_disable_unprepare; > } > > ret = brcm_pcie_setup(pcie); > @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) > msi_np = of_parse_phandle(pcie->np, "msi-parent", 0); > if (pci_msi_enabled() && msi_np == pcie->np) { > ret = brcm_pcie_enable_msi(pcie); > - if (ret) { > - dev_err(pcie->dev, "probe of internal MSI failed"); > + if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed")) > goto fail; > - } > } > > bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops; > @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > fail: > __brcm_pcie_remove(pcie); > +clk_disable_unprepare: > + clk_disable_unprepare(pcie->clk); > + TBH, this is not improving the code readability. __brcm_pcie_remove() used to free all the resources and now you just moved clk_disable_unprepare() out of it to save 2 lines in probe(). And you ended up calling clk_disable_unprepare() separately in brcm_pcie_remove(). So please remove the label and call clk_disable_unprepare() in the error path (just 2 instances) and continue to use __brcm_pcie_remove() to free all resources (I would've preferred to have separate error labels instead of calling __brcm_pcie_remove() though, but not this). - Mani
On Wed, Jul 31, 2024 at 06:28:17PM -0400, Jim Quinlan wrote: > o Move the clk_prepare_enable() below the resource allocations. > o Move the clk_prepare_enable() out of __brcm_pcie_remove() but > add it to the end of brcm_pcie_remove(). > o Add a jump target (clk_disable_unprepare) so that a bit of exception > handling can be better reused at the end of this function implementation. > o Use dev_err_probe() where it makes sense. > Thanks for using the imperative tone. It would be nice to have the patch description as a single paragraph in a continuous manner instead of bullet points. - Mani > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++--------------- > 1 file changed, 15 insertions(+), 19 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index c08683febdd4..7595e7009192 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) > dev_err(pcie->dev, "Could not stop phy\n"); > if (reset_control_rearm(pcie->rescal)) > dev_err(pcie->dev, "Could not rearm rescal reset\n"); > - clk_disable_unprepare(pcie->clk); > } > > static void brcm_pcie_remove(struct platform_device *pdev) > @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev) > pci_stop_root_bus(bridge->bus); > pci_remove_root_bus(bridge->bus); > __brcm_pcie_remove(pcie); > + clk_disable_unprepare(pcie->clk); > } > > static const int pcie_offsets[] = { > @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc"); > > - ret = clk_prepare_enable(pcie->clk); > - if (ret) { > - dev_err(&pdev->dev, "could not enable clock\n"); > - return ret; > - } > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); > - if (IS_ERR(pcie->rescal)) { > - clk_disable_unprepare(pcie->clk); > + if (IS_ERR(pcie->rescal)) > return PTR_ERR(pcie->rescal); > - } > + > pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst"); > - if (IS_ERR(pcie->perst_reset)) { > - clk_disable_unprepare(pcie->clk); > + if (IS_ERR(pcie->perst_reset)) > return PTR_ERR(pcie->perst_reset); > - } > > - ret = reset_control_reset(pcie->rescal); > + ret = clk_prepare_enable(pcie->clk); > if (ret) > - dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); > + return dev_err_probe(&pdev->dev, ret, "could not enable clock\n"); > + > + ret = reset_control_reset(pcie->rescal); > + if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) > + goto clk_disable_unprepare; > > ret = brcm_phy_start(pcie); > if (ret) { > reset_control_rearm(pcie->rescal); > - clk_disable_unprepare(pcie->clk); > - return ret; > + goto clk_disable_unprepare; > } > > ret = brcm_pcie_setup(pcie); > @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) > msi_np = of_parse_phandle(pcie->np, "msi-parent", 0); > if (pci_msi_enabled() && msi_np == pcie->np) { > ret = brcm_pcie_enable_msi(pcie); > - if (ret) { > - dev_err(pcie->dev, "probe of internal MSI failed"); > + if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed")) > goto fail; > - } > } > > bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops; > @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > fail: > __brcm_pcie_remove(pcie); > +clk_disable_unprepare: > + clk_disable_unprepare(pcie->clk); > + > return ret; > } > > -- > 2.17.1 >
On Tue, Aug 6, 2024 at 10:53 PM Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Wed, Jul 31, 2024 at 06:28:17PM -0400, Jim Quinlan wrote: > > o Move the clk_prepare_enable() below the resource allocations. > > o Move the clk_prepare_enable() out of __brcm_pcie_remove() but > > add it to the end of brcm_pcie_remove(). > > o Add a jump target (clk_disable_unprepare) so that a bit of exception > > handling can be better reused at the end of this function implementation. > > o Use dev_err_probe() where it makes sense. > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++--------------- > > 1 file changed, 15 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index c08683febdd4..7595e7009192 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) > > dev_err(pcie->dev, "Could not stop phy\n"); > > if (reset_control_rearm(pcie->rescal)) > > dev_err(pcie->dev, "Could not rearm rescal reset\n"); > > - clk_disable_unprepare(pcie->clk); > > } > > > > static void brcm_pcie_remove(struct platform_device *pdev) > > @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev) > > pci_stop_root_bus(bridge->bus); > > pci_remove_root_bus(bridge->bus); > > __brcm_pcie_remove(pcie); > > + clk_disable_unprepare(pcie->clk); > > } > > > > static const int pcie_offsets[] = { > > @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > > > pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc"); > > > > - ret = clk_prepare_enable(pcie->clk); > > - if (ret) { > > - dev_err(&pdev->dev, "could not enable clock\n"); > > - return ret; > > - } > > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); > > - if (IS_ERR(pcie->rescal)) { > > - clk_disable_unprepare(pcie->clk); > > + if (IS_ERR(pcie->rescal)) > > return PTR_ERR(pcie->rescal); > > - } > > + > > pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst"); > > - if (IS_ERR(pcie->perst_reset)) { > > - clk_disable_unprepare(pcie->clk); > > + if (IS_ERR(pcie->perst_reset)) > > return PTR_ERR(pcie->perst_reset); > > - } > > > > - ret = reset_control_reset(pcie->rescal); > > + ret = clk_prepare_enable(pcie->clk); > > if (ret) > > - dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); > > + return dev_err_probe(&pdev->dev, ret, "could not enable clock\n"); > > + > > + ret = reset_control_reset(pcie->rescal); > > + if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) > > + goto clk_disable_unprepare; > > > > ret = brcm_phy_start(pcie); > > if (ret) { > > reset_control_rearm(pcie->rescal); > > - clk_disable_unprepare(pcie->clk); > > - return ret; > > + goto clk_disable_unprepare; > > } > > > > ret = brcm_pcie_setup(pcie); > > @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > msi_np = of_parse_phandle(pcie->np, "msi-parent", 0); > > if (pci_msi_enabled() && msi_np == pcie->np) { > > ret = brcm_pcie_enable_msi(pcie); > > - if (ret) { > > - dev_err(pcie->dev, "probe of internal MSI failed"); > > + if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed")) > > goto fail; > > - } > > } > > > > bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops; > > @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > > > fail: > > __brcm_pcie_remove(pcie); > > +clk_disable_unprepare: > > + clk_disable_unprepare(pcie->clk); > > + > > TBH, this is not improving the code readability. __brcm_pcie_remove() used to > free all the resources and now you just moved clk_disable_unprepare() out of it > to save 2 lines in probe(). And you ended up calling clk_disable_unprepare() > separately in brcm_pcie_remove(). Actually it saves more lines than that; the "swinit reset" commit adds two more "goto clk_disable_unprepare" instances. Nonetheless I will make the change Regards, Jim Quinlan Broadcom STB/CM > > So please remove the label and call clk_disable_unprepare() in the error path > (just 2 instances) and continue to use __brcm_pcie_remove() to free all > resources (I would've preferred to have separate error labels instead of calling > __brcm_pcie_remove() though, but not this). > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
Hi Jim, On 8/1/24 01:28, Jim Quinlan wrote: > o Move the clk_prepare_enable() below the resource allocations. > o Move the clk_prepare_enable() out of __brcm_pcie_remove() but > add it to the end of brcm_pcie_remove(). > o Add a jump target (clk_disable_unprepare) so that a bit of exception > handling can be better reused at the end of this function implementation. > o Use dev_err_probe() where it makes sense. Those dev_err_probe produce these errors on RPi5: rpi5:~ # dmesg -l err [ 1.004960] brcm-pcie 1000110000.pcie: error 0000000000000000: could not assert reset 'swinit' [ 1.013812] brcm-pcie 1000110000.pcie: error 0000000000000000: could not de-assert reset 'swinit' after asserting [ 1.024222] brcm-pcie 1000110000.pcie: error 0000000000000000: failed to deassert 'rescal' [ 1.533839] brcm-pcie 1000110000.pcie: link down [ 1.627564] brcm-pcie 1000120000.pcie: error 0000000000000000: could not assert reset 'swinit' [ 1.636415] brcm-pcie 1000120000.pcie: error 0000000000000000: could not de-assert reset 'swinit' after asserting [ 1.646829] brcm-pcie 1000120000.pcie: error 0000000000000000: failed to deassert 'rescal' ... as you can see there is no error at all. ~Stan > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++--------------- > 1 file changed, 15 insertions(+), 19 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index c08683febdd4..7595e7009192 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) > dev_err(pcie->dev, "Could not stop phy\n"); > if (reset_control_rearm(pcie->rescal)) > dev_err(pcie->dev, "Could not rearm rescal reset\n"); > - clk_disable_unprepare(pcie->clk); > } > > static void brcm_pcie_remove(struct platform_device *pdev) > @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev) > pci_stop_root_bus(bridge->bus); > pci_remove_root_bus(bridge->bus); > __brcm_pcie_remove(pcie); > + clk_disable_unprepare(pcie->clk); > } > > static const int pcie_offsets[] = { > @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc"); > > - ret = clk_prepare_enable(pcie->clk); > - if (ret) { > - dev_err(&pdev->dev, "could not enable clock\n"); > - return ret; > - } > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); > - if (IS_ERR(pcie->rescal)) { > - clk_disable_unprepare(pcie->clk); > + if (IS_ERR(pcie->rescal)) > return PTR_ERR(pcie->rescal); > - } > + > pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst"); > - if (IS_ERR(pcie->perst_reset)) { > - clk_disable_unprepare(pcie->clk); > + if (IS_ERR(pcie->perst_reset)) > return PTR_ERR(pcie->perst_reset); > - } > > - ret = reset_control_reset(pcie->rescal); > + ret = clk_prepare_enable(pcie->clk); > if (ret) > - dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); > + return dev_err_probe(&pdev->dev, ret, "could not enable clock\n"); > + > + ret = reset_control_reset(pcie->rescal); > + if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) > + goto clk_disable_unprepare; > > ret = brcm_phy_start(pcie); > if (ret) { > reset_control_rearm(pcie->rescal); > - clk_disable_unprepare(pcie->clk); > - return ret; > + goto clk_disable_unprepare; > } > > ret = brcm_pcie_setup(pcie); > @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) > msi_np = of_parse_phandle(pcie->np, "msi-parent", 0); > if (pci_msi_enabled() && msi_np == pcie->np) { > ret = brcm_pcie_enable_msi(pcie); > - if (ret) { > - dev_err(pcie->dev, "probe of internal MSI failed"); > + if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed")) > goto fail; > - } > } > > bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops; > @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > fail: > __brcm_pcie_remove(pcie); > +clk_disable_unprepare: > + clk_disable_unprepare(pcie->clk); > + > return ret; > } >
On 8/13/24 12:45, Stanimir Varbanov wrote: > Hi Jim, > > On 8/1/24 01:28, Jim Quinlan wrote: >> o Move the clk_prepare_enable() below the resource allocations. >> o Move the clk_prepare_enable() out of __brcm_pcie_remove() but >> add it to the end of brcm_pcie_remove(). >> o Add a jump target (clk_disable_unprepare) so that a bit of exception >> handling can be better reused at the end of this function implementation. >> o Use dev_err_probe() where it makes sense.PASSWORD > Those dev_err_probe produce these errors on RPi5: Hi Stan, Sorry, I clearly missed that. My perception of the dev_err_probe() semantics were incorrect -- I thought it didn't print anything at all if the "ret" param was zero, which would allow it to be used as an "if" conditional, as I am doing. Obviously not the case. Will fix. BTW I do not yet possess a CM5 so my testing is done on a CM4, 7712, and other STB chips. Regards and thanks, Jim Quinlan Broadcom STB/CM > > rpi5:~ # dmesg -l err > [ 1.004960] brcm-pcie 1000110000.pcie: error 0000000000000000: could > not assert reset 'swinit' > [ 1.013812] brcm-pcie 1000110000.pcie: error 0000000000000000: could > not de-assert reset 'swinit' after asserting > [ 1.024222] brcm-pcie 1000110000.pcie: error 0000000000000000: failed > to deassert 'rescal' > [ 1.533839] brcm-pcie 1000110000.pcie: link down > [ 1.627564] brcm-pcie 1000120000.pcie: error 0000000000000000: could > not assert reset 'swinit' > [ 1.636415] brcm-pcie 1000120000.pcie: error 0000000000000000: could > not de-assert reset 'swinit' after asserting > [ 1.646829] brcm-pcie 1000120000.pcie: error 0000000000000000: failed > to deassert 'rescal' > > ... as you can see there is no error at all. > > ~Stan > >> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> >> --- >> drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++--------------- >> 1 file changed, 15 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c >> index c08683febdd4..7595e7009192 100644 >> --- a/drivers/pci/controller/pcie-brcmstb.c >> +++ b/drivers/pci/controller/pcie-brcmstb.c >> @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) >> dev_err(pcie->dev, "Could not stop phy\n"); >> if (reset_control_rearm(pcie->rescal)) >> dev_err(pcie->dev, "Could not rearm rescal reset\n"); >> - clk_disable_unprepare(pcie->clk); >> } >> >> static void brcm_pcie_remove(struct platform_device *pdev) >> @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev) >> pci_stop_root_bus(bridge->bus); >> pci_remove_root_bus(bridge->bus); >> __brcm_pcie_remove(pcie); >> + clk_disable_unprepare(pcie->clk); >> } >> >> static const int pcie_offsets[] = { >> @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev) >> >> pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc"); >> >> - ret = clk_prepare_enable(pcie->clk); >> - if (ret) { >> - dev_err(&pdev->dev, "could not enable clock\n"); >> - return ret; >> - } >> pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); >> - if (IS_ERR(pcie->rescal)) { >> - clk_disable_unprepare(pcie->clk); >> + if (IS_ERR(pcie->rescal)) >> return PTR_ERR(pcie->rescal); >> - } >> + >> pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst"); >> - if (IS_ERR(pcie->perst_reset)) { >> - clk_disable_unprepare(pcie->clk); >> + if (IS_ERR(pcie->perst_reset)) >> return PTR_ERR(pcie->perst_reset); >> - } >> >> - ret = reset_control_reset(pcie->rescal); >> + ret = clk_prepare_enable(pcie->clk); >> if (ret) >> - dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); >> + return dev_err_probe(&pdev->dev, ret, "could not enable clock\n"); >> + >> + ret = reset_control_reset(pcie->rescal); >> + if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) >> + goto clk_disable_unprepare; >> >> ret = brcm_phy_start(pcie); >> if (ret) { >> reset_control_rearm(pcie->rescal); >> - clk_disable_unprepare(pcie->clk); >> - return ret; >> + goto clk_disable_unprepare; >> } >> >> ret = brcm_pcie_setup(pcie); >> @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) >> msi_np = of_parse_phandle(pcie->np, "msi-parent", 0); >> if (pci_msi_enabled() && msi_np == pcie->np) { >> ret = brcm_pcie_enable_msi(pcie); >> - if (ret) { >> - dev_err(pcie->dev, "probe of internal MSI failed"); >> + if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed")) >> goto fail; >> - } >> } >> >> bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops; >> @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) >> >> fail: >> __brcm_pcie_remove(pcie); >> +clk_disable_unprepare: >> + clk_disable_unprepare(pcie->clk); >> + >> return ret; >> } >>
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index c08683febdd4..7595e7009192 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) dev_err(pcie->dev, "Could not stop phy\n"); if (reset_control_rearm(pcie->rescal)) dev_err(pcie->dev, "Could not rearm rescal reset\n"); - clk_disable_unprepare(pcie->clk); } static void brcm_pcie_remove(struct platform_device *pdev) @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev) pci_stop_root_bus(bridge->bus); pci_remove_root_bus(bridge->bus); __brcm_pcie_remove(pcie); + clk_disable_unprepare(pcie->clk); } static const int pcie_offsets[] = { @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev) pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc"); - ret = clk_prepare_enable(pcie->clk); - if (ret) { - dev_err(&pdev->dev, "could not enable clock\n"); - return ret; - } pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); - if (IS_ERR(pcie->rescal)) { - clk_disable_unprepare(pcie->clk); + if (IS_ERR(pcie->rescal)) return PTR_ERR(pcie->rescal); - } + pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst"); - if (IS_ERR(pcie->perst_reset)) { - clk_disable_unprepare(pcie->clk); + if (IS_ERR(pcie->perst_reset)) return PTR_ERR(pcie->perst_reset); - } - ret = reset_control_reset(pcie->rescal); + ret = clk_prepare_enable(pcie->clk); if (ret) - dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); + return dev_err_probe(&pdev->dev, ret, "could not enable clock\n"); + + ret = reset_control_reset(pcie->rescal); + if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n")) + goto clk_disable_unprepare; ret = brcm_phy_start(pcie); if (ret) { reset_control_rearm(pcie->rescal); - clk_disable_unprepare(pcie->clk); - return ret; + goto clk_disable_unprepare; } ret = brcm_pcie_setup(pcie); @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev) msi_np = of_parse_phandle(pcie->np, "msi-parent", 0); if (pci_msi_enabled() && msi_np == pcie->np) { ret = brcm_pcie_enable_msi(pcie); - if (ret) { - dev_err(pcie->dev, "probe of internal MSI failed"); + if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed")) goto fail; - } } bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops; @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev) fail: __brcm_pcie_remove(pcie); +clk_disable_unprepare: + clk_disable_unprepare(pcie->clk); + return ret; }
o Move the clk_prepare_enable() below the resource allocations. o Move the clk_prepare_enable() out of __brcm_pcie_remove() but add it to the end of brcm_pcie_remove(). o Add a jump target (clk_disable_unprepare) so that a bit of exception handling can be better reused at the end of this function implementation. o Use dev_err_probe() where it makes sense. Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> --- drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++--------------- 1 file changed, 15 insertions(+), 19 deletions(-)