Hi Michal,

Am 20.04.2023 um 13:06 schrieb Michal Simek:
Hi,

On 4/19/23 09:58, Stefan Herbrechtsmeier wrote:
Hi Michal,

Am 17.04.2023 um 12:16 schrieb Michal Simek:

On 4/3/23 15:34, Stefan Herbrechtsmeier wrote:
From: Stefan Herbrechtsmeier <stefan.herbrechtsme...@weidmueller.com>

Mask the expected and show the unexpected warning "No permission to
change config object" for NODE_OCM_BANK_0 because this node is used to
detect if further zynqmp_pmufw_node function calls should be skipped.

Signed-off-by: Stefan Herbrechtsmeier
<stefan.herbrechtsme...@weidmueller.com>
---

  drivers/firmware/firmware-zynqmp.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/firmware-zynqmp.c
b/drivers/firmware/firmware-zynqmp.c
index dc8e3ad2b9..8435b58ef9 100644
--- a/drivers/firmware/firmware-zynqmp.c
+++ b/drivers/firmware/firmware-zynqmp.c
@@ -251,7 +251,7 @@ int zynqmp_pmufw_load_config_object(const void
*cfg_obj, size_t size)
         err = xilinx_pm_request(PM_SET_CONFIGURATION,
(u32)(u64)cfg_obj, 0, 0,
                                 0, ret_payload);
         if (err == XST_PM_NO_ACCESS) {
-               if (((u32 *)cfg_obj)[NODE_ID_LOCATION] ==
NODE_OCM_BANK_0) {
+               if (((u32 *)cfg_obj)[NODE_ID_LOCATION] !=
NODE_OCM_BANK_0) {
                         printf("PMUFW:  No permission to change
config object\n");
                         return err;
                 }

First of all we should very likely create a macro for NODE_OCM_BANK_0
to cover that dependency that it is used in 3 different locations
which have to match.

Okay, I will add a PMUFW_CFG_OBJ_SUPPORT_NODE macro.

The second is the change you have in 2/2 should be the part of this
patch because when only 1/2 is applied you change behavior.

The patches should be independent, and the behavior change is intended.
The message should be printed if you don’t heave the permission for a
specific config object and not if the driver checks for support of
config objects. The NODE_OCM_BANK_0 call should never fail if load of
config objects is supported.

And changes in 2/2 makes sense.

I would be even fine to move skip_config out of zynqmp_pmufw_node()

The zynqmp_pmufw_node() function doesn't return an error and the
skip_config variable is static inside the function.

and setting up skip_config value directly in zynqmp_power_probe() not
to check in every call.

We still need to check the skip_config variable inside zynqmp_pmufw_node
to skip the load of the config object if the pmufw doesn't support it.


 85         if (ret == XST_PM_NO_ACCESS && id == NODE_OCM_BANK_0)
 86                 skip_config = true;

Without testing on HW I though to change it like this that skip_config
is configured and checked only once at probe time.

What do you think?

Patch looks okay except the printf. Is this really necessary? Could we
use a debug instead?

The patch change the return value of zynqmp_pmufw_node() but this
doesn't matter because if the config object isn't supported the function
will always return zero.

 diff --git a/drivers/firmware/firmware-zynqmp.c
b/drivers/firmware/firmware-zynqmp.c
index ece00e7958a4..aebb6f6d6d95 100644
--- a/drivers/firmware/firmware-zynqmp.c
+++ b/drivers/firmware/firmware-zynqmp.c
@@ -62,6 +62,8 @@ static unsigned int xpm_configobject_close[] = {
     0U,    /* Loading permission to Overlay config object */
 };

+static bool skip_config;
+
 int zynqmp_pmufw_config_close(void)
 {
     zynqmp_pmufw_load_config_object(xpm_configobject_close,
@@ -71,22 +73,14 @@ int zynqmp_pmufw_config_close(void)

 int zynqmp_pmufw_node(u32 id)
 {
-    static bool skip_config;
-    int ret;
-
     if (skip_config)
         return 0;

     /* Record power domain id */
     xpm_configobject[NODE_ID_LOCATION] = id;

-    ret = zynqmp_pmufw_load_config_object(xpm_configobject,
-                          sizeof(xpm_configobject));
-
-    if (ret == XST_PM_NO_ACCESS && id == PMUFW_CFG_OBJ_SUPPORT_NODE)
-        skip_config = true;
-
-    return 0;
+    return zynqmp_pmufw_load_config_object(xpm_configobject,
+                           sizeof(xpm_configobject));
 }

 static int do_pm_probe(void)
@@ -251,13 +245,8 @@ int zynqmp_pmufw_load_config_object(const void
*cfg_obj, size_t size)

     err = xilinx_pm_request(PM_SET_CONFIGURATION, (u32)(u64)cfg_obj,
0, 0,
                 0, ret_payload);
-    if (err == XST_PM_NO_ACCESS) {
-        if (((u32 *)cfg_obj)[NODE_ID_LOCATION] ==
PMUFW_CFG_OBJ_SUPPORT_NODE) {
-            printf("PMUFW:  No permission to change config object\n");
-            return err;
-        }
+    if (err == XST_PM_NO_ACCESS)
         return -EACCES;
-    }

     if (err == XST_PM_ALREADY_CONFIGURED) {
         debug("PMUFW Node is already configured\n");
@@ -299,8 +288,14 @@ static int zynqmp_power_probe(struct udevice *dev)
            ret >> ZYNQMP_PM_VERSION_MAJOR_SHIFT,
            ret & ZYNQMP_PM_VERSION_MINOR_MASK);

-    if (IS_ENABLED(CONFIG_ARCH_ZYNQMP))
-        zynqmp_pmufw_node(PMUFW_CFG_OBJ_SUPPORT_NODE);
+    if (IS_ENABLED(CONFIG_ARCH_ZYNQMP)) {
+        ret = zynqmp_pmufw_node(PMUFW_CFG_OBJ_SUPPORT_NODE);
+        if (ret == -EACCES)
+            skip_config = true;
+
+        if (!ret)
+            printf("PMUFW:  Permission to change config object\n");
+    }

     return 0;
 };



________________________________
Kommanditgesellschaft - Sitz: Detmold - Amtsgericht Lemgo HRA 2790 -
Komplementärin: Weidmüller Interface Führungsgesellschaft mbH -
Sitz: Detmold - Amtsgericht Lemgo HRB 3924;
Geschäftsführer: Dr. Timo Berger, Volker Bibelhausen, Dr. Sebastian Durst, 
André Sombecki;
USt-ID-Nr. DE124599660

Reply via email to