Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber wrote:
>
> +struct picogw_device {
> + struct serdev_device *serdev;
> +
> + u8 rx_buf[1024];
No, you cannot do that. AFAICT this buffer can be used for DMA.
Thus putting it into another data structure violates the rules
of DMA coherency. You must allocate it separately.
> +static int picogw_send_cmd(struct picogw_device *picodev, char cmd,
> + u8 addr, const void *data, u16 data_len)
> +{
> + struct serdev_device *sdev = picodev->serdev;
> + u8 buf[4];
> + int i, ret;
> +
> + buf[0] = cmd;
> + buf[1] = (data_len >> 8) & 0xff;
> + buf[2] = (data_len >> 0) & 0xff;
We have macros for converting endianness and unaligned access.
> + buf[3] = addr;
> +
> + dev_dbg(>dev, "%s: %c %02x %02x %02x\n", __func__, buf[0],
> + (unsigned int)buf[1], (unsigned int)buf[2], (unsigned
> int)buf[3]);
> + for (i = 0; i < data_len; i++) {
> + dev_dbg(>dev, "%s: data %02x\n", __func__, (unsigned
> int)((const u8*)data)[i]);
> + }
> +
> + ret = serdev_device_write_buf(sdev, buf, 4);
> + if (ret < 0)
> + return ret;
> + if (ret != 4)
> + return -EIO;
> +
> + if (data_len) {
> + ret = serdev_device_write_buf(sdev, data, data_len);
> + if (ret < 0)
> + return ret;
> + if (ret != data_len)
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int picogw_recv_answer(struct picogw_device *picodev,
> + char *cmd, bool *ack, void *buf, int buf_len,
> + unsigned long timeout)
> +{
> + int len;
> +
> + timeout = wait_for_completion_timeout(>answer_comp, timeout);
> + if (!timeout)
> + return -ETIMEDOUT;
And? The IO is still scheduled. Simply erroring out is problematic.
If you see a timeout you need to kill the scheduled IO.
> +static int picogw_handle_answer(struct picogw_device *picodev)
> +{
> + struct device *dev = >serdev->dev;
> + unsigned int data_len = ((u16)picodev->rx_buf[1] << 8) |
> picodev->rx_buf[2];
> + int cmd_len = 4 + data_len;
> + int i, ret;
> +
> + if (picodev->rx_len < 4)
> + return 0;
> +
> + if (cmd_len > sizeof(picodev->rx_buf)) {
> + dev_warn(dev, "answer too long (%u)\n", data_len);
> + return 0;
> + }
> +
> + if (picodev->rx_len < cmd_len) {
> + dev_dbg(dev, "got %u, need %u bytes\n", picodev->rx_len,
> cmd_len);
> + return 0;
> + }
> +
> + dev_dbg(dev, "Answer %c =%u %s (%u)\n", picodev->rx_buf[0],
> + (unsigned int)picodev->rx_buf[3],
> + (picodev->rx_buf[3] == 1) ? "OK" : "K0",
> + data_len);
> + for (i = 0; i < data_len; i++) {
> + //dev_dbg(dev, "%s: %02x\n", __func__, (unsigned
> int)picodev->rx_buf[4 + i]);
> + }
> +
> + complete(>answer_comp);
> + ret =
> wait_for_completion_interruptible_timeout(>answer_read_comp, HZ / 2);
Problematic. You have no idea when that complete() will have an effect.
You are operating with an undefined timeout here. Theoretically it
could be negative.
Regards
Oliver
Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
Hi,
On 04/01/2019 20:21, Andreas Färber wrote:
The picoGW reference MCU firmware implements a USB CDC or UART interface
with a set of serial commands. It can be found on multiple mPCIe cards
as well as USB adapters.
https://github.com/Lora-net/picoGW_mcu
That MCU design superseded earlier attempts of using FTDI chips (MPSSE)
for controlling the SPI based chip directly from the host.
This commit adds a serdev driver implementing another regmap_bus to
abstract the register access and reuses our existing regmap driver on top.
Thereby we have an early proof of concept that we can drive both types
of modules/cards with a single driver core!
It assumes there is only one SX130x (with its radios) accessible, whereas
some new cards reportedly also have an SX127x on-board. So the DT/driver
design may need to be reconsidered once such a card or documentation
becomes available.
It also assumes SX1255/1258 are fully compatible with "semtech,sx1257".
Currently there's still some bugs to be investigated, with communication
stalling on one device and another reading the radio versions wrong
(07 / 1f instead of 21, also observed on spi once).
Signed-off-by: Andreas Färber
---
drivers/net/lora/Makefile| 2 +
drivers/net/lora/sx130x_picogw.c | 466 +++
We are missing a Kconfig select or depend, compilation fails if
CONFIG_SERIAL_DEV_BUS is not selected.
2 files changed, 468 insertions(+)
create mode 100644 drivers/net/lora/sx130x_picogw.c
diff --git a/drivers/net/lora/Makefile b/drivers/net/lora/Makefile
index c6a0410f80ce..5fef38abf5ed 100644
--- a/drivers/net/lora/Makefile
+++ b/drivers/net/lora/Makefile
@@ -25,6 +25,8 @@ lora-sx127x-y := sx127x.o
obj-$(CONFIG_LORA_SX130X) += lora-sx130x.o
lora-sx130x-y := sx130x.o
lora-sx130x-y += sx130x_radio.o
+obj-$(CONFIG_LORA_SX130X) += lora-sx130x-picogw.o
+lora-sx130x-picogw-y := sx130x_picogw.o
obj-$(CONFIG_LORA_USI) += lora-usi.o
lora-usi-y := usi.o
diff --git a/drivers/net/lora/sx130x_picogw.c b/drivers/net/lora/sx130x_picogw.c
new file mode 100644
index ..577f9fb71d46
--- /dev/null
+++ b/drivers/net/lora/sx130x_picogw.c
@@ -0,0 +1,466 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Semtech SX1301/1308 PicoCell gateway serial MCU interface
+ *
+ * Copyright (c) 2018-2019 Andreas Färber
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct picogw_device {
+ struct serdev_device *serdev;
+
+ u8 rx_buf[1024];
+ int rx_len;
+
+ struct completion answer_comp;
+ struct completion answer_read_comp;
+};
+
+static inline struct picogw_device *picogw_get_drvdata(struct serdev_device
*sdev)
+{
+ return sx130x_get_drvdata(>dev);
+}
+
+static bool picogw_valid_cmd(char ch)
+{
+ switch (ch) {
+ case 'k': /* invalid command error */
+ case 'r':
+ case 'w':
+ case 'l':
+ return true;
+ default:
+ return false;
+ }
+}
+
+static int picogw_send_cmd(struct picogw_device *picodev, char cmd,
+ u8 addr, const void *data, u16 data_len)
+{
+ struct serdev_device *sdev = picodev->serdev;
+ u8 buf[4];
+ int i, ret;
+
+ buf[0] = cmd;
+ buf[1] = (data_len >> 8) & 0xff;
+ buf[2] = (data_len >> 0) & 0xff;
+ buf[3] = addr;
+
+ dev_dbg(>dev, "%s: %c %02x %02x %02x\n", __func__, buf[0],
+ (unsigned int)buf[1], (unsigned int)buf[2], (unsigned
int)buf[3]);
+ for (i = 0; i < data_len; i++) {
+ dev_dbg(>dev, "%s: data %02x\n", __func__, (unsigned
int)((const u8*)data)[i]);
+ }
+
+ ret = serdev_device_write_buf(sdev, buf, 4);
+ if (ret < 0)
+ return ret;
+ if (ret != 4)
+ return -EIO;
+
+ if (data_len) {
+ ret = serdev_device_write_buf(sdev, data, data_len);
+ if (ret < 0)
+ return ret;
+ if (ret != data_len)
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int picogw_recv_answer(struct picogw_device *picodev,
+ char *cmd, bool *ack, void *buf, int buf_len,
+ unsigned long timeout)
+{
+ int len;
+
+ timeout = wait_for_completion_timeout(>answer_comp, timeout);
+ if (!timeout)
+ return -ETIMEDOUT;
+
+ if (cmd)
+ *cmd = picodev->rx_buf[0];
+
+ if (ack)
+ *ack = (picodev->rx_buf[3] == 1);
+
+ len = min(picodev->rx_len - 4, buf_len);
+ if (buf)
+ memcpy(buf, picodev->rx_buf + 4, len);
+
+ reinit_completion(>answer_comp);
+ complete(>answer_read_comp);
+
+ return len;
+}
+
+static int picogw_reg_read(struct picogw_device *picodev, u8 addr, u8 *val,
unsigned long timeout)
+{
+ const u8 dummy = 0;
+ char cmd;
+ bool ack;
+ int ret;
+
+ ret = picogw_send_cmd(picodev,
Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
Am 04.01.19 um 12:21 schrieb Andreas Färber: > Currently there's still some bugs to be investigated, with communication > stalling on one device and another reading the radio versions wrong > (07 / 1f instead of 21, also observed on spi once). Reproducable 100% on SPI by setting REGCACHE_RBTREE in sx130x.c. Since this serdev driver was using REGCACHE_NONE still and I don't spot a register missing as volatile either, I guess it may be a timing issue? My earlier locking patch is applied, to rule out any non-determinism in the register access order due to radio vs. concentrator interactions. Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
Am 04.01.19 um 12:21 schrieb Andreas Färber: > diff --git a/drivers/net/lora/sx130x_picogw.c > b/drivers/net/lora/sx130x_picogw.c > new file mode 100644 > index ..577f9fb71d46 > --- /dev/null > +++ b/drivers/net/lora/sx130x_picogw.c [...] > + serdev_device_set_baudrate(sdev, 115200); > + serdev_device_set_parity(sdev, SERDEV_PARITY_NONE); > + serdev_device_set_flow_control(sdev, true); This should probably be false? https://github.com/Lora-net/picoGW_hal/blob/master/libloragw/src/loragw_com_linux.c#L65 tty.c_iflag &= ~(IXON | IXOFF | IXANY | ICRNL); ... tty.c_oflag &= ~(IXON | IXOFF | IXANY | ICRNL); Nothing that looks like RTS/CTS anywhere, which appears to be what the serdev implementation is switching with the above function. However, both true and false appeared to work equally so far. With one particular USB device I get the following warning/error, seemingly independent of whether I enable flow control above or not: [68640.481454] lora-picogw-usb 4-1:1.0: failed to set dtr/rts (imagine s/lora-picogw-usb/cdc_acm/ - cf. cdc-acm.c:acm_port_dtr_rts()) Looks like a firmware/hardware issue to me, since it appeared with the original cdc_acm driver on reboot, plus across reset, ports and hosts. Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
