From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot Date: Fri, 12 Sep 2014 11:08:26 +0100 Message-ID: <1410516506.567.44.camel@kazak.uk.xensource.com> References: <1410456310-31415-1-git-send-email-suriyan.r@gmail.com> <5411EEC2.7050108@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Suriyan Ramasami Cc: keir@xen.org, Julien Grall , Tim Deegan , "xen-devel@lists.xen.org" , Jan Beulich , ian.jackson@citrix.com, Julien Grall List-Id: xen-devel@lists.xenproject.org On Thu, 2014-09-11 at 14:01 -0700, Suriyan Ramasami wrote: > On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall wrote: > > Hello Suriyan, > > > > My email address is @linaro.org not @linaro.com. I nearly miss this patch > > because of this. > > > Sorry about that. A slip on my part. Apologies. > > > Depending if Ian apply his patch (see the conversation on "Odroid-XU..."), I > > would update the title and the message. > > > > > Would the title change back to the previous XU patch - "Add Odroid-XU > board ..etc" with patch version 7 and modified message? > I think something like "xen: arm: add support for exynos secure firmware" or something like that. > > On 11/09/14 10:25, Suriyan Ramasami wrote: > >> > >> diff --git a/xen/arch/arm/platforms/exynos5.c > >> b/xen/arch/arm/platforms/exynos5.c > >> index bc9ae15..334650c 100644 > >> --- a/xen/arch/arm/platforms/exynos5.c > >> +++ b/xen/arch/arm/platforms/exynos5.c > >> @@ -28,6 +28,9 @@ > >> #include > >> #include > >> > >> +/* This corresponds to CONFIG_NR_CPUS in linux config */ > >> +#define EXYNOS_CONFIG_NR_CPUS 0x08 > >> + > >> #define EXYNOS_ARM_CORE0_CONFIG 0x2000 > >> #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr)) > >> #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4) > >> @@ -42,8 +45,6 @@ static int exynos5_init_time(void) > >> u64 mct_base_addr; > >> u64 size; > >> > >> - BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE); > >> - > >> node = dt_find_compatible_node(NULL, NULL, > >> "samsung,exynos4210-mct"); > >> if ( !node ) > >> { > >> @@ -61,7 +62,14 @@ static int exynos5_init_time(void) > >> dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n", > >> mct_base_addr, size); > >> > >> - mct = ioremap_attr(mct_base_addr, PAGE_SIZE, > >> PAGE_HYPERVISOR_NOCACHE); > >> + if ( size <= EXYNOS5_MCT_G_TCON ) > > > > > > Honestly, I don't think this check is very useful. The device tree should > > always contains valid size. > > > No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON? > I thought we are trying to catch wrong values of the offsets that we > use, to poke at the registers. It's basically to catch buggy device tree, or perhaps buggy use of compatible variables which don't guarantee that the registers which we use exist. But I'm mostly ambivalent about the inclusion of the check. I'm not sure if we globally prefer one way or the other but if others feel that such checks don't make sense lets not bother. > >> +static int __init exynos5_smp_init(void) > >> +{ > >> + void __iomem *sysram; > >> + u64 sysram_addr; > >> + u64 sysram_offset; > >> + int rc; > >> + > >> + rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset); > >> + if ( rc ) > >> + return rc; > >> > >> - sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE); > >> + dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n", > >> + sysram_addr, sysram_offset); > >> + > >> + if ( sysram_offset >= PAGE_SIZE ) > >> + { > >> + dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n"); > >> + return -EINVAL; > >> + } > > > > > > As the previous check for the MCT. I don't think it's useful. Also why don't > > do get the size from the device tree? > > > In this case as we are poking only offset 0 or 0x1c which is always > less than PAGE_SIZE, I didn't bother with the size. I could if you > insist. > I can get rid of this check as sysram_offset is always less that PAGE_SIZE. You should certainly map the size given from DT and not just PAGE_SIZE. That might mean the getter function returning the size, or perhaps just have it establish the mapping and return the virtual address of the sysram region. > Also, the next version of this patch, do I still maintain the same > subject and comments, or should this come up as something else? I think we've agreed that a new subject is in order, not sure but I think the bulk of the commit log is still appropriate. A few procedural comments: Please use "git format-patch" (or even git send-email) and not "git show" to export the patch for sending, this will remove the unnecessary indent. The email's subject line will be included into the commit message, so no need to repeat it in the body. "git format-patch" will do the right thing. "Changes between versions" stuff should come after your S-o-b and a "---" (on a line of its own) marker, which means that they won't get included in the commit message. Ian.