[PATCH 0/2] irqchip: imx-intmux: add PM support

[PATCH 0/2] irqchip: imx-intmux: add PM support

[Joakim Zhang]

Add system and runtime PM support for intmux irqchip.

Joakim Zhang (2):
  irqchip: imx-intmux: add system PM support
  irqchip: imx-intmux: add runtime PM support

 drivers/irqchip/irq-imx-intmux.c | 82 +++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] irqchip: imx-intmux: add system PM support

[Joakim Zhang]

Add system PM support for intmux interrupt controller.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/irqchip/irq-imx-intmux.c | 59 ++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/irqchip/irq-imx-intmux.c b/drivers/irqchip/irq-imx-intmux.c
index c27577c81126..6095f76c4f0d 100644
--- a/drivers/irqchip/irq-imx-intmux.c
+++ b/drivers/irqchip/irq-imx-intmux.c
@@ -70,6 +70,9 @@ struct intmux_data {
 	void __iomem			*regs;
 	struct clk			*ipg_clk;
 	int				channum;
+#ifdef CONFIG_PM
+	unsigned int			*saved_reg;
+#endif
 	struct intmux_irqchip_data	irqchip_data[];
 };
 
@@ -232,6 +235,15 @@ static int imx_intmux_probe(struct platform_device *pdev)
 	data->channum = channum;
 	raw_spin_lock_init(&data->lock);
 
+	if (IS_ENABLED(CONFIG_PM)) {
+		/* save CHANIER register */
+		data->saved_reg = devm_kzalloc(&pdev->dev,
+					       sizeof(unsigned int) * channum,
+					       GFP_KERNEL);
+		if (!data->saved_reg)
+			return -ENOMEM;
+	}
+
 	ret = clk_prepare_enable(data->ipg_clk);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
@@ -293,6 +305,53 @@ static int imx_intmux_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static void imx_intmux_save_regs(struct intmux_data *data)
+{
+	int i;
+
+	for (i = 0; i < data->channum; i++)
+		data->saved_reg[i] = readl_relaxed(data->regs + CHANIER(i));
+}
+
+static void imx_intmux_restore_regs(struct intmux_data *data)
+{
+	int i;
+
+	for (i = 0; i < data->channum; i++)
+		writel_relaxed(data->saved_reg[i], data->regs + CHANIER(i));
+}
+
+static int imx_intmux_suspend(struct device *dev)
+{
+	struct intmux_data *data = dev_get_drvdata(dev);
+
+	imx_intmux_save_regs(data);
+	clk_disable_unprepare(data->ipg_clk);
+
+	return 0;
+}
+
+static int imx_intmux_resume(struct device *dev)
+{
+	struct intmux_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(data->ipg_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable ipg clk: %d\n", ret);
+		return ret;
+	}
+	imx_intmux_restore_regs(data);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops imx_intmux_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend, imx_intmux_resume)
+};
+
 static const struct of_device_id imx_intmux_id[] = {
 	{ .compatible = "fsl,imx-intmux", },
 	{ /* sentinel */ },
-- 
2.17.1

[PATCH 2/2] irqchip: imx-intmux: add runtime PM support

[Joakim Zhang]

Add runtime PM support for intmux interrupt controller.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/irqchip/irq-imx-intmux.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-imx-intmux.c b/drivers/irqchip/irq-imx-intmux.c
index 6095f76c4f0d..a631815c7bb4 100644
--- a/drivers/irqchip/irq-imx-intmux.c
+++ b/drivers/irqchip/irq-imx-intmux.c
@@ -53,6 +53,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/spinlock.h>
+#include <linux/pm_runtime.h>
 
 #define CHANIER(n)	(0x10 + (0x40 * n))
 #define CHANIPR(n)	(0x20 + (0x40 * n))
@@ -60,6 +61,7 @@
 #define CHAN_MAX_NUM		0x8
 
 struct intmux_irqchip_data {
+	struct irq_chip		chip;
 	int			chanidx;
 	int			irq;
 	struct irq_domain	*domain;
@@ -123,8 +125,10 @@ static struct irq_chip imx_intmux_irq_chip = {
 static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
 			      irq_hw_number_t hwirq)
 {
-	irq_set_chip_data(irq, h->host_data);
-	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, handle_level_irq);
+	struct intmux_irqchip_data *data = h->host_data;
+
+	irq_set_chip_data(irq, data);
+	irq_set_chip_and_handler(irq, &data->chip, handle_level_irq);
 
 	return 0;
 }
@@ -244,6 +248,10 @@ static int imx_intmux_probe(struct platform_device *pdev)
 			return -ENOMEM;
 	}
 
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	ret = clk_prepare_enable(data->ipg_clk);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
@@ -251,6 +259,8 @@ static int imx_intmux_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < channum; i++) {
+		data->irqchip_data[i].chip = imx_intmux_irq_chip;
+		data->irqchip_data[i].chip.parent_device = &pdev->dev;
 		data->irqchip_data[i].chanidx = i;
 
 		data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
@@ -279,6 +289,12 @@ static int imx_intmux_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
+	/*
+	 * Let pm_runtime_put() disable clock.
+	 * If CONFIG_PM is not enabled, the clock will stay powered.
+	 */
+	pm_runtime_put(&pdev->dev);
+
 	return 0;
 out:
 	clk_disable_unprepare(data->ipg_clk);
@@ -300,7 +316,7 @@ static int imx_intmux_remove(struct platform_device *pdev)
 		irq_domain_remove(data->irqchip_data[i].domain);
 	}
 
-	clk_disable_unprepare(data->ipg_clk);
+	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
@@ -322,7 +338,7 @@ static void imx_intmux_restore_regs(struct intmux_data *data)
 		writel_relaxed(data->saved_reg[i], data->regs + CHANIER(i));
 }
 
-static int imx_intmux_suspend(struct device *dev)
+static int imx_intmux_runtime_suspend(struct device *dev)
 {
 	struct intmux_data *data = dev_get_drvdata(dev);
 
@@ -332,7 +348,7 @@ static int imx_intmux_suspend(struct device *dev)
 	return 0;
 }
 
-static int imx_intmux_resume(struct device *dev)
+static int imx_intmux_runtime_resume(struct device *dev)
 {
 	struct intmux_data *data = dev_get_drvdata(dev);
 	int ret;
@@ -349,7 +365,10 @@ static int imx_intmux_resume(struct device *dev)
 #endif
 
 static const struct dev_pm_ops imx_intmux_pm_ops = {
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend, imx_intmux_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				      pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(imx_intmux_runtime_suspend,
+			   imx_intmux_runtime_resume, NULL)
 };
 
 static const struct of_device_id imx_intmux_id[] = {
-- 
2.17.1

Re: [PATCH 1/2] irqchip: imx-intmux: add system PM support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

Hi Joakim,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/irq/core]
[also build test WARNING on linux/master linus/master v5.8-rc5 next-20200716]
[cannot apply to arm-jcooper/irqchip/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Joakim-Zhang/irqchip-imx-intmux-add-PM-support/20200716-193311
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 8fa88a88d573093868565a1afba43b5ae5b3a316
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/irqchip/irq-imx-intmux.c:351:32: warning: unused variable 'imx_intmux_pm_ops' [-Wunused-const-variable]
   static const struct dev_pm_ops imx_intmux_pm_ops = {
                                  ^
   1 warning generated.

vim +/imx_intmux_pm_ops +351 drivers/irqchip/irq-imx-intmux.c

   350	
 > 351	static const struct dev_pm_ops imx_intmux_pm_ops = {
   352		SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend, imx_intmux_resume)
   353	};
   354	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75349 bytes --]

Re: [PATCH 1/2] irqchip: imx-intmux: add system PM support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4823 bytes --]

Hi Joakim,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/irq/core]
[also build test ERROR on linux/master linus/master v5.8-rc5 next-20200716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Joakim-Zhang/irqchip-imx-intmux-add-PM-support/20200716-193311
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 8fa88a88d573093868565a1afba43b5ae5b3a316
config: c6x-randconfig-r011-20200717 (attached as .config)
compiler: c6x-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=c6x 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/irqchip/irq-imx-intmux.c: In function 'imx_intmux_probe':
>> drivers/irqchip/irq-imx-intmux.c:240:7: error: 'struct intmux_data' has no member named 'saved_reg'
     240 |   data->saved_reg = devm_kzalloc(&pdev->dev,
         |       ^~
   drivers/irqchip/irq-imx-intmux.c:243:12: error: 'struct intmux_data' has no member named 'saved_reg'
     243 |   if (!data->saved_reg)
         |            ^~
   At top level:
   drivers/irqchip/irq-imx-intmux.c:351:32: warning: 'imx_intmux_pm_ops' defined but not used [-Wunused-const-variable=]
     351 | static const struct dev_pm_ops imx_intmux_pm_ops = {
         |                                ^~~~~~~~~~~~~~~~~

vim +240 drivers/irqchip/irq-imx-intmux.c

   198	
   199	static int imx_intmux_probe(struct platform_device *pdev)
   200	{
   201		struct device_node *np = pdev->dev.of_node;
   202		struct irq_domain *domain;
   203		struct intmux_data *data;
   204		int channum;
   205		int i, ret;
   206	
   207		channum = platform_irq_count(pdev);
   208		if (channum == -EPROBE_DEFER) {
   209			return -EPROBE_DEFER;
   210		} else if (channum > CHAN_MAX_NUM) {
   211			dev_err(&pdev->dev, "supports up to %d multiplex channels\n",
   212				CHAN_MAX_NUM);
   213			return -EINVAL;
   214		}
   215	
   216		data = devm_kzalloc(&pdev->dev, sizeof(*data) +
   217				    channum * sizeof(data->irqchip_data[0]), GFP_KERNEL);
   218		if (!data)
   219			return -ENOMEM;
   220	
   221		data->regs = devm_platform_ioremap_resource(pdev, 0);
   222		if (IS_ERR(data->regs)) {
   223			dev_err(&pdev->dev, "failed to initialize reg\n");
   224			return PTR_ERR(data->regs);
   225		}
   226	
   227		data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
   228		if (IS_ERR(data->ipg_clk)) {
   229			ret = PTR_ERR(data->ipg_clk);
   230			if (ret != -EPROBE_DEFER)
   231				dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
   232			return ret;
   233		}
   234	
   235		data->channum = channum;
   236		raw_spin_lock_init(&data->lock);
   237	
   238		if (IS_ENABLED(CONFIG_PM)) {
   239			/* save CHANIER register */
 > 240			data->saved_reg = devm_kzalloc(&pdev->dev,
   241						       sizeof(unsigned int) * channum,
   242						       GFP_KERNEL);
   243			if (!data->saved_reg)
   244				return -ENOMEM;
   245		}
   246	
   247		ret = clk_prepare_enable(data->ipg_clk);
   248		if (ret) {
   249			dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
   250			return ret;
   251		}
   252	
   253		for (i = 0; i < channum; i++) {
   254			data->irqchip_data[i].chanidx = i;
   255	
   256			data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
   257			if (data->irqchip_data[i].irq <= 0) {
   258				ret = -EINVAL;
   259				dev_err(&pdev->dev, "failed to get irq\n");
   260				goto out;
   261			}
   262	
   263			domain = irq_domain_add_linear(np, 32, &imx_intmux_domain_ops,
   264						       &data->irqchip_data[i]);
   265			if (!domain) {
   266				ret = -ENOMEM;
   267				dev_err(&pdev->dev, "failed to create IRQ domain\n");
   268				goto out;
   269			}
   270			data->irqchip_data[i].domain = domain;
   271	
   272			/* disable all interrupt sources of this channel firstly */
   273			writel_relaxed(0, data->regs + CHANIER(i));
   274	
   275			irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
   276							 imx_intmux_irq_handler,
   277							 &data->irqchip_data[i]);
   278		}
   279	
   280		platform_set_drvdata(pdev, data);
   281	
   282		return 0;
   283	out:
   284		clk_disable_unprepare(data->ipg_clk);
   285		return ret;
   286	}
   287	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27604 bytes --]

Re: [PATCH 1/2] irqchip: imx-intmux: add system PM support

[Marc Zyngier]

On 2020-07-16 20:32, Joakim Zhang wrote:
> Add system PM support for intmux interrupt controller.

Care to be a little more descriptive?

> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/irqchip/irq-imx-intmux.c | 59 ++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-imx-intmux.c 
> b/drivers/irqchip/irq-imx-intmux.c
> index c27577c81126..6095f76c4f0d 100644
> --- a/drivers/irqchip/irq-imx-intmux.c
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -70,6 +70,9 @@ struct intmux_data {
>  	void __iomem			*regs;
>  	struct clk			*ipg_clk;
>  	int				channum;
> +#ifdef CONFIG_PM
> +	unsigned int			*saved_reg;

Please use u32 for 32bit HW registers.

> +#endif
>  	struct intmux_irqchip_data	irqchip_data[];
>  };
> 
> @@ -232,6 +235,15 @@ static int imx_intmux_probe(struct platform_device 
> *pdev)
>  	data->channum = channum;
>  	raw_spin_lock_init(&data->lock);
> 
> +	if (IS_ENABLED(CONFIG_PM)) {
> +		/* save CHANIER register */
> +		data->saved_reg = devm_kzalloc(&pdev->dev,
> +					       sizeof(unsigned int) * channum,
> +					       GFP_KERNEL);
> +		if (!data->saved_reg)

Which isn't defined when !CONFIG_PM. The compiler is allowed to
bail here, and does (see the two kbuild failures).

> +			return -ENOMEM;
> +	}
> +
>  	ret = clk_prepare_enable(data->ipg_clk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> @@ -293,6 +305,53 @@ static int imx_intmux_remove(struct 
> platform_device *pdev)
>  	return 0;
>  }
> 
> +#ifdef CONFIG_PM
> +static void imx_intmux_save_regs(struct intmux_data *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < data->channum; i++)
> +		data->saved_reg[i] = readl_relaxed(data->regs + CHANIER(i));
> +}
> +
> +static void imx_intmux_restore_regs(struct intmux_data *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < data->channum; i++)
> +		writel_relaxed(data->saved_reg[i], data->regs + CHANIER(i));
> +}

Please move these two trivial functions into their respective callers.

> +
> +static int imx_intmux_suspend(struct device *dev)
> +{
> +	struct intmux_data *data = dev_get_drvdata(dev);
> +
> +	imx_intmux_save_regs(data);
> +	clk_disable_unprepare(data->ipg_clk);
> +
> +	return 0;
> +}
> +
> +static int imx_intmux_resume(struct device *dev)
> +{
> +	struct intmux_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(data->ipg_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable ipg clk: %d\n", ret);
> +		return ret;
> +	}
> +	imx_intmux_restore_regs(data);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops imx_intmux_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend, imx_intmux_resume)
> +};
> +
>  static const struct of_device_id imx_intmux_id[] = {
>  	{ .compatible = "fsl,imx-intmux", },
>  	{ /* sentinel */ },

Thanks,

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

Re: [PATCH 2/2] irqchip: imx-intmux: add runtime PM support

[Marc Zyngier]

On 2020-07-16 20:32, Joakim Zhang wrote:
> Add runtime PM support for intmux interrupt controller.

Same as the previous patch. The changes are significant enough
that you need to write a proper change log.

> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/irqchip/irq-imx-intmux.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-imx-intmux.c 
> b/drivers/irqchip/irq-imx-intmux.c
> index 6095f76c4f0d..a631815c7bb4 100644
> --- a/drivers/irqchip/irq-imx-intmux.c
> +++ b/drivers/irqchip/irq-imx-intmux.c
> @@ -53,6 +53,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/spinlock.h>
> +#include <linux/pm_runtime.h>
> 
>  #define CHANIER(n)	(0x10 + (0x40 * n))
>  #define CHANIPR(n)	(0x20 + (0x40 * n))
> @@ -60,6 +61,7 @@
>  #define CHAN_MAX_NUM		0x8
> 
>  struct intmux_irqchip_data {
> +	struct irq_chip		chip;
>  	int			chanidx;
>  	int			irq;
>  	struct irq_domain	*domain;
> @@ -123,8 +125,10 @@ static struct irq_chip imx_intmux_irq_chip = {
>  static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
>  			      irq_hw_number_t hwirq)
>  {
> -	irq_set_chip_data(irq, h->host_data);
> -	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, 
> handle_level_irq);
> +	struct intmux_irqchip_data *data = h->host_data;
> +
> +	irq_set_chip_data(irq, data);
> +	irq_set_chip_and_handler(irq, &data->chip, handle_level_irq);
> 
>  	return 0;
>  }
> @@ -244,6 +248,10 @@ static int imx_intmux_probe(struct platform_device 
> *pdev)
>  			return -ENOMEM;
>  	}
> 
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
>  	ret = clk_prepare_enable(data->ipg_clk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> @@ -251,6 +259,8 @@ static int imx_intmux_probe(struct platform_device 
> *pdev)
>  	}
> 
>  	for (i = 0; i < channum; i++) {
> +		data->irqchip_data[i].chip = imx_intmux_irq_chip;
> +		data->irqchip_data[i].chip.parent_device = &pdev->dev;

At some point, we will have to find a way to throw the parent_device
thing out of the irq_chip structure. This thing is a liability.
Nothing you can do about it for now though.

>  		data->irqchip_data[i].chanidx = i;
> 
>  		data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
> @@ -279,6 +289,12 @@ static int imx_intmux_probe(struct platform_device 
> *pdev)
> 
>  	platform_set_drvdata(pdev, data);
> 
> +	/*
> +	 * Let pm_runtime_put() disable clock.
> +	 * If CONFIG_PM is not enabled, the clock will stay powered.
> +	 */
> +	pm_runtime_put(&pdev->dev);
> +
>  	return 0;
>  out:
>  	clk_disable_unprepare(data->ipg_clk);
> @@ -300,7 +316,7 @@ static int imx_intmux_remove(struct platform_device 
> *pdev)
>  		irq_domain_remove(data->irqchip_data[i].domain);
>  	}
> 
> -	clk_disable_unprepare(data->ipg_clk);
> +	pm_runtime_disable(&pdev->dev);
> 
>  	return 0;
>  }
> @@ -322,7 +338,7 @@ static void imx_intmux_restore_regs(struct
> intmux_data *data)
>  		writel_relaxed(data->saved_reg[i], data->regs + CHANIER(i));
>  }
> 
> -static int imx_intmux_suspend(struct device *dev)
> +static int imx_intmux_runtime_suspend(struct device *dev)
>  {
>  	struct intmux_data *data = dev_get_drvdata(dev);
> 
> @@ -332,7 +348,7 @@ static int imx_intmux_suspend(struct device *dev)
>  	return 0;
>  }
> 
> -static int imx_intmux_resume(struct device *dev)
> +static int imx_intmux_runtime_resume(struct device *dev)

You just introduced these two functions, and rename them immediately?

>  {
>  	struct intmux_data *data = dev_get_drvdata(dev);
>  	int ret;
> @@ -349,7 +365,10 @@ static int imx_intmux_resume(struct device *dev)
>  #endif
> 
>  static const struct dev_pm_ops imx_intmux_pm_ops = {
> -	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend, imx_intmux_resume)
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				      pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(imx_intmux_runtime_suspend,
> +			   imx_intmux_runtime_resume, NULL)
>  };
> 
>  static const struct of_device_id imx_intmux_id[] = {

I think you'd might as well squash the two patches together.
Splitting the two serves little purpose.

Thanks,

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

Re: [PATCH 1/2] irqchip: imx-intmux: add system PM support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11476 bytes --]

Hi Joakim,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/irq/core]
[also build test ERROR on linux/master linus/master v5.8-rc5 next-20200716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Joakim-Zhang/irqchip-imx-intmux-add-PM-support/20200716-193311
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 8fa88a88d573093868565a1afba43b5ae5b3a316
config: s390-randconfig-r014-20200717 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:19:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x000000ffUL) << 24) |            \
                     ^
   In file included from drivers/irqchip/irq-imx-intmux.c:49:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |            \
                     ^
   In file included from drivers/irqchip/irq-imx-intmux.c:49:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |            \
                     ^
   In file included from drivers/irqchip/irq-imx-intmux.c:49:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32'
           ___constant_swab32(x) :                 \
                              ^
   include/uapi/linux/swab.h:22:12: note: expanded from macro '___constant_swab32'
           (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
                     ^
   In file included from drivers/irqchip/irq-imx-intmux.c:49:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:490:45: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu(__raw_readl(PCI_IOBASE + addr));
                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:120:12: note: expanded from macro '__swab32'
           __fswab32(x))
                     ^
   In file included from drivers/irqchip/irq-imx-intmux.c:49:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:72:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew(cpu_to_le16(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:46: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel(cpu_to_le32(value), PCI_IOBASE + addr);
                                            ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/irqchip/irq-imx-intmux.c:240:9: error: no member named 'saved_reg' in 'struct intmux_data'
                   data->saved_reg = devm_kzalloc(&pdev->dev,
                   ~~~~  ^
   drivers/irqchip/irq-imx-intmux.c:243:14: error: no member named 'saved_reg' in 'struct intmux_data'
                   if (!data->saved_reg)
                        ~~~~  ^
   20 warnings and 2 errors generated.

vim +240 drivers/irqchip/irq-imx-intmux.c

   198	
   199	static int imx_intmux_probe(struct platform_device *pdev)
   200	{
   201		struct device_node *np = pdev->dev.of_node;
   202		struct irq_domain *domain;
   203		struct intmux_data *data;
   204		int channum;
   205		int i, ret;
   206	
   207		channum = platform_irq_count(pdev);
   208		if (channum == -EPROBE_DEFER) {
   209			return -EPROBE_DEFER;
   210		} else if (channum > CHAN_MAX_NUM) {
   211			dev_err(&pdev->dev, "supports up to %d multiplex channels\n",
   212				CHAN_MAX_NUM);
   213			return -EINVAL;
   214		}
   215	
   216		data = devm_kzalloc(&pdev->dev, sizeof(*data) +
   217				    channum * sizeof(data->irqchip_data[0]), GFP_KERNEL);
   218		if (!data)
   219			return -ENOMEM;
   220	
   221		data->regs = devm_platform_ioremap_resource(pdev, 0);
   222		if (IS_ERR(data->regs)) {
   223			dev_err(&pdev->dev, "failed to initialize reg\n");
   224			return PTR_ERR(data->regs);
   225		}
   226	
   227		data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
   228		if (IS_ERR(data->ipg_clk)) {
   229			ret = PTR_ERR(data->ipg_clk);
   230			if (ret != -EPROBE_DEFER)
   231				dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
   232			return ret;
   233		}
   234	
   235		data->channum = channum;
   236		raw_spin_lock_init(&data->lock);
   237	
   238		if (IS_ENABLED(CONFIG_PM)) {
   239			/* save CHANIER register */
 > 240			data->saved_reg = devm_kzalloc(&pdev->dev,
   241						       sizeof(unsigned int) * channum,
   242						       GFP_KERNEL);
   243			if (!data->saved_reg)
   244				return -ENOMEM;
   245		}
   246	
   247		ret = clk_prepare_enable(data->ipg_clk);
   248		if (ret) {
   249			dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
   250			return ret;
   251		}
   252	
   253		for (i = 0; i < channum; i++) {
   254			data->irqchip_data[i].chanidx = i;
   255	
   256			data->irqchip_data[i].irq = irq_of_parse_and_map(np, i);
   257			if (data->irqchip_data[i].irq <= 0) {
   258				ret = -EINVAL;
   259				dev_err(&pdev->dev, "failed to get irq\n");
   260				goto out;
   261			}
   262	
   263			domain = irq_domain_add_linear(np, 32, &imx_intmux_domain_ops,
   264						       &data->irqchip_data[i]);
   265			if (!domain) {
   266				ret = -ENOMEM;
   267				dev_err(&pdev->dev, "failed to create IRQ domain\n");
   268				goto out;
   269			}
   270			data->irqchip_data[i].domain = domain;
   271	
   272			/* disable all interrupt sources of this channel firstly */
   273			writel_relaxed(0, data->regs + CHANIER(i));
   274	
   275			irq_set_chained_handler_and_data(data->irqchip_data[i].irq,
   276							 imx_intmux_irq_handler,
   277							 &data->irqchip_data[i]);
   278		}
   279	
   280		platform_set_drvdata(pdev, data);
   281	
   282		return 0;
   283	out:
   284		clk_disable_unprepare(data->ipg_clk);
   285		return ret;
   286	}
   287	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35935 bytes --]

RE: [PATCH 2/2] irqchip: imx-intmux: add runtime PM support

[Joakim Zhang]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2020年7月17日 16:58
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.or
> Subject: Re: [PATCH 2/2] irqchip: imx-intmux: add runtime PM support
> 
> On 2020-07-16 20:32, Joakim Zhang wrote:
> > Add runtime PM support for intmux interrupt controller.
> 
> Same as the previous patch. The changes are significant enough that you need
> to write a proper change log.
> 
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/irqchip/irq-imx-intmux.c | 31 +++++++++++++++++++++++++------
> >  1 file changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-intmux.c
> > b/drivers/irqchip/irq-imx-intmux.c
> > index 6095f76c4f0d..a631815c7bb4 100644
> > --- a/drivers/irqchip/irq-imx-intmux.c
> > +++ b/drivers/irqchip/irq-imx-intmux.c
> > @@ -53,6 +53,7 @@
> >  #include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #define CHANIER(n)	(0x10 + (0x40 * n))
> >  #define CHANIPR(n)	(0x20 + (0x40 * n))
> > @@ -60,6 +61,7 @@
> >  #define CHAN_MAX_NUM		0x8
> >
> >  struct intmux_irqchip_data {
> > +	struct irq_chip		chip;
> >  	int			chanidx;
> >  	int			irq;
> >  	struct irq_domain	*domain;
> > @@ -123,8 +125,10 @@ static struct irq_chip imx_intmux_irq_chip = {
> > static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
> >  			      irq_hw_number_t hwirq)
> >  {
> > -	irq_set_chip_data(irq, h->host_data);
> > -	irq_set_chip_and_handler(irq, &imx_intmux_irq_chip,
> > handle_level_irq);
> > +	struct intmux_irqchip_data *data = h->host_data;
> > +
> > +	irq_set_chip_data(irq, data);
> > +	irq_set_chip_and_handler(irq, &data->chip, handle_level_irq);
> >
> >  	return 0;
> >  }
> > @@ -244,6 +248,10 @@ static int imx_intmux_probe(struct
> > platform_device
> > *pdev)
> >  			return -ENOMEM;
> >  	}
> >
> > +	pm_runtime_get_noresume(&pdev->dev);
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +
> >  	ret = clk_prepare_enable(data->ipg_clk);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret); @@
> > -251,6 +259,8 @@ static int imx_intmux_probe(struct platform_device
> > *pdev)
> >  	}
> >
> >  	for (i = 0; i < channum; i++) {
> > +		data->irqchip_data[i].chip = imx_intmux_irq_chip;
> > +		data->irqchip_data[i].chip.parent_device = &pdev->dev;
> 
> At some point, we will have to find a way to throw the parent_device thing out
> of the irq_chip structure. This thing is a liability.
> Nothing you can do about it for now though.
> 
> >  		data->irqchip_data[i].chanidx = i;
> >
> >  		data->irqchip_data[i].irq = irq_of_parse_and_map(np, i); @@ -279,6
> > +289,12 @@ static int imx_intmux_probe(struct platform_device
> > *pdev)
> >
> >  	platform_set_drvdata(pdev, data);
> >
> > +	/*
> > +	 * Let pm_runtime_put() disable clock.
> > +	 * If CONFIG_PM is not enabled, the clock will stay powered.
> > +	 */
> > +	pm_runtime_put(&pdev->dev);
> > +
> >  	return 0;
> >  out:
> >  	clk_disable_unprepare(data->ipg_clk);
> > @@ -300,7 +316,7 @@ static int imx_intmux_remove(struct
> > platform_device
> > *pdev)
> >  		irq_domain_remove(data->irqchip_data[i].domain);
> >  	}
> >
> > -	clk_disable_unprepare(data->ipg_clk);
> > +	pm_runtime_disable(&pdev->dev);
> >
> >  	return 0;
> >  }
> > @@ -322,7 +338,7 @@ static void imx_intmux_restore_regs(struct
> > intmux_data *data)
> >  		writel_relaxed(data->saved_reg[i], data->regs + CHANIER(i));  }
> >
> > -static int imx_intmux_suspend(struct device *dev)
> > +static int imx_intmux_runtime_suspend(struct device *dev)
> >  {
> >  	struct intmux_data *data = dev_get_drvdata(dev);
> >
> > @@ -332,7 +348,7 @@ static int imx_intmux_suspend(struct device *dev)
> >  	return 0;
> >  }
> >
> > -static int imx_intmux_resume(struct device *dev)
> > +static int imx_intmux_runtime_resume(struct device *dev)
> 
> You just introduced these two functions, and rename them immediately?
> 
> >  {
> >  	struct intmux_data *data = dev_get_drvdata(dev);
> >  	int ret;
> > @@ -349,7 +365,10 @@ static int imx_intmux_resume(struct device *dev)
> > #endif
> >
> >  static const struct dev_pm_ops imx_intmux_pm_ops = {
> > -	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend,
> imx_intmux_resume)
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +				      pm_runtime_force_resume)
> > +	SET_RUNTIME_PM_OPS(imx_intmux_runtime_suspend,
> > +			   imx_intmux_runtime_resume, NULL)
> >  };
> >
> >  static const struct of_device_id imx_intmux_id[] = {
> 
> I think you'd might as well squash the two patches together.
> Splitting the two serves little purpose.

Thanks Marc for your kindly review, I will improve all following your comments in next version.

Best Regards,
Joakim Zhang
> Thanks,
> 
>          M.
> --
> Jazz is not dead. It just smells funny...