* Re: [PATCH 1/2] dt-bindings: serial: stm32: add wakeup option using note
From: Rob Herring @ 2018-03-06 0:52 UTC (permalink / raw)
To: Bich HEMON
Cc: Greg Kroah-Hartman, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1519815044-22669-2-git-send-email-bich.hemon@st.com>
On Wed, Feb 28, 2018 at 10:51:17AM +0000, Bich HEMON wrote:
> Update bindings with interrupt-names and wakeup-source information
>
> Signed-off-by: Bich Hemon <bich.hemon@st.com>
> ---
> Documentation/devicetree/bindings/serial/st,stm32-usart.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
> index d150b04..aaeb564 100644
> --- a/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
> +++ b/Documentation/devicetree/bindings/serial/st,stm32-usart.txt
> @@ -10,6 +10,7 @@ Required properties:
> - interrupts:
> - The interrupt line for the USART instance,
> - An optional wake-up interrupt.
> +- interrupt-names: Contains "event" for the USART interrupt line.
This should be moved down into optional properties.
> - clocks: The input clock of the USART instance
>
> Optional properties:
> @@ -17,6 +18,9 @@ Optional properties:
> - st,hw-flow-ctrl: bool flag to enable hardware flow control.
> - dmas: phandle(s) to DMA controller node(s). Refer to stm32-dma.txt
> - dma-names: "rx" and/or "tx"
> +- wakeup-source: bool flag to indicate this device has wakeup capabilities
> +- interrupt-names : Should contain "wakeup" if optional wake-up interrupt is
> + used.
>
> Examples:
> usart4: serial@40004c00 {
> --
> 1.9.1
^ permalink raw reply
* Re: [PATCH 2/2] serial: stm32: update interrupt initialization
From: Rob Herring @ 2018-03-06 0:50 UTC (permalink / raw)
To: Bich HEMON
Cc: Greg Kroah-Hartman, Mark Rutland, Maxime Coquelin,
Alexandre TORGUE, Jiri Slaby, linux-serial@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1519815044-22669-3-git-send-email-bich.hemon@st.com>
On Wed, Feb 28, 2018 at 10:51:17AM +0000, Bich HEMON wrote:
> For each port, get each IRQ using its specific name instead of its index.
>
> Signed-off-by: Bich Hemon <bich.hemon@st.com>
> ---
> drivers/tty/serial/stm32-usart.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 0fa735b..5c85cbc 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -680,8 +680,8 @@ static int stm32_init_port(struct stm32_port *stm32port,
> port->flags = UPF_BOOT_AUTOCONF;
> port->ops = &stm32_uart_ops;
> port->dev = &pdev->dev;
> - port->irq = platform_get_irq(pdev, 0);
> - stm32port->wakeirq = platform_get_irq(pdev, 1);
> + port->irq = platform_get_irq_byname(pdev, "event");
> + stm32port->wakeirq = platform_get_irq_byname(pdev, "wakeup");
This change is not necessary. The interrupts should be at fixed indices.
> stm32port->fifoen = stm32port->info->cfg.has_fifo;
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> --
> 1.9.1
^ permalink raw reply
* Re: [PATCH 1/2] ARM: dts: STi: Fix aliases property name for STi boards
From: Patrice CHOTARD @ 2018-03-05 16:53 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree@vger.kernel.org, Greg Kroah-Hartman,
linux@armlinux.org.uk, Linux Kernel Mailing List, Rob Herring,
linux-serial@vger.kernel.org, jslaby@suse.com, linux-arm-kernel
In-Reply-To: <CABGGiszEcvKHGE54vOXPBUJeReAKbgmrxRyRxNfQQoL6D54GCw@mail.gmail.com>
Hi Rob
On 03/05/2018 04:28 PM, Rob Herring wrote:
> On Mon, Mar 5, 2018 at 9:00 AM, <patrice.chotard@st.com> wrote:
>> From: Patrice Chotard <patrice.chotard@st.com>
>>
>> Since dtc v1.4.6-9-gaadd0b65c987, aliases property name must
>> be lowercase only.
>> This allows to fix following warnings when compiling dtb
>> with W=1 option :
>
> I'm so glad to see fixes before the dtc update landed in the kernel tree!
;-)
>
>> arch/arm/boot/dts/stih418-b2199.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
>> arch/arm/boot/dts/stih407-b2120.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
>> arch/arm/boot/dts/stih410-b2260.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
>> arch/arm/boot/dts/stih410-b2120.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
>> ---
>> arch/arm/boot/dts/stih407-b2120.dts | 4 ++--
>> arch/arm/boot/dts/stih410-b2120.dts | 4 ++--
>> arch/arm/boot/dts/stih410-b2260.dts | 4 ++--
>> arch/arm/boot/dts/stih418-b2199.dts | 4 ++--
>> 4 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/stih407-b2120.dts b/arch/arm/boot/dts/stih407-b2120.dts
>> index c8ad905d0309..fa1186af6574 100644
>> --- a/arch/arm/boot/dts/stih407-b2120.dts
>> +++ b/arch/arm/boot/dts/stih407-b2120.dts
>> @@ -14,7 +14,7 @@
>> compatible = "st,stih407-b2120", "st,stih407";
>>
>> chosen {
>> - bootargs = "console=ttyAS0,115200 clk_ignore_unused";
>> + bootargs = "console=ttyas0,115200 clk_ignore_unused";
>
> The kernel change for this is an ABI breakage both DT <-> kernel and
> kernel <-> userspace.
>
> Really, if you are okay with doing this, you should change it to ttySx IMO.
OK i will use serialN instead
>
>> linux,stdout-path = &sbc_serial0;
>
> And since you have this, you don't even need the console praram.
Ok, i will remove stdout-path in an additional patch
>
>> };
>>
>> @@ -24,7 +24,7 @@
>> };
>>
>> aliases {
>> - ttyAS0 = &sbc_serial0;
>> + ttyas0 = &sbc_serial0;
>
> The correct fix is use "serialN" for aliases.
Ok
>
> I have a check for only known alias names which would give a better
> warning, but David thought it was too restrictive.
>
>> ethernet0 = ðernet0;
>> };
>>
Thanks
Patrice
^ permalink raw reply
* Re: [RFC PATCH] cdc-acm: do not drop data from fast devices
From: Oliver Neukum @ 2018-03-05 16:04 UTC (permalink / raw)
To: Romain Izard, johan
Cc: Greg Kroah-Hartman, linux-kernel, linux-serial, linux-usb
In-Reply-To: <20180305095539.13698-1-romain.izard.pro@gmail.com>
On Mon, 2018-03-05 at 10:55 +0100, Romain Izard wrote:
> The TTY buffer is 4096 bytes large, throttling when there are only 128
> free bytes left, and unthrottling when there are only 128 bytes available.
> But the TTY buffer is filled from an intermediate flip buffer that
> contains up to 64 KiB of data, and each time unthrottle() is called 16
> URBs are scheduled by the driver, sending up to 16 KiB to the flip buffer.
> As the result of tty_insert_flip_string() is not checked in the URB
> reception callback, data can be lost when the flip buffer is filled faster
> than the TTY is emptied.
It seems to me that the problem is in the concept here. If you cannot
take all data, you should tell the lower layer how much data you can
take.
> Moreover, as the URB callbacks are called from a tasklet context, whereas
> throttling is called from the system workqueue, it is possible for the
> throttling to be delayed by high traffic in the tasklet. As a result,
> completed URBs can be resubmitted even if the flip buffer is full, and we
> request more data from the device only to drop it immediately.
That points to a deficiency with how we throttle. Maybe we should poison
URBs upon throttle() being called?
> To prevent this problem, the URBs whose data did not reach the flip buffer
> are placed in a waiting list, which is only processed when the serial port
> is unthrottled.
So we introduce yet another buffer? That does look like we are papering
over a design problem.
> This is working when using the normal line discipline on ttyACM. But
> there is a big hole in this: other line disciplines do not use throttling
> and thus unthrottle is never called. The URBs will never get resubmitted,
Now that is a real problem. This introduces a regression.
> and the port is dead. Unfortunately, there is no notification when the
> flip buffer is ready to receive new data, so the only alternative would
> be to schedule a timer polling the flip buffer. But with an additional
> asynchronous process in the mix, the code starts to be very brittle.
Well, no. This tells us something is broken in the tty layer.
> I believe this issue is not limited to ttyACM. As the TTY layer is not
> performance-oriented, it can be easy to overwhelm devices with a low
> available processing power. In my case, a modem sending a sustained 2 MB/s
> on a debug port (exported as a CDC-ACM port) was enough to trigger the
> issue. The code handling the CDC-ACM class and the generic USB serial port
> is very similar when it comes to URB handling, so all drivers that rely on
> that code have the same issue.
>
> But in general, it seems to me that there is no code in the kernel that
> checks the return value of tty_insert_flip_string(). This means that we
> are working with the assumption that the kernel will consume the data
> faster than the source can send it, or that the upper layer will be
> able or willing to throttle it fast enough to avoid losing data.
Yes. And if the assumption is false, you need to go for the tty layer.
I am sorry, but NACK.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 1/2] ARM: dts: STi: Fix aliases property name for STi boards
From: Rob Herring @ 2018-03-05 15:28 UTC (permalink / raw)
To: patrice.chotard
Cc: Rob Herring, Mark Rutland, linux, linux-arm-kernel,
Linux Kernel Mailing List, devicetree, Greg Kroah-Hartman, jslaby,
linux-serial
In-Reply-To: <1520262025-21088-2-git-send-email-patrice.chotard@st.com>
On Mon, Mar 5, 2018 at 9:00 AM, <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
>
> Since dtc v1.4.6-9-gaadd0b65c987, aliases property name must
> be lowercase only.
> This allows to fix following warnings when compiling dtb
> with W=1 option :
I'm so glad to see fixes before the dtc update landed in the kernel tree!
> arch/arm/boot/dts/stih418-b2199.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
> arch/arm/boot/dts/stih407-b2120.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
> arch/arm/boot/dts/stih410-b2260.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
> arch/arm/boot/dts/stih410-b2120.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
>
> Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
> ---
> arch/arm/boot/dts/stih407-b2120.dts | 4 ++--
> arch/arm/boot/dts/stih410-b2120.dts | 4 ++--
> arch/arm/boot/dts/stih410-b2260.dts | 4 ++--
> arch/arm/boot/dts/stih418-b2199.dts | 4 ++--
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/boot/dts/stih407-b2120.dts b/arch/arm/boot/dts/stih407-b2120.dts
> index c8ad905d0309..fa1186af6574 100644
> --- a/arch/arm/boot/dts/stih407-b2120.dts
> +++ b/arch/arm/boot/dts/stih407-b2120.dts
> @@ -14,7 +14,7 @@
> compatible = "st,stih407-b2120", "st,stih407";
>
> chosen {
> - bootargs = "console=ttyAS0,115200 clk_ignore_unused";
> + bootargs = "console=ttyas0,115200 clk_ignore_unused";
The kernel change for this is an ABI breakage both DT <-> kernel and
kernel <-> userspace.
Really, if you are okay with doing this, you should change it to ttySx IMO.
> linux,stdout-path = &sbc_serial0;
And since you have this, you don't even need the console praram.
> };
>
> @@ -24,7 +24,7 @@
> };
>
> aliases {
> - ttyAS0 = &sbc_serial0;
> + ttyas0 = &sbc_serial0;
The correct fix is use "serialN" for aliases.
I have a check for only known alias names which would give a better
warning, but David thought it was too restrictive.
> ethernet0 = ðernet0;
> };
>
^ permalink raw reply
* [PATCH 2/2] tty: st-asc: Convert tty alias in lowercase
From: patrice.chotard @ 2018-03-05 15:00 UTC (permalink / raw)
To: robh+dt, mark.rutland, linux, linux-arm-kernel, linux-kernel,
devicetree, gregkh, jslaby, linux-serial
Cc: patrice.chotard
In-Reply-To: <1520262025-21088-1-git-send-email-patrice.chotard@st.com>
From: Patrice Chotard <patrice.chotard@st.com>
Since dtc v1.4.6-9-gaadd0b65c987, aliases property name
must include only lowercase and '-'.
After having converted all STi boards serial aliases in lowercase,
st-asc driver need to be updated accordingly as tty aliases id is
retrieved using of_alias_get_id(np, ASC_SERIAL_NAME);
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---
drivers/tty/serial/st-asc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index c763253514e9..8d2167759258 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -29,7 +29,7 @@
#include <linux/gpio/consumer.h>
#define DRIVER_NAME "st-asc"
-#define ASC_SERIAL_NAME "ttyAS"
+#define ASC_SERIAL_NAME "ttyas"
#define ASC_FIFO_SIZE 16
#define ASC_MAX_PORTS 8
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] ARM: dts: STi: Fix aliases property name for STi boards
From: patrice.chotard @ 2018-03-05 15:00 UTC (permalink / raw)
To: robh+dt, mark.rutland, linux, linux-arm-kernel, linux-kernel,
devicetree, gregkh, jslaby, linux-serial
Cc: patrice.chotard
In-Reply-To: <1520262025-21088-1-git-send-email-patrice.chotard@st.com>
From: Patrice Chotard <patrice.chotard@st.com>
Since dtc v1.4.6-9-gaadd0b65c987, aliases property name must
be lowercase only.
This allows to fix following warnings when compiling dtb
with W=1 option :
arch/arm/boot/dts/stih418-b2199.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
arch/arm/boot/dts/stih407-b2120.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
arch/arm/boot/dts/stih410-b2260.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
arch/arm/boot/dts/stih410-b2120.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
Signed-off-by: Patrice Chotard <patrice.chotard@st.com>
---
arch/arm/boot/dts/stih407-b2120.dts | 4 ++--
arch/arm/boot/dts/stih410-b2120.dts | 4 ++--
arch/arm/boot/dts/stih410-b2260.dts | 4 ++--
arch/arm/boot/dts/stih418-b2199.dts | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/arm/boot/dts/stih407-b2120.dts b/arch/arm/boot/dts/stih407-b2120.dts
index c8ad905d0309..fa1186af6574 100644
--- a/arch/arm/boot/dts/stih407-b2120.dts
+++ b/arch/arm/boot/dts/stih407-b2120.dts
@@ -14,7 +14,7 @@
compatible = "st,stih407-b2120", "st,stih407";
chosen {
- bootargs = "console=ttyAS0,115200 clk_ignore_unused";
+ bootargs = "console=ttyas0,115200 clk_ignore_unused";
linux,stdout-path = &sbc_serial0;
};
@@ -24,7 +24,7 @@
};
aliases {
- ttyAS0 = &sbc_serial0;
+ ttyas0 = &sbc_serial0;
ethernet0 = ðernet0;
};
diff --git a/arch/arm/boot/dts/stih410-b2120.dts b/arch/arm/boot/dts/stih410-b2120.dts
index 9830be577433..00258212b6fb 100644
--- a/arch/arm/boot/dts/stih410-b2120.dts
+++ b/arch/arm/boot/dts/stih410-b2120.dts
@@ -14,7 +14,7 @@
compatible = "st,stih410-b2120", "st,stih410";
chosen {
- bootargs = "console=ttyAS0,115200 clk_ignore_unused";
+ bootargs = "console=ttyas0,115200 clk_ignore_unused";
linux,stdout-path = &sbc_serial0;
};
@@ -24,7 +24,7 @@
};
aliases {
- ttyAS0 = &sbc_serial0;
+ ttyas0 = &sbc_serial0;
ethernet0 = ðernet0;
};
diff --git a/arch/arm/boot/dts/stih410-b2260.dts b/arch/arm/boot/dts/stih410-b2260.dts
index c663b70c43a7..3d122c3a1ad6 100644
--- a/arch/arm/boot/dts/stih410-b2260.dts
+++ b/arch/arm/boot/dts/stih410-b2260.dts
@@ -15,7 +15,7 @@
compatible = "st,stih410-b2260", "st,stih410";
chosen {
- bootargs = "console=ttyAS1,115200 clk_ignore_unused";
+ bootargs = "console=ttyas1,115200 clk_ignore_unused";
linux,stdout-path = &uart1;
};
@@ -25,7 +25,7 @@
};
aliases {
- ttyAS1 = &uart1;
+ ttyas1 = &uart1;
ethernet0 = ðernet0;
};
diff --git a/arch/arm/boot/dts/stih418-b2199.dts b/arch/arm/boot/dts/stih418-b2199.dts
index 4e6d915c85ff..8459a3766b7c 100644
--- a/arch/arm/boot/dts/stih418-b2199.dts
+++ b/arch/arm/boot/dts/stih418-b2199.dts
@@ -14,7 +14,7 @@
compatible = "st,stih418-b2199", "st,stih418";
chosen {
- bootargs = "console=ttyAS0,115200 clk_ignore_unused";
+ bootargs = "console=ttyas0,115200 clk_ignore_unused";
linux,stdout-path = &sbc_serial0;
};
@@ -24,7 +24,7 @@
};
aliases {
- ttyAS0 = &sbc_serial0;
+ ttyas0 = &sbc_serial0;
ethernet0 = ðernet0;
};
--
1.9.1
^ permalink raw reply related
* [PATCH 0/2] Fix STi aliases property name
From: patrice.chotard @ 2018-03-05 15:00 UTC (permalink / raw)
To: robh+dt, mark.rutland, linux, linux-arm-kernel, linux-kernel,
devicetree, gregkh, jslaby, linux-serial
Cc: patrice.chotard
From: Patrice Chotard <patrice.chotard@st.com>
Since dtc v1.4.6-9-gaadd0b65c987, when compiling dtb with W=1 option,
the following warnings are triggered :
arch/arm/boot/dts/stih418-b2199.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
arch/arm/boot/dts/stih407-b2120.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
arch/arm/boot/dts/stih410-b2260.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
arch/arm/boot/dts/stih410-b2120.dtb: Warning (alias_paths): /aliases: aliases property name must include only lowercase and '-'
_ Patch 1, convert the aliases property name in lowercase in
all STi board dts files.
_ Patch 2, rework the tty driver st-asc accordingly, as aliases id is retrieved
using of_alias_get_id() with a defined string with is not lowercase only.
Patrice Chotard (2):
ARM: dts: STi: Fix aliases property name for STi boards
tty: st-asc: Convert tty alias in lowercase
arch/arm/boot/dts/stih407-b2120.dts | 4 ++--
arch/arm/boot/dts/stih410-b2120.dts | 4 ++--
arch/arm/boot/dts/stih410-b2260.dts | 4 ++--
arch/arm/boot/dts/stih418-b2199.dts | 4 ++--
drivers/tty/serial/st-asc.c | 2 +-
5 files changed, 9 insertions(+), 9 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH v2] serial: 8250: Add Nuvoton NPCM UART
From: Joel Stanley @ 2018-03-05 11:47 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Mark Rutland
Cc: linux-serial, devicetree, linux-kernel, Tomer Maimon, Avi Fishman,
Jeremy Kerr
The Nuvoton UART is almost compatible with the 8250 driver when probed
via the 8250_of driver, however it requires some extra configuration
at startup.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2:
Remove redundant whitespace
Use port number 40 to fill in a gap
v3:
Remove some whitespace in serial_reg.h
Move UART_NPCM_TOR and UART_NPCM_TOIE out of userspace header
Add Rob's review
---
Documentation/devicetree/bindings/serial/8250.txt | 1 +
drivers/tty/serial/8250/8250_of.c | 1 +
drivers/tty/serial/8250/8250_port.c | 33 +++++++++++++++++++++++
include/uapi/linux/serial_core.h | 3 +++
4 files changed, 38 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
index dad3b2ec66d4..aeb6db4e35c3 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -24,6 +24,7 @@ Required properties:
- "ti,da830-uart"
- "aspeed,ast2400-vuart"
- "aspeed,ast2500-vuart"
+ - "nuvoton,npcm750-uart"
- "serial" if the port type is unknown.
- reg : offset and length of the register set for the device.
- interrupts : should contain uart interrupt.
diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 160b8906d9b9..9835b1c1cbe1 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -316,6 +316,7 @@ static const struct of_device_id of_platform_serial_table[] = {
{ .compatible = "mrvl,mmp-uart",
.data = (void *)PORT_XSCALE, },
{ .compatible = "ti,da830-uart", .data = (void *)PORT_DA830, },
+ { .compatible = "nuvoton,npcm750-uart", .data = (void *)PORT_NPCM, },
{ /* end of list */ },
};
MODULE_DEVICE_TABLE(of, of_platform_serial_table);
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1328c7e70108..804c1af6fd33 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -47,6 +47,10 @@
#define UART_EXAR_SLEEP 0x8b /* Sleep mode */
#define UART_EXAR_DVID 0x8d /* Device identification */
+/* Nuvoton NPCM timeout register */
+#define UART_NPCM_TOR 7
+#define UART_NPCM_TOIE BIT(7) /* Timeout Interrupt Enable */
+
/*
* Debugging.
*/
@@ -293,6 +297,15 @@ static const struct serial8250_config uart_config[] = {
UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
.flags = UART_CAP_FIFO,
},
+ [PORT_NPCM] = {
+ .name = "Nuvoton 16550",
+ .fifo_size = 16,
+ .tx_loadsz = 16,
+ .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10 |
+ UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT,
+ .rxtrig_bytes = {1, 4, 8, 14},
+ .flags = UART_CAP_FIFO,
+ },
};
/* Uart divisor latch read */
@@ -2140,6 +2153,15 @@ int serial8250_do_startup(struct uart_port *port)
UART_DA830_PWREMU_MGMT_FREE);
}
+ if (port->type == PORT_NPCM) {
+ /*
+ * Nuvoton calls the scratch register 'UART_TOR' (timeout
+ * register). Enable it, and set TIOC (timeout interrupt
+ * comparator) to be 0x20 for correct operation.
+ */
+ serial_port_out(port, UART_NPCM_TOR, UART_NPCM_TOIE | 0x20);
+ }
+
#ifdef CONFIG_SERIAL_8250_RSA
/*
* If this is an RSA port, see if we can kick it up to the
@@ -2462,6 +2484,15 @@ static unsigned int xr17v35x_get_divisor(struct uart_8250_port *up,
return quot_16 >> 4;
}
+/* Nuvoton NPCM UARTs have a custom divisor calculation */
+static unsigned int npcm_get_divisor(struct uart_8250_port *up,
+ unsigned int baud)
+{
+ struct uart_port *port = &up->port;
+
+ return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2;
+}
+
static unsigned int serial8250_get_divisor(struct uart_8250_port *up,
unsigned int baud,
unsigned int *frac)
@@ -2482,6 +2513,8 @@ static unsigned int serial8250_get_divisor(struct uart_8250_port *up,
quot = 0x8002;
else if (up->port.type == PORT_XR17V35X)
quot = xr17v35x_get_divisor(up, baud, frac);
+ else if (up->port.type == PORT_NPCM)
+ quot = npcm_get_divisor(up, baud);
else
quot = uart_get_divisor(port, baud);
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 1c8413f93e3d..dce5f9dae121 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -76,6 +76,9 @@
#define PORT_SUNZILOG 38
#define PORT_SUNSAB 39
+/* Nuvoton UART */
+#define PORT_NPCM 40
+
/* Intel EG20 */
#define PORT_PCH_8LINE 44
#define PORT_PCH_2LINE 45
--
2.15.1
^ permalink raw reply related
* [RFC PATCH] cdc-acm: do not drop data from fast devices
From: Romain Izard @ 2018-03-05 9:55 UTC (permalink / raw)
To: Oliver Neukum, Greg Kroah-Hartman
Cc: linux-usb, linux-serial, linux-kernel, Romain Izard
There are some devices using their USB CDC-ACM interfaces as a debug port
that are able to send data at a very high speed, but with the current
driver implementation it is not possible to receive it when using a
relatively slow embedded system without dropping an important part of the
data.
The existing driver uses the throttling mechanism of the TTY line
discipline to regulate the speed of the data transmitted from the port to
the upper layers. This throttling prevents URBs from being resubmitted,
slowing down the transfer on the USB line. But the existing code does not
work correctly when the internal bufferring gets filled.
The TTY buffer is 4096 bytes large, throttling when there are only 128
free bytes left, and unthrottling when there are only 128 bytes available.
But the TTY buffer is filled from an intermediate flip buffer that
contains up to 64 KiB of data, and each time unthrottle() is called 16
URBs are scheduled by the driver, sending up to 16 KiB to the flip buffer.
As the result of tty_insert_flip_string() is not checked in the URB
reception callback, data can be lost when the flip buffer is filled faster
than the TTY is emptied.
Moreover, as the URB callbacks are called from a tasklet context, whereas
throttling is called from the system workqueue, it is possible for the
throttling to be delayed by high traffic in the tasklet. As a result,
completed URBs can be resubmitted even if the flip buffer is full, and we
request more data from the device only to drop it immediately.
To prevent this problem, the URBs whose data did not reach the flip buffer
are placed in a waiting list, which is only processed when the serial port
is unthrottled.
Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
--
This is working when using the normal line discipline on ttyACM. But
there is a big hole in this: other line disciplines do not use throttling
and thus unthrottle is never called. The URBs will never get resubmitted,
and the port is dead. Unfortunately, there is no notification when the
flip buffer is ready to receive new data, so the only alternative would
be to schedule a timer polling the flip buffer. But with an additional
asynchronous process in the mix, the code starts to be very brittle.
I believe this issue is not limited to ttyACM. As the TTY layer is not
performance-oriented, it can be easy to overwhelm devices with a low
available processing power. In my case, a modem sending a sustained 2 MB/s
on a debug port (exported as a CDC-ACM port) was enough to trigger the
issue. The code handling the CDC-ACM class and the generic USB serial port
is very similar when it comes to URB handling, so all drivers that rely on
that code have the same issue.
But in general, it seems to me that there is no code in the kernel that
checks the return value of tty_insert_flip_string(). This means that we
are working with the assumption that the kernel will consume the data
faster than the source can send it, or that the upper layer will be
able or willing to throttle it fast enough to avoid losing data.
---
drivers/usb/class/cdc-acm.c | 80 ++++++++++++++++++++++++++++++++++++++++-----
drivers/usb/class/cdc-acm.h | 4 +++
2 files changed, 75 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 7b366a6c0b49..833fa0a43ddd 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -150,12 +150,18 @@ static inline int acm_set_control(struct acm *acm, int control)
static void acm_kill_urbs(struct acm *acm)
{
int i;
+ struct acm_rb *rb, *t;
usb_kill_urb(acm->ctrlurb);
for (i = 0; i < ACM_NW; i++)
usb_kill_urb(acm->wb[i].urb);
for (i = 0; i < acm->rx_buflimit; i++)
usb_kill_urb(acm->read_urbs[i]);
+ list_for_each_entry_safe(rb, t, &acm->wait_list, node) {
+ set_bit(rb->index, &acm->read_urbs_free);
+ rb->offset = 0;
+ list_del_init(&rb->node);
+ }
}
/*
@@ -454,14 +460,27 @@ static int acm_submit_read_urbs(struct acm *acm, gfp_t mem_flags)
return 0;
}
-static void acm_process_read_urb(struct acm *acm, struct urb *urb)
+static int acm_process_read_urb(struct acm *acm, struct urb *urb)
{
+ int flipped, remainder;
+ struct acm_rb *rb = urb->context;
if (!urb->actual_length)
- return;
+ return 0;
+ flipped = tty_insert_flip_string(&acm->port,
+ urb->transfer_buffer + rb->offset,
+ urb->actual_length - rb->offset);
+ rb->offset += flipped;
+ remainder = urb->actual_length - rb->offset;
+ if (remainder != 0)
+ dev_dbg(&acm->data->dev,
+ "remaining data: usb %d len %d offset %d flipped %d\n",
+ rb->index, urb->actual_length, rb->offset, flipped);
+ else
+ rb->offset = 0;
- tty_insert_flip_string(&acm->port, urb->transfer_buffer,
- urb->actual_length);
tty_flip_buffer_push(&acm->port);
+
+ return remainder;
}
static void acm_read_bulk_callback(struct urb *urb)
@@ -474,17 +493,29 @@ static void acm_read_bulk_callback(struct urb *urb)
dev_vdbg(&acm->data->dev, "got urb %d, len %d, status %d\n",
rb->index, urb->actual_length, status);
- set_bit(rb->index, &acm->read_urbs_free);
-
if (!acm->dev) {
dev_dbg(&acm->data->dev, "%s - disconnected\n", __func__);
+ set_bit(rb->index, &acm->read_urbs_free);
return;
}
+ if (status == 0) {
+ int rem = urb->actual_length;
+
+ usb_mark_last_busy(acm->dev);
+ if (list_empty_careful(&acm->wait_list))
+ rem = acm_process_read_urb(acm, urb);
+ if (rem) {
+ dev_vdbg(&acm->data->dev,
+ "queuing urb %d\n", rb->index);
+ list_add_tail(&rb->node, &acm->wait_list);
+ return;
+ }
+ }
+ set_bit(rb->index, &acm->read_urbs_free);
+
switch (status) {
case 0:
- usb_mark_last_busy(acm->dev);
- acm_process_read_urb(acm, urb);
break;
case -EPIPE:
set_bit(EVENT_RX_STALL, &acm->flags);
@@ -518,6 +549,8 @@ static void acm_read_bulk_callback(struct urb *urb)
acm_submit_read_urb(acm, rb->index, GFP_ATOMIC);
} else {
spin_unlock_irqrestore(&acm->read_lock, flags);
+ dev_vdbg(&acm->data->dev,
+ "urb %d is suspended by throttling\n", rb->index);
}
}
@@ -549,8 +582,14 @@ static void acm_softint(struct work_struct *work)
if (test_bit(EVENT_RX_STALL, &acm->flags)) {
if (!(usb_autopm_get_interface(acm->data))) {
+ struct acm_rb *rb, *t;
for (i = 0; i < acm->rx_buflimit; i++)
usb_kill_urb(acm->read_urbs[i]);
+ list_for_each_entry_safe(rb, t, &acm->wait_list, node) {
+ set_bit(rb->index, &acm->read_urbs_free);
+ rb->offset = 0;
+ list_del_init(&rb->node);
+ }
usb_clear_halt(acm->dev, acm->in);
acm_submit_read_urbs(acm, GFP_KERNEL);
usb_autopm_put_interface(acm->data);
@@ -901,14 +940,32 @@ static void acm_tty_unthrottle(struct tty_struct *tty)
{
struct acm *acm = tty->driver_data;
unsigned int was_throttled;
+ struct acm_rb *rb, *t;
+ bool processed = false;
+ int rem = 0;
spin_lock_irq(&acm->read_lock);
+
+ list_for_each_entry_safe(rb, t, &acm->wait_list, node) {
+ dev_vdbg(&acm->data->dev, "processing urb %d: %d/%d\n",
+ rb->index, rb->offset, rb->urb->actual_length);
+ rem = acm_process_read_urb(acm, rb->urb);
+ if (rem) {
+ spin_unlock_irq(&acm->read_lock);
+ return;
+ }
+ processed = true;
+ set_bit(rb->index, &acm->read_urbs_free);
+ rb->offset = 0;
+ list_del_init(&rb->node);
+ }
+
was_throttled = acm->throttled;
acm->throttled = 0;
acm->throttle_req = 0;
spin_unlock_irq(&acm->read_lock);
- if (was_throttled)
+ if (was_throttled || processed)
acm_submit_read_urbs(acm, GFP_KERNEL);
}
@@ -1430,6 +1487,8 @@ static int acm_probe(struct usb_interface *intf,
if (!acm->ctrlurb)
goto alloc_fail5;
+ INIT_LIST_HEAD(&acm->wait_list);
+
for (i = 0; i < num_rx_buf; i++) {
struct acm_rb *rb = &(acm->read_buffers[i]);
struct urb *urb;
@@ -1445,6 +1504,9 @@ static int acm_probe(struct usb_interface *intf,
if (!urb)
goto alloc_fail6;
+ rb->offset = 0;
+ rb->urb = urb;
+ INIT_LIST_HEAD(&rb->node);
urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
urb->transfer_dma = rb->dma;
if (usb_endpoint_xfer_int(epread))
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index eacc116e83da..cee849a79371 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -77,7 +77,10 @@ struct acm_rb {
unsigned char *base;
dma_addr_t dma;
int index;
+ struct urb *urb;
struct acm *instance;
+ unsigned int offset;
+ struct list_head node;
};
struct acm {
@@ -96,6 +99,7 @@ struct acm {
unsigned long read_urbs_free;
struct urb *read_urbs[ACM_NR];
struct acm_rb read_buffers[ACM_NR];
+ struct list_head wait_list;
struct acm_wb *putbuffer; /* for acm_tty_put_char() */
int rx_buflimit;
spinlock_t read_lock;
--
2.14.1
^ permalink raw reply related
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Aaron Durbin @ 2018-03-03 19:27 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Daniel Kurtz, Aaron Durbin, Brian Norris, Jonathan Corbet,
Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
Christoffer Dall, Paul McKenney, Marc Zyngier,
Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
Levin, Alexander (Sasha Levin), Linux Documentation List,
Linux Kernel Mailing List, SE
In-Reply-To: <CAHp75Veu=0w2sWpgn0mNFa0G3Gwp2SpHv55vPgLiWjrCAzaiGw@mail.gmail.com>
On Sat, Mar 3, 2018 at 8:56 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Mar 2, 2018 at 8:35 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
>> andy.shevchenko@gmail.com>
>>> > wrote:
>
>> the UART bitclock
>> is
>>> > always "BASE_BAUD * 16" (1843200). While this may be true for many
>> UARTs,
>>> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
>> MHz
>>> > clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk"
>> and
>>> > uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
>>> > actually a set up in acpi_apd.c when there is an acpi match for
>> "AMD0020",
>>> > with a rate read from the .fixed_clk_rate param of the corresponding
>>> > apd_device_desc.
>
>> As
>>> > noted above, the information is actually already in the kernel and used
>> by
>>> > 8250_dw - I would happy be to hear recommendations for wiring this data
>>> > into earlycon that doesn't require adding another command line arg.
>
> Brief look at the code shows that ->setup() call back is executed
> after setting initial (which is hardcoded) clock.
>
> What you need is to either create another type of earlycon for your
> device with accompanied ->setup() callback, or patch 8250_early.c.
If I'm understand you correctly, you are suggesting new driver code
that sets the clock accordingly within the port structure such that
the baud rate calculation actually works based on the correct input
clock? Why wouldn't we just extened the generic earlycon driver like
the original patch to support providing the clock? Anything in the
earlycon path that tries to set a baud rate will fail once the
hard-coded input clock assumption doesn't hold true. Why not provide
the ability to correct the assumption for platforms that need it?
>
>>> > I see that support was also added recently to earlycon to let it use
>> ACPI
>>> > SPCR to choose a console and configure its parameters... but AFAICT,
>> this
>>> > path also doesn't allow specifying the uart clock.
>
> It does specify baudrate. It means it's _firmware_ responsibility to
> configure UART device properly.
baudrate != input clock. The issue is that once the driver code
attempts to set a baud rate w/o having the correct input clock then
things break.
>
>>> Fix your firmware then. It should set console to 115200 like (almost)
>>> everyone does.
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> The console is 115200 when it is enabled. However, the firmware does not
>> always enable it by default.
>
> Another firmware bug.
It's more complex than that. You seem to take the stance that the
firmware should bring every IP block out of reset and/or clock and
power gating. That then subsequently requires the kernel to enable all
those soc drivers that put those devices in power or clock gated state
to reach the maximum power savings. While that's certainly possible,
it just leads to bloated kernels. That is orthogonal to the problem of
setting baud rate w/o knowing the proper input clock frequency,
though.
If we do make the assumption the firmware sets up the uart, but the
kernel earlycon has a different baud rate specified than what firmware
set up then the baud rate calculation would be wrong as well since the
input clock isn't known. As such one cannot use earlycon when: 1.
firmware doesn't set up uart 2. firmware baud rate != earlycon
specified baud rate. Providing the proper input clock for the device
in question solves both 1 and 2.
>
>> The problem is that the UART IP block has a fixed 48 MHz input clock, but
>> earlycon assumes this clock is always 1843200.
>
>> I looked a bit further, and I think this patch (or something similar) is
>> still required to teach generic earlycon how to handle an explicit
>> port->uartclk (ie, one that is not 1843200).
>> The extended string can then be explicitly set on the kernel command line
>> for this kind of hardware.
>
> No.
>
>> In addition, we can add another patch with a new quirk detector in
>> drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
>> acpi_parse_spcr() can then use the extended option string to pass in the
>> appropriate UART clock to setup_eralycon().
>
> Definitely no. It's not defined in SPCR spec.
>
>> This would again allow a user to just use the simple command line parameter
>> "earlycon" if the device's firmware has a correctly confiured ACPI SPCR
>> table.
>
> NAK to the patch, see above alternatives.
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Aaron Durbin @ 2018-03-03 19:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Aaron Durbin, Daniel Kurtz, Brian Norris, Jonathan Corbet,
Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
Christoffer Dall, Paul E. McKenney, Marc Zyngier,
Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
Levin, Alexander (Sasha Levin), Linux Documentation List,
Linux Kernel Mailing List, open
In-Reply-To: <CAHp75Vcnuc48xb167hu5nz3iv9uSp2jRw+M-w=yBihex6RTXYQ@mail.gmail.com>
On Sat, Mar 3, 2018 at 8:38 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>>>> wrote:
>>>
>>>> "earlycon simply does not utilize the information".
>>>>
>>>> earlycon parses iotype, mapbase and baud (from options). However, it is
>>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>>> always "BASE_BAUD * 16" (1843200). While this may be true for many UARTs,
>>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>>> clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>>> uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
>>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>>> apd_device_desc.
>>>>
>>>> This patch attempts to add a way to inform earlycon about this clock. As
>>>> noted above, the information is actually already in the kernel and used by
>>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>>> into earlycon that doesn't require adding another command line arg.
>>>
>>> And it should not require that for sure!
>>
>> But it does require that. There's an input clock to the uart ip block.
>> That is a design constraint by the hardware and is required to make
>> baud calculation work.
>
> I mean it should not be user's headache to provide this information to
> the system.
>
>> It's not a firmware problem.
>
> If it's ACPI, then it's definitely firmware issue, since SPCR provides
> a baudrate.
>
earlycon is another implemetnation of driver binding/settings that is
done before the rest of the kernel driver stack. SPCR provides
baudrate, but that's only one piece to the puzzle. You need to know
the UART input clock which you admit is hard coded in the current
driver. It's not possible to configure the divisor in the UART without
knowing the external clock. That's a real dependency that can't be
worked around. The moment the code attempts to configure the baud it
has to know the input clock -- otherwise the calculation is inherently
wrong. Or are you suggesting to lie about the baud such that the the
math works out even though the values are not real?
>> Its the driver's problem in that it
>> assumes an input clock to the uart block that does not reflect
>> reality.
>
> So, driver can't get this info from device tree or what?
There is no device tree on x86. And ACPI driver binding is not fully
initialized when earlycon is being processed. Yes, this can be solved
by getting up the entire ACPI stack, but then that's not really
'early' in the kernel's initialization sequence.
>
>>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>>
>> That's only possible if there is a clock divider on the front end of
>> the uart block. For this hardware that's not the case. I actually did
>> this very thing on intel chromebook devices, but it was only possible
>> because there was a hardware divider that could be tuned to reach the
>> assumed clock that the code currently assumes.
>
> OK.
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Andy Shevchenko @ 2018-03-03 15:56 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Aaron Durbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
Paul McKenney, Marc Zyngier, Frederic Weisbecker, David Woodhouse,
Tom Saeger, Mimi Zohar, Levin, Alexander (Sasha Levin),
Linux Documentation List, Linux Kernel Mailing List,
open list:SERIAL DRIVERS
In-Reply-To: <CAGS+omB76qbiuYjs7Rakff2JB7263_m5TRYDgvKM+wdBpaedNA@mail.gmail.com>
On Fri, Mar 2, 2018 at 8:35 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:
>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
> andy.shevchenko@gmail.com>
>> > wrote:
> the UART bitclock
> is
>> > always "BASE_BAUD * 16" (1843200). While this may be true for many
> UARTs,
>> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
> MHz
>> > clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk"
> and
>> > uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
>> > actually a set up in acpi_apd.c when there is an acpi match for
> "AMD0020",
>> > with a rate read from the .fixed_clk_rate param of the corresponding
>> > apd_device_desc.
> As
>> > noted above, the information is actually already in the kernel and used
> by
>> > 8250_dw - I would happy be to hear recommendations for wiring this data
>> > into earlycon that doesn't require adding another command line arg.
Brief look at the code shows that ->setup() call back is executed
after setting initial (which is hardcoded) clock.
What you need is to either create another type of earlycon for your
device with accompanied ->setup() callback, or patch 8250_early.c.
>> > I see that support was also added recently to earlycon to let it use
> ACPI
>> > SPCR to choose a console and configure its parameters... but AFAICT,
> this
>> > path also doesn't allow specifying the uart clock.
It does specify baudrate. It means it's _firmware_ responsibility to
configure UART device properly.
>> Fix your firmware then. It should set console to 115200 like (almost)
>> everyone does.
>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>
> The console is 115200 when it is enabled. However, the firmware does not
> always enable it by default.
Another firmware bug.
> The problem is that the UART IP block has a fixed 48 MHz input clock, but
> earlycon assumes this clock is always 1843200.
> I looked a bit further, and I think this patch (or something similar) is
> still required to teach generic earlycon how to handle an explicit
> port->uartclk (ie, one that is not 1843200).
> The extended string can then be explicitly set on the kernel command line
> for this kind of hardware.
No.
> In addition, we can add another patch with a new quirk detector in
> drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
> acpi_parse_spcr() can then use the extended option string to pass in the
> appropriate UART clock to setup_eralycon().
Definitely no. It's not defined in SPCR spec.
> This would again allow a user to just use the simple command line parameter
> "earlycon" if the device's firmware has a correctly confiured ACPI SPCR
> table.
NAK to the patch, see above alternatives.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Andy Shevchenko @ 2018-03-03 15:38 UTC (permalink / raw)
To: Aaron Durbin
Cc: Daniel Kurtz, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
David Woodhouse, Tom Saeger, Mimi Zohar,
Levin, Alexander (Sasha Levin), Linux Documentation List,
Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <CAE2855vbQyMwu8pmH-Sq1kA+=h5TJPWddTWPQoVdEqOn6sAyBw@mail.gmail.com>
On Thu, Mar 1, 2018 at 11:24 PM, Aaron Durbin <adurbin@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>>> wrote:
>>
>>> "earlycon simply does not utilize the information".
>>>
>>> earlycon parses iotype, mapbase and baud (from options). However, it is
>>> hard-coded to assume that the clock used to generate the UART bitclock is
>>> always "BASE_BAUD * 16" (1843200). While this may be true for many UARTs,
>>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>>> clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>>> uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
>>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>>> with a rate read from the .fixed_clk_rate param of the corresponding
>>> apd_device_desc.
>>>
>>> This patch attempts to add a way to inform earlycon about this clock. As
>>> noted above, the information is actually already in the kernel and used by
>>> 8250_dw - I would happy be to hear recommendations for wiring this data
>>> into earlycon that doesn't require adding another command line arg.
>>
>> And it should not require that for sure!
>
> But it does require that. There's an input clock to the uart ip block.
> That is a design constraint by the hardware and is required to make
> baud calculation work.
I mean it should not be user's headache to provide this information to
the system.
> It's not a firmware problem.
If it's ACPI, then it's definitely firmware issue, since SPCR provides
a baudrate.
> Its the driver's problem in that it
> assumes an input clock to the uart block that does not reflect
> reality.
So, driver can't get this info from device tree or what?
>> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
>
> That's only possible if there is a clock divider on the front end of
> the uart block. For this hardware that's not the case. I actually did
> this very thing on intel chromebook devices, but it was only possible
> because there was a hardware divider that could be tuned to reach the
> assumed clock that the code currently assumes.
OK.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] serial: mxs-auart: disable clks of Alphascale ASM9260
From: Alexey Khoroshilov @ 2018-03-02 22:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Alexey Khoroshilov, Wolfram Sang, linux-serial, linux-kernel,
ldv-project
In case of Alphascale ASM9260 probe() enables s->clk and s->clk_ahb
via mxs_get_clks(), but there is no disable of the clocks.
The patch adds it to error paths and to mxs_auart_remove().
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Fixes: 254da0d753fb ("serial: mxs-auart: add Alphascale ASM9260 support")
---
drivers/tty/serial/mxs-auart.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 079dc47aa142..531c374a5e3e 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -1674,8 +1674,10 @@ static int mxs_auart_probe(struct platform_device *pdev)
return ret;
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!r)
- return -ENXIO;
+ if (!r) {
+ ret = -ENXIO;
+ goto out_disable_clks;
+ }
s->port.mapbase = r->start;
s->port.membase = ioremap(r->start, resource_size(r));
@@ -1690,21 +1692,23 @@ static int mxs_auart_probe(struct platform_device *pdev)
s->mctrl_prev = 0;
irq = platform_get_irq(pdev, 0);
- if (irq < 0)
- return irq;
+ if (irq < 0) {
+ ret = irq;
+ goto out_disable_clks;
+ }
s->port.irq = irq;
ret = devm_request_irq(&pdev->dev, irq, mxs_auart_irq_handle, 0,
dev_name(&pdev->dev), s);
if (ret)
- return ret;
+ goto out_disable_clks;
platform_set_drvdata(pdev, s);
ret = mxs_auart_init_gpios(s, &pdev->dev);
if (ret) {
dev_err(&pdev->dev, "Failed to initialize GPIOs.\n");
- return ret;
+ goto out_disable_clks;
}
/*
@@ -1712,7 +1716,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
*/
ret = mxs_auart_request_gpio_irq(s);
if (ret)
- return ret;
+ goto out_disable_clks;
auart_port[s->port.line] = s;
@@ -1720,7 +1724,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
ret = uart_add_one_port(&auart_driver, &s->port);
if (ret)
- goto out_disable_clks_free_qpio_irq;
+ goto out_free_qpio_irq;
/* ASM9260 don't have version reg */
if (is_asm9260_auart(s)) {
@@ -1734,13 +1738,15 @@ static int mxs_auart_probe(struct platform_device *pdev)
return 0;
-out_disable_clks_free_qpio_irq:
- if (s->clk)
- clk_disable_unprepare(s->clk_ahb);
- if (s->clk_ahb)
- clk_disable_unprepare(s->clk_ahb);
+out_free_qpio_irq:
mxs_auart_free_gpio_irq(s);
auart_port[pdev->id] = NULL;
+
+out_disable_clks:
+ if (is_asm9260_auart(s)) {
+ clk_disable_unprepare(s->clk);
+ clk_disable_unprepare(s->clk_ahb);
+ }
return ret;
}
@@ -1751,6 +1757,10 @@ static int mxs_auart_remove(struct platform_device *pdev)
uart_remove_one_port(&auart_driver, &s->port);
auart_port[pdev->id] = NULL;
mxs_auart_free_gpio_irq(s);
+ if (is_asm9260_auart(s)) {
+ clk_disable_unprepare(s->clk);
+ clk_disable_unprepare(s->clk_ahb);
+ }
return 0;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Daniel Kurtz @ 2018-03-02 18:35 UTC (permalink / raw)
To: Andy Shevchenko
Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
jslaby, mingo, Thomas Gleixner, Christoffer Dall, Paul McKenney,
marc.zyngier, frederic, dwmw, tom.saeger, zohar, alexander.levin,
linux-doc, linux-kernel, linux-serial
In-Reply-To: <CAHp75VfL82ujwBa1h0B8nEgBtFcBJPmS9ir7H5YWByDgxFGskA@mail.gmail.com>
On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
wrote:
> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
andy.shevchenko@gmail.com>
> > wrote:
> > "earlycon simply does not utilize the information".
> >
> > earlycon parses iotype, mapbase and baud (from options). However, it is
> > hard-coded to assume that the clock used to generate the UART bitclock
is
> > always "BASE_BAUD * 16" (1843200). While this may be true for many
UARTs,
> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
MHz
> > clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk"
and
> > uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
> > actually a set up in acpi_apd.c when there is an acpi match for
"AMD0020",
> > with a rate read from the .fixed_clk_rate param of the corresponding
> > apd_device_desc.
> >
> > This patch attempts to add a way to inform earlycon about this clock.
As
> > noted above, the information is actually already in the kernel and used
by
> > 8250_dw - I would happy be to hear recommendations for wiring this data
> > into earlycon that doesn't require adding another command line arg.
> And it should not require that for sure!
> I would look to this later. It's late here. I need to do a bit of
> research for the answer.
> > I see that support was also added recently to earlycon to let it use
ACPI
> > SPCR to choose a console and configure its parameters... but AFAICT,
this
> > path also doesn't allow specifying the uart clock.
> Fix your firmware then. It should set console to 115200 like (almost)
> everyone does.
> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
The console is 115200 when it is enabled. However, the firmware does not
always enable it by default.
The problem is that the UART IP block has a fixed 48 MHz input clock, but
earlycon assumes this clock is always 1843200.
I looked a bit further, and I think this patch (or something similar) is
still required to teach generic earlycon how to handle an explicit
port->uartclk (ie, one that is not 1843200).
The extended string can then be explicitly set on the kernel command line
for this kind of hardware.
In addition, we can add another patch with a new quirk detector in
drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
acpi_parse_spcr() can then use the extended option string to pass in the
appropriate UART clock to setup_eralycon().
This would again allow a user to just use the simple command line parameter
"earlycon" if the device's firmware has a correctly confiured ACPI SPCR
table.
Thanks,
-Dan
^ permalink raw reply
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Aaron Durbin @ 2018-03-01 21:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Daniel Kurtz, Aaron Durbin, Brian Norris, Jonathan Corbet,
Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
Christoffer Dall, Paul E. McKenney, Marc Zyngier,
Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
Levin, Alexander (Sasha Levin), Linux Documentation List,
Linux Kernel Mailing List, open
In-Reply-To: <CAHp75VfL82ujwBa1h0B8nEgBtFcBJPmS9ir7H5YWByDgxFGskA@mail.gmail.com>
On Thu, Mar 1, 2018 at 1:02 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
>> wrote:
>
>> "earlycon simply does not utilize the information".
>>
>> earlycon parses iotype, mapbase and baud (from options). However, it is
>> hard-coded to assume that the clock used to generate the UART bitclock is
>> always "BASE_BAUD * 16" (1843200). While this may be true for many UARTs,
>> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
>> clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
>> uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
>> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
>> with a rate read from the .fixed_clk_rate param of the corresponding
>> apd_device_desc.
>>
>> This patch attempts to add a way to inform earlycon about this clock. As
>> noted above, the information is actually already in the kernel and used by
>> 8250_dw - I would happy be to hear recommendations for wiring this data
>> into earlycon that doesn't require adding another command line arg.
>
> And it should not require that for sure!
But it does require that. There's an input clock to the uart ip block.
That is a design constraint by the hardware and is required to make
baud calculation work.
>
> I would look to this later. It's late here. I need to do a bit of
> research for the answer.
>
>> I see that support was also added recently to earlycon to let it use ACPI
>> SPCR to choose a console and configure its parameters... but AFAICT, this
>> path also doesn't allow specifying the uart clock.
>
> Fix your firmware then. It should set console to 115200 like (almost)
> everyone does.
It's not a firmware problem. Its the driver's problem in that it
assumes an input clock to the uart block that does not reflect
reality.
> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
That's only possible if there is a clock divider on the front end of
the uart block. For this hardware that's not the case. I actually did
this very thing on intel chromebook devices, but it was only possible
because there was a hardware divider that could be tuned to reach the
assumed clock that the code currently assumes.
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Andy Shevchenko @ 2018-03-01 20:02 UTC (permalink / raw)
To: Daniel Kurtz
Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
David Woodhouse, Tom Saeger, Mimi Zohar,
Levin, Alexander (Sasha Levin), Linux Documentation List,
Linux Kernel Mailing List, open list:SERIAL DRIVERS
In-Reply-To: <CAGS+omCfVUPb0yq3XOEqO_zQ0_uppkKaGhqtB0OqXmQDK9xH1Q@mail.gmail.com>
On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
> wrote:
> "earlycon simply does not utilize the information".
>
> earlycon parses iotype, mapbase and baud (from options). However, it is
> hard-coded to assume that the clock used to generate the UART bitclock is
> always "BASE_BAUD * 16" (1843200). While this may be true for many UARTs,
> it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
> clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
> uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
> actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
> with a rate read from the .fixed_clk_rate param of the corresponding
> apd_device_desc.
>
> This patch attempts to add a way to inform earlycon about this clock. As
> noted above, the information is actually already in the kernel and used by
> 8250_dw - I would happy be to hear recommendations for wiring this data
> into earlycon that doesn't require adding another command line arg.
And it should not require that for sure!
I would look to this later. It's late here. I need to do a bit of
research for the answer.
> I see that support was also added recently to earlycon to let it use ACPI
> SPCR to choose a console and configure its parameters... but AFAICT, this
> path also doesn't allow specifying the uart clock.
Fix your firmware then. It should set console to 115200 like (almost)
everyone does.
Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Daniel Kurtz @ 2018-03-01 19:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: adurbin, Brian Norris, corbet, Greg Kroah-Hartman, jslaby, mingo,
Thomas Gleixner, cdall, paulmck, marc.zyngier, frederic, dwmw,
tom.saeger, zohar, alexander.levin, linux-doc, linux-kernel,
linux-serial
In-Reply-To: <CAHp75VfHuNHnjMqsvLZPSNeP57YX1CDZgf=EH-JNSh5bAVdUBA@mail.gmail.com>
On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <andy.shevchenko@gmail.com>
wrote:
> On Thu, Mar 1, 2018 at 8:43 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Please, hold on with new versions.
> I'm not satisfied (yet?) by the approach.
Copying over your comment on v1:
> It needs to be discussed.
Sure.
> First of all, if you are going to do this you need to add a parse of
> human readable formats (IIRC kernel has helpers), i.e. "48M", "38.4M"
> and so on.
> Next, I was under impression that purpose of earlycon (in difference
> to earlyprintk) is to re-use existing drivers as fully as possible.
> So, what exactly happens in your case? Are your driver lacks of
> properly set clock? Or earlycon does simple not utilizing this
> information?
"earlycon simply does not utilize the information".
earlycon parses iotype, mapbase and baud (from options). However, it is
hard-coded to assume that the clock used to generate the UART bitclock is
always "BASE_BAUD * 16" (1843200). While this may be true for many UARTs,
it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48 MHz
clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk" and
uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
actually a set up in acpi_apd.c when there is an acpi match for "AMD0020",
with a rate read from the .fixed_clk_rate param of the corresponding
apd_device_desc.
This patch attempts to add a way to inform earlycon about this clock. As
noted above, the information is actually already in the kernel and used by
8250_dw - I would happy be to hear recommendations for wiring this data
into earlycon that doesn't require adding another command line arg.
I see that support was also added recently to earlycon to let it use ACPI
SPCR to choose a console and configure its parameters... but AFAICT, this
path also doesn't allow specifying the uart clock.
-Dan
^ permalink raw reply
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Andy Shevchenko @ 2018-03-01 18:47 UTC (permalink / raw)
To: Daniel Kurtz
Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
Jiri Slaby, Ingo Molnar, Thomas Gleixner, Christoffer Dall,
Paul E. McKenney, Marc Zyngier, Frederic Weisbecker,
David Woodhouse, Tom Saeger, Mimi Zohar,
Levin, Alexander (Sasha Levin), open list:DOCUMENTATION,
open list, open list:SERIAL DRIVERS
In-Reply-To: <20180301184335.248378-1-djkurtz@chromium.org>
On Thu, Mar 1, 2018 at 8:43 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
Please, hold on with new versions.
I'm not satisfied (yet?) by the approach.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] earlycon: Allow specifying a uartclk in options
From: Andy Shevchenko @ 2018-03-01 18:46 UTC (permalink / raw)
To: Daniel Kurtz
Cc: adurbin, Brian Norris, Greg Kroah-Hartman, Jiri Slaby,
open list:SERIAL DRIVERS, open list
In-Reply-To: <20180301182028.237856-1-djkurtz@chromium.org>
On Thu, Mar 1, 2018 at 8:20 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Currently when an earlycon is registered, the uartclk is assumed to be
> BASE_BAUD * 16 = 1843200. If a baud rate is specified in the earlycon
> options, then 8250_early's init_port will program the UART clock divider
> registers based on this assumed uartclk.
>
> However, not all uarts have a UART clock of 1843200. For example, the
> 8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
> cz_uart_desc in acpi_apd.c). Thus, specifying a baud when using earlycon
> on such a device will result in incorrect divider values and a wrong UART
> clock.
>
> Fix this by extending the earlycon options parameter to allow specification
> of a uartclk, like so:
>
> earlycon=uart,mmio32,0xfedc6000,115200,48000000
>
> If none is specified, fall-back to prior behavior - 1843200.
It needs to be discussed.
First of all, if you are going to do this you need to add a parse of
human readable formats (IIRC kernel has helpers), i.e. "48M", "38.4M"
and so on.
Next, I was under impression that purpose of earlycon (in difference
to earlyprintk) is to re-use existing drivers as fully as possible.
So, what exactly happens in your case? Are your driver lacks of
properly set clock? Or earlycon does simple not utilizing this
information?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Daniel Kurtz @ 2018-03-01 18:43 UTC (permalink / raw)
Cc: adurbin, briannorris, Daniel Kurtz, Jonathan Corbet,
Greg Kroah-Hartman, Jiri Slaby, Ingo Molnar, Thomas Gleixner,
Christoffer Dall, Paul E. McKenney, Marc Zyngier,
Frederic Weisbecker, David Woodhouse, Tom Saeger, Mimi Zohar,
Levin, Alexander (Sasha Levin), open list:DOCUMENTATION,
open list, open list:SERIAL DRIVERS
Currently when an earlycon is registered, the uartclk is assumed to be
BASE_BAUD * 16 = 1843200. If a baud rate is specified in the earlycon
options, then 8250_early's init_port will program the UART clock divider
registers based on this assumed uartclk.
However, not all uarts have a UART clock of 1843200. For example, the
8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
cz_uart_desc in acpi_apd.c). Thus, specifying a baud when using earlycon
on such a device will result in incorrect divider values and a wrong UART
clock.
Fix this by extending the earlycon options parameter to allow specification
of a uartclk, like so:
earlycon=uart,mmio32,0xfedc6000,115200,48000000
If none is specified, fall-back to prior behavior - 1843200.
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
Documentation/admin-guide/kernel-parameters.txt | 3 +++
drivers/tty/serial/earlycon.c | 8 ++++++--
include/linux/serial_core.h | 2 +-
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..20e72cada38e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -958,6 +958,9 @@
to be equivalent to 'mmio'. 'options' are specified
in the same format described for "console=ttyS<n>"; if
unspecified, the h/w is not initialized.
+ A UART clock rate can also be appended after the baud,
+ as in <options> = 115200n8,48000000; if unspecified,
+ the default 1843200 will be used.
pl011,<addr>
pl011,mmio32,<addr>
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 870e84fb6e39..e846f406a1c6 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -115,12 +115,17 @@ static int __init parse_options(struct earlycon_device *device, char *options)
}
if (options) {
- device->baud = simple_strtoul(options, NULL, 0);
+ char *uartclk;
+ device->baud = simple_strtoul(options, &uartclk, 0);
+ if (*uartclk++ == ',')
+ port->uartclk = simple_strtoul(uartclk, NULL, 0);
length = min(strcspn(options, " ") + 1,
(size_t)(sizeof(device->options)));
strlcpy(device->options, options, length);
}
+ port->uartclk = (port->uartclk) ?: BASE_BAUD * 16;
+
return 0;
}
@@ -134,7 +139,6 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
buf = NULL;
spin_lock_init(&port->lock);
- port->uartclk = BASE_BAUD * 16;
if (port->mapbase)
port->membase = earlycon_map(port->mapbase, 64);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..b772f0d20b18 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -343,7 +343,7 @@ static inline int uart_poll_timeout(struct uart_port *port)
struct earlycon_device {
struct console *con;
struct uart_port port;
- char options[16]; /* e.g., 115200n8 */
+ char options[32]; /* e.g., 115200n8,48000000 */
unsigned int baud;
};
--
2.16.2.395.g2e18187dfd-goog
^ permalink raw reply related
* Re: [PATCH] earlycon: Allow specifying a uartclk in options
From: Greg Kroah-Hartman @ 2018-03-01 18:42 UTC (permalink / raw)
To: Randy Dunlap
Cc: Daniel Kurtz, adurbin, briannorris, Jiri Slaby,
open list:SERIAL DRIVERS, open list
In-Reply-To: <57fb4f35-fd75-b2cc-75f7-8157ab1081d5@infradead.org>
On Thu, Mar 01, 2018 at 10:30:45AM -0800, Randy Dunlap wrote:
> On 03/01/2018 10:20 AM, Daniel Kurtz wrote:
> > Currently when an earlycon is registered, the uartclk is assumed to be
> > BASE_BAUD * 16 = 1843200. If a baud rate is specified in the earlycon
> > options, then 8250_early's init_port will program the UART clock divider
> > registers based on this assumed uartclk.
> >
> > However, not all uarts have a UART clock of 1843200. For example, the
> > 8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
> > cz_uart_desc in acpi_apd.c). Thus, specifying a baud when using earlycon
> > on such a device will result in incorrect divider values and a wrong UART
> > clock.
> >
> > Fix this by extending the earlycon options parameter to allow specification
> > of a uartclk, like so:
> >
> > earlycon=uart,mmio32,0xfedc6000,115200,48000000
> >
> > If none is specified, fall-back to prior behavior - 1843200.
> >
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>
> Hi,
>
> Hopefully there will also be an update to one of
> Documentation/admin-guide/kernel-parameters.txt or
> Documentation/admin-guide/serial-console.rst.
Well, considering I'm not taking this patch unless it comes with such an
update, I'm hoping as well :)
greg k-h
^ permalink raw reply
* Re: [PATCH] earlycon: Allow specifying a uartclk in options
From: Randy Dunlap @ 2018-03-01 18:30 UTC (permalink / raw)
To: Daniel Kurtz
Cc: adurbin, briannorris, Greg Kroah-Hartman, Jiri Slaby,
open list:SERIAL DRIVERS, open list
In-Reply-To: <20180301182028.237856-1-djkurtz@chromium.org>
On 03/01/2018 10:20 AM, Daniel Kurtz wrote:
> Currently when an earlycon is registered, the uartclk is assumed to be
> BASE_BAUD * 16 = 1843200. If a baud rate is specified in the earlycon
> options, then 8250_early's init_port will program the UART clock divider
> registers based on this assumed uartclk.
>
> However, not all uarts have a UART clock of 1843200. For example, the
> 8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
> cz_uart_desc in acpi_apd.c). Thus, specifying a baud when using earlycon
> on such a device will result in incorrect divider values and a wrong UART
> clock.
>
> Fix this by extending the earlycon options parameter to allow specification
> of a uartclk, like so:
>
> earlycon=uart,mmio32,0xfedc6000,115200,48000000
>
> If none is specified, fall-back to prior behavior - 1843200.
>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Hi,
Hopefully there will also be an update to one of
Documentation/admin-guide/kernel-parameters.txt or
Documentation/admin-guide/serial-console.rst.
Thanks.
> ---
> drivers/tty/serial/earlycon.c | 8 ++++++--
> include/linux/serial_core.h | 2 +-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index 870e84fb6e39..c9b38f520057 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -115,12 +115,17 @@ static int __init parse_options(struct earlycon_device *device, char *options)
> }
>
> if (options) {
> - device->baud = simple_strtoul(options, NULL, 0);
> + char *uartclk;
> + device->baud = simple_strtoul(options, &uartclk, 0);
> + if (*uartclk++ == ',')
> + port->uartclk = simple_strtoul(uartclk, NULL, 0);
> length = min(strcspn(options, " ") + 1,
> (size_t)(sizeof(device->options)));
> strlcpy(device->options, options, length);
> }
>
> + port->uartclk = (port->uartclk) ?: BASE_BAUD * 16;
> +
> return 0;
> }
>
> @@ -134,7 +139,6 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
> buf = NULL;
>
> spin_lock_init(&port->lock);
> - port->uartclk = BASE_BAUD * 16;
> if (port->mapbase)
> port->membase = earlycon_map(port->mapbase, 64);
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index b32df49a3bd5..b772f0d20b18 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -343,7 +343,7 @@ static inline int uart_poll_timeout(struct uart_port *port)
> struct earlycon_device {
> struct console *con;
> struct uart_port port;
> - char options[16]; /* e.g., 115200n8 */
> + char options[32]; /* e.g., 115200n8,48000000 */
> unsigned int baud;
> };
>
>
--
~Randy
^ permalink raw reply
* [PATCH] earlycon: Allow specifying a uartclk in options
From: Daniel Kurtz @ 2018-03-01 18:20 UTC (permalink / raw)
Cc: adurbin, briannorris, Daniel Kurtz, Greg Kroah-Hartman,
Jiri Slaby, open list:SERIAL DRIVERS, open list
Currently when an earlycon is registered, the uartclk is assumed to be
BASE_BAUD * 16 = 1843200. If a baud rate is specified in the earlycon
options, then 8250_early's init_port will program the UART clock divider
registers based on this assumed uartclk.
However, not all uarts have a UART clock of 1843200. For example, the
8250_dw uart in AMD's CZ/ST uses a fixed 48 MHz clock (as specified in
cz_uart_desc in acpi_apd.c). Thus, specifying a baud when using earlycon
on such a device will result in incorrect divider values and a wrong UART
clock.
Fix this by extending the earlycon options parameter to allow specification
of a uartclk, like so:
earlycon=uart,mmio32,0xfedc6000,115200,48000000
If none is specified, fall-back to prior behavior - 1843200.
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
drivers/tty/serial/earlycon.c | 8 ++++++--
include/linux/serial_core.h | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index 870e84fb6e39..c9b38f520057 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -115,12 +115,17 @@ static int __init parse_options(struct earlycon_device *device, char *options)
}
if (options) {
- device->baud = simple_strtoul(options, NULL, 0);
+ char *uartclk;
+ device->baud = simple_strtoul(options, &uartclk, 0);
+ if (*uartclk++ == ',')
+ port->uartclk = simple_strtoul(uartclk, NULL, 0);
length = min(strcspn(options, " ") + 1,
(size_t)(sizeof(device->options)));
strlcpy(device->options, options, length);
}
+ port->uartclk = (port->uartclk) ?: BASE_BAUD * 16;
+
return 0;
}
@@ -134,7 +139,6 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
buf = NULL;
spin_lock_init(&port->lock);
- port->uartclk = BASE_BAUD * 16;
if (port->mapbase)
port->membase = earlycon_map(port->mapbase, 64);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..b772f0d20b18 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -343,7 +343,7 @@ static inline int uart_poll_timeout(struct uart_port *port)
struct earlycon_device {
struct console *con;
struct uart_port port;
- char options[16]; /* e.g., 115200n8 */
+ char options[32]; /* e.g., 115200n8,48000000 */
unsigned int baud;
};
--
2.16.2.395.g2e18187dfd-goog
^ 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