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 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.
And that approach can be useful for many platforms.
Anyway coming back with the same point again and again is not productive.
I gave my opinion and suggestions. Rest is on the maintainers.

> As you can see, the driver patch is parsing the limits in its
> ufs_hba_variant_ops::init() callback, which gets called during
> ufshcd_hba_init().
> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்