Re: [PATCH v1 3/3] xen/arm: Introduce support for Renesas R-Car Gen2 platform

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

Hi Iurii,

On 16/01/15 12:50, Iurii Konovalenko wrote:
> From: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> 
> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
> ---
>  xen/arch/arm/platforms/Makefile          |   1 +
>  xen/arch/arm/platforms/shmobile.c        | 149 +++++++++++++++++++++++++++++++
>  xen/include/asm-arm/platforms/shmobile.h |  24 +++++

We are trying to avoid introduce new platform header. If you don't need
it in other files, please move the defines in the platform code.

[..]

> +#include <asm/p2m.h>
> +#include <xen/config.h>
> +#include <xen/device_tree.h>
> +#include <xen/domain_page.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +#include <asm/platforms/shmobile.h>
> +#include <asm/platform.h>
> +#include <asm/io.h>

The convention is to first include <xen/*.h> then <asm/*.h>.

Futhermore, I think most of the includes are not necessary here.

The following includes should be enough:

#include <xen/mm.h>
#include <xen/vmap.h>
#include <asm/platform.h>
#include <asm/io.h>


> +
> +u32 shmobile_read_mode_pins(void)

static

> +{
> +    static uint32_t mode;

Why the static here?

> +
> +    void __iomem *modemr = ioremap_nocache(SHMOBILE_MODEMR, 4);

Missing a blank here between the variable declaration and the code.

> +    if( !modemr )
> +    {
> +        dprintk( XENLOG_ERR, "Unable to map shmobile Mode MR\n");
> +        return 0;
> +    }
> +
> +    mode = readl_relaxed(modemr);
> +    iounmap(modemr);
> +
> +    return mode;
> +}
> +
> +static int shmobile_init_time(void)

I know the other platform code doesn't do it. But this function is only
call during Xen boot. So it should be __init.

> +{
> +    uint32_t freq;
> +    void __iomem *tmu;
> +    int extal_mhz = 0;
> +    uint32_t mode = shmobile_read_mode_pins();
> +
> +    /* At Linux boot time the Renesas R-Car Gen2 arch timer comes

The coding style for multiline comment block is:

/*
 * My comment
 * line 2
 */

> +     * up with the counter disabled. Moreover, it may also report
> +     * a potentially incorrect fixed 13 MHz frequency. To be
> +     * correct these registers need to be updated to use the
> +     * frequency EXTAL / 2 which can be determined by the MD pins.
> +     */
> +
> +    switch ( mode & (MD(14) | MD(13)) ) {

The coding style require the bracket to be on a separate line.


switch
{

> +    case 0:
> +        extal_mhz = 15;
> +        break;
> +    case MD(13):
> +        extal_mhz = 20;
> +        break;
> +    case MD(14):
> +        extal_mhz = 26;
> +        break;
> +    case MD(13) | MD(14):
> +        extal_mhz = 30;
> +        break;
> +    }
> +
> +    /* The arch timer frequency equals EXTAL / 2 */
> +    freq = extal_mhz * (1000000 / 2);
> +
> +    /* Remap "armgcnt address map" space */
> +    tmu = ioremap_attr(SHMOBILE_ARCH_TIMER_BASE, PAGE_SIZE, 
> +            PAGE_HYPERVISOR_NOCACHE);

ioremap_nocache

> +    if ( !tmu )
> +    {
> +        dprintk(XENLOG_ERR, "Unable to map TMU\n");
> +        return -ENOMEM;
> +    }
> +    /*
> +     * Update the timer if it is either not running, or is not at the
> +     * right frequency. The timer is only configurable in secure mode
> +     * so this avoids an abort if the loader started the timer and
> +     * entered the kernel in non-secure mode.
> +     */
> +
> +    if ( (readl_relaxed(tmu + SHMOBILE_ARCH_TIMER_CNTCR) & 1) == 0 ||
> +            readl_relaxed(tmu + SHMOBILE_ARCH_TIMER_CNTFID0) != freq ) {

if
{

> +        /* Update registers with correct frequency */
> +        writel_relaxed(freq, tmu + SHMOBILE_ARCH_TIMER_CNTFID0);
> +
> +        /* make sure arch timer is started by setting bit 0 of CNTCR */
> +        writel_relaxed(1, tmu + SHMOBILE_ARCH_TIMER_CNTCR);
> +    }

AFAIU, this code is based on the Linux version, right? If so some part
of the code differs from upstream:
	- Linux is using iowrite32 (i.e writel on Xen)
        - Linux is update CNTFRQ

Is there any reason that this code diverge?

> +
> +    iounmap(tmu);
> +
> +    return 0;
> +}
> +
> +static int __init shmobile_smp_init(void)
> +{
> +    void __iomem *p;

Missing blank line here.

> +    /* setup reset vectors */
> +    p = ioremap_nocache(SHMOBILE_RAM, 0x1000);
> +    if( !p )
> +    {
> +        dprintk( XENLOG_ERR, "Unable to map shmobile ICRAM\n");
> +        return -ENOMEM;
> +    }
> +
> +    writel_relaxed(__pa(init_secondary), p + SHMOBILE_SMP_START_OFFSET);
> +    iounmap(p);
> +
> +    asm("sev;");

This sev can be reordered by the compiler. We have a macro sev() which
should avoid this issue.

Regards,

-- 
Julien Grall