From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 59C62C2D0C0 for ; Thu, 5 Dec 2019 20:26:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2A54F24674 for ; Thu, 5 Dec 2019 20:26:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="Vv6YHYn/" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729145AbfLEU0c (ORCPT ); Thu, 5 Dec 2019 15:26:32 -0500 Received: from mail-vk1-f196.google.com ([209.85.221.196]:39166 "EHLO mail-vk1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729187AbfLEU0c (ORCPT ); Thu, 5 Dec 2019 15:26:32 -0500 Received: by mail-vk1-f196.google.com with SMTP id x199so1575713vke.6 for ; Thu, 05 Dec 2019 12:26:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PRZIeGTgLYe32xwj6wCkSy8WULcrgIUyS3clF5He5As=; b=Vv6YHYn/g7rA9ELgKQoYQBL4QdGjWvSx07OcIA+s9uJL79lPxzqHKScJ91kLi7GiAE GXHu3NHrNsWuSxMm9shl63nmOcnQvcD4lZUYki4NMrdMbd38NJ1Hd4PrUqRxiFDuNM4N uSujBoWplByotggU8Dzw/msWAkT+G02TE0YzmQdhqODbvLzKMQG3cCUT8hWM4hCtJ8N2 9j1EUphxgvJkCDh6nwicSDpSbCmZGOWKOU+2nTLV92i9wY3JFDrwJ8IHz3BrFr6/RXjo OzPJd2Bdh7PNC7/wSbEi9b5GB5Odlo9aZDFwjDn70JelWkP3cMeEg4A0DSxQJfZrO7bx cCJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=PRZIeGTgLYe32xwj6wCkSy8WULcrgIUyS3clF5He5As=; b=l1CbZRou7s5yz5riDlIhkkxPYA4zVh+wVmzPX+peWbvW3Z6ecQiAPakICGoe8ZqKsx uYskVyNynuEjqMfW8RRLc9nbUzJ5oju/zpqoOWBJO5vkOSn+UrUNGPPA5RylLNmgBexb lXCj0ZLJcka5E2LnPismNNwhwBqLT5Kn35WtaNjiXLiUlmo3IWItsf3UEdufKrRam6FL +Gpcr0hTZrQ66oecrjjq85Gwkx86JGq+I3ndYOoqYaYXJ0d6ZSJrDGXjayMb1ba93Ju9 ojtGlOEH39rBuTYgwzFBMHN1J7IbHPwshPnPhEfiYkMyYfp3yN6xLVCTlM08e7Rh7vp6 qllQ== X-Gm-Message-State: APjAAAVI8iWOR+udelcCkl/9Wd0TKH71iCMc09DqHuBf6sg2u+C+L0ZE KzpI5Gw9ov8JucYkK59BRRfDlUrTrteRPtLyUXjxoQ== X-Google-Smtp-Source: APXvYqwJJ8Ov6kfOALtgEnyiA5wFf5YLHSlajlzw+cIFNSq6BfMBdVmzkt4dpZD618trXduh3PKpSjDTLxUI2kReD0M= X-Received: by 2002:a1f:2f04:: with SMTP id v4mr8247656vkv.101.1575577591049; Thu, 05 Dec 2019 12:26:31 -0800 (PST) MIME-Version: 1.0 References: <20191127102914.18729-1-ulf.hansson@linaro.org> <20191127102914.18729-11-ulf.hansson@linaro.org> <20191205183544.GB1516@e121166-lin.cambridge.arm.com> In-Reply-To: <20191205183544.GB1516@e121166-lin.cambridge.arm.com> From: Ulf Hansson Date: Thu, 5 Dec 2019 21:25:54 +0100 Message-ID: Subject: Re: [PATCH v3 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains To: Lorenzo Pieralisi Cc: Sudeep Holla , Rob Herring , Linux PM , "Rafael J . Wysocki" , Daniel Lezcano , Mark Rutland , Lina Iyer , Vincent Guittot , Stephen Boyd , Andy Gross , Bjorn Andersson , Kevin Hilman , Linux ARM , linux-arm-msm , Lina Iyer Content-Type: text/plain; charset="UTF-8" Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On Thu, 5 Dec 2019 at 19:35, Lorenzo Pieralisi wrote: > > On Wed, Nov 27, 2019 at 11:29:11AM +0100, Ulf Hansson wrote: > > [...] > > > -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, > > +static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv, > > + struct device_node *cpu_node, > > unsigned int state_count, int cpu) > > { > > int i, ret = 0; > > @@ -118,6 +152,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, > > goto free_mem; > > } > > > > + /* Manage the deepest state via a dedicated enter-function. */ > > + if (dev) > > + drv->states[state_count - 1].enter = > > + psci_enter_domain_idle_state; > > > It is unfortunate to make this arbitrary choice, it would be best > if you could detect which states are "domain" states aka are governed > by multiple cpus. The domain states are managed and selected by the genpd providers, via using runtime PM reference counting. Please have a closer look at the code in cpuidle-psci-domain.c and in the generic PM domain, that should give you the needed details. I am overriding the enter callback for the *deepest* known idle state of the CPU, which is according to what you requested [1]. So, unless I am missing your point, I think the above code does exactly what you want, no? In regards to the "arbitrary choice" of what cpuidle state to use, there are more details about why that is, in the changelog. > > This inizialization though does not belong in here, it is done at driver > level, it should not be done in this per-cpu path. IIUC the logic the > enter pointer should only be overridden if and only if all cpus managed > by the driver have a corresponding device associated. I think you have overlooked the fact that there are one cpuidle driver registered per CPU. The above doesn't make sense to me, sorry. > > To be frank I would even move the psci_has_osi_support() check from > psci_dt_attach_cpu() to this path and prevent calling > psci_dt_attach_cpu() and the tail of the function if > (!psci_has_osi_support()). > > > data->dev = dev; > > I think Sudeep already mentioned that, by using psci_has_osi_support() > as above you can prevent running this code, there is really no point, > the data->dev NULL sentinel is already initialized. Yes, I discussed this with Sudeep, but we didn't reach a consensus. Let me explain the reasons behind the selected approach, once more. The data->dev is a pointer within a static declared struct. Are you sure it's assigned NULL by initialization? Don't we explicitly need to set it to NULL, else it will be undefined, no? Of course, I can move the check for psci_has_osi_support() into here and avoid calling psci_dt_attach_cpu(). Just wondering what that actually gain us, especially if we need to explicitly set the pointer to NULL anyway. That said, can you please confirm your thoughts around this, I will change to whatever you think is best. [...] Kind regards Uffe [1] https://www.spinics.net/lists/arm-kernel/msg770558.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32B59C43603 for ; Thu, 5 Dec 2019 20:26:37 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 07B1D24670 for ; Thu, 5 Dec 2019 20:26:36 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="TQNFJrrS"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="Vv6YHYn/" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 07B1D24670 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=vEblCO/h+N6WgjwpiVnXHz4UGBruc/ojUJ7+vN2tiok=; b=TQNFJrrSnvLanI e269NS+wa7kdVLU6R2HLiZ93x+NVXyUCgyE4Qilo9/ntQGkvNLSqR6xCc2v3pymINl1f97/fb/jdT EtBkczrJPd0pxGvlNGqmXVJ3j28kthH2ZmzdcTasAxgJ8jYGndVGS7sXJAxD9EsQA4VMP7UkXCyyq hqpU1mtEidQSFzD+GQbh69hGj+O+4Gqp3S9B8UqptZJ8fG7To79VL0lYFmbIueK8Tm1vkqf0zEO5K yyy5RSB+RdDpHz7uwp17XkGrJ0M9zhZ4nRd3r7AZkKRE7D8W1IxUbmIoQ7kQuvhjN/JkdEPycTAub h0IjTsuD7QY2zu6inU8w==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1icxhs-0001da-Dl; Thu, 05 Dec 2019 20:26:36 +0000 Received: from mail-vk1-xa43.google.com ([2607:f8b0:4864:20::a43]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1icxho-0001cy-Mk for linux-arm-kernel@lists.infradead.org; Thu, 05 Dec 2019 20:26:34 +0000 Received: by mail-vk1-xa43.google.com with SMTP id i78so1581300vke.0 for ; Thu, 05 Dec 2019 12:26:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=PRZIeGTgLYe32xwj6wCkSy8WULcrgIUyS3clF5He5As=; b=Vv6YHYn/g7rA9ELgKQoYQBL4QdGjWvSx07OcIA+s9uJL79lPxzqHKScJ91kLi7GiAE GXHu3NHrNsWuSxMm9shl63nmOcnQvcD4lZUYki4NMrdMbd38NJ1Hd4PrUqRxiFDuNM4N uSujBoWplByotggU8Dzw/msWAkT+G02TE0YzmQdhqODbvLzKMQG3cCUT8hWM4hCtJ8N2 9j1EUphxgvJkCDh6nwicSDpSbCmZGOWKOU+2nTLV92i9wY3JFDrwJ8IHz3BrFr6/RXjo OzPJd2Bdh7PNC7/wSbEi9b5GB5Odlo9aZDFwjDn70JelWkP3cMeEg4A0DSxQJfZrO7bx cCJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=PRZIeGTgLYe32xwj6wCkSy8WULcrgIUyS3clF5He5As=; b=MyhCDjchC7j1vVWejxOc8oyPzJgd1tUGoXnaVfXL9RBIbV+AOLHQnODqQ6PiPmlmOs eit/4GPjby2hrrNSLO0x0GNjXh0QwyDN72ztRqwvLs45W/IjEDCsYd3KsgFLGcVBMhfx NULkBQza9oYbv65R1GAeBLr+cezLpSmD+Bp0MF6xEPdoTiP1WwEE4g6mEzR+ZIH+5yCg ygRoiIKUE1WjRIB/Xcn4IcBgDopOgCBOKQvMdQ11ymsuCjEzBCtWJ+GOdgLU3/ICPcJa gx+UCmnFGnMF93TUBnBUBivXZFvvTSZe/MhcbrVz1AYJ8dTOsHLY898IvyJYTWp1Apgs sngg== X-Gm-Message-State: APjAAAVWDrXn9X/YRl6eBZSDH5hcLt9jmPwdv02czNLY13RsOAYgqGzp dw3NRcW1U2IuxSHCBmSxbYXFfgDeS8+3xItaj47DLQ== X-Google-Smtp-Source: APXvYqwJJ8Ov6kfOALtgEnyiA5wFf5YLHSlajlzw+cIFNSq6BfMBdVmzkt4dpZD618trXduh3PKpSjDTLxUI2kReD0M= X-Received: by 2002:a1f:2f04:: with SMTP id v4mr8247656vkv.101.1575577591049; Thu, 05 Dec 2019 12:26:31 -0800 (PST) MIME-Version: 1.0 References: <20191127102914.18729-1-ulf.hansson@linaro.org> <20191127102914.18729-11-ulf.hansson@linaro.org> <20191205183544.GB1516@e121166-lin.cambridge.arm.com> In-Reply-To: <20191205183544.GB1516@e121166-lin.cambridge.arm.com> From: Ulf Hansson Date: Thu, 5 Dec 2019 21:25:54 +0100 Message-ID: Subject: Re: [PATCH v3 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains To: Lorenzo Pieralisi X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191205_122632_949403_C20823CA X-CRM114-Status: GOOD ( 21.55 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Linux PM , Stephen Boyd , linux-arm-msm , Daniel Lezcano , "Rafael J . Wysocki" , Andy Gross , Lina Iyer , Bjorn Andersson , Kevin Hilman , Rob Herring , Lina Iyer , Sudeep Holla , Linux ARM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 5 Dec 2019 at 19:35, Lorenzo Pieralisi wrote: > > On Wed, Nov 27, 2019 at 11:29:11AM +0100, Ulf Hansson wrote: > > [...] > > > -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, > > +static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv, > > + struct device_node *cpu_node, > > unsigned int state_count, int cpu) > > { > > int i, ret = 0; > > @@ -118,6 +152,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, > > goto free_mem; > > } > > > > + /* Manage the deepest state via a dedicated enter-function. */ > > + if (dev) > > + drv->states[state_count - 1].enter = > > + psci_enter_domain_idle_state; > > > It is unfortunate to make this arbitrary choice, it would be best > if you could detect which states are "domain" states aka are governed > by multiple cpus. The domain states are managed and selected by the genpd providers, via using runtime PM reference counting. Please have a closer look at the code in cpuidle-psci-domain.c and in the generic PM domain, that should give you the needed details. I am overriding the enter callback for the *deepest* known idle state of the CPU, which is according to what you requested [1]. So, unless I am missing your point, I think the above code does exactly what you want, no? In regards to the "arbitrary choice" of what cpuidle state to use, there are more details about why that is, in the changelog. > > This inizialization though does not belong in here, it is done at driver > level, it should not be done in this per-cpu path. IIUC the logic the > enter pointer should only be overridden if and only if all cpus managed > by the driver have a corresponding device associated. I think you have overlooked the fact that there are one cpuidle driver registered per CPU. The above doesn't make sense to me, sorry. > > To be frank I would even move the psci_has_osi_support() check from > psci_dt_attach_cpu() to this path and prevent calling > psci_dt_attach_cpu() and the tail of the function if > (!psci_has_osi_support()). > > > data->dev = dev; > > I think Sudeep already mentioned that, by using psci_has_osi_support() > as above you can prevent running this code, there is really no point, > the data->dev NULL sentinel is already initialized. Yes, I discussed this with Sudeep, but we didn't reach a consensus. Let me explain the reasons behind the selected approach, once more. The data->dev is a pointer within a static declared struct. Are you sure it's assigned NULL by initialization? Don't we explicitly need to set it to NULL, else it will be undefined, no? Of course, I can move the check for psci_has_osi_support() into here and avoid calling psci_dt_attach_cpu(). Just wondering what that actually gain us, especially if we need to explicitly set the pointer to NULL anyway. That said, can you please confirm your thoughts around this, I will change to whatever you think is best. [...] Kind regards Uffe [1] https://www.spinics.net/lists/arm-kernel/msg770558.html _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel