Re: [PATCH v2 1/2] phy: add new phy_notify_pmstate() api

[Neil Armstrong <neil.armstrong@linaro.org>]

Hi,

On 25/07/2025 12:21, Peter Griffin wrote:
> Hi Vinod, Mani & Neil,
> 
> Thanks a lot for the valuable review feedback.
> 
> On Wed, 23 Jul 2025 at 09:04, Manivannan Sadhasivam <mani@kernel.org> wrote:
>>
>> On Wed, Jul 23, 2025 at 12:37:31PM GMT, Vinod Koul wrote:
>>> On 22-07-25, 22:04, Manivannan Sadhasivam wrote:
>>>> On Thu, Jul 03, 2025 at 02:03:22PM GMT, Peter Griffin wrote:
>>>>> Add a new phy_notify_pmstate() api that notifies and configures a phy for a
>>>>> given PM link state transition.
>>>>>
>>>>> This is intended to be by phy drivers which need to do some runtime
>>>>> configuration of parameters during the transition that can't be handled by
>>>>> phy_calibrate() or phy_power_{on|off}().
>>>>>
>>>>> The first usage of this API is in the Samsung UFS phy that needs to issue
>>>>> some register writes when entering and exiting the hibernate link state.
>>>>>
>>>>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>>>>> ---
>>>>>   drivers/phy/phy-core.c  | 25 +++++++++++++++++++++++++
>>>>>   include/linux/phy/phy.h | 25 +++++++++++++++++++++++++
>>>>>   2 files changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>>> index 04a5a34e7a950ae94fae915673c25d476fc071c1..0b29bc2c709890d7fc27d1480a35cda6a826fd30 100644
>>>>> --- a/drivers/phy/phy-core.c
>>>>> +++ b/drivers/phy/phy-core.c
>>>>> @@ -520,6 +520,31 @@ int phy_notify_disconnect(struct phy *phy, int port)
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(phy_notify_disconnect);
>>>>>
>>>>> +/**
>>>>> + * phy_notify_pmstate() - phy link state notification
>>>>
>>>> 'pmstate' doesn't correspond to 'link state'. So how about,
>>>> phy_notify_link_state()?
>>>>
>>>> s/phy/PHY (here and below)
> 
> will fix
> 
>>>>
>>>>> + * @phy: the phy returned by phy_get()
>>>>> + * @state: the link state
>>>>> + *
>>>>> + * Notify the phy of some PM link state transition. Used to notify and
>>>>
>>>> Link state change is common for the PHY. So remove 'PM'.
>>>
>>> Is it really link or phy state?
> 
> I think it is likely both link and phy state.
> 
> Looking at the wording in section '9.5 Hibernate' in mipi unipro 1.8
> spec we have phrases such as
> 
> 9.5 Hibernate "Hibernate is a UniPro state in which the PHY is in the
> HIBERNATE_STATE, and the UniPro stack keeps only a minimal set of
> features active."
> 
> 9.5 Figure 99 describes Link hibernation where one Device, typically a
> Host Device, initiates Link hibernation with a peer Device.

So this is part handled by the PHY

> 
> 9.5.1.2 The local PA Layer receives this request and places the M-PHY
> Link into hibernate using the PACP protocol (detailed description of
> PACP protocol can be found in Section 5.7.7). Once the PA Layer has
> successfully hibernated the M-PHY Link, subsequent layers of the local
> and peer
> UniPro stack (L4 to L2) shall be hibernated by the DME by sending a
> <layer-identifer>_LM_HIBERNATE_ENTER.req SAP primitive to the
> respective layers.
> 

And this by the controller, so yeah we set the PHY state, and the PHY will
set the link state accordingly.

>>>
>>
>> This is a bit of ambiguity. But as per the spec, Hibern8 is the low power state
>> of the M-PHY and Unipro controller.
>>
>> Maybe, phy_notify_state()?
>>
> 
> phy_notify_state() seems like a good name. It might be better suited
> for other peripherals as well rather than narrowing it with link_state
> or pmstate.

Ack

> 
> Vinod, any thoughts on your preferred name?
> 
>>>>
> 
>>>>> + * configure the phy accordingly.
>>>>> + *
>>>>> + * Returns: %0 if successful, a negative error code otherwise
>>>>> + */
>>>>> +int phy_notify_pmstate(struct phy *phy, enum phy_linkstate state)
>>>>
>>>> I think you need to use 'int state' and let drivers pass their own link state
>>>> values. You cannot have generic link states across all peripherals.
>>>
>>> I would avoid that, people start overloading this if we let it keep
>>> open! I would like to avoid the api -(ab)use
>>>
>>
>> Then we will end up with peripheral specific enums in include/linux/phy/phy.h.
>> If you are OK with that, fine with me.
> 
> Ok I'll add peripheral specific enums to include/linux/phy/phy.h in
> the next version.
> 
> Thanks,
> 
> Peter
> 

Thanks,
Neil