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
>