| Message ID | 20230330194736.2400593-7-vladimir.zapolskiy@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | serial: msm-geni: fix UART baudrate on modern platforms | expand |
On 30.03.2023 21:47, Vladimir Zapolskiy wrote: > 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/serial_msm_geni.c | 32 +++++++++++++++++++++++++++++++- > 1 file changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c > index 03fc704182d3..cdca7e83daa6 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 Should we perhaps store this in the parent's dev / priv data? If we had to take care of it for other GENI peripherals, we would have to redo it over and over again. > + > /* > * 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,27 @@ static const struct dm_serial_ops msm_serial_ops = { > .setbrg = msm_serial_setbrg, > }; > > +static inline void geni_get_oversampling(struct udevice *dev) Nit: functions with _get_ in their names are generally expected to return the value they're getting, perhaps _adjust_ or _set_ would be more fitting. > +{ > + 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 QUP driver is disabled or GENI UART > + * device tree 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, 4); sizeof(int) or 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 +550,8 @@ static int msm_serial_probe(struct udevice *dev) > { > struct msm_serial_data *priv = dev_get_priv(dev); > > + geni_get_oversampling(dev); > + > /* No need to reinitialize the UART after relocation */ > if (gd->flags & GD_FLG_RELOC) > return 0; > @@ -557,6 +586,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, This change was not mentioned in the commit message. You can pick up https://lore.kernel.org/u-boot/20230327-qc_cleanups-v2-4-9a80cc563c76@linaro.org/ which also cleans up the remnants of this in the DTs. It will however need to be rebased against the `next` branch and 8c103c33fb14 ("dm: dts: Convert driver model tags to use new schema") Konrad > }; > > #ifdef CONFIG_DEBUG_UART_MSM_GENI
Hi Konrad, On 3/31/23 04:36, Konrad Dybcio wrote: > > > On 30.03.2023 21:47, Vladimir Zapolskiy wrote: >> 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/serial_msm_geni.c | 32 +++++++++++++++++++++++++++++++- >> 1 file changed, 31 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c >> index 03fc704182d3..cdca7e83daa6 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 > Should we perhaps store this in the parent's dev / priv data? > If we had to take care of it for other GENI peripherals, we > would have to redo it over and over again. Generally speaking the defines should be found in a (not introduced) GENI SE header, but for the matter of simplicity I decide to put it right in the consumer driver, and it allows to stick to the generic misc DM interface. Looking at the Linux source code, there is no other users of the define but the GENI serial driver, so I hope there is a minimal risk of spreading of the macro over the drivers. >> + >> /* >> * 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,27 @@ static const struct dm_serial_ops msm_serial_ops = { >> .setbrg = msm_serial_setbrg, >> }; >> >> +static inline void geni_get_oversampling(struct udevice *dev) > Nit: functions with _get_ in their names are generally expected to > return the value they're getting, perhaps _adjust_ or _set_ would > be more fitting. Acked, I'll rename it to 'set'. >> +{ >> + 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 QUP driver is disabled or GENI UART >> + * device tree 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, 4); > sizeof(int) or sizeof(geni_se_version)? Sure, no objections. >> + 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 +550,8 @@ static int msm_serial_probe(struct udevice *dev) >> { >> struct msm_serial_data *priv = dev_get_priv(dev); >> >> + geni_get_oversampling(dev); >> + >> /* No need to reinitialize the UART after relocation */ >> if (gd->flags & GD_FLG_RELOC) >> return 0; >> @@ -557,6 +586,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, > This change was not mentioned in the commit message. You can > pick up > > https://lore.kernel.org/u-boot/20230327-qc_cleanups-v2-4-9a80cc563c76@linaro.org/ > > which also cleans up the remnants of this in the DTs. > It will however need to be rebased against the `next` branch and > > 8c103c33fb14 ("dm: dts: Convert driver model tags to use new schema") Sure, I'll take your change to the v2 of the series. Thank you for review! -- Best wishes, Vladimir
diff --git a/drivers/serial/serial_msm_geni.c b/drivers/serial/serial_msm_geni.c index 03fc704182d3..cdca7e83daa6 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,27 @@ static const struct dm_serial_ops msm_serial_ops = { .setbrg = msm_serial_setbrg, }; +static inline void geni_get_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 QUP driver is disabled or GENI UART + * device tree 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, 4); + 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 +550,8 @@ static int msm_serial_probe(struct udevice *dev) { struct msm_serial_data *priv = dev_get_priv(dev); + geni_get_oversampling(dev); + /* No need to reinitialize the UART after relocation */ if (gd->flags & GD_FLG_RELOC) return 0; @@ -557,6 +586,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
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/serial_msm_geni.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)