linux-kernel - Re: [PATCH 2/2] soc: nxp: Add a RCPM driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHwnS+UOjxY4Vj37P2zsQJEjzqVzK5LeTOKJ9KeT26kLqgLjUg@mail.gmail.com>
Date:	Wed, 3 Aug 2016 11:42:23 +0800
From:	Chenhui Zhao <z.chenhui@...il.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	linux-arm-kernel@...ts.infradead.org,
	Chenhui Zhao <chenhui.zhao@....com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	jason.jin@....com, Thomas Gleixner <tglx@...utronix.de>,
	Jason Cooper <jason@...edaemon.net>,
	Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH 2/2] soc: nxp: Add a RCPM driver

On Mon, Aug 1, 2016 at 8:25 PM, Arnd Bergmann <arnd@...db.de> 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 <chenhui.zhao@....com>
>
> Adding irqchip maintainers to cc, as this wakeup handling is normally
> part of the irq controller.
>
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/io.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/suspend.h>
>> +
>> +/* 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