[PATCH v3 2/2] firmware: zynqmp: Move permission to change config object message
Stefan Herbrechtsmeier
stefan.herbrechtsmeier-oss at weidmueller.com
Mon May 22 12:55:10 CEST 2023
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.
Regards
Stefan
More information about the U-Boot
mailing list