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

[Stefan Herbrechtsmeier]

Hi Michal,

Am 20.04.2023 um 14:39 schrieb Michal Simek:

> On 4/20/23 14:30, Stefan Herbrechtsmeier wrote:
>> 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?
>
> For production build no. But there are other messages which are likely 
> not needed. Like a silicon version (production is only one version) 
> for example.

Could we use log_info instead of printf?

>>> 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.
>
> By default that feature should be disabled in standard pmufw build.
> I don't have a preference but I want to see that message only once, 
> disabled or enabled.

Is it possible to call the zynqmp_pmufw_node() in the probe() for the 
other platforms?

>>> 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.
>
> It should be but question is if it is in all cases. At least you can 
> disable power domain driver and then first call can be via command.

We should call the zynqmp_pmufw_node() in probe() for all platforms to 
enable / disable the feature.

I have test your changes and they works.

Regards
   Stefan