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
+ * @phy: the phy returned by phy_get()
+ * @state: the link state
+ *
+ * Notify the phy of some PM link state transition. Used to notify and
+ * configure the phy accordingly.
+ *
+ * Returns: %0 if successful, a negative error code otherwise
+ */
+int phy_notify_pmstate(struct phy *phy, enum phy_linkstate state)
+{
+ int ret;
+
+ if (!phy || !phy->ops->notify_pmstate)
+ return 0;
+
+ mutex_lock(&phy->mutex);
+ ret = phy->ops->notify_pmstate(phy, state);
+ mutex_unlock(&phy->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(phy_notify_pmstate);
+
/**
* phy_configure() - Changes the phy parameters
* @phy: the phy returned by phy_get()
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 13add0c2c40721fe9ca3f0350d13c035cd25af45..d904ec4edb7e2be41fcf6ab780d3148c2ee8a950 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -53,6 +53,11 @@ enum phy_media {
PHY_MEDIA_DAC,
};
+enum phy_linkstate {
+ PHY_UFS_HIBERN8_ENTER,
+ PHY_UFS_HIBERN8_EXIT,
+};
+
/**
* union phy_configure_opts - Opaque generic phy configuration
*
@@ -132,6 +137,18 @@ struct phy_ops {
int (*connect)(struct phy *phy, int port);
int (*disconnect)(struct phy *phy, int port);
+ /**
+ * @notify_pmstate:
+ *
+ * Optional.
+ *
+ * Used to notify and configure the phy for a PM link state
+ * transition.
+ *
+ * Returns: 0 if successful, an negative error code otherwise
+ */
+ int (*notify_pmstate)(struct phy *phy, enum phy_linkstate state);
+
void (*release)(struct phy *phy);
struct module *owner;
};
@@ -255,6 +272,7 @@ int phy_reset(struct phy *phy);
int phy_calibrate(struct phy *phy);
int phy_notify_connect(struct phy *phy, int port);
int phy_notify_disconnect(struct phy *phy, int port);
+int phy_notify_pmstate(struct phy *phy, enum phy_linkstate state);
static inline int phy_get_bus_width(struct phy *phy)
{
return phy->attrs.bus_width;
@@ -412,6 +430,13 @@ static inline int phy_notify_disconnect(struct phy *phy, int index)
return -ENOSYS;
}
+static inline int phy_notify_pmstate(struct phy *phy, enum phy_linkstate state)
+{
+ if (!phy)
+ return 0;
+ return -ENOSYS;
+}
+
static inline int phy_configure(struct phy *phy,
union phy_configure_opts *opts)
{
--
2.50.0.727.gbf7dc18ff4-goog
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) > + * @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'. > + * 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. > +{ > + int ret; > + > + if (!phy || !phy->ops->notify_pmstate) > + return 0; > + > + mutex_lock(&phy->mutex); > + ret = phy->ops->notify_pmstate(phy, state); 'notify_link_state' > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_notify_pmstate); > + > /** > * phy_configure() - Changes the phy parameters > * @phy: the phy returned by phy_get() > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index 13add0c2c40721fe9ca3f0350d13c035cd25af45..d904ec4edb7e2be41fcf6ab780d3148c2ee8a950 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -53,6 +53,11 @@ enum phy_media { > PHY_MEDIA_DAC, > }; > > +enum phy_linkstate { > + PHY_UFS_HIBERN8_ENTER, > + PHY_UFS_HIBERN8_EXIT, > +}; Please move these to include/linux/phy/ufs.h as defines. > + > /** > * union phy_configure_opts - Opaque generic phy configuration > * > @@ -132,6 +137,18 @@ struct phy_ops { > int (*connect)(struct phy *phy, int port); > int (*disconnect)(struct phy *phy, int port); > > + /** > + * @notify_pmstate: > + * > + * Optional. > + * > + * Used to notify and configure the phy for a PM link state > + * transition. > + * > + * Returns: 0 if successful, an negative error code otherwise I think you can drop the inline comment and just add to the top level kernel-doc. - Mani -- மணிவண்ணன் சதாசிவம்
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) > > > + * @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? > > > + * 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 -- ~Vinod
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) > > > > > + * @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? > 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()? > > > > > + * 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. - Mani -- மணிவண்ணன் சதாசிவம்
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. 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. > > > > 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. 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
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
On 23/07/2025 09:07, 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) >> >>> + * @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? It seems to be a link state, and I think adding a way to force a link state to a phy, which is basically the role of a phy, more coherent. Neil > >> >>> + * 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 >
© 2016 - 2025 Red Hat, Inc.