[PATCH v2 0/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[PATCH v2 0/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

v2 -- refactor rescal-reset driver to implement assert/deassert rather than
      reset because the reset call only fires once per lifetime and we need
      to reset after every resume from S2 or S3.
   -- Split the use of "ahci" and "rescal" controllers in separate fields
      to keep things simple.

v1 -- original

Jim Quinlan (2):
  reset: brcmstb rescal: implement {de}assert() instead of reset()
  ata: ahci_brcm: Fix use of BCM7216 reset controller

 drivers/ata/ahci_brcm.c              | 46 ++++++++++++++--------------
 drivers/reset/reset-brcmstb-rescal.c | 13 ++++++--
 2 files changed, 33 insertions(+), 26 deletions(-)

-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

[PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]

Before, only control_reset() was implemented.  However, the reset core only
invokes control_reset() once in its lifetime.  Because we need it to invoke
control_reset() again after resume out of S2 or S3, we have switched to
putting the reset functionality into the control_deassert() method and
having an empty control_assert() method.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/reset/reset-brcmstb-rescal.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/reset/reset-brcmstb-rescal.c b/drivers/reset/reset-brcmstb-rescal.c
index b6f074d6a65f..1f54ae4f91fe 100644
--- a/drivers/reset/reset-brcmstb-rescal.c
+++ b/drivers/reset/reset-brcmstb-rescal.c
@@ -20,8 +20,8 @@ struct brcm_rescal_reset {
 	struct reset_controller_dev rcdev;
 };
 
-static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
-				 unsigned long id)
+static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev,
+				unsigned long id)
 {
 	struct brcm_rescal_reset *data =
 		container_of(rcdev, struct brcm_rescal_reset, rcdev);
@@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
 	return 0;
 }
 
+static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	return 0;
+}
+
 static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
 				   const struct of_phandle_args *reset_spec)
 {
@@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
 }
 
 static const struct reset_control_ops brcm_rescal_reset_ops = {
-	.reset = brcm_rescal_reset_set,
+	.deassert = brcm_rescal_reset_deassert,
+	.assert = brcm_rescal_reset_assert,
 };
 
 static int brcm_rescal_reset_probe(struct platform_device *pdev)
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Philipp Zabel]

Hi Jim,

On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote:
> Before, only control_reset() was implemented.  However, the reset core only
> invokes control_reset() once in its lifetime.  Because we need it to invoke
> control_reset() again after resume out of S2 or S3, we have switched to
> putting the reset functionality into the control_deassert() method and
> having an empty control_assert() method.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

You are switching to the wrong abstraction to work around a deficiency
of the reset controller framework. Instead, it would be better to allow
to "reactivate" shared pulsed resets so they can be triggered again.

Could you please have a look at [1], which tries to implement this with
a new API call, and check if this can fix your problem? If so, it would
be great if you could coordinate with Amjad to see this fixed in the
core.

[1] https://lore.kernel.org/lkml/20201001132758.12280-1-aouledameur@baylibre.com/

regards
Philipp

> ---
>  drivers/reset/reset-brcmstb-rescal.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/reset/reset-brcmstb-rescal.c b/drivers/reset/reset-brcmstb-rescal.c
> index b6f074d6a65f..1f54ae4f91fe 100644
> --- a/drivers/reset/reset-brcmstb-rescal.c
> +++ b/drivers/reset/reset-brcmstb-rescal.c
> @@ -20,8 +20,8 @@ struct brcm_rescal_reset {
>  	struct reset_controller_dev rcdev;
>  };
>  
> -static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
> -				 unsigned long id)
> +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev,
> +				unsigned long id)
>  {
>  	struct brcm_rescal_reset *data =
>  		container_of(rcdev, struct brcm_rescal_reset, rcdev);
> @@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
>  	return 0;
>  }
>  
> +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev,
> +			      unsigned long id)
> +{
> +	return 0;
> +}
> +
>  static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
>  				   const struct of_phandle_args *reset_spec)
>  {
> @@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
>  }
>  
>  static const struct reset_control_ops brcm_rescal_reset_ops = {
> -	.reset = brcm_rescal_reset_set,
> +	.deassert = brcm_rescal_reset_deassert,
> +	.assert = brcm_rescal_reset_assert,
>  };
>  
>  static int brcm_rescal_reset_probe(struct platform_device *pdev)

Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]

On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Jim,
>
> On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote:
> > Before, only control_reset() was implemented.  However, the reset core only
> > invokes control_reset() once in its lifetime.  Because we need it to invoke
> > control_reset() again after resume out of S2 or S3, we have switched to
> > putting the reset functionality into the control_deassert() method and
> > having an empty control_assert() method.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>
> You are switching to the wrong abstraction to work around a deficiency
> of the reset controller framework. Instead, it would be better to allow
> to "reactivate" shared pulsed resets so they can be triggered again.


True.
>
>
> Could you please have a look at [1], which tries to implement this with
> a new API call, and check if this can fix your problem? If so, it would
> be great if you could coordinate with Amjad to see this fixed in the
> core.
>
> [1] https://lore.kernel.org/lkml/20201001132758.12280-1-aouledameur@baylibre.com/


Yes, this would work for our usage.  Amjad please let me know if I can
help here.  The only "nit" I have is that I favor the name 'unreset'
over 'resettable' but truly I don't care one way or the other.

Thanks and kind regards,
Jim Quinaln
Broadcom STB
>
>
>
> regards
> Philipp
>
> > ---
> >  drivers/reset/reset-brcmstb-rescal.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/reset/reset-brcmstb-rescal.c b/drivers/reset/reset-brcmstb-rescal.c
> > index b6f074d6a65f..1f54ae4f91fe 100644
> > --- a/drivers/reset/reset-brcmstb-rescal.c
> > +++ b/drivers/reset/reset-brcmstb-rescal.c
> > @@ -20,8 +20,8 @@ struct brcm_rescal_reset {
> >       struct reset_controller_dev rcdev;
> >  };
> >
> > -static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
> > -                              unsigned long id)
> > +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev,
> > +                             unsigned long id)
> >  {
> >       struct brcm_rescal_reset *data =
> >               container_of(rcdev, struct brcm_rescal_reset, rcdev);
> > @@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
> >       return 0;
> >  }
> >
> > +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev,
> > +                           unsigned long id)
> > +{
> > +     return 0;
> > +}
> > +
> >  static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
> >                                  const struct of_phandle_args *reset_spec)
> >  {
> > @@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
> >  }
> >
> >  static const struct reset_control_ops brcm_rescal_reset_ops = {
> > -     .reset = brcm_rescal_reset_set,
> > +     .deassert = brcm_rescal_reset_deassert,
> > +     .assert = brcm_rescal_reset_assert,
> >  };
> >
> >  static int brcm_rescal_reset_probe(struct platform_device *pdev)

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]

Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Philipp Zabel]

On Mon, 2020-11-09 at 11:21 -0500, Jim Quinlan wrote:
> On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Hi Jim,
> > 
> > On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote:
> > > Before, only control_reset() was implemented.  However, the reset core only
> > > invokes control_reset() once in its lifetime.  Because we need it to invoke
> > > control_reset() again after resume out of S2 or S3, we have switched to
> > > putting the reset functionality into the control_deassert() method and
> > > having an empty control_assert() method.
> > > 
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > 
> > You are switching to the wrong abstraction to work around a deficiency
> > of the reset controller framework. Instead, it would be better to allow
> > to "reactivate" shared pulsed resets so they can be triggered again.
> 
> True.
> > 
> > Could you please have a look at [1], which tries to implement this with
> > a new API call, and check if this can fix your problem? If so, it would
> > be great if you could coordinate with Amjad to see this fixed in the
> > core.
> > 
> > [1] https://lore.kernel.org/lkml/20201001132758.12280-1-aouledameur@baylibre.com/
> 
> Yes, this would work for our usage.  Amjad please let me know if I can
> help here.  The only "nit" I have is that I favor the name 'unreset'
> over 'resettable' but truly I don't care one way or the other.

Both unreset and resettable are adjectives, maybe it would be better to
have an imperative verb like the other API functions. I would have liked
reset_control_trigger/rearm as a pair, but I can't find anything I like
that fits with the somewhat unfortunate reset_control_reset name in my
mind.
In that sense, I don't have a preference one way or the other either.

regards
Philipp

Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Amjad Ouled-Ameur]

Hi everyone,


On 09/11/2020 18:25, Philipp Zabel wrote:
> On Mon, 2020-11-09 at 11:21 -0500, Jim Quinlan wrote:
>> On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>>> Hi Jim,
>>>
>>> On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote:
>>>> Before, only control_reset() was implemented.  However, the reset core only
>>>> invokes control_reset() once in its lifetime.  Because we need it to invoke
>>>> control_reset() again after resume out of S2 or S3, we have switched to
>>>> putting the reset functionality into the control_deassert() method and
>>>> having an empty control_assert() method.
>>>>
>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> You are switching to the wrong abstraction to work around a deficiency
>>> of the reset controller framework. Instead, it would be better to allow
>>> to "reactivate" shared pulsed resets so they can be triggered again.
>> True.
>>> Could you please have a look at [1], which tries to implement this with
>>> a new API call, and check if this can fix your problem? If so, it would
>>> be great if you could coordinate with Amjad to see this fixed in the
>>> core.
>>>
>>> [1] https://lore.kernel.org/lkml/20201001132758.12280-1-aouledameur@baylibre.com/
>> Yes, this would work for our usage.  Amjad please let me know if I can
>> help here.  The only "nit" I have is that I favor the name 'unreset'
>> over 'resettable' but truly I don't care one way or the other.

My pleasure, I will send a V2 soon of the patch, when it is done, please
let me know if I can add anything that would suit best your use case as well.

> Both unreset and resettable are adjectives, maybe it would be better to
> have an imperative verb like the other API functions. I would have liked
> reset_control_trigger/rearm as a pair, but I can't find anything I like
> that fits with the somewhat unfortunate reset_control_reset name in my
> mind.
> In that sense, I don't have a preference one way or the other either.

I think reset_control_rearm would be a very good candidate, resettable
is quite representative but I think it is best to keep using verbs for
the sake of homogeneity

>
> regards
> Philipp

Sincerely,

Amjad

[PATCH v2 2/2] ata: ahci_brcm: Fix use of BCM7216 reset controller

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3703 bytes --]

From: Jim Quinlan <jquinlan@broadcom.com>

This driver may use one of two resets controllers.  Keep them in separate
variables to keep things simple.  The 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@broadcom.com>
---
 drivers/ata/ahci_brcm.c | 46 ++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 49f7acbfcf01..58eae075d790 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -86,7 +86,8 @@ struct brcm_ahci_priv {
 	u32 port_mask;
 	u32 quirks;
 	enum brcm_ahci_version version;
-	struct reset_control *rcdev;
+	struct reset_control *rcdev_rescal;
+	struct reset_control *rcdev_ahci;
 };
 
 static inline u32 brcm_sata_readreg(void __iomem *addr)
@@ -352,8 +353,8 @@ static int brcm_ahci_suspend(struct device *dev)
 	else
 		ret = 0;
 
-	if (priv->version != BRCM_SATA_BCM7216)
-		reset_control_assert(priv->rcdev);
+	reset_control_assert(priv->rcdev_ahci);
+	reset_control_assert(priv->rcdev_rescal);
 
 	return ret;
 }
@@ -365,10 +366,10 @@ static int __maybe_unused brcm_ahci_resume(struct device *dev)
 	struct brcm_ahci_priv *priv = hpriv->plat_data;
 	int ret = 0;
 
-	if (priv->version == BRCM_SATA_BCM7216)
-		ret = reset_control_reset(priv->rcdev);
-	else
-		ret = reset_control_deassert(priv->rcdev);
+	ret = reset_control_deassert(priv->rcdev_ahci);
+	if (ret)
+		return ret;
+	ret = reset_control_deassert(priv->rcdev_rescal);
 	if (ret)
 		return ret;
 
@@ -428,7 +429,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;
@@ -450,15 +450,15 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->top_ctrl))
 		return PTR_ERR(priv->top_ctrl);
 
-	/* Reset is optional depending on platform and named differently */
-	if (priv->version == BRCM_SATA_BCM7216)
-		reset_name = "rescal";
-	else
-		reset_name = "ahci";
-
-	priv->rcdev = devm_reset_control_get_optional(&pdev->dev, reset_name);
-	if (IS_ERR(priv->rcdev))
-		return PTR_ERR(priv->rcdev);
+	if (priv->version == BRCM_SATA_BCM7216) {
+		priv->rcdev_rescal = devm_reset_control_get_optional_shared(
+			&pdev->dev, "rescal");
+		if (IS_ERR(priv->rcdev_rescal))
+			return PTR_ERR(priv->rcdev_rescal);
+	}
+	priv->rcdev_ahci = devm_reset_control_get_optional(&pdev->dev, "ahci");
+	if (IS_ERR(priv->rcdev_ahci))
+		return PTR_ERR(priv->rcdev_ahci);
 
 	hpriv = ahci_platform_get_resources(pdev, 0);
 	if (IS_ERR(hpriv))
@@ -479,10 +479,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 		break;
 	}
 
-	if (priv->version == BRCM_SATA_BCM7216)
-		ret = reset_control_reset(priv->rcdev);
-	else
-		ret = reset_control_deassert(priv->rcdev);
+	ret = reset_control_deassert(priv->rcdev_rescal);
+	if (ret)
+		return ret;
+	ret = reset_control_deassert(priv->rcdev_ahci);
 	if (ret)
 		return ret;
 
@@ -527,8 +527,8 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 out_disable_clks:
 	ahci_platform_disable_clks(hpriv);
 out_reset:
-	if (priv->version != BRCM_SATA_BCM7216)
-		reset_control_assert(priv->rcdev);
+	reset_control_assert(priv->rcdev_ahci);
+	reset_control_assert(priv->rcdev_rescal);
 	return ret;
 }
 
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4167 bytes --]