[PATCH v3 1/2] firmware: zynqmp: Remove extraordinary return value

[PATCH v3 1/2] firmware: zynqmp: Remove extraordinary return value

[Stefan Herbrechtsmeier]

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;
 	}
 
-- 
2.30.2

[PATCH v3 2/2] firmware: zynqmp: Move config object permission check

[Stefan Herbrechtsmeier]

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;
 };
 
-- 
2.30.2

Re: [PATCH v3 2/2] firmware: zynqmp: Move config object permission check

[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.

And tbh zynqmp_pmufw_config_close should also do the same thing.

The rest looks good to me.

Thanks,
Michal

Re: [PATCH v3 2/2] firmware: zynqmp: Move config object permission check

[Stefan Herbrechtsmeier]

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

Re: [PATCH v3 2/2] firmware: zynqmp: Move config object permission check

[Michal Simek]

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