* Re: [RFC PATCH v1 08/25] printk: add ring buffer and kthread
From: John Ogness @ 2019-03-11 10:51 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Sergey Senozhatsky, linux-kernel, Peter Zijlstra, Petr Mladek,
Steven Rostedt, Daniel Wang, Andrew Morton, Linus Torvalds,
Greg Kroah-Hartman, Alan Cox, Jiri Slaby, Peter Feiner,
linux-serial
In-Reply-To: <20190307051530.GC4893@jagdpanzerIV>
On 2019-03-07, Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
>> The current printk implementation will do a better job of getting the
>> informational messages out, but at an enormous cost to all the tasks
>> on the system (including the realtime tasks). I am proposing a printk
>> implementation where the tasks are not affected by console printing
>> floods.
>
> In new printk design the tasks are still affected by printing floods.
> Tasks have to line up and (busy) wait for each other, regardless of
> contexts.
They only line up and busy wait is to add the informational message to
the ring buffer. The current printk implementation is the same in this
respect. And as you've noted, the logbuf spinlock is not a source of
latencies.
> One of the late patch sets which I had (I never ever published it) was
> a different kind of printk-kthread offloading. The idea was that
> whatever should be printed (suppress_message_printing()) should be
> printed. We obviously can't loop in console_unlock() for ever and
> there is only one way to figure out if we can print out more messages,
> that's why printk became RCU stall detector and watchdog aware; and
> printk would break out and wake up printk_kthread if it sees that
> watchdog is about to get angry on that particular CPU. printk_kthread
> would run with preemption disabled and do the same thing: if it spent
> watchdog_threshold / 2 printing - breakout, enable local IRQ,
> cond_resched(). IOW watchdogs determine how much time we can spend on
> printing.
I studied and experimented with this (from your git). It was an
interesting idea of keeping the current logic, but allowing to offload
to a separate kthread if things were getting too overloaded. (It is also
where I got term "emergency" from.)
But I was satisfied with neither the direct printing (winner takes all,
printk-safe defers) nor _relying_ on a kthread for important messages in
an offload situation. This is what convinced me that the kernel needs a
new interface so that it can communicate the _really_ important things
synchronously: write_atomic().
>> I want messages of the information category to cause no disturbance
>> to the system. Give the kernel the freedom to communicate to users
>> without destroying its own performance. This can only be achieved if
>> the messages are printed from a _fully_ preemptible context.
> [..]
>> And I want messages of the emergency category to be as reliable as
>> possible, regardless of the costs to the system. Give the kernel a
>> clear mechanism to _reliably_ communicate critical information. Such
>> messages should never appear on a correctly functioning system.
>
> I don't really understand the role of loglevel anymore.
>
> When I do ./a.out --loglevel=X I have a clear understanding that
> all messages which fall into [critical, X] range will be in the logs,
> because I told that application that those messages are important to
> me right now. And it used to be the same with the kernel loglevel.
The loglevel is not related to logging. It specifies the amount of
console printing. But I will assume you are referring to creating log
files by having an external device store the console printing.
> But now the kernel will do its own thing:
>
> - what the kernel considers important will go into the logs
> - what the kernel doesn't consider important _maybe_ will end up
> in the logs (preemptible printk kthread). And this is where
> loglevel now. After the _maybe_ part.
"what the kernel considers" is a configuration option of the
administrator. The administrator can increase the verbocity of the
console (loglevel) without having negative effects on the system
itself. Also, if the system were to suddenly crash, those crash messages
shouldn't be in jeopardy just because the verbocity of the console was
turned up.
You (and Petr) talk about that _all_ console printing is for
emergencies. That if an administrator sets the loglevel to 7 it is
because the pr_info messages are just as important as the pr_emerg. And
if that is indeed the intention of console printing and loglevel, then
why is asynchronous printk calls for console messages even allowed
today? IMO that isn't taking the importance of the message very
seriously.
> If I'm not mistaken, Tetsuo reported that on a box under heavy OOM
> pressure he saw preemptible printk dragging 5 minutes behind the
> logbuf head. Preemptible printk is good for nothing. It's beyond
> useless, it's something else.
The informational messages are correctly timestamped and can be sorted
offline. They are informational, so any loss is less tragic. And they
aren't affecting system performance because they are being printed in
preemptible contexts.
John Ogness
^ permalink raw reply
* [PATCH v2] serial: sh-sci: Missing uart_unregister_driver() on error in sci_probe_single()
From: Mao Wenan @ 2019-03-11 9:51 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, kernel-janitors, linux-kernel,
dan.carpenter
Add the missing uart_unregister_driver() before return
from sci_probe_single() in the error handling case.
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
v1->v2: add uart_unregister_driver() if mctrl_gpio_init is failed.
drivers/tty/serial/sh-sci.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 64bbeb7d7e0c..fb5034390795 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3254,12 +3254,16 @@ static int sci_probe_single(struct platform_device *dev,
mutex_unlock(&sci_uart_registration_lock);
ret = sci_init_single(dev, sciport, index, p, false);
- if (ret)
+ if (ret) {
+ uart_unregister_driver(&sci_uart_driver);
return ret;
+ }
sciport->gpios = mctrl_gpio_init(&sciport->port, 0);
- if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS)
+ if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS) {
+ uart_unregister_driver(&sci_uart_driver);
return PTR_ERR(sciport->gpios);
+ }
if (sciport->has_rtscts) {
if (!IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(sciport->gpios,
--
2.20.1
^ permalink raw reply related
* [PATCH serial v3] sc16is7xx: missing unregister/delete driver on error in sc16is7xx_init()
From: Mao Wenan @ 2019-03-11 9:39 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, linux-kernel, kernel-janitors, vz,
dan.carpenter
Add the missing uart_unregister_driver() and i2c_del_driver() before
return from sc16is7xx_init() in the error handling case.
Signed-off-by: Mao Wenan <maowenan@huawei.com>
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
---
v1->v2: fix compile warning if CONFIG_SERIAL_SC16IS7XX_SPI is not exist.
v2->v3: create functions for i2c and spi to do error handling as dan carpenter
suggestion.
drivers/tty/serial/sc16is7xx.c | 53 ++++++++++++++++++++++++----------
1 file changed, 38 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 268098681856..8be3adaf4782 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1436,6 +1436,19 @@ static struct spi_driver sc16is7xx_spi_uart_driver = {
};
MODULE_ALIAS("spi:sc16is7xx");
+
+static int __init register_sc16is7xx_spi(void)
+{
+ return spi_register_driver(&sc16is7xx_spi_uart_driver);
+}
+
+static void __exit unregister_sc16is7xx_spi(void)
+{
+ spi_unregister_driver(&sc16is7xx_spi_uart_driver);
+}
+#else
+static int __init register_sc16is7xx_spi(void) { return 0; }
+static void __exit unregister_sc16is7xx_spi(void) {}
#endif
#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
@@ -1493,6 +1506,18 @@ static struct i2c_driver sc16is7xx_i2c_uart_driver = {
.id_table = sc16is7xx_i2c_id_table,
};
+static int __init register_sc16is7xx_i2c(void)
+{
+ return i2c_add_driver(&sc16is7xx_i2c_uart_driver);
+}
+
+static void __exit del_sc16is7xx_i2c(void)
+{
+ i2c_del_driver(&sc16is7xx_i2c_uart_driver);
+}
+#else
+static int __init register_sc16is7xx_i2c(void) { return 0; }
+static void __exit del_sc16is7xx_i2c(void) {}
#endif
static int __init sc16is7xx_init(void)
@@ -1505,34 +1530,32 @@ static int __init sc16is7xx_init(void)
return ret;
}
-#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
- ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
+ ret = register_sc16is7xx_i2c();
if (ret < 0) {
pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
- return ret;
+ goto err_i2c;
}
-#endif
-#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
- ret = spi_register_driver(&sc16is7xx_spi_uart_driver);
+ ret = register_sc16is7xx_spi();
if (ret < 0) {
pr_err("failed to init sc16is7xx spi --> %d\n", ret);
- return ret;
+ goto err_spi;
}
-#endif
+
+ return ret;
+
+err_spi:
+ del_sc16is7xx_i2c();
+err_i2c:
+ uart_unregister_driver(&sc16is7xx_uart);
return ret;
}
module_init(sc16is7xx_init);
static void __exit sc16is7xx_exit(void)
{
-#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
- i2c_del_driver(&sc16is7xx_i2c_uart_driver);
-#endif
-
-#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
- spi_unregister_driver(&sc16is7xx_spi_uart_driver);
-#endif
+ del_sc16is7xx_i2c();
+ unregister_sc16is7xx_spi();
uart_unregister_driver(&sc16is7xx_uart);
}
module_exit(sc16is7xx_exit);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] serial: sh-sci: Missing uart_unregister_driver() on error in sci_probe_single()
From: maowenan @ 2019-03-11 9:33 UTC (permalink / raw)
To: Dan Carpenter; +Cc: gregkh, jslaby, linux-serial, kernel-janitors, linux-kernel
In-Reply-To: <20190311072201.GA2434@kadam>
On 2019/3/11 15:22, Dan Carpenter wrote:
> On Fri, Mar 08, 2019 at 10:23:10PM +0800, Mao Wenan wrote:
>> Add the missing uart_unregister_driver() before return
>> from sci_probe_single() in the error handling case.
>>
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>> ---
>> drivers/tty/serial/sh-sci.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
>> index 64bbeb7d7e0c..dde4eac9d222 100644
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -3254,8 +3254,10 @@ static int sci_probe_single(struct platform_device *dev,
>> mutex_unlock(&sci_uart_registration_lock);
>>
>> ret = sci_init_single(dev, sciport, index, p, false);
>> - if (ret)
>> + if (ret) {
>> + uart_unregister_driver(&sci_uart_driver);
>> return ret;
>
> Why only this error path and not the next one?
>
>> + }
>>
>> sciport->gpios = mctrl_gpio_init(&sciport->port, 0);
>> if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS)
> ^^^^^^^^^^^^^^^^^^^^^
> This one.
yes, this one need it too. resend v2.
thank you.
>
> regards,
> dan carpenter
>
>
> .
>
^ permalink raw reply
* Re: [PATCH serial v2] sc16is7xx: missing unregister/delete driver on error in sc16is7xx_init()
From: Dan Carpenter @ 2019-03-11 9:24 UTC (permalink / raw)
To: maowenan; +Cc: gregkh, jslaby, linux-serial, linux-kernel, kernel-janitors, vz
In-Reply-To: <9f611db1-11e3-9e9b-e7f3-d3dcc83b57f7@huawei.com>
On Mon, Mar 11, 2019 at 05:12:03PM +0800, maowenan wrote:
>
>
> On 2019/3/11 15:46, Dan Carpenter wrote:
> > On Sat, Mar 09, 2019 at 10:50:37AM +0800, Mao Wenan wrote:
> >> Add the missing uart_unregister_driver() and i2c_del_driver() before return
> >> from sc16is7xx_init() in the error handling case.
> >>
> >> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
> >> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> >> ---
> >> v1->v2: fix compile warning if CONFIG_SERIAL_SC16IS7XX_SPI is not exist.
> >> drivers/tty/serial/sc16is7xx.c | 14 ++++++++++++--
> >> 1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >> index 268098681856..b9e5941ce767 100644
> >> --- a/drivers/tty/serial/sc16is7xx.c
> >> +++ b/drivers/tty/serial/sc16is7xx.c
> >> @@ -1509,7 +1509,7 @@ static int __init sc16is7xx_init(void)
> >> ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
> >> if (ret < 0) {
> >> pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
> >> - return ret;
> >> + goto err_i2c;
> >> }
> >> #endif
> >>
> >> @@ -1517,10 +1517,20 @@ static int __init sc16is7xx_init(void)
> >> ret = spi_register_driver(&sc16is7xx_spi_uart_driver);
> >> if (ret < 0) {
> >> pr_err("failed to init sc16is7xx spi --> %d\n", ret);
> >> - return ret;
> >> + goto err_spi;
> >> }
> >> #endif
> >> return ret;
> >> +
> >> +#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
> >> +err_spi:
> >> +#endif
> >> +#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
> >> + i2c_del_driver(&sc16is7xx_i2c_uart_driver);
> >> +err_i2c:
> >> +#endif
> >> + uart_unregister_driver(&sc16is7xx_uart);
> >> + return ret;
> >
> > This is quite ulgy. Why not create two functions:
> >
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 635178cf3eed..9986cb2479ad 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -1491,6 +1491,21 @@ static struct i2c_driver sc16is7xx_i2c_uart_driver = {
> > .id_table = sc16is7xx_i2c_id_table,
> > };
> >
> > +static int __init register_sc16is7xx_i2c(void)
> > +{
> > + return i2c_add_driver(&sc16is7xx_i2c_uart_driver);
> > +}
> > +
> > +static void __exit del_sc16is7xx_i2c(void)
> > +{
> > + i2c_del_driver(&sc16is7xx_i2c_uart_driver);
> > +}
> > +
> > +#else
> > +
> > +static int __init register_sc16is7xx_i2c(void) { return 0; }
> > +static void __exit del_sc16is7xx_i2c(void) {}
> > +
> > #endif
> >
> > static int __init sc16is7xx_init(void)
> >
> > Then you can remove all the ifdefs.
> ok， thank you, I will send v3. Do you mind Reviewed-by ?
I'll add after I review the patch. :)
regards,
dan carpenter
^ permalink raw reply
* [PATCH v8 2/2] arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and Makefile
From: Erin Lo @ 2019-03-11 8:54 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
mars.cheng, eddie.huang, linux-clk, zhiyong.tao, Mengqi.Zhang,
Ben Ho, Seiya Wang, Weiyi Lu, Hsin-Hsiung Wang
In-Reply-To: <1552294472-32929-1-git-send-email-erin.lo@mediatek.com>
From: Ben Ho <Ben.Ho@mediatek.com>
Add basic chip support for Mediatek 8183, include
uart node with correct uart clocks, pwrap device
Add clock controller nodes, include topckgen, infracfg,
apmixedsys and subsystem.
Signed-off-by: Ben Ho <Ben.Ho@mediatek.com>
Signed-off-by: Erin Lo <erin.lo@mediatek.com>
Signed-off-by: Seiya Wang <seiya.wang@mediatek.com>
Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
Signed-off-by: Zhiyong Tao <zhiyong.tao@mediatek.com>
Signed-off-by: Hsin-Hsiung Wang <hsin-hsiung.wang@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
arch/arm64/boot/dts/mediatek/Makefile | 1 +
arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 31 +++
arch/arm64/boot/dts/mediatek/mt8183.dtsi | 335 ++++++++++++++++++++++++++++
3 files changed, 367 insertions(+)
create mode 100644 arch/arm64/boot/dts/mediatek/mt8183-evb.dts
create mode 100644 arch/arm64/boot/dts/mediatek/mt8183.dtsi
diff --git a/arch/arm64/boot/dts/mediatek/Makefile b/arch/arm64/boot/dts/mediatek/Makefile
index e8f952f..458bbc4 100644
--- a/arch/arm64/boot/dts/mediatek/Makefile
+++ b/arch/arm64/boot/dts/mediatek/Makefile
@@ -7,3 +7,4 @@ dtb-$(CONFIG_ARCH_MEDIATEK) += mt6797-x20-dev.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-rfb1.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt7622-bananapi-bpi-r64.dtb
dtb-$(CONFIG_ARCH_MEDIATEK) += mt8173-evb.dtb
+dtb-$(CONFIG_ARCH_MEDIATEK) += mt8183-evb.dtb
diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
new file mode 100644
index 0000000..9b52559
--- /dev/null
+++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Ben Ho <ben.ho@mediatek.com>
+ * Erin Lo <erin.lo@mediatek.com>
+ */
+
+/dts-v1/;
+#include "mt8183.dtsi"
+
+/ {
+ model = "MediaTek MT8183 evaluation board";
+ compatible = "mediatek,mt8183-evb", "mediatek,mt8183";
+
+ aliases {
+ serial0 = &uart0;
+ };
+
+ memory@40000000 {
+ device_type = "memory";
+ reg = <0 0x40000000 0 0x80000000>;
+ };
+
+ chosen {
+ stdout-path = "serial0:921600n8";
+ };
+};
+
+&uart0 {
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
new file mode 100644
index 0000000..64f8bd6
--- /dev/null
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Copyright (c) 2018 MediaTek Inc.
+ * Author: Ben Ho <ben.ho@mediatek.com>
+ * Erin Lo <erin.lo@mediatek.com>
+ */
+
+#include <dt-bindings/clock/mt8183-clk.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
+/ {
+ compatible = "mediatek,mt8183";
+ interrupt-parent = <&sysirq>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ cpu-map {
+ cluster0 {
+ core0 {
+ cpu = <&cpu0>;
+ };
+ core1 {
+ cpu = <&cpu1>;
+ };
+ core2 {
+ cpu = <&cpu2>;
+ };
+ core3 {
+ cpu = <&cpu3>;
+ };
+ };
+
+ cluster1 {
+ core0 {
+ cpu = <&cpu4>;
+ };
+ core1 {
+ cpu = <&cpu5>;
+ };
+ core2 {
+ cpu = <&cpu6>;
+ };
+ core3 {
+ cpu = <&cpu7>;
+ };
+ };
+ };
+
+ cpu0: cpu@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x000>;
+ enable-method = "psci";
+ };
+
+ cpu1: cpu@1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x001>;
+ enable-method = "psci";
+ };
+
+ cpu2: cpu@2 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x002>;
+ enable-method = "psci";
+ };
+
+ cpu3: cpu@3 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x003>;
+ enable-method = "psci";
+ };
+
+ cpu4: cpu@100 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a73";
+ reg = <0x100>;
+ enable-method = "psci";
+ };
+
+ cpu5: cpu@101 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a73";
+ reg = <0x101>;
+ enable-method = "psci";
+ };
+
+ cpu6: cpu@102 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a73";
+ reg = <0x102>;
+ enable-method = "psci";
+ };
+
+ cpu7: cpu@103 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a73";
+ reg = <0x103>;
+ enable-method = "psci";
+ };
+ };
+
+ pmu-a53 {
+ compatible = "arm,cortex-a53-pmu";
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW &ppi_cluster0>;
+ };
+
+ pmu-a73 {
+ compatible = "arm,cortex-a73-pmu";
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW &ppi_cluster1>;
+ };
+
+ psci {
+ compatible = "arm,psci-1.0";
+ method = "smc";
+ };
+
+ clk26m: oscillator {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <26000000>;
+ clock-output-names = "clk26m";
+ };
+
+ timer {
+ compatible = "arm,armv8-timer";
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_LOW 0>,
+ <GIC_PPI 14 IRQ_TYPE_LEVEL_LOW 0>,
+ <GIC_PPI 11 IRQ_TYPE_LEVEL_LOW 0>,
+ <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW 0>;
+ };
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ compatible = "simple-bus";
+ ranges;
+
+ gic: interrupt-controller@c000000 {
+ compatible = "arm,gic-v3";
+ #interrupt-cells = <4>;
+ interrupt-parent = <&gic>;
+ interrupt-controller;
+ reg = <0 0x0c000000 0 0x40000>, /* GICD */
+ <0 0x0c100000 0 0x200000>, /* GICR */
+ <0 0x0c400000 0 0x2000>, /* GICC */
+ <0 0x0c410000 0 0x1000>, /* GICH */
+ <0 0x0c420000 0 0x2000>; /* GICV */
+
+ interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH 0>;
+ ppi-partitions {
+ ppi_cluster0: interrupt-partition-0 {
+ affinity = <&cpu0 &cpu1 &cpu2 &cpu3>;
+ };
+ ppi_cluster1: interrupt-partition-1 {
+ affinity = <&cpu4 &cpu5 &cpu6 &cpu7>;
+ };
+ };
+ };
+
+ mcucfg: syscon@c530000 {
+ compatible = "mediatek,mt8183-mcucfg", "syscon";
+ reg = <0 0x0c530000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ sysirq: interrupt-controller@c530a80 {
+ compatible = "mediatek,mt8183-sysirq",
+ "mediatek,mt6577-sysirq";
+ interrupt-controller;
+ #interrupt-cells = <3>;
+ interrupt-parent = <&gic>;
+ reg = <0 0x0c530a80 0 0x50>;
+ };
+
+ topckgen: syscon@10000000 {
+ compatible = "mediatek,mt8183-topckgen", "syscon";
+ reg = <0 0x10000000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ infracfg: syscon@10001000 {
+ compatible = "mediatek,mt8183-infracfg", "syscon";
+ reg = <0 0x10001000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ pio: pinctrl@10005000 {
+ compatible = "mediatek,mt8183-pinctrl";
+ reg = <0 0x10005000 0 0x1000>,
+ <0 0x11f20000 0 0x1000>,
+ <0 0x11e80000 0 0x1000>,
+ <0 0x11e70000 0 0x1000>,
+ <0 0x11e90000 0 0x1000>,
+ <0 0x11d30000 0 0x1000>,
+ <0 0x11d20000 0 0x1000>,
+ <0 0x11c50000 0 0x1000>,
+ <0 0x11f30000 0 0x1000>,
+ <0 0x1000b000 0 0x1000>;
+ reg-names = "iocfg0", "iocfg1", "iocfg2",
+ "iocfg3", "iocfg4", "iocfg5",
+ "iocfg6", "iocfg7", "iocfg8",
+ "eint";
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&pio 0 0 192>;
+ interrupt-controller;
+ interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
+ #interrupt-cells = <3>;
+ };
+
+ apmixedsys: syscon@1000c000 {
+ compatible = "mediatek,mt8183-apmixedsys", "syscon";
+ reg = <0 0x1000c000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ pwrap: pwrap@1000d000 {
+ compatible = "mediatek,mt8183-pwrap";
+ reg = <0 0x1000d000 0 0x1000>;
+ reg-names = "pwrap";
+ interrupts = <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&topckgen CLK_TOP_MUX_PMICSPI>,
+ <&infracfg CLK_INFRA_PMIC_AP>;
+ clock-names = "spi", "wrap";
+ };
+
+ uart0: serial@11002000 {
+ compatible = "mediatek,mt8183-uart",
+ "mediatek,mt6577-uart";
+ reg = <0 0x11002000 0 0x1000>;
+ interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clk26m>, <&infracfg CLK_INFRA_UART0>;
+ clock-names = "baud", "bus";
+ status = "disabled";
+ };
+
+ uart1: serial@11003000 {
+ compatible = "mediatek,mt8183-uart",
+ "mediatek,mt6577-uart";
+ reg = <0 0x11003000 0 0x1000>;
+ interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clk26m>, <&infracfg CLK_INFRA_UART1>;
+ clock-names = "baud", "bus";
+ status = "disabled";
+ };
+
+ uart2: serial@11004000 {
+ compatible = "mediatek,mt8183-uart",
+ "mediatek,mt6577-uart";
+ reg = <0 0x11004000 0 0x1000>;
+ interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clk26m>, <&infracfg CLK_INFRA_UART2>;
+ clock-names = "baud", "bus";
+ status = "disabled";
+ };
+
+ audiosys: syscon@11220000 {
+ compatible = "mediatek,mt8183-audiosys", "syscon";
+ reg = <0 0x11220000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ mfgcfg: syscon@13000000 {
+ compatible = "mediatek,mt8183-mfgcfg", "syscon";
+ reg = <0 0x13000000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ mmsys: syscon@14000000 {
+ compatible = "mediatek,mt8183-mmsys", "syscon";
+ reg = <0 0x14000000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ imgsys: syscon@15020000 {
+ compatible = "mediatek,mt8183-imgsys", "syscon";
+ reg = <0 0x15020000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ vdecsys: syscon@16000000 {
+ compatible = "mediatek,mt8183-vdecsys", "syscon";
+ reg = <0 0x16000000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ vencsys: syscon@17000000 {
+ compatible = "mediatek,mt8183-vencsys", "syscon";
+ reg = <0 0x17000000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ ipu_conn: syscon@19000000 {
+ compatible = "mediatek,mt8183-ipu_conn", "syscon";
+ reg = <0 0x19000000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ ipu_adl: syscon@19010000 {
+ compatible = "mediatek,mt8183-ipu_adl", "syscon";
+ reg = <0 0x19010000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ ipu_core0: syscon@19180000 {
+ compatible = "mediatek,mt8183-ipu_core0", "syscon";
+ reg = <0 0x19180000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ ipu_core1: syscon@19280000 {
+ compatible = "mediatek,mt8183-ipu_core1", "syscon";
+ reg = <0 0x19280000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+
+ camsys: syscon@1a000000 {
+ compatible = "mediatek,mt8183-camsys", "syscon";
+ reg = <0 0x1a000000 0 0x1000>;
+ #clock-cells = <1>;
+ };
+ };
+};
--
1.9.1
^ permalink raw reply related
* [PATCH v8 1/2] dt-bindings: serial: Add compatible for Mediatek MT8183
From: Erin Lo @ 2019-03-11 8:54 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
mars.cheng, eddie.huang, linux-clk, zhiyong.tao, Mengqi.Zhang
In-Reply-To: <1552294472-32929-1-git-send-email-erin.lo@mediatek.com>
This adds dt-binding documentation of uart for Mediatek MT8183 SoC
Platform.
Signed-off-by: Erin Lo <erin.lo@mediatek.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/serial/mtk-uart.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/serial/mtk-uart.txt b/Documentation/devicetree/bindings/serial/mtk-uart.txt
index 742cb47..bcfb131 100644
--- a/Documentation/devicetree/bindings/serial/mtk-uart.txt
+++ b/Documentation/devicetree/bindings/serial/mtk-uart.txt
@@ -16,6 +16,7 @@ Required properties:
* "mediatek,mt8127-uart" for MT8127 compatible UARTS
* "mediatek,mt8135-uart" for MT8135 compatible UARTS
* "mediatek,mt8173-uart" for MT8173 compatible UARTS
+ * "mediatek,mt8183-uart", "mediatek,mt6577-uart" for MT8183 compatible UARTS
* "mediatek,mt6577-uart" for MT6577 and all of the above
- reg: The base address of the UART register bank.
--
1.9.1
^ permalink raw reply related
* [PATCH v8 0/2] Add basic and clock support for Mediatek MT8183 SoC
From: Erin Lo @ 2019-03-11 8:54 UTC (permalink / raw)
To: Matthias Brugger, Rob Herring, Mark Rutland, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Greg Kroah-Hartman, Stephen Boyd
Cc: devicetree, srv_heupstream, linux-kernel, linux-serial,
linux-mediatek, linux-arm-kernel, yingjoe.chen, erin.lo,
mars.cheng, eddie.huang, linux-clk, zhiyong.tao, Mengqi.Zhang
MT8183 is a SoC based on 64bit ARMv8 architecture.
It contains 4 CA53 and 4 CA73 cores.
MT8183 share many HW IP with MT65xx series.
This patchset was tested on MT8183 evaluation board and use correct clock to shell.
This series contains document bindings, device tree including interrupt, uart, clock, and pwrap.
Based on v5.0-rc1 and
http://lists.infradead.org/pipermail/linux-mediatek/2018-December/016243.html
Change in v8:
1. Fix interrupt-parent of pio node
2. Remove pinfunc.h and spi node patches
Change in v7:
1. Place all the MMIO peripherals under one or more simple-bus nodes
2. Make the pinfunc.h and spi node into seperate patch
3. Modify SPIs pamerater from 4 back to 3
and remove patch "support 4 interrupt parameters for sysirq"
4. Rename intpol-controller to interrupt-controller
5. Rename pinctrl@1000b000 to pinctrl@10005000
Change in v6:
1. Remove power and iommu nodes
2. Fix dtb build warning
3. Fix pinctrl binding doc
4. Fix '_' in node names
Change in v5:
1. Collect all device tree nodes to the last patch
2. Add PMU
3. Add Signed-off-by
4. Remove clock driver code and binding doc
5. Add pinctrl, iommu, spi, and pwrap nodes
Change in v4:
1. Correct syntax error in dtsi
2. Add MT8183 clock support
Change in v3:
1. Fill out GICC, GICH, GICV regions
2. Update Copyright to 2018
Change in v2:
1. Split dt-bindings into different patches
2. Correct bindings for supported SoCs (mtk-uart.txt)
Ben Ho (1):
arm64: dts: Add Mediatek SoC MT8183 and evaluation board dts and
Makefile
Erin Lo (1):
dt-bindings: serial: Add compatible for Mediatek MT8183
.../devicetree/bindings/serial/mtk-uart.txt | 1 +
arch/arm64/boot/dts/mediatek/Makefile | 1 +
arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 31 ++
arch/arm64/boot/dts/mediatek/mt8183.dtsi | 335 +++++++++++++++++++++
4 files changed, 368 insertions(+)
create mode 100644 arch/arm64/boot/dts/mediatek/mt8183-evb.dts
create mode 100644 arch/arm64/boot/dts/mediatek/mt8183.dtsi
--
1.9.1
^ permalink raw reply
* Re: [RFC PATCH v1 25/25] printk: remove unused code
From: Sebastian Andrzej Siewior @ 2019-03-11 8:18 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: John Ogness, linux-kernel, Peter Zijlstra, Petr Mladek,
Steven Rostedt, Daniel Wang, Andrew Morton, Linus Torvalds,
Greg Kroah-Hartman, Alan Cox, Jiri Slaby, Peter Feiner,
linux-serial, Sergey Senozhatsky
In-Reply-To: <20190311024600.GA25608@jagdpanzerIV>
On 2019-03-11 11:46:00 [+0900], Sergey Senozhatsky wrote:
> On (03/08/19 15:02), Sebastian Andrzej Siewior wrote:
> > On 2019-02-12 15:30:03 [+0100], John Ogness wrote:
> >
> > you removed the whole `irq_work' thing. You can also remove the include
> > for linux/irq_work.h.
>
> It may be too early to remove the whole `irq_work' thing.
> printk()->call_console_driver() should take console_sem
> lock.
I would be _very_ glad to see that irq_work thingy gone. I just stumbled
upon this irq_work and cursed a little while doing other things.
Checking John's series and seeing that it was gone, was a relief.
Printing the whole thing in irq context does not look sane. printing the
import things right away and printing the remaining things later in
kthread looks good to me.
> -ss
Sebastian
^ permalink raw reply
* Re: [PATCH serial v2] sc16is7xx: missing unregister/delete driver on error in sc16is7xx_init()
From: Dan Carpenter @ 2019-03-11 7:46 UTC (permalink / raw)
To: Mao Wenan; +Cc: gregkh, jslaby, linux-serial, linux-kernel, kernel-janitors, vz
In-Reply-To: <20190309025037.116449-1-maowenan@huawei.com>
On Sat, Mar 09, 2019 at 10:50:37AM +0800, Mao Wenan wrote:
> Add the missing uart_unregister_driver() and i2c_del_driver() before return
> from sc16is7xx_init() in the error handling case.
>
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> v1->v2: fix compile warning if CONFIG_SERIAL_SC16IS7XX_SPI is not exist.
> drivers/tty/serial/sc16is7xx.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 268098681856..b9e5941ce767 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1509,7 +1509,7 @@ static int __init sc16is7xx_init(void)
> ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
> if (ret < 0) {
> pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
> - return ret;
> + goto err_i2c;
> }
> #endif
>
> @@ -1517,10 +1517,20 @@ static int __init sc16is7xx_init(void)
> ret = spi_register_driver(&sc16is7xx_spi_uart_driver);
> if (ret < 0) {
> pr_err("failed to init sc16is7xx spi --> %d\n", ret);
> - return ret;
> + goto err_spi;
> }
> #endif
> return ret;
> +
> +#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
> +err_spi:
> +#endif
> +#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
> + i2c_del_driver(&sc16is7xx_i2c_uart_driver);
> +err_i2c:
> +#endif
> + uart_unregister_driver(&sc16is7xx_uart);
> + return ret;
This is quite ulgy. Why not create two functions:
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 635178cf3eed..9986cb2479ad 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1491,6 +1491,21 @@ static struct i2c_driver sc16is7xx_i2c_uart_driver = {
.id_table = sc16is7xx_i2c_id_table,
};
+static int __init register_sc16is7xx_i2c(void)
+{
+ return i2c_add_driver(&sc16is7xx_i2c_uart_driver);
+}
+
+static void __exit del_sc16is7xx_i2c(void)
+{
+ i2c_del_driver(&sc16is7xx_i2c_uart_driver);
+}
+
+#else
+
+static int __init register_sc16is7xx_i2c(void) { return 0; }
+static void __exit del_sc16is7xx_i2c(void) {}
+
#endif
static int __init sc16is7xx_init(void)
Then you can remove all the ifdefs.
regards,
dan carpenter
^ permalink raw reply related
* Re: [PATCH] serial: sh-sci: Missing uart_unregister_driver() on error in sci_probe_single()
From: Dan Carpenter @ 2019-03-11 7:22 UTC (permalink / raw)
To: Mao Wenan; +Cc: gregkh, jslaby, linux-serial, kernel-janitors, linux-kernel
In-Reply-To: <20190308142310.112914-1-maowenan@huawei.com>
On Fri, Mar 08, 2019 at 10:23:10PM +0800, Mao Wenan wrote:
> Add the missing uart_unregister_driver() before return
> from sci_probe_single() in the error handling case.
>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> drivers/tty/serial/sh-sci.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 64bbeb7d7e0c..dde4eac9d222 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3254,8 +3254,10 @@ static int sci_probe_single(struct platform_device *dev,
> mutex_unlock(&sci_uart_registration_lock);
>
> ret = sci_init_single(dev, sciport, index, p, false);
> - if (ret)
> + if (ret) {
> + uart_unregister_driver(&sci_uart_driver);
> return ret;
Why only this error path and not the next one?
> + }
>
> sciport->gpios = mctrl_gpio_init(&sciport->port, 0);
> if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS)
^^^^^^^^^^^^^^^^^^^^^
This one.
regards,
dan carpenter
^ permalink raw reply
* Re: [RFC PATCH v1 25/25] printk: remove unused code
From: Sergey Senozhatsky @ 2019-03-11 2:46 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: John Ogness, linux-kernel, Peter Zijlstra, Petr Mladek,
Sergey Senozhatsky, Steven Rostedt, Daniel Wang, Andrew Morton,
Linus Torvalds, Greg Kroah-Hartman, Alan Cox, Jiri Slaby,
Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20190308140229.xje5ay7w2qtlada5@flow>
On (03/08/19 15:02), Sebastian Andrzej Siewior wrote:
> On 2019-02-12 15:30:03 [+0100], John Ogness wrote:
>
> you removed the whole `irq_work' thing. You can also remove the include
> for linux/irq_work.h.
It may be too early to remove the whole `irq_work' thing.
printk()->call_console_driver() should take console_sem
lock.
-ss
^ permalink raw reply
* Re: [PATCH v11 1/4] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Sean Wang @ 2019-03-11 0:31 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Nicolas Boichat, Matthias Brugger, Dan Williams,
Greg Kroah-Hartman, Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, linux-serial,
srv_heupstream, Yingjoe Chen, YT Shen, Zhenbao Liu
In-Reply-To: <1551923135-32479-2-git-send-email-long.cheng@mediatek.com>
Hi, Long
List some comments as the below and this week I will find a board to
test and then improve the driver.
Sean
On Wed, Mar 6, 2019 at 5:45 PM Long Cheng <long.cheng@mediatek.com> wrote:
>
> In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> the performance, can enable the function.
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> drivers/dma/mediatek/Kconfig | 11 +
> drivers/dma/mediatek/Makefile | 1 +
> drivers/dma/mediatek/mtk-uart-apdma.c | 660 +++++++++++++++++++++++++++++++++
> 3 files changed, 672 insertions(+)
> create mode 100644 drivers/dma/mediatek/mtk-uart-apdma.c
>
> diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> index 680fc05..ac49eb6 100644
> --- a/drivers/dma/mediatek/Kconfig
> +++ b/drivers/dma/mediatek/Kconfig
> @@ -24,3 +24,14 @@ config MTK_CQDMA
>
> This controller provides the channels which is dedicated to
> memory-to-memory transfer to offload from CPU.
> +
> +config MTK_UART_APDMA
> + tristate "MediaTek SoCs APDMA support for UART"
> + depends on OF && SERIAL_8250_MT6577
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Support for the UART DMA engine found on MediaTek MTK SoCs.
> + When SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
> + you can enable the config. The DMA engine can only be used
> + with MediaTek SoCs.
> diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
> index 41bb381..61a6d29 100644
> --- a/drivers/dma/mediatek/Makefile
> +++ b/drivers/dma/mediatek/Makefile
> @@ -1,2 +1,3 @@
> +obj-$(CONFIG_MTK_UART_APDMA) += mtk-uart-apdma.o
> obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> obj-$(CONFIG_MTK_CQDMA) += mtk-cqdma.o
> diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
> new file mode 100644
> index 0000000..9ed7a49
> --- /dev/null
> +++ b/drivers/dma/mediatek/mtk-uart-apdma.c
> @@ -0,0 +1,660 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MediaTek Uart APDMA driver.
> + *
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Long Cheng <long.cheng@mediatek.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "../virt-dma.h"
> +
> +/* The default number of virtual channel */
> +#define MTK_UART_APDMA_NR_VCHANS 8
> +
> +#define VFF_EN_B BIT(0)
> +#define VFF_STOP_B BIT(0)
> +#define VFF_FLUSH_B BIT(0)
> +#define VFF_4G_SUPPORT_B BIT(0)
> +#define VFF_RX_INT_EN0_B BIT(0) /* rx valid size >= vff thre */
> +#define VFF_RX_INT_EN1_B BIT(1)
> +#define VFF_TX_INT_EN_B BIT(0) /* tx left size >= vff thre */
> +#define VFF_WARM_RST_B BIT(0)
> +#define VFF_RX_INT_CLR_B (BIT(0) | BIT(1))
> +#define VFF_TX_INT_CLR_B 0
> +#define VFF_STOP_CLR_B 0
> +#define VFF_INT_EN_CLR_B 0
> +#define VFF_4G_SUPPORT_CLR_B 0
> +
> +/* 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 VFF_RING_SIZE 0xffffU
> +/* invert this bit when wrap ring head again */
> +#define VFF_RING_WRAP 0x10000U
> +
> +#define VFF_INT_FLAG 0x00
> +#define VFF_INT_EN 0x04
> +#define VFF_EN 0x08
> +#define VFF_RST 0x0c
> +#define VFF_STOP 0x10
> +#define VFF_FLUSH 0x14
> +#define VFF_ADDR 0x1c
> +#define VFF_LEN 0x24
> +#define VFF_THRE 0x28
> +#define VFF_WPT 0x2c
> +#define VFF_RPT 0x30
> +/* TX: the buffer size HW can read. RX: the buffer size SW can read. */
> +#define VFF_VALID_SIZE 0x3c
> +/* TX: the buffer size SW can write. RX: the buffer size HW can write. */
> +#define VFF_LEFT_SIZE 0x40
> +#define VFF_DEBUG_STATUS 0x50
> +#define VFF_4G_SUPPORT 0x54
> +
> +struct mtk_uart_apdmadev {
> + struct dma_device ddev;
> + struct clk *clk;
> + bool support_33bits;
> + unsigned int dma_requests;
> + unsigned int *dma_irq;
> +};
> +
> +struct mtk_uart_apdma_desc {
> + struct virt_dma_desc vd;
> +
> + unsigned int avail_len;
> +};
> +
> +struct mtk_chan {
> + struct virt_dma_chan vc;
> + struct dma_slave_config cfg;
> + void __iomem *base;
> + struct mtk_uart_apdma_desc *desc;
> +
> + enum dma_transfer_direction dir;
> +
> + bool requested;
> +
> + unsigned int rx_status;
> +};
> +
> +static inline struct mtk_uart_apdmadev *
> +to_mtk_uart_apdma_dev(struct dma_device *d)
> +{
> + return container_of(d, struct mtk_uart_apdmadev, ddev);
> +}
> +
> +static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
> +{
> + return container_of(c, struct mtk_chan, vc.chan);
> +}
> +
> +static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
> + (struct dma_async_tx_descriptor *t)
> +{
> + return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
> +}
> +
> +static void mtk_uart_apdma_write(struct mtk_chan *c,
> + unsigned int reg, unsigned int val)
> +{
> + writel(val, c->base + reg);
> +}
> +
> +static unsigned int mtk_uart_apdma_read(struct mtk_chan *c, unsigned int reg)
> +{
> + return readl(c->base + reg);
> +}
> +
> +static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
> +{
> + struct dma_chan *chan = vd->tx.chan;
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + kfree(c->desc);
> +}
> +
> +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> +{
> + unsigned int len, send, left, wpt, d_wpt, tmp;
> + int ret;
> +
> + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> + if (!left) {
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + return;
> + }
> +
> + /* Wait 1sec for flush, can't sleep */
> + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> + tmp != VFF_FLUSH_B, 0, 1000000);
It is really not a good idea that polling up to 1 second in an
interrupt context.
> + if (ret)
> + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + send = min_t(unsigned int, left, c->desc->avail_len);
> + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> + len = c->cfg.dst_port_window_size;
> +
> + d_wpt = wpt + send;
> + if ((d_wpt & VFF_RING_SIZE) >= len) {
I am confused what size of VFF is. Either VFF_RING_SIZE or
c->cfg.dst_port_window_size?
> + d_wpt = d_wpt - len;
> + d_wpt = d_wpt ^ VFF_RING_WRAP;
> + }
> + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> +
> + c->desc->avail_len -= send;
> +
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
Why should we need to program interrupt enabled bit again?
> + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> +}
> +
> +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> +{
> + struct mtk_uart_apdma_desc *d = c->desc;
> + unsigned int len, wg, rg;
> + int cnt;
> +
> + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> + !d || !vchan_next_desc(&c->vc))
> + return;
If the current descriptor is not available, the hardware should be
idle or stopped. so I think the condition can be removed or there is
somewhere your handle descriptors incorrectly.
> +
> + len = c->cfg.src_port_window_size;
> + rg = mtk_uart_apdma_read(c, VFF_RPT);
> + wg = mtk_uart_apdma_read(c, VFF_WPT);
> + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
Is it possible that rg and wg would be greater than VFF_RING_SIZE?
> + /*
> + * The buffer is ring buffer. If wrap bit different,
> + * represents the start of the next cycle for WPT
> + */
> + if ((rg ^ wg) & VFF_RING_WRAP)
> + cnt += len;
Again, I am confused what size of VFF is. Either VFF_RING_SIZE or
c->cfg.dst_port_window_size?
> +
> + c->rx_status = d->avail_len - cnt;
> + mtk_uart_apdma_write(c, VFF_RPT, wg);
> +
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> +}
> +
> +static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
> +{
> + struct dma_chan *chan = (struct dma_chan *)dev_id;
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->dir == DMA_DEV_TO_MEM) {
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_CLR_B);
> + mtk_uart_apdma_start_rx(c);
> + } else if (c->dir == DMA_MEM_TO_DEV) {
> + d = c->desc;
> +
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
> +
> + if (d->avail_len != 0U) {
> + mtk_uart_apdma_start_tx(c);
> + } else {
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> + }
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + unsigned int tmp;
> + int ret;
> +
> + pm_runtime_get_sync(mtkd->ddev.dev);
Add an error handling, something like
err = pm_runtime_get_sync(mtkd->ddev.dev);
if (err < 0) {
pm_runtime_put_noidle(dev);
...
}
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> + mtk_uart_apdma_write(c, VFF_THRE, 0);
> + mtk_uart_apdma_write(c, VFF_LEN, 0);
> + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> +
> + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp, !tmp, 10, 100);
> + if (ret) {
> + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> + return ret;
> + }
> +
> + if (!c->requested) {
> + c->requested = true;
The variable c->requested can be saved since the same channel
shouldn't be requested more one time
> + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> + KBUILD_MODNAME, chan);
> + if (ret < 0) {
> + dev_err(chan->device->dev, "Can't request dma IRQ\n");
> + return -EINVAL;
> + }
> + }
> +
> + if (mtkd->support_33bits)
> + mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> +
> + return ret;
> +}
> +
> +static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + if (c->requested) {
ditto as the above
> + c->requested = false;
> + free_irq(mtkd->dma_irq[chan->chan_id], chan);
> + }
> +
> + tasklet_kill(&c->vc.task);
> +
> + vchan_free_chan_resources(&c->vc);
> +
> + pm_runtime_put_sync(mtkd->ddev.dev);
> +}
> +
> +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + enum dma_status ret;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> +
> + dma_set_residue(txstate, c->rx_status);
> +
The handling is not enough. You should get the descriptor
corresponding to the cookie and then calculate and return the
->tx_status by the descriptor
> + return ret;
> +}
> +
> +static void mtk_uart_apdma_config_write(struct dma_chan *chan,
> + struct dma_slave_config *cfg,
> + enum dma_transfer_direction dir)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdmadev *mtkd =
> + to_mtk_uart_apdma_dev(c->vc.chan.device);
> + unsigned int tmp;
> +
> + if (mtk_uart_apdma_read(c, VFF_EN) == VFF_EN_B)
> + return;
> +
> + c->dir = dir;
The direction is fixed by the device, I don't think it is required to
keep it in a software state.
> +
> + if (dir == DMA_DEV_TO_MEM) {
> + tmp = cfg->src_port_window_size;
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, cfg->src_addr);
That is wrong. ->src_addr is the physical address where DMA slave data
should be read (RX), not the memory address.
You should program the register VFF_ADDR and VFF_LEN by sg address and
length from device_prep_slave_sg.
> + mtk_uart_apdma_write(c, VFF_LEN, tmp);
> + mtk_uart_apdma_write(c, VFF_THRE, VFF_RX_THRE(tmp));
> + mtk_uart_apdma_write(c, VFF_INT_EN,
> + VFF_RX_INT_EN0_B | VFF_RX_INT_EN1_B);
> + mtk_uart_apdma_write(c, VFF_RPT, 0);
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_CLR_B);
> + } else if (dir == DMA_MEM_TO_DEV) {
> + tmp = cfg->dst_port_window_size;
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, cfg->dst_addr);
That is also wrong. st_addr: this is the physical address where DMA
slave data should be written (TX), not the memory address similar to
the above explanation.
> + mtk_uart_apdma_write(c, VFF_LEN, tmp);
> + mtk_uart_apdma_write(c, VFF_THRE, VFF_TX_THRE(tmp));
> + mtk_uart_apdma_write(c, VFF_WPT, 0);
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
> + }
> +
> + mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
> +
> + if (mtkd->support_33bits)
> + mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> +
> + if (mtk_uart_apdma_read(c, VFF_EN) != VFF_EN_B)
> + dev_err(chan->device->dev, "dir[%d] fail\n", dir);
> +}
> +
> +/*
> + * dmaengine_prep_slave_single will call the function. and sglen is 1.
> + * 8250 uart using one ring buffer, and deal with one sg.
> + */
> +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> + (struct dma_chan *chan, struct scatterlist *sgl,
> + unsigned int sglen, enum dma_transfer_direction dir,
> + unsigned long tx_flags, void *context)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> +
> + if (!is_slave_direction(dir))
> + return NULL;
> +
> + mtk_uart_apdma_config_write(chan, &c->cfg, dir);
> +
> + /* Now allocate and setup the descriptor */
> + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> + if (!d)
> + return NULL;
> +
> + /* sglen is 1 */
> + d->avail_len = sg_dma_len(sgl);
> + c->rx_status = d->avail_len;
> +
> + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> +}
> +
> +static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (vchan_issue_pending(&c->vc)) {
> + vd = vchan_next_desc(&c->vc);
> + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> + }
> +
> + if (c->dir == DMA_DEV_TO_MEM)
> + mtk_uart_apdma_start_rx(c);
> + else if (c->dir == DMA_MEM_TO_DEV)
> + mtk_uart_apdma_start_tx(c);
> +
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +}
> +
> +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> + struct dma_slave_config *config)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + memcpy(&c->cfg, config, sizeof(*config));
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + unsigned long flags;
> + unsigned int tmp;
> + int ret;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> +
> + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> + /* Wait 1sec for flush, can't sleep */
> + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> + tmp != VFF_FLUSH_B, 0, 1000000);
It is extremely bad pending so long is in the spin_lock_irqsave
> + if (ret)
> + dev_err(c->vc.chan.device->dev, "flush: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + /* set stop as 1 -> wait until en is 0 -> set stop as 0 */
> + mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_B);
> + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp, !tmp, 10, 100);
> + if (ret)
> + dev_err(c->vc.chan.device->dev, "stop: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_CLR_B);
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> +
> + if (c->dir == DMA_DEV_TO_MEM)
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_CLR_B);
> + else if (c->dir == DMA_MEM_TO_DEV)
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
> +
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + dev_err(chan->device->dev, "Pause can't support\n");
> +
If the device can't support hardware pause, we can do it as a software
pause in an implementation based on vdesc.
> + return 0;
> +}
> +
> +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> +{
> + while (!list_empty(&mtkd->ddev.channels)) {
> + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> + struct mtk_chan, vc.chan.device_node);
> +
> + list_del(&c->vc.chan.device_node);
> + tasklet_kill(&c->vc.task);
> + }
> +}
> +
> +static const struct of_device_id mtk_uart_apdma_match[] = {
> + { .compatible = "mediatek,mt6577-uart-dma", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> +
> +static int mtk_uart_apdma_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct mtk_uart_apdmadev *mtkd;
> + struct resource *res;
> + struct mtk_chan *c;
> + int bit_mask = 32, rc;
> + unsigned int i;
> +
> + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> + if (!mtkd)
> + return -ENOMEM;
> +
> + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(mtkd->clk)) {
> + dev_err(&pdev->dev, "No clock specified\n");
> + rc = PTR_ERR(mtkd->clk);
> + return rc;
> + }
> +
> + if (of_property_read_bool(np, "mediatek,dma-33bits"))
> + mtkd->support_33bits = true;
> +
> + if (mtkd->support_33bits)
> + bit_mask = 33;
> +
> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(bit_mask));
> + if (rc)
> + return rc;
> +
> + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> + mtkd->ddev.device_alloc_chan_resources =
> + mtk_uart_apdma_alloc_chan_resources;
> + mtkd->ddev.device_free_chan_resources =
> + mtk_uart_apdma_free_chan_resources;
> + mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
> + mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
> + mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
> + mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
> + mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
> + mtkd->ddev.device_terminate_all = mtk_uart_apdma_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);
> +
> + mtkd->dma_requests = MTK_UART_APDMA_NR_VCHANS;
> + if (of_property_read_u32(np, "dma-requests", &mtkd->dma_requests)) {
> + dev_info(&pdev->dev,
> + "Using %u as missing dma-requests property\n",
> + MTK_UART_APDMA_NR_VCHANS);
> + }
> +
> + mtkd->dma_irq = devm_kcalloc(&pdev->dev, mtkd->dma_requests,
> + sizeof(*mtkd->dma_irq), GFP_KERNEL);
> + if (!mtkd->dma_irq)
> + return -ENOMEM;
> +
> + for (i = 0; i < mtkd->dma_requests; i++) {
> + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> + if (!c) {
> + rc = -ENODEV;
> + goto err_no_dma;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> + if (!res) {
> + rc = -ENODEV;
> + goto err_no_dma;
> + }
> +
> + c->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(c->base)) {
> + rc = PTR_ERR(c->base);
> + goto err_no_dma;
> + }
> + c->requested = false;
> + c->vc.desc_free = mtk_uart_apdma_desc_free;
> + vchan_init(&c->vc, &mtkd->ddev);
> +
> + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> + if ((int)mtkd->dma_irq[i] < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> + rc = -EINVAL;
> + goto err_no_dma;
> + }
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> +
> + rc = dma_async_device_register(&mtkd->ddev);
> + if (rc)
> + goto rpm_disable;
> +
> + platform_set_drvdata(pdev, mtkd);
> +
> + /* Device-tree DMA controller registration */
> + rc = of_dma_controller_register(np, of_dma_xlate_by_chan_id, mtkd);
> + if (rc)
> + goto dma_remove;
> +
> + return rc;
> +
> +dma_remove:
> + dma_async_device_unregister(&mtkd->ddev);
> +rpm_disable:
> + pm_runtime_disable(&pdev->dev);
> +err_no_dma:
> + mtk_uart_apdma_free(mtkd);
> + return rc;
> +}
> +
> +static int mtk_uart_apdma_remove(struct platform_device *pdev)
> +{
> + struct mtk_uart_apdmadev *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);
That pm_runtime_put_noidle should be removed or it causes an
inconsistency with the probe handler.
> +
> + dma_async_device_unregister(&mtkd->ddev);
> + mtk_uart_apdma_free(mtkd);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_uart_apdma_suspend(struct device *dev)
> +{
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev))
> + clk_disable_unprepare(mtkd->clk);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev)) {
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM
> +static int mtk_uart_apdma_runtime_suspend(struct device *dev)
> +{
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(mtkd->clk);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_runtime_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
> + SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
> + mtk_uart_apdma_runtime_resume, NULL)
> +};
It probably causes a build error when CONFIG_PM is not enabled.
and use a UNIVERSAL_DEV_PM_OPS because the runtime suspend/resume and
system suspend/resume for the dma are
almost the same.
> +
> +static struct platform_driver mtk_uart_apdma_driver = {
> + .probe = mtk_uart_apdma_probe,
> + .remove = mtk_uart_apdma_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .pm = &mtk_uart_apdma_pm_ops,
> + .of_match_table = of_match_ptr(mtk_uart_apdma_match),
> + },
> +};
> +
> +module_platform_driver(mtk_uart_apdma_driver);
> +
> +MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
> +MODULE_AUTHOR("Long Cheng <long.cheng@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> +
> --
> 1.7.9.5
>
^ permalink raw reply
* Re: [PATCH v11 1/4] dmaengine: 8250_mtk_dma: add MediaTek uart DMA support
From: Nicolas Boichat @ 2019-03-10 11:15 UTC (permalink / raw)
To: Long Cheng
Cc: Vinod Koul, Randy Dunlap, Rob Herring, Mark Rutland, Ryder Lee,
Sean Wang, Matthias Brugger, Dan Williams, Greg Kroah-Hartman,
Jiri Slaby, Sean Wang, dmaengine, devicetree,
linux-arm Mailing List, moderated list:ARM/Mediatek SoC support,
lkml, linux-serial, srv_heupstream, Yingjoe Chen
In-Reply-To: <1551923135-32479-2-git-send-email-long.cheng@mediatek.com>
On Thu, Mar 7, 2019 at 9:45 AM Long Cheng <long.cheng@mediatek.com> wrote:
>
> In DMA engine framework, add 8250 uart dma to support MediaTek uart.
> If MediaTek uart enabled(SERIAL_8250_MT6577), and want to improve
> the performance, can enable the function.
>
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
> drivers/dma/mediatek/Kconfig | 11 +
> drivers/dma/mediatek/Makefile | 1 +
> drivers/dma/mediatek/mtk-uart-apdma.c | 660 +++++++++++++++++++++++++++++++++
> 3 files changed, 672 insertions(+)
> create mode 100644 drivers/dma/mediatek/mtk-uart-apdma.c
>
> diff --git a/drivers/dma/mediatek/Kconfig b/drivers/dma/mediatek/Kconfig
> index 680fc05..ac49eb6 100644
> --- a/drivers/dma/mediatek/Kconfig
> +++ b/drivers/dma/mediatek/Kconfig
> @@ -24,3 +24,14 @@ config MTK_CQDMA
>
> This controller provides the channels which is dedicated to
> memory-to-memory transfer to offload from CPU.
> +
> +config MTK_UART_APDMA
> + tristate "MediaTek SoCs APDMA support for UART"
> + depends on OF && SERIAL_8250_MT6577
> + select DMA_ENGINE
> + select DMA_VIRTUAL_CHANNELS
> + help
> + Support for the UART DMA engine found on MediaTek MTK SoCs.
> + When SERIAL_8250_MT6577 is enabled, and if you want to use DMA,
> + you can enable the config. The DMA engine can only be used
> + with MediaTek SoCs.
> diff --git a/drivers/dma/mediatek/Makefile b/drivers/dma/mediatek/Makefile
> index 41bb381..61a6d29 100644
> --- a/drivers/dma/mediatek/Makefile
> +++ b/drivers/dma/mediatek/Makefile
> @@ -1,2 +1,3 @@
> +obj-$(CONFIG_MTK_UART_APDMA) += mtk-uart-apdma.o
> obj-$(CONFIG_MTK_HSDMA) += mtk-hsdma.o
> obj-$(CONFIG_MTK_CQDMA) += mtk-cqdma.o
> diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
> new file mode 100644
> index 0000000..9ed7a49
> --- /dev/null
> +++ b/drivers/dma/mediatek/mtk-uart-apdma.c
> @@ -0,0 +1,660 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MediaTek Uart APDMA driver.
> + *
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Long Cheng <long.cheng@mediatek.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_dma.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "../virt-dma.h"
> +
> +/* The default number of virtual channel */
> +#define MTK_UART_APDMA_NR_VCHANS 8
> +
> +#define VFF_EN_B BIT(0)
> +#define VFF_STOP_B BIT(0)
> +#define VFF_FLUSH_B BIT(0)
> +#define VFF_4G_SUPPORT_B BIT(0)
> +#define VFF_RX_INT_EN0_B BIT(0) /* rx valid size >= vff thre */
> +#define VFF_RX_INT_EN1_B BIT(1)
> +#define VFF_TX_INT_EN_B BIT(0) /* tx left size >= vff thre */
> +#define VFF_WARM_RST_B BIT(0)
> +#define VFF_RX_INT_CLR_B (BIT(0) | BIT(1))
> +#define VFF_TX_INT_CLR_B 0
> +#define VFF_STOP_CLR_B 0
> +#define VFF_INT_EN_CLR_B 0
> +#define VFF_4G_SUPPORT_CLR_B 0
> +
> +/* 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 VFF_RING_SIZE 0xffffU
Drop the U, it's not very useful (there are a a few more below, grep
for [0-9a-f]U).
> +/* invert this bit when wrap ring head again */
> +#define VFF_RING_WRAP 0x10000U
> +
> +#define VFF_INT_FLAG 0x00
> +#define VFF_INT_EN 0x04
> +#define VFF_EN 0x08
> +#define VFF_RST 0x0c
> +#define VFF_STOP 0x10
> +#define VFF_FLUSH 0x14
> +#define VFF_ADDR 0x1c
> +#define VFF_LEN 0x24
> +#define VFF_THRE 0x28
> +#define VFF_WPT 0x2c
> +#define VFF_RPT 0x30
> +/* TX: the buffer size HW can read. RX: the buffer size SW can read. */
> +#define VFF_VALID_SIZE 0x3c
> +/* TX: the buffer size SW can write. RX: the buffer size HW can write. */
> +#define VFF_LEFT_SIZE 0x40
> +#define VFF_DEBUG_STATUS 0x50
> +#define VFF_4G_SUPPORT 0x54
> +
> +struct mtk_uart_apdmadev {
> + struct dma_device ddev;
> + struct clk *clk;
> + bool support_33bits;
> + unsigned int dma_requests;
> + unsigned int *dma_irq;
> +};
> +
> +struct mtk_uart_apdma_desc {
> + struct virt_dma_desc vd;
> +
> + unsigned int avail_len;
> +};
> +
> +struct mtk_chan {
> + struct virt_dma_chan vc;
> + struct dma_slave_config cfg;
> + void __iomem *base;
> + struct mtk_uart_apdma_desc *desc;
> +
> + enum dma_transfer_direction dir;
> +
> + bool requested;
> +
> + unsigned int rx_status;
> +};
> +
> +static inline struct mtk_uart_apdmadev *
> +to_mtk_uart_apdma_dev(struct dma_device *d)
> +{
> + return container_of(d, struct mtk_uart_apdmadev, ddev);
> +}
> +
> +static inline struct mtk_chan *to_mtk_uart_apdma_chan(struct dma_chan *c)
> +{
> + return container_of(c, struct mtk_chan, vc.chan);
> +}
> +
> +static inline struct mtk_uart_apdma_desc *to_mtk_uart_apdma_desc
> + (struct dma_async_tx_descriptor *t)
> +{
> + return container_of(t, struct mtk_uart_apdma_desc, vd.tx);
> +}
> +
> +static void mtk_uart_apdma_write(struct mtk_chan *c,
> + unsigned int reg, unsigned int val)
> +{
> + writel(val, c->base + reg);
> +}
> +
> +static unsigned int mtk_uart_apdma_read(struct mtk_chan *c, unsigned int reg)
> +{
> + return readl(c->base + reg);
> +}
> +
> +static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
> +{
> + struct dma_chan *chan = vd->tx.chan;
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + kfree(c->desc);
> +}
> +
> +static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
> +{
> + unsigned int len, send, left, wpt, d_wpt, tmp;
> + int ret;
> +
> + left = mtk_uart_apdma_read(c, VFF_LEFT_SIZE);
> + if (!left) {
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + return;
> + }
> +
> + /* Wait 1sec for flush, can't sleep */
> + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> + tmp != VFF_FLUSH_B, 0, 1000000);
> + if (ret)
> + dev_warn(c->vc.chan.device->dev, "tx: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + send = min_t(unsigned int, left, c->desc->avail_len);
> + wpt = mtk_uart_apdma_read(c, VFF_WPT);
> + len = c->cfg.dst_port_window_size;
> +
> + d_wpt = wpt + send;
> + if ((d_wpt & VFF_RING_SIZE) >= len) {
> + d_wpt = d_wpt - len;
> + d_wpt = d_wpt ^ VFF_RING_WRAP;
> + }
> + mtk_uart_apdma_write(c, VFF_WPT, d_wpt);
> +
> + c->desc->avail_len -= send;
> +
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
> + if (mtk_uart_apdma_read(c, VFF_FLUSH) == 0U)
> + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> +}
> +
> +static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
> +{
> + struct mtk_uart_apdma_desc *d = c->desc;
> + unsigned int len, wg, rg;
> + int cnt;
> +
> + if ((mtk_uart_apdma_read(c, VFF_VALID_SIZE) == 0U) ||
> + !d || !vchan_next_desc(&c->vc))
> + return;
> +
> + len = c->cfg.src_port_window_size;
> + rg = mtk_uart_apdma_read(c, VFF_RPT);
> + wg = mtk_uart_apdma_read(c, VFF_WPT);
> + cnt = (wg & VFF_RING_SIZE) - (rg & VFF_RING_SIZE);
> + /*
> + * The buffer is ring buffer. If wrap bit different,
> + * represents the start of the next cycle for WPT
> + */
> + if ((rg ^ wg) & VFF_RING_WRAP)
> + cnt += len;
> +
> + c->rx_status = d->avail_len - cnt;
> + mtk_uart_apdma_write(c, VFF_RPT, wg);
> +
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> +}
> +
> +static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
> +{
> + struct dma_chan *chan = (struct dma_chan *)dev_id;
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (c->dir == DMA_DEV_TO_MEM) {
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_CLR_B);
> + mtk_uart_apdma_start_rx(c);
> + } else if (c->dir == DMA_MEM_TO_DEV) {
> + d = c->desc;
> +
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
> +
> + if (d->avail_len != 0U) {
> + mtk_uart_apdma_start_tx(c);
> + } else {
> + list_del(&d->vd.node);
> + vchan_cookie_complete(&d->vd);
> + }
> + }
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int mtk_uart_apdma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + unsigned int tmp;
> + int ret;
> +
> + pm_runtime_get_sync(mtkd->ddev.dev);
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, 0);
> + mtk_uart_apdma_write(c, VFF_THRE, 0);
> + mtk_uart_apdma_write(c, VFF_LEN, 0);
> + mtk_uart_apdma_write(c, VFF_RST, VFF_WARM_RST_B);
> +
> + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp, !tmp, 10, 100);
> + if (ret) {
> + dev_err(chan->device->dev, "dma reset: fail, timeout\n");
> + return ret;
> + }
> +
> + if (!c->requested) {
> + c->requested = true;
> + ret = request_irq(mtkd->dma_irq[chan->chan_id],
> + mtk_uart_apdma_irq_handler, IRQF_TRIGGER_NONE,
> + KBUILD_MODNAME, chan);
> + if (ret < 0) {
> + dev_err(chan->device->dev, "Can't request dma IRQ\n");
> + return -EINVAL;
> + }
> + }
> +
> + if (mtkd->support_33bits)
> + mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> +
> + return ret;
> +}
> +
> +static void mtk_uart_apdma_free_chan_resources(struct dma_chan *chan)
> +{
> + struct mtk_uart_apdmadev *mtkd = to_mtk_uart_apdma_dev(chan->device);
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + if (c->requested) {
> + c->requested = false;
> + free_irq(mtkd->dma_irq[chan->chan_id], chan);
> + }
> +
> + tasklet_kill(&c->vc.task);
> +
> + vchan_free_chan_resources(&c->vc);
> +
> + pm_runtime_put_sync(mtkd->ddev.dev);
> +}
> +
> +static enum dma_status mtk_uart_apdma_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + enum dma_status ret;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> +
> + dma_set_residue(txstate, c->rx_status);
> +
> + return ret;
> +}
> +
> +static void mtk_uart_apdma_config_write(struct dma_chan *chan,
> + struct dma_slave_config *cfg,
> + enum dma_transfer_direction dir)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdmadev *mtkd =
> + to_mtk_uart_apdma_dev(c->vc.chan.device);
> + unsigned int tmp;
> +
> + if (mtk_uart_apdma_read(c, VFF_EN) == VFF_EN_B)
> + return;
> +
> + c->dir = dir;
> +
> + if (dir == DMA_DEV_TO_MEM) {
> + tmp = cfg->src_port_window_size;
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, cfg->src_addr);
> + mtk_uart_apdma_write(c, VFF_LEN, tmp);
> + mtk_uart_apdma_write(c, VFF_THRE, VFF_RX_THRE(tmp));
> + mtk_uart_apdma_write(c, VFF_INT_EN,
> + VFF_RX_INT_EN0_B | VFF_RX_INT_EN1_B);
> + mtk_uart_apdma_write(c, VFF_RPT, 0);
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_CLR_B);
> + } else if (dir == DMA_MEM_TO_DEV) {
> + tmp = cfg->dst_port_window_size;
> +
> + mtk_uart_apdma_write(c, VFF_ADDR, cfg->dst_addr);
> + mtk_uart_apdma_write(c, VFF_LEN, tmp);
> + mtk_uart_apdma_write(c, VFF_THRE, VFF_TX_THRE(tmp));
> + mtk_uart_apdma_write(c, VFF_WPT, 0);
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
> + }
> +
> + mtk_uart_apdma_write(c, VFF_EN, VFF_EN_B);
> +
> + if (mtkd->support_33bits)
> + mtk_uart_apdma_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> +
> + if (mtk_uart_apdma_read(c, VFF_EN) != VFF_EN_B)
> + dev_err(chan->device->dev, "dir[%d] fail\n", dir);
> +}
> +
> +/*
> + * dmaengine_prep_slave_single will call the function. and sglen is 1.
> + * 8250 uart using one ring buffer, and deal with one sg.
> + */
> +static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
> + (struct dma_chan *chan, struct scatterlist *sgl,
> + unsigned int sglen, enum dma_transfer_direction dir,
> + unsigned long tx_flags, void *context)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct mtk_uart_apdma_desc *d;
> +
> + if (!is_slave_direction(dir))
> + return NULL;
> +
> + mtk_uart_apdma_config_write(chan, &c->cfg, dir);
> +
> + /* Now allocate and setup the descriptor */
> + d = kzalloc(sizeof(*d), GFP_ATOMIC);
> + if (!d)
> + return NULL;
> +
> + /* sglen is 1 */
> + d->avail_len = sg_dma_len(sgl);
> + c->rx_status = d->avail_len;
> +
> + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> +}
> +
> +static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> + if (vchan_issue_pending(&c->vc)) {
> + vd = vchan_next_desc(&c->vc);
> + c->desc = to_mtk_uart_apdma_desc(&vd->tx);
> + }
> +
> + if (c->dir == DMA_DEV_TO_MEM)
> + mtk_uart_apdma_start_rx(c);
> + else if (c->dir == DMA_MEM_TO_DEV)
> + mtk_uart_apdma_start_tx(c);
> +
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +}
> +
> +static int mtk_uart_apdma_slave_config(struct dma_chan *chan,
> + struct dma_slave_config *config)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> +
> + memcpy(&c->cfg, config, sizeof(*config));
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_terminate_all(struct dma_chan *chan)
> +{
> + struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
> + unsigned long flags;
> + unsigned int tmp;
> + int ret;
> +
> + spin_lock_irqsave(&c->vc.lock, flags);
> +
> + mtk_uart_apdma_write(c, VFF_FLUSH, VFF_FLUSH_B);
> + /* Wait 1sec for flush, can't sleep */
> + ret = readx_poll_timeout(readl, c->base + VFF_FLUSH, tmp,
> + tmp != VFF_FLUSH_B, 0, 1000000);
> + if (ret)
> + dev_err(c->vc.chan.device->dev, "flush: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + /* set stop as 1 -> wait until en is 0 -> set stop as 0 */
> + mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_B);
> + ret = readx_poll_timeout(readl, c->base + VFF_EN, tmp, !tmp, 10, 100);
> + if (ret)
> + dev_err(c->vc.chan.device->dev, "stop: fail, debug=0x%x\n",
> + mtk_uart_apdma_read(c, VFF_DEBUG_STATUS));
> +
> + mtk_uart_apdma_write(c, VFF_STOP, VFF_STOP_CLR_B);
> + mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> +
> + if (c->dir == DMA_DEV_TO_MEM)
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_RX_INT_CLR_B);
> + else if (c->dir == DMA_MEM_TO_DEV)
> + mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
> +
> + spin_unlock_irqrestore(&c->vc.lock, flags);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_device_pause(struct dma_chan *chan)
> +{
> + /* just for check caps pass */
> + dev_err(chan->device->dev, "Pause can't support\n");
> +
> + return 0;
> +}
I think we've said repeatedly that leaving this stub function is incorrect.
> +
> +static void mtk_uart_apdma_free(struct mtk_uart_apdmadev *mtkd)
> +{
> + while (!list_empty(&mtkd->ddev.channels)) {
> + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> + struct mtk_chan, vc.chan.device_node);
> +
> + list_del(&c->vc.chan.device_node);
> + tasklet_kill(&c->vc.task);
> + }
> +}
> +
> +static const struct of_device_id mtk_uart_apdma_match[] = {
> + { .compatible = "mediatek,mt6577-uart-dma", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_uart_apdma_match);
> +
> +static int mtk_uart_apdma_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct mtk_uart_apdmadev *mtkd;
> + struct resource *res;
> + struct mtk_chan *c;
> + int bit_mask = 32, rc;
> + unsigned int i;
> +
> + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> + if (!mtkd)
> + return -ENOMEM;
> +
> + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(mtkd->clk)) {
> + dev_err(&pdev->dev, "No clock specified\n");
> + rc = PTR_ERR(mtkd->clk);
> + return rc;
> + }
> +
> + if (of_property_read_bool(np, "mediatek,dma-33bits"))
> + mtkd->support_33bits = true;
> +
> + if (mtkd->support_33bits)
> + bit_mask = 33;
> +
> + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(bit_mask));
> + if (rc)
> + return rc;
> +
> + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> + mtkd->ddev.device_alloc_chan_resources =
> + mtk_uart_apdma_alloc_chan_resources;
> + mtkd->ddev.device_free_chan_resources =
> + mtk_uart_apdma_free_chan_resources;
> + mtkd->ddev.device_tx_status = mtk_uart_apdma_tx_status;
> + mtkd->ddev.device_issue_pending = mtk_uart_apdma_issue_pending;
> + mtkd->ddev.device_prep_slave_sg = mtk_uart_apdma_prep_slave_sg;
> + mtkd->ddev.device_config = mtk_uart_apdma_slave_config;
> + mtkd->ddev.device_pause = mtk_uart_apdma_device_pause;
> + mtkd->ddev.device_terminate_all = mtk_uart_apdma_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);
> +
> + mtkd->dma_requests = MTK_UART_APDMA_NR_VCHANS;
> + if (of_property_read_u32(np, "dma-requests", &mtkd->dma_requests)) {
> + dev_info(&pdev->dev,
> + "Using %u as missing dma-requests property\n",
> + MTK_UART_APDMA_NR_VCHANS);
> + }
> +
> + mtkd->dma_irq = devm_kcalloc(&pdev->dev, mtkd->dma_requests,
> + sizeof(*mtkd->dma_irq), GFP_KERNEL);
> + if (!mtkd->dma_irq)
> + return -ENOMEM;
> +
> + for (i = 0; i < mtkd->dma_requests; i++) {
> + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> + if (!c) {
> + rc = -ENODEV;
> + goto err_no_dma;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> + if (!res) {
> + rc = -ENODEV;
> + goto err_no_dma;
> + }
> +
> + c->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(c->base)) {
> + rc = PTR_ERR(c->base);
> + goto err_no_dma;
> + }
> + c->requested = false;
> + c->vc.desc_free = mtk_uart_apdma_desc_free;
> + vchan_init(&c->vc, &mtkd->ddev);
> +
> + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> + if ((int)mtkd->dma_irq[i] < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> + rc = -EINVAL;
> + goto err_no_dma;
> + }
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> +
> + rc = dma_async_device_register(&mtkd->ddev);
> + if (rc)
> + goto rpm_disable;
> +
> + platform_set_drvdata(pdev, mtkd);
> +
> + /* Device-tree DMA controller registration */
> + rc = of_dma_controller_register(np, of_dma_xlate_by_chan_id, mtkd);
> + if (rc)
> + goto dma_remove;
> +
> + return rc;
> +
> +dma_remove:
> + dma_async_device_unregister(&mtkd->ddev);
> +rpm_disable:
> + pm_runtime_disable(&pdev->dev);
> +err_no_dma:
> + mtk_uart_apdma_free(mtkd);
> + return rc;
> +}
> +
> +static int mtk_uart_apdma_remove(struct platform_device *pdev)
> +{
> + struct mtk_uart_apdmadev *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_uart_apdma_free(mtkd);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int mtk_uart_apdma_suspend(struct device *dev)
> +{
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev))
> + clk_disable_unprepare(mtkd->clk);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + if (!pm_runtime_suspended(dev)) {
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM
> +static int mtk_uart_apdma_runtime_suspend(struct device *dev)
> +{
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(mtkd->clk);
> +
> + return 0;
> +}
> +
> +static int mtk_uart_apdma_runtime_resume(struct device *dev)
> +{
> + int ret;
> + struct mtk_uart_apdmadev *mtkd = dev_get_drvdata(dev);
> +
> + ret = clk_prepare_enable(mtkd->clk);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops mtk_uart_apdma_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(mtk_uart_apdma_suspend, mtk_uart_apdma_resume)
> + SET_RUNTIME_PM_OPS(mtk_uart_apdma_runtime_suspend,
> + mtk_uart_apdma_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver mtk_uart_apdma_driver = {
> + .probe = mtk_uart_apdma_probe,
> + .remove = mtk_uart_apdma_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .pm = &mtk_uart_apdma_pm_ops,
> + .of_match_table = of_match_ptr(mtk_uart_apdma_match),
> + },
> +};
> +
> +module_platform_driver(mtk_uart_apdma_driver);
> +
> +MODULE_DESCRIPTION("MediaTek UART APDMA Controller Driver");
> +MODULE_AUTHOR("Long Cheng <long.cheng@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> +
> --
> 1.7.9.5
>
^ permalink raw reply
* [PATCH serial v2] sc16is7xx: missing unregister/delete driver on error in sc16is7xx_init()
From: Mao Wenan @ 2019-03-09 2:50 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, linux-kernel, kernel-janitors, vz
Add the missing uart_unregister_driver() and i2c_del_driver() before return
from sc16is7xx_init() in the error handling case.
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
v1->v2: fix compile warning if CONFIG_SERIAL_SC16IS7XX_SPI is not exist.
drivers/tty/serial/sc16is7xx.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 268098681856..b9e5941ce767 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1509,7 +1509,7 @@ static int __init sc16is7xx_init(void)
ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
if (ret < 0) {
pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
- return ret;
+ goto err_i2c;
}
#endif
@@ -1517,10 +1517,20 @@ static int __init sc16is7xx_init(void)
ret = spi_register_driver(&sc16is7xx_spi_uart_driver);
if (ret < 0) {
pr_err("failed to init sc16is7xx spi --> %d\n", ret);
- return ret;
+ goto err_spi;
}
#endif
return ret;
+
+#ifdef CONFIG_SERIAL_SC16IS7XX_SPI
+err_spi:
+#endif
+#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
+ i2c_del_driver(&sc16is7xx_i2c_uart_driver);
+err_i2c:
+#endif
+ uart_unregister_driver(&sc16is7xx_uart);
+ return ret;
}
module_init(sc16is7xx_init);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH serial] sc16is7xx: missing unregister/delete driver on error in sc16is7xx_init()
From: Vladimir Zapolskiy @ 2019-03-08 20:26 UTC (permalink / raw)
To: Mao Wenan, gregkh, jslaby, linux-serial, kernel-janitors,
linux-kernel
In-Reply-To: <20190308140933.55262-1-maowenan@huawei.com>
On 03/08/2019 04:09 PM, Mao Wenan wrote:
> Add the missing uart_unregister_driver() and i2c_del_driver() before return
> from sc16is7xx_init() in the error handling case.
>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> drivers/tty/serial/sc16is7xx.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 268098681856..114e94f476c6 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1509,7 +1509,7 @@ static int __init sc16is7xx_init(void)
> ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
> if (ret < 0) {
> pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
> - return ret;
> + goto err_i2c;
> }
> #endif
>
> @@ -1517,10 +1517,18 @@ static int __init sc16is7xx_init(void)
> ret = spi_register_driver(&sc16is7xx_spi_uart_driver);
> if (ret < 0) {
> pr_err("failed to init sc16is7xx spi --> %d\n", ret);
> - return ret;
> + goto err_spi;
> }
> #endif
> return ret;
> +
> +err_spi:
> +#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
> + i2c_del_driver(&sc16is7xx_i2c_uart_driver);
> +#endif
> +err_i2c:
> + uart_unregister_driver(&sc16is7xx_uart);
> + return ret;
> }
> module_init(sc16is7xx_init);
>
>
Nice catch, thank you!
Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
--
Best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH] tty: serial: qcom_geni_serial: Initialize baud in qcom_geni_console_setup
From: Nick Desaulniers @ 2019-03-08 18:40 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Andy Gross, David Brown, Greg Kroah-Hartman, linux-arm-msm,
linux-serial, LKML, clang-built-linux
In-Reply-To: <20190308183743.11145-1-natechancellor@gmail.com>
On Fri, Mar 8, 2019 at 10:38 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/tty/serial/qcom_geni_serial.c:1079:6: warning: variable 'baud'
> is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
>
> It's not wrong; when options is NULL, baud has no default value. Use
> 9600 as that is a sane default.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/395
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Great find. Thanks for the fix, and thanks Greg for the advice and
additional analysis here.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3bcec1c20219..35e5f9c5d5be 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1050,7 +1050,7 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
> {
> struct uart_port *uport;
> struct qcom_geni_serial_port *port;
> - int baud;
> + int baud = 9600;
> int bits = 8;
> int parity = 'n';
> int flow = 'n';
> --
> 2.21.0
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* [PATCH] tty: serial: qcom_geni_serial: Initialize baud in qcom_geni_console_setup
From: Nathan Chancellor @ 2019-03-08 18:37 UTC (permalink / raw)
To: Andy Gross, David Brown, Greg Kroah-Hartman
Cc: linux-arm-msm, linux-serial, linux-kernel, Nick Desaulniers,
clang-built-linux, Nathan Chancellor
In-Reply-To: <20190308084022.GA31474@kroah.com>
When building with -Wsometimes-uninitialized, Clang warns:
drivers/tty/serial/qcom_geni_serial.c:1079:6: warning: variable 'baud'
is used uninitialized whenever 'if' condition is false
[-Wsometimes-uninitialized]
It's not wrong; when options is NULL, baud has no default value. Use
9600 as that is a sane default.
Link: https://github.com/ClangBuiltLinux/linux/issues/395
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/tty/serial/qcom_geni_serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3bcec1c20219..35e5f9c5d5be 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1050,7 +1050,7 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
{
struct uart_port *uport;
struct qcom_geni_serial_port *port;
- int baud;
+ int baud = 9600;
int bits = 8;
int parity = 'n';
int flow = 'n';
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 3/4] printk: Add consoles to a virtual "console" bus
From: Greg Kroah-Hartman @ 2019-03-08 16:34 UTC (permalink / raw)
To: Petr Mladek
Cc: Calvin Owens, John Ogness, Sergey Senozhatsky, Steven Rostedt,
Jonathan Corbet, linux-kernel, linux-serial
In-Reply-To: <20190308155814.rzsjrzgwfcm4p5sk@pathway.suse.cz>
On Fri, Mar 08, 2019 at 04:58:14PM +0100, Petr Mladek wrote:
> On Fri 2019-03-08 03:56:19, John Ogness wrote:
> > On 2019-03-02, Calvin Owens <calvinowens@fb.com> wrote:
> > > This patch embeds a device struct in the console struct, and registers
> > > them on a "console" bus so we can expose attributes in sysfs.
> >
> > I expect that "class" would be more appropriate than "bus". These
> > devices really are grouped together based on their function and not the
> > medium by which they are accessed.
>
> Good point. "class" looks better to me as well.
>
> Greg, any opinion, where to put the entries for struct console ?
Hang them off of the device that the console belongs to?
Classes and busses are almost identical except:
- busses is the binding of a driver to a device (usb, pci, etc.)
- classes are usually userspace interactions to a device (input,
tty, etc.)
So this sounds like a class to me.
If you want me to review this, I'll be glad to so do once 5.1-rc1 is
out...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/4] printk: Add consoles to a virtual "console" bus
From: Petr Mladek @ 2019-03-08 15:58 UTC (permalink / raw)
Cc: Calvin Owens, John Ogness, Sergey Senozhatsky, Steven Rostedt,
Greg Kroah-Hartman, Jonathan Corbet, linux-kernel, linux-serial
In-Reply-To: <87ef7ioyzw.fsf@linutronix.de>
On Fri 2019-03-08 03:56:19, John Ogness wrote:
> On 2019-03-02, Calvin Owens <calvinowens@fb.com> wrote:
> > This patch embeds a device struct in the console struct, and registers
> > them on a "console" bus so we can expose attributes in sysfs.
>
> I expect that "class" would be more appropriate than "bus". These
> devices really are grouped together based on their function and not the
> medium by which they are accessed.
Good point. "class" looks better to me as well.
Greg, any opinion, where to put the entries for struct console ?
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH 3/4] printk: Add consoles to a virtual "console" bus
From: Petr Mladek @ 2019-03-08 15:53 UTC (permalink / raw)
To: Calvin Owens
Cc: Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Jonathan Corbet, linux-kernel, linux-serial
In-Reply-To: <087b13f7812b32cc7c3f9efea71c9bcf324dd031.1551486732.git.calvinowens@fb.com>
On Fri 2019-03-01 16:48:19, Calvin Owens wrote:
> This patch embeds a device struct in the console struct, and registers
> them on a "console" bus so we can expose attributes in sysfs.
>
> Early console structures must still be static, since they're required
> before we're able to allocate memory. The least ugly way I can come up
> with to handle this is an "is_static" flag in the structure which makes
> the gets and puts NOPs, and is checked in ->release() to catch mistakes.
I wonder if it might get detected by is_kernel_inittext().
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH 2/4] printk: Add ability to set loglevel via "console=" cmdline
From: Petr Mladek @ 2019-03-08 15:44 UTC (permalink / raw)
To: Calvin Owens
Cc: Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Jonathan Corbet, linux-kernel, linux-serial
In-Reply-To: <e1311e98d6803f678cdcaa2be1cfb58d667fe3fd.1551486732.git.calvinowens@fb.com>
On Fri 2019-03-01 16:48:18, Calvin Owens wrote:
> This extends the "console=" interface to allow setting the per-console
> loglevel by adding "/N" to the string, where N is the desired loglevel
> expressed as a base 10 integer. Invalid values are silently ignored.
>
> Signed-off-by: Calvin Owens <calvinowens@fb.com>
> ---
> .../admin-guide/kernel-parameters.txt | 6 ++--
> kernel/printk/console_cmdline.h | 1 +
> kernel/printk/printk.c | 30 +++++++++++++++----
> 3 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 858b6c0b9a15..afada61dcbce 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -612,10 +612,10 @@
> ttyS<n>[,options]
> ttyUSB0[,options]
> Use the specified serial port. The options are of
> - the form "bbbbpnf", where "bbbb" is the baud rate,
> + the form "bbbbpnf/l", where "bbbb" is the baud rate,
> "p" is parity ("n", "o", or "e"), "n" is number of
> - bits, and "f" is flow control ("r" for RTS or
> - omit it). Default is "9600n8".
> + bits, "f" is flow control ("r" for RTS or omit it),
> + and "l" is the loglevel on [0,7]. Default is "9600n8".
We should a more detailed explanation about the loglevel semantic. It
is either minimal loglevel. Or that it overrides the global
console_loglevel if you accept my proposal.
>
> See Documentation/admin-guide/serial-console.rst for more
> information. See
> diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
> index 11f19c466af5..fbf9b539366e 100644
> --- a/kernel/printk/console_cmdline.h
> +++ b/kernel/printk/console_cmdline.h
> @@ -6,6 +6,7 @@ struct console_cmdline
> {
> char name[16]; /* Name of the driver */
> int index; /* Minor dev. to use */
> + int loglevel; /* Loglevel to use */
The comment will be true only with the new proposal.
> char *options; /* Options for the driver */
> #ifdef CONFIG_A11Y_BRAILLE_CONSOLE
> char *brl_options; /* Options for braille driver */
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 6ead14f8c2bc..2e0eb89f046c 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2104,8 +2105,8 @@ __setup("console_msg_format=", console_msg_format_setup);
> static int __init console_setup(char *str)
> {
> char buf[sizeof(console_cmdline[0].name) + 4]; /* 4 for "ttyS" */
> - char *s, *options, *brl_options = NULL;
> - int idx;
> + char *s, *options, *llevel, *brl_options = NULL;
> + int idx, loglevel = LOGLEVEL_EMERG;
>
> if (_braille_console_setup(&str, &brl_options))
> return 1;
> @@ -2123,6 +2124,14 @@ static int __init console_setup(char *str)
> options = strchr(str, ',');
> if (options)
> *(options++) = 0;
> +
> + llevel = strchr(str, '/');
This should be:
if (options)
llevel = strchr(options, '/');
> + if (llevel) {
> + *(llevel++) = 0;
> + if (kstrtoint(llevel, 10, &loglevel))
> + loglevel = LOGLEVEL_EMERG;
> + }
> +
> #ifdef __sparc__
> if (!strcmp(str, "ttya"))
> strcpy(buf, "ttyS0");
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH 1/4] printk: Introduce per-console loglevel setting
From: Petr Mladek @ 2019-03-08 15:09 UTC (permalink / raw)
To: Calvin Owens
Cc: Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Jonathan Corbet, linux-kernel, linux-serial
In-Reply-To: <06cd267ef5439a9391368423b608959f5f1b1a63.1551486732.git.calvinowens@fb.com>
On Fri 2019-03-01 16:48:17, Calvin Owens wrote:
> Not all consoles are created equal: depending on the actual hardware,
> the latency of a printk() call can vary dramatically. The worst examples
> are serial consoles, where it can spin for tens of milliseconds banging
> the UART to emit a message, which can cause application-level problems
> when the kernel spews onto the console.
>
> At Facebook we use netconsole to monitor our fleet, but we still have
> serial consoles attached on each host for live debugging, and the latter
> has caused problems. An obvious solution is to disable the kernel
> console output to ttyS0, but this makes live debugging frustrating,
> since crashes become silent and opaque to the ttyS0 user. Enabling it on
> the fly when needed isn't feasible, since boxes you need to debug via
> serial are likely to be borked in ways that make this impossible.
I guess that many other people have similar problem.
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index d3d170374ceb..6ead14f8c2bc 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1164,9 +1164,14 @@ module_param(ignore_loglevel, bool, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(ignore_loglevel,
> "ignore loglevel setting (prints all kernel messages to the console)");
>
> -static bool suppress_message_printing(int level)
> +static int effective_loglevel(struct console *con)
> {
> - return (level >= console_loglevel && !ignore_loglevel);
> + return max(console_loglevel, con ? con->level : LOGLEVEL_EMERG);
> +}
> +
> +static bool suppress_message_printing(int level, struct console *con)
> +{
> + return (level >= effective_loglevel(con) && !ignore_loglevel);
Hmm, the semantic is cleaner when the per-console level defines
the minimal loglevel. But it is still complicated. Also it is
very confusing that the per-console value is called "level"
or "loglevel" but it is actually minimal loglevel.
It might be even more straightforward when the per-console value
defines the effective console level. I mean the following semantic:
+ "console_loglevel" would define the default loglevel used
by consoles at runtime.
+ the per-console loglevel could override the default
console_loglevel.
+ We would need a custom handler for the sysctl "console_loglevel".
It would write the given value to the global console_loglevel
variable and for all already registered consoles (con->loglevel).
The value will be used also for all newly registered consoles
when they do not have any custom one.
+ The handler for "loglevel" early param should behave the same
as the sysctl handler.
IMHO, there is no perfect solution. The advantage of the above
proposal is that you "see" and "use" exactly what you "set".
> }
>
> #ifdef CONFIG_BOOT_PRINTK_DELAY
> @@ -1198,7 +1203,7 @@ static void boot_delay_msec(int level)
> unsigned long timeout;
>
> if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
> - || suppress_message_printing(level)) {
> + || suppress_message_printing(level, NULL)) {
We should delay the message only when it will really reach the
console. The same check might be used also for formatting
the text as pointed out by Sergey in the other mail.
If the above proposal was accepted, we would have custom
handlers for sysctl. Then we could easily maintain a global
variable with maximal effective console loglevel.
Best Regards,
Petr
^ permalink raw reply
* [PATCH] serial: sh-sci: Missing uart_unregister_driver() on error in sci_probe_single()
From: Mao Wenan @ 2019-03-08 14:23 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, kernel-janitors, linux-kernel
Add the missing uart_unregister_driver() before return
from sci_probe_single() in the error handling case.
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/tty/serial/sh-sci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 64bbeb7d7e0c..dde4eac9d222 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3254,8 +3254,10 @@ static int sci_probe_single(struct platform_device *dev,
mutex_unlock(&sci_uart_registration_lock);
ret = sci_init_single(dev, sciport, index, p, false);
- if (ret)
+ if (ret) {
+ uart_unregister_driver(&sci_uart_driver);
return ret;
+ }
sciport->gpios = mctrl_gpio_init(&sciport->port, 0);
if (IS_ERR(sciport->gpios) && PTR_ERR(sciport->gpios) != -ENOSYS)
--
2.20.1
^ permalink raw reply related
* [PATCH serial] sc16is7xx: missing unregister/delete driver on error in sc16is7xx_init()
From: Mao Wenan @ 2019-03-08 14:09 UTC (permalink / raw)
To: gregkh, jslaby, linux-serial, kernel-janitors, linux-kernel
Add the missing uart_unregister_driver() and i2c_del_driver() before return
from sc16is7xx_init() in the error handling case.
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/tty/serial/sc16is7xx.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 268098681856..114e94f476c6 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1509,7 +1509,7 @@ static int __init sc16is7xx_init(void)
ret = i2c_add_driver(&sc16is7xx_i2c_uart_driver);
if (ret < 0) {
pr_err("failed to init sc16is7xx i2c --> %d\n", ret);
- return ret;
+ goto err_i2c;
}
#endif
@@ -1517,10 +1517,18 @@ static int __init sc16is7xx_init(void)
ret = spi_register_driver(&sc16is7xx_spi_uart_driver);
if (ret < 0) {
pr_err("failed to init sc16is7xx spi --> %d\n", ret);
- return ret;
+ goto err_spi;
}
#endif
return ret;
+
+err_spi:
+#ifdef CONFIG_SERIAL_SC16IS7XX_I2C
+ i2c_del_driver(&sc16is7xx_i2c_uart_driver);
+#endif
+err_i2c:
+ uart_unregister_driver(&sc16is7xx_uart);
+ return ret;
}
module_init(sc16is7xx_init);
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox