Re: [Xen-devel] [Patch v3 1/1] Add Odroid-XU (Exynos5410)

[Suriyan Ramasami <suriyan.r@xxxxxxxxx>]

Thank you Julien for your review, much appreciated!

On Mon, Aug 18, 2014 at 12:07 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hello Suriyan,
>
>
> On 13/08/14 22:12, Suriyan Ramasami wrote:
>>
>> -static int __init exynos5_smp_init(void)
>> +static int __init exynos5250_cpu_up(int cpu)
>> +{
>> +    return cpu_up_send_sgi(cpu);
>> +}
>> +
>
>
> This is not necessary. You can directly assign cpu_up_send_sgi to the cpu_up
> callback of the platform structure.
>
I shall do that.

>
>> +static int __init exynos_smp_init(unsigned long pa_sysram)
>
>
> paddr_t pa_sysram
>
>
>>   {
>>       void __iomem *sysram;
>>
>> -    sysram = ioremap_nocache(S5P_PA_SYSRAM, PAGE_SIZE);
>> +    sysram = ioremap_nocache(pa_sysram, PAGE_SIZE);
>>       if ( !sysram )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>> @@ -84,6 +95,121 @@ static int __init exynos5_smp_init(void)
>>       return 0;
>>   }
>>
>> +static int __init exynos5250_smp_init(void)
>> +{
>> +    return exynos_smp_init(S5P_PA_SYSRAM);
>> +}
>
>
> I would rename S5P_PA_SYSRAM and EXYNOS5_* to something specific to exynos
> 5250. It would make clear that new platform should use the device tree to
> get information.
>
I shall rename S5P_PA_SYSRAM to EXYNOS5250_PA_SYSRAM.
Do you also want me to rename the other ones, like EXYNOS5_MCT_*
defines in the exynos5.h header file as well?
If so, do you want me to get rid of exynos5.h and dump all the defines
in exynos5.c ?

>
>> +static int __init exynos5_smp_init(void)
>> +{
>> +    struct dt_device_node *node;
>> +    u64 sysram_ns_base_addr = 0;
>> +    u64 size;
>> +    int rc;
>> +
>> +    node = dt_find_compatible_node(NULL, NULL,
>> "samsung,exynos4210-sysram-ns");
>> +    if ( !node ) {
>
>
> Coding style:
>
>
> if ( !node )
> {
>
I shall correct this.

>> +       dprintk(XENLOG_ERR, "samsung,exynos4210-sysram-ns missing in
>> DT\n");
>
>
> The coding style doesn't allow to use hard tab.
>
>
I shall correct this too.

>> +        return -ENXIO;
>> +    }
>> +
>> +    rc = dt_device_get_address(node, 0, &sysram_ns_base_addr, &size);
>> +    if (rc) {
>
>
> Coding style:
>
> if ( rc )
> {
>
and this one as well.

>> +        dprintk(XENLOG_ERR, "Error in
>> \"samsung,exynos4210-sysram-ns\"\n");
>>
>> +        return -ENXIO;
>> +    }
>> +
>> +    dprintk(XENLOG_INFO, "sysram_ns_base_addr: %08x size: %08x\n",
>> +            (unsigned int) sysram_ns_base_addr, (unsigned int) size);
>
>
> Why do you cast to unsigned int rather than right printf format?
>
Cause I am an idiot :-) I shall correct this to use %016llx.
>
>> +
>> +    return exynos_smp_init(sysram_ns_base_addr + 0x1c);
>> +}
>> +
>> +static int exynos_cpu_power_state(void __iomem *power, int cpu)
>> +{
>> +    return readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>> +                  S5P_CORE_LOCAL_PWR_EN;
>
>
> Please correctly align the second line to the open branch. I.e:
>
> return readl(power ....
>              S5P_CORE_...
>
I shall do so.

> Also, why did you replace the __raw_readl by a readl? The former one doesn't
> use barrier while the latter does.
>
> As Linux is using the former one, I don't understand why we would require
> barrier on Xen.
>
I shall use __raw_readl()
>
>> +}
>> +
>> +static void exynos_cpu_set_power_up(void __iomem *power, int cpu)
>> +{
>> +    writel(S5P_CORE_LOCAL_PWR_EN,
>> +        power + EXYNOS_ARM_CORE_CONFIG(cpu));
>
>
> Same here for both coding style and writel.
>
Shall correct them both.

>
>> +}
>> +
>> +static int exynos_cpu_power_up(void __iomem *power, int cpu)
>> +{
>> +    int timeout;
>
>
> unsigned int.
>
>
I shall correct this one too.

>> +
>> +    if ( !exynos_cpu_power_state(power, cpu) ) {
>
>
> Coding style:
>
> if ( ... )
> {
>
This one as well.

>> +        exynos_cpu_set_power_up(power, cpu);
>
>
> I would keep the same name as Linux ie exynos_cpu_power_up to avoid
> confusion and make backporting easier.
>
OK, I shall make it exynos_cpu_power_up() like in linux, and the
caller exynos5_cpu_powerup().

>
>> +        timeout = 10;
>> +
>> +        /* wait max 10 ms until cpu is on */
>> +        while (exynos_cpu_power_state(power, cpu) !=
>> S5P_CORE_LOCAL_PWR_EN) {
>
>
> Coding style:
>
> while ( ... )
>
> {
>
Shall correct this.

>> +            if (timeout-- == 0)
>> +                break;
>
>
> Coding style:
>
> if ( ... )
>
>
>> +
>> +            mdelay(1);
>> +        }
>> +
>> +        if (timeout == 0) {
>
>
> Coding style:
>
> if ( ... )
>
> {
>
Sorry about so many coding style errors. Shall correct this.

>> +            dprintk(XENLOG_ERR, "CPU%d power enable failed", cpu);
>> +            return -ETIMEDOUT;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int exynos5_cpu_up(int cpu)
>> +{
>> +    static const struct dt_device_match exynos_dt_pmu_matches[]
>> __initconst =
>> +    {
>
>
> [..]
>
>> +        DT_MATCH_COMPATIBLE("samsung,exynos5422-pmu"),
>
>
> Where does this compatible come from? I don't find any reference in Linux
> upstream documentation
> (Documentation/devicetree/bindings/arm/samsung/pmu.txt).
>
This was my wrong assumption. I wanted to add odroid xu3's pmu in
this, but as you pointed out there is no exynos5422-pmu. Odroid XU3
uses the "exysno5420-pmu" compatibility strings. I shall remove this
line.

>
>> +        { /*sentinel*/ },
>> +    };
>> +    void __iomem *power;
>> +    u64 power_base_addr = 0;
>> +    u64 size;
>> +    struct dt_device_node *node;
>> +    int rc;
>> +
>> +    node = dt_find_matching_node(NULL, exynos_dt_pmu_matches);
>> +    if ( !node ) {
>
>
> Coding style:
>
>
> if ( !node )
> {
>
Shall correct it.

>> +       dprintk(XENLOG_ERR, "samsung,exynos5XXX-pmu missing in DT\n");
>> +        return -ENXIO;
>> +    }
>> +
>> +    rc = dt_device_get_address(node, 0, &power_base_addr, &size);
>> +    if ( rc ) {
>
>
> Coding style
>
> if ( rc )
> {
>
Shall correct this one too.

>> +           dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXXX-pmu\"\n");
>> +            return -ENXIO;
>> +    }
>> +
>> +    dprintk(XENLOG_INFO, "power_base_addr: %08x size: %08x\n",
>
>
> I would use XENLOG_DEBUG
>
>
Shall change it.

>> +            (unsigned int) power_base_addr, (unsigned int) size);
>> +
>> +    power = ioremap_nocache(power_base_addr +
>> +                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>> +    if ( !power )
>> +    {
>> +        dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
>> +        return -EFAULT;
>> +    }
>> +
>> +    rc = exynos_cpu_power_up(power, cpu);
>> +    if ( rc ) {
>> +        return -ETIMEDOUT;
>
>
> If exynos_cpu_power_up is failing you never unmap the power region.
>
I shall add a iounmap(power) on failure.

>> +PLATFORM_START(exynos5410, "SAMSUNG EXYNOS5410")
>
>
> Nothing looks 5410 specific in structure. I would rename it to exynos5,
> "SAMSUNG EXYNOS5". So if someone want to add a new exynos5 (such as the
> octa) it wouldn't have to rename this platform.
>
Shall rename it

> Regards,
>
> --
> Julien Grall

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