From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v1 3/3] xen/arm: Introduce support for Renesas R-Car Gen2 platform Date: Fri, 16 Jan 2015 14:08:22 +0000 Message-ID: <54B91B56.1060206@linaro.org> References: <1421412610-19313-1-git-send-email-oiurii.konovalenko@globallogic.com> <1421412610-19313-4-git-send-email-oiurii.konovalenko@globallogic.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1421412610-19313-4-git-send-email-oiurii.konovalenko@globallogic.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Iurii Konovalenko , xen-devel@lists.xen.org Cc: oleksandr.tyshchenko@globallogic.com, tim@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org Hi Iurii, On 16/01/15 12:50, Iurii Konovalenko wrote: > From: Iurii Konovalenko > > Signed-off-by: Iurii Konovalenko > --- > 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include The convention is to first include then . Futhermore, I think most of the includes are not necessary here. The following includes should be enough: #include #include #include #include > + > +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