Re: [Xen-devel] [Patch v2 1/1] Add Odroid-XU (Exynos 5410)

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Ian,

On 07/28/2014 10:03 AM, Ian Campbell wrote:
> On Sat, 2014-07-26 at 23:17 +0100, Julien Grall wrote:
>>> @@ -67,8 +68,19 @@ static int exynos5_specific_mapping(struct domain *d)
>>>   static int __init exynos5_smp_init(void)
>>>   {
>>>       void __iomem *sysram;
>>> +    unsigned long pa_sysram;
>>>
>>> -    sysram = ioremap_nocache(S5P_PA_SYSRAM, PAGE_SIZE);
>>> +    if ( dt_machine_is_compatible(EXYNOS5250_COMPAT) )
>>> +        pa_sysram = S5P_PA_SYSRAM;
>>> +    else if ( dt_machine_is_compatible(EXYNOS5410_COMPAT) )
>>> +        pa_sysram = EXYNOS5410_PA_SYSRAM;
>>
>> I dislike this solution. We've introduced the platform API to avoid 
>> checking the compatible string every time we call callbacks and make 
>> them very simply.
>>
>> Rather than if/else stuff I would prefer if you define a different 
>> platform, and create a common function which will be called with the 
>> right parameters.
> 
> I disagree. For platforms which are very similar and differ only in
> small details a single platform and some small details a single platform
> and a few compatible checks are IMHO a perfectly fine approach.

While it's perfectly fine when the platform code support only 2
different version processor. The if/else will become very hard to read
when new board will be added (such as the Arndale octa).

> For example in this case the smp_init is identical apart from the load
> address, and a single platform+callback and detecting the load address
> via is_compatible is absolutely fine.

Hence, at every dt_machine_is_compatible you have to think: "Is this my
machine?". Having 2 platform would be easier to read such as:

exynos5_smp_init(paddr_t sysram)
{
}

exynos5250_smp_init(void)
{
   return exynos_smp_init(sysram_exynos5250);
}

exynos5410_smp_init(void)
{
   return exynos_smp_init(sysram_exynos5410);
}

No need to check the compatible every time and save us from expensive
checking.

> 
>> On another side, Linux has introduced recently a new bindings to 
>> retrieve the sysram from the device tree. See commit b3205dea
>> "ARM: EXYNOS: Map SYSRAM through generic DT bindings" in the Linux 
>> upstream tree.
>>
>> I would definitely prefer to use this way on Xen to make simpler adding 
>> a new exynos support. The drawback is we don't support older kernel on 
>> Xen for those processors.
> 
> I think it would be reasonable to use SYSRAM if present but have a
> fallback to something based on the compat string for backwards compat
> (which I presume is what the kernel must do too).

The Linux kernel is tight to a specific device tree. While the fallback
is perfectly fine for the Arndale (because we already supported it), the
support of the Odroid xu in Linux will use the bindings by default.

We should use the same things and avoid if/else as much as possible and
use the device tree for this purpose. Hence the binding is very simple.

>> For odroid XU (aka exynos 5410) it's perfectly fine as we don't have yet 
>> a support.
>> For the exynos5250, I think it's arguable. I don't think this board is 
>> used in production but we may want to keep binding compatibility support 
>> with Xen 4.4. I will let the ARM maintainers decide on this point.
>>
>> If we decide to keep compatibility, then I still would prefer a clean 
>> support for Odroid Xu. Even if it's mean to rework the current exynos 
>> platform code.
>>
>>> +    else
>>> +    {
>>> +        dprintk(XENLOG_ERR, "Machine not compatible!\n");
>>> +        return -EFAULT;
>>> +    }
> 
> This can be a BUG() since the compat list only contains two options and
> you've considered both of them.
> 
>>> +
>>> +    sysram = ioremap_nocache(pa_sysram, PAGE_SIZE);
>>>       if ( !sysram )
>>>       {
>>>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>>> @@ -84,6 +96,48 @@ static int __init exynos5_smp_init(void)
>>>       return 0;
>>>   }
>>>
>>> +static int __init exynos5_cpu_up(int cpu)
>>> +{
>>> +    void __iomem *power;
>>> +    char byte0, byte4;
>>> +    int rc;
>>> +
>>> +    if ( dt_machine_is_compatible(EXYNOS5250_COMPAT) ) {
>>> +        rc = cpu_up_send_sgi(cpu);
>>> +        return rc;
>>> +    }
>>
>> Introducing 2 differents platform (one for the exynos5250 and the other 
>> for the 5410) would permit to have a separate callbacks for the cpu up 
>> and avoid again the if/else.
> 
> I'd be happy if this was structured:
> 
>       if ( dt_machine_is_compatible(....5410) )
>       {
>           /* EXYNOS5410: Power the secondary core */
>                 ...
>       }
> 
>       return cpu_up_send_sgi(cpu);

So you add an indentation for about 50 lines just because we want to
keep a single platform structure...

Less than 5% of the code is shared. It's perfectly fine to have 2
different platforms

PLATFORM_EXYNOS5250
        .cpu_up = cpu_up_send_sgi

PLATFORM_EXYNOS5410
        .cpu_up = exynos5410_cpu_up

>>
>>> +    /* EXYNOS5410: Power the secondary core */
>>> +    power = ioremap_nocache(EXYNOS5410_POWER_CPU_BASE +
>>> +                            cpu * EXYNOS5410_POWER_CPU_OFFSET, PAGE_SIZE);
>>
>> Is it really exynos5410 specific? Linux upstream seems to have a generic 
>> way to bring up secondary CPUs. It would be better if we have a similar 
>> one. So bring up a new exynos platform will be more easier.
> 
> Certainly if this could be applied to all exynos (even the 5250) it
> would be much preferable.
> 
>>
>>> +    if ( !power )
>>> +    {
>>> +        dprintk(XENLOG_ERR, "Unable to map exynos5410 MMIO\n");
>>> +        return -EFAULT;
>>> +    }
>>> +
>>> +    byte0 = readb(power);
>>> +    byte4 = readb(power + 4);
>>> +    dprintk(XENLOG_INFO, "Power: %x status: %x\n", byte0, byte4);
>>> +
>>> +    writeb(EXYNOS5410_POWER_ENABLE, power);
>>> +    dprintk(XENLOG_INFO, "Waiting for power status to change to %x\n",
>>> +            EXYNOS5410_POWER_ENABLE);
>>> +
>>> +    do
>>> +    {
>>> +        udelay(1);
>>> +        byte4 = readb(power + 4);
>>
>> Xen will go in an infinite loop, if the CPU has not been correctly 
>> enabled. I think you should add a timeout there.
> 
> Yes, please.
> 
> If you can find more meaningful names than byte0 and byte4 that would be
> good too.
> 
>>> @@ -122,7 +177,7 @@ PLATFORM_START(exynos5, "SAMSUNG EXYNOS5")
>>>       .init_time = exynos5_init_time,
>>>       .specific_mapping = exynos5_specific_mapping,
>>
>> I'm not sure if those specific mapping are relevant for the odroid XU... 
>> Hence IIRC, they are now defined in the device tree.
> 
> A difference in this *would* require a separate platform, since this is
> only a list not a function.
> 

Hrm... the specific mapping is a function. You are mixing with the
blacklist field.


> Is it possible that the 52xx DTBs have caught up and these are no longer
> required even there though?

The chipid is not defined in the device tree bindings. IIRC, it's not
the case for the PWM region. I will have to check the code.

Regards,

-- 
Julien Grall

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