Re: [PATCH v3 2/2] phy: qcom-snps-femto-v2: Add support for SA8255P

[Manivannan Sadhasivam]

On 2026-10-16 15:14 +0200, Mattijs Korpershoek wrote:
> Hi Mani,
> 
> On Fri, Oct 16, 2026 at 15:20, Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote:
> 
> > On 2026-10-02 11:41 +0200, Mattijs Korpershoek wrote:
> >> From: Sriram Dash <quic_sriramd@xxxxxxxxxxx>
> >> 
> >> The Qualcomm automotive SA8255p SoC relies on firmware to configure
> >> platform resources, including clocks, interconnects and TLMM.
> >> The driver requests resources operations over SCMI using power protocols.
> >> 
> >> The SCMI power protocol enables or disables resources like clocks,
> >> interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
> >> such as resume/suspend, to control power on/off.
> >> 
> >> Power domain "core" controls the GDSC, resets, and TLMM (GPIOs)
> >> whereas power domain "transfer" controls clocks and interconnects.
> >> 
> >
> > This is not consistent with other SCMI based drivers. Most of them are using
> > 'power' and 'perf' power domains, not 'core', 'transfer'.
> 
> That's correct. Krzysztof had the same concerns [1] in the binding
> reviews. To sum up from that thread:
> 
> For usb PHY, 2 SCMI protocols are exposed by the firmware:
> * power domain management protocol (protocol_id=0x11)
> * performance domain management protocol (protocol_id=0x13)
> 
> The power domain has 2 indices: 'core' and 'transfer'.
> * core controls the regulators, the clocks and the reset
> * transfer only controls the clocks
> 

But this contradicts with your commit message:

"Power domain "core" controls the GDSC, resets, and TLMM (GPIOs)
whereas power domain "transfer" controls clocks and interconnects."

> The performance domain (0x13) exposes opaques values (0xa to 0x11)
> for which we don't have any information.
> 

It is a shame that it is being defined as opaque values, but as long as you
don't define it in the binding and not use it in the driver, I'm OK with it.

> >
> > Why are these discrete powers domains be required? In my point of view, the PHY
> > should have only one power domain for controlling power and be done with it. If
> > you start having these fine grain controls over the resources from SCMI clients,
> > then it defeats the purpose of the SCMI based firmware abstraction design.
> 
> From what I understand, thse fine-grain controls are useful for runtime
> suspend/resume (on the Linux side).
> For example:
> 1. Enable 'core' power domain to enable the PHY
> 2. Disable 'transfer' to turn of the clocks. PHY is still powered but is
> "idle"
> 
> If we look at the patch, we can see that this is what's done in
> qcom_snps_hsphy_suspend().
> 
> Does that clarify things?
> 

Even though the SCMI spec allows defining multiple sub-domains inside a
protocol, I'd argue that there should only be one power domain 'power'
controlling all resources such as clock, icc, resets etc... But in this case,
that ship has already sailed.

> This is also explained in the binding [2]
> Should I rephrase the commit message here to include that?
> 

Yes, please. Driver commit message should be independent.

> >
> >> In this variant, two disjoint sets of resources are modeled against
> >> the power domain suppliers which can be turned on/off to achieve modeled
> >> device states. Two modeled low power states d1 and d3 are supported.
> >> 
> >> transition: d0 <--> d3  :: both core and transfer suppliers
> >> transition: d0 <--> d1  :: only transfer supplier
> >> 
> >
> > What are these transition levels and who defines these? We cannot have different
> > SCMI drivers inventing these kind of independent low power modes.
> >
> 
> Maybe the naming is not as good as it should be.
> I took this originally from a downstream qcom patch and did not change
> it.
> 
> I interpret this as:
> * d3: phy is powered off, before probe (no regulators, no clocks and in reset mode)
> * d1: phy is in lower power mode (regulator on, no clocks, no reset)
> * d0: phy is powered on an operational (after probe)
> 

These looks like ACPI D-states, which are neither defined in the SCMI spec, nor
used by any other Qcom SCMI drivers. So this driver cannot just invent these
states on its own. Just use plain wording for the function names based on the
indices.

> I see that drivers/tty/serial/qcom_geni_serial.c uses a similar
> mechanism. Should I try to follow that naming convention?
> 
> If you have other suggestions, I'm happy to get them.
> 
> Note that I can't change the power domains ('core' and 'transfer' since
> those are enforced by the SCMI firmware)
> 

SCMI firmware don't define the power domain names, isn't it? They just define
indices and it is up to the OS to name it however it wants.

But if it is defined in any of the internal doc, then it is fine.

- Mani