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

Re: [Xen-devel] [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware



Hi Suriyan,

On 12/09/14 16:01, Suriyan Ramasami wrote:
+static int __init exynos5_smp_init(void)
+{
+    void __iomem *sysram;
+    u64 sysram_addr;
+    u64 size;
+    u64 sysram_offset;
+    int rc;
+
+    rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size, 
&sysram_offset);

The function name is odd. As you call the function only here, couldn't you inline it?

+    if ( rc )
+        return rc;
+
+    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset: 
%016llx\n",
+            sysram_addr, size, sysram_offset);
+
+    sysram = ioremap_nocache(sysram_addr, size);
      if ( !sysram )
      {
          dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
@@ -125,7 +158,7 @@ static int __init exynos5_smp_init(void)

      printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
             __pa(init_secondary), init_secondary);
-    writel(__pa(init_secondary), sysram);
+    writel(__pa(init_secondary), sysram + sysram_offset);

      iounmap(sysram);

@@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
  static int exynos_cpu_power_state(void __iomem *power, int cpu)
  {
      return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
-                       S5P_CORE_LOCAL_PWR_EN;
+           S5P_CORE_LOCAL_PWR_EN;

Why this change?

  }

  static void exynos_cpu_power_up(void __iomem *power, int cpu)
@@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem *power, int 
cpu)
      return 0;
  }

-static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
-    u64 size;
+static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size) {

The Xen coding style is

static int foo(...)
{

      struct dt_device_node *node;
      int rc;
      static const struct dt_device_match exynos_dt_pmu_matches[] __initconst =
@@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
          return -ENXIO;
      }

-    rc = dt_device_get_address(node, 0, power_base_addr, &size);
+    rc = dt_device_get_address(node, 0, power_base_addr, size);
      if ( rc )
      {
          dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
@@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64 
*power_base_addr) {
      }

      dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
-            *power_base_addr, size);
+            *power_base_addr, *size);

      return 0;
  }

+static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
+{
+    asm(
+        "dsb;"
+        "smc    #0;"
+    );
+}
+

The compiler may decide to inline the function. This will end up to the command register not in register r0.

Give a look to __invoke_psci_fn_smc in xen/arch/arm/psci.c. It might be worth to introduce an SMC helper.

  static int exynos5_cpu_up(int cpu)
  {
      u64 power_base_addr;
+    u64 size;
      void __iomem *power;
      int rc;

-    rc = exynos5_get_pmu_base_addr(&power_base_addr);
+    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
      if ( rc )
          return rc;

-    power = ioremap_nocache(power_base_addr +
-                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
+    power = ioremap_nocache(power_base_addr, size);
      if ( !power )
      {
          dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
@@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu)

      iounmap(power);

+    exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
+

The call is not done unconditionally on Linux. It's only done when the secure firmware is present.

      return cpu_up_send_sgi(cpu);
  }

  static void exynos5_reset(void)
  {
      u64 power_base_addr;
+    u64 size;
      void __iomem *pmu;
      int rc;

-    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
-
-    rc = exynos5_get_pmu_base_addr(&power_base_addr);
+    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
      if ( rc )
          return;

-    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
+    pmu = ioremap_nocache(power_base_addr, size);
      if ( !pmu )
      {
          dprintk(XENLOG_ERR, "Unable to map PMU\n");
@@ -264,6 +305,7 @@ static const struct dt_device_match exynos5_blacklist_dev[] 
__initconst =
       * This is result to random freeze.
       */
      DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
+    DT_MATCH_COMPATIBLE("samsung,secure-firmware"),

Can you add a comment explaining why we blacklist the secure firmware?

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®.