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

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

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.

> +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.

> +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 )
{

> +	dprintk(XENLOG_ERR, "samsung,exynos4210-sysram-ns missing in DT\n");

The coding style doesn't allow to use hard tab.

> +        return -ENXIO;
> +    }
> +
> +    rc = dt_device_get_address(node, 0, &sysram_ns_base_addr, &size);
> +    if (rc) {

Coding style:

if ( rc )
{

> +        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?

> +
> +    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_...

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.

> +}
> +
> +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.

> +}
> +
> +static int exynos_cpu_power_up(void __iomem *power, int cpu)
> +{
> +    int timeout;

unsigned int.

> +
> +    if ( !exynos_cpu_power_state(power, cpu) ) {

Coding style:

if ( ... )
{

> +        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.

> +        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 ( ... )
{

> +            if (timeout-- == 0)
> +                break;

Coding style:

if ( ... )

> +
> +            mdelay(1);
> +        }
> +
> +        if (timeout == 0) {

Coding style:

if ( ... )
{

> +            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).

> +        { /*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 )
{

> +	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 )
{

> +	    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

> +            (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.

> +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.

Regards,

-- 
Julien Grall