| Message ID | 20240719113313.145587-1-a-singh21@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drivers: char: omap-uart: add "clock-hz" option | expand |
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
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
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,
Was not using my usual mailing client, hence the abomination on the web view of the mailing list. Pardon the subject too. Apologies Amneesh
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
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,
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
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,
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
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,
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 ) {
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(-)