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

Michal Simek michal.simek at amd.com
Thu May 18 13:29:53 CEST 2023



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");

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

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


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

Thanks,
Michal


More information about the U-Boot mailing list