Re: [PATCH] arm64: dts: qcom: sm8450: delete incorrect ufs interconnect fields

[Jonathan Marek]

On 4/11/22 10:16 PM, Bjorn Andersson wrote:
> On Thu 07 Apr 17:38 CDT 2022, Jonathan Marek wrote:
>
> > On 4/7/22 5:16 PM, Krzysztof Kozlowski wrote:
> > > On 07/04/2022 21:40, Vladimir Zapolskiy wrote:
> > > > On 4/7/22 20:21, Jonathan Marek wrote:
> > > > > Upstream sm8450.dtsi has #interconnect-cells = <2>; so these are wrong.
> > > > > Ignored and undocumented with upstream UFS driver so delete for now.
> > >
> > > This is the upstream and they are documented here, although as pointed
> > > by Vladimir this was rather a reverse-documentation. The documentation
> > > might be incorrect, but then the bindings should be corrected instead of
> > > only modifying the DTS.
> > >
> > > > Basically the description was added by a commit 462c5c0aa798 ("dt-bindings: ufs:
> > > > qcom,ufs: convert to dtschema").
> > > >
> > > > It's questionable, if an example in the new yaml file is totally correct
> > > > in connection to the discussed issue.
> > >
> > > To be honest - the example probably is not correct, because it was based
> > > on existing DTS without your patch. :)
> > >
> > > Another question is whether the interconnect properties are here correct
> > > at all. I assumed that DTS is correct because it should describe the
> > > hardware, even if driver does not use it. However maybe that was a false
> > > assumption...
> >
> > writing-bindings.rst says it is OK to document even if it isn't used by the
> > driver (seems wrong to me, at least for interconnects which are a firmware
> > abstraction and not hardware).
>
> The devicetree, and hence the binding, should describe the hardware, so
> that an implementation can make use of the hardware. So there's no
> problem expressing the interconnect in the binding/dts even though the
> driver isn't using it.
>
> I'm not sure if I'm misunderstanding you, the interconnect paths
> described here are a description of the hardware requirements for this
> device. (I.e. it need the buses between ufs and ddr, and cpu and ufs to
> operate).

This is pedantic but what if my kernel lives in imem and not ddr. Or it 
runs on adsp and not cpu? "ufs-ddr" and "cpu-ufs" are not necessarily 
hardware requirements.

(I was thinking of something else when I wrote that comment, but it 
doesn't actually matter if its firmware/hardware if a driver can 
implement the same functionality either way)

> > 462c5c0aa798 wasn't in my 5.17+ tree pulled after dts changes were merged (I
> > guess doc changes come later), so my commit message is incorrect, but I
> > think it makes more sense to have the documentation reflect the driver. Its
> > also not an important issue, so I'll let others sort it out.
>
> I believe that the correctness of the interconnect property will ensure
> that the interconnect provider doesn't hit sync_state until the ufs
> driver has probed - regardless of the driver actually implementing the
> interconnect voting. But perhaps I've misunderstood the magic involved?

AFAICT, if its not used by the driver it will be ignored completely, 
unless you use OPP (which has some interconnect magic).

> Regards,
> Bjorn