[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