[PATCH v3 0/3] serial: 8250_of: support an optional bus clock

[PATCH v3 0/3] 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.

Changes between v2 and v3:
  - A third patch was added to disable the bus clock on suspend and
    enable it again on resume, as requested by Yixun Lan
  - Rob's Reviewed-by tag has been added to patch 1
  - The first two patches are otherwise identical to what was in v2

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

Changes between v1 and v2:
  - The DT binding was fixed to properly define the number of clocks and
    clock names based on the compatible string, as suggested by Rob Herring
  - Logic to look up the optional bus clock was changed as requested
    by Andy Shevchenko
  - The bus clock pointer (which was never used after it was enabled)
    was renmoved from 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 (3):
  dt-bindings: serial: 8250: support an optional second clock
  serial: 8250_of: add support for an optional bus clock
  serial: 8250_of: manage bus clock in suspend/resume

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

base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
-- 
2.45.2

[PATCH v3 1/3] 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>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
v3: Added Rob's Reviewed-by tag

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

[PATCH v3 2/3] 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>
---
 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 v3 2/3] serial: 8250_of: add support for an optional bus clock

[Andy Shevchenko]

Fri, Apr 11, 2025 at 10:44:17AM -0500, Alex Elder kirjoitti:
> 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.

Okay, it's slightly different to what we discussed, but at least it looks much
better than previous version and acceptable in my point of view.gq

Reviewed-by: Andy Shevchenko <andy@kernel.org>

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

[Alex Elder]

Save the bus clock pointer in the of_serial_info structure, and use
that to disable the bus clock on suspend and re-enable it on resume.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v3: New patch, managing the bus clock during suspend and resume

 drivers/tty/serial/8250/8250_of.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index a90a5462aa72a..8f2529f699e0a 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -24,6 +24,7 @@
 
 struct of_serial_info {
 	struct clk *clk;
+	struct clk *bus_clk;
 	struct reset_control *rst;
 	int type;
 	int line;
@@ -123,16 +124,15 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 
 	/* Get clk rate through clk driver if present */
 	if (!port->uartclk) {
-		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");
+		info->bus_clk = devm_clk_get_optional_enabled(dev, "bus");
+		if (IS_ERR(info->bus_clk)) {
+			ret = dev_err_probe(dev, PTR_ERR(info->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);
+		info->clk = devm_clk_get_enabled(dev, info->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;
@@ -299,6 +299,7 @@ static int of_serial_suspend(struct device *dev)
 	if (!uart_console(port) || console_suspend_enabled) {
 		pm_runtime_put_sync(dev);
 		clk_disable_unprepare(info->clk);
+		clk_disable_unprepare(info->bus_clk);
 	}
 	return 0;
 }
@@ -311,6 +312,7 @@ static int of_serial_resume(struct device *dev)
 
 	if (!uart_console(port) || console_suspend_enabled) {
 		pm_runtime_get_sync(dev);
+		clk_prepare_enable(info->bus_clk);
 		clk_prepare_enable(info->clk);
 	}
 
-- 
2.45.2

Re: [PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

[Andy Shevchenko]

Fri, Apr 11, 2025 at 10:44:18AM -0500, Alex Elder kirjoitti:
> Save the bus clock pointer in the of_serial_info structure, and use
> that to disable the bus clock on suspend and re-enable it on resume.

...

>  	if (!port->uartclk) {
> -		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");
> +		info->bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> +		if (IS_ERR(info->bus_clk)) {
> +			ret = dev_err_probe(dev, PTR_ERR(info->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);
> +		info->clk = devm_clk_get_enabled(dev, info->bus_clk ? "core" : NULL);
>  		if (IS_ERR(info->clk)) {
>  			ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");

While the first patch against this file looks okay now, this one inherits the
same problem (seems like not enought thinking about the code representation).

Instead of rewritting half of the lines you just introduced (which is also a
bad practice), add a one-liner that assigns a field to the local variable.

>  			goto err_pmruntime;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

[Alex Elder]

On 4/11/25 2:30 PM, Andy Shevchenko wrote:
> Fri, Apr 11, 2025 at 10:44:18AM -0500, Alex Elder kirjoitti:
>> Save the bus clock pointer in the of_serial_info structure, and use
>> that to disable the bus clock on suspend and re-enable it on resume.
> 
> ...
> 
>>   	if (!port->uartclk) {
>> -		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");
>> +		info->bus_clk = devm_clk_get_optional_enabled(dev, "bus");
>> +		if (IS_ERR(info->bus_clk)) {
>> +			ret = dev_err_probe(dev, PTR_ERR(info->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);
>> +		info->clk = devm_clk_get_enabled(dev, info->bus_clk ? "core" : NULL);
>>   		if (IS_ERR(info->clk)) {
>>   			ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
> 
> While the first patch against this file looks okay now, this one inherits the
> same problem (seems like not enought thinking about the code representation).
> 
> Instead of rewritting half of the lines you just introduced (which is also a
> bad practice), add a one-liner that assigns a field to the local variable.

So you want me to re-spin this again so that I use the local variable?

I understand what you're saying based on ease of review, but this
is a simple patch and the change is very understandable, and the
code is no more or less clear when using the local variable.

					-Alex


> 
>>   			goto err_pmruntime;
> 
> 

Re: [PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

[Andy Shevchenko]

On Fri, Apr 11, 2025 at 10:36 PM Alex Elder <elder@riscstar.com> wrote:
> On 4/11/25 2:30 PM, Andy Shevchenko wrote:
> > Fri, Apr 11, 2025 at 10:44:18AM -0500, Alex Elder kirjoitti:
> >> Save the bus clock pointer in the of_serial_info structure, and use
> >> that to disable the bus clock on suspend and re-enable it on resume.

...

> >>      if (!port->uartclk) {
> >> -            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");
> >> +            info->bus_clk = devm_clk_get_optional_enabled(dev, "bus");
> >> +            if (IS_ERR(info->bus_clk)) {
> >> +                    ret = dev_err_probe(dev, PTR_ERR(info->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);
> >> +            info->clk = devm_clk_get_enabled(dev, info->bus_clk ? "core" : NULL);
> >>              if (IS_ERR(info->clk)) {
> >>                      ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
> >
> > While the first patch against this file looks okay now, this one inherits the
> > same problem (seems like not enought thinking about the code representation).
> >
> > Instead of rewritting half of the lines you just introduced (which is also a
> > bad practice), add a one-liner that assigns a field to the local variable.
>
> So you want me to re-spin this again so that I use the local variable?

Yes.

> I understand what you're saying based on ease of review,

No, not only review. Here the main issue is ping-pong between patches.
If you know ahead that these lines should be changed, do it from the
start. But I understand the need to have separate changes for
logically different pieces. That's why

> but this
> is a simple patch

It doesn't matter how simple it is.

> and the change is very understandable, and the
> code is no more or less clear when using the local variable.

See above.

> >>                      goto err_pmruntime;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

[Alex Elder]

On 4/11/25 2:44 PM, Andy Shevchenko wrote:
> On Fri, Apr 11, 2025 at 10:36 PM Alex Elder <elder@riscstar.com> wrote:
>> On 4/11/25 2:30 PM, Andy Shevchenko wrote:
>>> Fri, Apr 11, 2025 at 10:44:18AM -0500, Alex Elder kirjoitti:
>>>> Save the bus clock pointer in the of_serial_info structure, and use
>>>> that to disable the bus clock on suspend and re-enable it on resume.
> 
> ...
> 
>>>>       if (!port->uartclk) {
>>>> -            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");
>>>> +            info->bus_clk = devm_clk_get_optional_enabled(dev, "bus");
>>>> +            if (IS_ERR(info->bus_clk)) {
>>>> +                    ret = dev_err_probe(dev, PTR_ERR(info->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);
>>>> +            info->clk = devm_clk_get_enabled(dev, info->bus_clk ? "core" : NULL);
>>>>               if (IS_ERR(info->clk)) {
>>>>                       ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
>>>
>>> While the first patch against this file looks okay now, this one inherits the
>>> same problem (seems like not enought thinking about the code representation).
>>>
>>> Instead of rewritting half of the lines you just introduced (which is also a
>>> bad practice), add a one-liner that assigns a field to the local variable.
>>
>> So you want me to re-spin this again so that I use the local variable?
> 
> Yes.
> 
>> I understand what you're saying based on ease of review,
> 
> No, not only review. Here the main issue is ping-pong between patches.
> If you know ahead that these lines should be changed, do it from the
> start. But I understand the need to have separate changes for
> logically different pieces. That's why

I did do it from the start (v1 of the series).  Then I reworked
it based on your feedback and concluded there was no need for
keeping a copy of the bus clock in the of_serial_info structure.
In the interest of keeping the code simple I dropped it.

Yixun asked me to disable/enable the bus clock in suspend/resume,
so I was preparing the patches for that, when Greg informed me
he had already accepted the first two patches.  So I added this
new feature on the end.  This is the reason for this "ping pong."
I would have done it from the start in this series otherwise in v3.

Anyway, I'm sending v4 shortly.  I just have to run it through
another test.  It is indeed a much simpler change.

					-Alex

>> but this
>> is a simple patch
> 
> It doesn't matter how simple it is.
> 
>> and the change is very understandable, and the
>> code is no more or less clear when using the local variable.
> 
> See above.
> 
>>>>                       goto err_pmruntime;
> 
> 

Re: [PATCH v3 3/3] serial: 8250_of: manage bus clock in suspend/resume

[Andy Shevchenko]

On Fri, Apr 11, 2025 at 10:56 PM Alex Elder <elder@riscstar.com> wrote:
> On 4/11/25 2:44 PM, Andy Shevchenko wrote:

...

> when Greg informed me
> he had already accepted the first two patches.

Indeed. Just send a last patch in a simplified form as I suggested here.

-- 
With Best Regards,
Andy Shevchenko