Re: [PATCH v1] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

On Tue, Aug 25, 2020 at 4:16 AM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:
>
> On Mon, 2020-08-24 at 16:40 -0400, Jim Quinlan wrote:
> > From: Jim Quinlan <jquinlan@xxxxxxxxxxxx>
> >
> > A reset controller "rescal" is shared between the AHCI driver and the PCIe
> > driver for the BrcmSTB 7216 chip.  Use
> > devm_reset_control_get_optional_shared() to handle this sharing.
> >
> > Fixes: 272ecd60a636 ("ata: ahci_brcm: BCM7216 reset is self de-asserting")
> > Fixes: c345ec6a50e9 ("ata: ahci_brcm: Support BCM7216 reset controller name")
> > Signed-off-by: Jim Quinlan <jquinlan@xxxxxxxxxxxx>
> > ---
> >  drivers/ata/ahci_brcm.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> > index 6853dbb4131d..d6115bc04b09 100644
> > --- a/drivers/ata/ahci_brcm.c
> > +++ b/drivers/ata/ahci_brcm.c
> > @@ -428,7 +428,6 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> >  {
> >       const struct of_device_id *of_id;
> >       struct device *dev = &pdev->dev;
> > -     const char *reset_name = NULL;
> >       struct brcm_ahci_priv *priv;
> >       struct ahci_host_priv *hpriv;
> >       struct resource *res;
> > @@ -452,11 +451,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> >
> >       /* Reset is optional depending on platform and named differently */
> >       if (priv->version == BRCM_SATA_BCM7216)
> > -             reset_name = "rescal";
> > +             priv->rcdev = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> >       else
> > -             reset_name = "ahci";
> > +             priv->rcdev = devm_reset_control_get_optional(&pdev->dev, "ahci");
>
> I think it would be cleaner to use two separate reset control handles
> here. It is hard to reason about what the code does when the reset
> control is shared on one platform and exclusive on the other.
>
> > -     priv->rcdev = devm_reset_control_get_optional(&pdev->dev, reset_name);
> >       if (IS_ERR(priv->rcdev))
> >               return PTR_ERR(priv->rcdev);
> >
> > @@ -479,10 +477,7 @@ static int brcm_ahci_probe(struct platform_device *pdev)
> >               break;
> >       }
> >
> > -     if (priv->version == BRCM_SATA_BCM7216)
> > -             ret = reset_control_reset(priv->rcdev);
>
> I think we might have a similar issue currently with
> "usb: dwc3: meson-g12a: fix shared reset control use", where two IP
> cores try to share a pulsed reset line.
>
> > -     else
> > -             ret = reset_control_deassert(priv->rcdev);
> > +     ret = reset_control_deassert(priv->rcdev);
>
> Isn't the shared 'rescal' reset a triggered reset pulse? Looking at the
> reset-brcmstb-rescal reset controller driver, without reset line level
> control implemented, this will turn into a no-op for BCM7216. Yes, the
> reset line will be deasserted after this call, but there is no guarantee
> that the reset line was ever pulsed.

Hi Philipp,

I believe you have observed that our reset-brcmstb-rescal.c driver
sets only the "reset" op and implements a pulse when called (it's
actually more of a "start the engine" operation).  This suits us fine
except such a driver can only fire its reset once in its lifetime.  We
would actually like a "new lifetime" when we emerge from resume()
after being suspended.  This would probably require a modification to
core.c to set triggered_count to 0 on resume_early time.  My instincts
say this approach would upset some apple carts.

What we could do instead is use the deassert op instead of the reset
op so that the pulse and be activated on probe and every time we
resume().

Are you okay with that?

Thanks,
Jim
>
> regards
> Philipp