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

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

					-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      |  6 ++++-
 drivers/tty/serial/8250/8250_of.c             | 25 +++++++++++++++++--
 2 files changed, 28 insertions(+), 3 deletions(-)

-- 
2.45.2

[PATCH 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".

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 Documentation/devicetree/bindings/serial/8250.yaml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index dc0d52920575f..1adf935b7f36f 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -135,7 +135,11 @@ properties:
   clock-frequency: true
 
   clocks:
-    maxItems: 1
+    oneOf:
+      - maxItems: 1
+      - items:
+          - const: core
+          - const: bus
 
   resets:
     maxItems: 1
-- 
2.45.2

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

[Rob Herring]

On Tue, Apr 8, 2025 at 12:51 PM Alex Elder <elder@riscstar.com> 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".

This should be conditional on the compatible associated with 2 clocks.

>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
>  Documentation/devicetree/bindings/serial/8250.yaml | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index dc0d52920575f..1adf935b7f36f 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -135,7 +135,11 @@ properties:
>    clock-frequency: true
>
>    clocks:
> -    maxItems: 1
> +    oneOf:
> +      - maxItems: 1
> +      - items:
> +          - const: core
> +          - const: bus

'clocks' does not take string values. You want:

minItems: 1
items:
  - description: ...
  - description: ...

Then an if/then schema with 'minItems: 2' for spacemit and 'maxItems:
1' for everyone else.

[PATCH 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 core clock by name, and if
that's found, look up the bus clock.  If named clocks are used, both
are required.

Supplying a bus clock is optional.  If no bus clock is needed, the clocks
aren't named and we only look up the first clock.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/tty/serial/8250/8250_of.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 11c860ea80f60..0ffb9f2727b92 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,14 +124,34 @@ 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);
+		info->clk = devm_clk_get_optional_enabled(dev, "core");
 		if (IS_ERR(info->clk)) {
-			ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
+			ret = dev_err_probe(dev, PTR_ERR(info->clk),
+					    "failed to get core clock\n");
 			goto err_pmruntime;
 		}
 
+		/* If we got the core clock, look for a bus clock */
+		if (info->clk) {
+			info->bus_clk = devm_clk_get_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;
+			}
+		} else {
+			/* Fall back to getting the one unnamed clock */
+			info->clk = devm_clk_get_enabled(dev, NULL);
+			if (IS_ERR(info->clk)) {
+				ret = dev_err_probe(dev, PTR_ERR(info->clk),
+						"failed to get clock\n");
+				goto err_pmruntime;
+			}
+		}
+
 		port->uartclk = clk_get_rate(info->clk);
 	}
+
 	/* If current-speed was set, then try not to change it. */
 	if (of_property_read_u32(np, "current-speed", &spd) == 0)
 		port->custom_divisor = port->uartclk / (16 * spd);
-- 
2.45.2

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

[Andy Shevchenko]

On Tue, Apr 08, 2025 at 12:51:43PM -0500, Alex Elder wrote:
> The SpacemiT UART requires a bus clock to be enabled, in addition to
> it's "normal" core clock.  Look up the core clock by name, and if
> that's found, look up the bus clock.  If named clocks are used, both
> are required.
> 
> Supplying a bus clock is optional.  If no bus clock is needed, the clocks
> aren't named and we only look up the first clock.

Code and description are not aligned. And What you are described above make
more sense to me than what's done below.

Also can we simply not not touch this conditional at all, but just add
a new one before? Like

	/* Get clk rate through clk driver if present */

	/* Try named clock first */
	if (!port->uartclk) {
		...
	}

	/* Try unnamed core clock */
// the below is just an existing code.
	if (!port->uartclk) {
		...
	}

...

>  	/* Get clk rate through clk driver if present */
>  	if (!port->uartclk) {
> -		info->clk = devm_clk_get_enabled(dev, NULL);
> +		info->clk = devm_clk_get_optional_enabled(dev, "core");
>  		if (IS_ERR(info->clk)) {
> -			ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
> +			ret = dev_err_probe(dev, PTR_ERR(info->clk),
> +					    "failed to get core clock\n");

Can be still one line.

>  			goto err_pmruntime;
>  		}
>  
> +		/* If we got the core clock, look for a bus clock */
> +		if (info->clk) {
> +			info->bus_clk = devm_clk_get_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");

Something with indentation happened here and below.

> +				goto err_pmruntime;
> +			}
> +		} else {
> +			/* Fall back to getting the one unnamed clock */
> +			info->clk = devm_clk_get_enabled(dev, NULL);
> +			if (IS_ERR(info->clk)) {
> +				ret = dev_err_probe(dev, PTR_ERR(info->clk),
> +						"failed to get clock\n");
> +				goto err_pmruntime;
> +			}
> +		}
> +
>  		port->uartclk = clk_get_rate(info->clk);
>  	}

> +

Stray change.

>  	/* If current-speed was set, then try not to change it. */
>  	if (of_property_read_u32(np, "current-speed", &spd) == 0)
>  		port->custom_divisor = port->uartclk / (16 * spd);

-- 
With Best Regards,
Andy Shevchenko

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

[Alex Elder]

On 4/8/25 2:52 PM, Andy Shevchenko wrote:
> On Tue, Apr 08, 2025 at 12:51:43PM -0500, Alex Elder wrote:
>> The SpacemiT UART requires a bus clock to be enabled, in addition to
>> it's "normal" core clock.  Look up the core clock by name, and if
>> that's found, look up the bus clock.  If named clocks are used, both
>> are required.
>>
>> Supplying a bus clock is optional.  If no bus clock is needed, the clocks
>> aren't named and we only look up the first clock.
> 
> Code and description are not aligned. And What you are described above make
> more sense to me than what's done below.

I want to do this the right way.

My explanation says:
- look up the core clock by name
     - if that is found, look up the bus clock
     - both clocks are required in this case (error otherwise)
- If the "core" clock is not found:
     - look up the first clock.

And my code does:
- look up the core clock by name (not found is OK)
     - if it is found, look up the bus clock by name
     - If that is not found or error, it's an error
- if the "core" clock is not found
     - look up the first clock

What is not aligned?


> Also can we simply not not touch this conditional at all, but just add
> a new one before? Like
> 
> 	/* Get clk rate through clk driver if present */
> 
> 	/* Try named clock first */
> 	if (!port->uartclk) {
> 		...
> 	}
> 
> 	/* Try unnamed core clock */
> // the below is just an existing code.

That's easy enough.  I think it might be a little more code
but I have no problem with that.

> 	if (!port->uartclk) {
> 		...
> 	}
> 
> ...
> 
>>   	/* Get clk rate through clk driver if present */
>>   	if (!port->uartclk) {
>> -		info->clk = devm_clk_get_enabled(dev, NULL);
>> +		info->clk = devm_clk_get_optional_enabled(dev, "core");
>>   		if (IS_ERR(info->clk)) {
>> -			ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n");
>> +			ret = dev_err_probe(dev, PTR_ERR(info->clk),
>> +					    "failed to get core clock\n");
> 
> Can be still one line.
> 
>>   			goto err_pmruntime;
>>   		}
>>   
>> +		/* If we got the core clock, look for a bus clock */
>> +		if (info->clk) {
>> +			info->bus_clk = devm_clk_get_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");
> 
> Something with indentation happened here and below.
> 
>> +				goto err_pmruntime;
>> +			}
>> +		} else {
>> +			/* Fall back to getting the one unnamed clock */
>> +			info->clk = devm_clk_get_enabled(dev, NULL);
>> +			if (IS_ERR(info->clk)) {
>> +				ret = dev_err_probe(dev, PTR_ERR(info->clk),
>> +						"failed to get clock\n");
>> +				goto err_pmruntime;
>> +			}
>> +		}
>> +
>>   		port->uartclk = clk_get_rate(info->clk);
>>   	}
> 
>> +
> 
> Stray change.

Sorry, I didn't notice that.  Next time it won't be there.

Thanks for your (quick) review.

					-Alex


>>   	/* If current-speed was set, then try not to change it. */
>>   	if (of_property_read_u32(np, "current-speed", &spd) == 0)
>>   		port->custom_divisor = port->uartclk / (16 * spd);
> 

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

[Andy Shevchenko]

On Tue, Apr 08, 2025 at 03:11:10PM -0500, Alex Elder wrote:
> On 4/8/25 2:52 PM, Andy Shevchenko wrote:
> > On Tue, Apr 08, 2025 at 12:51:43PM -0500, Alex Elder wrote:

> > > The SpacemiT UART requires a bus clock to be enabled, in addition to
> > > it's "normal" core clock.  Look up the core clock by name, and if
> > > that's found, look up the bus clock.  If named clocks are used, both
> > > are required.
> > > 
> > > Supplying a bus clock is optional.  If no bus clock is needed, the clocks
> > > aren't named and we only look up the first clock.
> > 
> > Code and description are not aligned. And What you are described above make
> > more sense to me than what's done below.
> 
> I want to do this the right way.
> 
> My explanation says:
> - look up the core clock by name
>     - if that is found, look up the bus clock
>     - both clocks are required in this case (error otherwise)
> - If the "core" clock is not found:
>     - look up the first clock.
> 
> And my code does:
> - look up the core clock by name (not found is OK)
>     - if it is found, look up the bus clock by name
>     - If that is not found or error, it's an error
> - if the "core" clock is not found
>     - look up the first clock
> 
> What is not aligned?

That you are telling that bus is optional and core is not, the code does the
opposite (and yes, I understand the logic behind, but why not doing the same in
the code, i.e. check for the *optional* bus clock first?

> > Also can we simply not not touch this conditional at all, but just add
> > a new one before? Like
> > 
> > 	/* Get clk rate through clk driver if present */
> > 
> > 	/* Try named clock first */
> > 	if (!port->uartclk) {
> > 		...
> > 	}
> > 
> > 	/* Try unnamed core clock */
> > // the below is just an existing code.
> 
> That's easy enough.  I think it might be a little more code
> but I have no problem with that.

I;m fine with a little more code, but it will be much cleaner in my point of
view and as a bonus the diff will be easier to review.

> > 	if (!port->uartclk) {
> > 		...
> > 	}

-- 
With Best Regards,
Andy Shevchenko

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

[Alex Elder]

On 4/9/25 2:29 AM, Andy Shevchenko wrote:
> On Tue, Apr 08, 2025 at 03:11:10PM -0500, Alex Elder wrote:
>> On 4/8/25 2:52 PM, Andy Shevchenko wrote:
>>> On Tue, Apr 08, 2025 at 12:51:43PM -0500, Alex Elder wrote:
> 
>>>> The SpacemiT UART requires a bus clock to be enabled, in addition to
>>>> it's "normal" core clock.  Look up the core clock by name, and if
>>>> that's found, look up the bus clock.  If named clocks are used, both
>>>> are required.
>>>>
>>>> Supplying a bus clock is optional.  If no bus clock is needed, the clocks
>>>> aren't named and we only look up the first clock.
>>>
>>> Code and description are not aligned. And What you are described above make
>>> more sense to me than what's done below.
>>
>> I want to do this the right way.
>>
>> My explanation says:
>> - look up the core clock by name
>>      - if that is found, look up the bus clock
>>      - both clocks are required in this case (error otherwise)
>> - If the "core" clock is not found:
>>      - look up the first clock.
>>
>> And my code does:
>> - look up the core clock by name (not found is OK)
>>      - if it is found, look up the bus clock by name
>>      - If that is not found or error, it's an error
>> - if the "core" clock is not found
>>      - look up the first clock
>>
>> What is not aligned?
> 
> That you are telling that bus is optional and core is not, the code does the
> opposite (and yes, I understand the logic behind, but why not doing the same in
> the code, i.e. check for the *optional* bus clock first?

Ahh, now I see what you mean.  The result will be the same,
but if it "reads better" that way to you then I'm all for it.

One of the reasons I did it this way was that I wasn't sure
how to express a "don't care" clock name as a DTS binding,
so I just tried to avoid that.

In other words, I thought about adding the "bus" clock as an
optional first lookup, and then leaving the existing code to
look up the core clock by position--without caring about the
name.  But I assume named clocks aren't guaranteed to be in
any particular order ("core" clock *could* be listed second).

So I look up the "core" clock by (optional) name, and if not
found look it up by position.  If it is found, I look up the
bus clock--required in this case.

Now that I write that I understand why you felt the logic was
a bit inverted.

I'll send v2 today and will rearrange the logic to match what
you're talking about.

>>> Also can we simply not not touch this conditional at all, but just add
>>> a new one before? Like
>>>
>>> 	/* Get clk rate through clk driver if present */
>>>
>>> 	/* Try named clock first */
>>> 	if (!port->uartclk) {
>>> 		...
>>> 	}
>>>
>>> 	/* Try unnamed core clock */
>>> // the below is just an existing code.
>>
>> That's easy enough.  I think it might be a little more code
>> but I have no problem with that.
> 
> I;m fine with a little more code, but it will be much cleaner in my point of
> view and as a bonus the diff will be easier to review.

I understand that completely.  Thanks a lot for clarifying.

					-Alex

>>> 	if (!port->uartclk) {
>>> 		...
>>> 	}
>