[PATCH v2 0/2] ata: ahci_brcm: Follow-up changes for BCM7216

[PATCH v2 0/2] ata: ahci_brcm: Follow-up changes for BCM7216

[Florian Fainelli]

Hi Jens,

These two patches are a follow-up to my previous series titled: ata:
ahci_brcm: Fixes and new device support.

After submitting the BCM7216 RESCAL reset driver, Philipp the reset
controller maintained indicated that the reset line should be self
de-asserting and so reset_control_reset() should be used instead.

These two patches update the driver in that regard. It would be great if
you could apply those and get them queued up for 5.6 since they are
directly related to the previous series.

Changes in v2:
- updated error path after moving the reset line control

Thanks!

Florian Fainelli (2):
  ata: ahci_brcm: Perform reset after obtaining resources
  ata: ahci_brcm: BCM7216 reset is self de-asserting

 drivers/ata/ahci_brcm.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] ata: ahci_brcm: Perform reset after obtaining resources

[Florian Fainelli]

Resources such as clocks, PHYs, regulators are likely to get a probe
deferral return code, which could lead to the AHCI controller being
reset a few times until it gets successfully probed. Since this is
typically the most time consuming operation, move it after the resources
have been acquired.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/ata/ahci_brcm.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 13ceca687104..70e3e386d52e 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -453,15 +453,9 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 	else
 		reset_name = "ahci";
 
-	priv->rcdev = devm_reset_control_get(&pdev->dev, reset_name);
-	if (!IS_ERR_OR_NULL(priv->rcdev))
-		reset_control_deassert(priv->rcdev);
-
 	hpriv = ahci_platform_get_resources(pdev, 0);
-	if (IS_ERR(hpriv)) {
-		ret = PTR_ERR(hpriv);
-		goto out_reset;
-	}
+	if (IS_ERR(hpriv))
+		return PTR_ERR(hpriv);
 
 	hpriv->plat_data = priv;
 	hpriv->flags = AHCI_HFLAG_WAKE_BEFORE_STOP | AHCI_HFLAG_NO_WRITE_TO_RO;
@@ -478,6 +472,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 		break;
 	}
 
+	priv->rcdev = devm_reset_control_get(&pdev->dev, reset_name);
+	if (!IS_ERR_OR_NULL(priv->rcdev))
+		reset_control_deassert(priv->rcdev);
+
 	ret = ahci_platform_enable_clks(hpriv);
 	if (ret)
 		goto out_reset;
-- 
2.17.1

Re: [PATCH v2 1/2] ata: ahci_brcm: Perform reset after obtaining resources

[Philipp Zabel]

Hi Florian,

On Mon, 2020-01-06 at 11:19 -0800, Florian Fainelli wrote:
> Resources such as clocks, PHYs, regulators are likely to get a probe
> deferral return code, which could lead to the AHCI controller being
> reset a few times until it gets successfully probed. Since this is
> typically the most time consuming operation, move it after the resources
> have been acquired.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/ata/ahci_brcm.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 13ceca687104..70e3e386d52e 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -453,15 +453,9 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>  	else
>  		reset_name = "ahci";
>  
> -	priv->rcdev = devm_reset_control_get(&pdev->dev, reset_name);
> -	if (!IS_ERR_OR_NULL(priv->rcdev))
> -		reset_control_deassert(priv->rcdev);
> -
>  	hpriv = ahci_platform_get_resources(pdev, 0);
> -	if (IS_ERR(hpriv)) {
> -		ret = PTR_ERR(hpriv);
> -		goto out_reset;
> -	}
> +	if (IS_ERR(hpriv))
> +		return PTR_ERR(hpriv);
>  
>  	hpriv->plat_data = priv;
>  	hpriv->flags = AHCI_HFLAG_WAKE_BEFORE_STOP | AHCI_HFLAG_NO_WRITE_TO_RO;
> @@ -478,6 +472,10 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>  		break;
>  	}
>  
> +	priv->rcdev = devm_reset_control_get(&pdev->dev, reset_name);
> +	if (!IS_ERR_OR_NULL(priv->rcdev))
> +		reset_control_deassert(priv->rcdev);
> +

Please use devm_reset_control_get_optional_exclusive(), that returns
NULL if the requested reset is not specified in the device tree. Do
return the error code on IS_ERR(). You can then call
reset_control_deassert() unconditionally, it checks for and ignores
the NULL pointer.

regards
Philipp

[PATCH v2 2/2] ata: ahci_brcm: BCM7216 reset is self de-asserting

[Florian Fainelli]

The BCM7216 reset controller line is self-deasserting, unlike other
platforms, so make use of reset_control_reset() instead of
reset_control_deassert().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/ata/ahci_brcm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 70e3e386d52e..75931cfb70bc 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -363,8 +363,12 @@ static int brcm_ahci_resume(struct device *dev)
 	struct brcm_ahci_priv *priv = hpriv->plat_data;
 	int ret = 0;
 
-	if (!IS_ERR_OR_NULL(priv->rcdev))
-		ret = reset_control_deassert(priv->rcdev);
+	if (!IS_ERR_OR_NULL(priv->rcdev)) {
+		if (priv->version == BRCM_SATA_BCM7216)
+			ret = reset_control_reset(priv->rcdev);
+		else
+			ret = reset_control_deassert(priv->rcdev);
+	}
 	if (ret)
 		return ret;
 
@@ -473,8 +477,12 @@ static int brcm_ahci_probe(struct platform_device *pdev)
 	}
 
 	priv->rcdev = devm_reset_control_get(&pdev->dev, reset_name);
-	if (!IS_ERR_OR_NULL(priv->rcdev))
-		reset_control_deassert(priv->rcdev);
+	if (!IS_ERR_OR_NULL(priv->rcdev)) {
+		if (priv->version == BRCM_SATA_BCM7216)
+			reset_control_reset(priv->rcdev);
+		else
+			reset_control_deassert(priv->rcdev);
+	}
 
 	ret = ahci_platform_enable_clks(hpriv);
 	if (ret)
-- 
2.17.1