[PATCH v1] usb: phy: add usb phy notify port status API

[PATCH v1] usb: phy: add usb phy notify port status API

[Stanley Chang]

In Realtek SoC, the parameter of usb phy is designed to can dynamic
tuning base on port status. Therefore, add a notify callback of phy
driver when usb port status change.

Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
---
 drivers/usb/core/hub.c  | 13 +++++++++++++
 include/linux/usb/phy.h | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 97a0f8faea6e..b4fbbeae1927 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub, int port1, int type,
 		ret = 0;
 	}
 	mutex_unlock(&hub->status_mutex);
+
+	if (!ret) {
+		struct usb_device *hdev = hub->hdev;
+
+		if (hdev && !hdev->parent) {
+			struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
+
+			if (hcd->usb_phy)
+				usb_phy_notify_port_status(hcd->usb_phy,
+					    port1 - 1, *status, *change);
+		}
+	}
+
 	return ret;
 }
 
diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index e4de6bc1f69b..53bf3540098f 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -144,6 +144,10 @@ struct usb_phy {
 	 */
 	int	(*set_wakeup)(struct usb_phy *x, bool enabled);
 
+	/* notify phy port status change */
+	int	(*notify_port_status)(struct usb_phy *x,
+		int port, u16 portstatus, u16 portchange);
+
 	/* notify phy connect status change */
 	int	(*notify_connect)(struct usb_phy *x,
 			enum usb_device_speed speed);
@@ -316,6 +320,16 @@ usb_phy_set_wakeup(struct usb_phy *x, bool enabled)
 		return 0;
 }
 
+static inline int
+usb_phy_notify_port_status(struct usb_phy *x, int port, u16 portstatus,
+	    u16 portchange)
+{
+	if (x && x->notify_port_status)
+		return x->notify_port_status(x, port, portstatus, portchange);
+	else
+		return 0;
+}
+
 static inline int
 usb_phy_notify_connect(struct usb_phy *x, enum usb_device_speed speed)
 {
-- 
2.34.1

Re: [PATCH v1] usb: phy: add usb phy notify port status API

[Greg Kroah-Hartman]

On Fri, Apr 21, 2023 at 04:03:31PM +0800, Stanley Chang wrote:
> In Realtek SoC, the parameter of usb phy is designed to can dynamic
> tuning base on port status. Therefore, add a notify callback of phy
> driver when usb port status change.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
>  drivers/usb/core/hub.c  | 13 +++++++++++++
>  include/linux/usb/phy.h | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)

We can not add callbacks in the kernel that are not actually used,
otherwise they will just be instantly removed.

Please submit any drivers that need this change at the same time so that
we can verify that the callback is actually correct and needed,
otherwise we can not take this change.

thanks,

greg k-h

RE: [PATCH v1] usb: phy: add usb phy notify port status API

[Stanley Chang[昌育德]]

> 
> On Fri, Apr 21, 2023 at 04:03:31PM +0800, Stanley Chang wrote:
> > In Realtek SoC, the parameter of usb phy is designed to can dynamic
> > tuning base on port status. Therefore, add a notify callback of phy
> > driver when usb port status change.
> >
> > Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> > ---
> >  drivers/usb/core/hub.c  | 13 +++++++++++++  include/linux/usb/phy.h |
> > 14 ++++++++++++++
> >  2 files changed, 27 insertions(+)
> 
> We can not add callbacks in the kernel that are not actually used, otherwise
> they will just be instantly removed.
> 
> Please submit any drivers that need this change at the same time so that we
> can verify that the callback is actually correct and needed, otherwise we can
> not take this change.
> 

In this stage, we usb phy driver is not at linux upstream. 
For the android GKI, we have to add this callback to upstream or use the vendor hook of android.
I will plan to upstream the realtek usb phy driver.

Thanks,
Stanley

Re: [PATCH v1] usb: phy: add usb phy notify port status API

[Greg Kroah-Hartman]

On Fri, Apr 21, 2023 at 08:32:19AM +0000, Stanley Chang[昌育德] wrote:
> > 
> > On Fri, Apr 21, 2023 at 04:03:31PM +0800, Stanley Chang wrote:
> > > In Realtek SoC, the parameter of usb phy is designed to can dynamic
> > > tuning base on port status. Therefore, add a notify callback of phy
> > > driver when usb port status change.
> > >
> > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> > > ---
> > >  drivers/usb/core/hub.c  | 13 +++++++++++++  include/linux/usb/phy.h |
> > > 14 ++++++++++++++
> > >  2 files changed, 27 insertions(+)
> > 
> > We can not add callbacks in the kernel that are not actually used, otherwise
> > they will just be instantly removed.
> > 
> > Please submit any drivers that need this change at the same time so that we
> > can verify that the callback is actually correct and needed, otherwise we can
> > not take this change.
> > 
> 
> In this stage, we usb phy driver is not at linux upstream. 

Then obviously we can not take this change (nor would you want us to.)

> For the android GKI, we have to add this callback to upstream or use the vendor hook of android.
> I will plan to upstream the realtek usb phy driver.

As you already have this driver, why not send it to us now?

thanks,

greg k-h

RE: [PATCH v1] usb: phy: add usb phy notify port status API

[Stanley Chang[昌育德]]

> > > We can not add callbacks in the kernel that are not actually used,
> > > otherwise they will just be instantly removed.
> > >
> > > Please submit any drivers that need this change at the same time so
> > > that we can verify that the callback is actually correct and needed,
> > > otherwise we can not take this change.
> > >
> >
> > In this stage, we usb phy driver is not at linux upstream.
> 
> Then obviously we can not take this change (nor would you want us to.)

I will prepare a complete driver for review.

> > For the android GKI, we have to add this callback to upstream or use the
> vendor hook of android.
> > I will plan to upstream the realtek usb phy driver.
> 
> As you already have this driver, why not send it to us now?
> 
Now the driver doesn't match the coding style.
After I refactor it, I will send this driver upstream.

Thanks,
Stanley

Re: [PATCH v1] usb: phy: add usb phy notify port status API

[Greg Kroah-Hartman]

On Fri, Apr 21, 2023 at 04:03:31PM +0800, Stanley Chang wrote:
> In Realtek SoC, the parameter of usb phy is designed to can dynamic
> tuning base on port status. Therefore, add a notify callback of phy
> driver when usb port status change.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
>  drivers/usb/core/hub.c  | 13 +++++++++++++
>  include/linux/usb/phy.h | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 97a0f8faea6e..b4fbbeae1927 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub, int port1, int type,
>  		ret = 0;
>  	}
>  	mutex_unlock(&hub->status_mutex);
> +
> +	if (!ret) {
> +		struct usb_device *hdev = hub->hdev;
> +
> +		if (hdev && !hdev->parent) {

How can you have a device without a parent?  And why does it matter?

And how could hdev be NULL?  And if it can change to be NULL, what
prevents it from changing right after you checked for it?

thanks,

greg k-h

RE: [PATCH v1] usb: phy: add usb phy notify port status API

[Stanley Chang[昌育德]]

> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
> > 97a0f8faea6e..b4fbbeae1927 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub,
> int port1, int type,
> >               ret = 0;
> >       }
> >       mutex_unlock(&hub->status_mutex);
> > +
> > +     if (!ret) {
> > +             struct usb_device *hdev = hub->hdev;
> > +
> > +             if (hdev && !hdev->parent) {
> 
> How can you have a device without a parent?  And why does it matter?

If the hub is a root hub, the parent of hub will be NULL.
And we only send the port status to phy driver for root hub.
> 
> And how could hdev be NULL?  And if it can change to be NULL, what prevents
> it from changing right after you checked for it?
> 

It is right. hdev is never NULL, this is a redundant check.

Thanks,
Stanley