[PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[Michael Tretter]

Hi,

Patch 1 of this series fixes the ZynqMP PMU FW power management
initialization, which was done by the wrong driver. PM_INIT_FINALIZE must be
called from the zynqmp_pm_domains driver, which handles power domains, instead
of the zynmp_power driver, which is responsible for suspend and shutdown.

Patches 2 to 4 are various cleanup patches to improve the readability and
debugging experience of the zynqmp_pm_domains driver.

Michael

Michael Tretter (4):
  soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver
  soc: xilinx: cleanup debug and error messages
  soc: xilinx: use a properly named field instead of flags
  soc: xilinx: add a to_zynqmp_pm_domain macro

 drivers/soc/xilinx/zynqmp_pm_domains.c | 79 +++++++++++++-------------
 drivers/soc/xilinx/zynqmp_power.c      |  1 -
 2 files changed, 38 insertions(+), 42 deletions(-)

-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver

[Michael Tretter]

PM_INIT_FINALIZE tells the PMU FW that Linux is able to handle the power
management nodes that are provided by the PMU FW. Nodes that are not
requested are shut down after this call.

Calling PM_INIT_FINALIZE from the zynqmp_power driver is wrong. The PM
node request mechanism is implemented in the zynqmp_pm_domains driver,
which must also call PM_INIT_FINALIZE.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/soc/xilinx/zynqmp_pm_domains.c | 2 ++
 drivers/soc/xilinx/zynqmp_power.c      | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/xilinx/zynqmp_pm_domains.c b/drivers/soc/xilinx/zynqmp_pm_domains.c
index 226d343f0a6a..841773f2ef8d 100644
--- a/drivers/soc/xilinx/zynqmp_pm_domains.c
+++ b/drivers/soc/xilinx/zynqmp_pm_domains.c
@@ -289,6 +289,8 @@ static int zynqmp_gpd_probe(struct platform_device *pdev)
 	zynqmp_pd_data->num_domains = ZYNQMP_NUM_DOMAINS;
 	of_genpd_add_provider_onecell(dev->parent->of_node, zynqmp_pd_data);
 
+	zynqmp_pm_init_finalize();
+
 	return 0;
 }
 
diff --git a/drivers/soc/xilinx/zynqmp_power.c b/drivers/soc/xilinx/zynqmp_power.c
index c556623dae02..f8c301984d4f 100644
--- a/drivers/soc/xilinx/zynqmp_power.c
+++ b/drivers/soc/xilinx/zynqmp_power.c
@@ -178,7 +178,6 @@ static int zynqmp_pm_probe(struct platform_device *pdev)
 	u32 pm_api_version;
 	struct mbox_client *client;
 
-	zynqmp_pm_init_finalize();
 	zynqmp_pm_get_api_version(&pm_api_version);
 
 	/* Check PM API version number */
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] soc: xilinx: cleanup debug and error messages

[Michael Tretter]

Use dev_err/dev_dbg instead of pr_err/pr_debug.

Add the PM node ids to supplement the (arbitrary) power domain names to
include information which PM nodes are requested by the driver.

Drop function names from the messages, because they can easily be added
with dynamic debug.

Remove comments explaining that error messages are printed on errors.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/soc/xilinx/zynqmp_pm_domains.c | 45 +++++++++++++-------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/soc/xilinx/zynqmp_pm_domains.c b/drivers/soc/xilinx/zynqmp_pm_domains.c
index 841773f2ef8d..67e1c2b56a7d 100644
--- a/drivers/soc/xilinx/zynqmp_pm_domains.c
+++ b/drivers/soc/xilinx/zynqmp_pm_domains.c
@@ -80,12 +80,15 @@ static int zynqmp_gpd_power_on(struct generic_pm_domain *domain)
 					ZYNQMP_PM_MAX_QOS,
 					ZYNQMP_PM_REQUEST_ACK_BLOCKING);
 	if (ret) {
-		pr_err("%s() %s set requirement for node %d failed: %d\n",
-		       __func__, domain->name, pd->node_id, ret);
+		dev_err(&domain->dev,
+			"failed to set requirement to 0x%x for PM node id %d: %d\n",
+			ZYNQMP_PM_CAPABILITY_ACCESS, pd->node_id, ret);
 		return ret;
 	}
 
-	pr_debug("%s() Powered on %s domain\n", __func__, domain->name);
+	dev_dbg(&domain->dev, "set requirement to 0x%x for PM node id %d\n",
+		ZYNQMP_PM_CAPABILITY_ACCESS, pd->node_id);
+
 	return 0;
 }
 
@@ -110,8 +113,8 @@ static int zynqmp_gpd_power_off(struct generic_pm_domain *domain)
 
 	/* If domain is already released there is nothing to be done */
 	if (!(pd->flags & ZYNQMP_PM_DOMAIN_REQUESTED)) {
-		pr_debug("%s() %s domain is already released\n",
-			 __func__, domain->name);
+		dev_dbg(&domain->dev, "PM node id %d is already released\n",
+			pd->node_id);
 		return 0;
 	}
 
@@ -128,17 +131,16 @@ static int zynqmp_gpd_power_off(struct generic_pm_domain *domain)
 
 	ret = zynqmp_pm_set_requirement(pd->node_id, capabilities, 0,
 					ZYNQMP_PM_REQUEST_ACK_NO);
-	/**
-	 * If powering down of any node inside this domain fails,
-	 * report and return the error
-	 */
 	if (ret) {
-		pr_err("%s() %s set requirement for node %d failed: %d\n",
-		       __func__, domain->name, pd->node_id, ret);
+		dev_err(&domain->dev,
+			"failed to set requirement to 0x%x for PM node id %d: %d\n",
+			capabilities, pd->node_id, ret);
 		return ret;
 	}
 
-	pr_debug("%s() Powered off %s domain\n", __func__, domain->name);
+	dev_dbg(&domain->dev, "set requirement to 0x%x for PM node id %d\n",
+		capabilities, pd->node_id);
+
 	return 0;
 }
 
@@ -163,17 +165,17 @@ static int zynqmp_gpd_attach_dev(struct generic_pm_domain *domain,
 
 	ret = zynqmp_pm_request_node(pd->node_id, 0, 0,
 				     ZYNQMP_PM_REQUEST_ACK_BLOCKING);
-	/* If requesting a node fails print and return the error */
 	if (ret) {
-		pr_err("%s() %s request failed for node %d: %d\n",
-		       __func__, domain->name, pd->node_id, ret);
+		dev_err(&domain->dev, "%s() %s request failed for node %d: %d\n",
+			__func__, domain->name, pd->node_id, ret);
 		return ret;
 	}
 
 	pd->flags |= ZYNQMP_PM_DOMAIN_REQUESTED;
 
-	pr_debug("%s() %s attached to %s domain\n", __func__,
-		 dev_name(dev), domain->name);
+	dev_dbg(&domain->dev, "%s requested PM node id %d\n",
+		dev_name(dev), pd->node_id);
+
 	return 0;
 }
 
@@ -195,17 +197,16 @@ static void zynqmp_gpd_detach_dev(struct generic_pm_domain *domain,
 		return;
 
 	ret = zynqmp_pm_release_node(pd->node_id);
-	/* If releasing a node fails print the error and return */
 	if (ret) {
-		pr_err("%s() %s release failed for node %d: %d\n",
-		       __func__, domain->name, pd->node_id, ret);
+		dev_err(&domain->dev, "failed to release PM node id %d: %d\n",
+			pd->node_id, ret);
 		return;
 	}
 
 	pd->flags &= ~ZYNQMP_PM_DOMAIN_REQUESTED;
 
-	pr_debug("%s() %s detached from %s domain\n", __func__,
-		 dev_name(dev), domain->name);
+	dev_dbg(&domain->dev, "%s released PM node id %d\n",
+		dev_name(dev), pd->node_id);
 }
 
 static struct generic_pm_domain *zynqmp_gpd_xlate
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] soc: xilinx: use a properly named field instead of flags

[Michael Tretter]

Instead of defining a flags field and a single bit in this field to
signal that a PM node has been requested, use a boolean field with a
descriptive name.

No functional change, but using a proper name instead of flags makes the
code easier to read.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/soc/xilinx/zynqmp_pm_domains.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/soc/xilinx/zynqmp_pm_domains.c b/drivers/soc/xilinx/zynqmp_pm_domains.c
index 67e1c2b56a7d..c793041bf51e 100644
--- a/drivers/soc/xilinx/zynqmp_pm_domains.c
+++ b/drivers/soc/xilinx/zynqmp_pm_domains.c
@@ -20,8 +20,6 @@
 #include <linux/firmware/xlnx-zynqmp.h>
 
 #define ZYNQMP_NUM_DOMAINS		(100)
-/* Flag stating if PM nodes mapped to the PM domain has been requested */
-#define ZYNQMP_PM_DOMAIN_REQUESTED	BIT(0)
 
 static int min_capability;
 
@@ -29,12 +27,12 @@ static int min_capability;
  * struct zynqmp_pm_domain - Wrapper around struct generic_pm_domain
  * @gpd:		Generic power domain
  * @node_id:		PM node ID corresponding to device inside PM domain
- * @flags:		ZynqMP PM domain flags
+ * @requested:		The PM node mapped to the PM domain has been requested
  */
 struct zynqmp_pm_domain {
 	struct generic_pm_domain gpd;
 	u32 node_id;
-	u8 flags;
+	bool requested;
 };
 
 /**
@@ -112,7 +110,7 @@ static int zynqmp_gpd_power_off(struct generic_pm_domain *domain)
 	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
 
 	/* If domain is already released there is nothing to be done */
-	if (!(pd->flags & ZYNQMP_PM_DOMAIN_REQUESTED)) {
+	if (!pd->requested) {
 		dev_dbg(&domain->dev, "PM node id %d is already released\n",
 			pd->node_id);
 		return 0;
@@ -171,7 +169,7 @@ static int zynqmp_gpd_attach_dev(struct generic_pm_domain *domain,
 		return ret;
 	}
 
-	pd->flags |= ZYNQMP_PM_DOMAIN_REQUESTED;
+	pd->requested = true;
 
 	dev_dbg(&domain->dev, "%s requested PM node id %d\n",
 		dev_name(dev), pd->node_id);
@@ -203,7 +201,7 @@ static void zynqmp_gpd_detach_dev(struct generic_pm_domain *domain,
 		return;
 	}
 
-	pd->flags &= ~ZYNQMP_PM_DOMAIN_REQUESTED;
+	pd->requested = false;
 
 	dev_dbg(&domain->dev, "%s released PM node id %d\n",
 		dev_name(dev), pd->node_id);
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] soc: xilinx: add a to_zynqmp_pm_domain macro

[Michael Tretter]

Replace container_of for converting a generic_pm_domain to a
zynqmp_pm_domain with a macro definition to simplify the code.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/soc/xilinx/zynqmp_pm_domains.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/soc/xilinx/zynqmp_pm_domains.c b/drivers/soc/xilinx/zynqmp_pm_domains.c
index c793041bf51e..d365203c2e52 100644
--- a/drivers/soc/xilinx/zynqmp_pm_domains.c
+++ b/drivers/soc/xilinx/zynqmp_pm_domains.c
@@ -35,6 +35,9 @@ struct zynqmp_pm_domain {
 	bool requested;
 };
 
+#define to_zynqmp_pm_domain(pm_domain) \
+	container_of(pm_domain, struct zynqmp_pm_domain, gpd)
+
 /**
  * zynqmp_gpd_is_active_wakeup_path() - Check if device is in wakeup source
  *					path
@@ -69,10 +72,9 @@ static int zynqmp_gpd_is_active_wakeup_path(struct device *dev, void *not_used)
  */
 static int zynqmp_gpd_power_on(struct generic_pm_domain *domain)
 {
+	struct zynqmp_pm_domain *pd = to_zynqmp_pm_domain(domain);
 	int ret;
-	struct zynqmp_pm_domain *pd;
 
-	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
 	ret = zynqmp_pm_set_requirement(pd->node_id,
 					ZYNQMP_PM_CAPABILITY_ACCESS,
 					ZYNQMP_PM_MAX_QOS,
@@ -101,14 +103,12 @@ static int zynqmp_gpd_power_on(struct generic_pm_domain *domain)
  */
 static int zynqmp_gpd_power_off(struct generic_pm_domain *domain)
 {
+	struct zynqmp_pm_domain *pd = to_zynqmp_pm_domain(domain);
 	int ret;
 	struct pm_domain_data *pdd, *tmp;
-	struct zynqmp_pm_domain *pd;
 	u32 capabilities = min_capability;
 	bool may_wakeup;
 
-	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
-
 	/* If domain is already released there is nothing to be done */
 	if (!pd->requested) {
 		dev_dbg(&domain->dev, "PM node id %d is already released\n",
@@ -152,10 +152,8 @@ static int zynqmp_gpd_power_off(struct generic_pm_domain *domain)
 static int zynqmp_gpd_attach_dev(struct generic_pm_domain *domain,
 				 struct device *dev)
 {
+	struct zynqmp_pm_domain *pd = to_zynqmp_pm_domain(domain);
 	int ret;
-	struct zynqmp_pm_domain *pd;
-
-	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
 
 	/* If this is not the first device to attach there is nothing to do */
 	if (domain->device_count)
@@ -185,10 +183,8 @@ static int zynqmp_gpd_attach_dev(struct generic_pm_domain *domain,
 static void zynqmp_gpd_detach_dev(struct generic_pm_domain *domain,
 				  struct device *dev)
 {
+	struct zynqmp_pm_domain *pd = to_zynqmp_pm_domain(domain);
 	int ret;
-	struct zynqmp_pm_domain *pd;
-
-	pd = container_of(domain, struct zynqmp_pm_domain, gpd);
 
 	/* If this is not the last device to detach there is nothing to do */
 	if (domain->device_count)
@@ -214,7 +210,7 @@ static struct generic_pm_domain *zynqmp_gpd_xlate
 	unsigned int i, idx = genpdspec->args[0];
 	struct zynqmp_pm_domain *pd;
 
-	pd = container_of(genpd_data->domains[0], struct zynqmp_pm_domain, gpd);
+	pd = to_zynqmp_pm_domain(genpd_data->domains[0]);
 
 	if (genpdspec->args_count != 1)
 		return ERR_PTR(-EINVAL);
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[Rajan Vaja]

Hi Michael,

Thanks for the patch.

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: 17 March 2021 09:34 PM
> To: linux-arm-kernel@lists.infradead.org
> Cc: Michal Simek <michals@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly
> Shah <JOLLYS@xilinx.com>; m.tretter@pengutronix.de
> Subject: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE
> 
> CAUTION: This message has originated from an External Source. Please use proper
> judgment and caution when opening attachments, clicking links, or responding to
> this email.
> 
> 
> Hi,
> 
> Patch 1 of this series fixes the ZynqMP PMU FW power management
> initialization, which was done by the wrong driver. PM_INIT_FINALIZE must be
> called from the zynqmp_pm_domains driver, which handles power domains, instead
> of the zynmp_power driver, which is responsible for suspend and shutdown.
[Rajan] I am fine with moving to genpd but zynqmp_pm_init_finalize() needs to be late call.
zynqmp_pm_init_finalize() should be called when Linux has requested all the devices through
genpd driver. Making it late call will make sure it. 

> 
> Patches 2 to 4 are various cleanup patches to improve the readability and
> debugging experience of the zynqmp_pm_domains driver.
> 
> Michael
> 
> Michael Tretter (4):
>   soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver
>   soc: xilinx: cleanup debug and error messages
>   soc: xilinx: use a properly named field instead of flags
>   soc: xilinx: add a to_zynqmp_pm_domain macro
> 
>  drivers/soc/xilinx/zynqmp_pm_domains.c | 79 +++++++++++++-------------
>  drivers/soc/xilinx/zynqmp_power.c      |  1 -
>  2 files changed, 38 insertions(+), 42 deletions(-)
> 
> --
> 2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[Michael Tretter]

Hi Rajan,

On Thu, 15 Apr 2021 16:27:58 +0000, Rajan Vaja wrote:
> Thanks for the patch.
> 
> > -----Original Message-----
> > From: Michael Tretter <m.tretter@pengutronix.de>
> > Sent: 17 March 2021 09:34 PM
> > To: linux-arm-kernel@lists.infradead.org
> > Cc: Michal Simek <michals@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly
> > Shah <JOLLYS@xilinx.com>; m.tretter@pengutronix.de
> > Subject: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE
> > 
> > Patch 1 of this series fixes the ZynqMP PMU FW power management
> > initialization, which was done by the wrong driver. PM_INIT_FINALIZE must be
> > called from the zynqmp_pm_domains driver, which handles power domains, instead
> > of the zynmp_power driver, which is responsible for suspend and shutdown.
> [Rajan] I am fine with moving to genpd but zynqmp_pm_init_finalize() needs to be late call.
> zynqmp_pm_init_finalize() should be called when Linux has requested all the devices through
> genpd driver. Making it late call will make sure it. 

What is the reason why all devices have to be requested before calling
zynqmp_pm_init_finalize()?

I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that
Linux is using the PM API and the PMU_FW should power down/up PM slaves as
requested by Linux. It is somewhat surprising that this isn't the case and all
PM slaves have to be powered up before calling PM_INIT_FINALIZE.

What would happen if some driver is built as a module? In that case, the
module would be loaded and request the pm node only after PM_INIT_FINALIZE was
called. Do we have to avoid/disallow such cases?

For USB, I am actually observing a similar situation: If I do not request the
USB PM slave before I call PM_INIT_FINALIZE, I see communication errors with
connected USB devices. Could this be related?

Thanks,

Michael

> 
> > 
> > Patches 2 to 4 are various cleanup patches to improve the readability and
> > debugging experience of the zynqmp_pm_domains driver.
> > 
> > Michael
> > 
> > Michael Tretter (4):
> >   soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver
> >   soc: xilinx: cleanup debug and error messages
> >   soc: xilinx: use a properly named field instead of flags
> >   soc: xilinx: add a to_zynqmp_pm_domain macro
> > 
> >  drivers/soc/xilinx/zynqmp_pm_domains.c | 79 +++++++++++++-------------
> >  drivers/soc/xilinx/zynqmp_power.c      |  1 -
> >  2 files changed, 38 insertions(+), 42 deletions(-)
> > 
> > --
> > 2.29.2

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[Rajan Vaja]

Hi Michal,

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: 19 April 2021 01:03 PM
> To: Rajan Vaja <RAJANV@xilinx.com>
> Cc: linux-arm-kernel@lists.infradead.org; Michal Simek <michals@xilinx.com>; Jolly
> Shah <JOLLYS@xilinx.com>
> Subject: Re: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix
> PM_INIT_FINALIZE
> 
> Hi Rajan,
> 
> On Thu, 15 Apr 2021 16:27:58 +0000, Rajan Vaja wrote:
> > Thanks for the patch.
> >
> > > -----Original Message-----
> > > From: Michael Tretter <m.tretter@pengutronix.de>
> > > Sent: 17 March 2021 09:34 PM
> > > To: linux-arm-kernel@lists.infradead.org
> > > Cc: Michal Simek <michals@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>; Jolly
> > > Shah <JOLLYS@xilinx.com>; m.tretter@pengutronix.de
> > > Subject: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix
> PM_INIT_FINALIZE
> > >
> > > Patch 1 of this series fixes the ZynqMP PMU FW power management
> > > initialization, which was done by the wrong driver. PM_INIT_FINALIZE must be
> > > called from the zynqmp_pm_domains driver, which handles power domains,
> instead
> > > of the zynmp_power driver, which is responsible for suspend and shutdown.
> > [Rajan] I am fine with moving to genpd but zynqmp_pm_init_finalize() needs to
> be late call.
> > zynqmp_pm_init_finalize() should be called when Linux has requested all the
> devices through
> > genpd driver. Making it late call will make sure it.
> 
> What is the reason why all devices have to be requested before calling
> zynqmp_pm_init_finalize()?
[Rajan] This is required if device is not to be powered down. If zynqmp_pm_init_finalize()
Is called before requesting, device may go power down. So settings done in probe() may be
lost. In this case, driver needs to do config during runtime resume. However, some driver
may not do all config during runtime resume which may not work.

> 
> I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that
> Linux is using the PM API and the PMU_FW should power down/up PM slaves as
> requested by Linux. It is somewhat surprising that this isn't the case and all
> PM slaves have to be powered up before calling PM_INIT_FINALIZE.
[Rajan] Basically PMUFW will do power up/down as per request. However, device
config may be lost due to this power down. So device driver mat need to take care
during runtime suspend/resume



> 
> What would happen if some driver is built as a module? In that case, the
> module would be loaded and request the pm node only after PM_INIT_FINALIZE
> was
> called. Do we have to avoid/disallow such cases?
[Rajan] We allow that. When driver is init, it needs to setup device which will be fine
As probe do most of settings.

> 
> For USB, I am actually observing a similar situation: If I do not request the
> USB PM slave before I call PM_INIT_FINALIZE, I see communication errors with
> connected USB devices. Could this be related?
[Rajan] Yes, device config might have lost due to power down because of init finalize call.

Thanks,
Rajan

> 
> Thanks,
> 
> Michael
> 
> >
> > >
> > > Patches 2 to 4 are various cleanup patches to improve the readability and
> > > debugging experience of the zynqmp_pm_domains driver.
> > >
> > > Michael
> > >
> > > Michael Tretter (4):
> > >   soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver
> > >   soc: xilinx: cleanup debug and error messages
> > >   soc: xilinx: use a properly named field instead of flags
> > >   soc: xilinx: add a to_zynqmp_pm_domain macro
> > >
> > >  drivers/soc/xilinx/zynqmp_pm_domains.c | 79 +++++++++++++-------------
> > >  drivers/soc/xilinx/zynqmp_power.c      |  1 -
> > >  2 files changed, 38 insertions(+), 42 deletions(-)
> > >
> > > --
> > > 2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[Sudeep Holla]

On Mon, Apr 19, 2021 at 09:32:39AM +0200, Michael Tretter wrote:

Sorry for chiming in randomly. I always though the way PM_INIT_FINALIZE
is designed has issues(e.g. racy). I was involved in discussion with
Xilinx when we will designing more generic version of EEMI - SCMI
which is now supported in upstream. EEMI was in production already when
we started on SCMI 3-4 years back and wanted to get feedback.

[...]

>
> What is the reason why all devices have to be requested before calling
> zynqmp_pm_init_finalize()?
>

Yes that is wrong assumption/expectation from the firmware.

> I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that
> Linux is using the PM API and the PMU_FW should power down/up PM slaves as
> requested by Linux. It is somewhat surprising that this isn't the case and all
> PM slaves have to be powered up before calling PM_INIT_FINALIZE.
>

Agreed that was my understanding too.

> What would happen if some driver is built as a module? In that case, the
> module would be loaded and request the pm node only after PM_INIT_FINALIZE was
> called. Do we have to avoid/disallow such cases?
>

I was told it will work. But it will be always racy if there are multiple
channels to talk to firmware.

My argument firmware can turn off all the devices before giving control
to OS and no need for that. But there is some boot time optimisation
possible I am told which I could well be. But this interface for too
racy IMO, just happens to be fine with limited configurations it operates
in.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[Michal Simek]

Hi,

On 4/20/21 4:18 PM, Sudeep Holla wrote:
> On Mon, Apr 19, 2021 at 09:32:39AM +0200, Michael Tretter wrote:
> 
> Sorry for chiming in randomly. I always though the way PM_INIT_FINALIZE
> is designed has issues(e.g. racy). I was involved in discussion with
> Xilinx when we will designing more generic version of EEMI - SCMI
> which is now supported in upstream. EEMI was in production already when
> we started on SCMI 3-4 years back and wanted to get feedback.
> 
> [...]
> 
>>
>> What is the reason why all devices have to be requested before calling
>> zynqmp_pm_init_finalize()?
>>
> 
> Yes that is wrong assumption/expectation from the firmware.
> 
>> I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that
>> Linux is using the PM API and the PMU_FW should power down/up PM slaves as
>> requested by Linux. It is somewhat surprising that this isn't the case and all
>> PM slaves have to be powered up before calling PM_INIT_FINALIZE.
>>
> 
> Agreed that was my understanding too.
> 
>> What would happen if some driver is built as a module? In that case, the
>> module would be loaded and request the pm node only after PM_INIT_FINALIZE was
>> called. Do we have to avoid/disallow such cases?
>>
> 
> I was told it will work. But it will be always racy if there are multiple
> channels to talk to firmware.
> 
> My argument firmware can turn off all the devices before giving control
> to OS and no need for that. But there is some boot time optimisation
> possible I am told which I could well be. But this interface for too
> racy IMO, just happens to be fine with limited configurations it operates
> in.

Rajan: Can you please do deep dive to this in pmufw and try to figured
it out how to fix this on firmware side?

Thanks,
Michal

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[Michael Tretter]

Hi Rajan,

On Wed, 28 Apr 2021 15:17:25 +0200, Michal Simek wrote:
> On 4/20/21 4:18 PM, Sudeep Holla wrote:
> > On Mon, Apr 19, 2021 at 09:32:39AM +0200, Michael Tretter wrote:
> > 
> > Sorry for chiming in randomly. I always though the way PM_INIT_FINALIZE
> > is designed has issues(e.g. racy). I was involved in discussion with
> > Xilinx when we will designing more generic version of EEMI - SCMI
> > which is now supported in upstream. EEMI was in production already when
> > we started on SCMI 3-4 years back and wanted to get feedback.

Is it possible to use SCMI on the ZynqMP? I guess no, as I couldn't find any
code that would make this possible. Correct?

> > 
> > [...]
> > 
> >>
> >> What is the reason why all devices have to be requested before calling
> >> zynqmp_pm_init_finalize()?
> >>
> > 
> > Yes that is wrong assumption/expectation from the firmware.
> > 
> >> I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW that
> >> Linux is using the PM API and the PMU_FW should power down/up PM slaves as
> >> requested by Linux. It is somewhat surprising that this isn't the case and all
> >> PM slaves have to be powered up before calling PM_INIT_FINALIZE.
> >>
> > 
> > Agreed that was my understanding too.
> > 
> >> What would happen if some driver is built as a module? In that case, the
> >> module would be loaded and request the pm node only after PM_INIT_FINALIZE was
> >> called. Do we have to avoid/disallow such cases?
> >>
> > 
> > I was told it will work. But it will be always racy if there are multiple
> > channels to talk to firmware.
> > 
> > My argument firmware can turn off all the devices before giving control
> > to OS and no need for that. But there is some boot time optimisation
> > possible I am told which I could well be. But this interface for too
> > racy IMO, just happens to be fine with limited configurations it operates
> > in.
> 
> Rajan: Can you please do deep dive to this in pmufw and try to figured
> it out how to fix this on firmware side?

Did you have time to look into this?

There are 3 more cleanup patches in this series. Are there any objections
against these patches? I think the other patches are still useful by
themselves.

Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix PM_INIT_FINALIZE

[Rajan Vaja]

Hi Michael,

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: 01 June 2021 02:19 PM
> To: Rajan Vaja <RAJANV@xilinx.com>; Michal Simek <michals@xilinx.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-arm-kernel@lists.infradead.org;
> kernel@pengutronix.de
> Subject: Re: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix
> PM_INIT_FINALIZE
> 
> Hi Rajan,
> 
> On Wed, 28 Apr 2021 15:17:25 +0200, Michal Simek wrote:
> > On 4/20/21 4:18 PM, Sudeep Holla wrote:
> > > On Mon, Apr 19, 2021 at 09:32:39AM +0200, Michael Tretter wrote:
> > >
> > > Sorry for chiming in randomly. I always though the way PM_INIT_FINALIZE
> > > is designed has issues(e.g. racy). I was involved in discussion with
> > > Xilinx when we will designing more generic version of EEMI - SCMI
> > > which is now supported in upstream. EEMI was in production already when
> > > we started on SCMI 3-4 years back and wanted to get feedback.
> 
> Is it possible to use SCMI on the ZynqMP? I guess no, as I couldn't find any
> code that would make this possible. Correct?
[Rajan] Yes.

> 
> > >
> > > [...]
> > >
> > >>
> > >> What is the reason why all devices have to be requested before calling
> > >> zynqmp_pm_init_finalize()?
> > >>
> > >
> > > Yes that is wrong assumption/expectation from the firmware.
> > >
> > >> I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW
> that
> > >> Linux is using the PM API and the PMU_FW should power down/up PM slaves
> as
> > >> requested by Linux. It is somewhat surprising that this isn't the case and all
> > >> PM slaves have to be powered up before calling PM_INIT_FINALIZE.
> > >>
> > >
> > > Agreed that was my understanding too.
> > >
> > >> What would happen if some driver is built as a module? In that case, the
> > >> module would be loaded and request the pm node only after
> PM_INIT_FINALIZE was
> > >> called. Do we have to avoid/disallow such cases?
> > >>
> > >
> > > I was told it will work. But it will be always racy if there are multiple
> > > channels to talk to firmware.
> > >
> > > My argument firmware can turn off all the devices before giving control
> > > to OS and no need for that. But there is some boot time optimisation
> > > possible I am told which I could well be. But this interface for too
> > > racy IMO, just happens to be fine with limited configurations it operates
> > > in.
> >
> > Rajan: Can you please do deep dive to this in pmufw and try to figured
> > it out how to fix this on firmware side?
> 
> Did you have time to look into this?
> 
> There are 3 more cleanup patches in this series. Are there any objections
> against these patches? I think the other patches are still useful by
> themselves.
[Rajan] I am okay with this however, pm_init_finalize() needs to be late call. 
It can be taken later as it is not late call right now in mainline. 

> 
> Michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel