Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1420645 > unrolled thread

Re: [PATCH V6 9/9] irqchip/gic: Add platform driver for non-root GICs that require RPM

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V6 9/9] irqchip/gic: Add platform driver for non-root GICs  that require RPM Marc Zyngier <marc.zyngier@arm.com> - 2016-06-13 11:20 +0200
    Re: [PATCH V6 9/9] irqchip/gic: Add platform driver for non-root GICs  that require RPM Jon Hunter <jonathanh@nvidia.com> - 2016-06-13 12:00 +0200
      Re: [PATCH V6 9/9] irqchip/gic: Add platform driver for non-root GICs  that require RPM Marc Zyngier <marc.zyngier@arm.com> - 2016-06-13 12:20 +0200
        Re: [PATCH V6 9/9] irqchip/gic: Add platform driver for non-root GICs  that require RPM Jon Hunter <jonathanh@nvidia.com> - 2016-06-13 12:30 +0200

#1420645 — Re: [PATCH V6 9/9] irqchip/gic: Add platform driver for non-root GICs that require RPM

Hi Jon,

On 07/06/16 16:12, Jon Hunter wrote:
> Add a platform driver to support non-root GICs that require runtime
> power-management. Currently, only non-root GICs are supported because
> the functions, smp_cross_call() and set_handle_irq(), that need to
> be called for a root controller are located in the __init section and
> so cannot be called by the platform driver.
> 
> The GIC platform driver re-uses many functions from the existing GIC
> driver including some functions to save and restore the GIC context
> during power transitions. The functions for saving and restoring the
> GIC context are currently only defined if CONFIG_CPU_PM is enabled and
> to ensure that these functions are always defined when the platform
> driver is enabled, a dependency on CONFIG_ARM_GIC_PM (which selects the
> platform driver) has been added.
> 
> In order to re-use the private GIC initialisation code, a new public
> function, gic_of_init_child(), has been added which calls various
> private functions to initialise the GIC. This is different from the
> existing gic_of_init() because it only supports non-root GICs (ie. does
> not call smp_cross_call() is set_handle_irq()) and is not located in
> the __init section (so can be used by platform drivers). Furthermore,
> gic_of_init_child() dynamically allocates memory for the GIC chip data
> which is also different from gic_of_init().
> 
> There is no specific suspend handling for GICs registered as platform
> devices. Non-wakeup interrupts will be disabled by the kernel during
> late suspend, however, this alone will not power down the GIC if
> interrupts have been requested and not freed. Therefore, requestors of
> non-wakeup interrupts will need to free them on entering suspend in
> order to power-down the GIC.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/irqchip/Kconfig         |   6 ++
>  drivers/irqchip/Makefile        |   1 +
>  drivers/irqchip/irq-gic-pm.c    | 184 ++++++++++++++++++++++++++++++++++++++++
>  drivers/irqchip/irq-gic.c       |  38 ++++++++-
>  include/linux/irqchip/arm-gic.h |   6 ++
>  5 files changed, 232 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/irqchip/irq-gic-pm.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index fa33c50b0e5a..5495a5ba8039 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -8,6 +8,12 @@ config ARM_GIC
>  	select IRQ_DOMAIN_HIERARCHY
>  	select MULTI_IRQ_HANDLER
>  
> +config ARM_GIC_PM
> +	bool
> +	depends on PM
> +	select ARM_GIC
> +	select PM_CLK
> +
>  config ARM_GIC_MAX_NR
>  	int
>  	default 2 if ARCH_REALVIEW
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 38853a187607..bd0257e0aab6 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
>  obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
>  obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
>  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
> +obj-$(CONFIG_ARM_GIC_PM)		+= irq-gic-pm.o
>  obj-$(CONFIG_REALVIEW_DT)		+= irq-gic-realview.o
>  obj-$(CONFIG_ARM_GIC_V2M)		+= irq-gic-v2m.o
>  obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
> diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
> new file mode 100644
> index 000000000000..4cbffba3ff13
> --- /dev/null
> +++ b/drivers/irqchip/irq-gic-pm.c
> @@ -0,0 +1,184 @@
> +/*
> + * Copyright (C) 2016 NVIDIA CORPORATION, All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqchip/arm-gic.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +
> +struct gic_clk_data {
> +	unsigned int num_clocks;
> +	const char *const *clocks;
> +};
> +
> +static int gic_runtime_resume(struct device *dev)
> +{
> +	struct gic_chip_data *gic = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pm_clk_resume(dev);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * On the very first resume, the pointer to the driver data
> +	 * will be NULL and this is intentional, because we do not
> +	 * want to restore the GIC on the very first resume. So if
> +	 * the pointer is not valid just return.
> +	 */
> +	if (!gic)
> +		return 0;
> +
> +	gic_dist_restore(gic);
> +	gic_cpu_restore(gic);
> +
> +	return 0;
> +}
> +
> +static int gic_runtime_suspend(struct device *dev)
> +{
> +	struct gic_chip_data *gic = dev_get_drvdata(dev);
> +
> +	gic_dist_save(gic);
> +	gic_cpu_save(gic);
> +
> +	return pm_clk_suspend(dev);
> +}
> +
> +static int gic_get_clocks(struct device *dev, const struct gic_clk_data *data)
> +{
> +	struct clk *clk;
> +	unsigned int i;
> +	int ret;
> +
> +	if (!dev || !data)
> +		return -EINVAL;
> +
> +	ret = pm_clk_create(dev);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < data->num_clocks; i++) {
> +		clk = of_clk_get_by_name(dev->of_node, data->clocks[i]);
> +		if (IS_ERR(clk)) {
> +			dev_err(dev, "failed to get clock %s\n",
> +				data->clocks[i]);
> +			ret = PTR_ERR(clk);
> +			goto error;
> +		}
> +
> +		ret = pm_clk_add_clk(dev, clk);
> +		if (ret) {
> +			dev_err(dev, "failed to add clock at index %d\n", i);
> +			clk_put(clk);
> +			goto error;
> +		}
> +	}
> +
> +	return 0;
> +
> +error:
> +	pm_clk_destroy(dev);
> +
> +	return ret;
> +}
> +
> +static int gic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct gic_clk_data *data;
> +	struct gic_chip_data *gic;
> +	int ret, irq;
> +
> +	data = of_device_get_match_data(&pdev->dev);
> +	if (!data) {
> +		dev_err(&pdev->dev, "no device match found\n");
> +		return -ENODEV;
> +	}
> +
> +	irq = irq_of_parse_and_map(dev->of_node, 0);
> +	if (!irq) {
> +		dev_err(dev, "no parent interrupt found!\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = gic_get_clocks(dev, data);
> +	if (ret)
> +		goto irq_dispose;
> +
> +	pm_runtime_enable(dev);
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0)
> +		goto rpm_disable;
> +
> +	ret = gic_of_init_child(dev, &gic, irq);
> +	if (ret)
> +		goto rpm_put;
> +
> +	platform_set_drvdata(pdev, gic);
> +
> +	pm_runtime_put(dev);
> +
> +	dev_info(dev, "GIC IRQ controller registered\n");
> +
> +	return 0;
> +
> +rpm_put:
> +	pm_runtime_put_sync(dev);
> +rpm_disable:
> +	pm_runtime_disable(dev);
> +	pm_clk_destroy(dev);
> +irq_dispose:
> +	irq_dispose_mapping(irq);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops gic_pm_ops = {
> +	SET_RUNTIME_PM_OPS(gic_runtime_suspend,
> +			   gic_runtime_resume, NULL)
> +};
> +
> +static const char * const gic400_clocks[] = {
> +	"clk",
> +};
> +
> +static const struct gic_clk_data gic400_data = {
> +	.num_clocks = ARRAY_SIZE(gic400_clocks),
> +	.clocks = gic400_clocks,
> +};
> +
> +static const struct of_device_id gic_match[] = {
> +	{ .compatible = "nvidia,tegra210-agic",	.data = &gic400_data },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gic_match);
> +
> +static struct platform_driver gic_driver = {
> +	.probe		= gic_probe,
> +	.driver		= {
> +		.name	= "gic",
> +		.of_match_table	= gic_match,
> +		.pm	= &gic_pm_ops,
> +	}
> +};
> +
> +builtin_platform_driver(gic_driver);
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 141ea5801784..57423f3b218b 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -75,7 +75,7 @@ struct gic_chip_data {
>  	void __iomem *raw_dist_base;
>  	void __iomem *raw_cpu_base;
>  	u32 percpu_offset;
> -#ifdef CONFIG_CPU_PM
> +#if defined(CONFIG_CPU_PM) || defined(CONFIG_ARM_GIC_PM)
>  	u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
>  	u32 saved_spi_active[DIV_ROUND_UP(1020, 32)];
>  	u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
> @@ -528,7 +528,7 @@ int gic_cpu_if_down(unsigned int gic_nr)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_CPU_PM
> +#if defined(CONFIG_CPU_PM) || defined(CONFIG_ARM_GIC_PM)
>  /*
>   * Saves the GIC distributor registers during suspend or idle.  Must be called
>   * with interrupts disabled but before powering down the GIC.  After calling
> @@ -1296,6 +1296,34 @@ error:
>  	return -ENOMEM;
>  }
>  
> +int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq)

I got this:

WARNING: drivers/built-in.o(.text+0x2464): Section mismatch in reference from the function gic_of_init_child() to the function .init.text:gic_of_setup()
The function gic_of_init_child() references
the function __init gic_of_setup().
This is often because gic_of_init_child lacks a __init 
annotation or the annotation of gic_of_setup is wrong.

I've fixed it locally by making the function __init.

Thanks,

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

[toc] | [next] | [standalone]

#1420662

Hi Marc,

On 13/06/16 10:10, Marc Zyngier wrote:
> Hi Jon,
> 
> On 07/06/16 16:12, Jon Hunter wrote:
>> Add a platform driver to support non-root GICs that require runtime
>> power-management. Currently, only non-root GICs are supported because
>> the functions, smp_cross_call() and set_handle_irq(), that need to
>> be called for a root controller are located in the __init section and
>> so cannot be called by the platform driver.
>>
>> The GIC platform driver re-uses many functions from the existing GIC
>> driver including some functions to save and restore the GIC context
>> during power transitions. The functions for saving and restoring the
>> GIC context are currently only defined if CONFIG_CPU_PM is enabled and
>> to ensure that these functions are always defined when the platform
>> driver is enabled, a dependency on CONFIG_ARM_GIC_PM (which selects the
>> platform driver) has been added.
>>
>> In order to re-use the private GIC initialisation code, a new public
>> function, gic_of_init_child(), has been added which calls various
>> private functions to initialise the GIC. This is different from the
>> existing gic_of_init() because it only supports non-root GICs (ie. does
>> not call smp_cross_call() is set_handle_irq()) and is not located in
>> the __init section (so can be used by platform drivers). Furthermore,
>> gic_of_init_child() dynamically allocates memory for the GIC chip data
>> which is also different from gic_of_init().
>>
>> There is no specific suspend handling for GICs registered as platform
>> devices. Non-wakeup interrupts will be disabled by the kernel during
>> late suspend, however, this alone will not power down the GIC if
>> interrupts have been requested and not freed. Therefore, requestors of
>> non-wakeup interrupts will need to free them on entering suspend in
>> order to power-down the GIC.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/irqchip/Kconfig         |   6 ++
>>  drivers/irqchip/Makefile        |   1 +
>>  drivers/irqchip/irq-gic-pm.c    | 184 ++++++++++++++++++++++++++++++++++++++++
>>  drivers/irqchip/irq-gic.c       |  38 ++++++++-
>>  include/linux/irqchip/arm-gic.h |   6 ++
>>  5 files changed, 232 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/irqchip/irq-gic-pm.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index fa33c50b0e5a..5495a5ba8039 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -8,6 +8,12 @@ config ARM_GIC
>>  	select IRQ_DOMAIN_HIERARCHY
>>  	select MULTI_IRQ_HANDLER
>>  
>> +config ARM_GIC_PM
>> +	bool
>> +	depends on PM
>> +	select ARM_GIC
>> +	select PM_CLK
>> +
>>  config ARM_GIC_MAX_NR
>>  	int
>>  	default 2 if ARCH_REALVIEW
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 38853a187607..bd0257e0aab6 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -24,6 +24,7 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
>>  obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
>>  obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
>>  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
>> +obj-$(CONFIG_ARM_GIC_PM)		+= irq-gic-pm.o
>>  obj-$(CONFIG_REALVIEW_DT)		+= irq-gic-realview.o
>>  obj-$(CONFIG_ARM_GIC_V2M)		+= irq-gic-v2m.o
>>  obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
>> diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
>> new file mode 100644
>> index 000000000000..4cbffba3ff13
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-gic-pm.c
>> @@ -0,0 +1,184 @@
>> +/*
>> + * Copyright (C) 2016 NVIDIA CORPORATION, All Rights Reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/clk.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/irqchip/arm-gic.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_clock.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +
>> +struct gic_clk_data {
>> +	unsigned int num_clocks;
>> +	const char *const *clocks;
>> +};
>> +
>> +static int gic_runtime_resume(struct device *dev)
>> +{
>> +	struct gic_chip_data *gic = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = pm_clk_resume(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * On the very first resume, the pointer to the driver data
>> +	 * will be NULL and this is intentional, because we do not
>> +	 * want to restore the GIC on the very first resume. So if
>> +	 * the pointer is not valid just return.
>> +	 */
>> +	if (!gic)
>> +		return 0;
>> +
>> +	gic_dist_restore(gic);
>> +	gic_cpu_restore(gic);
>> +
>> +	return 0;
>> +}
>> +
>> +static int gic_runtime_suspend(struct device *dev)
>> +{
>> +	struct gic_chip_data *gic = dev_get_drvdata(dev);
>> +
>> +	gic_dist_save(gic);
>> +	gic_cpu_save(gic);
>> +
>> +	return pm_clk_suspend(dev);
>> +}
>> +
>> +static int gic_get_clocks(struct device *dev, const struct gic_clk_data *data)
>> +{
>> +	struct clk *clk;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	if (!dev || !data)
>> +		return -EINVAL;
>> +
>> +	ret = pm_clk_create(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	for (i = 0; i < data->num_clocks; i++) {
>> +		clk = of_clk_get_by_name(dev->of_node, data->clocks[i]);
>> +		if (IS_ERR(clk)) {
>> +			dev_err(dev, "failed to get clock %s\n",
>> +				data->clocks[i]);
>> +			ret = PTR_ERR(clk);
>> +			goto error;
>> +		}
>> +
>> +		ret = pm_clk_add_clk(dev, clk);
>> +		if (ret) {
>> +			dev_err(dev, "failed to add clock at index %d\n", i);
>> +			clk_put(clk);
>> +			goto error;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +error:
>> +	pm_clk_destroy(dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int gic_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	const struct gic_clk_data *data;
>> +	struct gic_chip_data *gic;
>> +	int ret, irq;
>> +
>> +	data = of_device_get_match_data(&pdev->dev);
>> +	if (!data) {
>> +		dev_err(&pdev->dev, "no device match found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	irq = irq_of_parse_and_map(dev->of_node, 0);
>> +	if (!irq) {
>> +		dev_err(dev, "no parent interrupt found!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = gic_get_clocks(dev, data);
>> +	if (ret)
>> +		goto irq_dispose;
>> +
>> +	pm_runtime_enable(dev);
>> +
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0)
>> +		goto rpm_disable;
>> +
>> +	ret = gic_of_init_child(dev, &gic, irq);
>> +	if (ret)
>> +		goto rpm_put;
>> +
>> +	platform_set_drvdata(pdev, gic);
>> +
>> +	pm_runtime_put(dev);
>> +
>> +	dev_info(dev, "GIC IRQ controller registered\n");
>> +
>> +	return 0;
>> +
>> +rpm_put:
>> +	pm_runtime_put_sync(dev);
>> +rpm_disable:
>> +	pm_runtime_disable(dev);
>> +	pm_clk_destroy(dev);
>> +irq_dispose:
>> +	irq_dispose_mapping(irq);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct dev_pm_ops gic_pm_ops = {
>> +	SET_RUNTIME_PM_OPS(gic_runtime_suspend,
>> +			   gic_runtime_resume, NULL)
>> +};
>> +
>> +static const char * const gic400_clocks[] = {
>> +	"clk",
>> +};
>> +
>> +static const struct gic_clk_data gic400_data = {
>> +	.num_clocks = ARRAY_SIZE(gic400_clocks),
>> +	.clocks = gic400_clocks,
>> +};
>> +
>> +static const struct of_device_id gic_match[] = {
>> +	{ .compatible = "nvidia,tegra210-agic",	.data = &gic400_data },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, gic_match);
>> +
>> +static struct platform_driver gic_driver = {
>> +	.probe		= gic_probe,
>> +	.driver		= {
>> +		.name	= "gic",
>> +		.of_match_table	= gic_match,
>> +		.pm	= &gic_pm_ops,
>> +	}
>> +};
>> +
>> +builtin_platform_driver(gic_driver);
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 141ea5801784..57423f3b218b 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -75,7 +75,7 @@ struct gic_chip_data {
>>  	void __iomem *raw_dist_base;
>>  	void __iomem *raw_cpu_base;
>>  	u32 percpu_offset;
>> -#ifdef CONFIG_CPU_PM
>> +#if defined(CONFIG_CPU_PM) || defined(CONFIG_ARM_GIC_PM)
>>  	u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
>>  	u32 saved_spi_active[DIV_ROUND_UP(1020, 32)];
>>  	u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
>> @@ -528,7 +528,7 @@ int gic_cpu_if_down(unsigned int gic_nr)
>>  	return 0;
>>  }
>>  
>> -#ifdef CONFIG_CPU_PM
>> +#if defined(CONFIG_CPU_PM) || defined(CONFIG_ARM_GIC_PM)
>>  /*
>>   * Saves the GIC distributor registers during suspend or idle.  Must be called
>>   * with interrupts disabled but before powering down the GIC.  After calling
>> @@ -1296,6 +1296,34 @@ error:
>>  	return -ENOMEM;
>>  }
>>  
>> +int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq)
> 
> I got this:
> 
> WARNING: drivers/built-in.o(.text+0x2464): Section mismatch in reference from the function gic_of_init_child() to the function .init.text:gic_of_setup()
> The function gic_of_init_child() references
> the function __init gic_of_setup().
> This is often because gic_of_init_child lacks a __init 
> annotation or the annotation of gic_of_setup is wrong.

Ugh! A lot of the changes were to avoid these section mismatches. Somehow I missed that one :-(
 
> I've fixed it locally by making the function __init.

So gic_of_init_child() cannot be __init because it is called by the driver. If you do this you should get ...

WARNING: drivers/irqchip/built-in.o(.text+0x2314): Section mismatch in reference from the function gic_probe() to the function .init.text:gic_of_init_child()
The function gic_probe() references
the function __init gic_of_init_child().
This is often because gic_probe lacks a __init 
annotation or the annotation of gic_of_init_child is wrong.


The correct fix is ...

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 57423f3b218b..1de07eb5839c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1272,7 +1272,7 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
        return true;
 }
 
-static int __init gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
+static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
 {
        if (!gic || !node)
                return -EINVAL;

Cheers
Jon

-- 
nvpublic

[toc] | [prev] | [next] | [standalone]

#1420682

On 13/06/16 10:58, Jon Hunter wrote:
> Hi Marc,
> 
> On 13/06/16 10:10, Marc Zyngier wrote:
>> Hi Jon,
>>
>> On 07/06/16 16:12, Jon Hunter wrote:
>>> Add a platform driver to support non-root GICs that require runtime
>>> power-management. Currently, only non-root GICs are supported because
>>> the functions, smp_cross_call() and set_handle_irq(), that need to
>>> be called for a root controller are located in the __init section and
>>> so cannot be called by the platform driver.
>>>
>>> The GIC platform driver re-uses many functions from the existing GIC
>>> driver including some functions to save and restore the GIC context
>>> during power transitions. The functions for saving and restoring the
>>> GIC context are currently only defined if CONFIG_CPU_PM is enabled and
>>> to ensure that these functions are always defined when the platform
>>> driver is enabled, a dependency on CONFIG_ARM_GIC_PM (which selects the
>>> platform driver) has been added.
>>>
>>> In order to re-use the private GIC initialisation code, a new public
>>> function, gic_of_init_child(), has been added which calls various
>>> private functions to initialise the GIC. This is different from the
>>> existing gic_of_init() because it only supports non-root GICs (ie. does
>>> not call smp_cross_call() is set_handle_irq()) and is not located in
>>> the __init section (so can be used by platform drivers). Furthermore,
>>> gic_of_init_child() dynamically allocates memory for the GIC chip data
>>> which is also different from gic_of_init().
>>>
>>> There is no specific suspend handling for GICs registered as platform
>>> devices. Non-wakeup interrupts will be disabled by the kernel during
>>> late suspend, however, this alone will not power down the GIC if
>>> interrupts have been requested and not freed. Therefore, requestors of
>>> non-wakeup interrupts will need to free them on entering suspend in
>>> order to power-down the GIC.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  drivers/irqchip/Kconfig         |   6 ++
>>>  drivers/irqchip/Makefile        |   1 +
>>>  drivers/irqchip/irq-gic-pm.c    | 184 ++++++++++++++++++++++++++++++++++++++++
>>>  drivers/irqchip/irq-gic.c       |  38 ++++++++-
>>>  include/linux/irqchip/arm-gic.h |   6 ++
>>>  5 files changed, 232 insertions(+), 3 deletions(-)
>>>  create mode 100644 drivers/irqchip/irq-gic-pm.c
>>>
>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>> index fa33c50b0e5a..5495a5ba8039 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -8,6 +8,12 @@ config ARM_GIC
>>>  	select IRQ_DOMAIN_HIERARCHY
>>>  	select MULTI_IRQ_HANDLER
>>>  
>>> +config ARM_GIC_PM
>>> +	bool
>>> +	depends on PM
>>> +	select ARM_GIC
>>> +	select PM_CLK
>>> +
>>>  config ARM_GIC_MAX_NR
>>>  	int
>>>  	default 2 if ARCH_REALVIEW
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index 38853a187607..bd0257e0aab6 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -24,6 +24,7 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
>>>  obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
>>>  obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
>>>  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
>>> +obj-$(CONFIG_ARM_GIC_PM)		+= irq-gic-pm.o
>>>  obj-$(CONFIG_REALVIEW_DT)		+= irq-gic-realview.o
>>>  obj-$(CONFIG_ARM_GIC_V2M)		+= irq-gic-v2m.o
>>>  obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
>>> diff --git a/drivers/irqchip/irq-gic-pm.c b/drivers/irqchip/irq-gic-pm.c
>>> new file mode 100644
>>> index 000000000000..4cbffba3ff13
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-gic-pm.c
>>> @@ -0,0 +1,184 @@
>>> +/*
>>> + * Copyright (C) 2016 NVIDIA CORPORATION, All Rights Reserved.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * 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.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +#include <linux/module.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/irqchip/arm-gic.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_clock.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/slab.h>
>>> +
>>> +struct gic_clk_data {
>>> +	unsigned int num_clocks;
>>> +	const char *const *clocks;
>>> +};
>>> +
>>> +static int gic_runtime_resume(struct device *dev)
>>> +{
>>> +	struct gic_chip_data *gic = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	ret = pm_clk_resume(dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/*
>>> +	 * On the very first resume, the pointer to the driver data
>>> +	 * will be NULL and this is intentional, because we do not
>>> +	 * want to restore the GIC on the very first resume. So if
>>> +	 * the pointer is not valid just return.
>>> +	 */
>>> +	if (!gic)
>>> +		return 0;
>>> +
>>> +	gic_dist_restore(gic);
>>> +	gic_cpu_restore(gic);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int gic_runtime_suspend(struct device *dev)
>>> +{
>>> +	struct gic_chip_data *gic = dev_get_drvdata(dev);
>>> +
>>> +	gic_dist_save(gic);
>>> +	gic_cpu_save(gic);
>>> +
>>> +	return pm_clk_suspend(dev);
>>> +}
>>> +
>>> +static int gic_get_clocks(struct device *dev, const struct gic_clk_data *data)
>>> +{
>>> +	struct clk *clk;
>>> +	unsigned int i;
>>> +	int ret;
>>> +
>>> +	if (!dev || !data)
>>> +		return -EINVAL;
>>> +
>>> +	ret = pm_clk_create(dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	for (i = 0; i < data->num_clocks; i++) {
>>> +		clk = of_clk_get_by_name(dev->of_node, data->clocks[i]);
>>> +		if (IS_ERR(clk)) {
>>> +			dev_err(dev, "failed to get clock %s\n",
>>> +				data->clocks[i]);
>>> +			ret = PTR_ERR(clk);
>>> +			goto error;
>>> +		}
>>> +
>>> +		ret = pm_clk_add_clk(dev, clk);
>>> +		if (ret) {
>>> +			dev_err(dev, "failed to add clock at index %d\n", i);
>>> +			clk_put(clk);
>>> +			goto error;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +error:
>>> +	pm_clk_destroy(dev);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int gic_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	const struct gic_clk_data *data;
>>> +	struct gic_chip_data *gic;
>>> +	int ret, irq;
>>> +
>>> +	data = of_device_get_match_data(&pdev->dev);
>>> +	if (!data) {
>>> +		dev_err(&pdev->dev, "no device match found\n");
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	irq = irq_of_parse_and_map(dev->of_node, 0);
>>> +	if (!irq) {
>>> +		dev_err(dev, "no parent interrupt found!\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = gic_get_clocks(dev, data);
>>> +	if (ret)
>>> +		goto irq_dispose;
>>> +
>>> +	pm_runtime_enable(dev);
>>> +
>>> +	ret = pm_runtime_get_sync(dev);
>>> +	if (ret < 0)
>>> +		goto rpm_disable;
>>> +
>>> +	ret = gic_of_init_child(dev, &gic, irq);
>>> +	if (ret)
>>> +		goto rpm_put;
>>> +
>>> +	platform_set_drvdata(pdev, gic);
>>> +
>>> +	pm_runtime_put(dev);
>>> +
>>> +	dev_info(dev, "GIC IRQ controller registered\n");
>>> +
>>> +	return 0;
>>> +
>>> +rpm_put:
>>> +	pm_runtime_put_sync(dev);
>>> +rpm_disable:
>>> +	pm_runtime_disable(dev);
>>> +	pm_clk_destroy(dev);
>>> +irq_dispose:
>>> +	irq_dispose_mapping(irq);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const struct dev_pm_ops gic_pm_ops = {
>>> +	SET_RUNTIME_PM_OPS(gic_runtime_suspend,
>>> +			   gic_runtime_resume, NULL)
>>> +};
>>> +
>>> +static const char * const gic400_clocks[] = {
>>> +	"clk",
>>> +};
>>> +
>>> +static const struct gic_clk_data gic400_data = {
>>> +	.num_clocks = ARRAY_SIZE(gic400_clocks),
>>> +	.clocks = gic400_clocks,
>>> +};
>>> +
>>> +static const struct of_device_id gic_match[] = {
>>> +	{ .compatible = "nvidia,tegra210-agic",	.data = &gic400_data },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, gic_match);
>>> +
>>> +static struct platform_driver gic_driver = {
>>> +	.probe		= gic_probe,
>>> +	.driver		= {
>>> +		.name	= "gic",
>>> +		.of_match_table	= gic_match,
>>> +		.pm	= &gic_pm_ops,
>>> +	}
>>> +};
>>> +
>>> +builtin_platform_driver(gic_driver);
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index 141ea5801784..57423f3b218b 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -75,7 +75,7 @@ struct gic_chip_data {
>>>  	void __iomem *raw_dist_base;
>>>  	void __iomem *raw_cpu_base;
>>>  	u32 percpu_offset;
>>> -#ifdef CONFIG_CPU_PM
>>> +#if defined(CONFIG_CPU_PM) || defined(CONFIG_ARM_GIC_PM)
>>>  	u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
>>>  	u32 saved_spi_active[DIV_ROUND_UP(1020, 32)];
>>>  	u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
>>> @@ -528,7 +528,7 @@ int gic_cpu_if_down(unsigned int gic_nr)
>>>  	return 0;
>>>  }
>>>  
>>> -#ifdef CONFIG_CPU_PM
>>> +#if defined(CONFIG_CPU_PM) || defined(CONFIG_ARM_GIC_PM)
>>>  /*
>>>   * Saves the GIC distributor registers during suspend or idle.  Must be called
>>>   * with interrupts disabled but before powering down the GIC.  After calling
>>> @@ -1296,6 +1296,34 @@ error:
>>>  	return -ENOMEM;
>>>  }
>>>  
>>> +int gic_of_init_child(struct device *dev, struct gic_chip_data **gic, int irq)
>>
>> I got this:
>>
>> WARNING: drivers/built-in.o(.text+0x2464): Section mismatch in reference from the function gic_of_init_child() to the function .init.text:gic_of_setup()
>> The function gic_of_init_child() references
>> the function __init gic_of_setup().
>> This is often because gic_of_init_child lacks a __init 
>> annotation or the annotation of gic_of_setup is wrong.
> 
> Ugh! A lot of the changes were to avoid these section mismatches. Somehow I missed that one :-(
>  
>> I've fixed it locally by making the function __init.
> 
> So gic_of_init_child() cannot be __init because it is called by the driver. If you do this you should get ...
> 
> WARNING: drivers/irqchip/built-in.o(.text+0x2314): Section mismatch in reference from the function gic_probe() to the function .init.text:gic_of_init_child()
> The function gic_probe() references
> the function __init gic_of_init_child().
> This is often because gic_probe lacks a __init 
> annotation or the annotation of gic_of_init_child is wrong.

Odd, I'm not getting that one... [digging]

OK, that's because ARM_GIC_PM is never selected (and there is no way I
can actually select). Do you have some additional patches that selects
it for Tegra?

> 
> 
> The correct fix is ...
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 57423f3b218b..1de07eb5839c 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1272,7 +1272,7 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>         return true;
>  }
>  
> -static int __init gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
> +static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
>  {
>         if (!gic || !node)
>                 return -EINVAL;

OK, I've applied this locally (no need to resend).

Thanks,

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

[toc] | [prev] | [next] | [standalone]

#1420687

On 13/06/16 11:13, Marc Zyngier wrote:

...

> OK, that's because ARM_GIC_PM is never selected (and there is no way I
> can actually select). Do you have some additional patches that selects
> it for Tegra?

Yes locally, I have the following for testing ...

diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
index 03089ad2fc65..ffd02a3c093a 100644
--- a/drivers/soc/tegra/Kconfig
+++ b/drivers/soc/tegra/Kconfig
@@ -63,6 +63,7 @@ config ARCH_TEGRA_132_SOC

 config ARCH_TEGRA_210_SOC
        bool "NVIDIA Tegra210 SoC"
+       select ARM_GIC_PM
        select PINCTRL_TEGRA210

>> The correct fix is ...
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index 57423f3b218b..1de07eb5839c 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -1272,7 +1272,7 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>>         return true;
>>  }
>>  
>> -static int __init gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
>> +static int gic_of_setup(struct gic_chip_data *gic, struct device_node *node)
>>  {
>>         if (!gic || !node)
>>                 return -EINVAL;
> 
> OK, I've applied this locally (no need to resend).

Great. Thanks. Sorry about this.

Jon

-- 
nvpublic

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web