[PATCH v2 0/9] serial: msm-geni: fix UART baudrate on modern platforms
The changeset touches Qualcomm platforms, it adds a new quite trivial misc wrapper driver to be accessed by GENI UART to get information about a proper clock divisor.
The change does not intend to break any currently supported Qualcomm platforms, there should be no need to update board config or dts files.
Changes from v1 to v2: * fixes according to the code review by Konrad, * added two changes developed by Konrad to the series, * removed .bind from the GENI SE wrapper driver, * minor fix in MSM GENI serial driver Kconfig to match the new changes, * changed dts files of SDM845 powered boards by adding GENI SE wrapper.
Konrad Dybcio (2): serial: msm-geni: Always bind before relocation serial: msm-geni: Use upstream Linux bindings
Vladimir Zapolskiy (7): misc: add Qualcomm GENI SE QUP device driver serial: msm-geni: remove redundant includes serial: msm-geni: remove invalid se-clk clock name serial: msm-geni: fix code indentation serial: msm-geni: fix a compile time warning from msm_serial_setbrg() serial: msm-geni: correct oversampling value based on QUP hardware revision arm: dts: sdm845: add GENI SE QUP device tree node
arch/arm/dts/dragonboard845c.dts | 2 +- arch/arm/dts/sdm845.dtsi | 25 +++++--- arch/arm/dts/starqltechn.dts | 2 +- .../serial/msm-geni-serial.txt | 2 +- drivers/misc/Kconfig | 7 +++ drivers/misc/Makefile | 1 + drivers/misc/qcom-geni-se.c | 41 ++++++++++++ drivers/serial/Kconfig | 2 + drivers/serial/serial_msm_geni.c | 62 +++++++++++++------ 9 files changed, 113 insertions(+), 31 deletions(-) create mode 100644 drivers/misc/qcom-geni-se.c
This change adds a Qualcomm GENI SE QUP device driver as a wrapper for actually enabled and used serial devices found on a board.
At the moment the driver is pretty simple, its intention is to populate childred devices and provide I/O mem read interface to them as clients, this is needed for GENI UART driver to set up a proper clock divider and provide the actually asked baud rate.
Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org Reviewed-by: Konrad Dybcio konrad.dybcio@linaro.org --- drivers/misc/Kconfig | 7 +++++++ drivers/misc/Makefile | 1 + drivers/misc/qcom-geni-se.c | 41 +++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 drivers/misc/qcom-geni-se.c
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 4e1ae03e9fd4..04460f1acb25 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -511,6 +511,13 @@ config WINBOND_W83627 legacy UART or other devices in the Winbond Super IO chips on X86 platforms.
+config QCOM_GENI_SE + bool "Qualcomm GENI Serial Engine Driver" + depends on ARCH_SNAPDRAGON + help + The driver manages Generic Interface (GENI) firmware based + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. + config QFW bool help diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 3b792f2a14ce..52aed096021f 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -60,6 +60,7 @@ obj-$(CONFIG_NUVOTON_NCT6102D) += nuvoton_nct6102d.o obj-$(CONFIG_P2SB) += p2sb-uclass.o obj-$(CONFIG_PCA9551_LED) += pca9551_led.o obj-$(CONFIG_$(SPL_)PWRSEQ) += pwrseq-uclass.o +obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o ifdef CONFIG_QFW obj-y += qfw.o obj-$(CONFIG_QFW_PIO) += qfw_pio.o diff --git a/drivers/misc/qcom-geni-se.c b/drivers/misc/qcom-geni-se.c new file mode 100644 index 000000000000..b1443ad66d26 --- /dev/null +++ b/drivers/misc/qcom-geni-se.c @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Qualcomm Generic Interface (GENI) Serial Engine (SE) Wrapper + * + * Copyright (C) 2023 Linaro Ltd. vladimir.zapolskiy@linaro.org + */ + +#include <common.h> +#include <dm.h> +#include <misc.h> +#include <asm/io.h> + +static int geni_se_qup_read(struct udevice *dev, int offset, + void *buf, int size) +{ + fdt_addr_t base = dev_read_addr(dev); + + if (size != sizeof(u32)) + return -EINVAL; + + *(u32 *)buf = readl(base + offset); + + return 0; +} + +static struct misc_ops geni_se_qup_ops = { + .read = geni_se_qup_read, +}; + +static const struct udevice_id geni_se_qup_ids[] = { + { .compatible = "qcom,geni-se-qup" }, + {} +}; + +U_BOOT_DRIVER(geni_se_qup) = { + .name = "geni_se_qup", + .id = UCLASS_MISC, + .of_match = geni_se_qup_ids, + .ops = &geni_se_qup_ops, + .flags = DM_FLAG_PRE_RELOC, +};
For whatever reason, likely a driver stub was copied from another driver, the driver contains a bunch of unnecessary and confusing includes like watchdog.h etc., the change reduces the list.
Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org Reviewed-by: Konrad Dybcio konrad.dybcio@linaro.org --- drivers/serial/serial_msm_geni.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 3943ca43e49e..df61ae04df0a 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -11,15 +11,9 @@ #include <clk.h> #include <common.h> #include <dm.h> -#include <dm/pinctrl.h> #include <errno.h> -#include <linux/compiler.h> -#include <log.h> #include <linux/delay.h> -#include <malloc.h> #include <serial.h> -#include <watchdog.h> -#include <linux/bug.h>
#define UART_OVERSAMPLING 32 #define STALE_TIMEOUT 160
From: Konrad Dybcio konrad.dybcio@linaro.org
In preparation for supporting upstream Linux device trees on Qualcomm platforms, make this the default behavior.
[vzapolskiy: extracted the driver change from a combination with dts changes] Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org --- drivers/serial/serial_msm_geni.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index df61ae04df0a..146b05748459 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -558,6 +558,7 @@ U_BOOT_DRIVER(serial_msm_geni) = { .priv_auto = sizeof(struct msm_serial_data), .probe = msm_serial_probe, .ops = &msm_serial_ops, + .flags = DM_FLAG_PRE_RELOC, };
#ifdef CONFIG_DEBUG_UART_MSM_GENI
There is only one clock supplier to the serial IP, thus getting it by name is not needed, also note that "clock-names" property is not listed under doc/device-tree-bindings/serial/msm-geni-serial.txt, and finally "se-clk" clock name is invalid, if added, it shall get "se" value like it's already described in Linux device tree documentation.
Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org Reviewed-by: Konrad Dybcio konrad.dybcio@linaro.org --- drivers/serial/serial_msm_geni.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 146b05748459..8fd769eb4d0d 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -183,7 +183,7 @@ static int geni_serial_set_clock_rate(struct udevice *dev, u64 rate) struct clk *clk; int ret;
- clk = devm_clk_get(dev, "se-clk"); + clk = devm_clk_get(dev, NULL); if (!clk) return -EINVAL;
This a cosmetic change, which corrects code indentation in a few places.
Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org Reviewed-by: Konrad Dybcio konrad.dybcio@linaro.org --- drivers/serial/serial_msm_geni.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 8fd769eb4d0d..55dd9188a56c 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -130,8 +130,8 @@ struct msm_serial_data { };
unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200, - 32000000, 48000000, 64000000, 80000000, - 96000000, 100000000}; + 32000000, 48000000, 64000000, 80000000, + 96000000, 100000000};
/** * get_clk_cfg() - Get clock rate to apply on clock supplier. @@ -160,8 +160,7 @@ static int get_clk_cfg(unsigned long clk_freq) * * Return: frequency, supported by clock supplier, multiple of clk_freq. */ -static int get_clk_div_rate(u32 baud, - u64 sampling_rate, u32 *clk_div) +static int get_clk_div_rate(u32 baud, u64 sampling_rate, u32 *clk_div) { unsigned long ser_clk; unsigned long desired_clk; @@ -228,7 +227,7 @@ static inline u32 geni_se_get_tx_fifo_width(long base) }
static inline void geni_serial_baud(phys_addr_t base_address, u32 clk_div, - int baud) + int baud) { u32 s_clk_cfg = 0;
@@ -268,7 +267,7 @@ int msm_serial_setbrg(struct udevice *dev, int baud) * reached. */ static bool qcom_geni_serial_poll_bit(const struct udevice *dev, int offset, - int field, bool set) + int field, bool set) { u32 reg; struct msm_serial_data *priv = dev_get_priv(dev);
A compiler warns about a missing function prototype, which is valid and fixed by converting the function into static one, also fix interleaved local variable declarations and assignments.
Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org Fixes: 324df15a292e ("serial: qcom: add support for GENI serial driver") Reviewed-by: Konrad Dybcio konrad.dybcio@linaro.org --- drivers/serial/serial_msm_geni.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 55dd9188a56c..3a200f45a6ce 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -238,13 +238,13 @@ static inline void geni_serial_baud(phys_addr_t base_address, u32 clk_div, writel(s_clk_cfg, base_address + GENI_SER_S_CLK_CFG); }
-int msm_serial_setbrg(struct udevice *dev, int baud) +static int msm_serial_setbrg(struct udevice *dev, int baud) { struct msm_serial_data *priv = dev_get_priv(dev); + u64 clk_rate; + u32 clk_div;
priv->baud = baud; - u32 clk_div; - u64 clk_rate;
clk_rate = get_clk_div_rate(baud, UART_OVERSAMPLING, &clk_div); geni_serial_set_clock_rate(dev, clk_rate);
From: Konrad Dybcio konrad.dybcio@linaro.org
The name "se" is used in upstream Linux device trees and has been for ages, long before this U-Boot-ism was introduced. Same goes for the existing compatible. Get rid of that.
[vzapolskiy: removed a ready change in the driver] Signed-off-by: Konrad Dybcio konrad.dybcio@linaro.org Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org --- arch/arm/dts/sdm845.dtsi | 4 ++-- doc/device-tree-bindings/serial/msm-geni-serial.txt | 2 +- drivers/serial/serial_msm_geni.c | 4 +++- 3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi index 607af277f8be..92bdc82177d6 100644 --- a/arch/arm/dts/sdm845.dtsi +++ b/arch/arm/dts/sdm845.dtsi @@ -52,10 +52,10 @@ };
debug_uart: serial@a84000 { - compatible = "qcom,msm-geni-uart"; + compatible = "qcom,geni-debug-uart"; reg = <0xa84000 0x4000>; reg-names = "se_phys"; - clock-names = "se-clk"; + clock-names = "se"; clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>; pinctrl-names = "default"; pinctrl-0 = <&qup_uart9>; diff --git a/doc/device-tree-bindings/serial/msm-geni-serial.txt b/doc/device-tree-bindings/serial/msm-geni-serial.txt index 9eadc2561b4b..eaa39c949b10 100644 --- a/doc/device-tree-bindings/serial/msm-geni-serial.txt +++ b/doc/device-tree-bindings/serial/msm-geni-serial.txt @@ -1,6 +1,6 @@ Qualcomm GENI UART
Required properties: -- compatible: must be "qcom,msm-geni-uart" +- compatible: must be "qcom,geni-debug-uart" - reg: start address and size of the registers - clock: interface clock (must accept baudrate as a frequency) diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 3a200f45a6ce..29fae810d6fe 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -547,7 +547,9 @@ static int msm_serial_ofdata_to_platdata(struct udevice *dev) }
static const struct udevice_id msm_serial_ids[] = { - {.compatible = "qcom,msm-geni-uart"}, {}}; + { .compatible = "qcom,geni-debug-uart" }, + { } +};
U_BOOT_DRIVER(serial_msm_geni) = { .name = "serial_msm_geni",
Starting from QUP v2.5 the value of oversampling is changed from 32 to 16, keeping the old value on newer platforms results on wrong set UART IP clock divider, thus the asked baudrate does not correspond to the actually set with all the consequencies for a user.
The change links the driver to a new Qualcomm GENI SE QUP driver to get its hardware version and update the oversampling value.
Deliberately the code under CONFIG_DEBUG_UART_MSM_GENI is not touched, since a wanted baudrate can be controlled by setting a modified CONFIG_DEBUG_UART_CLOCK build time variable.
Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org --- drivers/serial/Kconfig | 2 ++ drivers/serial/serial_msm_geni.c | 32 +++++++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig index 10d07daf2777..7faf67844424 100644 --- a/drivers/serial/Kconfig +++ b/drivers/serial/Kconfig @@ -946,6 +946,8 @@ config MSM_SERIAL
config MSM_GENI_SERIAL bool "Qualcomm on-chip GENI UART" + select MISC + imply QCOM_GENI_SE help Support UART based on Generic Interface (GENI) Serial Engine (SE), used on Qualcomm Snapdragon SoCs. Should support all qualcomm SOCs diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 29fae810d6fe..b76ed3a30017 100644 --- a/drivers/serial/serial_msm_geni.c +++ b/drivers/serial/serial_msm_geni.c @@ -13,6 +13,7 @@ #include <dm.h> #include <errno.h> #include <linux/delay.h> +#include <misc.h> #include <serial.h>
#define UART_OVERSAMPLING 32 @@ -110,6 +111,10 @@ #define TX_FIFO_DEPTH_MSK (GENMASK(21, 16)) #define TX_FIFO_DEPTH_SHFT 16
+/* GENI SE QUP Registers */ +#define QUP_HW_VER_REG 0x4 +#define QUP_SE_VERSION_2_5 0x20050000 + /* * Predefined packing configuration of the serial engine (CFG0, CFG1 regs) * for uart mode. @@ -127,6 +132,7 @@ DECLARE_GLOBAL_DATA_PTR; struct msm_serial_data { phys_addr_t base; u32 baud; + u32 oversampling; };
unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200, @@ -246,7 +252,7 @@ static int msm_serial_setbrg(struct udevice *dev, int baud)
priv->baud = baud;
- clk_rate = get_clk_div_rate(baud, UART_OVERSAMPLING, &clk_div); + clk_rate = get_clk_div_rate(baud, priv->oversampling, &clk_div); geni_serial_set_clock_rate(dev, clk_rate); geni_serial_baud(priv->base, clk_div, baud);
@@ -480,6 +486,28 @@ static const struct dm_serial_ops msm_serial_ops = { .setbrg = msm_serial_setbrg, };
+static void geni_set_oversampling(struct udevice *dev) +{ + struct msm_serial_data *priv = dev_get_priv(dev); + struct udevice *parent_dev = dev_get_parent(dev); + u32 geni_se_version; + int ret; + + priv->oversampling = UART_OVERSAMPLING; + + /* + * It could happen that GENI SE IP is missing in the board's device + * tree or GENI UART node is a direct child of SoC device tree node. + */ + if (device_get_uclass_id(parent_dev) != UCLASS_MISC) + return; + + ret = misc_read(parent_dev, QUP_HW_VER_REG, + &geni_se_version, sizeof(geni_se_version)); + if (!ret && geni_se_version >= QUP_SE_VERSION_2_5) + priv->oversampling /= 2; +} + static inline void geni_serial_init(struct udevice *dev) { struct msm_serial_data *priv = dev_get_priv(dev); @@ -523,6 +551,8 @@ static int msm_serial_probe(struct udevice *dev) { struct msm_serial_data *priv = dev_get_priv(dev);
+ geni_set_oversampling(dev); + /* No need to reinitialize the UART after relocation */ if (gd->flags & GD_FLG_RELOC) return 0;
On modern Qualcomm platforms including SDM845 a GENI SE QUP IP description is supposed to be found in board device tree nodes, the version of the IP is used by the GENI UART driver to properly set an oversampling divider value, which impacts UART baudrate.
The change touches dragonboard845c and starqltechn board device tree source files, a device tree node label to "debug" UART is renamed to 'uart9' according to the naming found in Linux.
Signed-off-by: Vladimir Zapolskiy vladimir.zapolskiy@linaro.org --- arch/arm/dts/dragonboard845c.dts | 2 +- arch/arm/dts/sdm845.dtsi | 25 +++++++++++++++---------- arch/arm/dts/starqltechn.dts | 2 +- 3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/arch/arm/dts/dragonboard845c.dts b/arch/arm/dts/dragonboard845c.dts index 1722dce33ff2..b4f057ac6537 100644 --- a/arch/arm/dts/dragonboard845c.dts +++ b/arch/arm/dts/dragonboard845c.dts @@ -21,7 +21,7 @@ };
aliases { - serial0 = &debug_uart; + serial0 = &uart9; };
memory { diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi index 92bdc82177d6..3b86b9328fc6 100644 --- a/arch/arm/dts/sdm845.dtsi +++ b/arch/arm/dts/sdm845.dtsi @@ -51,16 +51,21 @@ }; };
- debug_uart: serial@a84000 { - compatible = "qcom,geni-debug-uart"; - reg = <0xa84000 0x4000>; - reg-names = "se_phys"; - clock-names = "se"; - clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>; - pinctrl-names = "default"; - pinctrl-0 = <&qup_uart9>; - qcom,wrapper-core = <0x8a>; - status = "disabled"; + qupv3_id_1: geniqup@ac0000 { + compatible = "qcom,geni-se-qup"; + reg = <0x00ac0000 0x6000>; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + uart9: serial@a84000 { + compatible = "qcom,geni-debug-uart"; + reg = <0xa84000 0x4000>; + clock-names = "se"; + clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>; + pinctrl-names = "default"; + pinctrl-0 = <&qup_uart9>; + }; };
spmi@c440000 { diff --git a/arch/arm/dts/starqltechn.dts b/arch/arm/dts/starqltechn.dts index 34a4f59cbd17..dcbc3b6d4966 100644 --- a/arch/arm/dts/starqltechn.dts +++ b/arch/arm/dts/starqltechn.dts @@ -21,7 +21,7 @@ };
aliases { - serial0 = &debug_uart; + serial0 = &uart9; };
memory {
participants (1)
-
Vladimir Zapolskiy