From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suriyan Ramasami Subject: Re: [XEN/ARM PATCH v6 1/1] Add OdroidXU board (Exynos 5410) Date: Mon, 8 Sep 2014 10:26:54 -0700 Message-ID: References: <1409871443-15641-1-git-send-email-suriyan.r@gmail.com> <1410171636.10156.148.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1410171636.10156.148.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: keir@xen.org, Tim Deegan , "xen-devel@lists.xen.org" , Jan Beulich , ian.jackson@citrix.com, Julien Grall List-Id: xen-devel@lists.xenproject.org On Mon, Sep 8, 2014 at 3:20 AM, Ian Campbell wrote: > On Thu, 2014-09-04 at 15:57 -0700, Suriyan Ramasami wrote: >> XEN/ARM: Add support for the Odroid-XU board. >> >> The Odroid-XU from hardkernel is an Exynos 5410 based board. >> This patch adds support to the above said board. > > Please could you describe a bit more fully the refactoring you've done > and the new platform you've introduced. e.g. that we now have a general > exynos5 (which currently includes only the 4210) and that the previous > 5250 is a separate platform because of A, B, and C which differ from the > generic. > > Assuming there are no other changes to the patch please feel free to > just reply here with the updated commit text and I'll fold it in as I > commit. > OK, and here it is ... Introduced a generic PLATFORM exynos5 which hopefully is applicable to the majority of exynos5 based SoCs. It currently has only been tested on an exynos5410 based (OdroidXU) board and hence that is the only board listed. Previously only the Arndale board, based on an exynos5250 was supported. It was the only exynos based platform that was supported and it was called exynos5. It has now been renamed to exynos5250. The Arndale currently is a separate platform mostly cause I do not have one to test and for the most part the code path for that board is preserved. To be specific it varies from the generic implementation as follows: 1. exynos5250 based specific DT mapping for CHIPID and PWM region. I believe mainline kernel's DTS for the arndale has those mappings already in place. 2. exynos5250 based cpu up code. It appears that exynos5250 already has the secondary core powered up and in wfe and hence a cpu_up_send_sgi suffices. Here too, I believe that the generic code path might be acceptable. Most of the code for the cpu bring up has been ported over from mainline linux, and hence should be generic enough for future exynos based SoCs. All reference to hardcoded memory locations have been avoided. They are now gleaned from the device tree. >> @@ -77,20 +125,127 @@ static int __init exynos5_smp_init(void) >> >> printk("Set SYSRAM to %"PRIpaddr" (%p)\n", >> __pa(init_secondary), init_secondary); >> - writel(__pa(init_secondary), sysram); >> + writel(__pa(init_secondary), sysram + 0x1c); > > I think this 01c offset is new, where does it come from? Was the old > code wrong (again, if this just needs a few words in the commit log I > can fold them in as I commit). > This 0x1c comes from the Non Secure memory area that u-boot SPL moves its code into. I believe this is the same for all exynos's (i believe they have this same code pattern in lowlevel_init.S. This is what u-boot copies over to start of IRAM_NS_BASE: nscode_base: b 1f nop @ for backward compatibility .word 0x0 @ REG0: RESUME_ADDR .word 0x0 @ REG1: RESUME_FLAG .word 0x0 @ REG2 .word 0x0 @ REG3 _switch_addr: .word 0x0 @ REG4: SWITCH_ADDR _hotplug_addr: <------------------------------------------------------------- +0x1c .word 0x0 @ REG5: CPU1_BOOT_REG ... 1: -> blah blah HYP entry ... blah blah adr r0, _hotplug_addr wait_for_addr: ldr r1, [r0] cmp r1, #0x0 bxne r1 wfe b wait_for_addr ... As can be seen, the secondary CPUs check _hotplug_addr for a non zero value on first being powered on, or after woken up from a wfe. This _hotplug_addr happens to be at offset +0x1c from NS_RAM_BASE. Linux mainline too has this hardcoded +0x1c. >> + static const struct dt_device_match exynos_dt_pmu_matches[] __initconst = >> + { >> + DT_MATCH_COMPATIBLE("samsung,exynos5250-pmu"), >> + DT_MATCH_COMPATIBLE("samsung,exynos5410-pmu"), >> + DT_MATCH_COMPATIBLE("samsung,exynos5420-pmu"), > > I suppose these are all similar enough for our purposes? > Yes, I could have just left it with the exynos5410-pmu and the exynos5250-pmu, but then gathered the other compatible ones which could be used for exynos5 generic boards in the future. >> + { /*sentinel*/ }, >> + }; >> + >> + node = dt_find_matching_node(NULL, exynos_dt_pmu_matches); >> + if ( !node ) >> + { >> + dprintk(XENLOG_ERR, "samsung,exynos5XXX-pmu missing in DT\n"); > > We have some 4XXX in the list too. Make it exynosXXXX? (likewise the > next message which was below but I've trimmed it by mistake). > I do not see any 4XXX in the pmu list that I have added. I think you are referring the sysram compatible string which has exynos4 stuff in it. And those dprintk the right string. Same comment for the next message, but I see a typo where I have an extra X in the exynos5XXXX instead of exynos5XXX Please let me know if I should roll another patch with these comments and the minor 'X' change. Thanks! > Ian. >