| Message ID | 20250122134732.2318554-1-catalin.popescu@leica-geosystems.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: core: support interface node for simple USB devices | expand |
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
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
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
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
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
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
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
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
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;
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(-) base-commit: f066b5a6c7a06adfb666b7652cc99b4ff264f4ed