Re: [PATCH v2 04/12] PCI: brcmstb: Use swinit reset if available

[Philipp Zabel]

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@xxxxxxx> 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@xxxxxxxxxxxx>
> > > > > ---
> > > > >  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