| Message ID | 20240703180300.42959-5-james.quinlan@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| 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");
On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote: > 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. The main reason for the existence of reset_control_reset() is that there are reset controllers that can only be triggered (e.g. by writing a bit to a self-clearing register) to produce a complete reset pulse, with assertion, delay, and deassertion all handled by the reset controller. regards Philipp
Hi Philipp, On 7/8/24 12:37, Philipp Zabel wrote: > On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote: >> 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. > > The main reason for the existence of reset_control_reset() is that > there are reset controllers that can only be triggered (e.g. by writing > a bit to a self-clearing register) to produce a complete reset pulse, > with assertion, delay, and deassertion all handled by the reset > controller. Got it. Thank you for explanation. But, IMO that means that the consumer driver should have knowledge of low-level reset implementation, which is not generic API? Otherwise, I don't see a problem to implement asset/deassert sequence in .reset op in this particular reset-brcmstb.c low-level driver. ~Stan
Hi Stanimir, On Mo, 2024-07-08 at 14:14 +0300, Stanimir Varbanov wrote: > Hi Philipp, > > On 7/8/24 12:37, Philipp Zabel wrote: > > On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote: > > > 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. > > > > The main reason for the existence of reset_control_reset() is that > > there are reset controllers that can only be triggered (e.g. by writing > > a bit to a self-clearing register) to produce a complete reset pulse, > > with assertion, delay, and deassertion all handled by the reset > > controller. > > Got it. Thank you for explanation. > > But, IMO that means that the consumer driver should have knowledge of > low-level reset implementation, which is not generic API? Kind of. If the reset controller hardware has self-clearing resets, it is impossible to implement manual reset_control_assert/deassert() on top. So if a reset consumer requires that level of control, it just can't work with a self-deasserting controller. The other way around, a reset controller driver can emulate self-deasserting resets, iff it knows the timing requirements of all consumers. If the reset consumer only needs to see a pulse on the reset line, and there are no ordering requirements with other resets or clocks, and the device either doesn't care about timing or the reset controller knows how to produce the required delay, then using reset_control_reset() would be semantically correct. > Otherwise, I don't see a problem to implement asset/deassert sequence in > .reset op in this particular reset-brcmstb.c low-level driver. When reset_control_reset() is used, every reset controller that can be paired with this consumer needs to implement the .reset method, requiring to know the delay requirements for all of their consumers. The reset-simple driver implements this with a configurable worst-case delay, for example. As far as I can see, that has never been used. So yes, in this particular case, pcie-brcmstb only ever being paired with reset-brcmstb, it might be no problem to implement .reset in reset-brcmstb correctly, if all its consumers and their required delays are known. regards Philipp
On Mon, Jul 8, 2024 at 9:26 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Stanimir, > > On Mo, 2024-07-08 at 14:14 +0300, Stanimir Varbanov wrote: > > Hi Philipp, > > > > On 7/8/24 12:37, Philipp Zabel wrote: > > > On Fr, 2024-07-05 at 13:46 -0400, Jim Quinlan wrote: > > > > 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. > > > > > > The main reason for the existence of reset_control_reset() is that > > > there are reset controllers that can only be triggered (e.g. by writing > > > a bit to a self-clearing register) to produce a complete reset pulse, > > > with assertion, delay, and deassertion all handled by the reset > > > controller. > > > > Got it. Thank you for explanation. > > > > But, IMO that means that the consumer driver should have knowledge of > > low-level reset implementation, which is not generic API? > > Kind of. If the reset controller hardware has self-clearing resets, it > is impossible to implement manual reset_control_assert/deassert() on > top. So if a reset consumer requires that level of control, it just > can't work with a self-deasserting controller. The other way around, a > reset controller driver can emulate self-deasserting resets, iff it > knows the timing requirements of all consumers. > > If the reset consumer only needs to see a pulse on the reset line, and > there are no ordering requirements with other resets or clocks, and the > device either doesn't care about timing or the reset controller knows > how to produce the required delay, then using reset_control_reset() > would be semantically correct. > > > Otherwise, I don't see a problem to implement asset/deassert sequence in > > .reset op in this particular reset-brcmstb.c low-level driver. > > When reset_control_reset() is used, every reset controller that can be > paired with this consumer needs to implement the .reset method, > requiring to know the delay requirements for all of their consumers. > The reset-simple driver implements this with a configurable worst-case > delay, for example. As far as I can see, that has never been used. > > So yes, in this particular case, pcie-brcmstb only ever being paired > with reset-brcmstb, it might be no problem to implement .reset in > reset-brcmstb correctly, if all its consumers and their required delays > are known. This will not work. reset-brcmstb.c is a reset provider; it provides resets for dozens of different HW blocks. Forcing dozens of the resets to operate at the worst-case delay of the slowest one of them is unacceptable, especially if a particular HW block uses its reset often and/or requires timely execution. Regards, Jim Quinlan Broadcom STB/CM > > regards > Philipp
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(+)