All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] quectel: Power on/off with a gpio pulse
@ 2020-10-01 11:42 poeschel
  2020-10-02 14:50 ` Denis Kenzior
  0 siblings, 1 reply; 5+ messages in thread
From: poeschel @ 2020-10-01 11:42 UTC (permalink / raw)
  To: ofono

[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]

From: Lars Poeschel <poeschel@lemonage.de>

Current implementation uses a gpio level of 1 for powering on quectel
modems using a gpio and a level of 0 for powering off.
This is wrong. Quectel modems use pulses for either power on and power
off. They turn on by the first pulse and turn then off by the next
pulse. The pulse length varies between different modems.
For power on the longest I could in the quectel hardware is "more than
2 seconds" from Quectel M95 Hardware Design Manual.
For Quectel EC21 this is ">= 100 ms".
For Quectel MC60 this is "recommended to be 100 ms".
For Quectel UC15 this is "at least 0.1 s".
For power off the four modems in question vary between a minimum pulse
length of 600-700ms.
This implements a 2100ms pulse for power on and 750ms for power off.
---
 plugins/quectel.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 6456775d..61ac906e 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -234,10 +234,21 @@ static void close_ngsm(struct ofono_modem *modem)
 		ofono_warn("Failed to restore line discipline");
 }
 
+static gboolean gpio_power_off_cb(gpointer user_data)
+{
+	struct ofono_modem *modem = (struct ofono_modem *)user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+	const uint32_t gpio_value = 0;
+
+	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	ofono_modem_set_powered(modem, FALSE);
+	return false;
+}
+
 static void close_serial(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
-	uint32_t gpio_value = 0;
+	uint32_t gpio_value = 1;
 
 	DBG("%p", modem);
 
@@ -258,8 +269,12 @@ static void close_serial(struct ofono_modem *modem)
 	else
 		close_ngsm(modem);
 
-	l_gpio_writer_set(data->gpio, 1, &gpio_value);
-	ofono_modem_set_powered(modem, FALSE);
+	if (data->gpio) {
+		l_gpio_writer_set(data->gpio, 1, &gpio_value);
+		g_timeout_add(750, gpio_power_off_cb, modem);
+	} else
+		ofono_modem_set_powered(modem, FALSE);
+
 }
 
 static void dbus_hw_reply_properties(struct dbus_hw *hw)
@@ -1096,6 +1111,15 @@ static void init_timeout_cb(struct l_timeout *timeout, void *user_data)
 	l_timeout_modify_ms(timeout, 500);
 }
 
+static gboolean gpio_power_on_cb(gpointer user_data)
+{
+	struct quectel_data *data = user_data;
+	const uint32_t gpio_value = 0;
+
+	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	return false;
+}
+
 static int open_serial(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
@@ -1139,6 +1163,9 @@ static int open_serial(struct ofono_modem *modem)
 		return -EIO;
 	}
 
+	if (data->gpio)
+		g_timeout_add(2100, gpio_power_on_cb, data);
+
 	/*
 	 * there are three different power-up scenarios:
 	 *
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] quectel: Power on/off with a gpio pulse
  2020-10-01 11:42 [PATCH v2] quectel: Power on/off with a gpio pulse poeschel
@ 2020-10-02 14:50 ` Denis Kenzior
  2020-10-05 15:10   ` Lars Poeschel
  0 siblings, 1 reply; 5+ messages in thread
From: Denis Kenzior @ 2020-10-02 14:50 UTC (permalink / raw)
  To: ofono

[-- Attachment #1: Type: text/plain, Size: 3920 bytes --]

Hi Lars,

On 10/1/20 6:42 AM, poeschel(a)lemonage.de wrote:
> From: Lars Poeschel <poeschel@lemonage.de>
> 
> Current implementation uses a gpio level of 1 for powering on quectel
> modems using a gpio and a level of 0 for powering off.
> This is wrong. Quectel modems use pulses for either power on and power
> off. They turn on by the first pulse and turn then off by the next
> pulse. The pulse length varies between different modems.
> For power on the longest I could in the quectel hardware is "more than
> 2 seconds" from Quectel M95 Hardware Design Manual.
> For Quectel EC21 this is ">= 100 ms".
> For Quectel MC60 this is "recommended to be 100 ms".
> For Quectel UC15 this is "at least 0.1 s".
> For power off the four modems in question vary between a minimum pulse
> length of 600-700ms.
> This implements a 2100ms pulse for power on and 750ms for power off.
> ---
>   plugins/quectel.c | 33 ++++++++++++++++++++++++++++++---
>   1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/quectel.c b/plugins/quectel.c
> index 6456775d..61ac906e 100644
> --- a/plugins/quectel.c
> +++ b/plugins/quectel.c
> @@ -234,10 +234,21 @@ static void close_ngsm(struct ofono_modem *modem)
>   		ofono_warn("Failed to restore line discipline");
>   }
>   
> +static gboolean gpio_power_off_cb(gpointer user_data)
> +{
> +	struct ofono_modem *modem = (struct ofono_modem *)user_data;
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +	const uint32_t gpio_value = 0;
> +
> +	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> +	ofono_modem_set_powered(modem, FALSE);
> +	return false;
> +}
> +

Ok, this makes sense now...

>   static void close_serial(struct ofono_modem *modem)
>   {
>   	struct quectel_data *data = ofono_modem_get_data(modem);
> -	uint32_t gpio_value = 0;
> +	uint32_t gpio_value = 1;
>   
>   	DBG("%p", modem);
>   
> @@ -258,8 +269,12 @@ static void close_serial(struct ofono_modem *modem)
>   	else
>   		close_ngsm(modem);
>   
> -	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> -	ofono_modem_set_powered(modem, FALSE);
> +	if (data->gpio) {
> +		l_gpio_writer_set(data->gpio, 1, &gpio_value);
> +		g_timeout_add(750, gpio_power_off_cb, modem);

Have you considered what happens if the gpio_power_on_cb timeout is still 
running here?  For example, if the modem is turned on / off quickly?

Maybe the old timeout should be canceled just in case?

> +	} else
> +		ofono_modem_set_powered(modem, FALSE);
> +
>   }
>   
>   static void dbus_hw_reply_properties(struct dbus_hw *hw)
> @@ -1096,6 +1111,15 @@ static void init_timeout_cb(struct l_timeout *timeout, void *user_data)
>   	l_timeout_modify_ms(timeout, 500);
>   }
>   
> +static gboolean gpio_power_on_cb(gpointer user_data)
> +{
> +	struct quectel_data *data = user_data;
> +	const uint32_t gpio_value = 0;
> +
> +	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> +	return false;
> +}

It seems that this timeout is completely independent of 
ofono_modem_set_powered(TRUE), so the core won't prevent the modem from being 
powered off while this is running...

> +
>   static int open_serial(struct ofono_modem *modem)
>   {
>   	struct quectel_data *data = ofono_modem_get_data(modem);
> @@ -1139,6 +1163,9 @@ static int open_serial(struct ofono_modem *modem)
>   		return -EIO;
>   	}
>   
> +	if (data->gpio)
> +		g_timeout_add(2100, gpio_power_on_cb, data);
> +

Generally it is a good idea to keep track of any timeout objects you're adding. 
This is especially true on hot-plug/unplug capable hardware since the modem 
object and its data might go away, but the timer would still be running, causing 
a SIGSEGV later.

Granted this is a serial device, so this won't likely happen in this case...

>   	/*
>   	 * there are three different power-up scenarios:
>   	 *
> 

Regards,
-Denis

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] quectel: Power on/off with a gpio pulse
  2020-10-02 14:50 ` Denis Kenzior
@ 2020-10-05 15:10   ` Lars Poeschel
  2020-10-06 10:10     ` [PATCH v3] " poeschel
  0 siblings, 1 reply; 5+ messages in thread
From: Lars Poeschel @ 2020-10-05 15:10 UTC (permalink / raw)
  To: ofono

[-- Attachment #1: Type: text/plain, Size: 18908 bytes --]

On Fri, Oct 02, 2020 at 09:50:22AM -0500, Denis Kenzior wrote:
> Hi Lars,
> 
> On 10/1/20 6:42 AM, poeschel(a)lemonage.de wrote:
> > From: Lars Poeschel <poeschel@lemonage.de>
> > 
> > Current implementation uses a gpio level of 1 for powering on quectel
> > modems using a gpio and a level of 0 for powering off.
> > This is wrong. Quectel modems use pulses for either power on and power
> > off. They turn on by the first pulse and turn then off by the next
> > pulse. The pulse length varies between different modems.
> > For power on the longest I could in the quectel hardware is "more than
> > 2 seconds" from Quectel M95 Hardware Design Manual.
> > For Quectel EC21 this is ">= 100 ms".
> > For Quectel MC60 this is "recommended to be 100 ms".
> > For Quectel UC15 this is "at least 0.1 s".
> > For power off the four modems in question vary between a minimum pulse
> > length of 600-700ms.
> > This implements a 2100ms pulse for power on and 750ms for power off.
> > ---
> >   plugins/quectel.c | 33 ++++++++++++++++++++++++++++++---
> >   1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/plugins/quectel.c b/plugins/quectel.c
> > index 6456775d..61ac906e 100644
> > --- a/plugins/quectel.c
> > +++ b/plugins/quectel.c
> > @@ -234,10 +234,21 @@ static void close_ngsm(struct ofono_modem *modem)
> >   		ofono_warn("Failed to restore line discipline");
> >   }
> > +static gboolean gpio_power_off_cb(gpointer user_data)
> > +{
> > +	struct ofono_modem *modem = (struct ofono_modem *)user_data;
> > +	struct quectel_data *data = ofono_modem_get_data(modem);
> > +	const uint32_t gpio_value = 0;
> > +
> > +	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> > +	ofono_modem_set_powered(modem, FALSE);
> > +	return false;
> > +}
> > +
> 
> Ok, this makes sense now...
> 
> >   static void close_serial(struct ofono_modem *modem)
> >   {
> >   	struct quectel_data *data = ofono_modem_get_data(modem);
> > -	uint32_t gpio_value = 0;
> > +	uint32_t gpio_value = 1;
> >   	DBG("%p", modem);
> > @@ -258,8 +269,12 @@ static void close_serial(struct ofono_modem *modem)
> >   	else
> >   		close_ngsm(modem);
> > -	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> > -	ofono_modem_set_powered(modem, FALSE);
> > +	if (data->gpio) {
> > +		l_gpio_writer_set(data->gpio, 1, &gpio_value);
> > +		g_timeout_add(750, gpio_power_off_cb, modem);
> 
> Have you considered what happens if the gpio_power_on_cb timeout is still
> running here?  For example, if the modem is turned on / off quickly?
> 
> Maybe the old timeout should be canceled just in case?

I am in a big luck here! :-)
Setting "Powered" on "org.ofono.Modem" is synchronous. And even if
another process is trying to unset "Powered" in the meantime. This is
blocked:

dbus.exceptions.DBusException: org.ofono.Error.InProgress: Operation already in progress

And powering on the modem is taking as long as the first replies from
the modem are received. This a lot of time after the timeout happened.

> > +	} else
> > +		ofono_modem_set_powered(modem, FALSE);
> > +
> >   }
> >   static void dbus_hw_reply_properties(struct dbus_hw *hw)
> > @@ -1096,6 +1111,15 @@ static void init_timeout_cb(struct l_timeout *timeout, void *user_data)
> >   	l_timeout_modify_ms(timeout, 500);
> >   }
> > +static gboolean gpio_power_on_cb(gpointer user_data)
> > +{
> > +	struct quectel_data *data = user_data;
> > +	const uint32_t gpio_value = 0;
> > +
> > +	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> > +	return false;
> > +}
> 
> It seems that this timeout is completely independent of
> ofono_modem_set_powered(TRUE), so the core won't prevent the modem from
> being powered off while this is running...

What exactly are you concerned about ? What should not work ?
The ofono_modem_set_powered(TRUE) is done later either in cpin_cb or
qinistat_cb, after communication with the modem and mux are properly
set up.
Powering off during this phase is prevented by ofono:

root(a)bboxvx:/usr/lib/ofono/test# ./enable-modem & sleep 11; ./disable-modem

ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property SystemPath
Connecting modem /quectel_0...
ofonod[507]: ../git/plugins/quectel.c:quectel_enable() 0x583b80
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property Device
ofonod[507]: ../git/plugins/quectel.c:open_serial() 0x583b80
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property RtsCts
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property Device
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: UART: < \r\nR
ofonod[507]: UART: < DY\r\n
ofonod[507]: UART: < \r\n
ofonod[507]: UART: < +CFUN: 1\r\n
ofonod[507]: ../git/plugins/quectel.c:init_timeout_cb() 0x583b80
ofonod[507]: UART: > AT\r
ofonod[507]: UART: < A
ofonod[507]: UART: < T\r\r\nOK\r
ofonod[507]: UART: < \n
ofonod[507]: ../git/plugins/quectel.c:init_cmd_cb() 0x583b80
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property RtsCts
ofonod[507]: UART: > ATE0\r
ofonod[507]: UART: < ATE
ofonod[507]: UART: < 0\r
ofonod[507]: UART: < \r
ofonod[507]: UART: < \nOK\r\n
ofonod[507]: ../git/plugins/quectel.c:ate_cb() 0x583b80
ofonod[507]: UART: > AT+CGMM\r
ofonod[507]: UART: < \r
ofonod[507]: UART: < \nEC21\r\n
ofonod[507]: UART: < \r\nOK\r
ofonod[507]: ../git/plugins/quectel.c:cgmm_cb() 0x583b80 ok 1
ofonod[507]: ../git/plugins/quectel.c:cgmm_cb() 0x583b80 model EC21
ofonod[507]: UART: > AT+CMUX=0,0,5,127,10,3,30,10,2\r
ofonod[507]: UART: < \n
ofonod[507]: UART: < \r
ofonod[507]: UART: < \nOK\r\n
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property Mux
ofonod[507]: ../git/plugins/quectel.c:cmux_cb() 0x583b80
ofonod[507]: ../git/plugins/quectel.c:cmux_ngsm() 0x583b80
ofonod[507]: ../git/src/modem.c:set_modem_property() modem 0x583b80 property Modem
ofonod[507]: ../git/src/modem.c:unregister_property() property 0x57b088
ofonod[507]: ../git/src/modem.c:set_modem_property() modem 0x583b80 property Aux
ofonod[507]: ../git/src/modem.c:unregister_property() property 0x581070
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/quectel.c:mux_ready_cb() 0x583b80
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property Modem
ofonod[507]: ../git/plugins/quectel.c:open_ttys() 0x583b80
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property Modem
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property Aux
ofonod[507]: ../git/plugins/quectel.c:setup_aux() 0x583b80
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: Aux: > ATE0; &C0; +CMEE=1\r
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: Aux: < \r\nOK\r\n
ofonod[507]: Aux: > AT+QURCCFG="urcport","uart1"\r
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: Aux: < \r\nOK\r\n
ofonod[507]: Aux: > AT+CFUN?\r
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: ../git/plugins/udevng.c:add_serial_device() Device is missing required OFONO_DRIVER property
ofonod[507]: Aux: < \r\n+CFUN: 1\r\n\r\nOK\r\n
ofonod[507]: ../git/plugins/quectel.c:cfun_query() 0x583b80 ok 1
ofonod[507]: ../git/plugins/quectel.c:cfun_enable() 0x583b80 ok 1
ofonod[507]: Aux: > AT+CFUN=4\r
ofonod[507]: ../git/src/modem.c:get_modem_property() modem 0x583b80 property SystemPath
==================================
Disconnecting modem /quectel_0...
Traceback (most recent call last):
  File "./disable-modem", line 20, in <module>
    modem.SetProperty("Powered", dbus.Boolean(0), timeout = 120)
  File "/usr/lib/python3.8/site-packages/dbus/proxies.py", line 72, in __call__
    return self._proxy_method(*args, **keywords)
  File "/usr/lib/python3.8/site-packages/dbus/proxies.py", line 141, in __call__
    return self._connection.call_blocking(self._named_service,
  File "/usr/lib/python3.8/site-packages/dbus/connection.py", line 652, in call_blocking
    reply_message = self.send_message_with_reply_and_block(
dbus.exceptions.DBusException: org.ofono.Error.InProgress: Operation already in progress
=================================
root(a)bboxvx:/usr/lib/ofono/test# ofonod[507]: Aux: < \r\nOK\r\n
ofonod[507]: ../git/plugins/quectel.c:cfun_cb() 0x583b80 ok 1
ofonod[507]: ../git/plugins/quectel.c:dbus_hw_enable() 0x583b80
ofonod[507]: Aux: > AT+CPIN?\r
ofonod[507]: Aux: < \r\n+QIND: PB DONE\r\n
ofonod[507]: ../git/plugins/quectel.c:qind_notify() 0x583b80
ofonod[507]: Aux: < \r\n+CPIN: READY\r\n\r\nOK\r\n
ofonod[507]: ../git/plugins/quectel.c:sim_state_cb() 0x583b80 present 1
ofonod[507]: Aux: > AT+CPIN?\r
ofonod[507]: Aux: < \r\n+CPIN: READY\r\n\r\nOK\r\n
ofonod[507]: ../git/plugins/quectel.c:cpin_cb() 0x583b80
ofonod[507]: ../git/plugins/quectel.c:init_timer_cb() 0x583b80
ofonod[507]: Aux: > AT+QINISTAT\r
ofonod[507]: Aux: < \r\n+QINISTAT: 7\r\n\r\nOK\r\n
ofonod[507]: ../git/plugins/quectel.c:qinistat_cb() 0x583b80
ofonod[507]: ../git/plugins/quectel.c:qinistat_cb() qinistat: 7
ofonod[507]: ../git/src/modem.c:modem_change_state() old state: 0, new state: 1
ofonod[507]: ../git/plugins/quectel.c:quectel_pre_sim() 0x583b80

I highlighted the output of ./disable-modem between the ====
As disable-modem is done before AT+QINISTAT returned in my case, this
does give "Operation already in progress". The same for everything less
than the 10 seconds.
If I do sleep a few seconds longer in between enable-modem and
disable-modem, the disable-modem does fire after AT+QINISTAT and that
then does power down the modem successfully.

> > +
> >   static int open_serial(struct ofono_modem *modem)
> >   {
> >   	struct quectel_data *data = ofono_modem_get_data(modem);
> > @@ -1139,6 +1163,9 @@ static int open_serial(struct ofono_modem *modem)
> >   		return -EIO;
> >   	}
> > +	if (data->gpio)
> > +		g_timeout_add(2100, gpio_power_on_cb, data);
> > +
> 
> Generally it is a good idea to keep track of any timeout objects you're
> adding. This is especially true on hot-plug/unplug capable hardware since
> the modem object and its data might go away, but the timer would still be
> running, causing a SIGSEGV later.
> 
> Granted this is a serial device, so this won't likely happen in this case...

Ok, you mean it is better to use l_timeout_create_ms and keep track of
the returned object manually ?
As a first guess: Either the timeout fired and I destroy it in the cb
or I destroy it in close_serial. close_serial is part of quectel_disable.
I can try that if you want.

Regards,
Lars

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3] quectel: Power on/off with a gpio pulse
  2020-10-05 15:10   ` Lars Poeschel
@ 2020-10-06 10:10     ` poeschel
  2020-10-06 20:43       ` Denis Kenzior
  0 siblings, 1 reply; 5+ messages in thread
From: poeschel @ 2020-10-06 10:10 UTC (permalink / raw)
  To: ofono

[-- Attachment #1: Type: text/plain, Size: 4882 bytes --]

From: Lars Poeschel <poeschel@lemonage.de>

Current implementation uses a gpio level of 1 for powering on quectel
modems using a gpio and a level of 0 for powering off.
Normally quectel modems are powered on or off by a gpio pulse on their
PWR_KEY pin. They turn on by the first pulse and turn then off by the
next pulse. The pulse length varies between different modems.
For power on the longest I could in the quectel hardware is "more than
2 seconds" from Quectel M95 Hardware Design Manual.
For Quectel EC21 this is ">= 100 ms".
For Quectel MC60 this is "recommended to be 100 ms".
For Quectel UC15 this is "at least 0.1 s".
For power off the four modems in question vary between a minimum pulse
length of 600-700ms.
This implements a 2100ms pulse for power on and 750ms for power off.

If you have some special circuitry that powers your modem by gpio level
and you need the old behaviour, you can switch to gpio level powering
by setting environment variable OFONO_QUECTEL_GPIO_LEVEL. The gpio goes
to high level for the modem to power on and to low level if it should
power off.

---
Changes in v3:
- switched to l_timeout_create_ms instead of g_timeout for the gpio
  pulse
- try to keep track of the gpio_timeout object
- combined patch with the level patch
---
 plugins/quectel.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 plugins/udevng.c  |  5 +++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 6456775d..aa519563 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -104,6 +104,7 @@ struct quectel_data {
 	int initial_ldisc;
 	struct l_gpio_writer *gpio;
 	struct l_timeout *init_timeout;
+	struct l_timeout *gpio_timeout;
 	size_t init_count;
 	guint init_cmd;
 };
@@ -191,6 +192,7 @@ static void quectel_remove(struct ofono_modem *modem)
 
 	ofono_modem_set_data(modem, NULL);
 	l_timeout_remove(data->init_timeout);
+	l_timeout_remove(data->gpio_timeout);
 	l_gpio_writer_free(data->gpio);
 	at_util_sim_state_query_free(data->sim_state_query);
 	g_at_chat_unref(data->aux);
@@ -234,10 +236,22 @@ static void close_ngsm(struct ofono_modem *modem)
 		ofono_warn("Failed to restore line discipline");
 }
 
+static void gpio_power_off_cb(struct l_timeout *timeout, void *user_data)
+{
+	struct ofono_modem *modem = (struct ofono_modem *)user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+	const uint32_t gpio_value = 0;
+
+	l_timeout_remove(timeout);
+	data->gpio_timeout = NULL;
+	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	ofono_modem_set_powered(modem, FALSE);
+}
+
 static void close_serial(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
-	uint32_t gpio_value = 0;
+	uint32_t gpio_value = 1;
 
 	DBG("%p", modem);
 
@@ -258,7 +272,20 @@ static void close_serial(struct ofono_modem *modem)
 	else
 		close_ngsm(modem);
 
-	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	if (data->gpio) {
+		if (ofono_modem_get_boolean(modem, "GpioLevel")) {
+			gpio_value = 0;
+			l_gpio_writer_set(data->gpio, 1, &gpio_value);
+		} else {
+			l_gpio_writer_set(data->gpio, 1, &gpio_value);
+			l_timeout_remove(data->gpio_timeout);
+			data->gpio_timeout = l_timeout_create_ms(750,
+							gpio_power_off_cb,
+							modem, NULL);
+			return;
+		}
+	}
+
 	ofono_modem_set_powered(modem, FALSE);
 }
 
@@ -1096,6 +1123,16 @@ static void init_timeout_cb(struct l_timeout *timeout, void *user_data)
 	l_timeout_modify_ms(timeout, 500);
 }
 
+static void gpio_power_on_cb(struct l_timeout *timeout, void *user_data)
+{
+	struct quectel_data *data = user_data;
+	const uint32_t gpio_value = 0;
+
+	l_timeout_remove(timeout);
+	data->gpio_timeout = NULL;
+	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+}
+
 static int open_serial(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
@@ -1139,6 +1176,10 @@ static int open_serial(struct ofono_modem *modem)
 		return -EIO;
 	}
 
+	if (data->gpio && !ofono_modem_get_boolean(modem, "GpioLevel"))
+		data->gpio_timeout = l_timeout_create_ms(2100, gpio_power_on_cb,
+							 data, NULL);
+
 	/*
 	 * there are three different power-up scenarios:
 	 *
diff --git a/plugins/udevng.c b/plugins/udevng.c
index db13073e..8b1943aa 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -921,6 +921,11 @@ static gboolean setup_quectel_serial(struct modem_info *modem)
 	if (value)
 		ofono_modem_set_string(modem->modem, "GpioOffset", value);
 
+	value = udev_device_get_property_value(info->dev,
+						"OFONO_QUECTEL_GPIO_LEVEL");
+	if (value)
+		ofono_modem_set_boolean(modem->modem, "GpioLevel", TRUE);
+
 	value = udev_device_get_property_value(info->dev,
 						"OFONO_QUECTEL_MUX");
 	if (value)
-- 
2.28.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3] quectel: Power on/off with a gpio pulse
  2020-10-06 10:10     ` [PATCH v3] " poeschel
@ 2020-10-06 20:43       ` Denis Kenzior
  0 siblings, 0 replies; 5+ messages in thread
From: Denis Kenzior @ 2020-10-06 20:43 UTC (permalink / raw)
  To: ofono

[-- Attachment #1: Type: text/plain, Size: 2094 bytes --]

Hi Lars,

On 10/6/20 5:10 AM, poeschel(a)lemonage.de wrote:
> From: Lars Poeschel <poeschel@lemonage.de>
> 
> Current implementation uses a gpio level of 1 for powering on quectel
> modems using a gpio and a level of 0 for powering off.
> Normally quectel modems are powered on or off by a gpio pulse on their
> PWR_KEY pin. They turn on by the first pulse and turn then off by the
> next pulse. The pulse length varies between different modems.
> For power on the longest I could in the quectel hardware is "more than
> 2 seconds" from Quectel M95 Hardware Design Manual.
> For Quectel EC21 this is ">= 100 ms".
> For Quectel MC60 this is "recommended to be 100 ms".
> For Quectel UC15 this is "at least 0.1 s".
> For power off the four modems in question vary between a minimum pulse
> length of 600-700ms.
> This implements a 2100ms pulse for power on and 750ms for power off.
> 
> If you have some special circuitry that powers your modem by gpio level
> and you need the old behaviour, you can switch to gpio level powering
> by setting environment variable OFONO_QUECTEL_GPIO_LEVEL. The gpio goes
> to high level for the modem to power on and to low level if it should
> power off.
> 
> ---
> Changes in v3:
> - switched to l_timeout_create_ms instead of g_timeout for the gpio
>    pulse
> - try to keep track of the gpio_timeout object
> - combined patch with the level patch
> ---
>   plugins/quectel.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
>   plugins/udevng.c  |  5 +++++
>   2 files changed, 48 insertions(+), 2 deletions(-)
> 

Applied with a minor tweak:

> +static void gpio_power_off_cb(struct l_timeout *timeout, void *user_data)
> +{
> +	struct ofono_modem *modem = (struct ofono_modem *)user_data;

This cast was not needed

> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +	const uint32_t gpio_value = 0;
> +
> +	l_timeout_remove(timeout);
> +	data->gpio_timeout = NULL;
> +	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> +	ofono_modem_set_powered(modem, FALSE);
> +}
> +

Regards,
-Denis

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-10-06 20:43 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-01 11:42 [PATCH v2] quectel: Power on/off with a gpio pulse poeschel
2020-10-02 14:50 ` Denis Kenzior
2020-10-05 15:10   ` Lars Poeschel
2020-10-06 10:10     ` [PATCH v3] " poeschel
2020-10-06 20:43       ` Denis Kenzior

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.