Re: [PATCH 2/4] arm64: topology: Add support for topology DT bindings

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

 



On Tue, Jan 14, 2014 at 11:43:37AM +0000, Lorenzo Pieralisi wrote:
> On Sun, Jan 12, 2014 at 07:20:39PM +0000, Mark Brown wrote:

> > +static void __init parse_core(struct device_node *core, int cluster_id,
> > +			      int core_id)
> > +{
> > +	char name[10];
> > +	bool leaf = true;
> > +	int i, cpu;
> > +	struct device_node *t;
> > +
> > +	i = 0;

> You could initialize i at declaration, I can understand why you are doing that
> explictly in parse_cluster (two loops, to make code clearer), but here
> it does not make much sense to add a line for that.

I still find it clearer for do { } while loops to have the start
condition required for the loop to function right next to the loop.
Yes, you can save a line code but that's about it.

> > +	do {
> > +		snprintf(name, sizeof(name), "thread%d", i);

> If we wanted to be very picky, you need to copy "thread" just once (same
> goes for other strings), but we'd better leave code as is IMHO.

That would just make the code more complex, we need to handle tens of
cores so just doing i + '0' won't cut it.

> > +		t = of_get_child_by_name(core, name);

> Should we check the MT bit in MPIDR_EL1 before validating threads as well ?

> I do not like the idea because this means reliance on MPIDR_EL1 for MT
> and DT for topology bits, but it might be a worthwhile check.

> It is certainly odd to have a DT with threads and an MPIDR_EL1 with the MT
> bit clear.

Checking seems counter to the idea of forcing everyone to provide this
information from the firmware in the first place - checking that one bit
and ignoring the rest of the information even if it's good would seem
perverse.

Attachment: signature.asc
Description: Digital signature

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [CentOS ARM]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]     [Photos]