soc: imx: Add power domain driver support for i.mx8m family

Commit Message

Abel Vesa April 27, 2020, 2:58 p.m. UTC

From: Jacky Bai <[e-mail]>

The i.MX8M family is a set of NXP product focus on delivering
the latest and greatest video and audio experience combining
state-of-the-art media-specific features with high-performance
processing while optimized for lowest power consumption.

i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all
belong to this family. A GPC module is used to manage all the
PU power domain on/off. But the situation is that the number of
power domains & the power up sequence has significate difference
on those SoCs. Even on the same SoC. The power up sequence still
has big difference. It makes us hard to reuse the GPCv2 driver to
cover the whole i.MX8M family. Each time a new SoC is supported in
the mainline kernel, we need to modify the GPCv2 driver to support
it. We need to add or modify hundred lines of code in worst case.
It is a bad practice for the driver maintainability.

This driver add a more generic power domain driver that the actual
power on/off is done by TF-A code. the abstraction give us the
possibility that using one driver to cover the whole i.MX8M family
in kernel side.

Signed-off-by: Jacky Bai <[e-mail]>
Signed-off-by: Abel Vesa <[e-mail]>
---
 drivers/soc/imx/Kconfig            |   6 +
 drivers/soc/imx/Makefile           |   1 +
 drivers/soc/imx/imx8m_pm_domains.c | 224 +++++++++++++++++++++++++++++++++++++
 include/soc/imx/imx_sip.h          |  12 ++
 4 files changed, 243 insertions(+)
 create mode 100644 drivers/soc/imx/imx8m_pm_domains.c
 create mode 100644 include/soc/imx/imx_sip.h

Comments

Lucas Stach April 27, 2020, 3:10 p.m. UTC | #1

Am Montag, den 27.04.2020, 17:58 +0300 schrieb Abel Vesa:
> From: Jacky Bai <[e-mail]>
> 
> The i.MX8M family is a set of NXP product focus on delivering
> the latest and greatest video and audio experience combining
> state-of-the-art media-specific features with high-performance
> processing while optimized for lowest power consumption.
> 
> i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all
> belong to this family. A GPC module is used to manage all the
> PU power domain on/off. But the situation is that the number of
> power domains & the power up sequence has significate difference
> on those SoCs. Even on the same SoC. The power up sequence still
> has big difference. It makes us hard to reuse the GPCv2 driver to
> cover the whole i.MX8M family. Each time a new SoC is supported in
> the mainline kernel, we need to modify the GPCv2 driver to support
> it. We need to add or modify hundred lines of code in worst case.
> It is a bad practice for the driver maintainability.
> 
> This driver add a more generic power domain driver that the actual
> power on/off is done by TF-A code. the abstraction give us the
> possibility that using one driver to cover the whole i.MX8M family
> in kernel side.
> 

Again: what does this driver bring to the table, other than moving a
fraction of the power domain functionality into the firmware?

The discussions on the last submissions of this driver already
established that we can't move all functionality for the power domains
into the firmware, as controlling regulators is probably not easy to do
from this context. Also the TF-A side implementation of this driver is
"interesting" IMHO, it does stuff like accessing the clock controller
registers without any locking or other means of mutual exclusion with
the Linux kernel clock controller driver.

Why can't we just extend the existing GPCv2 driver with support for the
other i.MX8M family members?

Regards,
Lucas

> Signed-off-by: Jacky Bai <[e-mail]>
> Signed-off-by: Abel Vesa <[e-mail]>
> ---
>  drivers/soc/imx/Kconfig            |   6 +
>  drivers/soc/imx/Makefile           |   1 +
>  drivers/soc/imx/imx8m_pm_domains.c | 224 +++++++++++++++++++++++++++++++++++++
>  include/soc/imx/imx_sip.h          |  12 ++
>  4 files changed, 243 insertions(+)
>  create mode 100644 drivers/soc/imx/imx8m_pm_domains.c
>  create mode 100644 include/soc/imx/imx_sip.h
> 
> diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig
> index d515d2c..7837199 100644
> --- a/drivers/soc/imx/Kconfig
> +++ b/drivers/soc/imx/Kconfig
> @@ -27,4 +27,10 @@ config SOC_IMX8M
>  	  support, it will provide the SoC info like SoC family,
>  	  ID and revision etc.
>  
> +config IMX8M_PM_DOMAINS
> +	bool "i.MX8M PM domains"
> +	depends on ARCH_MXC || (COMPILE_TEST && OF)
> +	depends on PM
> +	select PM_GENERIC_DOMAINS
> +
>  endmenu
> diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
> index 103e2c9..a22e24b 100644
> --- a/drivers/soc/imx/Makefile
> +++ b/drivers/soc/imx/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
>  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
>  obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
>  obj-$(CONFIG_IMX_SCU_SOC) += soc-imx-scu.o
> +obj-$(CONFIG_IMX8M_PM_DOMAINS) += imx8m_pm_domains.o
> diff --git a/drivers/soc/imx/imx8m_pm_domains.c b/drivers/soc/imx/imx8m_pm_domains.c
> new file mode 100644
> index 00000000..ce06a05
> --- /dev/null
> +++ b/drivers/soc/imx/imx8m_pm_domains.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 NXP.
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <soc/imx/imx_sip.h>
> +
> +#define MAX_CLK_NUM	6
> +#define to_imx8m_pm_domain(_genpd) container_of(_genpd, struct imx8m_pm_domain, pd)
> +
> +
> +struct imx8m_pm_domain {
> +	struct device *dev;
> +	struct generic_pm_domain pd;
> +	u32 domain_index;
> +	struct clk *clk[MAX_CLK_NUM];
> +	unsigned int num_clks;
> +	struct regulator *reg;
> +};
> +
> +enum imx8m_pm_domain_state {
> +	PD_STATE_OFF,
> +	PD_STATE_ON,
> +};
> +
> +static DEFINE_MUTEX(gpc_pd_mutex);
> +
> +static int imx8m_pd_power_on(struct generic_pm_domain *genpd)
> +{
> +	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
> +	struct arm_smccc_res res;
> +	int index, ret = 0;
> +
> +	/* power on the external supply */
> +	if (!IS_ERR(domain->reg)) {
> +		ret = regulator_enable(domain->reg);
> +		if (ret) {
> +			dev_warn(domain->dev, "failed to power up the reg%d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	/* enable the necessary clks needed by the power domain */
> +	if (domain->num_clks) {
> +		for (index = 0; index < domain->num_clks; index++)
> +			clk_prepare_enable(domain->clk[index]);
> +	}
> +
> +	mutex_lock(&gpc_pd_mutex);
> +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN, domain->domain_index,
> +		      PD_STATE_ON, 0, 0, 0, 0, &res);
> +	mutex_unlock(&gpc_pd_mutex);
> +
> +	return 0;
> +}
> +
> +static int imx8m_pd_power_off(struct generic_pm_domain *genpd)
> +{
> +	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
> +	struct arm_smccc_res res;
> +	int index, ret = 0;
> +
> +	mutex_lock(&gpc_pd_mutex);
> +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN, domain->domain_index,
> +		      PD_STATE_OFF, 0, 0, 0, 0, &res);
> +	mutex_unlock(&gpc_pd_mutex);
> +
> +	/* power off the external supply */
> +	if (!IS_ERR(domain->reg)) {
> +		ret = regulator_disable(domain->reg);
> +		if (ret) {
> +			dev_warn(domain->dev, "failed to power off the reg%d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	/* disable clks when power domain is off */
> +	if (domain->num_clks) {
> +		for (index = 0; index < domain->num_clks; index++)
> +			clk_disable_unprepare(domain->clk[index]);
> +	}
> +
> +	return ret;
> +};
> +
> +static int imx8m_pd_get_clocks(struct imx8m_pm_domain *domain)
> +{
> +	int i, ret;
> +
> +	for (i = 0; ; i++) {
> +		struct clk *clk = of_clk_get(domain->dev->of_node, i);
> +		if (IS_ERR(clk))
> +			break;
> +		if (i >= MAX_CLK_NUM) {
> +			dev_err(domain->dev, "more than %d clocks\n",
> +				MAX_CLK_NUM);
> +			ret = -EINVAL;
> +			goto clk_err;
> +		}
> +		domain->clk[i] = clk;
> +	}
> +	domain->num_clks = i;
> +
> +	return 0;
> +
> +clk_err:
> +	while (i--)
> +		clk_put(domain->clk[i]);
> +
> +	return ret;
> +}
> +
> +static void imx8m_pd_put_clocks(struct imx8m_pm_domain *domain)
> +{
> +	int i;
> +
> +	for (i = domain->num_clks - 1; i >= 0; i--)
> +		clk_put(domain->clk[i]);
> +}
> +
> +static const struct of_device_id imx8m_pm_domain_ids[] = {
> +	{.compatible = "fsl,imx8m-pm-domain"},
> +	{},
> +};
> +
> +static int imx8m_pm_domain_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct imx8m_pm_domain *domain;
> +	struct of_phandle_args parent, child;
> +	int ret;
> +
> +	domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
> +	if (!domain)
> +		return -ENOMEM;
> +
> +	child.np = np;
> +	domain->dev = dev;
> +
> +	ret = of_property_read_string(np, "domain-name", &domain->pd.name);
> +	if (ret) {
> +		dev_err(dev, "failed to get the domain name\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(np, "domain-index", &domain->domain_index);
> +	if (ret) {
> +		dev_err(dev, "failed to get the domain index\n");
> +		return -EINVAL;
> +	}
> +
> +	domain->reg = devm_regulator_get_optional(dev, "power");
> +	if (IS_ERR(domain->reg)) {
> +		if (PTR_ERR(domain->reg) != -ENODEV) {
> +			if (PTR_ERR(domain->reg) != -EPROBE_DEFER)
> +				dev_err(dev, "failed to get domain's regulator\n");
> +			return PTR_ERR(domain->reg);
> +		}
> +	}
> +
> +	ret = imx8m_pd_get_clocks(domain);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get domain's clocks\n");
> +		return ret;
> +	}
> +
> +	domain->pd.power_off = imx8m_pd_power_off;
> +	domain->pd.power_on = imx8m_pd_power_on;
> +
> +	pm_genpd_init(&domain->pd, NULL, true);
> +
> +	ret = of_genpd_add_provider_simple(np, &domain->pd);
> +	if (ret) {
> +		dev_err(dev, "failed to add the domain provider\n");
> +		pm_genpd_remove(&domain->pd);
> +		imx8m_pd_put_clocks(domain);
> +		return ret;
> +	}
> +
> +	/* add it as subdomain if necessary */
> +	if (!of_parse_phandle_with_args(np, "parent-domains",
> +			"#power-domain-cells", 0, &parent)) {
> +		ret = of_genpd_add_subdomain(&parent, &child);
> +		of_node_put(parent.np);
> +
> +		if (ret < 0) {
> +			dev_dbg(dev, "failed to add the subdomain: %s: %d",
> +				domain->pd.name, ret);
> +			of_genpd_del_provider(np);
> +			pm_genpd_remove(&domain->pd);
> +			imx8m_pd_put_clocks(domain);
> +			return driver_deferred_probe_check_state(dev);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver imx8m_pm_domain_driver = {
> +	.driver = {
> +		.name	= "imx8m_pm_domain",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = imx8m_pm_domain_ids,
> +	},
> +	.probe = imx8m_pm_domain_probe,
> +};
> +module_platform_driver(imx8m_pm_domain_driver);
> +
> +MODULE_AUTHOR("NXP");
> +MODULE_DESCRIPTION("NXP i.MX8M power domain driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/soc/imx/imx_sip.h b/include/soc/imx/imx_sip.h
> new file mode 100644
> index 00000000..6b96b33
> --- /dev/null
> +++ b/include/soc/imx/imx_sip.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2019 NXP
> + */
> +
> +#ifndef __IMX_SIP_H__
> +#define __IMX_SIP_H__
> +
> +#define IMX_SIP_GPC			0xC2000000
> +#define IMX_SIP_CONFIG_GPC_PM_DOMAIN	0x03
> +
> +#endif /* __IMX_SIP_H__ */

Jacky Bai April 27, 2020, 3:37 p.m. UTC | #2

> -----Original Message-----
> From: Lucas Stach <[e-mail]>
> Sent: Monday, April 27, 2020 11:11 PM
> To: Abel Vesa <[e-mail]>; Jacky Bai <[e-mail]>; Shawn
> Guo <[e-mail]>; Sascha Hauer <[e-mail]>; Liam
> Girdwood <[e-mail]>; Mark Brown <[e-mail]>
> Cc: Aisheng Dong <[e-mail]>; dl-linux-imx
> <[e-mail]>; [e-mail]; Linux Kernel
> Mailing List <[e-mail]>
> Subject: Re: [PATCH] soc: imx: Add power domain driver support for i.mx8m
> family
> 
> Am Montag, den 27.04.2020, 17:58 +0300 schrieb Abel Vesa:
> > From: Jacky Bai <[e-mail]>
> >
> > The i.MX8M family is a set of NXP product focus on delivering the
> > latest and greatest video and audio experience combining
> > state-of-the-art media-specific features with high-performance
> > processing while optimized for lowest power consumption.
> >
> > i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all belong to
> > this family. A GPC module is used to manage all the PU power domain
> > on/off. But the situation is that the number of power domains & the
> > power up sequence has significate difference on those SoCs. Even on
> > the same SoC. The power up sequence still has big difference. It makes
> > us hard to reuse the GPCv2 driver to cover the whole i.MX8M family.
> > Each time a new SoC is supported in the mainline kernel, we need to
> > modify the GPCv2 driver to support it. We need to add or modify
> > hundred lines of code in worst case.
> > It is a bad practice for the driver maintainability.
> >
> > This driver add a more generic power domain driver that the actual
> > power on/off is done by TF-A code. the abstraction give us the
> > possibility that using one driver to cover the whole i.MX8M family in
> > kernel side.
> >
> 
> Again: what does this driver bring to the table, other than moving a fraction of
> the power domain functionality into the firmware?
> 
> The discussions on the last submissions of this driver already established that
> we can't move all functionality for the power domains into the firmware, as
> controlling regulators is probably not easy to do from this context. Also the
> TF-A side implementation of this driver is "interesting" IMHO, it does stuff like
> accessing the clock controller registers without any locking or other means of
> mutual exclusion with the Linux kernel clock controller driver.
> 

The clock handling is in kernel side through CCF, not in ATF. See the patch below.

> Why can't we just extend the existing GPCv2 driver with support for the other
> i.MX8M family members?
> 

The reason that why I don’t like to extend the GPCv2 is that when doing domain on/off,
We need to access some special control register in each domain & do some special flow,
These control register(mediamix block control, vpumix block control) is not in GPC
module's address range. No benefit to reuse the GPCv2. Only bring complexity to the
GPCv2 driver each time a new SoC is added.

Yes, the i.MX8M power domain support has been pending for a while. ARM guys rejected this patchset
because they suggest us to use SCMI rather than SiP. But SCMI is only partial suitable for our
case.

BR
Jacky Bai
> Regards,
> Lucas
> 
> > Signed-off-by: Jacky Bai <[e-mail]>
> > Signed-off-by: Abel Vesa <[e-mail]>
> > ---
> >  drivers/soc/imx/Kconfig            |   6 +
> >  drivers/soc/imx/Makefile           |   1 +
> >  drivers/soc/imx/imx8m_pm_domains.c | 224
> +++++++++++++++++++++++++++++++++++++
> >  include/soc/imx/imx_sip.h          |  12 ++
> >  4 files changed, 243 insertions(+)
> >  create mode 100644 drivers/soc/imx/imx8m_pm_domains.c
> >  create mode 100644 include/soc/imx/imx_sip.h
> >
> > diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig index
> > d515d2c..7837199 100644
> > --- a/drivers/soc/imx/Kconfig
> > +++ b/drivers/soc/imx/Kconfig
> > @@ -27,4 +27,10 @@ config SOC_IMX8M
> >  	  support, it will provide the SoC info like SoC family,
> >  	  ID and revision etc.
> >
> > +config IMX8M_PM_DOMAINS
> > +	bool "i.MX8M PM domains"
> > +	depends on ARCH_MXC || (COMPILE_TEST && OF)
> > +	depends on PM
> > +	select PM_GENERIC_DOMAINS
> > +
> >  endmenu
> > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index
> > 103e2c9..a22e24b 100644
> > --- a/drivers/soc/imx/Makefile
> > +++ b/drivers/soc/imx/Makefile
> > @@ -3,3 +3,4 @@ obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> >  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> >  obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> >  obj-$(CONFIG_IMX_SCU_SOC) += soc-imx-scu.o
> > +obj-$(CONFIG_IMX8M_PM_DOMAINS) += imx8m_pm_domains.o
> > diff --git a/drivers/soc/imx/imx8m_pm_domains.c
> > b/drivers/soc/imx/imx8m_pm_domains.c
> > new file mode 100644
> > index 00000000..ce06a05
> > --- /dev/null
> > +++ b/drivers/soc/imx/imx8m_pm_domains.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019 NXP.
> > + */
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include <soc/imx/imx_sip.h>
> > +
> > +#define MAX_CLK_NUM	6
> > +#define to_imx8m_pm_domain(_genpd) container_of(_genpd, struct
> > +imx8m_pm_domain, pd)
> > +
> > +
> > +struct imx8m_pm_domain {
> > +	struct device *dev;
> > +	struct generic_pm_domain pd;
> > +	u32 domain_index;
> > +	struct clk *clk[MAX_CLK_NUM];
> > +	unsigned int num_clks;
> > +	struct regulator *reg;
> > +};
> > +
> > +enum imx8m_pm_domain_state {
> > +	PD_STATE_OFF,
> > +	PD_STATE_ON,
> > +};
> > +
> > +static DEFINE_MUTEX(gpc_pd_mutex);
> > +
> > +static int imx8m_pd_power_on(struct generic_pm_domain *genpd) {
> > +	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
> > +	struct arm_smccc_res res;
> > +	int index, ret = 0;
> > +
> > +	/* power on the external supply */
> > +	if (!IS_ERR(domain->reg)) {
> > +		ret = regulator_enable(domain->reg);
> > +		if (ret) {
> > +			dev_warn(domain->dev, "failed to power up the reg%d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* enable the necessary clks needed by the power domain */
> > +	if (domain->num_clks) {
> > +		for (index = 0; index < domain->num_clks; index++)
> > +			clk_prepare_enable(domain->clk[index]);
> > +	}
> > +
> > +	mutex_lock(&gpc_pd_mutex);
> > +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN,
> domain->domain_index,
> > +		      PD_STATE_ON, 0, 0, 0, 0, &res);
> > +	mutex_unlock(&gpc_pd_mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx8m_pd_power_off(struct generic_pm_domain *genpd) {
> > +	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
> > +	struct arm_smccc_res res;
> > +	int index, ret = 0;
> > +
> > +	mutex_lock(&gpc_pd_mutex);
> > +	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN,
> domain->domain_index,
> > +		      PD_STATE_OFF, 0, 0, 0, 0, &res);
> > +	mutex_unlock(&gpc_pd_mutex);
> > +
> > +	/* power off the external supply */
> > +	if (!IS_ERR(domain->reg)) {
> > +		ret = regulator_disable(domain->reg);
> > +		if (ret) {
> > +			dev_warn(domain->dev, "failed to power off the reg%d\n",
> ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* disable clks when power domain is off */
> > +	if (domain->num_clks) {
> > +		for (index = 0; index < domain->num_clks; index++)
> > +			clk_disable_unprepare(domain->clk[index]);
> > +	}
> > +
> > +	return ret;
> > +};
> > +
> > +static int imx8m_pd_get_clocks(struct imx8m_pm_domain *domain) {
> > +	int i, ret;
> > +
> > +	for (i = 0; ; i++) {
> > +		struct clk *clk = of_clk_get(domain->dev->of_node, i);
> > +		if (IS_ERR(clk))
> > +			break;
> > +		if (i >= MAX_CLK_NUM) {
> > +			dev_err(domain->dev, "more than %d clocks\n",
> > +				MAX_CLK_NUM);
> > +			ret = -EINVAL;
> > +			goto clk_err;
> > +		}
> > +		domain->clk[i] = clk;
> > +	}
> > +	domain->num_clks = i;
> > +
> > +	return 0;
> > +
> > +clk_err:
> > +	while (i--)
> > +		clk_put(domain->clk[i]);
> > +
> > +	return ret;
> > +}
> > +
> > +static void imx8m_pd_put_clocks(struct imx8m_pm_domain *domain) {
> > +	int i;
> > +
> > +	for (i = domain->num_clks - 1; i >= 0; i--)
> > +		clk_put(domain->clk[i]);
> > +}
> > +
> > +static const struct of_device_id imx8m_pm_domain_ids[] = {
> > +	{.compatible = "fsl,imx8m-pm-domain"},
> > +	{},
> > +};
> > +
> > +static int imx8m_pm_domain_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct imx8m_pm_domain *domain;
> > +	struct of_phandle_args parent, child;
> > +	int ret;
> > +
> > +	domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
> > +	if (!domain)
> > +		return -ENOMEM;
> > +
> > +	child.np = np;
> > +	domain->dev = dev;
> > +
> > +	ret = of_property_read_string(np, "domain-name", &domain->pd.name);
> > +	if (ret) {
> > +		dev_err(dev, "failed to get the domain name\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = of_property_read_u32(np, "domain-index",
> &domain->domain_index);
> > +	if (ret) {
> > +		dev_err(dev, "failed to get the domain index\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	domain->reg = devm_regulator_get_optional(dev, "power");
> > +	if (IS_ERR(domain->reg)) {
> > +		if (PTR_ERR(domain->reg) != -ENODEV) {
> > +			if (PTR_ERR(domain->reg) != -EPROBE_DEFER)
> > +				dev_err(dev, "failed to get domain's regulator\n");
> > +			return PTR_ERR(domain->reg);
> > +		}
> > +	}
> > +
> > +	ret = imx8m_pd_get_clocks(domain);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(dev, "failed to get domain's clocks\n");
> > +		return ret;
> > +	}
> > +
> > +	domain->pd.power_off = imx8m_pd_power_off;
> > +	domain->pd.power_on = imx8m_pd_power_on;
> > +
> > +	pm_genpd_init(&domain->pd, NULL, true);
> > +
> > +	ret = of_genpd_add_provider_simple(np, &domain->pd);
> > +	if (ret) {
> > +		dev_err(dev, "failed to add the domain provider\n");
> > +		pm_genpd_remove(&domain->pd);
> > +		imx8m_pd_put_clocks(domain);
> > +		return ret;
> > +	}
> > +
> > +	/* add it as subdomain if necessary */
> > +	if (!of_parse_phandle_with_args(np, "parent-domains",
> > +			"#power-domain-cells", 0, &parent)) {
> > +		ret = of_genpd_add_subdomain(&parent, &child);
> > +		of_node_put(parent.np);
> > +
> > +		if (ret < 0) {
> > +			dev_dbg(dev, "failed to add the subdomain: %s: %d",
> > +				domain->pd.name, ret);
> > +			of_genpd_del_provider(np);
> > +			pm_genpd_remove(&domain->pd);
> > +			imx8m_pd_put_clocks(domain);
> > +			return driver_deferred_probe_check_state(dev);
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver imx8m_pm_domain_driver = {
> > +	.driver = {
> > +		.name	= "imx8m_pm_domain",
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = imx8m_pm_domain_ids,
> > +	},
> > +	.probe = imx8m_pm_domain_probe,
> > +};
> > +module_platform_driver(imx8m_pm_domain_driver);
> > +
> > +MODULE_AUTHOR("NXP");
> > +MODULE_DESCRIPTION("NXP i.MX8M power domain driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/soc/imx/imx_sip.h b/include/soc/imx/imx_sip.h new
> > file mode 100644 index 00000000..6b96b33
> > --- /dev/null
> > +++ b/include/soc/imx/imx_sip.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright 2019 NXP
> > + */
> > +
> > +#ifndef __IMX_SIP_H__
> > +#define __IMX_SIP_H__
> > +
> > +#define IMX_SIP_GPC			0xC2000000
> > +#define IMX_SIP_CONFIG_GPC_PM_DOMAIN	0x03
> > +
> > +#endif /* __IMX_SIP_H__ */

Adam Ford April 30, 2020, 12:57 p.m. UTC | #3

On Mon, Apr 27, 2020 at 10:37 AM Jacky Bai <[e-mail]> wrote:
>
> > -----Original Message-----
> > From: Lucas Stach <[e-mail]>
> > Sent: Monday, April 27, 2020 11:11 PM
> > To: Abel Vesa <[e-mail]>; Jacky Bai <[e-mail]>; Shawn
> > Guo <[e-mail]>; Sascha Hauer <[e-mail]>; Liam
> > Girdwood <[e-mail]>; Mark Brown <[e-mail]>
> > Cc: Aisheng Dong <[e-mail]>; dl-linux-imx
> > <[e-mail]>; [e-mail]; Linux Kernel
> > Mailing List <[e-mail]>
> > Subject: Re: [PATCH] soc: imx: Add power domain driver support for i.mx8m
> > family
> >
> > Am Montag, den 27.04.2020, 17:58 +0300 schrieb Abel Vesa:
> > > From: Jacky Bai <[e-mail]>
> > >
> > > The i.MX8M family is a set of NXP product focus on delivering the
> > > latest and greatest video and audio experience combining
> > > state-of-the-art media-specific features with high-performance
> > > processing while optimized for lowest power consumption.
> > >
> > > i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all belong to
> > > this family. A GPC module is used to manage all the PU power domain
> > > on/off. But the situation is that the number of power domains & the
> > > power up sequence has significate difference on those SoCs. Even on
> > > the same SoC. The power up sequence still has big difference. It makes
> > > us hard to reuse the GPCv2 driver to cover the whole i.MX8M family.
> > > Each time a new SoC is supported in the mainline kernel, we need to
> > > modify the GPCv2 driver to support it. We need to add or modify
> > > hundred lines of code in worst case.
> > > It is a bad practice for the driver maintainability.
> > >
> > > This driver add a more generic power domain driver that the actual
> > > power on/off is done by TF-A code. the abstraction give us the
> > > possibility that using one driver to cover the whole i.MX8M family in
> > > kernel side.
> > >
> >
> > Again: what does this driver bring to the table, other than moving a fraction of
> > the power domain functionality into the firmware?
> >
> > The discussions on the last submissions of this driver already established that
> > we can't move all functionality for the power domains into the firmware, as
> > controlling regulators is probably not easy to do from this context. Also the
> > TF-A side implementation of this driver is "interesting" IMHO, it does stuff like
> > accessing the clock controller registers without any locking or other means of
> > mutual exclusion with the Linux kernel clock controller driver.
> >
>
> The clock handling is in kernel side through CCF, not in ATF. See the patch below.
>
> > Why can't we just extend the existing GPCv2 driver with support for the other
> > i.MX8M family members?
> >
>
> The reason that why I don’t like to extend the GPCv2 is that when doing domain on/off,
> We need to access some special control register in each domain & do some special flow,
> These control register(mediamix block control, vpumix block control) is not in GPC
> module's address range. No benefit to reuse the GPCv2. Only bring complexity to the
> GPCv2 driver each time a new SoC is added.
>
> Yes, the i.MX8M power domain support has been pending for a while. ARM guys rejected this patchset
> because they suggest us to use SCMI rather than SiP. But SCMI is only partial suitable for our
> case.

I have been anxiously waiting for this.  If I want to test this, what
ATF do we need to use?  I know for some of the DDRC stuff, we couldn't
use upstream ATF or U-Boot, so I had to use a custom version from
NXP's repo.  Ideally, it would be nice to have everything play nicely
together so we don't have to depend on external repos like the NXP
repo on Code Auroroa.  I am not complaining, but when I attempted to
push U-Boot, I mentioned that I needed a specific branch of ATF.  I
wanted that version of U-Boot for DDRC in the kernel, and now I'd like
to make sure I understand the requirements of this, so we can easily
build and test your patch.  You mention that that ARM rejected the
patch set, so I just want to make sure I know what I need to do.

thanks,

adam


>
> BR
> Jacky Bai
> > Regards,
> > Lucas
> >
> > > Signed-off-by: Jacky Bai <[e-mail]>
> > > Signed-off-by: Abel Vesa <[e-mail]>
> > > ---
> > >  drivers/soc/imx/Kconfig            |   6 +
> > >  drivers/soc/imx/Makefile           |   1 +
> > >  drivers/soc/imx/imx8m_pm_domains.c | 224
> > +++++++++++++++++++++++++++++++++++++
> > >  include/soc/imx/imx_sip.h          |  12 ++
> > >  4 files changed, 243 insertions(+)
> > >  create mode 100644 drivers/soc/imx/imx8m_pm_domains.c
> > >  create mode 100644 include/soc/imx/imx_sip.h
> > >
> > > diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig index
> > > d515d2c..7837199 100644
> > > --- a/drivers/soc/imx/Kconfig
> > > +++ b/drivers/soc/imx/Kconfig
> > > @@ -27,4 +27,10 @@ config SOC_IMX8M
> > >       support, it will provide the SoC info like SoC family,
> > >       ID and revision etc.
> > >
> > > +config IMX8M_PM_DOMAINS
> > > +   bool "i.MX8M PM domains"
> > > +   depends on ARCH_MXC || (COMPILE_TEST && OF)
> > > +   depends on PM
> > > +   select PM_GENERIC_DOMAINS
> > > +
> > >  endmenu
> > > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index
> > > 103e2c9..a22e24b 100644
> > > --- a/drivers/soc/imx/Makefile
> > > +++ b/drivers/soc/imx/Makefile
> > > @@ -3,3 +3,4 @@ obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
> > >  obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
> > >  obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> > >  obj-$(CONFIG_IMX_SCU_SOC) += soc-imx-scu.o
> > > +obj-$(CONFIG_IMX8M_PM_DOMAINS) += imx8m_pm_domains.o
> > > diff --git a/drivers/soc/imx/imx8m_pm_domains.c
> > > b/drivers/soc/imx/imx8m_pm_domains.c
> > > new file mode 100644
> > > index 00000000..ce06a05
> > > --- /dev/null
> > > +++ b/drivers/soc/imx/imx8m_pm_domains.c
> > > @@ -0,0 +1,224 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2019 NXP.
> > > + */
> > > +
> > > +#include <linux/arm-smccc.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_domain.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#include <soc/imx/imx_sip.h>
> > > +
> > > +#define MAX_CLK_NUM        6
> > > +#define to_imx8m_pm_domain(_genpd) container_of(_genpd, struct
> > > +imx8m_pm_domain, pd)
> > > +
> > > +
> > > +struct imx8m_pm_domain {
> > > +   struct device *dev;
> > > +   struct generic_pm_domain pd;
> > > +   u32 domain_index;
> > > +   struct clk *clk[MAX_CLK_NUM];
> > > +   unsigned int num_clks;
> > > +   struct regulator *reg;
> > > +};
> > > +
> > > +enum imx8m_pm_domain_state {
> > > +   PD_STATE_OFF,
> > > +   PD_STATE_ON,
> > > +};
> > > +
> > > +static DEFINE_MUTEX(gpc_pd_mutex);
> > > +
> > > +static int imx8m_pd_power_on(struct generic_pm_domain *genpd) {
> > > +   struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
> > > +   struct arm_smccc_res res;
> > > +   int index, ret = 0;
> > > +
> > > +   /* power on the external supply */
> > > +   if (!IS_ERR(domain->reg)) {
> > > +           ret = regulator_enable(domain->reg);
> > > +           if (ret) {
> > > +                   dev_warn(domain->dev, "failed to power up the reg%d\n", ret);
> > > +                   return ret;
> > > +           }
> > > +   }
> > > +
> > > +   /* enable the necessary clks needed by the power domain */
> > > +   if (domain->num_clks) {
> > > +           for (index = 0; index < domain->num_clks; index++)
> > > +                   clk_prepare_enable(domain->clk[index]);
> > > +   }
> > > +
> > > +   mutex_lock(&gpc_pd_mutex);
> > > +   arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN,
> > domain->domain_index,
> > > +                 PD_STATE_ON, 0, 0, 0, 0, &res);
> > > +   mutex_unlock(&gpc_pd_mutex);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static int imx8m_pd_power_off(struct generic_pm_domain *genpd) {
> > > +   struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
> > > +   struct arm_smccc_res res;
> > > +   int index, ret = 0;
> > > +
> > > +   mutex_lock(&gpc_pd_mutex);
> > > +   arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN,
> > domain->domain_index,
> > > +                 PD_STATE_OFF, 0, 0, 0, 0, &res);
> > > +   mutex_unlock(&gpc_pd_mutex);
> > > +
> > > +   /* power off the external supply */
> > > +   if (!IS_ERR(domain->reg)) {
> > > +           ret = regulator_disable(domain->reg);
> > > +           if (ret) {
> > > +                   dev_warn(domain->dev, "failed to power off the reg%d\n",
> > ret);
> > > +                   return ret;
> > > +           }
> > > +   }
> > > +
> > > +   /* disable clks when power domain is off */
> > > +   if (domain->num_clks) {
> > > +           for (index = 0; index < domain->num_clks; index++)
> > > +                   clk_disable_unprepare(domain->clk[index]);
> > > +   }
> > > +
> > > +   return ret;
> > > +};
> > > +
> > > +static int imx8m_pd_get_clocks(struct imx8m_pm_domain *domain) {
> > > +   int i, ret;
> > > +
> > > +   for (i = 0; ; i++) {
> > > +           struct clk *clk = of_clk_get(domain->dev->of_node, i);
> > > +           if (IS_ERR(clk))
> > > +                   break;
> > > +           if (i >= MAX_CLK_NUM) {
> > > +                   dev_err(domain->dev, "more than %d clocks\n",
> > > +                           MAX_CLK_NUM);
> > > +                   ret = -EINVAL;
> > > +                   goto clk_err;
> > > +           }
> > > +           domain->clk[i] = clk;
> > > +   }
> > > +   domain->num_clks = i;
> > > +
> > > +   return 0;
> > > +
> > > +clk_err:
> > > +   while (i--)
> > > +           clk_put(domain->clk[i]);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > > +static void imx8m_pd_put_clocks(struct imx8m_pm_domain *domain) {
> > > +   int i;
> > > +
> > > +   for (i = domain->num_clks - 1; i >= 0; i--)
> > > +           clk_put(domain->clk[i]);
> > > +}
> > > +
> > > +static const struct of_device_id imx8m_pm_domain_ids[] = {
> > > +   {.compatible = "fsl,imx8m-pm-domain"},
> > > +   {},
> > > +};
> > > +
> > > +static int imx8m_pm_domain_probe(struct platform_device *pdev) {
> > > +   struct device *dev = &pdev->dev;
> > > +   struct device_node *np = dev->of_node;
> > > +   struct imx8m_pm_domain *domain;
> > > +   struct of_phandle_args parent, child;
> > > +   int ret;
> > > +
> > > +   domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
> > > +   if (!domain)
> > > +           return -ENOMEM;
> > > +
> > > +   child.np = np;
> > > +   domain->dev = dev;
> > > +
> > > +   ret = of_property_read_string(np, "domain-name", &domain->pd.name);
> > > +   if (ret) {
> > > +           dev_err(dev, "failed to get the domain name\n");
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   ret = of_property_read_u32(np, "domain-index",
> > &domain->domain_index);
> > > +   if (ret) {
> > > +           dev_err(dev, "failed to get the domain index\n");
> > > +           return -EINVAL;
> > > +   }
> > > +
> > > +   domain->reg = devm_regulator_get_optional(dev, "power");
> > > +   if (IS_ERR(domain->reg)) {
> > > +           if (PTR_ERR(domain->reg) != -ENODEV) {
> > > +                   if (PTR_ERR(domain->reg) != -EPROBE_DEFER)
> > > +                           dev_err(dev, "failed to get domain's regulator\n");
> > > +                   return PTR_ERR(domain->reg);
> > > +           }
> > > +   }
> > > +
> > > +   ret = imx8m_pd_get_clocks(domain);
> > > +   if (ret) {
> > > +           if (ret != -EPROBE_DEFER)
> > > +                   dev_err(dev, "failed to get domain's clocks\n");
> > > +           return ret;
> > > +   }
> > > +
> > > +   domain->pd.power_off = imx8m_pd_power_off;
> > > +   domain->pd.power_on = imx8m_pd_power_on;
> > > +
> > > +   pm_genpd_init(&domain->pd, NULL, true);
> > > +
> > > +   ret = of_genpd_add_provider_simple(np, &domain->pd);
> > > +   if (ret) {
> > > +           dev_err(dev, "failed to add the domain provider\n");
> > > +           pm_genpd_remove(&domain->pd);
> > > +           imx8m_pd_put_clocks(domain);
> > > +           return ret;
> > > +   }
> > > +
> > > +   /* add it as subdomain if necessary */
> > > +   if (!of_parse_phandle_with_args(np, "parent-domains",
> > > +                   "#power-domain-cells", 0, &parent)) {
> > > +           ret = of_genpd_add_subdomain(&parent, &child);
> > > +           of_node_put(parent.np);
> > > +
> > > +           if (ret < 0) {
> > > +                   dev_dbg(dev, "failed to add the subdomain: %s: %d",
> > > +                           domain->pd.name, ret);
> > > +                   of_genpd_del_provider(np);
> > > +                   pm_genpd_remove(&domain->pd);
> > > +                   imx8m_pd_put_clocks(domain);
> > > +                   return driver_deferred_probe_check_state(dev);
> > > +           }
> > > +   }
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static struct platform_driver imx8m_pm_domain_driver = {
> > > +   .driver = {
> > > +           .name   = "imx8m_pm_domain",
> > > +           .owner  = THIS_MODULE,
> > > +           .of_match_table = imx8m_pm_domain_ids,
> > > +   },
> > > +   .probe = imx8m_pm_domain_probe,
> > > +};
> > > +module_platform_driver(imx8m_pm_domain_driver);
> > > +
> > > +MODULE_AUTHOR("NXP");
> > > +MODULE_DESCRIPTION("NXP i.MX8M power domain driver");
> > > +MODULE_LICENSE("GPL v2");
> > > diff --git a/include/soc/imx/imx_sip.h b/include/soc/imx/imx_sip.h new
> > > file mode 100644 index 00000000..6b96b33
> > > --- /dev/null
> > > +++ b/include/soc/imx/imx_sip.h
> > > @@ -0,0 +1,12 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright 2019 NXP
> > > + */
> > > +
> > > +#ifndef __IMX_SIP_H__
> > > +#define __IMX_SIP_H__
> > > +
> > > +#define IMX_SIP_GPC                        0xC2000000
> > > +#define IMX_SIP_CONFIG_GPC_PM_DOMAIN       0x03
> > > +
> > > +#endif /* __IMX_SIP_H__ */
>
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Lucas Stach May 4, 2020, 9:19 a.m. UTC | #4

Am Montag, den 27.04.2020, 15:37 +0000 schrieb Jacky Bai:
> > -----Original Message-----
> > From: Lucas Stach <[e-mail]>
> > Sent: Monday, April 27, 2020 11:11 PM
> > To: Abel Vesa <[e-mail]>; Jacky Bai <[e-mail]>; Shawn
> > Guo <[e-mail]>; Sascha Hauer <[e-mail]>; Liam
> > Girdwood <[e-mail]>; Mark Brown <[e-mail]>
> > Cc: Aisheng Dong <[e-mail]>; dl-linux-imx
> > <[e-mail]>; [e-mail]; Linux Kernel
> > Mailing List <[e-mail]>
> > Subject: Re: [PATCH] soc: imx: Add power domain driver support for i.mx8m
> > family
> > 
> > Am Montag, den 27.04.2020, 17:58 +0300 schrieb Abel Vesa:
> > > From: Jacky Bai <[e-mail]>
> > > 
> > > The i.MX8M family is a set of NXP product focus on delivering the
> > > latest and greatest video and audio experience combining
> > > state-of-the-art media-specific features with high-performance
> > > processing while optimized for lowest power consumption.
> > > 
> > > i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all belong to
> > > this family. A GPC module is used to manage all the PU power domain
> > > on/off. But the situation is that the number of power domains & the
> > > power up sequence has significate difference on those SoCs. Even on
> > > the same SoC. The power up sequence still has big difference. It makes
> > > us hard to reuse the GPCv2 driver to cover the whole i.MX8M family.
> > > Each time a new SoC is supported in the mainline kernel, we need to
> > > modify the GPCv2 driver to support it. We need to add or modify
> > > hundred lines of code in worst case.
> > > It is a bad practice for the driver maintainability.
> > > 
> > > This driver add a more generic power domain driver that the actual
> > > power on/off is done by TF-A code. the abstraction give us the
> > > possibility that using one driver to cover the whole i.MX8M family in
> > > kernel side.
> > > 
> > 
> > Again: what does this driver bring to the table, other than moving a fraction of
> > the power domain functionality into the firmware?
> > 
> > The discussions on the last submissions of this driver already established that
> > we can't move all functionality for the power domains into the firmware, as
> > controlling regulators is probably not easy to do from this context. Also the
> > TF-A side implementation of this driver is "interesting" IMHO, it does stuff like
> > accessing the clock controller registers without any locking or other means of
> > mutual exclusion with the Linux kernel clock controller driver.
> > 
> 
> The clock handling is in kernel side through CCF, not in ATF. See the patch below.
> 
> > Why can't we just extend the existing GPCv2 driver with support for the other
> > i.MX8M family members?
> > 
> 
> The reason that why I don’t like to extend the GPCv2 is that when doing domain on/off,
> We need to access some special control register in each domain & do some special flow,
> These control register(mediamix block control, vpumix block control) is not in GPC
> module's address range. No benefit to reuse the GPCv2. Only bring complexity to the
> GPCv2 driver each time a new SoC is added.
> 
> Yes, the i.MX8M power domain support has been pending for a while. ARM guys rejected this patchset
> because they suggest us to use SCMI rather than SiP. But SCMI is only partial suitable for our
> case.

Can you please point me to the most resent version of the TF-A side
implementation of this? The i.MX8MM implementation in the
imx_5.4.3_2.0.0 branch in the codeaurora imx-atf repo still contains
writes to the clock controller register range.

Also I would love to learn why the GPC needs to access Mediamix and
VPUmix domain registers. If you are talking about the NOC configuration
I would strongly suggest that those should be handled by a Linux side
interconnect driver, this has nothing to do with the power domain
sequencing, it just happens to lose state over the power down and needs
to be reprogrammed after power on. The NOC configuration though is use-
case dependent, so this should be properly handled in a rich OS driver.

Sure we needs to extend the Linux side GPC driver for each new SoC
generation, but that's no different from any other hardware driver in
Linux. Drivers are the abstraction around the hardware, there is no
need to invent another one if there are no clear benefits.

Regards,
Lucas

Tim Harvey May 21, 2020, 3:56 p.m. UTC | #5

On Mon, May 4, 2020 at 2:19 AM Lucas Stach <[e-mail]> wrote:
>
> Am Montag, den 27.04.2020, 15:37 +0000 schrieb Jacky Bai:
> > > -----Original Message-----
> > > From: Lucas Stach <[e-mail]>
> > > Sent: Monday, April 27, 2020 11:11 PM
> > > To: Abel Vesa <[e-mail]>; Jacky Bai <[e-mail]>; Shawn
> > > Guo <[e-mail]>; Sascha Hauer <[e-mail]>; Liam
> > > Girdwood <[e-mail]>; Mark Brown <[e-mail]>
> > > Cc: Aisheng Dong <[e-mail]>; dl-linux-imx
> > > <[e-mail]>; [e-mail]; Linux Kernel
> > > Mailing List <[e-mail]>
> > > Subject: Re: [PATCH] soc: imx: Add power domain driver support for i.mx8m
> > > family
> > >
> > > Am Montag, den 27.04.2020, 17:58 +0300 schrieb Abel Vesa:
> > > > From: Jacky Bai <[e-mail]>
> > > >
> > > > The i.MX8M family is a set of NXP product focus on delivering the
> > > > latest and greatest video and audio experience combining
> > > > state-of-the-art media-specific features with high-performance
> > > > processing while optimized for lowest power consumption.
> > > >
> > > > i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all belong to
> > > > this family. A GPC module is used to manage all the PU power domain
> > > > on/off. But the situation is that the number of power domains & the
> > > > power up sequence has significate difference on those SoCs. Even on
> > > > the same SoC. The power up sequence still has big difference. It makes
> > > > us hard to reuse the GPCv2 driver to cover the whole i.MX8M family.
> > > > Each time a new SoC is supported in the mainline kernel, we need to
> > > > modify the GPCv2 driver to support it. We need to add or modify
> > > > hundred lines of code in worst case.
> > > > It is a bad practice for the driver maintainability.
> > > >
> > > > This driver add a more generic power domain driver that the actual
> > > > power on/off is done by TF-A code. the abstraction give us the
> > > > possibility that using one driver to cover the whole i.MX8M family in
> > > > kernel side.
> > > >
> > >
> > > Again: what does this driver bring to the table, other than moving a fraction of
> > > the power domain functionality into the firmware?
> > >
> > > The discussions on the last submissions of this driver already established that
> > > we can't move all functionality for the power domains into the firmware, as
> > > controlling regulators is probably not easy to do from this context. Also the
> > > TF-A side implementation of this driver is "interesting" IMHO, it does stuff like
> > > accessing the clock controller registers without any locking or other means of
> > > mutual exclusion with the Linux kernel clock controller driver.
> > >
> >
> > The clock handling is in kernel side through CCF, not in ATF. See the patch below.
> >
> > > Why can't we just extend the existing GPCv2 driver with support for the other
> > > i.MX8M family members?
> > >
> >
> > The reason that why I don’t like to extend the GPCv2 is that when doing domain on/off,
> > We need to access some special control register in each domain & do some special flow,
> > These control register(mediamix block control, vpumix block control) is not in GPC
> > module's address range. No benefit to reuse the GPCv2. Only bring complexity to the
> > GPCv2 driver each time a new SoC is added.
> >
> > Yes, the i.MX8M power domain support has been pending for a while. ARM guys rejected this patchset
> > because they suggest us to use SCMI rather than SiP. But SCMI is only partial suitable for our
> > case.
>
> Can you please point me to the most resent version of the TF-A side
> implementation of this? The i.MX8MM implementation in the
> imx_5.4.3_2.0.0 branch in the codeaurora imx-atf repo still contains
> writes to the clock controller register range.
>
> Also I would love to learn why the GPC needs to access Mediamix and
> VPUmix domain registers. If you are talking about the NOC configuration
> I would strongly suggest that those should be handled by a Linux side
> interconnect driver, this has nothing to do with the power domain
> sequencing, it just happens to lose state over the power down and needs
> to be reprogrammed after power on. The NOC configuration though is use-
> case dependent, so this should be properly handled in a rich OS driver.
>
> Sure we needs to extend the Linux side GPC driver for each new SoC
> generation, but that's no different from any other hardware driver in
> Linux. Drivers are the abstraction around the hardware, there is no
> need to invent another one if there are no clear benefits.
>

Jacky / Abel,

Any movement on this? As I see it the lack of imx8mm power-domain
support in the kernel is holding up USB, PCIe, VPU, and perhaps
GPU/CSI as well. I would tend to agree that hiding this functionality
in the TF-A is probably not the best approach especially as that
requires a NXP version of the TF-A. I really don't see the issue with
the gpc driver getting a little more complicated if it needs to. There
is bound to be some complication as there is such a large variation of
IMX8 products out there! (talk about confusing!).

Best Regards,

Tim

Adam Ford May 21, 2020, 4:02 p.m. UTC | #6

On Thu, May 21, 2020 at 10:56 AM Tim Harvey <[e-mail]> wrote:
>
> On Mon, May 4, 2020 at 2:19 AM Lucas Stach <[e-mail]> wrote:
> >
> > Am Montag, den 27.04.2020, 15:37 +0000 schrieb Jacky Bai:
> > > > -----Original Message-----
> > > > From: Lucas Stach <[e-mail]>
> > > > Sent: Monday, April 27, 2020 11:11 PM
> > > > To: Abel Vesa <[e-mail]>; Jacky Bai <[e-mail]>; Shawn
> > > > Guo <[e-mail]>; Sascha Hauer <[e-mail]>; Liam
> > > > Girdwood <[e-mail]>; Mark Brown <[e-mail]>
> > > > Cc: Aisheng Dong <[e-mail]>; dl-linux-imx
> > > > <[e-mail]>; [e-mail]; Linux Kernel
> > > > Mailing List <[e-mail]>
> > > > Subject: Re: [PATCH] soc: imx: Add power domain driver support for i.mx8m
> > > > family
> > > >
> > > > Am Montag, den 27.04.2020, 17:58 +0300 schrieb Abel Vesa:
> > > > > From: Jacky Bai <[e-mail]>
> > > > >
> > > > > The i.MX8M family is a set of NXP product focus on delivering the
> > > > > latest and greatest video and audio experience combining
> > > > > state-of-the-art media-specific features with high-performance
> > > > > processing while optimized for lowest power consumption.
> > > > >
> > > > > i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all belong to
> > > > > this family. A GPC module is used to manage all the PU power domain
> > > > > on/off. But the situation is that the number of power domains & the
> > > > > power up sequence has significate difference on those SoCs. Even on
> > > > > the same SoC. The power up sequence still has big difference. It makes
> > > > > us hard to reuse the GPCv2 driver to cover the whole i.MX8M family.
> > > > > Each time a new SoC is supported in the mainline kernel, we need to
> > > > > modify the GPCv2 driver to support it. We need to add or modify
> > > > > hundred lines of code in worst case.
> > > > > It is a bad practice for the driver maintainability.
> > > > >
> > > > > This driver add a more generic power domain driver that the actual
> > > > > power on/off is done by TF-A code. the abstraction give us the
> > > > > possibility that using one driver to cover the whole i.MX8M family in
> > > > > kernel side.
> > > > >
> > > >
> > > > Again: what does this driver bring to the table, other than moving a fraction of
> > > > the power domain functionality into the firmware?
> > > >
> > > > The discussions on the last submissions of this driver already established that
> > > > we can't move all functionality for the power domains into the firmware, as
> > > > controlling regulators is probably not easy to do from this context. Also the
> > > > TF-A side implementation of this driver is "interesting" IMHO, it does stuff like
> > > > accessing the clock controller registers without any locking or other means of
> > > > mutual exclusion with the Linux kernel clock controller driver.
> > > >
> > >
> > > The clock handling is in kernel side through CCF, not in ATF. See the patch below.
> > >
> > > > Why can't we just extend the existing GPCv2 driver with support for the other
> > > > i.MX8M family members?
> > > >
> > >
> > > The reason that why I don’t like to extend the GPCv2 is that when doing domain on/off,
> > > We need to access some special control register in each domain & do some special flow,
> > > These control register(mediamix block control, vpumix block control) is not in GPC
> > > module's address range. No benefit to reuse the GPCv2. Only bring complexity to the
> > > GPCv2 driver each time a new SoC is added.
> > >
> > > Yes, the i.MX8M power domain support has been pending for a while. ARM guys rejected this patchset
> > > because they suggest us to use SCMI rather than SiP. But SCMI is only partial suitable for our
> > > case.
> >
> > Can you please point me to the most resent version of the TF-A side
> > implementation of this? The i.MX8MM implementation in the
> > imx_5.4.3_2.0.0 branch in the codeaurora imx-atf repo still contains
> > writes to the clock controller register range.
> >
> > Also I would love to learn why the GPC needs to access Mediamix and
> > VPUmix domain registers. If you are talking about the NOC configuration
> > I would strongly suggest that those should be handled by a Linux side
> > interconnect driver, this has nothing to do with the power domain
> > sequencing, it just happens to lose state over the power down and needs
> > to be reprogrammed after power on. The NOC configuration though is use-
> > case dependent, so this should be properly handled in a rich OS driver.
> >
> > Sure we needs to extend the Linux side GPC driver for each new SoC
> > generation, but that's no different from any other hardware driver in
> > Linux. Drivers are the abstraction around the hardware, there is no
> > need to invent another one if there are no clear benefits.
> >
>
> Jacky / Abel,
>
> Any movement on this? As I see it the lack of imx8mm power-domain
> support in the kernel is holding up USB, PCIe, VPU, and perhaps
> GPU/CSI as well. I would tend to agree that hiding this functionality
> in the TF-A is probably not the best approach especially as that
> requires a NXP version of the TF-A. I really don't see the issue with
> the gpc driver getting a little more complicated if it needs to. There
> is bound to be some complication as there is such a large variation of
> IMX8 products out there! (talk about confusing!).

NXP -

Please don't let perfect be the enemy of the good.  We have some
patches floating around which enable much of the functionality.  It
may not be perfect, but it's an improvement from what we currently
have - which is nothing.  Please let the patches in, and when the
alternative methods become available, we can remove these.

If TF_A is arguing about how to push the patches upstream, then maybe
Tim is wright and it's not a good idea, and we should consider
something else.

adam

>
> Best Regards,
>
> Tim

Patch

diff --git a/drivers/soc/imx/Kconfig b/drivers/soc/imx/Kconfig
index d515d2c..7837199 100644
--- a/drivers/soc/imx/Kconfig
+++ b/drivers/soc/imx/Kconfig
@@ -27,4 +27,10 @@  config SOC_IMX8M
 	  support, it will provide the SoC info like SoC family,
 	  ID and revision etc.
 
+config IMX8M_PM_DOMAINS
+	bool "i.MX8M PM domains"
+	depends on ARCH_MXC || (COMPILE_TEST && OF)
+	depends on PM
+	select PM_GENERIC_DOMAINS
+
 endmenu
diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile
index 103e2c9..a22e24b 100644
--- a/drivers/soc/imx/Makefile
+++ b/drivers/soc/imx/Makefile
@@ -3,3 +3,4 @@  obj-$(CONFIG_HAVE_IMX_GPC) += gpc.o
 obj-$(CONFIG_IMX_GPCV2_PM_DOMAINS) += gpcv2.o
 obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
 obj-$(CONFIG_IMX_SCU_SOC) += soc-imx-scu.o
+obj-$(CONFIG_IMX8M_PM_DOMAINS) += imx8m_pm_domains.o
diff --git a/drivers/soc/imx/imx8m_pm_domains.c b/drivers/soc/imx/imx8m_pm_domains.c
new file mode 100644
index 00000000..ce06a05
--- /dev/null
+++ b/drivers/soc/imx/imx8m_pm_domains.c
@@ -0,0 +1,224 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/regulator/consumer.h>
+
+#include <soc/imx/imx_sip.h>
+
+#define MAX_CLK_NUM	6
+#define to_imx8m_pm_domain(_genpd) container_of(_genpd, struct imx8m_pm_domain, pd)
+
+
+struct imx8m_pm_domain {
+	struct device *dev;
+	struct generic_pm_domain pd;
+	u32 domain_index;
+	struct clk *clk[MAX_CLK_NUM];
+	unsigned int num_clks;
+	struct regulator *reg;
+};
+
+enum imx8m_pm_domain_state {
+	PD_STATE_OFF,
+	PD_STATE_ON,
+};
+
+static DEFINE_MUTEX(gpc_pd_mutex);
+
+static int imx8m_pd_power_on(struct generic_pm_domain *genpd)
+{
+	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
+	struct arm_smccc_res res;
+	int index, ret = 0;
+
+	/* power on the external supply */
+	if (!IS_ERR(domain->reg)) {
+		ret = regulator_enable(domain->reg);
+		if (ret) {
+			dev_warn(domain->dev, "failed to power up the reg%d\n", ret);
+			return ret;
+		}
+	}
+
+	/* enable the necessary clks needed by the power domain */
+	if (domain->num_clks) {
+		for (index = 0; index < domain->num_clks; index++)
+			clk_prepare_enable(domain->clk[index]);
+	}
+
+	mutex_lock(&gpc_pd_mutex);
+	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN, domain->domain_index,
+		      PD_STATE_ON, 0, 0, 0, 0, &res);
+	mutex_unlock(&gpc_pd_mutex);
+
+	return 0;
+}
+
+static int imx8m_pd_power_off(struct generic_pm_domain *genpd)
+{
+	struct imx8m_pm_domain *domain = to_imx8m_pm_domain(genpd);
+	struct arm_smccc_res res;
+	int index, ret = 0;
+
+	mutex_lock(&gpc_pd_mutex);
+	arm_smccc_smc(IMX_SIP_GPC, IMX_SIP_CONFIG_GPC_PM_DOMAIN, domain->domain_index,
+		      PD_STATE_OFF, 0, 0, 0, 0, &res);
+	mutex_unlock(&gpc_pd_mutex);
+
+	/* power off the external supply */
+	if (!IS_ERR(domain->reg)) {
+		ret = regulator_disable(domain->reg);
+		if (ret) {
+			dev_warn(domain->dev, "failed to power off the reg%d\n", ret);
+			return ret;
+		}
+	}
+
+	/* disable clks when power domain is off */
+	if (domain->num_clks) {
+		for (index = 0; index < domain->num_clks; index++)
+			clk_disable_unprepare(domain->clk[index]);
+	}
+
+	return ret;
+};
+
+static int imx8m_pd_get_clocks(struct imx8m_pm_domain *domain)
+{
+	int i, ret;
+
+	for (i = 0; ; i++) {
+		struct clk *clk = of_clk_get(domain->dev->of_node, i);
+		if (IS_ERR(clk))
+			break;
+		if (i >= MAX_CLK_NUM) {
+			dev_err(domain->dev, "more than %d clocks\n",
+				MAX_CLK_NUM);
+			ret = -EINVAL;
+			goto clk_err;
+		}
+		domain->clk[i] = clk;
+	}
+	domain->num_clks = i;
+
+	return 0;
+
+clk_err:
+	while (i--)
+		clk_put(domain->clk[i]);
+
+	return ret;
+}
+
+static void imx8m_pd_put_clocks(struct imx8m_pm_domain *domain)
+{
+	int i;
+
+	for (i = domain->num_clks - 1; i >= 0; i--)
+		clk_put(domain->clk[i]);
+}
+
+static const struct of_device_id imx8m_pm_domain_ids[] = {
+	{.compatible = "fsl,imx8m-pm-domain"},
+	{},
+};
+
+static int imx8m_pm_domain_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct imx8m_pm_domain *domain;
+	struct of_phandle_args parent, child;
+	int ret;
+
+	domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
+	if (!domain)
+		return -ENOMEM;
+
+	child.np = np;
+	domain->dev = dev;
+
+	ret = of_property_read_string(np, "domain-name", &domain->pd.name);
+	if (ret) {
+		dev_err(dev, "failed to get the domain name\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(np, "domain-index", &domain->domain_index);
+	if (ret) {
+		dev_err(dev, "failed to get the domain index\n");
+		return -EINVAL;
+	}
+
+	domain->reg = devm_regulator_get_optional(dev, "power");
+	if (IS_ERR(domain->reg)) {
+		if (PTR_ERR(domain->reg) != -ENODEV) {
+			if (PTR_ERR(domain->reg) != -EPROBE_DEFER)
+				dev_err(dev, "failed to get domain's regulator\n");
+			return PTR_ERR(domain->reg);
+		}
+	}
+
+	ret = imx8m_pd_get_clocks(domain);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get domain's clocks\n");
+		return ret;
+	}
+
+	domain->pd.power_off = imx8m_pd_power_off;
+	domain->pd.power_on = imx8m_pd_power_on;
+
+	pm_genpd_init(&domain->pd, NULL, true);
+
+	ret = of_genpd_add_provider_simple(np, &domain->pd);
+	if (ret) {
+		dev_err(dev, "failed to add the domain provider\n");
+		pm_genpd_remove(&domain->pd);
+		imx8m_pd_put_clocks(domain);
+		return ret;
+	}
+
+	/* add it as subdomain if necessary */
+	if (!of_parse_phandle_with_args(np, "parent-domains",
+			"#power-domain-cells", 0, &parent)) {
+		ret = of_genpd_add_subdomain(&parent, &child);
+		of_node_put(parent.np);
+
+		if (ret < 0) {
+			dev_dbg(dev, "failed to add the subdomain: %s: %d",
+				domain->pd.name, ret);
+			of_genpd_del_provider(np);
+			pm_genpd_remove(&domain->pd);
+			imx8m_pd_put_clocks(domain);
+			return driver_deferred_probe_check_state(dev);
+		}
+	}
+
+	return 0;
+}
+
+static struct platform_driver imx8m_pm_domain_driver = {
+	.driver = {
+		.name	= "imx8m_pm_domain",
+		.owner	= THIS_MODULE,
+		.of_match_table = imx8m_pm_domain_ids,
+	},
+	.probe = imx8m_pm_domain_probe,
+};
+module_platform_driver(imx8m_pm_domain_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP i.MX8M power domain driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/soc/imx/imx_sip.h b/include/soc/imx/imx_sip.h
new file mode 100644
index 00000000..6b96b33
--- /dev/null
+++ b/include/soc/imx/imx_sip.h
@@ -0,0 +1,12 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2019 NXP
+ */
+
+#ifndef __IMX_SIP_H__
+#define __IMX_SIP_H__
+
+#define IMX_SIP_GPC			0xC2000000
+#define IMX_SIP_CONFIG_GPC_PM_DOMAIN	0x03
+
+#endif /* __IMX_SIP_H__ */