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

STALE1888d

12 messages, 4 authors, 2021-06-01 · open the first message on its own page

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

From: Michael Tretter <m.tretter@pengutronix.de>
Date: 2021-03-17 16:05:47

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 3/4] soc: xilinx: use a properly named field instead of flags

From: Michael Tretter <m.tretter@pengutronix.de>
Date: 2021-03-17 16:05:52

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 1/4] soc: xilinx: move PM_INIT_FINALIZE to zynqmp_pm_domains driver

From: Michael Tretter <m.tretter@pengutronix.de>
Date: 2021-03-17 16:05:56

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

From: Michael Tretter <m.tretter@pengutronix.de>
Date: 2021-03-17 16:05:59

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 4/4] soc: xilinx: add a to_zynqmp_pm_domain macro

From: Michael Tretter <m.tretter@pengutronix.de>
Date: 2021-03-17 16:06:06

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

From: Rajan Vaja <hidden>
Date: 2021-04-15 16:30:04

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 <redacted>; Rajan Vaja <redacted>; Jolly
Shah [off-list ref]; 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

From: Michael Tretter <m.tretter@pengutronix.de>
Date: 2021-04-19 07:35:09

Hi Rajan,

On Thu, 15 Apr 2021 16:27:58 +0000, Rajan Vaja wrote:

Thanks for the patch.

quoted

-----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 <redacted>; Rajan Vaja <redacted>; Jolly
Shah [off-list ref]; 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

quoted

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

From: Rajan Vaja <hidden>
Date: 2021-04-19 12:31:32

Hi Michal,

-----Original Message-----
From: Michael Tretter <m.tretter@pengutronix.de>
Sent: 19 April 2021 01:03 PM
To: Rajan Vaja <redacted>
Cc: linux-arm-kernel@lists.infradead.org; Michal Simek <redacted>; Jolly
Shah [off-list ref]
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:

quoted

Thanks for the patch.

quoted

-----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 <redacted>; Rajan Vaja <redacted>; Jolly
Shah [off-list ref]; m.tretter@pengutronix.de
Subject: [PATCH 0/4] soc: xilinx: pm_domains: cleanup and fix

PM_INIT_FINALIZE

quoted

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

quoted

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.

quoted

zynqmp_pm_init_finalize() should be called when Linux has requested all the

devices through

quoted

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

quoted

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

From: Sudeep Holla <hidden>
Date: 2021-04-20 14:20:55

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

From: Michal Simek <hidden>
Date: 2021-04-28 13:19:31

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.

[...]

quoted

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.

quoted

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.

quoted

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

From: Michael Tretter <m.tretter@pengutronix.de>
Date: 2021-06-01 08:51:05

Hi Rajan,

On Wed, 28 Apr 2021 15:17:25 +0200, Michal Simek wrote:

On 4/20/21 4:18 PM, Sudeep Holla wrote:

quoted

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?

quoted

[...]

quoted

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.

quoted

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.

quoted

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

From: Rajan Vaja <hidden>
Date: 2021-06-01 09:18:55

Hi Michael,

-----Original Message-----
From: Michael Tretter <m.tretter@pengutronix.de>
Sent: 01 June 2021 02:19 PM
To: Rajan Vaja <redacted>; Michal Simek <redacted>
Cc: Sudeep Holla <redacted>; 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:

quoted

On 4/20/21 4:18 PM, Sudeep Holla wrote:

quoted

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.

quoted

[...]

quoted

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.

quoted

I was expecting that calling PM_INIT_FINALIZE only would tell the PMU_FW

that

quoted

Linux is using the PM API and the PMU_FW should power down/up PM slaves

as

quoted

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.

quoted

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

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help