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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f2592ed7-9f1b-9a23-a6bd-ed8773a7873e@xilinx.com>
Date:   Thu, 20 Feb 2020 08:01:39 +0100
From:   Michal Simek <michal.simek@...inx.com>
To:     Mike Looijmans <mike.looijmans@...ic.nl>,
        Vesa Jääskeläinen <dachaac@...il.com>,
        robh+dt@...nel.org, michal.simek@...inx.com, mark.rutland@....com,
        devicetree@...r.kernel.org
Cc:     m.tretter@...gutronix.de, nava.manne@...inx.com,
        rajan.vaja@...inx.com, manish.narani@...inx.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] devicetree: zynqmp.dtsi: Add bootmode selection support

On 20. 02. 20 7:56, Mike Looijmans wrote:
> On 19-02-2020 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@...ic.nl>
>>> ---
>>>   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@...e0000 {
>>> +            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.
> 
> On the ZynqMP this is the only way to communicate with the ROM.
> 
>> 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. 
> 
> The boot mode setting is in the SOC, and is not board specific. The ROM
> interprets this field. The only board specific thing is that you may not
> actually have a NAND chip attached to it.
> 
> My idea was that a board could easily add say 'mode-recovery=<0x2100>;'
> to make the QPSI boot the method of recovery. The bootloader also has
> access to this register, so it can see that there was a boot mode
> override in effect.
> 
>> Now this change hijacks the "reboot <arg>" with this behavior which is
>> not so nice.
> 
> If anyone has a better suggestion as to where this should go, I'd be
> more than happy to hear about it. It's the only interface that I could
> find in the kernel to attach a bootmode override to.

IIRC as the part of PSCI 1.1 spec is SYSTEM_RESET2 where you can device
reset_type. IIRC that 0 as warm reset was coming based on discussion
with Xilinx (and maybe others) and I think this is what Xilinx is still
using. But didn't track it if that was really implemented or not.

Thanks,
Michal