From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from na01-bl2-obe.outbound.protection.outlook.com (mail-bl2lp0207.outbound.protection.outlook.com [207.46.163.207]) (using TLSv1 with cipher AES128-SHA (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 7173314027A for ; Mon, 5 May 2014 22:51:23 +1000 (EST) Message-ID: <53678595.4090707@freescale.com> Date: Mon, 5 May 2014 15:35:33 +0300 From: Tudor Laurentiu MIME-Version: 1.0 To: Alexander Graf Subject: Re: [PATCH] powerpc: move epapr paravirt init of power_save to an initcall References: <1398887641-20705-1-git-send-email-stuart.yoder@freescale.com> <5361556D.7080805@suse.de> <33939d8249c34c8fb694da3e94196211@DM2PR03MB352.namprd03.prod.outlook.com> <53615872.3010006@suse.de> <5367815F.5010509@freescale.com> <53678236.7060206@suse.de> In-Reply-To: <53678236.7060206@suse.de> Content-Type: text/plain; charset="UTF-8"; format=flowed Cc: Scott Wood , Laurentiu.Tudor@freescale.com, "linuxppc-dev@lists.ozlabs.org" , Stuart Yoder List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 05/05/2014 03:21 PM, Alexander Graf wrote: > On 05/05/2014 02:17 PM, Tudor Laurentiu wrote: >> On 04/30/2014 11:09 PM, Alexander Graf wrote: >>> >>> On 30.04.14 22:03, Stuart Yoder wrote: >>>> >>>>> -----Original Message----- >>>>> From: Alexander Graf [mailto:agraf@suse.de] >>>>> Sent: Wednesday, April 30, 2014 2:56 PM >>>>> To: Yoder Stuart-B08248; benh@kernel.crashing.org; Wood Scott-B07421 >>>>> Cc: linuxppc-dev@lists.ozlabs.org >>>>> Subject: Re: [PATCH] powerpc: move epapr paravirt init of >>>>> power_save to >>>>> an initcall >>>>> >>>>> >>>>> On 30.04.14 21:54, Stuart Yoder wrote: >>>>>> From: Stuart Yoder >>>>>> >>>>>> some restructuring of epapr paravirt init resulted in >>>>>> ppc_md.power_save being set, and then overwritten to >>>>>> NULL during machine_init. This patch splits the >>>>>> initialization of ppc_md.power_save out into a postcore >>>>>> init call. >>>>>> >>>>>> Signed-off-by: Stuart Yoder >>>>>> --- >>>>>> arch/powerpc/kernel/epapr_paravirt.c | 25 >>>>>> ++++++++++++++++++++----- >>>>>> 1 file changed, 20 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/arch/powerpc/kernel/epapr_paravirt.c >>>>> b/arch/powerpc/kernel/epapr_paravirt.c >>>>>> index 6300c13..c49b69c 100644 >>>>>> --- a/arch/powerpc/kernel/epapr_paravirt.c >>>>>> +++ b/arch/powerpc/kernel/epapr_paravirt.c >>>>>> @@ -52,11 +52,6 @@ static int __init >>>>>> early_init_dt_scan_epapr(unsigned >>>>> long node, >>>>>> #endif >>>>>> } >>>>>> >>>>>> -#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64) >>>>>> - if (of_get_flat_dt_prop(node, "has-idle", NULL)) >>>>>> - ppc_md.power_save = epapr_ev_idle; >>>>>> -#endif >>>>>> - >>>>>> epapr_paravirt_enabled = true; >>>>>> >>>>>> return 1; >>>>>> @@ -69,3 +64,23 @@ int __init epapr_paravirt_early_init(void) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +static int __init epapr_idle_init_dt_scan(unsigned long node, >>>>>> + const char *uname, >>>>>> + int depth, void *data) >>>>>> +{ >>>>>> +#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64) >>>>>> + if (of_get_flat_dt_prop(node, "has-idle", NULL)) >>>>>> + ppc_md.power_save = epapr_ev_idle; >>>>>> +#endif >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int __init epapr_idle_init(void) >>>>>> +{ >>>>>> + if (epapr_paravirt_enabled) >>>>>> + of_scan_flat_dt(epapr_idle_init_dt_scan, NULL); >>>>> Doesn't this scan all nodes? We only want to match on >>>>> /hypervisor/has-idle, no? >>>> I cut/pasted from the approach the existing code in that file >>>> took, but yes you're right we just need the one property. >>>> Let me respin that to look at the hypervisor node only. >>> >>> Yeah, the same commit that introduced the breakage on has-idle also >>> removed the explicit check for /hypervisor. >>> >>> Laurentiu, was this change on purpose? >>> >> >> Alex, >> >> IIRC, at that time i had to switch from the normal "of" functions to a >> completely different api that's available in early init stage. This >> early "of" api is pretty limited (e.g. doesn't have a way to address a >> specific node) and i had to use that function that scans the whole tree. > > Ok, so it is an accident. Could you please post a patch that checks that > the node we're looking at is called "hypervisor"? The simple API should > give you enough information for that at least. Maybe you could even > check that the parent node is the root node. > Just had a quick look and it looks that that early fdt api was improved with a function that allows specifying a starting path for the scan (of_scan_flat_dt_by_path() added in commit 57d74bcf3072b65bde5aa540cedc976a75c48e5c). So i think we can simply use that instead. --- Best Regards, Laurentiu