[PATCH V3 0/4] Add level shifter support for qualcomm SOC's

[PATCH V3 0/4] Add level shifter support for qualcomm SOC's

[Sarthak Garg]

Add level shifter support for qualcomm SOC's.

- Changed from v2
    - As suggested by Konrad Dybcio and Ulf Hansson redesigned logic
      to introduce a new DT property max-sd-hs-frequency and override
      the hs_max_dtr accordingly in sd.c file.

- Changed from v1
    - As suggested by Krzysztof Kozlowski redesigned logic to use
    compatible property for adding this level shifter support.
    - Addressed Adrian Hunter comments on V1 with resepect to
      checkpatch.
    - Cleared the bits first and then set bits in
      sdhci_msm_execute_tuning as suggested by Adrian Hunter.
    - Upated the if condition logic in msm_set_clock_rate_for_bus_mode
      as suggested by Adrian Hunter.

Sarthak Garg (4):
  mmc: sdhci-msm: Enable tuning for SDR50 mode for SD card
  dt-bindings: mmc: controller: Add max-sd-hs-frequency property
  mmc: core: Introduce a new flag max-sd-hs-frequency
  arm64: dts: qcom: sm8550: Remove SDR104/SDR50 broken capabilities

 .../bindings/mmc/mmc-controller-common.yaml       | 10 ++++++++++
 arch/arm64/boot/dts/qcom/sm8550.dtsi              |  4 +---
 drivers/mmc/core/host.c                           |  2 ++
 drivers/mmc/core/sd.c                             |  2 +-
 drivers/mmc/host/sdhci-msm.c                      | 15 +++++++++++++++
 include/linux/mmc/host.h                          |  1 +
 6 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH V3 1/4] mmc: sdhci-msm: Enable tuning for SDR50 mode for SD card

[Sarthak Garg]

For Qualcomm SoCs which needs level shifter for SD card, extra delay is
seen on receiver data path.

To compensate this delay enable tuning for SDR50 mode for targets which
has level shifter. SDHCI_SDR50_NEEDS_TUNING caps will be set for targets
with level shifter on Qualcomm SOC's.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-msm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 66c0d1ba2a33..bf91cb96a0ea 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -81,6 +81,7 @@
 #define CORE_IO_PAD_PWR_SWITCH_EN	BIT(15)
 #define CORE_IO_PAD_PWR_SWITCH	BIT(16)
 #define CORE_HC_SELECT_IN_EN	BIT(18)
+#define CORE_HC_SELECT_IN_SDR50	(4 << 19)
 #define CORE_HC_SELECT_IN_HS400	(6 << 19)
 #define CORE_HC_SELECT_IN_MASK	(7 << 19)
 
@@ -1133,6 +1134,10 @@ static bool sdhci_msm_is_tuning_needed(struct sdhci_host *host)
 {
 	struct mmc_ios *ios = &host->mmc->ios;
 
+	if (ios->timing == MMC_TIMING_UHS_SDR50 &&
+	    host->flags & SDHCI_SDR50_NEEDS_TUNING)
+		return true;
+
 	/*
 	 * Tuning is required for SDR104, HS200 and HS400 cards and
 	 * if clock frequency is greater than 100MHz in these modes.
@@ -1201,6 +1206,8 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	struct mmc_ios ios = host->mmc->ios;
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
+	u32 config;
 
 	if (!sdhci_msm_is_tuning_needed(host)) {
 		msm_host->use_cdr = false;
@@ -1217,6 +1224,14 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	 */
 	msm_host->tuning_done = 0;
 
+	if (ios.timing == MMC_TIMING_UHS_SDR50 &&
+	    host->flags & SDHCI_SDR50_NEEDS_TUNING) {
+		config = readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec);
+		config &= ~CORE_HC_SELECT_IN_MASK;
+		config |= CORE_HC_SELECT_IN_EN | CORE_HC_SELECT_IN_SDR50;
+		writel_relaxed(config, host->ioaddr + msm_offset->core_vendor_spec);
+	}
+
 	/*
 	 * For HS400 tuning in HS200 timing requires:
 	 * - select MCLK/2 in VENDOR_SPEC
-- 
2.34.1

[PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Sarthak Garg]

Introduce a new optional device tree property `max-sd-hs-frequency` to
limit the maximum frequency (in Hz) used for SD cards operating in
High-Speed (HS) mode.

This property is useful for platforms with vendor-specific hardware
constraints, such as the presence of a level shifter that cannot
reliably support the default 50 MHz HS frequency. It allows the host
driver to cap the HS mode frequency accordingly.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 .../devicetree/bindings/mmc/mmc-controller-common.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
index 9a7235439759..1976f5f8c401 100644
--- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
+++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
@@ -93,6 +93,16 @@ properties:
     minimum: 400000
     maximum: 384000000
 
+  max-sd-hs-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Maximum frequency (in Hz) to be used for SD cards operating in
+      High-Speed (HS) mode. This is useful for platforms with vendor-specific
+      limitations, such as the presence of a level shifter that cannot support
+      the default 50 MHz HS frequency or other.
+    minimum: 400000
+    maximum: 50000000
+
   disable-wp:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
-- 
2.34.1

[PATCH V3 3/4] mmc: core: Introduce a new flag max-sd-hs-frequency

[Sarthak Garg]

Introduce a new device tree flag to cap the maximum High-Speed (HS)
mode frequency for SD cards, accommodating vendor-specific
constraints such as the inclusion of a level shifter which cannot
support the default 50Mhz HS frequency and others.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 drivers/mmc/core/host.c  | 2 ++
 drivers/mmc/core/sd.c    | 2 +-
 include/linux/mmc/host.h | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index dacb5bd9bb71..7892e80bc073 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -302,6 +302,8 @@ int mmc_of_parse(struct mmc_host *host)
 	/* f_max is obtained from the optional "max-frequency" property */
 	device_property_read_u32(dev, "max-frequency", &host->f_max);
 
+	device_property_read_u32(dev, "max-sd-hs-frequency", &host->max_sd_hs_freq);
+
 	/*
 	 * Configure CD and WP pins. They are both by default active low to
 	 * match the SDHCI spec. If GPIOs are provided for CD and / or WP, the
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index ec02067f03c5..d2f1766bddb4 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -359,7 +359,7 @@ static int mmc_read_switch(struct mmc_card *card)
 	}
 
 	if (status[13] & SD_MODE_HIGH_SPEED)
-		card->sw_caps.hs_max_dtr = HIGH_SPEED_MAX_DTR;
+		card->sw_caps.hs_max_dtr = card->host->max_sd_hs_freq ?: HIGH_SPEED_MAX_DTR;
 
 	if (card->scr.sda_spec3) {
 		card->sw_caps.sd3_bus_mode = status[13];
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 68f09a955a90..552e8a22c0a2 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -565,6 +565,7 @@ struct mmc_host {
 	int			hsq_depth;
 
 	u32			err_stats[MMC_ERR_MAX];
+	unsigned int		max_sd_hs_freq;
 	unsigned long		private[] ____cacheline_aligned;
 };
 
-- 
2.34.1

[PATCH V3 4/4] arm64: dts: qcom: sm8550: Remove SDR104/SDR50 broken capabilities

[Sarthak Garg]

Kernel now handles all level shifter limitations related to SD card
modes.
As a result, the broken hardware capabilities for SDR104 and SDR50 modes
can be removed from the device tree.
Additionally, due to level shifter constraints, set the maximum
frequency for High Speed (HS) mode to 37.5 MHz using the
max-sd-hs-frequency property for sm8550.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index 82cabf777cd2..2c770c979d39 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -3180,6 +3180,7 @@ sdhc_2: mmc@8804000 {
 			iommus = <&apps_smmu 0x540 0>;
 			qcom,dll-config = <0x0007642c>;
 			qcom,ddr-config = <0x80040868>;
+			max-sd-hs-frequency = <37500000>;
 			power-domains = <&rpmhpd RPMHPD_CX>;
 			operating-points-v2 = <&sdhc2_opp_table>;
 
@@ -3191,9 +3192,6 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			bus-width = <4>;
 			dma-coherent;
 
-			/* Forbid SDR104/SDR50 - broken hw! */
-			sdhci-caps-mask = <0x3 0>;
-
 			status = "disabled";
 
 			sdhc2_opp_table: opp-table {
-- 
2.34.1

Re: [PATCH V3 4/4] arm64: dts: qcom: sm8550: Remove SDR104/SDR50 broken capabilities

[Krzysztof Kozlowski]

On 18/06/2025 09:28, Sarthak Garg wrote:
> Kernel now handles all level shifter limitations related to SD card
> modes.
> As a result, the broken hardware capabilities for SDR104 and SDR50 modes
> can be removed from the device tree.
> Additionally, due to level shifter constraints, set the maximum
> frequency for High Speed (HS) mode to 37.5 MHz using the
> max-sd-hs-frequency property for sm8550.
> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8550.dtsi | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> index 82cabf777cd2..2c770c979d39 100644
> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
> @@ -3180,6 +3180,7 @@ sdhc_2: mmc@8804000 {
>  			iommus = <&apps_smmu 0x540 0>;
>  			qcom,dll-config = <0x0007642c>;
>  			qcom,ddr-config = <0x80040868>;
> +			max-sd-hs-frequency = <37500000>;
So my previous comments stay... This is SoC thus deducible from compatible.

Best regards,
Krzysztof

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Krzysztof Kozlowski]

On 18/06/2025 09:28, Sarthak Garg wrote:
> Introduce a new optional device tree property `max-sd-hs-frequency` to
> limit the maximum frequency (in Hz) used for SD cards operating in
> High-Speed (HS) mode.
> 
> This property is useful for platforms with vendor-specific hardware
> constraints, such as the presence of a level shifter that cannot
> reliably support the default 50 MHz HS frequency. It allows the host
> driver to cap the HS mode frequency accordingly.
> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  .../devicetree/bindings/mmc/mmc-controller-common.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
> index 9a7235439759..1976f5f8c401 100644
> --- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
> @@ -93,6 +93,16 @@ properties:
>      minimum: 400000
>      maximum: 384000000
>  
> +  max-sd-hs-frequency:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Maximum frequency (in Hz) to be used for SD cards operating in
> +      High-Speed (HS) mode. This is useful for platforms with vendor-specific
> +      limitations, such as the presence of a level shifter that cannot support
> +      the default 50 MHz HS frequency or other.
> +    minimum: 400000
> +    maximum: 50000000

This might be fine, but your DTS suggests clearly this is SoC compatible
deducible, which I already said at v1.

So now you send v3 which is the same as v1, so you get the same comments.

Best regards,
Krzysztof

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Sarthak Garg]

On 6/18/2025 1:13 PM, Krzysztof Kozlowski wrote:
> On 18/06/2025 09:28, Sarthak Garg wrote:
>> Introduce a new optional device tree property `max-sd-hs-frequency` to
>> limit the maximum frequency (in Hz) used for SD cards operating in
>> High-Speed (HS) mode.
>>
>> This property is useful for platforms with vendor-specific hardware
>> constraints, such as the presence of a level shifter that cannot
>> reliably support the default 50 MHz HS frequency. It allows the host
>> driver to cap the HS mode frequency accordingly.
>>
>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>> ---
>>   .../devicetree/bindings/mmc/mmc-controller-common.yaml | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>> index 9a7235439759..1976f5f8c401 100644
>> --- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>> @@ -93,6 +93,16 @@ properties:
>>       minimum: 400000
>>       maximum: 384000000
>>   
>> +  max-sd-hs-frequency:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
>> +      Maximum frequency (in Hz) to be used for SD cards operating in
>> +      High-Speed (HS) mode. This is useful for platforms with vendor-specific
>> +      limitations, such as the presence of a level shifter that cannot support
>> +      the default 50 MHz HS frequency or other.
>> +    minimum: 400000
>> +    maximum: 50000000
> 
> This might be fine, but your DTS suggests clearly this is SoC compatible
> deducible, which I already said at v1.
> 
> So now you send v3 which is the same as v1, so you get the same comments.
> 
> Best regards,
> Krzysztof

Introducing this flag no longer becomes SoC compatible because as per 
discussions in V2 patchset with Ulf and Konrad this new property can be 
used by any vendor who wants to limit the HS mode frequency due to any 
reason. Thats why moved to this generic approach again in V3 as compared 
to compatible based approach in V2.

Re: [PATCH V3 4/4] arm64: dts: qcom: sm8550: Remove SDR104/SDR50 broken capabilities

[Sarthak Garg]

On 6/18/2025 1:11 PM, Krzysztof Kozlowski wrote:
> On 18/06/2025 09:28, Sarthak Garg wrote:
>> Kernel now handles all level shifter limitations related to SD card
>> modes.
>> As a result, the broken hardware capabilities for SDR104 and SDR50 modes
>> can be removed from the device tree.
>> Additionally, due to level shifter constraints, set the maximum
>> frequency for High Speed (HS) mode to 37.5 MHz using the
>> max-sd-hs-frequency property for sm8550.
>>
>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8550.dtsi | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> index 82cabf777cd2..2c770c979d39 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>> @@ -3180,6 +3180,7 @@ sdhc_2: mmc@8804000 {
>>   			iommus = <&apps_smmu 0x540 0>;
>>   			qcom,dll-config = <0x0007642c>;
>>   			qcom,ddr-config = <0x80040868>;
>> +			max-sd-hs-frequency = <37500000>;
> So my previous comments stay... This is SoC thus deducible from compatible.
> 
> Best regards,
> Krzysztof

" I agree that a DT property for the mmc controller would make sense.

Although, this seems limited to SD UHS-I speed modes, so perhaps
"max-sd-uhs-frequency" would be a better name for it?

Kind regards
Uffe "

https://patchwork.kernel.org/project/linux-mmc/cover/20250523105745.6210-1-quic_sartgarg@quicinc.com/

This was the comment given on V2 to introduce a generic dt
property.

Best regards,
Sarthak

Re: [PATCH V3 4/4] arm64: dts: qcom: sm8550: Remove SDR104/SDR50 broken capabilities

[Krzysztof Kozlowski]

On 18/06/2025 10:44, Sarthak Garg wrote:
> 
> 
> On 6/18/2025 1:11 PM, Krzysztof Kozlowski wrote:
>> On 18/06/2025 09:28, Sarthak Garg wrote:
>>> Kernel now handles all level shifter limitations related to SD card
>>> modes.
>>> As a result, the broken hardware capabilities for SDR104 and SDR50 modes
>>> can be removed from the device tree.
>>> Additionally, due to level shifter constraints, set the maximum
>>> frequency for High Speed (HS) mode to 37.5 MHz using the
>>> max-sd-hs-frequency property for sm8550.
>>>
>>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/sm8550.dtsi | 4 +---
>>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>>> index 82cabf777cd2..2c770c979d39 100644
>>> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
>>> @@ -3180,6 +3180,7 @@ sdhc_2: mmc@8804000 {
>>>   			iommus = <&apps_smmu 0x540 0>;
>>>   			qcom,dll-config = <0x0007642c>;
>>>   			qcom,ddr-config = <0x80040868>;
>>> +			max-sd-hs-frequency = <37500000>;
>> So my previous comments stay... This is SoC thus deducible from compatible.
>>
>> Best regards,
>> Krzysztof
> 
> " I agree that a DT property for the mmc controller would make sense.
> 
> Although, this seems limited to SD UHS-I speed modes, so perhaps
> "max-sd-uhs-frequency" would be a better name for it?
> 
> Kind regards
> Uffe "
> 
> https://patchwork.kernel.org/project/linux-mmc/cover/20250523105745.6210-1-quic_sartgarg@quicinc.com/
> 
> This was the comment given on V2 to introduce a generic dt
> property.


I know, it does not matter. If this is here, it is a 100% proof this is
SoC specific, thus you have compatible for that.

Best regards,
Krzysztof

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Krzysztof Kozlowski]

On 18/06/2025 10:38, Sarthak Garg wrote:
> 
> 
> On 6/18/2025 1:13 PM, Krzysztof Kozlowski wrote:
>> On 18/06/2025 09:28, Sarthak Garg wrote:
>>> Introduce a new optional device tree property `max-sd-hs-frequency` to
>>> limit the maximum frequency (in Hz) used for SD cards operating in
>>> High-Speed (HS) mode.
>>>
>>> This property is useful for platforms with vendor-specific hardware
>>> constraints, such as the presence of a level shifter that cannot
>>> reliably support the default 50 MHz HS frequency. It allows the host
>>> driver to cap the HS mode frequency accordingly.
>>>
>>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/mmc/mmc-controller-common.yaml | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>> index 9a7235439759..1976f5f8c401 100644
>>> --- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>> @@ -93,6 +93,16 @@ properties:
>>>       minimum: 400000
>>>       maximum: 384000000
>>>   
>>> +  max-sd-hs-frequency:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: |
>>> +      Maximum frequency (in Hz) to be used for SD cards operating in
>>> +      High-Speed (HS) mode. This is useful for platforms with vendor-specific
>>> +      limitations, such as the presence of a level shifter that cannot support
>>> +      the default 50 MHz HS frequency or other.
>>> +    minimum: 400000
>>> +    maximum: 50000000
>>
>> This might be fine, but your DTS suggests clearly this is SoC compatible
>> deducible, which I already said at v1.
>>
>> So now you send v3 which is the same as v1, so you get the same comments.
>>
>> Best regards,
>> Krzysztof
> 
> Introducing this flag no longer becomes SoC compatible because as per 
> discussions in V2 patchset with Ulf and Konrad this new property can be 
> used by any vendor who wants to limit the HS mode frequency due to any 
> reason. Thats why moved to this generic approach again in V3 as compared 
> to compatible based approach in V2.

The are no arguments provided in favor, so my review from v1 stays. You
get the same comments.

Best regards,
Krzysztof

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Sarthak Garg]

On 6/18/2025 2:38 PM, Krzysztof Kozlowski wrote:
> On 18/06/2025 10:38, Sarthak Garg wrote:
>>
>>
>> On 6/18/2025 1:13 PM, Krzysztof Kozlowski wrote:
>>> On 18/06/2025 09:28, Sarthak Garg wrote:
>>>> Introduce a new optional device tree property `max-sd-hs-frequency` to
>>>> limit the maximum frequency (in Hz) used for SD cards operating in
>>>> High-Speed (HS) mode.
>>>>
>>>> This property is useful for platforms with vendor-specific hardware
>>>> constraints, such as the presence of a level shifter that cannot
>>>> reliably support the default 50 MHz HS frequency. It allows the host
>>>> driver to cap the HS mode frequency accordingly.
>>>>
>>>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>>>> ---
>>>>    .../devicetree/bindings/mmc/mmc-controller-common.yaml | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>>> index 9a7235439759..1976f5f8c401 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>>> @@ -93,6 +93,16 @@ properties:
>>>>        minimum: 400000
>>>>        maximum: 384000000
>>>>    
>>>> +  max-sd-hs-frequency:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description: |
>>>> +      Maximum frequency (in Hz) to be used for SD cards operating in
>>>> +      High-Speed (HS) mode. This is useful for platforms with vendor-specific
>>>> +      limitations, such as the presence of a level shifter that cannot support
>>>> +      the default 50 MHz HS frequency or other.
>>>> +    minimum: 400000
>>>> +    maximum: 50000000
>>>
>>> This might be fine, but your DTS suggests clearly this is SoC compatible
>>> deducible, which I already said at v1.
>>>
>>> So now you send v3 which is the same as v1, so you get the same comments.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Introducing this flag no longer becomes SoC compatible because as per
>> discussions in V2 patchset with Ulf and Konrad this new property can be
>> used by any vendor who wants to limit the HS mode frequency due to any
>> reason. Thats why moved to this generic approach again in V3 as compared
>> to compatible based approach in V2.
> 
> The are no arguments provided in favor, so my review from v1 stays. You
> get the same comments.
> 
> Best regards,
> Krzysztof

@Ulf, @Konrad — since you previously supported the idea of a generic 
property for HS frequency limitation, could you please share your 
thoughts on whether this still seems like a valid approach?

Best regards,
Sarthak

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Konrad Dybcio]

On 6/18/25 9:43 AM, Krzysztof Kozlowski wrote:
> On 18/06/2025 09:28, Sarthak Garg wrote:
>> Introduce a new optional device tree property `max-sd-hs-frequency` to
>> limit the maximum frequency (in Hz) used for SD cards operating in
>> High-Speed (HS) mode.
>>
>> This property is useful for platforms with vendor-specific hardware
>> constraints, such as the presence of a level shifter that cannot
>> reliably support the default 50 MHz HS frequency. It allows the host
>> driver to cap the HS mode frequency accordingly.
>>
>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>> ---
>>  .../devicetree/bindings/mmc/mmc-controller-common.yaml | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>> index 9a7235439759..1976f5f8c401 100644
>> --- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>> @@ -93,6 +93,16 @@ properties:
>>      minimum: 400000
>>      maximum: 384000000
>>  
>> +  max-sd-hs-frequency:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: |
>> +      Maximum frequency (in Hz) to be used for SD cards operating in
>> +      High-Speed (HS) mode. This is useful for platforms with vendor-specific
>> +      limitations, such as the presence of a level shifter that cannot support
>> +      the default 50 MHz HS frequency or other.
>> +    minimum: 400000
>> +    maximum: 50000000
> 
> This might be fine, but your DTS suggests clearly this is SoC compatible
> deducible, which I already said at v1.

I don't understand why you're rejecting a common solution to a problem
that surely exists outside this one specific chip from one specific
vendor, which may be caused by a multitude of design choices, including
erratic board (not SoC) electrical design

Konrad

Re: [PATCH V3 4/4] arm64: dts: qcom: sm8550: Remove SDR104/SDR50 broken capabilities

[Konrad Dybcio]

On 6/18/25 9:28 AM, Sarthak Garg wrote:
> Kernel now handles all level shifter limitations related to SD card
> modes.
> As a result, the broken hardware capabilities for SDR104 and SDR50 modes
> can be removed from the device tree.
> Additionally, due to level shifter constraints, set the maximum
> frequency for High Speed (HS) mode to 37.5 MHz using the
> max-sd-hs-frequency property for sm8550.

It's a little bit
hard to read text that is formatted
like that, please stick to ~72 chars per line instead

Konrad

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Krzysztof Kozlowski]

On 21/06/2025 12:20, Konrad Dybcio wrote:
> On 6/18/25 9:43 AM, Krzysztof Kozlowski wrote:
>> On 18/06/2025 09:28, Sarthak Garg wrote:
>>> Introduce a new optional device tree property `max-sd-hs-frequency` to
>>> limit the maximum frequency (in Hz) used for SD cards operating in
>>> High-Speed (HS) mode.
>>>
>>> This property is useful for platforms with vendor-specific hardware
>>> constraints, such as the presence of a level shifter that cannot
>>> reliably support the default 50 MHz HS frequency. It allows the host
>>> driver to cap the HS mode frequency accordingly.
>>>
>>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>>> ---
>>>  .../devicetree/bindings/mmc/mmc-controller-common.yaml | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>> index 9a7235439759..1976f5f8c401 100644
>>> --- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>> @@ -93,6 +93,16 @@ properties:
>>>      minimum: 400000
>>>      maximum: 384000000
>>>  
>>> +  max-sd-hs-frequency:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description: |
>>> +      Maximum frequency (in Hz) to be used for SD cards operating in
>>> +      High-Speed (HS) mode. This is useful for platforms with vendor-specific
>>> +      limitations, such as the presence of a level shifter that cannot support
>>> +      the default 50 MHz HS frequency or other.
>>> +    minimum: 400000
>>> +    maximum: 50000000
>>
>> This might be fine, but your DTS suggests clearly this is SoC compatible
>> deducible, which I already said at v1.
> 
> I don't understand why you're rejecting a common solution to a problem
> that surely exists outside this one specific chip from one specific
> vendor, which may be caused by a multitude of design choices, including
> erratic board (not SoC) electrical design

No one brought any arguments so far that common solution is needed. The
only argument provided - sm8550 - is showing this is soc design.

I don't reject common solution. I provided review at v1 to which no one
responded, no one argued, no one provided other arguments.

Best regards,
Krzysztof

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Konrad Dybcio]

On 6/22/25 11:48 AM, Krzysztof Kozlowski wrote:
> On 21/06/2025 12:20, Konrad Dybcio wrote:
>> On 6/18/25 9:43 AM, Krzysztof Kozlowski wrote:
>>> On 18/06/2025 09:28, Sarthak Garg wrote:
>>>> Introduce a new optional device tree property `max-sd-hs-frequency` to
>>>> limit the maximum frequency (in Hz) used for SD cards operating in
>>>> High-Speed (HS) mode.
>>>>
>>>> This property is useful for platforms with vendor-specific hardware
>>>> constraints, such as the presence of a level shifter that cannot
>>>> reliably support the default 50 MHz HS frequency. It allows the host
>>>> driver to cap the HS mode frequency accordingly.
>>>>
>>>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>>>> ---
>>>>  .../devicetree/bindings/mmc/mmc-controller-common.yaml | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>>> index 9a7235439759..1976f5f8c401 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>>> +++ b/Documentation/devicetree/bindings/mmc/mmc-controller-common.yaml
>>>> @@ -93,6 +93,16 @@ properties:
>>>>      minimum: 400000
>>>>      maximum: 384000000
>>>>  
>>>> +  max-sd-hs-frequency:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description: |
>>>> +      Maximum frequency (in Hz) to be used for SD cards operating in
>>>> +      High-Speed (HS) mode. This is useful for platforms with vendor-specific
>>>> +      limitations, such as the presence of a level shifter that cannot support
>>>> +      the default 50 MHz HS frequency or other.
>>>> +    minimum: 400000
>>>> +    maximum: 50000000
>>>
>>> This might be fine, but your DTS suggests clearly this is SoC compatible
>>> deducible, which I already said at v1.
>>
>> I don't understand why you're rejecting a common solution to a problem
>> that surely exists outside this one specific chip from one specific
>> vendor, which may be caused by a multitude of design choices, including
>> erratic board (not SoC) electrical design
> 
> No one brought any arguments so far that common solution is needed. The
> only argument provided - sm8550 - is showing this is soc design.
> 
> I don't reject common solution. I provided review at v1 to which no one
> responded, no one argued, no one provided other arguments.

Okay, so the specific problem that causes this observable limitation
exists on SM8550 and at least one more platform which is not upstream
today. It can be caused by various electrical issues, in our specific
case by something internal to the SoC (but external factors may apply
too)

Looking at the docs, a number of platforms have various limitations
with regards to frequency at specific speed-modes, some of which seem
to be handled implicitly by rounding in the clock framework's
round/set_rate().

I can very easily imagine there are either boards or platforms in the
wild, where the speed must be limited for various reasons, maybe some
of them currently don't advertise it (like sm8550 on next/master) to
hide that

Konrad

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Krzysztof Kozlowski]

On 23/06/2025 14:08, Konrad Dybcio wrote:
>>>>
>>>> This might be fine, but your DTS suggests clearly this is SoC compatible
>>>> deducible, which I already said at v1.
>>>
>>> I don't understand why you're rejecting a common solution to a problem
>>> that surely exists outside this one specific chip from one specific
>>> vendor, which may be caused by a multitude of design choices, including
>>> erratic board (not SoC) electrical design
>>
>> No one brought any arguments so far that common solution is needed. The
>> only argument provided - sm8550 - is showing this is soc design.
>>
>> I don't reject common solution. I provided review at v1 to which no one
>> responded, no one argued, no one provided other arguments.
> 
> Okay, so the specific problem that causes this observable limitation
> exists on SM8550 and at least one more platform which is not upstream
> today. It can be caused by various electrical issues, in our specific
> case by something internal to the SoC (but external factors may apply
> too)
> 
> Looking at the docs, a number of platforms have various limitations
> with regards to frequency at specific speed-modes, some of which seem
> to be handled implicitly by rounding in the clock framework's
> round/set_rate().
> 
> I can very easily imagine there are either boards or platforms in the
> wild, where the speed must be limited for various reasons, maybe some
> of them currently don't advertise it (like sm8550 on next/master) to
> hide that

But there are no such now. The only argument (fact) provided in this
patchset is: this is issue specific to SM8550 SoC, not the board. See
last patch. Therefore this is compatible-deducible and this makes
property without any upstream user.


Best regards,
Krzysztof

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Konrad Dybcio]

On 6/23/25 2:16 PM, Krzysztof Kozlowski wrote:
> On 23/06/2025 14:08, Konrad Dybcio wrote:
>>>>>
>>>>> This might be fine, but your DTS suggests clearly this is SoC compatible
>>>>> deducible, which I already said at v1.
>>>>
>>>> I don't understand why you're rejecting a common solution to a problem
>>>> that surely exists outside this one specific chip from one specific
>>>> vendor, which may be caused by a multitude of design choices, including
>>>> erratic board (not SoC) electrical design
>>>
>>> No one brought any arguments so far that common solution is needed. The
>>> only argument provided - sm8550 - is showing this is soc design.
>>>
>>> I don't reject common solution. I provided review at v1 to which no one
>>> responded, no one argued, no one provided other arguments.
>>
>> Okay, so the specific problem that causes this observable limitation
>> exists on SM8550 and at least one more platform which is not upstream
>> today. It can be caused by various electrical issues, in our specific
>> case by something internal to the SoC (but external factors may apply
>> too)
>>
>> Looking at the docs, a number of platforms have various limitations
>> with regards to frequency at specific speed-modes, some of which seem
>> to be handled implicitly by rounding in the clock framework's
>> round/set_rate().
>>
>> I can very easily imagine there are either boards or platforms in the
>> wild, where the speed must be limited for various reasons, maybe some
>> of them currently don't advertise it (like sm8550 on next/master) to
>> hide that
> 
> But there are no such now. The only argument (fact) provided in this
> patchset is: this is issue specific to SM8550 SoC, not the board. See
> last patch. Therefore this is compatible-deducible and this makes
> property without any upstream user.

When one appears, we will have to carry code to repeat what the property
does, based on a specific compatible.. And all OS implementations will
have to do the same, instead of parsing the explicit information

Konrad

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Krzysztof Kozlowski]

On 23/06/2025 14:31, Konrad Dybcio wrote:
> On 6/23/25 2:16 PM, Krzysztof Kozlowski wrote:
>> On 23/06/2025 14:08, Konrad Dybcio wrote:
>>>>>>
>>>>>> This might be fine, but your DTS suggests clearly this is SoC compatible
>>>>>> deducible, which I already said at v1.
>>>>>
>>>>> I don't understand why you're rejecting a common solution to a problem
>>>>> that surely exists outside this one specific chip from one specific
>>>>> vendor, which may be caused by a multitude of design choices, including
>>>>> erratic board (not SoC) electrical design
>>>>
>>>> No one brought any arguments so far that common solution is needed. The
>>>> only argument provided - sm8550 - is showing this is soc design.
>>>>
>>>> I don't reject common solution. I provided review at v1 to which no one
>>>> responded, no one argued, no one provided other arguments.
>>>
>>> Okay, so the specific problem that causes this observable limitation
>>> exists on SM8550 and at least one more platform which is not upstream
>>> today. It can be caused by various electrical issues, in our specific
>>> case by something internal to the SoC (but external factors may apply
>>> too)
>>>
>>> Looking at the docs, a number of platforms have various limitations
>>> with regards to frequency at specific speed-modes, some of which seem
>>> to be handled implicitly by rounding in the clock framework's
>>> round/set_rate().
>>>
>>> I can very easily imagine there are either boards or platforms in the
>>> wild, where the speed must be limited for various reasons, maybe some
>>> of them currently don't advertise it (like sm8550 on next/master) to
>>> hide that
>>
>> But there are no such now. The only argument (fact) provided in this
>> patchset is: this is issue specific to SM8550 SoC, not the board. See
>> last patch. Therefore this is compatible-deducible and this makes
>> property without any upstream user.
> 
> When one appears, we will have to carry code to repeat what the property
> does, based on a specific compatible.. And all OS implementations will
> have to do the same, instead of parsing the explicit information

Adding new property in such case will be trivial and simple, unlike
having to maintain unused ABI.

And it will be unused, because last patch DTS should be rejected on that
basis: adding redundant properties which are already defined by the
compatible.

Best regards,
Krzysztof

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Konrad Dybcio]

On 24-Jun-25 08:06, Krzysztof Kozlowski wrote:
> On 23/06/2025 14:31, Konrad Dybcio wrote:
>> On 6/23/25 2:16 PM, Krzysztof Kozlowski wrote:
>>> On 23/06/2025 14:08, Konrad Dybcio wrote:
>>>>>>>
>>>>>>> This might be fine, but your DTS suggests clearly this is SoC compatible
>>>>>>> deducible, which I already said at v1.
>>>>>>
>>>>>> I don't understand why you're rejecting a common solution to a problem
>>>>>> that surely exists outside this one specific chip from one specific
>>>>>> vendor, which may be caused by a multitude of design choices, including
>>>>>> erratic board (not SoC) electrical design
>>>>>
>>>>> No one brought any arguments so far that common solution is needed. The
>>>>> only argument provided - sm8550 - is showing this is soc design.
>>>>>
>>>>> I don't reject common solution. I provided review at v1 to which no one
>>>>> responded, no one argued, no one provided other arguments.
>>>>
>>>> Okay, so the specific problem that causes this observable limitation
>>>> exists on SM8550 and at least one more platform which is not upstream
>>>> today. It can be caused by various electrical issues, in our specific
>>>> case by something internal to the SoC (but external factors may apply
>>>> too)
>>>>
>>>> Looking at the docs, a number of platforms have various limitations
>>>> with regards to frequency at specific speed-modes, some of which seem
>>>> to be handled implicitly by rounding in the clock framework's
>>>> round/set_rate().
>>>>
>>>> I can very easily imagine there are either boards or platforms in the
>>>> wild, where the speed must be limited for various reasons, maybe some
>>>> of them currently don't advertise it (like sm8550 on next/master) to
>>>> hide that
>>>
>>> But there are no such now. The only argument (fact) provided in this
>>> patchset is: this is issue specific to SM8550 SoC, not the board. See
>>> last patch. Therefore this is compatible-deducible and this makes
>>> property without any upstream user.
>>
>> When one appears, we will have to carry code to repeat what the property
>> does, based on a specific compatible.. And all OS implementations will
>> have to do the same, instead of parsing the explicit information
> 
> Adding new property in such case will be trivial and simple, unlike
> having to maintain unused ABI.
> 
> And it will be unused, because last patch DTS should be rejected on that
> basis: adding redundant properties which are already defined by the
> compatible.

Got some more fresh information.. This apparently *does* vary across
boards, as there is a recommended hardware workaround to this rate
limitation (requiring an external clock source, which is up to the
OEM to implement or not)

Konrad

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Krzysztof Kozlowski]

On 01/07/2025 11:04, Konrad Dybcio wrote:
>>>>>
>>>>> Looking at the docs, a number of platforms have various limitations
>>>>> with regards to frequency at specific speed-modes, some of which seem
>>>>> to be handled implicitly by rounding in the clock framework's
>>>>> round/set_rate().
>>>>>
>>>>> I can very easily imagine there are either boards or platforms in the
>>>>> wild, where the speed must be limited for various reasons, maybe some
>>>>> of them currently don't advertise it (like sm8550 on next/master) to
>>>>> hide that
>>>>
>>>> But there are no such now. The only argument (fact) provided in this
>>>> patchset is: this is issue specific to SM8550 SoC, not the board. See
>>>> last patch. Therefore this is compatible-deducible and this makes
>>>> property without any upstream user.
>>>
>>> When one appears, we will have to carry code to repeat what the property
>>> does, based on a specific compatible.. And all OS implementations will
>>> have to do the same, instead of parsing the explicit information
>>
>> Adding new property in such case will be trivial and simple, unlike
>> having to maintain unused ABI.
>>
>> And it will be unused, because last patch DTS should be rejected on that
>> basis: adding redundant properties which are already defined by the
>> compatible.
> 
> Got some more fresh information.. This apparently *does* vary across
> boards, as there is a recommended hardware workaround to this rate
> limitation (requiring an external clock source, which is up to the
> OEM to implement or not)


This should be clearly explained in commit msg and the DTS patch
re-written because it seems it is not a property of the SoC.

I mean, really, that last patch here makes entire discussion pointless,
because till it is in the patchset is a proof this is a SoC level property.

Best regards,
Krzysztof

Re: [PATCH V3 2/4] dt-bindings: mmc: controller: Add max-sd-hs-frequency property

[Sarthak Garg]

On 7/1/2025 3:00 PM, Krzysztof Kozlowski wrote:
> On 01/07/2025 11:04, Konrad Dybcio wrote:
>>>>>>
>>>>>> Looking at the docs, a number of platforms have various limitations
>>>>>> with regards to frequency at specific speed-modes, some of which seem
>>>>>> to be handled implicitly by rounding in the clock framework's
>>>>>> round/set_rate().
>>>>>>
>>>>>> I can very easily imagine there are either boards or platforms in the
>>>>>> wild, where the speed must be limited for various reasons, maybe some
>>>>>> of them currently don't advertise it (like sm8550 on next/master) to
>>>>>> hide that
>>>>>
>>>>> But there are no such now. The only argument (fact) provided in this
>>>>> patchset is: this is issue specific to SM8550 SoC, not the board. See
>>>>> last patch. Therefore this is compatible-deducible and this makes
>>>>> property without any upstream user.
>>>>
>>>> When one appears, we will have to carry code to repeat what the property
>>>> does, based on a specific compatible.. And all OS implementations will
>>>> have to do the same, instead of parsing the explicit information
>>>
>>> Adding new property in such case will be trivial and simple, unlike
>>> having to maintain unused ABI.
>>>
>>> And it will be unused, because last patch DTS should be rejected on that
>>> basis: adding redundant properties which are already defined by the
>>> compatible.
>>
>> Got some more fresh information.. This apparently *does* vary across
>> boards, as there is a recommended hardware workaround to this rate
>> limitation (requiring an external clock source, which is up to the
>> OEM to implement or not)
> 
> 
> This should be clearly explained in commit msg and the DTS patch
> re-written because it seems it is not a property of the SoC.
> 
> I mean, really, that last patch here makes entire discussion pointless,
> because till it is in the patchset is a proof this is a SoC level property.
> 
> Best regards,
> Krzysztof


Sure I'll update the commit message clearly mentioning it as board 
specific and update the DTS patch and have these changes in board 
specific dts files (for e.g sm8550-mtp.dts).

As rightly stated above this configuration will vary across boards as well.

Regards,
Sarthak