| Message ID | 20210308195037.1503-3-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ata: ahci_brcm: Fix use of BCM7216 reset controller | expand |
Hi, Thank you for sending the patches over! > The Brcmstb PCIe RC uses a reset control "rescal" for certain chips. This [...] A small suggestion: it would be nicer to mention "Broadcom STB" rather than "Brcmstb" in the sentence above. [...] > +err1: > + reset_control_rearm(pcie->rescal); > +err0: > clk_disable_unprepare(pcie->clk); > return ret; [...] A small nitpick. Now that there are two labels on the error recovery path, it might be better to name both of these labels a little bit better. Some examples from the PCI tree: error_clock_unprepare err_disable_clock err_disable_clk err_clk_disable So it could be: err_reset: <-- or err_rearm or even err_reset_rearm, etc. reset_control_rearm(pcie->rescal); err_disable_clk: clk_disable_unprepare(pcie->clk); What do you think? Krzysztof
If you update this, please fix the s/nPCI: /PCI: / in the subject On Mon, Mar 08, 2021 at 02:50:37PM -0500, Jim Quinlan wrote: > The Brcmstb PCIe RC uses a reset control "rescal" for certain chips. This > reset implements a "pulse reset" so it matches more the reset/rearm > calls instead of the deassert/assert calls. You say "also" below, but the paragraph above doesn't tell us the *first* thing this patch does. It just tells us that some chips use "rescal" and that "rescal" implements a "pulse reset". I guess you're replacing reset_control_deassert() with reset_control_reset(), and reset_control_assert() with reset_control_rearm(). It's not obvious to me that those are equivalent or why it's safe to do this for all chips, including those that don't use the "rescal" (since it sounds like only certain chips have that). > Also, add reset_control calls in suspend/resume functions. > > Fixes: 740d6c3708a9 ("PCI: brcmstb: Add control of rescal reset") > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index e330e6811f0b..18f23cba7e3a 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -1148,6 +1148,7 @@ static int brcm_pcie_suspend(struct device *dev) > > brcm_pcie_turn_off(pcie); > ret = brcm_phy_stop(pcie); > + reset_control_rearm(pcie->rescal); > clk_disable_unprepare(pcie->clk); > > return ret; > @@ -1163,9 +1164,13 @@ static int brcm_pcie_resume(struct device *dev) > base = pcie->base; > clk_prepare_enable(pcie->clk); > > + ret = reset_control_reset(pcie->rescal); > + if (ret) > + goto err0; > + > ret = brcm_phy_start(pcie); > if (ret) > - goto err; > + goto err1; > > /* Take bridge out of reset so we can access the SERDES reg */ > pcie->bridge_sw_init_set(pcie, 0); > @@ -1180,14 +1185,16 @@ static int brcm_pcie_resume(struct device *dev) > > ret = brcm_pcie_setup(pcie); > if (ret) > - goto err; > + goto err1; > > if (pcie->msi) > brcm_msi_set_regs(pcie->msi); > > return 0; > > -err: > +err1: > + reset_control_rearm(pcie->rescal); > +err0: > clk_disable_unprepare(pcie->clk); > return ret; > } > @@ -1197,7 +1204,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) > brcm_msi_remove(pcie); > brcm_pcie_turn_off(pcie); > brcm_phy_stop(pcie); > - reset_control_assert(pcie->rescal); > + reset_control_rearm(pcie->rescal); > clk_disable_unprepare(pcie->clk); > } > > @@ -1278,13 +1285,13 @@ static int brcm_pcie_probe(struct platform_device *pdev) > return PTR_ERR(pcie->perst_reset); > } > > - ret = reset_control_deassert(pcie->rescal); > + ret = reset_control_reset(pcie->rescal); > if (ret) > dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); > > ret = brcm_phy_start(pcie); > if (ret) { > - reset_control_assert(pcie->rescal); > + reset_control_rearm(pcie->rescal); > clk_disable_unprepare(pcie->clk); > return ret; > } > -- > 2.17.1 >
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index e330e6811f0b..18f23cba7e3a 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -1148,6 +1148,7 @@ static int brcm_pcie_suspend(struct device *dev) brcm_pcie_turn_off(pcie); ret = brcm_phy_stop(pcie); + reset_control_rearm(pcie->rescal); clk_disable_unprepare(pcie->clk); return ret; @@ -1163,9 +1164,13 @@ static int brcm_pcie_resume(struct device *dev) base = pcie->base; clk_prepare_enable(pcie->clk); + ret = reset_control_reset(pcie->rescal); + if (ret) + goto err0; + ret = brcm_phy_start(pcie); if (ret) - goto err; + goto err1; /* Take bridge out of reset so we can access the SERDES reg */ pcie->bridge_sw_init_set(pcie, 0); @@ -1180,14 +1185,16 @@ static int brcm_pcie_resume(struct device *dev) ret = brcm_pcie_setup(pcie); if (ret) - goto err; + goto err1; if (pcie->msi) brcm_msi_set_regs(pcie->msi); return 0; -err: +err1: + reset_control_rearm(pcie->rescal); +err0: clk_disable_unprepare(pcie->clk); return ret; } @@ -1197,7 +1204,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie) brcm_msi_remove(pcie); brcm_pcie_turn_off(pcie); brcm_phy_stop(pcie); - reset_control_assert(pcie->rescal); + reset_control_rearm(pcie->rescal); clk_disable_unprepare(pcie->clk); } @@ -1278,13 +1285,13 @@ static int brcm_pcie_probe(struct platform_device *pdev) return PTR_ERR(pcie->perst_reset); } - ret = reset_control_deassert(pcie->rescal); + ret = reset_control_reset(pcie->rescal); if (ret) dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); ret = brcm_phy_start(pcie); if (ret) { - reset_control_assert(pcie->rescal); + reset_control_rearm(pcie->rescal); clk_disable_unprepare(pcie->clk); return ret; }
The Brcmstb PCIe RC uses a reset control "rescal" for certain chips. This reset implements a "pulse reset" so it matches more the reset/rearm calls instead of the deassert/assert calls. Also, add reset_control calls in suspend/resume functions. Fixes: 740d6c3708a9 ("PCI: brcmstb: Add control of rescal reset") Signed-off-by: Jim Quinlan <jim2101024@gmail.com> Acked-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)