2019-12-10 18:56:21

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support

Hi Jens,

The first 4 patches are fixes and should ideally be queued up/picked up
by stable. The last 4 patches add support for BCM7216 which is one of
our latest devices supported by this driver.

Patch #2 does a few things, but it was pretty badly broken before and it
is hard not to fix all call sites (probe, suspend, resume) in one shot.

Please let me know if you have any comments.

Thanks!

Florian Fainelli (8):
ata: libahci_platform: Export again ahci_platform_<en/dis>able_phys()
ata: ahci_brcm: Fix AHCI resources management
ata: ahci_brcm: BCM7425 AHCI requires AHCI_HFLAG_DELAY_ENGINE
ata: ahci_brcm: Add missing clock management during recovery
ata: ahci_brcm: Manage reset line during suspend/resume
ata: ahci_brcm: Add a shutdown callback
dt-bindings: ata: Document BCM7216 AHCI controller compatible
ata: ahci_brcm: Support BCM7216 reset controller name

.../bindings/ata/brcm,sata-brcm.txt | 7 +
drivers/ata/ahci_brcm.c | 167 ++++++++++++++----
drivers/ata/libahci_platform.c | 6 +-
include/linux/ahci_platform.h | 2 +
4 files changed, 141 insertions(+), 41 deletions(-)

--
2.17.1


2019-12-10 18:56:32

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 5/8] ata: ahci_brcm: Manage reset line during suspend/resume

We were not managing the reset line during suspend/resume, but this
needs to be done to ensure that the controller can exit low power modes
correctly, especially with deep sleep suspend mode that may reset parts
of the logic.

Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/ata/ahci_brcm.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 66a570d0da83..76612577a59a 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -343,10 +343,16 @@ static int brcm_ahci_suspend(struct device *dev)
struct ata_host *host = dev_get_drvdata(dev);
struct ahci_host_priv *hpriv = host->private_data;
struct brcm_ahci_priv *priv = hpriv->plat_data;
+ int ret;

brcm_sata_phys_disable(priv);

- return ahci_platform_suspend(dev);
+ ret = ahci_platform_suspend(dev);
+
+ if (!IS_ERR_OR_NULL(priv->rcdev))
+ reset_control_assert(priv->rcdev);
+
+ return ret;
}

static int brcm_ahci_resume(struct device *dev)
@@ -354,7 +360,12 @@ static int brcm_ahci_resume(struct device *dev)
struct ata_host *host = dev_get_drvdata(dev);
struct ahci_host_priv *hpriv = host->private_data;
struct brcm_ahci_priv *priv = hpriv->plat_data;
- int ret;
+ int ret = 0;
+
+ if (!IS_ERR_OR_NULL(priv->rcdev))
+ ret = reset_control_deassert(priv->rcdev);
+ if (ret)
+ return ret;

/* Make sure clocks are turned on before re-configuration */
ret = ahci_platform_enable_clks(hpriv);
--
2.17.1

2019-12-10 18:56:49

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 7/8] dt-bindings: ata: Document BCM7216 AHCI controller compatible

The BCM7216 AHCI controller makes use of a specific reset controller
line/name, document the compatible string and the optional reset
properties.

Signed-off-by: Florian Fainelli <[email protected]>
---
Documentation/devicetree/bindings/ata/brcm,sata-brcm.txt | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/brcm,sata-brcm.txt b/Documentation/devicetree/bindings/ata/brcm,sata-brcm.txt
index 7713a413c6a7..b9ae4ce4a0a0 100644
--- a/Documentation/devicetree/bindings/ata/brcm,sata-brcm.txt
+++ b/Documentation/devicetree/bindings/ata/brcm,sata-brcm.txt
@@ -5,6 +5,7 @@ Each SATA controller should have its own node.

Required properties:
- compatible : should be one or more of
+ "brcm,bcm7216-ahci"
"brcm,bcm7425-ahci"
"brcm,bcm7445-ahci"
"brcm,bcm-nsp-ahci"
@@ -14,6 +15,12 @@ Required properties:
- reg-names : "ahci" and "top-ctrl"
- interrupts : interrupt mapping for SATA IRQ

+Optional properties:
+
+- reset: for "brcm,bcm7216-ahci" must be a valid reset phandle
+ pointing to the RESCAL reset controller provider node.
+- reset-names: for "brcm,bcm7216-ahci", must be "rescal".
+
Also see ahci-platform.txt.

Example:
--
2.17.1

2019-12-10 18:57:08

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 2/8] ata: ahci_brcm: Fix AHCI resources management

The AHCI resources management within ahci_brcm.c is a little
convoluted, largely because it historically had a dedicated clock that
was managed within this file in the downstream tree. Once brough
upstream though, the clock was left to be managed by libahci_platform.c
which is entirely appropriate.

This patch series ensures that the AHCI resources are fetched and
enabled before any register access is done, thus avoiding bus errors on
platforms which clock gate the controller by default.

As a result we need to re-arrange the suspend() and resume() functions
in order to avoid accessing registers after the clocks have been turned
off respectively before the clocks have been turned on. Finally, we can
refactor brcm_ahci_get_portmask() in order to fetch the number of ports
from hpriv->mmio which is now accessible without jumping through hoops
like we used to do.

The commit pointed in the Fixes tag is both old and new enough not to
require major headaches for backporting of this patch.

Fixes: eba68f829794 ("ata: ahci_brcmstb: rename to support across Broadcom SoC's")
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/ata/ahci_brcm.c | 105 +++++++++++++++++++++++++++++-----------
1 file changed, 76 insertions(+), 29 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index f41744b9b38a..a8b2f3f7bbbc 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -213,19 +213,12 @@ static void brcm_sata_phys_disable(struct brcm_ahci_priv *priv)
brcm_sata_phy_disable(priv, i);
}

-static u32 brcm_ahci_get_portmask(struct platform_device *pdev,
+static u32 brcm_ahci_get_portmask(struct ahci_host_priv *hpriv,
struct brcm_ahci_priv *priv)
{
- void __iomem *ahci;
- struct resource *res;
u32 impl;

- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ahci");
- ahci = devm_ioremap_resource(&pdev->dev, res);
- if (IS_ERR(ahci))
- return 0;
-
- impl = readl(ahci + HOST_PORTS_IMPL);
+ impl = readl(hpriv->mmio + HOST_PORTS_IMPL);

if (fls(impl) > SATA_TOP_MAX_PHYS)
dev_warn(priv->dev, "warning: more ports than PHYs (%#x)\n",
@@ -233,9 +226,6 @@ static u32 brcm_ahci_get_portmask(struct platform_device *pdev,
else if (!impl)
dev_info(priv->dev, "no ports found\n");

- devm_iounmap(&pdev->dev, ahci);
- devm_release_mem_region(&pdev->dev, res->start, resource_size(res));
-
return impl;
}

@@ -347,11 +337,10 @@ static int brcm_ahci_suspend(struct device *dev)
struct ata_host *host = dev_get_drvdata(dev);
struct ahci_host_priv *hpriv = host->private_data;
struct brcm_ahci_priv *priv = hpriv->plat_data;
- int ret;

- ret = ahci_platform_suspend(dev);
brcm_sata_phys_disable(priv);
- return ret;
+
+ return ahci_platform_suspend(dev);
}

static int brcm_ahci_resume(struct device *dev)
@@ -359,11 +348,44 @@ static int brcm_ahci_resume(struct device *dev)
struct ata_host *host = dev_get_drvdata(dev);
struct ahci_host_priv *hpriv = host->private_data;
struct brcm_ahci_priv *priv = hpriv->plat_data;
+ int ret;
+
+ /* Make sure clocks are turned on before re-configuration */
+ ret = ahci_platform_enable_clks(hpriv);
+ if (ret)
+ return ret;

brcm_sata_init(priv);
brcm_sata_phys_enable(priv);
brcm_sata_alpm_init(hpriv);
- return ahci_platform_resume(dev);
+
+ /* Since we had to enable clocks earlier on, we cannot use
+ * ahci_platform_resume() as-is since a second call to
+ * ahci_platform_enable_resources() would bump up the resources
+ * (regulators, clocks, PHYs) count artificially so we copy the part
+ * after ahci_platform_enable_resources().
+ */
+ ret = ahci_platform_enable_phys(hpriv);
+ if (ret)
+ goto out_disable_phys;
+
+ ret = ahci_platform_resume_host(dev);
+ if (ret)
+ goto out_disable_platform_phys;
+
+ /* We resumed so update PM runtime state */
+ pm_runtime_disable(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+
+ return 0;
+
+out_disable_platform_phys:
+ ahci_platform_disable_phys(hpriv);
+out_disable_phys:
+ brcm_sata_phys_disable(priv);
+ ahci_platform_disable_clks(hpriv);
+ return ret;
}
#endif

@@ -416,38 +438,63 @@ static int brcm_ahci_probe(struct platform_device *pdev)
priv->quirks |= BRCM_AHCI_QUIRK_SKIP_PHY_ENABLE;
}

+ hpriv = ahci_platform_get_resources(pdev, 0);
+ if (IS_ERR(hpriv)) {
+ ret = PTR_ERR(hpriv);
+ goto out_reset;
+ }
+
+ ret = ahci_platform_enable_clks(hpriv);
+ if (ret)
+ goto out_reset;
+
+ /* Must be first so as to configure endianness including that
+ * of the standard AHCI register space.
+ */
brcm_sata_init(priv);

- priv->port_mask = brcm_ahci_get_portmask(pdev, priv);
- if (!priv->port_mask)
- return -ENODEV;
+ /* Initializes priv->port_mask which is used below */
+ priv->port_mask = brcm_ahci_get_portmask(hpriv, priv);
+ if (!priv->port_mask) {
+ ret = -ENODEV;
+ goto out_disable_clks;
+ }

+ /* Must be done before ahci_platform_enable_phys() */
brcm_sata_phys_enable(priv);

- hpriv = ahci_platform_get_resources(pdev, 0);
- if (IS_ERR(hpriv))
- return PTR_ERR(hpriv);
hpriv->plat_data = priv;
hpriv->flags = AHCI_HFLAG_WAKE_BEFORE_STOP;

brcm_sata_alpm_init(hpriv);

- ret = ahci_platform_enable_resources(hpriv);
- if (ret)
- return ret;
-
if (priv->quirks & BRCM_AHCI_QUIRK_NO_NCQ)
hpriv->flags |= AHCI_HFLAG_NO_NCQ;
hpriv->flags |= AHCI_HFLAG_NO_WRITE_TO_RO;

+ ret = ahci_platform_enable_phys(hpriv);
+ if (ret)
+ goto out_disable_phys;
+
ret = ahci_platform_init_host(pdev, hpriv, &ahci_brcm_port_info,
&ahci_platform_sht);
if (ret)
- return ret;
+ goto out_disable_platform_phys;

dev_info(dev, "Broadcom AHCI SATA3 registered\n");

return 0;
+
+out_disable_platform_phys:
+ ahci_platform_disable_phys(hpriv);
+out_disable_phys:
+ brcm_sata_phys_disable(priv);
+out_disable_clks:
+ ahci_platform_disable_clks(hpriv);
+out_reset:
+ if (!IS_ERR_OR_NULL(priv->rcdev))
+ reset_control_assert(priv->rcdev);
+ return ret;
}

static int brcm_ahci_remove(struct platform_device *pdev)
@@ -457,12 +504,12 @@ static int brcm_ahci_remove(struct platform_device *pdev)
struct brcm_ahci_priv *priv = hpriv->plat_data;
int ret;

+ brcm_sata_phys_disable(priv);
+
ret = ata_platform_remove_one(pdev);
if (ret)
return ret;

- brcm_sata_phys_disable(priv);
-
return 0;
}

--
2.17.1

2019-12-10 18:57:10

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 3/8] ata: ahci_brcm: BCM7425 AHCI requires AHCI_HFLAG_DELAY_ENGINE

Set AHCI_HFLAG_DELAY_ENGINE for the BCM7425 AHCI controller thus making
it conforming to the 'strict' AHCI implementation which this controller
is based on.

This solves long link establishment with specific hard drives (e.g.:
Seagate ST1000VM002-9ZL1 SC12) that would otherwise have to complete the
error recovery handling before finally establishing a succesful SATA
link at the desired speed.

We re-order the hpriv->flags assignment to also remove the NONCQ quirk
since we can set the flag directly.

Fixes: 9586114cf1e9 ("ata: ahci_brcmstb: add support MIPS-based platforms")
Fixes: 423be77daabe ("ata: ahci_brcmstb: add quirk for broken ncq")
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/ata/ahci_brcm.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index a8b2f3f7bbbc..58f8fd7bb8b8 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -76,8 +76,7 @@ enum brcm_ahci_version {
};

enum brcm_ahci_quirks {
- BRCM_AHCI_QUIRK_NO_NCQ = BIT(0),
- BRCM_AHCI_QUIRK_SKIP_PHY_ENABLE = BIT(1),
+ BRCM_AHCI_QUIRK_SKIP_PHY_ENABLE = BIT(0),
};

struct brcm_ahci_priv {
@@ -432,18 +431,27 @@ static int brcm_ahci_probe(struct platform_device *pdev)
if (!IS_ERR_OR_NULL(priv->rcdev))
reset_control_deassert(priv->rcdev);

- if ((priv->version == BRCM_SATA_BCM7425) ||
- (priv->version == BRCM_SATA_NSP)) {
- priv->quirks |= BRCM_AHCI_QUIRK_NO_NCQ;
- priv->quirks |= BRCM_AHCI_QUIRK_SKIP_PHY_ENABLE;
- }
-
hpriv = ahci_platform_get_resources(pdev, 0);
if (IS_ERR(hpriv)) {
ret = PTR_ERR(hpriv);
goto out_reset;
}

+ hpriv->plat_data = priv;
+ hpriv->flags = AHCI_HFLAG_WAKE_BEFORE_STOP | AHCI_HFLAG_NO_WRITE_TO_RO;
+
+ switch (priv->version) {
+ case BRCM_SATA_BCM7425:
+ hpriv->flags |= AHCI_HFLAG_DELAY_ENGINE;
+ /* fall through */
+ case BRCM_SATA_NSP:
+ hpriv->flags |= AHCI_HFLAG_NO_NCQ;
+ priv->quirks |= BRCM_AHCI_QUIRK_SKIP_PHY_ENABLE;
+ break;
+ default:
+ break;
+ }
+
ret = ahci_platform_enable_clks(hpriv);
if (ret)
goto out_reset;
@@ -463,15 +471,8 @@ static int brcm_ahci_probe(struct platform_device *pdev)
/* Must be done before ahci_platform_enable_phys() */
brcm_sata_phys_enable(priv);

- hpriv->plat_data = priv;
- hpriv->flags = AHCI_HFLAG_WAKE_BEFORE_STOP;
-
brcm_sata_alpm_init(hpriv);

- if (priv->quirks & BRCM_AHCI_QUIRK_NO_NCQ)
- hpriv->flags |= AHCI_HFLAG_NO_NCQ;
- hpriv->flags |= AHCI_HFLAG_NO_WRITE_TO_RO;
-
ret = ahci_platform_enable_phys(hpriv);
if (ret)
goto out_disable_phys;
--
2.17.1

2019-12-10 18:57:39

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 6/8] ata: ahci_brcm: Add a shutdown callback

Make sure that we quiesce the controller and shut down the clocks in a
shutdown callback.

Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/ata/ahci_brcm.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 76612577a59a..58e1a6e5478d 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -532,11 +532,26 @@ static int brcm_ahci_remove(struct platform_device *pdev)
return 0;
}

+static void brcm_ahci_shutdown(struct platform_device *pdev)
+{
+ int ret;
+
+ /* All resources releasing happens via devres, but our device, unlike a
+ * proper remove is not disappearing, therefore using
+ * brcm_ahci_suspend() here which does explicit power management is
+ * appropriate.
+ */
+ ret = brcm_ahci_suspend(&pdev->dev);
+ if (ret)
+ dev_err(&pdev->dev, "failed to shutdown\n");
+}
+
static SIMPLE_DEV_PM_OPS(ahci_brcm_pm_ops, brcm_ahci_suspend, brcm_ahci_resume);

static struct platform_driver brcm_ahci_driver = {
.probe = brcm_ahci_probe,
.remove = brcm_ahci_remove,
+ .shutdown = brcm_ahci_shutdown,
.driver = {
.name = DRV_NAME,
.of_match_table = ahci_of_match,
--
2.17.1

2019-12-10 18:57:54

by Florian Fainelli

[permalink] [raw]
Subject: [PATCH 8/8] ata: ahci_brcm: Support BCM7216 reset controller name

BCM7216 uses a different reset controller name which is "rescal" instead
of "ahci", match the compatible string to account for that minor
difference, the reset is otherwise identical to how other generations of
SATA controllers work.

Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/ata/ahci_brcm.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 58e1a6e5478d..13ceca687104 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -73,6 +73,7 @@ enum brcm_ahci_version {
BRCM_SATA_BCM7425 = 1,
BRCM_SATA_BCM7445,
BRCM_SATA_NSP,
+ BRCM_SATA_BCM7216,
};

enum brcm_ahci_quirks {
@@ -415,6 +416,7 @@ static const struct of_device_id ahci_of_match[] = {
{.compatible = "brcm,bcm7445-ahci", .data = (void *)BRCM_SATA_BCM7445},
{.compatible = "brcm,bcm63138-ahci", .data = (void *)BRCM_SATA_BCM7445},
{.compatible = "brcm,bcm-nsp-ahci", .data = (void *)BRCM_SATA_NSP},
+ {.compatible = "brcm,bcm7216-ahci", .data = (void *)BRCM_SATA_BCM7216},
{},
};
MODULE_DEVICE_TABLE(of, ahci_of_match);
@@ -423,6 +425,7 @@ 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;
@@ -444,8 +447,13 @@ 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 */
- priv->rcdev = devm_reset_control_get(&pdev->dev, "ahci");
+ /* 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(&pdev->dev, reset_name);
if (!IS_ERR_OR_NULL(priv->rcdev))
reset_control_deassert(priv->rcdev);

--
2.17.1

2019-12-11 13:32:27

by Hans de Goede

[permalink] [raw]
Subject: Re: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support

Hi,

On 10-12-2019 19:53, Florian Fainelli wrote:
> Hi Jens,
>
> The first 4 patches are fixes and should ideally be queued up/picked up
> by stable. The last 4 patches add support for BCM7216 which is one of
> our latest devices supported by this driver.
>
> Patch #2 does a few things, but it was pretty badly broken before and it
> is hard not to fix all call sites (probe, suspend, resume) in one shot.
>
> Please let me know if you have any comments.
>
> Thanks!

The entire series looks good to me:

Reviewed-by: Hans de Goede <[email protected]>

Regards,

Hans



>
> Florian Fainelli (8):
> ata: libahci_platform: Export again ahci_platform_<en/dis>able_phys()
> ata: ahci_brcm: Fix AHCI resources management
> ata: ahci_brcm: BCM7425 AHCI requires AHCI_HFLAG_DELAY_ENGINE
> ata: ahci_brcm: Add missing clock management during recovery
> ata: ahci_brcm: Manage reset line during suspend/resume
> ata: ahci_brcm: Add a shutdown callback
> dt-bindings: ata: Document BCM7216 AHCI controller compatible
> ata: ahci_brcm: Support BCM7216 reset controller name
>
> .../bindings/ata/brcm,sata-brcm.txt | 7 +
> drivers/ata/ahci_brcm.c | 167 ++++++++++++++----
> drivers/ata/libahci_platform.c | 6 +-
> include/linux/ahci_platform.h | 2 +
> 4 files changed, 141 insertions(+), 41 deletions(-)
>

2019-12-19 20:22:32

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 7/8] dt-bindings: ata: Document BCM7216 AHCI controller compatible

On Tue, 10 Dec 2019 10:53:50 -0800, Florian Fainelli wrote:
> The BCM7216 AHCI controller makes use of a specific reset controller
> line/name, document the compatible string and the optional reset
> properties.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> Documentation/devicetree/bindings/ata/brcm,sata-brcm.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>

Reviewed-by: Rob Herring <[email protected]>

2019-12-26 03:35:49

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support



On 12/11/2019 5:31 AM, Hans de Goede wrote:
> Hi,
>
> On 10-12-2019 19:53, Florian Fainelli wrote:
>> Hi Jens,
>>
>> The first 4 patches are fixes and should ideally be queued up/picked up
>> by stable. The last 4 patches add support for BCM7216 which is one of
>> our latest devices supported by this driver.
>>
>> Patch #2 does a few things, but it was pretty badly broken before and it
>> is hard not to fix all call sites (probe, suspend, resume) in one shot.
>>
>> Please let me know if you have any comments.
>>
>> Thanks!
>
> The entire series looks good to me:
>
> Reviewed-by: Hans de Goede <[email protected]>
>
> Regards,

Thanks Hans, Jens is this good to go from your perspective?
--
Florian

2019-12-26 03:47:45

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support

On 12/25/19 8:34 PM, Florian Fainelli wrote:
>
>
> On 12/11/2019 5:31 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 10-12-2019 19:53, Florian Fainelli wrote:
>>> Hi Jens,
>>>
>>> The first 4 patches are fixes and should ideally be queued up/picked up
>>> by stable. The last 4 patches add support for BCM7216 which is one of
>>> our latest devices supported by this driver.
>>>
>>> Patch #2 does a few things, but it was pretty badly broken before and it
>>> is hard not to fix all call sites (probe, suspend, resume) in one shot.
>>>
>>> Please let me know if you have any comments.
>>>
>>> Thanks!
>>
>> The entire series looks good to me:
>>
>> Reviewed-by: Hans de Goede <[email protected]>
>>
>> Regards,
>
> Thanks Hans, Jens is this good to go from your perspective?

I'll queue 1-4 up for 5.5 and mark for stable, then add 5-8 for
5.6. Thanks!

--
Jens Axboe

2020-01-02 23:16:04

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support

On 12/25/19 7:46 PM, Jens Axboe wrote:
> On 12/25/19 8:34 PM, Florian Fainelli wrote:
>>
>>
>> On 12/11/2019 5:31 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 10-12-2019 19:53, Florian Fainelli wrote:
>>>> Hi Jens,
>>>>
>>>> The first 4 patches are fixes and should ideally be queued up/picked up
>>>> by stable. The last 4 patches add support for BCM7216 which is one of
>>>> our latest devices supported by this driver.
>>>>
>>>> Patch #2 does a few things, but it was pretty badly broken before and it
>>>> is hard not to fix all call sites (probe, suspend, resume) in one shot.
>>>>
>>>> Please let me know if you have any comments.
>>>>
>>>> Thanks!
>>>
>>> The entire series looks good to me:
>>>
>>> Reviewed-by: Hans de Goede <[email protected]>
>>>
>>> Regards,
>>
>> Thanks Hans, Jens is this good to go from your perspective?
>
> I'll queue 1-4 up for 5.5 and mark for stable, then add 5-8 for
> 5.6. Thanks!

It looks like I will have two incremental changes on top to minimize the
number of resources that get cycled through during EPROBE_DEFER and also
ensure that the 7216 reset line gets properly managed with a call to
reset_control_reset() per review feedback from the reset controller
maintainer.
--
Florian

2020-01-07 16:48:23

by Naresh Kamboju

[permalink] [raw]
Subject: Re: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support

On Wed, 11 Dec 2019 at 00:25, Florian Fainelli <[email protected]> wrote:
>
> Hi Jens,
>
> The first 4 patches are fixes and should ideally be queued up/picked up
> by stable. The last 4 patches add support for BCM7216 which is one of
> our latest devices supported by this driver.
>
> Patch #2 does a few things, but it was pretty badly broken before and it
> is hard not to fix all call sites (probe, suspend, resume) in one shot.
>
> Please let me know if you have any comments.
>
> Thanks!
>
> Florian Fainelli (8):
> ata: libahci_platform: Export again ahci_platform_<en/dis>able_phys()
> ata: ahci_brcm: Fix AHCI resources management

Following error on stable-rc 4.14 and 4.9 branch for arm build.

drivers/ata/ahci_brcm.c: In function 'brcm_ahci_probe':
drivers/ata/ahci_brcm.c:412:28: error: 'struct brcm_ahci_priv' has no
member named 'rcdev'; did you mean 'dev'?
if (!IS_ERR_OR_NULL(priv->rcdev))
^~~~~
dev
CC fs/pnode.o
CC block/genhd.o
drivers/ata/ahci_brcm.c:413:3: error: implicit declaration of
function 'reset_control_assert'; did you mean 'ahci_reset_controller'?
[-Werror=implicit-function-declaration]
reset_control_assert(priv->rcdev);
^~~~~~~~~~~~~~~~~~~~
ahci_reset_controller
drivers/ata/ahci_brcm.c:413:30: error: 'struct brcm_ahci_priv' has no
member named 'rcdev'; did you mean 'dev'?
reset_control_assert(priv->rcdev);
^~~~~
dev
cc1: some warnings being treated as errors

Full build log links,
https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.14/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/702/consoleText
https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.9/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/773/consoleText


--
Linaro LKFT
https://lkft.linaro.org

2020-01-07 17:41:14

by Florian Fainelli

[permalink] [raw]
Subject: Re: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support

On 1/7/20 9:29 AM, Naresh Kamboju wrote:
> On Tue, 7 Jan 2020 at 22:17, Naresh Kamboju <[email protected]> wrote:
>>
>> On Wed, 11 Dec 2019 at 00:25, Florian Fainelli <[email protected]> wrote:
>>>
>>> Hi Jens,
>>>
>>> The first 4 patches are fixes and should ideally be queued up/picked up
>>> by stable. The last 4 patches add support for BCM7216 which is one of
>>> our latest devices supported by this driver.
>>>
>>> Patch #2 does a few things, but it was pretty badly broken before and it
>>> is hard not to fix all call sites (probe, suspend, resume) in one shot.
>>>
>>> Please let me know if you have any comments.
>>>
>>> Thanks!
>>>
>>> Florian Fainelli (8):
>>> ata: libahci_platform: Export again ahci_platform_<en/dis>able_phys()
>>> ata: ahci_brcm: Fix AHCI resources management
>>
>> Following error on stable-rc 4.14 and 4.9 branch for arm build.
>
> Following error on stable-rc 4.19, 4.14 and 4.9 branch for arm build.
>
>>
>> drivers/ata/ahci_brcm.c: In function 'brcm_ahci_probe':
>> drivers/ata/ahci_brcm.c:412:28: error: 'struct brcm_ahci_priv' has no
>> member named 'rcdev'; did you mean 'dev'?
>> if (!IS_ERR_OR_NULL(priv->rcdev))
>> ^~~~~
>> dev
>> CC fs/pnode.o
>> CC block/genhd.o
>> drivers/ata/ahci_brcm.c:413:3: error: implicit declaration of
>> function 'reset_control_assert'; did you mean 'ahci_reset_controller'?
>> [-Werror=implicit-function-declaration]
>> reset_control_assert(priv->rcdev);
>> ^~~~~~~~~~~~~~~~~~~~
>> ahci_reset_controller
>> drivers/ata/ahci_brcm.c:413:30: error: 'struct brcm_ahci_priv' has no
>> member named 'rcdev'; did you mean 'dev'?
>> reset_control_assert(priv->rcdev);
>> ^~~~~
>> dev
>> cc1: some warnings being treated as errors
>>
>> Full build log links,
>> https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.14/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/702/consoleText
>> https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.9/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/773/consoleText
> https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.19/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/404/consoleText

The reset controller support was added in
2b2c47d9e1fe90311b725125d6252a859ee87a79 ("ata: ahci_brcm: Allow
optional reset controller to be used") which was include in v4.20 and
newer so that explains the build failure.

You may want to cherry pick that change into the respective stable
branches and then back port the fixes if that is not too much trouble.
If that does not work or is impractical, please let me know and I can
provide directed backport changes for 4.9, 4.14 and 4.19.

Thank you
--
Florian

2020-01-07 18:38:24

by Naresh Kamboju

[permalink] [raw]
Subject: Re: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support

On Tue, 7 Jan 2020 at 22:17, Naresh Kamboju <[email protected]> wrote:
>
> On Wed, 11 Dec 2019 at 00:25, Florian Fainelli <[email protected]> wrote:
> >
> > Hi Jens,
> >
> > The first 4 patches are fixes and should ideally be queued up/picked up
> > by stable. The last 4 patches add support for BCM7216 which is one of
> > our latest devices supported by this driver.
> >
> > Patch #2 does a few things, but it was pretty badly broken before and it
> > is hard not to fix all call sites (probe, suspend, resume) in one shot.
> >
> > Please let me know if you have any comments.
> >
> > Thanks!
> >
> > Florian Fainelli (8):
> > ata: libahci_platform: Export again ahci_platform_<en/dis>able_phys()
> > ata: ahci_brcm: Fix AHCI resources management
>
> Following error on stable-rc 4.14 and 4.9 branch for arm build.

Following error on stable-rc 4.19, 4.14 and 4.9 branch for arm build.

>
> drivers/ata/ahci_brcm.c: In function 'brcm_ahci_probe':
> drivers/ata/ahci_brcm.c:412:28: error: 'struct brcm_ahci_priv' has no
> member named 'rcdev'; did you mean 'dev'?
> if (!IS_ERR_OR_NULL(priv->rcdev))
> ^~~~~
> dev
> CC fs/pnode.o
> CC block/genhd.o
> drivers/ata/ahci_brcm.c:413:3: error: implicit declaration of
> function 'reset_control_assert'; did you mean 'ahci_reset_controller'?
> [-Werror=implicit-function-declaration]
> reset_control_assert(priv->rcdev);
> ^~~~~~~~~~~~~~~~~~~~
> ahci_reset_controller
> drivers/ata/ahci_brcm.c:413:30: error: 'struct brcm_ahci_priv' has no
> member named 'rcdev'; did you mean 'dev'?
> reset_control_assert(priv->rcdev);
> ^~~~~
> dev
> cc1: some warnings being treated as errors
>
> Full build log links,
> https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.14/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/702/consoleText
> https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.9/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/773/consoleText
https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.19/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/404/consoleText
>
>
> --
> Linaro LKFT
> https://lkft.linaro.org

2020-01-07 19:32:50

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 0/8] ata: ahci_brcm: Fixes and new device support

On Tue, Jan 07, 2020 at 09:39:58AM -0800, Florian Fainelli wrote:
> On 1/7/20 9:29 AM, Naresh Kamboju wrote:
> > On Tue, 7 Jan 2020 at 22:17, Naresh Kamboju <[email protected]> wrote:
> >>
> >> On Wed, 11 Dec 2019 at 00:25, Florian Fainelli <[email protected]> wrote:
> >>>
> >>> Hi Jens,
> >>>
> >>> The first 4 patches are fixes and should ideally be queued up/picked up
> >>> by stable. The last 4 patches add support for BCM7216 which is one of
> >>> our latest devices supported by this driver.
> >>>
> >>> Patch #2 does a few things, but it was pretty badly broken before and it
> >>> is hard not to fix all call sites (probe, suspend, resume) in one shot.
> >>>
> >>> Please let me know if you have any comments.
> >>>
> >>> Thanks!
> >>>
> >>> Florian Fainelli (8):
> >>> ata: libahci_platform: Export again ahci_platform_<en/dis>able_phys()
> >>> ata: ahci_brcm: Fix AHCI resources management
> >>
> >> Following error on stable-rc 4.14 and 4.9 branch for arm build.
> >
> > Following error on stable-rc 4.19, 4.14 and 4.9 branch for arm build.
> >
> >>
> >> drivers/ata/ahci_brcm.c: In function 'brcm_ahci_probe':
> >> drivers/ata/ahci_brcm.c:412:28: error: 'struct brcm_ahci_priv' has no
> >> member named 'rcdev'; did you mean 'dev'?
> >> if (!IS_ERR_OR_NULL(priv->rcdev))
> >> ^~~~~
> >> dev
> >> CC fs/pnode.o
> >> CC block/genhd.o
> >> drivers/ata/ahci_brcm.c:413:3: error: implicit declaration of
> >> function 'reset_control_assert'; did you mean 'ahci_reset_controller'?
> >> [-Werror=implicit-function-declaration]
> >> reset_control_assert(priv->rcdev);
> >> ^~~~~~~~~~~~~~~~~~~~
> >> ahci_reset_controller
> >> drivers/ata/ahci_brcm.c:413:30: error: 'struct brcm_ahci_priv' has no
> >> member named 'rcdev'; did you mean 'dev'?
> >> reset_control_assert(priv->rcdev);
> >> ^~~~~
> >> dev
> >> cc1: some warnings being treated as errors
> >>
> >> Full build log links,
> >> https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.14/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/702/consoleText
> >> https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.9/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/773/consoleText
> > https://ci.linaro.org/view/lkft/job/openembedded-lkft-linux-stable-rc-4.19/DISTRO=lkft,MACHINE=am57xx-evm,label=docker-lkft/404/consoleText
>
> The reset controller support was added in
> 2b2c47d9e1fe90311b725125d6252a859ee87a79 ("ata: ahci_brcm: Allow
> optional reset controller to be used") which was include in v4.20 and
> newer so that explains the build failure.
>
> You may want to cherry pick that change into the respective stable
> branches and then back port the fixes if that is not too much trouble.
> If that does not work or is impractical, please let me know and I can
> provide directed backport changes for 4.9, 4.14 and 4.19.

No need, I'll just queue up the other needed patch now, thanks.

greg k-h