[PATCH v3 1/2] firmware: zynqmp: Remove extraordinary return value
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Return a common -EACCES error value instead of a positive private error value XST_PM_NO_ACCESS (2002) in zynqmp_pmufw_load_config_object function if the config object is not loadable to simplify the error checking.
Signed-off-by: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com ---
Changes in v3: - Rebase
Changes in v2: - Use macro for node id
drivers/firmware/firmware-zynqmp.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index dc8e3ad2b9..2b1ad5d2c3 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -82,7 +82,7 @@ int zynqmp_pmufw_node(u32 id) ret = zynqmp_pmufw_load_config_object(xpm_configobject, sizeof(xpm_configobject));
- if (ret == XST_PM_NO_ACCESS && id == NODE_OCM_BANK_0) + if (ret == -EACCES && id == NODE_OCM_BANK_0) skip_config = true;
return 0; @@ -235,8 +235,7 @@ int zynqmp_pm_is_function_supported(const u32 api_id, const u32 id) * * @cfg_obj: Pointer to the configuration object * @size: Size of @cfg_obj in bytes - * Return: 0 on success otherwise negative errno. If the config object - * is not loadable returns positive errno XST_PM_NO_ACCESS(2002) + * Return: 0 on success otherwise negative errno. */ int zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size) { @@ -251,10 +250,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] == 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; - } return -EACCES; }
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Move the check of the permission to change a config object from zynqmp_pmufw_node function to zynqmp_pmufw_load_config_object function to simplify the code and check the permission only if required.
Signed-off-by: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
---
Changes in v3: - Added
drivers/firmware/firmware-zynqmp.c | 32 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index 2b1ad5d2c3..d12159fa78 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -70,20 +70,11 @@ 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 == -EACCES && id == NODE_OCM_BANK_0) - skip_config = true; + zynqmp_pmufw_load_config_object(xpm_configobject, + sizeof(xpm_configobject));
return 0; } @@ -239,9 +230,23 @@ int zynqmp_pm_is_function_supported(const u32 api_id, const u32 id) */ int zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size) { + static bool checked; + static bool skip; int err; u32 ret_payload[PAYLOAD_ARG_CNT];
+ if (!checked) { + checked = true; + + if (zynqmp_pmufw_node(NODE_OCM_BANK_0) == -EACCES) { + printf("PMUFW: No permission to change config object\n"); + skip = true; + } + } + + if (skip) + return -EACCES; + if (IS_ENABLED(CONFIG_SPL_BUILD)) printf("Loading new PMUFW cfg obj (%ld bytes)\n", size);
@@ -250,8 +255,6 @@ 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) - printf("PMUFW: No permission to change config object\n"); return -EACCES; }
@@ -295,9 +298,6 @@ 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(NODE_OCM_BANK_0); - return 0; };
Hi,
first of all sorry for delay.
On 4/27/23 12:31, Stefan Herbrechtsmeier wrote:
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Move the check of the permission to change a config object from zynqmp_pmufw_node function to zynqmp_pmufw_load_config_object function to simplify the code and check the permission only if required.
Signed-off-by: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Changes in v3:
Added
drivers/firmware/firmware-zynqmp.c | 32 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index 2b1ad5d2c3..d12159fa78 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -70,20 +70,11 @@ 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 == -EACCES && id == NODE_OCM_BANK_0)
skip_config = true;
- zynqmp_pmufw_load_config_object(xpm_configobject,
sizeof(xpm_configobject));
This is not right. It should be return zynqmp_pmufw_load... for error propagation.
And tbh zynqmp_pmufw_config_close should also do the same thing.
The rest looks good to me.
Thanks, Michal
Hi,
Am 16.05.2023 um 10:26 schrieb Michal Simek:
Hi,
first of all sorry for delay.
On 4/27/23 12:31, Stefan Herbrechtsmeier wrote:
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Move the check of the permission to change a config object from zynqmp_pmufw_node function to zynqmp_pmufw_load_config_object function to simplify the code and check the permission only if required.
Signed-off-by: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Changes in v3:
- Added
drivers/firmware/firmware-zynqmp.c | 32 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index 2b1ad5d2c3..d12159fa78 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -70,20 +70,11 @@ 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 == -EACCES && id == NODE_OCM_BANK_0) - skip_config = true; + zynqmp_pmufw_load_config_object(xpm_configobject, + sizeof(xpm_configobject));
This is not right. It should be return zynqmp_pmufw_load... for error propagation.
At the moment the zynqmp_pmufw_node and zynqmp_pmufw_config_close doesn't return an error. Should the zynqmp_pmufw_load_config_object return 0 or -EACCES if it is skipped?
Regards Stefan
On 5/16/23 13:23, Stefan Herbrechtsmeier wrote:
Hi,
Am 16.05.2023 um 10:26 schrieb Michal Simek:
Hi,
first of all sorry for delay.
On 4/27/23 12:31, Stefan Herbrechtsmeier wrote:
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Move the check of the permission to change a config object from zynqmp_pmufw_node function to zynqmp_pmufw_load_config_object function to simplify the code and check the permission only if required.
Signed-off-by: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Changes in v3:
- Added
drivers/firmware/firmware-zynqmp.c | 32 +++++++++++++++--------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index 2b1ad5d2c3..d12159fa78 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -70,20 +70,11 @@ 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 == -EACCES && id == NODE_OCM_BANK_0) - skip_config = true; + zynqmp_pmufw_load_config_object(xpm_configobject, + sizeof(xpm_configobject));
This is not right. It should be return zynqmp_pmufw_load... for error propagation.
At the moment the zynqmp_pmufw_node and zynqmp_pmufw_config_close doesn't return an error. Should the zynqmp_pmufw_load_config_object return 0 or -EACCES if it is skipped?
In context zynqmp_pmufw_node and it's dependency on returning EACESS for failure case which your code depends on here
+ if (zynqmp_pmufw_node(NODE_OCM_BANK_0) == -EACCES) { + printf("PMUFW: No permission to change config object\n"); + skip = true; + }
And for second part around skip and return code. I would say what you have is fine. It means returning -EACCES is appropriate here.
And as I see do_zynqmp_pmufw should also return that value. That command will simply fail if there is no permission.
And for close part I would say the same. Error should be propagated. I expect current command behavior when you call "pmufw node close" on regular system will just pass but it should just fail because command wasn't successful.
Thanks, Michal
participants (2)
-
Michal Simek -
Stefan Herbrechtsmeier