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

[Florian Fainelli <florian.fainelli@broadcom.com>]

On 10/21/26 23:50, Damien Le Moal wrote:
> 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?

The NSP platform does not support suspend/resume but the consistency 
should be preserved, logically this should be just like during 
initialization, thanks!
-- 
Florian