Re: [PATCH 3/6] ata: ahci_brcm: Initialize the NSP interface clock and reset

[Damien Le Moal <dlemoal@kernel.org>]

On 2026/10/21 18:55, Florian Fainelli wrote:
> The NSP probe path assumes firmware has enabled the SATA IDM interface
> clock and released the core reset. The downstream iProc config_AHCI()
> sequence explicitly performs both operations before accessing AHCI.
> Without that setup, a cold boot can access an inactive controller.
> 
> Map the optional NSP IDM register region and enable the interface clock
> before releasing reset, retaining the downstream settling intervals.
> Perform the sequence at probe and resume before reconfiguring SATA.
> 
> Keep the resource optional so older device trees continue to rely on
> firmware initialization.
> 
> Fixes: 3ee2e6dcaa35 ("ata: ahci_brcm: Add support for Broadcom NSP SoC")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/ata/ahci_brcm.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 29be74fedcf0..2ed60806b671 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -6,7 +6,9 @@
>   */
>  
>  #include <linux/ahci_platform.h>
> +#include <linux/bcma/bcma_regs.h>
>  #include <linux/compiler.h>
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> @@ -83,6 +85,7 @@ enum brcm_ahci_quirks {
>  struct brcm_ahci_priv {
>  	struct device *dev;
>  	void __iomem *top_ctrl;
> +	void __iomem *idm;
>  	u32 port_mask;
>  	u32 quirks;
>  	enum brcm_ahci_version version;
> @@ -230,6 +233,25 @@ static u32 brcm_ahci_get_portmask(struct ahci_host_priv *hpriv,
>  	return impl;
>  }
>  
> +static void brcm_sata_init_idm(struct brcm_ahci_priv *priv)
> +{
> +	u32 data;
> +
> +	if (!priv->idm)
> +		return;
> +
> +	/* Enable the interface clock before taking the SATA core out of reset. */

Long line. Please split before 80 chars.

> +	data = readl(priv->idm + BCMA_IOCTL);
> +	writel(data | BCMA_IOCTL_CLK, priv->idm + BCMA_IOCTL);
> +	readl(priv->idm + BCMA_IOCTL);
> +	usleep_range(1000, 2000);
> +
> +	data = readl(priv->idm + BCMA_RESET_CTL);
> +	writel(data & ~BCMA_RESET_CTL_RESET, priv->idm + BCMA_RESET_CTL);
> +	readl(priv->idm + BCMA_RESET_CTL);
> +	usleep_range(1000, 2000);
> +}
> +
>  static void brcm_sata_init(struct brcm_ahci_priv *priv)
>  {
>  	void __iomem *ctrl = priv->top_ctrl + SATA_TOP_CTRL_BUS_CTRL;
> @@ -369,6 +391,8 @@ static int __maybe_unused brcm_ahci_resume(struct device *dev)
>  	struct brcm_ahci_priv *priv = hpriv->plat_data;
>  	int ret = 0;
>  
> +	brcm_sata_init_idm(priv);
> +
>  	ret = reset_control_deassert(priv->rcdev_ahci);
>  	if (ret)
>  		return ret;
> @@ -453,6 +477,14 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>  	priv->version = (unsigned long)of_id->data;
>  	priv->dev = dev;
>  
> +	/* Older NSP device trees rely on firmware to initialize the IDM. */
> +	if (priv->version == BRCM_SATA_NSP &&
> +	    platform_get_resource_byname(pdev, IORESOURCE_MEM, "idm")) {
> +		priv->idm = devm_platform_ioremap_resource_byname(pdev, "idm");
> +		if (IS_ERR(priv->idm))
> +			return PTR_ERR(priv->idm);
> +	}
> +
>  	priv->top_ctrl = devm_platform_ioremap_resource_byname(pdev, "top-ctrl");
>  	if (IS_ERR(priv->top_ctrl))
>  		return PTR_ERR(priv->top_ctrl);
> @@ -493,6 +525,8 @@ static int brcm_ahci_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	brcm_sata_init_idm(priv);

Here, this is called *before* the clocks are enabled, but in brcm_ahci_resume(),
brcm_sata_init() -> brcm_sata_init_idm(() is called *after* the clocks are
enabled. Does this matter?

> +
>  	ret = ahci_platform_enable_clks(hpriv);
>  	if (ret)
>  		goto out_reset;


-- 
Damien Le Moal
Western Digital Research