Message-ID: <728cde88-1601-4c36-976a-3c934a64be35@quicinc.com>
Date: Thu, 31 Jul 2025 21:58:47 +0530
From: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
CC: <mani@...nel.org>, <alim.akhtar@...sung.com>, <avri.altman@....com>,
        <bvanassche@....org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>, <andersson@...nel.org>,
        <konradybcio@...nel.org>, <James.Bottomley@...senpartnership.com>,
        <martin.petersen@...cle.com>, <agross@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] ufs: ufs-qcom: Add support for DT-based gear and rate
 limiting



On 24-Jul-25 2:11 PM, Dmitry Baryshkov wrote:
> On Thu, 24 Jul 2025 at 10:35, Ram Kumar Dwivedi
> <quic_rdwivedi@...cinc.com> wrote:
>>
>>
>>
>> On 23-Jul-25 12:24 AM, Dmitry Baryshkov wrote:
>>> On Tue, Jul 22, 2025 at 09:41:01PM +0530, Ram Kumar Dwivedi wrote:
>>>> Add optional device tree properties to limit Tx/Rx gear and rate during UFS
>>>> initialization. Parse these properties in ufs_qcom_init() and apply them to
>>>> host->host_params to enforce platform-specific constraints.
>>>>
>>>> Use this mechanism to cap the maximum gear or rate on platforms with
>>>> hardware limitations, such as those required by some automotive customers
>>>> using SA8155. Preserve the default behavior if the properties are not
>>>> specified in the device tree.
>>>>
>>>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>
>>>> ---
>>>>  drivers/ufs/host/ufs-qcom.c | 28 ++++++++++++++++++++++------
>>>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index 4bbe4de1679b..5e7fd3257aca 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> @@ -494,12 +494,8 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>>>>       * If the HS-G5 PHY gear is used, update host_params->hs_rate to Rate-A,
>>>>       * so that the subsequent power mode change shall stick to Rate-A.
>>>>       */
>>>> -    if (host->hw_ver.major == 0x5) {
>>>> -            if (host->phy_gear == UFS_HS_G5)
>>>> -                    host_params->hs_rate = PA_HS_MODE_A;
>>>> -            else
>>>> -                    host_params->hs_rate = PA_HS_MODE_B;
>>>> -    }
>>>> +    if (host->hw_ver.major == 0x5 && host->phy_gear == UFS_HS_G5)
>>>> +            host_params->hs_rate = PA_HS_MODE_A;
>>>
>>> Why? This doesn't seem related.
>>
>> Hi Dmitry,
>>
>> I have refactored the patch to put this part in a separate patch in latest patchset.
>>
>> Thanks,
>> Ram.
>>
>>>
>>>>
>>>>      mode = host_params->hs_rate == PA_HS_MODE_B ? PHY_MODE_UFS_HS_B : PHY_MODE_UFS_HS_A;
>>>>
>>>> @@ -1096,6 +1092,25 @@ static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host)
>>>>      }
>>>>  }
>>>>
>>>> +static void ufs_qcom_parse_limits(struct ufs_qcom_host *host)
>>>> +{
>>>> +    struct ufs_host_params *host_params = &host->host_params;
>>>> +    struct device_node *np = host->hba->dev->of_node;
>>>> +    u32 hs_gear, hs_rate = 0;
>>>> +
>>>> +    if (!np)
>>>> +            return;
>>>> +
>>>> +    if (!of_property_read_u32(np, "limit-hs-gear", &hs_gear)) {
>>>
>>> These are generic properties, so they need to be handled in a generic
>>> code path.
>>
>> Hi Dmitry,
>>
>>
>> Below is the probe path for the UFS-QCOM platform driver:
>>
>> ufs_qcom_probe
>>   └─ ufshcd_platform_init
>>        └─ ufshcd_init
>>             └─ ufs_qcom_init
>>                  └─ ufs_qcom_set_host_params
>>                       └─ ufshcd_init_host_params (initialized with default values)
>>                            └─ ufs_qcom_get_hs_gear (overrides gear based on controller capability)
>>                                 └─ ufs_qcom_set_phy_gear (further overrides based on controller limitations)
>>
>>
>> The reason I added the logic in ufs-qcom.c is that even if it's placed in ufshcd-platform.c, the values get overridden in ufs-qcom.c.
>> If you prefer, I can move the parsing logic API to ufshcd-platform.c but still it needs to be called from ufs-qcom.c.
> 
> I was thinking about ufshcd_init() or similar function.
Hi Dmitry,

It appears we can't move the logic to ufshcd.c because the PHY is initialized in ufs-qcom.c, and the gear must be set during that initialization.

Limiting the gear and lane in ufshcd.c won’t be effective, as qcom_init sets the PHY to the maximum supported gear by default. As a result, the PHY would still initialize with the max gear, defeating the purpose of the limits.

To ensure the PHY is initialized with the intended gear, the limits needs to be applied directly in ufs_qcom.c

Please let me know if you have any concerns.

Thanks,
Ram.


> 
>>
>> Thanks,
>> Ram.
>>
>>
>>>
>>> Also, the patch with bindings should preceed driver and DT changes.
>>
>> Hi Dmitry,
>>
>> I have reordered the patch series to place the DT binding change as the first patch in latest patchset.
>>
>> Thanks,
>> Ram.
>>
>>
>>>
>>>> +            host_params->hs_tx_gear = hs_gear;
>>>> +            host_params->hs_rx_gear = hs_gear;
>>>> +            host->phy_gear = hs_gear;
>>>> +    }
>>>> +
>>>> +    if (!of_property_read_u32(np, "limit-rate", &hs_rate))
>>>> +            host_params->hs_rate = hs_rate;
>>>> +}
>>>> +
>>>>  static void ufs_qcom_set_host_params(struct ufs_hba *hba)
>>>>  {
>>>>      struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>>> @@ -1337,6 +1352,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>>>>      ufs_qcom_advertise_quirks(hba);
>>>>      ufs_qcom_set_host_params(hba);
>>>>      ufs_qcom_set_phy_gear(host);
>>>> +    ufs_qcom_parse_limits(host);
>>>>
>>>>      err = ufs_qcom_ice_init(host);
>>>>      if (err)
>>>> --
>>>> 2.50.1
>>>>
>>>
>>
> 
> 

Powered by blists - more mailing lists