by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCH 10/13] tty: serial: omap: remove some dead code

+Neil

On Wed, Apr 23, 2014 at 09:58:34AM -0500, Felipe Balbi wrote:
> nobody passes a DTR_gpio to this driver, so
> this code is not necessary.
>
> Signed-off-by: Felipe Balbi <[email protected]>
> ---
> drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
> 1 file changed, 39 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index b46aaf3..6654682 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -163,10 +163,6 @@ struct uart_omap_port {
> u8 wakeups_enabled;
> u32 features;
>
> - int DTR_gpio;
> - int DTR_inverted;
> - int DTR_active;
> -
> struct serial_rs485 rs485;
> int rts_gpio;
>
> @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
> serial_out(up, UART_MCR, up->mcr);
> pm_runtime_mark_last_busy(up->dev);
> pm_runtime_put_autosuspend(up->dev);
> -
> - if (gpio_is_valid(up->DTR_gpio) &&
> - !!(mctrl & TIOCM_DTR) != up->DTR_active) {
> - up->DTR_active = !up->DTR_active;
> - if (gpio_cansleep(up->DTR_gpio))
> - schedule_work(&up->qos_work);
> - else
> - gpio_set_value(up->DTR_gpio,
> - up->DTR_active != up->DTR_inverted);
> - }
> }
>
> static void serial_omap_break_ctl(struct uart_port *port, int break_state)
> @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
> qos_work);
>
> pm_qos_update_request(&up->pm_qos_request, up->latency);
> - if (gpio_is_valid(up->DTR_gpio))
> - gpio_set_value_cansleep(up->DTR_gpio,
> - up->DTR_active != up->DTR_inverted);
> }
>
> static void
> @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
> if (IS_ERR(base))
> return PTR_ERR(base);
>
> - if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> - omap_up_info->DTR_present) {
> - ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
> - "omap-serial");
> - if (ret < 0)
> - return ret;
> - ret = gpio_direction_output(omap_up_info->DTR_gpio,
> - omap_up_info->DTR_inverted);
> - if (ret < 0)
> - return ret;
> - }
> -
> - if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> - omap_up_info->DTR_present) {
> - up->DTR_gpio = omap_up_info->DTR_gpio;
> - up->DTR_inverted = omap_up_info->DTR_inverted;
> - } else {
> - up->DTR_gpio = -EINVAL;
> - }
> -
> - up->DTR_active = 0;
> -
> up->dev = &pdev->dev;
> up->port.dev = &pdev->dev;
> up->port.type = PORT_OMAP;
> --
> 1.9.2.459.g68773ac
>

--
balbi

Attachments:

(No filename) (2.50 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-04-23 15:35:18

by Nishanth Menon

Dear Felipe Balbi,

On Wed, Apr 23, 2014 at 09:58:26AM -0500, Felipe Balbi wrote:
> LDISCs shouldn't call tty->ops->write() from within
> ->write_wakeup().
>
> ->write_wakeup() is called with port lock taken and
> IRQs disabled, tty->ops->write() will try to acquire
> the same port lock and we will deadlock.
>
> Acked-by: Marcel Holtmann <[email protected]>
> Reviewed-by: Peter Hurley <[email protected]>
> Reported-by: Huang Shijie <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>

Tested-by: Andreas Bie?mann <[email protected]>

on custom TI AM37xx board with 3.4.87. Therefore I vote for adding this to
stable branches (at least 3.4). It applies with a slight line shifting
(init_work is not available there).

I wonder if you have seen my approach [1] to fix this deadlock. This stuff is
quiet new to me. As I understood the work_queue has a bit more overhead than a
tasklet. Therefore I implemented my fix with a tasklet. Would be great if one
could shed some light on that.

Best regards

Andreas Bie?mann

[1] https://lkml.org/lkml/2014/4/16/425

> ---
> drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
> drivers/bluetooth/hci_uart.h | 1 +
> 2 files changed, 20 insertions(+), 5 deletions(-)

2014-04-24 14:19:46

by Alan Cox

[permalink] [raw]

Subject: Re: [PATCH 10/13] tty: serial: omap: remove some dead code

> > But I don't have discrete hardware. I have a bunch of stuff soldered onto a
> > board with ad-hoc connections chosen to make the life of the hardware builder
> > easy rather than chosen to make the life of the software developer easy
> > (which I think is the correct choice).
> >
> > So I need to tell DT "This device is plugged into this UART, and there is no
> > DTR line, but when the UARTs DTR line would be asserted (if it had one), then
> > I need that regulator of there turned on". or maybe "I need to toggle this
> > GPIO with exactly this pattern, while watching that GPIO to see if it is
> > working".
> >
> > So I thought:
> >
> > 1/ give the UART a "virtual" DTR so it could drive any GPIO
> > 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
> > and responded to state changes on that GPIO by turning on or off the
> > regulator
> > 3/ create a dedicated driver which knows how to toggle the magic GPIO while
> > watching the other GPIO to convince the silly device to wakeup, or go to
> > sleep, as required, and have this appear as a (virtual) GPIO.

Unless you are using it as a "real' DTR line then I think this is the
wrong approach. This problem actually is turning up in both PC class and
ARM boxes now all over the place for three reasons I am seeing.

1. We are getting a lot of hardware moving to serial attached
bluetooth/gps/etc because of the power win. In addition ACPI can describe
power relationships and what is on the other end of a UART embedded in
the device

2. We've got cheap hardware with modem lines being "retrofitted" via gpio

3. There are a subset of devices that have extra control lines beyond the
usual serial port ones. These range from additional control lines for
things like smartcard programmers to port muxing.

> I have no problem either way, just that unused code doesn't have to be
> sitting in the tree and I'm not entirely sure this GPIO should be
> handled by omap-serial.c, perhaps something more generic inside
> serial-core so other UART drivers can benefit from it.

serial-core provides power hooks. If the goal is that this comes on when
you power up the uart and goes away on the last close then the hooks are
there already. If its ldisc specific then the tty layer also calls the
device on ldisc changes precisely so it can make hardware specific
twiddles in such cases.

A set of gpios on the tty_port object would not go amiss and would also
address the PC case.

> considering this is a BTUART device, why didn't you do this at the ldisc
> level ? hci_uart_open() sounds like a good choice from a quick thinking.

ldiscs are hardware independent. Nothing about hardware belongs in an
ldisc. Any ldisc should within reason work on any port.

What I propsed when it came up ages ago was to expose some gpio settings
in the tty, to provide ways for them to be set by default and also ioctls
to configure them. I still think this (but moved into the tty_port as its
a persistent hardware property) is a good idea now that we are starting
to see more use cases than weird dongles and muxes on pre-production
reference boards.

With my tty and serial hat on I think a power gpio is a no-brainer even
without doing the other work and therefore it should probably be described
generically for a serial port in the DT as well as in the ACPI data. If
you should also be able to give it a regulator to use as well that also
seems to make complete sense.

Alan

2014-04-25 09:34:33

> I had a quick look and I guess that uart_change_pm() is the most likely
> candidate for what you are referring to.
> I can see how that interfaces to the specific piece of uart hardware, such as
> omap-serial.c
> But I cannot see how you would plumb that though to the device that was
> plugged in to the serial port. I guess if that device could be registered as
> a child of the omap_serial device, then power management inheritance might
> come in to play, but I cannot see any way to tell a serial port that it has
> some arbitrary child device.

If your device is powered by something like a regulator then you can
drive the regulator from the uart_pm helpers.

> In one case the "power on" sequence is substantially more complex that just a
> gpio or regulator. I would need to write a device driver for the (GPS) chip
> which could receive a poweron/poweroff signal from the uart and do the
> required magic.

In which case giving the tty a child device (or devices) would probably
be more sensible yes. No problem with that either.

> I would really like to see the "runtime interpreted power sequences" become a
> real thing. Then serial-core could activate a "RIPS", and that would have
> the flexibility to do whatever is needed without adding complexity to
> serial-core.

Something like ACPI has you mean ? When we put the device into D0 the
ACPI methods can do stuff.

> So ... with your "serial" hat on, if I were to write/test a patch to allow
> any serial port to have a "power-gpio" described in DT and the gpio would be
> driven in uart_change_pm(), would you consider accepting that patch, or did I
> misunderstand?

We are going to need it anyway for other devices fairly soon. It's common
for new PC class hardware to have gpio management for the bluetooth,
gps and and sometimes sensor devices attached to the tty. That's true
irrespective of whether you happen to choose to use it for virtual gpio
hacks.

Alan