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

[Alex Elder]

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@xxxxxxxxxxxx>
> > ---
> > 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.

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.

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

					-Alex

> 	if (of_device_is_compatible(pdev->dev.of_node, "spacemit,k1-uart")) {
> 		bus_clk = devm_clk_get_enabled(dev, "bus");
> 		..
> 	}
>
> }
> > +		if (IS_ERR(bus_clk)) {
> > +			ret = dev_err_probe(dev, PTR_ERR(bus_clk), "failed to get bus clock\n");
> > +			goto err_pmruntime;
> > +		}
> > +
> > +		/* If the bus clock is required, core clock must be named */
> > +		info->clk = devm_clk_get_enabled(dev, bus_clk ? "core" : NULL);
> >   		if (IS_ERR(info->clk)) {
> >   			ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
> >   			goto err_pmruntime;
> > --
> > 2.45.2