[PATCH 0/2] mmc: sdhci-msm: Add firmware-managed resource support for Nord SDCC

[PATCH 0/2] mmc: sdhci-msm: Add firmware-managed resource support for Nord SDCC

[Anjana Hari]

On certain Qualcomm automotive SoCs such as Nord, platform resources
including clocks, interconnect bandwidth, resets, and regulators are
owned and managed by firmware rather than directly by Linux. The
firmware abstracts these resources behind logical power domains and
exposes control through the SCMI power protocol.

This series extends the sdhci-msm driver to support this
firmware-managed model. Instead of directly programming GCC clocks and
PMIC regulators, the driver operates through two logical power domains:

  - "power": controls the SDCC rail. The driver requests power state
    transitions via pm_runtime_resume_and_get() / pm_runtime_put_sync()
    on the domain device, which are forwarded to firmware as SCMI
    power-state messages.

  - "perf": the SCMI perf domain driver dynamically registers the
    supported clock frequencies as OPPs by querying firmware at probe
    time. Clock rate selection uses dev_pm_opp_find_freq_floor() and
    dev_pm_opp_set_opp() on this domain.

Patch 1 adds the Device Tree binding for the new "qcom,nord-sdhci"
compatible string, documenting the two power-domain properties and the
absence of clock, interconnect, reset, and regulator entries.

Patch 2 introduces the firmware-managed variant in the driver. A new
sdhci_msm_fw_v5_var descriptor is added with its pd_data populated;
probe detects this and selects firmware-specific platform data and ops.
The new ops implement clock rate selection through the perf domain OPP
path and route suspend/resume through SCMI power-state transitions
rather than local clock and regulator management.

Signed-off-by: Anjana Hari <anjana.hari@oss.qualcomm.com>
---
Anjana Hari (2):
      dt-bindings: mmc: sdhci-msm: Document bindings for Nord SDCC
      mmc: sdhci-msm: Add support for firmware-managed resource platforms

 .../devicetree/bindings/mmc/qcom,nord-sdhci.yaml   |  89 +++++++++
 drivers/mmc/host/sdhci-msm.c                       | 213 ++++++++++++++++++++-
 2 files changed, 297 insertions(+), 5 deletions(-)
---
base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c
change-id: 20261102-sdcc_scmi-3c47baa8dba9

Best regards,
--  
Anjana Hari <anjana.hari@oss.qualcomm.com>

[PATCH 1/2] dt-bindings: mmc: sdhci-msm: Document bindings for Nord SDCC

[Anjana Hari]

Add Device Tree binding for the Nord SDHCI variant of the Qualcomm
sdhci-msm controller, where platform resources are managed by firmware
through the SCMI power protocol rather than being directly accessible
to Linux.

Unlike existing sdhci-msm variants, this binding does not require
clock, interconnect, reset, or regulator properties. Instead, platform
resource control is abstracted behind two logical power domains:

  - "power": controls the SDCC rail power state. The kernel requests
    power-on and power-off transitions via the runtime PM framework;
    these are forwarded to firmware as SCMI power-state messages.

  - "perf": exposes the supported clock frequencies as OPPs populated
    dynamically by the SCMI perf domain driver at probe time via the
    SCMI performance protocol. The driver selects operating points via
    dev_pm_opp_find_freq_floor() and dev_pm_opp_set_opp().

Both domains must be listed under power-domains with their names
declared in power-domain-names.

The compatible string "qcom,nord-sdhci" selects this variant at probe
time and causes the driver to attach the two power domains via
dev_pm_domain_attach_list() with PD_FLAG_DEV_LINK_ON, ensuring correct
suspend/resume ordering between the SDCC controller and the firmware
power domain devices.

Signed-off-by: Anjana Hari <anjana.hari@oss.qualcomm.com>
---
 .../devicetree/bindings/mmc/qcom,nord-sdhci.yaml   | 89 ++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/qcom,nord-sdhci.yaml b/Documentation/devicetree/bindings/mmc/qcom,nord-sdhci.yaml
new file mode 100644
index 000000000000..c512628617b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/qcom,nord-sdhci.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mmc/qcom,nord-sdhci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Nord SDHCI Controller
+
+maintainers:
+  - Anjana Hari <anjana.hari@oss.qualcomm.com>
+
+description:
+  SDHCI (SD Host Controller Interface) present on Qualcomm Nord SoC.
+  Clock and power resources are managed by firmware through an SCMI
+  server, so no clock entries are required in the device tree node.
+
+properties:
+  compatible:
+    const: qcom,nord-sdhci
+
+  reg:
+    maxItems: 1
+
+  reg-names:
+    items:
+      - const: hc
+
+  interrupts:
+    maxItems: 2
+
+  interrupt-names:
+    items:
+      - const: hc_irq
+      - const: pwr_irq
+
+  iommus:
+    maxItems: 1
+    description: phandle to the apps SMMU node with SID mask.
+
+  dma-coherent: true
+
+  power-domains:
+    maxItems: 2
+
+  power-domain-names:
+    items:
+      - const: power
+      - const: perf
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - iommus
+  - dma-coherent
+  - power-domains
+  - power-domain-names
+
+allOf:
+  - $ref: sdhci-common.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    mmc@b84000 {
+        compatible = "qcom,nord-sdhci";
+        reg = <0x000b84000 0x1000>;
+        reg-names = "hc";
+
+        interrupts = <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 227 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "hc_irq", "pwr_irq";
+
+        bus-width = <4>;
+        no-sd;
+        no-mmc;
+        non-removable;
+
+        iommus = <&apps_smmu 0x1580 0x0>;
+        dma-coherent;
+
+        power-domains = <&scmi34_pd 0>, <&scmi34_dvfs 0>;
+        power-domain-names = "power", "perf";
+    };

-- 
2.34.1

[PATCH 2/2] mmc: sdhci-msm: Add support for firmware-managed resource platforms

[Anjana Hari]

On certain Qualcomm automotive SoCs such as Nord, platform resources
including clocks, interconnect bandwidth, resets, and regulators are
owned and managed by firmware rather than directly by Linux. The
firmware abstracts these resources behind logical power domains and
exposes control through the SCMI power protocol.

Introduce a firmware-managed variant of the sdhci-msm driver that
operates through this abstraction instead of directly programming GCC
clocks and PMIC regulators.

The firmware-managed model uses two logical power domains, attached at
probe time via dev_pm_domain_attach_list():

  - "power" domain: governs SDCC rail power state. It is activated at
    probe and released at remove via pm_runtime_resume_and_get() and
    pm_runtime_put_sync() on the domain device. These calls are
    translated by the SCMI power protocol into firmware power-state
    requests.

  - "perf" domain: the SCMI perf domain driver populates OPPs at probe
    time by querying the firmware-reported performance levels via
    perf_ops->device_opps_add(). Clock rate selection uses
    dev_pm_opp_find_freq_floor() to locate the nearest OPP at or below
    the requested frequency, then dev_pm_opp_set_opp() to apply it,
    rather than directly programming the GCC clock tree.

A new variant sdhci_msm_fw_v5_var is introduced with the compatible
string "qcom,nord-sdhci". Detection is automatic at probe: a non-NULL
pd_data.pd_names in the variant descriptor marks the platform as
firmware-managed, causing probe to select firmware-specific platform
data (sdhci_msm_fw_pdata) and ops (sdhci_msm_fw_ops).

The firmware-managed ops provide:
  - sdhci_msm_fw_get_max_clock: queries the perf domain OPP table
    using dev_pm_opp_find_freq_floor() at ULONG_MAX to discover the
    maximum supported frequency, replacing the direct GCC readout.
  - sdhci_msm_fw_set_clock: routes clock rate programming through the
    perf domain OPP path, reusing the existing bus-mode multiplier
    logic to derive the target rate.

Suspend and resume sequences bypass local clock and regulator management
for firmware-managed platforms, as firmware handles those transitions
autonomously in response to SCMI power-state transitions from the
runtime PM framework.

Co-developed by: Shazad Hussain <shazad.hussain@oss.qualcomm.com>
Signed-off-by: Shazad Hussain <shazad.hussain@oss.qualcomm.com>
Signed-off-by: Anjana Hari <anjana.hari@oss.qualcomm.com>
---
 drivers/mmc/host/sdhci-msm.c | 213 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 208 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index aecd425bf095..da52a45203a5 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -8,8 +8,9 @@
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/mmc/mmc.h>
-#include <linux/pm_runtime.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/iopoll.h>
 #include <linux/regulator/consumer.h>
@@ -168,6 +169,9 @@
 #define ICE_HCI_PARAM_CCI	GENMASK(7, 0)
 #define ICE_HCI_PARAM_CE	GENMASK(8, 8)
 
+#define DOMAIN_IDX_POWER	0
+#define DOMAIN_IDX_PERF		1
+
 struct sdhci_msm_offset {
 	u32 core_hc_mode;
 	u32 core_mci_data_cnt;
@@ -275,6 +279,11 @@ struct sdhci_msm_variant_info {
 	bool restore_dll_config;
 	const struct sdhci_msm_variant_ops *var_ops;
 	const struct sdhci_msm_offset *offset;
+	/*
+	 * pd_names != NULL marks this variant as firmware managed;
+	 * see fw_managed detection in sdhci_msm_probe().
+	 */
+	const struct dev_pm_domain_attach_data pd_data;
 };
 
 struct sdhci_msm_host {
@@ -285,6 +294,7 @@ struct sdhci_msm_host {
 	struct clk *xo_clk;	/* TCXO clk needed for FLL feature of cm_dll*/
 	/* core, iface, cal and sleep clocks */
 	struct clk_bulk_data bulk_clks[4];
+	struct dev_pm_domain_list *pd_list;
 #ifdef CONFIG_MMC_CRYPTO
 	struct qcom_ice *ice;
 #endif
@@ -302,6 +312,7 @@ struct sdhci_msm_host {
 	u32 caps_0;
 	bool mci_removed;
 	bool restore_dll_config;
+	bool fw_managed;
 	const struct sdhci_msm_variant_ops *var_ops;
 	const struct sdhci_msm_offset *offset;
 	bool use_cdr;
@@ -414,6 +425,45 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
 		 mmc_hostname(host->mmc), achieved_rate, timing);
 }
 
+static int msm_fw_set_clock_rate_for_bus_mode(struct sdhci_host *host,
+					      unsigned int clock)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	struct device *dev = mmc_dev(host->mmc);
+	struct dev_pm_opp *opp;
+	struct device *perf_dev;
+	struct mmc_ios ios = host->mmc->ios;
+	unsigned long clock_long = (unsigned long)clock;
+	unsigned long desired_rate;
+	unsigned int mult;
+	int ret;
+
+	perf_dev = msm_host->pd_list->pd_devs[DOMAIN_IDX_PERF];
+
+	mult = msm_get_clock_mult_for_bus_mode(host, clock, ios.timing);
+	desired_rate = clock_long * mult;
+
+	/* Find the nearest OPP at or below the requested frequency. */
+	opp = dev_pm_opp_find_freq_floor(perf_dev, &desired_rate);
+	if (IS_ERR(opp)) {
+		dev_err(dev, "Failed to find OPP for freq %lu: %pe\n", clock_long, opp);
+		return PTR_ERR(opp);
+	}
+
+	ret = dev_pm_opp_set_opp(perf_dev, opp);
+	dev_pm_opp_put(opp);
+	if (ret) {
+		dev_err(dev, "Failed to set OPP for freq %lu: %d\n", clock_long, ret);
+		return ret;
+	}
+
+	/* desired_rate was rounded down by find_freq_floor to the actual OPP rate. */
+	host->mmc->actual_clock = mult ? desired_rate / mult : desired_rate;
+	msm_host->clk_rate = desired_rate;
+	return 0;
+}
+
 /* Platform specific tuning */
 static inline int msm_dll_poll_ck_out_en(struct sdhci_host *host, u8 poll)
 {
@@ -1253,7 +1303,10 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	 */
 	if (host->flags & SDHCI_HS400_TUNING) {
 		sdhci_msm_hc_select_mode(host);
-		msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
+		if (msm_host->fw_managed)
+			msm_fw_set_clock_rate_for_bus_mode(host, ios.clock);
+		else
+			msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
 		host->flags &= ~SDHCI_HS400_TUNING;
 	}
 
@@ -1847,6 +1900,28 @@ static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
 	return SDHCI_MSM_MIN_CLOCK;
 }
 
+static unsigned int sdhci_msm_fw_get_max_clock(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	unsigned long max_freq = ULONG_MAX;
+	struct dev_pm_opp *opp;
+	struct device *perf_dev;
+	struct device *dev = mmc_dev(host->mmc);
+
+	perf_dev = msm_host->pd_list->pd_devs[DOMAIN_IDX_PERF];
+
+	opp = dev_pm_opp_find_freq_floor(perf_dev, &max_freq);
+	if (IS_ERR(opp)) {
+		dev_err(dev, "Failed to find opp for freq %lu\n", max_freq);
+		return SDHCI_MSM_MIN_CLOCK;
+	}
+
+	dev_pm_opp_put(opp);
+
+	return max_freq;
+}
+
 /*
  * __sdhci_msm_set_clock - sdhci_msm clock control.
  *
@@ -1892,6 +1967,40 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
 	__sdhci_msm_set_clock(host, clock);
 }
 
+static void sdhci_msm_fw_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	if (!clock) {
+		host->mmc->actual_clock = 0;
+		msm_host->clk_rate = 0;
+		goto out;
+	}
+
+	sdhci_msm_hc_select_mode(host);
+
+	if (msm_fw_set_clock_rate_for_bus_mode(host, clock))
+		return;
+out:
+	__sdhci_msm_set_clock(host, clock);
+}
+
+static int sdhci_msm_fw_power_state(struct device *dev, bool power_on)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	struct device *pwr_dev;
+
+	pwr_dev = msm_host->pd_list->pd_devs[DOMAIN_IDX_POWER];
+
+	if (power_on)
+		return pm_runtime_resume_and_get(pwr_dev);
+	else
+		return pm_runtime_put_sync(pwr_dev);
+}
+
 /*****************************************************************************\
  *                                                                           *
  * Inline Crypto Engine (ICE) support                                        *
@@ -2550,6 +2659,17 @@ static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
 	.offset = &sdhci_msm_v5_offset,
 };
 
+static const struct sdhci_msm_variant_info sdhci_msm_fw_v5_var = {
+	.mci_removed = true,
+	.var_ops = &v5_var_ops,
+	.offset = &sdhci_msm_v5_offset,
+	.pd_data = {
+		.pd_flags = PD_FLAG_DEV_LINK_ON,
+		.pd_names = (const char * []) { "power", "perf" },
+		.num_pd_names = 2,
+	},
+};
+
 static const struct sdhci_msm_variant_info sdm845_sdhci_var = {
 	.mci_removed = true,
 	.restore_dll_config = true,
@@ -2566,6 +2686,7 @@ static const struct of_device_id sdhci_msm_dt_match[] = {
 	{.compatible = "qcom,sdhci-msm-v5", .data = &sdhci_msm_v5_var},
 	{.compatible = "qcom,sdm670-sdhci", .data = &sdm845_sdhci_var},
 	{.compatible = "qcom,sdm845-sdhci", .data = &sdm845_sdhci_var},
+	{.compatible = "qcom,nord-sdhci", .data = &sdhci_msm_fw_v5_var},
 	{.compatible = "qcom,sc7180-sdhci", .data = &sdm845_sdhci_var},
 	{},
 };
@@ -2587,6 +2708,21 @@ static const struct sdhci_ops sdhci_msm_ops = {
 	.set_timeout = sdhci_msm_set_timeout,
 };
 
+static const struct sdhci_ops sdhci_msm_fw_ops = {
+	.reset = sdhci_and_cqhci_reset,
+	.set_clock = sdhci_msm_fw_set_clock,
+	.get_min_clock = sdhci_msm_get_min_clock,
+	.get_max_clock = sdhci_msm_fw_get_max_clock,
+	.set_bus_width = sdhci_set_bus_width,
+	.set_uhs_signaling = sdhci_msm_set_uhs_signaling,
+	.write_w = sdhci_msm_writew,
+	.write_b = sdhci_msm_writeb,
+	.irq	= sdhci_msm_cqe_irq,
+	.dump_vendor_regs = sdhci_msm_dump_vendor_regs,
+	.set_power = sdhci_set_power_noreg,
+	.set_timeout = sdhci_msm_set_timeout,
+};
+
 static const struct sdhci_pltfm_data sdhci_msm_pdata = {
 	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
 		  SDHCI_QUIRK_SINGLE_POWER_WRITE |
@@ -2597,6 +2733,16 @@ static const struct sdhci_pltfm_data sdhci_msm_pdata = {
 	.ops = &sdhci_msm_ops,
 };
 
+static const struct sdhci_pltfm_data sdhci_msm_fw_pdata = {
+	.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
+		  SDHCI_QUIRK_SINGLE_POWER_WRITE |
+		  SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
+		  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
+
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+	.ops = &sdhci_msm_fw_ops,
+};
+
 static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
 		struct sdhci_host *host)
 {
@@ -2665,8 +2811,21 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	const struct sdhci_msm_offset *msm_offset;
 	const struct sdhci_msm_variant_info *var_info;
 	struct device_node *node = pdev->dev.of_node;
+	bool fw_managed;
 
-	host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
+	var_info = of_device_get_match_data(&pdev->dev);
+	if (!var_info)
+		return -ENODEV;
+	fw_managed = !!var_info->pd_data.pd_names;
+
+	/*
+	 * Based on variant info, differentiate platform initialization
+	 * for local and remote managed resource platforms.
+	 */
+	if (fw_managed)
+		host = sdhci_pltfm_init(pdev, &sdhci_msm_fw_pdata, sizeof(*msm_host));
+	else
+		host = sdhci_pltfm_init(pdev, &sdhci_msm_pdata, sizeof(*msm_host));
 	if (IS_ERR(host))
 		return PTR_ERR(host);
 
@@ -2684,12 +2843,11 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	 * Based on the compatible string, load the required msm host info from
 	 * the data associated with the version info.
 	 */
-	var_info = of_device_get_match_data(&pdev->dev);
-
 	msm_host->mci_removed = var_info->mci_removed;
 	msm_host->restore_dll_config = var_info->restore_dll_config;
 	msm_host->var_ops = var_info->var_ops;
 	msm_host->offset = var_info->offset;
+	msm_host->fw_managed = fw_managed;
 
 	msm_offset = msm_host->offset;
 
@@ -2698,6 +2856,32 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
 
+	if (msm_host->fw_managed) {
+		ret = dev_pm_domain_attach_list(&pdev->dev, &var_info->pd_data,
+						&msm_host->pd_list);
+		if (ret < 0)
+			return dev_err_probe(&pdev->dev, ret,
+					     "Failed to attach power domains\n");
+
+		if (ret != var_info->pd_data.num_pd_names)
+			return dev_err_probe(&pdev->dev, -ENODEV,
+					     "Expected %d power domains, got %d\n",
+					     var_info->pd_data.num_pd_names, ret);
+
+		ret = sdhci_msm_fw_power_state(&pdev->dev, true);
+		if (ret) {
+			dev_pm_domain_detach_list(msm_host->pd_list);
+			return dev_err_probe(&pdev->dev, ret,
+					     "Failed to power on domain\n");
+		}
+
+		/*
+		 * Skip initializing clocks, regulators and reset in kernel
+		 * for firmware managed platforms.
+		 */
+		goto skip_resource_init;
+	}
+
 	ret = sdhci_msm_gcc_reset(&pdev->dev, host);
 	if (ret)
 		return ret;
@@ -2786,6 +2970,7 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 		}
 	}
 
+skip_resource_init:
 	/* Reset the vendor spec register to power on reset state */
 	writel_relaxed(CORE_VENDOR_SPEC_POR_VAL,
 			host->ioaddr + msm_offset->core_vendor_spec);
@@ -2917,6 +3102,12 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 bus_clk_disable:
 	if (!IS_ERR(msm_host->bus_clk))
 		clk_disable_unprepare(msm_host->bus_clk);
+fw_detach:
+	if (msm_host->fw_managed) {
+		sdhci_msm_fw_power_state(&pdev->dev, false);
+		dev_pm_domain_detach_list(msm_host->pd_list);
+		msm_host->pd_list = NULL;
+	}
 	return ret;
 }
 
@@ -2930,6 +3121,12 @@ static void sdhci_msm_remove(struct platform_device *pdev)
 
 	sdhci_remove_host(host, dead);
 
+	if (msm_host->fw_managed) {
+		sdhci_msm_fw_power_state(&pdev->dev, false);
+		dev_pm_domain_detach_list(msm_host->pd_list);
+		msm_host->pd_list = NULL;
+	}
+
 	pm_runtime_get_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_put_noidle(&pdev->dev);
@@ -2947,6 +3144,9 @@ static int sdhci_msm_runtime_suspend(struct device *dev)
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	unsigned long flags;
 
+	if (msm_host->fw_managed)
+		return sdhci_msm_fw_power_state(dev, false);
+
 	spin_lock_irqsave(&host->lock, flags);
 	host->runtime_suspended = true;
 	spin_unlock_irqrestore(&host->lock, flags);
@@ -2967,6 +3167,9 @@ static int sdhci_msm_runtime_resume(struct device *dev)
 	unsigned long flags;
 	int ret;
 
+	if (msm_host->fw_managed)
+		return sdhci_msm_fw_power_state(dev, true);
+
 	ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
 				       msm_host->bulk_clks);
 	if (ret)

-- 
2.34.1

Re: [PATCH 2/2] mmc: sdhci-msm: Add support for firmware-managed resource platforms

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mmc: sdhci-msm: NULL deref and PM refcount underflow in sdhci_msm_remove()
- [High] mmc: sdhci-msm: host->runtime_suspended not updated on fw_managed runtime PM
- [Medium] mmc: sdhci-msm: redundant pwr_dev RPM refs with PD_FLAG_DEV_LINK_ON
- [Low] mmc: sdhci-msm: unused label fw_detach in sdhci_msm_probe()
--

commit 23a31b3dbe728fdf7361ca3e72723808a56fa53e
Author: Anjana Hari <anjana.hari@oss.qualcomm.com>

mmc: sdhci-msm: Add support for firmware-managed resource platforms

This commit introduces a firmware-managed variant of the sdhci-msm driver for
Qualcomm SoCs such as Nord, managing power and performance states via logical
power domains and OPPs rather than programming GCC clocks and regulators
directly.

> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index aecd425bf0951..da52a45203a57 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c

[ ... ]

> @@ -1892,6 +1967,40 @@ static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>  	__sdhci_msm_set_clock(host, clock);
>  }
>  

[ ... ]

> +static int sdhci_msm_fw_power_state(struct device *dev, bool power_on)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	struct device *pwr_dev;
> +
> +	pwr_dev = msm_host->pd_list->pd_devs[DOMAIN_IDX_POWER];
> +
> +	if (power_on)
> +		return pm_runtime_resume_and_get(pwr_dev);
> +	else
> +		return pm_runtime_put_sync(pwr_dev);
> +}
> +
>  /*****************************************************************************\
>   *                                                                           *
>   * Inline Crypto Engine (ICE) support                                        *
> @@ -2550,6 +2659,17 @@ static const struct sdhci_msm_variant_info sdhci_msm_v5_var = {
>  	.offset = &sdhci_msm_v5_offset,
>  };
>  
> +static const struct sdhci_msm_variant_info sdhci_msm_fw_v5_var = {
> +	.mci_removed = true,
> +	.var_ops = &v5_var_ops,
> +	.offset = &sdhci_msm_v5_offset,
> +	.pd_data = {
> +		.pd_flags = PD_FLAG_DEV_LINK_ON,
> +		.pd_names = (const char * []) { "power", "perf" },
> +		.num_pd_names = 2,
> +	},
> +};

[Severity: Medium]
Does combining PD_FLAG_DEV_LINK_ON in sdhci_msm_fw_v5_var with manual
pm_runtime_resume_and_get(pwr_dev) and pm_runtime_put_sync(pwr_dev) calls in
sdhci_msm_fw_power_state() cause duplicate runtime PM reference counting on
pwr_dev?

When dev_pm_domain_attach_list() runs with PD_FLAG_DEV_LINK_ON (and without
PD_FLAG_NO_DEV_LINK), it creates a device link with DL_FLAG_STATELESS |
DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE, which already calls
pm_runtime_get_sync(pwr_dev), and pm_runtime_set_active(&pdev->dev) in
sdhci_msm_probe() also calls rpm_get_suppliers(&pdev->dev). Calling
sdhci_msm_fw_power_state(&pdev->dev, true) in sdhci_msm_probe() and
sdhci_msm_runtime_resume() then takes an additional manual runtime PM
reference on pwr_dev.

Because __rpm_callback() invokes sdhci_msm_runtime_suspend() before
__rpm_put_suppliers(&pdev->dev, false), the device link still holds its
runtime PM reference on pwr_dev when sdhci_msm_fw_power_state(dev, false)
calls pm_runtime_put_sync(pwr_dev). As a result, __pm_runtime_idle() sees
usage_count > 0 and returns 0 without suspending pwr_dev synchronously.

Similarly, in the error path of sdhci_msm_probe() and in sdhci_msm_remove(),
sdhci_msm_fw_power_state(&pdev->dev, false) runs before
dev_pm_domain_detach_list() deletes the device link, and during
pm_runtime_force_suspend(&pdev->dev), if pm_runtime_need_not_resume() is
false, pm_runtime_set_suspended() / rpm_put_suppliers() is skipped, leaving
the device link's runtime PM reference on pwr_dev active.

[ ... ]

> @@ -2917,6 +3102,12 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  bus_clk_disable:
>  	if (!IS_ERR(msm_host->bus_clk))
>  		clk_disable_unprepare(msm_host->bus_clk);
> +fw_detach:
> +	if (msm_host->fw_managed) {
> +		sdhci_msm_fw_power_state(&pdev->dev, false);
> +		dev_pm_domain_detach_list(msm_host->pd_list);
> +		msm_host->pd_list = NULL;
> +	}
>  	return ret;
>  }

[Severity: Low]
Is the fw_detach label in sdhci_msm_probe() needed? Nothing in
sdhci_msm_probe() jumps to fw_detach, which triggers an unused-label compiler
warning (-Wunused-label) and breaks builds with CONFIG_WERROR=y.

>  
> @@ -2930,6 +3121,12 @@ static void sdhci_msm_remove(struct platform_device *pdev)
>  
>  	sdhci_remove_host(host, dead);
>  
> +	if (msm_host->fw_managed) {
> +		sdhci_msm_fw_power_state(&pdev->dev, false);
> +		dev_pm_domain_detach_list(msm_host->pd_list);
> +		msm_host->pd_list = NULL;
> +	}
> +
>  	pm_runtime_get_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_put_noidle(&pdev->dev);

[Severity: High]
Can detaching and clearing msm_host->pd_list before pm_runtime_get_sync() and
pm_runtime_disable() in sdhci_msm_remove() cause a NULL pointer dereference
and runtime PM refcount underflow?

In sdhci_msm_remove(), sdhci_remove_host() releases the MMC host while runtime
PM with autosuspend is still enabled on &pdev->dev (or &pdev->dev may already
be runtime-suspended when entering sdhci_msm_remove()):

  sdhci_msm_remove()
    sdhci_remove_host()
    if (msm_host->fw_managed) {
        sdhci_msm_fw_power_state(&pdev->dev, false);
        dev_pm_domain_detach_list(msm_host->pd_list);
        msm_host->pd_list = NULL;
    }
    pm_runtime_get_sync(&pdev->dev)
      -> sdhci_msm_runtime_resume()
        -> sdhci_msm_fw_power_state(&pdev->dev, true)
          -> msm_host->pd_list->pd_devs[DOMAIN_IDX_POWER]

If &pdev->dev is runtime-suspended at that point,
sdhci_msm_fw_power_state(&pdev->dev, false) calls pm_runtime_put_sync(pwr_dev)
a second time (underflowing pwr_dev's usage_count), and the subsequent
pm_runtime_get_sync(&pdev->dev) invokes sdhci_msm_runtime_resume() ->
sdhci_msm_fw_power_state(&pdev->dev, true), which dereferences
msm_host->pd_list after it has already been detached and set to NULL. Even if
&pdev->dev is active, an autosuspend timer can fire concurrently before
pm_runtime_disable(&pdev->dev) is called.

Should the msm_host->fw_managed cleanup in sdhci_msm_remove() be moved after
pm_runtime_get_sync(), pm_runtime_disable(), and pm_runtime_put_noidle(),
matching clk_bulk_disable_unprepare() and the unwind order in
sdhci_msm_probe()?

> @@ -2947,6 +3144,9 @@ static int sdhci_msm_runtime_suspend(struct device *dev)
>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>  	unsigned long flags;
>  
> +	if (msm_host->fw_managed)
> +		return sdhci_msm_fw_power_state(dev, false);
> +
>  	spin_lock_irqsave(&host->lock, flags);
>  	host->runtime_suspended = true;
>  	spin_unlock_irqrestore(&host->lock, flags);
> @@ -2967,6 +3167,9 @@ static int sdhci_msm_runtime_resume(struct device *dev)
>  	unsigned long flags;
>  	int ret;
>  
> +	if (msm_host->fw_managed)
> +		return sdhci_msm_fw_power_state(dev, true);
> +
>  	ret = clk_bulk_prepare_enable(ARRAY_SIZE(msm_host->bulk_clks),
>  				       msm_host->bulk_clks);
>  	if (ret)

[Severity: High]
Should sdhci_msm_runtime_suspend() and sdhci_msm_runtime_resume() still update
host->runtime_suspended under host->lock when msm_host->fw_managed is true?

In drivers/mmc/host/sdhci.c, both sdhci_led_control() and sdhci_irq() check
host->runtime_suspended under host->lock before accessing SDHCI MMIO registers
(SDHCI_HOST_CONTROL and SDHCI_INT_STATUS):

  sdhci_led_control()
    spin_lock_irqsave(&host->lock, flags);
    if (host->runtime_suspended)
        goto out;

  sdhci_irq()
    spin_lock(&host->lock);
    if (host->runtime_suspended) {
        spin_unlock(&host->lock);
        return IRQ_NONE;
    }
    intmask = sdhci_readl(host, SDHCI_INT_STATUS);

Because sdhci_msm_runtime_suspend() and sdhci_msm_runtime_resume() return
early for msm_host->fw_managed without updating host->runtime_suspended (as
added in commit f8def10f73a5 ("mmc: sdhci-msm: pervent access to suspended
controller")), host->runtime_suspended remains false while the SDCC power
domain is powered off, allowing LED brightness updates or shared/spurious
interrupts to access unpowered MMIO registers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-sdcc_scmi-v1-0-244dbd893051@oss.qualcomm.com?part=2