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

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

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