Messages in this thread |  | | Date | Wed, 9 Apr 2025 17:47:30 -0500 | | Subject | Re: [PATCH v2 2/2] serial: 8250_of: add support for an optional bus clock | | From | 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@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.
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 >> >
|  |