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

[Amneesh Singh <a-singh21@xxxxxx>]

Hi, thanks a lot for the review!

On 06/08/24 13:23, Michal Orzel wrote:

> 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.
>     
    Apologies, was not aware of that.
> 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.
>     
    The clock input is usually supposed to be at 48MHz, but can still
differ between SoCs, and if I am not wrong can also be changed from
the bootloader. This makes it hard to specify this value in the device
tree itself; which is however required by the current omap-uart driver
as it neither has a default nor a way to parse config params, and relies
on parsing the DT.


> > 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.
>     
    Apologies for the carelessness.
> > +
> > +    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?
>     
    Done, don't think it matters much, but sure thing.
> > +    char *value, *start = options;
> Scope of value could be limited to the while loop
>     
    Yes, that's true.
> > +
> > +    if ( !strcmp(config, "") )
> > +        return;
> > +
> > +    strlcpy(options, config, ARRAY_SIZE(options));
> > +
> > +    while (start != NULL)
> White spaces missing. Refer CODING_STYLE.
>     
    Thx.
> > +    {
> > +        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.
>     
    Indeed, both the name and value can be NULL at this point. Thx.

> > +
> > +        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
>     
    That's true, thx.
> > +    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.
>     
    Got it.
> > +        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
>     
    Regards
    Amneesh

Attachment: OpenPGP_0x104493B4D46917EC.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature