Message-ID: <06d201dc0689$9f438200$ddca8600$@samsung.com>
Date: Wed, 6 Aug 2025 09:51:43 +0530
From: "Alim Akhtar" <alim.akhtar@...sung.com>
To: "'Konrad Dybcio'" <konrad.dybcio@....qualcomm.com>, "'Manivannan
 Sadhasivam'" <mani@...nel.org>
Cc: "'Krzysztof Kozlowski'" <krzk@...nel.org>, "'Ram Kumar Dwivedi'"
	<quic_rdwivedi@...cinc.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 2/3] arm64: dts: qcom: sa8155: Add gear and rate limit
 properties to UFS



> -----Original Message-----
> From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> Sent: Tuesday, August 5, 2025 11:57 PM
> To: Alim Akhtar <alim.akhtar@...sung.com>; 'Manivannan Sadhasivam'
> <mani@...nel.org>
> Cc: 'Krzysztof Kozlowski' <krzk@...nel.org>; 'Ram Kumar Dwivedi'
> <quic_rdwivedi@...cinc.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 2/3] arm64: dts: qcom: sa8155: Add gear and rate limit
> properties to UFS
> 
> On 8/5/25 7:52 PM, Alim Akhtar wrote:
> >
> >
> >> -----Original Message-----
> >> From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> >> Sent: Tuesday, August 5, 2025 11:10 PM
> >> To: Alim Akhtar <alim.akhtar@...sung.com>; 'Manivannan Sadhasivam'
> >> <mani@...nel.org>
> >> Cc: 'Krzysztof Kozlowski' <krzk@...nel.org>; 'Ram Kumar Dwivedi'
> >> <quic_rdwivedi@...cinc.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 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@...nel.org>
> >>>> Sent: Tuesday, August 5, 2025 10:52 PM
> >>>> To: Alim Akhtar <alim.akhtar@...sung.com>
> >>>> Cc: 'Konrad Dybcio' <konrad.dybcio@....qualcomm.com>; 'Krzysztof
> >>>> Kozlowski' <krzk@...nel.org>; 'Ram Kumar Dwivedi'
> >>>> <quic_rdwivedi@...cinc.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 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@....qualcomm.com>
> >>>>>> Sent: Tuesday, August 5, 2025 10:36 PM
> >>>>>> To: Manivannan Sadhasivam <mani@...nel.org>
> >>>>>> Cc: Krzysztof Kozlowski <krzk@...nel.org>; Ram Kumar Dwivedi
> >>>>>> <quic_rdwivedi@...cinc.com>; 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 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
> 
But the solution for all possible board problems can't be by limiting Gear speed.
So it should be known why one particular board need to limit the gear.
I understand that this is a static configuration, where it is already known that board is broken for higher Gear.
Can this be achieved by limiting the clock? If not, can we add a board specific _quirk_ and let the _quirk_ to be enabled from vendor specific hooks? 

> Konrad

Powered by blists - more mailing lists