Re: [PATCH v3 RESEND 1/4] phy: core: add notify_connect and notify_disconnect callback

Re: [PATCH v3 RESEND 1/4] phy: core: add notify_connect and notify_disconnect callback

[Sergei Shtylyov]

On 12/7/23 10:38 AM, 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

   To be able to do dynamic tuning based in the port status, maybe?

> driver when usb connection/disconnection change.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
[...]
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 96a0b1e111f3..a84ad4896b7f 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -489,6 +489,53 @@ int phy_calibrate(struct phy *phy)
>  }
>  EXPORT_SYMBOL_GPL(phy_calibrate);
>  
> +/**
> + * phy_notify_connect() - phy connect notify

   Notification?

> + * @phy: the phy returned by phy_get()
> + * @port: the port index for connect
> + *
> + * If phy need the get connection status, the callback can be used.

   If the PHY needs to get the connection status, maybe?

> + * Returns: %0 if successful, a negative error code otherwise
> + */
> +int phy_notify_connect(struct phy *phy, int port)
> +{
> +	int ret;
> +
> +	if (!phy || !phy->ops->connect)
> +		return 0;
> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->connect(phy, port);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_notify_connect);
> +
> +/**
> + * phy_notify_disconnect() - phy disconnect notify

   Notification?

> + * @phy: the phy returned by phy_get()
> + * @port: the port index for disconnect
> + *
> + * If phy need the get disconnection status, the callback can be used.

   If the PHY needs to get the connection status, maybe?

[...]
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index f6d607ef0e80..cf98cb29ddaa 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
[...]

MBR, Sergey

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 RESEND 1/4] phy: core: add notify_connect and notify_disconnect callback

[Greg Kroah-Hartman]

On Thu, Dec 07, 2023 at 03:38:04PM +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 connection/disconnection change.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> RESEND:
>     Because there is no extcon device provided in the USB framework to
>     notify connect and disconnect.
>     Therefore, I added the notification connection/disconnection based
>     on the generic phy. So I no use the EXTCON framework for notifying
>     connect/disconnect.
> v2 to v3:
>     No change
> v1 to v2:
>     No change
> ---
>  drivers/phy/phy-core.c  | 47 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/phy/phy.h | 18 ++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 96a0b1e111f3..a84ad4896b7f 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -489,6 +489,53 @@ int phy_calibrate(struct phy *phy)
>  }
>  EXPORT_SYMBOL_GPL(phy_calibrate);
>  
> +/**
> + * phy_notify_connect() - phy connect notify
> + * @phy: the phy returned by phy_get()
> + * @port: the port index for connect
> + *
> + * If phy need the get connection status, the callback can be used.
> + * Returns: %0 if successful, a negative error code otherwise
> + */
> +int phy_notify_connect(struct phy *phy, int port)
> +{
> +	int ret;
> +
> +	if (!phy || !phy->ops->connect)
> +		return 0;

How can phy be null?

And it is not successful if connect is not valid, so why not return an
error there?

> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->connect(phy, port);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_notify_connect);
> +
> +/**
> + * phy_notify_disconnect() - phy disconnect notify
> + * @phy: the phy returned by phy_get()
> + * @port: the port index for disconnect
> + *
> + * If phy need the get disconnection status, the callback can be used.
> + *
> + * Returns: %0 if successful, a negative error code otherwise
> + */
> +int phy_notify_disconnect(struct phy *phy, int port)
> +{
> +	int ret;
> +
> +	if (!phy || !phy->ops->disconnect)
> +		return 0;

Same as above.

> +
> +	mutex_lock(&phy->mutex);
> +	ret = phy->ops->disconnect(phy, port);
> +	mutex_unlock(&phy->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_notify_disconnect);
> +
>  /**
>   * 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 f6d607ef0e80..cf98cb29ddaa 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -122,6 +122,8 @@ struct phy_ops {
>  			    union phy_configure_opts *opts);
>  	int	(*reset)(struct phy *phy);
>  	int	(*calibrate)(struct phy *phy);
> +	int	(*connect)(struct phy *phy, int port);
> +	int	(*disconnect)(struct phy *phy, int port);

You forgot to document these and would have a warning from the
documentation build if this was applied :(


>  	void	(*release)(struct phy *phy);
>  	struct module *owner;
>  };
> @@ -243,6 +245,8 @@ static inline enum phy_mode phy_get_mode(struct phy *phy)
>  }
>  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);
>  static inline int phy_get_bus_width(struct phy *phy)
>  {
>  	return phy->attrs.bus_width;
> @@ -396,6 +400,20 @@ static inline int phy_calibrate(struct phy *phy)
>  	return -ENOSYS;
>  }
>  
> +static inline int phy_notify_connect(struct phy *phy, int index)
> +{
> +	if (!phy)
> +		return 0;

Why check this?

> +	return -ENOSYS;
> +}
> +
> +static inline int phy_notify_disconnect(struct phy *phy, int index)
> +{
> +	if (!phy)
> +		return 0;

Again, why check this?

thanks,

greg k-h

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v3 RESEND 4/4] usb: core: add phy notify connect and disconnect

[Greg Kroah-Hartman]

On Thu, Dec 07, 2023 at 03:38:07PM +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 generic
> phy driver when usb device connect and disconnect change.
> 
> The Realtek phy driver is designed to dynamically adjust disconnection
> level and calibrate phy parameters. When the device connected bit changes
> and when the disconnected bit changes, do connection change notification:
> 
> Check if portstatus is USB_PORT_STAT_CONNECTION and portchange is
> USB_PORT_STAT_C_CONNECTION.
> 1. The device is connected, the driver lowers the disconnection level and
>    calibrates the phy parameters.
> 2. The device disconnects, the driver increases the disconnect level and
>    calibrates the phy parameters.
> 
> Generic phy driver in usb core framework does not support device connect
> and disconnect notifications. Therefore, we add an api to notify phy
> the connection changes.
> 
> Additionally, the generic phy only specifies primary_hcd in the original
> design. Added specific "usb2-phy" on primary_hcd and "usb3-phy" on
> shared_hcd.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> v2 to v3:
>     No change
> v1 to v2 change:
>     rebase the driver to remove the part of usb phy notify API
> ---
>  drivers/usb/core/hcd.c | 14 +++++--
>  drivers/usb/core/hub.c | 29 +++++++++++++
>  drivers/usb/core/phy.c | 94 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/core/phy.h |  3 ++
>  4 files changed, 136 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 12b6dfeaf658..992284461ad8 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2794,10 +2794,16 @@ int usb_add_hcd(struct usb_hcd *hcd,
>  	struct usb_device *rhdev;
>  	struct usb_hcd *shared_hcd;
>  
> -	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
> -		if (IS_ERR(hcd->phy_roothub))
> -			return PTR_ERR(hcd->phy_roothub);
> +	if (!hcd->skip_phy_initialization) {
> +		if (usb_hcd_is_primary_hcd(hcd)) {
> +			hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
> +			if (IS_ERR(hcd->phy_roothub))
> +				return PTR_ERR(hcd->phy_roothub);
> +		} else {
> +			hcd->phy_roothub = usb_phy_roothub_alloc_usb3_phy(hcd->self.sysdev);
> +			if (IS_ERR(hcd->phy_roothub))
> +				return PTR_ERR(hcd->phy_roothub);
> +		}
>  
>  		retval = usb_phy_roothub_init(hcd->phy_roothub);
>  		if (retval)
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 87480a6e6d93..65c0454ee70a 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -37,6 +37,7 @@
>  #include <asm/byteorder.h>
>  
>  #include "hub.h"
> +#include "phy.h"
>  #include "otg_productlist.h"
>  
>  #define USB_VENDOR_GENESYS_LOGIC		0x05e3
> @@ -622,6 +623,34 @@ static int hub_ext_port_status(struct usb_hub *hub, int port1, int type,
>  		ret = 0;
>  	}
>  	mutex_unlock(&hub->status_mutex);
> +
> +	/*
> +	 * There is no need to lock status_mutex here, because status_mutex
> +	 * protects hub->status, and the phy driver only checks the port
> +	 * status without changing the status.
> +	 */
> +	if (!ret) {
> +		struct usb_device *hdev = hub->hdev;
> +
> +		/*
> +		 * Only roothub will be notified of connection changes,
> +		 * since the USB PHY only cares about changes at the next
> +		 * level.
> +		 */
> +		if (is_root_hub(hdev)) {
> +			struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
> +			bool connect;
> +			bool connect_change;
> +
> +			connect_change = *change & USB_PORT_STAT_C_CONNECTION;
> +			connect = *status & USB_PORT_STAT_CONNECTION;
> +			if (connect_change && connect)
> +				usb_phy_roothub_notify_connect(hcd->phy_roothub, port1 - 1);
> +			else if (connect_change)
> +				usb_phy_roothub_notify_disconnect(hcd->phy_roothub, port1 - 1);
> +		}
> +	}
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index fb1588e7c282..26585fc1ec32 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -19,6 +19,29 @@ struct usb_phy_roothub {
>  	struct list_head	list;
>  };
>  
> +static int usb_phy_roothub_add_phy_by_name(struct device *dev, const char *name,
> +					   struct list_head *list)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct phy *phy;
> +
> +	phy = devm_of_phy_get(dev, dev->of_node, name);
> +	if (IS_ERR(phy))
> +		return PTR_ERR(phy);
> +
> +	roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
> +	if (!roothub_entry)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&roothub_entry->list);
> +
> +	roothub_entry->phy = phy;
> +
> +	list_add_tail(&roothub_entry->list, list);
> +
> +	return 0;
> +}
> +
>  static int usb_phy_roothub_add_phy(struct device *dev, int index,
>  				   struct list_head *list)
>  {
> @@ -65,6 +88,9 @@ struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  
>  	INIT_LIST_HEAD(&phy_roothub->list);
>  
> +	if (!usb_phy_roothub_add_phy_by_name(dev, "usb2-phy", &phy_roothub->list))
> +		return phy_roothub;
> +
>  	for (i = 0; i < num_phys; i++) {
>  		err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
>  		if (err)
> @@ -75,6 +101,32 @@ struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc);
>  
> +struct usb_phy_roothub *usb_phy_roothub_alloc_usb3_phy(struct device *dev)
> +{
> +	struct usb_phy_roothub *phy_roothub;
> +	int num_phys;
> +
> +	if (!IS_ENABLED(CONFIG_GENERIC_PHY))
> +		return NULL;
> +
> +	num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> +					      "#phy-cells");
> +	if (num_phys <= 0)
> +		return NULL;
> +
> +	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
> +	if (!phy_roothub)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&phy_roothub->list);
> +
> +	if (!usb_phy_roothub_add_phy_by_name(dev, "usb3-phy", &phy_roothub->list))
> +		return phy_roothub;
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(usb_phy_roothub_alloc_usb3_phy);
> +
>  int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub)
>  {
>  	struct usb_phy_roothub *roothub_entry;
> @@ -172,6 +224,48 @@ int usb_phy_roothub_calibrate(struct usb_phy_roothub *phy_roothub)
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_calibrate);
>  
> +int usb_phy_roothub_notify_connect(struct usb_phy_roothub *phy_roothub, int port)
> +{
> +	struct usb_phy_roothub *roothub_entry;
> +	struct list_head *head;
> +	int err;
> +
> +	if (!phy_roothub)
> +		return 0;

How can phy_roothub ever be NULL?

> +
> +	head = &phy_roothub->list;
> +
> +	list_for_each_entry(roothub_entry, head, list) {
> +		err = phy_notify_connect(roothub_entry->phy, port);
> +		if (err)
> +			return err;
> +	}
>

You walk a list with no locking at all?  That does not seem right at
all.

Also, this is a new function that is exported with no documentation?
Please fix.

thanks,

greg k-h

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy