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

[Stefan Herbrechtsmeier]

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 <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?

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 (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;
>  };
>
>
>
________________________________
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