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

[Awais Masood <awais.masood@xxxxxxxxxx>]

Hi,

On 10/04/2017 02:26 PM, Andre Przywara wrote:
> Hi Awais,
> 
> On 04/10/17 10:16, Awais Masood wrote:
>> Hi,
>>
>> On 09/29/2017 09:35 PM, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 09/28/2017 03:49 PM, Andre Przywara wrote:
>>>> 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.
>>>
>>> ...
>>>
>>>>>> --- 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?
>>>
>>> Scratch that. I just see that this is solely used for the reset function. 
>>> So we should not need this for the H5 (and the A64 for that matter). We may 
>>> need this for the H3 (Cortex-A7) support, however, which seems quite 
>>> popular on cheap boards.
>>>
>>
>> Since reset routine will not be required with PSCI, I assume should revert 
>> the reset code changes for this H5 patch and leave the DT retrieval for 
>> another patch that adds H3 support. Or should I try that stuff for next 
>> version of this patch? 
> 
> Thanks for the offer, but I already made a patch that adds support for
> basically all virtualization capable Allwinner SoCs (both v7 and v8
> ones). This looks into the DT for ARMv7 SoCs, but relies entirely on
> PSCI for ARMv8 SoCs. I just need to test it, then will send it out.
> 
> So actually we won't need anything from that patch here at all, since my
> patch supersedes it in a more generic way.
> Do you plan on reworking/resending the UART fix (which should come
> first, btw, as it is a prerequisite for H5 enablement)?
> 
> You could either send the UART fix on its own if there are changes or I
> include it as patch 1/2 of my Allwinner "series".

I'll send the latest version of UART fix as a standalone patch.

Awais

> 
> Thanks!
> Andre.
> 
>>> Cheers,
>>> Andre
>>>
>>>>>>   #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