Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
Hi Michal,
Am 24.04.2023 um 15:43 schrieb Michal Simek:
On 4/21/23 13:39, Stefan Herbrechtsmeier wrote:
Am 21.04.2023 um 12:08 schrieb Michal Simek:
On 4/21/23 11:56, Stefan Herbrechtsmeier wrote:
Hi Michal,
Am 20.04.2023 um 14:39 schrieb Michal Simek:
On 4/20/23 14:30, Stefan Herbrechtsmeier wrote:
Am 20.04.2023 um 14:11 schrieb Michal Simek:
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
Is this information really necessary for a production build?
For production build no. But there are other messages which are
likely not needed. Like a silicon version (production is only one
version) for example.
Could we use log_info instead of printf?
That should be fine that you can filter it out if you like.
It means having information in boot log is quite worth.
Either we should print a message in any case or only if the
feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.
By default that feature should be disabled in standard pmufw build.
I don't have a preference but I want to see that message only
once, disabled or enabled.
Is it possible to call the zynqmp_pmufw_node() in the probe() for
the other platforms?
Not sure what you mean by other platforms.
If you mean different xilinx SoCs then no.
If you mean other then SOM. You can enable that feature and use it
but it is only tested and enabled by default on SOMs.
I was confused by the `IS_ENABLED(CONFIG_ARCH_ZYNQMP)`. Why is this
needed?
Because driver is used by other Xilinx/AMD SOCs but pmufw is only
ZynqMP specific firmware. Newer one are using PLM.
The source file contains two drivers:
- zynqmp_firmware
- zynqmp_power
Isn't the zynqmp_power driver zynqmp specific and thereby the
zynqmp_power_probe function which contains the
`IS_ENABLED(CONFIG_ARCH_ZYNQMP)` check?
Actually maybe even we should create variable based on it to be
able
to use it in scripts.
Because it is everybody decision if you want to let OS to send that
config fragments to PMUFW or just close that doors (right now
you can
do it via command).
Also
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
On 4/21/23 13:39, Stefan Herbrechtsmeier wrote:
Am 21.04.2023 um 12:08 schrieb Michal Simek:
On 4/21/23 11:56, Stefan Herbrechtsmeier wrote:
Hi Michal,
Am 20.04.2023 um 14:39 schrieb Michal Simek:
On 4/20/23 14:30, Stefan Herbrechtsmeier wrote:
Am 20.04.2023 um 14:11 schrieb Michal Simek:
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
Is this information really necessary for a production build?
For production build no. But there are other messages which are likely not
needed. Like a silicon version (production is only one version) for example.
Could we use log_info instead of printf?
That should be fine that you can filter it out if you like.
It means having information in boot log is quite worth.
Either we should print a message in any case or only if the feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.
By default that feature should be disabled in standard pmufw build.
I don't have a preference but I want to see that message only once, disabled
or enabled.
Is it possible to call the zynqmp_pmufw_node() in the probe() for the other
platforms?
Not sure what you mean by other platforms.
If you mean different xilinx SoCs then no.
If you mean other then SOM. You can enable that feature and use it but it is
only tested and enabled by default on SOMs.
I was confused by the `IS_ENABLED(CONFIG_ARCH_ZYNQMP)`. Why is this needed?
Because driver is used by other Xilinx/AMD SOCs but pmufw is only ZynqMP
specific firmware. Newer one are using PLM.
Actually maybe even we should create variable based on it to be able
to use it in scripts.
Because it is everybody decision if you want to let OS to send that
config fragments to PMUFW or just close that doors (right now you can
do it via command).
Also thinking that by default that skip_config should be false by
default and only enable it before calling that OCM. Or just change the
name to enable_config to be able to place it to bss section.
The skip_config is false by default and the function is called by the
probe as first user.
It should be but question is if
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
Am 21.04.2023 um 12:08 schrieb Michal Simek:
On 4/21/23 11:56, Stefan Herbrechtsmeier wrote:
Hi Michal,
Am 20.04.2023 um 14:39 schrieb Michal Simek:
On 4/20/23 14:30, Stefan Herbrechtsmeier wrote:
Am 20.04.2023 um 14:11 schrieb Michal Simek:
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
Is this information really necessary for a production build?
For production build no. But there are other messages which are
likely not needed. Like a silicon version (production is only one
version) for example.
Could we use log_info instead of printf?
That should be fine that you can filter it out if you like.
It means having information in boot log is quite worth.
Either we should print a message in any case or only if the feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.
By default that feature should be disabled in standard pmufw build.
I don't have a preference but I want to see that message only once,
disabled or enabled.
Is it possible to call the zynqmp_pmufw_node() in the probe() for the
other platforms?
Not sure what you mean by other platforms.
If you mean different xilinx SoCs then no.
If you mean other then SOM. You can enable that feature and use it but
it is only tested and enabled by default on SOMs.
I was confused by the `IS_ENABLED(CONFIG_ARCH_ZYNQMP)`. Why is this needed?
Actually maybe even we should create variable based on it to be able
to use it in scripts.
Because it is everybody decision if you want to let OS to send that
config fragments to PMUFW or just close that doors (right now you can
do it via command).
Also thinking that by default that skip_config should be false by
default and only enable it before calling that OCM. Or just change
the
name to enable_config to be able to place it to bss section.
The skip_config is false by default and the function is called by the
probe as first user.
It should be but question is if it is in all cases. At least you can
disable power domain driver and then first call can be via command.
We should call the zynqmp_pmufw_node() in
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
On 4/21/23 11:56, Stefan Herbrechtsmeier wrote:
Hi Michal,
Am 20.04.2023 um 14:39 schrieb Michal Simek:
On 4/20/23 14:30, Stefan Herbrechtsmeier wrote:
Am 20.04.2023 um 14:11 schrieb Michal Simek:
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
Is this information really necessary for a production build?
For production build no. But there are other messages which are likely not
needed. Like a silicon version (production is only one version) for example.
Could we use log_info instead of printf?
That should be fine that you can filter it out if you like.
It means having information in boot log is quite worth.
Either we should print a message in any case or only if the feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.
By default that feature should be disabled in standard pmufw build.
I don't have a preference but I want to see that message only once, disabled
or enabled.
Is it possible to call the zynqmp_pmufw_node() in the probe() for the other
platforms?
Not sure what you mean by other platforms.
If you mean different xilinx SoCs then no.
If you mean other then SOM. You can enable that feature and use it but it is
only tested and enabled by default on SOMs.
Actually maybe even we should create variable based on it to be able
to use it in scripts.
Because it is everybody decision if you want to let OS to send that
config fragments to PMUFW or just close that doors (right now you can
do it via command).
Also thinking that by default that skip_config should be false by
default and only enable it before calling that OCM. Or just change the
name to enable_config to be able to place it to bss section.
The skip_config is false by default and the function is called by the
probe as first user.
It should be but question is if it is in all cases. At least you can disable
power domain driver and then first call can be via command.
We should call the zynqmp_pmufw_node() in probe() for all platforms to enable /
disable the feature.
as above. Please explain what you mean by all platforms.
And it is called from
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
Hi Michal,
Am 20.04.2023 um 14:39 schrieb Michal Simek:
On 4/20/23 14:30, Stefan Herbrechtsmeier wrote:
Am 20.04.2023 um 14:11 schrieb Michal Simek:
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
Is this information really necessary for a production build?
For production build no. But there are other messages which are likely
not needed. Like a silicon version (production is only one version)
for example.
Could we use log_info instead of printf?
It means having information in boot log is quite worth.
Either we should print a message in any case or only if the feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.
By default that feature should be disabled in standard pmufw build.
I don't have a preference but I want to see that message only once,
disabled or enabled.
Is it possible to call the zynqmp_pmufw_node() in the probe() for the
other platforms?
Actually maybe even we should create variable based on it to be able
to use it in scripts.
Because it is everybody decision if you want to let OS to send that
config fragments to PMUFW or just close that doors (right now you can
do it via command).
Also thinking that by default that skip_config should be false by
default and only enable it before calling that OCM. Or just change the
name to enable_config to be able to place it to bss section.
The skip_config is false by default and the function is called by the
probe as first user.
It should be but question is if it is in all cases. At least you can
disable power domain driver and then first call can be via command.
We should call the zynqmp_pmufw_node() in probe() for all platforms to
enable / disable the feature.
I have test your changes and they works.
Regards
Stefan
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
Am 20.04.2023 um 14:39 schrieb Michal Simek:
On 4/20/23 14:30, Stefan Herbrechtsmeier wrote:
Am 20.04.2023 um 14:11 schrieb Michal Simek:
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
Is this information really necessary for a production build?
For production build no. But there are other messages which are likely
not needed. Like a silicon version (production is only one version)
for example.
It means having information in boot log is quite worth.
Either we should print a message in any case or only if the feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.
By default that feature should be disabled in standard pmufw build.
I don't have a preference but I want to see that message only once,
disabled or enabled.
Actually maybe even we should create variable based on it to be able
to use it in scripts.
Because it is everybody decision if you want to let OS to send that
config fragments to PMUFW or just close that doors (right now you can
do it via command).
Also thinking that by default that skip_config should be false by
default and only enable it before calling that OCM. Or just change the
name to enable_config to be able to place it to bss section.
The skip_config is false by default and the function is called by the
probe as first user.
It should be but question is if it is in all cases. At least you can
disable power domain driver and then first call can be via command.
Isn't the probe function the first caller always because it setup the ipi?
Regards
Stefan
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
On 4/20/23 14:30, Stefan Herbrechtsmeier wrote:
Am 20.04.2023 um 14:11 schrieb Michal Simek:
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
Is this information really necessary for a production build?
For production build no. But there are other messages which are likely not
needed. Like a silicon version (production is only one version) for example.
It means having information in boot log is quite worth.
Either we should print a message in any case or only if the feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.
By default that feature should be disabled in standard pmufw build.
I don't have a preference but I want to see that message only once, disabled or
enabled.
Actually maybe even we should create variable based on it to be able
to use it in scripts.
Because it is everybody decision if you want to let OS to send that
config fragments to PMUFW or just close that doors (right now you can
do it via command).
Also thinking that by default that skip_config should be false by
default and only enable it before calling that OCM. Or just change the
name to enable_config to be able to place it to bss section.
The skip_config is false by default and the function is called by the
probe as first user.
It should be but question is if it is in all cases. At least you can disable
power domain driver and then first call can be via command.
Thanks,
Michal
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
Am 20.04.2023 um 14:11 schrieb Michal Simek:
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
Is this information really necessary for a production build?
It means having information in boot log is quite worth.
Either we should print a message in any case or only if the feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.
Actually maybe even we should create variable based on it to be able
to use it in scripts.
Because it is everybody decision if you want to let OS to send that
config fragments to PMUFW or just close that doors (right now you can
do it via command).
Also thinking that by default that skip_config should be false by
default and only enable it before calling that OCM. Or just change the
name to enable_config to be able to place it to bss section.
The skip_config is false by default and the function is called by the
probe as first user.
Definitely it should be tested to make sure that we don't break
existing behavior.
Regards
Stefan
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
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
On 4/20/23 14:03, Stefan Herbrechtsmeier wrote:
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
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
---
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?
It is feature which you need to explicitly enable in PMUFW to work.
It means having information in boot log is quite worth. Actually maybe even we
should create variable based on it to be able to use it in scripts.
Because it is everybody decision if you want to let OS to send that config
fragments to PMUFW or just close that doors (right now you can do it via command).
Also thinking that by default that skip_config should be false by default and
only enable it before calling that OCM. Or just change the name to enable_config
to be able to place it to bss section.
Definitely it should be tested to make sure that we don't break existing
behavior.
Thanks,
Michal
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
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
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
---
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
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
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
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
---
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)
+
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
Hi Michal,
Am 17.04.2023 um 12:16 schrieb Michal Simek:
On 4/3/23 15:34, Stefan Herbrechtsmeier wrote:
From: Stefan Herbrechtsmeier
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
---
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;
Regards
Stefan
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
Re: [PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
On 4/3/23 15:34, Stefan Herbrechtsmeier wrote:
From: Stefan Herbrechtsmeier
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
---
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.
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.
And changes in 2/2 makes sense.
I would be even fine to move skip_config out of zynqmp_pmufw_node()
and setting up skip_config value directly in zynqmp_power_probe() not to check
in every call.
85 if (ret == XST_PM_NO_ACCESS && id == NODE_OCM_BANK_0)
86 skip_config = true;
Thanks,
Michal
[PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
From: Stefan Herbrechtsmeier
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
---
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;
}
--
2.30.2
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
