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 <[email protected]>
> >> ---
> >> xen/drivers/char/omap-uart.c | 8 ++++++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >> ---
> >> v1: https://lore.kernel.org/all/[email protected]/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
