[PATCH v3 2/2] firmware: zynqmp: Move permission to change config object message
Michal Simek
michal.simek at amd.com
Wed May 17 14:12:31 CEST 2023
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.
It means variables should be placed to different section or initialized them
directly from the code.
>
> - 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;
}
>
> 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;
> };
>
Thanks,
Michal
More information about the U-Boot
mailing list