[PATCH v3 2/2] firmware: zynqmp: Move permission to change config object message
Stefan Herbrechtsmeier
stefan.herbrechtsmeier-oss at weidmueller.com
Wed May 17 16:11:31 CEST 2023
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"?
> 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.
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.
>> - 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?
>
>> static int do_pm_probe(void)
>> @@ -250,8 +253,6 @@ 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)
>> - printf("PMUFW: No permission to change config object\n");
>> return -EACCES;
>> }
>> @@ -295,9 +296,6 @@ static int zynqmp_power_probe(struct udevice *dev)
>> ret >> ZYNQMP_PM_VERSION_MAJOR_SHIFT,
>> ret & ZYNQMP_PM_VERSION_MINOR_MASK);
>> - if (IS_ENABLED(CONFIG_ARCH_ZYNQMP))
>> - zynqmp_pmufw_node(NODE_OCM_BANK_0);
>> -
>> return 0;
>> };
More information about the U-Boot
mailing list