[PATCH v6 0/5] mmc: sdhci-msm: Rectify DLL programming sequence

[PATCH v6 0/5] mmc: sdhci-msm: Rectify DLL programming sequence

[Ram Prakash Gupta]

With the current DLL sequence stability issues are seen in
HS400 and HS200 mode for data transfers.

Rectify the DLL programming sequence as per latest hardware
programming guide and also incorporate support for HS200 and
HS400 DLL settings using the device tree.

Changes from v5:
1. Addressed Adrian Hunter's comments:
   a. SDHCI_HS400_TUNING flag handling
   b. Use of readl_relaxed_poll_timeout api instead of loop.
   c. Reording of variables.
2. Krzysztof's request for DTS user.

Changes from v4:
1. Addressed Rob Herrirng & Konrad Dybcio comments:
   a. Regarding naming of dt entry.
2. Addressed Adrian Hunter comments:
   a. Regarding parsing of dt and storing variable in driver.
3. Additional change:
   a. Changes in patch 4/4 according to parsing change.

Changes from v3:
1. Addressed Dmitry Baryshkov comments:
   a. Regarding clk division by in V2 patchset
2. Addressed Konrad Dybcio comments:
   a. Renaming of parameters
   b. Memory allocation
   c. couldn't address __free, as didn't fit here
3. Addressed Krzysztof Kozlowsk comment:
   a. Regarding the dt binding
   b. commit message to reflect the need of dt
4. Additional change:
   a. DT parsing logic
   b. Maintain backward compatibility

Changes from v2:
1. Addressed Dmitry Baryshkov comments:
   a. Regarding TCXO frequency.
   b. Regarding clock rate.
   c. regarding checkpatch.

Changes from v1:
1. Addressed Tengfei Fan comment, added missing semicolocon
   in sdhci_msm_host structure.

Ram Prakash Gupta (1):
  arm64: dts: qcom: Add sdhc dll-presets

Sachin Gupta (4):
  dt-bindings: mmc: Add dll-presets values for HS400 and HS200 modes
  mmc: sdhci-msm: Add core_major, minor to msm_host structure
  mmc: sdhci-msm: Add Device tree parsing logic for DLL settings
  mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

 .../devicetree/bindings/mmc/sdhci-msm.yaml    |   5 +
 arch/arm64/boot/dts/qcom/qdu1000.dtsi         |   4 +-
 drivers/mmc/host/sdhci-msm.c                  | 324 +++++++++++++++++-
 3 files changed, 314 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH v6 1/5] dt-bindings: mmc: Add dll-presets values for HS400 and HS200 modes

[Ram Prakash Gupta]

From: Sachin Gupta <quic_sachgupt@quicinc.com>

Document the 'dll-presets' property for MMC device tree bindings.
The 'dll-presets' property defines the DLL configurations for HS400
and HS200 modes.

QC SoCs can have 0 to 4 SDHCI instances, and each one may need
different tuning.

Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
 Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
index 938be8228d66..a1a16e6e12ce 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
@@ -140,6 +140,11 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: platform specific settings for DLL_CONFIG reg.
 
+  qcom,dll-presets:
+    maxItems: 10
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: platform specific settings for DLL registers.
+
   iommus:
     minItems: 1
     maxItems: 8
-- 
2.34.1

Re: [PATCH v6 1/5] dt-bindings: mmc: Add dll-presets values for HS400 and HS200 modes

[Krzysztof Kozlowski]

On 15/12/2025 13:00, Ram Prakash Gupta wrote:
> From: Sachin Gupta <quic_sachgupt@quicinc.com>
> 
> Document the 'dll-presets' property for MMC device tree bindings.
> The 'dll-presets' property defines the DLL configurations for HS400
> and HS200 modes.
> 
> QC SoCs can have 0 to 4 SDHCI instances, and each one may need
> different tuning.
> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> index 938be8228d66..a1a16e6e12ce 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> @@ -140,6 +140,11 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: platform specific settings for DLL_CONFIG reg.
>  
> +  qcom,dll-presets:
> +    maxItems: 10
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: platform specific settings for DLL registers.


So look at your DTS - this is clearly incomplete now. So obvious. I
don't understand why you were hiding the DTS change, you just make
review more difficult.

Anyway, your DTS says you replace some other DLL properties and nothing
in the binding or commit msg explains that. Neither the reason nor the
impact.

Best regards,
Krzysztof

Re: [PATCH v6 1/5] dt-bindings: mmc: Add dll-presets values for HS400 and HS200 modes

[Krzysztof Kozlowski]

On 15/12/2025 13:06, Krzysztof Kozlowski wrote:
> On 15/12/2025 13:00, Ram Prakash Gupta wrote:
>> From: Sachin Gupta <quic_sachgupt@quicinc.com>
>>
>> Document the 'dll-presets' property for MMC device tree bindings.
>> The 'dll-presets' property defines the DLL configurations for HS400
>> and HS200 modes.
>>
>> QC SoCs can have 0 to 4 SDHCI instances, and each one may need
>> different tuning.
>>
>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>> index 938be8228d66..a1a16e6e12ce 100644
>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>> @@ -140,6 +140,11 @@ properties:
>>      $ref: /schemas/types.yaml#/definitions/uint32
>>      description: platform specific settings for DLL_CONFIG reg.
>>  
>> +  qcom,dll-presets:
>> +    maxItems: 10
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    description: platform specific settings for DLL registers.
> 
> 
> So look at your DTS - this is clearly incomplete now. So obvious. I
> don't understand why you were hiding the DTS change, you just make
> review more difficult.
> 
> Anyway, your DTS says you replace some other DLL properties and nothing
> in the binding or commit msg explains that. Neither the reason nor the
> impact.
> 

Plus it looks like you are adding some meaning to the entries, judging
by the DTS. DTS suggested something here is e.g. DLL_CONFIG, so that
meaning - including different modes - needs description in the schema in
items.

Best regards,
Krzysztof

Re: [PATCH v6 1/5] dt-bindings: mmc: Add dll-presets values for HS400 and HS200 modes

[Ram Prakash Gupta]

On 12/15/2025 5:41 PM, Krzysztof Kozlowski wrote:
> On 15/12/2025 13:06, Krzysztof Kozlowski wrote:
>> On 15/12/2025 13:00, Ram Prakash Gupta wrote:
>>> From: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>
>>> Document the 'dll-presets' property for MMC device tree bindings.
>>> The 'dll-presets' property defines the DLL configurations for HS400
>>> and HS200 modes.
>>>
>>> QC SoCs can have 0 to 4 SDHCI instances, and each one may need
>>> different tuning.
>>>
>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> index 938be8228d66..a1a16e6e12ce 100644
>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>> @@ -140,6 +140,11 @@ properties:
>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>      description: platform specific settings for DLL_CONFIG reg.
>>>  
>>> +  qcom,dll-presets:
>>> +    maxItems: 10
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    description: platform specific settings for DLL registers.
>>
>> So look at your DTS - this is clearly incomplete now. So obvious. I
>> don't understand why you were hiding the DTS change, you just make
>> review more difficult.
>>
>> Anyway, your DTS says you replace some other DLL properties and nothing
>> in the binding or commit msg explains that. Neither the reason nor the
>> impact.

Sorry dt change was not pushed earlier, will add dt change as well
going forward.

The existing qcom,dll-config or qcom-ddr-config are not deprecated,
but certainly I will add below in commit regarding the issue as well
which is observed without this change as below:

"Document the 'dll-presets' property for MMC device tree bindings.
The 'dll-presets' property defines the DLL configurations for HS400
and HS200 modes.

It have 2 arrays of 5 elements each for HS400 and HS200 mode.
The 5 elements are DLL_CONFIG, DLL_CONFIG_2,DLL_CONFIG_3, DLL_USER_CTRL
and DDR_CONFIG.

dll-presets fixes instances of CRC error observed with targets having
artanis dll.

QC SoCs can have 0 to 4 SDHCI instances, and each one may need
different tuning using dll-presets."

> Plus it looks like you are adding some meaning to the entries, judging
> by the DTS. DTS suggested something here is e.g. DLL_CONFIG, so that
> meaning - including different modes - needs description in the schema in
> items.
>
> Best regards,
> Krzysztof

I will update the schema description as well with the 2 modes and 5 elements
of it and will update the MaxItems to 2.

Thanks,
Ram

Re: [PATCH v6 1/5] dt-bindings: mmc: Add dll-presets values for HS400 and HS200 modes

[Krzysztof Kozlowski]

On 16/12/2025 13:51, Ram Prakash Gupta wrote:
> 
> On 12/15/2025 5:41 PM, Krzysztof Kozlowski wrote:
>> On 15/12/2025 13:06, Krzysztof Kozlowski wrote:
>>> On 15/12/2025 13:00, Ram Prakash Gupta wrote:
>>>> From: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>>
>>>> Document the 'dll-presets' property for MMC device tree bindings.
>>>> The 'dll-presets' property defines the DLL configurations for HS400
>>>> and HS200 modes.
>>>>
>>>> QC SoCs can have 0 to 4 SDHCI instances, and each one may need
>>>> different tuning.
>>>>
>>>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>>>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> index 938be8228d66..a1a16e6e12ce 100644
>>>> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
>>>> @@ -140,6 +140,11 @@ properties:
>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>      description: platform specific settings for DLL_CONFIG reg.
>>>>  
>>>> +  qcom,dll-presets:
>>>> +    maxItems: 10
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>> +    description: platform specific settings for DLL registers.
>>>
>>> So look at your DTS - this is clearly incomplete now. So obvious. I
>>> don't understand why you were hiding the DTS change, you just make
>>> review more difficult.
>>>
>>> Anyway, your DTS says you replace some other DLL properties and nothing
>>> in the binding or commit msg explains that. Neither the reason nor the
>>> impact.
> 
> Sorry dt change was not pushed earlier, will add dt change as well
> going forward.
> 
> The existing qcom,dll-config or qcom-ddr-config are not deprecated,
> but certainly I will add below in commit regarding the issue as well
> which is observed without this change as below:

No.

You need schema for this. Actually for both cases - replacing properties
and defining the list.

> 
> "Document the 'dll-presets' property for MMC device tree bindings.
> The 'dll-presets' property defines the DLL configurations for HS400
> and HS200 modes.
> 
> It have 2 arrays of 5 elements each for HS400 and HS200 mode.
> The 5 elements are DLL_CONFIG, DLL_CONFIG_2,DLL_CONFIG_3, DLL_USER_CTRL
> and DDR_CONFIG.
> 
> dll-presets fixes instances of CRC error observed with targets having
> artanis dll.
> 
> QC SoCs can have 0 to 4 SDHCI instances, and each one may need
> different tuning using dll-presets."
> 
>> Plus it looks like you are adding some meaning to the entries, judging
>> by the DTS. DTS suggested something here is e.g. DLL_CONFIG, so that
>> meaning - including different modes - needs description in the schema in
>> items.

Here I wrote it already. Don't ask for repeating.

>>
>> Best regards,
>> Krzysztof
> 
> I will update the schema description as well with the 2 modes and 5 elements

I did not say description. I said you need schema.


Best regards,
Krzysztof

[PATCH v6 2/5] arm64: dts: qcom: Add sdhc dll-presets

[Ram Prakash Gupta]

Add sdhc dll-presets for qdu1000 target.

Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qdu1000.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/qdu1000.dtsi b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
index 846e5e5899aa..bc31504d5c8c 100644
--- a/arch/arm64/boot/dts/qcom/qdu1000.dtsi
+++ b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
@@ -912,8 +912,8 @@ sdhc: mmc@8804000 {
 
 			bus-width = <8>;
 
-			qcom,dll-config = <0x0007642c>;
-			qcom,ddr-config = <0x80040868>;
+			qcom,dll-presets = <0x000F64EC 0x0 0x01	0x2C010800 0x80040868>,
+					   <0x0007642C 0x0 0x10 0x2C010800 0x80040868>;
 
 			status = "disabled";
 
-- 
2.34.1

Re: [PATCH v6 2/5] arm64: dts: qcom: Add sdhc dll-presets

[Krzysztof Kozlowski]

On 15/12/2025 13:00, Ram Prakash Gupta wrote:
> Add sdhc dll-presets for qdu1000 target.
> 
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qdu1000.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> 
> diff --git a/arch/arm64/boot/dts/qcom/qdu1000.dtsi b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
> index 846e5e5899aa..bc31504d5c8c 100644
> --- a/arch/arm64/boot/dts/qcom/qdu1000.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
> @@ -912,8 +912,8 @@ sdhc: mmc@8804000 {
>  
>  			bus-width = <8>;
>  
> -			qcom,dll-config = <0x0007642c>;
> -			qcom,ddr-config = <0x80040868>;
> +			qcom,dll-presets = <0x000F64EC 0x0 0x01	0x2C010800 0x80040868>,
> +					   <0x0007642C 0x0 0x10 0x2C010800 0x80040868>;


That's non-bisectable. You just broke the users of this DTS. Also, case
change is not explained and your binding said nothing about deprecating
other properties.


Best regards,
Krzysztof

Re: [PATCH v6 2/5] arm64: dts: qcom: Add sdhc dll-presets

[Krzysztof Kozlowski]

On 15/12/2025 13:05, Krzysztof Kozlowski wrote:
> On 15/12/2025 13:00, Ram Prakash Gupta wrote:
>> Add sdhc dll-presets for qdu1000 target.
>>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>>  arch/arm64/boot/dts/qcom/qdu1000.dtsi | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qdu1000.dtsi b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>> index 846e5e5899aa..bc31504d5c8c 100644
>> --- a/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>> @@ -912,8 +912,8 @@ sdhc: mmc@8804000 {
>>  
>>  			bus-width = <8>;
>>  
>> -			qcom,dll-config = <0x0007642c>;
>> -			qcom,ddr-config = <0x80040868>;
>> +			qcom,dll-presets = <0x000F64EC 0x0 0x01	0x2C010800 0x80040868>,
>> +					   <0x0007642C 0x0 0x10 0x2C010800 0x80040868>;
> 
> 
> That's non-bisectable. You just broke the users of this DTS. Also, case
> change is not explained and your binding said nothing about deprecating
> other properties.


Also, your binding said 10 items, not two.


Best regards,
Krzysztof

Re: [PATCH v6 2/5] arm64: dts: qcom: Add sdhc dll-presets

[Ram Prakash Gupta]

On 12/15/2025 5:37 PM, Krzysztof Kozlowski wrote:
> On 15/12/2025 13:05, Krzysztof Kozlowski wrote:
>> On 15/12/2025 13:00, Ram Prakash Gupta wrote:
>>> Add sdhc dll-presets for qdu1000 target.
>>>
>>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>>> ---
>>>  arch/arm64/boot/dts/qcom/qdu1000.dtsi | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching. For bindings, the preferred subjects are
>> explained here:
>> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

sure I missed to add qdu1000:, will add in subject.

>>> diff --git a/arch/arm64/boot/dts/qcom/qdu1000.dtsi b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>>> index 846e5e5899aa..bc31504d5c8c 100644
>>> --- a/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>>> @@ -912,8 +912,8 @@ sdhc: mmc@8804000 {
>>>  
>>>  			bus-width = <8>;
>>>  
>>> -			qcom,dll-config = <0x0007642c>;
>>> -			qcom,ddr-config = <0x80040868>;
>>> +			qcom,dll-presets = <0x000F64EC 0x0 0x01	0x2C010800 0x80040868>,
>>> +					   <0x0007642C 0x0 0x10 0x2C010800 0x80040868>;
>>
>> That's non-bisectable. You just broke the users of this DTS. Also, case
>> change is not explained and your binding said nothing about deprecating
>> other properties.

right I will update the sequence of dt as last change in the series.

I couldn't get "case change is not explained" but I guess ask is for
explanation why is this change needed. As the soc is using artanis
dll, it needs to have 3 more register values as presets. So in total
2 arrays, for HS400 and HS200, are passed each having 5 elements,
DLL_CONFIG, DLL_CONFIG_2, DLL_CONFIG_3, DLL_USER_CTRL & DDR_CONFIG.
Without these sampling of data would not be optimal and might lead
to CRC errors.

The existing qcom,dll-config or qcom-ddr-config are not deprecated,
as upcoming soc could still be non-artanis and only dll-config and
ddr-config might be needed. But since it was added earlier in qdu1000,
removing it now as the code change is pushed into driver to support
dll-presets.

with QDU1000 there were instance of CRC errors and to fix dll-presets
is used instead of qcom,dll-config & qcom,ddr-config.

I will update the binding as well with more details.

>
> Also, your binding said 10 items, not two.
>
>
> Best regards,
> Krzysztof

Sorry I mistook the elements of array with Items. Will update in binding
to 2 items.

Re: [PATCH v6 2/5] arm64: dts: qcom: Add sdhc dll-presets

[Krzysztof Kozlowski]

On 16/12/2025 13:41, Ram Prakash Gupta wrote:
> 
> On 12/15/2025 5:37 PM, Krzysztof Kozlowski wrote:
>> On 15/12/2025 13:05, Krzysztof Kozlowski wrote:
>>> On 15/12/2025 13:00, Ram Prakash Gupta wrote:
>>>> Add sdhc dll-presets for qdu1000 target.
>>>>
>>>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/qdu1000.dtsi | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>> Please use subject prefixes matching the subsystem. You can get them for
>>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>>> your patch is touching. For bindings, the preferred subjects are
>>> explained here:
>>> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
> sure I missed to add qdu1000:, will add in subject.
> 
>>>> diff --git a/arch/arm64/boot/dts/qcom/qdu1000.dtsi b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>>>> index 846e5e5899aa..bc31504d5c8c 100644
>>>> --- a/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/qdu1000.dtsi
>>>> @@ -912,8 +912,8 @@ sdhc: mmc@8804000 {
>>>>  
>>>>  			bus-width = <8>;
>>>>  
>>>> -			qcom,dll-config = <0x0007642c>;
>>>> -			qcom,ddr-config = <0x80040868>;
>>>> +			qcom,dll-presets = <0x000F64EC 0x0 0x01	0x2C010800 0x80040868>,
>>>> +					   <0x0007642C 0x0 0x10 0x2C010800 0x80040868>;
>>>
>>> That's non-bisectable. You just broke the users of this DTS. Also, case
>>> change is not explained and your binding said nothing about deprecating
>>> other properties.
> 
> right I will update the sequence of dt as last change in the series.

This will fix nothing. Please read maintainer soc profile how DTS is
handled. It's still no go, shall I be explicit with NAK?

> 
> I couldn't get "case change is not explained" but I guess ask is for
> explanation why is this change needed. As the soc is using artanis

No, why are you making it upper case. What is the preferred style for DTS?


Best regards,
Krzysztof

[PATCH v6 3/5] mmc: sdhci-msm: Add core_major, minor to msm_host structure

[Ram Prakash Gupta]

From: Sachin Gupta <quic_sachgupt@quicinc.com>

Add the core_major and core_minor variables to
the msm_host structure, allowing these variables to be
accessed more easily throughout the msm_host context.
This update is necessary for an upcoming follow-up patch.

Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 3b85233131b3..dc79f828522b 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -283,6 +283,8 @@ struct sdhci_msm_host {
 	bool tuning_done;
 	bool calibration_done;
 	u8 saved_tuning_phase;
+	u8 core_major;
+	u16 core_minor;
 	bool use_cdclp533;
 	u32 curr_pwr_state;
 	u32 curr_io_level;
@@ -2691,6 +2693,10 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 	core_major = (core_version & CORE_VERSION_MAJOR_MASK) >>
 		      CORE_VERSION_MAJOR_SHIFT;
 	core_minor = core_version & CORE_VERSION_MINOR_MASK;
+
+	msm_host->core_major = core_major;
+	msm_host->core_minor = core_minor;
+
 	dev_dbg(&pdev->dev, "MCI Version: 0x%08x, major: 0x%04x, minor: 0x%02x\n",
 		core_version, core_major, core_minor);
 
-- 
2.34.1

[PATCH v6 4/5] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Ram Prakash Gupta]

From: Sachin Gupta <quic_sachgupt@quicinc.com>

This update introduces the capability to configure HS200
and HS400 DLL settings via the device tree and parsing it.

Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 41 ++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index dc79f828522b..1fcd92158bee 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -266,6 +266,19 @@ struct sdhci_msm_variant_info {
 	const struct sdhci_msm_offset *offset;
 };
 
+/*
+ * DLL registers which needs be programmed with HSR settings.
+ * Add any new register only at the end and don't change the
+ * sequence.
+ */
+struct sdhci_msm_dll {
+	u32 dll_config;
+	u32 dll_config_2;
+	u32 dll_config_3;
+	u32 dll_usr_ctl;
+	u32 ddr_config;
+};
+
 struct sdhci_msm_host {
 	struct platform_device *pdev;
 	void __iomem *core_mem;	/* MSM SDCC mapped address */
@@ -274,6 +287,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 sdhci_msm_dll dll[2];
 #ifdef CONFIG_MMC_CRYPTO
 	struct qcom_ice *ice;
 #endif
@@ -302,6 +316,7 @@ struct sdhci_msm_host {
 	u32 dll_config;
 	u32 ddr_config;
 	bool vqmmc_enabled;
+	bool artanis_dll;
 };
 
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
@@ -2534,6 +2549,23 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
 	return ret;
 }
 
+#define DLL_SIZE 10
+static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
+{
+	u32 *dll_table = &msm_host->dll[0].dll_config;
+	int ret;
+
+	msm_host->artanis_dll = false;
+
+	ret = of_property_read_variable_u32_array(dev->of_node,
+						  "qcom,dll-presets",
+						  dll_table, DLL_SIZE, DLL_SIZE);
+	if (ret == DLL_SIZE)
+		msm_host->artanis_dll = true;
+
+	return ret;
+}
+
 static int sdhci_msm_probe(struct platform_device *pdev)
 {
 	struct sdhci_host *host;
@@ -2580,6 +2612,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
 
 	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
 
+	/*
+	 * Parse HSR dll only when property is present in DT.
+	 */
+	ret = sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host);
+	if (ret == -ENODATA || ret == -EOVERFLOW) {
+		dev_err(&pdev->dev, "Bad DLL in dt (%d)\n", ret);
+		return ret;
+	}
+
 	ret = sdhci_msm_gcc_reset(&pdev->dev, host);
 	if (ret)
 		return ret;
-- 
2.34.1

Re: [PATCH v6 4/5] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Konrad Dybcio]

On 12/15/25 1:00 PM, Ram Prakash Gupta wrote:
> From: Sachin Gupta <quic_sachgupt@quicinc.com>
> 
> This update introduces the capability to configure HS200
> and HS400 DLL settings via the device tree and parsing it.
> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---

[...]

> +#define DLL_SIZE 10
> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
> +{
> +	u32 *dll_table = &msm_host->dll[0].dll_config;
> +	int ret;
> +
> +	msm_host->artanis_dll = false;
> +
> +	ret = of_property_read_variable_u32_array(dev->of_node,
> +						  "qcom,dll-presets",
> +						  dll_table, DLL_SIZE, DLL_SIZE);
> +	if (ret == DLL_SIZE)
> +		msm_host->artanis_dll = true;

This feels backwards.. can we first somehow determine whether this
platform has the artanis_dll (whatever that is since you didn't explain)
and then make decisions on what to retrieve from the DT & how to
interpret it?

> +	return ret;
> +}
> +
>  static int sdhci_msm_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host;
> @@ -2580,6 +2612,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  
>  	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>  
> +	/*
> +	 * Parse HSR dll only when property is present in DT.
> +	 */

/* Parse ... */, it's short enough

Probably also "DLL"

Konrad

Re: [PATCH v6 4/5] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Ram Prakash Gupta]

On 12/16/2025 7:59 PM, Konrad Dybcio wrote:
> On 12/15/25 1:00 PM, Ram Prakash Gupta wrote:
>> From: Sachin Gupta <quic_sachgupt@quicinc.com>
>>
>> This update introduces the capability to configure HS200
>> and HS400 DLL settings via the device tree and parsing it.
>>
>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
> [...]
>
>> +#define DLL_SIZE 10
>> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
>> +{
>> +	u32 *dll_table = &msm_host->dll[0].dll_config;
>> +	int ret;
>> +
>> +	msm_host->artanis_dll = false;
>> +
>> +	ret = of_property_read_variable_u32_array(dev->of_node,
>> +						  "qcom,dll-presets",
>> +						  dll_table, DLL_SIZE, DLL_SIZE);
>> +	if (ret == DLL_SIZE)
>> +		msm_host->artanis_dll = true;
> This feels backwards.. can we first somehow determine whether this
> platform has the artanis_dll (whatever that is since you didn't explain)
> and then make decisions on what to retrieve from the DT & how to
> interpret it?

I checked document and with hardware designers, currently there is no
distinguishable hardware configuration which can be used, will have to
rely on dt only.

>
>> +	return ret;
>> +}
>> +
>>  static int sdhci_msm_probe(struct platform_device *pdev)
>>  {
>>  	struct sdhci_host *host;
>> @@ -2580,6 +2612,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>  
>>  	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>>  
>> +	/*
>> +	 * Parse HSR dll only when property is present in DT.
>> +	 */
> /* Parse ... */, it's short enough
>
> Probably also "DLL"
>
> Konrad

sure, will update.

Thanks,
Ram

Re: [PATCH v6 4/5] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Bjorn Andersson]

On Mon, Dec 15, 2025 at 05:30:08PM +0530, Ram Prakash Gupta wrote:
> From: Sachin Gupta <quic_sachgupt@quicinc.com>
> 
> This update introduces the capability to configure HS200
> and HS400 DLL settings via the device tree and parsing it.

No it doesn't, it merely reads a bunch of integers from DeviceTree and
does nothing with them.

Please write your commit message in imperative mood (avoid "This
update") and please include a reasoning for why this commit exists, or
as
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
says "Describe your problem.".

> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  drivers/mmc/host/sdhci-msm.c | 41 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index dc79f828522b..1fcd92158bee 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -266,6 +266,19 @@ struct sdhci_msm_variant_info {
>  	const struct sdhci_msm_offset *offset;
>  };
>  
> +/*
> + * DLL registers which needs be programmed with HSR settings.
> + * Add any new register only at the end and don't change the
> + * sequence.

/* You have to only add entries at the end, but I'm not going to tell you why... */

> + */
> +struct sdhci_msm_dll {
> +	u32 dll_config;
> +	u32 dll_config_2;
> +	u32 dll_config_3;
> +	u32 dll_usr_ctl;
> +	u32 ddr_config;
> +};
> +
>  struct sdhci_msm_host {
>  	struct platform_device *pdev;
>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
> @@ -274,6 +287,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 sdhci_msm_dll dll[2];
>  #ifdef CONFIG_MMC_CRYPTO
>  	struct qcom_ice *ice;
>  #endif
> @@ -302,6 +316,7 @@ struct sdhci_msm_host {
>  	u32 dll_config;
>  	u32 ddr_config;

So this dll_config/ddr_config pair is no longer supposed to be used? Or
are there now two sets of dll and ddr configurations to be provided?

Regards,
Bjorn

>  	bool vqmmc_enabled;
> +	bool artanis_dll;
>  };
>  
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
> @@ -2534,6 +2549,23 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>  	return ret;
>  }
>  
> +#define DLL_SIZE 10
> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
> +{
> +	u32 *dll_table = &msm_host->dll[0].dll_config;
> +	int ret;
> +
> +	msm_host->artanis_dll = false;
> +
> +	ret = of_property_read_variable_u32_array(dev->of_node,
> +						  "qcom,dll-presets",
> +						  dll_table, DLL_SIZE, DLL_SIZE);
> +	if (ret == DLL_SIZE)
> +		msm_host->artanis_dll = true;
> +
> +	return ret;
> +}
> +
>  static int sdhci_msm_probe(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host;
> @@ -2580,6 +2612,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>  
>  	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>  
> +	/*
> +	 * Parse HSR dll only when property is present in DT.
> +	 */
> +	ret = sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host);
> +	if (ret == -ENODATA || ret == -EOVERFLOW) {
> +		dev_err(&pdev->dev, "Bad DLL in dt (%d)\n", ret);
> +		return ret;
> +	}
> +
>  	ret = sdhci_msm_gcc_reset(&pdev->dev, host);
>  	if (ret)
>  		return ret;
> -- 
> 2.34.1
> 

Re: [PATCH v6 4/5] mmc: sdhci-msm: Add Device tree parsing logic for DLL settings

[Ram Prakash Gupta]

On 12/18/2025 7:02 PM, Bjorn Andersson wrote:
> On Mon, Dec 15, 2025 at 05:30:08PM +0530, Ram Prakash Gupta wrote:
>> From: Sachin Gupta <quic_sachgupt@quicinc.com>
>>
>> This update introduces the capability to configure HS200
>> and HS400 DLL settings via the device tree and parsing it.
> No it doesn't, it merely reads a bunch of integers from DeviceTree and
> does nothing with them.
>
> Please write your commit message in imperative mood (avoid "This
> update") and please include a reasoning for why this commit exists, or
> as
> https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
> says "Describe your problem.".

Will update this commit with only parsing explanation. and would remove "This update"

>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 41 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index dc79f828522b..1fcd92158bee 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -266,6 +266,19 @@ struct sdhci_msm_variant_info {
>>  	const struct sdhci_msm_offset *offset;
>>  };
>>  
>> +/*
>> + * DLL registers which needs be programmed with HSR settings.
>> + * Add any new register only at the end and don't change the
>> + * sequence.
> /* You have to only add entries at the end, but I'm not going to tell you why... */

Its for future proofing for any new register addition so that it does not break
existing targets. I ll update this comment.

>
>> + */
>> +struct sdhci_msm_dll {
>> +	u32 dll_config;
>> +	u32 dll_config_2;
>> +	u32 dll_config_3;
>> +	u32 dll_usr_ctl;
>> +	u32 ddr_config;
>> +};
>> +
>>  struct sdhci_msm_host {
>>  	struct platform_device *pdev;
>>  	void __iomem *core_mem;	/* MSM SDCC mapped address */
>> @@ -274,6 +287,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 sdhci_msm_dll dll[2];
>>  #ifdef CONFIG_MMC_CRYPTO
>>  	struct qcom_ice *ice;
>>  #endif
>> @@ -302,6 +316,7 @@ struct sdhci_msm_host {
>>  	u32 dll_config;
>>  	u32 ddr_config;
> So this dll_config/ddr_config pair is no longer supposed to be used? Or
> are there now two sets of dll and ddr configurations to be provided?
>
> Regards,
> Bjorn

These are kept to continue support for the older targets, with newer targets
which are all with artanis dll, these wont be used.

Thanks,
Ram

>
>>  	bool vqmmc_enabled;
>> +	bool artanis_dll;
>>  };
>>  
>>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>> @@ -2534,6 +2549,23 @@ static int sdhci_msm_gcc_reset(struct device *dev, struct sdhci_host *host)
>>  	return ret;
>>  }
>>  
>> +#define DLL_SIZE 10
>> +static int sdhci_msm_dt_parse_dll_info(struct device *dev, struct sdhci_msm_host *msm_host)
>> +{
>> +	u32 *dll_table = &msm_host->dll[0].dll_config;
>> +	int ret;
>> +
>> +	msm_host->artanis_dll = false;
>> +
>> +	ret = of_property_read_variable_u32_array(dev->of_node,
>> +						  "qcom,dll-presets",
>> +						  dll_table, DLL_SIZE, DLL_SIZE);
>> +	if (ret == DLL_SIZE)
>> +		msm_host->artanis_dll = true;
>> +
>> +	return ret;
>> +}
>> +
>>  static int sdhci_msm_probe(struct platform_device *pdev)
>>  {
>>  	struct sdhci_host *host;
>> @@ -2580,6 +2612,15 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>>  
>>  	msm_host->saved_tuning_phase = INVALID_TUNING_PHASE;
>>  
>> +	/*
>> +	 * Parse HSR dll only when property is present in DT.
>> +	 */
>> +	ret = sdhci_msm_dt_parse_dll_info(&pdev->dev, msm_host);
>> +	if (ret == -ENODATA || ret == -EOVERFLOW) {
>> +		dev_err(&pdev->dev, "Bad DLL in dt (%d)\n", ret);
>> +		return ret;
>> +	}
>> +
>>  	ret = sdhci_msm_gcc_reset(&pdev->dev, host);
>>  	if (ret)
>>  		return ret;
>> -- 
>> 2.34.1
>>

[PATCH v6 5/5] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Ram Prakash Gupta]

From: Sachin Gupta <quic_sachgupt@quicinc.com>

With the current DLL sequence stability issues for data
transfer seen in HS400 and HS200 modes.

"mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
data error 0"

Rectify the DLL programming sequence as per latest hardware
programming guide

Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
---
 drivers/mmc/host/sdhci-msm.c | 277 ++++++++++++++++++++++++++++++++---
 1 file changed, 260 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index 1fcd92158bee..f459b40db41c 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -28,6 +28,7 @@
 #define CORE_VERSION_MAJOR_SHIFT	28
 #define CORE_VERSION_MAJOR_MASK		(0xf << CORE_VERSION_MAJOR_SHIFT)
 #define CORE_VERSION_MINOR_MASK		0xff
+#define SDHCI_MSM_MIN_V_7FF		0x6e
 
 #define CORE_MCI_GENERICS		0x70
 #define SWITCHABLE_SIGNALING_VOLTAGE	BIT(29)
@@ -119,7 +120,8 @@
 #define CORE_PWRSAVE_DLL	BIT(3)
 
 #define DDR_CONFIG_POR_VAL	0x80040873
-
+#define DLL_CONFIG_3_POR_VAL	0x10
+#define TCXO_FREQ               19200000
 
 #define INVALID_TUNING_PHASE	-1
 #define SDHCI_MSM_MIN_CLOCK	400000
@@ -319,6 +321,15 @@ struct sdhci_msm_host {
 	bool artanis_dll;
 };
 
+enum dll_init_context {
+	DLL_INIT_NORMAL,
+};
+
+enum mode {
+	HS400, // equivalent to SDR104 mode for DLL.
+	HS200, // equivalent to SDR50 mode for DLL.
+};
+
 static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -805,6 +816,207 @@ static int msm_init_cm_dll(struct sdhci_host *host)
 	return 0;
 }
 
+static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
+{
+	return SDHCI_MSM_MIN_CLOCK;
+}
+
+static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+	struct clk *core_clk = msm_host->bulk_clks[0].clk;
+	struct mmc_ios ios = host->mmc->ios;
+	unsigned int sup_clk;
+
+	if (req_clk < sdhci_msm_get_min_clock(host))
+		return sdhci_msm_get_min_clock(host);
+
+	sup_clk = clk_get_rate(core_clk);
+
+	if (ios.timing == MMC_TIMING_MMC_HS400 ||
+	    host->flags & SDHCI_HS400_TUNING)
+		sup_clk = sup_clk / 2;
+
+	return sup_clk;
+}
+
+/* Initialize the DLL (Programmable Delay Line) */
+static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
+				 init_context, enum mode index)
+{
+	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;
+	struct mmc_host *mmc = host->mmc;
+	u32 ddr_cfg_offset, core_vendor_spec, config;
+	void __iomem *ioaddr = host->ioaddr;
+	unsigned long flags, dll_clock;
+	int rc = 0;
+	u32 dll_lock;
+
+	dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
+	spin_lock_irqsave(&host->lock, flags);
+
+	core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
+
+	/*
+	 * Always disable PWRSAVE during the DLL power
+	 * up regardless of its current setting.
+	 */
+	core_vendor_spec &= ~CORE_CLK_PWRSAVE;
+	writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
+
+	if (msm_host->use_14lpp_dll_reset) {
+		/* Disable CK_OUT */
+		config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+		config &= ~CORE_CK_OUT_EN;
+		writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+		/* Disable the DLL clock */
+		config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+		config |= CORE_DLL_CLOCK_DISABLE;
+		writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+	}
+
+	/*
+	 * Write 1 to DLL_RST bit of DLL_CONFIG register
+	 * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
+	 */
+	config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+	config |= (CORE_DLL_RST | CORE_DLL_PDN);
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+	/*
+	 * Configure DLL_CONFIG_3 and USER_CTRL
+	 * (Only applicable for 7FF projects).
+	 */
+	if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
+		writel_relaxed(msm_host->dll[index].dll_config_3,
+			       ioaddr + msm_offset->core_dll_config_3);
+		writel_relaxed(msm_host->dll[index].dll_usr_ctl,
+			       ioaddr + msm_offset->core_dll_usr_ctl);
+	}
+
+	/*
+	 * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
+	 */
+	ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
+					: msm_offset->core_ddr_config_old;
+
+	config = msm_host->dll[index].ddr_config;
+	writel_relaxed(config, ioaddr + ddr_cfg_offset);
+
+	/* Set DLL_CONFIG_2 */
+	if (msm_host->use_14lpp_dll_reset) {
+		u32 mclk_freq;
+		int cycle_cnt;
+
+		/*
+		 * Only configure the mclk_freq in normal DLL init
+		 * context. If the DLL init is coming from
+		 * CX Collapse Exit context, the host->clock may be zero.
+		 * The DLL_CONFIG_2 register has already been restored to
+		 * proper value prior to getting here.
+		 */
+		if (init_context == DLL_INIT_NORMAL) {
+			cycle_cnt = readl_relaxed(ioaddr +
+					msm_offset->core_dll_config_2)
+					& CORE_FLL_CYCLE_CNT ? 8 : 4;
+
+			mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
+
+			if (dll_clock < 100000000) {
+				pr_err("%s: %s: Non standard clk freq =%u\n",
+				       mmc_hostname(mmc), __func__, dll_clock);
+				rc = -EINVAL;
+				goto out;
+			}
+
+			config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+			config = (config & ~GENMASK(17, 10)) |
+					FIELD_PREP(GENMASK(17, 10), mclk_freq);
+			writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+		}
+		/* wait for 5us before enabling DLL clock */
+		udelay(5);
+	}
+
+	config = msm_host->dll[index].dll_config;
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+	/* Wait for 52us */
+	spin_unlock_irqrestore(&host->lock, flags);
+	usleep_range(60, 70);
+	spin_lock_irqsave(&host->lock, flags);
+
+	/*
+	 * Write 0 to DLL_RST bit of DLL_CONFIG register
+	 * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
+	 */
+	config &= ~CORE_DLL_RST;
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+	config &= ~CORE_DLL_PDN;
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+	/* Write 1 to DLL_RST bit of DLL_CONFIG register */
+	config |= CORE_DLL_RST;
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+	/* Write 0 to DLL_RST bit of DLL_CONFIG register */
+	config &= ~CORE_DLL_RST;
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+	/* Set CORE_DLL_CLOCK_DISABLE to 0 */
+	if (msm_host->use_14lpp_dll_reset) {
+		config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
+		config &= ~CORE_DLL_CLOCK_DISABLE;
+		writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
+	}
+
+	/* Set DLL_EN bit to 1. */
+	config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
+	config |= CORE_DLL_EN;
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+	/*
+	 * Wait for 8000 input clock. Here we calculate the
+	 * delay from fixed clock freq 192MHz, which turns out 42us.
+	 */
+	spin_unlock_irqrestore(&host->lock, flags);
+	usleep_range(50, 60);
+	spin_lock_irqsave(&host->lock, flags);
+
+	/* Set CK_OUT_EN bit to 1. */
+	config |= CORE_CK_OUT_EN;
+	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
+
+	/*
+	 * Wait until DLL_LOCK bit of DLL_STATUS register
+	 * becomes '1'.
+	 */
+	rc = readl_relaxed_poll_timeout(ioaddr +
+					msm_offset->core_dll_status,
+					dll_lock,
+					dll_lock & CORE_DLL_LOCK,
+					10,
+					100);
+	if (rc == -ETIMEDOUT)
+		pr_err("%s: Unable to get DLL_LOCK, dll_status: 0x%08x\n",
+		       mmc_hostname(host->mmc), dll_lock);
+
+out:
+	if (core_vendor_spec & CORE_CLK_PWRSAVE) {
+		/* Reenable PWRSAVE as needed */
+		config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
+		config |= CORE_CLK_PWRSAVE;
+		writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
+	}
+
+	spin_unlock_irqrestore(&host->lock, flags);
+	return rc;
+}
+
 static void msm_hc_select_default(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -927,6 +1139,24 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
 		msm_hc_select_default(host);
 }
 
+static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
+{
+	if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104 ||
+	    host->mmc->ios.timing == MMC_TIMING_MMC_HS400)
+		return sdhci_msm_configure_dll(host, init_context, HS400);
+
+	return sdhci_msm_configure_dll(host, init_context, HS200);
+}
+
+static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
+
+	return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
+		msm_init_cm_dll(host);
+}
+
 static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -942,7 +1172,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
 	 * Retuning in HS400 (DDR mode) will fail, just reset the
 	 * tuning block and restore the saved tuning phase.
 	 */
-	ret = msm_init_cm_dll(host);
+	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
 	if (ret)
 		goto out;
 
@@ -1030,7 +1260,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
 	return ret;
 }
 
-static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
+static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
 {
 	struct mmc_host *mmc = host->mmc;
 	u32 dll_status, config, ddr_cfg_offset;
@@ -1053,7 +1283,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
 		ddr_cfg_offset = msm_offset->core_ddr_config;
 	else
 		ddr_cfg_offset = msm_offset->core_ddr_config_old;
-	writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
+
+	if (msm_host->artanis_dll)
+		writel_relaxed(msm_host->dll[index].ddr_config, host->ioaddr + ddr_cfg_offset);
+	else
+		writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
 
 	if (mmc->ios.enhanced_strobe) {
 		config = readl_relaxed(host->ioaddr +
@@ -1122,7 +1356,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
 	 * Retuning in HS400 (DDR mode) will fail, just reset the
 	 * tuning block and restore the saved tuning phase.
 	 */
-	ret = msm_init_cm_dll(host);
+	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
+
 	if (ret)
 		goto out;
 
@@ -1142,7 +1377,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
 	if (msm_host->use_cdclp533)
 		ret = sdhci_msm_cdclp533_calibration(host);
 	else
-		ret = sdhci_msm_cm_dll_sdc4_calibration(host);
+		ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
 out:
 	pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
 		 __func__, ret);
@@ -1185,7 +1420,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
 		return 0;
 
 	/* Reset the tuning block */
-	ret = msm_init_cm_dll(host);
+	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
+
 	if (ret)
 		return ret;
 
@@ -1227,6 +1463,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
 	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
 	u32 config;
+	bool hs400_tuning;
 
 	if (!sdhci_msm_is_tuning_needed(host)) {
 		msm_host->use_cdr = false;
@@ -1256,17 +1493,23 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	 * - select MCLK/2 in VENDOR_SPEC
 	 * - program MCLK to 400MHz (or nearest supported) in GCC
 	 */
-	if (host->flags & SDHCI_HS400_TUNING) {
+	hs400_tuning = !!(host->flags & SDHCI_HS400_TUNING);
+	if (hs400_tuning) {
 		sdhci_msm_hc_select_mode(host);
 		msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
-		host->flags &= ~SDHCI_HS400_TUNING;
 	}
 
 retry:
 	/* First of all reset the tuning block */
-	rc = msm_init_cm_dll(host);
-	if (rc)
+	rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
+	if (rc) {
+		if (hs400_tuning)
+			host->flags &= ~SDHCI_HS400_TUNING;
 		return rc;
+	}
+
+	if (hs400_tuning)
+		host->flags &= ~SDHCI_HS400_TUNING;
 
 	phase = 0;
 	do {
@@ -1297,6 +1540,8 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 				mmc_hostname(mmc));
 			if (--tuning_seq_cnt) {
 				tuned_phase_cnt = 0;
+				if (hs400_tuning)
+					host->flags |= SDHCI_HS400_TUNING;
 				goto retry;
 			}
 		}
@@ -1319,8 +1564,11 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
 		dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n",
 			 mmc_hostname(mmc), phase);
 	} else {
-		if (--tuning_seq_cnt)
+		if (--tuning_seq_cnt) {
+			if (hs400_tuning)
+				host->flags |= SDHCI_HS400_TUNING;
 			goto retry;
+		}
 		/* Tuning failed */
 		dev_dbg(mmc_dev(mmc), "%s: No tuning point found\n",
 		       mmc_hostname(mmc));
@@ -1847,11 +2095,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
 	return clk_round_rate(core_clk, ULONG_MAX);
 }
 
-static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
-{
-	return SDHCI_MSM_MIN_CLOCK;
-}
-
 /*
  * __sdhci_msm_set_clock - sdhci_msm clock control.
  *
-- 
2.34.1

Re: [PATCH v6 5/5] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[kernel test robot]

Hi Ram,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on krzk-dt/for-next v6.19-rc1]
[cannot apply to robh/for-next krzk/for-next ulf-hansson-mmc-mirror/next next-20251216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ram-Prakash-Gupta/dt-bindings-mmc-Add-dll-presets-values-for-HS400-and-HS200-modes/20251215-200814
base:   linus/master
patch link:    https://lore.kernel.org/r/20251215120009.3877889-6-quic_rampraka%40quicinc.com
patch subject: [PATCH v6 5/5] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20251217/202512171135.hzSvlDE9-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251217/202512171135.hzSvlDE9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202512171135.hzSvlDE9-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:31,
                    from arch/x86/include/asm/bug.h:193,
                    from arch/x86/include/asm/alternative.h:9,
                    from arch/x86/include/asm/barrier.h:5,
                    from include/linux/list.h:11,
                    from include/linux/module.h:12,
                    from drivers/mmc/host/sdhci-msm.c:8:
   drivers/mmc/host/sdhci-msm.c: In function 'sdhci_msm_configure_dll':
>> include/linux/kern_levels.h:5:25: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
       5 | #define KERN_SOH        "\001"          /* ASCII Start Of Header */
         |                         ^~~~~~
   include/linux/printk.h:484:25: note: in definition of macro 'printk_index_wrap'
     484 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                         ^~~~
   include/linux/printk.h:555:9: note: in expansion of macro 'printk'
     555 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~
   include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
      11 | #define KERN_ERR        KERN_SOH "3"    /* error conditions */
         |                         ^~~~~~~~
   include/linux/printk.h:555:16: note: in expansion of macro 'KERN_ERR'
     555 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                ^~~~~~~~
   drivers/mmc/host/sdhci-msm.c:930:33: note: in expansion of macro 'pr_err'
     930 |                                 pr_err("%s: %s: Non standard clk freq =%u\n",
         |                                 ^~~~~~
--
   In file included from include/asm-generic/bug.h:31,
                    from arch/x86/include/asm/bug.h:193,
                    from arch/x86/include/asm/alternative.h:9,
                    from arch/x86/include/asm/barrier.h:5,
                    from include/linux/list.h:11,
                    from include/linux/module.h:12,
                    from sdhci-msm.c:8:
   sdhci-msm.c: In function 'sdhci_msm_configure_dll':
>> include/linux/kern_levels.h:5:25: warning: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'long unsigned int' [-Wformat=]
       5 | #define KERN_SOH        "\001"          /* ASCII Start Of Header */
         |                         ^~~~~~
   include/linux/printk.h:484:25: note: in definition of macro 'printk_index_wrap'
     484 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                         ^~~~
   include/linux/printk.h:555:9: note: in expansion of macro 'printk'
     555 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |         ^~~~~~
   include/linux/kern_levels.h:11:25: note: in expansion of macro 'KERN_SOH'
      11 | #define KERN_ERR        KERN_SOH "3"    /* error conditions */
         |                         ^~~~~~~~
   include/linux/printk.h:555:16: note: in expansion of macro 'KERN_ERR'
     555 |         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                ^~~~~~~~
   sdhci-msm.c:930:33: note: in expansion of macro 'pr_err'
     930 |                                 pr_err("%s: %s: Non standard clk freq =%u\n",
         |                                 ^~~~~~


vim +5 include/linux/kern_levels.h

314ba3520e513a Joe Perches 2012-07-30  4  
04d2c8c83d0e3a Joe Perches 2012-07-30 @5  #define KERN_SOH	"\001"		/* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30  6  #define KERN_SOH_ASCII	'\001'
04d2c8c83d0e3a Joe Perches 2012-07-30  7  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 5/5] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Bjorn Andersson]

On Mon, Dec 15, 2025 at 05:30:09PM +0530, Ram Prakash Gupta wrote:
> From: Sachin Gupta <quic_sachgupt@quicinc.com>
> 
> With the current DLL sequence stability issues for data
> transfer seen in HS400 and HS200 modes.
> 
> "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
> data error 0"
> 
> Rectify the DLL programming sequence as per latest hardware
> programming guide

You're talking about how things are unstable, but what the commit
message completely fails to mention is that all this new spaghetti is
conditional on msm_host->artanis_dll, so I believe this statement
only applies to a single supported SoC?

No where in this series do you mention that this only (?) applies to
QDU1000. 

> 
> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
> ---
>  drivers/mmc/host/sdhci-msm.c | 277 ++++++++++++++++++++++++++++++++---
>  1 file changed, 260 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 1fcd92158bee..f459b40db41c 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -28,6 +28,7 @@
>  #define CORE_VERSION_MAJOR_SHIFT	28
>  #define CORE_VERSION_MAJOR_MASK		(0xf << CORE_VERSION_MAJOR_SHIFT)
>  #define CORE_VERSION_MINOR_MASK		0xff
> +#define SDHCI_MSM_MIN_V_7FF		0x6e
>  
>  #define CORE_MCI_GENERICS		0x70
>  #define SWITCHABLE_SIGNALING_VOLTAGE	BIT(29)
> @@ -119,7 +120,8 @@
>  #define CORE_PWRSAVE_DLL	BIT(3)
>  
>  #define DDR_CONFIG_POR_VAL	0x80040873
> -
> +#define DLL_CONFIG_3_POR_VAL	0x10
> +#define TCXO_FREQ               19200000

If you read the current implementation of msm_init_cm_dll() you'll see
that we don't hard code the XO clock...

>  
>  #define INVALID_TUNING_PHASE	-1
>  #define SDHCI_MSM_MIN_CLOCK	400000
> @@ -319,6 +321,15 @@ struct sdhci_msm_host {
>  	bool artanis_dll;
>  };
>  
> +enum dll_init_context {
> +	DLL_INIT_NORMAL,

What is the purpose of a single-entry enum to capture the value 0?

> +};
> +
> +enum mode {
> +	HS400, // equivalent to SDR104 mode for DLL.
> +	HS200, // equivalent to SDR50 mode for DLL.

This is effectively a bool, with a terribly generic name.

> +};
> +
>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -805,6 +816,207 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>  	return 0;
>  }
>  
> +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> +{
> +	return SDHCI_MSM_MIN_CLOCK;
> +}
> +
> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +	struct clk *core_clk = msm_host->bulk_clks[0].clk;
> +	struct mmc_ios ios = host->mmc->ios;
> +	unsigned int sup_clk;
> +
> +	if (req_clk < sdhci_msm_get_min_clock(host))
> +		return sdhci_msm_get_min_clock(host);
> +
> +	sup_clk = clk_get_rate(core_clk);
> +
> +	if (ios.timing == MMC_TIMING_MMC_HS400 ||
> +	    host->flags & SDHCI_HS400_TUNING)
> +		sup_clk = sup_clk / 2;
> +
> +	return sup_clk;

Isn't sup_clk the same as host->mmc->actual_clock? Isn't this already
calculated for you?

> +}
> +
> +/* Initialize the DLL (Programmable Delay Line) */

So now we're going to have two functions that are named pretty much the
same and look pretty similar?

With the main difference that the slightly more generically named
sdhci_msm_configure_dll() is only called if we have a artanis_dll,
otherwise msm_init_cm_dll() is used?

> +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
> +				 init_context, enum mode index)
> +{

Large parts of this function is duplicated from msm_init_cm_dll(). This
isn't okay.

> +	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;
> +	struct mmc_host *mmc = host->mmc;
> +	u32 ddr_cfg_offset, core_vendor_spec, config;
> +	void __iomem *ioaddr = host->ioaddr;
> +	unsigned long flags, dll_clock;
> +	int rc = 0;
> +	u32 dll_lock;
> +
> +	dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
> +	spin_lock_irqsave(&host->lock, flags);
> +
> +	core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> +
> +	/*
> +	 * Always disable PWRSAVE during the DLL power
> +	 * up regardless of its current setting.
> +	 */
> +	core_vendor_spec &= ~CORE_CLK_PWRSAVE;
> +	writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
> +
> +	if (msm_host->use_14lpp_dll_reset) {
> +		/* Disable CK_OUT */
> +		config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> +		config &= ~CORE_CK_OUT_EN;
> +		writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> +		/* Disable the DLL clock */
> +		config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> +		config |= CORE_DLL_CLOCK_DISABLE;
> +		writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> +	}
> +
> +	/*
> +	 * Write 1 to DLL_RST bit of DLL_CONFIG register
> +	 * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
> +	 */
> +	config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> +	config |= (CORE_DLL_RST | CORE_DLL_PDN);
> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> +	/*
> +	 * Configure DLL_CONFIG_3 and USER_CTRL
> +	 * (Only applicable for 7FF projects).

What is a 7FF project?

> +	 */
> +	if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
> +		writel_relaxed(msm_host->dll[index].dll_config_3,
> +			       ioaddr + msm_offset->core_dll_config_3);
> +		writel_relaxed(msm_host->dll[index].dll_usr_ctl,
> +			       ioaddr + msm_offset->core_dll_usr_ctl);
> +	}
> +
> +	/*
> +	 * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
> +	 */
> +	ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
> +					: msm_offset->core_ddr_config_old;
> +
> +	config = msm_host->dll[index].ddr_config;
> +	writel_relaxed(config, ioaddr + ddr_cfg_offset);
> +
> +	/* Set DLL_CONFIG_2 */
> +	if (msm_host->use_14lpp_dll_reset) {
> +		u32 mclk_freq;
> +		int cycle_cnt;
> +
> +		/*
> +		 * Only configure the mclk_freq in normal DLL init
> +		 * context. If the DLL init is coming from
> +		 * CX Collapse Exit context, the host->clock may be zero.
> +		 * The DLL_CONFIG_2 register has already been restored to
> +		 * proper value prior to getting here.
> +		 */
> +		if (init_context == DLL_INIT_NORMAL) {

Guess what...enum mode only has a single possible value, so init_context
is DLL_INIT_NORMAL...

> +			cycle_cnt = readl_relaxed(ioaddr +
> +					msm_offset->core_dll_config_2)
> +					& CORE_FLL_CYCLE_CNT ? 8 : 4;
> +
> +			mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
> +
> +			if (dll_clock < 100000000) {
> +				pr_err("%s: %s: Non standard clk freq =%u\n",
> +				       mmc_hostname(mmc), __func__, dll_clock);
> +				rc = -EINVAL;
> +				goto out;
> +			}
> +
> +			config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> +			config = (config & ~GENMASK(17, 10)) |
> +					FIELD_PREP(GENMASK(17, 10), mclk_freq);
> +			writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> +		}
> +		/* wait for 5us before enabling DLL clock */
> +		udelay(5);
> +	}
> +
> +	config = msm_host->dll[index].dll_config;
> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> +	/* Wait for 52us */
> +	spin_unlock_irqrestore(&host->lock, flags);

How come this isn't racy? What is the purpose of holding this spinlock
throughout this method and are you certain that whatever concurrent
execution might happen doesn't happen here?

> +	usleep_range(60, 70);
> +	spin_lock_irqsave(&host->lock, flags);
> +
> +	/*
> +	 * Write 0 to DLL_RST bit of DLL_CONFIG register
> +	 * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
> +	 */
> +	config &= ~CORE_DLL_RST;
> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> +	config &= ~CORE_DLL_PDN;
> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +	/* Write 1 to DLL_RST bit of DLL_CONFIG register */
> +	config |= CORE_DLL_RST;
> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> +	/* Write 0 to DLL_RST bit of DLL_CONFIG register */
> +	config &= ~CORE_DLL_RST;
> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> +	/* Set CORE_DLL_CLOCK_DISABLE to 0 */
> +	if (msm_host->use_14lpp_dll_reset) {
> +		config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
> +		config &= ~CORE_DLL_CLOCK_DISABLE;
> +		writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
> +	}
> +
> +	/* Set DLL_EN bit to 1. */
> +	config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
> +	config |= CORE_DLL_EN;
> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> +	/*
> +	 * Wait for 8000 input clock. Here we calculate the
> +	 * delay from fixed clock freq 192MHz, which turns out 42us.
> +	 */
> +	spin_unlock_irqrestore(&host->lock, flags);

As above, how do we know that this isn't racy?

> +	usleep_range(50, 60);
> +	spin_lock_irqsave(&host->lock, flags);
> +
> +	/* Set CK_OUT_EN bit to 1. */
> +	config |= CORE_CK_OUT_EN;
> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
> +
> +	/*
> +	 * Wait until DLL_LOCK bit of DLL_STATUS register
> +	 * becomes '1'.
> +	 */
> +	rc = readl_relaxed_poll_timeout(ioaddr +
> +					msm_offset->core_dll_status,
> +					dll_lock,
> +					dll_lock & CORE_DLL_LOCK,
> +					10,
> +					100);
> +	if (rc == -ETIMEDOUT)
> +		pr_err("%s: Unable to get DLL_LOCK, dll_status: 0x%08x\n",

Don't pr_err() when you have a struct device.

> +		       mmc_hostname(host->mmc), dll_lock);
> +
> +out:
> +	if (core_vendor_spec & CORE_CLK_PWRSAVE) {
> +		/* Reenable PWRSAVE as needed */
> +		config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
> +		config |= CORE_CLK_PWRSAVE;
> +		writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
> +	}
> +
> +	spin_unlock_irqrestore(&host->lock, flags);
> +	return rc;
> +}
> +
>  static void msm_hc_select_default(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -927,6 +1139,24 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
>  		msm_hc_select_default(host);
>  }
>  
> +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
> +{
> +	if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104 ||
> +	    host->mmc->ios.timing == MMC_TIMING_MMC_HS400)
> +		return sdhci_msm_configure_dll(host, init_context, HS400);
> +
> +	return sdhci_msm_configure_dll(host, init_context, HS200);
> +}
> +
> +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
> +
> +	return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
> +		msm_init_cm_dll(host);

So dll_config() calls init_dll() or init_cm_dll() which then might call
configure_dll(). Is it init or config?!

> +}
> +
>  static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -942,7 +1172,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>  	 * Retuning in HS400 (DDR mode) will fail, just reset the
>  	 * tuning block and restore the saved tuning phase.
>  	 */
> -	ret = msm_init_cm_dll(host);
> +	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>  	if (ret)
>  		goto out;
>  
> @@ -1030,7 +1260,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>  	return ret;
>  }
>  
> -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
> +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
>  {
>  	struct mmc_host *mmc = host->mmc;
>  	u32 dll_status, config, ddr_cfg_offset;
> @@ -1053,7 +1283,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>  		ddr_cfg_offset = msm_offset->core_ddr_config;
>  	else
>  		ddr_cfg_offset = msm_offset->core_ddr_config_old;
> -	writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
> +
> +	if (msm_host->artanis_dll)
> +		writel_relaxed(msm_host->dll[index].ddr_config, host->ioaddr + ddr_cfg_offset);
> +	else
> +		writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>  
>  	if (mmc->ios.enhanced_strobe) {
>  		config = readl_relaxed(host->ioaddr +
> @@ -1122,7 +1356,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>  	 * Retuning in HS400 (DDR mode) will fail, just reset the
>  	 * tuning block and restore the saved tuning phase.
>  	 */
> -	ret = msm_init_cm_dll(host);
> +	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> +
>  	if (ret)
>  		goto out;
>  
> @@ -1142,7 +1377,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>  	if (msm_host->use_cdclp533)
>  		ret = sdhci_msm_cdclp533_calibration(host);
>  	else
> -		ret = sdhci_msm_cm_dll_sdc4_calibration(host);
> +		ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
>  out:
>  	pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
>  		 __func__, ret);
> @@ -1185,7 +1420,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
>  		return 0;
>  
>  	/* Reset the tuning block */
> -	ret = msm_init_cm_dll(host);
> +	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> +
>  	if (ret)
>  		return ret;
>  
> @@ -1227,6 +1463,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>  	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
>  	u32 config;
> +	bool hs400_tuning;
>  
>  	if (!sdhci_msm_is_tuning_needed(host)) {
>  		msm_host->use_cdr = false;
> @@ -1256,17 +1493,23 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  	 * - select MCLK/2 in VENDOR_SPEC
>  	 * - program MCLK to 400MHz (or nearest supported) in GCC
>  	 */
> -	if (host->flags & SDHCI_HS400_TUNING) {
> +	hs400_tuning = !!(host->flags & SDHCI_HS400_TUNING);

I don't see the reason for this change. If it actually impact the
behavior of the init/config dll, did you ensure that it doesn't impact
all currently supported targets?

Regards,
Bjorn

> +	if (hs400_tuning) {
>  		sdhci_msm_hc_select_mode(host);
>  		msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
> -		host->flags &= ~SDHCI_HS400_TUNING;
>  	}
>  
>  retry:
>  	/* First of all reset the tuning block */
> -	rc = msm_init_cm_dll(host);
> -	if (rc)
> +	rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
> +	if (rc) {
> +		if (hs400_tuning)
> +			host->flags &= ~SDHCI_HS400_TUNING;
>  		return rc;
> +	}
> +
> +	if (hs400_tuning)
> +		host->flags &= ~SDHCI_HS400_TUNING;
>  
>  	phase = 0;
>  	do {
> @@ -1297,6 +1540,8 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  				mmc_hostname(mmc));
>  			if (--tuning_seq_cnt) {
>  				tuned_phase_cnt = 0;
> +				if (hs400_tuning)
> +					host->flags |= SDHCI_HS400_TUNING;
>  				goto retry;
>  			}
>  		}
> @@ -1319,8 +1564,11 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  		dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n",
>  			 mmc_hostname(mmc), phase);
>  	} else {
> -		if (--tuning_seq_cnt)
> +		if (--tuning_seq_cnt) {
> +			if (hs400_tuning)
> +				host->flags |= SDHCI_HS400_TUNING;
>  			goto retry;
> +		}
>  		/* Tuning failed */
>  		dev_dbg(mmc_dev(mmc), "%s: No tuning point found\n",
>  		       mmc_hostname(mmc));
> @@ -1847,11 +2095,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
>  	return clk_round_rate(core_clk, ULONG_MAX);
>  }
>  
> -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
> -{
> -	return SDHCI_MSM_MIN_CLOCK;
> -}
> -
>  /*
>   * __sdhci_msm_set_clock - sdhci_msm clock control.
>   *
> -- 
> 2.34.1
> 

Re: [PATCH v6 5/5] mmc: sdhci-msm: Rectify DLL programming sequence for SDCC

[Ram Prakash Gupta]

On 12/18/2025 7:48 PM, Bjorn Andersson wrote:
> On Mon, Dec 15, 2025 at 05:30:09PM +0530, Ram Prakash Gupta wrote:
>> From: Sachin Gupta <quic_sachgupt@quicinc.com>
>>
>> With the current DLL sequence stability issues for data
>> transfer seen in HS400 and HS200 modes.
>>
>> "mmc0: cqhci: error IRQ status: 0x00000000 cmd error -84
>> data error 0"
>>
>> Rectify the DLL programming sequence as per latest hardware
>> programming guide
> You're talking about how things are unstable, but what the commit
> message completely fails to mention is that all this new spaghetti is
> conditional on msm_host->artanis_dll, so I believe this statement
> only applies to a single supported SoC?
>
> No where in this series do you mention that this only (?) applies to
> QDU1000. 

This DLL change is required to work with SoCs having artanis DLL.
This applies to multiple targets but QDU1000 is what is picked up to
start with, and gradually after testing this with other artanis DLL
supported target, dt change will be updated for other targets.

And this doesn't apply only to QDU1000, this would go for SM8650, SM8750
and upcoming SoCs for SDC2 instance (SDCard) and other targets for SDC1
instance if supported.

I will further update the commit message in to show, artanis dll
support addition with this work.

>> Signed-off-by: Sachin Gupta <quic_sachgupt@quicinc.com>
>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com>
>> ---
>>  drivers/mmc/host/sdhci-msm.c | 277 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 260 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
>> index 1fcd92158bee..f459b40db41c 100644
>> --- a/drivers/mmc/host/sdhci-msm.c
>> +++ b/drivers/mmc/host/sdhci-msm.c
>> @@ -28,6 +28,7 @@
>>  #define CORE_VERSION_MAJOR_SHIFT	28
>>  #define CORE_VERSION_MAJOR_MASK		(0xf << CORE_VERSION_MAJOR_SHIFT)
>>  #define CORE_VERSION_MINOR_MASK		0xff
>> +#define SDHCI_MSM_MIN_V_7FF		0x6e
>>  
>>  #define CORE_MCI_GENERICS		0x70
>>  #define SWITCHABLE_SIGNALING_VOLTAGE	BIT(29)
>> @@ -119,7 +120,8 @@
>>  #define CORE_PWRSAVE_DLL	BIT(3)
>>  
>>  #define DDR_CONFIG_POR_VAL	0x80040873
>> -
>> +#define DLL_CONFIG_3_POR_VAL	0x10
>> +#define TCXO_FREQ               19200000
> If you read the current implementation of msm_init_cm_dll() you'll see
> that we don't hard code the XO clock...

Thanks for point out.I will remove this and use xo_clock.

>
>>  
>>  #define INVALID_TUNING_PHASE	-1
>>  #define SDHCI_MSM_MIN_CLOCK	400000
>> @@ -319,6 +321,15 @@ struct sdhci_msm_host {
>>  	bool artanis_dll;
>>  };
>>  
>> +enum dll_init_context {
>> +	DLL_INIT_NORMAL,
> What is the purpose of a single-entry enum to capture the value 0?

I will remove this and make the further changes in code accordingly.

>
>> +};
>> +
>> +enum mode {
>> +	HS400, // equivalent to SDR104 mode for DLL.
>> +	HS200, // equivalent to SDR50 mode for DLL.
> This is effectively a bool, with a terribly generic name.

ok, will update to HS400_DLL_CONFIG_VAL and HS200_DLL_CONFIG_VAL if this
is ok.

>
>> +};
>> +
>>  static const struct sdhci_msm_offset *sdhci_priv_msm_offset(struct sdhci_host *host)
>>  {
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -805,6 +816,207 @@ static int msm_init_cm_dll(struct sdhci_host *host)
>>  	return 0;
>>  }
>>  
>> +static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>> +{
>> +	return SDHCI_MSM_MIN_CLOCK;
>> +}
>> +
>> +static unsigned int sdhci_msm_get_clk_rate(struct sdhci_host *host, u32 req_clk)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +	struct clk *core_clk = msm_host->bulk_clks[0].clk;
>> +	struct mmc_ios ios = host->mmc->ios;
>> +	unsigned int sup_clk;
>> +
>> +	if (req_clk < sdhci_msm_get_min_clock(host))
>> +		return sdhci_msm_get_min_clock(host);
>> +
>> +	sup_clk = clk_get_rate(core_clk);
>> +
>> +	if (ios.timing == MMC_TIMING_MMC_HS400 ||
>> +	    host->flags & SDHCI_HS400_TUNING)
>> +		sup_clk = sup_clk / 2;
>> +
>> +	return sup_clk;
> Isn't sup_clk the same as host->mmc->actual_clock? Isn't this already
> calculated for you?

Thanks for pointing out, I will use this. And remove this code, will update
this in next patchset.

>
>> +}
>> +
>> +/* Initialize the DLL (Programmable Delay Line) */
> So now we're going to have two functions that are named pretty much the
> same and look pretty similar?
>
> With the main difference that the slightly more generically named
> sdhci_msm_configure_dll() is only called if we have a artanis_dll,
> otherwise msm_init_cm_dll() is used?

They appear similar as artanis_dll have multiple common sequence
with the previous one. The new addition in the existing function
was adding multiple if/else checks making the function cluttered.
This approach keeps the implementation cleaner.

>> +static int sdhci_msm_configure_dll(struct sdhci_host *host, enum dll_init_context
>> +				 init_context, enum mode index)
>> +{
> Large parts of this function is duplicated from msm_init_cm_dll(). This
> isn't okay.

Right it may look like a duplicate but the new required sequence shares many
steps with the existing sequence but the new addition in the existing function
was adding multiple if/else checks making the function cluttered and hard to
read. To keep the code clean, a separate function was created instead.

>
>> +	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;
>> +	struct mmc_host *mmc = host->mmc;
>> +	u32 ddr_cfg_offset, core_vendor_spec, config;
>> +	void __iomem *ioaddr = host->ioaddr;
>> +	unsigned long flags, dll_clock;
>> +	int rc = 0;
>> +	u32 dll_lock;
>> +
>> +	dll_clock = sdhci_msm_get_clk_rate(host, host->clock);
>> +	spin_lock_irqsave(&host->lock, flags);
>> +
>> +	core_vendor_spec = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>> +
>> +	/*
>> +	 * Always disable PWRSAVE during the DLL power
>> +	 * up regardless of its current setting.
>> +	 */
>> +	core_vendor_spec &= ~CORE_CLK_PWRSAVE;
>> +	writel_relaxed(core_vendor_spec, ioaddr + msm_offset->core_vendor_spec);
>> +
>> +	if (msm_host->use_14lpp_dll_reset) {
>> +		/* Disable CK_OUT */
>> +		config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> +		config &= ~CORE_CK_OUT_EN;
>> +		writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> +		/* Disable the DLL clock */
>> +		config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> +		config |= CORE_DLL_CLOCK_DISABLE;
>> +		writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> +	}
>> +
>> +	/*
>> +	 * Write 1 to DLL_RST bit of DLL_CONFIG register
>> +	 * and Write 1 to DLL_PDN bit of DLL_CONFIG register.
>> +	 */
>> +	config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> +	config |= (CORE_DLL_RST | CORE_DLL_PDN);
>> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> +	/*
>> +	 * Configure DLL_CONFIG_3 and USER_CTRL
>> +	 * (Only applicable for 7FF projects).
> What is a 7FF project?

This refers to 7nm technode.

>
>> +	 */
>> +	if (msm_host->core_minor >= SDHCI_MSM_MIN_V_7FF) {
>> +		writel_relaxed(msm_host->dll[index].dll_config_3,
>> +			       ioaddr + msm_offset->core_dll_config_3);
>> +		writel_relaxed(msm_host->dll[index].dll_usr_ctl,
>> +			       ioaddr + msm_offset->core_dll_usr_ctl);
>> +	}
>> +
>> +	/*
>> +	 * Set DDR_CONFIG since step 7 is setting TEST_CTRL that can be skipped.
>> +	 */
>> +	ddr_cfg_offset = msm_host->updated_ddr_cfg ? msm_offset->core_ddr_config
>> +					: msm_offset->core_ddr_config_old;
>> +
>> +	config = msm_host->dll[index].ddr_config;
>> +	writel_relaxed(config, ioaddr + ddr_cfg_offset);
>> +
>> +	/* Set DLL_CONFIG_2 */
>> +	if (msm_host->use_14lpp_dll_reset) {
>> +		u32 mclk_freq;
>> +		int cycle_cnt;
>> +
>> +		/*
>> +		 * Only configure the mclk_freq in normal DLL init
>> +		 * context. If the DLL init is coming from
>> +		 * CX Collapse Exit context, the host->clock may be zero.
>> +		 * The DLL_CONFIG_2 register has already been restored to
>> +		 * proper value prior to getting here.
>> +		 */
>> +		if (init_context == DLL_INIT_NORMAL) {
> Guess what...enum mode only has a single possible value, so init_context
> is DLL_INIT_NORMAL...

I will remove this completely and update the code.

>> +			cycle_cnt = readl_relaxed(ioaddr +
>> +					msm_offset->core_dll_config_2)
>> +					& CORE_FLL_CYCLE_CNT ? 8 : 4;
>> +
>> +			mclk_freq = DIV_ROUND_CLOSEST_ULL(dll_clock * cycle_cnt, TCXO_FREQ);
>> +
>> +			if (dll_clock < 100000000) {
>> +				pr_err("%s: %s: Non standard clk freq =%u\n",
>> +				       mmc_hostname(mmc), __func__, dll_clock);
>> +				rc = -EINVAL;
>> +				goto out;
>> +			}
>> +
>> +			config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> +			config = (config & ~GENMASK(17, 10)) |
>> +					FIELD_PREP(GENMASK(17, 10), mclk_freq);
>> +			writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> +		}
>> +		/* wait for 5us before enabling DLL clock */
>> +		udelay(5);
>> +	}
>> +
>> +	config = msm_host->dll[index].dll_config;
>> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> +	/* Wait for 52us */
>> +	spin_unlock_irqrestore(&host->lock, flags);
> How come this isn't racy? What is the purpose of holding this spinlock
> throughout this method and are you certain that whatever concurrent
> execution might happen doesn't happen here?

Since the delay needed is 60u, usleep_range was added, and to avoid sleep
holding lock, unlock and lock were used. And I got your point. But is it ok
to use udelay here for 60u second? I don't see any other way.

>
>> +	usleep_range(60, 70);
>> +	spin_lock_irqsave(&host->lock, flags);
>> +
>> +	/*
>> +	 * Write 0 to DLL_RST bit of DLL_CONFIG register
>> +	 * and Write 0 to DLL_PDN bit of DLL_CONFIG register.
>> +	 */
>> +	config &= ~CORE_DLL_RST;
>> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> +	config &= ~CORE_DLL_PDN;
>> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +	/* Write 1 to DLL_RST bit of DLL_CONFIG register */
>> +	config |= CORE_DLL_RST;
>> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> +	/* Write 0 to DLL_RST bit of DLL_CONFIG register */
>> +	config &= ~CORE_DLL_RST;
>> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> +	/* Set CORE_DLL_CLOCK_DISABLE to 0 */
>> +	if (msm_host->use_14lpp_dll_reset) {
>> +		config = readl_relaxed(ioaddr + msm_offset->core_dll_config_2);
>> +		config &= ~CORE_DLL_CLOCK_DISABLE;
>> +		writel_relaxed(config, ioaddr + msm_offset->core_dll_config_2);
>> +	}
>> +
>> +	/* Set DLL_EN bit to 1. */
>> +	config = readl_relaxed(ioaddr + msm_offset->core_dll_config);
>> +	config |= CORE_DLL_EN;
>> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> +	/*
>> +	 * Wait for 8000 input clock. Here we calculate the
>> +	 * delay from fixed clock freq 192MHz, which turns out 42us.
>> +	 */
>> +	spin_unlock_irqrestore(&host->lock, flags);
> As above, how do we know that this isn't racy?

Will update as per other review comment about race.

>
>> +	usleep_range(50, 60);
>> +	spin_lock_irqsave(&host->lock, flags);
>> +
>> +	/* Set CK_OUT_EN bit to 1. */
>> +	config |= CORE_CK_OUT_EN;
>> +	writel_relaxed(config, ioaddr + msm_offset->core_dll_config);
>> +
>> +	/*
>> +	 * Wait until DLL_LOCK bit of DLL_STATUS register
>> +	 * becomes '1'.
>> +	 */
>> +	rc = readl_relaxed_poll_timeout(ioaddr +
>> +					msm_offset->core_dll_status,
>> +					dll_lock,
>> +					dll_lock & CORE_DLL_LOCK,
>> +					10,
>> +					100);
>> +	if (rc == -ETIMEDOUT)
>> +		pr_err("%s: Unable to get DLL_LOCK, dll_status: 0x%08x\n",
> Don't pr_err() when you have a struct device.

ok, will update to dev_err.

>
>> +		       mmc_hostname(host->mmc), dll_lock);
>> +
>> +out:
>> +	if (core_vendor_spec & CORE_CLK_PWRSAVE) {
>> +		/* Reenable PWRSAVE as needed */
>> +		config = readl_relaxed(ioaddr + msm_offset->core_vendor_spec);
>> +		config |= CORE_CLK_PWRSAVE;
>> +		writel_relaxed(config, ioaddr + msm_offset->core_vendor_spec);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&host->lock, flags);
>> +	return rc;
>> +}
>> +
>>  static void msm_hc_select_default(struct sdhci_host *host)
>>  {
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -927,6 +1139,24 @@ static void sdhci_msm_hc_select_mode(struct sdhci_host *host)
>>  		msm_hc_select_default(host);
>>  }
>>  
>> +static int sdhci_msm_init_dll(struct sdhci_host *host, enum dll_init_context init_context)
>> +{
>> +	if (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104 ||
>> +	    host->mmc->ios.timing == MMC_TIMING_MMC_HS400)
>> +		return sdhci_msm_configure_dll(host, init_context, HS400);
>> +
>> +	return sdhci_msm_configure_dll(host, init_context, HS200);
>> +}
>> +
>> +static int sdhci_msm_dll_config(struct sdhci_host *host, enum dll_init_context init_context)
>> +{
>> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>> +
>> +	return msm_host->artanis_dll ? sdhci_msm_init_dll(host, init_context) :
>> +		msm_init_cm_dll(host);
> So dll_config() calls init_dll() or init_cm_dll() which then might call
> configure_dll(). Is it init or config?!

init is what is needed, but I see this can be improved.
I will use dll_config -> init_dll, will remove the other function calls.

>
>> +}
>> +
>>  static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>>  {
>>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -942,7 +1172,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>>  	 * Retuning in HS400 (DDR mode) will fail, just reset the
>>  	 * tuning block and restore the saved tuning phase.
>>  	 */
>> -	ret = msm_init_cm_dll(host);
>> +	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>>  	if (ret)
>>  		goto out;
>>  
>> @@ -1030,7 +1260,7 @@ static int sdhci_msm_cdclp533_calibration(struct sdhci_host *host)
>>  	return ret;
>>  }
>>  
>> -static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>> +static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host, enum mode index)
>>  {
>>  	struct mmc_host *mmc = host->mmc;
>>  	u32 dll_status, config, ddr_cfg_offset;
>> @@ -1053,7 +1283,11 @@ static int sdhci_msm_cm_dll_sdc4_calibration(struct sdhci_host *host)
>>  		ddr_cfg_offset = msm_offset->core_ddr_config;
>>  	else
>>  		ddr_cfg_offset = msm_offset->core_ddr_config_old;
>> -	writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>> +
>> +	if (msm_host->artanis_dll)
>> +		writel_relaxed(msm_host->dll[index].ddr_config, host->ioaddr + ddr_cfg_offset);
>> +	else
>> +		writel_relaxed(msm_host->ddr_config, host->ioaddr + ddr_cfg_offset);
>>  
>>  	if (mmc->ios.enhanced_strobe) {
>>  		config = readl_relaxed(host->ioaddr +
>> @@ -1122,7 +1356,8 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>>  	 * Retuning in HS400 (DDR mode) will fail, just reset the
>>  	 * tuning block and restore the saved tuning phase.
>>  	 */
>> -	ret = msm_init_cm_dll(host);
>> +	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> +
>>  	if (ret)
>>  		goto out;
>>  
>> @@ -1142,7 +1377,7 @@ static int sdhci_msm_hs400_dll_calibration(struct sdhci_host *host)
>>  	if (msm_host->use_cdclp533)
>>  		ret = sdhci_msm_cdclp533_calibration(host);
>>  	else
>> -		ret = sdhci_msm_cm_dll_sdc4_calibration(host);
>> +		ret = sdhci_msm_cm_dll_sdc4_calibration(host, HS400);
>>  out:
>>  	pr_debug("%s: %s: Exit, ret %d\n", mmc_hostname(host->mmc),
>>  		 __func__, ret);
>> @@ -1185,7 +1420,8 @@ static int sdhci_msm_restore_sdr_dll_config(struct sdhci_host *host)
>>  		return 0;
>>  
>>  	/* Reset the tuning block */
>> -	ret = msm_init_cm_dll(host);
>> +	ret = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> +
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -1227,6 +1463,7 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>  	struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>  	const struct sdhci_msm_offset *msm_offset = msm_host->offset;
>>  	u32 config;
>> +	bool hs400_tuning;
>>  
>>  	if (!sdhci_msm_is_tuning_needed(host)) {
>>  		msm_host->use_cdr = false;
>> @@ -1256,17 +1493,23 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>  	 * - select MCLK/2 in VENDOR_SPEC
>>  	 * - program MCLK to 400MHz (or nearest supported) in GCC
>>  	 */
>> -	if (host->flags & SDHCI_HS400_TUNING) {
>> +	hs400_tuning = !!(host->flags & SDHCI_HS400_TUNING);
> I don't see the reason for this change. If it actually impact the
> behavior of the init/config dll, did you ensure that it doesn't impact
> all currently supported targets?
>
> Regards,
> Bjorn

I had changed this to get right sup_clk value by retaining the tuning flag,
but will use actual_clk as suggested in top review comment and this change won't
be required anymore.

Thanks,
Ram

>
>> +	if (hs400_tuning) {
>>  		sdhci_msm_hc_select_mode(host);
>>  		msm_set_clock_rate_for_bus_mode(host, ios.clock, ios.timing);
>> -		host->flags &= ~SDHCI_HS400_TUNING;
>>  	}
>>  
>>  retry:
>>  	/* First of all reset the tuning block */
>> -	rc = msm_init_cm_dll(host);
>> -	if (rc)
>> +	rc = sdhci_msm_dll_config(host, DLL_INIT_NORMAL);
>> +	if (rc) {
>> +		if (hs400_tuning)
>> +			host->flags &= ~SDHCI_HS400_TUNING;
>>  		return rc;
>> +	}
>> +
>> +	if (hs400_tuning)
>> +		host->flags &= ~SDHCI_HS400_TUNING;
>>  
>>  	phase = 0;
>>  	do {
>> @@ -1297,6 +1540,8 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>  				mmc_hostname(mmc));
>>  			if (--tuning_seq_cnt) {
>>  				tuned_phase_cnt = 0;
>> +				if (hs400_tuning)
>> +					host->flags |= SDHCI_HS400_TUNING;
>>  				goto retry;
>>  			}
>>  		}
>> @@ -1319,8 +1564,11 @@ static int sdhci_msm_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>  		dev_dbg(mmc_dev(mmc), "%s: Setting the tuning phase to %d\n",
>>  			 mmc_hostname(mmc), phase);
>>  	} else {
>> -		if (--tuning_seq_cnt)
>> +		if (--tuning_seq_cnt) {
>> +			if (hs400_tuning)
>> +				host->flags |= SDHCI_HS400_TUNING;
>>  			goto retry;
>> +		}
>>  		/* Tuning failed */
>>  		dev_dbg(mmc_dev(mmc), "%s: No tuning point found\n",
>>  		       mmc_hostname(mmc));
>> @@ -1847,11 +2095,6 @@ static unsigned int sdhci_msm_get_max_clock(struct sdhci_host *host)
>>  	return clk_round_rate(core_clk, ULONG_MAX);
>>  }
>>  
>> -static unsigned int sdhci_msm_get_min_clock(struct sdhci_host *host)
>> -{
>> -	return SDHCI_MSM_MIN_CLOCK;
>> -}
>> -
>>  /*
>>   * __sdhci_msm_set_clock - sdhci_msm clock control.
>>   *
>> -- 
>> 2.34.1
>>

Re: [PATCH v6 0/5] mmc: sdhci-msm: Rectify DLL programming sequence

[Krzysztof Kozlowski]

On 15/12/2025 13:00, Ram Prakash Gupta wrote:
> With the current DLL sequence stability issues are seen in
> HS400 and HS200 mode for data transfers.
> 
> Rectify the DLL programming sequence as per latest hardware
> programming guide and also incorporate support for HS200 and
> HS400 DLL settings using the device tree.
> 

Please trim your CC list. You cc-ed 4 internal Qualcomm addresses and
also internal mailing list (kernel@oss) which you are not suppose to mix
with upstream!

Best regards,
Krzysztof

Re: [PATCH v6 0/5] mmc: sdhci-msm: Rectify DLL programming sequence

[Ram Prakash Gupta]

On 12/15/2025 7:28 PM, Krzysztof Kozlowski wrote:
> On 15/12/2025 13:00, Ram Prakash Gupta wrote:
>> With the current DLL sequence stability issues are seen in
>> HS400 and HS200 mode for data transfers.
>>
>> Rectify the DLL programming sequence as per latest hardware
>> programming guide and also incorporate support for HS200 and
>> HS400 DLL settings using the device tree.
>>
> Please trim your CC list. You cc-ed 4 internal Qualcomm addresses and
> also internal mailing list (kernel@oss) which you are not suppose to mix
> with upstream!
>
> Best regards,
> Krzysztof

ok. I will remove.

Thanks,
Ram