[PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning

[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.herbrechtsmeier at 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.herbrechtsmeier at 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;
  };