Re: [PATCH 3/5] ARM: vexpress: Add DT support in v2m

[Pawel Moll <pawel.moll@arm.com>]

On Thu, 2011-11-17 at 16:05 +0000, Russell King - ARM Linux wrote:
> On Fri, Nov 11, 2011 at 06:27:04PM +0000, Pawel Moll wrote:
> > +#if defined(CONFIG_ARCH_VEXPRESS_DT)
> > +		int err;
> > +		const char *path;
> > +		struct device_node *node;
> > +
> > +		node = of_find_compatible_node(NULL, NULL, "arm,sp810");
> > +		BUG_ON(!node);
> > +		sysctl_base = of_iomap(node, 0);
> > +		BUG_ON(!sysctl_base);
> > +
> > +		err = of_property_read_string(of_aliases, "timer", &path);
> > +		BUG_ON(err);
> > +		node = of_find_node_by_path(path);
> > +		BUG_ON(!node);
> > +		timer01_base = of_iomap(node, 0);
> > +		BUG_ON(!timer01_base);
> > +		timer01_irq = irq_of_parse_and_map(node, 0);
> 
> Are you sure you have enough BUG_ON()s there?  

Yes, just enough, thank you.

> It's well worth reading
> Linus' various messages on the use of BUG(), which can be found:
> 
> 	http://yarchive.net/comp/linux/BUG.html

It was worth reading indeed. I will adapt to the policy.

> The lack of the timer and sysctl registers are really a 'report and maybe
> we could get a message out' kind of scenario rather than a 'oops, kill
> the machine'.
> 
> > +#endif
> > +	} else {
> >  		sysctl_base = ioremap(V2M_SYSCTL, SZ_4K);
> >  		BUG_ON(!sysctl_base);
> >  		timer01_base = ioremap(V2M_TIMER01, SZ_4K);
> >  		BUG_ON(!timer01_base);
> >  		timer01_irq = IRQ_V2M_TIMER0;
> > +	}
> 
> And in any case, both these paths have a common set of BUG_ON()s:
> 	BUG_ON(!sysctl_base);
> 	BUG_ON(!timer01_base);
> 
> So do we really need them having this independently?

No, you are right.

> > @@ -383,11 +412,18 @@ static struct clk_lookup v2m_lookups[] = {
> >  	},
> >  };
> >  
> > +static void __init v2m_system_id(void)
> > +{
> > +	if (!system_rev)
> > +		system_rev = readl(v2m_sysreg_base + V2M_SYS_ID);
> 
> You do understand that system_rev is for the system _revision_ not for
> some kind of system ID.  For example, it's to identify whether we're on
> a revision 4, 5 or 6 system.

SYS_ID encodes the motherboard revision (SYS_ID >> 28) and the FPGA
build (SYS_ID & 0xf) - those two together constitute the system
revision. As as 4 < 5 < 6, SYS_IDs of Rev A < Rev B < Rev C. I should
have masked out the non-relevant bits.

> However, with DT the differences in system revision should be encoded
> into the DT itself, and the kernel should not be making choices about
> the hardware off this.

This change had nothing to do DT or making choices - I was simply asked
to expose this information to the userspace for testing purposes and the
system_rev, available via /proc/cpuinfo seemed the best choice.

But as this change has nothing to do with DT it shouldn't be included in
this patch in the first place. I will split it out.

All comments appreciated as always, thanks!

Pawel