[PATCH 6/6] serial: msm-geni: correct oversampling value based on QUP hardware revision

[Konrad Dybcio]

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 at 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