[PATCH v3 2/2] firmware: zynqmp: Move permission to change config object message

Michal Simek michal.simek at amd.com
Mon May 22 13:59:40 CEST 2023


Hi,

On 5/22/23 12:55, Stefan Herbrechtsmeier wrote:
> Hi Michal,
> 
> Am 18.05.2023 um 13:29 schrieb Michal Simek:
> 
>> On 5/17/23 16:11, Stefan Herbrechtsmeier wrote:
>>> Am 17.05.2023 um 14:12 schrieb Michal Simek:
>>>> On 5/16/23 16:05, Stefan Herbrechtsmeier wrote:
>>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>>>>>
>>>>> Move the permission to change a config object message from
>>>>> zynqmp_pmufw_load_config_object function to zynqmp_pmufw_node function
>>>>> to simplify the code and check the permission only if required.
>>>>>
>>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at weidmueller.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Changes in v4:
>>>>> - Reword
>>>>> - Move the check back to zynqmp_pmufw_node because the check need to be
>>>>>    run after the config object load.
>>>>> - Return error in zynqmp_pmufw_config_close and zynqmp_pmufw_node
>>>>>
>>>>> Changes in v3:
>>>>> - Added
>>>>>
>>>>>   drivers/firmware/firmware-zynqmp.c | 36 ++++++++++++++----------------
>>>>>   1 file changed, 17 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/firmware/firmware-zynqmp.c 
>>>>> b/drivers/firmware/firmware-zynqmp.c
>>>>> index 2b1ad5d2c3..6dc745bd14 100644
>>>>> --- a/drivers/firmware/firmware-zynqmp.c
>>>>> +++ b/drivers/firmware/firmware-zynqmp.c
>>>>> @@ -63,29 +63,32 @@ static unsigned int xpm_configobject_close[] = {
>>>>>     int zynqmp_pmufw_config_close(void)
>>>>>   {
>>>>> -    zynqmp_pmufw_load_config_object(xpm_configobject_close,
>>>>> -                    sizeof(xpm_configobject_close));
>>>>> -    return 0;
>>>>> +    return zynqmp_pmufw_load_config_object(xpm_configobject_close,
>>>>> +                           sizeof(xpm_configobject_close));
>>>>>   }
>>>>>     int zynqmp_pmufw_node(u32 id)
>>>>>   {
>>>>> -    static bool skip_config;
>>>>> -    int ret;
>>>>> +    static bool checked;
>>>>> +    static bool skip;
>>>>
>>>> I see interesting behavior in connection to these variables.
>>>> I did this change and keep test variable to see behavior.
>>>>
>>>>
>>>> diff --git a/drivers/firmware/firmware-zynqmp.c 
>>>> b/drivers/firmware/firmware-zynqmp.c
>>>> index 6dc745bd1424..becbea7b64ea 100644
>>>> --- a/drivers/firmware/firmware-zynqmp.c
>>>> +++ b/drivers/firmware/firmware-zynqmp.c
>>>> @@ -67,10 +67,14 @@ int zynqmp_pmufw_config_close(void)
>>>> sizeof(xpm_configobject_close));
>>>>  }
>>>>
>>>> +static bool checked;
>>>> +static bool skip;
>>>> +
>>>>  int zynqmp_pmufw_node(u32 id)
>>>>  {
>>>> -       static bool checked;
>>>> -       static bool skip;
>>>> +       static bool test;
>>>> +
>>>> +       printf("----------------%s, id %d, ch %d, skp %d - test %d\n", 
>>>> __func__, id, checked, skip, test);
>>>>
>>>>         if (!checked) {
>>>>                 checked = true;
>>>> @@ -379,6 +391,9 @@ static int zynqmp_firmware_bind(struct udevice *dev)
>>>>         int ret;
>>>>         struct udevice *child;
>>>>
>>>> +       checked = 0;
>>>> +       skip = 0;
>>>> +
>>>>         if ((IS_ENABLED(CONFIG_SPL_BUILD) &&
>>>>              IS_ENABLED(CONFIG_SPL_POWER_DOMAIN) &&
>>>>              IS_ENABLED(CONFIG_ZYNQMP_POWER_DOMAIN)) ||
>>>>
>>>>
>>>> <debug_uart>
>>>> zynqmp_power_domain zynqmp_power_domain: Request for id: 34
>>>> zynqmp_pmufw_node, id 34, ch 0, skp 0 - test 255/815a2fa
>>>> zynqmp_pmufw_node, id 11, ch 1, skp 0 - test 255/815a2fa
>>>> -----------zynqmp_pmufw_node ACCESS OK
>>>> --------------------zynqmp_pmufw_load_config_object
>>>> --------------------zynqmp_pmufw_load_config_object44
>>>> -----------zynqmp_pmufw_node ACCESS OK
>>>> --------------------zynqmp_pmufw_load_config_object
>>>> --------------------zynqmp_pmufw_load_config_object44
>>>> zynqmp_power_domain zynqmp_power_domain: Domain ON for id: 34
>>>> zynq_serial_setbrg: CLK 99999999
>>>>
>>>>
>>>> U-Boot 2023.07-rc2-00053-gaf7817988644-dirty (May 17 2023 - 14:03:37 +0200)
>>>>
>>>> CPU:   ZynqMP
>>>> Silicon: v3
>>>> Chip:  xck26
>>>> zynqmp_power_domain zynqmp_power_domain: Request for id: 38
>>>> zynqmp_pmufw_node, id 38, ch 1, skp 0 - test 1/815a2fa
>>>> -----------zynqmp_pmufw_node ACCESS OK
>>>> --------------------zynqmp_pmufw_load_config_object
>>>> --------------------zynqmp_pmufw_load_config_object44
>>>> zynqmp_power_domain zynqmp_power_domain: Domain ON for id: 38
>>>> Detected name: zynqmp-smk-k26-xcl2g-revA-sck-kv-g-revB
>>>> Model: ZynqMP KV260 revB
>>>> Board: Xilinx ZynqMP
>>>> DRAM:  2 GiB (effective 4 GiB)
>>>> zynqmp_power_domain zynqmp_power_domain: Request for id: 46
>>>> zynqmp_pmufw_node, id 46, ch 0, skp 0 - test 0/7ffd42fa
>>>> zynqmp_pmufw_node, id 11, ch 1, skp 0 - test 0/7ffd42fa
>>>> -----------zynqmp_pmufw_node ACCESS OK
>>>> --------------------zynqmp_pmufw_load_config_object
>>>> --------------------zynqmp_pmufw_load_config_object44
>>>> -----------zynqmp_pmufw_node ACCESS OK
>>>> --------------------zynqmp_pmufw_load_config_object
>>>> --------------------zynqmp_pmufw_load_config_object44
>>>> zynqmp_power_domain zynqmp_power_domain: Domain ON for id: 46
>>>> PMUFW:    v1.1
>>>> zynqmp_power_domain zynqmp_power_domain: Request for id: 38
>>>> zynqmp_pmufw_node, id 38, ch 1, skp 0 - test 1/7ffd42fa
>>>> -----------zynqmp_pmufw_node ACCESS OK
>>>>
>>>>
>>>> As you see test variable is in BSS section but it is not initialized at this 
>>>> stage. If you look at arch/arm/lib/crt0_64.S debug uart is called before 
>>>> calling board_init_f and bss is cleared before board_init_r is called.
>>>
>>> What does "but BSS and  initialized non-const data are still not available" 
>>> mean? Could we use variables from the data section like "static bool check = 
>>> true"?
>>
>> Yes - when you move that variable to data section then it should be fine.
>> Or just move it like this
>> struct fru_table fru_data __section(".data");
> 
> Okay, I will use global data and move it to the data section.
> 
>>>> It means variables should be placed to different section or initialized them 
>>>> directly from the code.
>>>
>>> I think the zynqmp_power variable could have the same problem.
>>
>> If it is called in early phase then yes.
> It is used by the ipi_req function to know if the driver need to be probed. But 
> this is only a problem if the U-Boot is used in EL3.
> 
>>> The initialization from the code doesn't work because the class is dynamic 
>>> probed.
>>> zynqmp_pmufw_node --> zynqmp_pmufw_load_config_object --> xilinx_pm_request
>>> -(SPL)-> ipi_req --> do_pm_probe
>>>
>>> Maybe we need to rework the driver to use private driver data and probe the 
>>> driver early in the chain.
>>
>> But in SPL flow bss is initialized before board_init_r() which is done before 
>> sending request to PMUFW.
>>
>> <debug_uart>
>> --------------board_init_f
>> >>SPL: board_init_r()
>> zynq_serial_setbrg: CLK 99999999
>>
>> U-Boot SPL 2023.07-rc2-00051-g08bab040a7d7-dirty (May 18 2023 - 13:27:14 +0200)
>> --------------------zynqmp_pmufw_load_config_object
>> Loading new PMUFW cfg obj (2032 bytes)
>> ipi_req, tx/rx - 0/0
>> ipi_req, tx/rx - 536871440/
> 
> The problem is the zynqmp_power_domain driver. This driver is used before 
> relocation and even before PMU Firmware load in SPL. The first could be fixed by 
> the data section but after the error forwarding change below the driver could 
> break the SPL and the firmware need do be loaded in the probe function.
> 
>>>>>   -    if (skip_config)
>>>>> -        return 0;
>>>>> +    if (!checked) {
>>>>> +        checked = true;
>>>>>   -    /* Record power domain id */
>>>>> -    xpm_configobject[NODE_ID_LOCATION] = id;
>>>>> +        if (zynqmp_pmufw_node(NODE_OCM_BANK_0) == -EACCES) {
>>>>> +            printf("PMUFW:  No permission to change config object\n");
>>>>> +            skip = true;
>>>>> +        }
>>>>> +    }
>>>>>   -    ret = zynqmp_pmufw_load_config_object(xpm_configobject,
>>>>> -                          sizeof(xpm_configobject));
>>>>> +    if (skip)
>>>>> +        return -EACCES;
>>>>>   -    if (ret == -EACCES && id == NODE_OCM_BANK_0)
>>>>> -        skip_config = true;
>>>>> +    /* Record power domain id */
>>>>> +    xpm_configobject[NODE_ID_LOCATION] = id;
>>>>>   -    return 0;
>>>>> +    return zynqmp_pmufw_load_config_object(xpm_configobject,
>>>>> +                           sizeof(xpm_configobject));
>>>>>   }
>>>>
>>>> With this change there is also need to change
>>>> drivers/power/domain/zynqmp-power-domain.c
>>>>
>>>> to handle return value for case that node is already configured.
>>>> I would prefer to have separate error code for it just in case.
>>>>
>>>>  static int zynqmp_power_domain_request(struct power_domain *power_domain)
>>>>  {
>>>> +       int ret = 0;
>>>> +
>>>>         dev_dbg(power_domain->dev, "Request for id: %ld\n", power_domain->id);
>>>>
>>>> -       if (IS_ENABLED(CONFIG_ARCH_ZYNQMP))
>>>> -               return zynqmp_pmufw_node(power_domain->id);
>>>> +       if (IS_ENABLED(CONFIG_ARCH_ZYNQMP)) {
>>>> +               ret = zynqmp_pmufw_node(power_domain->id);
>>>> +               if (ret == -ENODEV)
>>>> +                       ret = 0;
>>>> +       }
>>>>
>>>> -       return 0;
>>>> +       return ret;
>>>>  }
>>>>
>>> Should I add a patch to the series before this patch?
>>
>> That would be the best solution.
> 
> I assume this breaks the SPL if the CONFIG_SPL_POWER_DOMAIN is enabled because 
> the dev_power_domain_on will fail.

I likely never tried it myself. I don't think pmufw keeps track of resources and 
all power domains are enabled after power up anyway.

M





More information about the U-Boot mailing list