Re: [PATCH V2 31/33] xen/arm: Add platform specific code for the exynos5

[Julien Grall <julien.grall@linaro.org>]

On 05/09/2013 11:19 AM, Ian Campbell wrote:

> On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> Changes in v2:
>>     - Add dom0 1:1 mapping quirk for the arndale board
>>     - s/mapping/mappings/ in comment
>>     - Remove debug trap (unnecessary with linux 3.9)
>> ---
>>  xen/arch/arm/platforms/Makefile         |    1 +
>>  xen/arch/arm/platforms/exynos5.c        |   86 +++++++++++++++++++++++++++++++
>>  xen/include/asm-arm/platforms/exynos5.h |   40 ++++++++++++++
>>  3 files changed, 127 insertions(+)
>>  create mode 100644 xen/arch/arm/platforms/exynos5.c
>>  create mode 100644 xen/include/asm-arm/platforms/exynos5.h
>>
>> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
>> index 4313e95..ff2b65b 100644
>> --- a/xen/arch/arm/platforms/Makefile
>> +++ b/xen/arch/arm/platforms/Makefile
>> @@ -1 +1,2 @@
>>  obj-y += vexpress.o
>> +obj-y += exynos5.o
>> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
>> new file mode 100644
>> index 0000000..8620390
>> --- /dev/null
>> +++ b/xen/arch/arm/platforms/exynos5.c
>> @@ -0,0 +1,86 @@
>> +/*
>> + * xen/arch/arm/platforms/exynos5.c
>> + *
>> + * Exynos5 specific settings
>> + *
>> + * Julien Grall <julien.grall@linaro.org>
>> + * Copyright (c) 2013 Linaro Limited.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <asm/p2m.h>
>> +#include <xen/config.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/domain_page.h>
>> +#include <xen/mm.h>
>> +#include <asm/platforms/exynos5.h>
>> +#include <asm/platform.h>
>> +
>> +static int exynos5_init_time(void)
>> +{
>> +    uint32_t reg;
>> +
>> +    // enable timer on exynos5 arndale board
>> +    // should probably be done by u-boot
> 
> /* */ please.
> 
>> +    reg = platform_read_register(EXYNOS5_MCT_G_TCON);
>> +    platform_write_register(EXYNOS5_MCT_G_TCON, reg | EXYNOS5_MCT_G_TCON_START);
>> +
>> +    return 0;
>> +}
>> +
>> +/* Additionnal mappings for dom0 (Not in the DTS) */
> 
> "Additional"
> 
>> +static int exynos5_specific_mapping(struct domain *d)
>> +{
>> +    /* Map the chip ID */
>> +    map_mmio_regions(d, EXYNOS5_PA_CHIPID, EXYNOS5_PA_CHIPID + PAGE_SIZE - 1,
>> +                     EXYNOS5_PA_CHIPID);
>> +
>> +    /* Map the PWM region */
>> +    map_mmio_regions(d, EXYNOS5_PA_TIMER,
>> +                     EXYNOS5_PA_TIMER + (PAGE_SIZE * 2) - 1,
>> +                     EXYNOS5_PA_TIMER);
> 
> If these get added to a future version of the DTS will badness ensue? I
> suppose all we can do is add a check for the new compatible node at that
> time, which we obviously can't do now...

In this case, we can remove this both lines.
map_devices_from_device_tree will do all the work.
But I think it's too specific to be added in the DTS.

>> diff --git a/xen/include/asm-arm/platforms/exynos5.h b/xen/include/asm-arm/platforms/exynos5.h
>> new file mode 100644
>> index 0000000..d77623c
>> --- /dev/null
>> +++ b/xen/include/asm-arm/platforms/exynos5.h
>> @@ -0,0 +1,40 @@
>> +#ifndef __ASM_ARM_PLATFORMS_EXYNOS5_H
>> +#define __ASM_ASM_PLATFORMS_EXYSNO5_H
>> +
>> +#define EXYNOS5_MCT_BASE            0x101c0000
>> +#define EXYNOS5_MCTREG(x)           (EXYNOS5_MCT_BASE + (x))
>> +#define EXYNOS5_MCT_G_TCON		    EXYNOS5_MCTREG(0x240)
>> +#define EXYNOS5_MCT_G_TCON_START	(1 << 8)
> 
> Inconsistent indent, hard tabs perhaps?

Right. Will be fix on the next patch series.

>> +
>> +#define EXYNOS5_PA_CHIPID           0x10000000
>> +#define EXYNOS5_PA_TIMER            0x12dd0000
>> +/* Base address of system controller */
>> +#define EXYNOS5_PA_PMU              0x10040000
>> +
>> +#define EXYNOS5_SWRESET             (EXYNOS5_PA_PMU + 0x0400)
>> +
>> +#define S5P_PA_SYSRAM   0x02020000
>> +
>> +/* Constants below is only used in assembly because the DTS is not yet parsed */
> 
> In a subsequent patch?

Yes. It's used in patch 32.

>> +#ifdef __ASSEMBLY__
>> +
>> +/* GIC Base Address */
>> +#define EXYNOS5_GIC_BASE_ADDRESS    0x10480000
>> +
>> +/* Timer's frequency */
>> +#define EXYNOS5_TIMER_FREQUENCY     (24 * 1000 * 1000) /* 24 MHz */
>> +
>> +/* Arndale machine ID */
>> +#define MACH_TYPE_SMDK5250          3774
>> +
>> +#endif /* __ASSEMBLY__ */
>> +
>> +#endif /* __ASM_ARM_PLATFORMS_EXYNOS5_H */
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> 
> 


-- 
Julien