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

Stefan Herbrechtsmeier stefan.herbrechtsmeier-oss at weidmueller.com
Tue Apr 25 09:02:35 CEST 2023


Hi Michal,

Am 24.04.2023 um 15:43 schrieb 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.

The source file contains two drivers:
- zynqmp_firmware
- zynqmp_power

Isn't the zynqmp_power driver zynqmp specific and thereby the 
zynqmp_power_probe function which contains the 
`IS_ENABLED(CONFIG_ARCH_ZYNQMP)` check?

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

I mean the exported functions like zynqmp_pmufw_load_config_object(). 
This function calls xilinx_pm_request() which calls ipi_req() which 
calls do_pm_probe() if the global data isn't initialized. I think a 
cleaner solution would be if the ipi_req functions required the udevice 
as parameter and the xilinx_pm_request function would call the 
uclass_get_device_by_driver to get the udevice. This would also 
eliminate the global data in the driver.

Maybe even the zynqmp_pmufw_node() function should require an udevice 
because this is already described in the device tree and struct 
power_domain and makes clear that this functions requires a probed 
zynqmp_power driver.

At the moment you have to analyses the whole driver code to understand 
that a first call of zynqmp_pmufw_node() will probe the zynqmp_power 
driver via zynqmp_pmufw_load_config_object() --> xilinx_pm_request() --> 
ipi_req() --> do_pm_probe() only if the CONFIG_ZYNQMP_IPI is enabled in 
SPL or EL 3. Otherwise the xilinx_pm_request() will use smc_call() or 
failed.

This means that after your change the zynqmp_pmufw_node() function 
content will only be skipped if IPI is enabled and we are in SPL or EL 3.

Regards
   Stefan



More information about the U-Boot mailing list