Re: [PATCH] devicetree: zynqmp.dtsi: Add bootmode selection support

[Michal Simek]

On 19. 02. 20 19:23, Vesa JÃÃskelÃinen wrote:
> Hi Mike,
> 
> On 19.2.2020 14.20, Mike Looijmans wrote:
>> Add bootmode override support for ZynqMP devices. Allows one to select
>> a boot device by running "reboot qspi32" for example. Activate config
>> item CONFIG_SYSCON_REBOOT_MODE to make this work.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@xxxxxxxx>
>> ---
>> Â arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 24 ++++++++++++++++++++++++
>> Â 1 file changed, 24 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> index 26d926eb1431..4c38d77ecbba 100644
>> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
>> @@ -246,6 +246,30 @@
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ };
>> ÂÂÂÂÂÂÂÂÂ };
>> Â +ÂÂÂÂÂÂÂ /* Clock and Reset control registers for LPD */
>> +ÂÂÂÂÂÂÂ lpd_apb: apb@ff5e0000 {
>> +ÂÂÂÂÂÂÂÂÂÂÂ compatible = "syscon", "simple-mfd";
>> +ÂÂÂÂÂÂÂÂÂÂÂ reg = <0x0 0xff5e0000 0x0 0x400>;
>> +ÂÂÂÂÂÂÂÂÂÂÂ reboot-mode {
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ compatible = "syscon-reboot-mode";
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ offset = <0x200>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mask = <0xf100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* Bit(8) is the "force user" bit */
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-normal = <0x0000>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-psjtag = <0x0100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-qspi24 = <0x1100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-qspi32 = <0x2100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-sd0ÂÂÂ = <0x3100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-nandÂÂ = <0x4100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-sd1ÂÂÂ = <0x6100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-emmcÂÂ = <0x6100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-usb0ÂÂ = <0x7100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-pjtag0 = <0x8100>;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mode-pjtag1 = <0x9100>;
>> + mode-sd1ls = <0xe100>;
> 
> This kinda looks a bit misuse of reboot mode support.
> 
> Usually you are signal with reboot-mode that you want to do factory
> reset, enter recovery mode or such things.
> 
> Now this signaling here is telling that this is used for selecting from
> what device to boot from.
> 
> Another problem is that this now modifies all Xilinx Zynq MPSoCs which
> is kinda wrong. This behavior should really be product/board specific
> and not common for all boards -- undoing this in product/board is
> somewhat cumbersome. Now this change hijacks the "reboot <arg>" with
> this behavior which is not so nice.

Another reason is that on arm64 these regs shoulnd't be accessed by non
secure software and you should setup protection not to enable it.

If this functionality is useful for your design please keep it in your
board dts file.

Thanks,
Michal