RE: [PATCH 2/3] arm64: dts: qcom: sa8155: Add gear and rate limit properties to UFS

["Alim Akhtar" <alim.akhtar@samsung.com>]

> -----Original Message-----
> From: 'Manivannan Sadhasivam' <mani@kernel.org>
> Sent: Friday, August 8, 2025 11:37 PM
> To: Alim Akhtar <alim.akhtar@samsung.com>
> Cc: 'Konrad Dybcio' <konrad.dybcio@oss.qualcomm.com>; 'Krzysztof
> Kozlowski' <krzk@kernel.org>; 'Ram Kumar Dwivedi'
> <quic_rdwivedi@quicinc.com>; avri.altman@wdc.com;
> bvanassche@acm.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; andersson@kernel.org; konradybcio@kernel.org;
> James.Bottomley@hansenpartnership.com; martin.petersen@oracle.com;
> agross@kernel.org; linux-arm-msm@vger.kernel.org; linux-
> scsi@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] arm64: dts: qcom: sa8155: Add gear and rate limit
> properties to UFS
> 
> On Fri, Aug 08, 2025 at 08:38:09PM GMT, Alim Akhtar wrote:
> >
> >
> > > -----Original Message-----
> > > From: 'Manivannan Sadhasivam' <mani@kernel.org>
> > > Sent: Friday, August 8, 2025 6:14 PM
> > > To: Alim Akhtar <alim.akhtar@samsung.com>
> > > Cc: 'Konrad Dybcio' <konrad.dybcio@oss.qualcomm.com>; 'Krzysztof
> > > Kozlowski' <krzk@kernel.org>; 'Ram Kumar Dwivedi'
> > > <quic_rdwivedi@quicinc.com>; avri.altman@wdc.com;
> > > bvanassche@acm.org; robh@kernel.org; krzk+dt@kernel.org;
> > > conor+dt@kernel.org; andersson@kernel.org; konradybcio@kernel.org;
> > > James.Bottomley@hansenpartnership.com;
> martin.petersen@oracle.com;
> > > agross@kernel.org; linux-arm-msm@vger.kernel.org; linux-
> > > scsi@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH 2/3] arm64: dts: qcom: sa8155: Add gear and rate
> > > limit properties to UFS
> > >
> > > On Thu, Aug 07, 2025 at 10:08:32PM GMT, Alim Akhtar wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: 'Manivannan Sadhasivam' <mani@kernel.org>
> > > > > Sent: Wednesday, August 6, 2025 4:56 PM
> > > > > To: Alim Akhtar <alim.akhtar@samsung.com>
> > > > > Cc: 'Konrad Dybcio' <konrad.dybcio@oss.qualcomm.com>; 'Krzysztof
> > > > [...]
> > > >
> > > > > > >
> > > > > > > On Wed, Aug 06, 2025 at 09:51:43AM GMT, Alim Akhtar wrote:
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > > >> 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.
> > > > > > >
> > > > > > > Devicetree properties should precisely reflect how they are
> > > > > > > relevant to the hardware. 'limiting-gear-speed' is
> > > > > > > self-explanatory that the gear speed is getting limited (for
> > > > > > > a reason), but the devicetree doesn't need to describe the
> > > > > > > *reason* itself.
> > > > > > >
> > > > > > > > So it should be known why one particular board need to
> > > > > > > > limit the
> > > gear.
> > > > > > >
> > > > > > > That goes into the description, not in the property name.
> > > > > > >
> > > > > > > > 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?
> > > > > > > >
> > > > > > >
> > > > > > > How can we limit the clock without limiting the gears? When
> > > > > > > we limit the gear/mode, both clock and power are implicitly
> limited.
> > > > > > >
> > > > > > Possibly someone need to check with designer of the SoC if
> > > > > > that is possible
> > > > > or not.
> > > > >
> > > > > It's not just clock. We need to consider reducing regulator,
> > > > > interconnect votes also. But as I said above, limiting the
> > > > > gear/mode will take care of all these parameters.
> > > > >
> > > > > > Did we already tried _quirk_? If not, why not?
> > > > > > If the board is so poorly designed and can't take care of the
> > > > > > channel loses or heat dissipation etc, Then I assumed the gear
> > > > > > negotiation between host and device should fail for the higher
> > > > > > gear and driver can have
> > > > > a re-try logic to re-init / re-try "power mode change" at the
> > > > > lower gear. Is that not possible / feasible?
> > > > > >
> > > > >
> > > > > I don't see why we need to add extra logic in the UFS driver if
> > > > > we can extract that information from DT.
> > > > >
> > > > You didn’t answer my question entirely, I am still not able to
> > > > visualised how come Linkup is happening in higher gear and then
> > > > Suddenly
> > > it is failing and we need to reduce the gear to solve that?
> > >
> > > Oh well, this is the source of confusion here. I didn't (also the
> > > patch) claim that the link up will happen with higher speed. It will
> > > most likely fail if it couldn't operate at the higher speed and
> > > that's why we need to limit it to lower gear/mode *before* bringing the
> link up.
> > >
> > Right, that's why a re-try logic to negotiate a __working__ power mode
> change can help, instead of introducing new binding for this case.
> 
> Retry logic is already in place in the ufshcd core, but with this kind of signal
> integrity issue, we cannot guarantee that it will gracefully fail and then we
> could retry. The link up *may* succeed, then it could blow up later also
> (when doing heavy I/O operations etc...). So with this non-deterministic
> behavior, we cannot rely on this logic.
> 
I would image in that case , PHY tuning / programming is not proper.

> > And that approach can be useful for many platforms.
> 
> Other platforms could also reuse the same DT properties to workaround
> similar issues.
> 
> > Anyway coming back with the same point again and again is not productive.
> > I gave my opinion and suggestions. Rest is on the maintainers.
> 
> Suggestions are always welcomed. It is important to have comments to try
> out different things instead of sticking to the proposed solution. But in my
> opinion, the retry logic is not reliable in this case. Moreover, we do have
> similar properties for other peripherals like PCIe, MMC, where the vendors
> would use DT properties to limit the speed to workaround the board issues.
> So we are not doing anything insane here.
> 
> If there are better solutions than what is proposed here, we would indeed
> like to hear.
> 
For that, more _technical_ things need to be discussed (e.g. Is it the PHY which has problem, or problem is happening at unipro level or somewhere else), 
I didn't saw any technical backing from the patch Author/Submitter
(I assume Author should be knowing a bit more in-depth then what we are assuming and discussing here). 

> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்