Hi, Julien.
On Fri, Jan 16, 2015 at 4:08 PM, Julien Grall <julien.gr...@linaro.org>
wrote:
> Hi Iurii,
>
> On 16/01/15 12:50, Iurii Konovalenko wrote:
>> From: Iurii Konovalenko <iurii.konovale...@globallogic.com>
>>
>> Signed-off-by: Iurii Konovalenko <iurii.konovale...@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.
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
