[PATCH v3] serial: 8250_of: Add basic PM runtime support

[PATCH v3] serial: 8250_of: Add basic PM runtime support

[Franklin S Cooper Jr]

66AK2G UART instances are not apart of the ALWAYS_ON power domain.
Therefore, pm_runtime calls must be made to properly insure the appropriate
power domains needed by UART are on. Keep legacy clk api calls since other
users of this driver may not support PM runtime.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
Version 3 changes:
Commit message updated
Drop extra parenthesis

Version 2 changes:
Fix build error
Build tested using allmodconfig

 drivers/tty/serial/8250/8250_of.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 6c5a8ca..be90c16 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -18,6 +18,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/reset.h>
 
@@ -65,6 +66,10 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 	int ret;
 
 	memset(port, 0, sizeof *port);
+
+	pm_runtime_enable(&ofdev->dev);
+	pm_runtime_get_sync(&ofdev->dev);
+
 	if (of_property_read_u32(np, "clock-frequency", &clk)) {
 
 		/* Get clk rate through clk driver if present */
@@ -72,12 +77,13 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 		if (IS_ERR(info->clk)) {
 			dev_warn(&ofdev->dev,
 				"clk or clock-frequency not defined\n");
-			return PTR_ERR(info->clk);
+			ret = PTR_ERR(info->clk);
+			goto err_pmruntime;
 		}
 
 		ret = clk_prepare_enable(info->clk);
 		if (ret < 0)
-			return ret;
+			goto err_pmruntime;
 
 		clk = clk_get_rate(info->clk);
 	}
@@ -170,8 +176,10 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 err_dispose:
 	irq_dispose_mapping(port->irq);
 err_unprepare:
-	if (info->clk)
-		clk_disable_unprepare(info->clk);
+	clk_disable_unprepare(info->clk);
+err_pmruntime:
+	pm_runtime_put_sync(&ofdev->dev);
+	pm_runtime_disable(&ofdev->dev);
 	return ret;
 }
 
@@ -227,8 +235,9 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
 	return 0;
 err_dispose:
 	irq_dispose_mapping(port8250.port.irq);
-	if (info->clk)
-		clk_disable_unprepare(info->clk);
+	pm_runtime_put_sync(&ofdev->dev);
+	pm_runtime_disable(&ofdev->dev);
+	clk_disable_unprepare(info->clk);
 err_free:
 	kfree(info);
 	return ret;
@@ -244,8 +253,9 @@ static int of_platform_serial_remove(struct platform_device *ofdev)
 	serial8250_unregister_port(info->line);
 
 	reset_control_assert(info->rst);
-	if (info->clk)
-		clk_disable_unprepare(info->clk);
+	pm_runtime_put_sync(&ofdev->dev);
+	pm_runtime_disable(&ofdev->dev);
+	clk_disable_unprepare(info->clk);
 	kfree(info);
 	return 0;
 }
@@ -259,9 +269,10 @@ static int of_serial_suspend(struct device *dev)
 
 	serial8250_suspend_port(info->line);
 
-	if (info->clk && (!uart_console(port) || console_suspend_enabled))
+	if (!uart_console(port) || console_suspend_enabled) {
+		pm_runtime_put_sync(dev);
 		clk_disable_unprepare(info->clk);
-
+	}
 	return 0;
 }
 
@@ -271,8 +282,10 @@ static int of_serial_resume(struct device *dev)
 	struct uart_8250_port *port8250 = serial8250_get_port(info->line);
 	struct uart_port *port = &port8250->port;
 
-	if (info->clk && (!uart_console(port) || console_suspend_enabled))
+	if (!uart_console(port) || console_suspend_enabled) {
+		pm_runtime_get_sync(dev);
 		clk_prepare_enable(info->clk);
+	}
 
 	serial8250_resume_port(info->line);
 
-- 
2.9.4.dirty

[PATCH v3] serial: 8250_of: Add basic PM runtime support

[Franklin S Cooper Jr]

66AK2G UART instances are not apart of the ALWAYS_ON power domain.
Therefore, pm_runtime calls must be made to properly insure the appropriate
power domains needed by UART are on. Keep legacy clk api calls since other
users of this driver may not support PM runtime.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
Version 3 changes:
Commit message updated
Drop extra parenthesis

Version 2 changes:
Fix build error
Build tested using allmodconfig

 drivers/tty/serial/8250/8250_of.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
index 6c5a8ca..be90c16 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -18,6 +18,7 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/reset.h>
 
@@ -65,6 +66,10 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 	int ret;
 
 	memset(port, 0, sizeof *port);
+
+	pm_runtime_enable(&ofdev->dev);
+	pm_runtime_get_sync(&ofdev->dev);
+
 	if (of_property_read_u32(np, "clock-frequency", &clk)) {
 
 		/* Get clk rate through clk driver if present */
@@ -72,12 +77,13 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 		if (IS_ERR(info->clk)) {
 			dev_warn(&ofdev->dev,
 				"clk or clock-frequency not defined\n");
-			return PTR_ERR(info->clk);
+			ret = PTR_ERR(info->clk);
+			goto err_pmruntime;
 		}
 
 		ret = clk_prepare_enable(info->clk);
 		if (ret < 0)
-			return ret;
+			goto err_pmruntime;
 
 		clk = clk_get_rate(info->clk);
 	}
@@ -170,8 +176,10 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
 err_dispose:
 	irq_dispose_mapping(port->irq);
 err_unprepare:
-	if (info->clk)
-		clk_disable_unprepare(info->clk);
+	clk_disable_unprepare(info->clk);
+err_pmruntime:
+	pm_runtime_put_sync(&ofdev->dev);
+	pm_runtime_disable(&ofdev->dev);
 	return ret;
 }
 
@@ -227,8 +235,9 @@ static int of_platform_serial_probe(struct platform_device *ofdev)
 	return 0;
 err_dispose:
 	irq_dispose_mapping(port8250.port.irq);
-	if (info->clk)
-		clk_disable_unprepare(info->clk);
+	pm_runtime_put_sync(&ofdev->dev);
+	pm_runtime_disable(&ofdev->dev);
+	clk_disable_unprepare(info->clk);
 err_free:
 	kfree(info);
 	return ret;
@@ -244,8 +253,9 @@ static int of_platform_serial_remove(struct platform_device *ofdev)
 	serial8250_unregister_port(info->line);
 
 	reset_control_assert(info->rst);
-	if (info->clk)
-		clk_disable_unprepare(info->clk);
+	pm_runtime_put_sync(&ofdev->dev);
+	pm_runtime_disable(&ofdev->dev);
+	clk_disable_unprepare(info->clk);
 	kfree(info);
 	return 0;
 }
@@ -259,9 +269,10 @@ static int of_serial_suspend(struct device *dev)
 
 	serial8250_suspend_port(info->line);
 
-	if (info->clk && (!uart_console(port) || console_suspend_enabled))
+	if (!uart_console(port) || console_suspend_enabled) {
+		pm_runtime_put_sync(dev);
 		clk_disable_unprepare(info->clk);
-
+	}
 	return 0;
 }
 
@@ -271,8 +282,10 @@ static int of_serial_resume(struct device *dev)
 	struct uart_8250_port *port8250 = serial8250_get_port(info->line);
 	struct uart_port *port = &port8250->port;
 
-	if (info->clk && (!uart_console(port) || console_suspend_enabled))
+	if (!uart_console(port) || console_suspend_enabled) {
+		pm_runtime_get_sync(dev);
 		clk_prepare_enable(info->clk);
+	}
 
 	serial8250_resume_port(info->line);
 
-- 
2.9.4.dirty

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Sekhar Nori]

On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
> Therefore, pm_runtime calls must be made to properly insure the appropriate
> power domains needed by UART are on. Keep legacy clk api calls since other
> users of this driver may not support PM runtime.

Do we really have users like that? And even if there are, cant they use
PM clock handling support available in drivers/base/power/clock_ops.c ?

The clock enable support itself was added pretty "recently" - about 5
years back with 0bbeb3c3e84b ("of serial port driver - add
clk_get_rate() support"). So I doubt any really legacy platforms relied
on clock support being there. It was added by Murali, I assume for
Keystone devices. Keystone devices can work with runtime PM using the PM
clock support pointed to above.

Perhaps linux-arm-kernel list should be copied on this submission too,
since most users of this driver are likely to be there on that list.

> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>

Thanks,
Sekhar

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Sekhar Nori]

On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
> Therefore, pm_runtime calls must be made to properly insure the appropriate
> power domains needed by UART are on. Keep legacy clk api calls since other
> users of this driver may not support PM runtime.

Do we really have users like that? And even if there are, cant they use
PM clock handling support available in drivers/base/power/clock_ops.c ?

The clock enable support itself was added pretty "recently" - about 5
years back with 0bbeb3c3e84b ("of serial port driver - add
clk_get_rate() support"). So I doubt any really legacy platforms relied
on clock support being there. It was added by Murali, I assume for
Keystone devices. Keystone devices can work with runtime PM using the PM
clock support pointed to above.

Perhaps linux-arm-kernel list should be copied on this submission too,
since most users of this driver are likely to be there on that list.

> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>

Thanks,
Sekhar

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Franklin S Cooper Jr]

On 08/21/2017 07:20 AM, Sekhar Nori wrote:
> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>> power domains needed by UART are on. Keep legacy clk api calls since other
>> users of this driver may not support PM runtime.

There are a significant amount of users across a wide variety of
architectures and boards that I have no means to properly test to insure
I'm avoiding regressions. My current approach which I've seen other
drivers in the past use when in similar situations allows things to work
without regressions.
> 
> Do we really have users like that? And even if there are, cant they use
> PM clock handling support available in drivers/base/power/clock_ops.c ?

I don't see any current defconfig that enables CONFIG_PM_CLK and only a
handful of instances where functions from clock_ops.c are actually used.
I don't quite understand what your suggestion is but in general I'm
concerned since any approach to move everyone to different apis is
rather risky especially for a critical driver.

If I'm missing your point please let me know.

> 
> The clock enable support itself was added pretty "recently" - about 5
> years back with 0bbeb3c3e84b ("of serial port driver - add
> clk_get_rate() support"). So I doubt any really legacy platforms relied
> on clock support being there. It was added by Murali, I assume for
> Keystone devices. Keystone devices can work with runtime PM using the PM
> clock support pointed to above.

You might be right but I can't be confident that it is indeed the case.
But its possible that at some point people will start having problems if
they try to use this driver for other UART instances. The currently
could not be aware of an issue because of the bootloader powering things
for them or even that different UART instances could be apart of a non
always on power domain.

> 
> Perhaps linux-arm-kernel list should be copied on this submission too,
> since most users of this driver are likely to be there on that list.
> 

Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
PowerPC, MIPS, Xtensa and Arc.

Looking at dts that enable some of the compatible it is still a
combination of quite a bit of architectures.

The current approach I've taken should be safe for all users of this
driver which. I have no issue going another approach as long as its
understood that I'm fairly limited in what I can test when you take into
account the large number of users of this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> 
> Thanks,
> Sekhar
> 

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Franklin S Cooper Jr]

On 08/21/2017 07:20 AM, Sekhar Nori wrote:
> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>> power domains needed by UART are on. Keep legacy clk api calls since other
>> users of this driver may not support PM runtime.

There are a significant amount of users across a wide variety of
architectures and boards that I have no means to properly test to insure
I'm avoiding regressions. My current approach which I've seen other
drivers in the past use when in similar situations allows things to work
without regressions.
> 
> Do we really have users like that? And even if there are, cant they use
> PM clock handling support available in drivers/base/power/clock_ops.c ?

I don't see any current defconfig that enables CONFIG_PM_CLK and only a
handful of instances where functions from clock_ops.c are actually used.
I don't quite understand what your suggestion is but in general I'm
concerned since any approach to move everyone to different apis is
rather risky especially for a critical driver.

If I'm missing your point please let me know.

> 
> The clock enable support itself was added pretty "recently" - about 5
> years back with 0bbeb3c3e84b ("of serial port driver - add
> clk_get_rate() support"). So I doubt any really legacy platforms relied
> on clock support being there. It was added by Murali, I assume for
> Keystone devices. Keystone devices can work with runtime PM using the PM
> clock support pointed to above.

You might be right but I can't be confident that it is indeed the case.
But its possible that at some point people will start having problems if
they try to use this driver for other UART instances. The currently
could not be aware of an issue because of the bootloader powering things
for them or even that different UART instances could be apart of a non
always on power domain.

> 
> Perhaps linux-arm-kernel list should be copied on this submission too,
> since most users of this driver are likely to be there on that list.
> 

Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
PowerPC, MIPS, Xtensa and Arc.

Looking at dts that enable some of the compatible it is still a
combination of quite a bit of architectures.

The current approach I've taken should be safe for all users of this
driver which. I have no issue going another approach as long as its
understood that I'm fairly limited in what I can test when you take into
account the large number of users of this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> 
> Thanks,
> Sekhar
> 

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Sekhar Nori]

On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
> 
> 
> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>> users of this driver may not support PM runtime.
> 
> There are a significant amount of users across a wide variety of
> architectures and boards that I have no means to properly test to insure
> I'm avoiding regressions. My current approach which I've seen other
> drivers in the past use when in similar situations allows things to work
> without regressions.

Since the users are all device-tree users, my hope is they can be
tracked by looking at the DT files and the maintainers be copied on this
patch to make sure nothing breaks for them. I understand its not a
trivial list, but its finite :)

>>
>> Do we really have users like that? And even if there are, cant they use
>> PM clock handling support available in drivers/base/power/clock_ops.c ?
> 
> I don't see any current defconfig that enables CONFIG_PM_CLK and only a

Thats because its a non-user-selectable def_bool. Most configs should
have it enabled after the 'make config' step.

> handful of instances where functions from clock_ops.c are actually used.
> I don't quite understand what your suggestion is but in general I'm
> concerned since any approach to move everyone to different apis is
> rather risky especially for a critical driver.
> 
> If I'm missing your point please let me know.

I do agree we dont want to break anyone. But at the same time, this
patch redundantly enables/disables clock for most known (and possible
future) users without no clear indication of who benefits from such
redundancy.

If we ignore this now, the problem will only get worse as time passes. I
suggest biting the bullet now, attempt to reach-out as many affected
parties as possible but switch to pm_runtime once and for all.

> 
>>
>> The clock enable support itself was added pretty "recently" - about 5
>> years back with 0bbeb3c3e84b ("of serial port driver - add
>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>> on clock support being there. It was added by Murali, I assume for
>> Keystone devices. Keystone devices can work with runtime PM using the PM
>> clock support pointed to above.
> 
> You might be right but I can't be confident that it is indeed the case.
> But its possible that at some point people will start having problems if
> they try to use this driver for other UART instances. The currently
> could not be aware of an issue because of the bootloader powering things
> for them or even that different UART instances could be apart of a non
> always on power domain.

I dont think you got my point there. I have no disagreement that
pm_runtime support is needed. In fact, it should have been that way when
clock enable/disable was added. But thats history now.

>> Perhaps linux-arm-kernel list should be copied on this submission too,
>> since most users of this driver are likely to be there on that list.
>>
> 
> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
> PowerPC, MIPS, Xtensa and Arc.>
> Looking at dts that enable some of the compatible it is still a
> combination of quite a bit of architectures.

Right, I do think there are quite a few users. But they should still be
reachable and provide testing inputs for the change.

> The current approach I've taken should be safe for all users of this
> driver which. I have no issue going another approach as long as its
> understood that I'm fairly limited in what I can test when you take into
> account the large number of users of this driver.

Understood that you are limited by hardware you possess. But we can
probably reach out to as many folks affected by this as possible so we
get a fair chance at doing the "right thing" without breaking anyone.

Thanks,
Sekhar

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Sekhar Nori]

On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
> 
> 
> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>> users of this driver may not support PM runtime.
> 
> There are a significant amount of users across a wide variety of
> architectures and boards that I have no means to properly test to insure
> I'm avoiding regressions. My current approach which I've seen other
> drivers in the past use when in similar situations allows things to work
> without regressions.

Since the users are all device-tree users, my hope is they can be
tracked by looking at the DT files and the maintainers be copied on this
patch to make sure nothing breaks for them. I understand its not a
trivial list, but its finite :)

>>
>> Do we really have users like that? And even if there are, cant they use
>> PM clock handling support available in drivers/base/power/clock_ops.c ?
> 
> I don't see any current defconfig that enables CONFIG_PM_CLK and only a

Thats because its a non-user-selectable def_bool. Most configs should
have it enabled after the 'make config' step.

> handful of instances where functions from clock_ops.c are actually used.
> I don't quite understand what your suggestion is but in general I'm
> concerned since any approach to move everyone to different apis is
> rather risky especially for a critical driver.
> 
> If I'm missing your point please let me know.

I do agree we dont want to break anyone. But at the same time, this
patch redundantly enables/disables clock for most known (and possible
future) users without no clear indication of who benefits from such
redundancy.

If we ignore this now, the problem will only get worse as time passes. I
suggest biting the bullet now, attempt to reach-out as many affected
parties as possible but switch to pm_runtime once and for all.

> 
>>
>> The clock enable support itself was added pretty "recently" - about 5
>> years back with 0bbeb3c3e84b ("of serial port driver - add
>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>> on clock support being there. It was added by Murali, I assume for
>> Keystone devices. Keystone devices can work with runtime PM using the PM
>> clock support pointed to above.
> 
> You might be right but I can't be confident that it is indeed the case.
> But its possible that at some point people will start having problems if
> they try to use this driver for other UART instances. The currently
> could not be aware of an issue because of the bootloader powering things
> for them or even that different UART instances could be apart of a non
> always on power domain.

I dont think you got my point there. I have no disagreement that
pm_runtime support is needed. In fact, it should have been that way when
clock enable/disable was added. But thats history now.

>> Perhaps linux-arm-kernel list should be copied on this submission too,
>> since most users of this driver are likely to be there on that list.
>>
> 
> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
> PowerPC, MIPS, Xtensa and Arc.>
> Looking at dts that enable some of the compatible it is still a
> combination of quite a bit of architectures.

Right, I do think there are quite a few users. But they should still be
reachable and provide testing inputs for the change.

> The current approach I've taken should be safe for all users of this
> driver which. I have no issue going another approach as long as its
> understood that I'm fairly limited in what I can test when you take into
> account the large number of users of this driver.

Understood that you are limited by hardware you possess. But we can
probably reach out to as many folks affected by this as possible so we
get a fair chance at doing the "right thing" without breaking anyone.

Thanks,
Sekhar

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Franklin S Cooper Jr]

On 08/23/2017 01:34 AM, Sekhar Nori wrote:
> On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
>>
>>
>> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>>> users of this driver may not support PM runtime.
>>
>> There are a significant amount of users across a wide variety of
>> architectures and boards that I have no means to properly test to insure
>> I'm avoiding regressions. My current approach which I've seen other
>> drivers in the past use when in similar situations allows things to work
>> without regressions.
> 
> Since the users are all device-tree users, my hope is they can be
> tracked by looking at the DT files and the maintainers be copied on this
> patch to make sure nothing breaks for them. I understand its not a
> trivial list, but its finite :)
> 
>>>
>>> Do we really have users like that? And even if there are, cant they use
>>> PM clock handling support available in drivers/base/power/clock_ops.c ?
>>
>> I don't see any current defconfig that enables CONFIG_PM_CLK and only a
> 
> Thats because its a non-user-selectable def_bool. Most configs should
> have it enabled after the 'make config' step.
> 
>> handful of instances where functions from clock_ops.c are actually used.
>> I don't quite understand what your suggestion is but in general I'm
>> concerned since any approach to move everyone to different apis is
>> rather risky especially for a critical driver.
>>
>> If I'm missing your point please let me know.
> 
> I do agree we dont want to break anyone. But at the same time, this
> patch redundantly enables/disables clock for most known (and possible
> future) users without no clear indication of who benefits from such
> redundancy.
> 
> If we ignore this now, the problem will only get worse as time passes. I
> suggest biting the bullet now, attempt to reach-out as many affected
> parties as possible but switch to pm_runtime once and for all.
> 
>>
>>>
>>> The clock enable support itself was added pretty "recently" - about 5
>>> years back with 0bbeb3c3e84b ("of serial port driver - add
>>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>>> on clock support being there. It was added by Murali, I assume for
>>> Keystone devices. Keystone devices can work with runtime PM using the PM
>>> clock support pointed to above.
>>
>> You might be right but I can't be confident that it is indeed the case.
>> But its possible that at some point people will start having problems if
>> they try to use this driver for other UART instances. The currently
>> could not be aware of an issue because of the bootloader powering things
>> for them or even that different UART instances could be apart of a non
>> always on power domain.
> 
> I dont think you got my point there. I have no disagreement that
> pm_runtime support is needed. In fact, it should have been that way when
> clock enable/disable was added. But thats history now.
> 
>>> Perhaps linux-arm-kernel list should be copied on this submission too,
>>> since most users of this driver are likely to be there on that list.
>>>
>>
>> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
>> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
>> PowerPC, MIPS, Xtensa and Arc.>
>> Looking at dts that enable some of the compatible it is still a
>> combination of quite a bit of architectures.
> 
> Right, I do think there are quite a few users. But they should still be
> reachable and provide testing inputs for the change.
> 
>> The current approach I've taken should be safe for all users of this
>> driver which. I have no issue going another approach as long as its
>> understood that I'm fairly limited in what I can test when you take into
>> account the large number of users of this driver.
> 
> Understood that you are limited by hardware you possess. But we can
> probably reach out to as many folks affected by this as possible so we
> get a fair chance at doing the "right thing" without breaking anyone.

I guess I don't understand the acceptable criteria when we can go
forward and make this change for something that impacts so many users
across so many architectures and SoC families.

I have no issue increasing the audience for a pm_runtime only version of
this patch to hopefully get more eyes on it. But is the expectation for
all the various maintainers of all the various dts to come out and say
they are ok with the change and then we will go forward? Or just atleast
half of them say they are ok with switching and no one else objects?
What is the criteria so this doesn't stay in indefinite limbo or when
will we be confident enough that this (some variant of the patch)
doesn't break things for someone? If using pm_runtime only breaks things
for someone is the answer to revert pm_runtime switch or will the answer
that they need to fix their SoC to fix the problem.

Forgive me for all the questions and worrying. This is new grounds for
me especially when I'm unable to test things to avoid causing problems
for people.
> 
> Thanks,
> Sekhar
> 

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Franklin S Cooper Jr]

On 08/23/2017 01:34 AM, Sekhar Nori wrote:
> On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
>>
>>
>> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>>> users of this driver may not support PM runtime.
>>
>> There are a significant amount of users across a wide variety of
>> architectures and boards that I have no means to properly test to insure
>> I'm avoiding regressions. My current approach which I've seen other
>> drivers in the past use when in similar situations allows things to work
>> without regressions.
> 
> Since the users are all device-tree users, my hope is they can be
> tracked by looking at the DT files and the maintainers be copied on this
> patch to make sure nothing breaks for them. I understand its not a
> trivial list, but its finite :)
> 
>>>
>>> Do we really have users like that? And even if there are, cant they use
>>> PM clock handling support available in drivers/base/power/clock_ops.c ?
>>
>> I don't see any current defconfig that enables CONFIG_PM_CLK and only a
> 
> Thats because its a non-user-selectable def_bool. Most configs should
> have it enabled after the 'make config' step.
> 
>> handful of instances where functions from clock_ops.c are actually used.
>> I don't quite understand what your suggestion is but in general I'm
>> concerned since any approach to move everyone to different apis is
>> rather risky especially for a critical driver.
>>
>> If I'm missing your point please let me know.
> 
> I do agree we dont want to break anyone. But at the same time, this
> patch redundantly enables/disables clock for most known (and possible
> future) users without no clear indication of who benefits from such
> redundancy.
> 
> If we ignore this now, the problem will only get worse as time passes. I
> suggest biting the bullet now, attempt to reach-out as many affected
> parties as possible but switch to pm_runtime once and for all.
> 
>>
>>>
>>> The clock enable support itself was added pretty "recently" - about 5
>>> years back with 0bbeb3c3e84b ("of serial port driver - add
>>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>>> on clock support being there. It was added by Murali, I assume for
>>> Keystone devices. Keystone devices can work with runtime PM using the PM
>>> clock support pointed to above.
>>
>> You might be right but I can't be confident that it is indeed the case.
>> But its possible that at some point people will start having problems if
>> they try to use this driver for other UART instances. The currently
>> could not be aware of an issue because of the bootloader powering things
>> for them or even that different UART instances could be apart of a non
>> always on power domain.
> 
> I dont think you got my point there. I have no disagreement that
> pm_runtime support is needed. In fact, it should have been that way when
> clock enable/disable was added. But thats history now.
> 
>>> Perhaps linux-arm-kernel list should be copied on this submission too,
>>> since most users of this driver are likely to be there on that list.
>>>
>>
>> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
>> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
>> PowerPC, MIPS, Xtensa and Arc.>
>> Looking at dts that enable some of the compatible it is still a
>> combination of quite a bit of architectures.
> 
> Right, I do think there are quite a few users. But they should still be
> reachable and provide testing inputs for the change.
> 
>> The current approach I've taken should be safe for all users of this
>> driver which. I have no issue going another approach as long as its
>> understood that I'm fairly limited in what I can test when you take into
>> account the large number of users of this driver.
> 
> Understood that you are limited by hardware you possess. But we can
> probably reach out to as many folks affected by this as possible so we
> get a fair chance at doing the "right thing" without breaking anyone.

I guess I don't understand the acceptable criteria when we can go
forward and make this change for something that impacts so many users
across so many architectures and SoC families.

I have no issue increasing the audience for a pm_runtime only version of
this patch to hopefully get more eyes on it. But is the expectation for
all the various maintainers of all the various dts to come out and say
they are ok with the change and then we will go forward? Or just atleast
half of them say they are ok with switching and no one else objects?
What is the criteria so this doesn't stay in indefinite limbo or when
will we be confident enough that this (some variant of the patch)
doesn't break things for someone? If using pm_runtime only breaks things
for someone is the answer to revert pm_runtime switch or will the answer
that they need to fix their SoC to fix the problem.

Forgive me for all the questions and worrying. This is new grounds for
me especially when I'm unable to test things to avoid causing problems
for people.
> 
> Thanks,
> Sekhar
> 

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Sekhar Nori]

On Wednesday 23 August 2017 12:49 PM, Franklin S Cooper Jr wrote:
> 
> 
> On 08/23/2017 01:34 AM, Sekhar Nori wrote:
>> On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
>>>
>>>
>>> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>>>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>>>> users of this driver may not support PM runtime.
>>>
>>> There are a significant amount of users across a wide variety of
>>> architectures and boards that I have no means to properly test to insure
>>> I'm avoiding regressions. My current approach which I've seen other
>>> drivers in the past use when in similar situations allows things to work
>>> without regressions.
>>
>> Since the users are all device-tree users, my hope is they can be
>> tracked by looking at the DT files and the maintainers be copied on this
>> patch to make sure nothing breaks for them. I understand its not a
>> trivial list, but its finite :)
>>
>>>>
>>>> Do we really have users like that? And even if there are, cant they use
>>>> PM clock handling support available in drivers/base/power/clock_ops.c ?
>>>
>>> I don't see any current defconfig that enables CONFIG_PM_CLK and only a
>>
>> Thats because its a non-user-selectable def_bool. Most configs should
>> have it enabled after the 'make config' step.
>>
>>> handful of instances where functions from clock_ops.c are actually used.
>>> I don't quite understand what your suggestion is but in general I'm
>>> concerned since any approach to move everyone to different apis is
>>> rather risky especially for a critical driver.
>>>
>>> If I'm missing your point please let me know.
>>
>> I do agree we dont want to break anyone. But at the same time, this
>> patch redundantly enables/disables clock for most known (and possible
>> future) users without no clear indication of who benefits from such
>> redundancy.
>>
>> If we ignore this now, the problem will only get worse as time passes. I
>> suggest biting the bullet now, attempt to reach-out as many affected
>> parties as possible but switch to pm_runtime once and for all.
>>
>>>
>>>>
>>>> The clock enable support itself was added pretty "recently" - about 5
>>>> years back with 0bbeb3c3e84b ("of serial port driver - add
>>>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>>>> on clock support being there. It was added by Murali, I assume for
>>>> Keystone devices. Keystone devices can work with runtime PM using the PM
>>>> clock support pointed to above.
>>>
>>> You might be right but I can't be confident that it is indeed the case.
>>> But its possible that at some point people will start having problems if
>>> they try to use this driver for other UART instances. The currently
>>> could not be aware of an issue because of the bootloader powering things
>>> for them or even that different UART instances could be apart of a non
>>> always on power domain.
>>
>> I dont think you got my point there. I have no disagreement that
>> pm_runtime support is needed. In fact, it should have been that way when
>> clock enable/disable was added. But thats history now.
>>
>>>> Perhaps linux-arm-kernel list should be copied on this submission too,
>>>> since most users of this driver are likely to be there on that list.
>>>>
>>>
>>> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
>>> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
>>> PowerPC, MIPS, Xtensa and Arc.>
>>> Looking at dts that enable some of the compatible it is still a
>>> combination of quite a bit of architectures.
>>
>> Right, I do think there are quite a few users. But they should still be
>> reachable and provide testing inputs for the change.
>>
>>> The current approach I've taken should be safe for all users of this
>>> driver which. I have no issue going another approach as long as its
>>> understood that I'm fairly limited in what I can test when you take into
>>> account the large number of users of this driver.
>>
>> Understood that you are limited by hardware you possess. But we can
>> probably reach out to as many folks affected by this as possible so we
>> get a fair chance at doing the "right thing" without breaking anyone.
> 
> I guess I don't understand the acceptable criteria when we can go
> forward and make this change for something that impacts so many users
> across so many architectures and SoC families.
> 
> I have no issue increasing the audience for a pm_runtime only version of
> this patch to hopefully get more eyes on it. But is the expectation for
> all the various maintainers of all the various dts to come out and say
> they are ok with the change and then we will go forward? Or just atleast

In a perfect world, yes..

> half of them say they are ok with switching and no one else objects?
> What is the criteria so this doesn't stay in indefinite limbo or when
> will we be confident enough that this (some variant of the patch)

.. but I think we are just trying to get as many acks as possible
without someone saying it breaks their system. I don't think we want to
wait indefinitely, just long enough to give everyone a reasonable chance
to test and comment (a couple of weeks).

> doesn't break things for someone? If using pm_runtime only breaks things
> for someone is the answer to revert pm_runtime switch or will the answer
> that they need to fix their SoC to fix the problem.

Its tough to say now what will be the course if switching to pm_runtime
does break someone's system. If it happens, most probably its fixable by
calling pm_clk_add_notifier() for that machine's platform bus.

> Forgive me for all the questions and worrying. This is new grounds for
> me especially when I'm unable to test things to avoid causing problems
> for people.

No problem. You may also wait a while to see if there are opposing ideas
before posting a v4.

Thanks,
Sekhar

Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support

[Sekhar Nori]

On Wednesday 23 August 2017 12:49 PM, Franklin S Cooper Jr wrote:
> 
> 
> On 08/23/2017 01:34 AM, Sekhar Nori wrote:
>> On Monday 21 August 2017 10:29 PM, Franklin S Cooper Jr wrote:
>>>
>>>
>>> On 08/21/2017 07:20 AM, Sekhar Nori wrote:
>>>> On Thursday 17 August 2017 02:25 AM, Franklin S Cooper Jr wrote:
>>>>> 66AK2G UART instances are not apart of the ALWAYS_ON power domain.
>>>>> Therefore, pm_runtime calls must be made to properly insure the appropriate
>>>>> power domains needed by UART are on. Keep legacy clk api calls since other
>>>>> users of this driver may not support PM runtime.
>>>
>>> There are a significant amount of users across a wide variety of
>>> architectures and boards that I have no means to properly test to insure
>>> I'm avoiding regressions. My current approach which I've seen other
>>> drivers in the past use when in similar situations allows things to work
>>> without regressions.
>>
>> Since the users are all device-tree users, my hope is they can be
>> tracked by looking at the DT files and the maintainers be copied on this
>> patch to make sure nothing breaks for them. I understand its not a
>> trivial list, but its finite :)
>>
>>>>
>>>> Do we really have users like that? And even if there are, cant they use
>>>> PM clock handling support available in drivers/base/power/clock_ops.c ?
>>>
>>> I don't see any current defconfig that enables CONFIG_PM_CLK and only a
>>
>> Thats because its a non-user-selectable def_bool. Most configs should
>> have it enabled after the 'make config' step.
>>
>>> handful of instances where functions from clock_ops.c are actually used.
>>> I don't quite understand what your suggestion is but in general I'm
>>> concerned since any approach to move everyone to different apis is
>>> rather risky especially for a critical driver.
>>>
>>> If I'm missing your point please let me know.
>>
>> I do agree we dont want to break anyone. But at the same time, this
>> patch redundantly enables/disables clock for most known (and possible
>> future) users without no clear indication of who benefits from such
>> redundancy.
>>
>> If we ignore this now, the problem will only get worse as time passes. I
>> suggest biting the bullet now, attempt to reach-out as many affected
>> parties as possible but switch to pm_runtime once and for all.
>>
>>>
>>>>
>>>> The clock enable support itself was added pretty "recently" - about 5
>>>> years back with 0bbeb3c3e84b ("of serial port driver - add
>>>> clk_get_rate() support"). So I doubt any really legacy platforms relied
>>>> on clock support being there. It was added by Murali, I assume for
>>>> Keystone devices. Keystone devices can work with runtime PM using the PM
>>>> clock support pointed to above.
>>>
>>> You might be right but I can't be confident that it is indeed the case.
>>> But its possible that at some point people will start having problems if
>>> they try to use this driver for other UART instances. The currently
>>> could not be aware of an issue because of the bootloader powering things
>>> for them or even that different UART instances could be apart of a non
>>> always on power domain.
>>
>> I dont think you got my point there. I have no disagreement that
>> pm_runtime support is needed. In fact, it should have been that way when
>> clock enable/disable was added. But thats history now.
>>
>>>> Perhaps linux-arm-kernel list should be copied on this submission too,
>>>> since most users of this driver are likely to be there on that list.
>>>>
>>>
>>> Looking at configs that enable CONFIG_SERIAL_OF_PLATFORM I see quite a
>>> bit of users from different arch. ARM, OpenRISC, MicroBlaze, Nios2,
>>> PowerPC, MIPS, Xtensa and Arc.>
>>> Looking at dts that enable some of the compatible it is still a
>>> combination of quite a bit of architectures.
>>
>> Right, I do think there are quite a few users. But they should still be
>> reachable and provide testing inputs for the change.
>>
>>> The current approach I've taken should be safe for all users of this
>>> driver which. I have no issue going another approach as long as its
>>> understood that I'm fairly limited in what I can test when you take into
>>> account the large number of users of this driver.
>>
>> Understood that you are limited by hardware you possess. But we can
>> probably reach out to as many folks affected by this as possible so we
>> get a fair chance at doing the "right thing" without breaking anyone.
> 
> I guess I don't understand the acceptable criteria when we can go
> forward and make this change for something that impacts so many users
> across so many architectures and SoC families.
> 
> I have no issue increasing the audience for a pm_runtime only version of
> this patch to hopefully get more eyes on it. But is the expectation for
> all the various maintainers of all the various dts to come out and say
> they are ok with the change and then we will go forward? Or just atleast

In a perfect world, yes..

> half of them say they are ok with switching and no one else objects?
> What is the criteria so this doesn't stay in indefinite limbo or when
> will we be confident enough that this (some variant of the patch)

.. but I think we are just trying to get as many acks as possible
without someone saying it breaks their system. I don't think we want to
wait indefinitely, just long enough to give everyone a reasonable chance
to test and comment (a couple of weeks).

> doesn't break things for someone? If using pm_runtime only breaks things
> for someone is the answer to revert pm_runtime switch or will the answer
> that they need to fix their SoC to fix the problem.

Its tough to say now what will be the course if switching to pm_runtime
does break someone's system. If it happens, most probably its fixable by
calling pm_clk_add_notifier() for that machine's platform bus.

> Forgive me for all the questions and worrying. This is new grounds for
> me especially when I'm unable to test things to avoid causing problems
> for people.

No problem. You may also wait a while to see if there are opposing ideas
before posting a v4.

Thanks,
Sekhar