Re: [PATCH] Add support for the ESTeem 195E (PPC405EP) SBC

[Josh Boyer <jwboyer@linux.vnet.ibm.com>]

On Thu, Jul 30, 2009 at 03:45:06PM -0400, Solomon Peachy wrote:
>On Thu, Jul 30, 2009 at 10:06:30AM -0400, Josh Boyer wrote:
>> >+		devp = finddevice("/plb/opb/serial@ef600300");
>> >+		if (!devp)
>> >+			fatal("Can't find node for /plb/opb/serial@ef600300");
>> >+		del_node(devp);
>> 
>> Slightly confused here.  You delete the first serial node in the single eth
>> case?
>
>The board is actually single eth/serial or dual eth/serial. And strictly 
>speaking, this serial port is the second one in the devicetree...

Maybe a brief comment in the code explaining that would help.  Also, I did
notice the DTS had them in the order you mention, and I forgot to come back
and correct my question there.

>> >+
>> >+		devp = finddevice("/plb/opb/ethernet@ef600900");
>> >+		if (!devp)
>> >+			fatal("Can't find node for /plb/opb/ethernet@ef600900");
>> >+		del_node(devp);
>> >+	}
>> >+
>> >+	ibm4xx_quiesce_eth((u32 *)0xef600800, (u32 *)0xef600900);
>> 
>> Shouldn't you do the quiesce conditionally if the other eth port doesn't
>> exist?
>
>I don't know if this is strictly necessary with the modern ibm_emac 
>driver, but it's certainly safe to call because all 405EPs have dual 
>ethernet controllers.
>
>From the (pre-dts) driver perspecive, the only way to tell if the 
>hotfoot had one ethernet port or two was that the second PHY failed to 
>initialize.
>
>Additionally, the production bootloader (u-boot 1.2.0.x) isn't terribly 
>smart and tries to drive the second controller if the first one doesn't 
>have a cable plugged in, so it's possible the second controller is 
>running when control is handed over to Linux, even on single ethernet 
>boards.

OK, that makes sense.

>
>> >+			UART0: serial@ef600400 {
>> >+				device_type = "serial";
>> >+				compatible = "ns16550";
>> >+				reg = <0xef600400 0x00000008>;
>> >+				virtual-reg = <0xef600400>;
>> >+				clock-frequency = <0>; /* Filled in by zImage */
>> >+				current-speed = <0x9600>;
>> 
>> Just a question, but is the baud supposed to be 38400 or 9600?  At first glance
>> it almost seems like a typo :).
>
>It's supposed to be 38400 baud, hence the explicit 0x in front.  (I lost 
>count of the number of times I saw '38400' listed in various dts 
>files...)

Cool.  Just checking.

>> >+			gpio-leds {
>> >+			     compatible = "gpio-leds";
>> >+			     status {
>> >+			     	    label = "Status";
>> >+			     	    gpios = <&GPIO 1 0>;
>> >+				    /* linux,default=trigger = ".."; */
>> >+			     };
>>
>> What does that comment mean?
>
>Whoops, it's supposed to read 'linux,default-trigger', but the LEDs are 
>manually twiddled for the time being.  I'd forgotten to strip that out.  
>
>(see linux/Documentation/powerpc/dts-bindings/gpio/led.txt)

OK.  If it's not needed just yank it.

>
>> Ok.  So I'm not really all that thrilled with changes to ppcboot.h.  
>> We try to keep this file as much in-sync with U-Boot as we can.  Did 
>> your HOTFOOT changes get pulled into upstream U-Boot?
>
>Yeah, I thought this may be a problem, but I didn't know a better way to 
>go about this and still maintain compatibility with the many thousands 
>of boards already in the field.  I mean, I could strip out the ppcboot.h 
>changes and maintain that as an out-of-tree patch, but without that 
>patch, the kernel won't boot on in-the-field boards, rendering the 
>upstreaming of support for this board kinda pointless.
>
>I haven't tried to push anything to upstream u-boot, given how ancient 
>the in-production bootloader is.  The guy who originally mangled u-boot 
>for this board did so before the "standard" 405EP dual ethernet layout 
>was added, and never tried to push it upstream.  Any upstream uboot work 
>will take the form of a native dts/fdt port that probably won't use 
>ppcboot.h anyway, which brings us full circle...

There is another way.  Perhaps you could just copy ppcboot.h to a new file
called "hotfoot.h" and just use that.  It's a duplication of ppcboot.h to
some degree, but it seems to make sense for your board and it helps preserve
the "stock" ppcboot.h for other boards.

josh