| Message ID | 20250408175146.979557-3-elder@riscstar.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: 8250_of: support an optional bus clock | expand |
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);
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); >
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) { > > ... > > }
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) { >>> ... >>> } >
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);
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(-)