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

[Suriyan Ramasami <suriyan.r@xxxxxxxxx>]

On Fri, Sep 12, 2014 at 3:08 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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.
>
OK.

>> > 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.
>
OK, I shall get rid of the checks.

>> >> +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.
>
OK, shall map the full size as specified in the DT, and not PAGE_SIZE.

>> 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.
>
I actually use git format-patch to generate the patch, and I do edit
some of the comments in that patch file, before I use git send-email.
Can you please give me an example of the  "unnecessary indent" that
you mention, so I can check what I am doing wrong.

> 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.
>
Sorry about that. I did know that part (has been repeatedly mentioned
before). It was a mistake on my part, while editing the output of
format-patch, I manually copy and paste the version changes that I am
doing, and this time around I pasted it before the S-o-b. Is that not
the right approach?

> Ian.
>

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