Re: [Xen-devel] [PATCH v2 1/2] xen/arm64: Add Support for Allwinner H5 (sun50i)

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 09/28/2017 01:03 PM, Julien Grall wrote:
> Hi,
>
> On 09/26/2017 10:37 AM, Awais Masood wrote:
> > This patch adds support for Allwinner H5/sun50i SoC.
> >
> > Makefile updated to enable ARM64 compilation for sunxi.c.
> >
> > sunxi.c updates include:
> >    - Addition of H5/sun50i dt compatibility string.
> >    - Handling of different Watchdog timer base addresses on sun7i
> >      and sun50i.
> >
> > Tested on Orange Pi PC2
> >
> > Signed-off-by: Awais Masood <awais.masood@xxxxxxxxxx>
> >
> > ---
> > Changes since v1:
> >    - Improved patch description
> > ---
> >   xen/arch/arm/platforms/Makefile |  1 +
> >   xen/arch/arm/platforms/sunxi.c  | 40 
> > +++++++++++++++++++++++++++++++---------
> >   2 files changed, 32 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/arch/arm/platforms/Makefile 
> > b/xen/arch/arm/platforms/Makefile
> > index 49fa683..722897a 100644
> > --- a/xen/arch/arm/platforms/Makefile
> > +++ b/xen/arch/arm/platforms/Makefile
> > @@ -6,5 +6,6 @@ obj-$(CONFIG_ARM_32) += omap5.o
> >   obj-$(CONFIG_ARM_32) += rcar2.o
> >   obj-$(CONFIG_ARM_64) += seattle.o
> >   obj-$(CONFIG_ARM_32) += sunxi.o
> > +obj-$(CONFIG_ARM_64) += sunxi.o
> Please use obj-y += sunxi.o as the platform is now supported by both 
> Arm32 and Arm64.
> >   obj-$(CONFIG_ARM_64) += xgene-storm.o
> >   obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
> > diff --git a/xen/arch/arm/platforms/sunxi.c 
> > b/xen/arch/arm/platforms/sunxi.c
> > index 0ba7b3d..06d62e7 100644
> > --- a/xen/arch/arm/platforms/sunxi.c
> > +++ b/xen/arch/arm/platforms/sunxi.c
> > @@ -22,18 +22,18 @@
> >   #include <asm/io.h>
> >   /* Watchdog constants: */
> > -#define SUNXI_WDT_BASE            0x01c20c90
> > +#define SUNXI_WDT_A20_BASE        0x01c20c90
> > +#define SUNXI_WDT_H5_BASE         0x01c20cA0
> I know that we hardcoded this value for the A20. However, I am wondering 
> if we could find this address from the Device-Tree?
Yes, both sun7i-a20.dtsi and the H5 .dts have the WDT.
Its compatible strings are sun4i-a10-wdt and sun6i-a31-wdt, 
respectively. I have to check what the differences are, but I guess for 
our purposes these should be small.
That seems like a call to some proper DT driven timer/WDT driver?

> >   #define SUNXI_WDT_MODE            0x04
> > -#define SUNXI_WDT_MODEADDR        (SUNXI_WDT_BASE + SUNXI_WDT_MODE)
> >   #define SUNXI_WDT_MODE_EN         (1 << 0)
> >   #define SUNXI_WDT_MODE_RST_EN     (1 << 1)
> > -static void sunxi_reset(void)
> > +static void sunxi_reset(u32 base)
> >   {
> >       void __iomem *wdt;
> > -    wdt = ioremap_nocache(SUNXI_WDT_MODEADDR & PAGE_MASK, PAGE_SIZE);
> > +    wdt = ioremap_nocache((base + SUNXI_WDT_MODE) & PAGE_MASK, 
> > PAGE_SIZE);
> >       if ( !wdt )
> >       {
> >           dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
> > @@ -42,19 +42,35 @@ static void sunxi_reset(void)
> >       /* Enable watchdog to trigger a reset after 500 ms: */
> >       writel(SUNXI_WDT_MODE_EN | SUNXI_WDT_MODE_RST_EN,
> > -      wdt + (SUNXI_WDT_MODEADDR & ~PAGE_MASK));
> > +      wdt + ((base + SUNXI_WDT_MODE) & ~PAGE_MASK));
> >       iounmap(wdt); >
> >       for (;;)
> >           wfi();
> >   }
> >
> > -static const char * const sunxi_dt_compat[] __initconst =
> > +static void sunxi_a20_reset(void)
> > +{
> > +    sunxi_reset(SUNXI_WDT_A20_BASE);
> > +}
> > +
> > +static void sunxi_h5_reset(void)
> > +{
> > +    sunxi_reset(SUNXI_WDT_H5_BASE);
> If I read correctly the Device-Tree for 
> (linux/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi), the firmware is 
> supporting PSCI 0.2.
> PSCI 0.2 provides call for power-off/reset, so implementation the reset 
> callback should not be necessary.
Yes, indeed, on the H5 PSCI 0.2 reset works via ATF.

> Similarly the cubietrucks we have in osstest are using PSCI 0.2 and 
> should not need the reset. Andre do you know if it is the case for all 
> the A20?
It claims 0.2, but in fact it seems not to be fully compliant, as (from 
looking at the code) U-Boot lacks the reset and poweroff calls. But it 
looks rather straight-forward to add them, as U-Boot knows how to reset 
and one would just need to wire up psci_system_reset to this.
> For H5, I would impose PSCI 0.2 as the way to reset the platform.
Yes.

> I am 
> leaning towards the same for A20 given that it would just be a matter of 
> upgrading the bootloader. Most likely you would have already done that 
> to get fixes.
Not sure we should push people to upgrade U-Boot in general to be able 
to use Xen, but as even current mainline U-Boot doesn't seem to support 
it, I would rather leave the current reset support code in. Last time I 
checked Linux does the same.
Cheers,
Andre.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel