[PATCH 1/2] firmware: zynqmp: Mask expected and show unexpected warning
Stefan Herbrechtsmeier
stefan.herbrechtsmeier-oss at weidmueller.com
Fri Apr 21 13:39:17 CEST 2023
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?
>>>>> 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.
If we want to ensure that zynqmp_pmufw_node() is skipped without a
previous call of zynqmp_power_probe() we have to keep the check in the
function.
>> I have test your changes and they works.
>
> on zcu102 or also others?
I have test it on our hardware.
Regards
Stefan
More information about the U-Boot
mailing list