[PATCH] PowerMac11,2 i2c-bus@0 duplicate dev-tree workaround

[PATCH] PowerMac11,2 i2c-bus@0 duplicate dev-tree workaround

[Johannes Berg]

On a PowerMac11,2, there are two i2c-bus@0 nodes of which only the first
is correct. This patch makes the device tree unflattening code ignore
the second one on those machines.

Signed-Off-By: Johannes Berg <johannes@sipsolutions.net>

--- 
I'm not sure this is the right way to do it. Maybe we should have some
'dev-tree quirks fixer' that makes a third pass through the device tree
after the allnodes chain has been set up, and fixes it up. On the other
hand, as long as there aren't too many workarounds, this works fine.

The patch looks longer than it is because some code changed indentation
level.

--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -624,6 +624,11 @@ static void *__init unflatten_dt_alloc(u
 	return res;
 }
 
+/* PowerMac11,2 has a bug in the device tree where
+ * an i2c-bus@0 shows up twice. We ignore the second
+ * one since it is the bogus one */
+static __initdata int powermac_i2c_bus_0_workaround = 0;
+
 static unsigned long __init unflatten_dt_node(unsigned long mem,
 					      unsigned long *p,
 					      struct device_node *dad,
@@ -695,17 +700,31 @@ static unsigned long __init unflatten_dt
 			memcpy(p, pathp, l);
 		} else
 			memcpy(np->full_name, pathp, l);
+		DBG("node %s found\n", np->full_name);
 		prev_pp = &np->properties;
-		**allnextpp = np;
-		*allnextpp = &np->allnext;
-		if (dad != NULL) {
-			np->parent = dad;
-			/* we temporarily use the next field as `last_child'*/
-			if (dad->next == 0)
-				dad->child = np;
-			else
-				dad->next->sibling = np;
-			dad->next = np;
+		/* so if the workaround is in effect, and we have the right
+		 * node found, we increase the workaround count (we use the
+		 * same variable) and then set allnextpp to NULL on the second
+		 * one around so the node isn't added to the allnodes list */
+		if (powermac_i2c_bus_0_workaround &&
+		    strcmp(np->full_name,
+		           "/ht@0,f2000000/pci@8/mac-io@7/i2c@18000/i2c-bus@0") == 0) {
+			powermac_i2c_bus_0_workaround++;
+			if (powermac_i2c_bus_0_workaround == 3)
+				allnextpp = NULL;
+		}
+		if (allnextpp) {
+			**allnextpp = np;
+			*allnextpp = &np->allnext;
+			if (dad != NULL) {
+				np->parent = dad;
+				/* we temporarily use the next field as `last_child'*/
+				if (dad->next == 0)
+					dad->child = np;
+				else
+					dad->next->sibling = np;
+				dad->next = np;
+			}
 		}
 		kref_init(&np->kref);
 	}
@@ -745,6 +764,14 @@ static unsigned long __init unflatten_dt
 			}
 			if (strcmp(pname, "ibm,phandle") == 0)
 				np->linux_phandle = *((u32 *)*p);
+			/* we check the root node's compatible property
+			 * to see if we need to start the powermac i2c
+			 * workaround */
+			if ((pathp[0] == '\0') &&
+			    (strcmp(pname, "compatible") == 0) &&
+			    (strncmp((char*)*p, "PowerMac11,2", sz) == 0)) {
+				powermac_i2c_bus_0_workaround = 1;
+			}
 			pp->name = pname;
 			pp->length = sz;
 			pp->value = (void *)*p;

Re: [PATCH] PowerMac11,2 i2c-bus@0 duplicate dev-tree workaround

[Benjamin Herrenschmidt]

> +		/* so if the workaround is in effect, and we have the right
> +		 * node found, we increase the workaround count (we use the
> +		 * same variable) and then set allnextpp to NULL on the second
> +		 * one around so the node isn't added to the allnodes list */
> +		if (powermac_i2c_bus_0_workaround &&
> +		    strcmp(np->full_name,
> +		           "/ht@0,f2000000/pci@8/mac-io@7/i2c@18000/i2c-bus@0") == 0) {
> +			powermac_i2c_bus_0_workaround++;
> +			if (powermac_i2c_bus_0_workaround == 3)
> +				allnextpp = NULL;
> +		}
> +		if (allnextpp) {
> +			**allnextpp = np;
> +			*allnextpp = &np->allnext;
> +			if (dad != NULL) {
> +				np->parent = dad;
> +				/* we temporarily use the next field as `last_child'*/
> +				if (dad->next == 0)
> +					dad->child = np;
> +				else
> +					dad->next->sibling = np;
> +				dad->next = np;
> +			}
>  		}
>  		kref_init(&np->kref);
>  	}

Hrm... you set allnextpp to NULL ... won't that prevent any further node
from being enqueued in the global node list ?

In fact, I think the workaround should be in prom_init.c when
flattening... easier to skip a node there, and that's also where you are
reasonably sure of getting the nodes in the right order, not when
unflattening. 

Also, you don't even need to test for PowerMac11,2 .. .prom_init.c
already has a machine type, so just test that it's a mac and has this
node duplicated, and if yes, remove the dup. In fact, you could probably
even run a bit of forth with "interpret" to do so before the tree is
even walked though :)

Ben.

Re: [PATCH] PowerMac11,2 i2c-bus@0 duplicate dev-tree workaround

[Michael Ellerman]

On Wed, 2006-03-29 at 13:30 +0200, Johannes Berg wrote:
> On a PowerMac11,2, there are two i2c-bus@0 nodes of which only the first
> is correct. This patch makes the device tree unflattening code ignore
> the second one on those machines.
> 
> Signed-Off-By: Johannes Berg <johannes@sipsolutions.net>
> 
> --- 
> I'm not sure this is the right way to do it. Maybe we should have some
> 'dev-tree quirks fixer' that makes a third pass through the device tree
> after the allnodes chain has been set up, and fixes it up. On the other
> hand, as long as there aren't too many workarounds, this works fine.
> 
> The patch looks longer than it is because some code changed indentation
> level.

I don't understand why we need this patch? If we care about getting the
"right" node why not check for that where we care?

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

Re: [PATCH] PowerMac11,2 i2c-bus@0 duplicate dev-tree workaround

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

On Thu, 2006-03-30 at 15:26 +1100, Benjamin Herrenschmidt wrote:

> Hrm... you set allnextpp to NULL ... won't that prevent any further node
> from being enqueued in the global node list ?

No, it only prevents all *child* nodes, since the function recurses so
once you go out allnextpp is valid again. I'm not setting *allnextpp :)

> In fact, I think the workaround should be in prom_init.c when
> flattening... easier to skip a node there, and that's also where you are
> reasonably sure of getting the nodes in the right order, not when
> unflattening. 

Ok. I'll look at that code then.

> Also, you don't even need to test for PowerMac11,2 .. .prom_init.c
> already has a machine type, so just test that it's a mac and has this
> node duplicated, and if yes, remove the dup. In fact, you could probably
> even run a bit of forth with "interpret" to do so before the tree is
> even walked though :)

Heh. Well, if you tell me what you prefer I'll try to do it. Meanwhile
I'll use this patch so the sound stuff etc. doesn't get too confusing
(it keeps telling me about two onyx codecs found...)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [PATCH] PowerMac11,2 i2c-bus@0 duplicate dev-tree workaround

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

On Thu, 2006-03-30 at 17:28 +1100, Michael Ellerman wrote:

> I don't understand why we need this patch? If we care about getting the
> "right" node why not check for that where we care?

It's not just a single node but also contains sub-nodes. In one of them,
one of those sub nodes is there but contains bogus info, and in the
other both are there and contain the right info. Some things that work
off these bus nodes are detected twice if they're there, and they're
used in at least two places that I know of.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [PATCH] PowerMac11,2 i2c-bus@0 duplicate dev-tree workaround

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

On Fri, 2006-03-31 at 00:38 +0200, Johannes Berg wrote:
> On Thu, 2006-03-30 at 17:28 +1100, Michael Ellerman wrote:
> 
> > I don't understand why we need this patch? If we care about getting the
> > "right" node why not check for that where we care?
> 
> It's not just a single node but also contains sub-nodes. In one of them,
> one of those sub nodes is there but contains bogus info, and in the
> other both are there and contain the right info. Some things that work
> off these bus nodes are detected twice if they're there, and they're
> used in at least two places that I know of.

OK, that sounds a little complicated. I'm just worried about having lots
workarounds in the unflattening code, the code's hard enough to read as
it is. I think it's preferable to do the workarounds in the code that
needs the workaround, that way they're isolated in certain parts of the
code rather than all in the unflattening.

cheers

-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
-- 
Michael Ellerman
IBM OzLabs

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]

Re: [PATCH] PowerMac11,2 i2c-bus@0 duplicate dev-tree workaround

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

On Fri, 2006-03-31 at 16:37 +1100, Michael Ellerman wrote:

> OK, that sounds a little complicated. I'm just worried about having lots
> workarounds in the unflattening code, the code's hard enough to read as
> it is. 

Yeah that's a good argument.

> I think it's preferable to do the workarounds in the code that
> needs the workaround, that way they're isolated in certain parts of the
> code rather than all in the unflattening.

I'd really prefer to have these nodes vanish from the complete node
chain so we don't have the workarounds cluttering up the rather high
levels with machine dependent code. I'll find a better place to put it.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]