[PATCH] usb: core: support interface node for simple USB devices

[PATCH] usb: core: support interface node for simple USB devices

[Catalin Popescu]

A simple usb device has a single configuration and a single interface
and is considered as a "combined node" when defined in the devicetree.
If available, its interface node is simply ignored which is a problem
whenever the interface node has subnodes. To prevent that from happening
first check for any subnode and ignore the interface node only if no
subnode was found.

Example: FTDI chip FT234XD that has only one UART interface which is
being used as a serdev by other driver.

device@1 {
	compatible = "usb403,6015";
	reg = <1>;

	#address-cells = <2>;
	#size-cells = <0>;

	interface@0 {
		compatible = "usbif403,6015.config1.0";
		reg = <0 1>;

		bluetooth {
			compatible = "nxp,88w8987-bt";
		};
	};
};

Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
 drivers/usb/core/message.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index d2b2787be409..f4a9d1831f6a 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -2102,6 +2102,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		struct usb_interface_cache *intfc;
 		struct usb_interface *intf;
 		struct usb_host_interface *alt;
+		struct device_node *np;
 		u8 ifnum;
 
 		cp->interface[i] = intf = new_interfaces[i];
@@ -2126,12 +2127,11 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		intf->cur_altsetting = alt;
 		usb_enable_interface(dev, intf, true);
 		intf->dev.parent = &dev->dev;
-		if (usb_of_has_combined_node(dev)) {
+		np = usb_of_get_interface_node(dev, configuration, ifnum);
+		if (!of_get_child_count(np) && usb_of_has_combined_node(dev))
 			device_set_of_node_from_dev(&intf->dev, &dev->dev);
-		} else {
-			intf->dev.of_node = usb_of_get_interface_node(dev,
-					configuration, ifnum);
-		}
+		else
+			intf->dev.of_node = np;
 		ACPI_COMPANION_SET(&intf->dev, ACPI_COMPANION(&dev->dev));
 		intf->dev.driver = NULL;
 		intf->dev.bus = &usb_bus_type;

base-commit: f066b5a6c7a06adfb666b7652cc99b4ff264f4ed
-- 
2.34.1

Re: [PATCH] usb: core: support interface node for simple USB devices

[Greg KH]

On Wed, Jan 22, 2025 at 02:47:32PM +0100, Catalin Popescu wrote:
> A simple usb device has a single configuration and a single interface
> and is considered as a "combined node" when defined in the devicetree.
> If available, its interface node is simply ignored which is a problem
> whenever the interface node has subnodes. To prevent that from happening
> first check for any subnode and ignore the interface node only if no
> subnode was found.
> 
> Example: FTDI chip FT234XD that has only one UART interface which is
> being used as a serdev by other driver.
> 
> device@1 {
> 	compatible = "usb403,6015";
> 	reg = <1>;
> 
> 	#address-cells = <2>;
> 	#size-cells = <0>;
> 
> 	interface@0 {
> 		compatible = "usbif403,6015.config1.0";
> 		reg = <0 1>;
> 
> 		bluetooth {
> 			compatible = "nxp,88w8987-bt";
> 		};
> 	};
> };

serdev can not use usb-serial devices due to the lack of hotplugging, so
why is this an issue?  I thought that just would not work.

confused,

greg k-h

Re: [PATCH] usb: core: support interface node for simple USB devices

[POPESCU Catalin]

On 22/01/2025 14:53, Greg KH wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Wed, Jan 22, 2025 at 02:47:32PM +0100, Catalin Popescu wrote:
>> A simple usb device has a single configuration and a single interface
>> and is considered as a "combined node" when defined in the devicetree.
>> If available, its interface node is simply ignored which is a problem
>> whenever the interface node has subnodes. To prevent that from happening
>> first check for any subnode and ignore the interface node only if no
>> subnode was found.
>>
>> Example: FTDI chip FT234XD that has only one UART interface which is
>> being used as a serdev by other driver.
>>
>> device@1 {
>>        compatible = "usb403,6015";
>>        reg = <1>;
>>
>>        #address-cells = <2>;
>>        #size-cells = <0>;
>>
>>        interface@0 {
>>                compatible = "usbif403,6015.config1.0";
>>                reg = <0 1>;
>>
>>                bluetooth {
>>                        compatible = "nxp,88w8987-bt";
>>                };
>>        };
>> };
> serdev can not use usb-serial devices due to the lack of hotplugging, so
> why is this an issue?  I thought that just would not work.

Well, I can say that on 6.12.0 it's working :

[    0.929493] usb 1-1: new full-speed USB device number 2 using xhci-hcd
[    5.906574] usbcore: registered new interface driver ftdi_sio
[    5.906632] usbserial: USB Serial support registered for FTDI USB 
Serial Device
[    5.906727] ftdi_sio 1-1:1.0: FTDI USB Serial Device converter detected
[    5.906842] usb 1-1: Detected FT-X
[    5.907809] serial serial0: tty port ttyUSB0 registered
[    5.907832] usb 1-1: FTDI USB Serial Device converter now attached to 
ttyUSB0
[    5.918649] btnxpuart serial0-0: supply vcc not found, using dummy 
regulator

==> unplug FTDI from USB bus

[  109.410409] usb 1-1: USB disconnect, device number 2
[  109.432556] ftdi_sio ttyUSB0: FTDI USB Serial Device converter now 
disconnected from ttyUSB0
[  109.432634] ftdi_sio 1-1:1.0: device disconnected

==> plug back FTDI

[  150.403378] usb 1-1: new full-speed USB device number 3 using xhci-hcd
[  150.553794] ftdi_sio 1-1:1.0: FTDI USB Serial Device converter detected
[  150.553920] usb 1-1: Detected FT-X
[  150.554718] serial serial0: tty port ttyUSB0 registered
[  150.554731] usb 1-1: FTDI USB Serial Device converter now attached to 
ttyUSB0
[  150.554976] btnxpuart serial0-0: supply vcc not found, using dummy 
regulator

btnxpuart is basically the bluetooth driver that's behind the FTDI and 
uses ttyUSB0 to communicate with the bluetooth chip.
I can confirm that after unplug/plug the communication with the 
bluetooth chip is perfectly working.

BR,
Catalin

> confused,
>
> greg k-h

Re: [PATCH] usb: core: support interface node for simple USB devices

[Johan Hovold]

On Wed, Jan 22, 2025 at 05:17:36PM +0000, POPESCU Catalin wrote:
> On 22/01/2025 14:53, Greg KH wrote:

> > serdev can not use usb-serial devices due to the lack of hotplugging, so
> > why is this an issue?  I thought that just would not work.
> 
> Well, I can say that on 6.12.0 it's working :
> 
> [    0.929493] usb 1-1: new full-speed USB device number 2 using xhci-hcd
> [    5.906574] usbcore: registered new interface driver ftdi_sio
> [    5.906632] usbserial: USB Serial support registered for FTDI USB 
> Serial Device
> [    5.906727] ftdi_sio 1-1:1.0: FTDI USB Serial Device converter detected
> [    5.906842] usb 1-1: Detected FT-X
> [    5.907809] serial serial0: tty port ttyUSB0 registered

You'd need to patch USB serial too for this to happen so this in not
mainline 6.12.

And if the port is open during disconnect you'd see a bunch of warnings
because the serdev tty port implementation does not handle hangups.

Johan

Re: [PATCH] usb: core: support interface node for simple USB devices

[POPESCU Catalin]

On 23/01/2025 12:18, Johan Hovold wrote:
> [Some people who received this message don't often get email from johan@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Wed, Jan 22, 2025 at 05:17:36PM +0000, POPESCU Catalin wrote:
>> On 22/01/2025 14:53, Greg KH wrote:
>>> serdev can not use usb-serial devices due to the lack of hotplugging, so
>>> why is this an issue?  I thought that just would not work.
>> Well, I can say that on 6.12.0 it's working :
>>
>> [    0.929493] usb 1-1: new full-speed USB device number 2 using xhci-hcd
>> [    5.906574] usbcore: registered new interface driver ftdi_sio
>> [    5.906632] usbserial: USB Serial support registered for FTDI USB
>> Serial Device
>> [    5.906727] ftdi_sio 1-1:1.0: FTDI USB Serial Device converter detected
>> [    5.906842] usb 1-1: Detected FT-X
>> [    5.907809] serial serial0: tty port ttyUSB0 registered
> You'd need to patch USB serial too for this to happen so this in not
> mainline 6.12.
>
> And if the port is open during disconnect you'd see a bunch of warnings
> because the serdev tty port implementation does not handle hangups.
>
> Johan

My bad, I got a reminder that we're using a set of patches that add 
support for the missing pieces :
https://lore.kernel.org/all/20240807-v6-10-topic-usb-serial-serdev-v1-0-ed2cc5da591f@pengutronix.de/

Sorry for the confusion.

BR,
Catalin

Re: [PATCH] usb: core: support interface node for simple USB devices

[Johan Hovold]

On Wed, Jan 22, 2025 at 02:47:32PM +0100, Catalin Popescu wrote:
> A simple usb device has a single configuration and a single interface
> and is considered as a "combined node" when defined in the devicetree.
> If available, its interface node is simply ignored which is a problem
> whenever the interface node has subnodes. To prevent that from happening
> first check for any subnode and ignore the interface node only if no
> subnode was found.
> 
> Example: FTDI chip FT234XD that has only one UART interface which is
> being used as a serdev by other driver.
> 
> device@1 {
> 	compatible = "usb403,6015";
> 	reg = <1>;
> 
> 	#address-cells = <2>;
> 	#size-cells = <0>;
> 
> 	interface@0 {
> 		compatible = "usbif403,6015.config1.0";

Your example makes no sense since if this is the only interface then the
interface node should not be here.

> 		reg = <0 1>;
> 
> 		bluetooth {
> 			compatible = "nxp,88w8987-bt";
> 		};
> 	};
> };

And as Greg said, serdev is not enabled for USB serial as serdev does
not currently handle hotplugging (hangups).

The hotplug issue would need to be addressed first, and then we'd also
need a DT binding for usb serial devices which can have multiple ports
(per interface).

Johan

Re: [PATCH] usb: core: support interface node for simple USB devices

[POPESCU Catalin]

On 23/01/2025 12:15, Johan Hovold wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Wed, Jan 22, 2025 at 02:47:32PM +0100, Catalin Popescu wrote:
>> A simple usb device has a single configuration and a single interface
>> and is considered as a "combined node" when defined in the devicetree.
>> If available, its interface node is simply ignored which is a problem
>> whenever the interface node has subnodes. To prevent that from happening
>> first check for any subnode and ignore the interface node only if no
>> subnode was found.
>>
>> Example: FTDI chip FT234XD that has only one UART interface which is
>> being used as a serdev by other driver.
>>
>> device@1 {
>>        compatible = "usb403,6015";
>>        reg = <1>;
>>
>>        #address-cells = <2>;
>>        #size-cells = <0>;
>>
>>        interface@0 {
>>                compatible = "usbif403,6015.config1.0";
> Your example makes no sense since if this is the only interface then the
> interface node should not be here.

That's the problem my patch is trying to address ...
Why is it OK to describe multiple interfaces and it is not OK to 
describe the interface of a simple USB device ?
I don't really understand the reason behind, for me it looks completely 
arbitrary.

>
>>                reg = <0 1>;
>>
>>                bluetooth {
>>                        compatible = "nxp,88w8987-bt";
>>                };
>>        };
>> };
> And as Greg said, serdev is not enabled for USB serial as serdev does
> not currently handle hotplugging (hangups).
>
> The hotplug issue would need to be addressed first, and then we'd also
> need a DT binding for usb serial devices which can have multiple ports
> (per interface).
>
> Johan

Re: [PATCH] usb: core: support interface node for simple USB devices

[Johan Hovold]

On Thu, Jan 23, 2025 at 02:01:00PM +0000, POPESCU Catalin wrote:
> On 23/01/2025 12:15, Johan Hovold wrote:
> >
> > On Wed, Jan 22, 2025 at 02:47:32PM +0100, Catalin Popescu wrote:
> >> A simple usb device has a single configuration and a single interface
> >> and is considered as a "combined node" when defined in the devicetree.
> >> If available, its interface node is simply ignored which is a problem
> >> whenever the interface node has subnodes. To prevent that from happening
> >> first check for any subnode and ignore the interface node only if no
> >> subnode was found.
> >>
> >> Example: FTDI chip FT234XD that has only one UART interface which is
> >> being used as a serdev by other driver.
> >>
> >> device@1 {
> >>        compatible = "usb403,6015";
> >>        reg = <1>;
> >>
> >>        #address-cells = <2>;
> >>        #size-cells = <0>;
> >>
> >>        interface@0 {
> >>                compatible = "usbif403,6015.config1.0";
> > Your example makes no sense since if this is the only interface then the
> > interface node should not be here.
> 
> That's the problem my patch is trying to address ...
> Why is it OK to describe multiple interfaces and it is not OK to 
> describe the interface of a simple USB device ?

It's part of the spec. See commit 1a7e3948cb9f ("USB: add device-tree
support for interfaces") and its reference to "Open Firmware Recommended
Practice: Universal Serial Bus Version 1".

Johan

Re: [PATCH] usb: core: support interface node for simple USB devices

[POPESCU Catalin]

On 23/01/2025 16:20, Johan Hovold wrote:
> [Some people who received this message don't often get email from johan@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On Thu, Jan 23, 2025 at 02:01:00PM +0000, POPESCU Catalin wrote:
>> On 23/01/2025 12:15, Johan Hovold wrote:
>>> On Wed, Jan 22, 2025 at 02:47:32PM +0100, Catalin Popescu wrote:
>>>> A simple usb device has a single configuration and a single interface
>>>> and is considered as a "combined node" when defined in the devicetree.
>>>> If available, its interface node is simply ignored which is a problem
>>>> whenever the interface node has subnodes. To prevent that from happening
>>>> first check for any subnode and ignore the interface node only if no
>>>> subnode was found.
>>>>
>>>> Example: FTDI chip FT234XD that has only one UART interface which is
>>>> being used as a serdev by other driver.
>>>>
>>>> device@1 {
>>>>         compatible = "usb403,6015";
>>>>         reg = <1>;
>>>>
>>>>         #address-cells = <2>;
>>>>         #size-cells = <0>;
>>>>
>>>>         interface@0 {
>>>>                 compatible = "usbif403,6015.config1.0";
>>> Your example makes no sense since if this is the only interface then the
>>> interface node should not be here.
>> That's the problem my patch is trying to address ...
>> Why is it OK to describe multiple interfaces and it is not OK to
>> describe the interface of a simple USB device ?
> It's part of the spec. See commit 1a7e3948cb9f ("USB: add device-tree
> support for interfaces") and its reference to "Open Firmware Recommended
> Practice: Universal Serial Bus Version 1".
I see, thanks a lot for the clarification.
BR,
> Johan