| Message ID | 20240703180300.42959-5-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: brcnstb: Enable STB 7712 SOC | expand |
Hi Jim, On 7/3/24 21:02, Jim Quinlan wrote: > The 7712 SOC adds a software init reset device for the PCIe HW. > If found in the DT node, use it. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 4104c3668fdb..69926ee5c961 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -266,6 +266,7 @@ struct brcm_pcie { > struct reset_control *rescal; > struct reset_control *perst_reset; > struct reset_control *bridge; > + struct reset_control *swinit; > int num_memc; > u64 memc_size[PCIE_BRCM_MAX_MEMC]; > u32 hw_rev; > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev) > dev_err(&pdev->dev, "could not enable clock\n"); > return ret; > } > + > + pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit"); > + if (IS_ERR(pcie->swinit)) { > + ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit), > + "failed to get 'swinit' reset\n"); > + goto clk_out; > + } > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); > if (IS_ERR(pcie->rescal)) { > ret = PTR_ERR(pcie->rescal); > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev) > goto clk_out; > } > > + ret = reset_control_assert(pcie->swinit); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"); > + goto clk_out; > + } > + ret = reset_control_deassert(pcie->swinit); > + if (ret) { > + dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n"); > + goto clk_out; > + } why not call reset_control_reset(pcie->swinit) directly? ~Stan > + > ret = reset_control_reset(pcie->rescal); > if (ret) { > dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
Hi, On 7/3/24 21:02, Jim Quinlan wrote: > The 7712 SOC adds a software init reset device for the PCIe HW. > If found in the DT node, use it. > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > index 4104c3668fdb..69926ee5c961 100644 > --- a/drivers/pci/controller/pcie-brcmstb.c > +++ b/drivers/pci/controller/pcie-brcmstb.c > @@ -266,6 +266,7 @@ struct brcm_pcie { > struct reset_control *rescal; > struct reset_control *perst_reset; > struct reset_control *bridge; > + struct reset_control *swinit; > int num_memc; > u64 memc_size[PCIE_BRCM_MAX_MEMC]; > u32 hw_rev; > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev) > dev_err(&pdev->dev, "could not enable clock\n"); > return ret; > } > + > + pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit"); > + if (IS_ERR(pcie->swinit)) { > + ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit), > + "failed to get 'swinit' reset\n"); Why "swinit" reset is special in this series? For example previous 3/12 patch which add "bridge" reset does not use dev_err_probe. IMO you should use dev_err() here and below, and make a new follow-up patch which change dev_err -> dev_err_probe on all places where getting resources like resets and clocks. > + goto clk_out; > + } > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); > if (IS_ERR(pcie->rescal)) { > ret = PTR_ERR(pcie->rescal); > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev) > goto clk_out; > } > > + ret = reset_control_assert(pcie->swinit); > + if (ret) { > + dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"); > + goto clk_out; ditto > + } > + ret = reset_control_deassert(pcie->swinit); > + if (ret) { > + dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n"); > + goto clk_out; this is fine > + } > + > ret = reset_control_reset(pcie->rescal); > if (ret) { > dev_err(&pdev->dev, "failed to deassert 'rescal'\n"); ~Stan
On Thu, Jul 4, 2024 at 8:56 AM Stanimir Varbanov <svarbanov@suse.de> wrote: > > Hi Jim, > > On 7/3/24 21:02, Jim Quinlan wrote: > > The 7712 SOC adds a software init reset device for the PCIe HW. > > If found in the DT node, use it. > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > --- > > drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > index 4104c3668fdb..69926ee5c961 100644 > > --- a/drivers/pci/controller/pcie-brcmstb.c > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > @@ -266,6 +266,7 @@ struct brcm_pcie { > > struct reset_control *rescal; > > struct reset_control *perst_reset; > > struct reset_control *bridge; > > + struct reset_control *swinit; > > int num_memc; > > u64 memc_size[PCIE_BRCM_MAX_MEMC]; > > u32 hw_rev; > > @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > dev_err(&pdev->dev, "could not enable clock\n"); > > return ret; > > } > > + > > + pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit"); > > + if (IS_ERR(pcie->swinit)) { > > + ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit), > > + "failed to get 'swinit' reset\n"); > > + goto clk_out; > > + } > > pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); > > if (IS_ERR(pcie->rescal)) { > > ret = PTR_ERR(pcie->rescal); > > @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev) > > goto clk_out; > > } > > > > + ret = reset_control_assert(pcie->swinit); > > + if (ret) { > > + dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"); > > + goto clk_out; > > + } > > + ret = reset_control_deassert(pcie->swinit); > > + if (ret) { > > + dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n"); > > + goto clk_out; > > + } > > why not call reset_control_reset(pcie->swinit) directly? Hi Stan, There is no reset_control_reset() method defined for reset-brcmstb.c. The only reason I can think of for this is that it allows the callers of assert/deassert to insert a delay if desired. Regards, Jim Quinlan Broadcom STB/CM > > ~Stan > > + > > ret = reset_control_reset(pcie->rescal); > > if (ret) { > > dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c index 4104c3668fdb..69926ee5c961 100644 --- a/drivers/pci/controller/pcie-brcmstb.c +++ b/drivers/pci/controller/pcie-brcmstb.c @@ -266,6 +266,7 @@ struct brcm_pcie { struct reset_control *rescal; struct reset_control *perst_reset; struct reset_control *bridge; + struct reset_control *swinit; int num_memc; u64 memc_size[PCIE_BRCM_MAX_MEMC]; u32 hw_rev; @@ -1626,6 +1627,13 @@ static int brcm_pcie_probe(struct platform_device *pdev) dev_err(&pdev->dev, "could not enable clock\n"); return ret; } + + pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit"); + if (IS_ERR(pcie->swinit)) { + ret = dev_err_probe(&pdev->dev, PTR_ERR(pcie->swinit), + "failed to get 'swinit' reset\n"); + goto clk_out; + } pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal"); if (IS_ERR(pcie->rescal)) { ret = PTR_ERR(pcie->rescal); @@ -1637,6 +1645,17 @@ static int brcm_pcie_probe(struct platform_device *pdev) goto clk_out; } + ret = reset_control_assert(pcie->swinit); + if (ret) { + dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"); + goto clk_out; + } + ret = reset_control_deassert(pcie->swinit); + if (ret) { + dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n"); + goto clk_out; + } + ret = reset_control_reset(pcie->rescal); if (ret) { dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
The 7712 SOC adds a software init reset device for the PCIe HW. If found in the DT node, use it. Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> --- drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)