Message ID | 20230427103125.26719-2-stefan.herbrechtsmeier-oss@weidmueller.com |
---|---|
State | Superseded |
Delegated to: | Michal Simek |
Headers | show |
Series | [v3,1/2] firmware: zynqmp: Remove extraordinary return value | expand |
Hi, first of all sorry for delay. On 4/27/23 12:31, Stefan Herbrechtsmeier wrote: > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > Move the check of the permission to change a config object from > zynqmp_pmufw_node function to zynqmp_pmufw_load_config_object function > to simplify the code and check the permission only if required. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > --- > > Changes in v3: > - Added > > drivers/firmware/firmware-zynqmp.c | 32 +++++++++++++++--------------- > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c > index 2b1ad5d2c3..d12159fa78 100644 > --- a/drivers/firmware/firmware-zynqmp.c > +++ b/drivers/firmware/firmware-zynqmp.c > @@ -70,20 +70,11 @@ int zynqmp_pmufw_config_close(void) > > int zynqmp_pmufw_node(u32 id) > { > - static bool skip_config; > - int ret; > - > - if (skip_config) > - return 0; > - > /* Record power domain id */ > xpm_configobject[NODE_ID_LOCATION] = id; > > - ret = zynqmp_pmufw_load_config_object(xpm_configobject, > - sizeof(xpm_configobject)); > - > - if (ret == -EACCES && id == NODE_OCM_BANK_0) > - skip_config = true; > + zynqmp_pmufw_load_config_object(xpm_configobject, > + sizeof(xpm_configobject)); This is not right. It should be return zynqmp_pmufw_load... for error propagation. And tbh zynqmp_pmufw_config_close should also do the same thing. The rest looks good to me. Thanks, Michal
Hi, Am 16.05.2023 um 10:26 schrieb Michal Simek: > Hi, > > first of all sorry for delay. > > On 4/27/23 12:31, Stefan Herbrechtsmeier wrote: >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> Move the check of the permission to change a config object from >> zynqmp_pmufw_node function to zynqmp_pmufw_load_config_object function >> to simplify the code and check the permission only if required. >> >> Signed-off-by: Stefan Herbrechtsmeier >> <stefan.herbrechtsmeier@weidmueller.com> >> >> --- >> >> Changes in v3: >> - Added >> >> drivers/firmware/firmware-zynqmp.c | 32 +++++++++++++++--------------- >> 1 file changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/firmware/firmware-zynqmp.c >> b/drivers/firmware/firmware-zynqmp.c >> index 2b1ad5d2c3..d12159fa78 100644 >> --- a/drivers/firmware/firmware-zynqmp.c >> +++ b/drivers/firmware/firmware-zynqmp.c >> @@ -70,20 +70,11 @@ int zynqmp_pmufw_config_close(void) >> int zynqmp_pmufw_node(u32 id) >> { >> - static bool skip_config; >> - int ret; >> - >> - if (skip_config) >> - return 0; >> - >> /* Record power domain id */ >> xpm_configobject[NODE_ID_LOCATION] = id; >> - ret = zynqmp_pmufw_load_config_object(xpm_configobject, >> - sizeof(xpm_configobject)); >> - >> - if (ret == -EACCES && id == NODE_OCM_BANK_0) >> - skip_config = true; >> + zynqmp_pmufw_load_config_object(xpm_configobject, >> + sizeof(xpm_configobject)); > > This is not right. > It should be > return zynqmp_pmufw_load... for error propagation. At the moment the zynqmp_pmufw_node and zynqmp_pmufw_config_close doesn't return an error. Should the zynqmp_pmufw_load_config_object return 0 or -EACCES if it is skipped? Regards Stefan
On 5/16/23 13:23, Stefan Herbrechtsmeier wrote: > Hi, > > Am 16.05.2023 um 10:26 schrieb Michal Simek: > >> Hi, >> >> first of all sorry for delay. >> >> On 4/27/23 12:31, Stefan Herbrechtsmeier wrote: >>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>> >>> Move the check of the permission to change a config object from >>> zynqmp_pmufw_node function to zynqmp_pmufw_load_config_object function >>> to simplify the code and check the permission only if required. >>> >>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >>> >>> --- >>> >>> Changes in v3: >>> - Added >>> >>> drivers/firmware/firmware-zynqmp.c | 32 +++++++++++++++--------------- >>> 1 file changed, 16 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/firmware/firmware-zynqmp.c >>> b/drivers/firmware/firmware-zynqmp.c >>> index 2b1ad5d2c3..d12159fa78 100644 >>> --- a/drivers/firmware/firmware-zynqmp.c >>> +++ b/drivers/firmware/firmware-zynqmp.c >>> @@ -70,20 +70,11 @@ int zynqmp_pmufw_config_close(void) >>> int zynqmp_pmufw_node(u32 id) >>> { >>> - static bool skip_config; >>> - int ret; >>> - >>> - if (skip_config) >>> - return 0; >>> - >>> /* Record power domain id */ >>> xpm_configobject[NODE_ID_LOCATION] = id; >>> - ret = zynqmp_pmufw_load_config_object(xpm_configobject, >>> - sizeof(xpm_configobject)); >>> - >>> - if (ret == -EACCES && id == NODE_OCM_BANK_0) >>> - skip_config = true; >>> + zynqmp_pmufw_load_config_object(xpm_configobject, >>> + sizeof(xpm_configobject)); >> >> This is not right. >> It should be >> return zynqmp_pmufw_load... for error propagation. > > At the moment the zynqmp_pmufw_node and zynqmp_pmufw_config_close doesn't return > an error. Should the zynqmp_pmufw_load_config_object return 0 or -EACCES if it > is skipped? In context zynqmp_pmufw_node and it's dependency on returning EACESS for failure case which your code depends on here + if (zynqmp_pmufw_node(NODE_OCM_BANK_0) == -EACCES) { + printf("PMUFW: No permission to change config object\n"); + skip = true; + } And for second part around skip and return code. I would say what you have is fine. It means returning -EACCES is appropriate here. And as I see do_zynqmp_pmufw should also return that value. That command will simply fail if there is no permission. And for close part I would say the same. Error should be propagated. I expect current command behavior when you call "pmufw node close" on regular system will just pass but it should just fail because command wasn't successful. Thanks, Michal
diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c index 2b1ad5d2c3..d12159fa78 100644 --- a/drivers/firmware/firmware-zynqmp.c +++ b/drivers/firmware/firmware-zynqmp.c @@ -70,20 +70,11 @@ int zynqmp_pmufw_config_close(void) int zynqmp_pmufw_node(u32 id) { - static bool skip_config; - int ret; - - if (skip_config) - return 0; - /* Record power domain id */ xpm_configobject[NODE_ID_LOCATION] = id; - ret = zynqmp_pmufw_load_config_object(xpm_configobject, - sizeof(xpm_configobject)); - - if (ret == -EACCES && id == NODE_OCM_BANK_0) - skip_config = true; + zynqmp_pmufw_load_config_object(xpm_configobject, + sizeof(xpm_configobject)); return 0; } @@ -239,9 +230,23 @@ int zynqmp_pm_is_function_supported(const u32 api_id, const u32 id) */ int zynqmp_pmufw_load_config_object(const void *cfg_obj, size_t size) { + static bool checked; + static bool skip; int err; u32 ret_payload[PAYLOAD_ARG_CNT]; + if (!checked) { + checked = true; + + if (zynqmp_pmufw_node(NODE_OCM_BANK_0) == -EACCES) { + printf("PMUFW: No permission to change config object\n"); + skip = true; + } + } + + if (skip) + return -EACCES; + if (IS_ENABLED(CONFIG_SPL_BUILD)) printf("Loading new PMUFW cfg obj (%ld bytes)\n", size); @@ -250,8 +255,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 +298,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; };