Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi,

You sent this patch to xen-devel but forgot to CC maintainers. For the future, 
please use scripts/add_maintainers.pl.
CCing them now.

On 19/07/2024 13:33, Amneesh Singh wrote:
> 
> 
> Quite a few TI K3 devices do not have clock-frequency specified in their
> respective UART nodes. However hard-coding the frequency is not a
Is this property is required? If so, I'd mention that this is to overcome an 
incorrect device tree.

> solution as the function clock input can differ between SoCs. So,
> similar to com1/com2, let the user pass the frequency as a dtuart option
> via the bootargs. If not specified it will fallback to the same DT
> parsing as before. For example, dtuart=serial2:clock-hz=48000000 can be
> a valid bootarg.
> 
> Signed-off-by: Amneesh Singh <a-singh21@xxxxxx>
> ---
>  xen/drivers/char/omap-uart.c | 62 +++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
> index 1079198..660c486 100644
> --- a/xen/drivers/char/omap-uart.c
> +++ b/xen/drivers/char/omap-uart.c
> @@ -305,33 +305,71 @@ static struct uart_driver __read_mostly 
> omap_uart_driver = {
>      .vuart_info = omap_vuart_info,
>  };
> 
> +static void __init omap_uart_parse_config(struct omap_uart *uart,
> +                                         const char *config) {
Braces should be placed on their own lines. Refer CODING_STYLE.

> +
> +    char options[256];
256 is a max size of full dtuart string. Since we only parse for clock-hz, do 
we need the same size here?
Could we say e.g. 64 and extend it in the future if new options will be added?

> +    char *value, *start = options;
Scope of value could be limited to the while loop

> +
> +    if ( !strcmp(config, "") )
> +        return;
> +
> +    strlcpy(options, config, ARRAY_SIZE(options));
> +
> +    while (start != NULL)
White spaces missing. Refer CODING_STYLE.

> +    {
> +        char *name;
> +
> +        /* Parse next name-value pair. */
> +        value = strsep(&start, ",");
> +        name = strsep(&value, "=");
Can name be NULL here? This would want checking for the below strcmp.

> +
> +        if ( !strcmp(name, "clock-hz") )
> +            uart->clock_hz = simple_strtoul(value, NULL, 0);
> +        else
> +            printk("WARNING: UART configuration option %s is not 
> supported\n",
> +                   name);
> +
> +    }
> +}
> +
>  static int __init omap_uart_init(struct dt_device_node *dev,
>                                   const void *data)
>  {
>      const char *config = data;
>      struct omap_uart *uart;
> -    u32 clkspec;
>      int res;
>      paddr_t addr, size;
> 
> -    if ( strcmp(config, "") )
> -        printk("WARNING: UART configuration is not supported\n");
> -
>      uart = &omap_com;
> 
> -    res = dt_property_read_u32(dev, "clock-frequency", &clkspec);
> -    if ( !res )
> -    {
> -        printk("omap-uart: Unable to retrieve the clock frequency\n");
> -        return -EINVAL;
> -    }
> -
> -    uart->clock_hz = clkspec;
> +    /* Default configuration. */
> +    uart->clock_hz = 0;
>      uart->baud = 115200;
>      uart->data_bits = 8;
>      uart->parity = UART_PARITY_NONE;
>      uart->stop_bits = 1;
> 
> +    /*
> +     * Parse dtuart options.
> +     * Valid options:
> +     *   - clock-hz
> +     */
> +    omap_uart_parse_config(uart, config);
> +
> +    /* If clock-hz is missing. */
Apart from missing, clock_hz can be 0 also if user specifies 0

> +    if ( uart->clock_hz == 0 )
> +    {
> +        u32 clkspec;
We are moving away from Linux derived types so please take the occasion to use 
uint32_t here.
Also, there should be a blank line between definitions/code.

> +        res = dt_property_read_u32(dev, "clock-frequency", &clkspec);
> +        if ( !res )
> +        {
> +            printk("omap-uart: Unable to retrieve the clock frequency\n");
> +            return -EINVAL;
> +        }
> +        uart->clock_hz = clkspec;
> +    }
> +
>      res = dt_device_get_paddr(dev, 0, &addr, &size);
>      if ( res )
>      {
> --
> 2.34.1
> 
> 

~Michal