From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756119AbcHCEda (ORCPT ); Wed, 3 Aug 2016 00:33:30 -0400 Received: from mail-ua0-f182.google.com ([209.85.217.182]:34348 "EHLO mail-ua0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755109AbcHCEdJ (ORCPT ); Wed, 3 Aug 2016 00:33:09 -0400 MIME-Version: 1.0 In-Reply-To: <2024386.Ra8uvttlE9@wuerfel> References: <1470044943-3814-1-git-send-email-chenhui.zhao@nxp.com> <1470044943-3814-2-git-send-email-chenhui.zhao@nxp.com> <2024386.Ra8uvttlE9@wuerfel> From: Chenhui Zhao Date: Wed, 3 Aug 2016 11:42:23 +0800 Message-ID: Subject: Re: [PATCH 2/2] soc: nxp: Add a RCPM driver To: Arnd Bergmann Cc: linux-arm-kernel@lists.infradead.org, Chenhui Zhao , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, jason.jin@nxp.com, Thomas Gleixner , Jason Cooper , Marc Zyngier Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Aug 1, 2016 at 8:25 PM, Arnd Bergmann wrote: > On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote: >> The NXP's QorIQ Processors based on ARM Core have a RCPM module >> (Run Control and Power Management), which performs all device-level >> tasks associated with power management. >> >> This patch mainly implements the wakeup sources configuration before >> entering LPM20, a low power state of device-level. The devices can be >> waked up by specified sources, such as Flextimer, GPIO and so on. >> >> Signed-off-by: Chenhui Zhao > > Adding irqchip maintainers to cc, as this wakeup handling is normally > part of the irq controller. > >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* So far there are not more than two registers */ >> +#define RCPM_IPPDEXPCR0 0x140 >> +#define RCPM_IPPDEXPCR1 0x144 >> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x) >> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2 >> + >> +/* it reprents the number of the registers RCPM_IPPDEXPCR */ >> +static unsigned int rcpm_wakeup_cells; >> +static void __iomem *rcpm_reg_base; >> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE]; > > Can you make these local to the context of whoever > calls into the driver? > > >> +static void rcpm_wakeup_fixup(struct device *dev, void *data) >> +{ >> + struct device_node *node = dev ? dev->of_node : NULL; >> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1]; >> + int ret; >> + int i; >> + >> + if (!dev || !node || !device_may_wakeup(dev)) >> + return; >> + >> + /* >> + * Get the values in the "rcpm-wakeup" property. >> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt >> + */ >> + ret = of_property_read_u32_array(node, "rcpm-wakeup",c >> + value, rcpm_wakeup_cells + 1); > > My first impression is that you are trying to do something > in a platform specific way that should be handled by common > code here. > > You are parsing rcpm_wakeup_cells once for the global node, > but you don't check whether the device that has the rcpm-wakeup > node actually refers to this instance, and that would require > an incompatible change if we ever get an implementation that > has multiple such nodes. > > Arnd The code is specific to the QorIQ platform. For a given QorIQ SoC, the value of the "fsl,#rcpm-wakeup-cells" property would not change. Anyway, your suggestion is better getting the rcpm node from the "rcpm-wakeup" property. Thanks, Chenhui From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chenhui Zhao Subject: Re: [PATCH 2/2] soc: nxp: Add a RCPM driver Date: Wed, 3 Aug 2016 11:42:23 +0800 Message-ID: References: <1470044943-3814-1-git-send-email-chenhui.zhao@nxp.com> <1470044943-3814-2-git-send-email-chenhui.zhao@nxp.com> <2024386.Ra8uvttlE9@wuerfel> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <2024386.Ra8uvttlE9@wuerfel> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Arnd Bergmann Cc: devicetree@vger.kernel.org, Chenhui Zhao , jason.jin@nxp.com, Marc Zyngier , linux-kernel@vger.kernel.org, Thomas Gleixner , linux-arm-kernel@lists.infradead.org, Jason Cooper List-Id: devicetree@vger.kernel.org On Mon, Aug 1, 2016 at 8:25 PM, Arnd Bergmann wrote: > On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote: >> The NXP's QorIQ Processors based on ARM Core have a RCPM module >> (Run Control and Power Management), which performs all device-level >> tasks associated with power management. >> >> This patch mainly implements the wakeup sources configuration before >> entering LPM20, a low power state of device-level. The devices can be >> waked up by specified sources, such as Flextimer, GPIO and so on. >> >> Signed-off-by: Chenhui Zhao > > Adding irqchip maintainers to cc, as this wakeup handling is normally > part of the irq controller. > >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* So far there are not more than two registers */ >> +#define RCPM_IPPDEXPCR0 0x140 >> +#define RCPM_IPPDEXPCR1 0x144 >> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x) >> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2 >> + >> +/* it reprents the number of the registers RCPM_IPPDEXPCR */ >> +static unsigned int rcpm_wakeup_cells; >> +static void __iomem *rcpm_reg_base; >> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE]; > > Can you make these local to the context of whoever > calls into the driver? > > >> +static void rcpm_wakeup_fixup(struct device *dev, void *data) >> +{ >> + struct device_node *node = dev ? dev->of_node : NULL; >> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1]; >> + int ret; >> + int i; >> + >> + if (!dev || !node || !device_may_wakeup(dev)) >> + return; >> + >> + /* >> + * Get the values in the "rcpm-wakeup" property. >> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt >> + */ >> + ret = of_property_read_u32_array(node, "rcpm-wakeup",c >> + value, rcpm_wakeup_cells + 1); > > My first impression is that you are trying to do something > in a platform specific way that should be handled by common > code here. > > You are parsing rcpm_wakeup_cells once for the global node, > but you don't check whether the device that has the rcpm-wakeup > node actually refers to this instance, and that would require > an incompatible change if we ever get an implementation that > has multiple such nodes. > > Arnd The code is specific to the QorIQ platform. For a given QorIQ SoC, the value of the "fsl,#rcpm-wakeup-cells" property would not change. Anyway, your suggestion is better getting the rcpm node from the "rcpm-wakeup" property. Thanks, Chenhui From mboxrd@z Thu Jan 1 00:00:00 1970 From: z.chenhui@gmail.com (Chenhui Zhao) Date: Wed, 3 Aug 2016 11:42:23 +0800 Subject: [PATCH 2/2] soc: nxp: Add a RCPM driver In-Reply-To: <2024386.Ra8uvttlE9@wuerfel> References: <1470044943-3814-1-git-send-email-chenhui.zhao@nxp.com> <1470044943-3814-2-git-send-email-chenhui.zhao@nxp.com> <2024386.Ra8uvttlE9@wuerfel> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, Aug 1, 2016 at 8:25 PM, Arnd Bergmann wrote: > On Monday, August 1, 2016 5:49:03 PM CEST Chenhui Zhao wrote: >> The NXP's QorIQ Processors based on ARM Core have a RCPM module >> (Run Control and Power Management), which performs all device-level >> tasks associated with power management. >> >> This patch mainly implements the wakeup sources configuration before >> entering LPM20, a low power state of device-level. The devices can be >> waked up by specified sources, such as Flextimer, GPIO and so on. >> >> Signed-off-by: Chenhui Zhao > > Adding irqchip maintainers to cc, as this wakeup handling is normally > part of the irq controller. > >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +/* So far there are not more than two registers */ >> +#define RCPM_IPPDEXPCR0 0x140 >> +#define RCPM_IPPDEXPCR1 0x144 >> +#define RCPM_IPPDEXPCR(x) (RCPM_IPPDEXPCR0 + 4 * x) >> +#define RCPM_WAKEUP_CELL_MAX_SIZE 2 >> + >> +/* it reprents the number of the registers RCPM_IPPDEXPCR */ >> +static unsigned int rcpm_wakeup_cells; >> +static void __iomem *rcpm_reg_base; >> +static u32 ippdexpcr[RCPM_WAKEUP_CELL_MAX_SIZE]; > > Can you make these local to the context of whoever > calls into the driver? > > >> +static void rcpm_wakeup_fixup(struct device *dev, void *data) >> +{ >> + struct device_node *node = dev ? dev->of_node : NULL; >> + u32 value[RCPM_WAKEUP_CELL_MAX_SIZE + 1]; >> + int ret; >> + int i; >> + >> + if (!dev || !node || !device_may_wakeup(dev)) >> + return; >> + >> + /* >> + * Get the values in the "rcpm-wakeup" property. >> + * Refer to Documentation/devicetree/bindings/soc/fsl/rcpm.txt >> + */ >> + ret = of_property_read_u32_array(node, "rcpm-wakeup",c >> + value, rcpm_wakeup_cells + 1); > > My first impression is that you are trying to do something > in a platform specific way that should be handled by common > code here. > > You are parsing rcpm_wakeup_cells once for the global node, > but you don't check whether the device that has the rcpm-wakeup > node actually refers to this instance, and that would require > an incompatible change if we ever get an implementation that > has multiple such nodes. > > Arnd The code is specific to the QorIQ platform. For a given QorIQ SoC, the value of the "fsl,#rcpm-wakeup-cells" property would not change. Anyway, your suggestion is better getting the rcpm node from the "rcpm-wakeup" property. Thanks, Chenhui