Message ID | 20240603133447.173402-3-lukas.funke-oss@weidmueller.com |
---|---|
State | Superseded |
Delegated to: | Michal Simek |
Headers | show |
Series | Enable reset_cpu() in SPL for ZynqMP | expand |
On 6/3/24 15:34, lukas.funke-oss@weidmueller.com wrote: > From: Lukas Funke <lukas.funke@weidmueller.com> > > This commit enables SPL to reset the CPU via PMU-firmware. The usual > reset mechanism requires bl31 to be loaded which may not be the case in > SPL. > > Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> > --- > > board/xilinx/zynqmp/zynqmp.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c > index 95a134b972d..99f5c178c1d 100644 > --- a/board/xilinx/zynqmp/zynqmp.c > +++ b/board/xilinx/zynqmp/zynqmp.c > @@ -40,6 +40,7 @@ > #include <linux/bitops.h> > #include <linux/delay.h> > #include <linux/sizes.h> > +#include <dt-bindings/reset/xlnx-zynqmp-resets.h> > #include "../common/board.h" > > #include "pm_cfg_obj.h" > @@ -285,6 +286,14 @@ int dram_init(void) > #if !CONFIG_IS_ENABLED(SYSRESET) > void reset_cpu(void) > { > + if (!CONFIG_IS_ENABLED(ZYNQMP_FIRMWARE)) { > + log_warning("reset failed: ZYNQMP_FIRMWARE disabled"); > + return; > + } > + > + xilinx_pm_request(PM_RESET_ASSERT, > + ZYNQMP_PM_RESET_START + ZYNQMP_RESET_SOFT, > + PM_RESET_ACTION_ASSERT, 0, 0, NULL); If you disable ZYNQMP_FIRMWARE xilinx_pm_request() should fail. we should likely fix it in drivers/mmc/zynq_sdhci.c:114 Thanks, Michal
On 03.06.2024 16:32, Michal Simek wrote: > > > On 6/3/24 15:34, lukas.funke-oss@weidmueller.com wrote: >> From: Lukas Funke <lukas.funke@weidmueller.com> >> >> This commit enables SPL to reset the CPU via PMU-firmware. The usual >> reset mechanism requires bl31 to be loaded which may not be the case in >> SPL. >> >> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >> --- >> >> board/xilinx/zynqmp/zynqmp.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c >> index 95a134b972d..99f5c178c1d 100644 >> --- a/board/xilinx/zynqmp/zynqmp.c >> +++ b/board/xilinx/zynqmp/zynqmp.c >> @@ -40,6 +40,7 @@ >> #include <linux/bitops.h> >> #include <linux/delay.h> >> #include <linux/sizes.h> >> +#include <dt-bindings/reset/xlnx-zynqmp-resets.h> >> #include "../common/board.h" >> #include "pm_cfg_obj.h" >> @@ -285,6 +286,14 @@ int dram_init(void) >> #if !CONFIG_IS_ENABLED(SYSRESET) >> void reset_cpu(void) >> { >> + if (!CONFIG_IS_ENABLED(ZYNQMP_FIRMWARE)) { >> + log_warning("reset failed: ZYNQMP_FIRMWARE disabled"); >> + return; >> + } >> + >> + xilinx_pm_request(PM_RESET_ASSERT, >> + ZYNQMP_PM_RESET_START + ZYNQMP_RESET_SOFT, >> + PM_RESET_ACTION_ASSERT, 0, 0, NULL); > > If you disable ZYNQMP_FIRMWARE xilinx_pm_request() should fail. > > we should likely fix it in drivers/mmc/zynq_sdhci.c:114 That's an odd place for a default implementation. I'd like to move the functions to a common location but I've no idea where. That's probably why the last developer moved it here. Any suggestions? > > Thanks, > Michal
On 6/3/24 16:50, Lukas Funke wrote: > On 03.06.2024 16:32, Michal Simek wrote: >> >> >> On 6/3/24 15:34, lukas.funke-oss@weidmueller.com wrote: >>> From: Lukas Funke <lukas.funke@weidmueller.com> >>> >>> This commit enables SPL to reset the CPU via PMU-firmware. The usual >>> reset mechanism requires bl31 to be loaded which may not be the case in >>> SPL. >>> >>> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >>> --- >>> >>> board/xilinx/zynqmp/zynqmp.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c >>> index 95a134b972d..99f5c178c1d 100644 >>> --- a/board/xilinx/zynqmp/zynqmp.c >>> +++ b/board/xilinx/zynqmp/zynqmp.c >>> @@ -40,6 +40,7 @@ >>> #include <linux/bitops.h> >>> #include <linux/delay.h> >>> #include <linux/sizes.h> >>> +#include <dt-bindings/reset/xlnx-zynqmp-resets.h> >>> #include "../common/board.h" >>> #include "pm_cfg_obj.h" >>> @@ -285,6 +286,14 @@ int dram_init(void) >>> #if !CONFIG_IS_ENABLED(SYSRESET) >>> void reset_cpu(void) >>> { >>> + if (!CONFIG_IS_ENABLED(ZYNQMP_FIRMWARE)) { >>> + log_warning("reset failed: ZYNQMP_FIRMWARE disabled"); >>> + return; >>> + } >>> + >>> + xilinx_pm_request(PM_RESET_ASSERT, >>> + ZYNQMP_PM_RESET_START + ZYNQMP_RESET_SOFT, >>> + PM_RESET_ACTION_ASSERT, 0, 0, NULL); >> >> If you disable ZYNQMP_FIRMWARE xilinx_pm_request() should fail. >> >> we should likely fix it in drivers/mmc/zynq_sdhci.c:114 > > That's an odd place for a default implementation. I'd like to move the functions > to a common location but I've no idea where. That's probably why the last > developer moved it here. Any suggestions? static inline to include/zynqmp_firmware.h? M
On 03.06.2024 17:08, Michal Simek wrote: > > > On 6/3/24 16:50, Lukas Funke wrote: >> On 03.06.2024 16:32, Michal Simek wrote: >>> >>> >>> On 6/3/24 15:34, lukas.funke-oss@weidmueller.com wrote: >>>> From: Lukas Funke <lukas.funke@weidmueller.com> >>>> >>>> This commit enables SPL to reset the CPU via PMU-firmware. The usual >>>> reset mechanism requires bl31 to be loaded which may not be the case in >>>> SPL. >>>> >>>> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >>>> --- >>>> >>>> board/xilinx/zynqmp/zynqmp.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/board/xilinx/zynqmp/zynqmp.c >>>> b/board/xilinx/zynqmp/zynqmp.c >>>> index 95a134b972d..99f5c178c1d 100644 >>>> --- a/board/xilinx/zynqmp/zynqmp.c >>>> +++ b/board/xilinx/zynqmp/zynqmp.c >>>> @@ -40,6 +40,7 @@ >>>> #include <linux/bitops.h> >>>> #include <linux/delay.h> >>>> #include <linux/sizes.h> >>>> +#include <dt-bindings/reset/xlnx-zynqmp-resets.h> >>>> #include "../common/board.h" >>>> #include "pm_cfg_obj.h" >>>> @@ -285,6 +286,14 @@ int dram_init(void) >>>> #if !CONFIG_IS_ENABLED(SYSRESET) >>>> void reset_cpu(void) >>>> { >>>> + if (!CONFIG_IS_ENABLED(ZYNQMP_FIRMWARE)) { >>>> + log_warning("reset failed: ZYNQMP_FIRMWARE disabled"); >>>> + return; >>>> + } >>>> + >>>> + xilinx_pm_request(PM_RESET_ASSERT, >>>> + ZYNQMP_PM_RESET_START + ZYNQMP_RESET_SOFT, >>>> + PM_RESET_ACTION_ASSERT, 0, 0, NULL); >>> >>> If you disable ZYNQMP_FIRMWARE xilinx_pm_request() should fail. >>> >>> we should likely fix it in drivers/mmc/zynq_sdhci.c:114 >> >> That's an odd place for a default implementation. I'd like to move the >> functions to a common location but I've no idea where. That's probably >> why the last developer moved it here. Any suggestions? > > static inline to include/zynqmp_firmware.h? Sound like the right place. Currently zynqmp is not buildable when disabling CONFIG_ZYNQMP_FIRMWARE. I'm not sure if it even makes sense to disable it. Maybe this should be enforced!? However, I'll drop the 1/2 patch and convert 'CONFIG_IS_ENABLED' to 'IS_ENABLED' in v2 to get finish this series. Refactoring should be addressed in another series. Best regards - Lukas > > M
On 6/4/24 11:12, Lukas Funke wrote: > On 03.06.2024 17:08, Michal Simek wrote: >> >> >> On 6/3/24 16:50, Lukas Funke wrote: >>> On 03.06.2024 16:32, Michal Simek wrote: >>>> >>>> >>>> On 6/3/24 15:34, lukas.funke-oss@weidmueller.com wrote: >>>>> From: Lukas Funke <lukas.funke@weidmueller.com> >>>>> >>>>> This commit enables SPL to reset the CPU via PMU-firmware. The usual >>>>> reset mechanism requires bl31 to be loaded which may not be the case in >>>>> SPL. >>>>> >>>>> Signed-off-by: Lukas Funke <lukas.funke@weidmueller.com> >>>>> --- >>>>> >>>>> board/xilinx/zynqmp/zynqmp.c | 9 +++++++++ >>>>> 1 file changed, 9 insertions(+) >>>>> >>>>> diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c >>>>> index 95a134b972d..99f5c178c1d 100644 >>>>> --- a/board/xilinx/zynqmp/zynqmp.c >>>>> +++ b/board/xilinx/zynqmp/zynqmp.c >>>>> @@ -40,6 +40,7 @@ >>>>> #include <linux/bitops.h> >>>>> #include <linux/delay.h> >>>>> #include <linux/sizes.h> >>>>> +#include <dt-bindings/reset/xlnx-zynqmp-resets.h> >>>>> #include "../common/board.h" >>>>> #include "pm_cfg_obj.h" >>>>> @@ -285,6 +286,14 @@ int dram_init(void) >>>>> #if !CONFIG_IS_ENABLED(SYSRESET) >>>>> void reset_cpu(void) >>>>> { >>>>> + if (!CONFIG_IS_ENABLED(ZYNQMP_FIRMWARE)) { >>>>> + log_warning("reset failed: ZYNQMP_FIRMWARE disabled"); >>>>> + return; >>>>> + } >>>>> + >>>>> + xilinx_pm_request(PM_RESET_ASSERT, >>>>> + ZYNQMP_PM_RESET_START + ZYNQMP_RESET_SOFT, >>>>> + PM_RESET_ACTION_ASSERT, 0, 0, NULL); >>>> >>>> If you disable ZYNQMP_FIRMWARE xilinx_pm_request() should fail. >>>> >>>> we should likely fix it in drivers/mmc/zynq_sdhci.c:114 >>> >>> That's an odd place for a default implementation. I'd like to move the >>> functions to a common location but I've no idea where. That's probably why >>> the last developer moved it here. Any suggestions? >> >> static inline to include/zynqmp_firmware.h? > > Sound like the right place. > > Currently zynqmp is not buildable when disabling CONFIG_ZYNQMP_FIRMWARE. I'm not > sure if it even makes sense to disable it. Maybe this should be enforced!? > > However, I'll drop the 1/2 patch and convert 'CONFIG_IS_ENABLED' to 'IS_ENABLED' > in v2 to get finish this series. Refactoring should be addressed in another series. In past it was mandatory and then it was changed by this patch because mini u-boot configurations don't need it. commit 71efd45a5fc70e29e391e0b57c700de8708ae6d9 Author: Michal Simek <michal.simek@amd.com> AuthorDate: Fri Jan 14 13:08:42 2022 +0100 Commit: Michal Simek <michal.simek@amd.com> CommitDate: Wed Jan 19 11:36:11 2022 +0100 arm64: zynqmp: Change firmware dependency In case of mini U-Boot configurations there is no need to enable firmware driver which just consume space for nothing. That's why add an option to disable it. Signed-off-by: Michal Simek <michal.simek@xilinx.com> Link: https://lore.kernel.org/r/d439399160ff3374f2b39f54f7dd70fa8c8bfea0.1642162121.git.michal.simek@xilinx.com Back to your question. Even if we skip mini u-boot cases there could be still value not to use firmware interface but it is not tested or used at this stage. But if you simply used fixed clocks it should be possible to use it. That's why imply has been used not to force everybody to use it but also it is necessary to say that I don't want to block others to do it. Thanks, Michal
diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c index 95a134b972d..99f5c178c1d 100644 --- a/board/xilinx/zynqmp/zynqmp.c +++ b/board/xilinx/zynqmp/zynqmp.c @@ -40,6 +40,7 @@ #include <linux/bitops.h> #include <linux/delay.h> #include <linux/sizes.h> +#include <dt-bindings/reset/xlnx-zynqmp-resets.h> #include "../common/board.h" #include "pm_cfg_obj.h" @@ -285,6 +286,14 @@ int dram_init(void) #if !CONFIG_IS_ENABLED(SYSRESET) void reset_cpu(void) { + if (!CONFIG_IS_ENABLED(ZYNQMP_FIRMWARE)) { + log_warning("reset failed: ZYNQMP_FIRMWARE disabled"); + return; + } + + xilinx_pm_request(PM_RESET_ASSERT, + ZYNQMP_PM_RESET_START + ZYNQMP_RESET_SOFT, + PM_RESET_ACTION_ASSERT, 0, 0, NULL); } #endif