[PATCH 2/2] xilinx: zynqmp: Enable reset_cpu() in SPL

[Michal Simek]

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 at weidmueller.com wrote:
>>>>> From: Lukas Funke <lukas.funke at 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 at 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 at amd.com>
AuthorDate: Fri Jan 14 13:08:42 2022 +0100
Commit:     Michal Simek <michal.simek at 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 at 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