Message-ID: <335a0397-59e3-ad65-5e75-0bb7f399be66@quicinc.com>
Date: Thu, 28 Aug 2025 13:30:21 +0530
From: Ram Prakash Gupta <quic_rampraka@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
CC: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Sachin Gupta
	<quic_sachgupt@...cinc.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        "Ulf
 Hansson" <ulf.hansson@...aro.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-mmc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_cang@...cinc.com>,
        <quic_nguyenb@...cinc.com>, <quic_bhaskarv@...cinc.com>,
        <quic_mapa@...cinc.com>, <quic_nitirawa@...cinc.com>,
        <quic_sartgarg@...cinc.com>
Subject: Re: [PATCH V2 2/2] mmc: sdhci-msm: Rectify DLL programming sequence
 for SDCC


On 8/21/2025 7:45 PM, Ram Prakash Gupta wrote:
> On 8/9/2025 3:10 PM, Dmitry Baryshkov wrote:
>> On Thu, Aug 07, 2025 at 01:28:28AM +0530, Ram Prakash Gupta wrote:
>>> On 7/31/2025 7:49 PM, Dmitry Baryshkov wrote:
>>>> On 31/07/2025 14:46, Ram Prakash Gupta wrote:
>>>>> On 7/30/2025 11:26 PM, Dmitry Baryshkov wrote:
>>>>>> On Wed, Jul 23, 2025 at 03:43:37PM +0530, Ram Prakash Gupta wrote:
>>>>>>> On 1/22/2025 3:20 PM, Dmitry Baryshkov wrote:
>>>>>>>> On Wed, Jan 22, 2025 at 02:57:59PM +0530, Sachin Gupta wrote:
>>>>>>>>> On 1/7/2025 8:38 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Tue, Jan 07, 2025 at 11:13:32AM +0530, Sachin Gupta wrote:
>>>>>>>>>>> On 12/27/2024 12:23 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> On Thu, Dec 26, 2024 at 11:22:40AM +0530, Sachin Gupta wrote:
>>>>>>>>>>>>> On 12/19/2024 11:24 AM, Dmitry Baryshkov wrote:
>>>>>>>>>>>>>> On Wed, Dec 18, 2024 at 02:40:57PM +0530, Sachin Gupta wrote:
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +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;
>>>>>>>>>>>>>>> +    unsigned int sup_clk;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +    if (req_clk < sdhci_msm_get_min_clock(host))
>>>>>>>>>>>>>>> +        return sdhci_msm_get_min_clock(host);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +    sup_clk = clk_round_rate(core_clk, clk_get_rate(core_clk));
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +    if (host->clock != msm_host->clk_rate)
>>>>>>>>>>>>>>> +        sup_clk = sup_clk / 2;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +    return sup_clk;
>>>>>>>>>>>>>> Why?
>>>>>>>>>>>>> Sorry, I did not understand your question. Can you please explain in detail.
>>>>>>>>>>>> Please explain the maths. You get the rate from the clock, then you
>>>>>>>>>>>> round it, but it is the rate that has just been returned, so there
>>>>>>>>>>>> should be no need to round it. And after that there a division by two
>>>>>>>>>>>> for some reason. So I've asked for an explanation for that code.
>>>>>>>>>>>>
>>>>>>>>>>> clk_round_rate is used in case of over clocking issue we can round it to the
>>>>>>>>>>> usable frequency.
>>>>>>>>>> If it is a frequency _returned_ by the clock driver, why do you need to
>>>>>>>>>> round it? It sounds like that freq should be usable anyway.
>>>>>>>>>>
>>>>>>>>> I agree, rounding will be taken care by clock driver. Will remove in my next
>>>>>>>>> patch.
>>>>>>>>>
>>>>>>>>>>> Divide by 2 is used as for HS400 the tuning happens in
>>>>>>>>>>> HS200 mode only so to update the frequency to 192 Mhz.
>>>>>>>>>> Again, is it really 192 MHz? Or 19.2 MHz?
>>>>>>>>>> Also if it is for HS400, then shouldn't /2 be limited to that mode?
>>>>>>>>>>
>>>>>>>>> Yes, It is 192 MHz.
>>>>>>>> Good, thanks for the confirmation.
>>>>>>>>
>>>>>>>>> As part of eMMC Init, driver will try to init with the best mode supported
>>>>>>>>> by controller and device. In this case it is HS400 mode, But as part of
>>>>>>>>> HS400 mode, we perform Tuning in HS200 mode only where we need to configure
>>>>>>>>> half of the clock.
>>>>>>>> This isn't an answer to the question. Let me rephrase it for you: if the
>>>>>>>> /2 is only used for HS400, why should it be attempted in all other
>>>>>>>> modes? Please limit the /2 just to HS400.
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> like updated earlier by Sachin, HS400 tuning happens in HS200 mode, so if
>>>>>>> we try to use "ios->timing == MMC_TIMING_MMC_HS400" that wont help, as at
>>>>>>> this stage timing can be MMC_TIMING_MMC_HS200/MMC_TIMING_MMC_HS400 for
>>>>>>> hs200 tuning and hs400 selection. In this case we must divide clk by 2
>>>>>>> to get 192MHz and we find this as host->clock wont be equal to
>>>>>>> msm_host->clk_rate.
>>>>>> What are host->clock and msm_host->clk_rate at this point?
>>>>>> What is the host->flags value? See sdhci_msm_hc_select_mode().
>>>>> There are 2 paths which are traced to this function when card initializes
>>>>> in HS400 mode, please consider below call stack in 2 paths
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_execute_tuning
>>>>> mmc_execute_tuning
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>>> Please check the rates explicitly in the code rather than just checking that they are not equal.
>>> in function msm_get_clock_mult_for_bus_mode(), clk multiplier returns 2, with HS400
>>> DDR52 and DDR50 modes which is called from sdhci_msm_set_clock() and
>>> sdhci_msm_execute_tuning. And in sdhci_msm_execute_tuning(), we are calling
>>> sdhci_msm_dll_config() when SDHCI_HS400_TUNING is set and this flag is cleared
>>> immediately after return. And sdhci_msm_dll_config() is called after that.
>>>
>>> Now when the card is supporting HS400 mode, then from mmc_hs200_tuning(),
>>> sdhci_prepare_hs400_tuning is getting called, and there SDHCI_HS400_TUNING
>>> flag is set, and clock set is multiplying the clk rate by 2 in below call stack
>>>
>>> msm_set_clock_rate_for_bus_mode
>>> sdhci_msm_execute_tuning
>>> mmc_execute_tuning
>>> mmc_init_card
>>>
>>> so this clk rate is doubling only with HS400 mode selection and while setting up
>>> dll in HS400 dll configuration path sup_clk need to divide by 2 as msm_host->clk_rate
>>> is twice the host->clock as mentioned above.
>> I don't see how it's relevant. I'm asking you to check for the rate
>> values explicitly in the driver code rather than just checking that
>> rateA != rateB. You might error out if rateA != rateB and they are not
>> 192 MHz / 384 MHz
> ok I see, but I got one another alternate to this, I can try use similar checks used in
> function msm_get_clock_mult_for_bus_mode(), ios.timing == MMC_TIMING_MMC_HS400 
> host->flags & SDHCI_HS400_TUNING, but for this I ll have to move check
> host->flags &= ~SDHCI_HS400_TUNING in function sdhci_msm_execute_tuning () at the bottom
> of this function, this will make code more readable and consistent to checks at other
> places but I am testing it right now and will update.
>
> If this doesn't work then I ll explicitly use the rate value in check.
>
> Thanks,
> Ram

Hi Dmitry,

adding checks for ios.timing == MMC_TIMING_MMC_HS400 && host->flags & SDHCI_HS400_TUNING
serves the same purpose for dividing the clk when mode is hs400 and hs400_tuning flag is
set, and clk value checks can be avoided now.

With this HS200, HS400, HS400ES modes of eMMC is tested.

so if this approach looks good to you, then I would like to proceed with next patchset.

Thanks,
Ram

>>>>> and host->flags as 0x90c6.
>>>>>
>>>>> and
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_set_uhs_signaling
>>>>> sdhci_set_ios
>>>>> mmc_set_clock
>>>>> mmc_set_bus_speed
>>>>> mmc_select_hs400
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 400000000
>>>>> and host->flags as 0x90c6 which are same as 1st.
>>>>>
>>>>> Now if card is initialized in HS200 mode only below is the call stack
>>>>>
>>>>> sdhci_msm_configure_dll
>>>>> sdhci_msm_dll_config
>>>>> sdhci_msm_execute_tuning
>>>>> mmc_execute_tuning
>>>>> mmc_init_card
>>>>> _mmc_resume
>>>>> mmc_runtime_resume
>>>>>
>>>>> with values of host->clock as 200000000 & msm_host-clk_rate as 200000000
>>>>> and host->flags as 0x90c6.
>>>>>
>>>>> now if you see the host->flags value, its same across the modes, and if
>>>>> I am getting it right from the pointed out function
>>>>> sdhci_msm_hc_select_mode(), your suggestion seems to be using the check
>>>>> host->flags & SDHCI_HS400_TUNING which is bit[13], but in above dumped
>>>>> host->flags SDHCI_HS400_TUNING bit is not set where we are using the /2.
>>>>>
>>>>> and the reason is, this bit is getting cleared in sdhci_msm_execute_tuning()
>>>>> before sdhci_msm_dll_config() call.
>>>>>
>>>>> so this /2, is eventually called only for HS400 mode.
>>>>>
>>>>> Thanks,
>>>>> Ram
>>>>>
>>>>>>> Now if we go for only HS200 mode supported card, there
>>>>>>> the supported clock value would be 192Mhz itself and we need to pass
>>>>>>> clk freq as 192MHz itself, hence division by 2 wont be needed, that is
>>>>>>> achieved there as host->clock would be equal to msm_host->clk_rate. Hence
>>>>>>> no other check is needed here.
>>>>>> Please think about the cause, not about the symptom. Clocks being
>>>>>> unequal is a result of some other checks being performed by the driver.
>>>>>> Please use those checks too.
>>>>>>
>>>>>>> sorry for it took time to update as I was gathering all this data.
>>>>>> 6 months? Well, that's a nice time to "gather all this data".
>>>>> Took it up from sachin last month but still its a long gap.
>>>>> Thanks for helping revive.
>>>>>
>>>>>>> since Sachin have already pushed patchset #3, and if this explanation
>>>>>>> helps, let me know if we can continue on patchset #3.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ram
>>>>>>>

Powered by blists - more mailing lists