[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.