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

[Amneesh Singh <a-singh21@xxxxxx>]

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