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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

On Fri, Oct 16, 2026 at 21:13, Manivannan Sadhasivam <mani@kernel.org> wrote:

> On 2026-10-16 15:14 +0200, Mattijs Korpershoek wrote:
>> Hi Mani,
>> 
>> On Fri, Oct 16, 2026 at 15:20, Manivannan Sadhasivam <mani@kernel.org> wrote:
>> 
>> > On 2026-10-02 11:41 +0200, Mattijs Korpershoek wrote:
>> >> From: Sriram Dash <quic_sriramd@quicinc.com>
>> >> 
>> >> 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."

Yes, I need to update the commit message here. I should have done that
when updating the binding commit. Sorry about that.

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

Ack. It's not defined in the binding and I won't use it in the driver.

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

I agree. Unfortunately, I can't change what the SCMI firmware exposes to me.

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

Ok, will do.

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

Ok, will do. Thank you for the suggestion.

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

That's correct. It's been a while I looked into it so I got confused.
However, I'd like the names to stay like this since it's how they are
defined in the binding.

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