[PATCH 0/3] External VBUS port power handling for onboard USB devices

[PATCH 0/3] External VBUS port power handling for onboard USB devices

[Marco Felsch]

Hi,

some PCB designs don't connect the USB hub port power line to the
dedicated hub port power pin. Instead they use a host controllable GPIO
line to enable the VBUS power.

This patchset addresses this use-case by hooking into the
usb_clear/set_port_feature() function and call the usb-onboard-dev hook
if available. The usb-onboard-dev driver is taking care of the rest.

Regards,
  Marco

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Marco Felsch (3):
      usb: hub: add infrastructure to pass onboard_dev port features
      dt-bindings: usb: microchip,usb2514: add support for port vbus-supply
      usb: misc: onboard_dev: add ext-vbus-supply handling

 .../devicetree/bindings/usb/microchip,usb2514.yaml |  7 ++
 drivers/usb/core/hub.c                             | 22 ++++++-
 drivers/usb/misc/onboard_usb_dev.c                 | 76 ++++++++++++++++++++++
 drivers/usb/misc/onboard_usb_dev.h                 |  2 +
 include/linux/usb/onboard_dev.h                    |  6 ++
 5 files changed, 111 insertions(+), 2 deletions(-)
---
base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
change-id: 20240807-b4-v6-10-topic-usb-onboard-dev-49040782d2f9

Best regards,
-- 
Marco Felsch <m.felsch@pengutronix.de>

[PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[Marco Felsch]

On board devices may require special handling for en-/disable port
features due to PCB design decisions e.g. enable/disable the VBUS power
on the port. This commit adds the necessary infrastructure to prepare
the common code base for such use-cases.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/core/hub.c             | 22 ++++++++++++++++++++--
 drivers/usb/misc/onboard_usb_dev.c | 13 +++++++++++++
 include/linux/usb/onboard_dev.h    |  6 ++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 4b93c0bd1d4b..e639c25a729c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -450,9 +450,18 @@ static int clear_hub_feature(struct usb_device *hdev, int feature)
  */
 int usb_clear_port_feature(struct usb_device *hdev, int port1, int feature)
 {
-	return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+	int ret;
+
+	ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
 		USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
 		NULL, 0, 1000);
+	if (ret)
+		return ret;
+
+	if (!is_root_hub(hdev))
+		ret = onboard_dev_port_feature(hdev, false, feature, port1);
+
+	return ret;
 }
 
 /*
@@ -460,9 +469,18 @@ int usb_clear_port_feature(struct usb_device *hdev, int port1, int feature)
  */
 static int set_port_feature(struct usb_device *hdev, int port1, int feature)
 {
-	return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
+	int ret;
+
+	ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
 		USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
 		NULL, 0, 1000);
+	if (ret)
+		return ret;
+
+	if (!is_root_hub(hdev))
+		ret = onboard_dev_port_feature(hdev, true, feature, port1);
+
+	return ret;
 }
 
 static char *to_led_name(int selector)
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index f2bcc1a8b95f..f61de2c353d0 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -520,6 +520,19 @@ static struct usb_device_driver onboard_dev_usbdev_driver = {
 	.id_table = onboard_dev_id_table,
 };
 
+/************************** USB control **************************/
+
+int onboard_dev_port_feature(struct usb_device *udev, bool set,
+			     int feature, int port1)
+{
+	switch (feature) {
+	default:
+		return 0;
+	}
+}
+
+/************************** Kernel module ************************/
+
 static int __init onboard_dev_init(void)
 {
 	int ret;
diff --git a/include/linux/usb/onboard_dev.h b/include/linux/usb/onboard_dev.h
index b79db6d193c8..45e1f7b844d6 100644
--- a/include/linux/usb/onboard_dev.h
+++ b/include/linux/usb/onboard_dev.h
@@ -9,10 +9,16 @@ struct list_head;
 #if IS_ENABLED(CONFIG_USB_ONBOARD_DEV)
 void onboard_dev_create_pdevs(struct usb_device *parent_dev, struct list_head *pdev_list);
 void onboard_dev_destroy_pdevs(struct list_head *pdev_list);
+int onboard_dev_port_feature(struct usb_device *udev, bool set, int feature, int port1);
 #else
 static inline void onboard_dev_create_pdevs(struct usb_device *parent_dev,
 					    struct list_head *pdev_list) {}
 static inline void onboard_dev_destroy_pdevs(struct list_head *pdev_list) {}
+static inline int onboard_dev_port_feature(struct usb_device *udev, bool set,
+					   int feature, int port1)
+{
+	return 0;
+}
 #endif
 
 #endif /* __LINUX_USB_ONBOARD_DEV_H */

-- 
2.39.2

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[kernel test robot]

Hi Marco,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0c3836482481200ead7b416ca80c68a29cfdaabd]

url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Felsch/usb-hub-add-infrastructure-to-pass-onboard_dev-port-features/20240807-224100
base:   0c3836482481200ead7b416ca80c68a29cfdaabd
patch link:    https://lore.kernel.org/r/20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9%40pengutronix.de
patch subject: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features
config: i386-randconfig-141-20240808 (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408081557.FiEe9Tzz-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/usb/core/hub.o: in function `set_port_feature':
>> drivers/usb/core/hub.c:481: undefined reference to `onboard_dev_port_feature'
   ld: drivers/usb/core/hub.o: in function `usb_clear_port_feature':
   drivers/usb/core/hub.c:462: undefined reference to `onboard_dev_port_feature'


vim +481 drivers/usb/core/hub.c

   466	
   467	/*
   468	 * USB 2.0 spec Section 11.24.2.13
   469	 */
   470	static int set_port_feature(struct usb_device *hdev, int port1, int feature)
   471	{
   472		int ret;
   473	
   474		ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
   475			USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
   476			NULL, 0, 1000);
   477		if (ret)
   478			return ret;
   479	
   480		if (!is_root_hub(hdev))
 > 481			ret = onboard_dev_port_feature(hdev, true, feature, port1);
   482	
   483		return ret;
   484	}
   485	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[Marco Felsch]

Hi all,

On 24-08-08, kernel test robot wrote:
> Hi Marco,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on 0c3836482481200ead7b416ca80c68a29cfdaabd]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Felsch/usb-hub-add-infrastructure-to-pass-onboard_dev-port-features/20240807-224100
> base:   0c3836482481200ead7b416ca80c68a29cfdaabd
> patch link:    https://lore.kernel.org/r/20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9%40pengutronix.de
> patch subject: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features
> config: i386-randconfig-141-20240808 (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202408081557.FiEe9Tzz-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>    ld: drivers/usb/core/hub.o: in function `set_port_feature':
> >> drivers/usb/core/hub.c:481: undefined reference to `onboard_dev_port_feature'
>    ld: drivers/usb/core/hub.o: in function `usb_clear_port_feature':
>    drivers/usb/core/hub.c:462: undefined reference to `onboard_dev_port_feature'

I understood the isse but have a few questions. Before continue the work
on this topic I would like to ask if the patchset is okay in general?
I'm open for alternatives if the patchset approach is not okay.

I have a few ideas in mind (see below) to fix the 0day build issue which
was caused by the Kconfig selection:

 - CONFIG_USB=y
 - CONFIG_USB_ONBOARD_DEV=m.

Idea-1:
-------

Dropping the module support for CONFIG_USB_ONBOARD_DEV.

Idea-2:
-------

CONFIG_USB_ONBOARD_DEV follows CONFIG_USB:

CONFIG_USB=y -> CONFIG_USB_ONBOARD_DEV=y,
CONFIG_USB=m -> CONFIG_USB_ONBOARD_DEV=m.

and exporting usb_clear_port_feature().

I don't know to add such Kconfig dependency and also this idea require
that the usbcore have to load the usb_onboard_dev module always,
regardless if used.

So this idea is rather suboptimal.

Idea-3:
-------

Adding a function to the hub.c usbcore which can be used by the
usb-onboard-dev driver to register this function as hook. This removes
the dependency from the core and the usb-onboard-dev module is only
pulled if really required. Of course this require that the hub.c usbcore
driver allows custom hooks.

Idea-X:
-------

I'm open for your input :)


Regards,
  Marco

PS: My favourite is Idea-3 followed by Idea-1.

> vim +481 drivers/usb/core/hub.c
> 
>    466	
>    467	/*
>    468	 * USB 2.0 spec Section 11.24.2.13
>    469	 */
>    470	static int set_port_feature(struct usb_device *hdev, int port1, int feature)
>    471	{
>    472		int ret;
>    473	
>    474		ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
>    475			USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
>    476			NULL, 0, 1000);
>    477		if (ret)
>    478			return ret;
>    479	
>    480		if (!is_root_hub(hdev))
>  > 481			ret = onboard_dev_port_feature(hdev, true, feature, port1);
>    482	
>    483		return ret;
>    484	}
>    485	
> 
> -- 
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
> 

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[Matthias Kaehlcke]

El Fri, Aug 09, 2024 at 11:33:13AM GMT Marco Felsch ha dit:

> Hi all,
> 
> On 24-08-08, kernel test robot wrote:
> > Hi Marco,
> > 
> > kernel test robot noticed the following build errors:
> > 
> > [auto build test ERROR on 0c3836482481200ead7b416ca80c68a29cfdaabd]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Felsch/usb-hub-add-infrastructure-to-pass-onboard_dev-port-features/20240807-224100
> > base:   0c3836482481200ead7b416ca80c68a29cfdaabd
> > patch link:    https://lore.kernel.org/r/20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9%40pengutronix.de
> > patch subject: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features
> > config: i386-randconfig-141-20240808 (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/config)
> > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202408081557.FiEe9Tzz-lkp@intel.com/
> > 
> > All errors (new ones prefixed by >>):
> > 
> >    ld: drivers/usb/core/hub.o: in function `set_port_feature':
> > >> drivers/usb/core/hub.c:481: undefined reference to `onboard_dev_port_feature'
> >    ld: drivers/usb/core/hub.o: in function `usb_clear_port_feature':
> >    drivers/usb/core/hub.c:462: undefined reference to `onboard_dev_port_feature'
> 
> I understood the isse but have a few questions. Before continue the work
> on this topic I would like to ask if the patchset is okay in general?
> I'm open for alternatives if the patchset approach is not okay.

From the perspective of the onboard_usb_dev driver it seems sound to me.

So far the USB maintainers haven't raised objections, so I would say move
forward and we'll see if concerns arise and deal with them if needed.

> I have a few ideas in mind (see below) to fix the 0day build issue which
> was caused by the Kconfig selection:
> 
>  - CONFIG_USB=y
>  - CONFIG_USB_ONBOARD_DEV=m.
> 
> Idea-1:
> -------
> 
> Dropping the module support for CONFIG_USB_ONBOARD_DEV.

With that CONFIG_USB could not be 'm' when CONFIG_USB_ONBOARD_DEV
is set, which wouldn't be great.

> Idea-2:
> -------
> 
> CONFIG_USB_ONBOARD_DEV follows CONFIG_USB:
> 
> CONFIG_USB=y -> CONFIG_USB_ONBOARD_DEV=y,
> CONFIG_USB=m -> CONFIG_USB_ONBOARD_DEV=m.
> 
> and exporting usb_clear_port_feature().
> 
> I don't know to add such Kconfig dependency and also this idea require
> that the usbcore have to load the usb_onboard_dev module always,
> regardless if used.
> 
> So this idea is rather suboptimal.
> 
> Idea-3:
> -------
> 
> Adding a function to the hub.c usbcore which can be used by the
> usb-onboard-dev driver to register this function as hook. This removes
> the dependency from the core and the usb-onboard-dev module is only
> pulled if really required. Of course this require that the hub.c usbcore
> driver allows custom hooks.

This seems like the best approach IMO, if USB maintainers are onboard with
it.

Since this is about port features (only applicable to hubs) the function
should be associated with struct usb_hub, not struct usb_device. And we
probably want two functions, onboard_hub_set_port_feature() and
onboard_hub_clear_port_feature(), whose implementations may use shared
code.

> Idea-X:
> -------
> 
> I'm open for your input :)
> 
> 
> Regards,
>   Marco
> 
> PS: My favourite is Idea-3 followed by Idea-1.
> 
> > vim +481 drivers/usb/core/hub.c
> > 
> >    466	
> >    467	/*
> >    468	 * USB 2.0 spec Section 11.24.2.13
> >    469	 */
> >    470	static int set_port_feature(struct usb_device *hdev, int port1, int feature)
> >    471	{
> >    472		int ret;
> >    473	
> >    474		ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
> >    475			USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
> >    476			NULL, 0, 1000);
> >    477		if (ret)
> >    478			return ret;
> >    479	
> >    480		if (!is_root_hub(hdev))
> >  > 481			ret = onboard_dev_port_feature(hdev, true, feature, port1);
> >    482	
> >    483		return ret;
> >    484	}
> >    485	
> > 
> > -- 
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests/wiki
> > 

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[Marco Felsch]

Hi,

I found two mistakes I made in my v1. I would send a v2 if this series
is interesting for upstream. The remaining open question is how the
driver dependencies should be handled (see idea-1,2,3).

Regards,
  Marco

On 24-09-23, Matthias Kaehlcke wrote:
> El Fri, Aug 09, 2024 at 11:33:13AM GMT Marco Felsch ha dit:
> 
> > Hi all,
> > 
> > On 24-08-08, kernel test robot wrote:
> > > Hi Marco,
> > > 
> > > kernel test robot noticed the following build errors:
> > > 
> > > [auto build test ERROR on 0c3836482481200ead7b416ca80c68a29cfdaabd]
> > > 
> > > url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Felsch/usb-hub-add-infrastructure-to-pass-onboard_dev-port-features/20240807-224100
> > > base:   0c3836482481200ead7b416ca80c68a29cfdaabd
> > > patch link:    https://lore.kernel.org/r/20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9%40pengutronix.de
> > > patch subject: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features
> > > config: i386-randconfig-141-20240808 (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/config)
> > > compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408081557.FiEe9Tzz-lkp@intel.com/reproduce)
> > > 
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | Reported-by: kernel test robot <lkp@intel.com>
> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202408081557.FiEe9Tzz-lkp@intel.com/
> > > 
> > > All errors (new ones prefixed by >>):
> > > 
> > >    ld: drivers/usb/core/hub.o: in function `set_port_feature':
> > > >> drivers/usb/core/hub.c:481: undefined reference to `onboard_dev_port_feature'
> > >    ld: drivers/usb/core/hub.o: in function `usb_clear_port_feature':
> > >    drivers/usb/core/hub.c:462: undefined reference to `onboard_dev_port_feature'
> > 
> > I understood the isse but have a few questions. Before continue the work
> > on this topic I would like to ask if the patchset is okay in general?
> > I'm open for alternatives if the patchset approach is not okay.
> 
> From the perspective of the onboard_usb_dev driver it seems sound to me.
> 
> So far the USB maintainers haven't raised objections, so I would say move
> forward and we'll see if concerns arise and deal with them if needed.
> 
> > I have a few ideas in mind (see below) to fix the 0day build issue which
> > was caused by the Kconfig selection:
> > 
> >  - CONFIG_USB=y
> >  - CONFIG_USB_ONBOARD_DEV=m.
> > 
> > Idea-1:
> > -------
> > 
> > Dropping the module support for CONFIG_USB_ONBOARD_DEV.
> 
> With that CONFIG_USB could not be 'm' when CONFIG_USB_ONBOARD_DEV
> is set, which wouldn't be great.
> 
> > Idea-2:
> > -------
> > 
> > CONFIG_USB_ONBOARD_DEV follows CONFIG_USB:
> > 
> > CONFIG_USB=y -> CONFIG_USB_ONBOARD_DEV=y,
> > CONFIG_USB=m -> CONFIG_USB_ONBOARD_DEV=m.
> > 
> > and exporting usb_clear_port_feature().
> > 
> > I don't know to add such Kconfig dependency and also this idea require
> > that the usbcore have to load the usb_onboard_dev module always,
> > regardless if used.
> > 
> > So this idea is rather suboptimal.
> > 
> > Idea-3:
> > -------
> > 
> > Adding a function to the hub.c usbcore which can be used by the
> > usb-onboard-dev driver to register this function as hook. This removes
> > the dependency from the core and the usb-onboard-dev module is only
> > pulled if really required. Of course this require that the hub.c usbcore
> > driver allows custom hooks.
> 
> This seems like the best approach IMO, if USB maintainers are onboard with
> it.
> 
> Since this is about port features (only applicable to hubs) the function
> should be associated with struct usb_hub, not struct usb_device. And we
> probably want two functions, onboard_hub_set_port_feature() and
> onboard_hub_clear_port_feature(), whose implementations may use shared
> code.
> 
> > Idea-X:
> > -------
> > 
> > I'm open for your input :)
> > 
> > 
> > Regards,
> >   Marco
> > 
> > PS: My favourite is Idea-3 followed by Idea-1.
> > 
> > > vim +481 drivers/usb/core/hub.c
> > > 
> > >    466	
> > >    467	/*
> > >    468	 * USB 2.0 spec Section 11.24.2.13
> > >    469	 */
> > >    470	static int set_port_feature(struct usb_device *hdev, int port1, int feature)
> > >    471	{
> > >    472		int ret;
> > >    473	
> > >    474		ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
> > >    475			USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
> > >    476			NULL, 0, 1000);
> > >    477		if (ret)
> > >    478			return ret;
> > >    479	
> > >    480		if (!is_root_hub(hdev))
> > >  > 481			ret = onboard_dev_port_feature(hdev, true, feature, port1);
> > >    482	
> > >    483		return ret;
> > >    484	}
> > >    485	
> > > 
> > > -- 
> > > 0-DAY CI Kernel Test Service
> > > https://github.com/intel/lkp-tests/wiki
> > > 
> 

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[Junzhong Pan]

Hi Marco,

On Mon, 28 Oct 2024 22:49:56 +0100 Marco Felsch wrote:
> I found two mistakes I made in my v1. I would send a v2 if this series
> is interesting for upstream. The remaining open question is how the
> driver dependencies should be handled (see idea-1,2,3).

How's everything going? I wish you all good!
It's a very useful series for various hubs, gentle ping on it.

On Mon, 28 Oct 2024 22:49:56 +0100 Marco Felsch wrote:
> > > Idea-3:
> > > -------
> > >
> > > Adding a function to the hub.c usbcore which can be used by the
> > > usb-onboard-dev driver to register this function as hook. This removes> > > the dependency from the core and the usb-onboard-dev module is only
> > > pulled if really required. Of course this require that the hub.c usbcore> > > driver allows custom hooks.
> >
> > This seems like the best approach IMO, if USB maintainers are onboard with> > it.
Use the existing onboard_hub.h header to do the hooks looks fine.

I recently encountered some kind of platforms using an existing onboard
hub yet their HW don't utilize the USBPE port power control feature
while the hub support it.
Instead, we have another GPIO for controlling the vbus of those ports
to cut the cost.

Wonder any idea could use this driver considering the limitation of
the usb compatible set the properties of onboard_dev_pdata hard coded?

Best Regards,
Pan

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[Marco Felsch]

Hi,

On 24-12-31, Junzhong Pan wrote:
> Hi Marco,
> 
> On Mon, 28 Oct 2024 22:49:56 +0100 Marco Felsch wrote:
> > I found two mistakes I made in my v1. I would send a v2 if this series
> > is interesting for upstream. The remaining open question is how the
> > driver dependencies should be handled (see idea-1,2,3).
> 
> How's everything going? I wish you all good!

I'm fine, thanks for asking.

> It's a very useful series for various hubs, gentle ping on it.

Nice to hear that others find this series useful too :) I prepared a v2
but wanted to get some feedback from the maintainers first mainly
regarding the dependency handling.

> On Mon, 28 Oct 2024 22:49:56 +0100 Marco Felsch wrote:
> > > > Idea-3:
> > > > -------
> > > >
> > > > Adding a function to the hub.c usbcore which can be used by the
> > > > usb-onboard-dev driver to register this function as hook. This removes> > > the dependency from the core and the usb-onboard-dev module is only
> > > > pulled if really required. Of course this require that the hub.c usbcore> > > driver allows custom hooks.
> > >
> > > This seems like the best approach IMO, if USB maintainers are onboard with> > it.
> Use the existing onboard_hub.h header to do the hooks looks fine.
> 
> I recently encountered some kind of platforms using an existing onboard
> hub yet their HW don't utilize the USBPE port power control feature
> while the hub support it.
> Instead, we have another GPIO for controlling the vbus of those ports
> to cut the cost.

That's exactly our use-case too.

> Wonder any idea could use this driver considering the limitation of
> the usb compatible set the properties of onboard_dev_pdata hard coded?

Sorry but I don't get this.

Regards,
  Marco

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[Junzhong Pan]

Hi Marco,

Thank you for your reply!

On 2025/1/6 14:52, Marco Felsch Wrote:
> On 24-12-31, Junzhong Pan wrote:
>>
>> I recently encountered some kind of platforms using an existing onboard
>> hub yet their HW don't utilize the USBPE port power control feature
>> while the hub support it.
>> Instead, we have another GPIO for controlling the vbus of those ports
>> to cut the cost.
> 
> That's exactly our use-case too.
> 
>> Wonder any idea could use this driver considering the limitation of
>> the usb compatible set the properties of onboard_dev_pdata hard coded?
> 
> Sorry but I don't get this.
If the hub have 4 ports, but board only have one gpio to controll all 
those vbus at once, implemented as some kind of gang mode.

In this case, the onboard_dev driver may not respond to the 
USB_PORT_FEAT_POWER, but keep the supply always on except for the 
suspend states.

Do you have any idea how we handle this?

Best Regards,
Pan

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[Marco Felsch]

Hi,

sorry for the late response...

On 25-01-10, Junzhong Pan wrote:
> Hi Marco,
> 
> Thank you for your reply!
> 
> On 2025/1/6 14:52, Marco Felsch Wrote:
> > On 24-12-31, Junzhong Pan wrote:
> > > 
> > > I recently encountered some kind of platforms using an existing onboard
> > > hub yet their HW don't utilize the USBPE port power control feature
> > > while the hub support it.
> > > Instead, we have another GPIO for controlling the vbus of those ports
> > > to cut the cost.
> > 
> > That's exactly our use-case too.
> > 
> > > Wonder any idea could use this driver considering the limitation of
> > > the usb compatible set the properties of onboard_dev_pdata hard coded?
> > 
> > Sorry but I don't get this.
> If the hub have 4 ports, but board only have one gpio to controll all those
> vbus at once, implemented as some kind of gang mode.
> 
> In this case, the onboard_dev driver may not respond to the
> USB_PORT_FEAT_POWER, but keep the supply always on except for the suspend
> states.
> 
> Do you have any idea how we handle this?

I can think of one crude workaround. Adding 4-regulators which use the
same shared gpio. This requires the gpio to be requested as shared if
that is possible.

Regards,
  Marco

Re: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features

[kernel test robot]

Hi Marco,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0c3836482481200ead7b416ca80c68a29cfdaabd]

url:    https://github.com/intel-lab-lkp/linux/commits/Marco-Felsch/usb-hub-add-infrastructure-to-pass-onboard_dev-port-features/20240807-224100
base:   0c3836482481200ead7b416ca80c68a29cfdaabd
patch link:    https://lore.kernel.org/r/20240807-b4-v6-10-topic-usb-onboard-dev-v1-1-f33ce21353c9%40pengutronix.de
patch subject: [PATCH 1/3] usb: hub: add infrastructure to pass onboard_dev port features
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240808/202408082050.BjhmZIt6-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408082050.BjhmZIt6-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408082050.BjhmZIt6-lkp@intel.com/

All errors (new ones prefixed by >>):

   aarch64-linux-ld: Unexpected GOT/PLT entries detected!
   aarch64-linux-ld: Unexpected run-time procedure linkages detected!
   aarch64-linux-ld: drivers/usb/core/hub.o: in function `set_port_feature':
>> drivers/usb/core/hub.c:481:(.text+0x121c): undefined reference to `onboard_dev_port_feature'
   aarch64-linux-ld: drivers/usb/core/hub.o: in function `usb_clear_port_feature':
   drivers/usb/core/hub.c:462:(.text+0x23b0): undefined reference to `onboard_dev_port_feature'


vim +481 drivers/usb/core/hub.c

   466	
   467	/*
   468	 * USB 2.0 spec Section 11.24.2.13
   469	 */
   470	static int set_port_feature(struct usb_device *hdev, int port1, int feature)
   471	{
   472		int ret;
   473	
   474		ret = usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
   475			USB_REQ_SET_FEATURE, USB_RT_PORT, feature, port1,
   476			NULL, 0, 1000);
   477		if (ret)
   478			return ret;
   479	
   480		if (!is_root_hub(hdev))
 > 481			ret = onboard_dev_port_feature(hdev, true, feature, port1);
   482	
   483		return ret;
   484	}
   485	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/3] dt-bindings: usb: microchip,usb2514: add support for port vbus-supply

[Marco Felsch]

Some PCB designs don't connect the USB hub port power control GPIO and
instead make use of an host controllable regulator. Add support for this
use-case by introducing an portX-vbus-supply property.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 Documentation/devicetree/bindings/usb/microchip,usb2514.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
index 783c27591e56..51d02c4b8f2d 100644
--- a/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
+++ b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
@@ -35,6 +35,13 @@ required:
   - compatible
   - reg
 
+patternProperties:
+  "^port[1-7]-vbus-supply$"
+    type: object
+    description:
+      Regulator controlling the USB VBUS on portX. Only required if the host
+      controls the portX VBUS.
+
 unevaluatedProperties: false
 
 examples:

-- 
2.39.2

Re: [PATCH 2/3] dt-bindings: usb: microchip,usb2514: add support for port vbus-supply

[Rob Herring]

On Wed, Aug 07, 2024 at 04:36:52PM +0200, Marco Felsch wrote:
> Some PCB designs don't connect the USB hub port power control GPIO and
> instead make use of an host controllable regulator. Add support for this
> use-case by introducing an portX-vbus-supply property.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/usb/microchip,usb2514.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> index 783c27591e56..51d02c4b8f2d 100644
> --- a/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> +++ b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> @@ -35,6 +35,13 @@ required:
>    - compatible
>    - reg
>  
> +patternProperties:
> +  "^port[1-7]-vbus-supply$"
> +    type: object
> +    description:
> +      Regulator controlling the USB VBUS on portX. Only required if the host
> +      controls the portX VBUS.

This is completely external to the Microchip part, right?

I think each port node should have a 'vbus-supply' property instead.

Rob

Re: [PATCH 2/3] dt-bindings: usb: microchip,usb2514: add support for port vbus-supply

[Marco Felsch]

Hi Rob,

On 24-08-13, Rob Herring wrote:
> On Wed, Aug 07, 2024 at 04:36:52PM +0200, Marco Felsch wrote:
> > Some PCB designs don't connect the USB hub port power control GPIO and
> > instead make use of an host controllable regulator. Add support for this
> > use-case by introducing an portX-vbus-supply property.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  Documentation/devicetree/bindings/usb/microchip,usb2514.yaml | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> > index 783c27591e56..51d02c4b8f2d 100644
> > --- a/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> > +++ b/Documentation/devicetree/bindings/usb/microchip,usb2514.yaml
> > @@ -35,6 +35,13 @@ required:
> >    - compatible
> >    - reg
> >  
> > +patternProperties:
> > +  "^port[1-7]-vbus-supply$"
> > +    type: object
> > +    description:
> > +      Regulator controlling the USB VBUS on portX. Only required if the host
> > +      controls the portX VBUS.
> 
> This is completely external to the Microchip part, right?
> 
> I think each port node should have a 'vbus-supply' property instead.

This was my first approach but the problem is that we currently don't
support such use-case:

	parent-node {
		/* Parent controlling the supply of the child node */

		child-node {
			vbus-supply = <&reg>;
		};
	};

at least I didn't found such use-case. I'm happy for pointers if you
know more :) At the moment *-supply properties and device-nodes are
bound together:
	
	parent-node {
		vbus-supply = <&reg>;
	};

Regards,
  Marco

> 
> Rob
> 

[PATCH 3/3] usb: misc: onboard_dev: add ext-vbus-supply handling

[Marco Felsch]

Add support to power the port VBUS via host controlled regulators since
some embedded hub PCB designs don't connect the dedicated USB hub port
power GPIO accordingly.

To support the above use-case the USB_PORT_FEAT_POWER port feature
handling must be added. At the moment this feature is limited to the
following hubs:
  - usb424,2412
  - usb424,2414
  - usb424,2417.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/usb/misc/onboard_usb_dev.c | 63 ++++++++++++++++++++++++++++++++++++++
 drivers/usb/misc/onboard_usb_dev.h |  2 ++
 2 files changed, 65 insertions(+)

diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index f61de2c353d0..f8ca6ef9956b 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -36,6 +36,8 @@ static DECLARE_WORK(attach_usb_driver_work, onboard_dev_attach_usb_driver);
 
 /************************** Platform driver **************************/
 
+#define MAX_DOWNSTREAM_PORTS	7
+
 struct usbdev_node {
 	struct usb_device *udev;
 	struct list_head list;
@@ -52,6 +54,7 @@ struct onboard_dev {
 	struct list_head udev_list;
 	struct mutex lock;
 	struct clk *clk;
+	struct regulator *ext_vbus_supplies[MAX_DOWNSTREAM_PORTS];
 };
 
 static int onboard_dev_get_regulators(struct onboard_dev *onboard_dev)
@@ -212,6 +215,48 @@ static int onboard_dev_add_usbdev(struct onboard_dev *onboard_dev,
 	return err;
 }
 
+static int onboard_dev_port_power(struct onboard_dev *onboard_dev, int port1,
+				  bool enable)
+{
+	struct regulator *vbus_supply;
+
+	vbus_supply = onboard_dev->ext_vbus_supplies[port1 - 1];
+
+	/* External supplies are optional */
+	if (!vbus_supply)
+		return 0;
+
+	if (enable)
+		return regulator_enable(vbus_supply);
+
+	return regulator_disable(vbus_supply);
+}
+
+static int onboard_dev_add_ext_vbus_supplies(struct onboard_dev *onboard_dev)
+{
+	struct device *dev = onboard_dev->dev;
+	unsigned int i;
+
+	if (!onboard_dev->pdata->support_ext_vbus_supplies)
+		return 0;
+
+	for (i = 0; i < MAX_DOWNSTREAM_PORTS; i++) {
+		char *supply_name = "portX-vbus";
+		struct regulator *reg;
+
+		sprintf(supply_name, "port%u-vbus", i + 1);
+		reg = devm_regulator_get_optional(dev, supply_name);
+		if (!IS_ERR(reg)) {
+			onboard_dev->ext_vbus_supplies[i] = reg;
+		} else {
+			if (PTR_ERR(reg) != -ENODEV)
+				return PTR_ERR(reg);
+		}
+	}
+
+	return 0;
+}
+
 static void onboard_dev_remove_usbdev(struct onboard_dev *onboard_dev,
 				      const struct usb_device *udev)
 {
@@ -339,6 +384,10 @@ static int onboard_dev_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	err = onboard_dev_add_ext_vbus_supplies(onboard_dev);
+	if (err)
+		return err;
+
 	/*
 	 * The USB driver might have been detached from the USB devices by
 	 * onboard_dev_remove() (e.g. through an 'unbind' by userspace),
@@ -525,7 +574,21 @@ static struct usb_device_driver onboard_dev_usbdev_driver = {
 int onboard_dev_port_feature(struct usb_device *udev, bool set,
 			     int feature, int port1)
 {
+	struct device *dev = &udev->dev;
+	struct onboard_dev *onboard_dev;
+
+	if (!dev->of_node)
+		return 0;
+
+	onboard_dev = _find_onboard_dev(dev);
+	if (IS_ERR(onboard_dev))
+		return 0;
+
 	switch (feature) {
+	case USB_PORT_FEAT_POWER:
+		if (!onboard_dev->pdata->is_hub)
+			return -EINVAL;
+		return onboard_dev_port_power(onboard_dev, port1, set);
 	default:
 		return 0;
 	}
diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
index fbba549c0f47..e828bfe006ba 100644
--- a/drivers/usb/misc/onboard_usb_dev.h
+++ b/drivers/usb/misc/onboard_usb_dev.h
@@ -13,6 +13,7 @@ struct onboard_dev_pdata {
 	unsigned int num_supplies;	/* number of supplies */
 	const char * const supply_names[MAX_SUPPLIES];
 	bool is_hub;
+	bool support_ext_vbus_supplies;
 };
 
 static const struct onboard_dev_pdata microchip_usb424_data = {
@@ -20,6 +21,7 @@ static const struct onboard_dev_pdata microchip_usb424_data = {
 	.num_supplies = 1,
 	.supply_names = { "vdd" },
 	.is_hub = true,
+	.support_ext_vbus_supplies = true,
 };
 
 static const struct onboard_dev_pdata microchip_usb5744_data = {

-- 
2.39.2