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=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 DDED7CA9EB9 for ; Tue, 22 Oct 2019 09:18:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AAB6C2064B for ; Tue, 22 Oct 2019 09:18:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1571735913; bh=Hlu8GaM3pyER1WvD6ECYPessAWesR9RC+922Q+vHyEo=; h=References:In-Reply-To:From:Date:Subject:To:Cc:List-ID:From; b=r4t7kB2AvdfnYqN+rYfadKPUkCXxmHyUUPSrp3p07Yqj0njaDLZVKSifz8LniU3rb URzo02/eKYBBdwqFU8Hwk8p0IsJ9cMgLlb8ub6uyHHNoQx12dN+4SwqFSzhqgAYMMB uNWeLV3iw0G1iVaIYlu66N0pdki21Kt2GUG1hw64= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388520AbfJVJSc (ORCPT ); Tue, 22 Oct 2019 05:18:32 -0400 Received: from mail-oi1-f195.google.com ([209.85.167.195]:37248 "EHLO mail-oi1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730247AbfJVJSc (ORCPT ); Tue, 22 Oct 2019 05:18:32 -0400 Received: by mail-oi1-f195.google.com with SMTP id i16so13568285oie.4; Tue, 22 Oct 2019 02:18:31 -0700 (PDT) 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=nuz84DmbVNqkBwZM2dSH7bNPNbMKWhjIIPtW7H0YPbs=; b=MPDm7Fm3Eo027By8vapjzYhqtuUXysOvIwesTAILCwGG2gFBzdgQ8UIqRz4UJdoCGk OlMafMdWO9SgJabIhzuRAz76bQUKnFc1rL7kJNJzLJGNXYXu/Pv3elElVjgp+sP+QMC/ RAOqnxJwMIDxtP3yjHMDLmP3q8geZ8FosHPZ4agcP1mRnQQC2Obsj7/VESs9kaFwGyF2 embbeL291r5NDxr73hnWyiYyQmEQeXCawpaQRIm23tEhMISvNmAswvj9fdI7rE94orFI ggUMGEfakkJnC+L0mB0V3FByvsm9r0IRPLMxO8YB8GHukA2ErPDoFnh5E+3L2CSjeF6i p/ow== X-Gm-Message-State: APjAAAXqW6aFb1oNHuZ98UgMqSugMkWwhYlUIxtRSXsvowFVTXP7BZey Br3bJNWc0AURP70W8VBJHO6amtdLx4fs4OC2rJA= X-Google-Smtp-Source: APXvYqwwDlsgGkAn9HrCd8VV+9jpvEy0YfjvtOplz9BjtCnFCm2kwmx5+01G/UWl5wByWCylJuSaEQxiHp/YzPN/6bE= X-Received: by 2002:aca:b6c5:: with SMTP id g188mr2084579oif.103.1571735910539; Tue, 22 Oct 2019 02:18:30 -0700 (PDT) MIME-Version: 1.0 References: <20191022075123.17057-1-ran.wang_1@nxp.com> <20191022075123.17057-3-ran.wang_1@nxp.com> In-Reply-To: <20191022075123.17057-3-ran.wang_1@nxp.com> From: "Rafael J. Wysocki" Date: Tue, 22 Oct 2019 11:18:18 +0200 Message-ID: Subject: Re: [PATCH 3/3] soc: fsl: add RCPM driver To: Ran Wang Cc: "Rafael J . Wysocki" , Rob Herring , Li Yang , Mark Rutland , Pavel Machek , Huang Anson , Li Biwen , Len Brown , Greg Kroah-Hartman , linuxppc-dev , Linux ARM , "devicetree@vger.kernel.org" , Linux Kernel Mailing List , Linux PM Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 22, 2019 at 9:52 AM Ran Wang wrote: > > The NXP's QorIQ Processors based on ARM Core have RCPM module > (Run Control and Power Management), which performs system level > tasks associated with power management such as wakeup source control. > > This driver depends on PM wakeup source framework which help to > collect wake information. > > Signed-off-by: Ran Wang > --- > Change in v8: > - Adjust related API usage to meet wakeup.c's update in patch 1/3. > - Add sanity checking for the case of ws->dev or ws->dev->parent > is null. > > Change in v7: > - Replace 'ws->dev' with 'ws->dev->parent' to get aligned with > c8377adfa781 ("PM / wakeup: Show wakeup sources stats in sysfs") > - Remove '+obj-y += ftm_alarm.o' since it is wrong. > - Cosmetic work. > > Change in v6: > - Adjust related API usage to meet wakeup.c's update in patch 1/3. > > Change in v5: > - Fix v4 regression of the return value of wakeup_source_get_next() > didn't pass to ws in while loop. > - Rename wakeup_source member 'attached_dev' to 'dev'. > - Rename property 'fsl,#rcpm-wakeup-cells' to '#fsl,rcpm-wakeup-cells'. > please see https://lore.kernel.org/patchwork/patch/1101022/ > > Change in v4: > - Remove extra ',' in author line of rcpm.c > - Update usage of wakeup_source_get_next() to be less confusing to the > reader, code logic remain the same. > > Change in v3: > - Some whitespace ajdustment. > > Change in v2: > - Rebase Kconfig and Makefile update to latest mainline. > > drivers/soc/fsl/Kconfig | 8 +++ > drivers/soc/fsl/Makefile | 1 + > drivers/soc/fsl/rcpm.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 142 insertions(+) > create mode 100644 drivers/soc/fsl/rcpm.c > > diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig > index f9ad8ad..4918856 100644 > --- a/drivers/soc/fsl/Kconfig > +++ b/drivers/soc/fsl/Kconfig > @@ -40,4 +40,12 @@ config DPAA2_CONSOLE > /dev/dpaa2_mc_console and /dev/dpaa2_aiop_console, > which can be used to dump the Management Complex and AIOP > firmware logs. > + > +config FSL_RCPM > + bool "Freescale RCPM support" > + depends on PM_SLEEP > + help > + The NXP QorIQ Processors based on ARM Core have RCPM module > + (Run Control and Power Management), which performs all device-level > + tasks associated with power management, such as wakeup source control. > endmenu > diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile > index 71dee8d..906f1cd 100644 > --- a/drivers/soc/fsl/Makefile > +++ b/drivers/soc/fsl/Makefile > @@ -6,6 +6,7 @@ > obj-$(CONFIG_FSL_DPAA) += qbman/ > obj-$(CONFIG_QUICC_ENGINE) += qe/ > obj-$(CONFIG_CPM) += qe/ > +obj-$(CONFIG_FSL_RCPM) += rcpm.o > obj-$(CONFIG_FSL_GUTS) += guts.o > obj-$(CONFIG_FSL_MC_DPIO) += dpio/ > obj-$(CONFIG_DPAA2_CONSOLE) += dpaa2-console.o > diff --git a/drivers/soc/fsl/rcpm.c b/drivers/soc/fsl/rcpm.c > new file mode 100644 > index 0000000..3ed135e > --- /dev/null > +++ b/drivers/soc/fsl/rcpm.c > @@ -0,0 +1,133 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// rcpm.c - Freescale QorIQ RCPM driver > +// > +// Copyright 2019 NXP > +// > +// Author: Ran Wang > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define RCPM_WAKEUP_CELL_MAX_SIZE 7 > + > +struct rcpm { > + unsigned int wakeup_cells; > + void __iomem *ippdexpcr_base; > + bool little_endian; > +}; > + Please add a kerneldoc comment describing this routine. > +static int rcpm_pm_prepare(struct device *dev) > +{ > + int i, ret, idx; > + void __iomem *base; > + struct wakeup_source *ws; > + struct rcpm *rcpm; > + struct device_node *np = dev->of_node; > + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp; > + > + rcpm = dev_get_drvdata(dev); > + if (!rcpm) > + return -EINVAL; > + > + base = rcpm->ippdexpcr_base; > + idx = wakeup_sources_read_lock(); > + > + /* Begin with first registered wakeup source */ > + for_each_wakeup_source(ws) { > + > + /* skip object which is not attached to device */ > + if (!ws->dev || !ws->dev->parent) > + continue; > + > + ret = device_property_read_u32_array(ws->dev->parent, > + "fsl,rcpm-wakeup", value, > + rcpm->wakeup_cells + 1); > + > + /* Wakeup source should refer to current rcpm device */ > + if (ret || (np->phandle != value[0])) { > + dev_info(dev, "%s doesn't refer to this rcpm\n", > + ws->name); IMO printing this message is not useful in general, because it looks like you just want to skip wakeup sources that aren't associated with rcpm devices. Maybe use pr_debug() to print it? Or maybe use pr_debug() to print a message if you have found a suitable device? Wouldn't that be more useful? > + continue; > + } > + It would be good to add a comment explaining what the code below does here. Or explain that in the function's kerneldoc comment. > + for (i = 0; i < rcpm->wakeup_cells; i++) { It looks like 'tmp' can be defined in this block. And I would store the value of value[i+1] in tmp upfront, that is u32 tmp = value[i+1]; > + /* We can only OR related bits */ > + if (value[i + 1]) { Also I would do if (!tmp) continue; to reduce the indentation level. > + if (rcpm->little_endian) { > + tmp = ioread32(base + i * 4); > + tmp |= value[i + 1]; > + iowrite32(tmp, base + i * 4); So it is sufficient to do tmp |= ioread32(base + i * 4); iowrite32(tmp, base + i * 4); here and analogously below. You may as well define void __iomem *address = base + i * 4; and use it everywhere in this block instead of the (base + I * 4) expression. > + } else { > + tmp = ioread32be(base + i * 4); > + tmp |= value[i + 1]; > + iowrite32be(tmp, base + i * 4); > + } > + } > + } > + } > + > + wakeup_sources_read_unlock(idx); > + > + return 0; > +} > + > +static const struct dev_pm_ops rcpm_pm_ops = { > + .prepare = rcpm_pm_prepare, > +}; > + 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=-7.0 required=3.0 tests=INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 DF257CA9EA0 for ; Tue, 22 Oct 2019 09:20:49 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (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 5B9162064B for ; Tue, 22 Oct 2019 09:20:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5B9162064B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 46y7KC00PhzDqMV for ; Tue, 22 Oct 2019 20:20:47 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gmail.com (client-ip=209.85.167.194; helo=mail-oi1-f194.google.com; envelope-from=rjwysocki@gmail.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=fail (p=none dis=none) header.from=kernel.org Received: from mail-oi1-f194.google.com (mail-oi1-f194.google.com [209.85.167.194]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 46y7Gd4r7dzDqJX for ; Tue, 22 Oct 2019 20:18:33 +1100 (AEDT) Received: by mail-oi1-f194.google.com with SMTP id 83so13589815oii.1 for ; Tue, 22 Oct 2019 02:18:33 -0700 (PDT) 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=nuz84DmbVNqkBwZM2dSH7bNPNbMKWhjIIPtW7H0YPbs=; b=otgsiO8woEeFVbTqmZ/PhoeOMWInQ4ytsB93QZzrrUIdtSxkzJYTIqB6K6SNT597ve cZgAi/qCYnoHDnZflulZuyClCP2wD4uebBed/285KBbPpkJy/unVdFQPXpcAb03HFRSV 6jk/wz86+uNC56btDa3PkIMRc6uUpuYk3Go0kmh+AvQ/cFgKcH1Gdj/32TClkeBJhUxu AV8LqVGyCaxu01YKeNBrp6kRJd01RoZJxwCVg8X6HQNNWZ0CZpIZlq/mduCME5Lpr0n9 +ebJyhuZUegcF3rfwRzp2/DA8BhjIasFKZGEPIR3E7TVf5yxsTrRH7ls462DFDcP5rjw xWmA== X-Gm-Message-State: APjAAAU/rGlMskSmmgyWncUvE+J3Leyt0/RKeKOc3+OzhgEcGYKkDRN5 NmWrzq6U7Z0E+vHdtZgMpCVqc3vPkAz//PvWapw= X-Google-Smtp-Source: APXvYqwwDlsgGkAn9HrCd8VV+9jpvEy0YfjvtOplz9BjtCnFCm2kwmx5+01G/UWl5wByWCylJuSaEQxiHp/YzPN/6bE= X-Received: by 2002:aca:b6c5:: with SMTP id g188mr2084579oif.103.1571735910539; Tue, 22 Oct 2019 02:18:30 -0700 (PDT) MIME-Version: 1.0 References: <20191022075123.17057-1-ran.wang_1@nxp.com> <20191022075123.17057-3-ran.wang_1@nxp.com> In-Reply-To: <20191022075123.17057-3-ran.wang_1@nxp.com> From: "Rafael J. Wysocki" Date: Tue, 22 Oct 2019 11:18:18 +0200 Message-ID: Subject: Re: [PATCH 3/3] soc: fsl: add RCPM driver To: Ran Wang Content-Type: text/plain; charset="UTF-8" X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , Li Biwen , Huang Anson , Len Brown , Greg Kroah-Hartman , Linux PM , "Rafael J . Wysocki" , Linux Kernel Mailing List , Li Yang , "devicetree@vger.kernel.org" , Rob Herring , Pavel Machek , linuxppc-dev , Linux ARM Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Tue, Oct 22, 2019 at 9:52 AM Ran Wang wrote: > > The NXP's QorIQ Processors based on ARM Core have RCPM module > (Run Control and Power Management), which performs system level > tasks associated with power management such as wakeup source control. > > This driver depends on PM wakeup source framework which help to > collect wake information. > > Signed-off-by: Ran Wang > --- > Change in v8: > - Adjust related API usage to meet wakeup.c's update in patch 1/3. > - Add sanity checking for the case of ws->dev or ws->dev->parent > is null. > > Change in v7: > - Replace 'ws->dev' with 'ws->dev->parent' to get aligned with > c8377adfa781 ("PM / wakeup: Show wakeup sources stats in sysfs") > - Remove '+obj-y += ftm_alarm.o' since it is wrong. > - Cosmetic work. > > Change in v6: > - Adjust related API usage to meet wakeup.c's update in patch 1/3. > > Change in v5: > - Fix v4 regression of the return value of wakeup_source_get_next() > didn't pass to ws in while loop. > - Rename wakeup_source member 'attached_dev' to 'dev'. > - Rename property 'fsl,#rcpm-wakeup-cells' to '#fsl,rcpm-wakeup-cells'. > please see https://lore.kernel.org/patchwork/patch/1101022/ > > Change in v4: > - Remove extra ',' in author line of rcpm.c > - Update usage of wakeup_source_get_next() to be less confusing to the > reader, code logic remain the same. > > Change in v3: > - Some whitespace ajdustment. > > Change in v2: > - Rebase Kconfig and Makefile update to latest mainline. > > drivers/soc/fsl/Kconfig | 8 +++ > drivers/soc/fsl/Makefile | 1 + > drivers/soc/fsl/rcpm.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 142 insertions(+) > create mode 100644 drivers/soc/fsl/rcpm.c > > diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig > index f9ad8ad..4918856 100644 > --- a/drivers/soc/fsl/Kconfig > +++ b/drivers/soc/fsl/Kconfig > @@ -40,4 +40,12 @@ config DPAA2_CONSOLE > /dev/dpaa2_mc_console and /dev/dpaa2_aiop_console, > which can be used to dump the Management Complex and AIOP > firmware logs. > + > +config FSL_RCPM > + bool "Freescale RCPM support" > + depends on PM_SLEEP > + help > + The NXP QorIQ Processors based on ARM Core have RCPM module > + (Run Control and Power Management), which performs all device-level > + tasks associated with power management, such as wakeup source control. > endmenu > diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile > index 71dee8d..906f1cd 100644 > --- a/drivers/soc/fsl/Makefile > +++ b/drivers/soc/fsl/Makefile > @@ -6,6 +6,7 @@ > obj-$(CONFIG_FSL_DPAA) += qbman/ > obj-$(CONFIG_QUICC_ENGINE) += qe/ > obj-$(CONFIG_CPM) += qe/ > +obj-$(CONFIG_FSL_RCPM) += rcpm.o > obj-$(CONFIG_FSL_GUTS) += guts.o > obj-$(CONFIG_FSL_MC_DPIO) += dpio/ > obj-$(CONFIG_DPAA2_CONSOLE) += dpaa2-console.o > diff --git a/drivers/soc/fsl/rcpm.c b/drivers/soc/fsl/rcpm.c > new file mode 100644 > index 0000000..3ed135e > --- /dev/null > +++ b/drivers/soc/fsl/rcpm.c > @@ -0,0 +1,133 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// rcpm.c - Freescale QorIQ RCPM driver > +// > +// Copyright 2019 NXP > +// > +// Author: Ran Wang > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define RCPM_WAKEUP_CELL_MAX_SIZE 7 > + > +struct rcpm { > + unsigned int wakeup_cells; > + void __iomem *ippdexpcr_base; > + bool little_endian; > +}; > + Please add a kerneldoc comment describing this routine. > +static int rcpm_pm_prepare(struct device *dev) > +{ > + int i, ret, idx; > + void __iomem *base; > + struct wakeup_source *ws; > + struct rcpm *rcpm; > + struct device_node *np = dev->of_node; > + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp; > + > + rcpm = dev_get_drvdata(dev); > + if (!rcpm) > + return -EINVAL; > + > + base = rcpm->ippdexpcr_base; > + idx = wakeup_sources_read_lock(); > + > + /* Begin with first registered wakeup source */ > + for_each_wakeup_source(ws) { > + > + /* skip object which is not attached to device */ > + if (!ws->dev || !ws->dev->parent) > + continue; > + > + ret = device_property_read_u32_array(ws->dev->parent, > + "fsl,rcpm-wakeup", value, > + rcpm->wakeup_cells + 1); > + > + /* Wakeup source should refer to current rcpm device */ > + if (ret || (np->phandle != value[0])) { > + dev_info(dev, "%s doesn't refer to this rcpm\n", > + ws->name); IMO printing this message is not useful in general, because it looks like you just want to skip wakeup sources that aren't associated with rcpm devices. Maybe use pr_debug() to print it? Or maybe use pr_debug() to print a message if you have found a suitable device? Wouldn't that be more useful? > + continue; > + } > + It would be good to add a comment explaining what the code below does here. Or explain that in the function's kerneldoc comment. > + for (i = 0; i < rcpm->wakeup_cells; i++) { It looks like 'tmp' can be defined in this block. And I would store the value of value[i+1] in tmp upfront, that is u32 tmp = value[i+1]; > + /* We can only OR related bits */ > + if (value[i + 1]) { Also I would do if (!tmp) continue; to reduce the indentation level. > + if (rcpm->little_endian) { > + tmp = ioread32(base + i * 4); > + tmp |= value[i + 1]; > + iowrite32(tmp, base + i * 4); So it is sufficient to do tmp |= ioread32(base + i * 4); iowrite32(tmp, base + i * 4); here and analogously below. You may as well define void __iomem *address = base + i * 4; and use it everywhere in this block instead of the (base + I * 4) expression. > + } else { > + tmp = ioread32be(base + i * 4); > + tmp |= value[i + 1]; > + iowrite32be(tmp, base + i * 4); > + } > + } > + } > + } > + > + wakeup_sources_read_unlock(idx); > + > + return 0; > +} > + > +static const struct dev_pm_ops rcpm_pm_ops = { > + .prepare = rcpm_pm_prepare, > +}; > + 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=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 DC368CA9EB9 for ; Tue, 22 Oct 2019 09:18:36 +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 B071A2084B for ; Tue, 22 Oct 2019 09:18: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="jqhkZE+5" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B071A2084B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.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=LMDEuwcwiYW9nFXS+rq/ZXrUb5/7KoCk1QDtTgIt/m4=; b=jqhkZE+59tXxRb WV1uLaUMSqAO25kcD6qaMLzCyDRPFH7eCUoR9OYX43hm8xfiGsv0JZsl+QrRHcEnQesS86TZL+EzZ VJu+Sf9Ye1kwUf+0YWEGalr0uVsCPG73u99HL/pUbRW9FO5GD7f85TZqx43qB1t+AIwQKxLQOBKYc j3/wEs/DzaEO5BwGoJfJrC1uMOS001+hjTrQrp42z/2tViJ5SpQpcTfFfuciURt895Q+szaWQNsOM AoYGcTVofpHoHxxf+1vhx77XL2mBSz4+T5DktdlMDduagAyTsGwCiDMXDsRsuxn659gOyONkTmhSE W2jdr7rGFvDC/YJ1FJ+A==; 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 1iMqJI-0001ki-CO; Tue, 22 Oct 2019 09:18:36 +0000 Received: from mail-oi1-f196.google.com ([209.85.167.196]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1iMqJE-0001jr-EL for linux-arm-kernel@lists.infradead.org; Tue, 22 Oct 2019 09:18:34 +0000 Received: by mail-oi1-f196.google.com with SMTP id s71so695193oih.11 for ; Tue, 22 Oct 2019 02:18:31 -0700 (PDT) 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=nuz84DmbVNqkBwZM2dSH7bNPNbMKWhjIIPtW7H0YPbs=; b=SGX7Bm1VRzgNzEFYU6DY+4NDIzyzpsigW8C02i6XT5s5MZoU4Fmi8rULma0B4YIQJm xy3xS8zEhfiJS8rV41IbqVasgczPtBuzFcEh1AZVu16Ob1d2Alj6UOoj5sLkaWymXrvG LhEtZ4UYflGCokmmuu11X/Py5mxGF45SScbt+RbuU2ObCTk3tQm1CH180PVPtGepOIai UOolgzzlnTgVyfONrQMRiSLzMW1xmyHiv5Iu7nhWnA13w5M1ivm3SaZzgr2fmGj+878D AIxe7+9nRwxg4ya7e8UmrJiAa2ddae4dpjh029ZQ+95BePaJin4ycVmcClpec3KGBKPF u4QQ== X-Gm-Message-State: APjAAAWQfUrr1xRu0k5tyTyFFpve61lVlrTawyoM8AJyE8uLnCRp/P4r 2dKkooTZTvY2u7GXn2xjtKQxPMg3pnhRPL/3e+s= X-Google-Smtp-Source: APXvYqwwDlsgGkAn9HrCd8VV+9jpvEy0YfjvtOplz9BjtCnFCm2kwmx5+01G/UWl5wByWCylJuSaEQxiHp/YzPN/6bE= X-Received: by 2002:aca:b6c5:: with SMTP id g188mr2084579oif.103.1571735910539; Tue, 22 Oct 2019 02:18:30 -0700 (PDT) MIME-Version: 1.0 References: <20191022075123.17057-1-ran.wang_1@nxp.com> <20191022075123.17057-3-ran.wang_1@nxp.com> In-Reply-To: <20191022075123.17057-3-ran.wang_1@nxp.com> From: "Rafael J. Wysocki" Date: Tue, 22 Oct 2019 11:18:18 +0200 Message-ID: Subject: Re: [PATCH 3/3] soc: fsl: add RCPM driver To: Ran Wang X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20191022_021832_486619_3D97B306 X-CRM114-Status: GOOD ( 32.31 ) 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 , Li Biwen , Huang Anson , Len Brown , Greg Kroah-Hartman , Linux PM , "Rafael J . Wysocki" , Linux Kernel Mailing List , Li Yang , "devicetree@vger.kernel.org" , Rob Herring , Pavel Machek , linuxppc-dev , 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 Tue, Oct 22, 2019 at 9:52 AM Ran Wang wrote: > > The NXP's QorIQ Processors based on ARM Core have RCPM module > (Run Control and Power Management), which performs system level > tasks associated with power management such as wakeup source control. > > This driver depends on PM wakeup source framework which help to > collect wake information. > > Signed-off-by: Ran Wang > --- > Change in v8: > - Adjust related API usage to meet wakeup.c's update in patch 1/3. > - Add sanity checking for the case of ws->dev or ws->dev->parent > is null. > > Change in v7: > - Replace 'ws->dev' with 'ws->dev->parent' to get aligned with > c8377adfa781 ("PM / wakeup: Show wakeup sources stats in sysfs") > - Remove '+obj-y += ftm_alarm.o' since it is wrong. > - Cosmetic work. > > Change in v6: > - Adjust related API usage to meet wakeup.c's update in patch 1/3. > > Change in v5: > - Fix v4 regression of the return value of wakeup_source_get_next() > didn't pass to ws in while loop. > - Rename wakeup_source member 'attached_dev' to 'dev'. > - Rename property 'fsl,#rcpm-wakeup-cells' to '#fsl,rcpm-wakeup-cells'. > please see https://lore.kernel.org/patchwork/patch/1101022/ > > Change in v4: > - Remove extra ',' in author line of rcpm.c > - Update usage of wakeup_source_get_next() to be less confusing to the > reader, code logic remain the same. > > Change in v3: > - Some whitespace ajdustment. > > Change in v2: > - Rebase Kconfig and Makefile update to latest mainline. > > drivers/soc/fsl/Kconfig | 8 +++ > drivers/soc/fsl/Makefile | 1 + > drivers/soc/fsl/rcpm.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 142 insertions(+) > create mode 100644 drivers/soc/fsl/rcpm.c > > diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig > index f9ad8ad..4918856 100644 > --- a/drivers/soc/fsl/Kconfig > +++ b/drivers/soc/fsl/Kconfig > @@ -40,4 +40,12 @@ config DPAA2_CONSOLE > /dev/dpaa2_mc_console and /dev/dpaa2_aiop_console, > which can be used to dump the Management Complex and AIOP > firmware logs. > + > +config FSL_RCPM > + bool "Freescale RCPM support" > + depends on PM_SLEEP > + help > + The NXP QorIQ Processors based on ARM Core have RCPM module > + (Run Control and Power Management), which performs all device-level > + tasks associated with power management, such as wakeup source control. > endmenu > diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile > index 71dee8d..906f1cd 100644 > --- a/drivers/soc/fsl/Makefile > +++ b/drivers/soc/fsl/Makefile > @@ -6,6 +6,7 @@ > obj-$(CONFIG_FSL_DPAA) += qbman/ > obj-$(CONFIG_QUICC_ENGINE) += qe/ > obj-$(CONFIG_CPM) += qe/ > +obj-$(CONFIG_FSL_RCPM) += rcpm.o > obj-$(CONFIG_FSL_GUTS) += guts.o > obj-$(CONFIG_FSL_MC_DPIO) += dpio/ > obj-$(CONFIG_DPAA2_CONSOLE) += dpaa2-console.o > diff --git a/drivers/soc/fsl/rcpm.c b/drivers/soc/fsl/rcpm.c > new file mode 100644 > index 0000000..3ed135e > --- /dev/null > +++ b/drivers/soc/fsl/rcpm.c > @@ -0,0 +1,133 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// rcpm.c - Freescale QorIQ RCPM driver > +// > +// Copyright 2019 NXP > +// > +// Author: Ran Wang > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define RCPM_WAKEUP_CELL_MAX_SIZE 7 > + > +struct rcpm { > + unsigned int wakeup_cells; > + void __iomem *ippdexpcr_base; > + bool little_endian; > +}; > + Please add a kerneldoc comment describing this routine. > +static int rcpm_pm_prepare(struct device *dev) > +{ > + int i, ret, idx; > + void __iomem *base; > + struct wakeup_source *ws; > + struct rcpm *rcpm; > + struct device_node *np = dev->of_node; > + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1], tmp; > + > + rcpm = dev_get_drvdata(dev); > + if (!rcpm) > + return -EINVAL; > + > + base = rcpm->ippdexpcr_base; > + idx = wakeup_sources_read_lock(); > + > + /* Begin with first registered wakeup source */ > + for_each_wakeup_source(ws) { > + > + /* skip object which is not attached to device */ > + if (!ws->dev || !ws->dev->parent) > + continue; > + > + ret = device_property_read_u32_array(ws->dev->parent, > + "fsl,rcpm-wakeup", value, > + rcpm->wakeup_cells + 1); > + > + /* Wakeup source should refer to current rcpm device */ > + if (ret || (np->phandle != value[0])) { > + dev_info(dev, "%s doesn't refer to this rcpm\n", > + ws->name); IMO printing this message is not useful in general, because it looks like you just want to skip wakeup sources that aren't associated with rcpm devices. Maybe use pr_debug() to print it? Or maybe use pr_debug() to print a message if you have found a suitable device? Wouldn't that be more useful? > + continue; > + } > + It would be good to add a comment explaining what the code below does here. Or explain that in the function's kerneldoc comment. > + for (i = 0; i < rcpm->wakeup_cells; i++) { It looks like 'tmp' can be defined in this block. And I would store the value of value[i+1] in tmp upfront, that is u32 tmp = value[i+1]; > + /* We can only OR related bits */ > + if (value[i + 1]) { Also I would do if (!tmp) continue; to reduce the indentation level. > + if (rcpm->little_endian) { > + tmp = ioread32(base + i * 4); > + tmp |= value[i + 1]; > + iowrite32(tmp, base + i * 4); So it is sufficient to do tmp |= ioread32(base + i * 4); iowrite32(tmp, base + i * 4); here and analogously below. You may as well define void __iomem *address = base + i * 4; and use it everywhere in this block instead of the (base + I * 4) expression. > + } else { > + tmp = ioread32be(base + i * 4); > + tmp |= value[i + 1]; > + iowrite32be(tmp, base + i * 4); > + } > + } > + } > + } > + > + wakeup_sources_read_unlock(idx); > + > + return 0; > +} > + > +static const struct dev_pm_ops rcpm_pm_ops = { > + .prepare = rcpm_pm_prepare, > +}; > + _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel