* Re: [RFC lora-next 5/5] HACK: net: lora: sx130x: Add PicoCell gateway shim for cdc-acm
From: Johan Hovold @ 2019-01-07 15:28 UTC (permalink / raw)
To: Andreas Färber
Cc: Rob Herring, linux-lpwan, linux-serial, Linux USB List,
devicetree, linux-kernel@vger.kernel.org, Johan Hovold,
David S. Miller, Oliver Neukum, Greg Kroah-Hartman, netdev,
linux-clk
In-Reply-To: <9d97fec5-31b0-8717-cd10-de4f36d1cf05@suse.de>
On Sat, Jan 05, 2019 at 12:43:48AM +0100, Andreas Färber wrote:
> Hi Rob,
>
> Am 04.01.19 um 18:07 schrieb Rob Herring:
> > On Fri, Jan 4, 2019 at 5:21 AM Andreas Färber <afaerber@suse.de> wrote:
> >>
> >> Ignore our device in cdc-acm probing and add a new driver for it,
> >> dispatching to cdc-acm for the actual implementation.
> >>
> >> WARNING: It is likely that this VID/PID is in use for unrelated devices.
> >> Only the Product string hints what this really is in current v0.2.1.
> >> Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping
> >> with such firmware is known to me.
> >>
> >> While this may seem unorthodox, no internals of the driver are accessed,
> >> just the set of driver callbacks is duplicated as shim.
> >>
> >> Use this shim construct to fake DT nodes for serdev on probe.
> >> For testing purposes these nodes do not have a parent yet.
> >> This results in two "Error -2 creating of_node link" warnings on probe.
> >
> > It looks like the DT is pretty static. Rather than creating the nodes
> > at run-time, can't you create a dts file and build that into your
> > module.
>
> Heh, if that were the only issue with this patch... ;)
My thoughts exactly. ;)
This clearly is too much of a hack, but maintaining serdev compatible
information in the corresponding tty drivers is probably what we'll want
to do.
When the tty driver binds and registers its ports with tty core, we can
could match again on the USB descriptors, but since the device has
already been matched, we can just pass the equivalent of a compatible
string, or more generally dt-fragment, instead.
Without having had time to look into it myself yet, this sounds like
something we should be using the new software nodes for (software
generated fw nodes). That way, we don't depend on CONFIG_OF either.
Johan
^ permalink raw reply
* Re: [PATCH RFC 4/5] usb: cdc-acm: Enable serdev support
From: Johan Hovold @ 2019-01-07 15:02 UTC (permalink / raw)
To: Oliver Neukum
Cc: Andreas Färber, linux-lpwan, linux-serial, Johan Hovold,
Rob Herring, Greg Kroah-Hartman, devicetree, linux-kernel,
linux-usb
In-Reply-To: <1546868906.3037.37.camel@suse.com>
On Mon, Jan 07, 2019 at 02:48:26PM +0100, Oliver Neukum wrote:
> On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber wrote:
> > Switch from tty_port_register_device() to tty_port_register_device_serdev()
> > and from tty_unregister_device() to tty_port_unregister_device().
> >
> > On removal of a serdev driver sometimes count mismatch warnings were seen:
> >
> > ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0))
> > ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0
> >
> > Note: The serdev drivers appear to probe asynchronously as soon as they
> > are registered. Should the USB quirks in probe be moved before registration?
> > No noticeable difference for the devices at hand.
>
> That is quite drastic a change.
> Johan, how complete in terms of features is serdev?
serdev doesn't support hangups yet, and that's precisely why it's not
enabled for hotpluggable buses. That would need to be fixed before
accepting something like this.
Johan
^ permalink raw reply
* Re: [PATCH RFC 4/5] usb: cdc-acm: Enable serdev support
From: Oliver Neukum @ 2019-01-07 13:48 UTC (permalink / raw)
To: Andreas Färber, linux-lpwan, linux-serial
Cc: Johan Hovold, Rob Herring, Greg Kroah-Hartman, devicetree,
linux-kernel, linux-usb
In-Reply-To: <20190104112131.14451-5-afaerber@suse.de>
On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber wrote:
> Switch from tty_port_register_device() to tty_port_register_device_serdev()
> and from tty_unregister_device() to tty_port_unregister_device().
>
> On removal of a serdev driver sometimes count mismatch warnings were seen:
>
> ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0))
> ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0
>
> Note: The serdev drivers appear to probe asynchronously as soon as they
> are registered. Should the USB quirks in probe be moved before registration?
> No noticeable difference for the devices at hand.
That is quite drastic a change.
Johan, how complete in terms of features is serdev?
Are you refering to CLEAR_HALT_CONDITIONS in terms of quirks?
Regards
Oliver
^ permalink raw reply
* [PATCH 4.20 001/145] panic: avoid deadlocks in re-entrant console drivers
From: Greg Kroah-Hartman @ 2019-01-07 12:30 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Steven Rostedt, Daniel Wang,
Peter Zijlstra, Andrew Morton, Linus Torvalds, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky, Petr Mladek
In-Reply-To: <20190107104437.308206189@linuxfoundation.org>
4.20-stable review patch. If anyone has any objections, please let me know.
------------------
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
commit c7c3f05e341a9a2bd1a92993d4f996cfd6e7348e upstream.
>From printk()/serial console point of view panic() is special, because
it may force CPU to re-enter printk() or/and serial console driver.
Therefore, some of serial consoles drivers are re-entrant. E.g. 8250:
serial8250_console_write()
{
if (port->sysrq)
locked = 0;
else if (oops_in_progress)
locked = spin_trylock_irqsave(&port->lock, flags);
else
spin_lock_irqsave(&port->lock, flags);
...
}
panic() does set oops_in_progress via bust_spinlocks(1), so in theory
we should be able to re-enter serial console driver from panic():
CPU0
<NMI>
uart_console_write()
serial8250_console_write() // if (oops_in_progress)
// spin_trylock_irqsave()
call_console_drivers()
console_unlock()
console_flush_on_panic()
bust_spinlocks(1) // oops_in_progress++
panic()
<NMI/>
spin_lock_irqsave(&port->lock, flags) // spin_lock_irqsave()
serial8250_console_write()
call_console_drivers()
console_unlock()
printk()
...
However, this does not happen and we deadlock in serial console on
port->lock spinlock. And the problem is that console_flush_on_panic()
called after bust_spinlocks(0):
void panic(const char *fmt, ...)
{
bust_spinlocks(1);
...
bust_spinlocks(0);
console_flush_on_panic();
...
}
bust_spinlocks(0) decrements oops_in_progress, so oops_in_progress
can go back to zero. Thus even re-entrant console drivers will simply
spin on port->lock spinlock. Given that port->lock may already be
locked either by a stopped CPU, or by the very same CPU we execute
panic() on (for instance, NMI panic() on printing CPU) the system
deadlocks and does not reboot.
Fix this by removing bust_spinlocks(0), so oops_in_progress is always
set in panic() now and, thus, re-entrant console drivers will trylock
the port->lock instead of spinning on it forever, when we call them
from console_flush_on_panic().
Link: http://lkml.kernel.org/r/20181025101036.6823-1-sergey.senozhatsky@gmail.com
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Daniel Wang <wonderfly@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Peter Feiner <pfeiner@google.com>
Cc: linux-serial@vger.kernel.org
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
kernel/panic.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -14,6 +14,7 @@
#include <linux/kmsg_dump.h>
#include <linux/kallsyms.h>
#include <linux/notifier.h>
+#include <linux/vt_kern.h>
#include <linux/module.h>
#include <linux/random.h>
#include <linux/ftrace.h>
@@ -237,7 +238,10 @@ void panic(const char *fmt, ...)
if (_crash_kexec_post_notifiers)
__crash_kexec(NULL);
- bust_spinlocks(0);
+#ifdef CONFIG_VT
+ unblank_screen();
+#endif
+ console_unblank();
/*
* We may have ended up stopping the CPU holding the lock (in
^ permalink raw reply
* Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
From: Oliver Neukum @ 2019-01-07 12:11 UTC (permalink / raw)
To: Andreas Färber, linux-lpwan, linux-serial
Cc: David S. Miller, Johan Hovold, Rob Herring, devicetree,
linux-kernel, linux-usb, netdev
In-Reply-To: <20190104112131.14451-4-afaerber@suse.de>
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(&sdev->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(&sdev->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(&picodev->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 = &picodev->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(&picodev->answer_comp);
> + ret = wait_for_completion_interruptible_timeout(&picodev->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
^ permalink raw reply
* Re: [BUG] tty: serial: mxs-auart: possible concurrency use-after-free bugs in mxs_auart_dma_exit_channel()
From: Jia-Ju Bai @ 2019-01-07 9:03 UTC (permalink / raw)
To: Greg KH; +Cc: jslaby, linux-serial, Linux Kernel Mailing List
In-Reply-To: <20190107085224.GA26384@kroah.com>
On 2019/1/7 16:52, Greg KH wrote:
> On Mon, Jan 07, 2019 at 04:47:43PM +0800, Jia-Ju Bai wrote:
>> The driver functions mxs_auart_settermios(), dma_rx_callback() and dma_tx_callback() can be concurrently executed.
>>
>> In Linux 4.19:
>>
>> mxs_auart_settermios
>> mxs_auart_dma_exit
>> mxs_auart_dma_exit_channel
>> line 918: kfree(s->tx_dma_buf);
>> line 919: kfree(s->rx_dma_buf);
>>
>> dma_rx_callback
>> line 862: tty_insert_flip_string(port, s->rx_dma_buf, count);
>> mxs_auart_dma_prep_rx
>> line 890: sg_init_one(sgl, s->rx_dma_buf, UART_XMIT_SIZE);
>>
>> dma_tx_callback
>> mxs_auart_tx_chars
>> line 590: void *buffer = s->tx_dma_buf;
>> mxs_auart_dma_tx
>> line 566: sg_init_one(sgl, s->tx_dma_buf, size);
>>
>> Thus, possible concurrency use-after-free bugs may occur.
>>
>> These possible bugs are found by a static analysis tool written by myself and my manual code review.
> Care to send a patch to fix up this issue?
I would like to, but I do not know how to fix these bugs properly...
There is no lock and lock-related function call in
drivers/tty/serial/mxs-auart.c.
Thus, we may need to introduce a new lock in this source file.
What is your opinion?
Best wishes,
Jia-Ju Bai
^ permalink raw reply
* Re: [BUG] tty: serial: mxs-auart: possible concurrency use-after-free bugs in mxs_auart_dma_exit_channel()
From: Greg KH @ 2019-01-07 8:52 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: jslaby, linux-serial, Linux Kernel Mailing List
In-Reply-To: <8da85649-f539-9c36-a97e-3582844e82fb@gmail.com>
On Mon, Jan 07, 2019 at 04:47:43PM +0800, Jia-Ju Bai wrote:
> The driver functions mxs_auart_settermios(), dma_rx_callback() and dma_tx_callback() can be concurrently executed.
>
> In Linux 4.19:
>
> mxs_auart_settermios
> mxs_auart_dma_exit
> mxs_auart_dma_exit_channel
> line 918: kfree(s->tx_dma_buf);
> line 919: kfree(s->rx_dma_buf);
>
> dma_rx_callback
> line 862: tty_insert_flip_string(port, s->rx_dma_buf, count);
> mxs_auart_dma_prep_rx
> line 890: sg_init_one(sgl, s->rx_dma_buf, UART_XMIT_SIZE);
>
> dma_tx_callback
> mxs_auart_tx_chars
> line 590: void *buffer = s->tx_dma_buf;
> mxs_auart_dma_tx
> line 566: sg_init_one(sgl, s->tx_dma_buf, size);
>
> Thus, possible concurrency use-after-free bugs may occur.
>
> These possible bugs are found by a static analysis tool written by myself and my manual code review.
Care to send a patch to fix up this issue?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v5 5/6] dt-bindings: pinctrl: mt8183: add binding document
From: Zhiyong Tao @ 2019-01-07 2:02 UTC (permalink / raw)
To: Sean Wang
Cc: Rob Herring, Mark Rutland, devicetree@vger.kernel.org,
Jason Cooper, srv_heupstream, Marc Zyngier, Greg Kroah-Hartman,
Erin Lo (羅雅齡), Stephen Boyd,
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux-serial@vger.kernel.org,
Mars Cheng (鄭森友), Matthias Brugger
In-Reply-To: <CAGp9LzpNTKxgPyPOFQefJ1B_459dFtGUseW2sKHTiESDVLj9jw@mail.gmail.com>
On Fri, 2019-01-04 at 01:55 -0800, Sean Wang wrote:
> On Fri, Jan 4, 2019 at 1:40 AM Zhiyong Tao <zhiyong.tao@mediatek.com> wrote:
> >
> > On Fri, 2019-01-04 at 01:14 -0800, Sean Wang wrote:
> > > On Thu, Jan 3, 2019 at 11:09 PM Zhiyong Tao <zhiyong.tao@mediatek.com> wrote:
> > > >
> > > > On Sat, 2018-12-29 at 06:04 +0800, Rob Herring wrote:
> > > > > On Fri, Dec 28, 2018 at 04:09:40PM +0800, Erin Lo wrote:
> > > > > > From: Zhiyong Tao <zhiyong.tao@mediatek.com>
> > > > > >
> > > > > > The commit adds mt8183 compatible node in binding document.
> > > > > >
> > > > > > Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
> > > > > > Signed-off-by: Erin Lo <erin.lo@mediatek.com>
> > > > > > ---
> > > > > > .../devicetree/bindings/pinctrl/pinctrl-mt8183.txt | 110 +++++++++++++++++++++
> > > > > > 1 file changed, 110 insertions(+)
> > > > > > create mode 100644 Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
> > > > > > new file mode 100644
> > > > > > index 0000000..7b5285e
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt8183.txt
> > > > > > @@ -0,0 +1,110 @@
> > > > > > +* Mediatek MT8183 Pin Controller
> > > > > > +
> > > > > > +The Mediatek's Pin controller is used to control SoC pins.
> > > > > > +
> > > > > > +Required properties:
> > > > > > +- compatible: value should be one of the following.
> > > > > > + "mediatek,mt8183-pinctrl", compatible with mt8183 pinctrl.
> > > > > > +- gpio-controller : Marks the device node as a gpio controller.
> > > > > > +- #gpio-cells: number of cells in GPIO specifier. Since the generic GPIO
> > > > > > + binding is used, the amount of cells must be specified as 2. See the below
> > > > > > + mentioned gpio binding representation for description of particular cells.
> > > > > > +- gpio-ranges : gpio valid number range.
> > > > > > +
> > > > > > + Eg: <&pio 6 0>
> > > > > > + <[phandle of the gpio controller node]
> > > > > > + [line number within the gpio controller]
> > > > > > + [flags]>
> > > > > > +
> > > > > > + Values for gpio specifier:
> > > > > > + - Line number: is a value between 0 to 202.
> > > > > > + - Flags: bit field of flags, as defined in <dt-bindings/gpio/gpio.h>.
> > > > > > + Only the following flags are supported:
> > > > > > + 0 - GPIO_ACTIVE_HIGH
> > > > > > + 1 - GPIO_ACTIVE_LOW
> > > > > > +
> > > > > > +Optional properties:
> > > > > > +- reg: physicall address base for gpio base registers.
> > > > >
> > > > > s/physicall/physical/
> > > > >
> > > > > reg should never be optional.
> > > > >
> > > > > Need to say how many reg entries.
> > > >
> > > > ==> Thanks for your suggestion. We will change 'reg' to Required
> > > > properties and add the reg entries in next version.
> > > > >
> > > > > > +- reg-names: gpio base registers name.
> > > > >
> > > > > Need to say what are the names. However, I don't find the names in the
> > > > > example all that useful, so I'd just drop it.
> > > >
> > > > ==> we will add the reg-names in next version.
> > > > They are used in the sample code, such as:
> > > > > + reg-names = "iocfg0", "iocfg1", "iocfg2",
> > > > > > + "iocfg3", "iocfg4", "iocfg5",
> > > > > > + "iocfg6", "iocfg7", "iocfg8";
> > > >
> > > > >
> > > > > > +- interrupt-controller: Marks the device node as an interrupt controller
> > > > > > +- #interrupt-cells: Should be two.
> > > > > > +- interrupts : The interrupt outputs from the controller.
> > > > > > +
> > > > > > +Please refer to pinctrl-bindings.txt in this directory for details of the
> > > > > > +common pinctrl bindings used by client devices.
> > > > > > +
> > > > > > +Subnode format
> > > > > > +A pinctrl node should contain at least one subnodes representing the
> > > > > > +pinctrl groups available on the machine. Each subnode will list the
> > > > > > +pins it needs, and how they should be configured, with regard to muxer
> > > > > > +configuration, pullups, drive strength, input enable/disable and input schmitt.
> > > > > > +
> > > > > > + node {
> > > > > > + pinmux = <PIN_NUMBER_PINMUX>;
> > > > > > + GENERIC_PINCONFIG;
> > > > > > + };
> > > > > > +
> > > > > > +Required properties:
> > > > > > +- pinmux: integer array, represents gpio pin number and mux setting.
> > > > > > + Supported pin number and mux varies for different SoCs, and are defined
> > > > > > + as macros in boot/dts/<soc>-pinfunc.h directly.
> > > > > > +
> > > > > > +Optional properties:
> > > > > > +- GENERIC_PINCONFIG: is the generic pinconfig options to use, bias-disable,
> > > > > > + bias-pull-down, bias-pull-up, input-enable, input-disable, output-low, output-high,
> > > > > > + input-schmitt-enable, input-schmitt-disable and drive-strength are valid.
> > > > > > +
> > > > > > + Some special pins have extra pull up strength, there are R0 and R1 pull-up
> > > > > > + resistors available, but for user, it's only need to set R1R0 as 00, 01, 10 or 11.
> > > > > > + So when config mediatek,pull-up-adv or mediatek,pull-down-adv,
> > > > > > + it support arguments for those special pins.
> > > > > > +
> > > > > > + When config drive-strength, it can support some arguments, such as
> > > > > > + MTK_DRIVE_4mA, MTK_DRIVE_6mA, etc. See dt-bindings/pinctrl/mt65xx.h.
> > > > > > +
> > >
> > > One point we can fix more is the drive-strength already is supported
> > > as the generic one, not need to depend on a dedicated header file.
> >
> > ==> We think that we use MTK_DRIVE_4mA to set as driving in dts. it it
> > more clear in v2 common driver for single pin -> single group mode.
> > How about the idea?
> >
>
> Sure, thank for post your idea and it sounds not bad to keep the
> convention for the single pin -> single group mode binding.
Thanks for your confirm. We will keep it in next version.
>
> > Thanks.
> > >
> > > > > > +Examples:
> > > > > > +
> > > > > > +#include "mt8183-pinfunc.h"
> > > > > > +
> > > > > > +...
> > > > > > +{
> > > > > > + pio: pinctrl@10005000 {
> > > > > > + compatible = "mediatek,mt8183-pinctrl";
> > > > > > + reg = <0 0x10005000 0 0x1000>,
> > > > > > + <0 0x11F20000 0 0x1000>,
> > > > > > + <0 0x11E80000 0 0x1000>,
> > > > > > + <0 0x11E70000 0 0x1000>,
> > > > > > + <0 0x11E90000 0 0x1000>,
> > > > > > + <0 0x11D30000 0 0x1000>,
> > > > > > + <0 0x11D20000 0 0x1000>,
> > > > > > + <0 0x11C50000 0 0x1000>,
> > > > > > + <0 0x11F30000 0 0x1000>;
> > > > > > + reg-names = "iocfg0", "iocfg1", "iocfg2",
> > > > > > + "iocfg3", "iocfg4", "iocfg5",
> > > > > > + "iocfg6", "iocfg7", "iocfg8";
> > > > > > + gpio-controller;
> > > > > > + #gpio-cells = <2>;
> > > > > > + gpio-ranges = <&pio 0 0 192>;
> > > > > > + interrupt-controller;
> > > > > > + interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > + interrupt-parent = <&gic>;
> > > > > > + #interrupt-cells = <2>;
> > > > > > +
> > > > > > + i2c0_pins_a: i2c0@0 {
> > > > >
> > > > > unit-address without reg property is not valid.
> > > >
> > > > ==> we will change "i2c0_pins_a: i2c0@0" to "i2c0_pins_a: i2c0" in the
> > > > next version.
> > > > >
> > > > > > + pins1 {
> > > > > > + pinmux = <PINMUX_GPIO48__FUNC_SCL5>,
> > > > > > + <PINMUX_GPIO49__FUNC_SDA5>;
> > > > > > + mediatek,pull-up-adv = <11>;
> > > > > > + };
> > > > > > + };
> > > > > > +
> > > > > > + i2c1_pins_a: i2c1@0 {
> > > > > > + pins {
> > > > > > + pinmux = <PINMUX_GPIO50__FUNC_SCL3>,
> > > > > > + <PINMUX_GPIO51__FUNC_SDA3>;
> > > > > > + mediatek,pull-down-adv = <10>;
> > > > > > + };
> > > > > > + };
> > > > > > + ...
> > > > > > + };
> > > > > > +};
> > > > > > --
> > > > > > 1.9.1
> > > > > >
> > > >
> > > >
> > > >
> > > > _______________________________________________
> > > > Linux-mediatek mailing list
> > > > Linux-mediatek@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> >
> >
^ permalink raw reply
* Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
From: Ben Whitten @ 2019-01-05 9:18 UTC (permalink / raw)
To: Andreas Färber, linux-lpwan, linux-serial
Cc: Rob Herring, devicetree, netdev, linux-usb, linux-kernel,
Johan Hovold, David S. Miller
In-Reply-To: <20190104112131.14451-4-afaerber@suse.de>
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 <afaerber@suse.de>
> ---
> 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 000000000000..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 <linux/completion.h>
> +#include <linux/lora/sx130x.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/serdev.h>
> +#include <linux/serial.h>
> +#include <linux/slab.h>
> +
> +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(&sdev->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(&sdev->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(&sdev->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(&picodev->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(&picodev->answer_comp);
> + complete(&picodev->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, 'r', addr, &dummy, false ? 1 : 0);
> + if (ret)
> + return ret;
> +
> + ret = picogw_recv_answer(picodev, &cmd, &ack, val, 1, timeout);
> + if (ret < 0)
> + return ret;
> + if (cmd != 'r')
> + return -EIO;
> + if (!ack || ret != 1)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int picogw_reg_write(struct picogw_device *picodev, u8 addr, u8 val, unsigned long timeout)
> +{
> + char cmd;
> + bool ack;
> + int ret;
> +
> + ret = picogw_send_cmd(picodev, 'w', addr, &val, 1);
> + if (ret)
> + return ret;
> +
> + ret = picogw_recv_answer(picodev, &cmd, &ack, NULL, 0, timeout);
> + if (ret < 0)
> + return ret;
> + if (cmd != 'w')
> + return -EIO;
> + if (!ack || ret != 0)
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int picogw_mcu_fw_check(struct picogw_device *picodev,
> + u32 fw_version, u8 *id, unsigned long timeout)
> +{
> + char cmd;
> + bool ack;
> + int ret;
> +
> + fw_version = cpu_to_be32(fw_version);
> + ret = picogw_send_cmd(picodev, 'l', 0, &fw_version, sizeof(fw_version));
> + if (ret)
> + return ret;
> +
> + ret = picogw_recv_answer(picodev, &cmd, &ack, id, id ? 8 : 0, timeout);
> + if (ret < 0)
> + return ret;
> + if (cmd != 'l')
> + return -EIO;
> + if (id && ret != 8)
> + return -EIO;
> +
> + return ack ? 0 : -ENOTSUPP;
> +}
> +
> +static int picogw_regmap_gather_write(void *context,
> + const void *reg_buf, size_t reg_size, const void *val_buf, size_t val_size)
> +{
> + struct picogw_device *picodev = context;
> + const u8 *addr_buf = reg_buf;
> + const u8 *val = val_buf;
> + u8 addr;
> + int ret;
> +
> + //dev_dbg(&picodev->serdev->dev, "%s: 0x%x (reg_size %zu) 0x%x (val_size %zu)\n",
> + // __func__, (unsigned int)addr_buf[0], reg_size, (unsigned int)val[0], val_size);
> +
> + if (reg_size != 1 || val_size > 0xffff)
> + return -EINVAL;
> +
> + addr = addr_buf[0] & ~BIT(7);
> + if (val_size == 1) {
> + ret = picogw_reg_write(picodev, addr, val[0], HZ);
> + if (ret)
> + return ret;
> + return 0;
> + } else {
> + /* TODO burst mode */
> + dev_err(&picodev->serdev->dev, "burst mode write not yet implemented\n");
> + return -ENOTSUPP;
> + }
> +}
> +
> +static int picogw_regmap_write(void *context,
> + const void *data_buf, size_t count)
> +{
> + const u8 *data = data_buf;
> + if (count < 1)
> + return -EINVAL;
> +
> + return picogw_regmap_gather_write(context, data, 1, data + 1, count - 1);
> +}
> +
> +static int picogw_regmap_read(void *context,
> + const void *reg_buf, size_t reg_size, void *val_buf, size_t val_size)
> +{
> + struct picogw_device *picodev = context;
> + const u8 *addr_buf = reg_buf;
> + u8 addr;
> + int ret;
> +
> + //dev_dbg(&picodev->serdev->dev, "%s: 0x%x (reg_size %zu) (val_size %zu)\n", __func__, (unsigned int)addr_buf[0], reg_size, val_size);
> +
> + if (reg_size != 1 || val_size > 0xffff)
> + return -EINVAL;
> +
> + addr = addr_buf[0] & ~BIT(7);
> + if (val_size == 1) {
> + ret = picogw_reg_read(picodev, addr, val_buf, HZ);
> + if (ret)
> + return ret;
> + return 0;
> + } else {
> + /* TODO burst mode */
> + dev_err(&picodev->serdev->dev, "burst mode read not yet implemented\n");
> + return -ENOTSUPP;
> + }
> +}
> +
> +static const struct regmap_bus picogw_regmap_bus = {
> + .write = picogw_regmap_write,
> + .gather_write = picogw_regmap_gather_write,
> + .read = picogw_regmap_read,
> +
> + .max_raw_write = 0xffff,
> + .max_raw_read = 0xffff,
> +};
> +
> +static int picogw_handle_answer(struct picogw_device *picodev)
> +{
> + struct device *dev = &picodev->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(&picodev->answer_comp);
> + ret = wait_for_completion_interruptible_timeout(&picodev->answer_read_comp, HZ / 2);
> + if (ret < 0)
> + return 0;
> +
> + reinit_completion(&picodev->answer_read_comp);
> +
> + return cmd_len;
> +}
> +
> +static int picogw_receive_buf(struct serdev_device *sdev, const u8 *data, size_t count)
> +{
> + struct picogw_device *picodev = picogw_get_drvdata(sdev);
> + size_t i;
> + int len = 0;
> +
> + dev_dbg(&sdev->dev, "Receive (%zu)\n", count);
> +
> + if (completion_done(&picodev->answer_comp)) {
> + dev_info(&sdev->dev, "RX waiting on completion\n");
> + return 0;
> + }
> + if (picodev->rx_len == sizeof(picodev->rx_buf)) {
> + dev_warn(&sdev->dev, "RX buffer full\n");
> + return 0;
> + }
> +
> + i = min(count, sizeof(picodev->rx_buf) - picodev->rx_len);
> + if (i > 0) {
> + memcpy(&picodev->rx_buf[picodev->rx_len], data, i);
> + picodev->rx_len += i;
> + len += i;
> + }
> +
> + while (picodev->rx_len > 0) {
> + /* search for valid beginning */
> + for (i = 0; i < picodev->rx_len; i++) {
> + if (picogw_valid_cmd(picodev->rx_buf[i]))
> + break;
> + }
> + if (i > 0) {
> + dev_dbg(&sdev->dev, "skipping %zu bytes of garbage\n", i);
> + if (i < picodev->rx_len) {
> + memmove(picodev->rx_buf, &picodev->rx_buf[i], picodev->rx_len - i);
> + picodev->rx_len -= i;
> + } else
> + picodev->rx_len = 0;
> + }
> +
> + i = picogw_handle_answer(picodev);
> + if (i == 0)
> + break;
> +
> + if (i % 64 == 0) {
> + dev_info(&sdev->dev, "skipping padding byte\n");
> + i++;
> + }
> + if (picodev->rx_len > i)
> + memmove(picodev->rx_buf, &picodev->rx_buf[i], picodev->rx_len - i);
> + if (picodev->rx_len >= i)
> + picodev->rx_len -= i;
> + }
> +
> + return len;
> +}
> +
> +static const struct serdev_device_ops picogw_serdev_client_ops = {
> + .receive_buf = picogw_receive_buf,
> + .write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +static int picogw_serdev_probe(struct serdev_device *sdev)
> +{
> + struct picogw_device *picodev;
> + struct regmap *regmap;
> + u32 fw_version = 0x010a0006;
Worth moving the version to a define?
> + u8 mac[8];here
> + int ret;
> +
> + //dev_info(&sdev->dev, "Probing\n");
> +
> + picodev = devm_kzalloc(&sdev->dev, sizeof(*picodev), GFP_KERNEL);
> + if (!picodev)
> + return -ENOMEM;
> +
> + picodev->serdev = sdev;
> + init_completion(&picodev->answer_comp);
> + init_completion(&picodev->answer_read_comp);
> +
> + ret = serdev_device_open(sdev);
> + if (ret) {
> + dev_err(&sdev->dev, "Failed to open (%d)\n", ret);
> + return ret;
> + }
> +
> + serdev_device_set_baudrate(sdev, 115200);
> + serdev_device_set_parity(sdev, SERDEV_PARITY_NONE);
> + serdev_device_set_flow_control(sdev, true);
> +
> + regmap = devm_regmap_init(&sdev->dev, &picogw_regmap_bus, picodev, &sx130x_regmap_config);
> + if (IS_ERR(regmap)) {
> + ret = PTR_ERR(regmap);
> + dev_err(&sdev->dev, "error initializing regmap (%d)\n", ret);
> + serdev_device_close(sdev);
> + return ret;
> + }
> +
> + ret = sx130x_early_probe(regmap, NULL);
> + if (ret) {
> + serdev_device_close(sdev);
> + return ret;
> + }
> +
> + sx130x_set_drvdata(&sdev->dev, picodev);
> + serdev_device_set_client_ops(sdev, &picogw_serdev_client_ops);
> +
> + //msleep(1000);
> + ret = picogw_mcu_fw_check(picodev, fw_version, mac, HZ);
> + if (!ret || ret == -ENOTSUPP)
> + dev_info(&sdev->dev, "ID = %02x%02x%02x%02x%02x%02x%02x%02x\n",
> + (unsigned int)mac[0],
> + (unsigned int)mac[1],
> + (unsigned int)mac[2],
> + (unsigned int)mac[3],
> + (unsigned int)mac[4],
> + (unsigned int)mac[5],
> + (unsigned int)mac[6],
> + (unsigned int)mac[7]);
> + while (ret == -ENOTSUPP && ((fw_version & 0xff) > 4)) {
> + ret = picogw_mcu_fw_check(picodev, --fw_version, NULL, HZ);
> + }
> + if (ret == -ENOTSUPP) {
> + dev_warn(&sdev->dev, "firmware check failed (%08x)\n", fw_version);
> + } else {
> + dev_err(&sdev->dev, "ID retrieval failed (%d)\n", ret);
> + serdev_device_close(sdev);
> + return ret;
> + }
> +
> + ret = sx130x_probe(&sdev->dev);
> + if (ret) {
> + serdev_device_close(sdev);
> + return ret;
> + }
> +
> + //dev_info(&sdev->dev, "Done.\n");
> +
> + return 0;
> +}
> +
> +static void picogw_serdev_remove(struct serdev_device *sdev)
> +{
> + sx130x_remove(&sdev->dev);
> +
> + serdev_device_close(sdev);
> +
> + //dev_info(&sdev->dev, "Removed\n");
> +}
> +
> +static const struct of_device_id picogw_serdev_of_match[] = {
> + { .compatible = "semtech,lora-picocell" },
lora-picocell or lora-picogw... picocell has mobile connotations, should
we match the module name?
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, picogw_serdev_of_match);
> +
> +static struct serdev_device_driver picogw_serdev_driver = {
> + .probe = picogw_serdev_probe,
> + .remove = picogw_serdev_remove,
> + .driver = {
> + .name = "lora-picogw",
> + .of_match_table = picogw_serdev_of_match,
> + },
> +};
> +
> +static int __init picogw_serdev_init(void)
> +{
> + int ret;
> +
> + ret = serdev_device_driver_register(&picogw_serdev_driver);
> + if (ret) {
> + pr_err("serdev_device_driver_register failed (%d)", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +module_init(picogw_serdev_init);
> +
> +static void __exit picogw_serdev_exit(void)
> +{
> + serdev_device_driver_unregister(&picogw_serdev_driver);
> +}
> +module_exit(picogw_serdev_exit);
> +
> +MODULE_LICENSE("GPL");
>
Thanks,
Ben Whitten
^ permalink raw reply
* Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
From: Andreas Färber @ 2019-01-05 1:49 UTC (permalink / raw)
To: linux-lpwan, Ben Whitten
Cc: linux-serial, Rob Herring, devicetree, netdev, linux-usb,
linux-kernel, Johan Hovold, David S. Miller, Mark Brown
In-Reply-To: <20190104112131.14451-4-afaerber@suse.de>
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)
^ permalink raw reply
* Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
From: Andreas Färber @ 2019-01-05 1:32 UTC (permalink / raw)
To: linux-lpwan, linux-serial
Cc: Rob Herring, netdev, linux-usb, linux-kernel, Johan Hovold
In-Reply-To: <20190104112131.14451-4-afaerber@suse.de>
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 000000000000..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)
^ permalink raw reply
* Re: [RFC lora-next 5/5] HACK: net: lora: sx130x: Add PicoCell gateway shim for cdc-acm
From: Andreas Färber @ 2019-01-04 23:43 UTC (permalink / raw)
To: Rob Herring
Cc: linux-lpwan, linux-serial, Linux USB List, devicetree,
linux-kernel@vger.kernel.org, Johan Hovold, David S. Miller,
Oliver Neukum, Greg Kroah-Hartman, netdev, linux-clk
In-Reply-To: <CAL_Jsq+BMOvRLEbD2Eo2D=n446vSubeXV3-FbkBSHjpfL=LYsg@mail.gmail.com>
Hi Rob,
Am 04.01.19 um 18:07 schrieb Rob Herring:
> On Fri, Jan 4, 2019 at 5:21 AM Andreas Färber <afaerber@suse.de> wrote:
>>
>> Ignore our device in cdc-acm probing and add a new driver for it,
>> dispatching to cdc-acm for the actual implementation.
>>
>> WARNING: It is likely that this VID/PID is in use for unrelated devices.
>> Only the Product string hints what this really is in current v0.2.1.
>> Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping
>> with such firmware is known to me.
>>
>> While this may seem unorthodox, no internals of the driver are accessed,
>> just the set of driver callbacks is duplicated as shim.
>>
>> Use this shim construct to fake DT nodes for serdev on probe.
>> For testing purposes these nodes do not have a parent yet.
>> This results in two "Error -2 creating of_node link" warnings on probe.
>
> It looks like the DT is pretty static. Rather than creating the nodes
> at run-time, can't you create a dts file and build that into your
> module.
Heh, if that were the only issue with this patch... ;)
I had read about that possibility here [1], but it just appeared to give
me a binary blob with no handy documentation on how to parse the
__dtb_XXX_begin..__dtb_XXX_end blob afterwards for assignment to
interface->dev.of_node. Two nodes and one compatible property were
enough to get me started, so that was quickest, given lack of knowledge.
I intentionally left it static to keep error handling and cleanup
manageable for now... Otherwise I'd need to kstrdup()/kzalloc() all
properties so that I can consistently kfree() them again on release.
Note that this was just a PoC, so there are properties missing here:
At least currently Ben's patch [2] (wrongly?) relies on the optional
clock-output-names property if #clock-cells property is specified -
which I did not in this patch. (Thus it'll disable clk_out, which would
break opening the netdev if we wouldn't run into other errors first.)
Any comments on how to best deal with clk names on the driver side would
be appreciated, so that we can just leave the property away here and get
a sane default name. Otherwise we'd need to generate a unique name here.
If #clock-cells were present, the driver would also rely on obtaining a
parent clock, which may be easiest for me to fix in the driver; but
assuming we need it, we'd need a clocks property pointing to phandles.
Wouldn't phandles need to be unique globally in the kernel for lookup?
phandles from a separate .dtb fragment wouldn't seem to tick that box.
(For reference there's also a clk locking issue under discussion at [3],
as well as multiple unresolved Kbuild reports about clk_hw not being
applicable on sparc64 allyesconfigs and m68k allmodconfig that I'm
unsure how to best resolve while keeping the driver broadly usable. Not
using clk would solve above DT worries but would leave us with ugly
driver dependencies across spi and a custom sx130x_radio bus.)
Kconfig may also be a topic to consider for this USB driver - my x86_64
host for instance doesn't have CONFIG_OF, so it might work to manually
allocate such nodes, whereas using API or &of_fwnode_ops (needed?) may
be a problem - although without CONFIG_OF the serdev code probably is
unable to enumerate the nodes in the first place?
And I assume on ACPI platforms hot-pluggable USB devices shouldn't need
a user-overridden ACPI table either - have you thought about some
serdev-specific lookup as fallback when OF and ACPI come up empty?
Your drivers/tty/serdev/core.c:serdev_controller_add() has access to
ctrl->dev->parent, so it could maintain a list of callbacks that drivers
(e.g., cdc-acm) could register callbacks with and cast the device here
to usb_interface; my module here would then only need to register such a
callback against cdc-acm in its module init to allow cdc-acm to provide
it with the usb_interface, where it could then check for the iProduct to
determine whether that device should be serdev-controlled or not - say
by returning 0 to bind, negative error to ignore - and loading/creating
an internal of_node or whatever necessary. Just a rough idea for now...
Even easier, serdev_device_driver could just get an optional callback!
Then my driver in 3/5 could just determine itself which device it wants
to bind against and still use the module_serdev_device_driver() macro.
(serdev is built-in, so not as easy to tweak on random boards here...)
Any comments on serdev in 4/5? I wonder whether that was an oversight
(in which case it should get a Fixes line) or an intentional choice due
to issues? You mentioned hangup and open/close mismatches before...
Thanks,
Andreas
[1] https://elinux.org/Device_Tree_Linux#FDT_built_into_kernel_as_data
[2] https://patchwork.ozlabs.org/patch/983173/
[3]
https://lists.infradead.org/pipermail/linux-lpwan/2019-January/000069.html
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* [PATCH] serial: max310x: Use struct_size() in devm_kzalloc()
From: Gustavo A. R. Silva @ 2019-01-04 21:39 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, linux-kernel, Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct foo {
int stuff;
void *entry[];
};
instance = devm_kzalloc(dev, sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = devm_kzalloc(dev, struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/tty/serial/max310x.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 4f479841769a..1d700a8ab517 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1197,8 +1197,7 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
return PTR_ERR(regmap);
/* Alloc port structure */
- s = devm_kzalloc(dev, sizeof(*s) +
- sizeof(struct max310x_one) * devtype->nr, GFP_KERNEL);
+ s = devm_kzalloc(dev, struct_size(s, p, devtype->nr), GFP_KERNEL);
if (!s) {
dev_err(dev, "Error allocating port structure\n");
return -ENOMEM;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables
From: Evan Green @ 2019-01-04 19:05 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, Doug Anderson, linux-kernel,
linux-serial, Stephen Boyd
In-Reply-To: <20190102213636.40866-5-ryandcase@chromium.org>
On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> drivers/tty/serial/qcom_geni_serial.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3103aa0adc86..fa67a2ced420 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -765,12 +765,12 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
>
> static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
> {
> - unsigned int m_irq_status;
> - unsigned int s_irq_status;
> - unsigned int geni_status;
> + u32 m_irq_en;
> + u32 m_irq_status;
> + u32 s_irq_status;
> + u32 geni_status;
> struct uart_port *uport = dev;
> unsigned long flags;
> - unsigned int m_irq_en;
> bool drop_rx = false;
> struct tty_port *tport = &uport->state->port;
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> @@ -948,14 +948,14 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> struct ktermios *termios, struct ktermios *old)
> {
> unsigned int baud;
> - unsigned int bits_per_char;
> - unsigned int tx_trans_cfg;
> - unsigned int tx_parity_cfg;
> - unsigned int rx_trans_cfg;
> - unsigned int rx_parity_cfg;
> - unsigned int stop_bit_len;
> + u32 bits_per_char;
> + u32 tx_trans_cfg;
> + u32 tx_parity_cfg;
> + u32 rx_trans_cfg;
> + u32 rx_parity_cfg;
> + u32 stop_bit_len;
> unsigned int clk_div;
> - unsigned long ser_clk_cfg;
> + u32 ser_clk_cfg;
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> unsigned long clk_rate;
>
> --
> 2.20.1.415.g653613c723-goog
>
I did a brief search tour of unsigned in this file, and also found
rxstale in qcom_geni_serial_port_setup. Other than that and the commit
message:
Reviewed-by: Evan Green <evgreen@chromium.org>
^ permalink raw reply
* Re: [PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable
From: Evan Green @ 2019-01-04 19:04 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, Doug Anderson, linux-kernel,
linux-serial, Stephen Boyd
In-Reply-To: <20190102213636.40866-4-ryandcase@chromium.org>
On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> The driver only supports FIFO mode so setting and checking this variable
> is unnecessary. If DMA support is ever addedd then such checks can be
s/addedd/added/
> introduced.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> drivers/tty/serial/qcom_geni_serial.c | 66 ++++++++++-----------------
> 1 file changed, 24 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 5521ed4a0708..3103aa0adc86 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -105,7 +105,6 @@ struct qcom_geni_serial_port {
> u32 tx_fifo_depth;
> u32 tx_fifo_width;
> u32 rx_fifo_depth;
> - enum geni_se_xfer_mode xfer_mode;
> bool setup;
> int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
> unsigned int baud;
> @@ -555,29 +554,20 @@ static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
> static void qcom_geni_serial_start_tx(struct uart_port *uport)
> {
> u32 irq_en;
> - struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> u32 status;
>
> - if (port->xfer_mode == GENI_SE_FIFO) {
> - /*
> - * readl ensures reading & writing of IRQ_EN register
> - * is not re-ordered before checking the status of the
> - * Serial Engine.
> - */
> - status = readl(uport->membase + SE_GENI_STATUS);
> - if (status & M_GENI_CMD_ACTIVE)
> - return;
> + status = readl(uport->membase + SE_GENI_STATUS);
> + if (status & M_GENI_CMD_ACTIVE)
> + return;
>
> - if (!qcom_geni_serial_tx_empty(uport))
> - return;
> + if (!qcom_geni_serial_tx_empty(uport))
> + return;
>
> - irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> - irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
> + irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
>
> - writel(DEF_TX_WM, uport->membase +
> - SE_GENI_TX_WATERMARK_REG);
> - writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> - }
> + writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> + writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> }
>
> static void qcom_geni_serial_stop_tx(struct uart_port *uport)
> @@ -588,11 +578,8 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
>
> irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> irq_en &= ~M_CMD_DONE_EN;
> - if (port->xfer_mode == GENI_SE_FIFO) {
> - irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> - writel(0, uport->membase +
> - SE_GENI_TX_WATERMARK_REG);
> - }
> + irq_en &= ~M_TX_FIFO_WATERMARK_EN;
This could be further coalesced into irq_en &= ~(M_CMD_DONE_EN |
M_TX_FIFO_WATERMARK_EN);
Reviewed-by: Evan Green <evgreen@chromium.org>
^ permalink raw reply
* Re: [PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables
From: Evan Green @ 2019-01-04 19:04 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, Doug Anderson, linux-kernel,
linux-serial, Stephen Boyd
In-Reply-To: <20190102213636.40866-3-ryandcase@chromium.org>
On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> The variables of tx_wm and rx_wm were set to the same define value in
> all cases, never updated, and the define was sometimes used
> interchangably. Remove the variables/function and use the fixed value.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> drivers/tty/serial/qcom_geni_serial.c | 23 +++--------------------
> 1 file changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index dc95b96148ed..5521ed4a0708 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -105,9 +105,6 @@ struct qcom_geni_serial_port {
> u32 tx_fifo_depth;
> u32 tx_fifo_width;
> u32 rx_fifo_depth;
> - u32 tx_wm;
> - u32 rx_wm;
> - u32 rx_rfr;
> enum geni_se_xfer_mode xfer_mode;
> bool setup;
> int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
> @@ -365,9 +362,7 @@ static int qcom_geni_serial_get_char(struct uart_port *uport)
> static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
> unsigned char c)
> {
> - struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> -
> - writel(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
> + writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> qcom_geni_serial_setup_tx(uport, 1);
> WARN_ON(!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> M_TX_FIFO_WATERMARK_EN, true));
> @@ -579,7 +574,7 @@ static void qcom_geni_serial_start_tx(struct uart_port *uport)
> irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
>
> - writel(port->tx_wm, uport->membase +
> + writel(DEF_TX_WM, uport->membase +
> SE_GENI_TX_WATERMARK_REG);
> writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> }
> @@ -852,17 +847,6 @@ static void get_tx_fifo_size(struct qcom_geni_serial_port *port)
> (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
> }
>
> -static void set_rfr_wm(struct qcom_geni_serial_port *port)
> -{
> - /*
> - * Set RFR (Flow off) to FIFO_DEPTH - 2.
> - * RX WM level at 10% RX_FIFO_DEPTH.
> - * TX WM level at 10% TX_FIFO_DEPTH.
> - */
> - port->rx_rfr = port->rx_fifo_depth - 2;
> - port->rx_wm = UART_CONSOLE_RX_WM;
> - port->tx_wm = DEF_TX_WM;
> -}
>
> static void qcom_geni_serial_shutdown(struct uart_port *uport)
> {
> @@ -903,7 +887,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>
> get_tx_fifo_size(port);
>
> - set_rfr_wm(port);
> writel(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
> /*
> * Make an unconditional cancel on the main sequencer to reset
> @@ -916,7 +899,7 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
> false, true, false);
> geni_se_config_packing(&port->se, BITS_PER_BYTE, port->rx_bytes_pw,
> false, false, true);
> - geni_se_init(&port->se, port->rx_wm, port->rx_rfr);
> + geni_se_init(&port->se, UART_CONSOLE_RX_WM, port->rx_fifo_depth - 2);
It looks like the CONSOLE part of the name was never really correct,
since this is also used by the regular uart_ops as well. You could
optionally fold in a rename of this define in this change.
I was also trying to reason about why that - 2 was there, and if that
should be - UART_CONSOLE_RX_WM. But I don't really get why it's there,
so I can't say for sure that it's conceptually the same value. So I
guess that's fine as is.
Reviewed-by: Evan Green <evgreen@chromium.org>
^ permalink raw reply
* Re: [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb()
From: Evan Green @ 2019-01-04 19:03 UTC (permalink / raw)
To: Ryan Case
Cc: Greg Kroah-Hartman, Jiri Slaby, Doug Anderson, linux-kernel,
linux-serial, Stephen Boyd
In-Reply-To: <20190102213636.40866-2-ryandcase@chromium.org>
On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> A frequent side comment has been to remove the use of writel_relaxed,
> readl_relaxed, and mb.
To provide a bit more motivation, you could add something to the
effect of "using the _relaxed variant introduces a significant amount
of complexity when reasoning about the code, and has not been shown to
provide a noticeable performance benefit in the case of this driver",
or something to that effect.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
> drivers/tty/serial/qcom_geni_serial.c | 187 +++++++++++---------------
> 1 file changed, 80 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 2b1e73193dad..dc95b96148ed 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -230,7 +230,7 @@ static unsigned int qcom_geni_serial_get_mctrl(struct uart_port *uport)
> if (uart_console(uport) || !uart_cts_enabled(uport)) {
> mctrl |= TIOCM_CTS;
> } else {
> - geni_ios = readl_relaxed(uport->membase + SE_GENI_IOS);
> + geni_ios = readl(uport->membase + SE_GENI_IOS);
> if (!(geni_ios & IO2_DATA_IN))
> mctrl |= TIOCM_CTS;
> }
> @@ -248,7 +248,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,
>
> if (!(mctrl & TIOCM_RTS))
> uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
> - writel_relaxed(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
> + writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
> }
>
> static const char *qcom_geni_serial_get_type(struct uart_port *uport)
> @@ -277,9 +277,6 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> unsigned int fifo_bits;
> unsigned long timeout_us = 20000;
>
> - /* Ensure polling is not re-ordered before the prior writes/reads */
> - mb();
> -
> if (uport->private_data) {
> port = to_dev_port(uport, uport);
> baud = port->baud;
> @@ -299,7 +296,7 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> */
> timeout_us = DIV_ROUND_UP(timeout_us, 10) * 10;
> while (timeout_us) {
> - reg = readl_relaxed(uport->membase + offset);
> + reg = readl(uport->membase + offset);
> if ((bool)(reg & field) == set)
> return true;
> udelay(10);
> @@ -312,7 +309,7 @@ static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
> {
> u32 m_cmd;
>
> - writel_relaxed(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
> + writel(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
> m_cmd = UART_START_TX << M_OPCODE_SHFT;
> writel(m_cmd, uport->membase + SE_GENI_M_CMD0);
> }
> @@ -325,13 +322,13 @@ static void qcom_geni_serial_poll_tx_done(struct uart_port *uport)
> done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> M_CMD_DONE_EN, true);
> if (!done) {
> - writel_relaxed(M_GENI_CMD_ABORT, uport->membase +
> + writel(M_GENI_CMD_ABORT, uport->membase +
> SE_GENI_M_CMD_CTRL_REG);
> irq_clear |= M_CMD_ABORT_EN;
> qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> M_CMD_ABORT_EN, true);
> }
> - writel_relaxed(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
> + writel(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
> }
>
> static void qcom_geni_serial_abort_rx(struct uart_port *uport)
> @@ -341,8 +338,8 @@ static void qcom_geni_serial_abort_rx(struct uart_port *uport)
> writel(S_GENI_CMD_ABORT, uport->membase + SE_GENI_S_CMD_CTRL_REG);
> qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> S_GENI_CMD_ABORT, false);
> - writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
> - writel_relaxed(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
> + writel(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
> + writel(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
> }
>
> #ifdef CONFIG_CONSOLE_POLL
> @@ -351,19 +348,13 @@ static int qcom_geni_serial_get_char(struct uart_port *uport)
> u32 rx_fifo;
> u32 status;
>
> - status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
> - writel_relaxed(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
> -
> - status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
> - writel_relaxed(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
> + status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
> + writel(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
>
> - /*
> - * Ensure the writes to clear interrupts is not re-ordered after
> - * reading the data.
> - */
> - mb();
> + status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
> + writel(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
>
> - status = readl_relaxed(uport->membase + SE_GENI_RX_FIFO_STATUS);
> + status = readl(uport->membase + SE_GENI_RX_FIFO_STATUS);
> if (!(status & RX_FIFO_WC_MSK))
> return NO_POLL_CHAR;
>
> @@ -376,12 +367,12 @@ static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
> {
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>
> - writel_relaxed(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
> + writel(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
> qcom_geni_serial_setup_tx(uport, 1);
> WARN_ON(!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> M_TX_FIFO_WATERMARK_EN, true));
> - writel_relaxed(c, uport->membase + SE_GENI_TX_FIFOn);
> - writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> + writel(c, uport->membase + SE_GENI_TX_FIFOn);
> + writel(M_TX_FIFO_WATERMARK_EN, uport->membase +
> SE_GENI_M_IRQ_CLEAR);
I think this could fit on one line now.
> qcom_geni_serial_poll_tx_done(uport);
> }
> @@ -390,7 +381,7 @@ static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
> #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
> static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> {
> - writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn);
> + writel(ch, uport->membase + SE_GENI_TX_FIFOn);
> }
>
> static void
> @@ -409,7 +400,7 @@ __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> bytes_to_send++;
> }
>
> - writel_relaxed(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> + writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> qcom_geni_serial_setup_tx(uport, bytes_to_send);
> for (i = 0; i < count; ) {
> size_t chars_to_write = 0;
> @@ -427,7 +418,7 @@ __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> chars_to_write = min_t(size_t, count - i, avail / 2);
> uart_console_write(uport, s + i, chars_to_write,
> qcom_geni_serial_wr_char);
> - writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> + writel(M_TX_FIFO_WATERMARK_EN, uport->membase +
> SE_GENI_M_IRQ_CLEAR);
> i += chars_to_write;
> }
> @@ -456,7 +447,7 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
> else
> spin_lock_irqsave(&uport->lock, flags);
>
> - geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + geni_status = readl(uport->membase + SE_GENI_STATUS);
>
> /* Cancel the current write to log the fault */
> if (!locked) {
> @@ -466,10 +457,10 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
> geni_se_abort_m_cmd(&port->se);
> qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> M_CMD_ABORT_EN, true);
> - writel_relaxed(M_CMD_ABORT_EN, uport->membase +
> + writel(M_CMD_ABORT_EN, uport->membase +
> SE_GENI_M_IRQ_CLEAR);
> }
> - writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> + writel(M_CMD_CANCEL_EN, uport->membase +
> SE_GENI_M_IRQ_CLEAR);
This one would also fit on one line now.
> } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
> /*
> @@ -479,9 +470,9 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
> qcom_geni_serial_poll_tx_done(uport);
>
> if (uart_circ_chars_pending(&uport->state->xmit)) {
> - irq_en = readl_relaxed(uport->membase +
> + irq_en = readl(uport->membase +
> SE_GENI_M_IRQ_EN);
This one too.
> - writel_relaxed(irq_en | M_TX_FIFO_WATERMARK_EN,
> + writel(irq_en | M_TX_FIFO_WATERMARK_EN,
> uport->membase + SE_GENI_M_IRQ_EN);
> }
> }
> @@ -585,12 +576,12 @@ static void qcom_geni_serial_start_tx(struct uart_port *uport)
> if (!qcom_geni_serial_tx_empty(uport))
> return;
>
> - irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
>
> - writel_relaxed(port->tx_wm, uport->membase +
> + writel(port->tx_wm, uport->membase +
> SE_GENI_TX_WATERMARK_REG);
This one too, though it looks like you fix that up in a later commit.
> - writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> + writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> }
> }
>
> @@ -600,35 +591,29 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
> u32 status;
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>
> - irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> irq_en &= ~M_CMD_DONE_EN;
> if (port->xfer_mode == GENI_SE_FIFO) {
> irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> - writel_relaxed(0, uport->membase +
> + writel(0, uport->membase +
> SE_GENI_TX_WATERMARK_REG);
Here too, though it also appears fixed up by a later commit.
> }
> - writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> - status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> + status = readl(uport->membase + SE_GENI_STATUS);
> /* Possible stop tx is called multiple times. */
> if (!(status & M_GENI_CMD_ACTIVE))
> return;
>
> - /*
> - * Ensure cancel command write is not re-ordered before checking
> - * the status of the Primary Sequencer.
> - */
> - mb();
> -
> geni_se_cancel_m_cmd(&port->se);
> if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> M_CMD_CANCEL_EN, true)) {
> geni_se_abort_m_cmd(&port->se);
> qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> M_CMD_ABORT_EN, true);
> - writel_relaxed(M_CMD_ABORT_EN, uport->membase +
> + writel(M_CMD_ABORT_EN, uport->membase +
> SE_GENI_M_IRQ_CLEAR);
Here too.
> }
> - writel_relaxed(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
> + writel(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
> }
>
> static void qcom_geni_serial_start_rx(struct uart_port *uport)
> @@ -637,26 +622,20 @@ static void qcom_geni_serial_start_rx(struct uart_port *uport)
> u32 status;
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>
> - status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + status = readl(uport->membase + SE_GENI_STATUS);
> if (status & S_GENI_CMD_ACTIVE)
> qcom_geni_serial_stop_rx(uport);
>
> - /*
> - * Ensure setup command write is not re-ordered before checking
> - * the status of the Secondary Sequencer.
> - */
> - mb();
> -
mmm, good deletes.
With the minor line coalescing fixed you can add my:
Reviewed-by: Evan Green <evgreen@chromium.org>
^ permalink raw reply
* [PATCH] sc16is7xx: Use struct_size() in devm_kzalloc()
From: Gustavo A. R. Silva @ 2019-01-04 17:48 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, linux-kernel, Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct foo {
int stuff;
void *entry[];
};
instance = devm_kzalloc(dev, sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = devm_kzalloc(dev, struct_size(instance, entry, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/tty/serial/sc16is7xx.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 268098681856..635178cf3eed 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1187,9 +1187,7 @@ static int sc16is7xx_probe(struct device *dev,
return PTR_ERR(regmap);
/* Alloc port structure */
- s = devm_kzalloc(dev, sizeof(*s) +
- sizeof(struct sc16is7xx_one) * devtype->nr_uart,
- GFP_KERNEL);
+ s = devm_kzalloc(dev, struct_size(s, p, devtype->nr_uart), GFP_KERNEL);
if (!s) {
dev_err(dev, "Error allocating port structure\n");
return -ENOMEM;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v9 1/2] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Vinod Koul @ 2019-01-04 17:19 UTC (permalink / raw)
To: Long Cheng
Cc: Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee, Sean Wang,
Nicolas Boichat, Matthias Brugger, Dan Williams,
Greg Kroah-Hartman, Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen, Zhenbao Liu
In-Reply-To: <1546395178-8880-2-git-send-email-long.cheng@mediatek.com>
On 02-01-19, 10:12, Long Cheng wrote:
> In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> the performance, can enable the function.
Is the DMA controller UART specific, can it work with other controllers
as well, if so you should get rid of uart name in patch
> +#define MTK_UART_APDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
Why are the channels not coming from DT?
> +
> +#define VFF_EN_B BIT(0)
> +#define VFF_STOP_B BIT(0)
> +#define VFF_FLUSH_B BIT(0)
> +#define VFF_4G_SUPPORT_B BIT(0)
> +#define VFF_RX_INT_EN0_B BIT(0) /*rx valid size >= vff thre*/
> +#define VFF_RX_INT_EN1_B BIT(1)
> +#define VFF_TX_INT_EN_B BIT(0) /*tx left size >= vff thre*/
space around /* space */ also run checkpatch to check for style errors
> +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> +{
> + unsigned int len, send, left, wpt, d_wpt, tmp;
> + int ret;
> +
> + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> + if (!left) {
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + return;
> + }
> +
> + /* Wait 1sec for flush, can't sleep*/
> + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> + tmp != VFF_FLUSH_B, 0, 1000000);
> + if (ret)
> + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + send = min_t(unsigned int, left, c->desc->avail_len);
> + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> + len = mtk_uart_apdma_read(c, VFF_LEN);
> +
> + d_wpt = wpt + send;
> + if ((d_wpt & VFF_RING_SIZE) >= len) {
> + d_wpt = d_wpt - len;
> + d_wpt = d_wpt ^ VFF_RING_WRAP;
> + }
> + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> +
> + c->desc->avail_len -= send;
> +
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> +}
> +
> +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> +{
> + struct mtk_uart_apdma_desc *d = c->desc;
> + unsigned int len, wg, rg, cnt;
> +
> + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> + !d || !vchan_next_desc(&c->vc))
> + return;
> +
> + len = mtk_uart_apdma_read(c, VFF_LEN);
> + rg = mtk_uart_apdma_read(c, VFF_RPT);
> + wg = mtk_uart_apdma_read(c, VFF_WPT);
> + if ((rg ^ wg) & VFF_RING_WRAP)
> + cnt = (wg & VFF_RING_SIZE) + len - (rg & VFF_RING_SIZE);
> + else
> + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> +
> + c->rx_status = cnt;
> + mtk_uart_apdma_write(c, VFF_RPT, wg);
> +
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> +}
this looks odd, why do you have different rx and tx start routines?
> +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + u32 tmp;
> + int ret;
> +
> + pm_runtime_get_sync(mtkd->ddev.dev);
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> + mtk_uart_apdma_write(c, VFF_THRE, 0);
> + mtk_uart_apdma_write(c, VFF_LEN, 0);
> + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> +
> + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp,
> + tmp == 0, 10, 100);
> + if (ret) {
> + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> + return ret;
> + }
register read does reset?
> +
> + if (!c->requested) {
> + c->requested = true;
> + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> + KBUILD_MODNAME, chan);
why is the irq not requested in driver probe?
> +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + enum dma_status ret;
> + unsigned long flags;
> +
> + if (!txstate)
> + return DMA_ERROR;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (ret == DMA_IN_PROGRESS) {
> + c->rx_status = mtk_uart_apdma_read(c, VFF_RPT) & VFF_RING_SIZE;
> + dma_set_residue(txstate, c->rx_status);
> + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
why set reside when it is complete? also reside can be null, that should
be checked as well
> +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> + (struct dma_chan *chan, struct scatterlist *sgl,
> + unsigned int sglen, enum dma_transfer_direction dir,
> + unsigned long tx_flags, void *context)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> +
> + if ((dir != DMA_DEV_TO_MEM) &&
> + (dir != DMA_MEM_TO_DEV)) {
> + dev_err(chan->device->dev, "bad direction\n");
> + return NULL;
> + }
we have a macro for this
> +
> + /* Now allocate and setup the descriptor */
> + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> + if (!d)
> + return NULL;
> +
> + /* sglen is 1 */
?
> +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> + struct dma_slave_config *cfg)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdmadev *mtkd =
> + to_mtk_uart_apdma_dev(c->vc.chan.device);
> +
> + c->cfg = *cfg;
> +
> + if (cfg->direction == DMA_DEV_TO_MEM) {
fg->direction is deprecated, in fact I have removed all users recently,
please do not use this
> + unsigned int rx_len = cfg->src_addr_width * 1024;
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, cfg->src_addr);
> + mtk_uart_apdma_write(c, VFF_LEN, rx_len);
> + mtk_uart_apdma_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> + mtk_uart_apdma_write(c, VFF_INT_EN,
> + VFF_RX_INT_EN0_B | VFF_RX_INT_EN1_B);
> + mtk_uart_apdma_write(c, VFF_RPT, 0);
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
why are we writing this here, we are supposed to do that when txn
starts!
> +static int mtk_uart_apdma_device_resume(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + return 0;
> +}
if you do not support this please remove this!
> +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> +{
> + while (list_empty(&mtkd->ddev.channels) == 0) {
> + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> + struct mtk_chan, vc.chan.device_node);
> +
> + list_del(&c->vc.chan.device_node);
> + tasklet_kill(&c->vc.task);
> + }
> +}
> +
> +static const struct of_device_id mtk_uart_apdma_match[] = {
> + { .compatible = "mediatek,mt6577-uart-dma", },
where is the binding document for ediatek,mt6577-uart-dma ??
--
~Vinod
^ permalink raw reply
* Re: [RFC lora-next 5/5] HACK: net: lora: sx130x: Add PicoCell gateway shim for cdc-acm
From: Rob Herring @ 2019-01-04 17:07 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-lpwan, open list:SERIAL DRIVERS, Linux USB List, devicetree,
linux-kernel@vger.kernel.org, Johan Hovold, David S. Miller,
Oliver Neukum, Greg Kroah-Hartman, netdev
In-Reply-To: <20190104112131.14451-6-afaerber@suse.de>
On Fri, Jan 4, 2019 at 5:21 AM Andreas Färber <afaerber@suse.de> wrote:
>
> Ignore our device in cdc-acm probing and add a new driver for it,
> dispatching to cdc-acm for the actual implementation.
>
> WARNING: It is likely that this VID/PID is in use for unrelated devices.
> Only the Product string hints what this really is in current v0.2.1.
> Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping
> with such firmware is known to me.
>
> While this may seem unorthodox, no internals of the driver are accessed,
> just the set of driver callbacks is duplicated as shim.
>
> Use this shim construct to fake DT nodes for serdev on probe.
> For testing purposes these nodes do not have a parent yet.
> This results in two "Error -2 creating of_node link" warnings on probe.
It looks like the DT is pretty static. Rather than creating the nodes
at run-time, can't you create a dts file and build that into your
module.
Rob
^ permalink raw reply
* [RFC lora-next 5/5] HACK: net: lora: sx130x: Add PicoCell gateway shim for cdc-acm
From: Andreas Färber @ 2019-01-04 11:21 UTC (permalink / raw)
To: linux-lpwan, linux-serial
Cc: linux-usb, devicetree, linux-kernel, Johan Hovold, Rob Herring,
Andreas Färber, David S. Miller, Oliver Neukum,
Greg Kroah-Hartman, netdev
In-Reply-To: <20190104112131.14451-1-afaerber@suse.de>
Ignore our device in cdc-acm probing and add a new driver for it,
dispatching to cdc-acm for the actual implementation.
WARNING: It is likely that this VID/PID is in use for unrelated devices.
Only the Product string hints what this really is in current v0.2.1.
Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping
with such firmware is known to me.
While this may seem unorthodox, no internals of the driver are accessed,
just the set of driver callbacks is duplicated as shim.
Use this shim construct to fake DT nodes for serdev on probe.
For testing purposes these nodes do not have a parent yet.
This results in two "Error -2 creating of_node link" warnings on probe.
Cc: Johan Hovold <johan@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
drivers/net/lora/Makefile | 2 +
drivers/net/lora/picogw.c | 337 ++++++++++++++++++++++++++++++++++++++++++++
drivers/usb/class/cdc-acm.c | 4 +
3 files changed, 343 insertions(+)
create mode 100644 drivers/net/lora/picogw.c
diff --git a/drivers/net/lora/Makefile b/drivers/net/lora/Makefile
index 5fef38abf5ed..bdcf7560dd65 100644
--- a/drivers/net/lora/Makefile
+++ b/drivers/net/lora/Makefile
@@ -27,6 +27,8 @@ 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_SX130X) += lora-picogw.o
+lora-picogw-y := picogw.o
obj-$(CONFIG_LORA_USI) += lora-usi.o
lora-usi-y := usi.o
diff --git a/drivers/net/lora/picogw.c b/drivers/net/lora/picogw.c
new file mode 100644
index 000000000000..aa5b83d21bb3
--- /dev/null
+++ b/drivers/net/lora/picogw.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Semtech PicoCell gateway USB interface
+ *
+ * Copyright (c) 2018-2019 Andreas Färber
+ */
+
+#define pr_fmt(fmt) "picocell: " fmt
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/serial.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/usb.h>
+#include <linux/usb/cdc.h>
+
+#define PICO_VID 0x0483
+#define PICO_PID 0x5740
+
+static struct usb_driver *picogw_get_acm_driver(struct usb_interface *iface)
+{
+ struct device_driver *drv;
+
+ drv = driver_find("cdc_acm", iface->dev.bus);
+ if (!drv)
+ return NULL;
+
+ return to_usb_driver(drv);
+}
+
+static void picogw_kobj_release(struct kobject *kobj)
+{
+ struct device_node *node = container_of(kobj, struct device_node, kobj);
+ struct property *prop;
+
+ prop = node->properties;
+ while (prop) {
+ struct property *next = prop->next;
+ kfree(prop);
+ prop = next;
+ }
+
+ kfree(node);
+}
+
+static struct kobj_type picogw_kobj_type = {
+ .release = picogw_kobj_release,
+};
+
+static u32 picogw_radio_a_reg = cpu_to_be32(0);
+static u32 picogw_radio_b_reg = cpu_to_be32(1);
+
+static int picogw_fake_of_nodes(struct device *dev)
+{
+ struct device_node *node = NULL;
+ struct device_node *child, *spi, *radio_a, *radio_b;
+ struct property *prop;
+
+ node = kzalloc(sizeof(*node), GFP_KERNEL);
+ if (!node)
+ return -ENOMEM;
+ node->name = "<NULL>";
+ node->full_name = "usb0483,5740";
+ node->type = "<NULL>";
+ kobject_init(&node->kobj, &picogw_kobj_type);
+ node->fwnode.ops = &of_fwnode_ops;
+
+ child = kzalloc(sizeof(*child), GFP_KERNEL);
+ if (!child) {
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ child->name = "lora";
+ child->full_name = "lora";
+ child->type = "<NULL>";
+ child->parent = node;
+ kobject_init(&child->kobj, &picogw_kobj_type);
+ child->fwnode.ops = &of_fwnode_ops;
+ node->child = child;
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (!prop) {
+ of_node_put(child);
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ prop->name = "compatible";
+ prop->value = "semtech,lora-picocell";
+ prop->length = 22;
+ child->properties = prop;
+
+ spi = kzalloc(sizeof(*spi), GFP_KERNEL);
+ if (!spi) {
+ of_node_put(child);
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ spi->name = "radio-spi";
+ spi->full_name = "radio-spi";
+ spi->type = "<NULL>";
+ spi->parent = child;
+ kobject_init(&spi->kobj, &picogw_kobj_type);
+ spi->fwnode.ops = &of_fwnode_ops;
+ child->child = spi;
+
+ radio_a = kzalloc(sizeof(*radio_a), GFP_KERNEL);
+ if (!radio_a) {
+ of_node_put(spi);
+ of_node_put(child);
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ radio_a->name = "lora@0";
+ radio_a->full_name = "lora@0";
+ radio_a->type = "<NULL>";
+ radio_a->parent = spi;
+ kobject_init(&radio_a->kobj, &picogw_kobj_type);
+ radio_a->fwnode.ops = &of_fwnode_ops;
+ spi->child = radio_a;
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (!prop) {
+ of_node_put(radio_a);
+ of_node_put(spi);
+ of_node_put(child);
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ prop->name = "compatible";
+ prop->value = "semtech,sx1257";
+ prop->length = 15;
+ radio_a->properties = prop;
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (!prop) {
+ of_node_put(radio_a);
+ of_node_put(spi);
+ of_node_put(child);
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ prop->name = "reg";
+ prop->value = &picogw_radio_a_reg;
+ prop->length = 4;
+ radio_a->properties->next = prop;
+
+ radio_b = kzalloc(sizeof(*radio_b), GFP_KERNEL);
+ if (!radio_b) {
+ of_node_put(radio_a);
+ of_node_put(spi);
+ of_node_put(child);
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ radio_b->name = "lora@1";
+ radio_b->full_name = "Lora@1";
+ radio_b->type = "<NULL>";
+ radio_b->parent = spi;
+ kobject_init(&radio_b->kobj, &picogw_kobj_type);
+ radio_b->fwnode.ops = &of_fwnode_ops;
+ radio_a->sibling = radio_b;
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (!prop) {
+ of_node_put(radio_b);
+ of_node_put(radio_a);
+ of_node_put(spi);
+ of_node_put(child);
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ prop->name = "compatible";
+ prop->value = "semtech,sx1257";
+ prop->length = 15;
+ radio_b->properties = prop;
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ if (!prop) {
+ of_node_put(radio_a);
+ of_node_put(spi);
+ of_node_put(child);
+ of_node_put(node);
+ return -ENOMEM;
+ }
+ prop->name = "reg";
+ prop->value = &picogw_radio_b_reg;
+ prop->length = 4;
+ radio_b->properties->next = prop;
+
+ dev->of_node = node;
+
+ return 0;
+}
+
+static void picogw_cleanup_of_nodes(struct device *dev)
+{
+ if (dev->of_node->parent)
+ return;
+
+ of_node_put(dev->of_node->child->child->child->sibling); /* lora@1 */
+ of_node_put(dev->of_node->child->child->child); /* lora@0 */
+ of_node_put(dev->of_node->child->child); /* radio-spi*/
+ of_node_put(dev->of_node->child); /* serdev */
+ of_node_put(dev->of_node); /* usb */
+ dev->of_node = NULL;
+}
+
+static int picogw_probe(struct usb_interface *interface,
+ const struct usb_device_id *id)
+{
+ struct usb_driver *drv;
+ int ret;
+
+ drv = picogw_get_acm_driver(interface);
+ if (!drv) {
+ dev_err(&interface->dev, "driver_find failed\n");
+ return -EPROBE_DEFER;
+ }
+
+ if (!interface->dev.of_node) {
+ dev_dbg(&interface->dev, "no of_node\n");
+ ret = picogw_fake_of_nodes(&interface->dev);
+ if (ret)
+ return ret;
+ }
+
+ ret = drv->probe(interface, id);
+ if (ret) {
+ picogw_cleanup_of_nodes(&interface->dev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void picogw_disconnect(struct usb_interface *intf)
+{
+ struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+ if (drv)
+ drv->disconnect(intf);
+ else
+ dev_warn(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+
+ picogw_cleanup_of_nodes(&intf->dev);
+}
+
+static int picogw_suspend(struct usb_interface *intf, pm_message_t message)
+{
+ struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+ if (!drv) {
+ dev_err(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+ return -ENODEV;
+ }
+
+ return drv->suspend(intf, message);
+}
+
+static int picogw_resume(struct usb_interface *intf)
+{
+ struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+ if (!drv) {
+ dev_err(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+ return -ENODEV;
+ }
+
+ return drv->resume(intf);
+}
+
+static int picogw_reset_resume(struct usb_interface *intf)
+{
+ struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+ if (!drv) {
+ dev_err(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+ return -ENODEV;
+ }
+
+ return drv->reset_resume(intf);
+}
+
+static int picogw_pre_reset(struct usb_interface *intf)
+{
+ struct usb_driver *drv = picogw_get_acm_driver(intf);
+
+ if (!drv) {
+ dev_err(&intf->dev, "%s: failed to get cdc_acm driver\n", __func__);
+ return -ENODEV;
+ }
+
+ return drv->pre_reset(intf);
+}
+
+static const struct usb_device_id picogw_usb_id_table[] = {
+ { USB_DEVICE_AND_INTERFACE_INFO(PICO_VID, PICO_PID,
+ USB_CLASS_COMM, USB_CDC_SUBCLASS_ACM, USB_CDC_ACM_PROTO_AT_V25TER) },
+ {}
+};
+MODULE_DEVICE_TABLE(usb, picogw_usb_id_table);
+
+static struct usb_driver picogw_usb_driver = {
+ .name = "lora-picogw-usb",
+ .probe = picogw_probe,
+ .disconnect = picogw_disconnect,
+ .suspend = picogw_suspend,
+ .resume = picogw_resume,
+ .reset_resume = picogw_reset_resume,
+ .pre_reset = picogw_pre_reset,
+ .id_table = picogw_usb_id_table,
+ .supports_autosuspend = 1,
+ .disable_hub_initiated_lpm = 1,
+};
+
+static int __init picogw_init(void)
+{
+ int ret;
+
+ ret = usb_register(&picogw_usb_driver);
+ if (ret < 0){
+ pr_err("usb_register failed (%d)\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+module_init(picogw_init);
+
+static void __exit picogw_exit(void)
+{
+ usb_deregister(&picogw_usb_driver);
+}
+module_exit(picogw_exit);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index c225a586c524..541c23b4fbfe 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1865,6 +1865,10 @@ static const struct usb_device_id acm_ids[] = {
.driver_info = IGNORE_DEVICE,
},
+ { USB_DEVICE_AND_INTERFACE_INFO(0x0483, 0x5740,
+ USB_CLASS_COMM, USB_CDC_SUBCLASS_ACM, USB_CDC_ACM_PROTO_AT_V25TER),
+ .driver_info = IGNORE_DEVICE },
+
/* control interfaces without any protocol set */
{ USB_INTERFACE_INFO(USB_CLASS_COMM, USB_CDC_SUBCLASS_ACM,
USB_CDC_PROTO_NONE) },
--
2.16.4
^ permalink raw reply related
* [PATCH RFC 4/5] usb: cdc-acm: Enable serdev support
From: Andreas Färber @ 2019-01-04 11:21 UTC (permalink / raw)
To: linux-lpwan, linux-serial
Cc: linux-usb, devicetree, linux-kernel, Johan Hovold, Rob Herring,
Andreas Färber, Oliver Neukum, Greg Kroah-Hartman
In-Reply-To: <20190104112131.14451-1-afaerber@suse.de>
Switch from tty_port_register_device() to tty_port_register_device_serdev()
and from tty_unregister_device() to tty_port_unregister_device().
On removal of a serdev driver sometimes count mismatch warnings were seen:
ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0))
ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0
Note: The serdev drivers appear to probe asynchronously as soon as they
are registered. Should the USB quirks in probe be moved before registration?
No noticeable difference for the devices at hand.
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
drivers/usb/class/cdc-acm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index ed8c62b2d9d1..c225a586c524 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1460,7 +1460,7 @@ static int acm_probe(struct usb_interface *intf,
usb_set_intfdata(data_interface, acm);
usb_get_intf(control_interface);
- tty_dev = tty_port_register_device(&acm->port, acm_tty_driver, minor,
+ tty_dev = tty_port_register_device_serdev(&acm->port, acm_tty_driver, minor,
&control_interface->dev);
if (IS_ERR(tty_dev)) {
rv = PTR_ERR(tty_dev);
@@ -1534,7 +1534,7 @@ static void acm_disconnect(struct usb_interface *intf)
acm_kill_urbs(acm);
cancel_work_sync(&acm->work);
- tty_unregister_device(acm_tty_driver, acm->minor);
+ tty_port_unregister_device(&acm->port, acm_tty_driver, acm->minor);
usb_free_urb(acm->ctrlurb);
for (i = 0; i < ACM_NW; i++)
--
2.16.4
^ permalink raw reply related
* [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
From: Andreas Färber @ 2019-01-04 11:21 UTC (permalink / raw)
To: linux-lpwan, linux-serial
Cc: linux-usb, devicetree, linux-kernel, Johan Hovold, Rob Herring,
Andreas Färber, David S. Miller, netdev
In-Reply-To: <20190104112131.14451-1-afaerber@suse.de>
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 <afaerber@suse.de>
---
drivers/net/lora/Makefile | 2 +
drivers/net/lora/sx130x_picogw.c | 466 +++++++++++++++++++++++++++++++++++++++
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 000000000000..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 <linux/completion.h>
+#include <linux/lora/sx130x.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/serdev.h>
+#include <linux/serial.h>
+#include <linux/slab.h>
+
+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(&sdev->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(&sdev->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(&sdev->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(&picodev->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(&picodev->answer_comp);
+ complete(&picodev->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, 'r', addr, &dummy, false ? 1 : 0);
+ if (ret)
+ return ret;
+
+ ret = picogw_recv_answer(picodev, &cmd, &ack, val, 1, timeout);
+ if (ret < 0)
+ return ret;
+ if (cmd != 'r')
+ return -EIO;
+ if (!ack || ret != 1)
+ return -EIO;
+
+ return 0;
+}
+
+static int picogw_reg_write(struct picogw_device *picodev, u8 addr, u8 val, unsigned long timeout)
+{
+ char cmd;
+ bool ack;
+ int ret;
+
+ ret = picogw_send_cmd(picodev, 'w', addr, &val, 1);
+ if (ret)
+ return ret;
+
+ ret = picogw_recv_answer(picodev, &cmd, &ack, NULL, 0, timeout);
+ if (ret < 0)
+ return ret;
+ if (cmd != 'w')
+ return -EIO;
+ if (!ack || ret != 0)
+ return -EIO;
+
+ return 0;
+}
+
+static int picogw_mcu_fw_check(struct picogw_device *picodev,
+ u32 fw_version, u8 *id, unsigned long timeout)
+{
+ char cmd;
+ bool ack;
+ int ret;
+
+ fw_version = cpu_to_be32(fw_version);
+ ret = picogw_send_cmd(picodev, 'l', 0, &fw_version, sizeof(fw_version));
+ if (ret)
+ return ret;
+
+ ret = picogw_recv_answer(picodev, &cmd, &ack, id, id ? 8 : 0, timeout);
+ if (ret < 0)
+ return ret;
+ if (cmd != 'l')
+ return -EIO;
+ if (id && ret != 8)
+ return -EIO;
+
+ return ack ? 0 : -ENOTSUPP;
+}
+
+static int picogw_regmap_gather_write(void *context,
+ const void *reg_buf, size_t reg_size, const void *val_buf, size_t val_size)
+{
+ struct picogw_device *picodev = context;
+ const u8 *addr_buf = reg_buf;
+ const u8 *val = val_buf;
+ u8 addr;
+ int ret;
+
+ //dev_dbg(&picodev->serdev->dev, "%s: 0x%x (reg_size %zu) 0x%x (val_size %zu)\n",
+ // __func__, (unsigned int)addr_buf[0], reg_size, (unsigned int)val[0], val_size);
+
+ if (reg_size != 1 || val_size > 0xffff)
+ return -EINVAL;
+
+ addr = addr_buf[0] & ~BIT(7);
+ if (val_size == 1) {
+ ret = picogw_reg_write(picodev, addr, val[0], HZ);
+ if (ret)
+ return ret;
+ return 0;
+ } else {
+ /* TODO burst mode */
+ dev_err(&picodev->serdev->dev, "burst mode write not yet implemented\n");
+ return -ENOTSUPP;
+ }
+}
+
+static int picogw_regmap_write(void *context,
+ const void *data_buf, size_t count)
+{
+ const u8 *data = data_buf;
+ if (count < 1)
+ return -EINVAL;
+
+ return picogw_regmap_gather_write(context, data, 1, data + 1, count - 1);
+}
+
+static int picogw_regmap_read(void *context,
+ const void *reg_buf, size_t reg_size, void *val_buf, size_t val_size)
+{
+ struct picogw_device *picodev = context;
+ const u8 *addr_buf = reg_buf;
+ u8 addr;
+ int ret;
+
+ //dev_dbg(&picodev->serdev->dev, "%s: 0x%x (reg_size %zu) (val_size %zu)\n", __func__, (unsigned int)addr_buf[0], reg_size, val_size);
+
+ if (reg_size != 1 || val_size > 0xffff)
+ return -EINVAL;
+
+ addr = addr_buf[0] & ~BIT(7);
+ if (val_size == 1) {
+ ret = picogw_reg_read(picodev, addr, val_buf, HZ);
+ if (ret)
+ return ret;
+ return 0;
+ } else {
+ /* TODO burst mode */
+ dev_err(&picodev->serdev->dev, "burst mode read not yet implemented\n");
+ return -ENOTSUPP;
+ }
+}
+
+static const struct regmap_bus picogw_regmap_bus = {
+ .write = picogw_regmap_write,
+ .gather_write = picogw_regmap_gather_write,
+ .read = picogw_regmap_read,
+
+ .max_raw_write = 0xffff,
+ .max_raw_read = 0xffff,
+};
+
+static int picogw_handle_answer(struct picogw_device *picodev)
+{
+ struct device *dev = &picodev->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(&picodev->answer_comp);
+ ret = wait_for_completion_interruptible_timeout(&picodev->answer_read_comp, HZ / 2);
+ if (ret < 0)
+ return 0;
+
+ reinit_completion(&picodev->answer_read_comp);
+
+ return cmd_len;
+}
+
+static int picogw_receive_buf(struct serdev_device *sdev, const u8 *data, size_t count)
+{
+ struct picogw_device *picodev = picogw_get_drvdata(sdev);
+ size_t i;
+ int len = 0;
+
+ dev_dbg(&sdev->dev, "Receive (%zu)\n", count);
+
+ if (completion_done(&picodev->answer_comp)) {
+ dev_info(&sdev->dev, "RX waiting on completion\n");
+ return 0;
+ }
+ if (picodev->rx_len == sizeof(picodev->rx_buf)) {
+ dev_warn(&sdev->dev, "RX buffer full\n");
+ return 0;
+ }
+
+ i = min(count, sizeof(picodev->rx_buf) - picodev->rx_len);
+ if (i > 0) {
+ memcpy(&picodev->rx_buf[picodev->rx_len], data, i);
+ picodev->rx_len += i;
+ len += i;
+ }
+
+ while (picodev->rx_len > 0) {
+ /* search for valid beginning */
+ for (i = 0; i < picodev->rx_len; i++) {
+ if (picogw_valid_cmd(picodev->rx_buf[i]))
+ break;
+ }
+ if (i > 0) {
+ dev_dbg(&sdev->dev, "skipping %zu bytes of garbage\n", i);
+ if (i < picodev->rx_len) {
+ memmove(picodev->rx_buf, &picodev->rx_buf[i], picodev->rx_len - i);
+ picodev->rx_len -= i;
+ } else
+ picodev->rx_len = 0;
+ }
+
+ i = picogw_handle_answer(picodev);
+ if (i == 0)
+ break;
+
+ if (i % 64 == 0) {
+ dev_info(&sdev->dev, "skipping padding byte\n");
+ i++;
+ }
+ if (picodev->rx_len > i)
+ memmove(picodev->rx_buf, &picodev->rx_buf[i], picodev->rx_len - i);
+ if (picodev->rx_len >= i)
+ picodev->rx_len -= i;
+ }
+
+ return len;
+}
+
+static const struct serdev_device_ops picogw_serdev_client_ops = {
+ .receive_buf = picogw_receive_buf,
+ .write_wakeup = serdev_device_write_wakeup,
+};
+
+static int picogw_serdev_probe(struct serdev_device *sdev)
+{
+ struct picogw_device *picodev;
+ struct regmap *regmap;
+ u32 fw_version = 0x010a0006;
+ u8 mac[8];
+ int ret;
+
+ //dev_info(&sdev->dev, "Probing\n");
+
+ picodev = devm_kzalloc(&sdev->dev, sizeof(*picodev), GFP_KERNEL);
+ if (!picodev)
+ return -ENOMEM;
+
+ picodev->serdev = sdev;
+ init_completion(&picodev->answer_comp);
+ init_completion(&picodev->answer_read_comp);
+
+ ret = serdev_device_open(sdev);
+ if (ret) {
+ dev_err(&sdev->dev, "Failed to open (%d)\n", ret);
+ return ret;
+ }
+
+ serdev_device_set_baudrate(sdev, 115200);
+ serdev_device_set_parity(sdev, SERDEV_PARITY_NONE);
+ serdev_device_set_flow_control(sdev, true);
+
+ regmap = devm_regmap_init(&sdev->dev, &picogw_regmap_bus, picodev, &sx130x_regmap_config);
+ if (IS_ERR(regmap)) {
+ ret = PTR_ERR(regmap);
+ dev_err(&sdev->dev, "error initializing regmap (%d)\n", ret);
+ serdev_device_close(sdev);
+ return ret;
+ }
+
+ ret = sx130x_early_probe(regmap, NULL);
+ if (ret) {
+ serdev_device_close(sdev);
+ return ret;
+ }
+
+ sx130x_set_drvdata(&sdev->dev, picodev);
+ serdev_device_set_client_ops(sdev, &picogw_serdev_client_ops);
+
+ //msleep(1000);
+ ret = picogw_mcu_fw_check(picodev, fw_version, mac, HZ);
+ if (!ret || ret == -ENOTSUPP)
+ dev_info(&sdev->dev, "ID = %02x%02x%02x%02x%02x%02x%02x%02x\n",
+ (unsigned int)mac[0],
+ (unsigned int)mac[1],
+ (unsigned int)mac[2],
+ (unsigned int)mac[3],
+ (unsigned int)mac[4],
+ (unsigned int)mac[5],
+ (unsigned int)mac[6],
+ (unsigned int)mac[7]);
+ while (ret == -ENOTSUPP && ((fw_version & 0xff) > 4)) {
+ ret = picogw_mcu_fw_check(picodev, --fw_version, NULL, HZ);
+ }
+ if (ret == -ENOTSUPP) {
+ dev_warn(&sdev->dev, "firmware check failed (%08x)\n", fw_version);
+ } else {
+ dev_err(&sdev->dev, "ID retrieval failed (%d)\n", ret);
+ serdev_device_close(sdev);
+ return ret;
+ }
+
+ ret = sx130x_probe(&sdev->dev);
+ if (ret) {
+ serdev_device_close(sdev);
+ return ret;
+ }
+
+ //dev_info(&sdev->dev, "Done.\n");
+
+ return 0;
+}
+
+static void picogw_serdev_remove(struct serdev_device *sdev)
+{
+ sx130x_remove(&sdev->dev);
+
+ serdev_device_close(sdev);
+
+ //dev_info(&sdev->dev, "Removed\n");
+}
+
+static const struct of_device_id picogw_serdev_of_match[] = {
+ { .compatible = "semtech,lora-picocell" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, picogw_serdev_of_match);
+
+static struct serdev_device_driver picogw_serdev_driver = {
+ .probe = picogw_serdev_probe,
+ .remove = picogw_serdev_remove,
+ .driver = {
+ .name = "lora-picogw",
+ .of_match_table = picogw_serdev_of_match,
+ },
+};
+
+static int __init picogw_serdev_init(void)
+{
+ int ret;
+
+ ret = serdev_device_driver_register(&picogw_serdev_driver);
+ if (ret) {
+ pr_err("serdev_device_driver_register failed (%d)", ret);
+ return ret;
+ }
+
+ return 0;
+}
+module_init(picogw_serdev_init);
+
+static void __exit picogw_serdev_exit(void)
+{
+ serdev_device_driver_unregister(&picogw_serdev_driver);
+}
+module_exit(picogw_serdev_exit);
+
+MODULE_LICENSE("GPL");
--
2.16.4
^ permalink raw reply related
* [PATCH lora-next 2/5] net: lora: sx130x: Prepare storing driver-specific data
From: Andreas Färber @ 2019-01-04 11:21 UTC (permalink / raw)
To: linux-lpwan, linux-serial
Cc: linux-usb, devicetree, linux-kernel, Johan Hovold, Rob Herring,
Andreas Färber, David S. Miller, netdev
In-Reply-To: <20190104112131.14451-1-afaerber@suse.de>
Some drivers (e.g., serdev) may need to access private data not part of
the core sx130x_priv, which is inaccessible to other source files.
As the sx130x core expects to obtain the net_device from the dev's drvdata,
we can't reuse that in derived drivers and need a new field plus helpers.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
drivers/net/lora/sx130x.c | 19 +++++++++++++++++++
include/linux/lora/sx130x.h | 2 ++
2 files changed, 21 insertions(+)
diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 840052955874..978c921ca5ec 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -58,6 +58,7 @@ struct sx130x_priv {
struct regmap *regmap;
struct regmap_field *regmap_fields[ARRAY_SIZE(sx130x_regmap_fields)];
struct mutex io_lock;
+ void *drvdata;
};
struct regmap *sx130x_get_regmap(struct device *dev)
@@ -68,6 +69,24 @@ struct regmap *sx130x_get_regmap(struct device *dev)
return priv->regmap;
}
+void sx130x_set_drvdata(struct device *dev, void *drvdata)
+{
+ struct net_device *netdev = dev_get_drvdata(dev);
+ struct sx130x_priv *priv = netdev_priv(netdev);
+
+ priv->drvdata = drvdata;
+}
+EXPORT_SYMBOL_GPL(sx130x_set_drvdata);
+
+void *sx130x_get_drvdata(struct device *dev)
+{
+ struct net_device *netdev = dev_get_drvdata(dev);
+ struct sx130x_priv *priv = netdev_priv(netdev);
+
+ return priv->drvdata;
+}
+EXPORT_SYMBOL_GPL(sx130x_get_drvdata);
+
void sx130x_io_lock(struct device *dev)
{
struct net_device *netdev = dev_get_drvdata(dev);
diff --git a/include/linux/lora/sx130x.h b/include/linux/lora/sx130x.h
index d6f027ef283f..85b088ec77b8 100644
--- a/include/linux/lora/sx130x.h
+++ b/include/linux/lora/sx130x.h
@@ -14,6 +14,8 @@
#include <linux/regmap.h>
extern const struct regmap_config sx130x_regmap_config;
+void sx130x_set_drvdata(struct device *dev, void *drvdata);
+void *sx130x_get_drvdata(struct device *dev);
int sx130x_early_probe(struct regmap *regmap, struct gpio_desc *rst);
int sx130x_probe(struct device *dev);
int sx130x_remove(struct device *dev);
--
2.16.4
^ permalink raw reply related
* [PATCH lora-next 1/5] net: lora: sx130x: Factor out SPI specific parts
From: Andreas Färber @ 2019-01-04 11:21 UTC (permalink / raw)
To: linux-lpwan, linux-serial
Cc: linux-usb, devicetree, linux-kernel, Johan Hovold, Rob Herring,
Andreas Färber, David S. Miller, netdev
In-Reply-To: <20190104112131.14451-1-afaerber@suse.de>
Prepare for the picoGW by factoring code out into helpers using device
rather than spi_device.
While touching those lines, clean up the error output.
Split the probe function in two, to allow derived drivers to insert code
before the common probing code. This may need some more work.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
drivers/net/lora/sx130x.c | 139 +++++++++++++++++++++++++++-----------------
include/linux/lora/sx130x.h | 7 +++
2 files changed, 92 insertions(+), 54 deletions(-)
diff --git a/drivers/net/lora/sx130x.c b/drivers/net/lora/sx130x.c
index 9cae9cea195f..840052955874 100644
--- a/drivers/net/lora/sx130x.c
+++ b/drivers/net/lora/sx130x.c
@@ -133,7 +133,7 @@ static bool sx130x_readable_noinc_reg(struct device *dev, unsigned int reg)
}
}
-static struct regmap_config sx130x_regmap_config = {
+const struct regmap_config sx130x_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
@@ -151,6 +151,7 @@ static struct regmap_config sx130x_regmap_config = {
.num_ranges = ARRAY_SIZE(sx130x_regmap_ranges),
.max_register = SX1301_MAX_REGISTER,
};
+EXPORT_SYMBOL_GPL(sx130x_regmap_config);
static int sx130x_field_write(struct sx130x_priv *priv,
enum sx130x_fields field_id, u8 val)
@@ -537,110 +538,98 @@ static const struct net_device_ops sx130x_net_device_ops = {
.ndo_start_xmit = sx130x_loradev_start_xmit,
};
-static int sx130x_probe(struct spi_device *spi)
+int sx130x_early_probe(struct regmap *regmap, struct gpio_desc *rst)
{
+ struct device *dev = regmap_get_device(regmap);
struct net_device *netdev;
struct sx130x_priv *priv;
- struct gpio_desc *rst;
int ret;
int i;
- unsigned int ver;
- unsigned int val;
-
- rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(rst)) {
- if (PTR_ERR(rst) != -EPROBE_DEFER)
- dev_err(&spi->dev, "Failed to obtain reset GPIO\n");
- return PTR_ERR(rst);
- }
-
- gpiod_set_value_cansleep(rst, 1);
- msleep(100);
- gpiod_set_value_cansleep(rst, 0);
- msleep(100);
-
- spi->bits_per_word = 8;
- spi_setup(spi);
- netdev = devm_alloc_loradev(&spi->dev, sizeof(*priv));
+ netdev = devm_alloc_loradev(dev, sizeof(*priv));
if (!netdev)
return -ENOMEM;
netdev->netdev_ops = &sx130x_net_device_ops;
- SET_NETDEV_DEV(netdev, &spi->dev);
+ SET_NETDEV_DEV(netdev, dev);
priv = netdev_priv(netdev);
+ priv->regmap = regmap;
priv->rst_gpio = rst;
mutex_init(&priv->io_lock);
- spi_set_drvdata(spi, netdev);
- priv->dev = &spi->dev;
-
- priv->regmap = devm_regmap_init_spi(spi, &sx130x_regmap_config);
- if (IS_ERR(priv->regmap)) {
- ret = PTR_ERR(priv->regmap);
- dev_err(&spi->dev, "Regmap allocation failed: %d\n", ret);
- return ret;
- }
+ dev_set_drvdata(dev, netdev);
+ priv->dev = dev;
for (i = 0; i < ARRAY_SIZE(sx130x_regmap_fields); i++) {
const struct reg_field *reg_fields = sx130x_regmap_fields;
- priv->regmap_fields[i] = devm_regmap_field_alloc(&spi->dev,
+ priv->regmap_fields[i] = devm_regmap_field_alloc(dev,
priv->regmap,
reg_fields[i]);
if (IS_ERR(priv->regmap_fields[i])) {
ret = PTR_ERR(priv->regmap_fields[i]);
- dev_err(&spi->dev, "Cannot allocate regmap field: %d\n", ret);
+ dev_err(dev, "Cannot allocate regmap field (%d)\n", ret);
return ret;
}
}
+ return 0;
+}
+EXPORT_SYMBOL_GPL(sx130x_early_probe);
+
+int sx130x_probe(struct device *dev)
+{
+ struct net_device *netdev = dev_get_drvdata(dev);
+ struct sx130x_priv *priv = netdev_priv(netdev);
+ unsigned int ver;
+ unsigned int val;
+ int ret;
ret = regmap_read(priv->regmap, SX1301_VER, &ver);
if (ret) {
- dev_err(&spi->dev, "version read failed\n");
+ dev_err(dev, "version read failed (%d)\n", ret);
return ret;
}
if (ver != SX1301_CHIP_VERSION) {
- dev_err(&spi->dev, "unexpected version: %u\n", ver);
+ dev_err(dev, "unexpected version: %u\n", ver);
return -ENXIO;
}
ret = regmap_write(priv->regmap, SX1301_PAGE, 0);
if (ret) {
- dev_err(&spi->dev, "page/reset write failed\n");
+ dev_err(dev, "page/reset write failed (%d)\n", ret);
return ret;
}
ret = sx130x_field_write(priv, F_SOFT_RESET, 1);
if (ret) {
- dev_err(&spi->dev, "soft reset failed\n");
+ dev_err(dev, "soft reset failed (%d)\n", ret);
return ret;
}
ret = sx130x_field_write(priv, F_GLOBAL_EN, 0);
if (ret) {
- dev_err(&spi->dev, "gate global clocks failed\n");
+ dev_err(dev, "gate global clocks failed (%d)\n", ret);
return ret;
}
ret = sx130x_field_write(priv, F_CLK32M_EN, 0);
if (ret) {
- dev_err(&spi->dev, "gate 32M clock failed\n");
+ dev_err(dev, "gate 32M clock failed (%d)\n", ret);
return ret;
}
ret = sx130x_field_write(priv, F_RADIO_A_EN, 1);
if (ret) {
- dev_err(&spi->dev, "radio a enable failed\n");
+ dev_err(dev, "radio A enable failed (%d)\n", ret);
return ret;
}
ret = sx130x_field_write(priv, F_RADIO_B_EN, 1);
if (ret) {
- dev_err(&spi->dev, "radio b enable failed\n");
+ dev_err(dev, "radio B enable failed (%d)\n", ret);
return ret;
}
@@ -648,7 +637,7 @@ static int sx130x_probe(struct spi_device *spi)
ret = sx130x_field_write(priv, F_RADIO_RST, 1);
if (ret) {
- dev_err(&spi->dev, "radio asert reset failed\n");
+ dev_err(dev, "radio assert reset failed (%d)\n", ret);
return ret;
}
@@ -656,13 +645,13 @@ static int sx130x_probe(struct spi_device *spi)
ret = sx130x_field_write(priv, F_RADIO_RST, 0);
if (ret) {
- dev_err(&spi->dev, "radio deasert reset failed\n");
+ dev_err(dev, "radio deassert reset failed (%d)\n", ret);
return ret;
}
/* radio */
- ret = devm_sx130x_register_radio_devices(&spi->dev);
+ ret = devm_sx130x_register_radio_devices(dev);
if (ret)
return ret;
@@ -672,7 +661,7 @@ static int sx130x_probe(struct spi_device *spi)
ret = regmap_read(priv->regmap, SX1301_GPMODE, &val);
if (ret) {
- dev_err(&spi->dev, "GPIO mode read failed\n");
+ dev_err(dev, "GPIO mode read failed (%d)\n", ret);
goto out;
}
@@ -680,13 +669,13 @@ static int sx130x_probe(struct spi_device *spi)
ret = regmap_write(priv->regmap, SX1301_GPMODE, val);
if (ret) {
- dev_err(&spi->dev, "GPIO mode write failed\n");
+ dev_err(dev, "GPIO mode write failed (%d)\n", ret);
goto out;
}
ret = regmap_read(priv->regmap, SX1301_GPSO, &val);
if (ret) {
- dev_err(&spi->dev, "GPIO select output read failed\n");
+ dev_err(dev, "GPIO select output read failed (%d)\n", ret);
goto out;
}
@@ -695,7 +684,7 @@ static int sx130x_probe(struct spi_device *spi)
ret = regmap_write(priv->regmap, SX1301_GPSO, val);
if (ret) {
- dev_err(&spi->dev, "GPIO select output write failed\n");
+ dev_err(dev, "GPIO select output write failed (%d)\n", ret);
goto out;
}
@@ -705,24 +694,66 @@ static int sx130x_probe(struct spi_device *spi)
if (ret)
goto out;
- dev_info(&spi->dev, "SX1301 module probed\n");
+ dev_info(dev, "SX1301 module probed\n");
out:
mutex_unlock(&priv->io_lock);
return ret;
}
+EXPORT_SYMBOL_GPL(sx130x_probe);
-static int sx130x_remove(struct spi_device *spi)
+int sx130x_remove(struct device *dev)
{
- struct net_device *netdev = spi_get_drvdata(spi);
+ struct net_device *netdev = dev_get_drvdata(dev);
unregister_loradev(netdev);
- dev_info(&spi->dev, "SX1301 module removed\n");
+ dev_info(dev, "SX1301 module removed\n");
return 0;
}
+EXPORT_SYMBOL_GPL(sx130x_remove);
+
+static int sx130x_spi_probe(struct spi_device *spi)
+{
+ struct gpio_desc *rst;
+ struct regmap *regmap;
+ int ret;
+
+ rst = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(rst)) {
+ if (PTR_ERR(rst) != -EPROBE_DEFER)
+ dev_err(&spi->dev, "Failed to obtain reset GPIO\n");
+ return PTR_ERR(rst);
+ }
+
+ gpiod_set_value_cansleep(rst, 1);
+ msleep(100);
+ gpiod_set_value_cansleep(rst, 0);
+ msleep(100);
+
+ spi->bits_per_word = 8;
+ spi_setup(spi);
+
+ regmap = devm_regmap_init_spi(spi, &sx130x_regmap_config);
+ if (IS_ERR(regmap)) {
+ ret = PTR_ERR(regmap);
+ dev_err(&spi->dev, "Regmap allocation failed: %d\n", ret);
+ return ret;
+ }
+
+ ret = sx130x_early_probe(regmap, rst);
+ if (ret)
+ return ret;
+
+ return sx130x_probe(&spi->dev);
+}
+
+static int sx130x_spi_remove(struct spi_device *spi)
+{
+ return sx130x_remove(&spi->dev);;
+}
#ifdef CONFIG_OF
static const struct of_device_id sx130x_dt_ids[] = {
@@ -737,8 +768,8 @@ static struct spi_driver sx130x_spi_driver = {
.name = "sx130x",
.of_match_table = of_match_ptr(sx130x_dt_ids),
},
- .probe = sx130x_probe,
- .remove = sx130x_remove,
+ .probe = sx130x_spi_probe,
+ .remove = sx130x_spi_remove,
};
static int __init sx130x_init(void)
diff --git a/include/linux/lora/sx130x.h b/include/linux/lora/sx130x.h
index ac4e2e7ae18a..d6f027ef283f 100644
--- a/include/linux/lora/sx130x.h
+++ b/include/linux/lora/sx130x.h
@@ -9,9 +9,16 @@
#define LORA_SX130X_H
#include <linux/device.h>
+#include <linux/gpio/consumer.h>
#include <linux/module.h>
#include <linux/regmap.h>
+extern const struct regmap_config sx130x_regmap_config;
+int sx130x_early_probe(struct regmap *regmap, struct gpio_desc *rst);
+int sx130x_probe(struct device *dev);
+int sx130x_remove(struct device *dev);
+
+
struct sx130x_radio_device {
struct device dev;
struct device *concentrator;
--
2.16.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox