[PATCH v3 1/2] firmware: zynqmp: Remove extraordinary return value
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com
Return a common -EACCES error value instead of a positive private error value XST_PM_NO_ACCESS (2002) in zynqmp_pmufw_load_config_object function if the config object is not loadable to simplify the error checking.
Signed-off-by: Stefan Herbrechtsmeier stefan.herbrechtsmeier@weidmueller.com ---
(no changes since v3)
Changes in v3: - Rebase
Changes in v2: - Use macro for node id
drivers/firmware/firmware-zynqmp.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index dc8e3ad2b9..2b1ad5d2c3 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -82,7 +82,7 @@ int zynqmp_pmufw_node(u32 id) ret = zynqmp_pmufw_load_config_object(xpm_configobject, sizeof(xpm_configobject));
- if (ret == XST_PM_NO_ACCESS && id == NODE_OCM_BANK_0) + if (ret == -EACCES && id == NODE_OCM_BANK_0) skip_config = true;
return 0; @@ -235,8 +235,7 @@ int zynqmp_pm_is_function_supported(const u32 api_id, const u32 id) * * @cfg_obj: Pointer to the configuration object * @size: Size of @cfg_obj in bytes - * Return: 0 on success otherwise negative errno. If the config object - * is not loadable returns positive errno XST_PM_NO_ACCESS(2002) + * Return: 0 on success otherwise negative errno. */ int zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size) { @@ -251,10 +250,8 @@ 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) { + if (((u32 *)cfg_obj)[NODE_ID_LOCATION] == NODE_OCM_BANK_0) printf("PMUFW: No permission to change config object\n"); - return err; - } return -EACCES; }
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@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@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;
- 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)); }
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; };
On 5/16/23 16:05, Stefan Herbrechtsmeier wrote:
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@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@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");@@ -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
Am 17.05.2023 um 14:12 schrieb Michal Simek:
On 5/16/23 16:05, Stefan Herbrechtsmeier wrote:
From: Stefan Herbrechtsmeier stefan.herbrechtsmeier@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@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; };
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@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@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
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@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@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
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@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@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
participants (2)
-
Michal Simek -
Stefan Herbrechtsmeier