[PATCH 1/2] quectel: Extend power-on timeout

[PATCH 1/2] quectel: Extend power-on timeout

[poeschel]

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

From: Lars Poeschel <poeschel@lemonage.de>

More complicated modems emerge and they need longer start-up times. An
EC21 takes about 13 seconds to boot up. This is slightly longer than the
20 * 500 ms we have at the moment. This extends the retries to 30, so we
have 30 * 500 ms and this does successfully power up an EC21 modem.
---
 plugins/quectel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index c3343008..6456775d 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -1086,8 +1086,8 @@ static void init_timeout_cb(struct l_timeout *timeout, void *user_data)
 
 	DBG("%p", modem);
 
-	if (data->init_count++ >= 20) {
-		ofono_error("failed to init modem after 20 attempts");
+	if (data->init_count++ >= 30) {
+		ofono_error("failed to init modem after 30 attempts");
 		close_serial(modem);
 		return;
 	}
-- 
2.28.0

[PATCH 2/2] quectel: Power on/off with a gpio pulse

[poeschel]

[-- Attachment #1: Type: text/plain, Size: 2740 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 | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 6456775d..1d8da528 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -103,6 +103,7 @@ struct quectel_data {
 	int mux_ready_count;
 	int initial_ldisc;
 	struct l_gpio_writer *gpio;
+	struct l_timeout *gpio_timeout;
 	struct l_timeout *init_timeout;
 	size_t init_count;
 	guint init_cmd;
@@ -133,6 +134,16 @@ static void quectel_debug(const char *str, void *user_data)
 	ofono_info("%s%s", prefix, str);
 }
 
+static void gpio_timeout_cb(struct l_timeout *timeout, void *user_data)
+{
+	const uint32_t gpio_value = 0;
+	struct quectel_data *data = user_data;
+
+	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	l_timeout_remove(data->gpio_timeout);
+	data->gpio_timeout = NULL;
+}
+
 static int quectel_probe_gpio(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
@@ -237,7 +248,7 @@ static void close_ngsm(struct ofono_modem *modem)
 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 +269,13 @@ static void close_serial(struct ofono_modem *modem)
 	else
 		close_ngsm(modem);
 
-	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	if (data->gpio) {
+		l_gpio_writer_set(data->gpio, 1, &gpio_value);
+		usleep(750000);
+		gpio_value = 0;
+		l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	}
+
 	ofono_modem_set_powered(modem, FALSE);
 }
 
@@ -1139,6 +1156,10 @@ static int open_serial(struct ofono_modem *modem)
 		return -EIO;
 	}
 
+	if (data->gpio)
+		data->gpio_timeout = l_timeout_create_ms(2100, gpio_timeout_cb,
+				data, NULL);
+
 	/*
 	 * there are three different power-up scenarios:
 	 *
-- 
2.28.0

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Hi Lars,

On 24/09/2020 12.38, 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.

We actually implemented this on the M95 by keeping the POWER pin always 
high, and then controlling power using the RESET pin. This avoids the 
need for pulses, which I find simpler.

Can't the same be done for the EC21 ?

// Martin

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[janez.zupan]

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

Hi guys,
I've ask Quectel support about reset signal (active low) on EC21 and this was the answer:
----
R&D said that if the reset low time is more than 480ms, the module will reset again.
We suggest use PERST# only when failed to turn off the module by AT+QPOWD command and PWRKEY pin for protecting the internal flash. The fewer reset times of the module, the better.

So we suggest the reset low time to be no more than 460ms and 20ms margin reserved.
---

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Hi Lars and Janez,

On 24/09/2020 13.02, Martin Hundebøll wrote:
> Hi Lars,
> 
> On 24/09/2020 12.38, 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.
> 
> We actually implemented this on the M95 by keeping the POWER pin always 
> high, and then controlling power using the RESET pin. This avoids the 
> need for pulses, which I find simpler.

Sorry, this is not accurate. We control the regulator supplying the 
modem, not the reset pin.

// Martin

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[Lars Poeschel]

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

On Fri, Sep 25, 2020 at 08:43:15AM +0200, Martin Hundebøll wrote:
> Hi Lars and Janez,
> 
> On 24/09/2020 13.02, Martin Hundebøll wrote:
> > Hi Lars,
> > 
> > On 24/09/2020 12.38, 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.
> > 
> > We actually implemented this on the M95 by keeping the POWER pin always
> > high, and then controlling power using the RESET pin. This avoids the
> > need for pulses, which I find simpler.
> 
> Sorry, this is not accurate. We control the regulator supplying the modem,
> not the reset pin.

But this is bad also. The modem does a lot of things after you tell it
to power-off for example unregistering from the network gracefully. It
also might save some settings it uses internally or whatever.

You are using a way that does not seem to be intended by the vendor and
it reading the hardware design document it is also not obvious to me.
The obivous way to me is just using the PWR_KEY pin that every supported
quectel modem in ofono provides ready to use. And doing a pulse on this
is also not very hard.

I am open to discuss if we should power-off the modem with the pulse or
use the AT+QPOWD at command instead, but for power-on I think the pulse
is the way to go.

By the way this is not specific to the M95 or EC21. This does apply to
all ofono-supported quectel modems.

For me I don't have the choice. I have to support the pulse-approach
somehow, either ofono accepts it upstream or I have to maintain it as a
private patch, as I have to support a hardware that is built as is.
I can imagine the same somehow applies to you. The question is now: What
to do ?
I would like to see the pulse-approach in ofono as a default. We can
then work out a way to configure the level-solution you need maybe as a
new envionment option ?
What do others think ?

Regards,
Lars

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

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

Hi Lars,

On 25/09/2020 10.11, Lars Poeschel wrote:
> On Fri, Sep 25, 2020 at 08:43:15AM +0200, Martin Hundebøll wrote:
>> Hi Lars and Janez,
>>
>> On 24/09/2020 13.02, Martin Hundebøll wrote:
>>> Hi Lars,
>>>
>>> On 24/09/2020 12.38, 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.
>>>
>>> We actually implemented this on the M95 by keeping the POWER pin always
>>> high, and then controlling power using the RESET pin. This avoids the
>>> need for pulses, which I find simpler.
>>
>> Sorry, this is not accurate. We control the regulator supplying the modem,
>> not the reset pin.
> 
> But this is bad also. The modem does a lot of things after you tell it
> to power-off for example unregistering from the network gracefully. It
> also might save some settings it uses internally or whatever.

I see your point. We do, however, send AT+CFUN=0 before pulling the 
plug, so most of what you mention should happen anyways.

> You are using a way that does not seem to be intended by the vendor and
> it reading the hardware design document it is also not obvious to me.
> The obivous way to me is just using the PWR_KEY pin that every supported
> quectel modem in ofono provides ready to use. And doing a pulse on this
> is also not very hard.

I haven't looked too deeply into this, but how can we tell if we are 
powering the module on or off when sending a pulse the first time?

> I am open to discuss if we should power-off the modem with the pulse or
> use the AT+QPOWD at command instead, but for power-on I think the pulse
> is the way to go.
> 
> By the way this is not specific to the M95 or EC21. This does apply to
> all ofono-supported quectel modems.

Yeah, I've seen this on other modems too, and I think it's a weird way 
to do hardware for embedded systems... A power-key would make sense if I 
was building a mobile phone, and even then I would probably prefer it to 
be a dedicated circuit instead of a modem-thing :)

> For me I don't have the choice. I have to support the pulse-approach
> somehow, either ofono accepts it upstream or I have to maintain it as a
> private patch, as I have to support a hardware that is built as is.
> I can imagine the same somehow applies to you. The question is now: What
> to do ?
> I would like to see the pulse-approach in ofono as a default. We can
> then work out a way to configure the level-solution you need maybe as a
> new envionment option ?
> What do others think ?

I have both options available, so we should be able to figure something 
out :)

// Martin

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[Denis Kenzior]

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

Hi Lars,

On 9/24/20 5:38 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 | 25 +++++++++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/plugins/quectel.c b/plugins/quectel.c
> index 6456775d..1d8da528 100644
> --- a/plugins/quectel.c
> +++ b/plugins/quectel.c
> @@ -103,6 +103,7 @@ struct quectel_data {
>   	int mux_ready_count;
>   	int initial_ldisc;
>   	struct l_gpio_writer *gpio;
> +	struct l_timeout *gpio_timeout;
>   	struct l_timeout *init_timeout;
>   	size_t init_count;
>   	guint init_cmd;
> @@ -133,6 +134,16 @@ static void quectel_debug(const char *str, void *user_data)
>   	ofono_info("%s%s", prefix, str);
>   }
>   
> +static void gpio_timeout_cb(struct l_timeout *timeout, void *user_data)
> +{
> +	const uint32_t gpio_value = 0;
> +	struct quectel_data *data = user_data;
> +
> +	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> +	l_timeout_remove(data->gpio_timeout);
> +	data->gpio_timeout = NULL;
> +}
> +
>   static int quectel_probe_gpio(struct ofono_modem *modem)
>   {
>   	struct quectel_data *data = ofono_modem_get_data(modem);
> @@ -237,7 +248,7 @@ static void close_ngsm(struct ofono_modem *modem)
>   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 +269,13 @@ static void close_serial(struct ofono_modem *modem)
>   	else
>   		close_ngsm(modem);
>   
> -	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> +	if (data->gpio) {
> +		l_gpio_writer_set(data->gpio, 1, &gpio_value);
> +		usleep(750000);

usleep blocks the entire process, which is kinda bad if you have multiple modems 
/ clients.  My normal rule of thumb is to NAK anything that can block for longer 
than 50-100ms...

Can this be done using g_timeout instead?

> +		gpio_value = 0;
> +		l_gpio_writer_set(data->gpio, 1, &gpio_value);
> +	}
> +
>   	ofono_modem_set_powered(modem, FALSE);
>   }
>   
> @@ -1139,6 +1156,10 @@ static int open_serial(struct ofono_modem *modem)
>   		return -EIO;
>   	}
>   
> +	if (data->gpio)
> +		data->gpio_timeout = l_timeout_create_ms(2100, gpio_timeout_cb,
> +				data, NULL);
> +
>   	/*
>   	 * there are three different power-up scenarios:
>   	 *
> 

Regards,
-Denis

Re: [PATCH 1/2] quectel: Extend power-on timeout

[Denis Kenzior]

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

Hi Lars,

On 9/24/20 5:38 AM, poeschel(a)lemonage.de wrote:
> From: Lars Poeschel <poeschel@lemonage.de>
> 
> More complicated modems emerge and they need longer start-up times. An
> EC21 takes about 13 seconds to boot up. This is slightly longer than the
> 20 * 500 ms we have at the moment. This extends the retries to 30, so we
> have 30 * 500 ms and this does successfully power up an EC21 modem.
> ---
>   plugins/quectel.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[Lars Poeschel]

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

Hi Denis,

On Tue, Sep 29, 2020 at 09:22:26AM -0500, Denis Kenzior wrote:
> > @@ -237,7 +248,7 @@ static void close_ngsm(struct ofono_modem *modem)
> >   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 +269,13 @@ static void close_serial(struct ofono_modem *modem)
> >   	else
> >   		close_ngsm(modem);
> > -	l_gpio_writer_set(data->gpio, 1, &gpio_value);
> > +	if (data->gpio) {
> > +		l_gpio_writer_set(data->gpio, 1, &gpio_value);
> > +		usleep(750000);
> 
> usleep blocks the entire process, which is kinda bad if you have multiple
> modems / clients.  My normal rule of thumb is to NAK anything that can block
> for longer than 50-100ms...
> 
> Can this be done using g_timeout instead?

This was indeed not the first solution I tried. But I used the l_timeout_
thing that is used all over in quectel.c. I could not get this to work
in a way I was satisfied with. When I send a SIGTERM to the ofono
process close_serial is called but the timeout_callback never fires
and thus the modem does not power off. This is why I then tried usleep
and this works reliably.
Just to be sure I tried with g_timeout and this is the same like
l_timeout. The callback is not called.
Do you have another idea ?

Patch with g_timeout is below for reference

Regards,
Lars

-- >8 --
Subject: [PATCH] quectel: Power on/off with a gpio pulse

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, 31 insertions(+), 2 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 6456775d..9ddb15e8 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -103,6 +103,7 @@ struct quectel_data {
 	int mux_ready_count;
 	int initial_ldisc;
 	struct l_gpio_writer *gpio;
+	struct l_timeout *gpio_timeout;
 	struct l_timeout *init_timeout;
 	size_t init_count;
 	guint init_cmd;
@@ -133,6 +134,16 @@ static void quectel_debug(const char *str, void *user_data)
 	ofono_info("%s%s", prefix, str);
 }

+static void gpio_timeout_cb(struct l_timeout *timeout, void *user_data)
+{
+	const uint32_t gpio_value = 0;
+	struct quectel_data *data = user_data;
+
+	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	l_timeout_remove(data->gpio_timeout);
+	data->gpio_timeout = NULL;
+}
+
 static int quectel_probe_gpio(struct ofono_modem *modem)
 {
 	struct quectel_data *data = ofono_modem_get_data(modem);
@@ -234,10 +245,20 @@ static void close_ngsm(struct ofono_modem *modem)
 		ofono_warn("Failed to restore line discipline");
 }

+static gboolean gpio_cb(gpointer user_data)
+{
+	struct l_gpio_writer *gpio = (struct l_gpio_writer *)user_data;
+	uint32_t gpio_value = 0;
+	DBG("%s", __func__);
+
+	l_gpio_writer_set(gpio, 1, &gpio_value);
+	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,7 +279,11 @@ static void close_serial(struct ofono_modem *modem)
 	else
 		close_ngsm(modem);

-	l_gpio_writer_set(data->gpio, 1, &gpio_value);
+	if (data->gpio) {
+		l_gpio_writer_set(data->gpio, 1, &gpio_value);
+		g_timeout_add(750, gpio_cb, data->gpio);
+	}
+
 	ofono_modem_set_powered(modem, FALSE);
 }

@@ -1139,6 +1164,10 @@ static int open_serial(struct ofono_modem *modem)
 		return -EIO;
 	}

+	if (data->gpio)
+		data->gpio_timeout = l_timeout_create_ms(2100, gpio_timeout_cb,
+				data, NULL);
+
 	/*
 	 * there are three different power-up scenarios:
 	 *
--
2.28.0

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[Denis Kenzior]

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

Hi Lars,

>> Can this be done using g_timeout instead?
> 
> This was indeed not the first solution I tried. But I used the l_timeout_
> thing that is used all over in quectel.c. I could not get this to work
> in a way I was satisfied with. When I send a SIGTERM to the ofono
> process close_serial is called but the timeout_callback never fires

It seems this is because you call ofono_modem_set_powered(..., FALSE) in 
close_serial().  What you should be doing is delaying this until your gpio 
operation is complete.

> and thus the modem does not power off. This is why I then tried usleep
> and this works reliably.
> Just to be sure I tried with g_timeout and this is the same like
> l_timeout. The callback is not called.

See above, l_timeout / g_timeout won't matter here...  The driver needs to tell 
the core when it is done powering up / powering down.  This is accomplished by 
returning -EINPROGRESS from .enable/.disable and calling 
ofono_modem_set_powered() once the operation completes.

Regards,
-Denis