| Message ID | 20240820082202.326644-1-a-singh21@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drivers: char: omap-uart: provide a default clock frequency | expand |
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
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,
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
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,
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;
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.