Re: [Xen-devel] [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-09-11 at 14:01 -0700, Suriyan Ramasami wrote:
> On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@xxxxxxxxxx> 
> 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 <asm/platform.h>
> >>   #include <asm/io.h>
> >>
> >> +/* 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.


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