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?
M
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;
};
