Hi Tomasz,

On Monday, June 30, 2014 Tomasz Figa wrote:

Hi Pankaj,

In general the patch seems quite nice, but please see few comments inline.

On 25.06.2014 16:03, Pankaj Dubey wrote:

quoted

Add support for mapping Samsung Power Management Unit (PMU) base
address from device tree.

Signed-off-by: Pankaj Dubey <redacted>
---
 arch/arm/mach-exynos/common.h |    1 +
 arch/arm/mach-exynos/exynos.c |   45

+++++++++++++++++++++++++++++++++++++++++

quoted

 2 files changed, 46 insertions(+)

[snip]

quoted

+static void exynos_map_pmu(void)
+{
+	struct device_node *np = NULL;

nit: Unnecessary variable initialization.

OK, will correct it.
 

quoted

+
+	np = of_find_matching_node(NULL, exynos_dt_pmu_match);
+

nit: Unnecessary blank line.

OK, will remove this.

quoted

+	if (!np) {
+		pr_err("Failed to find PMU node\n");
+		return;

Returning here probably doesn't make too much sense, especially when you

just panic

if the mapping fails and you remove the static mapping in patch 2/5, so

backwards

compatibility isn't provided anyway.

So something like this might be a better idea:

	np = of_find_matching_node(NULL, exynos_dt_pmu_match);
	if (np)
		pmu_base_addr = of_iomap(np, 0);

	if (!pmu_base_addr)
		panic("failed to find exynos pmu register\n");

Yes, this will be better. I will modify this part.

quoted

+	}
+
+	pmu_base_addr = of_iomap(np, 0);
+
+	if (!pmu_base_addr)
+		panic("failed to find exynos pmu register\n"); }
+
+static void __init exynos_init_time(void) {
+	/* Nothing to do timer specific.
+	 * Since platsmp.c needs pmu base address by the time
+	 * DT is not unflatten so we can't use DT APIs before
+	 * init_time
+	 */
+	exynos_map_pmu();

Would moving this to .init_irq() callback work too? There are more things

happening

in .init_time() so it seems more fragile and some platforms (e.g.

mach-tegra) do such

platform-specific initialization in
.init_irq() instead.

I need to check this, if it works I will update as suggested.

Best regards,
Tomasz

Thanks,
Pankaj Dubey