[PATCH V1 0/3] Add level shifter support for qualcomm SOC's

[PATCH V1 0/3] Add level shifter support for qualcomm SOC's

[Sarthak Garg]

Add level shifter support for qualcomm SOC's.

Sarthak Garg (3):
  dt-bindings: mmc: qcom: Document level shifter flag for SD card
  mmc: sdhci-msm: Enable tuning for SDR50 mode for SD card
  mmc: sdhci-msm: Limit HS mode frequency to 37.5MHz

 .../devicetree/bindings/mmc/sdhci-msm.yaml    |  3 +++
 drivers/mmc/host/sdhci-msm.c                  | 26 +++++++++++++++++++
 2 files changed, 29 insertions(+)

-- 
2.17.1

[PATCH V1 1/3] dt-bindings: mmc: qcom: Document level shifter flag for SD card

[Sarthak Garg]

Introduce a flag to indicate if the Qualcomm platform has a level
shifter for SD cards. With level shifter addition some extra delay is
seen on RX data path leading to CRC errors. To compensate these delays
and avoid CRC errors below things needs to be done:

1) Enable tuning for SDR50 mode
2) Limit HS mode frequency to 37.5MHz from 50MHz

Add this flag for all targets with a level shifter to handle these
issues for SD card.

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

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
index b32253c60919..14f1ec9ae5bd 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
@@ -131,6 +131,9 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: platform specific settings for DLL_CONFIG reg.
 
+  qcom,use-level-shifter:
+    description: Flag to indicate if platform has level shifter for SD card.
+
   iommus:
     minItems: 1
     maxItems: 8
-- 
2.17.1

[PATCH V1 2/3] 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.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index e00208535bd1..16325c21de52 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)
 
@@ -1124,6 +1125,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.
@@ -1192,6 +1197,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;
@@ -1208,6 +1215,15 @@ 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_EN;
+		config &= ~CORE_HC_SELECT_IN_MASK;
+		config |= 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.17.1

[PATCH V1 3/3] mmc: sdhci-msm: Limit HS mode frequency to 37.5MHz

[Sarthak Garg]

For Qualcomm SoCs with level shifter delays are seen on receivers data
path due to latency added by level shifter.

To bring these delays in normal range and avoid CMD CRC errors
reduce frequency for HS mode SD cards to 37.5MHz for targets which has
level shifter.

Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 16325c21de52..5e1dc06c4707 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -138,6 +138,8 @@
 /* Max load for eMMC Vdd-io supply */
 #define MMC_VQMMC_MAX_LOAD_UA	325000
 
+#define LEVEL_SHIFTER_HIGH_SPEED_FREQ	37500000
+
 #define msm_host_readl(msm_host, host, offset) \
 	msm_host->var_ops->msm_readl_relaxed(host, offset)
 
@@ -287,6 +289,7 @@ struct sdhci_msm_host {
 	bool use_cdr;
 	u32 transfer_mode;
 	bool updated_ddr_cfg;
+	bool uses_level_shifter;
 	bool uses_tassadar_dll;
 	u32 dll_config;
 	u32 ddr_config;
@@ -366,6 +369,11 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
 
 	mult = msm_get_clock_mult_for_bus_mode(host);
 	desired_rate = clock * mult;
+
+	if (curr_ios.timing == MMC_TIMING_SD_HS && desired_rate == 50000000
+		&& msm_host->uses_level_shifter)
+		desired_rate = LEVEL_SHIFTER_HIGH_SPEED_FREQ;
+
 	rc = dev_pm_opp_set_rate(mmc_dev(host->mmc), desired_rate);
 	if (rc) {
 		pr_err("%s: Failed to set clock at rate %u at timing %d\n",
@@ -2372,6 +2380,8 @@ static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
 
 	of_property_read_u32(node, "qcom,dll-config", &msm_host->dll_config);
 
+	msm_host->uses_level_shifter = of_property_read_bool(node, "qcom,use-level-shifter");
+
 	if (of_device_is_compatible(node, "qcom,msm8916-sdhci"))
 		host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
 }
-- 
2.17.1

Re: [PATCH V1 1/3] dt-bindings: mmc: qcom: Document level shifter flag for SD card

[Rob Herring (Arm)]

On Thu, 07 Nov 2024 13:35:03 +0530, Sarthak Garg wrote:
> Introduce a flag to indicate if the Qualcomm platform has a level
> shifter for SD cards. With level shifter addition some extra delay is
> seen on RX data path leading to CRC errors. To compensate these delays
> and avoid CRC errors below things needs to be done:
> 
> 1) Enable tuning for SDR50 mode
> 2) Limit HS mode frequency to 37.5MHz from 50MHz
> 
> Add this flag for all targets with a level shifter to handle these
> issues for SD card.
> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml: qcom,use-level-shifter: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241107080505.29244-2-quic_sartgarg@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH V1 1/3] dt-bindings: mmc: qcom: Document level shifter flag for SD card

[Krzysztof Kozlowski]

On Thu, Nov 07, 2024 at 01:35:03PM +0530, Sarthak Garg wrote:
> Introduce a flag to indicate if the Qualcomm platform has a level
> shifter for SD cards. With level shifter addition some extra delay is
> seen on RX data path leading to CRC errors. To compensate these delays
> and avoid CRC errors below things needs to be done:
> 
> 1) Enable tuning for SDR50 mode
> 2) Limit HS mode frequency to 37.5MHz from 50MHz
> 
> Add this flag for all targets with a level shifter to handle these
> issues for SD card.
> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 

This wasn't tested, so just short review - platform means SoC usually,
so this looks SoC specific, thus implied by compatible.

Best regards,
Krzysztof

Re: [PATCH V1 2/3] mmc: sdhci-msm: Enable tuning for SDR50 mode for SD card

[Adrian Hunter]

On 7/11/24 10:05, Sarthak Garg wrote:
> 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.
> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  drivers/mmc/host/sdhci-msm.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index e00208535bd1..16325c21de52 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)
>  
> @@ -1124,6 +1125,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)

Please do line up code as suggested by checkpatch:

CHECK: Alignment should match open parenthesis
#35: FILE: drivers/mmc/host/sdhci-msm.c:1129:
+       if (ios->timing == MMC_TIMING_UHS_SDR50 &&
+                       host->flags & SDHCI_SDR50_NEEDS_TUNING)

CHECK: Alignment should match open parenthesis
#55: FILE: drivers/mmc/host/sdhci-msm.c:1219:
+       if (ios.timing == MMC_TIMING_UHS_SDR50 &&
+                       host->flags & SDHCI_SDR50_NEEDS_TUNING) {

total: 0 errors, 0 warnings, 2 checks, 40 lines checked


> +		return true;
> +
>  	/*
>  	 * Tuning is required for SDR104, HS200 and HS400 cards and
>  	 * if clock frequency is greater than 100MHz in these modes.
> @@ -1192,6 +1197,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;
> @@ -1208,6 +1215,15 @@ 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) {

Ditto alignment

> +		config = readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec);
> +		config |= CORE_HC_SELECT_IN_EN;
> +		config &= ~CORE_HC_SELECT_IN_MASK;
> +		config |= CORE_HC_SELECT_IN_SDR50;

Perhaps clear bits first, then set bits e.g.

		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

Re: [PATCH V1 3/3] mmc: sdhci-msm: Limit HS mode frequency to 37.5MHz

[Adrian Hunter]

On 7/11/24 10:05, Sarthak Garg wrote:
> For Qualcomm SoCs with level shifter delays are seen on receivers data
> path due to latency added by level shifter.
> 
> To bring these delays in normal range and avoid CMD CRC errors
> reduce frequency for HS mode SD cards to 37.5MHz for targets which has
> level shifter.
> 
> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
> ---
>  drivers/mmc/host/sdhci-msm.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 16325c21de52..5e1dc06c4707 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -138,6 +138,8 @@
>  /* Max load for eMMC Vdd-io supply */
>  #define MMC_VQMMC_MAX_LOAD_UA	325000
>  
> +#define LEVEL_SHIFTER_HIGH_SPEED_FREQ	37500000
> +
>  #define msm_host_readl(msm_host, host, offset) \
>  	msm_host->var_ops->msm_readl_relaxed(host, offset)
>  
> @@ -287,6 +289,7 @@ struct sdhci_msm_host {
>  	bool use_cdr;
>  	u32 transfer_mode;
>  	bool updated_ddr_cfg;
> +	bool uses_level_shifter;
>  	bool uses_tassadar_dll;
>  	u32 dll_config;
>  	u32 ddr_config;
> @@ -366,6 +369,11 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
>  
>  	mult = msm_get_clock_mult_for_bus_mode(host);
>  	desired_rate = clock * mult;
> +
> +	if (curr_ios.timing == MMC_TIMING_SD_HS && desired_rate == 50000000

Wouldn't desired_rate > LEVEL_SHIFTER_HIGH_SPEED_FREQ make more sense?

> +		&& msm_host->uses_level_shifter)
> +		desired_rate = LEVEL_SHIFTER_HIGH_SPEED_FREQ;

As checkpatch says:

CHECK: Logical continuations should be on the previous line
#46: FILE: drivers/mmc/host/sdhci-msm.c:374:
+       if (curr_ios.timing == MMC_TIMING_SD_HS && desired_rate == 50000000
+               && msm_host->uses_level_shifter)

total: 0 errors, 0 warnings, 1 checks, 34 lines checked


> +
>  	rc = dev_pm_opp_set_rate(mmc_dev(host->mmc), desired_rate);
>  	if (rc) {
>  		pr_err("%s: Failed to set clock at rate %u at timing %d\n",
> @@ -2372,6 +2380,8 @@ static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
>  
>  	of_property_read_u32(node, "qcom,dll-config", &msm_host->dll_config);
>  
> +	msm_host->uses_level_shifter = of_property_read_bool(node, "qcom,use-level-shifter");
> +
>  	if (of_device_is_compatible(node, "qcom,msm8916-sdhci"))
>  		host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
>  }

Re: [PATCH V1 1/3] dt-bindings: mmc: qcom: Document level shifter flag for SD card

[Sarthak Garg]

On 11/7/2024 3:29 PM, Krzysztof Kozlowski wrote:
> On Thu, Nov 07, 2024 at 01:35:03PM +0530, Sarthak Garg wrote:
>> Introduce a flag to indicate if the Qualcomm platform has a level
>> shifter for SD cards. With level shifter addition some extra delay is
>> seen on RX data path leading to CRC errors. To compensate these delays
>> and avoid CRC errors below things needs to be done:
>>
>> 1) Enable tuning for SDR50 mode
>> 2) Limit HS mode frequency to 37.5MHz from 50MHz
>>
>> Add this flag for all targets with a level shifter to handle these
>> issues for SD card.
>>
>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 3 +++
>>   1 file changed, 3 insertions(+)
>>
> 
> This wasn't tested, so just short review - platform means SoC usually,
> so this looks SoC specific, thus implied by compatible.
>  > Best regards,
> Krzysztof
>

Sure will redesign this logic and use compatible in patch V2.

Re: [PATCH V1 2/3] mmc: sdhci-msm: Enable tuning for SDR50 mode for SD card

[Sarthak Garg]

On 11/11/2024 2:21 PM, Adrian Hunter wrote:
> On 7/11/24 10:05, Sarthak Garg wrote:
>> 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.
>>
>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index e00208535bd1..16325c21de52 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)
>>   
>> @@ -1124,6 +1125,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)
> 
> Please do line up code as suggested by checkpatch:
> 
> CHECK: Alignment should match open parenthesis
> #35: FILE: drivers/mmc/host/sdhci-msm.c:1129:
> +       if (ios->timing == MMC_TIMING_UHS_SDR50 &&
> +                       host->flags & SDHCI_SDR50_NEEDS_TUNING)
> 
> CHECK: Alignment should match open parenthesis
> #55: FILE: drivers/mmc/host/sdhci-msm.c:1219:
> +       if (ios.timing == MMC_TIMING_UHS_SDR50 &&
> +                       host->flags & SDHCI_SDR50_NEEDS_TUNING) {
> 
> total: 0 errors, 0 warnings, 2 checks, 40 lines checked
> 
> 

Sure will update in V2.

>> +		return true;
>> +
>>   	/*
>>   	 * Tuning is required for SDR104, HS200 and HS400 cards and
>>   	 * if clock frequency is greater than 100MHz in these modes.
>> @@ -1192,6 +1197,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;
>> @@ -1208,6 +1215,15 @@ 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) {
> 
> Ditto alignment
> 

Sure will update in V2.

>> +		config = readl_relaxed(host->ioaddr + msm_offset->core_vendor_spec);
>> +		config |= CORE_HC_SELECT_IN_EN;
>> +		config &= ~CORE_HC_SELECT_IN_MASK;
>> +		config |= CORE_HC_SELECT_IN_SDR50;
> 
> Perhaps clear bits first, then set bits e.g.
> 
> 		config &= ~CORE_HC_SELECT_IN_MASK;
> 		config |= CORE_HC_SELECT_IN_EN | CORE_HC_SELECT_IN_SDR50;
> 

Sure will update in V2.

>> +		writel_relaxed(config, host->ioaddr + msm_offset->core_vendor_spec);
>> +	}
>> +
>>   	/*
>>   	 * For HS400 tuning in HS200 timing requires:
>>   	 * - select MCLK/2 in VENDOR_SPEC
> 

Re: [PATCH V1 3/3] mmc: sdhci-msm: Limit HS mode frequency to 37.5MHz

[Sarthak Garg]

On 11/11/2024 2:28 PM, Adrian Hunter wrote:
> On 7/11/24 10:05, Sarthak Garg wrote:
>> For Qualcomm SoCs with level shifter delays are seen on receivers data
>> path due to latency added by level shifter.
>>
>> To bring these delays in normal range and avoid CMD CRC errors
>> reduce frequency for HS mode SD cards to 37.5MHz for targets which has
>> level shifter.
>>
>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>> ---
>>   drivers/mmc/host/sdhci-msm.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 16325c21de52..5e1dc06c4707 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -138,6 +138,8 @@
>>   /* Max load for eMMC Vdd-io supply */
>>   #define MMC_VQMMC_MAX_LOAD_UA	325000
>>   
>> +#define LEVEL_SHIFTER_HIGH_SPEED_FREQ	37500000
>> +
>>   #define msm_host_readl(msm_host, host, offset) \
>>   	msm_host->var_ops->msm_readl_relaxed(host, offset)
>>   
>> @@ -287,6 +289,7 @@ struct sdhci_msm_host {
>>   	bool use_cdr;
>>   	u32 transfer_mode;
>>   	bool updated_ddr_cfg;
>> +	bool uses_level_shifter;
>>   	bool uses_tassadar_dll;
>>   	u32 dll_config;
>>   	u32 ddr_config;
>> @@ -366,6 +369,11 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
>>   
>>   	mult = msm_get_clock_mult_for_bus_mode(host);
>>   	desired_rate = clock * mult;
>> +
>> +	if (curr_ios.timing == MMC_TIMING_SD_HS && desired_rate == 50000000
> 
> Wouldn't desired_rate > LEVEL_SHIFTER_HIGH_SPEED_FREQ make more sense?
> 

Sure will update in V2.

>> +		&& msm_host->uses_level_shifter)
>> +		desired_rate = LEVEL_SHIFTER_HIGH_SPEED_FREQ;
> 
> As checkpatch says:
> 
> CHECK: Logical continuations should be on the previous line
> #46: FILE: drivers/mmc/host/sdhci-msm.c:374:
> +       if (curr_ios.timing == MMC_TIMING_SD_HS && desired_rate == 50000000
> +               && msm_host->uses_level_shifter)
> 
> total: 0 errors, 0 warnings, 1 checks, 34 lines checked
> 
> 

Sure will fix this in V2.

>> +
>>   	rc = dev_pm_opp_set_rate(mmc_dev(host->mmc), desired_rate);
>>   	if (rc) {
>>   		pr_err("%s: Failed to set clock at rate %u at timing %d\n",
>> @@ -2372,6 +2380,8 @@ static inline void sdhci_msm_get_of_property(struct platform_device *pdev,
>>   
>>   	of_property_read_u32(node, "qcom,dll-config", &msm_host->dll_config);
>>   
>> +	msm_host->uses_level_shifter = of_property_read_bool(node, "qcom,use-level-shifter");
>> +
>>   	if (of_device_is_compatible(node, "qcom,msm8916-sdhci"))
>>   		host->quirks2 |= SDHCI_QUIRK2_BROKEN_64_BIT_DMA;
>>   }
> 

Re: [PATCH V1 1/3] dt-bindings: mmc: qcom: Document level shifter flag for SD card

[Krzysztof Kozlowski]

On 20/05/2025 08:58, Sarthak Garg wrote:
> 
> 
> On 11/7/2024 3:29 PM, Krzysztof Kozlowski wrote:
>> On Thu, Nov 07, 2024 at 01:35:03PM +0530, Sarthak Garg wrote:
>>> Introduce a flag to indicate if the Qualcomm platform has a level
>>> shifter for SD cards. With level shifter addition some extra delay is
>>> seen on RX data path leading to CRC errors. To compensate these delays
>>> and avoid CRC errors below things needs to be done:
>>>
>>> 1) Enable tuning for SDR50 mode
>>> 2) Limit HS mode frequency to 37.5MHz from 50MHz
>>>
>>> Add this flag for all targets with a level shifter to handle these
>>> issues for SD card.
>>>
>>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>>> ---
>>>   Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>
>> This wasn't tested, so just short review - platform means SoC usually,
>> so this looks SoC specific, thus implied by compatible.
>>  > Best regards,
>> Krzysztof
>>
> 
> Sure will redesign this logic and use compatible in patch V2.

Hi, I hope you are well and that was just some mishap, but I cannot help
but notice that you received review within two hours after posting
patch, but now you responded to my review after 6 months.

Sometimes I really consider reviewing at the end of 2 weeks - the usual
maximum time frame.

Best regards,
Krzysztof

Re: [PATCH V1 1/3] dt-bindings: mmc: qcom: Document level shifter flag for SD card

[Sarthak Garg]

On 5/20/2025 12:48 PM, Krzysztof Kozlowski wrote:
> On 20/05/2025 08:58, Sarthak Garg wrote:
>>
>>
>> On 11/7/2024 3:29 PM, Krzysztof Kozlowski wrote:
>>> On Thu, Nov 07, 2024 at 01:35:03PM +0530, Sarthak Garg wrote:
>>>> Introduce a flag to indicate if the Qualcomm platform has a level
>>>> shifter for SD cards. With level shifter addition some extra delay is
>>>> seen on RX data path leading to CRC errors. To compensate these delays
>>>> and avoid CRC errors below things needs to be done:
>>>>
>>>> 1) Enable tuning for SDR50 mode
>>>> 2) Limit HS mode frequency to 37.5MHz from 50MHz
>>>>
>>>> Add this flag for all targets with a level shifter to handle these
>>>> issues for SD card.
>>>>
>>>> Signed-off-by: Sarthak Garg <quic_sartgarg@quicinc.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>
>>> This wasn't tested, so just short review - platform means SoC usually,
>>> so this looks SoC specific, thus implied by compatible.
>>>   > Best regards,
>>> Krzysztof
>>>
>>
>> Sure will redesign this logic and use compatible in patch V2.
> 
> Hi, I hope you are well and that was just some mishap, but I cannot help
> but notice that you received review within two hours after posting
> patch, but now you responded to my review after 6 months.
> 
> Sometimes I really consider reviewing at the end of 2 weeks - the usual
> maximum time frame.
> 
> Best regards,
> Krzysztof

Sorry I was on a break.
My apologies that I couldn't give a heads up for this in advance.
I have started this activity again and will be actively working now.