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

From: Marc Zyngier
Date: Mon Aug 01 2016 - 09:22:38 EST


On 01/08/16 10:49, 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@xxxxxxx>
> ---
> drivers/soc/fsl/Makefile | 1 +
> drivers/soc/fsl/pm/Makefile | 1 +
> drivers/soc/fsl/pm/ls-rcpm.c | 144 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 146 insertions(+)
> create mode 100644 drivers/soc/fsl/pm/Makefile
> create mode 100644 drivers/soc/fsl/pm/ls-rcpm.c
>
> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile
> index 203307f..b5adbaf 100644
> --- a/drivers/soc/fsl/Makefile
> +++ b/drivers/soc/fsl/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_QUICC_ENGINE) += qe/
> obj-$(CONFIG_CPM) += qe/
> +obj-$(CONFIG_SUSPEND) += pm/
> diff --git a/drivers/soc/fsl/pm/Makefile b/drivers/soc/fsl/pm/Makefile
> new file mode 100644
> index 0000000..e275d63
> --- /dev/null
> +++ b/drivers/soc/fsl/pm/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ARCH_LAYERSCAPE) += ls-rcpm.o
> diff --git a/drivers/soc/fsl/pm/ls-rcpm.c b/drivers/soc/fsl/pm/ls-rcpm.c
> new file mode 100644
> index 0000000..c5f2b97
> --- /dev/null
> +++ b/drivers/soc/fsl/pm/ls-rcpm.c
> @@ -0,0 +1,144 @@
> +/*
> + * Run Control and Power Management (RCPM) driver
> + *
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#define pr_fmt(fmt) "RCPM: %s: " fmt, __func__
> +
> +#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];
> +
> +static inline void rcpm_reg_write(u32 offset, u32 value)
> +{
> + iowrite32be(value, rcpm_reg_base + offset);
> +}
> +
> +static inline u32 rcpm_reg_read(u32 offset)
> +{
> + return ioread32be(rcpm_reg_base + offset);
> +}
> +
> +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",
> + value, rcpm_wakeup_cells + 1);
> + if (ret)
> + return;
> +
> + pr_debug("wakeup source: the device %s\n", node->full_name);

This looks absolutely dreadful. You are parsing every node for each
device, and apply a set-in-stone configuration.

The normal way to handle this is by making this device an interrupt
controller, and honour the irq_set_wake API. This has already been done
on other FSL/NXP hardware (see the iMX GPC stuff).

Thanks,

M.
--
Jazz is not dead. It just smells funny...