[PATCH v2 2/4] ARM: vexpress: Add DT support in v2m

On Mon, Nov 28, 2011 at 10:54:22AM +0000, Pawel Moll wrote:

On Fri, 2011-11-25 at 16:18 +0000, Dave Martin wrote:

quoted

[Since this text is now stable enough to be proofread, I'll list minor
pedantic nits along with the other comments -- they aren't vital to the
meaning though, and the documentation still "works" if they aren't
acted on.]

[snip]

Most appreciated! I'll "process" all your suggestions, thanks!

quoted

quoted

@@ -50,10 +55,34 @@ static void __iomem *v2m_sysreg_base;

 static void __init v2m_timer_init(void)
 {
-     void __iomem *sysctl_base;
-     void __iomem *timer01_base;
+     void __iomem *sysctl_base = NULL;
+     void __iomem *timer01_base = NULL;
+     unsigned int timer01_irq = NO_IRQ;
+
+     if (of_have_populated_dt()) {
+#if defined(CONFIG_ARCH_VEXPRESS_DT)
+             int err;
+             const char *path;
+             struct device_node *node;
+
+             node = of_find_compatible_node(NULL, NULL, "arm,sp810");
+             if (node)
+                     sysctl_base = of_iomap(node, 0);
+
+             err = of_property_read_string(of_aliases, "timer", &path);
+             if (!err)
+                     node = of_find_node_by_path(path);
+             if (node) {
+                     timer01_base = of_iomap(node, 0);
+                     timer01_irq = irq_of_parse_and_map(node, 0);
+             }
+#endif
+     } else {
+             sysctl_base = ioremap(V2M_SYSCTL, SZ_4K);
+             timer01_base = ioremap(V2M_TIMER01, SZ_4K);
+             timer01_irq = IRQ_V2M_TIMER0;
+     }

Do we even have of_have_populated_dt() in a non-DT kernel?

Maybe change this to

#if defined(CONFIG_ARCH_VEXPRESS_DT)
        if (of_have_populated_dt()) {
                /* ... */
        } else
#endif
        /* follow on from previous else */
        {
                /* ... */
        }

...or if that feels a little unclear, maybe do this:

#if defined(CONFIG_ARCH_VEXPRESS_DT)
        if (of_have_populated_dt()) {
                /* ... */
        } else {
#else
        {
#endif
                /* ... */
        }

of_have_populated_dt() is safe, see "include/linux/of.h":

You're right, but this code is still making assumptions about the
relationship between a build-time option (defined(
CONFIG_ARCH_VEXPRESS_DT)) and a run-time condition
(of_have_populated_dt()).  The relationship between the two is far from
transparent.

The effective condition we want is if(of_have_populated_dt()
&& defined(CONFIG_ARCH_VEXPRESS_DT)).  This is implemented in my
versions, but in your case the code can compile as

if(of_have_populated_dt()) { } else {
	sysctl_base = ioremap(V2M_SYSCTL, SZ_4K);
	...
}

I don't believe we should be writing code which can compile as
if(condition) { } else { do something useful; } unless there are
scenarios under which executing that empty block (and skipping the else)
would be the correct thing to do.  In this code, that's never the
right thing.  

I don't know how likely this is to be an issue, but we should avoid 
putting too many booby-traps in the code for future maintainers.

Since you have the #ifdef anyway, I don't see a strong argument against
moving it into the correct place.

#ifdef CONFIG_OF 
static inline bool of_have_populated_dt(void)
{       
        return allnodes != NULL;
}       
#else /* CONFIG_OF */
static inline bool of_have_populated_dt(void)
{       
        return false;
}       
#endif /* CONFIG_OF */

quoted

quoted

@@ -63,20 +92,20 @@ static void __init v2m_timer_init(void)
              writel(scctrl, sysctl_base + SCCTRL);
      }

-     timer01_base = ioremap(V2M_TIMER01, SZ_4K);
-     WARN_ON(!timer01_base);
-     if (timer01_base) {
+     WARN_ON(!timer01_base || timer01_irq != NO_IRQ);

Is that supposed to be !timer01_base || timer01_irq == NO_IRQ ?

Yes, I spotted and fixed this in the mean time.

quoted

If so, is might be better to write

WARN_ON(!(expr));
if (expr) {
        ...

so that the conditions are clear inverses.

Good point, will do.

quoted

quoted

@@ -470,3 +499,99 @@ MACHINE_START(VEXPRESS, "ARM-Versatile Express")
      .timer          = &v2m_timer,
      .init_machine   = v2m_init,
 MACHINE_END

It would be useful to have a comment somewhere indicating that the
DT_MACHINE_START() entries live in the corresponding ct-*.c files for
DT-enabled coretiles.

Not essential, though ... most people do know how to use grep.

Where exactly would you see that comment?

Next to the MACHINE_START() probably makes sense.  That was the point at
which I wondered where the DT equivalent(s) were.

Cheers
---Dave