1. Patchew
  2. Xen
  3. View series

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

Amneesh Singh posted 1 patch 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240820082202.326644-1-a-singh21@ti.com
xen/drivers/char/omap-uart.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
[PATCH v2] drivers: char: omap-uart: provide a default clock frequency
Posted by Amneesh Singh 3 months ago 
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.

Signed-off-by: Amneesh Singh <a-singh21@ti.com>
---
 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@ti.com/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
+
 #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);
+        clkspec = UART_OMAP_DEFAULT_CLK_SPEED;
     }
 
     uart->clock_hz = clkspec;
-- 
2.34.1
Re: [PATCH v2] drivers: char: omap-uart: provide a default clock frequency
Posted by Michal Orzel 3 months ago 

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@ti.com>
> ---
>  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@ti.com/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

> +
>  #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:
        printk("omap-uart: Clock frequency not specified, defaulting to %uHz\n",
               UART_OMAP_DEFAULT_CLK_SPEED);

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal
Re: [PATCH v2] drivers: char: omap-uart: provide a default clock frequency
Posted by Julien Grall 3 months ago 
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@ti.com>
>> ---
>>   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@ti.com/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
> 
>> +
>>   #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.

Cheers,

-- 
Julien Grall
Re: [EXTERNAL] Re: [PATCH v2] drivers: char: omap-uart: provide a default clock frequency
Posted by Amneesh Singh 3 months ago 
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@ti.com>
> >> ---
> >>   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@ti.com/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
Re: [EXTERNAL] Re: [PATCH v2] drivers: char: omap-uart: provide a default clock frequency
Posted by Julien Grall 3 months ago 

On 21/08/2024 06:35, Amneesh Singh wrote:
> Hello,

Hi Amneesh,

> 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@ti.com>
>>>> ---
>>>>    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@ti.com/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]

Thanks for the updated commit message. I have updated it while commiting 
the patch.

Cheers,

-- 
Julien Grall

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.