Xen project Mailing List

Hi, Julien.

On Fri, Jan 16, 2015 at 4:08 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Iurii,
>
> On 16/01/15 12:50, Iurii Konovalenko wrote:
>> From: Iurii Konovalenko <iurii.konovalenko@xxxxxxxxxxxxxxx>
>>
>> Signed-off-by: Iurii Konovalenko <iurii.konovalenko@xxxxxxxxxxxxxxx>
>> ---
>> Â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.

No problem, will move all defines to shmobile.c.

>
> [..]
>
>> +#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>
>

Yes, sure. Will fix it.

>
>> +
>> +u32 shmobile_read_mode_pins(void)
>
> static

OK, will add it.

>
>> +{
>> + Â Âstatic uint32_t mode;
>
> Why the static here?

Sorry, migrated here as static from Linux kernel. Will fix it.

>
>> +
>> + Â Âvoid __iomem *modemr = ioremap_nocache(SHMOBILE_MODEMR, 4);
>
> Missing a blank here between the variable declaration and the code.

OK

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

I used xen/arch/arm/platforms/omap5.c file as a reference. In that file this function has no such attribute.
But I agree with you and will add this attribute.

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

OK, will fix it.

>> + Â Â * 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
> {
>

OK, will fix it.

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

OK, will use this API.

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

OK, will fix it.

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

Agree, I will use writel/readl API

> Â Â Â Â - Linux is update CNTFRQ
>
> Is there any reason that this code diverge?
>

Linux kernel 3.14-ltsi, which I used as a reference, use CNTFID0
in file arch/arm/mach-shmobile/setup-rcar-gen2.c:
iowrite32(freq, base + CNTFID0);

>> +
>> + Â Âiounmap(tmu);
>> +
>> + Â Âreturn 0;
>> +}
>> +
>> +static int __init shmobile_smp_init(void)
>> +{
>> + Â Âvoid __iomem *p;
>
> Missing blank line here.
>

OK, will fix it.

>> + Â Â/* 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.
>

OK, will use this API.

> Regards,
>
> --
> Julien Grall

Thanks a lot for valuable advices.

Best regards.

Iurii Konovalenko | Senior Software Engineer
GlobalLogic
P +3.8044.492.9695 M +38.099.932.2909 Â
S yufuntik

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