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

Amneesh Singh posted 1 patch 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240719113313.145587-1-a-singh21@ti.com
xen/drivers/char/omap-uart.c | 62 +++++++++++++++++++++++++++++-------
1 file changed, 50 insertions(+), 12 deletions(-)
[PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Amneesh Singh 1 month, 2 weeks 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,
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@ti.com>
---
 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) {
+
+    char options[256];
+    char *value, *start = options;
+
+    if ( !strcmp(config, "") )
+        return;
+
+    strlcpy(options, config, ARRAY_SIZE(options));
+
+    while (start != NULL)
+    {
+        char *name;
+
+        /* Parse next name-value pair. */
+        value = strsep(&start, ",");
+        name = strsep(&value, "=");
+
+        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. */
+    if ( uart->clock_hz == 0 )
+    {
+        u32 clkspec;
+        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
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Julien Grall 1 month ago
Hi,

On 19/07/2024 12:33, Amneesh Singh wrote:
> Quite a few TI K3 devices do not have clock-frequency specified in their
> respective UART nodes.

Can you outline why fixing the device-tree is not solution?

> However hard-coding the frequency is not a
 > solution as the function clock input can differ between SoCs.

Can you provide some details how Linux is handling those 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.

Regardless my questions, any change to the command line needs to be 
documented in docs/misc/xen-command-line.pandoc.

Cheers,

-- 
Julien Grall
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Amneesh Singh 1 month ago
On 10:16-20240806, Julien Grall wrote:
> Hi,
> 
> On 19/07/2024 12:33, Amneesh Singh wrote:
> > Quite a few TI K3 devices do not have clock-frequency specified in their
> > respective UART nodes.
> 
> Can you outline why fixing the device-tree is not solution?
Because other frequencies, say 96MHz or 192 MHz are also valid inputs.
> 
> > However hard-coding the frequency is not a
>  > solution as the function clock input can differ between SoCs.
> 
> Can you provide some details how Linux is handling those SoCs?
Yes, like omap-uart under xen, the 8250_omap driver also parses the DT,
but unlike omap-uart, falls back to clk_get_rate() and if the value is
still zero, it defaults to 48MHz.
> 
> > 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.
> 
> Regardless my questions, any change to the command line needs to be
> documented in docs/misc/xen-command-line.pandoc.
I am not sure if that will be necessary as the dtuart option already
says: "The options are device specific."
> 
> Cheers,
> 
> --
> Julien Grall
> 
> 
Regards
Amneesh
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Julien Grall 1 month ago

On 06/08/2024 10:50, Amneesh Singh wrote:
> On 10:16-20240806, Julien Grall wrote:
>> Hi,
>>
>> On 19/07/2024 12:33, Amneesh Singh wrote:
>>> Quite a few TI K3 devices do not have clock-frequency specified in their
>>> respective UART nodes.
>>
>> Can you outline why fixing the device-tree is not solution?
> Because other frequencies, say 96MHz or 192 MHz are also valid inputs.

Are you saying this is configurable by the user? Or do you mean the 
firmware can configure it?

>>
>>> However hard-coding the frequency is not a
>>   > solution as the function clock input can differ between SoCs.
>>
>> Can you provide some details how Linux is handling those SoCs?
> Yes, like omap-uart under xen, the 8250_omap driver also parses the DT,
> but unlike omap-uart, falls back to clk_get_rate() and if the value is
> still zero, it defaults to 48MHz.

Thanks for the information. Then my next question is why Linux can get 
away with a default value and not Xen?

To give more context, I don't feel it is right to ask the user to 
specify the clock speed. Xen should have a sane default like Linux.

>>
>>> 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.
>>
>> Regardless my questions, any change to the command line needs to be
>> documented in docs/misc/xen-command-line.pandoc.
> I am not sure if that will be necessary as the dtuart option already
> says: "The options are device specific."

Let me ask differently, if you don't document in 
xen-command-line.pandoc, then how do you expect the user to know that 
the option clockspeed exists and what is the expected value?

To me the right place is in xen-command-line.pandoc because it should 
describe all the options (including device specific ones).

Cheers,

-- 
Julien Grall
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Amneesh Singh 1 month ago
On 10:56-20240806, Julien Grall wrote:
> On 06/08/2024 10:50, Amneesh Singh wrote:
> > On 10:16-20240806, Julien Grall wrote:
> >> Hi,
> >>
> >> On 19/07/2024 12:33, Amneesh Singh wrote:
> >>> Quite a few TI K3 devices do not have clock-frequency specified in their
> >>> respective UART nodes.
> >>
> >> Can you outline why fixing the device-tree is not solution?
> > Because other frequencies, say 96MHz or 192 MHz are also valid inputs.
> 
> Are you saying this is configurable by the user? Or do you mean the
> firmware can configure it?
u-boot or some other bootloader are free to configure it. And usually,
linux's clock driver will pick it up using clk_get_rate (if not
specified in the DT), I think. Now, in case we add the frequency to the
DT, it may not match with the actual frequency configured before Xen
initialisation. Since, there is no equivalent to clk_get_rate under Xen,
and the fact I'm using imagebuilder, I found it easier to pass the
frequency the way I did.
Apologies if I come off as unclear. I recently joined as an intern and
am playing around with Xen.
> 
> >>
> >>> However hard-coding the frequency is not a
> >>   > solution as the function clock input can differ between SoCs.
> >>
> >> Can you provide some details how Linux is handling those SoCs?
> > Yes, like omap-uart under xen, the 8250_omap driver also parses the DT,
> > but unlike omap-uart, falls back to clk_get_rate() and if the value is
> > still zero, it defaults to 48MHz.
> 
> Thanks for the information. Then my next question is why Linux can get
> away with a default value and not Xen?
Sure why not? I guess, we can use a default value if everything fails
but there should still be a way for the user to specify the frequency.
Of course, we can instead just force the user to change the DT slightly
by just specifying the frequency. However, I feel it is easier to add it
here, especially when there's already a method to pass these options via
the command-line in place.
I believe, this is the best we can do with this.
> 
> To give more context, I don't feel it is right to ask the user to
> specify the clock speed. Xen should have a sane default like Linux.
> 
> >>
> >>> 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.
> >>
> >> Regardless my questions, any change to the command line needs to be
> >> documented in docs/misc/xen-command-line.pandoc.
> > I am not sure if that will be necessary as the dtuart option already
> > says: "The options are device specific."
> 
> Let me ask differently, if you don't document in
> xen-command-line.pandoc, then how do you expect the user to know that
> the option clockspeed exists and what is the expected value?
> 
> To me the right place is in xen-command-line.pandoc because it should
> describe all the options (including device specific ones).
I have no qualms with adding device specific options, if that's not an
issue with everyone else :)
> 
> Cheers,
> 
> --
> Julien Grall
> 
> 
Thanks and regards
Amneesh
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Julien Grall 1 month ago
Hi,

On 06/08/2024 11:35, Amneesh Singh wrote:
> On 10:56-20240806, Julien Grall wrote:
>> On 06/08/2024 10:50, Amneesh Singh wrote:
>>> On 10:16-20240806, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 19/07/2024 12:33, Amneesh Singh wrote:
>>>>> Quite a few TI K3 devices do not have clock-frequency specified in their
>>>>> respective UART nodes.
>>>>
>>>> Can you outline why fixing the device-tree is not solution?
>>> Because other frequencies, say 96MHz or 192 MHz are also valid inputs.
>>
>> Are you saying this is configurable by the user? Or do you mean the
>> firmware can configure it?
> u-boot or some other bootloader are free to configure it. And usually,
> linux's clock driver will pick it up using clk_get_rate (if not
> specified in the DT), I think. Now, in case we add the frequency to the
> DT, it may not match with the actual frequency configured before Xen
> initialisation. Since, there is no equivalent to clk_get_rate under Xen,
> and the fact I'm using imagebuilder, I found it easier to pass the
> frequency the way I did.

Thanks for the explanation. I haven't looked in details, but how 
difficult would it be to implement clk_get_rate() (at least just enough 
to work for the UART) in Xen?

>>>>
>>>>> However hard-coding the frequency is not a
>>>>    > solution as the function clock input can differ between SoCs.
>>>>
>>>> Can you provide some details how Linux is handling those SoCs?
>>> Yes, like omap-uart under xen, the 8250_omap driver also parses the DT,
>>> but unlike omap-uart, falls back to clk_get_rate() and if the value is
>>> still zero, it defaults to 48MHz.
>>
>> Thanks for the information. Then my next question is why Linux can get
>> away with a default value and not Xen?
> Sure why not? I guess, we can use a default value if everything fails
> but there should still be a way for the user to specify the frequency.

I think I am still missing something. Why would Xen allow the user to 
specify the clock speed if Linux doesn't? At least to me, it is more 
likely that a user would want to boot Linux on your HW than Xen...

> Of course, we can instead just force the user to change the DT slightly
> by just specifying the frequency. However, I feel it is easier to add it
> here, especially when there's already a method to pass these options via
> the command-line in place.
 > I believe, this is the best we can do with this.

The problem is the command line is OS/hypervisor specific. But the 
problem you describe doesn't seem Xen specific. This is where the DT is 
handy because you describe once and it can be used everywhere.

Overall, at the moment, I don't think the command line option is the 
right approach. If I am not mistaken, it would make Xen less 
user-friendly (compare to Linux) to boot on your HW as the user would 
need to specify the clockspeed on the command line. I think we should 
investigate other approaches such as implementing partially 
clk_get_rate() (if this is how Linux manage to retrieve the clock speed 
without any command line option).

Cheers,

-- 
Julien Grall
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Amneesh Singh 1 month ago
Hi

On 12:08-20240806, Julien Grall wrote:
> Hi,
> 
> On 06/08/2024 11:35, Amneesh Singh wrote:
> > On 10:56-20240806, Julien Grall wrote:
> >> On 06/08/2024 10:50, Amneesh Singh wrote:
> >>> On 10:16-20240806, Julien Grall wrote:
> >>>> Hi,
> >>>>
> >>>> On 19/07/2024 12:33, Amneesh Singh wrote:
> >>>>> Quite a few TI K3 devices do not have clock-frequency specified in their
> >>>>> respective UART nodes.
> >>>>
> >>>> Can you outline why fixing the device-tree is not solution?
> >>> Because other frequencies, say 96MHz or 192 MHz are also valid inputs.
> >>
> >> Are you saying this is configurable by the user? Or do you mean the
> >> firmware can configure it?
> > u-boot or some other bootloader are free to configure it. And usually,
> > linux's clock driver will pick it up using clk_get_rate (if not
> > specified in the DT), I think. Now, in case we add the frequency to the
> > DT, it may not match with the actual frequency configured before Xen
> > initialisation. Since, there is no equivalent to clk_get_rate under Xen,
> > and the fact I'm using imagebuilder, I found it easier to pass the
> > frequency the way I did.
> 
> Thanks for the explanation. I haven't looked in details, but how
> difficult would it be to implement clk_get_rate() (at least just enough
> to work for the UART) in Xen?
To be honest, I have no idea, I am not at all familiar with linux's
clock API. I just did this to get UART to work under Xen.
> >>>>
> >>>>> However hard-coding the frequency is not a
> >>>>    > solution as the function clock input can differ between SoCs.
> >>>>
> >>>> Can you provide some details how Linux is handling those SoCs?
> >>> Yes, like omap-uart under xen, the 8250_omap driver also parses the DT,
> >>> but unlike omap-uart, falls back to clk_get_rate() and if the value is
> >>> still zero, it defaults to 48MHz.
> >>
> >> Thanks for the information. Then my next question is why Linux can get
> >> away with a default value and not Xen?
> > Sure why not? I guess, we can use a default value if everything fails
> > but there should still be a way for the user to specify the frequency.
> 
> I think I am still missing something. Why would Xen allow the user to
> specify the clock speed if Linux doesn't? At least to me, it is more
> likely that a user would want to boot Linux on your HW than Xen...
That was not worded correctly, apologies. What I meant was a way for
the user to specify it since apart from DT, unlike linux, there is no
way for Xen to determine the frequency other than using default of
course.
> 
> > Of course, we can instead just force the user to change the DT slightly
> > by just specifying the frequency. However, I feel it is easier to add it
> > here, especially when there's already a method to pass these options via
> > the command-line in place.
>  > I believe, this is the best we can do with this.
> 
> The problem is the command line is OS/hypervisor specific. But the
> problem you describe doesn't seem Xen specific. This is where the DT is
> handy because you describe once and it can be used everywhere.
> 
> Overall, at the moment, I don't think the command line option is the
> right approach. If I am not mistaken, it would make Xen less
> user-friendly (compare to Linux) to boot on your HW as the user would
You are correct that it's an extra step, but curently, it's either
specifying it in the bootarg or specifying in the DT. Both are the
things that the user would have to do. I just went with what was easier
for me as a user. It is pretty similar to the current com1/com2
interface anyway.

> need to specify the clockspeed on the command line. I think we should
> investigate other approaches such as implementing partially
> clk_get_rate() (if this is how Linux manage to retrieve the clock speed
> without any command line option).
I guess, we can just ditch the entire idea, and just use the a default
fallback and just print a message informing the user that there is no
frequency in the DT. I do not think implementing clk_get_rate (or any
clock API) is feasible or worth the effort to be completely honest.
What do you think? I do not particularly have any issues with that. I 
feel like this is a niche case (at least now) anyway.
> 
> Cheers,
> 
> --
> Julien Grall
> 
>

Regards
Amneesh
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Julien Grall 3 weeks, 4 days ago
Hi Amneesh,

Sorry for the late answer.

On 06/08/2024 12:50, Amneesh Singh wrote:
>> need to specify the clockspeed on the command line. I think we should
>> investigate other approaches such as implementing partially
>> clk_get_rate() (if this is how Linux manage to retrieve the clock speed
>> without any command line option).
> I guess, we can just ditch the entire idea, and just use the a default
> fallback and just print a message informing the user that there is no
> frequency in the DT. I do not think implementing clk_get_rate (or any
> clock API) is feasible or worth the effort to be completely honest.
> What do you think? I do not particularly have any issues with that. I
> feel like this is a niche case (at least now) anyway.

Michal, Stefano and I had a chat. We agree that implementing 
clk_get_rate() may be too complex. So the best next solution is to ask 
the user to update the DT and read the property from Xen.

Cheers,

-- 
Julien Grall
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Michal Orzel 1 month ago
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.

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.

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

> +
> +    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?

> +    char *value, *start = options;
Scope of value could be limited to the while loop

> +
> +    if ( !strcmp(config, "") )
> +        return;
> +
> +    strlcpy(options, config, ARRAY_SIZE(options));
> +
> +    while (start != NULL)
White spaces missing. Refer CODING_STYLE.

> +    {
> +        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.

> +
> +        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

> +    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.

> +        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
Re: [EXTERNAL] Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Amneesh Singh 1 month ago
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@ti.com>
> > ---
> >  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
Re: [PATCH] drivers: char: omap-uart: add "clock-hz" option
Posted by Amneesh Singh 1 month ago
Was not using my usual mailing client, hence the abomination on the web
view of the mailing list. Pardon the subject too.

Apologies
Amneesh