Re: [PATCH v4 5/5] phy: qcom: qmp-pcie: add x1e80100 qref supplies

[Johan Hovold <johan@kernel.org>]

On Fri, Jun 06, 2025 at 03:17:50AM -0700, Qiang Yu wrote:
> On Wed, Jun 04, 2025 at 05:10:19PM +0200, Johan Hovold wrote:
> > On Wed, Jun 04, 2025 at 04:02:37PM +0800, Wenbin Yao wrote:
> > > From: Qiang Yu <qiang.yu@oss.qualcomm.com>
> > > 
> > > All PCIe PHYs on the X1E80100 SOC require the vdda-qref, which feeds QREF
> > > clocks provided by the TCSR device.
> > 
> > As I just mentioned in the thread where this is still being discussed:
> > 
> > 	https://lore.kernel.org/all/aEBfV2M-ZqDF7aRz@hovoldconsulting.com
> > 
> > you need to provide a lot more detail on why you think modelling these
> > supplies as PHY supplies (which they are not) is the right thing to do.
> 
> TCSR_PCIE_xx_CLKREF_EN is not always in TCSR, they're custom
> bits to enable pieces of the distribution network. We always classify them
> as "TCSR" even though they're not always in that module.
> 
> So even if we put the QREF supplies in tscr device tree node, it still
> doesn't describe the hardware correctly as the hardware itself does't have
> a unified structure.

It still seems like a better approximation of the hardware.

> Since the TCSR_PCIE_xx_CLKREF_EN is only required by PCIe, why can't we
> model these supplies consumed by TCSR_PCIE_xx_CLKREF_EN as PHY supplies,
> treating PCIe PHY and TCSR_PCIE_xx_CLKREF_EN as a whole.

First, you are only adding one qref supply to the PHY binding, but
apparently there are two or three supplies needed per refclock on X1E
based on the mapping you provided below.

At least on the T14s, these additional qref supplies are identical to
the "phy" and "pll" supplies currently managed by the PHY driver, but is
that always guaranteed to be the case?

Second, the supply properties are supposed to reflect the actual supply
pins on the SoC, but the mapping from the qref supply pins to this new
"qref" supply cannot be inferred without access to internal
documentation. That mapping could go in a driver with a new binding
describing all of the qref supplies, which an integrator can easily
look up from the machine schematics. That driver would also handle any
ordering constraints between the supplies.

Third, what about the other TCSR reference clocks? On X1E there are at
least eleven that besides PCIe are used for USB, eDP and UFS. Don't you
risk disabling a qref supply underneath these drivers as well? A
complete mapping in a clock driver would take care of this too.

What does the mapping look like for the remaining TCSR clocks?

> > Also please answer the question I've asked three times now on how the
> > QREF supplies map to PHY supplies on X1E as no one will be able to use
> > this binding unless this is documented somewhere (and similar for other
> > SoCs).
> >
> 
> PCIe3,
> VDD_A_QREFS_0P875_0,
> VDD_A_QREFS_0P875_B,
> VDD_A_QREFS_1P2_B,
> 
> PCIe4,
> VDD_A_QREFS_0P875_B,
> VDD_A_QREFS_1P2_B
> 
> PCIe5,
> VDD_A_QREFS_0P875_2,
> VDD_A_QREFS_0P875_B,
> VDD_A_QREFS_1P2_B,
> 
> PCIe6
> VDD_A_QREFS_0P875_A,
> VDD_A_QREFS_1P2_A

Thanks for providing these.

Johan