mbox series

[0/3] USB-Serial serdev support

Message ID 20240807-v6-10-topic-usb-serial-serdev-v1-0-ed2cc5da591f@pengutronix.de (mailing list archive)
Headers show
Series USB-Serial serdev support | expand

Message

Marco Felsch Aug. 7, 2024, 2:08 p.m. UTC
Hi,

this patchset is based on Johan's patches [1] but dropped the need of
the special 'serial' of-node [2].

With the patches in place and the usb hierarchy described in properly we
can use serdev on-top of usb-serial. The below example adds the support
for the following hierarchy:
 - host->usb-hub->ftdi-usb-uart->bt/wlan-module:

&usb_dwc3_1 {
	dr_mode = "host";
	status = "okay";

	hub@1 {
		compatible = "usb424,2514";
		reg = <1>;

		vdd-supply = <&reg>;
		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>;

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

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

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

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

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

				bluetooth {
					compatible = "nxp,88w8987-bt";
					fw-init-baudrate = <3000000>;
				};
			};
		};
	};
};

If no serdev node is found the usb-serial is exposed as usual and can be
accessed via /dev/ttyUSBx.

Regards,
  Marco

[1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of
[2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Marco Felsch (3):
      serdev: ttyport: make use of tty_kopen_exclusive
      USB: serial: cosmetic cleanup <space><tab> mix
      USB: serial: enable serdev support

 drivers/tty/serdev/serdev-ttyport.c |  9 ++++++---
 drivers/usb/serial/bus.c            | 10 ++++++----
 2 files changed, 12 insertions(+), 7 deletions(-)
---
base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
change-id: 20240807-v6-10-topic-usb-serial-serdev-83a7f8f86432

Best regards,

Comments

Johan Hovold Sept. 9, 2024, 12:03 p.m. UTC | #1
On Wed, Aug 07, 2024 at 04:08:47PM +0200, Marco Felsch wrote:
> this patchset is based on Johan's patches [1] but dropped the need of
> the special 'serial' of-node [2].

That's great that you found and referenced my proof-of-concept patches,
but it doesn't seem like you tried to understand why this hasn't been
merged yet.

First, as the commit message you refer to below explain, we need some
way to describe multiport controllers. Just dropping the 'serial' node
does not make that issue go away.

Second, and more importantly, you do not address the main obstacle for
enabling serdev for USB serial which is that the serdev cannot handle
hotplugging.


> [1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f

Johan
Marco Felsch Sept. 17, 2024, 4:49 a.m. UTC | #2
Hi Johan,

On 24-09-09, Johan Hovold wrote:
> On Wed, Aug 07, 2024 at 04:08:47PM +0200, Marco Felsch wrote:
> > this patchset is based on Johan's patches [1] but dropped the need of
> > the special 'serial' of-node [2].
> 
> That's great that you found and referenced my proof-of-concept patches,
> but it doesn't seem like you tried to understand why this hasn't been
> merged yet.

I'm glad for your input.

> First, as the commit message you refer to below explain, we need some
> way to describe multiport controllers. Just dropping the 'serial' node
> does not make that issue go away.

Sorry for asking but isn't the current OF abstraction [1] enough? As far
as I understood we can describe the whole USB tree within OF. I used [1]
and the this patchset to describe the following hierarchy:

 usb-root -> usb-hub port-1 -> usb-serial interface-0 -> serial
                                                         bt-module

[1] Documentation/devicetree/bindings/usb/usb-device.yaml

> Second, and more importantly, you do not address the main obstacle for
> enabling serdev for USB serial which is that the serdev cannot handle
> hotplugging.

Hotplugging is a good point but out-of-scope IMHO (at least for now)
since the current serdev implementation rely on additional firmware
information e.g OF node to be present. E.g. if the above mentioned setup
would connect the "serial bt-module" directly to the UART port you still
need an OF node to bind the serdev driver. If the node isn't present
user-space would need to do the hci handling.

So from my POV the serdev abstraction is for manufacturers which make
use of "onboard" usb-devices which are always present at the same USB
tree location. Serdev is not made for general purpose USB ports (yet)
where a user can plug-in all types of USB devices.

Regards,
  Marco

> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f
> 
> Johan
>
Marco Felsch Oct. 1, 2024, 7:24 a.m. UTC | #3
Hi,

gentle ping as this is series is two months old now.

Regards,
  Marco

On 24-08-07, Marco Felsch wrote:
> Hi,
> 
> this patchset is based on Johan's patches [1] but dropped the need of
> the special 'serial' of-node [2].
> 
> With the patches in place and the usb hierarchy described in properly we
> can use serdev on-top of usb-serial. The below example adds the support
> for the following hierarchy:
>  - host->usb-hub->ftdi-usb-uart->bt/wlan-module:
> 
> &usb_dwc3_1 {
> 	dr_mode = "host";
> 	status = "okay";
> 
> 	hub@1 {
> 		compatible = "usb424,2514";
> 		reg = <1>;
> 
> 		vdd-supply = <&reg>;
> 		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>;
> 
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		device@1 {
> 			compatible = "usb403,6010";
> 			reg = <1>;
> 
> 			#address-cells = <2>;
> 			#size-cells = <0>;
> 
> 			interface@0 {
> 				compatible = "usbif403,6010.config1.0";
> 				reg = <0 1>;
> 
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 
> 				bluetooth {
> 					compatible = "nxp,88w8987-bt";
> 					fw-init-baudrate = <3000000>;
> 				};
> 			};
> 		};
> 	};
> };
> 
> If no serdev node is found the usb-serial is exposed as usual and can be
> accessed via /dev/ttyUSBx.
> 
> Regards,
>   Marco
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Marco Felsch (3):
>       serdev: ttyport: make use of tty_kopen_exclusive
>       USB: serial: cosmetic cleanup <space><tab> mix
>       USB: serial: enable serdev support
> 
>  drivers/tty/serdev/serdev-ttyport.c |  9 ++++++---
>  drivers/usb/serial/bus.c            | 10 ++++++----
>  2 files changed, 12 insertions(+), 7 deletions(-)
> ---
> base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
> change-id: 20240807-v6-10-topic-usb-serial-serdev-83a7f8f86432
> 
> Best regards,
> -- 
> Marco Felsch <m.felsch@pengutronix.de>
> 
>
Greg KH Oct. 1, 2024, 7:29 a.m. UTC | #4
On Tue, Oct 01, 2024 at 09:24:53AM +0200, Marco Felsch wrote:
> Hi,
> 
> gentle ping as this is series is two months old now.

And it was rejected as serdev does not support hotplug which of course,
usb-serial does.

So until serdev is fixed up to handle that correctly, this is not going
anywhere, nor should you want it to as then you would be in charge of
code that does not work properly :)

thanks,

greg k-h
Marco Felsch Oct. 1, 2024, 7:47 a.m. UTC | #5
On 24-10-01, Greg Kroah-Hartman wrote:
> On Tue, Oct 01, 2024 at 09:24:53AM +0200, Marco Felsch wrote:
> > Hi,
> > 
> > gentle ping as this is series is two months old now.
> 
> And it was rejected as serdev does not support hotplug which of course,
> usb-serial does.

I just read concerns which I tried to explain/argue but didn't saw it as
rejected.

> So until serdev is fixed up to handle that correctly, this is not going
> anywhere, nor should you want it to as then you would be in charge of
> code that does not work properly :)

Sure as always :)

Regards,
  Marco
Marco Felsch Oct. 28, 2024, 10:57 p.m. UTC | #6
Hi Greg,

On 24-10-01, Greg Kroah-Hartman wrote:
> On Tue, Oct 01, 2024 at 09:24:53AM +0200, Marco Felsch wrote:
> > Hi,
> > 
> > gentle ping as this is series is two months old now.
> 
> And it was rejected as serdev does not support hotplug which of course,
> usb-serial does.

I hoped to get some feedback on my answer [1]. Regarding hotplug
support: serdev _requires_ some sort of firmware like OF (not sure if it
does work with ACPI too). That said, if serdev finds no firmware a
fallback is provided to the standard serial handling.

The firmware could either be added directly by the platform OF file or
via OF-overlays. By making use of overlays we could gain some kind of
hotplug: Once a usb devices was detected and the driver has an
overlay, the overlay gets applied and the probe continues, like we do it
for PCIe devices now [2]. For devices which don't have a registered
overlay the standard usb-serial setup is done by exposing the serial
interface to the userspace.

> So until serdev is fixed up to handle that correctly, this is not going
> anywhere, nor should you want it to as then you would be in charge of
> code that does not work properly :)

Regards,
  Marco

[1] https://lore.kernel.org/all/20240917044948.i2eog4ondf7vna7q@pengutronix.de/
[2] https://lore.kernel.org/all/7512cbb7911b8395d926e9e9e390fbb55ce3aea9.camel@pengutronix.de/

> 
> thanks,
> 
> greg k-h
>
Marco Felsch March 3, 2025, 11:25 a.m. UTC | #7
Hi,

On 24-10-28, Marco Felsch wrote:
> Hi Greg,
> 
> On 24-10-01, Greg Kroah-Hartman wrote:
> > On Tue, Oct 01, 2024 at 09:24:53AM +0200, Marco Felsch wrote:
> > > Hi,
> > > 
> > > gentle ping as this is series is two months old now.
> > 
> > And it was rejected as serdev does not support hotplug which of course,
> > usb-serial does.
> 
> I hoped to get some feedback on my answer [1]. Regarding hotplug
> support: serdev _requires_ some sort of firmware like OF (not sure if it
> does work with ACPI too). That said, if serdev finds no firmware a
> fallback is provided to the standard serial handling.
> 
> The firmware could either be added directly by the platform OF file or
> via OF-overlays. By making use of overlays we could gain some kind of
> hotplug: Once a usb devices was detected and the driver has an
> overlay, the overlay gets applied and the probe continues, like we do it
> for PCIe devices now [2]. For devices which don't have a registered
> overlay the standard usb-serial setup is done by exposing the serial
> interface to the userspace.

is this idea worth to give it a try for a v2 or do you have something
different in mind?

I'm happy for any input.

Regards,
  Marco

> > So until serdev is fixed up to handle that correctly, this is not going
> > anywhere, nor should you want it to as then you would be in charge of
> > code that does not work properly :)
> 
> Regards,
>   Marco
> 
> [1] https://lore.kernel.org/all/20240917044948.i2eog4ondf7vna7q@pengutronix.de/
> [2] https://lore.kernel.org/all/7512cbb7911b8395d926e9e9e390fbb55ce3aea9.camel@pengutronix.de/
> 
> > 
> > thanks,
> > 
> > greg k-h
> >
Johan Hovold March 11, 2025, 8:12 a.m. UTC | #8
On Tue, Sep 17, 2024 at 06:49:48AM +0200, Marco Felsch wrote:
> On 24-09-09, Johan Hovold wrote:
> > On Wed, Aug 07, 2024 at 04:08:47PM +0200, Marco Felsch wrote:
> > > this patchset is based on Johan's patches [1] but dropped the need of
> > > the special 'serial' of-node [2].
> > 
> > That's great that you found and referenced my proof-of-concept patches,
> > but it doesn't seem like you tried to understand why this hasn't been
> > merged yet.

> > First, as the commit message you refer to below explain, we need some
> > way to describe multiport controllers. Just dropping the 'serial' node
> > does not make that issue go away.
> 
> Sorry for asking but isn't the current OF abstraction [1] enough? As far
> as I understood we can describe the whole USB tree within OF. I used [1]
> and the this patchset to describe the following hierarchy:
> 
>  usb-root -> usb-hub port-1 -> usb-serial interface-0 -> serial
>                                                          bt-module
> 
> [1] Documentation/devicetree/bindings/usb/usb-device.yaml

Again, you still need to consider devices with multiple serial ports
(and they do not always map neatly to one port per interface either).

> > Second, and more importantly, you do not address the main obstacle for
> > enabling serdev for USB serial which is that the serdev cannot handle
> > hotplugging.
> 
> Hotplugging is a good point but out-of-scope IMHO (at least for now)
> since the current serdev implementation rely on additional firmware
> information e.g OF node to be present. E.g. if the above mentioned setup
> would connect the "serial bt-module" directly to the UART port you still
> need an OF node to bind the serdev driver. If the node isn't present
> user-space would need to do the hci handling.

There's nothing preventing you from adding a devicetree node for a USB
device that can be unplugged.

> So from my POV the serdev abstraction is for manufacturers which make
> use of "onboard" usb-devices which are always present at the same USB
> tree location. Serdev is not made for general purpose USB ports (yet)
> where a user can plug-in all types of USB devices.

Right, but someone need to make sure that serdev can handle devices
going away first as nothing is currently preventing that from happening.

> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of
> > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f

Johan
Johan Hovold March 11, 2025, 8:20 a.m. UTC | #9
On Mon, Oct 28, 2024 at 11:57:02PM +0100, Marco Felsch wrote:

> On 24-10-01, Greg Kroah-Hartman wrote:
> > On Tue, Oct 01, 2024 at 09:24:53AM +0200, Marco Felsch wrote:

> > > gentle ping as this is series is two months old now.
> > 
> > And it was rejected as serdev does not support hotplug which of course,
> > usb-serial does.
> 
> I hoped to get some feedback on my answer [1]. Regarding hotplug
> support: serdev _requires_ some sort of firmware like OF (not sure if it
> does work with ACPI too). That said, if serdev finds no firmware a
> fallback is provided to the standard serial handling.

It's devices going away not being supported which is the main concern.
The serdev ttyport implementation does not implement hangup() which is
used for serial port tear down.

> The firmware could either be added directly by the platform OF file or
> via OF-overlays. By making use of overlays we could gain some kind of
> hotplug: Once a usb devices was detected and the driver has an
> overlay, the overlay gets applied and the probe continues, like we do it
> for PCIe devices now [2]. For devices which don't have a registered
> overlay the standard usb-serial setup is done by exposing the serial
> interface to the userspace.

Then it would also be nice to have a way to describe hotplugged devices
on the fly, and overlays could indeed be used for that. But that's a
separate story.

Johan
Marco Felsch March 13, 2025, 7:40 p.m. UTC | #10
On 25-03-11, Johan Hovold wrote:
> On Tue, Sep 17, 2024 at 06:49:48AM +0200, Marco Felsch wrote:
> > On 24-09-09, Johan Hovold wrote:
> > > On Wed, Aug 07, 2024 at 04:08:47PM +0200, Marco Felsch wrote:
> > > > this patchset is based on Johan's patches [1] but dropped the need of
> > > > the special 'serial' of-node [2].
> > > 
> > > That's great that you found and referenced my proof-of-concept patches,
> > > but it doesn't seem like you tried to understand why this hasn't been
> > > merged yet.
> 
> > > First, as the commit message you refer to below explain, we need some
> > > way to describe multiport controllers. Just dropping the 'serial' node
> > > does not make that issue go away.
> > 
> > Sorry for asking but isn't the current OF abstraction [1] enough? As far
> > as I understood we can describe the whole USB tree within OF. I used [1]
> > and the this patchset to describe the following hierarchy:
> > 
> >  usb-root -> usb-hub port-1 -> usb-serial interface-0 -> serial
> >                                                          bt-module
> > 
> > [1] Documentation/devicetree/bindings/usb/usb-device.yaml
> 
> Again, you still need to consider devices with multiple serial ports
> (and they do not always map neatly to one port per interface either).

We use a dual-port FTDI and our USB tree looks as followed:

/:  Bus 001.Port 001: Dev 001, Class=root_hub, Driver=xhci-hcd/1p, 480M
    ID 1d6b:0002 Linux Foundation 2.0 root hub
    |__ Port 001: Dev 002, If 0, Class=Hub, Driver=hub/4p, 480M
        ID 0424:2514 Microchip Technology, Inc. (formerly SMSC) USB 2.0 Hub
        |__ Port 001: Dev 003, If 0, Class=Vendor Specific Class, Driver=ftdi_sio, 480M
            ID 0403:6010 Future Technology Devices International, Ltd FT2232C/D/H Dual UART/FIFO IC
        |__ Port 001: Dev 003, If 1, Class=Vendor Specific Class, Driver=ftdi_sio, 480M
            ID 0403:6010 Future Technology Devices International, Ltd FT2232C/D/H Dual UART/FIFO IC

interface-0 is used for the bt-module which is served by the serdev
driver.

interface-1 is used by an userspace driver which makes use of the
/dev/ttyUSB1 port.

So we do have the multiple serial ports use-case already. Can you please
explain what I miss?

> > > Second, and more importantly, you do not address the main obstacle for
> > > enabling serdev for USB serial which is that the serdev cannot handle
> > > hotplugging.
> > 
> > Hotplugging is a good point but out-of-scope IMHO (at least for now)
> > since the current serdev implementation rely on additional firmware
> > information e.g OF node to be present. E.g. if the above mentioned setup
> > would connect the "serial bt-module" directly to the UART port you still
> > need an OF node to bind the serdev driver. If the node isn't present
> > user-space would need to do the hci handling.
> 
> There's nothing preventing you from adding a devicetree node for a USB
> device that can be unplugged.

I see and I have to admit that I didn't test this :/ But since you
pointed it out I tested it now!

So as explained, our USB tree looks as above and our DTS looks like the
one in the cover letter. Of course I run on an embedded system but the
USB FTDI based module is powered by the VBUS of the hub. Therefore I
ran the test by disabling the downstream port which in turn disabled the
VBUS supply. This should come very close to a physical unplug event.

8<----------------------------------------------------------------

## The test system before the "unplug"

root@test:~# ls -al /sys/class/bluetooth/
total 0
drwxr-xr-x  2 root root 0 Jan  8 18:31 .
drwxr-xr-x 62 root root 0 Jan  8 18:31 ..
lrwxrwxrwx  1 root root 0 Jan  8 18:31 hci0 -> ../../devices/platform/soc@0/32f10108.usb/38200000.usb/xhci-hcd.1.auto/usb1/1-1/1-1.1/1-1.1:1.0/ttyUSB0/serial0/serial0-0/bluetooth/hci0

root@test:~# ls -al /sys/bus/serial/devices/
total 0
drwxr-xr-x 2 root root 0 Jan  8 18:31 .
drwxr-xr-x 4 root root 0 Jan  8 18:28 ..
lrwxrwxrwx 1 root root 0 Jan  8 18:31 serial0 -> ../../../devices/platform/soc@0/32f10108.usb/38200000.usb/xhci-hcd.1.auto/usb1/1-1/1-1.1/1-1.1:1.0/ttyUSB0/serial0
lrwxrwxrwx 1 root root 0 Jan  8 18:31 serial0-0 -> ../../../devices/platform/soc@0/32f10108.usb/38200000.usb/xhci-hcd.1.auto/usb1/1-1/1-1.1/1-1.1:1.0/ttyUSB0/serial0/serial0-0

## The "unplug" event and the system after the event

root@test:~# echo 1 > /sys/bus/usb/devices/usb1/1-1/1-1\:1.0/1-1-port1/disable

root@test:~# ls -al /sys/class/bluetooth/
total 0
drwxr-xr-x  2 root root 0 Jan  8 18:40 .
drwxr-xr-x 62 root root 0 Jan  8 18:31 ..

root@test:~# ls -al /sys/bus/serial/devices/
total 0
drwxr-xr-x 2 root root 0 Jan  8 18:40 .
drwxr-xr-x 4 root root 0 Jan  8 18:28 ..

## The "plug" event and the system after the event

root@test:~# echo 0 > /sys/bus/usb/devices/usb1/1-1/1-1\:1.0/1-1-port1/disable
root@test:~# [ 1121.297918] btnxpuart serial0-0: supply vcc not found, using dummy regulator

root@test:~# ls -al /sys/class/bluetooth/
total 0
drwxr-xr-x  2 root root 0 Jan  8 18:41 .
drwxr-xr-x 62 root root 0 Jan  8 18:31 ..
lrwxrwxrwx  1 root root 0 Jan  8 18:41 hci0 -> ../../devices/platform/soc@0/32f10108.usb/38200000.usb/xhci-hcd.1.auto/usb1/1-1/1-1.1/1-1.1:1.0/ttyUSB0/serial0/serial0-0/bluetooth/hci0

root@test:~# ls -al /sys/bus/serial/devices/
total 0
drwxr-xr-x 2 root root 0 Jan  8 18:41 .
drwxr-xr-x 4 root root 0 Jan  8 18:28 ..
lrwxrwxrwx 1 root root 0 Jan  8 18:41 serial0 -> ../../../devices/platform/soc@0/32f10108.usb/38200000.usb/xhci-hcd.1.auto/usb1/1-1/1-1.1/1-1.1:1.0/ttyUSB0/serial0
lrwxrwxrwx 1 root root 0 Jan  8 18:41 serial0-0 -> ../../../devices/platform/soc@0/32f10108.usb/38200000.usb/xhci-hcd.1.auto/usb1/1-1/1-1.1/1-1.1:1.0/ttyUSB0/serial0/serial0-0

8<----------------------------------------------------------------

> > So from my POV the serdev abstraction is for manufacturers which make
> > use of "onboard" usb-devices which are always present at the same USB
> > tree location. Serdev is not made for general purpose USB ports (yet)
> > where a user can plug-in all types of USB devices.
> 
> Right, but someone need to make sure that serdev can handle devices
> going away first as nothing is currently preventing that from happening.

Can you please check my above tests? Maybe I do miss something but for
me it looks like it's working. Looking forwards for your input.

Regards,
  Marco


> > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of
> > > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f
> 
> Johan
>