Re: [EXTERNAL] Re: [PATCH v2] drivers: char: omap-uart: provide a default clock frequency

[Amneesh Singh <a-singh21@xxxxxx>]

Hello,

On 10:03-20240820, Julien Grall wrote:
> Hi,
> 
> On 20/08/2024 10:00, Michal Orzel wrote:
> > On 20/08/2024 10:22, 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
> >> solution as the function clock input can differ between SoCs. So, use a
> >> default frequency of 48MHz if the device tree does not specify one.
> > I'd mention that this is same as Linux
> >
> >>
> >> Signed-off-by: Amneesh Singh <a-singh21@xxxxxx>
> >> ---
> >>   xen/drivers/char/omap-uart.c | 8 ++++++--
> >>   1 file changed, 6 insertions(+), 2 deletions(-)
> >> ---
> >> v1: https://lore.kernel.org/all/20240719113313.145587-1-a-singh21@xxxxxx/T/
> >>
> >> v1 -> v2
> >> - Ditch adding a dtuart option
> >> - Use a default value instead
> >>
> >> This default is the same one as found in the 8250_omap driver of the
> >> linux tree. Already tested with Xen.
> >>
> >> diff --git a/xen/drivers/char/omap-uart.c b/xen/drivers/char/omap-uart.c
> >> index 1079198..9d3d39c 100644
> >> --- a/xen/drivers/char/omap-uart.c
> >> +++ b/xen/drivers/char/omap-uart.c
> >> @@ -48,6 +48,9 @@
> >>   /* System configuration register */
> >>   #define UART_OMAP_SYSC_DEF_CONF   0x0d   /* autoidle mode, wakeup is 
> >> enabled */
> >>
> >> +/* default clock frequency in hz */
> >> +#define UART_OMAP_DEFAULT_CLK_SPEED 48000000
> > I think this should have U suffix to please MISRA 7.2

Can I count on you to add this unsigned suffix as well? Thanks.

> >
> >> +
> >>   #define omap_read(uart, off)       readl((uart)->regs + ((off) << 
> >> REG_SHIFT))
> >>   #define omap_write(uart, off, val) writel(val, \
> >>                                             (uart)->regs + ((off) << 
> >> REG_SHIFT))
> >> @@ -322,8 +325,9 @@ static int __init omap_uart_init(struct dt_device_node 
> >> *dev,
> >>       res = dt_property_read_u32(dev, "clock-frequency", &clkspec);
> >>       if ( !res )
> >>       {
> >> -        printk("omap-uart: Unable to retrieve the clock frequency\n");
> >> -        return -EINVAL;
> >> +        printk("omap-uart: Unable to retrieve the clock frequency, "
> >> +               "defaulting to %uHz\n", UART_OMAP_DEFAULT_CLK_SPEED);
> > Even though there is a comma, printk messages should not really be split. 
> > In such cases it's fine
> > to exceed 80 chars limit. Or do:
> 
> In general, we allow to exceed the limit only for the message. If there
> are arguments then ...
> 
> >          printk("omap-uart: Clock frequency not specified, defaulting to 
> > %uHz\n",
> >                 UART_OMAP_DEFAULT_CLK_SPEED);
> 
> ... this is the preferred approach. I can do the update on commit if an
> updated commit message is provided.

Please do, thanks. You can use this as the commit message:

[quote]

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
solution as the function clock input can differ between SoCs. So, use a
default frequency of 48MHz, which is the same as the linux default (see
8250_omap.c), if the device tree does not specify it.

[/quote]

Or if you'd prefer, I can resend the thing with the suggested changes.

> 
> Cheers,
> 
> --
> Julien Grall
> 
>

Regards
Amneesh