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

[Stefan Herbrechtsmeier]

Am 20.04.2023 um 14:11 schrieb 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.

Is this information really necessary for a production build?

> It means having information in boot log is quite worth.

Either we should print a message in any case or only if the feature is
disabled because in this case the zynqmp_pmufw_node() is a nop.

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

The skip_config is false by default and the function is called by the
probe as first user.

> Definitely it should be tested to make sure that we don't break
> existing behavior.

Regards
   Stefan

________________________________
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