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

[Michal Simek]

On 4/21/23 13:39, Stefan Herbrechtsmeier wrote:
> Am 21.04.2023 um 12:08 schrieb Michal Simek:
>> On 4/21/23 11:56, Stefan Herbrechtsmeier wrote:
>>> 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?
>>
>> That should be fine that you can filter it out if you like.
>>
>>>
>>>>>> 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?
>>
>> Not sure what you mean by other platforms.
>> If you mean different xilinx SoCs then no.
>> If you mean other then SOM. You can enable that feature and use it but it is 
>> only tested and enabled by default on SOMs.
> 
> I was confused by the `IS_ENABLED(CONFIG_ARCH_ZYNQMP)`. Why is this needed?

Because driver is used by other Xilinx/AMD SOCs but pmufw is only ZynqMP 
specific firmware. Newer one are using PLM.

> 
>>>>>> 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.
>>
>> as above. Please explain what you mean by all platforms.
>> And it is called from probe() already.
> 
> The problem is that this driver doesn't really follow the driver model and is 
> hard to understand. Other drivers requires an udevice for its functions. In this 
> case the uclass_get_device_by_phandle() will ensure that the driver is probed or 
> a failure is returned.

Not quite sure I am following you.
zynqmp_power_domain is bind from zynqmp-firmware bind method.
And both of these drivers are based on driver model.

The same technique is used in Linux kernel.

We can definitely talk if zynqmp_firmware_bind() is the right location when 
power domain should be bound but not likely an issue.

Or maybe you see there another issue. Definitely happy to get to more details.

Thanks,
Michal