| Message ID | 20141211215943.4127.24792.stgit@notabene.brown |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi, On Fri, Dec 12, 2014 at 08:59:44AM +1100, NeilBrown wrote: > A "tty slave" is a device connected via UART. > It may need a driver to, for example, power the device on > when the tty is opened, and power it off when the tty > is released. > > A "tty slave" is a platform device which is declared as a > child of the uart in device-tree: > > &uart1 { > bluetooth { > compatible = "tty,regulator"; > vdd-supply = <&vaux4>; > }; > }; > > runtime power management is used to power-up the device > on tty_open() and power-down on tty_release(). > > Signed-off-by: NeilBrown <neilb@suse.de> FWIW: Acked-By: Sebastian Reichel <sre@kernel.org> -- Sebastian
On 12/11/2014 04:59 PM, NeilBrown wrote: > A "tty slave" is a device connected via UART. > It may need a driver to, for example, power the device on > when the tty is opened, and power it off when the tty > is released. > > A "tty slave" is a platform device which is declared as a > child of the uart in device-tree: > > &uart1 { > bluetooth { > compatible = "tty,regulator"; > vdd-supply = <&vaux4>; > }; > }; > > runtime power management is used to power-up the device > on tty_open() and power-down on tty_release(). I have a couple of issues with this: 1) why does a child device imply power management and a platform device? iow, what happens when someone else wants to have a child device that does something different? 2) why is this tied to the tty core and not the serial core if this is only for UART? Regards, Peter Hurley > Signed-off-by: NeilBrown <neilb@suse.de> > --- > .../devicetree/bindings/serial/of-serial.txt | 4 ++++ > drivers/tty/tty_io.c | 22 ++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt > index 8c4fd0332028..b59501ee2f21 100644 > --- a/Documentation/devicetree/bindings/serial/of-serial.txt > +++ b/Documentation/devicetree/bindings/serial/of-serial.txt > @@ -39,6 +39,10 @@ Optional properties: > driver is allowed to detect support for the capability even without this > property. > > +Optional child node: > +- a platform device listed as a child node will be probed and > + powered-on whenever the tty is in use (open). > + > Example: > > uart@80230000 { > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > index 0508a1d8e4cd..7acdc6f093f4 100644 > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -95,6 +95,8 @@ > #include <linux/seq_file.h> > #include <linux/serial.h> > #include <linux/ratelimit.h> > +#include <linux/pm_runtime.h> > +#include <linux/of_platform.h> > > #include <linux/uaccess.h> > > @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty, > return 0; > } > > +static int open_child(struct device *dev, void *data) > +{ > + pm_runtime_get_sync(dev); > + return 0; > +} > +static int release_child(struct device *dev, void *data) > +{ > + pm_runtime_put_autosuspend(dev); > + return 0; > +} > + > /** > * tty_release - vfs callback for close > * @inode: inode of tty > @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp) > long timeout = 0; > int once = 1; > > + if (tty->dev) > + device_for_each_child(tty->dev, NULL, release_child); > if (tty_paranoia_check(tty, inode, __func__)) > return 0; > > @@ -2118,6 +2133,8 @@ retry_open: > __proc_set_tty(current, tty); > spin_unlock_irq(¤t->sighand->siglock); > tty_unlock(tty); > + if (tty->dev) > + device_for_each_child(tty->dev, NULL, open_child); > mutex_unlock(&tty_mutex); > return 0; > err_unlock: > @@ -3207,6 +3224,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver, > retval = device_register(dev); > if (retval) > goto error; > + if (device && device->of_node) > + /* Children are platform devices and will be > + * runtime_pm managed by this tty. > + */ > + of_platform_populate(device->of_node, NULL, NULL, dev); > > return dev; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 11 Dec 2014 18:18:37 -0500 Peter Hurley <peter@hurleysoftware.com> wrote: > On 12/11/2014 04:59 PM, NeilBrown wrote: > > A "tty slave" is a device connected via UART. > > It may need a driver to, for example, power the device on > > when the tty is opened, and power it off when the tty > > is released. > > > > A "tty slave" is a platform device which is declared as a > > child of the uart in device-tree: > > > > &uart1 { > > bluetooth { > > compatible = "tty,regulator"; > > vdd-supply = <&vaux4>; > > }; > > }; > > > > runtime power management is used to power-up the device > > on tty_open() and power-down on tty_release(). > > I have a couple of issues with this: > 1) why does a child device imply power management and a platform > device? iow, what happens when someone else wants to have a > child device that does something different? Why does it imply power management? Because it seems to make obvious sense to turn something on when the tty is open. If the device has other reason to remain on when the tty is closed, it can arrange for extra references to be taken of course. Is it conceivable that you would want the device to remain off when the tty device is open? In that case just make it a regular platform device. Why a platform device? Things on an i2c bus are i2c devices. Things on a usb bus are usb devices. Ideally a thing on a uart 'bus' would be a 'uart device', but no such thing exists. I did contemplate the possibility of creating an explicit "uart" or "tty" bus type, but I could find no value in that. The door is certainly still open to that possibility if a meaning for the idea becomes apparent. As far as device tree is concerned it is just a child device node. The fact that it is implemented as a "platform" device could easily be changed later if needed without device tree noticing. What could conceivably want to be different? The only (purely hypothetical) concept I can come up with which wouldn't fit is a device with both an UART port and a USB port, or similar. However device tree, and the device model in general, just isn't geared towards devices being on multiple buses so if that were real it would have much greater implications that just here. But maybe I misunderstand... > 2) why is this tied to the tty core and not the serial core > if this is only for UART? Because the knowledge of "the device is being opened" or "the device is being closed" seems to exist in the tty core but not in the serial core. The "of_platform_populate()" call could certainly go in serial_core.c, in uart_add_one_port() I think. I did have it there originally. I moved it for a reason that I think is no longer relevant. As the on/off code is (and I think has to be) in tty_io.c, I left all of it there. I'm happy to move it to serial_core.c if that is preferred. I'll also move the open/close handling if you can point to where it should go. Thanks, NeilBrown > > Regards, > Peter Hurley > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > .../devicetree/bindings/serial/of-serial.txt | 4 ++++ > > drivers/tty/tty_io.c | 22 ++++++++++++++++++++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt > > index 8c4fd0332028..b59501ee2f21 100644 > > --- a/Documentation/devicetree/bindings/serial/of-serial.txt > > +++ b/Documentation/devicetree/bindings/serial/of-serial.txt > > @@ -39,6 +39,10 @@ Optional properties: > > driver is allowed to detect support for the capability even without this > > property. > > > > +Optional child node: > > +- a platform device listed as a child node will be probed and > > + powered-on whenever the tty is in use (open). > > + > > Example: > > > > uart@80230000 { > > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > > index 0508a1d8e4cd..7acdc6f093f4 100644 > > --- a/drivers/tty/tty_io.c > > +++ b/drivers/tty/tty_io.c > > @@ -95,6 +95,8 @@ > > #include <linux/seq_file.h> > > #include <linux/serial.h> > > #include <linux/ratelimit.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/of_platform.h> > > > > #include <linux/uaccess.h> > > > > @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty, > > return 0; > > } > > > > +static int open_child(struct device *dev, void *data) > > +{ > > + pm_runtime_get_sync(dev); > > + return 0; > > +} > > +static int release_child(struct device *dev, void *data) > > +{ > > + pm_runtime_put_autosuspend(dev); > > + return 0; > > +} > > + > > /** > > * tty_release - vfs callback for close > > * @inode: inode of tty > > @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp) > > long timeout = 0; > > int once = 1; > > > > + if (tty->dev) > > + device_for_each_child(tty->dev, NULL, release_child); > > if (tty_paranoia_check(tty, inode, __func__)) > > return 0; > > > > @@ -2118,6 +2133,8 @@ retry_open: > > __proc_set_tty(current, tty); > > spin_unlock_irq(¤t->sighand->siglock); > > tty_unlock(tty); > > + if (tty->dev) > > + device_for_each_child(tty->dev, NULL, open_child); > > mutex_unlock(&tty_mutex); > > return 0; > > err_unlock: > > @@ -3207,6 +3224,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver, > > retval = device_register(dev); > > if (retval) > > goto error; > > + if (device && device->of_node) > > + /* Children are platform devices and will be > > + * runtime_pm managed by this tty. > > + */ > > + of_platform_populate(device->of_node, NULL, NULL, dev); > > > > return dev; > > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 12 Dec 2014 08:59:44 +1100 , NeilBrown <neilb@suse.de> wrote: > A "tty slave" is a device connected via UART. > It may need a driver to, for example, power the device on > when the tty is opened, and power it off when the tty > is released. Some nit picking below, mostly about avoiding nasty surprises on devices that may already have child nodes below the tty node. > > A "tty slave" is a platform device which is declared as a > child of the uart in device-tree: As far as the binding is concerned, talking about platform devices isn't relevant. That's an implementation detail. They are just devices. Using a platform device is somewhat problematic for the kernel because the DT could specify pretty much *any* platform device compatible value here and the kernel will happily bind a driver to it. I strongly recommend a separate bus type so that the pool of drivers remains separate. It would be a good time to look at platform_bus_type and see if more of it can be generalized so that subsystems can have custom bus types without very much code at all. > > &uart1 { > bluetooth { > compatible = "tty,regulator"; > vdd-supply = <&vaux4>; > }; The example here isn't great. The compatible value should specify the specific device, not a generic "tty,regulator" type binding. The expectation is the driver understands the attachment and understands how to do PM on the device. > }; > > runtime power management is used to power-up the device > on tty_open() and power-down on tty_release(). What about devices that need to stay powered up even if the tty is not opened? The runtime PM model needs to be better described for tty slaves. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > .../devicetree/bindings/serial/of-serial.txt | 4 ++++ > drivers/tty/tty_io.c | 22 ++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt > index 8c4fd0332028..b59501ee2f21 100644 > --- a/Documentation/devicetree/bindings/serial/of-serial.txt > +++ b/Documentation/devicetree/bindings/serial/of-serial.txt > @@ -39,6 +39,10 @@ Optional properties: > driver is allowed to detect support for the capability even without this > property. > > +Optional child node: > +- a platform device listed as a child node will be probed and > + powered-on whenever the tty is in use (open). > + The biggest concern I have is what happens to nodes that already have child devices that /don't/ match this use case? It is possible that some UART nodes already have a child node used to store other data. There are two ways to handle this; 1) add a new bool property that indicates the child nodes are tty slave devices, or 2) Make each uart driver explicitly enable the feature so that driver authors can check if it is a problem for that device. I personally would suggest #1 because then it can be enabled in generic code. > Example: > > uart@80230000 { > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > index 0508a1d8e4cd..7acdc6f093f4 100644 > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -95,6 +95,8 @@ > #include <linux/seq_file.h> > #include <linux/serial.h> > #include <linux/ratelimit.h> > +#include <linux/pm_runtime.h> > +#include <linux/of_platform.h> > > #include <linux/uaccess.h> > > @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty, > return 0; > } > > +static int open_child(struct device *dev, void *data) > +{ > + pm_runtime_get_sync(dev); > + return 0; > +} > +static int release_child(struct device *dev, void *data) > +{ > + pm_runtime_put_autosuspend(dev); > + return 0; > +} > + > /** > * tty_release - vfs callback for close > * @inode: inode of tty > @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp) > long timeout = 0; > int once = 1; > > + if (tty->dev) > + device_for_each_child(tty->dev, NULL, release_child); > if (tty_paranoia_check(tty, inode, __func__)) > return 0; > > @@ -2118,6 +2133,8 @@ retry_open: > __proc_set_tty(current, tty); > spin_unlock_irq(¤t->sighand->siglock); > tty_unlock(tty); > + if (tty->dev) > + device_for_each_child(tty->dev, NULL, open_child); > mutex_unlock(&tty_mutex); > return 0; > err_unlock: > @@ -3207,6 +3224,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver, > retval = device_register(dev); > if (retval) > goto error; > + if (device && device->of_node) > + /* Children are platform devices and will be > + * runtime_pm managed by this tty. > + */ > + of_platform_populate(device->of_node, NULL, NULL, dev); Also need to remove all the tty slaves on driver unbind. > > return dev; > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/12/2014 12:23 AM, NeilBrown wrote: > On Thu, 11 Dec 2014 18:18:37 -0500 Peter Hurley <peter@hurleysoftware.com> > wrote: > >> On 12/11/2014 04:59 PM, NeilBrown wrote: >>> A "tty slave" is a device connected via UART. >>> It may need a driver to, for example, power the device on >>> when the tty is opened, and power it off when the tty >>> is released. >>> >>> A "tty slave" is a platform device which is declared as a >>> child of the uart in device-tree: >>> >>> &uart1 { >>> bluetooth { >>> compatible = "tty,regulator"; >>> vdd-supply = <&vaux4>; >>> }; >>> }; >>> >>> runtime power management is used to power-up the device >>> on tty_open() and power-down on tty_release(). >> >> I have a couple of issues with this: >> 1) why does a child device imply power management and a platform >> device? iow, what happens when someone else wants to have a >> child device that does something different? > > Why does it imply power management? > Because it seems to make obvious sense to turn something on when the tty is > open. If the device has other reason to remain on when the tty is closed, > it can arrange for extra references to be taken of course. > Is it conceivable that you would want the device to remain off when the > tty device is open? In that case just make it a regular platform device. > > Why a platform device? > Things on an i2c bus are i2c devices. Things on a usb bus are usb devices. > Ideally a thing on a uart 'bus' would be a 'uart device', but no such thing > exists. I did contemplate the possibility of creating an explicit "uart" or > "tty" bus type, but I could find no value in that. > The door is certainly still open to that possibility if a meaning for the > idea becomes apparent. As far as device tree is concerned it is just a > child device node. The fact that it is implemented as a "platform" device > could easily be changed later if needed without device tree noticing. > > What could conceivably want to be different? The only (purely hypothetical) > concept I can come up with which wouldn't fit is a device with both an UART > port and a USB port, or similar. However device tree, and the device model > in general, just isn't geared towards devices being on multiple buses so > if that were real it would have much greater implications that just here. > > But maybe I misunderstand... Yeah, misunderstanding. This patch is using a very generic abstraction (parent-child device association) for a really specific purpose (power management for platform devices). What about PCI, PNP, etc.? Also, what happens for existing child device associations like bluetooth & serio? Why not just provide a serial core interface for associating power-managed child devices? Which brings up another point: why not do this in the runtime power management; ie, turn on the slave device when the master device is turned on? Sebastian recently made the 8250 driver power-managed per access, which would enable significant power savings over just leaving the slave device on the whole time. >> 2) why is this tied to the tty core and not the serial core >> if this is only for UART? > > Because the knowledge of "the device is being opened" or "the device is being > closed" seems to exist in the tty core but not in the serial core. uart_open() = device file is being opened uart_close() = device file is being released > The "of_platform_populate()" call could certainly go in serial_core.c, in > uart_add_one_port() I think. I did have it there originally. I moved it for > a reason that I think is no longer relevant. As the on/off code is (and I > think has to be) in tty_io.c, I left all of it there. > > I'm happy to move it to serial_core.c if that is preferred. > I'll also move the open/close handling if you can point to where it should go. Yes, please. The reverse dependency (tty core => of) is undesirable. Regards, Peter Hurley >>> Signed-off-by: NeilBrown <neilb@suse.de> >>> --- >>> .../devicetree/bindings/serial/of-serial.txt | 4 ++++ >>> drivers/tty/tty_io.c | 22 ++++++++++++++++++++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt >>> index 8c4fd0332028..b59501ee2f21 100644 >>> --- a/Documentation/devicetree/bindings/serial/of-serial.txt >>> +++ b/Documentation/devicetree/bindings/serial/of-serial.txt >>> @@ -39,6 +39,10 @@ Optional properties: >>> driver is allowed to detect support for the capability even without this >>> property. >>> >>> +Optional child node: >>> +- a platform device listed as a child node will be probed and >>> + powered-on whenever the tty is in use (open). >>> + >>> Example: >>> >>> uart@80230000 { >>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c >>> index 0508a1d8e4cd..7acdc6f093f4 100644 >>> --- a/drivers/tty/tty_io.c >>> +++ b/drivers/tty/tty_io.c >>> @@ -95,6 +95,8 @@ >>> #include <linux/seq_file.h> >>> #include <linux/serial.h> >>> #include <linux/ratelimit.h> >>> +#include <linux/pm_runtime.h> >>> +#include <linux/of_platform.h> >>> >>> #include <linux/uaccess.h> >>> >>> @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty, >>> return 0; >>> } >>> >>> +static int open_child(struct device *dev, void *data) >>> +{ >>> + pm_runtime_get_sync(dev); >>> + return 0; >>> +} >>> +static int release_child(struct device *dev, void *data) >>> +{ >>> + pm_runtime_put_autosuspend(dev); >>> + return 0; >>> +} >>> + >>> /** >>> * tty_release - vfs callback for close >>> * @inode: inode of tty >>> @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp) >>> long timeout = 0; >>> int once = 1; >>> >>> + if (tty->dev) >>> + device_for_each_child(tty->dev, NULL, release_child); >>> if (tty_paranoia_check(tty, inode, __func__)) >>> return 0; >>> >>> @@ -2118,6 +2133,8 @@ retry_open: >>> __proc_set_tty(current, tty); >>> spin_unlock_irq(¤t->sighand->siglock); >>> tty_unlock(tty); >>> + if (tty->dev) >>> + device_for_each_child(tty->dev, NULL, open_child); >>> mutex_unlock(&tty_mutex); >>> return 0; >>> err_unlock: >>> @@ -3207,6 +3224,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver, >>> retval = device_register(dev); >>> if (retval) >>> goto error; >>> + if (device && device->of_node) >>> + /* Children are platform devices and will be >>> + * runtime_pm managed by this tty. >>> + */ >>> + of_platform_populate(device->of_node, NULL, NULL, dev); >>> >>> return dev; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 2) why is this tied to the tty core and not the serial core > if this is only for UART? I'm a bit baffled why you would try and tie it to serial core given that serial core only covers a random subset of the uart drivers we have. If we have a USB connected device with USB gpio lines doing power management then it needs to be tied tty core. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 12 Dec 2014 08:02:48 -0500 > Which brings up another point: why not do this in the runtime power management; > ie, turn on the slave device when the master device is turned on? Sebastian > recently made the 8250 driver power-managed per access, which would enable > significant power savings over just leaving the slave device on the whole time. That per access model only works on 8250 (in fact only on some 8250). On most devices if the UART is in low power you lose bytes send to it rather than then queuing up anywhere. It doesn't handle cases where you need to keep power on to different rules (for example there are cases where you do things like power the uart down after no bits have been received for 'n' millseconds). Right now if you look into Androidspace you'll find people doing this in userspace by having the sysfs node open and playing crazy games with sysfs, Android glue hacks and gpio poking via the gpio sysfs/gpio lib routines. Good example is some of the GPS and serial based sensor chips. The latency of tty open/close is unacceptable and risks losing bits so they keep the tty open and whack the power management by hand right now. The kind of thing many of them want is stuff like "hold power on until we see receive data, then keep it on until it shuts up for a bit" "assert gpio, send, receive, wait idle, drop gpio" "on GPIO interrupt wake uart, wait for data, when idle, power uart down" > >> 2) why is this tied to the tty core and not the serial core > >> if this is only for UART? > > > > Because the knowledge of "the device is being opened" or "the device is being > > closed" seems to exist in the tty core but not in the serial core. > > uart_open() = device file is being opened > uart_close() = device file is being released Only for a subset of uart type devices that use the uart layer. Quite a few don't, including all the USB ones, and the sd ones. > > I'm happy to move it to serial_core.c if that is preferred. > > I'll also move the open/close handling if you can point to where it should go. > > Yes, please. NAK - I certainly object to it being moved to uart. That's the wrong abstraction layer for this. It's a generic behaviour, it's a tty level behaviour without any uart specific properties. It should work for uarts that don't use the legacy serial_core code, uarts that do, uarts that use USB, uarts that are directly connected to things like sdio etc. That means it has to be tty layer. Alan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Dec 12, 2014 at 11:59:20AM +0000, Grant Likely wrote: > [...] > > --- a/Documentation/devicetree/bindings/serial/of-serial.txt > > +++ b/Documentation/devicetree/bindings/serial/of-serial.txt > > @@ -39,6 +39,10 @@ Optional properties: > > driver is allowed to detect support for the capability even without this > > property. > > > > +Optional child node: > > +- a platform device listed as a child node will be probed and > > + powered-on whenever the tty is in use (open). > > + > > The biggest concern I have is what happens to nodes that already have > child devices that /don't/ match this use case? It is possible that some > UART nodes already have a child node used to store other data. There are > two ways to handle this; 1) add a new bool property that indicates the > child nodes are tty slave devices, or 2) Make each uart driver > explicitly enable the feature so that driver authors can check if it is > a problem for that device. I personally would suggest #1 because then it > can be enabled in generic code. maybe simple depend on the compatible value? If the UART node has child nodes to store other random data it should not have a compatible value? -- Sebastian
On Sat, Dec 13, 2014 at 5:46 PM, Sebastian Reichel <sre@kernel.org> wrote: > Hi, > > On Fri, Dec 12, 2014 at 11:59:20AM +0000, Grant Likely wrote: >> [...] >> > --- a/Documentation/devicetree/bindings/serial/of-serial.txt >> > +++ b/Documentation/devicetree/bindings/serial/of-serial.txt >> > @@ -39,6 +39,10 @@ Optional properties: >> > driver is allowed to detect support for the capability even without this >> > property. >> > >> > +Optional child node: >> > +- a platform device listed as a child node will be probed and >> > + powered-on whenever the tty is in use (open). >> > + >> >> The biggest concern I have is what happens to nodes that already have >> child devices that /don't/ match this use case? It is possible that some >> UART nodes already have a child node used to store other data. There are >> two ways to handle this; 1) add a new bool property that indicates the >> child nodes are tty slave devices, or 2) Make each uart driver >> explicitly enable the feature so that driver authors can check if it is >> a problem for that device. I personally would suggest #1 because then it >> can be enabled in generic code. > > maybe simple depend on the compatible value? If the UART node has > child nodes to store other random data it should not have a > compatible value? That's not a given. It is entirely possible that drivers have used compatible in the child nodes. However, I may be stirring up trouble for nothing. We could enable it for all child nodes that have a compatible value, and then blacklist any parent nodes that want to use a different behaviour. If someone complains then we will fix it. g. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/13/2014 09:23 AM, One Thousand Gnomes wrote: > On Fri, 12 Dec 2014 08:02:48 -0500 >> Which brings up another point: why not do this in the runtime power management; >> ie, turn on the slave device when the master device is turned on? Sebastian >> recently made the 8250 driver power-managed per access, which would enable >> significant power savings over just leaving the slave device on the whole time. > > That per access model only works on 8250 (in fact only on some 8250). On > most devices if the UART is in low power you lose bytes send to it rather > than then queuing up anywhere. > > It doesn't handle cases where you need to keep power on to different rules > (for example there are cases where you do things like power the uart down > after no bits have been received for 'n' millseconds). Right now if you > look into Androidspace you'll find people doing this in userspace by > having the sysfs node open and playing crazy games with sysfs, Android > glue hacks and gpio poking via the gpio sysfs/gpio lib routines. > > Good example is some of the GPS and serial based sensor chips. The > latency of tty open/close is unacceptable and risks losing bits so they > keep the tty open and whack the power management by hand right now. > > The kind of thing many of them want is stuff like > > "hold power on until we see receive data, then keep it on until it shuts > up for a bit" > > "assert gpio, send, receive, wait idle, drop gpio" > > "on GPIO interrupt wake uart, wait for data, when idle, power uart down" > >>>> 2) why is this tied to the tty core and not the serial core >>>> if this is only for UART? >>> >>> Because the knowledge of "the device is being opened" or "the device is being >>> closed" seems to exist in the tty core but not in the serial core. >> >> uart_open() = device file is being opened >> uart_close() = device file is being released > > Only for a subset of uart type devices that use the uart layer. Quite a > few don't, including all the USB ones, and the sd ones. > >>> I'm happy to move it to serial_core.c if that is preferred. >>> I'll also move the open/close handling if you can point to where it should go. >> >> Yes, please. > > NAK - I certainly object to it being moved to uart. That's the wrong > abstraction layer for this. It's a generic behaviour, it's a tty level > behaviour without any uart specific properties. It should work for uarts > that don't use the legacy serial_core code, uarts that do, uarts that use > USB, uarts that are directly connected to things like sdio etc. > > That means it has to be tty layer. I was reviewing the patch submitted, which does none of the things you outlined and does not provide the necessary infrastructure to build upon it. I'm all for the functionality you describe but I think that design will end up subsystem-agnostic. Regards, Peter Hurley -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! > index 8c4fd0332028..b59501ee2f21 100644 > --- a/Documentation/devicetree/bindings/serial/of-serial.txt > +++ b/Documentation/devicetree/bindings/serial/of-serial.txt > @@ -39,6 +39,10 @@ Optional properties: > driver is allowed to detect support for the capability even without this > property. > > +Optional child node: > +- a platform device listed as a child node will be probed and > + powered-on whenever the tty is in use (open). > + > Example: > > uart@80230000 { Hmm. Other devices may want it the other way around: probe the device behind the uart, make it control power and open/close of the uart, and hide the /dev/ttyXX from userspace... Pavel
On Sun, 28 Dec 2014 15:20:10 +0100 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > index 8c4fd0332028..b59501ee2f21 100644 > > --- a/Documentation/devicetree/bindings/serial/of-serial.txt > > +++ b/Documentation/devicetree/bindings/serial/of-serial.txt > > @@ -39,6 +39,10 @@ Optional properties: > > driver is allowed to detect support for the capability even without this > > property. > > > > +Optional child node: > > +- a platform device listed as a child node will be probed and > > + powered-on whenever the tty is in use (open). > > + > > Example: > > > > uart@80230000 { > > Hmm. Other devices may want it the other way around: probe the device > behind the uart, make it control power and open/close of the uart, and > hide the /dev/ttyXX from userspace... > Pavel > I've been thinking a bit about this. The current "tty" seems to be a combination of two things: - a line discipline - a char_dev But some line disciplines don't really want the char_dev. N_MOUSE wants a serio device. N_HCI wants an HCI device N_GSM07010 wants 63 different tty char_devs. N_IRDA and N_PPP ultimately want a net_dev. etc. It would be really nice if the uart would register the line disciple as a child device, then the line discipline would register whatever it wants. Then if a uart had no children, it would register the 'N_TTY' line discipline and get a tty char_dev. If it had a child, it would probe the child device and leave it to register and appropriate line discipline. The child device could interpose itself in the control flow somehow so my driver could add power control at open/close. But that isn't how it works. The line discipline doesn't talk to the uart. Rather the tty layer talks to the uart (through tty_operations) and to the line discipline (through tty_ldisc_ops) and also registers the char_dev. One of the several difficulties with allowing a child device to select a line discipline is the different line disciplines activate differently. N_HCI activates (registers the hci dev) on HCIUARTSETPROTO ioctl. A child device would need a way to specify the protocol I resume. N_MOUSE activates on a 'read' on the tty - and deactivates when the read completes. N_GSM0710 activates immediately that the ldisc is activated, as does N_IRDA N_PPP seems to want a PPPIOCNEWUNIT ioctl to fully register. Doing any of these in a driver for a uart slave device would certainly be possible. I wonder if it is something we really want to do in the kernel though. What is the gain over providing sufficient information in the KOBJ_ADD uevent so that udev can do the required work in user-space? I'm not against the idea, I am just finding it hard to justify. However I do like the idea of having the UART probe the child instead of registering a tty. It could pass the tty_operations structure to the child, and the child could then register a tty with a slightly different tty_operations structure, allowing it to capture any operations that it wants to capture (such as open/close). I might try coding that and see what it looks like... Thanks, NeilBrown
Hi! > > > +Optional child node: > > > +- a platform device listed as a child node will be probed and > > > + powered-on whenever the tty is in use (open). > > > + > > > Example: > > > > > > uart@80230000 { > > > But some line disciplines don't really want the char_dev. > N_MOUSE wants a serio device. > N_HCI wants an HCI device > N_GSM07010 wants 63 different tty char_devs. > N_IRDA and N_PPP ultimately want a net_dev. > etc. > > It would be really nice if the uart would register the line disciple as a > child device, then the line discipline would register whatever it wants. Yes. > N_HCI activates (registers the hci dev) on HCIUARTSETPROTO ioctl. A child > device would need a way to specify the protocol I resume. > N_MOUSE activates on a 'read' on the tty - and deactivates when the read > completes. > N_GSM0710 activates immediately that the ldisc is activated, as does N_IRDA > N_PPP seems to want a PPPIOCNEWUNIT ioctl to fully register. > > Doing any of these in a driver for a uart slave device would certainly be > possible. I wonder if it is something we really want to do in the kernel > though. What is the gain over providing sufficient information in the > KOBJ_ADD uevent so that udev can do the required work in user-space? Consistency. If you have bluetooth on USB, it automatically works, without userspace help. If we have mouse on USB, it automatically works, etc... > However I do like the idea of having the UART probe the child instead of > registering a tty. It could pass the tty_operations structure to the child, > and the child could then register a tty with a slightly different > tty_operations structure, allowing it to capture any operations that it wants > to capture (such as open/close). > I might try coding that and see what it looks like... Lets have a look... Pavel
On Sun, 4 Jan 2015 11:18:47 +0100 Pavel Machek <pavel@ucw.cz> wrote: > Hi! > > > > > +Optional child node: > > > > +- a platform device listed as a child node will be probed and > > > > + powered-on whenever the tty is in use (open). > > > > + > > > > Example: > > > > > > > > uart@80230000 { > > > > > > But some line disciplines don't really want the char_dev. > > N_MOUSE wants a serio device. > > N_HCI wants an HCI device > > N_GSM07010 wants 63 different tty char_devs. > > N_IRDA and N_PPP ultimately want a net_dev. > > etc. > > > > It would be really nice if the uart would register the line disciple as a > > child device, then the line discipline would register whatever it wants. > > Yes. > > > N_HCI activates (registers the hci dev) on HCIUARTSETPROTO ioctl. A child > > device would need a way to specify the protocol I resume. > > N_MOUSE activates on a 'read' on the tty - and deactivates when the read > > completes. > > N_GSM0710 activates immediately that the ldisc is activated, as does N_IRDA > > N_PPP seems to want a PPPIOCNEWUNIT ioctl to fully register. > > > > Doing any of these in a driver for a uart slave device would certainly be > > possible. I wonder if it is something we really want to do in the kernel > > though. What is the gain over providing sufficient information in the > > KOBJ_ADD uevent so that udev can do the required work in user-space? > > Consistency. If you have bluetooth on USB, it automatically works, > without userspace help. If we have mouse on USB, it automatically > works, etc... My point is: why is the kernel/userspace distinction important? As long as it "automatically works", does it really matter where the code is? We can put a little bit of code in the kernel (to report the details of the attached device) and a little bit of code in udev (to run hciattach) and it will still be automatic. NeilBrown > > > However I do like the idea of having the UART probe the child instead of > > registering a tty. It could pass the tty_operations structure to the child, > > and the child could then register a tty with a slightly different > > tty_operations structure, allowing it to capture any operations that it wants > > to capture (such as open/close). > > I might try coding that and see what it looks like... > > Lets have a look... > Pavel
Hi! > > > N_HCI activates (registers the hci dev) on HCIUARTSETPROTO ioctl. A child > > > device would need a way to specify the protocol I resume. > > > N_MOUSE activates on a 'read' on the tty - and deactivates when the read > > > completes. > > > N_GSM0710 activates immediately that the ldisc is activated, as does N_IRDA > > > N_PPP seems to want a PPPIOCNEWUNIT ioctl to fully register. > > > > > > Doing any of these in a driver for a uart slave device would certainly be > > > possible. I wonder if it is something we really want to do in the kernel > > > though. What is the gain over providing sufficient information in the > > > KOBJ_ADD uevent so that udev can do the required work in user-space? > > > > Consistency. If you have bluetooth on USB, it automatically works, > > without userspace help. If we have mouse on USB, it automatically > > works, etc... > > My point is: why is the kernel/userspace distinction important? As long as > it "automatically works", does it really matter where the code is? For some cases, it does, as "devices needed during boot". Someone is going to attach keyboard or something powermanagement related, sooner or later. > We can put a little bit of code in the kernel (to report the details of the > attached device) and a little bit of code in udev (to run hciattach) and it > will still be automatic. Of course, that's also possible. There will be parameters such as speed / parity / flow control that will need to be passed, and userland will still see /dev/ttySX device that it perhaps does not need to see. And there will be problems with devices such as bcm2048 on Nokia, which is connected by a serial line and (interrupt capable) GPIO lines and clocks. Rather than extending hciattach to pass the neccessary GPIO/clock information and extending udev to pass GPIO/clock information to hciattach (and extending kernel to pass it to udev and accept from hciattach), it makes sense to just do it internally. Pavel
> It would be really nice if the uart would register the line disciple as a > child device, then the line discipline would register whatever it wants. For almost every case this doesn't work. You need a tty interface as well because thats how you manage it. > But that isn't how it works. The line discipline doesn't talk to the uart. > Rather the tty layer talks to the uart (through tty_operations) and to the > line discipline (through tty_ldisc_ops) and also registers the char_dev. This is intentional. An ldisc has no business knowing what it's connected to. Try running bluetooth over a tty/pty pair remotely to a dongle - and you can do it or faking a GSM mux over a tty/pty pair for testing. There are lots of cases where we know the correct ldisc from information in the ACPI, DT or even USB identifiers in order to plug in the right ldisc and daemon but I think that pretty much has to be in user space. The kernel might be able to set an ldisc but it can't go around starting bluetooth daemons, doing modem chats to go into mux mode or firing up JMRI and java when it sees a Sprog. That's a job for systemd/udev. Alan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 2015-01-05 15:41:41, One Thousand Gnomes wrote: > > It would be really nice if the uart would register the line disciple as a > > child device, then the line discipline would register whatever it wants. > > For almost every case this doesn't work. You need a tty interface as well > because thats how you manage it. > > > But that isn't how it works. The line discipline doesn't talk to the uart. > > Rather the tty layer talks to the uart (through tty_operations) and to the > > line discipline (through tty_ldisc_ops) and also registers the char_dev. > > This is intentional. An ldisc has no business knowing what it's connected > to. Try running bluetooth over a tty/pty pair remotely to a dongle - and > you can do it or faking a GSM mux over a tty/pty pair for testing. Not all cases are like that. IIRC Neil's hardware uses CTS/RTS in an interesting way. bc2048 is connected to serial+clocks+gpios... > There are lots of cases where we know the correct ldisc from information > in the ACPI, DT or even USB identifiers in order to plug in the right > ldisc and daemon but I think that pretty much has to be in user space. > The kernel might be able to set an ldisc but it can't go around starting > bluetooth daemons, doing modem chats to go into mux mode or firing up > JMRI and java when it sees a Sprog. That's a job for systemd/udev. No need to start bluetooth daemons: userspace can start them itself. Just like in btusb case. Pavel
diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt index 8c4fd0332028..b59501ee2f21 100644 --- a/Documentation/devicetree/bindings/serial/of-serial.txt +++ b/Documentation/devicetree/bindings/serial/of-serial.txt @@ -39,6 +39,10 @@ Optional properties: driver is allowed to detect support for the capability even without this property. +Optional child node: +- a platform device listed as a child node will be probed and + powered-on whenever the tty is in use (open). + Example: uart@80230000 { diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index 0508a1d8e4cd..7acdc6f093f4 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -95,6 +95,8 @@ #include <linux/seq_file.h> #include <linux/serial.h> #include <linux/ratelimit.h> +#include <linux/pm_runtime.h> +#include <linux/of_platform.h> #include <linux/uaccess.h> @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty, return 0; } +static int open_child(struct device *dev, void *data) +{ + pm_runtime_get_sync(dev); + return 0; +} +static int release_child(struct device *dev, void *data) +{ + pm_runtime_put_autosuspend(dev); + return 0; +} + /** * tty_release - vfs callback for close * @inode: inode of tty @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp) long timeout = 0; int once = 1; + if (tty->dev) + device_for_each_child(tty->dev, NULL, release_child); if (tty_paranoia_check(tty, inode, __func__)) return 0; @@ -2118,6 +2133,8 @@ retry_open: __proc_set_tty(current, tty); spin_unlock_irq(¤t->sighand->siglock); tty_unlock(tty); + if (tty->dev) + device_for_each_child(tty->dev, NULL, open_child); mutex_unlock(&tty_mutex); return 0; err_unlock: @@ -3207,6 +3224,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver, retval = device_register(dev); if (retval) goto error; + if (device && device->of_node) + /* Children are platform devices and will be + * runtime_pm managed by this tty. + */ + of_platform_populate(device->of_node, NULL, NULL, dev); return dev;
A "tty slave" is a device connected via UART. It may need a driver to, for example, power the device on when the tty is opened, and power it off when the tty is released. A "tty slave" is a platform device which is declared as a child of the uart in device-tree: &uart1 { bluetooth { compatible = "tty,regulator"; vdd-supply = <&vaux4>; }; }; runtime power management is used to power-up the device on tty_open() and power-down on tty_release(). Signed-off-by: NeilBrown <neilb@suse.de> --- .../devicetree/bindings/serial/of-serial.txt | 4 ++++ drivers/tty/tty_io.c | 22 ++++++++++++++++++++ 2 files changed, 26 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html