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