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.2Fri, 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
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;
>
>
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
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;
>
>
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
© 2016 - 2025 Red Hat, Inc.