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

[Mike Looijmans]

On 20-02-2020 08:01, Michal Simek wrote:
> 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@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.
> >
> > 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.

I checked the arm-trusted-firmware code, there's no Xilinx vendor specific 
implementation of SYSTEM_RESET2 today, so it will only do a generic warm 
reboot and nothing else.

I'll move the patch to our BSP, we need the functionality for software 
update/recovery mechanisms.