* RE: [PATCH 1/1] tty: serial: imx: disable ageing timer interrupt if dma in use
From: Han, Nandor (GE Healthcare) @ 2017-10-11 6:22 UTC (permalink / raw)
To: Troy Kisky, gregkh@linuxfoundation.org
Cc: fabio.estevam@nxp.com, u.kleine-koenig@pengutronix.de,
linux-arm-kernel@lists.infradead.org,
linux-serial@vger.kernel.org, l.stach@pengutronix.de
In-Reply-To: <20171007014648.6989-1-troy.kisky@boundarydevices.com>
> -----Original Message-----
> From: linux-serial-owner@vger.kernel.org [mailto:linux-serial-
> owner@vger.kernel.org] On Behalf Of Troy Kisky
> Sent: 07 October 2017 04:47
> To: gregkh@linuxfoundation.org
> Cc: fabio.estevam@nxp.com; l.stach@pengutronix.de; u.kleine-
> koenig@pengutronix.de; linux-serial@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Troy Kisky <troy.kisky@boundarydevices.com>
> Subject: EXT: [PATCH 1/1] tty: serial: imx: disable ageing timer interrupt if
> dma in use
>
> Since commit 4dec2f119e86 ("imx-serial: RX DMA startup latency")
>
> the interrupt routine no longer will start rx dma.
I think you have 2 commits here:
- one that disables ageing timer when DMA is in use
- one that cleans the leftovers after 4dec2f119e86.
>
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
> drivers/tty/serial/imx.c | 34 ++--------------------------------
> 1 file changed, 2 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> dfeff3951f93..15b0ecb4cf60 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -732,29 +732,6 @@ static void imx_disable_rx_int(struct imx_port
> *sport) }
>
> static void clear_rx_errors(struct imx_port *sport); -static int
> start_rx_dma(struct imx_port *sport);
> -/*
> - * If the RXFIFO is filled with some data, and then we
> - * arise a DMA operation to receive them.
> - */
> -static void imx_dma_rxint(struct imx_port *sport) -{
> - unsigned long temp;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&sport->port.lock, flags);
> -
> - temp = readl(sport->port.membase + USR2);
> - if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
> -
> - imx_disable_rx_int(sport);
> -
> - /* tell the DMA to receive the data. */
> - start_rx_dma(sport);
> - }
> -
> - spin_unlock_irqrestore(&sport->port.lock, flags);
> -}
>
> /*
> * We have a modem side uart, so the meanings of RTS and CTS are inverted.
> @@ -816,11 +793,8 @@ static irqreturn_t imx_int(int irq, void *dev_id)
> sts = readl(sport->port.membase + USR1);
> sts2 = readl(sport->port.membase + USR2);
>
> - if (sts & (USR1_RRDY | USR1_AGTIM)) {
> - if (sport->dma_is_enabled)
> - imx_dma_rxint(sport);
> - else
> - imx_rxint(irq, dev_id);
> + if (!sport->dma_is_enabled && (sts & (USR1_RRDY |
> USR1_AGTIM))) {
> + imx_rxint(irq, dev_id);
I don't think we need the check `!sport->dma_is_enabled ` since RRDY and AGTIM interrupts should be disable when DMA is enabled.
> ret = IRQ_HANDLED;
> }
>
> @@ -1207,10 +1181,6 @@ static void imx_enable_dma(struct imx_port
> *sport)
> temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN;
> writel(temp, sport->port.membase + UCR1);
>
> - temp = readl(sport->port.membase + UCR2);
> - temp |= UCR2_ATEN;
> - writel(temp, sport->port.membase + UCR2);
> -
I think this should be moved to imx_startup. At least RRDY is configured there.
<snip>
Regards,
Nandor
^ permalink raw reply
* RE: [PATCH] tty fix oops when rmmod 8250
From: Nixiaoming @ 2017-10-11 2:03 UTC (permalink / raw)
To: Jiri Slaby, adobriyan@gmail.com, torvalds@linux-foundation.org,
gregkh@linuxfoundation.org, viro@zeniv.linux.org.uk
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-serial@vger.kernel.org
In-Reply-To: <c18e422a-00fe-00bd-75ba-384f67c4f241@suse.cz>
Test on 4.14.0-rc4:
CPU: 7 PID: 449 Comm: rmmod Tainted: G O 4.14.0-rc4+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Call Trace:
dump_stack+0x50/0x80
jtty_kref_put+0x5a/0x5c [jprobe_tty_kref_put]
uart_remove_one_port+0xe8/0x220 [serial_core]
? __might_sleep+0x4a/0x90
serial8250_unregister_port+0x71/0x100 [8250]
serial_pnp_remove+0x26/0x30 [8250]
pnp_device_remove+0x31/0x70
device_release_driver_internal+0x185/0x240
driver_detach+0x47/0x90
bus_remove_driver+0x50/0xb0
driver_unregister+0x30/0x50
pnp_unregister_driver+0x12/0x20
serial8250_pnp_exit+0x15/0x20 [8250]
serial8250_exit+0x34/0xbf8 [8250]
SyS_delete_module+0x17a/0x1f0
? exit_to_usermode_loop+0x9d/0xc0
do_syscall_64+0x5c/0x120
? syscall_return_slowpath+0xb9/0xc0
? schedule_tail+0xc1/0xe0
entry_SYSCALL64_slow_path+0x25/0x25
RIP: 0033:0x7ff7d37ab257
RSP: 002b:00007ffdb7879f08 EFLAGS: 00000202 ORIG_RAX: 00000000000000b0
RAX: ffffffffffffffda RBX: 0000000000000800 RCX: 00007ff7d37ab257
RDX: 00007ff7d38128c0 RSI: 0000000000000800 RDI: 00000000006d60f0
RBP: 00000000006d6090 R08: 00007ff7d3a5bf40 R09: 00007ffdb7878eb1
R10: 0000000000000000 R11: 0000000000000202 R12: 00007ffdb787be86
R13: 00000000006d6010 R14: 0000000000000000 R15: 00000000006d6090
BUG: unable to handle kernel paging request at ffffffffa00bdc91
IP: strchr+0x3/0x30
PGD 1c0b067 P4D 1c0b067 PUD 1c0c063 PMD 7f620067 PTE 0
Oops: 0000 [#1] PREEMPT SMP
Modules linked in: jprobe_tty_kref_put(O) iptable_filter br_netfilter bridge stp llc ipv6 ata_piix ahci libahci libata ext4 jbd2 8250_base serial_core ptp pps_core nfsd auth_rpcgss oid_registry nfsv3 nfs nfs_acl lockd sunrpc grace vfat fat quota_v2 quota_v1 quota_tree [last unloaded: 8250]
CPU: 6 PID: 74 Comm: kworker/6:1 Tainted: G O 4.14.0-rc4+ #1
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
Workqueue: events release_one_tty
task: ffff8800dbb88000 task.stack: ffffc900008d0000
RIP: 0010:strchr+0x3/0x30
RSP: 0018:ffffc900008d3b38 EFLAGS: 00010286
RAX: ffffffff81c682c0 RBX: ffffffffa00bdc91 RCX: 000000018040003c
RDX: 000000000000002f RSI: 000000000000002f RDI: ffffffffa00bdc91
RBP: ffffc900008d3b78 R08: 0000000000000000 R09: ffffffff8140e1ed
R10: ffffea0001fd6b00 R11: 0000000000000000 R12: ffff8800df412900
R13: 0000000000000010 R14: ffffc900008d3b90 R15: ffffffffa00bdc91
FS: 0000000000000000(0000) GS:ffff8800dfb80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffa00bdc91 CR3: 0000000072b7a000 CR4: 00000000000006e0
Call Trace:
? __xlate_proc_name+0x66/0xb0
remove_proc_entry+0x37/0x140
proc_tty_unregister_driver+0x28/0x40
destruct_tty_driver+0x84/0xe0
tty_driver_kref_put+0x1e/0x30
release_one_tty+0x62/0xe0
process_one_work+0x1d0/0x440
? sched_clock_local+0x1c/0x90
? schedule+0x4e/0xc0
? preempt_count_add+0xaa/0xc0
worker_thread+0x110/0x4c0
? __schedule+0x4ee/0x8b0
? default_wake_function+0x12/0x20
? __wake_up_common+0x85/0x130
? schedule+0x4e/0xc0
kthread+0x13a/0x140
? process_one_work+0x440/0x440
? kthreadd+0x1c0/0x1c0
ret_from_fork+0x22/0x30
Code: 01 41 38 c0 75 13 48 ff c1 45 84 c0 74 05 48 ff ca 75 e3 31 c0 c9 66 90 c3 41 38 c0 c9 19 c0 83 c8 01 c3 0f 1f 44 00 00 55 89 f2 <0f> b6 07 48 89 e5 40 38 f0 75 0c eb 12 48 ff c7 0f b6 07 38 d0
RIP: strchr+0x3/0x30 RSP: ffffc900008d3b38
CR2: ffffffffa00bdc91
-----
^ permalink raw reply
* Re: [PATCH 1/2] serdev: Add ACPI support
From: Ian W MORRISON @ 2017-10-10 23:13 UTC (permalink / raw)
To: Johan Hovold, Marcel Holtmann
Cc: Frédéric Danis, Rob Herring, Sebastian Reichel,
Loic Poulain, Lukas Wunner, Hans de Goede,
bluez mailin list (linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org),
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171010163630.GO4269@localhost>
On 11 October 2017 at 03:36, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Oct 10, 2017 at 10:22:19AM +0200, Marcel Holtmann wrote:
>>
>> What I was wondering the other day is if we need a lsserdev tool or
>> some integration in lshw to be able to debug what serdev devices and
>> ID are present. The lsusb and /sys/kernel/debug/usb/devices is just
>> super powerful and easy when it comes to figuring out what people have
>> in their system. Maybe /sys/kernel/debug/serdev/devices could be
>> helpful as well. Just thinking out loud here.
>
> Yeah, maybe. Since you'd typically only have small number of serdev
> devices (say, max 4), using /sys/bus/serial/devices directly should not
> be too bad meanwhile. Not that much common information we can expose
> either, at least not in comparison to USB. But I'll keep it mind. :)
>
Yes in the interim, if you have 'tree' installed then for example
$ alias lsserdev='tree /sys/bus/serial/devices/*-0'
$ lsserdev
/sys/bus/serial/devices/serial0-0
├── bluetooth
│ └── hci0
│ ├── device -> ../../../serial0-0
│ ├── power
│ │ ├── async
│ │ ├── autosuspend_delay_ms
│ │ ├── control
│ │ ├── runtime_active_kids
│ │ ├── runtime_active_time
│ │ ├── runtime_enabled
│ │ ├── runtime_status
│ │ ├── runtime_suspended_time
│ │ └── runtime_usage
│ ├── rfkill0
│ │ ├── device -> ../../hci0
│ │ ├── hard
│ │ ├── index
│ │ ├── name
│ │ ├── persistent
│ │ ├── power
│ │ │ ├── async
│ │ │ ├── autosuspend_delay_ms
│ │ │ ├── control
│ │ │ ├── runtime_active_kids
│ │ │ ├── runtime_active_time
│ │ │ ├── runtime_enabled
│ │ │ ├── runtime_status
│ │ │ ├── runtime_suspended_time
│ │ │ └── runtime_usage
│ │ ├── soft
│ │ ├── state
│ │ ├── subsystem -> ../../../../../../../../class/rfkill
│ │ ├── type
│ │ └── uevent
│ ├── subsystem -> ../../../../../../../class/bluetooth
│ └── uevent
├── driver -> ../../../../../bus/serial/drivers/hci_uart_bcm
├── firmware_node ->
../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/8086228A:00/BCM2EA4:00
├── modalias
├── power
│ ├── async
│ ├── autosuspend_delay_ms
│ ├── control
│ ├── runtime_active_kids
│ ├── runtime_active_time
│ ├── runtime_enabled
│ ├── runtime_status
│ ├── runtime_suspended_time
│ └── runtime_usage
├── subsystem -> ../../../../../bus/serial
└── uevent
13 directories, 38 files
$
It is a bit shabby when there is nothing to report
$ lsserdev
/sys/bus/serial/devices/*-0 [error opening dir]
0 directories, 0 files
$
^ permalink raw reply
* Re: [PATCH 1/2] serdev: Add ACPI support
From: Johan Hovold @ 2017-10-10 16:36 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hovold, Frédéric Danis, robh, sre, loic.poulain,
lukas, hdegoede, linux-bluetooth, linux-serial, linux-acpi
In-Reply-To: <3F3CFF22-4FCE-4283-8A41-73A9A7944494@holtmann.org>
On Tue, Oct 10, 2017 at 10:22:19AM +0200, Marcel Holtmann wrote:
> > Some of the above are minor nits that can be addressed by a follow-up
> > patch indeed, but the handling of nodes with no child devices needs to
> > be correct (or you could end up breaking normal serial-port support).
>
> that sounds good to me as well. Lets get this feature into the ACPI
> tree since I am going to push Hans’ patches into bluetooth-next as
> soon as he re-submits the missing ID change. That hopefully gives us
> then fully serdev support for ACPI and DT when it comes to Broadcom
> Bluetooth controllers.
But with Hans's work these devices should continue to function using the
platform-device hacks until the ACPI patch eventually lands in Linus's
tree (via the acpi tree).
> Next step is to convert the Intel driver to also use ACPI based serdev
> :)
>
> And for the brave, I think there are Realtek based systems using ACPI
> as well.
>
> What I was wondering the other day is if we need a lsserdev tool or
> some integration in lshw to be able to debug what serdev devices and
> ID are present. The lsusb and /sys/kernel/debug/usb/devices is just
> super powerful and easy when it comes to figuring out what people have
> in their system. Maybe /sys/kernel/debug/serdev/devices could be
> helpful as well. Just thinking out loud here.
Yeah, maybe. Since you'd typically only have small number of serdev
devices (say, max 4), using /sys/bus/serial/devices directly should not
be too bad meanwhile. Not that much common information we can expose
either, at least not in comparison to USB. But I'll keep it mind. :)
Johan
^ permalink raw reply
* [PATCH] serdev: fix registration of second slave
From: Johan Hovold @ 2017-10-10 16:09 UTC (permalink / raw)
To: Rob Herring
Cc: Greg Kroah-Hartman, linux-serial, linux-kernel,
Frédéric Danis, Johan Hovold, stable
Serdev currently only supports a single slave device, but the required
sanity checks to prevent further registration attempts were missing.
If a serial-port node has two child nodes with compatible properties,
the OF code would try to register two slave devices using the same id
and name. Driver core will not allow this (and there will be loud
complaints), but the controller's slave pointer would already have been
set to address of the soon to be deallocated second struct
serdev_device. As the first slave device remains registered, this can
lead to later use-after-free issues when the slave callbacks are
accessed.
Note that while the serdev registration helpers are exported, they are
typically only called by serdev core. Any other (out-of-tree) callers
must serialise registration and deregistration themselves.
Fixes: cd6484e1830b ("serdev: Introduce new bus for serial attached devices")
Cc: stable <stable@vger.kernel.org> # 4.11
Signed-off-by: Johan Hovold <johan@kernel.org>
---
drivers/tty/serdev/core.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c68fb3a8ea1c..97db76afced2 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -65,21 +65,32 @@ static int serdev_uevent(struct device *dev, struct kobj_uevent_env *env)
*/
int serdev_device_add(struct serdev_device *serdev)
{
+ struct serdev_controller *ctrl = serdev->ctrl;
struct device *parent = serdev->dev.parent;
int err;
dev_set_name(&serdev->dev, "%s-%d", dev_name(parent), serdev->nr);
+ /* Only a single slave device is currently supported. */
+ if (ctrl->serdev) {
+ dev_err(&serdev->dev, "controller busy\n");
+ return -EBUSY;
+ }
+ ctrl->serdev = serdev;
+
err = device_add(&serdev->dev);
if (err < 0) {
dev_err(&serdev->dev, "Can't add %s, status %d\n",
dev_name(&serdev->dev), err);
- goto err_device_add;
+ goto err_clear_serdev;
}
dev_dbg(&serdev->dev, "device %s registered\n", dev_name(&serdev->dev));
-err_device_add:
+ return 0;
+
+err_clear_serdev:
+ ctrl->serdev = NULL;
return err;
}
EXPORT_SYMBOL_GPL(serdev_device_add);
@@ -90,7 +101,10 @@ EXPORT_SYMBOL_GPL(serdev_device_add);
*/
void serdev_device_remove(struct serdev_device *serdev)
{
+ struct serdev_controller *ctrl = serdev->ctrl;
+
device_unregister(&serdev->dev);
+ ctrl->serdev = NULL;
}
EXPORT_SYMBOL_GPL(serdev_device_remove);
@@ -295,7 +309,6 @@ struct serdev_device *serdev_device_alloc(struct serdev_controller *ctrl)
return NULL;
serdev->ctrl = ctrl;
- ctrl->serdev = serdev;
device_initialize(&serdev->dev);
serdev->dev.parent = &ctrl->dev;
serdev->dev.bus = &serdev_bus_type;
--
2.14.2
^ permalink raw reply related
* Re: [PATCH v2 1/2] serdev: Add ACPI support
From: Johan Hovold @ 2017-10-10 15:35 UTC (permalink / raw)
To: Frédéric Danis
Cc: robh, marcel, sre, loic.poulain, johan, lukas, hdegoede, rafael,
greg, linux-bluetooth, linux-serial, linux-acpi
In-Reply-To: <1507630365-26692-2-git-send-email-frederic.danis.oss@gmail.com>
On Tue, Oct 10, 2017 at 12:12:44PM +0200, Frédéric Danis wrote:
> This patch allows SerDev module to manage serial devices declared as
> attached to an UART in ACPI table.
>
> acpi_serdev_add_device() callback will only take into account entries
> without enumerated flag set. This flags is set for all entries during
> ACPI scan, except for SPI and I2C serial devices, and for UART with
> 2nd patch in the series.
>
> Check if a serdev device as been allocated during acpi_walk_namespace()
> to prevent serdev controller registration instead of the tty-class device.
Good that we caught that one before every ACPI serial port broke. ;)
> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> drivers/tty/serdev/core.c | 106 +++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 99 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> @@ -385,6 +401,78 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
> return 0;
> }
>
> +#ifdef CONFIG_ACPI
> +static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
> + struct acpi_device *adev)
> +{
> + struct serdev_device *serdev = NULL;
> + int err;
> +
> + if (acpi_bus_get_status(adev) || !adev->status.present ||
> + acpi_device_enumerated(adev))
> + return AE_OK;
> +
> + serdev = serdev_device_alloc(ctrl);
> + if (!serdev) {
> + dev_err(&ctrl->dev, "failed to allocate serdev device for %s\n",
> + dev_name(&adev->dev));
> + return AE_NO_MEMORY;
> + }
> +
> + ACPI_COMPANION_SET(&serdev->dev, adev);
> + acpi_device_set_enumerated(adev);
Looks like this flag needs to be cleared when the device is later
deregistered or you will not be able to reprobe the device (mostly
useful for development). Can probably done in a follow-up patch.
> +
> + err = serdev_device_add(serdev);
> + if (err) {
> + dev_err(&serdev->dev,
> + "failure adding ACPI serdev device. status %d\n", err);
> + serdev_device_put(serdev);
> + ctrl->serdev = NULL;
You should not clear this pointer here. This should be taken care of by
serdev core when registration fails, something which would also address
the problem with how to deal with multiple child nodes which is a
problem for ACPI as well as DT (as I mentioned briefly at Kernel
Recipes).
I'll submit a fix for that shortly, but the NULL assignment above needs
to go.
> + }
> +
> + return AE_OK;
> +}
> +
> +static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
> + void *data, void **return_value)
> +{
> + struct serdev_controller *ctrl = data;
> + struct acpi_device *adev;
> +
> + if (acpi_bus_get_device(handle, &adev))
> + return AE_OK;
> +
> + return acpi_serdev_register_device(ctrl, adev);
> +}
> +
> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
> +{
> + acpi_status status;
> + acpi_handle handle;
> +
> + handle = ACPI_HANDLE(ctrl->dev.parent);
> + if (!handle)
> + return -ENODEV;
> +
> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> + acpi_serdev_add_device, NULL, ctrl, NULL);
> + if (ACPI_FAILURE(status)) {
> + dev_dbg(&ctrl->dev, "failed to enumerate serdev slaves\n");
So what if there are more than one child node defined? You could end up
with an error here but with a slave still registered. And then when you
return -ENODEV, that memory will leak.
> + return -ENODEV;
Simply falling through here, and then return 0 or -ENODEV based on
ctrl->serdev should suffice for now.
> + }
> +
> + if (!ctrl->serdev)
> + return -ENODEV;
> +
> + return 0;
> +}
> +#else
> +static inline int acpi_serdev_register_devices(struct serdev_controller *ctrl)
> +{
> + return -ENODEV;
> +}
> +#endif /* CONFIG_ACPI */
> +
> /**
> * serdev_controller_add() - Add an serdev controller
> * @ctrl: controller to be registered.
> @@ -394,7 +482,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
> */
> int serdev_controller_add(struct serdev_controller *ctrl)
> {
> - int ret;
> + int ret_of, ret_acpi, ret;
>
> /* Can't register until after driver model init */
> if (WARN_ON(!is_registered))
> @@ -404,12 +492,16 @@ int serdev_controller_add(struct serdev_controller *ctrl)
> if (ret)
> return ret;
>
> - ret = of_serdev_register_devices(ctrl);
> - if (ret)
> + ret_of = of_serdev_register_devices(ctrl);
> + ret_acpi = acpi_serdev_register_devices(ctrl);
> + if (ret_of && ret_acpi) {
> + dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d\n",
> + ret_of, ret_acpi);
> + ret = -ENODEV;
> goto out_dev_del;
> + }
>
> - dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
> - ctrl->nr, &ctrl->dev);
> + dev_dbg(&ctrl->dev, "registered: dev:%p\n", &ctrl->dev);
This change belongs in a separate patch. There seem to be a few more
instances like this one, which can all be fixed in a later patch
instead.
> return 0;
>
> out_dev_del:
Johan
^ permalink raw reply
* Re: [PATCH v2 1/2] serdev: Add ACPI support
From: Ian W MORRISON @ 2017-10-10 15:16 UTC (permalink / raw)
To: Frédéric Danis
Cc: Rob Herring, Marcel Holtmann, Sebastian Reichel, Loic Poulain,
Johan Hovold, Lukas Wunner, Hans de Goede, Rafael J. Wysocki,
Greg KH, bluez mailin list (linux-bluetooth@vger.kernel.org),
linux-serial, linux-acpi
In-Reply-To: <1507630365-26692-2-git-send-email-frederic.danis.oss@gmail.com>
On 10 October 2017 at 21:12, Frédéric Danis
<frederic.danis.oss@gmail.com> wrote:
> This patch allows SerDev module to manage serial devices declared as
> attached to an UART in ACPI table.
>
> acpi_serdev_add_device() callback will only take into account entries
> without enumerated flag set. This flags is set for all entries during
> ACPI scan, except for SPI and I2C serial devices, and for UART with
> 2nd patch in the series.
>
> Check if a serdev device as been allocated during acpi_walk_namespace()
> to prevent serdev controller registration instead of the tty-class device.
<snip>
Hi Fred,
I tested the v2 patch and can confirm it works:
Before v1 patch:
$ dmesg | grep tty
[ 0.000000] console [tty0] enabled
[ 2.409003] 00:01: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200)
is a 16550A
[ 5.380336] 8086228A:00: ttyS4 at MMIO 0x9151b000 (irq = 39,
base_baud = 2764800) is a 16550A
[ 5.388412] 8086228A:01: ttyS5 at MMIO 0x91519000 (irq = 40,
base_baud = 2764800) is a 16550A
$
After v1 patch:
$ dmesg | grep tty
[ 0.000000] console [tty0] enabled
[ 2.416973] 00:01: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200)
is a 16550A
[ 2.417317] serial serial0: tty port ttyS0 registered
[ 5.928492] 8086228A:00: ttyS4 at MMIO 0x9151b000 (irq = 39,
base_baud = 2764800) is a 16550A
[ 5.982195] serial serial1: tty port ttyS4 registered
[ 6.004426] 8086228A:01: ttyS5 at MMIO 0x91519000 (irq = 40,
base_baud = 2764800) is a 16550A
[ 6.019369] serial serial2: tty port ttyS5 registered
$
After v2 patch:
$ dmesg | grep tty
[ 0.000000] console [tty0] enabled
[ 2.420295] 00:01: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200)
is a 16550A
[ 5.884777] 8086228A:00: ttyS4 at MMIO 0x9151b000 (irq = 39,
base_baud = 2764800) is a 16550A
[ 5.917980] serial serial0: tty port ttyS4 registered
[ 5.933160] 8086228A:01: ttyS5 at MMIO 0x91519000 (irq = 40,
base_baud = 2764800) is a 16550A
$
Regards,
Ian
^ permalink raw reply
* [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Han, Nandor (GE Healthcare) @ 2017-10-10 10:17 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Greg Kroah-Hartman, Jiri Slaby,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Martyn Welch
In-Reply-To: <20171010082556.q47bahujdlffxj5e-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> Sent: 10 October 2017 11:26
> To: Han, Nandor (GE Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>
> Cc: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>; Greg Kroah-Hartman
> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby <jslaby-IBi9RG/b67k@public.gmane.org>; linux-
> serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer configuration
> to DT
>
> Hello,
>
> On Tue, Oct 10, 2017 at 07:58:31AM +0000, Han, Nandor (GE Healthcare)
> wrote:
> > > > > That doesn't sound like a good fix but more like a work around.
> > > > > Which other options did you test to fix your problem?
> > > > >
> > > >
> > > > I haven't tried any other, because except using maybe, ioctl I
> > > > haven't got anything better.
> > >
> > > My question didn't target where to configure the buffer size instead
> > > of dts. I wonder if it would help to change the fifo watermark limits for
> example.
> > >
> >
> > Ok. Sorry I didn't understand correct. Watermark will not help in this
> > case because it is used only to trigger the moving of the data from RX
> > FIFO to DMA buffer. The iMX DMA (check the iMX6 RM A.3.1.2.4 since is
> > missing from iMX53 RM) will only return data to serial driver when no more
> data exist in the RX FIFO (is using aging timer for this check) or BD (DMA
> buffer) is full. If data is sent continuously DMA will return data to driver only
> when BD is full.
>
> I read once over the description but I failed to understand it. At least it talks
> about water mark levels and min(WML-1,Count) which makes me think that
> the DMA script makes use of water marking too and my idea to play with that
> instead of buffer sizes might be sensible.
>
:). It is tricky, I agree. I will try to explain it even though I think the most important
part is to understand that watermark level control when data is moved from out from rx FIFO
to DMA buffer (BD) and is not the trigger that returns the data from DMA buffer (BD) to serial driver
(which is the one that I need).
The SDMA script is using 2 triggers to move data from rx FIFO to DMA buffer (BD): 1) watermark hit and 2) aging timer. It ignores completely the "Idle line" trigger. When watermark trigger is received it will move "min(WML-1,BD count)" data from rx FIFO to BD. After this step in the rx FIFO will remain always at least 1 byte (since we move WML-1). The last byte will be moved based on the aging timer. During which SDMA will check if more data is available and move the rest. SDMA will return the BD in 2 situations: 1) BD is full 2) during aging time trigger the rx FIFO is empty.
> > So what we need here is a trigger that will force DMA to return data
> > faster to serial driver. The only one that I could find was the size
> > of the buffer. Of course another way will be to create different SDMA
> > scripts that can do all kinds of handling -- but dint' want to go on
> > that route :)
>
> :-)
>
> > > > Our problem is that in our system some serial ports needs to have
> > > > really low data latency, where others trade more bytes over data
> > > > latency. This situation results in a need of beeing able to have
> > > > different DMA buffer size for different ports.
> > > >
> > > > How can DMA buffer size affect latency?
> > > > DMA works like this: (To answer to your question DMA buffer is not
> > > > FIFO) 1. Transfer the data from HW FIFO to DMA buffer based on
> > > > some interrupts (character received, etc) 2. Transfer the DMA
> > > > buffer back to serial port based on some events (buffer full,
> > > > aging timer, etc) 3. Serial
> > > port forwards to tty buffer.
> > >
> > > BTW In the past I saw the serial core introduce latency, too. Are
> > > you sure that's not your bottle neck?
> >
> > Can you be more specific?
>
> http://www.spinics.net/lists/linux-serial/msg17767.html
>
> > > > Data availability to consumer depends on: DMA buffer size, baud
> > > > rate and communication pattern. By communication patter I'm
> > > > refering that we send data continuoselly (serial line is never
> > > > idle) or packet by packet (serial line is idle in between)
> > > > Example:
> > > > Baud: 19200 (1Byte = 0.52 ms)
> > > > DMA buffer size: 100 bytes
> > > > Communication pattern: continuously
> > > > => DMA will return data to serial port only when DMA buffer is
> > > > full, since the communication is continuously. This result in a
> > > > data latency of 0.52 ms* 100bytes = 52ms. In case the buffer
> > > > will be 200bytes the letency will be double.
> > > >
> > > > I agree with you, this is not directly a hw property but a DMA
> > > > configuration
> > > item.
> > > > But I've found this to be the best way to configure this comparing
> > > > with using
> > > ioctl.
> > > >
> > > > Let me know if you need more clarification and I would really be
> > > > open to other options that will solve our problem.
> > > >
> > > > <snip>
> > > >
> > > > > > +- fsl,dma-size : Indicate the size of the DMA buffer and its
> > > > > > +periods
> > > > >
> > > > > This is a sparse description, just from reading that I don't
> > > > > understand what it does.
> > > > >
> > > >
> > > > Serial driver configures a circular ring of buffers for DMA. Here
> > > > we can configure the size and the number of buffers.
> > >
> > > The problem is: How should a person, who wants to make available a
> > > port on a machine via dts, choose what value to use for fsl,dma-size?
> >
> > It doesn't have too. If this configuration is not provided the serial port will
> have a default value.
> > The user has the possibility to tweak this settings in case is needed.
>
> "The default value works in general" is no good justification to not provide
> easy-to-grasp and well documented knobs.
>
I agree with you on this. But my intention was to say that the driver is using the same settings as before
and users are not impacted by this change, and I only add the option to control that value.
> Best regards
> Uwe
>
Regards,
Nandor
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2 2/2] ACPI / scan: Fix enumeration for special UART devices
From: Frédéric Danis @ 2017-10-10 10:12 UTC (permalink / raw)
To: robh, marcel, sre, loic.poulain, johan, lukas, hdegoede, rafael,
greg
Cc: linux-bluetooth, linux-serial, linux-acpi, frederic.danis.oss
In-Reply-To: <1507630365-26692-1-git-send-email-frederic.danis.oss@gmail.com>
UART devices is expected to be enumerated by SerDev subsystem.
During ACPI scan, serial devices behind SPI, I2C or UART buses are not
enumerated, allowing them to be enumerated by their respective parents.
Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
devices on serial buses (SPI, I2C or UART).
On Macs an empty ResourceTemplate is returned for uart slaves.
Instead the device properties "baud", "parity", "dataBits", "stopBits" are
provided. Add a check for "baud" in acpi_is_serial_bus_slave().
Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
drivers/acpi/scan.c | 37 +++++++++++++++++--------------------
include/acpi/acpi_bus.h | 2 +-
2 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 602f8ff..860b698 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1505,41 +1505,38 @@ static void acpi_init_coherency(struct acpi_device *adev)
adev->flags.coherent_dma = cca;
}
-static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
+static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
{
- bool *is_spi_i2c_slave_p = data;
+ bool *is_serial_bus_slave_p = data;
if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
return 1;
- /*
- * devices that are connected to UART still need to be enumerated to
- * platform bus
- */
- if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)
- *is_spi_i2c_slave_p = true;
+ *is_serial_bus_slave_p = true;
/* no need to do more checking */
return -1;
}
-static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
+static bool acpi_is_serial_bus_slave(struct acpi_device *device)
{
struct list_head resource_list;
- bool is_spi_i2c_slave = false;
+ bool is_serial_bus_slave = false;
/* Macs use device properties in lieu of _CRS resources */
if (x86_apple_machine &&
(fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
- fwnode_property_present(&device->fwnode, "i2cAddress")))
+ fwnode_property_present(&device->fwnode, "i2cAddress") ||
+ fwnode_property_present(&device->fwnode, "baud")))
return true;
INIT_LIST_HEAD(&resource_list);
- acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
- &is_spi_i2c_slave);
+ acpi_dev_get_resources(device, &resource_list,
+ acpi_check_serial_bus_slave,
+ &is_serial_bus_slave);
acpi_dev_free_resource_list(&resource_list);
- return is_spi_i2c_slave;
+ return is_serial_bus_slave;
}
void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
@@ -1557,7 +1554,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
acpi_bus_get_flags(device);
device->flags.match_driver = false;
device->flags.initialized = true;
- device->flags.spi_i2c_slave = acpi_is_spi_i2c_slave(device);
+ device->flags.serial_bus_slave = acpi_is_serial_bus_slave(device);
acpi_device_clear_enumerated(device);
device_initialize(&device->dev);
dev_set_uevent_suppress(&device->dev, true);
@@ -1841,10 +1838,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
static void acpi_default_enumeration(struct acpi_device *device)
{
/*
- * Do not enumerate SPI/I2C slaves as they will be enumerated by their
- * respective parents.
+ * Do not enumerate SPI/I2C/UART slaves as they will be enumerated by
+ * their respective parents.
*/
- if (!device->flags.spi_i2c_slave) {
+ if (!device->flags.serial_bus_slave) {
acpi_create_platform_device(device, NULL);
acpi_device_set_enumerated(device);
} else {
@@ -1941,7 +1938,7 @@ static void acpi_bus_attach(struct acpi_device *device)
return;
device->flags.match_driver = true;
- if (ret > 0 && !device->flags.spi_i2c_slave) {
+ if (ret > 0 && !device->flags.serial_bus_slave) {
acpi_device_set_enumerated(device);
goto ok;
}
@@ -1950,7 +1947,7 @@ static void acpi_bus_attach(struct acpi_device *device)
if (ret < 0)
return;
- if (!device->pnp.type.platform_id && !device->flags.spi_i2c_slave)
+ if (!device->pnp.type.platform_id && !device->flags.serial_bus_slave)
acpi_device_set_enumerated(device);
else
acpi_default_enumeration(device);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index fa15052..f849be2 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -211,7 +211,7 @@ struct acpi_device_flags {
u32 of_compatible_ok:1;
u32 coherent_dma:1;
u32 cca_seen:1;
- u32 spi_i2c_slave:1;
+ u32 serial_bus_slave:1;
u32 reserved:19;
};
--
2.7.4
^ permalink raw reply related
* [PATCH v2 1/2] serdev: Add ACPI support
From: Frédéric Danis @ 2017-10-10 10:12 UTC (permalink / raw)
To: robh, marcel, sre, loic.poulain, johan, lukas, hdegoede, rafael,
greg
Cc: linux-bluetooth, linux-serial, linux-acpi, frederic.danis.oss
In-Reply-To: <1507630365-26692-1-git-send-email-frederic.danis.oss@gmail.com>
This patch allows SerDev module to manage serial devices declared as
attached to an UART in ACPI table.
acpi_serdev_add_device() callback will only take into account entries
without enumerated flag set. This flags is set for all entries during
ACPI scan, except for SPI and I2C serial devices, and for UART with
2nd patch in the series.
Check if a serdev device as been allocated during acpi_walk_namespace()
to prevent serdev controller registration instead of the tty-class device.
Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/tty/serdev/core.c | 106 +++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 99 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c68fb3a..7e00e2e 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -14,6 +14,7 @@
* GNU General Public License for more details.
*/
+#include <linux/acpi.h>
#include <linux/errno.h>
#include <linux/idr.h>
#include <linux/kernel.h>
@@ -49,13 +50,22 @@ static const struct device_type serdev_ctrl_type = {
static int serdev_device_match(struct device *dev, struct device_driver *drv)
{
- /* TODO: ACPI and platform matching */
+ /* TODO: platform matching */
+ if (acpi_driver_match_device(dev, drv))
+ return 1;
+
return of_driver_match_device(dev, drv);
}
static int serdev_uevent(struct device *dev, struct kobj_uevent_env *env)
{
- /* TODO: ACPI and platform modalias */
+ int rc;
+
+ /* TODO: platform modalias */
+ rc = acpi_device_uevent_modalias(dev, env);
+ if (rc != -ENODEV)
+ return rc;
+
return of_device_uevent_modalias(dev, env);
}
@@ -260,6 +270,12 @@ static int serdev_drv_remove(struct device *dev)
static ssize_t modalias_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
+ int len;
+
+ len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
+ if (len != -ENODEV)
+ return len;
+
return of_device_modalias(dev, buf, PAGE_SIZE);
}
DEVICE_ATTR_RO(modalias);
@@ -385,6 +401,78 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
return 0;
}
+#ifdef CONFIG_ACPI
+static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
+ struct acpi_device *adev)
+{
+ struct serdev_device *serdev = NULL;
+ int err;
+
+ if (acpi_bus_get_status(adev) || !adev->status.present ||
+ acpi_device_enumerated(adev))
+ return AE_OK;
+
+ serdev = serdev_device_alloc(ctrl);
+ if (!serdev) {
+ dev_err(&ctrl->dev, "failed to allocate serdev device for %s\n",
+ dev_name(&adev->dev));
+ return AE_NO_MEMORY;
+ }
+
+ ACPI_COMPANION_SET(&serdev->dev, adev);
+ acpi_device_set_enumerated(adev);
+
+ err = serdev_device_add(serdev);
+ if (err) {
+ dev_err(&serdev->dev,
+ "failure adding ACPI serdev device. status %d\n", err);
+ serdev_device_put(serdev);
+ ctrl->serdev = NULL;
+ }
+
+ return AE_OK;
+}
+
+static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
+ void *data, void **return_value)
+{
+ struct serdev_controller *ctrl = data;
+ struct acpi_device *adev;
+
+ if (acpi_bus_get_device(handle, &adev))
+ return AE_OK;
+
+ return acpi_serdev_register_device(ctrl, adev);
+}
+
+static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+ acpi_status status;
+ acpi_handle handle;
+
+ handle = ACPI_HANDLE(ctrl->dev.parent);
+ if (!handle)
+ return -ENODEV;
+
+ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+ acpi_serdev_add_device, NULL, ctrl, NULL);
+ if (ACPI_FAILURE(status)) {
+ dev_dbg(&ctrl->dev, "failed to enumerate serdev slaves\n");
+ return -ENODEV;
+ }
+
+ if (!ctrl->serdev)
+ return -ENODEV;
+
+ return 0;
+}
+#else
+static inline int acpi_serdev_register_devices(struct serdev_controller *ctrl)
+{
+ return -ENODEV;
+}
+#endif /* CONFIG_ACPI */
+
/**
* serdev_controller_add() - Add an serdev controller
* @ctrl: controller to be registered.
@@ -394,7 +482,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
*/
int serdev_controller_add(struct serdev_controller *ctrl)
{
- int ret;
+ int ret_of, ret_acpi, ret;
/* Can't register until after driver model init */
if (WARN_ON(!is_registered))
@@ -404,12 +492,16 @@ int serdev_controller_add(struct serdev_controller *ctrl)
if (ret)
return ret;
- ret = of_serdev_register_devices(ctrl);
- if (ret)
+ ret_of = of_serdev_register_devices(ctrl);
+ ret_acpi = acpi_serdev_register_devices(ctrl);
+ if (ret_of && ret_acpi) {
+ dev_dbg(&ctrl->dev, "no devices registered: of:%d acpi:%d\n",
+ ret_of, ret_acpi);
+ ret = -ENODEV;
goto out_dev_del;
+ }
- dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
- ctrl->nr, &ctrl->dev);
+ dev_dbg(&ctrl->dev, "registered: dev:%p\n", &ctrl->dev);
return 0;
out_dev_del:
--
2.7.4
^ permalink raw reply related
* [PATCH v2 0/2] ACPI serdev support
From: Frédéric Danis @ 2017-10-10 10:12 UTC (permalink / raw)
To: robh-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
sre-DgEjT+Ai2ygdnm+yROfE0A, loic.poulain-Re5JQEeQqe8AvxtiuMwx3w,
johan-DgEjT+Ai2ygdnm+yROfE0A, lukas-JFq808J9C/izQB+pC5nmwQ,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
greg-U8xfFu+wG4EAvxtiuMwx3w
Cc: linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w
Add ACPI support for serial attached devices.
Currently, serial devices are not set as enumerated during ACPI scan for SPI
or i2c buses (but not for UART). This should also be done for UART serial
devices.
I renamed *spi_i2c_slave* to *serial_bus_slave* to reflect this.
Tested on T100TA with Broadcom BCM2E39.
Since v1:
- Check if a serdev device as been allocated during acpi_walk_namespace() to
prevent serdev controller registration instead of the tty-class device.
- Reword dev_dbg() strings replacing Serial by serdev
- Removing redundant "serdev%d" in dev_dbg() calls in serdev_controller_add()
Since RFC:
- Add or reword commit messages
- Rename *serial_slave* to *serial_bus_slave*
- Add specific check for Apple in acpi_is_serial_bus_slave(), thanks to
Lukas Wunner
- Update comment in acpi_default_enumeration()
- Remove patch 3 "Bluetooth: hci_bcm: Add ACPI serdev support for BCM2E39"
in favor of patches from Hans de Goede
Frédéric Danis (2):
serdev: Add ACPI support
ACPI / scan: Fix enumeration for special UART devices
drivers/acpi/scan.c | 37 ++++++++--------
drivers/tty/serdev/core.c | 106 +++++++++++++++++++++++++++++++++++++++++++---
include/acpi/acpi_bus.h | 2 +-
3 files changed, 117 insertions(+), 28 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Uwe Kleine-König @ 2017-10-10 8:25 UTC (permalink / raw)
To: Han, Nandor (GE Healthcare)
Cc: Romain Perier, Greg Kroah-Hartman, Jiri Slaby,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <AM3P101MB0180BF8845D0CCFD9C76C63EE6750-Irc2Ng3OI610aGNDtyle9VAr667LJVwUiGd9ebBGJoev3QGu/rdwKA@public.gmane.org>
Hello,
On Tue, Oct 10, 2017 at 07:58:31AM +0000, Han, Nandor (GE Healthcare) wrote:
> > > > That doesn't sound like a good fix but more like a work around.
> > > > Which other options did you test to fix your problem?
> > > >
> > >
> > > I haven't tried any other, because except using maybe, ioctl I haven't
> > > got anything better.
> >
> > My question didn't target where to configure the buffer size instead of dts. I
> > wonder if it would help to change the fifo watermark limits for example.
> >
>
> Ok. Sorry I didn't understand correct. Watermark will not help in this case because it is used only to trigger
> the moving of the data from RX FIFO to DMA buffer. The iMX DMA (check the iMX6 RM A.3.1.2.4 since is missing
> from iMX53 RM) will only return data to serial driver when no more data exist in the RX FIFO (is using aging timer for this check)
> or BD (DMA buffer) is full. If data is sent continuously DMA will return data to driver only when BD is full.
I read once over the description but I failed to understand it. At least
it talks about water mark levels and min(WML-1,Count) which makes me
think that the DMA script makes use of water marking too and my idea to
play with that instead of buffer sizes might be sensible.
> So what we need here is a trigger that will force DMA to return data
> faster to serial driver. The only one that I could find was the size
> of the buffer. Of course another way will be to create different SDMA
> scripts that can do all kinds of handling -- but dint' want to go on
> that route :)
:-)
> > > Our problem is that in our system some serial ports needs to have
> > > really low data latency, where others trade more bytes over data
> > > latency. This situation results in a need of beeing able to have
> > > different DMA buffer size for different ports.
> > >
> > > How can DMA buffer size affect latency?
> > > DMA works like this: (To answer to your question DMA buffer is not
> > > FIFO) 1. Transfer the data from HW FIFO to DMA buffer based on some
> > > interrupts (character received, etc) 2. Transfer the DMA buffer back
> > > to serial port based on some events (buffer full, aging timer, etc) 3. Serial
> > port forwards to tty buffer.
> >
> > BTW In the past I saw the serial core introduce latency, too. Are you sure
> > that's not your bottle neck?
>
> Can you be more specific?
http://www.spinics.net/lists/linux-serial/msg17767.html
> > > Data availability to consumer depends on: DMA buffer size, baud rate
> > > and communication pattern. By communication patter I'm refering that
> > > we send data continuoselly (serial line is never idle) or packet by
> > > packet (serial line is idle in between)
> > > Example:
> > > Baud: 19200 (1Byte = 0.52 ms)
> > > DMA buffer size: 100 bytes
> > > Communication pattern: continuously
> > > => DMA will return data to serial port only when DMA buffer is
> > > full, since the communication is continuously. This result in a
> > > data latency of 0.52 ms* 100bytes = 52ms. In case the buffer
> > > will be 200bytes the letency will be double.
> > >
> > > I agree with you, this is not directly a hw property but a DMA configuration
> > item.
> > > But I've found this to be the best way to configure this comparing with using
> > ioctl.
> > >
> > > Let me know if you need more clarification and I would really be open
> > > to other options that will solve our problem.
> > >
> > > <snip>
> > >
> > > > > +- fsl,dma-size : Indicate the size of the DMA buffer and its
> > > > > +periods
> > > >
> > > > This is a sparse description, just from reading that I don't
> > > > understand what it does.
> > > >
> > >
> > > Serial driver configures a circular ring of buffers for DMA. Here we
> > > can configure the size and the number of buffers.
> >
> > The problem is: How should a person, who wants to make available a port on a
> > machine via dts, choose what value to use for fsl,dma-size?
>
> It doesn't have too. If this configuration is not provided the serial port will have a default value.
> The user has the possibility to tweak this settings in case is needed.
"The default value works in general" is no good justification to not
provide easy-to-grasp and well documented knobs.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] serdev: Add ACPI support
From: Marcel Holtmann @ 2017-10-10 8:22 UTC (permalink / raw)
To: Johan Hovold
Cc: Frédéric Danis, robh, sre, loic.poulain, lukas,
hdegoede, linux-bluetooth, linux-serial, linux-acpi
In-Reply-To: <20171010081528.GJ4269@localhost>
Hi Johan,
>>>> This patch allows SerDev module to manage serial devices declared as
>>>> attached to an UART in ACPI table.
>>>>
>>>> acpi_serdev_add_device() callback will only take into account entries
>>>> without enumerated flag set. This flags is set for all entries during
>>>> ACPI scan, except for SPI and I2C serial devices, and for UART with
>>>> 2nd patch in the series.
>>>>
>>>> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
>>>> ---
>>>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>>>> index c68fb3a..104777d 100644
>>>> --- a/drivers/tty/serdev/core.c
>>>> +++ b/drivers/tty/serdev/core.c
>
>>>> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
>>>> +{
>>>> + acpi_status status;
>>>> + acpi_handle handle;
>>>> +
>>>> + handle = ACPI_HANDLE(ctrl->dev.parent);
>>>> + if (!handle)
>>>> + return -ENODEV;
>>>> +
>>>> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
>>>> + acpi_serdev_add_device, NULL, ctrl, NULL);
>>>> + if (ACPI_FAILURE(status)) {
>>>> + dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
>>>
>>> s/Serial/serdev/
>>>
>>>> + return -ENODEV;
>>>> + }
>>>> +
>>>> + return 0;
>>>
>>> What if there are no slaves defined? I'm not very familiar with the ACPI
>>> helpers, but from a quick look it seems you'd then end up returning zero
>>> here which would cause the serdev controller to be registered instead of
>>> the tty-class device.
>>>
>>> [ And if I'm mistaken, you do want to suppress that error message for
>>> when there are no slaves defined. ]
>
>>>> - ret = of_serdev_register_devices(ctrl);
>>>> - if (ret)
>>>> + ret_of = of_serdev_register_devices(ctrl);
>>>> + ret_acpi = acpi_serdev_register_devices(ctrl);
>>>> + if (ret_of && ret_acpi) {
>>>> + dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
>>>
>>> "serdev%d" is redundant here as you're using dev_dbg (which will print
>>> the device name).
>>>
>>>> + ctrl->nr, ret_of, ret_acpi);
>>>> + ret = -ENODEV;
>>>> goto out_dev_del;
>>>> + }
>>>>
>>>> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>>>> ctrl->nr, &ctrl->dev);
>>>
>>> Hmm, I see it's already used here. No need to follow that example
>>> though.
>>
>> lets have an extra patch on top of it that fixes these.
>
> Some of the above are minor nits that can be addressed by a follow-up
> patch indeed, but the handling of nodes with no child devices needs to
> be correct (or you could end up breaking normal serial-port support).
that sounds good to me as well. Lets get this feature into the ACPI tree since I am going to push Hans’ patches into bluetooth-next as soon as he re-submits the missing ID change. That hopefully gives us then fully serdev support for ACPI and DT when it comes to Broadcom Bluetooth controllers.
Next step is to convert the Intel driver to also use ACPI based serdev :)
And for the brave, I think there are Realtek based systems using ACPI as well.
What I was wondering the other day is if we need a lsserdev tool or some integration in lshw to be able to debug what serdev devices and ID are present. The lsusb and /sys/kernel/debug/usb/devices is just super powerful and easy when it comes to figuring out what people have in their system. Maybe /sys/kernel/debug/serdev/devices could be helpful as well. Just thinking out loud here.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Revert "Bluetooth: btusb: Add workaround for Broadcom devices without product id"
From: Marcel Holtmann @ 2017-10-10 8:16 UTC (permalink / raw)
To: Hans de Goede
Cc: Gustavo F. Padovan, Johan Hedberg, Frédéric Danis,
Sebastian Reichel, robh, linux-bluetooth, linux-serial,
linux-acpi
In-Reply-To: <20171005074251.7274-1-hdegoede@redhat.com>
Hi Hans,
> Commit 9834e586fa ("Bluetooth: btusb: Add workaround for Broadcom devices
> without product id") was added to deal with the BT part of the BCM4356A2
> on GPD pocket laptops having an usb vid:pid of 0000:0000.
>
> After another commit to add support for the BCM UART connected BT ACPI-id
> BCM2E7E used on the GPD win, it turns out that the BT on the GPD pocket is
> connected via both USB and UART. Adding support for the BCM2E7E ACPI-id
> causes it to switch to UART mode.
>
> The Windows shipped with the device is using it in UART mode and the
> presence of the BCM2E7E ACPI-id combined with the 0 USB vid:pid also
> indicates that the BT part was never meant to be used in USB mode.
>
> But not only is there no need to support the USB mode, supporting it
> actually is troublesome, because with the workaround to support the all 0
> USB vid:pid, BT will just work for end users, until we add support for
> the ACPI-id, after which users need to have userspace doing a btattach,
> which may be considered a regression.
>
> To avoid this regression when switching things over to UART mode as
> intended by the ACPI tables all along, this commit reverts the commit
> adding the workaround to support the all 0 USB vid:pid.
>
> This reverts commit 9834e586fa ("Bluetooth: btusb: Add workaround for
> Broadcom devices without product id").
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/bluetooth/btusb.c | 18 ------------------
> 1 file changed, 18 deletions(-)
can you send a series of patches, that first reverts this patch and then a patch adds the BCM2E7E id to the hci_bcm driver so that it will start working again.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/2] serdev: Add ACPI support
From: Johan Hovold @ 2017-10-10 8:15 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hovold, Frédéric Danis, robh, sre, loic.poulain,
lukas, hdegoede, linux-bluetooth, linux-serial, linux-acpi
In-Reply-To: <BC1C29B8-C76B-499D-B8BE-365B5D3A0065@holtmann.org>
On Tue, Oct 10, 2017 at 10:10:58AM +0200, Marcel Holtmann wrote:
> Hi Johan,
>
> >> This patch allows SerDev module to manage serial devices declared as
> >> attached to an UART in ACPI table.
> >>
> >> acpi_serdev_add_device() callback will only take into account entries
> >> without enumerated flag set. This flags is set for all entries during
> >> ACPI scan, except for SPI and I2C serial devices, and for UART with
> >> 2nd patch in the series.
> >>
> >> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
> >> ---
> >> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
> >> 1 file changed, 94 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> >> index c68fb3a..104777d 100644
> >> --- a/drivers/tty/serdev/core.c
> >> +++ b/drivers/tty/serdev/core.c
> >> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
> >> +{
> >> + acpi_status status;
> >> + acpi_handle handle;
> >> +
> >> + handle = ACPI_HANDLE(ctrl->dev.parent);
> >> + if (!handle)
> >> + return -ENODEV;
> >> +
> >> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> >> + acpi_serdev_add_device, NULL, ctrl, NULL);
> >> + if (ACPI_FAILURE(status)) {
> >> + dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
> >
> > s/Serial/serdev/
> >
> >> + return -ENODEV;
> >> + }
> >> +
> >> + return 0;
> >
> > What if there are no slaves defined? I'm not very familiar with the ACPI
> > helpers, but from a quick look it seems you'd then end up returning zero
> > here which would cause the serdev controller to be registered instead of
> > the tty-class device.
> >
> > [ And if I'm mistaken, you do want to suppress that error message for
> > when there are no slaves defined. ]
> >> - ret = of_serdev_register_devices(ctrl);
> >> - if (ret)
> >> + ret_of = of_serdev_register_devices(ctrl);
> >> + ret_acpi = acpi_serdev_register_devices(ctrl);
> >> + if (ret_of && ret_acpi) {
> >> + dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
> >
> > "serdev%d" is redundant here as you're using dev_dbg (which will print
> > the device name).
> >
> >> + ctrl->nr, ret_of, ret_acpi);
> >> + ret = -ENODEV;
> >> goto out_dev_del;
> >> + }
> >>
> >> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
> >> ctrl->nr, &ctrl->dev);
> >
> > Hmm, I see it's already used here. No need to follow that example
> > though.
>
> lets have an extra patch on top of it that fixes these.
Some of the above are minor nits that can be addressed by a follow-up
patch indeed, but the handling of nodes with no child devices needs to
be correct (or you could end up breaking normal serial-port support).
Johan
^ permalink raw reply
* Re: [PATCH v3 2/2] serdev: Update drivers/bluetooth/Kconfig for ACPI serdev support
From: Johan Hovold @ 2017-10-10 8:11 UTC (permalink / raw)
To: Ian W MORRISON
Cc: Johan Hovold, Marcel Holtmann, Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth-u79uwXL29TY76Z2rM5mHXA@public.gmane.org),
Hans de Goede, Frédéric Danis, Rob Herring,
Sebastian Reichel, Loic Poulain, Lukas Wunner,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA, Rafael J. Wysocki,
Greg Kroah-Hartman
In-Reply-To: <CAFXWsS8=t9SfUdWPOSpmHt4-6cgdrVtQChryyzJ74XOQQP3-Ww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Oct 10, 2017 at 06:16:46PM +1100, Ian W MORRISON wrote:
> On 9 October 2017 at 19:06, Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Mon, Oct 09, 2017 at 11:43:31AM +1100, Ian W MORRISON wrote:
> <snip>
> >>
> >> This patch set addresses this by making BT_HCIUART_BCM dependent on
> >> SERIAL_DEV_CTRL_TTYPORT which in turn is dependent on SERIAL_DEV_BUS
> >> and ensures that if SERIAL_DEV_BUS is selected is the code is build it.
> >
> > Ok, so you didn't even bother to write two distinct commit messages for
> > your two patches, and my comments to the first patch apply also here.
> >
> <snip>
> >
> > Johan
>
> Hi Johan,
>
> My experience in submitting patches is still limited and I am very
> much learning so thanks for the helpful comments and apologies for
> trying your patience. My keenness on submitting patches was to show
> that I had tested my suggestions so as to given them credibility
> rather than any attempt to own the patch.
No worries. I can recommend taking a closer look at
Documentation/process/submitting-patches.rst
where some of the process is described.
> What I was trying to do here was to suggested two patches with one
> requiring the other. Rather than keep as a single patch affecting two
> files I separated into two patches attempting to create a patch series
> with a single changelog. This was the wrong approach and I may have
> misled so apologies for that.
So next, time just submit your patches as a series (consider using
git-send-email to get the threading right), where each patch is
self-contained and has a good commit message describing why that change
is needed. It is implicit that a series is to be applied in order and
that therefore later patches in a series can depend on earlier ones.
When a patch series span multiple subsystems, things can gets more
complicated, though...
Johan
^ permalink raw reply
* Re: [PATCH 1/2] serdev: Add ACPI support
From: Marcel Holtmann @ 2017-10-10 8:10 UTC (permalink / raw)
To: Johan Hovold
Cc: Frédéric Danis, robh, sre, loic.poulain, lukas,
hdegoede, linux-bluetooth, linux-serial, linux-acpi
In-Reply-To: <20171007151255.GH2618@localhost>
Hi Johan,
>> This patch allows SerDev module to manage serial devices declared as
>> attached to an UART in ACPI table.
>>
>> acpi_serdev_add_device() callback will only take into account entries
>> without enumerated flag set. This flags is set for all entries during
>> ACPI scan, except for SPI and I2C serial devices, and for UART with
>> 2nd patch in the series.
>>
>> Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
>> ---
>> drivers/tty/serdev/core.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 94 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
>> index c68fb3a..104777d 100644
>> --- a/drivers/tty/serdev/core.c
>> +++ b/drivers/tty/serdev/core.c
>
>> @@ -385,6 +401,74 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_ACPI
>> +static acpi_status acpi_serdev_register_device(struct serdev_controller *ctrl,
>> + struct acpi_device *adev)
>> +{
>> + struct serdev_device *serdev = NULL;
>> + int err;
>> +
>> + if (acpi_bus_get_status(adev) || !adev->status.present ||
>> + acpi_device_enumerated(adev))
>> + return AE_OK;
>> +
>> + serdev = serdev_device_alloc(ctrl);
>> + if (!serdev) {
>> + dev_err(&ctrl->dev, "failed to allocate Serial device for %s\n",
>
> s/Serial/serdev/
>
>> + dev_name(&adev->dev));
>> + return AE_NO_MEMORY;
>> + }
>> +
>> + ACPI_COMPANION_SET(&serdev->dev, adev);
>> + acpi_device_set_enumerated(adev);
>> +
>> + err = serdev_device_add(serdev);
>> + if (err) {
>> + dev_err(&serdev->dev,
>> + "failure adding ACPI device. status %d\n", err);
>
> s/ACPI/ACPI serdev/?
>
>> + serdev_device_put(serdev);
>> + }
>> +
>> + return AE_OK;
>> +}
>> +
>> +static acpi_status acpi_serdev_add_device(acpi_handle handle, u32 level,
>> + void *data, void **return_value)
>> +{
>> + struct serdev_controller *ctrl = data;
>> + struct acpi_device *adev;
>> +
>> + if (acpi_bus_get_device(handle, &adev))
>> + return AE_OK;
>> +
>> + return acpi_serdev_register_device(ctrl, adev);
>> +}
>> +
>> +static int acpi_serdev_register_devices(struct serdev_controller *ctrl)
>> +{
>> + acpi_status status;
>> + acpi_handle handle;
>> +
>> + handle = ACPI_HANDLE(ctrl->dev.parent);
>> + if (!handle)
>> + return -ENODEV;
>> +
>> + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
>> + acpi_serdev_add_device, NULL, ctrl, NULL);
>> + if (ACPI_FAILURE(status)) {
>> + dev_warn(&ctrl->dev, "failed to enumerate Serial slaves\n");
>
> s/Serial/serdev/
>
>> + return -ENODEV;
>> + }
>> +
>> + return 0;
>
> What if there are no slaves defined? I'm not very familiar with the ACPI
> helpers, but from a quick look it seems you'd then end up returning zero
> here which would cause the serdev controller to be registered instead of
> the tty-class device.
>
> [ And if I'm mistaken, you do want to suppress that error message for
> when there are no slaves defined. ]
>
>> +}
>> +#else
>> +static inline int acpi_serdev_register_devices(struct serdev_controller *ctlr)
>
> s/ctlr/ctrl/
>
>> +{
>> + return -ENODEV;
>> +}
>> +#endif /* CONFIG_ACPI */
>> +
>> /**
>> * serdev_controller_add() - Add an serdev controller
>> * @ctrl: controller to be registered.
>> @@ -394,7 +478,7 @@ static int of_serdev_register_devices(struct serdev_controller *ctrl)
>> */
>> int serdev_controller_add(struct serdev_controller *ctrl)
>> {
>> - int ret;
>> + int ret_of, ret_acpi, ret;
>>
>> /* Can't register until after driver model init */
>> if (WARN_ON(!is_registered))
>> @@ -404,9 +488,14 @@ int serdev_controller_add(struct serdev_controller *ctrl)
>> if (ret)
>> return ret;
>>
>> - ret = of_serdev_register_devices(ctrl);
>> - if (ret)
>> + ret_of = of_serdev_register_devices(ctrl);
>> + ret_acpi = acpi_serdev_register_devices(ctrl);
>> + if (ret_of && ret_acpi) {
>> + dev_dbg(&ctrl->dev, "serdev%d no devices registered: of:%d acpi:%d\n",
>
> "serdev%d" is redundant here as you're using dev_dbg (which will print
> the device name).
>
>> + ctrl->nr, ret_of, ret_acpi);
>> + ret = -ENODEV;
>> goto out_dev_del;
>> + }
>>
>> dev_dbg(&ctrl->dev, "serdev%d registered: dev:%p\n",
>> ctrl->nr, &ctrl->dev);
>
> Hmm, I see it's already used here. No need to follow that example
> though.
lets have an extra patch on top of it that fixes these.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Bluetooth: avoid silent hci_bcm ACPI PM regression
From: Marcel Holtmann @ 2017-10-10 8:07 UTC (permalink / raw)
To: Johan Hovold
Cc: Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth@vger.kernel.org),
Sebastian Reichel, Frédéric Danis, Rob Herring,
Loic Poulain, Lukas Wunner, Hans de Goede, linux-serial,
linux-acpi
In-Reply-To: <20171010080152.18444-1-johan@kernel.org>
Hi Johan,
> The hci_bcm platform-device hack which was used to implement
> power management for ACPI devices is being replaced by a
> serial-device-bus implementation.
>
> Unfortunately, when the corresponding change to the ACPI code lands (a
> change that will stop enumerating and registering the serial-device-node
> child as a platform device) PM will break silently unless serdev
> TTY-port controller support has been enabled. Specifically, hciattach
> (btattach) would still succeed, but power management would no longer
> work.
>
> Although this is strictly a runtime dependency, let's make the driver
> depend on SERIAL_DEV_CTRL_TTYPORT, which is the particular serdev
> controller implementation used by the ACPI devices currently managed by
> this driver, to avoid breaking PM without anyone noticing.
>
> Note that the driver already has a (build-time) dependency on the serdev
> bus code.
>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
> drivers/bluetooth/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* [PATCH] Bluetooth: avoid silent hci_bcm ACPI PM regression
From: Johan Hovold @ 2017-10-10 8:01 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo Padovan, Johan Hedberg,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA, Sebastian Reichel,
Frédéric Danis, robh-DgEjT+Ai2ygdnm+yROfE0A,
loic.poulain-Re5JQEeQqe8AvxtiuMwx3w, lukas-JFq808J9C/izQB+pC5nmwQ,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA, Johan Hovold
The hci_bcm platform-device hack which was used to implement
power management for ACPI devices is being replaced by a
serial-device-bus implementation.
Unfortunately, when the corresponding change to the ACPI code lands (a
change that will stop enumerating and registering the serial-device-node
child as a platform device) PM will break silently unless serdev
TTY-port controller support has been enabled. Specifically, hciattach
(btattach) would still succeed, but power management would no longer
work.
Although this is strictly a runtime dependency, let's make the driver
depend on SERIAL_DEV_CTRL_TTYPORT, which is the particular serdev
controller implementation used by the ACPI devices currently managed by
this driver, to avoid breaking PM without anyone noticing.
Note that the driver already has a (build-time) dependency on the serdev
bus code.
Signed-off-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
drivers/bluetooth/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index fae5a74dc737..082e1c7329de 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -169,6 +169,7 @@ config BT_HCIUART_BCM
bool "Broadcom protocol support"
depends on BT_HCIUART
depends on BT_HCIUART_SERDEV
+ depends on (!ACPI || SERIAL_DEV_CTRL_TTYPORT)
select BT_HCIUART_H4
select BT_BCM
help
--
2.14.2
^ permalink raw reply related
* [PATCH] serial: imx-serial - move DMA buffer configuration to DT
From: Han, Nandor (GE Healthcare) @ 2017-10-10 7:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Romain Perier, Greg Kroah-Hartman, Jiri Slaby,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20171004214935.nejsdqmlj4wwyq5v-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> Sent: 05 October 2017 00:50
> To: Han, Nandor (GE Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>
> Cc: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>; Greg Kroah-Hartman
> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby <jslaby-IBi9RG/b67k@public.gmane.org>; linux-
> serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer configuration
> to DT
>
> Hello,
>
> On Mon, Oct 02, 2017 at 01:17:41PM +0000, Han, Nandor (GE Healthcare)
> wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-König [mailto:u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
> > > Sent: 29 June 2017 21:26
> > > To: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > > Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>; Jiri Slaby
> > > <jslaby-IBi9RG/b67k@public.gmane.org>; linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Han, Nandor (GE
> > > Healthcare) <nandor.han-JJi787mZWgc@public.gmane.org>; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > Subject: EXT: Re: [PATCH] serial: imx-serial - move DMA buffer
> > > configuration to DT
> > >
> > > Hello,
> > >
> > > Cc: += devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > >
> > > On Wed, Jun 28, 2017 at 12:15:14PM +0200, Romain Perier wrote:
> > > > From: Nandor Han <nandor.han-JJi787mZWgc@public.gmane.org>
> > > >
> > > > The size of the DMA buffer can affect the delta time between data
> > > > being produced and data being consumed. Basically the DMA system
> > > > will move data to tty buffer when a) DMA buffer is full b) serial line is
> idle.
> > > > The situation is visible when producer generates data continuously
> > > > and there is no possibility for idle line. At this point the DMA
> > > > buffer is directly affecting the delta time.
> >
> > Hi Uwe,
> > Maybe I can explain a bit better the situation. At least I've tried
> > to explain well enough the problem and the fix. :)
> >
> > >
> > > This doesn't look like a hw property but a configuration item. Also
> > > I don't understand the problematic case. The i.MX is sending
> > > continuously and then doesn't receive bytes until the DMA buffer is full?
> >
> > Yes
> >
> > > What is the DMA buffer?
> > > You don't mean the FIFO here, do you?
> > >
> >
> > DMA buffer is not HW FIFO. Is the buffer provided by serial driver to DMA to
> store data.
> >
> > > That doesn't sound like a good fix but more like a work around.
> > > Which other options did you test to fix your problem?
> > >
> >
> > I haven't tried any other, because except using maybe, ioctl I haven't
> > got anything better.
>
> My question didn't target where to configure the buffer size instead of dts. I
> wonder if it would help to change the fifo watermark limits for example.
>
Ok. Sorry I didn't understand correct. Watermark will not help in this case because it is used only to trigger
the moving of the data from RX FIFO to DMA buffer. The iMX DMA (check the iMX6 RM A.3.1.2.4 since is missing
from iMX53 RM) will only return data to serial driver when no more data exist in the RX FIFO (is using aging timer for this check)
or BD (DMA buffer) is full. If data is sent continuously DMA will return data to driver only when BD is full.
So what we need here is a trigger that will force DMA to return data faster to serial driver. The only one that I could find was the size of the buffer. Of course another way will be to create different SDMA scripts that can do all kinds of handling -- but dint' want to go on that route :)
> > Our problem is that in our system some serial ports needs to have
> > really low data latency, where others trade more bytes over data
> > latency. This situation results in a need of beeing able to have
> > different DMA buffer size for different ports.
> >
> > How can DMA buffer size affect latency?
> > DMA works like this: (To answer to your question DMA buffer is not
> > FIFO) 1. Transfer the data from HW FIFO to DMA buffer based on some
> > interrupts (character received, etc) 2. Transfer the DMA buffer back
> > to serial port based on some events (buffer full, aging timer, etc) 3. Serial
> port forwards to tty buffer.
>
> BTW In the past I saw the serial core introduce latency, too. Are you sure
> that's not your bottle neck?
>
Can you be more specific? Receiving data seems very straight forward and serial core is not involved that much with or without DMA.
Basically the driver handles the moving of the data from rx FIFO to tty buffer. Beside this lowering the DMA buffer fixed our issue. So I will assume that no.
> > Data availability to consumer depends on: DMA buffer size, baud rate
> > and communication pattern. By communication patter I'm refering that
> > we send data continuoselly (serial line is never idle) or packet by
> > packet (serial line is idle in between)
> > Example:
> > Baud: 19200 (1Byte = 0.52 ms)
> > DMA buffer size: 100 bytes
> > Communication pattern: continuously
> > => DMA will return data to serial port only when DMA buffer is
> > full, since the communication is continuously. This result in a
> > data latency of 0.52 ms* 100bytes = 52ms. In case the buffer
> > will be 200bytes the letency will be double.
> >
> > I agree with you, this is not directly a hw property but a DMA configuration
> item.
> > But I've found this to be the best way to configure this comparing with using
> ioctl.
> >
> > Let me know if you need more clarification and I would really be open
> > to other options that will solve our problem.
> >
> > <snip>
> >
> > > > +- fsl,dma-size : Indicate the size of the DMA buffer and its
> > > > +periods
> > >
> > > This is a sparse description, just from reading that I don't
> > > understand what it does.
> > >
> >
> > Serial driver configures a circular ring of buffers for DMA. Here we
> > can configure the size and the number of buffers.
>
> The problem is: How should a person, who wants to make available a port on a
> machine via dts, choose what value to use for fsl,dma-size?
It doesn't have too. If this configuration is not provided the serial port will have a default value.
The user has the possibility to tweak this settings in case is needed.
>
> What you want (for a low latency port) is that also small amounts of data
> received are passed quickly to the upper layer. The knob you identified to be
> available for that is the dma buffer size.
>
> I'd prefer to talk about low latency instead of buffer sizes when setting
> parameters for the port. That this influences the buffer size (and maybe
> watermark settings) under the hood shouldn't matter for the person
> configuring the low latency property.
>
I understand your point of view. A simple grep in "Documentation/devicetree/bindings" shows me that both options are used (latency/buffer configurations). We could use latency here as configuration and then translate it to DMA buffer size taking in consideration the baud rate but it's a bit more work to do and beside this we have also the number of buffers configuration which will be trickier to convert it from latency. In my opinion, I don't see a significant difference between latency and buffer since most of the serial port users (userspace level) don't touch these settings. Of course, will be a different story if this configuration will be provided using termios or other means accessible from userspace.
> Best regards
> Uwe
>
Thanks Uwe :)
Regards,
Nandor
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/2] serdev: Update drivers/bluetooth/Kconfig for ACPI serdev support
From: Ian W MORRISON @ 2017-10-10 7:16 UTC (permalink / raw)
To: Johan Hovold
Cc: Marcel Holtmann, Gustavo F. Padovan, Johan Hedberg,
bluez mailin list (linux-bluetooth@vger.kernel.org),
Hans de Goede, Frédéric Danis, Rob Herring,
Sebastian Reichel, Loic Poulain, Lukas Wunner, linux-serial,
linux-acpi, Rafael J. Wysocki, Greg Kroah-Hartman
In-Reply-To: <20171009080606.GO2618@localhost>
On 9 October 2017 at 19:06, Johan Hovold <johan@kernel.org> wrote:
> On Mon, Oct 09, 2017 at 11:43:31AM +1100, Ian W MORRISON wrote:
<snip>
>>
>> This patch set addresses this by making BT_HCIUART_BCM dependent on
>> SERIAL_DEV_CTRL_TTYPORT which in turn is dependent on SERIAL_DEV_BUS
>> and ensures that if SERIAL_DEV_BUS is selected is the code is build it.
>
> Ok, so you didn't even bother to write two distinct commit messages for
> your two patches, and my comments to the first patch apply also here.
>
<snip>
>
> Johan
Hi Johan,
My experience in submitting patches is still limited and I am very
much learning so thanks for the helpful comments and apologies for
trying your patience. My keenness on submitting patches was to show
that I had tested my suggestions so as to given them credibility
rather than any attempt to own the patch.
What I was trying to do here was to suggested two patches with one
requiring the other. Rather than keep as a single patch affecting two
files I separated into two patches attempting to create a patch series
with a single changelog. This was the wrong approach and I may have
misled so apologies for that.
Regards,
Ian
^ permalink raw reply
* Re: [PATCH 2/2] ACPI / scan: Fix enumeration for special UART devices
From: Johan Hovold @ 2017-10-10 7:08 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hovold, Sebastian Reichel, Frédéric Danis,
robh-DgEjT+Ai2ygdnm+yROfE0A, loic.poulain-Re5JQEeQqe8AvxtiuMwx3w,
lukas-JFq808J9C/izQB+pC5nmwQ, hdegoede-H+wXaHxf7aLQT0dZR+AlfA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <E19C0643-85AA-4E80-BCDC-0C01EC0F88C2-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
On Mon, Oct 09, 2017 at 08:09:19PM +0200, Marcel Holtmann wrote:
> Hi Johan,
>
> >>>>>> UART devices is expected to be enumerated by SerDev subsystem.
> >>>>>>
> >>>>>> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> >>>>>> enumerated, allowing them to be enumerated by their respective parents.
> >>>>>>
> >>>>>> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> >>>>>> devices on serial buses (SPI, I2C or UART).
> >>>>>>
> >>>>>> On Macs an empty ResourceTemplate is returned for uart slaves.
> >>>>>> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> >>>>>> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
> >>>>>>
> >>>>>> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>>>
> >>>>> So just to reiterate what I just mentioned in a comment to one of Hans's
> >>>>> hci_bcm patches:
> >>>>>
> >>>>> This one would silently break PM for such devices on any system which
> >>>>> does not have serdev enabled (as the corresponding platform devices
> >>>>> would no longer be registered). And with serdev enabled, hciattach
> >>>>> (btattach) would start failing as the tty device would no longer be
> >>>>> registered (but I assume everyone is aware of that, and fine with it, by
> >>>>> now).
> >>>>>
> >>>>> Perhaps the hci_bcm driver should start depending on
> >>>>> SERIAL_DEV_CTRL_TTYPORT when ACPI is enabled?
> >>>>
> >>>> ACPI and DT both need SERIAL_DEV_CTRL_TTYPORT to work properly,
> >>>> since SERIAL_DEV_CTRL_TTYPORT is the only controller implemented
> >>>> for serdev. If any other controller is implemented that one could
> >>>> also be used.
> >>>
> >>> Not for hci_bcm, right? This particular driver specifically depends on
> >>> SERIAL_DEV_CTRL_TTYPORT for the ACPI devices and not just any (future)
> >>> serdev controller (or currently working systems soon breaks silently).
> >>>
> >>> I don't think the same is true for the DT case where we do not already
> >>> have child nodes defined in firmware (and in fact, this driver did not
> >>> really support DT before serdev).
> >>
> >> The serdev ACPI support has been added to the core and not to
> >> the ttyport and the hci_bcm driver only uses functions from the
> >> core. As far as I can see the ACPI part would also work fine with
> >> a different serdev controller.
> >
> > Indeed, but you need SERIAL_DEV_CTRL_TTYPORT to avoid silently breaking
> > current ACPI setups which breaks when this patch is applied (as these
> > devices all hang off of common serial ports managed by serial core).
> >
> >> Of course DT and ACPI currently require SERIAL_DEV_CTRL_TTYPORT,
> >> since it's the only serdev controller implementation. Also it
> >> covers most use cases. When SERIAL_DEV_BUS is selected it's
> >> very likely, that you also want SERIAL_DEV_CTRL_TTYPORT.
> >
> >>>> I wonder if we should just hide SERIAL_DEV_CTRL_TTYPORT and enable
> >>>> it together with SERDEV. I suspect that we won't see any other
> >>>> controller (it would be a UART device, that is not registered as
> >>>> tty device) in the next few years and the extra option seems to
> >>>> confuse people.
> >>>
> >>> I agree that it is somewhat confusing. But now that we have both,
> >>> perhaps simply having SERIAL_DEV_CTRL_TTYPORT default to "y" when
> >>> SERIAL_DEV_BUS is selected could be a compromise. The Kconfig entry
> >>> might need to be amended as well (e.g. if only to mention that you
> >>> need to select a controller as well).
> >>
> >> I think we should at least add a default "y" if SERIAL_DEV_BUS.
> >
> > I'm preparing a patch.
>
> yes, please prepare a patch since the discussion spans multiple email
> threads now. Lets get this back on track and find a patch that we are
> all happy with.
I submitted a patch amending the serdev Kconfig entries and making
SERIAL_DEV_CTRL_TTYPORT default to Y when serdev support has been
chosen.
While that hopefully reduces the confusion regarding the Kconfig options
somewhat, it's not really a fix for the potential silent hci_bcm
regression that could result from this patch.
[ The problem being that hci-attach would still succeed, but PM would be
broken, when SERIAL_DEV_CTRL_TTYPORT is not set. ]
I mentioned that making HCI_BCM depend on SERIAL_DEV_CTRL_TTYPORT might
be used to avoid this situation, although it is on some level is
conceptually wrong to describe runtime dependencies in Kconfig (cf.
having a USB device driver, depend on a particular host controller
rather than just USB support).
I'll write something like that up in a patch for Bluetooth and you can
decide if you want to apply it to remedy this particular situation,
though.
Johan
^ permalink raw reply
* Re: [PATCH v2] serial: sh-sci: Fix init data attribute for struct 'port_cfg'
From: Guenter Roeck @ 2017-10-10 1:54 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Arnd Bergmann, Guenter Roeck
In-Reply-To: <20171010012622.181557-1-mka@chromium.org>
On Mon, Oct 9, 2017 at 6:26 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> The __init attribute is meant to mark functions, use __initdata instead
> for the data structure.
>
> This fixes the following error when building with clang:
>
> drivers/tty/serial/sh-sci.c:3247:15: error: '__section__' attribute only
> applies to functions, methods, properties, and global variables
> static struct __init plat_sci_port port_cfg;
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
> Changes in v2:
> - Change the attribute to __initdata instead of removing it
> - Update commit message
>
> drivers/tty/serial/sh-sci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 784dd42002ea..f7f632213a8e 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3244,7 +3244,7 @@ early_platform_init_buffer("earlyprintk", &sci_driver,
> early_serial_buf, ARRAY_SIZE(early_serial_buf));
> #endif
> #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
> -static struct __init plat_sci_port port_cfg;
> +static struct plat_sci_port port_cfg __initdata;
>
> static int __init early_console_setup(struct earlycon_device *device,
> int type)
> --
> 2.14.2.920.gcf0c67979c-goog
>
^ permalink raw reply
* [PATCH v2] serial: sh-sci: Fix init data attribute for struct 'port_cfg'
From: Matthias Kaehlcke @ 2017-10-10 1:26 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, linux-kernel, Arnd Bergmann, Guenter Roeck,
Matthias Kaehlcke
The __init attribute is meant to mark functions, use __initdata instead
for the data structure.
This fixes the following error when building with clang:
drivers/tty/serial/sh-sci.c:3247:15: error: '__section__' attribute only
applies to functions, methods, properties, and global variables
static struct __init plat_sci_port port_cfg;
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- Change the attribute to __initdata instead of removing it
- Update commit message
drivers/tty/serial/sh-sci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 784dd42002ea..f7f632213a8e 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3244,7 +3244,7 @@ early_platform_init_buffer("earlyprintk", &sci_driver,
early_serial_buf, ARRAY_SIZE(early_serial_buf));
#endif
#ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
-static struct __init plat_sci_port port_cfg;
+static struct plat_sci_port port_cfg __initdata;
static int __init early_console_setup(struct earlycon_device *device,
int type)
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply related
* Re: [PATCH] serial: sh-sci: Remove __init attribute from static struct 'port_cfg'
From: Matthias Kaehlcke @ 2017-10-10 0:35 UTC (permalink / raw)
To: Guenter Roeck
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
Arnd Bergmann, Guenter Roeck
In-Reply-To: <CABXOdTdEH-=n8nf6a=0sC1iBeXLDKSxTXxoj1wUYmLUX_U=BUw@mail.gmail.com>
El Mon, Oct 09, 2017 at 05:18:32PM -0700 Guenter Roeck ha dit:
> On Mon, Oct 9, 2017 at 5:04 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > This fixes the following error when building with clang:
> >
> > drivers/tty/serial/sh-sci.c:3247:15: error: '__section__' attribute only
> > applies to functions, methods, properties, and global variables
> > static struct __init plat_sci_port port_cfg;
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > drivers/tty/serial/sh-sci.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> > index 784dd42002ea..0d1df1df9e89 100644
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -3244,7 +3244,7 @@ early_platform_init_buffer("earlyprintk", &sci_driver,
> > early_serial_buf, ARRAY_SIZE(early_serial_buf));
> > #endif
> > #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
> > -static struct __init plat_sci_port port_cfg;
> > +static struct plat_sci_port port_cfg;
>
> __initdata ?
Good point, thanks!
^ permalink raw reply
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