Re: [PATCH v2 2/2] serial: 8250_of: add support for an optional bus clock

[Yixun Lan <dlan@gentoo.org>]

Hi Alex,

On 17:47 Wed 09 Apr     , Alex Elder wrote:
> On 4/9/25 4:43 PM, Yixun Lan wrote:
> > Hi Alex,
> > 
> > On 14:22 Wed 09 Apr     , Alex Elder wrote:
> >> The SpacemiT UART requires a bus clock to be enabled, in addition to
> >> it's "normal" core clock.  Look up the optional bus clock by name,
> >> and if that's found, look up the core clock using the name "core".
> >>
> >> Supplying a bus clock is optional.  If no bus clock is needed, the
> >> the first/only clock is used for the core clock.
> >>
> >> Signed-off-by: Alex Elder <elder@riscstar.com>
> >> ---
> >> v2: Update logic to more check for the optional bus clock first
> >>
> >>   drivers/tty/serial/8250/8250_of.c | 11 ++++++++++-
> >>   1 file changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
> >> index 11c860ea80f60..a90a5462aa72a 100644
> >> --- a/drivers/tty/serial/8250/8250_of.c
> >> +++ b/drivers/tty/serial/8250/8250_of.c
> >> @@ -123,7 +123,16 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
> >>   
> >>   	/* Get clk rate through clk driver if present */
> >>   	if (!port->uartclk) {
> >> -		info->clk = devm_clk_get_enabled(dev, NULL);
> >> +		struct clk *bus_clk;
> > we also need to handle clk in suspend/resume procedure, so
> > I think you need to put bus_clk inside struct of_serial_info..
> 
> OK, I didn't do anything for that in previous versions of the
> series.
> 
> I think that means we'd call clk_disable_unprepare() on
> the bus clock after doing so for the function clock.  And
> clk_prepare_enable() on the bus clock before doing that for
> the function clock in of_serial_resume().  That's easy.
> 
right, pretty much similar to info->clk

> Is there anything further you think is required?  There is
> no clock rate associated with the bus clock that I know of,
> so even if the function clock rate changes, the bus clock
> can remain as-is.
> 
no further info I know of, my best guess, the rate doesn't
really matter, a wide clk range should just work fine.

> > 
> >> +
> >> +		bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> > for the 'optional', we can interpret it's optional for other vendor
> > UART, but a must required clk for SpacemiT's k1 UART controller
> > 
> > would it better to guard this inside a compatible test or even introduce
> > a flag in compatible data?
> 
> I don't personally think so. We could, but the DT binding is going
> out of its way to define when the bus clock is required.  This is
> simpler, and generic.
> 
I would personally choose the way of introducing a flag of compatible data,
but it's a lot change of the code (may not worth the effort)..

anyway, I'm fine with your current version, and yes, it's generic
thanks for doing this..

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55