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

[Mattijs Korpershoek]

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

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

>
> 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?

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

>
>> 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)

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)

Thanks again for the review
Mattijs

[1] https://lore.kernel.org/all/877blx2bga.fsf@xxxxxxxxxx/
[2] https://lore.kernel.org/all/20261002-phy-qcom-snps-femto-v2-8255-v3-1-342542bd9bc6@xxxxxxxxxx/

> - Mani