[PATCH 0/2] soc: imx: gpc: Power off PU domain in suspend/resume on 6qp

[PATCH 0/2] soc: imx: gpc: Power off PU domain in suspend/resume on 6qp

[Leonard Crestez]

Tested by doing `rtcwake -s 5 -m mem` while running glxgears on etnaviv.

The first patch is required because otherwise it is not easy to reach pgc
domains from the gpc itself when using new-style bindings. It's also
easier to understand.

The use of dynamic allocation in this driver is strange. Since there is
only one GPC physically present in each soc my impulse would be to make
most things global and delete imx_gpc_driver.remove entirely.

With current code (even without my patches) attempting to dynamically
remove/probe the GPC fils since since the per-pgc platform_device
instances are not removed. I'm trying something like this:

echo 130000.gpu > /sys/bus/platform/drivers/etnaviv-gpu/unbind
echo 134000.gpu > /sys/bus/platform/drivers/etnaviv-gpu/unbind
echo 20dc000.gpc  > /sys/bus/platform/drivers/imx-gpc/unbind
echo 20dc000.gpc  > /sys/bus/platform/drivers/imx-gpc/bind

But is there any usecase for dynamically removing the GPC? Instead of
trying to fix it I'd rather delete imx_gpc_driver.remove, just like
for gpcv2. Would anyone object to a patch doing this?

This series is not very pretty, constructive suggestions is welcome.
NXP internal tree has quite a lot of changes in gpc code and this causes
a lot of trouble when doing upgrades so I am trying to push some of the
internal features upstream.

Maybe instead of direct calls from mach-imx the gpc could implement
SLEEP_PM_OPS instead? It would still need a way to access the pgc
devices directly.

Leonard Crestez (2):
  soc: imx: gpc: Use static platform_device instances
  soc: imx: gpc: Power off PU domain in suspend/resume on 6qp

 arch/arm/mach-imx/gpc.c | 10 +++++
 drivers/soc/imx/gpc.c   | 93 ++++++++++++++++++++++++++---------------
 2 files changed, 70 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH 1/2] soc: imx: gpc: Use static platform_device instances

[Leonard Crestez]

Simplify the code by doing less dynamic allocation.

This also allows easier direct manipulation of individual power domains.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/soc/imx/gpc.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 32f0748fd067..83cb275592e9 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -283,10 +283,24 @@ static struct imx_pm_domain imx_gpc_domains[] = {
 		.reg_offs = 0x200,
 		.cntr_pdn_bit = 6,
 	},
 };
 
+#define DEFINE_IMX_GPC_PDEV(_id) \
+	{ \
+		.name	= "imx-pgc-power-domain", \
+		.id	= _id, \
+		.dev	= { .platform_data = &imx_gpc_domains[_id] }, \
+	}
+
+static struct platform_device imx_pgc_pdev[] = {
+	DEFINE_IMX_GPC_PDEV(0),
+	DEFINE_IMX_GPC_PDEV(1),
+	DEFINE_IMX_GPC_PDEV(2),
+	DEFINE_IMX_GPC_PDEV(3),
+};
+
 struct imx_gpc_dt_data {
 	int num_domains;
 	bool err009619_present;
 };
 
@@ -441,35 +455,20 @@ static int imx_gpc_probe(struct platform_device *pdev)
 				return ret;
 			}
 			if (domain_index >= of_id_data->num_domains)
 				continue;
 
-			pd_pdev = platform_device_alloc("imx-pgc-power-domain",
-							domain_index);
-			if (!pd_pdev) {
-				of_node_put(np);
-				return -ENOMEM;
-			}
-
-			ret = platform_device_add_data(pd_pdev,
-						       &imx_gpc_domains[domain_index],
-						       sizeof(imx_gpc_domains[domain_index]));
-			if (ret) {
-				platform_device_put(pd_pdev);
-				of_node_put(np);
-				return ret;
-			}
-			domain = pd_pdev->dev.platform_data;
+			domain = &imx_gpc_domains[domain_index];
 			domain->regmap = regmap;
 			domain->ipg_rate_mhz = ipg_rate_mhz;
 
+			pd_pdev = &imx_pgc_pdev[domain_index];
 			pd_pdev->dev.parent = &pdev->dev;
 			pd_pdev->dev.of_node = np;
 
-			ret = platform_device_add(pd_pdev);
+			ret = platform_device_register(pd_pdev);
 			if (ret) {
-				platform_device_put(pd_pdev);
 				of_node_put(np);
 				return ret;
 			}
 		}
 	}
-- 
2.17.1

[PATCH 2/2] soc: imx: gpc: Power off PU domain in suspend/resume on 6qp

[Leonard Crestez]

On imx6qp power gating on the PU domain is disabled because of errata
ERR009619. However power gating on suspend/resume can still work.

Enable this by calling the on/off functions directly from suspend code in
mach-imx.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
 arch/arm/mach-imx/gpc.c | 10 +++++++
 drivers/soc/imx/gpc.c   | 58 ++++++++++++++++++++++++++++++-----------
 2 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
index de535cb679b3..e99258238210 100644
--- a/arch/arm/mach-imx/gpc.c
+++ b/arch/arm/mach-imx/gpc.c
@@ -32,10 +32,14 @@
 
 static void __iomem *gpc_base;
 static u32 gpc_wake_irqs[IMR_NUM];
 static u32 gpc_saved_imrs[IMR_NUM];
 
+/* implemented in drivers/soc/imx/gpc.c */
+extern void _imx6_pm_pu_power_off(void);
+extern void _imx6_pm_pu_power_on(void);
+
 void imx_gpc_set_arm_power_up_timing(u32 sw2iso, u32 sw)
 {
 	writel_relaxed((sw2iso << GPC_PGC_SW2ISO_SHIFT) |
 		(sw << GPC_PGC_SW_SHIFT), gpc_base + GPC_PGC_CPU_PUPSCR);
 }
@@ -54,10 +58,13 @@ void imx_gpc_set_arm_power_in_lpm(bool power_off)
 void imx_gpc_pre_suspend(bool arm_power_off)
 {
 	void __iomem *reg_imr1 = gpc_base + GPC_IMR1;
 	int i;
 
+	if (cpu_is_imx6q() && imx_get_soc_revision() >= IMX_CHIP_REVISION_2_0)
+		_imx6_pm_pu_power_off();
+
 	/* Tell GPC to power off ARM core when suspend */
 	if (arm_power_off)
 		imx_gpc_set_arm_power_in_lpm(arm_power_off);
 
 	for (i = 0; i < IMR_NUM; i++) {
@@ -69,10 +76,13 @@ void imx_gpc_pre_suspend(bool arm_power_off)
 void imx_gpc_post_resume(void)
 {
 	void __iomem *reg_imr1 = gpc_base + GPC_IMR1;
 	int i;
 
+	if (cpu_is_imx6q() && imx_get_soc_revision() >= IMX_CHIP_REVISION_2_0)
+		_imx6_pm_pu_power_on();
+
 	/* Keep ARM core powered on for other low-power modes */
 	imx_gpc_set_arm_power_in_lpm(false);
 
 	for (i = 0; i < IMR_NUM; i++)
 		writel_relaxed(gpc_saved_imrs[i], reg_imr1 + i * 4);
diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
index 83cb275592e9..dddc2469eaac 100644
--- a/drivers/soc/imx/gpc.c
+++ b/drivers/soc/imx/gpc.c
@@ -54,19 +54,16 @@ static inline struct imx_pm_domain *
 to_imx_pm_domain(struct generic_pm_domain *genpd)
 {
 	return container_of(genpd, struct imx_pm_domain, base);
 }
 
-static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
+static void _imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
 {
 	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
 	int iso, iso2sw;
 	u32 val;
 
-	if (pd->flags & PGC_DOMAIN_FLAG_NO_PD)
-		return -EBUSY;
-
 	/* Read ISO and ISO2SW power down delays */
 	regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
 	iso = val & 0x3f;
 	iso2sw = (val >> 8) & 0x3f;
 
@@ -78,32 +75,33 @@ static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
 	val = BIT(pd->cntr_pdn_bit);
 	regmap_update_bits(pd->regmap, GPC_CNTR, val, val);
 
 	/* Wait ISO + ISO2SW IPG clock cycles */
 	udelay(DIV_ROUND_UP(iso + iso2sw, pd->ipg_rate_mhz));
+}
+
+static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
+{
+	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
+
+	if (to_imx_pm_domain(genpd)->flags & PGC_DOMAIN_FLAG_NO_PD)
+		return -EBUSY;
+
+	_imx6_pm_domain_power_off(genpd);
 
 	if (pd->supply)
 		regulator_disable(pd->supply);
 
 	return 0;
 }
 
-static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
+static void _imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
 {
 	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
-	int i, ret, sw, sw2iso;
+	int i, sw, sw2iso;
 	u32 val;
 
-	if (pd->supply) {
-		ret = regulator_enable(pd->supply);
-		if (ret) {
-			pr_err("%s: failed to enable regulator: %d\n",
-			       __func__, ret);
-			return ret;
-		}
-	}
-
 	/* Enable reset clocks for all devices in the domain */
 	for (i = 0; i < pd->num_clks; i++)
 		clk_prepare_enable(pd->clk[i]);
 
 	/* Gate off domain when powered down */
@@ -123,10 +121,27 @@ static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
 	udelay(DIV_ROUND_UP(sw + sw2iso, pd->ipg_rate_mhz));
 
 	/* Disable reset clocks for all devices in the domain */
 	for (i = 0; i < pd->num_clks; i++)
 		clk_disable_unprepare(pd->clk[i]);
+}
+
+static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
+{
+	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
+	int ret;
+
+	if (pd->supply) {
+		ret = regulator_enable(pd->supply);
+		if (ret) {
+			pr_err("%s: failed to enable regulator: %d\n",
+			       __func__, ret);
+			return ret;
+		}
+	}
+
+	_imx6_pm_domain_power_on(genpd);
 
 	return 0;
 }
 
 static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)
@@ -337,10 +352,23 @@ static const struct regmap_config imx_gpc_regmap_config = {
 	.val_bits = 32,
 	.reg_stride = 4,
 	.max_register = 0x2ac,
 };
 
+/* exported for suspend/resume code in arch/arm/mach-imx/gpc.c */
+void _imx6_pm_pu_power_off(void)
+{
+	_imx6_pm_domain_power_off(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base);
+}
+EXPORT_SYMBOL_GPL(_imx6_pm_pu_power_off);
+
+void _imx6_pm_pu_power_on(void)
+{
+	_imx6_pm_domain_power_on(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base);
+}
+EXPORT_SYMBOL_GPL(_imx6_pm_pu_power_on);
+
 static struct generic_pm_domain *imx_gpc_onecell_domains[] = {
 	&imx_gpc_domains[0].base,
 	&imx_gpc_domains[1].base,
 };
 
-- 
2.17.1

[PATCH 2/2] soc: imx: gpc: Power off PU domain in suspend/resume on 6qp

[Lucas Stach]

Am Montag, den 02.07.2018, 14:52 +0300 schrieb Leonard Crestez:
> On imx6qp power gating on the PU domain is disabled because of errata
> ERR009619. However power gating on suspend/resume can still work.
> 
> Enable this by calling the on/off functions directly from suspend code in
> mach-imx.

Sorry, but no. Please keep those two parts split. We don't want them to
interact.

What keeps you from using standard system suspend PM ops in the GPC
driver? The domain drivers within the GPC power management code are all
standard platform devices, so can implement PM ops themselves. There is
no need for any of this reaching between different drivers.

Regards,
Lucas

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> ---
> ?arch/arm/mach-imx/gpc.c | 10 +++++++
> ?drivers/soc/imx/gpc.c???| 58 ++++++++++++++++++++++++++++++-----------
> ?2 files changed, 53 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/gpc.c b/arch/arm/mach-imx/gpc.c
> index de535cb679b3..e99258238210 100644
> --- a/arch/arm/mach-imx/gpc.c
> +++ b/arch/arm/mach-imx/gpc.c
> @@ -32,10 +32,14 @@
> ?
> ?static void __iomem *gpc_base;
> ?static u32 gpc_wake_irqs[IMR_NUM];
> ?static u32 gpc_saved_imrs[IMR_NUM];
> ?
> +/* implemented in drivers/soc/imx/gpc.c */
> +extern void _imx6_pm_pu_power_off(void);
> +extern void _imx6_pm_pu_power_on(void);
> +
> ?void imx_gpc_set_arm_power_up_timing(u32 sw2iso, u32 sw)
> ?{
> > ?	writel_relaxed((sw2iso << GPC_PGC_SW2ISO_SHIFT) |
> > ?		(sw << GPC_PGC_SW_SHIFT), gpc_base + GPC_PGC_CPU_PUPSCR);
> ?}
> @@ -54,10 +58,13 @@ void imx_gpc_set_arm_power_in_lpm(bool power_off)
> ?void imx_gpc_pre_suspend(bool arm_power_off)
> ?{
> > ?	void __iomem *reg_imr1 = gpc_base + GPC_IMR1;
> > ?	int i;
> ?
> > +	if (cpu_is_imx6q() && imx_get_soc_revision() >= IMX_CHIP_REVISION_2_0)
> > +		_imx6_pm_pu_power_off();
> +
> > ?	/* Tell GPC to power off ARM core when suspend */
> > ?	if (arm_power_off)
> > ?		imx_gpc_set_arm_power_in_lpm(arm_power_off);
> ?
> > ?	for (i = 0; i < IMR_NUM; i++) {
> @@ -69,10 +76,13 @@ void imx_gpc_pre_suspend(bool arm_power_off)
> ?void imx_gpc_post_resume(void)
> ?{
> > ?	void __iomem *reg_imr1 = gpc_base + GPC_IMR1;
> > ?	int i;
> ?
> > +	if (cpu_is_imx6q() && imx_get_soc_revision() >= IMX_CHIP_REVISION_2_0)
> > +		_imx6_pm_pu_power_on();
> +
> > ?	/* Keep ARM core powered on for other low-power modes */
> > ?	imx_gpc_set_arm_power_in_lpm(false);
> ?
> > ?	for (i = 0; i < IMR_NUM; i++)
> > ?		writel_relaxed(gpc_saved_imrs[i], reg_imr1 + i * 4);
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index 83cb275592e9..dddc2469eaac 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -54,19 +54,16 @@ static inline struct imx_pm_domain *
> ?to_imx_pm_domain(struct generic_pm_domain *genpd)
> ?{
> > ?	return container_of(genpd, struct imx_pm_domain, base);
> ?}
> ?
> -static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
> +static void _imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
> ?{
> > ?	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
> > ?	int iso, iso2sw;
> > ?	u32 val;
> ?
> > -	if (pd->flags & PGC_DOMAIN_FLAG_NO_PD)
> > -		return -EBUSY;
> -
> > ?	/* Read ISO and ISO2SW power down delays */
> > ?	regmap_read(pd->regmap, pd->reg_offs + GPC_PGC_PUPSCR_OFFS, &val);
> > ?	iso = val & 0x3f;
> > ?	iso2sw = (val >> 8) & 0x3f;
> ?
> @@ -78,32 +75,33 @@ static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
> > ?	val = BIT(pd->cntr_pdn_bit);
> > ?	regmap_update_bits(pd->regmap, GPC_CNTR, val, val);
> ?
> > ?	/* Wait ISO + ISO2SW IPG clock cycles */
> > ?	udelay(DIV_ROUND_UP(iso + iso2sw, pd->ipg_rate_mhz));
> +}
> +
> +static int imx6_pm_domain_power_off(struct generic_pm_domain *genpd)
> +{
> > +	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
> +
> > +	if (to_imx_pm_domain(genpd)->flags & PGC_DOMAIN_FLAG_NO_PD)
> > +		return -EBUSY;
> +
> > +	_imx6_pm_domain_power_off(genpd);
> ?
> > ?	if (pd->supply)
> > ?		regulator_disable(pd->supply);
> ?
> > ?	return 0;
> ?}
> ?
> -static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
> +static void _imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
> ?{
> > ?	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
> > -	int i, ret, sw, sw2iso;
> > +	int i, sw, sw2iso;
> > ?	u32 val;
> ?
> > -	if (pd->supply) {
> > -		ret = regulator_enable(pd->supply);
> > -		if (ret) {
> > -			pr_err("%s: failed to enable regulator: %d\n",
> > -			???????__func__, ret);
> > -			return ret;
> > -		}
> > -	}
> -
> > ?	/* Enable reset clocks for all devices in the domain */
> > ?	for (i = 0; i < pd->num_clks; i++)
> > ?		clk_prepare_enable(pd->clk[i]);
> ?
> > ?	/* Gate off domain when powered down */
> @@ -123,10 +121,27 @@ static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
> > ?	udelay(DIV_ROUND_UP(sw + sw2iso, pd->ipg_rate_mhz));
> ?
> > ?	/* Disable reset clocks for all devices in the domain */
> > ?	for (i = 0; i < pd->num_clks; i++)
> > ?		clk_disable_unprepare(pd->clk[i]);
> +}
> +
> +static int imx6_pm_domain_power_on(struct generic_pm_domain *genpd)
> +{
> > +	struct imx_pm_domain *pd = to_imx_pm_domain(genpd);
> > +	int ret;
> +
> > +	if (pd->supply) {
> > +		ret = regulator_enable(pd->supply);
> > +		if (ret) {
> > +			pr_err("%s: failed to enable regulator: %d\n",
> > +			???????__func__, ret);
> > +			return ret;
> > +		}
> > +	}
> +
> > +	_imx6_pm_domain_power_on(genpd);
> ?
> > ?	return 0;
> ?}
> ?
> ?static int imx_pgc_get_clocks(struct device *dev, struct imx_pm_domain *domain)
> @@ -337,10 +352,23 @@ static const struct regmap_config imx_gpc_regmap_config = {
> > ?	.val_bits = 32,
> > ?	.reg_stride = 4,
> > ?	.max_register = 0x2ac,
> ?};
> ?
> +/* exported for suspend/resume code in arch/arm/mach-imx/gpc.c */
> +void _imx6_pm_pu_power_off(void)
> +{
> > +	_imx6_pm_domain_power_off(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base);
> +}
> +EXPORT_SYMBOL_GPL(_imx6_pm_pu_power_off);
> +
> +void _imx6_pm_pu_power_on(void)
> +{
> > +	_imx6_pm_domain_power_on(&imx_gpc_domains[GPC_PGC_DOMAIN_PU].base);
> +}
> +EXPORT_SYMBOL_GPL(_imx6_pm_pu_power_on);
> +
> ?static struct generic_pm_domain *imx_gpc_onecell_domains[] = {
> > ?	&imx_gpc_domains[0].base,
> > ?	&imx_gpc_domains[1].base,
> ?};
> ?

[PATCH 0/2] soc: imx: gpc: Power off PU domain in suspend/resume on 6qp

[Lucas Stach]

Am Montag, den 02.07.2018, 14:52 +0300 schrieb Leonard Crestez:
> Tested by doing `rtcwake -s 5 -m mem` while running glxgears on
> etnaviv.
> 
> The first patch is required because otherwise it is not easy to reach
> pgc
> domains from the gpc itself when using new-style bindings. It's also
> easier to understand.
> 
> The use of dynamic allocation in this driver is strange. Since there
> is
> only one GPC physically present in each soc my impulse would be to
> make
> most things global and delete imx_gpc_driver.remove entirely.
> 
> With current code (even without my patches) attempting to dynamically
> remove/probe the GPC fils since since the per-pgc platform_device
> instances are not removed. I'm trying something like this:
> 
> echo 130000.gpu > /sys/bus/platform/drivers/etnaviv-gpu/unbind
> echo 134000.gpu > /sys/bus/platform/drivers/etnaviv-gpu/unbind
> echo 20dc000.gpc??> /sys/bus/platform/drivers/imx-gpc/unbind
> echo 20dc000.gpc??> /sys/bus/platform/drivers/imx-gpc/bind
> 
> But is there any usecase for dynamically removing the GPC? Instead of
> trying to fix it I'd rather delete imx_gpc_driver.remove, just like
> for gpcv2. Would anyone object to a patch doing this?

Yes, as this is taking things in wrong direction. With device-links we
are able to unbind consumer devices when a provider is removed. As the
GPC is a consumer of a regulator, not having the ability to unbind it
would break that use case.

We might still have bugs in some of those functions, but then those
should really be fixed instead of removed.

Regards,
Lucas

[PATCH 0/2] soc: imx: gpc: Power off PU domain in suspend/resume on 6qp

[Leonard Crestez]

On Mon, 2018-07-02 at 14:15 +0200, Lucas Stach wrote:
> Am Montag, den 02.07.2018, 14:52 +0300 schrieb Leonard Crestez:

> > With current code (even without my patches) attempting to dynamically
> > remove/probe the GPC fils since since the per-pgc platform_device
> > instances are not removed. I'm trying something like this:
> > 
> > echo 130000.gpu > /sys/bus/platform/drivers/etnaviv-gpu/unbind
> > echo 134000.gpu > /sys/bus/platform/drivers/etnaviv-gpu/unbind
> > echo 20dc000.gpc  > /sys/bus/platform/drivers/imx-gpc/unbind
> > echo 20dc000.gpc  > /sys/bus/platform/drivers/imx-gpc/bind
> > 
> > But is there any usecase for dynamically removing the GPC? Instead of
> > trying to fix it I'd rather delete imx_gpc_driver.remove, just like
> > for gpcv2. Would anyone object to a patch doing this?
> 
> Yes, as this is taking things in wrong direction. With device-links we
> are able to unbind consumer devices when a provider is removed. As the
> GPC is a consumer of a regulator, not having the ability to unbind it
> would break that use case.

The GPC is a "consumer" of the LDO regulators which are built into the
SOC. Why would you want to unbind any of this stuff?

I don't understand the usecase, maybe you can elaborate?

[PATCH 0/2] soc: imx: gpc: Power off PU domain in suspend/resume on 6qp

[Lucas Stach]

Am Montag, den 02.07.2018, 13:49 +0000 schrieb Leonard Crestez:
> On Mon, 2018-07-02 at 14:15 +0200, Lucas Stach wrote:
> > Am Montag, den 02.07.2018, 14:52 +0300 schrieb Leonard Crestez:
> > > With current code (even without my patches) attempting to dynamically
> > > remove/probe the GPC fils since since the per-pgc platform_device
> > > instances are not removed. I'm trying something like this:
> > > 
> > > echo 130000.gpu > /sys/bus/platform/drivers/etnaviv-gpu/unbind
> > > echo 134000.gpu > /sys/bus/platform/drivers/etnaviv-gpu/unbind
> > > echo 20dc000.gpc??> /sys/bus/platform/drivers/imx-gpc/unbind
> > > echo 20dc000.gpc??> /sys/bus/platform/drivers/imx-gpc/bind
> > > 
> > > But is there any usecase for dynamically removing the GPC? Instead of
> > > trying to fix it I'd rather delete imx_gpc_driver.remove, just like
> > > for gpcv2. Would anyone object to a patch doing this?
> > 
> > Yes, as this is taking things in wrong direction. With device-links we
> > are able to unbind consumer devices when a provider is removed. As the
> > GPC is a consumer of a regulator, not having the ability to unbind it
> > would break that use case.
> 
> The GPC is a "consumer" of the LDO regulators which are built into the
> SOC. Why would you want to unbind any of this stuff?

Which in turn might be consumers of external regulators, which may have
a module driver that can be reloaded by the user at will.

> I don't understand the usecase, maybe you can elaborate?

You could make that argument for almost every device on a SoC, yet the
explicit default in Linux is for device drivers to be hot-pluggable.

Anything that changes the system layout at Linux runtime, like
devicetree overlays or dynamic system partition solutions like
Jailhouse depend on driver/device hotplug to work. I know that there
are quite a few bugs in this area, because it's less tested than other
code paths, but I'm unwilling to accept that we are actively going in
the direction of breaking this stuff.

Regards,
Lucas