Re: [PATCH v2] quectel: Power on/off with a gpio pulse
On Fri, Oct 02, 2020 at 09:50:22AM -0500, Denis Kenzior wrote: > Hi Lars, > > On 10/1/20 6:42 AM, poesc...@lemonage.de wrote: > > From: Lars Poeschel > > > > 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, _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, _value); > > - ofono_modem_set_powered(modem, FALSE); > > + if (data->gpio) { > > + l_gpio_writer_set(data->gpio, 1, _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 +,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, _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@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]:
Re: [PATCH v2] quectel: Power on/off with a gpio pulse
Hi Lars, On 10/1/20 6:42 AM, poesc...@lemonage.de wrote: From: Lars Poeschel 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, _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, _value); - ofono_modem_set_powered(modem, FALSE); + if (data->gpio) { + l_gpio_writer_set(data->gpio, 1, _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 +,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, _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 ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org
[PATCH v2] quectel: Power on/off with a gpio pulse
From: Lars Poeschel 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, _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, _value); - ofono_modem_set_powered(modem, FALSE); + if (data->gpio) { + l_gpio_writer_set(data->gpio, 1, _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 +,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, _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 ___ ofono mailing list -- ofono@ofono.org To unsubscribe send an email to ofono-le...@ofono.org