On 8/5/25 7:52 PM, Alim Akhtar wrote: > > >> -----Original Message----- >> From: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx> >> Sent: Tuesday, August 5, 2025 11:10 PM >> To: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>; 'Manivannan Sadhasivam' >> <mani@xxxxxxxxxx> >> Cc: 'Krzysztof Kozlowski' <krzk@xxxxxxxxxx>; 'Ram Kumar Dwivedi' >> <quic_rdwivedi@xxxxxxxxxxx>; avri.altman@xxxxxxx; >> bvanassche@xxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; >> conor+dt@xxxxxxxxxx; andersson@xxxxxxxxxx; konradybcio@xxxxxxxxxx; >> James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; martin.petersen@xxxxxxxxxx; >> agross@xxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux- >> scsi@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- >> kernel@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH 2/3] arm64: dts: qcom: sa8155: Add gear and rate limit >> properties to UFS >> >> On 8/5/25 7:36 PM, Alim Akhtar wrote: >>> >>> >>>> -----Original Message----- >>>> From: 'Manivannan Sadhasivam' <mani@xxxxxxxxxx> >>>> Sent: Tuesday, August 5, 2025 10:52 PM >>>> To: Alim Akhtar <alim.akhtar@xxxxxxxxxxx> >>>> Cc: 'Konrad Dybcio' <konrad.dybcio@xxxxxxxxxxxxxxxx>; 'Krzysztof >>>> Kozlowski' <krzk@xxxxxxxxxx>; 'Ram Kumar Dwivedi' >>>> <quic_rdwivedi@xxxxxxxxxxx>; avri.altman@xxxxxxx; >> bvanassche@xxxxxxx; >>>> robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; >>>> conor+dt@xxxxxxxxxx; andersson@xxxxxxxxxx; konradybcio@xxxxxxxxxx; >>>> James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; >> martin.petersen@xxxxxxxxxx; >>>> agross@xxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux- >>>> scsi@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- >>>> kernel@xxxxxxxxxxxxxxx >>>> Subject: Re: [PATCH 2/3] arm64: dts: qcom: sa8155: Add gear and rate >>>> limit properties to UFS >>>> >>>> On Tue, Aug 05, 2025 at 10:49:45PM GMT, Alim Akhtar wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx> >>>>>> Sent: Tuesday, August 5, 2025 10:36 PM >>>>>> To: Manivannan Sadhasivam <mani@xxxxxxxxxx> >>>>>> Cc: Krzysztof Kozlowski <krzk@xxxxxxxxxx>; Ram Kumar Dwivedi >>>>>> <quic_rdwivedi@xxxxxxxxxxx>; alim.akhtar@xxxxxxxxxxx; >>>>>> avri.altman@xxxxxxx; bvanassche@xxxxxxx; robh@xxxxxxxxxx; >>>>>> krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; andersson@xxxxxxxxxx; >>>>>> konradybcio@xxxxxxxxxx; James.Bottomley@xxxxxxxxxxxxxxxxxxxxx; >>>>>> martin.petersen@xxxxxxxxxx; agross@xxxxxxxxxx; linux-arm- >>>>>> msm@xxxxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; >>>>>> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx >>>>>> Subject: Re: [PATCH 2/3] arm64: dts: qcom: sa8155: Add gear and >>>>>> rate limit properties to UFS >>>>>> >>>>>> On 8/5/25 6:55 PM, Manivannan Sadhasivam wrote: >>>>>>> On Tue, Aug 05, 2025 at 03:16:33PM GMT, Konrad Dybcio wrote: >>>>>>>> On 8/1/25 2:19 PM, Manivannan Sadhasivam wrote: >>>>>>>>> On Fri, Aug 01, 2025 at 11:12:42AM GMT, Krzysztof Kozlowski >> wrote: >>>>>>>>>> On 01/08/2025 11:10, Ram Kumar Dwivedi wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On 01-Aug-25 1:58 PM, Manivannan Sadhasivam wrote: >>>>>>>>>>>> On Thu, Jul 24, 2025 at 09:48:53AM GMT, Krzysztof Kozlowski >>>> wrote: >>>>>>>>>>>>> On 22/07/2025 18:11, Ram Kumar Dwivedi wrote: >>>>>>>>>>>>>> Add optional limit-hs-gear and limit-rate properties to the >>>>>>>>>>>>>> UFS node to support automotive use cases that require >>>>>>>>>>>>>> limiting the maximum Tx/Rx HS gear and rate due to >> hardware >>>> constraints. >>>>>>>>>>>>> >>>>>>>>>>>>> What hardware constraints? This needs to be clearly >>>> documented. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Ram, both Krzysztof and I asked this question, but you never >>>>>>>>>>>> bothered to reply, but keep on responding to other comments. >>>>>>>>>>>> This won't help you to get this series merged in any form. >>>>>>>>>>>> >>>>>>>>>>>> Please address *all* review comments before posting next >>>> iteration. >>>>>>>>>>> >>>>>>>>>>> Hi Mani, >>>>>>>>>>> >>>>>>>>>>> Apologies for the delay in responding. >>>>>>>>>>> I had planned to explain the hardware constraints in the next >>>>>> patchset’s commit message, which is why I didn’t reply earlier. >>>>>>>>>>> >>>>>>>>>>> To clarify: the limitations are due to customer board designs, >>>>>>>>>>> not our >>>>>> SoC. Some boards can't support higher gear operation, hence the >>>>>> need for optional limit-hs-gear and limit-rate properties. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> That's vague and does not justify the property. You need to >>>>>>>>>> document instead hardware capabilities or characteristic. Or >>>>>>>>>> explain why they cannot. With such form I will object to your >>>>>>>>>> next >>>>>> patch. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I had an offline chat with Ram and got clarified on what these >>>>>>>>> properties >>>>>> are. >>>>>>>>> The problem here is not with the SoC, but with the board design. >>>>>>>>> On some Qcom customer designs, both the UFS controller in the >>>>>>>>> SoC and the UFS device are capable of operating at higher gears >>>>>>>>> (say >>>> G5). >>>>>>>>> But due to board constraints like poor thermal dissipation, >>>>>>>>> routing loss, the board cannot efficiently operate at the higher >>>> speeds. >>>>>>>>> >>>>>>>>> So the customers wanted a way to limit the gear speed (say G3) >>>>>>>>> and rate (say Mode-A) on the specific board DTS. >>>>>>>> >>>>>>>> I'm not necessarily saying no, but have you explored sysfs for this? >>>>>>>> >>>>>>>> I suppose it may be too late (if the driver would e.g. init the >>>>>>>> UFS at max gear/rate at probe time, it could cause havoc as it >>>>>>>> tries to load the userland).. >>>>>>>> >>>>>>> >>>>>>> If the driver tries to run with unsupported max gear speed/mode, >>>>>>> it will just crash with the error spit. >>>>>> >>>>>> OK >>>>>> >>>>>> just a couple related nits that I won't bother splitting into >>>>>> separate emails >>>>>> >>>>>> rate (mode? I'm seeing both names) should probably have dt-bindings >>>>>> defines while gear doesn't have to since they're called G<number> >>>>>> anyway, with the bindings description strongly discouraging use, >>>>>> unless absolutely necessary (e.g. in the situation we have right >>>>>> there) >>>>>> >>>>>> I'd also assume the code should be moved into the ufs-common code, >>>>>> rather than making it ufs-qcom specific >>>>>> >>>>>> Konrad >>>>> Since this is a board specific constrains and not a SoC properties, >>>>> have an >>>> option of handling this via bootloader is explored? >>>> >>>> Both board and SoC specific properties *should* be described in >>>> devicetree if they are purely describing the hardware. >>>> >>> Agreed, what I understood from above conversation is that, we are try >>> to solve a very *specific* board problem here, this does not looks like a >> generic problem to me and probably should be handled within the specific >> driver. >> >> Introducing generic solutions preemptively for problems that are simple in >> concept and can occur widely is good practice (although it's sometimes hard >> to gauge whether this is a one-off), as if the issue spreads a generic solution >> will appear at some point, but we'll have to keep supporting the odd ones as >> well >> > Ok, > I would prefer if we add a property which sounds like "poor thermal dissipation" or > "routing channel loss" rather than adding limiting UFS gear properties. > Poor thermal design or channel losses are generic enough and can happen on any board. This is exactly what I'm trying to avoid through my suggestion - one board may have poor thermal dissipation, another may have channel losses, yet another one may feature a special batch of UFS chips that will set the world on fire if instructed to attempt link training at gear 7 - they all are causes, as opposed to describing what needs to happen (i.e. what the hardware must be treated as - gear N incapable despite what can be discovered at runtime), with perhaps a comment on the side Konrad
