[PATCH v2 0/2] serial: 8250_of: support an optional bus clock

[PATCH v2 0/2] serial: 8250_of: support an optional bus clock

[Alex Elder]

The SpacemiT UART hardware requires a bus clock to be enabled in addition
to the primary function clock.

This series makes it possible to specify both clocks via DTS.  If a
bus clock is required, it and the primary clock are fetched by name.

Since the first version, the DT binding (which had errors) was updated
to properly define the number of clocks and clock names, dependent on
the compatible string as suggested by Rob Herring.

The logic that looks up the optional bus clock has also been changed, as
requested by Andy Shevchenko.  In addition, the bus clock pointer (which
was never used after it was enabled) is no longer stored in the
of_serial_info structure.

Here is the first version of this series:
  https://lore.kernel.org/lkml/20250408175146.979557-1-elder@riscstar.com/

					-Alex

Alex Elder (2):
  dt-bindings: serial: 8250: support an optional second clock
  serial: 8250_of: add support for an optional bus clock

 .../devicetree/bindings/serial/8250.yaml      | 30 ++++++++++++++++++-
 drivers/tty/serial/8250/8250_of.c             | 11 ++++++-
 2 files changed, 39 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH v2 1/2] dt-bindings: serial: 8250: support an optional second clock

[Alex Elder]

The SpacemiT UART driver requires a bus clock to be enabled in addition
to the primary function clock.  Add the option to specify two clocks
for an 8250-compatible UART, named "core" and "bus".  If both are needed,
require them to be named.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v2: Require both clocks to be specified with names for SpacemiT

 .../devicetree/bindings/serial/8250.yaml      | 30 ++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index dc0d52920575f..33d2016b65090 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -135,7 +135,16 @@ properties:
   clock-frequency: true
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: The core function clock
+      - description: An optional bus clock
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: core
+      - const: bus
 
   resets:
     maxItems: 1
@@ -224,6 +233,25 @@ required:
   - reg
   - interrupts
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: spacemit,k1-uart
+then:
+  required: [clock-names]
+  properties:
+    clocks:
+      minItems: 2
+    clock-names:
+      minItems: 2
+else:
+  properties:
+    clocks:
+      maxItems: 1
+    clock-names:
+      maxItems: 1
+
 unevaluatedProperties: false
 
 examples:
-- 
2.45.2

Re: [PATCH v2 1/2] dt-bindings: serial: 8250: support an optional second clock

[Rob Herring (Arm)]

On Wed, 09 Apr 2025 14:22:11 -0500, Alex Elder wrote:
> The SpacemiT UART driver requires a bus clock to be enabled in addition
> to the primary function clock.  Add the option to specify two clocks
> for an 8250-compatible UART, named "core" and "bus".  If both are needed,
> require them to be named.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v2: Require both clocks to be specified with names for SpacemiT
> 
>  .../devicetree/bindings/serial/8250.yaml      | 30 ++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

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

[Alex Elder]

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;
+
+		bus_clk = devm_clk_get_optional_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

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

[Yixun Lan]

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

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

	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
> 

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

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

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

[Yixun Lan]

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