* [PATCH 1/2] tty: wipe buffer.
From: Greg Kroah-Hartman @ 2018-10-02 17:17 UTC (permalink / raw)
To: linux-serial
Cc: linux-kernel, jslaby, aszlig, gmazyland, torvalds, w,
Greg Kroah-Hartman
In-Reply-To: <20181002171708.1311-1-gregkh@linuxfoundation.org>
From: Linus Torvalds <torvalds@linux-foundation.org>
After we are done with the tty buffer, zero it out.
Reported-by: aszlig <aszlig@nix.build>
Tested-by: Milan Broz <gmazyland@gmail.com>
Tested-by: aszlig <aszlig@nix.build>
Cc: Willy Tarreau <w@1wt.eu>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/tty/tty_buffer.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index c996b6859c5e..ae3ce330200e 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -468,11 +468,15 @@ receive_buf(struct tty_port *port, struct tty_buffer *head, int count)
{
unsigned char *p = char_buf_ptr(head, head->read);
char *f = NULL;
+ int n;
if (~head->flags & TTYB_NORMAL)
f = flag_buf_ptr(head, head->read);
- return port->client_ops->receive_buf(port, p, f, count);
+ n = port->client_ops->receive_buf(port, p, f, count);
+ if (n > 0)
+ memset(p, 0, n);
+ return n;
}
/**
--
2.19.0
^ permalink raw reply related
* [PATCH 0/2] tty: erase buffers when the kernel is done with it.
From: Greg Kroah-Hartman @ 2018-10-02 17:17 UTC (permalink / raw)
To: linux-serial
Cc: linux-kernel, jslaby, aszlig, gmazyland, torvalds, w,
Greg Kroah-Hartman
azlig and Milan Broz reported that when the tty layer is done with a
buffer, the data can hang around in it for a very long time. That
sometimes can "leak" to userspace under some conditions.
Because of this, just zero out the data after the tty layer is finished
with it, for buffers that we "think" should be zeroed out.
Greg Kroah-Hartman (1):
tty: wipe buffer if not echoing data
Linus Torvalds (1):
tty: wipe buffer.
drivers/tty/n_tty.c | 20 +++++++++++++++++---
drivers/tty/tty_buffer.c | 6 +++++-
2 files changed, 22 insertions(+), 4 deletions(-)
--
2.19.0
^ permalink raw reply
* [PATCH] Revert "serial: 8250_dw: Fix runtime PM handling"
From: Guenter Roeck @ 2018-10-02 4:42 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Guenter Roeck,
Tony Lindgren, Andy Shevchenko, Phil Edworthy
This reverts commit d76c74387e1c978b6c5524a146ab0f3f72206f98.
While commit d76c74387e1c ("serial: 8250_dw: Fix runtime PM handling")
fixes runtime PM handling when using kgdb, it introduces a traceback for
everyone else.
BUG: sleeping function called from invalid context at
/mnt/host/source/src/third_party/kernel/next/drivers/base/power/runtime.c:1034
in_atomic(): 1, irqs_disabled(): 1, pid: 1, name: swapper/0
7 locks held by swapper/0/1:
#0: 000000005ec5bc72 (&dev->mutex){....}, at: __driver_attach+0xb5/0x12b
#1: 000000005d5fa9e5 (&dev->mutex){....}, at: __device_attach+0x3e/0x15b
#2: 0000000047e93286 (serial_mutex){+.+.}, at: serial8250_register_8250_port+0x51/0x8bb
#3: 000000003b328f07 (port_mutex){+.+.}, at: uart_add_one_port+0xab/0x8b0
#4: 00000000fa313d4d (&port->mutex){+.+.}, at: uart_add_one_port+0xcc/0x8b0
#5: 00000000090983ca (console_lock){+.+.}, at: vprintk_emit+0xdb/0x217
#6: 00000000c743e583 (console_owner){-...}, at: console_unlock+0x211/0x60f
irq event stamp: 735222
__down_trylock_console_sem+0x4a/0x84
console_unlock+0x338/0x60f
__do_softirq+0x4a4/0x50d
irq_exit+0x64/0xe2
CPU: 2 PID: 1 Comm: swapper/0 Not tainted 4.19.0-rc5 #6
Hardware name: Google Caroline/Caroline, BIOS Google_Caroline.7820.286.0 03/15/2017
Call Trace:
dump_stack+0x7d/0xbd
___might_sleep+0x238/0x259
__pm_runtime_resume+0x4e/0xa4
? serial8250_rpm_get+0x2e/0x44
serial8250_console_write+0x44/0x301
? lock_acquire+0x1b8/0x1fa
console_unlock+0x577/0x60f
vprintk_emit+0x1f0/0x217
printk+0x52/0x6e
register_console+0x43b/0x524
uart_add_one_port+0x672/0x8b0
? set_io_from_upio+0x150/0x162
serial8250_register_8250_port+0x825/0x8bb
dw8250_probe+0x80c/0x8b0
? dw8250_serial_inq+0x8e/0x8e
? dw8250_check_lcr+0x108/0x108
? dw8250_runtime_resume+0x5b/0x5b
? dw8250_serial_outq+0xa1/0xa1
? dw8250_remove+0x115/0x115
platform_drv_probe+0x76/0xc5
really_probe+0x1f1/0x3ee
? driver_allows_async_probing+0x5d/0x5d
driver_probe_device+0xd6/0x112
? driver_allows_async_probing+0x5d/0x5d
bus_for_each_drv+0xbe/0xe5
__device_attach+0xdd/0x15b
bus_probe_device+0x5a/0x10b
device_add+0x501/0x894
? _raw_write_unlock+0x27/0x3a
platform_device_add+0x224/0x2b7
mfd_add_device+0x718/0x75b
? __kmalloc+0x144/0x16a
? mfd_add_devices+0x38/0xdb
mfd_add_devices+0x9b/0xdb
intel_lpss_probe+0x7d4/0x8ee
intel_lpss_pci_probe+0xac/0xd4
pci_device_probe+0x101/0x18e
...
Revert the offending patch until a more comprehensive solution
is available.
Cc: Tony Lindgren <tony@atomide.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Fixes: d76c74387e1c ("serial: 8250_dw: Fix runtime PM handling")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/tty/serial/8250/8250_dw.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index fa8dcb470640..d31b975dd3fd 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -630,10 +630,6 @@ static int dw8250_probe(struct platform_device *pdev)
if (!data->skip_autocfg)
dw8250_setup_port(p);
-#ifdef CONFIG_PM
- uart.capabilities |= UART_CAP_RPM;
-#endif
-
/* If we have a valid fifosize, try hooking up DMA */
if (p->fifosize) {
data->dma.rxconf.src_maxburst = p->fifosize / 4;
--
2.7.4
^ permalink raw reply related
* Re: [BUG] sleep in atomic in 8250 runtime PM code path
From: Andy Shevchenko @ 2018-10-01 18:21 UTC (permalink / raw)
To: Tony Lindgren
Cc: Russell King - ARM Linux, Jisheng Zhang,
Sebastian Andrzej Siewior, Greg Kroah-Hartman, Phil Edworthy,
linux-arm-kernel, linux-kernel, linux-serial
In-Reply-To: <20181001180408.GY5662@atomide.com>
On Mon, Oct 01, 2018 at 11:04:08AM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@armlinux.org.uk> [180929 10:35]:
> > On Sat, Sep 29, 2018 at 01:20:36PM +0800, Jisheng Zhang wrote:
> > > Hi,
> > >
> > > Recently I found I could trigger sleep in atomic bug on berlin after commit
> > > d76c74387e1c ("serial: 8250_dw: Fix runtime PM handling"). The path looks like:
> > >
> > > dw8250_probe => serial850_register_8250_port => uart_add_one_port=>
> > > register_console => console_unlock => univ8250_console_write =>
> > > serial8250_console_write => serial8250_rpm_get => pm_runtime_get_sync
> > >
> > > The irq is disabled by printk_safe_enter_irqsave() in console_unlock, but
> > > pm_runtime_get_sync can't be called in atomic context...
> > >
> > > I guess the reason why we didn't notice it is due to the fact that
> > > only OMAP and DW sets UART_CAP_RPM currently, and DW set the flag in
> > > May 2018.
> > >
> > > Per my understanding, the bug sits in the 8250 core driver rather than
> > > 8250_dw.c.
Precisely!
It seats there from the day 1 of introducing PM runtime callbacks.
> >
> > (Adding Tony and Sebastian, presumably CAP_RPM comes from OMAP since
> > that is the only other user, and this same bug is present there too.)
>
> That only works because of pm_runtime_irq_safe() :( And we should not
> use pm_runtime_irq_safe() at all IMO, it takes a permanent RPM usecount
> on the parent device.
>
> Adding also Andy to Cc as he's been working on related fixes.
Thanks, Tony.
Unfortunately, I'm busy with some more important stuff, but I will return to this ASAP.
> > Correct. printk() can be called from atomic contexts (consider what
> > happens when an oops or similar occurs - we can be in any context,
> > holding any locks etc.) Plain printk() can also be used from within
> > spinlocked irqs-off regions.
> >
> > This means the console's write function may be called in these contexts.
> > Since pm_runtime_get_sync() is may sleep, it means that its use in the
> > console path is _fundamentally_ wrong, and will lead to exactly this
> > problem.
> >
> > I don't see a way around that other than to avoid RPM on consoles.
> > (which makes the presence of the RPM code in serial8250_console_write()
> > completely unnecessary.)
>
> Yup the way to go is to have some way to attach/detach kernel serial
> console via /sys, and have the serial layer take a usecount on the
> serial driver RPM when kernel serial console is attached. Then the
> user can detach serial console via /sys as needed and have RPM
> working.
Or disable runtime PM on kernel console completely, though it's not
the best solution, rather work around.
> > When I rewrote the serial drivers and created serial_core & 8250, this
> > is something that I realised, and I arranged the PM support at the time
> > to always maintain the console in active state (this is prior to RPM).
Have you had chance to see my series against this all mess?
> > While I'm looking at commit d74d5d1b7288 ("tty: serial: 8250_core: add
> > run time pm"):
> >
> > +static void serial8250_rpm_get_tx(struct uart_8250_port *p)
> > +{
> > + unsigned char rpm_active;
> > +
> > + if (!(p->capabilities & UART_CAP_RPM))
> > + return;
> > +
> > + rpm_active = xchg(&p->rpm_tx_active, 1);
> > + if (rpm_active)
> > + return;
> > + pm_runtime_get_sync(p->port.dev);
> > +}
> >
> > is particularly "interesting" - if this is called from sections of
> > code that allow it to be called concurrently from different contexts,
> > then we could have:
> >
> > rpm_tx_active thread 0 thread 1
> > 0
> > xchg(, 1)
> > 1
> > xchg(, 1)
> > ... goes on to use port ...
> > pm_runtime_get_sync()
> >
> > In other words, the port can be used _before_ pm_runtime_get_sync() is
> > called.
> >
> > If, on the other hand, this can't race, then considering the
> > serial8250_rpm_put_tx() path as well, what stops this race from
> > happening:
> >
> > rpm_tx_active thread 0 thread 1
> > 1
> > serial8250_rpm_get_tx()
> > serial8250_rpm_put_tx()
> > xchg(, 1)
> > 1
> > xchg(, 0)
> > 0
> > pm_runtime_put_autosuspend()
> >
> > Now to the real point about the above - if _neither_ race is possible,
> > then what is the point of the more expensive xchg() here rather than
> > simple test-and-assignment of rpm_tx_active? Either these paths can't
> > race with each other and xchg() is unnecessary, or they can and they
> > _could_ fail as shown above. My suspicion is that xchg() is an attempt
> > to reduce the likelyhood of one of these races being hit.
>
> Yeah the driver would need to maintain a is_suspended flag to prevent
> that. See also the is_suspended related parts for runtime PM docs at
> Documentation/power/runtime_pm.txt.
>
> But let's just make the serial layer take RPM usecount on the
> serial driver when console is in use and have some way for users
> to attach and detach the kernel serial console via /sys to prevent
> PM regressions.
Yes, it would be best solution that can cover old users and new comers.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [BUG] sleep in atomic in 8250 runtime PM code path
From: Tony Lindgren @ 2018-10-01 18:04 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Jisheng Zhang, Sebastian Andrzej Siewior, Greg Kroah-Hartman,
Phil Edworthy, linux-arm-kernel, linux-kernel, linux-serial,
Andy Shevchenko
In-Reply-To: <20180929103045.GQ30658@n2100.armlinux.org.uk>
* Russell King - ARM Linux <linux@armlinux.org.uk> [180929 10:35]:
> On Sat, Sep 29, 2018 at 01:20:36PM +0800, Jisheng Zhang wrote:
> > Hi,
> >
> > Recently I found I could trigger sleep in atomic bug on berlin after commit
> > d76c74387e1c ("serial: 8250_dw: Fix runtime PM handling"). The path looks like:
> >
> > dw8250_probe => serial850_register_8250_port => uart_add_one_port=>
> > register_console => console_unlock => univ8250_console_write =>
> > serial8250_console_write => serial8250_rpm_get => pm_runtime_get_sync
> >
> > The irq is disabled by printk_safe_enter_irqsave() in console_unlock, but
> > pm_runtime_get_sync can't be called in atomic context...
> >
> > I guess the reason why we didn't notice it is due to the fact that
> > only OMAP and DW sets UART_CAP_RPM currently, and DW set the flag in
> > May 2018.
> >
> > Per my understanding, the bug sits in the 8250 core driver rather than
> > 8250_dw.c.
>
> (Adding Tony and Sebastian, presumably CAP_RPM comes from OMAP since
> that is the only other user, and this same bug is present there too.)
That only works because of pm_runtime_irq_safe() :( And we should not
use pm_runtime_irq_safe() at all IMO, it takes a permanent RPM usecount
on the parent device.
Adding also Andy to Cc as he's been working on related fixes.
> Correct. printk() can be called from atomic contexts (consider what
> happens when an oops or similar occurs - we can be in any context,
> holding any locks etc.) Plain printk() can also be used from within
> spinlocked irqs-off regions.
>
> This means the console's write function may be called in these contexts.
> Since pm_runtime_get_sync() is may sleep, it means that its use in the
> console path is _fundamentally_ wrong, and will lead to exactly this
> problem.
>
> I don't see a way around that other than to avoid RPM on consoles.
> (which makes the presence of the RPM code in serial8250_console_write()
> completely unnecessary.)
Yup the way to go is to have some way to attach/detach kernel serial
console via /sys, and have the serial layer take a usecount on the
serial driver RPM when kernel serial console is attached. Then the
user can detach serial console via /sys as needed and have RPM
working.
> When I rewrote the serial drivers and created serial_core & 8250, this
> is something that I realised, and I arranged the PM support at the time
> to always maintain the console in active state (this is prior to RPM).
>
> While I'm looking at commit d74d5d1b7288 ("tty: serial: 8250_core: add
> run time pm"):
>
> +static void serial8250_rpm_get_tx(struct uart_8250_port *p)
> +{
> + unsigned char rpm_active;
> +
> + if (!(p->capabilities & UART_CAP_RPM))
> + return;
> +
> + rpm_active = xchg(&p->rpm_tx_active, 1);
> + if (rpm_active)
> + return;
> + pm_runtime_get_sync(p->port.dev);
> +}
>
> is particularly "interesting" - if this is called from sections of
> code that allow it to be called concurrently from different contexts,
> then we could have:
>
> rpm_tx_active thread 0 thread 1
> 0
> xchg(, 1)
> 1
> xchg(, 1)
> ... goes on to use port ...
> pm_runtime_get_sync()
>
> In other words, the port can be used _before_ pm_runtime_get_sync() is
> called.
>
> If, on the other hand, this can't race, then considering the
> serial8250_rpm_put_tx() path as well, what stops this race from
> happening:
>
> rpm_tx_active thread 0 thread 1
> 1
> serial8250_rpm_get_tx()
> serial8250_rpm_put_tx()
> xchg(, 1)
> 1
> xchg(, 0)
> 0
> pm_runtime_put_autosuspend()
>
> Now to the real point about the above - if _neither_ race is possible,
> then what is the point of the more expensive xchg() here rather than
> simple test-and-assignment of rpm_tx_active? Either these paths can't
> race with each other and xchg() is unnecessary, or they can and they
> _could_ fail as shown above. My suspicion is that xchg() is an attempt
> to reduce the likelyhood of one of these races being hit.
Yeah the driver would need to maintain a is_suspended flag to prevent
that. See also the is_suspended related parts for runtime PM docs at
Documentation/power/runtime_pm.txt.
But let's just make the serial layer take RPM usecount on the
serial driver when console is in use and have some way for users
to attach and detach the kernel serial console via /sys to prevent
PM regressions.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH] gpiolib: Fix incorrect use of find_next_zero_bit()
From: Linus Walleij @ 2018-10-01 9:37 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Miguel Ojeda Sandonis, Peter Korsgaard, Peter Rosin, Ulf Hansson,
Andrew Lunn, Florian Fainelli, David S. Miller, Dominik Brodowski,
Greg KH, kishon, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald, Jiri Slaby,
Willy Tarreau, Geert Uytterhoeven, Sebastien Bourdelin,
Lukas Wunner <luka>
In-Reply-To: <20180929122022.6825-1-jmkrzyszt@gmail.com>
On Sat, Sep 29, 2018 at 2:19 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> Commit b17566a6b08b ("gpiolib: Implement fast processing path in
> get/set array"), already fixed to some extent with commit 5d581d7e8cdc
> ("gpiolib: Fix missing updates of bitmap index"), introduced a new mode
> of processing bitmaps where bits applicable for fast bitmap processing
> path are supposed to be skipped while iterating bits which don't apply.
> Unfortunately, find_next_zero_bit() function supposed to skip over
> those fast bits is always called with a 'start' argument equal to an
> index of last zero bit found and returns that index value again an
> again, causing an infinite loop.
>
> Fix it by incrementing the index uncoditionally before
> find_next_zero_bit() is optionally called.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Patch applied with Marek's Tested-by.
Thanks to both of you for digging in and fixing this up!
Now we are in good shape for the v4.20 cycle :)
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] tty:serial:imx: use spin_lock instead of spin_lock_irqsave in isr
From: Uwe Kleine-König @ 2018-10-01 8:52 UTC (permalink / raw)
To: jun qian
Cc: Greg Kroah-Hartman, Barry song, Jiri Slaby, linux-serial,
linux-kernel, kernel
In-Reply-To: <20180827144904.17226-1-hangdianqj@163.com>
Hello,
On Mon, Aug 27, 2018 at 07:49:04AM -0700, jun qian wrote:
> Before the program enters the uart ISR, the local interrupt has been
> disabled by the system, so it's not appropriate to use spin_lock_irqsave
> interface in the ISR.
"not appropriate" is a bit strong. It's not as optimal as it could be
though, yes.
The change is fine,
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH] gpiolib: Fix incorrect use of find_next_zero_bit()
From: Marek Szyprowski @ 2018-10-01 6:46 UTC (permalink / raw)
To: Janusz Krzysztofik, Linus Walleij
Cc: Andrew Lunn, Ulf Hansson, Tony Lindgren, Dominik Brodowski,
Yegor Yefremov, netdev, linux-i2c, Peter Meerwald-Stadler, devel,
Florian Fainelli, Peter Rosin, Krzysztof Kozlowski,
Kishon Vijay Abraham I, linux-iio, Peter Korsgaard,
Geert Uytterhoeven, linux-serial, Jiri Slaby, Michael Hennerich,
Uwe Kleine-König, linux-gpio, Russell King,
Lars-Peter Clausen
In-Reply-To: <20180929122022.6825-1-jmkrzyszt@gmail.com>
Hi Janusz,
On 2018-09-29 14:20, Janusz Krzysztofik wrote:
> Commit b17566a6b08b ("gpiolib: Implement fast processing path in
> get/set array"), already fixed to some extent with commit 5d581d7e8cdc
> ("gpiolib: Fix missing updates of bitmap index"), introduced a new mode
> of processing bitmaps where bits applicable for fast bitmap processing
> path are supposed to be skipped while iterating bits which don't apply.
> Unfortunately, find_next_zero_bit() function supposed to skip over
> those fast bits is always called with a 'start' argument equal to an
> index of last zero bit found and returns that index value again an
> again, causing an infinite loop.
>
> Fix it by incrementing the index uncoditionally before
> find_next_zero_bit() is optionally called.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Marek,
>
> Could you please test it on top of next-20180920 with "gpiolib: Fix
> missing updates of bitmap index" and optionally "mmc: pwrseq_simple:
> Fix incorrect handling of GPIO bitmap" also applied?
This patch finally fixes the boot issue on Samsung Chromebook Snow.
Thanks!
>
> Thanks,
> Janusz
>
>
> drivers/gpio/gpiolib.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6ae13e3e05f1..940b543e966d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2878,12 +2878,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
> int hwgpio = gpio_chip_hwgpio(desc);
>
> __set_bit(hwgpio, mask);
> + i++;
>
> if (array_info)
> i = find_next_zero_bit(array_info->get_mask,
> array_size, i);
> - else
> - i++;
> } while ((i < array_size) &&
> (desc_array[i]->gdev->chip == chip));
>
> @@ -2903,12 +2902,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
> value = !value;
> __assign_bit(j, value_bitmap, value);
> trace_gpio_value(desc_to_gpio(desc), 1, value);
> + j++;
>
> if (array_info)
> j = find_next_zero_bit(array_info->get_mask, i,
> j);
> - else
> - j++;
> }
>
> if (mask != fastpath)
> @@ -3191,12 +3189,11 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
> __clear_bit(hwgpio, bits);
> count++;
> }
> + i++;
>
> if (array_info)
> i = find_next_zero_bit(array_info->set_mask,
> array_size, i);
> - else
> - i++;
> } while ((i < array_size) &&
> (desc_array[i]->gdev->chip == chip));
> /* push collected bits to outputs */
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH v2 1/3] arm64: dts: actions: s900: Enable Tx DMA for UART5
From: kbuild test robot @ 2018-09-30 2:04 UTC (permalink / raw)
Cc: kbuild-all, vkoul, dan.j.williams, afaerber, robh+dt, gregkh,
jslaby, linux-serial, dmaengine, liuwei, 96boards, devicetree,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-kernel,
hzhang, bdong, manivannanece23, thomas.liau, jeff.chen, pn,
edgar.righi, Manivannan Sadhasivam
In-Reply-To: <20180929074637.9766-2-manivannan.sadhasivam@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]
Hi Manivannan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.19-rc5 next-20180928]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Manivannan-Sadhasivam/Add-slave-DMA-support-for-Actions-Semi-S900-SoC/20180929-155016
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm64
All errors (new ones prefixed by >>):
>> ERROR: Input tree has errors, aborting (use -f to force output)
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40255 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] sc16is7xx: Fix for multi-channel stall
From: Andreas Färber @ 2018-09-29 14:04 UTC (permalink / raw)
To: Phil Elwell, Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Alexander Graf,
Stefan Wahren, Mian Yousaf Kaukab, Matthias Brugger,
Michael Allwright, Jakub Kicinski, Xue Liu
In-Reply-To: <1536762716-30673-2-git-send-email-phil@raspberrypi.org>
Hi Phil and Greg,
Am 12.09.18 um 16:31 schrieb Phil Elwell:
> The SC16IS752 is a dual-channel device. The two channels are largely
> independent, but the IRQ signals are wired together as an open-drain,
> active low signal which will be driven low while either of the
> channels requires attention, which can be for significant periods of
> time until operations complete and the interrupt can be acknowledged.
> In that respect it is should be treated as a true level-sensitive IRQ.
>
> The kernel, however, needs to be able to exit interrupt context in
> order to use I2C or SPI to access the device registers (which may
> involve sleeping). Therefore the interrupt needs to be masked out or
> paused in some way.
>
> The usual way to manage sleeping from within an interrupt handler
> is to use a threaded interrupt handler - a regular interrupt routine
> does the minimum amount of work needed to triage the interrupt before
> waking the interrupt service thread. If the threaded IRQ is marked as
> IRQF_ONESHOT the kernel will automatically mask out the interrupt
> until the thread runs to completion. The sc16is7xx driver used to
> use a threaded IRQ, but a patch switched to using a kthread_worker
> in order to set realtime priorities on the handler thread and for
> other optimisations. The end result is non-threaded IRQ that
> schedules some work then returns IRQ_HANDLED, making the kernel
> think that all IRQ processing has completed.
>
> The work-around to prevent a constant stream of interrupts is to
> mark the interrupt as edge-sensitive rather than level-sensitive,
> but interpreting an active-low source as a falling-edge source
> requires care to prevent a total cessation of interrupts. Whereas
> an edge-triggering source will generate a new edge for every interrupt
> condition a level-triggering source will keep the signal at the
> interrupting level until it no longer requires attention; in other
> words, the host won't see another edge until all interrupt conditions
> are cleared. It is therefore vital that the interrupt handler does not
> exit with an outstanding interrupt condition, otherwise the kernel
> will not receive another interrupt unless some other operation causes
> the interrupt state on the device to be cleared.
>
> The existing sc16is7xx driver has a very simple interrupt "thread"
> (kthread_work job) that processes interrupts on each channel in turn
> until there are no more. If both channels are active and the first
> channel starts interrupting while the handler for the second channel
> is running then it will not be detected and an IRQ stall ensues. This
> could be handled easily if there was a shared IRQ status register, or
> a convenient way to determine if the IRQ had been deasserted for any
> length of time, but both appear to be lacking.
>
> Avoid this problem (or at least make it much less likely to happen)
> by reducing the granularity of per-channel interrupt processing
> to one condition per iteration, only exiting the overall loop when
> both channels are no longer interrupting.
>
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> ---
> drivers/tty/serial/sc16is7xx.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
These two patches seem to be applied in linux-next tree, but are lacking
a Fixes: header for backporting to affected stable trees.
openSUSE Tumbleweed's 4.18 appears to be affected, and I didn't see it
in linux.git for upcoming 4.19 either.
Can the commit message still be updated to get this fixed everywhere?
Which is the offending commit, this one from 2016? The only other 2018
change seems an unrelated error handling cleanup.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/tty/serial/sc16is7xx.c?id=04da73803c05dc1150ccc31cbf93e8cd56679c09
Also, the above commit message confuses me as to how after this commit
the driver should be used: Should the interrupt still be falling-edge,
as documented in the DT bindings, or do user/documentation need to
switch to level-triggered now?
Thanks,
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
* [PATCH] gpiolib: Fix incorrect use of find_next_zero_bit()
From: Janusz Krzysztofik @ 2018-09-29 12:20 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Ulf Hansson, Tony Lindgren, Dominik Brodowski,
Yegor Yefremov, netdev, linux-i2c, Peter Meerwald-Stadler,
Marek Szyprowski, devel, Florian Fainelli, Peter Rosin,
Janusz Krzysztofik, Krzysztof Kozlowski, Kishon Vijay Abraham I,
linux-iio, Peter Korsgaard, Geert Uytterhoeven, linux-serial,
Jiri Slaby, Michael Hennerich, Uwe Kleine-König, linux-gpio
In-Reply-To: <20180923235336.22148-2-jmkrzyszt@gmail.com>
Commit b17566a6b08b ("gpiolib: Implement fast processing path in
get/set array"), already fixed to some extent with commit 5d581d7e8cdc
("gpiolib: Fix missing updates of bitmap index"), introduced a new mode
of processing bitmaps where bits applicable for fast bitmap processing
path are supposed to be skipped while iterating bits which don't apply.
Unfortunately, find_next_zero_bit() function supposed to skip over
those fast bits is always called with a 'start' argument equal to an
index of last zero bit found and returns that index value again an
again, causing an infinite loop.
Fix it by incrementing the index uncoditionally before
find_next_zero_bit() is optionally called.
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
Marek,
Could you please test it on top of next-20180920 with "gpiolib: Fix
missing updates of bitmap index" and optionally "mmc: pwrseq_simple:
Fix incorrect handling of GPIO bitmap" also applied?
Thanks,
Janusz
drivers/gpio/gpiolib.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6ae13e3e05f1..940b543e966d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2878,12 +2878,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
int hwgpio = gpio_chip_hwgpio(desc);
__set_bit(hwgpio, mask);
+ i++;
if (array_info)
i = find_next_zero_bit(array_info->get_mask,
array_size, i);
- else
- i++;
} while ((i < array_size) &&
(desc_array[i]->gdev->chip == chip));
@@ -2903,12 +2902,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
value = !value;
__assign_bit(j, value_bitmap, value);
trace_gpio_value(desc_to_gpio(desc), 1, value);
+ j++;
if (array_info)
j = find_next_zero_bit(array_info->get_mask, i,
j);
- else
- j++;
}
if (mask != fastpath)
@@ -3191,12 +3189,11 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
__clear_bit(hwgpio, bits);
count++;
}
+ i++;
if (array_info)
i = find_next_zero_bit(array_info->set_mask,
array_size, i);
- else
- i++;
} while ((i < array_size) &&
(desc_array[i]->gdev->chip == chip));
/* push collected bits to outputs */
--
2.16.4
^ permalink raw reply related
* Re: [BUG] sleep in atomic in 8250 runtime PM code path
From: Russell King - ARM Linux @ 2018-09-29 10:30 UTC (permalink / raw)
To: Jisheng Zhang, Tony Lindgren
Cc: Sebastian Andrzej Siewior, Greg Kroah-Hartman, Phil Edworthy,
linux-arm-kernel, linux-kernel, linux-serial
In-Reply-To: <20180929132036.0323e24b@xhacker.debian>
On Sat, Sep 29, 2018 at 01:20:36PM +0800, Jisheng Zhang wrote:
> Hi,
>
> Recently I found I could trigger sleep in atomic bug on berlin after commit
> d76c74387e1c ("serial: 8250_dw: Fix runtime PM handling"). The path looks like:
>
> dw8250_probe => serial850_register_8250_port => uart_add_one_port=>
> register_console => console_unlock => univ8250_console_write =>
> serial8250_console_write => serial8250_rpm_get => pm_runtime_get_sync
>
> The irq is disabled by printk_safe_enter_irqsave() in console_unlock, but
> pm_runtime_get_sync can't be called in atomic context...
>
> I guess the reason why we didn't notice it is due to the fact that
> only OMAP and DW sets UART_CAP_RPM currently, and DW set the flag in
> May 2018.
>
> Per my understanding, the bug sits in the 8250 core driver rather than
> 8250_dw.c.
(Adding Tony and Sebastian, presumably CAP_RPM comes from OMAP since
that is the only other user, and this same bug is present there too.)
Correct. printk() can be called from atomic contexts (consider what
happens when an oops or similar occurs - we can be in any context,
holding any locks etc.) Plain printk() can also be used from within
spinlocked irqs-off regions.
This means the console's write function may be called in these contexts.
Since pm_runtime_get_sync() is may sleep, it means that its use in the
console path is _fundamentally_ wrong, and will lead to exactly this
problem.
I don't see a way around that other than to avoid RPM on consoles.
(which makes the presence of the RPM code in serial8250_console_write()
completely unnecessary.)
When I rewrote the serial drivers and created serial_core & 8250, this
is something that I realised, and I arranged the PM support at the time
to always maintain the console in active state (this is prior to RPM).
While I'm looking at commit d74d5d1b7288 ("tty: serial: 8250_core: add
run time pm"):
+static void serial8250_rpm_get_tx(struct uart_8250_port *p)
+{
+ unsigned char rpm_active;
+
+ if (!(p->capabilities & UART_CAP_RPM))
+ return;
+
+ rpm_active = xchg(&p->rpm_tx_active, 1);
+ if (rpm_active)
+ return;
+ pm_runtime_get_sync(p->port.dev);
+}
is particularly "interesting" - if this is called from sections of
code that allow it to be called concurrently from different contexts,
then we could have:
rpm_tx_active thread 0 thread 1
0
xchg(, 1)
1
xchg(, 1)
... goes on to use port ...
pm_runtime_get_sync()
In other words, the port can be used _before_ pm_runtime_get_sync() is
called.
If, on the other hand, this can't race, then considering the
serial8250_rpm_put_tx() path as well, what stops this race from
happening:
rpm_tx_active thread 0 thread 1
1
serial8250_rpm_get_tx()
serial8250_rpm_put_tx()
xchg(, 1)
1
xchg(, 0)
0
pm_runtime_put_autosuspend()
Now to the real point about the above - if _neither_ race is possible,
then what is the point of the more expensive xchg() here rather than
simple test-and-assignment of rpm_tx_active? Either these paths can't
race with each other and xchg() is unnecessary, or they can and they
_could_ fail as shown above. My suspicion is that xchg() is an attempt
to reduce the likelyhood of one of these races being hit.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 14.8Mbps down 650kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply
* [PATCH v2 3/3] tty: serial: Add Tx DMA support for UART in Actions Semi Owl SoCs
From: Manivannan Sadhasivam @ 2018-09-29 7:46 UTC (permalink / raw)
To: vkoul, dan.j.williams, afaerber, robh+dt, gregkh, jslaby
Cc: linux-serial, dmaengine, liuwei, 96boards, devicetree,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-kernel,
hzhang, bdong, manivannanece23, thomas.liau, jeff.chen, pn,
edgar.righi, Manivannan Sadhasivam
In-Reply-To: <20180929074637.9766-1-manivannan.sadhasivam@linaro.org>
Add Tx DMA support for Actions Semi Owl SoCs. If there is no DMA
property specified in DT, it will fallback to default interrupt mode.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
drivers/tty/serial/owl-uart.c | 172 +++++++++++++++++++++++++++++++++-
1 file changed, 171 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/owl-uart.c b/drivers/tty/serial/owl-uart.c
index 29a6dc6a8d23..1b3016db7ae2 100644
--- a/drivers/tty/serial/owl-uart.c
+++ b/drivers/tty/serial/owl-uart.c
@@ -11,6 +11,8 @@
#include <linux/clk.h>
#include <linux/console.h>
#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -48,6 +50,8 @@
#define OWL_UART_CTL_RXIE BIT(18)
#define OWL_UART_CTL_TXIE BIT(19)
#define OWL_UART_CTL_LBEN BIT(20)
+#define OWL_UART_CTL_DRCR BIT(21)
+#define OWL_UART_CTL_DTCR BIT(22)
#define OWL_UART_STAT_RIP BIT(0)
#define OWL_UART_STAT_TIP BIT(1)
@@ -71,12 +75,21 @@ struct owl_uart_info {
struct owl_uart_port {
struct uart_port port;
struct clk *clk;
+
+ struct dma_chan *tx_ch;
+ dma_addr_t tx_dma_buf;
+ dma_cookie_t dma_tx_cookie;
+ u32 tx_size;
+ bool tx_dma;
+ bool dma_tx_running;
};
#define to_owl_uart_port(prt) container_of(prt, struct owl_uart_port, prt)
static struct owl_uart_port *owl_uart_ports[OWL_UART_PORT_NUM];
+static void owl_uart_dma_start_tx(struct owl_uart_port *owl_port);
+
static inline void owl_uart_write(struct uart_port *port, u32 val, unsigned int off)
{
writel(val, port->membase + off);
@@ -115,6 +128,83 @@ static unsigned int owl_uart_get_mctrl(struct uart_port *port)
return mctrl;
}
+static void owl_uart_dma_tx_callback(void *data)
+{
+ struct owl_uart_port *owl_port = data;
+ struct uart_port *port = &owl_port->port;
+ struct circ_buf *xmit = &port->state->xmit;
+ unsigned long flags;
+ u32 val;
+
+ dma_sync_single_for_cpu(port->dev, owl_port->tx_dma_buf,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+
+ spin_lock_irqsave(&port->lock, flags);
+
+ owl_port->dma_tx_running = 0;
+
+ xmit->tail += owl_port->tx_size;
+ xmit->tail &= UART_XMIT_SIZE - 1;
+ port->icount.tx += owl_port->tx_size;
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(port);
+
+ /* Disable Tx DRQ */
+ val = owl_uart_read(port, OWL_UART_CTL);
+ val &= ~OWL_UART_CTL_TXDE;
+ owl_uart_write(port, val, OWL_UART_CTL);
+
+ /* Clear pending Tx IRQ */
+ val = owl_uart_read(port, OWL_UART_STAT);
+ val |= OWL_UART_STAT_TIP;
+ owl_uart_write(port, val, OWL_UART_STAT);
+
+ if (!uart_circ_empty(xmit) && !uart_tx_stopped(port))
+ owl_uart_dma_start_tx(owl_port);
+
+ spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static void owl_uart_dma_start_tx(struct owl_uart_port *owl_port)
+{
+ struct uart_port *port = &owl_port->port;
+ struct circ_buf *xmit = &port->state->xmit;
+ struct dma_async_tx_descriptor *desc;
+ u32 val;
+
+ if (uart_tx_stopped(port) || uart_circ_empty(xmit) ||
+ owl_port->dma_tx_running)
+ return;
+
+ dma_sync_single_for_device(port->dev, owl_port->tx_dma_buf,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+
+ owl_port->tx_size = CIRC_CNT_TO_END(xmit->head, xmit->tail,
+ UART_XMIT_SIZE);
+
+ desc = dmaengine_prep_slave_single(owl_port->tx_ch,
+ owl_port->tx_dma_buf + xmit->tail,
+ owl_port->tx_size, DMA_MEM_TO_DEV,
+ DMA_PREP_INTERRUPT);
+ if (!desc)
+ return;
+
+ desc->callback = owl_uart_dma_tx_callback;
+ desc->callback_param = owl_port;
+
+ /* Enable Tx DRQ */
+ val = owl_uart_read(port, OWL_UART_CTL);
+ val &= ~OWL_UART_CTL_TXIE;
+ val |= OWL_UART_CTL_TXDE | OWL_UART_CTL_DTCR;
+ owl_uart_write(port, val, OWL_UART_CTL);
+
+ /* Start Tx DMA transfer */
+ owl_port->dma_tx_running = true;
+ owl_port->dma_tx_cookie = dmaengine_submit(desc);
+ dma_async_issue_pending(owl_port->tx_ch);
+}
+
static unsigned int owl_uart_tx_empty(struct uart_port *port)
{
unsigned long flags;
@@ -159,6 +249,7 @@ static void owl_uart_stop_tx(struct uart_port *port)
static void owl_uart_start_tx(struct uart_port *port)
{
+ struct owl_uart_port *owl_port = to_owl_uart_port(port);
u32 val;
if (uart_tx_stopped(port)) {
@@ -166,6 +257,11 @@ static void owl_uart_start_tx(struct uart_port *port)
return;
}
+ if (owl_port->tx_dma) {
+ owl_uart_dma_start_tx(owl_port);
+ return;
+ }
+
val = owl_uart_read(port, OWL_UART_STAT);
val |= OWL_UART_STAT_TIP;
owl_uart_write(port, val, OWL_UART_STAT);
@@ -273,13 +369,27 @@ static irqreturn_t owl_uart_irq(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static void owl_dma_channel_free(struct owl_uart_port *owl_port)
+{
+ dmaengine_terminate_all(owl_port->tx_ch);
+ dma_release_channel(owl_port->tx_ch);
+ dma_unmap_single(owl_port->port.dev, owl_port->tx_dma_buf,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+ owl_port->dma_tx_running = false;
+ owl_port->tx_ch = NULL;
+}
+
static void owl_uart_shutdown(struct uart_port *port)
{
- u32 val;
+ struct owl_uart_port *owl_port = to_owl_uart_port(port);
unsigned long flags;
+ u32 val;
spin_lock_irqsave(&port->lock, flags);
+ if (owl_port->tx_dma)
+ owl_dma_channel_free(owl_port);
+
val = owl_uart_read(port, OWL_UART_CTL);
val &= ~(OWL_UART_CTL_TXIE | OWL_UART_CTL_RXIE
| OWL_UART_CTL_TXDE | OWL_UART_CTL_RXDE | OWL_UART_CTL_EN);
@@ -290,6 +400,62 @@ static void owl_uart_shutdown(struct uart_port *port)
free_irq(port->irq, port);
}
+static int owl_uart_dma_tx_init(struct uart_port *port)
+{
+ struct owl_uart_port *owl_port = to_owl_uart_port(port);
+ struct device *dev = port->dev;
+ struct dma_slave_config slave_config;
+ int ret;
+
+ owl_port->tx_dma = false;
+
+ /* Request DMA TX channel */
+ owl_port->tx_ch = dma_request_slave_channel(dev, "tx");
+ if (!owl_port->tx_ch) {
+ dev_info(dev, "tx dma alloc failed\n");
+ return -ENODEV;
+ }
+
+ owl_port->tx_dma_buf = dma_map_single(dev,
+ owl_port->port.state->xmit.buf,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+ if (dma_mapping_error(dev, owl_port->tx_dma_buf)) {
+ ret = -ENOMEM;
+ goto alloc_err;
+ }
+
+ /* Configure DMA channel */
+ memset(&slave_config, 0, sizeof(slave_config));
+ slave_config.direction = DMA_MEM_TO_DEV;
+ slave_config.dst_addr = port->mapbase + OWL_UART_TXDAT;
+ slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+
+ ret = dmaengine_slave_config(owl_port->tx_ch, &slave_config);
+ if (ret < 0) {
+ dev_err(dev, "tx dma channel config failed\n");
+ ret = -ENODEV;
+ goto map_err;
+ }
+
+ /* Use DMA buffer size as the FIFO size */
+ port->fifosize = UART_XMIT_SIZE;
+
+ /* Set DMA flag */
+ owl_port->tx_dma = true;
+ owl_port->dma_tx_running = false;
+
+ return 0;
+
+map_err:
+ dma_unmap_single(dev, owl_port->tx_dma_buf, UART_XMIT_SIZE,
+ DMA_TO_DEVICE);
+alloc_err:
+ dma_release_channel(owl_port->tx_ch);
+ owl_port->tx_ch = NULL;
+
+ return ret;
+}
+
static int owl_uart_startup(struct uart_port *port)
{
u32 val;
@@ -301,6 +467,10 @@ static int owl_uart_startup(struct uart_port *port)
if (ret)
return ret;
+ ret = owl_uart_dma_tx_init(port);
+ if (!ret)
+ dev_info(port->dev, "using DMA for tx\n");
+
spin_lock_irqsave(&port->lock, flags);
val = owl_uart_read(port, OWL_UART_STAT);
--
2.17.1
^ permalink raw reply related
* [PATCH v2 2/3] dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC
From: Manivannan Sadhasivam @ 2018-09-29 7:46 UTC (permalink / raw)
To: vkoul, dan.j.williams, afaerber, robh+dt, gregkh, jslaby
Cc: linux-serial, dmaengine, liuwei, 96boards, devicetree,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-kernel,
hzhang, bdong, manivannanece23, thomas.liau, jeff.chen, pn,
edgar.righi, Manivannan Sadhasivam
In-Reply-To: <20180929074637.9766-1-manivannan.sadhasivam@linaro.org>
Add Slave and Cyclic mode support for Actions Semi Owl S900 SoC. The slave
mode supports bus width of 4 bytes common for all peripherals and 1 byte
specific for UART.
The cyclic mode supports only block mode transfer.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
drivers/dma/owl-dma.c | 279 ++++++++++++++++++++++++++++++++++++++++--
1 file changed, 272 insertions(+), 7 deletions(-)
diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
index 7812a6338acd..1d26db4c9229 100644
--- a/drivers/dma/owl-dma.c
+++ b/drivers/dma/owl-dma.c
@@ -21,6 +21,7 @@
#include <linux/mm.h>
#include <linux/module.h>
#include <linux/of_device.h>
+#include <linux/of_dma.h>
#include <linux/slab.h>
#include "virt-dma.h"
@@ -165,6 +166,7 @@ struct owl_dma_lli {
struct owl_dma_txd {
struct virt_dma_desc vd;
struct list_head lli_list;
+ bool cyclic;
};
/**
@@ -191,6 +193,8 @@ struct owl_dma_vchan {
struct virt_dma_chan vc;
struct owl_dma_pchan *pchan;
struct owl_dma_txd *txd;
+ struct dma_slave_config cfg;
+ u8 drq;
};
/**
@@ -336,9 +340,11 @@ static struct owl_dma_lli *owl_dma_alloc_lli(struct owl_dma *od)
static struct owl_dma_lli *owl_dma_add_lli(struct owl_dma_txd *txd,
struct owl_dma_lli *prev,
- struct owl_dma_lli *next)
+ struct owl_dma_lli *next,
+ bool is_cyclic)
{
- list_add_tail(&next->node, &txd->lli_list);
+ if (!is_cyclic)
+ list_add_tail(&next->node, &txd->lli_list);
if (prev) {
prev->hw.next_lli = next->phys;
@@ -351,7 +357,9 @@ static struct owl_dma_lli *owl_dma_add_lli(struct owl_dma_txd *txd,
static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
struct owl_dma_lli *lli,
dma_addr_t src, dma_addr_t dst,
- u32 len, enum dma_transfer_direction dir)
+ u32 len, enum dma_transfer_direction dir,
+ struct dma_slave_config *sconfig,
+ bool is_cyclic)
{
struct owl_dma_lli_hw *hw = &lli->hw;
u32 mode;
@@ -364,6 +372,32 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
OWL_DMA_MODE_DT_DCU | OWL_DMA_MODE_SAM_INC |
OWL_DMA_MODE_DAM_INC;
+ break;
+ case DMA_MEM_TO_DEV:
+ mode |= OWL_DMA_MODE_TS(vchan->drq)
+ | OWL_DMA_MODE_ST_DCU | OWL_DMA_MODE_DT_DEV
+ | OWL_DMA_MODE_SAM_INC | OWL_DMA_MODE_DAM_CONST;
+
+ /*
+ * Hardware only supports 32bit and 8bit buswidth. Since the
+ * default is 32bit, select 8bit only when requested.
+ */
+ if (sconfig->dst_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
+ mode |= OWL_DMA_MODE_NDDBW_8BIT;
+
+ break;
+ case DMA_DEV_TO_MEM:
+ mode |= OWL_DMA_MODE_TS(vchan->drq)
+ | OWL_DMA_MODE_ST_DEV | OWL_DMA_MODE_DT_DCU
+ | OWL_DMA_MODE_SAM_CONST | OWL_DMA_MODE_DAM_INC;
+
+ /*
+ * Hardware only supports 32bit and 8bit buswidth. Since the
+ * default is 32bit, select 8bit only when requested.
+ */
+ if (sconfig->src_addr_width == DMA_SLAVE_BUSWIDTH_1_BYTE)
+ mode |= OWL_DMA_MODE_NDDBW_8BIT;
+
break;
default:
return -EINVAL;
@@ -381,7 +415,10 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
OWL_DMA_LLC_SAV_LOAD_NEXT |
OWL_DMA_LLC_DAV_LOAD_NEXT);
- hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
+ if (is_cyclic)
+ hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
+ else
+ hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
return 0;
}
@@ -443,6 +480,16 @@ static void owl_dma_terminate_pchan(struct owl_dma *od,
spin_unlock_irqrestore(&od->lock, flags);
}
+static void owl_dma_pause_pchan(struct owl_dma_pchan *pchan)
+{
+ pchan_writel(pchan, 1, OWL_DMAX_PAUSE);
+}
+
+static void owl_dma_resume_pchan(struct owl_dma_pchan *pchan)
+{
+ pchan_writel(pchan, 0, OWL_DMAX_PAUSE);
+}
+
static int owl_dma_start_next_txd(struct owl_dma_vchan *vchan)
{
struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
@@ -464,7 +511,10 @@ static int owl_dma_start_next_txd(struct owl_dma_vchan *vchan)
lli = list_first_entry(&txd->lli_list,
struct owl_dma_lli, node);
- int_ctl = OWL_DMA_INTCTL_SUPER_BLOCK;
+ if (txd->cyclic)
+ int_ctl = OWL_DMA_INTCTL_BLOCK;
+ else
+ int_ctl = OWL_DMA_INTCTL_SUPER_BLOCK;
pchan_writel(pchan, OWL_DMAX_MODE, OWL_DMA_MODE_LME);
pchan_writel(pchan, OWL_DMAX_LINKLIST_CTL,
@@ -627,6 +677,54 @@ static int owl_dma_terminate_all(struct dma_chan *chan)
return 0;
}
+static int owl_dma_config(struct dma_chan *chan,
+ struct dma_slave_config *config)
+{
+ struct owl_dma_vchan *vchan = to_owl_vchan(chan);
+
+ /* Reject definitely invalid configurations */
+ if (config->src_addr_width == DMA_SLAVE_BUSWIDTH_8_BYTES ||
+ config->dst_addr_width == DMA_SLAVE_BUSWIDTH_8_BYTES)
+ return -EINVAL;
+
+ memcpy(&vchan->cfg, config, sizeof(struct dma_slave_config));
+
+ return 0;
+}
+
+static int owl_dma_pause(struct dma_chan *chan)
+{
+ struct owl_dma_vchan *vchan = to_owl_vchan(chan);
+ unsigned long flags;
+
+ spin_lock_irqsave(&vchan->vc.lock, flags);
+
+ owl_dma_pause_pchan(vchan->pchan);
+
+ spin_unlock_irqrestore(&vchan->vc.lock, flags);
+
+ return 0;
+}
+
+static int owl_dma_resume(struct dma_chan *chan)
+{
+ struct owl_dma_vchan *vchan = to_owl_vchan(chan);
+ unsigned long flags;
+
+ if (!vchan->pchan && !vchan->txd)
+ return 0;
+
+ dev_dbg(chan2dev(chan), "vchan %p: resume\n", &vchan->vc);
+
+ spin_lock_irqsave(&vchan->vc.lock, flags);
+
+ owl_dma_resume_pchan(vchan->pchan);
+
+ spin_unlock_irqrestore(&vchan->vc.lock, flags);
+
+ return 0;
+}
+
static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
{
struct owl_dma_pchan *pchan;
@@ -754,13 +852,14 @@ static struct dma_async_tx_descriptor
bytes = min_t(size_t, (len - offset), OWL_DMA_FRAME_MAX_LENGTH);
ret = owl_dma_cfg_lli(vchan, lli, src + offset, dst + offset,
- bytes, DMA_MEM_TO_MEM);
+ bytes, DMA_MEM_TO_MEM,
+ &vchan->cfg, txd->cyclic);
if (ret) {
dev_warn(chan2dev(chan), "failed to config lli\n");
goto err_txd_free;
}
- prev = owl_dma_add_lli(txd, prev, lli);
+ prev = owl_dma_add_lli(txd, prev, lli, false);
}
return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
@@ -770,6 +869,133 @@ static struct dma_async_tx_descriptor
return NULL;
}
+static struct dma_async_tx_descriptor
+ *owl_dma_prep_slave_sg(struct dma_chan *chan,
+ struct scatterlist *sgl,
+ unsigned int sg_len,
+ enum dma_transfer_direction dir,
+ unsigned long flags, void *context)
+{
+ struct owl_dma *od = to_owl_dma(chan->device);
+ struct owl_dma_vchan *vchan = to_owl_vchan(chan);
+ struct dma_slave_config *sconfig = &vchan->cfg;
+ struct owl_dma_txd *txd;
+ struct owl_dma_lli *lli, *prev = NULL;
+ struct scatterlist *sg;
+ dma_addr_t addr, src = 0, dst = 0;
+ size_t len;
+ int ret, i;
+
+ txd = kzalloc(sizeof(*txd), GFP_NOWAIT);
+ if (!txd)
+ return NULL;
+
+ INIT_LIST_HEAD(&txd->lli_list);
+
+ for_each_sg(sgl, sg, sg_len, i) {
+ addr = sg_dma_address(sg);
+ len = sg_dma_len(sg);
+
+ if (len > OWL_DMA_FRAME_MAX_LENGTH) {
+ dev_err(od->dma.dev,
+ "frame length exceeds max supported length");
+ goto err_txd_free;
+ }
+
+ lli = owl_dma_alloc_lli(od);
+ if (!lli) {
+ dev_err(chan2dev(chan), "failed to allocate lli");
+ goto err_txd_free;
+ }
+
+ if (dir == DMA_MEM_TO_DEV) {
+ src = addr;
+ dst = sconfig->dst_addr;
+ } else {
+ src = sconfig->src_addr;
+ dst = addr;
+ }
+
+ ret = owl_dma_cfg_lli(vchan, lli, src, dst, len, dir, sconfig,
+ txd->cyclic);
+ if (ret) {
+ dev_warn(chan2dev(chan), "failed to config lli");
+ goto err_txd_free;
+ }
+
+ prev = owl_dma_add_lli(txd, prev, lli, false);
+ }
+
+ return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
+
+err_txd_free:
+ owl_dma_free_txd(od, txd);
+
+ return NULL;
+}
+
+static struct dma_async_tx_descriptor
+ *owl_prep_dma_cyclic(struct dma_chan *chan,
+ dma_addr_t buf_addr, size_t buf_len,
+ size_t period_len,
+ enum dma_transfer_direction dir,
+ unsigned long flags)
+{
+ struct owl_dma *od = to_owl_dma(chan->device);
+ struct owl_dma_vchan *vchan = to_owl_vchan(chan);
+ struct dma_slave_config *sconfig = &vchan->cfg;
+ struct owl_dma_txd *txd;
+ struct owl_dma_lli *lli, *prev = NULL, *first = NULL;
+ dma_addr_t src = 0, dst = 0;
+ unsigned int periods = buf_len / period_len;
+ int ret, i;
+
+ txd = kzalloc(sizeof(*txd), GFP_NOWAIT);
+ if (!txd)
+ return NULL;
+
+ INIT_LIST_HEAD(&txd->lli_list);
+ txd->cyclic = true;
+
+ for (i = 0; i < periods; i++) {
+ lli = owl_dma_alloc_lli(od);
+ if (!lli) {
+ dev_warn(chan2dev(chan), "failed to allocate lli");
+ goto err_txd_free;
+ }
+
+ if (dir == DMA_MEM_TO_DEV) {
+ src = buf_addr + (period_len * i);
+ dst = sconfig->dst_addr;
+ } else if (dir == DMA_DEV_TO_MEM) {
+ src = sconfig->src_addr;
+ dst = buf_addr + (period_len * i);
+ }
+
+ ret = owl_dma_cfg_lli(vchan, lli, src, dst, period_len,
+ dir, sconfig, txd->cyclic);
+ if (ret) {
+ dev_warn(chan2dev(chan), "failed to config lli");
+ goto err_txd_free;
+ }
+
+ if (!first)
+ first = lli;
+
+ prev = owl_dma_add_lli(txd, prev, lli, false);
+ }
+
+ /* close the cyclic list */
+ owl_dma_add_lli(txd, prev, first, true);
+
+ return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
+
+err_txd_free:
+ owl_dma_free_txd(od, txd);
+
+ return NULL;
+}
+
static void owl_dma_free_chan_resources(struct dma_chan *chan)
{
struct owl_dma_vchan *vchan = to_owl_vchan(chan);
@@ -790,6 +1016,27 @@ static inline void owl_dma_free(struct owl_dma *od)
}
}
+static struct dma_chan *owl_dma_of_xlate(struct of_phandle_args *dma_spec,
+ struct of_dma *ofdma)
+{
+ struct owl_dma *od = ofdma->of_dma_data;
+ struct owl_dma_vchan *vchan;
+ struct dma_chan *chan;
+ u8 drq = dma_spec->args[0];
+
+ if (drq > od->nr_vchans)
+ return NULL;
+
+ chan = dma_get_any_slave_channel(&od->dma);
+ if (!chan)
+ return NULL;
+
+ vchan = to_owl_vchan(chan);
+ vchan->drq = drq;
+
+ return chan;
+}
+
static int owl_dma_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
@@ -833,12 +1080,19 @@ static int owl_dma_probe(struct platform_device *pdev)
spin_lock_init(&od->lock);
dma_cap_set(DMA_MEMCPY, od->dma.cap_mask);
+ dma_cap_set(DMA_SLAVE, od->dma.cap_mask);
+ dma_cap_set(DMA_CYCLIC, od->dma.cap_mask);
od->dma.dev = &pdev->dev;
od->dma.device_free_chan_resources = owl_dma_free_chan_resources;
od->dma.device_tx_status = owl_dma_tx_status;
od->dma.device_issue_pending = owl_dma_issue_pending;
od->dma.device_prep_dma_memcpy = owl_dma_prep_memcpy;
+ od->dma.device_prep_slave_sg = owl_dma_prep_slave_sg;
+ od->dma.device_prep_dma_cyclic = owl_prep_dma_cyclic;
+ od->dma.device_config = owl_dma_config;
+ od->dma.device_pause = owl_dma_pause;
+ od->dma.device_resume = owl_dma_resume;
od->dma.device_terminate_all = owl_dma_terminate_all;
od->dma.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
od->dma.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
@@ -910,8 +1164,18 @@ static int owl_dma_probe(struct platform_device *pdev)
goto err_pool_free;
}
+ /* Device-tree DMA controller registration */
+ ret = of_dma_controller_register(pdev->dev.of_node,
+ owl_dma_of_xlate, od);
+ if (ret) {
+ dev_err(&pdev->dev, "of_dma_controller_register failed\n");
+ goto err_dma_unregister;
+ }
+
return 0;
+err_dma_unregister:
+ dma_async_device_unregister(&od->dma);
err_pool_free:
clk_disable_unprepare(od->clk);
dma_pool_destroy(od->lli_pool);
@@ -923,6 +1187,7 @@ static int owl_dma_remove(struct platform_device *pdev)
{
struct owl_dma *od = platform_get_drvdata(pdev);
+ of_dma_controller_free(pdev->dev.of_node);
dma_async_device_unregister(&od->dma);
/* Mask all interrupts for this execution environment */
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/3] arm64: dts: actions: s900: Enable Tx DMA for UART5
From: Manivannan Sadhasivam @ 2018-09-29 7:46 UTC (permalink / raw)
To: vkoul, dan.j.williams, afaerber, robh+dt, gregkh, jslaby
Cc: linux-serial, dmaengine, liuwei, 96boards, devicetree,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-kernel,
hzhang, bdong, manivannanece23, thomas.liau, jeff.chen, pn,
edgar.righi, Manivannan Sadhasivam
In-Reply-To: <20180929074637.9766-1-manivannan.sadhasivam@linaro.org>
Enable Tx DMA for UART5 in Actions Semi S900 SoC.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
arch/arm64/boot/dts/actions/s900.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/actions/s900.dtsi b/arch/arm64/boot/dts/actions/s900.dtsi
index eceba914762c..39af1236f611 100644
--- a/arch/arm64/boot/dts/actions/s900.dtsi
+++ b/arch/arm64/boot/dts/actions/s900.dtsi
@@ -156,6 +156,8 @@
compatible = "actions,s900-uart", "actions,owl-uart";
reg = <0x0 0xe012a000 0x0 0x2000>;
interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>;
+ dma-names = "tx";
+ dmas = <&dma 26>;
status = "disabled";
};
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/3] Add slave DMA support for Actions Semi S900 SoC
From: Manivannan Sadhasivam @ 2018-09-29 7:46 UTC (permalink / raw)
To: vkoul, dan.j.williams, afaerber, robh+dt, gregkh, jslaby
Cc: linux-serial, dmaengine, liuwei, 96boards, devicetree,
daniel.thompson, amit.kucheria, linux-arm-kernel, linux-kernel,
hzhang, bdong, manivannanece23, thomas.liau, jeff.chen, pn,
edgar.righi, Manivannan Sadhasivam
This patchset adds slave DMA support for Actions Semi S900 SoC of the
Owl family. As a consumer, enable TX DMA support for UART peripheral
in S900. The UART driver still supports interrupt mode if there is no
DMA property specified in DT.
The dts patch depends on the previous DMA patches which is not yet
merged.
Thanks,
Mani
Changes in v2:
* Modified the comment for bus width as per Vinod's suggestion
Manivannan Sadhasivam (3):
arm64: dts: actions: s900: Enable Tx DMA for UART5
dmaengine: Add Slave and Cyclic mode support for Actions Semi Owl S900
SoC
tty: serial: Add Tx DMA support for UART in Actions Semi Owl SoCs
arch/arm64/boot/dts/actions/s900.dtsi | 2 +
drivers/dma/owl-dma.c | 279 +++++++++++++++++++++++++-
drivers/tty/serial/owl-uart.c | 172 +++++++++++++++-
3 files changed, 445 insertions(+), 8 deletions(-)
--
2.17.1
^ permalink raw reply
* [BUG] sleep in atomic in 8250 runtime PM code path
From: Jisheng Zhang @ 2018-09-29 5:20 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, Greg Kroah-Hartman, Phil Edworthy
Cc: linux-serial, linux-kernel, linux-arm-kernel
Hi,
Recently I found I could trigger sleep in atomic bug on berlin after commit
d76c74387e1c ("serial: 8250_dw: Fix runtime PM handling"). The path looks like:
dw8250_probe => serial850_register_8250_port => uart_add_one_port=>
register_console => console_unlock => univ8250_console_write =>
serial8250_console_write => serial8250_rpm_get => pm_runtime_get_sync
The irq is disabled by printk_safe_enter_irqsave() in console_unlock, but
pm_runtime_get_sync can't be called in atomic context...
I guess the reason why we didn't notice it is due to the fact that
only OMAP and DW sets UART_CAP_RPM currently, and DW set the flag in
May 2018.
Per my understanding, the bug sits in the 8250 core driver rather than
8250_dw.c.
Thanks
^ permalink raw reply
* Re: [PATCH] tty: Convert to using %pOFn instead of device_node.name
From: Rob Herring @ 2018-09-28 22:32 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, Greg Kroah-Hartman
Cc: Jiri Slaby, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, open list:SERIAL DRIVERS, linuxppc-dev
In-Reply-To: <CAL_JsqJfAsv-MOgk2Ycs2VBj2vK0m29B_3FxwJV5xUjnMbH4sg@mail.gmail.com>
On Fri, Sep 28, 2018 at 5:09 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Aug 27, 2018 at 8:55 PM Rob Herring <robh@kernel.org> wrote:
> >
> > In preparation to remove the node name pointer from struct device_node,
> > convert printf users to use the %pOFn format specifier.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: linux-serial@vger.kernel.org
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > drivers/tty/ehv_bytechan.c | 12 ++++++------
> > drivers/tty/serial/cpm_uart/cpm_uart_core.c | 8 ++++----
> > drivers/tty/serial/pmac_zilog.c | 4 ++--
> > 3 files changed, 12 insertions(+), 12 deletions(-)
>
> Hey Greg, Is this still in your queue? Maybe you've just been extra
> busy lately. ;)
NM. I see it's applied now. Sorry for the noise.
Rob
^ permalink raw reply
* Re: [PATCH] tty: Convert to using %pOFn instead of device_node.name
From: Rob Herring @ 2018-09-28 22:09 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org, Greg Kroah-Hartman
Cc: Jiri Slaby, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, open list:SERIAL DRIVERS, linuxppc-dev
In-Reply-To: <20180828015252.28511-46-robh@kernel.org>
On Mon, Aug 27, 2018 at 8:55 PM Rob Herring <robh@kernel.org> wrote:
>
> In preparation to remove the node name pointer from struct device_node,
> convert printf users to use the %pOFn format specifier.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linux-serial@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> drivers/tty/ehv_bytechan.c | 12 ++++++------
> drivers/tty/serial/cpm_uart/cpm_uart_core.c | 8 ++++----
> drivers/tty/serial/pmac_zilog.c | 4 ++--
> 3 files changed, 12 insertions(+), 12 deletions(-)
Hey Greg, Is this still in your queue? Maybe you've just been extra
busy lately. ;)
Rob
>
> diff --git a/drivers/tty/ehv_bytechan.c b/drivers/tty/ehv_bytechan.c
> index eea4049b5dcc..769e0a5d1dfc 100644
> --- a/drivers/tty/ehv_bytechan.c
> +++ b/drivers/tty/ehv_bytechan.c
> @@ -128,8 +128,8 @@ static int find_console_handle(void)
> */
> iprop = of_get_property(np, "hv-handle", NULL);
> if (!iprop) {
> - pr_err("ehv-bc: no 'hv-handle' property in %s node\n",
> - np->name);
> + pr_err("ehv-bc: no 'hv-handle' property in %pOFn node\n",
> + np);
> return 0;
> }
> stdout_bc = be32_to_cpu(*iprop);
> @@ -661,8 +661,8 @@ static int ehv_bc_tty_probe(struct platform_device *pdev)
>
> iprop = of_get_property(np, "hv-handle", NULL);
> if (!iprop) {
> - dev_err(&pdev->dev, "no 'hv-handle' property in %s node\n",
> - np->name);
> + dev_err(&pdev->dev, "no 'hv-handle' property in %pOFn node\n",
> + np);
> return -ENODEV;
> }
>
> @@ -682,8 +682,8 @@ static int ehv_bc_tty_probe(struct platform_device *pdev)
> bc->rx_irq = irq_of_parse_and_map(np, 0);
> bc->tx_irq = irq_of_parse_and_map(np, 1);
> if ((bc->rx_irq == NO_IRQ) || (bc->tx_irq == NO_IRQ)) {
> - dev_err(&pdev->dev, "no 'interrupts' property in %s node\n",
> - np->name);
> + dev_err(&pdev->dev, "no 'interrupts' property in %pOFn node\n",
> + np);
> ret = -ENODEV;
> goto error;
> }
> diff --git a/drivers/tty/serial/cpm_uart/cpm_uart_core.c b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> index 24a5f05e769b..ea7204d75022 100644
> --- a/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/tty/serial/cpm_uart/cpm_uart_core.c
> @@ -1151,8 +1151,8 @@ static int cpm_uart_init_port(struct device_node *np,
> if (!pinfo->clk) {
> data = of_get_property(np, "fsl,cpm-brg", &len);
> if (!data || len != 4) {
> - printk(KERN_ERR "CPM UART %s has no/invalid "
> - "fsl,cpm-brg property.\n", np->name);
> + printk(KERN_ERR "CPM UART %pOFn has no/invalid "
> + "fsl,cpm-brg property.\n", np);
> return -EINVAL;
> }
> pinfo->brg = *data;
> @@ -1160,8 +1160,8 @@ static int cpm_uart_init_port(struct device_node *np,
>
> data = of_get_property(np, "fsl,cpm-command", &len);
> if (!data || len != 4) {
> - printk(KERN_ERR "CPM UART %s has no/invalid "
> - "fsl,cpm-command property.\n", np->name);
> + printk(KERN_ERR "CPM UART %pOFn has no/invalid "
> + "fsl,cpm-command property.\n", np);
> return -EINVAL;
> }
> pinfo->command = *data;
> diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
> index 3d21790d961e..a4ec22d1f214 100644
> --- a/drivers/tty/serial/pmac_zilog.c
> +++ b/drivers/tty/serial/pmac_zilog.c
> @@ -1566,9 +1566,9 @@ static int pmz_attach(struct macio_dev *mdev, const struct of_device_id *match)
> * to work around bugs in ancient Apple device-trees
> */
> if (macio_request_resources(uap->dev, "pmac_zilog"))
> - printk(KERN_WARNING "%s: Failed to request resource"
> + printk(KERN_WARNING "%pOFn: Failed to request resource"
> ", port still active\n",
> - uap->node->name);
> + uap->node);
> else
> uap->flags |= PMACZILOG_FLAG_RSRC_REQUESTED;
>
> --
> 2.17.1
>
^ permalink raw reply
* Re: [SPAM][PATCH 2/4] dmaengine: mtk_uart_dma: add Mediatek uart DMA support
From: Sean Wang @ 2018-09-28 21:44 UTC (permalink / raw)
To: Long Cheng
Cc: Mark Rutland, devicetree, srv_heupstream, Greg Kroah-Hartman,
linux-kernel, Matthias Brugger, Vinod Koul, Rob Herring,
linux-mediatek, linux-serial, Jiri Slaby, dmaengine, Yingjoe Chen,
Dan Williams, Ed Blake, linux-arm-kernel
In-Reply-To: <1537425673-18807-3-git-send-email-long.cheng@mediatek.com>
Hi,
On Thu, 2018-09-20 at 14:41 +0800, Long Cheng wrote:
> In DMA engine framework, add 8250 mtk dma to support it.
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> drivers/dma/8250_mtk_dma.c | 1049 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/dma/Kconfig | 11 +
> drivers/dma/Makefile | 1 +
the driver should be moved to driver/dma/mediatek
> 3 files changed, 1061 insertions(+)
> create mode 100644 drivers/dma/8250_mtk_dma.c
>
> diff --git a/drivers/dma/8250_mtk_dma.c b/drivers/dma/8250_mtk_dma.c
> new file mode 100644
> index 0000000..a07844e
> --- /dev/null
> +++ b/drivers/dma/8250_mtk_dma.c
> @@ -0,0 +1,1049 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Mediatek 8250 DMA driver.
> + *
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Long Cheng <long.cheng@mediatek.com>
> + */
> +
> +#define pr_fmt(fmt) "8250-mtk-dma: " fmt
pr_fmt can be removed since no place is called with pr_fmt
> +#define DRV_NAME "8250-mtk-dma"
> +
use KBUILD_MODNAME instead
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of_dma.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "virt-dma.h"
> +
> +#define MTK_SDMA_REQUESTS 127
name it as MTK_SDMA_DEFAULT_REQUESTS seems to be more readable
> +#define MTK_SDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
total number of the channels should depend on the capability the hardware provides, it should be detected by the device tree
> +
> +#define VFF_RX_INT_FLAG_CLR_B (BIT(0U) | BIT(1U))
> +#define VFF_TX_INT_FLAG_CLR_B 0
> +#define VFF_RX_INT_EN0_B BIT(0U) /*rx valid size >= vff thre */
> +#define VFF_RX_INT_EN1_B BIT(1U)
> +#define VFF_TX_INT_EN_B BIT(0U) /*tx left size >= vff thre */
> +#define VFF_INT_EN_CLR_B 0
> +#define VFF_WARM_RST_B BIT(0U)
> +#define VFF_EN_B BIT(0U)
> +#define VFF_STOP_B BIT(0U)
> +#define VFF_STOP_CLR_B 0
> +#define VFF_FLUSH_B BIT(0U)
> +#define VFF_FLUSH_CLR_B 0
> +#define VFF_4G_SUPPORT_B BIT(0U)
> +#define VFF_4G_SUPPORT_CLR_B 0
> +
all postfix U can be removed, and register offset placed along with the bit definition would be good to read
> +/* interrupt trigger level for tx */
> +#define VFF_TX_THRE(n) ((n) * 7 / 8)
> +/* interrupt trigger level for rx */
> +#define VFF_RX_THRE(n) ((n) * 3 / 4)
> +
> +#define MTK_DMA_RING_SIZE 0xffffU
> +/* invert this bit when wrap ring head again*/
> +#define MTK_DMA_RING_WRAP 0x10000U
> +
> +struct mtk_dmadev {
> + struct dma_device ddev;
> + void __iomem *mem_base[MTK_SDMA_CHANNELS];
> + spinlock_t lock; /* dma dev lock */
> + struct tasklet_struct task;
tasklet should be removed in general in order to allow the task as soon as possible to be process
you could refer to mtk_hsdma.c I made in drivers/dma/mediatek/ first
> + struct list_head pending;
> + struct clk *clk;
> + unsigned int dma_requests;
> + bool support_33bits;
> + unsigned int dma_irq[MTK_SDMA_CHANNELS];
> + struct mtk_chan *lch_map[MTK_SDMA_CHANNELS];
why is the map required? do you offer any way for that the channel would be assigned or managed on the runtime?
> +};
> +
> +struct mtk_chan {
> + struct virt_dma_chan vc;
> + struct list_head node;
> + struct dma_slave_config cfg;
> + void __iomem *channel_base;
channel_base can be renamed to base since the member is located at mtk_chan
> + struct mtk_dma_desc *desc;
> +
> + bool paused;
I don't think the variable paused since it doesn't support pause function
> + bool requested;
> +
> + unsigned int dma_sig;
> + unsigned int dma_ch;
> + unsigned int sgidx;
> + unsigned int remain_size;
> + unsigned int rx_ptr;
> +
> + /*sync*/
> + struct completion done; /* dma transfer done */
> + spinlock_t lock; /* channel lock */
> + atomic_t loopcnt;
> + atomic_t entry; /* entry count */
Why the two atomic variables is introduced? they make the whole logic flow a little bit hard to understand.
In general, the generic dma_virtual_channels can support the most dma engines for how descriptors on each channel being managed.
You should check it first how list vc->desc_allocated, vc->desc_submitted and vc->desc_issued being maintained and really think if
it's also suitable to the dmaengine. You could refer to mtk-hsdma.c I made in drivers/dma/mediatek/ first.
The driver totally reuses these vc lists..
> +};
> +
> +struct mtk_dma_sg {
> + dma_addr_t addr;
> + unsigned int en; /* number of elements (24-bit) */
> + unsigned int fn; /* number of frames (16-bit) */
> +};
> +
> +struct mtk_dma_desc {
> + struct virt_dma_desc vd;
> + enum dma_transfer_direction dir;
> + dma_addr_t dev_addr;
> +
I don't see dir, dev_addr being used in code flow? or I missed something ?
> + unsigned int sglen;
> + struct mtk_dma_sg sg[0];
> +};
> +
> +enum {
> + VFF_INT_FLAG = 0x00,
> + VFF_INT_EN = 0x04,
> + VFF_EN = 0x08,
> + VFF_RST = 0x0c,
> + VFF_STOP = 0x10,
> + VFF_FLUSH = 0x14,
> + VFF_ADDR = 0x1c,
> + VFF_LEN = 0x24,
> + VFF_THRE = 0x28,
> + VFF_WPT = 0x2c,
> + VFF_RPT = 0x30,
> + /*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
> + VFF_VALID_SIZE = 0x3c,
> + /*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
> + VFF_LEFT_SIZE = 0x40,
> + VFF_DEBUG_STATUS = 0x50,
> + VFF_4G_SUPPORT = 0x54,
> +};
> +
Add register definition with #define and move them near the bit field definition.
> +static bool mtk_dma_filter_fn(struct dma_chan *chan, void *param);
> +static struct of_dma_filter_info mtk_dma_info = {
> + .filter_fn = mtk_dma_filter_fn,
> +};
> +
> +static inline struct mtk_dmadev *to_mtk_dma_dev(struct dma_device *d)
> +{
> + return container_of(d, struct mtk_dmadev, ddev);
> +}
> +
> +static inline struct mtk_chan *to_mtk_dma_chan(struct dma_chan *c)
> +{
> + return container_of(c, struct mtk_chan, vc.chan);
> +}
> +
> +static inline struct mtk_dma_desc *to_mtk_dma_desc
> + (struct dma_async_tx_descriptor *t)
> +{
> + return container_of(t, struct mtk_dma_desc, vd.tx);
> +}
> +
> +static void mtk_dma_chan_write(struct mtk_chan *c,
> + unsigned int reg, unsigned int val)
> +{
> + writel(val, c->channel_base + reg);
> +}
> +
> +static unsigned int mtk_dma_chan_read(struct mtk_chan *c, unsigned int reg)
> +{
> + return readl(c->channel_base + reg);
> +}
> +
> +static void mtk_dma_desc_free(struct virt_dma_desc *vd)
> +{
> + struct dma_chan *chan = vd->tx.chan;
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->desc && c->cfg.direction == DMA_DEV_TO_MEM)
> + atomic_dec(&c->entry);
> +
> + kfree(c->desc);
> + c->desc = NULL;
> + spin_unlock_irqrestore(&c->vc.lock, flags);
The lock should not be necessary and try to make *_desc_free be as simple as possible.
In general, the function is being only to free specific memory about the vd. It should be done by
kfree(container_of(vd, struct mtk_apdma_vdesc, vd)); And don't need to care what detail the channel and direction is.
> +}
> +
> +static int mtk_dma_clk_enable(struct mtk_dmadev *mtkd)
> +{
> + int ret;
> +
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret) {
> + dev_err(mtkd->ddev.dev, "Couldn't enable the clock\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void mtk_dma_clk_disable(struct mtk_dmadev *mtkd)
> +{
> + clk_disable_unprepare(mtkd->clk);
> +}
> +
> +static void mtk_dma_remove_virt_list(dma_cookie_t cookie,
> + struct virt_dma_chan *vc)
> +{
> + struct virt_dma_desc *vd;
> +
> + if (list_empty(&vc->desc_issued) == 0) {
> + list_for_each_entry(vd, &vc->desc_issued, node) {
> + if (cookie == vd->tx.cookie) {
> + INIT_LIST_HEAD(&vc->desc_issued);
Why force to reinit list desc_issued? that is enqueued by core layer. They are expected to be configured into the hardware by the driver in sequence
and then dequeued from the list when the descriptor is totally setting done by the driver or completed by the hardware.
> + break;
> + }
> + }
> + }
> +}
> +
> +static void mtk_dma_tx_flush(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> +
> + if (mtk_dma_chan_read(c, VFF_FLUSH) == 0U) {
> + mtk_dma_chan_write(c, VFF_FLUSH, VFF_FLUSH_B);
> + if (atomic_dec_and_test(&c->loopcnt))
I really think depending on atomic complete a synchronization is a tricky thing. Can we have another elegant way to let synchronization become good to read and easy to maintain?
Or you could refer to mtk_hsdma.c I made in drivers/dma/mediatek/ first, the similar synchronization is also being done here.
> + complete(&c->done);
> + }
> +}
> +
> +/*
> + * check whether the dma flush operation is finished or not.
> + * return 0 for flush success.
> + * return others for flush timeout.
> + */
> +static int mtk_dma_check_flush_result(struct dma_chan *chan)
> +{
> + struct timespec start, end;
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> +
> + start = ktime_to_timespec(ktime_get());
> +
> + while ((mtk_dma_chan_read(c, VFF_FLUSH) & VFF_FLUSH_B) == VFF_FLUSH_B) {
> + end = ktime_to_timespec(ktime_get());
> + if ((end.tv_sec - start.tv_sec) > 1 ||
> + ((end.tv_sec - start.tv_sec) == 1 &&
> + end.tv_nsec > start.tv_nsec)) {
> + dev_err(chan->device->dev,
> + "[DMA] Polling flush timeout\n");
> + return -1;
You can check readx_poll_timeout and related APIs instead of the open coding.
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void mtk_dma_tx_write(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> + unsigned int txcount = c->remain_size;
> + unsigned int len, send, left, wpt, wrap;
> +
> + if (atomic_inc_return(&c->entry) > 1) {
atomic variable again. Can you explain more about what the branches (if and else) are doing for ? Let's see if they can be replaced by more elegant ways
> + if (vchan_issue_pending(&c->vc) && !c->desc) {
> + spin_lock(&mtkd->lock);
> + list_add_tail(&c->node, &mtkd->pending);
> + spin_unlock(&mtkd->lock);
> + tasklet_schedule(&mtkd->task);
> + }
> + } else {
> + len = mtk_dma_chan_read(c, VFF_LEN);
> + if (mtk_dma_check_flush_result(chan) != 0)
> + return;
> +
> + while ((left = mtk_dma_chan_read(c, VFF_LEFT_SIZE)) > 0U) {
> + send = min(left, c->remain_size);
> + wpt = mtk_dma_chan_read(c, VFF_WPT);
> + wrap = wpt & MTK_DMA_RING_WRAP ? 0U : MTK_DMA_RING_WRAP;
> +
> + if ((wpt & (len - 1U)) + send < len)
> + mtk_dma_chan_write(c, VFF_WPT, wpt + send);
> + else
> + mtk_dma_chan_write(c, VFF_WPT,
> + ((wpt + send) & (len - 1U))
> + | wrap);
> +
> + c->remain_size -= send;
> + if (c->remain_size == 0U)
the if condition can be moved to the the beginning while condition
> + break;
> + }
> +
> + if (txcount != c->remain_size) {
> + mtk_dma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
why need to enable interrupt to trigger the next?
> + mtk_dma_tx_flush(chan);
> + }
> + }
> + atomic_dec(&c->entry);
> +}
> +
> +static void mtk_dma_start_tx(struct mtk_chan *c)
> +{
> + if (mtk_dma_chan_read(c, VFF_LEFT_SIZE) == 0U) {
> + pr_info("%s maybe need fix? @L %d\n", __func__, __LINE__);
the debug message can be removed
> + mtk_dma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
what is the step for?
> + } else {
> + reinit_completion(&c->done);
> +
> + /* inc twice, once for tx_flush, another for tx_interrupt */
> + atomic_inc(&c->loopcnt);
> + atomic_inc(&c->loopcnt);
if you have two events for which some point wants to wait, why not create two two completions for them?
> + mtk_dma_tx_write(&c->vc.chan);
> + }
> + c->paused = false;
> +}
> +
> +static void mtk_dma_get_rx_size(struct mtk_chan *c)
> +{
> + unsigned int count;
> + unsigned int rdptr, wrptr, wrreg, rdreg;
> + unsigned int rx_size = mtk_dma_chan_read(c, VFF_LEN);
> +
Sort all declarations in reversed Xmas tree, even along with all occurrences in the other functions
> + rdreg = mtk_dma_chan_read(c, VFF_RPT);
> + wrreg = mtk_dma_chan_read(c, VFF_WPT);
> + rdptr = rdreg & MTK_DMA_RING_SIZE;
> + wrptr = wrreg & MTK_DMA_RING_SIZE;
> + count = ((rdreg ^ wrreg) & MTK_DMA_RING_WRAP) ?
> + (wrptr + rx_size - rdptr) : (wrptr - rdptr);
> +
> + c->remain_size = count;
> + c->rx_ptr = rdptr;
> +
> + mtk_dma_chan_write(c, VFF_RPT, wrreg);
> +}
> +
> +static void mtk_dma_start_rx(struct mtk_chan *c)
> +{
> + struct dma_chan *chan = &c->vc.chan;
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> + struct mtk_dma_desc *d = c->desc;
> +
> + if (mtk_dma_chan_read(c, VFF_VALID_SIZE) != 0U &&
> + d && d->vd.tx.cookie != 0) {
> + mtk_dma_get_rx_size(c);
> + mtk_dma_remove_virt_list(d->vd.tx.cookie, &c->vc);
> + vchan_cookie_complete(&d->vd);
please make vchan_cookie_complete in a completion handler such as a completion ISR, not in a start handler.
> + } else {
> + if (mtk_dma_chan_read(c, VFF_VALID_SIZE) != 0U) {
> + spin_lock(&mtkd->lock);
> + if (list_empty(&mtkd->pending))
> + list_add_tail(&c->node, &mtkd->pending);
> + spin_unlock(&mtkd->lock);
> + tasklet_schedule(&mtkd->task);
> + } else {
> + if (atomic_read(&c->entry) > 0)
> + atomic_set(&c->entry, 0);
I am not not clear about the magical reset. could you explain more?
> + }
> + }
> +}
> +
> +static void mtk_dma_reset(struct mtk_chan *c)
> +{
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> +
> + mtk_dma_chan_write(c, VFF_ADDR, 0);
> + mtk_dma_chan_write(c, VFF_THRE, 0);
> + mtk_dma_chan_write(c, VFF_LEN, 0);
> + mtk_dma_chan_write(c, VFF_RST, VFF_WARM_RST_B);
> +
> + while
> + (mtk_dma_chan_read(c, VFF_EN));
add a timeout, otherwise it would get a hang possibly. you can check readx_poll_timeout and related APIs to add a timeout.
> +
> + if (c->cfg.direction == DMA_DEV_TO_MEM)
> + mtk_dma_chan_write(c, VFF_RPT, 0);
> + else if (c->cfg.direction == DMA_MEM_TO_DEV)
> + mtk_dma_chan_write(c, VFF_WPT, 0);
> + else
> + dev_info(c->vc.chan.device->dev, "Unknown direction.\n");
is it possible to happen?
> +
> + if (mtkd->support_33bits)
> + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> +}
> +
> +static void mtk_dma_stop(struct mtk_chan *c)
> +{
> + int polling_cnt;
> +
> + mtk_dma_chan_write(c, VFF_FLUSH, VFF_FLUSH_CLR_B);
> +
> + polling_cnt = 0;
> + while ((mtk_dma_chan_read(c, VFF_FLUSH) & VFF_FLUSH_B) ==
> + VFF_FLUSH_B) {
> + polling_cnt++;
> + if (polling_cnt > 10000) {
> + dev_err(c->vc.chan.device->dev,
> + "dma stop: polling FLUSH fail, DEBUG=0x%x\n",
> + mtk_dma_chan_read(c, VFF_DEBUG_STATUS));
> + break;
> + }
> + }
You can check readx_poll_timeout and related APIs to make the fucntion more readable
> +
> + polling_cnt = 0;
> + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> + mtk_dma_chan_write(c, VFF_STOP, VFF_STOP_B);
> + while (mtk_dma_chan_read(c, VFF_EN)) {
> + polling_cnt++;
> + if (polling_cnt > 10000) {
> + dev_err(c->vc.chan.device->dev,
> + "dma stop: polling VFF_EN fail, DEBUG=0x%x\n",
> + mtk_dma_chan_read(c, VFF_DEBUG_STATUS));
> + break;
> + }
> + }
You can check readx_poll_timeout and related APIs to make the function more readable.
> + mtk_dma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
> + mtk_dma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> +
> + if (c->cfg.direction == DMA_DEV_TO_MEM)
> + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + else
> + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> +
> + c->paused = true;
It's a stop, not a pause
> +}
> +
> +/*
> + * We need to deal with 'all channels in-use'
> + */
> +static void mtk_dma_rx_sched(struct mtk_chan *c)
> +{
> + struct dma_chan *chan = &c->vc.chan;
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> +
> + if (atomic_read(&c->entry) < 1) {
> + mtk_dma_start_rx(c);
> + } else {
> + spin_lock(&mtkd->lock);
> + if (list_empty(&mtkd->pending))
> + list_add_tail(&c->node, &mtkd->pending);
> + spin_unlock(&mtkd->lock);
> + tasklet_schedule(&mtkd->task);
I'm not clear why mixing start rx and tasklet rx here. In general, please make all tx/rx job as soon as poosible.
you can refer to the mtk_hsdma.c I made.
> + }
> +}
> +
> +/*
> + * This callback schedules all pending channels. We could be more
> + * clever here by postponing allocation of the real DMA channels to
> + * this point, and freeing them when our virtual channel becomes idle.
> + *
> + * We would then need to deal with 'all channels in-use'
> + */
> +static void mtk_dma_sched(unsigned long data)
> +{
> + struct mtk_dmadev *mtkd = (struct mtk_dmadev *)data;
> + struct mtk_chan *c;
> + struct virt_dma_desc *vd;
> + dma_cookie_t cookie;
> + LIST_HEAD(head);
> + unsigned long flags;
> +
> + spin_lock_irq(&mtkd->lock);
> + list_splice_tail_init(&mtkd->pending, &head);
> + spin_unlock_irq(&mtkd->lock);
> +
> + if (list_empty(&head) == 0) {
!list_empty
> + c = list_first_entry(&head, struct mtk_chan, node);
> + cookie = c->vc.chan.cookie;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
head is a local list so the lock is not neccessary
> + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> + list_del_init(&c->node);
why not use list_dec version
> + mtk_dma_rx_sched(c);
> + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> + vd = vchan_find_desc(&c->vc, cookie);
> +
> + c->desc = to_mtk_dma_desc(&vd->tx);
> + list_del_init(&c->node);
> + mtk_dma_start_tx(c);
why is direct call for tx and tasklet for rx?
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> + }
> +}
> +
> +static int mtk_dma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + int ret = -EBUSY;
> +
> + pm_runtime_get_sync(mtkd->ddev.dev);
> +
> + if (!mtkd->lch_map[c->dma_ch]) {
> + c->channel_base = mtkd->mem_base[c->dma_ch];
> + mtkd->lch_map[c->dma_ch] = c;
keep lch_map in mtkd is not necessary. instead, I think we can keep all information required by the physical channel inside mtk_chan, not exported to the whole dma engine.
> + ret = 1;
why returning 1 here?
> + }
> + c->requested = false;
> + mtk_dma_reset(c);
> +
> + return ret;
> +}
> +
> +static void mtk_dma_free_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> +
> + if (c->requested) {
> + c->requested = false;
> + free_irq(mtkd->dma_irq[c->dma_ch], chan);
> + }
> +
> + tasklet_kill(&mtkd->task);
> +
> + c->channel_base = NULL;
> + mtkd->lch_map[c->dma_ch] = NULL;
> + vchan_free_chan_resources(&c->vc);
I think we should call mtk_dma_terminate first to free all descriptors and then free the other channel resouce in the function.
> +
> + dev_dbg(mtkd->ddev.dev, "freeing channel for %u\n", c->dma_sig);
> + c->dma_sig = 0;
> +
> + pm_runtime_put_sync(mtkd->ddev.dev);
> +}
> +
> +static enum dma_status mtk_dma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + enum dma_status ret;
> + unsigned long flags;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
consider txstate null case and directly return when the descriptor is complete, such as
ret = dma_cookie_status(c, cookie, txstate);
if (ret == DMA_COMPLETE || !txstate)
return ret;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (ret == DMA_IN_PROGRESS) {
> + c->rx_ptr = mtk_dma_chan_read(c, VFF_RPT) & MTK_DMA_RING_SIZE;
> + txstate->residue = c->rx_ptr;
are you sure all descriptors DMA_IN_PROCESS all be processed by hardware ?
> + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> + txstate->residue = c->remain_size;
why is residue not zero when dma is complete ?
> + } else {
> + txstate->residue = 0;
> + }
setup ->residue by dma_set_residue
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
spin_lock can be dropped
> + return ret;
> +}
> +
> +static unsigned int mtk_dma_desc_size(struct mtk_dma_desc *d)
> +{
> + struct mtk_dma_sg *sg;
> + unsigned int i;
> + unsigned int size;
> +
> + for (size = i = 0; i < d->sglen; i++) {
> + sg = &d->sg[i];
> + size += sg->en * sg->fn;
> + }
> + return size;
drop the function definition, see the below reason.
> +}
> +
> +static struct dma_async_tx_descriptor *mtk_dma_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_dma_chan(chan);
> + struct scatterlist *sgent;
> + struct mtk_dma_desc *d;
> + dma_addr_t dev_addr;
> + unsigned int i, j, en, frame_bytes;
save the variable frame_bytes an en, it's always kept as 1 during the function call
> +
> + en = 1;
> + frame_bytes = 1;
> +
> + if (dir == DMA_DEV_TO_MEM) {
> + dev_addr = c->cfg.src_addr;
> + } else if (dir == DMA_MEM_TO_DEV) {
> + dev_addr = c->cfg.dst_addr;
> + } else {
> + dev_err(chan->device->dev, "bad direction\n");
the case seems never happens because the device registers the capability it can support to.
> + return NULL;
> + }
> +
> + /* Now allocate and setup the descriptor. */
> + d = kzalloc(sizeof(*d) + sglen * sizeof(d->sg[0]), GFP_ATOMIC);
> + if (!d)
> + return NULL;
> +
> + d->dir = dir;
> + d->dev_addr = dev_addr;
when and where is the d->dev_addr being used?
> +
> + j = 0;
> + for_each_sg(sgl, sgent, sglen, i) {
> + d->sg[j].addr = sg_dma_address(sgent);
> + d->sg[j].en = en;
> + d->sg[j].fn = sg_dma_len(sgent) / frame_bytes;
> + j++;
> + }
> +
> + d->sglen = j;
> +
> + if (dir == DMA_MEM_TO_DEV)
> + c->remain_size = mtk_dma_desc_size(d);
function mtk_dma_desc_size can be dropped since the function is only used once and the result can be accumulated by for_each_sg
> + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> +}
> +
> +static void mtk_dma_issue_pending(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + struct mtk_dmadev *mtkd;
> + struct virt_dma_desc *vd;
> + dma_cookie_t cookie;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> + cookie = c->vc.chan.cookie;
> + mtkd = to_mtk_dma_dev(chan->device);
> + if (vchan_issue_pending(&c->vc) && !c->desc) {
> + vd = vchan_find_desc(&c->vc, cookie);
> + c->desc = to_mtk_dma_desc(&vd->tx);
> + if (atomic_read(&c->entry) > 0)
> + atomic_set(&c->entry, 0);
> + }
> + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> + cookie = c->vc.chan.cookie;
> + if (vchan_issue_pending(&c->vc) && !c->desc) {
> + vd = vchan_find_desc(&c->vc, cookie);
> + c->desc = to_mtk_dma_desc(&vd->tx);
> + mtk_dma_start_tx(c);
> + }
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags)
in general, we have to dump all vc->desc_issued into the hw as soon as possible. how is the other descriptors except for the cookie stands for ?
> ;
> +}
> +
> +static irqreturn_t mtk_dma_rx_interrupt(int irq, void *dev_id)
> +{
> + struct dma_chan *chan = (struct dma_chan *)dev_id;
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> +
> + if (atomic_inc_return(&c->entry) > 1) {
> + if (list_empty(&mtkd->pending))
> + list_add_tail(&c->node, &mtkd->pending);
> + tasklet_schedule(&mtkd->task);
> + } else {
> + mtk_dma_start_rx(c);
> + }
what's the reason making rx into direct call or tasklet in some case?
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mtk_dma_tx_interrupt(int irq, void *dev_id)
> +{
> + struct dma_chan *chan = (struct dma_chan *)dev_id;
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> + struct mtk_dma_desc *d = c->desc;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->remain_size != 0U) {
> + list_add_tail(&c->node, &mtkd->pending);
> + tasklet_schedule(&mtkd->task);
> + } else {
> + mtk_dma_remove_virt_list(d->vd.tx.cookie, &c->vc);
> + vchan_cookie_complete(&d->vd);
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> + if (atomic_dec_and_test(&c->loopcnt))
> + complete(&c->done);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mtk_dma_slave_config(struct dma_chan *chan,
> + struct dma_slave_config *cfg)
> +{
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> + int ret;
> +
> + c->cfg = *cfg;
> +
> + if (cfg->direction == DMA_DEV_TO_MEM) {
> + unsigned int rx_len = cfg->src_addr_width * 1024;
the dma eingine only supports DMA_SLAVE_BUSWIDTH_1_BYTE, why do we need to consider any other src_addr_width here ?
> +
> + mtk_dma_chan_write(c, VFF_ADDR, cfg->src_addr);
> + mtk_dma_chan_write(c, VFF_LEN, rx_len);
> + mtk_dma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> + mtk_dma_chan_write(c,
> + VFF_INT_EN, VFF_RX_INT_EN0_B
> + | VFF_RX_INT_EN1_B);
> + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> +
> + if (!c->requested) {
> + atomic_set(&c->entry, 0);
> + c->requested = true;
> + ret = request_irq(mtkd->dma_irq[c->dma_ch],
> + mtk_dma_rx_interrupt,
> + IRQF_TRIGGER_NONE,
> +
move the request_irq to driver probe stage, it can be manage by the core such irq affinity
> DRV_NAME, chan);
> + if (ret < 0) {
> + dev_err(chan->device->dev, "Can't request rx dma IRQ\n");
> + return -EINVAL;
> + }
> + }
> + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> + unsigned int tx_len = cfg->dst_addr_width * 1024;
> +
the dma eingine only supports DMA_SLAVE_BUSWIDTH_1_BYTE, why do we need to consider any other src_addr_width here ?
> + mtk_dma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> + mtk_dma_chan_write(c, VFF_LEN, tx_len);
> + mtk_dma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> +
> + if (!c->requested) {
> + c->requested = true;
> + ret = request_irq(mtkd->dma_irq[c->dma_ch],
> + mtk_dma_tx_interrupt,
> + IRQF_TRIGGER_NONE,
> + DRV_NAME, chan);
move the request_irq to driver probe stage, it can be manage by the core such irq affinity
> + if (ret < 0) {
> + dev_err(chan->device->dev, "Can't request tx dma IRQ\n");
> + return -EINVAL;
> + }
> + }
> + } else {
> + dev_info(chan->device->dev, "Unknown direction!\n");
remove the the unnecessary log
> + }
> +
> + if (mtkd->support_33bits)
> + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> +
> + if (mtk_dma_chan_read(c, VFF_EN) != VFF_EN_B) {
when is the condition being satisfied ?
> + dev_err(chan->device->dev,
> + "config dma dir[%d] fail\n", cfg->direction);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int mtk_dma_terminate_all(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + unsigned long flags;
> + LIST_HEAD(head);
> +
> + if (atomic_read(&c->loopcnt) != 0)
> + wait_for_completion(&c->done);
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->desc) {
> + mtk_dma_remove_virt_list(c->desc->vd.tx.cookie, &c->vc);
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + mtk_dma_desc_free(&c->desc->vd);
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (!c->paused) {
> + list_del_init(&c->node);
> + mtk_dma_stop(c);
I think split logic between descriptors recycle and channel stop would
be good.
> + }
> + }
> + vchan_get_all_descriptors(&c->vc, &head);
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + vchan_dma_desc_free_list(&c->vc, &head);
Can you tell more about the situation for vc->desc_allocated, vc->desc_submitted and vc->desc_issued when the function is being called ?
Which list do the descriptors hardware is processing stay on?
> +
> + return 0;
> +}
> +
> +static int mtk_dma_device_pause(struct dma_chan *chan)
> +{
> + /* Pause/Resume only allowed with cyclic mode */
> + return -EINVAL;
> +}
I guess we don't nodde to register the dumb pause handler
> +
> +static int mtk_dma_device_resume(struct dma_chan *chan)
> +{
> + /* Pause/Resume only allowed with cyclic mode */
> + return -EINVAL;
> +}
I guess we don't nodde to register the dumb resume handler
> +
> +static int mtk_dma_chan_init(struct mtk_dmadev *mtkd)
> +{
Directly span the function in the driver probe is good to know how many channels are request and how large size its memory being allocated.
> + struct mtk_chan *c;
> +
> + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> + if (!c)
> + return -ENOMEM;
> +
> + c->vc.desc_free = mtk_dma_desc_free;
> + vchan_init(&c->vc, &mtkd->ddev);
> + spin_lock_init(&c->lock);
> + INIT_LIST_HEAD(&c->node);
> +
> + init_completion(&c->done);
> + atomic_set(&c->loopcnt, 0);
> + atomic_set(&c->entry, 0);
I really don't like to magic atomic variable. It would make it's harder to track each descriptors usage either pending or active in the dma engine.
> +
> + return 0;
> +}
> +
> +static void mtk_dma_free(struct mtk_dmadev *mtkd)
> +{
> + tasklet_kill(&mtkd->task);
> + 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);
> + devm_kfree(mtkd->ddev.dev, c);
devm_kfree isn't be called explicitly in the driver, it would be taken care when driver is unloaded by the core.
> + }
> +}
> +
> +static const struct of_device_id mtk_uart_dma_match[] = {
> + { .compatible = "mediatek,mt6577-uart-dma", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_uart_dma_match);
> +
> +static int mtk_dma_probe(struct platform_device *pdev)
> +{
> + struct mtk_dmadev *mtkd;
> + struct resource *res;
> + unsigned int i;
> + int rc;
> +
> + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> + if (!mtkd)
> + return -ENOMEM;
> +
> + for (i = 0; i < MTK_SDMA_CHANNELS; i++) {
> + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> + if (!res)
> + return -ENODEV;
> + mtkd->mem_base[i] = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(mtkd->mem_base[i]))
> + return PTR_ERR(mtkd->mem_base[i]);
> + }
> +
> + /* request irq */
remove the comment that is not identical to the implementation as below code snippet
> + for (i = 0; i < MTK_SDMA_CHANNELS; i++) {
> + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> + if ((int)mtkd->dma_irq[i] < 0) {
the cast seems not necessary
> + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> + return -EINVAL;
> + }
> + }
> +
> + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
assign a name to the "apdma" per the binding you adds
> + if (IS_ERR(mtkd->clk)) {
> + dev_err(&pdev->dev, "No clock specified\n");
> + return PTR_ERR(mtkd->clk);
> + }
> +
> + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
> + dev_info(&pdev->dev, "Support dma 33bits\n");
> + mtkd->support_33bits = true;
> + }
> +
> + if (mtkd->support_33bits)
> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(33));
> + else
> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> + if (rc)
> + return rc;
> +
> + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> + mtkd->ddev.device_alloc_chan_resources = mtk_dma_alloc_chan_resources;
> + mtkd->ddev.device_free_chan_resources = mtk_dma_free_chan_resources;
> + mtkd->ddev.device_tx_status = mtk_dma_tx_status;
> + mtkd->ddev.device_issue_pending = mtk_dma_issue_pending;
> + mtkd->ddev.device_prep_slave_sg = mtk_dma_prep_slave_sg;
> + mtkd->ddev.device_config = mtk_dma_slave_config;
> + mtkd->ddev.device_pause = mtk_dma_device_pause;
> + mtkd->ddev.device_resume = mtk_dma_device_resume;
Is it possible that we don't register pause and resume handler if the dma engine cannot support to?
> + mtkd->ddev.device_terminate_all = mtk_dma_terminate_all;
> + mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> + mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> + mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> + mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> + mtkd->ddev.dev = &pdev->dev;
> + INIT_LIST_HEAD(&mtkd->ddev.channels);
> + INIT_LIST_HEAD(&mtkd->pending);
> +
> + spin_lock_init(&mtkd->lock);
> + tasklet_init(&mtkd->task, mtk_dma_sched, (unsigned long)mtkd);
> +
> + mtkd->dma_requests = MTK_SDMA_REQUESTS;
> + if (of_property_read_u32(pdev->dev.of_node,
> + "dma-requests", &mtkd->dma_requests) != 0) {
extra "! = 0" can be dropped in the condition check
> + dev_info(&pdev->dev,
> + "Missing dma-requests property, using %u.\n",
> + MTK_SDMA_REQUESTS);
> + }
> +
> + for (i = 0; i < MTK_SDMA_CHANNELS; i++) {
> + rc = mtk_dma_chan_init(mtkd);
> + if (rc)
> + goto err_no_dma;
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> +
> + rc = dma_async_device_register(&mtkd->ddev);
> + if (rc) {
> + dev_warn(&pdev->dev, "fail to register DMA device: %d\n", rc);
drop the debug log
> + mtk_dma_clk_disable(mtkd);
move the undo jobs at the tail of the function to error handling clean
> + goto err_no_dma;
> + }
> +
> + platform_set_drvdata(pdev, mtkd);
> +
> + if (pdev->dev.of_node) {
you don't need that since all mtk drivers are dt based
> + mtk_dma_info.dma_cap = mtkd->ddev.cap_mask;
> +
> + /* Device-tree DMA controller registration */
> + rc = of_dma_controller_register(pdev->dev.of_node,
> + of_dma_simple_xlate,
> + &mtk_dma_info);
> + if (rc) {
> + dev_warn(&pdev->dev, "fail to register DMA controller\n");
drop the debug log
> + dma_async_device_unregister(&mtkd->ddev);
> + mtk_dma_clk_disable(mtkd);
move the undo jobs at the tail of the function to make error handling clean
> + goto err_no_dma;
> + }
> + }
> +
> + return rc;
> +
> +err_no_dma:
> + mtk_dma_free(mtkd);
> + return rc;
> +}
> +
> +static int mtk_dma_remove(struct platform_device *pdev)
> +{
> + struct mtk_dmadev *mtkd = platform_get_drvdata(pdev);
> +
> + if (pdev->dev.of_node)
> + of_dma_controller_free(pdev->dev.of_node);
> +
> + pm_runtime_disable(&pdev->dev);
> + pm_runtime_put_noidle(&pdev->dev);
> +
> + dma_async_device_unregister(&mtkd->ddev);
> +
> + mtk_dma_free(mtkd);
Extra task in the remove handler is needed to add here such as ensure that VC tasks being killed, disable hardware and its interrupts and wait for pending ISRs task all done.
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_dma_suspend(struct device *dev)
> +{
> + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev))
> + mtk_dma_clk_disable(mtkd);
> +
> + return 0;
> +}
> +
> +static int mtk_dma_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev)) {
> + ret = mtk_dma_clk_enable(mtkd);
> + if (ret) {
> + dev_info(dev, "fail to enable clk: %d\n", ret);
get rid of the debug message
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int mtk_dma_runtime_suspend(struct device *dev)
> +{
> + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> +
> + mtk_dma_clk_disable(mtkd);
> +
> + return 0;
> +}
> +
> +static int mtk_dma_runtime_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> +
> + ret = mtk_dma_clk_enable(mtkd);
> + if (ret) {
> + dev_warn(dev, "fail to enable clk: %d\n", ret);
get rid of the debug message
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct dev_pm_ops mtk_dma_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mtk_dma_suspend, mtk_dma_resume)
> + SET_RUNTIME_PM_OPS(mtk_dma_runtime_suspend,
> + mtk_dma_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver mtk_dma_driver = {
> + .probe = mtk_dma_probe,
mtk_apdma_uart_probe? I guess apdma is a real name for the dma engine
> + .remove = mtk_dma_remove,
mtk_apdma_uart_remove? same reason as the above
> + .driver = {
> + .name = "8250-mtk-dma",
.name = KBUILD_MODNAME,
> + .pm = &mtk_dma_pm_ops,
> + .of_match_table = of_match_ptr(mtk_uart_dma_match),
> + },
> +};
> +
> +static bool mtk_dma_filter_fn(struct dma_chan *chan, void *param)
> +{
> + if (chan->device->dev->driver == &mtk_dma_driver.driver) {
> + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_dma_chan(chan);
> + unsigned int req = *(unsigned int *)param;
> +
> + if (req <= mtkd->dma_requests) {
> + c->dma_sig = req;
> + c->dma_ch = req;
> + return true;
> + }
> + }
> + return false;
> +}
I really wonder if the filter fn is necessary stuff made here but I am not fully sure.
Can you look into more about of_dma_controller_register and its usage to see if the generic call satisfies your need?
> +
> +static int mtk_dma_init(void)
> +{
> + return platform_driver_register(&mtk_dma_driver);
> +}
> +subsys_initcall(mtk_dma_init);
> +
> +static void __exit mtk_dma_exit(void)
> +{
> + platform_driver_unregister(&mtk_dma_driver);
> +}
> +module_exit(mtk_dma_exit);
Use module_platform_driver instead and then add more information for the module such as MODULE_DESCRIPTION, MODULE_AUTHOR, MODULE_LICENSE.
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index dacf3f4..cfa1699 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -151,6 +151,17 @@ config DMA_JZ4780
> If you have a board based on such a SoC and wish to use DMA for
> devices which can use the DMA controller, say Y or M here.
>
> +config DMA_MTK_UART
> + tristate "MediaTek SoCs APDMA support for UART"
> + depends on OF
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Support for the UART DMA engine found on MediaTek MTK SoCs.
> + when 8250 mtk uart is enabled, and if you want to using DMA,
> + you can enable the config. the DMA engine just only be used
> + with MediaTek Socs.
> +
> config DMA_SA11X0
> tristate "SA-11x0 DMA support"
> depends on ARCH_SA1100 || COMPILE_TEST
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index c91702d..42690d8 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_COH901318) += coh901318.o coh901318_lli.o
> obj-$(CONFIG_DMA_BCM2835) += bcm2835-dma.o
> obj-$(CONFIG_DMA_JZ4740) += dma-jz4740.o
> obj-$(CONFIG_DMA_JZ4780) += dma-jz4780.o
> +obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
move Makefile update to drivers/dma/mediatek
> obj-$(CONFIG_DMA_SA11X0) += sa11x0-dma.o
> obj-$(CONFIG_DMA_SUN4I) += sun4i-dma.o
> obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
^ permalink raw reply
* [PATCH v2 3/3] serial: 8250_uniphier: add auto-flow-control support
From: Masahiro Yamada @ 2018-09-28 2:05 UTC (permalink / raw)
To: Greg KH, linux-serial
Cc: Dai Okamura, Masahiro Yamada, devicetree, Jiri Slaby,
linux-kernel, Rob Herring, Mark Rutland, linux-arm-kernel
In-Reply-To: <1538100309-24323-1-git-send-email-yamada.masahiro@socionext.com>
From: Dai Okamura <okamura.dai@socionext.com>
Add selective auto-flow-control support for UniPhier serial driver.
Signed-off-by: Dai Okamura <okamura.dai@socionext.com>
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v2: None
Documentation/devicetree/bindings/serial/uniphier-uart.txt | 3 +++
drivers/tty/serial/8250/8250_uniphier.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/uniphier-uart.txt b/Documentation/devicetree/bindings/serial/uniphier-uart.txt
index 811c479..7a1bf02 100644
--- a/Documentation/devicetree/bindings/serial/uniphier-uart.txt
+++ b/Documentation/devicetree/bindings/serial/uniphier-uart.txt
@@ -6,6 +6,9 @@ Required properties:
- interrupts: a single interrupt specifier.
- clocks: phandle to the input clock.
+Optional properties:
+-auto-flow-control: enable automatic flow control support.
+
Example:
aliases {
serial0 = &serial0;
diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
index 1028c02..164ba13 100644
--- a/drivers/tty/serial/8250/8250_uniphier.c
+++ b/drivers/tty/serial/8250/8250_uniphier.c
@@ -222,6 +222,9 @@ static int uniphier_uart_probe(struct platform_device *pdev)
up.port.flags = UPF_FIXED_PORT | UPF_FIXED_TYPE;
up.capabilities = UART_CAP_FIFO;
+ if (of_property_read_bool(dev->of_node, "auto-flow-control"))
+ up.capabilities |= UART_CAP_AFE;
+
up.port.serial_in = uniphier_serial_in;
up.port.serial_out = uniphier_serial_out;
up.dl_read = uniphier_serial_dl_read;
--
2.7.4
^ permalink raw reply related
* [PATCH v2 2/3] serial: 8250_uniphier: flatten probe function
From: Masahiro Yamada @ 2018-09-28 2:05 UTC (permalink / raw)
To: Greg KH, linux-serial
Cc: Dai Okamura, Masahiro Yamada, Jiri Slaby, linux-kernel,
linux-arm-kernel
In-Reply-To: <1538100309-24323-1-git-send-email-yamada.masahiro@socionext.com>
Currently, the DT-related settings are split out to
uniphier_of_serial_setup(), but it turned out to be not nice.
The next commit will add a DT property, but it will not fit in
the helper. Merge the helper into the probe function.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v2: None
drivers/tty/serial/8250/8250_uniphier.c | 49 ++++++++++++---------------------
1 file changed, 17 insertions(+), 32 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
index d292654..1028c02 100644
--- a/drivers/tty/serial/8250/8250_uniphier.c
+++ b/drivers/tty/serial/8250/8250_uniphier.c
@@ -155,36 +155,6 @@ static void uniphier_serial_dl_write(struct uart_8250_port *up, int value)
writel(value, up->port.membase + UNIPHIER_UART_DLR);
}
-static int uniphier_of_serial_setup(struct device *dev, struct uart_port *port,
- struct uniphier8250_priv *priv)
-{
- int ret;
- u32 prop;
- struct device_node *np = dev->of_node;
-
- ret = of_alias_get_id(np, "serial");
- if (ret < 0) {
- dev_err(dev, "failed to get alias id\n");
- return ret;
- }
- port->line = ret;
-
- /* Get clk rate through clk driver */
- priv->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(priv->clk)) {
- dev_err(dev, "failed to get clock\n");
- return PTR_ERR(priv->clk);
- }
-
- ret = clk_prepare_enable(priv->clk);
- if (ret < 0)
- return ret;
-
- port->uartclk = clk_get_rate(priv->clk);
-
- return 0;
-}
-
static int uniphier_uart_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -217,9 +187,24 @@ static int uniphier_uart_probe(struct platform_device *pdev)
memset(&up, 0, sizeof(up));
- ret = uniphier_of_serial_setup(dev, &up.port, priv);
- if (ret < 0)
+ ret = of_alias_get_id(dev->of_node, "serial");
+ if (ret < 0) {
+ dev_err(dev, "failed to get alias id\n");
return ret;
+ }
+ up.port.line = ret;
+
+ priv->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev, "failed to get clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ up.port.uartclk = clk_get_rate(priv->clk);
spin_lock_init(&priv->atomic_write_lock);
--
2.7.4
^ permalink raw reply related
* [PATCH v2 1/3] serial: 8250_uniphier: remove unused "fifo-size" property
From: Masahiro Yamada @ 2018-09-28 2:05 UTC (permalink / raw)
To: Greg KH, linux-serial
Cc: Dai Okamura, Masahiro Yamada, devicetree, Jiri Slaby,
linux-kernel, Rob Herring, Mark Rutland, linux-arm-kernel
In-Reply-To: <1538100309-24323-1-git-send-email-yamada.masahiro@socionext.com>
The FIFO size of the UART devices is 64 on almost all UniPhier SoCs
with the exception Pro4TV SoC (MN2WS0235), which used 128 FIFO size.
However, Pro4TV SoC was never upstreamed, and out of production.
So, this property has never been used in a useful way.
Let's remove old unused code.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
- Fix the product code in git-log: MN2WS0230 -> MN2WS0235
Documentation/devicetree/bindings/serial/uniphier-uart.txt | 4 ----
drivers/tty/serial/8250/8250_uniphier.c | 10 +---------
2 files changed, 1 insertion(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/uniphier-uart.txt b/Documentation/devicetree/bindings/serial/uniphier-uart.txt
index 0b3892a..811c479 100644
--- a/Documentation/devicetree/bindings/serial/uniphier-uart.txt
+++ b/Documentation/devicetree/bindings/serial/uniphier-uart.txt
@@ -6,9 +6,6 @@ Required properties:
- interrupts: a single interrupt specifier.
- clocks: phandle to the input clock.
-Optional properties:
-- fifo-size: the RX/TX FIFO size. Defaults to 64 if not specified.
-
Example:
aliases {
serial0 = &serial0;
@@ -19,5 +16,4 @@ Example:
reg = <0x54006800 0x40>;
interrupts = <0 33 4>;
clocks = <&uart_clk>;
- fifo-size = <64>;
};
diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
index 28d88ccf..d292654 100644
--- a/drivers/tty/serial/8250/8250_uniphier.c
+++ b/drivers/tty/serial/8250/8250_uniphier.c
@@ -12,9 +12,6 @@
#include "8250.h"
-/* Most (but not all) of UniPhier UART devices have 64-depth FIFO. */
-#define UNIPHIER_UART_DEFAULT_FIFO_SIZE 64
-
/*
* This hardware is similar to 8250, but its register map is a bit different:
* - MMIO32 (regshift = 2)
@@ -185,12 +182,6 @@ static int uniphier_of_serial_setup(struct device *dev, struct uart_port *port,
port->uartclk = clk_get_rate(priv->clk);
- /* Check for fifo size */
- if (of_property_read_u32(np, "fifo-size", &prop) == 0)
- port->fifosize = prop;
- else
- port->fifosize = UNIPHIER_UART_DEFAULT_FIFO_SIZE;
-
return 0;
}
@@ -241,6 +232,7 @@ static int uniphier_uart_probe(struct platform_device *pdev)
up.port.type = PORT_16550A;
up.port.iotype = UPIO_MEM32;
+ up.port.fifosize = 64;
up.port.regshift = UNIPHIER_UART_REGSHIFT;
up.port.flags = UPF_FIXED_PORT | UPF_FIXED_TYPE;
up.capabilities = UART_CAP_FIFO;
--
2.7.4
^ permalink raw reply related
* [PATCH v2 0/3] serial: 8250_uniphier: remove unused code and add auto-flow-control
From: Masahiro Yamada @ 2018-09-28 2:05 UTC (permalink / raw)
To: Greg KH, linux-serial
Cc: Dai Okamura, Masahiro Yamada, devicetree, Jiri Slaby,
linux-kernel, Rob Herring, Mark Rutland, linux-arm-kernel
- Remove never-used DT property
- Refactor code
- Add auto-flow-control support
Dai Okamura (1):
serial: 8250_uniphier: add auto-flow-control support
Masahiro Yamada (2):
serial: 8250_uniphier: remove unused "fifo-size" property
serial: 8250_uniphier: flatten probe function
.../devicetree/bindings/serial/uniphier-uart.txt | 3 +-
drivers/tty/serial/8250/8250_uniphier.c | 62 ++++++++--------------
2 files changed, 22 insertions(+), 43 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH 0/2] serial: sh-sci: Fix earlycon on Renesas ARM platforms
From: Geert Uytterhoeven @ 2018-09-27 7:38 UTC (permalink / raw)
To: Greg KH
Cc: Jiri Slaby, Chris Brandt, Wolfram Sang, uli+renesas,
open list:SERIAL DRIVERS, Linux-Renesas,
Linux Kernel Mailing List, Geert Uytterhoeven
In-Reply-To: <20180830125404.21192-1-geert+renesas@glider.be>
Hi Greg,
On Thu, Aug 30, 2018 at 2:54 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> Due to an unfortunate oversight, commit 2d4dd0da45401c7a ("serial: sh-sci:
> Allow for compressed SCIF address") broke earlycon on all Renesas ARM
> platforms using a SCIF port for the serial console (R-Car, RZ/A1, RZ/G1,
> RZ/G2 SoCs), due to an incorrect value of port->regshift.
>
> This patch series fixes that by reverting that commit, and a (reverse)
> dependency.
>
> Earlycon for RZ/A2 (which is a new platform) will be fixed later in a
> separate patch.
>
> Thanks for applying!
>
> Geert Uytterhoeven (2):
> Revert "serial: sh-sci: Remove SCIx_RZ_SCIFA_REGTYPE"
> Revert "serial: sh-sci: Allow for compressed SCIF address"
>
> drivers/tty/serial/sh-sci.c | 56 +++++++++++++++++++++++++++----------
> include/linux/serial_sci.h | 1 +
> 2 files changed, 42 insertions(+), 15 deletions(-)
These are now in tty-next:
10c63443b74d1ef5 Revert "serial: sh-sci: Remove SCIx_RZ_SCIFA_REGTYPE"
a1c2fd7e1098ea49 Revert "serial: sh-sci: Allow for compressed SCIF address"
Can you please include them in v4.19-rc6, as they fix a regression introduced
in v4.19-rc1?
In addition:
3d8b43ad9c0cf023 serial: sh-sci: Add earlycon for R7S9210
(also in tty-next) enables earlycon on RZ/A2 again, as it was disabled
by the two
reverts above.
Thanks a lot!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ 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