From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Date: Tue, 18 Mar 2014 16:03:23 +0000 Subject: Re: [PATCH] ARM: shmobile: Add Lager clock workarounds for SDHI and MMCIF Message-Id: <5388619.O8dDZb5JM2@avalon> List-Id: References: <20140318125247.21670.94176.sendpatchset@w520> In-Reply-To: <20140318125247.21670.94176.sendpatchset@w520> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-sh@vger.kernel.org Hi Geert, On Tuesday 18 March 2014 16:27:19 Geert Uytterhoeven wrote: > On Tue, Mar 18, 2014 at 3:31 PM, Ben Dooks wrote: > > On 18/03/14 15:01, Laurent Pinchart wrote: > >> On Tuesday 18 March 2014 14:48:36 Ben Dooks wrote: > >>> On 18/03/14 14:25, Laurent Pinchart wrote: > >>>> On Tuesday 18 March 2014 21:52:47 Magnus Damm wrote: > >>>>> From: Magnus Damm > >>>>> > >>>>> Add MMCIF1, SDHI0 and SDHI2 to the clock workaround list for > >>>>> Lager multiplatform. Without these additional lines wakeup > >>>>> from Suspend-to-RAM never happens. > >>>> > >>>> What about fixing the root cause instead of piling up hacks ? > >>> > >>> See RFC to move the drivers/sh/pm_runtime.c out of drivers/sh. > >> > >> I wasn't sure whether the problem that this patch tries to work around > >> was related to runtime PM as it wasn't mentioned in the commit message. > >> > >>> I need to address the issue that the code does not actually allow any > >>> devices to PM. > >> > >> I've replied to your RFC and have studied the issue further since then. > >> > >> First of all, I think we should get Rafael Wysocki and Felipe Balbi in > >> the loop, as Rafael is probably the reference person for runtime PM, and > >> Felipe has submitted an alternative proposal to manage device clocks > >> automatically. > > > > I thought Felipes was pretty much what was already implemented in the > > drivers/base/power/clock_ops.c driver? > > There are some similarities... > > >> Then, just copying the drivers/sh/pm-runtime.c over to drivers/base/ > >> doesn't seem to be a good idea to me. Core infrastructure code in > >> drivers/base/ needs to be properly documented, and thus properly > >> understood. I feel like we're rushing thing, taking code that seem to > >> work but that isn't really understood, and turning it into core code. > > > > It does not need to be moved there, it would probably be better off > > having drivers/sh bein build and at-least fixing the two issues with > > the drivers/sh/pm_runtime.c > > > >> I think we should take a step back here, understand exactly what we're > >> doing, and write documentation. > > > > I would suggest that we build drivers/sh/runtime_pm.c for the > > multiplatform case and add initialsiation calls from the relevant > > arch setup files in arch/arm/mach-shmobile. > > > > Once this is done we can then look at fixing the issues with making > > some generic code. > > I agree with Laurent that we should fix the root cause, and that moving > drivers/sh/runtime_pm.c to drivers/base without a full understanding of > the ramifications is not a good idea. > > I also agree with Ben that we can fix and enable drivers/sh/runtime_pm.c > for the multi-platform case as a short-term solution, until we have a proper > understanding, and have fixed the root cause in a way acceptable for all > parties. > > May I also emphasize that, while we may not understand > drivers/sh/runtime_pm.c 100% yet, this code has been in active use for a > while. FWIW, disabling it in a -legacy kernel also leads to disabled clocks. I agree with you, I think enabling drivers/sh/pm_runtime.c as a short term fix is a better workaround than keeping the clocks enabled at all time in platform code. > Now, back to the "understanding" part... I've had a quick look around and found that pm_clk_add_notifier() is used by three other platforms only, in arch/arm/mach-keystone/pm_domain.c, arch/arm/mach-omap1/pm_bus.c and arch/arm/mach-davinci/pm_domain.c. The three implementations are very similar, so there's room for code sharing. On the other hand, four platforms using that infrastructure is not a lot, given that the use case should be quite common. There could be several reasons for that, from pm_clk_add_notifier() being too well hidden to a better solution used on other platforms. I believe this should be investigated to find out what the best solution to the problem at hand is. -- Regards, Laurent Pinchart