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

[Amneesh Singh <a-singh21@xxxxxx>]

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