Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753548AbdHWIRH (ORCPT ); Wed, 23 Aug 2017 04:17:07 -0400 Received: from lelnx193.ext.ti.com ([198.47.27.77]:59917 "EHLO lelnx193.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753433AbdHWIRD (ORCPT ); Wed, 23 Aug 2017 04:17:03 -0400 Subject: Re: [PATCH v3] serial: 8250_of: Add basic PM runtime support To: Franklin S Cooper Jr , , , , , , , , , , CC: Murali Karicheri References: <20170816205536.19634-1-fcooper@ti.com> <1e3ad33d-882e-d4a0-6295-ac06397b11d2@ti.com> <1f92eeec-d1d4-1f84-299a-ddb5160869e2@ti.com> From: Sekhar Nori Message-ID: <64d0c8ad-40d1-2162-ff90-f4a21c1455da@ti.com> Date: Wed, 23 Aug 2017 13:45:11 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6116 Lines: 129 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