Re: [HACK] add sandpoint + flattened dt support to arch/powerpc/boot | linuxppc-dev

quoted

Mark, Ben,

On Mon, 22 May 2006, Mark A. Greer wrote:

Thanks for your time, Ben.  Sorry for taking so long to get back to you.
don't have much to comment to specific points, but quoting the entire 
discussion below in case you'll want to further add to it.

What's the status of this work? Mark, have you been further working on 
your port? I'd like to integrate kurobox/linkstation support with 
sandpoint (also 8241). I think, it would be very interesting as it would 
allow us to develop / test the "universal" kernel idea, and at the same 
time explore the limits of board-specific support. Kurobox have a couple 
of very nice "specialities", like a serially attached AVR acting as a 
power-manager, thus requiring __special__ halt and reboot callbacks (so, 
please, Ben, don't remove them).

Any progress with fdt-parsing library for the wrapper / kernel and with 
replacing hard-coded constants and tables with dt-entries?

I'd love to discuss the stuff and come to an acceptable design / APIs.

Thanks
Guennadi

For the record, this patch was only a hack that I've done to get a sandpoint
up ASAP.  I should have made it clear to not take Kconfig, etc. stuffs very
seriously.  I was more worried about how we should fit flattened dt into the
bootwrapper.

I'm glad you did look at it so closely though and bring up a lot of good
questions.  I'm not familiar with 64-bit platforms so all of my babbling
below is based on my 32-bit platform knowledge.

Mark
---

On Thu, May 18, 2006 at 03:32:55PM +1000, Benjamin Herrenschmidt wrote:
On Wed, 2006-05-17 at 17:21 -0700, Mark A. Greer wrote:
<snip>

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6729c98..de09eac 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -323,7 +323,10 @@ config PPC_ISERIES
 
 config EMBEDDED6xx
 	bool "Embedded 6xx/7xx/7xxx-based board"
-	depends on PPC32 && BROKEN
+	depends on PPC32
+	select PPC_UDBG_16550
+	select MPIC
+	select MPIC_SERIAL
Not totally related to your patch but I'm tempted to turn that into a
"Generic 6xx/7xx/7xxx" rather than "embedded" if we manage to always
avoid board specific code most of the time, but instead add necessary
bits in the device-tree. We still need a per-board Kconfig option I
think that will just select the necessary bits and pieces (and more than
one can be selected at one time). Also, I think right now, the embedded
stuff is +/- exclusive from the MULTIPLATFORM stuff, that must be fixed
asap. We are all living in the same kernel now :)
OK.

+
+config MPIC_SERIAL
+	depends on EMBEDDED6xx
+	bool
+	default n
Not sure what the above is, I'll have a look further in the patch but I
can already tell you that it should be your per-board
CONFIG_PPC_SANDPOINT for example that "selects" the various bits you
need, and thus the above should depend on it, not EMBEDDED6xx... In fact
I'm wondering wether we should kill EMBEDDED6xx :) Unless it's purely a
reference to arch/powerpc/platforms/embedded6xx/ as a storage place for
board setup_xxx.c files which is useful for platforms that really don't
need more than one or 2 files in there.
I have no attachment to "embedded" so I'm happy to see it
go...especially if I correctly understand what you're saying below.

+ifeq ($(CONFIG_EMBEDDED6xx),y)
+src-boot += prom_embedded.c ns16550.c dts/sandpoint.S \
+	dt_utils.c sandpoint.c mpc10x.c
+else
+src-boot += prom.c
+endif
Again, I think CONFIG_EMBEDDED6xx is the wrong approach.  I'd like to
avoid the word "embedded" altogether anyway :) Thing is, it would be
good if ultimately, embedded boards firmware was capable of passing a
device-tree. Thus, embedded a device-tree in the zImage wrapper is more
like a "fallback" solution which has the side effect of creating machine
specific zImage's.

That is why, I think, what we should do is have rules for building
separate zImage wrappers. The basic OF one (which exists in both real
and virtual mode versions and that I'd like to make capable of also
working if a flat device-tree is passed in), the zImage.sandpoint, the
zImage.prep, and whatever...

Thus enabling CONFIG_PPC_MYBOARD should trigger the build of the
board-flavored version of the wrapper in addition to all the other ones
for the other boards built as part of a given config.

I know it's a bit difficult because the current boot Kbuild rules aren't
really design for multiple targets but I think that's the way to go.
Okay, to make sure I understand, you're saying that you want to support a
.config file having several CONFIG_PPC_BOARD1, CONFIG_PPC_BOARD2,
CONFIG_PPC_BOARD3 defined at the same time.  The resulting zImage would
run on all 3 boards, right?

I'd be happy with that.  It should meet the requirements of both
the multiplatform and embedded folks.  You could combine that with the
tool paulus talked about a couple weeks ago which would tack the
appropriate flattened device tree (fdt) onto the zImage after the fact.

http://ozlabs.org/pipermail/linuxppc-dev/2006-April/022435.html

We just need to come up with the tool and make the bootwrapper code
smart enough to know what dt to use (an OF dt, an fdt from a firmware, a
fdt tacked on by the tool (along with code to take bd stuff & put it
into the fdt?)).

All of the boards would likely have to be of the same "class".
Where "class" would be:
	1) OF system
	2) e300 core + PQIIPro (e.g., 83xx)
	3) e500 core + PQIII (e.g., 85xx)
	4) 6xx/7xx/74xx + mpc10x (e.g., sandpoint)
etc.  Something like that, anyway--maybe there are only "OF" and
"non-OF'?

Basically, if its not OF, then its classified by its core + bridge.

So, first thing the zImage does is look for an attached ftb (attached by
paulus' tool).  If its there, use it.  If its not there, use either the
OFDT (for OF class) or expect a dt to be passed in from the firmware (for
all the other classes).  That way you can easily override the dt in the field.

 .../...

About the .dts file, we should look into "shipping" a pre-built one so
that dtc is not required for building a kernel.
Okay, paulus' tool would allow that.  We should also make it smart
enough to replace an attached dts (really dtb--'b' for 'binary') with
a new one.

I think we already have
a mecanism for providing "shipped" versions of files and having the
option of rebuilding them from source (though I don't have the details
in mind at the moment) 
I don't know what you're referring to here.

diff --git a/arch/powerpc/boot/io.h b/arch/powerpc/boot/io.h
 .../.. io stuffs ...

Well... io accessors in here can't harm, the question is more how they
are used ;)

+#define	PCI_DEVFN(slot,func)	((((slot) & 0x1f) << 3) | ((func) & 0x07))
+
+/* Indirect PCI config space access routines */
+static inline void
+pci_indirect_read_config_byte(u32 *cfg_addr, u32 *cfg_data, int devfn,
+		int offset, u8 *val)
+{
+	out_be32(cfg_addr,
+		((offset & 0xfc) << 24) | (devfn << 16) | (0 << 8) | 0x80);
+	*val = in_8((u8 *)(cfg_data + (offset & 3)));
+	return;
+}
+
+static inline void
+pci_indirect_read_config_dword(u32 *cfg_addr, u32 *cfg_data, int devfn,
+		int offset, u32 *val)
+{
+	out_be32(cfg_addr,
+		((offset & 0xfc) << 24) | (devfn << 16) | (0 << 8) | 0x80);
+	*val = in_le32(cfg_data + (offset & 3));
+	return;
+}
That's more annoying... if we start having PCI config space access in
the boot wrapper how far will it stop ? I'd rather keep that sort of
stuff local to myboard.c file unless it's really worth sharing... Unless
we end up with really common need for them in which case those should be
in separate files, possibly in a separate dir (thinking about it, prep
will probably need that too)
Sure.  I was just grabbing code I needed and sticking it whereever to
get things to work.  If mpc10x.c is the only one that uses those routines
then they should be in that file only.

The main problem is how do we do printf kind of things from such a
bootloader. That needs IO, thus uart drivers etc... possibly parsing
from the device-tree etc... I'm tempted to just have a global
"boot_puts" function pointer set by the board and/or OF code to use
through the bootloader, maybe a shared uart.c file with various uart
bits and keep the gory implementation details of mapping the uart and
using those uart bits in the myboard.c file...
Okay.

Question: are we still going to allow cmdline editing in the
bootwrapper?  If so, we also need to get uart input and munge the dtb
to put the new args back into the /chosen/bootargs field.

+#ifdef CONFIG_PPC_OF
 typedef void (*kernel_entry_t)( unsigned long,
                                 unsigned long,
                                 void *,
 				void *);
+#else
+void platform_fixups(void *dt_blob);
+extern void *dt_blob_start;
+extern void *dt_blob_end;
+void edit_cmdline(void *dt_blob_start);
+typedef void (*kernel_entry_t)(void *dtb_addr, void *kernel_entry_paddr,
+		void *must_be_null);
+#endif
Why 2 different prototypes for kernel_entry ?
One for "OF", one for "non-OF" but I'll change it to
"typedef void (*kernel_entry_t)(void *addr, ...);"
to support both.  Hrm, I may have another idea...  Anyway, I'll do
something about the #ifdef.

I think we need something like CONFIG_PPC_BOOT_HAS_BUILTIN_DT that
enables various DT related stuff in a clean way and have a builtin_dt.h
file with related prototypes & definitions.
If we have the tool mentioned above, we shouldn't need a CONFIG option.
Just check for a NULL ptr or something like that which indicates there's
no dtb attached.

 #undef DEBUG
@@ -218,7 +227,7 @@ void start(unsigned long a1, unsigned lo
 	if (getprop(chosen_handle, "stdout", &stdout, sizeof(stdout)) != 4)
 		exit();
 
-	printf("\n\rzImage starting: loaded at 0x%p (sp: 0x%p)\n\r", _start, sp);
+	printf("\n\rzImage starting: loaded at 0x%p (sp: 0x%p)\n\r", _start,sp);
Gratuituous uglyfication :)
Again, I was just hacking stuff...

<snip>

 
 	/* Eventually gunzip the kernel */
@@ -299,6 +311,7 @@ #endif
 	flush_cache((void *)vmlinux.addr, vmlinux.size);
 
 	kernel_entry = (kernel_entry_t)vmlinux.addr;
+#ifdef CONFIG_PPC_OF
 #ifdef DEBUG
 	printf( "kernel:\n\r"
 		"        entry addr = 0x%lx\n\r"
@@ -311,9 +324,20 @@ #ifdef DEBUG
 #endif

 	kernel_entry(a1, a2, prom, NULL);
+#else /* !CONFIG_PPC_OF ==> flattened device tree */
+#ifdef DEBUG
+	printf("kernel:\n\r"
+		"        entry addr   = 0x%lx\n\r"
+		"        flattened dt = 0x%lx\n\r",
+		(unsigned long)kernel_entry, &dt_blob_start);
+#endif
+	edit_cmdline(&dt_blob_start);
+	platform_fixups(&dt_blob_start);
 
+	kernel_entry(&dt_blob_start, (void *)vmlinux.addr, NULL);
+#endif
Let's avoid #ifdef's if we can... provide function pointers where it
matter setup by the board/OF init code maybe but not ifdef's. Afaik,
only WD actually _likes_ ifdef's :)
I'll fix it up.

+++ b/arch/powerpc/boot/mpc10x.c
We'll need some subdirs here or it will get messy quick...
Yep.

diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
Same comment as above

+/* XXXX Get info from dt instead */
+static struct {
+	int baud_base;
+	unsigned long com_port;
+	u16 iomem_reg_shift;
+} rs_table[RS_TABLE_SIZE] = {
+        { BASE_BAUD, SANDPOINT_SERIAL_0, 0 },
+        { BASE_BAUD, SANDPOINT_SERIAL_1, 0 },
+};
Well, your own comment says it all :) I think we might need an
equivalent of prom_parse.c in the bootloader
Yes, this was just a hack to get the sandpoint going.

+
+/* #ifndef CONFIG_PPC_OF XXXX */
+
+/* Definitions used by the flattened device tree */
+#define OF_DT_HEADER		0xd00dfeed	/* marker */
+#define OF_DT_BEGIN_NODE	0x1		/* Start of node, full name */
+#define OF_DT_END_NODE		0x2		/* End node */
+#define OF_DT_PROP		0x3		/* Property: name off, size,
+						 * content */
+#define OF_DT_NOP		0x4		/* nop */
+#define OF_DT_END		0x9
+
+#define OF_DT_VERSION		0x10
   .../... (flat DT definitions)

Can't we share the kernel definition here ? Maybe split the kenrel one
in two if necessary ? I don't like that sort of duplication...
I hope so.  I'm hoping we can make a special dir somewhere that has the
definitions of stuff shared between the bootwrapper & the kernel.

How about arch/powerpc/shared?  I dunno, something sensible.

+unsigned long serial_init(int chan, void *ignored);
+void serial_putc(unsigned long com_port, unsigned char c);
+unsigned char serial_getc(unsigned long com_port);
+int serial_tstc(unsigned long com_port);
Forgot to mention that earlier, but those should be uart_* imho (or even
16550_*) since we'll surely have different species of serial ports to
deal with and I don't like name mixup. Unless we consider that only ever
one of those serial files get built in a given zImage wrapper in which
case it makes some sense...
Okay.

+call_prom(const char *service, int nargs, int nret, ...)
+{
+
+	static struct {
+		char	*service;
+		int	((*rtn)(int nargs, int nret, va_list args));
+	} services[] = {
+		{ "exit", service_exit },
+		{ "finddevice", service_finddevice },
+		{ "getprop", service_getprop },
+		/* { "write", service_write }, */
+	};
+	va_list args;
+	int i, rc = 0;
+
+	for (i=0; i<ARRAY_SIZE(services); i++)
+		if (!strcmp(service, services[i].service)) {
+			va_start(args, nret);
+			rc = services[i].rtn(nargs, nret, args);
+			va_end(args);
+		}
+
+	return (nret > 0)? rc : 0;
+}
I don't think call_prom is the right abstraction :) We have to decide
here, we have two choices:

 - Either a given zImage wrapper can be booted from both a real OF and
have an embedded flat DT, in which case we need function pointers and I
think the right abstraction is for individual prom functions to have
their function pointer rathaer than "multiplex" through call_prom

 - Or a given zImage wrapper is a single board thingy. That is it is
either the OF wrapper _or_ it can contain an embedded DT. Probably
easier that way and no need for funciton pointers nor mux at all... I
tend to think that might be the right way to go at this point...
Especially since as I wrote before, I'm tempted to think we should just
in that case build multiple zImage wrappers. The only thing there is
that if we go that route, I'd like the "OF" one to also be able to boot
with a flat DT passed on entry to it so it will be useable as a generic
zImage for firmwares that have a flat DT to pass to the kernel. The only
"issue" with that is it might be difficult to have console output unless
we do the putc function pointer thing I described earlier and have
several "uart" impleentations with differnet names and enough
device-tree parsing to be able to instanciate them (almost a copy of
kernel's legacy serial stuff). Another option if we want console output
is to add something to the DT header specifically for use by such early
boot code that specifies a function that can be called back for
displaying thing as long as a given memory range isn't overriden (the FW
itself)

Comments are welcome here, this is I think the most tricky point and
where we need to make a decision...
I'm very happy to not use the 'call_prom' abstraction. :)

I guess I have the first one in mind.  You have a zImage that runs on
1 to n different platforms.  The zImage has to determine which DT to use
(OF DT, fdt from firmware, fdt attached by tool) and pick it apart to
find the I/O resources and serial driver to use.  The CONFIG_<BOARD>'s
that are defined would determine the drivers that get built in to the zImage.

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index 6e67b5b..2feca55 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -147,9 +147,11 @@ static int __init add_legacy_isa_port(st
 	if (reg == NULL)
 		return -1;
 
+#if 0 /* XXXX */
 	/* Verify it's an IO port, we don't support anything else */
 	if (!(reg[0] & 0x00000001))
 		return -1;
+#endif
Gack ? Care to explain ?
This 'if' always executed and threw me into the weeds.  I didn't get
back to really figuring it out.  This was a hack to get going.

+obj-$(CONFIG_EMBEDDED6xx)	+= embedded6xx/
Hrmph.... (random noises)
?  No, I needed that to get the build to go into platforms/embedded6xx.
Since platforms/embedded6xx was already there, I assumed that was where
I was to put things like the sandpoint.  If/when we get rid of
"embedded", I'll move it to whereever it should go then.

+/*
+ * Define all of the IRQ senses and polarities.  Taken from the
+ * Sandpoint X3 User's manual.
+ */
+static u_char sandpoint_openpic_initsenses[] __initdata = {
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 0: SIOINT */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 1: XXXX FILL?? */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 2: PCI Slot 1 */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 3: PCI Slot 2 */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 4: PCI Slot 3 */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 5: PCI Slot 4 */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 6: XXXX FILL?? */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 7: XXXX FILL?? */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 8: IDE (INT C) */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 9: IDE (INT D) */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 10: */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 11: */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 12: */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 13: */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 14: */
+	(IRQ_SENSE_LEVEL | IRQ_POLARITY_NEGATIVE),	/* 15: */
+};
Of course you already know that this has to go into the device-tree :)
Yeah, I guess it should.  I'll have to figure out how to do that.

+/*
+ * Interrupt setup and service.  Interrrupts on the Sandpoint come
+ * from the four PCI slots plus the 8259 in the Winbond Super I/O (SIO).
+ * The 8259 is cascaded from EPIC IRQ0, IRQ1-4 map to PCI slots 1-4,
+ * IDE is on EPIC 7 and 8.
+ */
+static void __init
+sandpoint_init_IRQ(void)
+{
+	struct mpic *mpic;
+	phys_addr_t openpic_paddr = 0xfc000000 + /* XXXX */
+		MPC10X_EUMB_EPIC_OFFSET;
+
+	/* This doesn't handle i2c, dma, i2o, or timer intrs right now XXXX */
+	mpic = mpic_alloc(openpic_paddr, MPIC_PRIMARY,
+			16 /* XXXX otherwise num_sources used */ ,
+			NUM_8259_INTERRUPTS,
+			0, /* was 16 */
+			NR_IRQS - 4 /* XXXX */,
+			sandpoint_openpic_initsenses,
+			sizeof(sandpoint_openpic_initsenses), " EPIC     ");
+
+	BUG_ON(mpic == NULL); /* XXXX */
+	mpic_assign_isu(mpic,  0, openpic_paddr + 0x10200);
+	mpic_init(mpic);
+	mpic_setup_cascade(NUM_8259_INTERRUPTS, i8259_irq_cascade, NULL);
+	i8259_init(0xfef00000, 0); /* pci iack addr */
+}
It would be really nice if we could define properties to put in the
"mpic" node that defines the various attributes to pass to it and that
sort of thing so that we can have a generic "mpic_init_IRQ" that walks
all MPICs and instanciate & link them in cascade when necessary... Oh
well, food for thought, no high priority there.
That would be great to have!

+/*
+ * Freescale Sandpoint interrupt routing.
+ */
+static inline int
+x3_map_irq(struct pci_dev *dev, unsigned char idsel, unsigned char pin)
+{
+	static char pci_irq_table[][4] =
+	/*
+	 *	PCI IDSEL/INTPIN->INTLINE
+	 * 	   A   B   C   D
+	 */
+	{
+		{ 16,  0,  0,  0 },	/* IDSEL 11 - i8259 on Winbond */
+		{  0,  0,  0,  0 },	/* IDSEL 12 - unused */
+		{ 18, 21, 20, 19 },	/* IDSEL 13 - PCI slot 1 */
+		{ 19, 18, 21, 20 },	/* IDSEL 14 - PCI slot 2 */
+		{ 20, 19, 18, 21 },	/* IDSEL 15 - PCI slot 3 */
+		{ 21, 20, 19, 18 },	/* IDSEL 16 - PCI slot 4 */
+	};
+
+	const long min_idsel = 11, max_idsel = 16, irqs_per_slot = 4;
+	return PCI_IRQ_TABLE_LOOKUP;
+}
Of course you also know that the above has to go into the device-tree as
well :)
Okay...  :)

+static void __init
+sandpoint_setup_winbond_83553(struct pci_controller *hose)
+{
+	int		devfn;
+
+	/*
+	 * Route IDE interrupts directly to the 8259's IRQ 14 & 15.
+	 * We can't route the IDE interrupt to PCI INTC# or INTD# because those
+	 * woule interfere with the PMC's INTC# and INTD# lines.
+	 */
+	/*
+	 * Winbond Fcn 0
+	 */
+	devfn = PCI_DEVFN(11,0);
+
+	early_write_config_byte(hose,
+				0,
+				devfn,
+				0x43, /* IDE Interrupt Routing Control */
+				0xef);
+	early_write_config_word(hose,
+				0,
+				devfn,
+				0x44, /* PCI Interrupt Routing Control */
+				0x0000);
+
+	/* Want ISA memory cycles to be forwarded to PCI bus */
+	early_write_config_byte(hose,
+				0,
+				devfn,
+				0x48, /* ISA-to-PCI Addr Decoder Control */
+				0xf0);
+
+	/* Enable Port 92.  */
+	early_write_config_byte(hose,
+				0,
+				devfn,
+				0x4e,	/* AT System Control Register */
+				0x06);
+	/*
+	 * Winbond Fcn 1
+	 */
+	devfn = PCI_DEVFN(11,1);
+
+	/* Put IDE controller into native mode. */
+	early_write_config_byte(hose,
+				0,
+				devfn,
+				0x09,	/* Programming interface Register */
+				0x8f);
+
+	/* Init IRQ routing, enable both ports, disable fast 16 */
+	early_write_config_dword(hose,
+				0,
+				devfn,
+				0x40,	/* IDE Control/Status Register */
+				0x00ff0011);
+	return;
+}
How much of the above could/should be done by the wrapper since it does
config space access hacks ? My idea is that we might be able to remove
pretty much _everything_ from this sandpoint.c file ... My dream would
be to have a generic board support that matches a list of known boards
for which no special code is needed, only the device-tree (and possibly
the zImage wrapper).
That's a good dream.  :)

The main things preventing us from doing that at the moment is that we
need to define enough standard nodes/properties to have the ability to
the setup interrupt controller tree entirely from it, and the PCI host
bridges as well. For the later, that means encoding enough of the bridge
type and config access method to allow having a generic device-tree
based piece of code that does what add_bridges() does in a generic way. 
Yep.

+static int
+x3_exclude_device(u_char bus, u_char devfn)
+{
+	if ((bus == 0) && (PCI_SLOT(devfn) == 0))
+		return PCIBIOS_DEVICE_NOT_FOUND;
+	else
+		return PCIBIOS_SUCCESSFUL;
+}
If we need to exclude devices in a generic way, we could define a
specific exclude list property... or use what I think has already been
defined by OF for indicating which slots should be probed :)
I'll try to do it through pci quirks, if I can.  If not, then I agree.

+static int __init
+add_bridge(struct device_node *dev)
+{
+	int len;
+	struct pci_controller *hose;
+	int *bus_range;
+
+	printk("Adding PCI host bridge %s\n", dev->full_name);
+
+	bus_range = (int *) get_property(dev, "bus-range", &len);
+	if (bus_range == NULL || len < 2 * sizeof(int))
+		printk(KERN_WARNING "Can't get bus-range for %s, assume"
+				" bus 0\n", dev->full_name);
+
+	hose = pcibios_alloc_controller();
+	if (hose == NULL)
+		return -ENOMEM;
+	hose->first_busno = bus_range ? bus_range[0] : 0;
+	hose->last_busno = bus_range ? bus_range[1] : 0xff;
+	setup_indirect_pci(hose, 0xfec00000, 0xfee00000);
+
+	/* Interpret the "ranges" property */
+	/* This also maps the I/O region and sets isa_io/mem_base */
+	pci_process_bridge_OF_ranges(hose, dev, 1);
+
+	return 0;
+}
See above comments.... Looks ok but how many boards will end up having
very small variations of that same code and thus can we find a way to
make that totally generic...
Many...

+u32 mag_dbg = 0;
+
+static void __init
+sandpoint_setup_arch(void)
+{
+	loops_per_jiffy = 100000000 / HZ;
+	isa_io_base = 0xfe000000;
+	isa_mem_base = 0x80000000;
+	pci_dram_offset = 0;
The isa stuff should probably be deduced from the device-tree, the
default LPJ too, probably not even useful anymore since we are using the
timebase nowadays.
Okay.

+#ifdef CONFIG_BLK_DEV_INITRD
+	if (initrd_start)
+		ROOT_DEV = Root_RAM0;
+	else
+#endif
+#ifdef	CONFIG_ROOT_NFS
+		ROOT_DEV = Root_NFS;
+#else
+		ROOT_DEV = Root_HDA1;
+#endif
Same comment as above, this is fairly generic and we could even imagine
something like a linux,default-boot-device thing in the DT...
OK.

+#if 1 /* XXXX NEW */
+	{
+		struct device_node *np;
+
+		mag_dbg = 1;
+		for (np = NULL; (np = of_find_node_by_type(np, "pci")) != NULL;)
+			add_bridge(np);
+		mag_dbg = 0;
+
+		ppc_md.pci_swizzle = common_swizzle;
+		ppc_md.pci_map_irq = x3_map_irq;
+		ppc_md.pci_exclude_device = x3_exclude_device; /* XXXX */
+	}
+#endif
The problem of IRQ mapping & swizzling will be solved real soon
(hopefully early 2.6.18) so that we can completely rely on the
device-tree without needing device_node's for every PCI device. Only P2P
bridges that don't do standard swizzling (typically bridges soldered on
the motherboard) will require device nodes. I'm working on it :)

The idea is that the host bridge will need the standard interrupt-map &
interrupt-map-mask which will indicate the interrupt mapping for every
line of every slot/device in the standard way handled by OF (that
already works). And I'll add code that will be able to handle PCI
devices without a node by artificially building their "interrupts"
property based on the interrupt pin config space entry and walking up
the PCI tree, doing standard swizzling when encountering a P2P bridge
until we hit something with a device-node in which case we hook our
result to the interrupt-map of that device-node.
Cool.  I'll keep an eye out for that.

+	if (cpu_has_feature(CPU_FTR_SPEC7450))
+		/* 745x is different.  We only want to pass along enable. */
+		_set_L2CR(L2CR_L2E);
+	else if (cpu_has_feature(CPU_FTR_L2CR))
+		/* All modules have 1MB of L2.  We also assume that an
+		 * L2 divisor of 3 will work.
+		 */
+		_set_L2CR(L2CR_L2E | L2CR_L2SIZ_1MB | L2CR_L2CLK_DIV3
+				| L2CR_L2RAM_PIPE | L2CR_L2OH_1_0 | L2CR_L2DF);
Keeping in mind my idea of having as much generic code as possible,
whould we do the above in the boot wrapper or if not, should we provide
some DT entries indicating that L2CR should be reset and to which
values ?
I vote for a DT entry.

+#if 0
+	/* Untested right now. */
+	if (cpu_has_feature(CPU_FTR_L3CR)) {
+		/* Magic value. */
+		_set_L3CR(0x8f032000);
+	}
+#endif
+}
Same comment as above but for L3CR
DT entry. :)

 .../...

+/*
+ * Fix IDE interrupts.
+ */
+static int __init
+sandpoint_fix_winbond_83553(void)
+{
+	/* Make some 8259 interrupt level sensitive */
+	outb(0xe0, 0x4d0);
+	outb(0xde, 0x4d1);
+
+	return 0;
+}
+
+arch_initcall(sandpoint_fix_winbond_83553);
+
+/*
+ * Initialize the ISA devices on the Nat'l PC87308VUL SuperIO chip.
+ */
+static int __init
+sandpoint_setup_natl_87308(void)
+{
+	u_char	reg;
+
+	/*
+	 * Enable all the devices on the Super I/O chip.
+	 */
+	SANDPOINT_87308_SELECT_DEV(0x00); /* Select kbd logical device */
+	SANDPOINT_87308_CFG_OUTB(0xf0, 0x00); /* Set KBC clock to 8 Mhz */
+	SANDPOINT_87308_DEV_ENABLE(0x00); /* Enable keyboard */
+	SANDPOINT_87308_DEV_ENABLE(0x01); /* Enable mouse */
+	SANDPOINT_87308_DEV_ENABLE(0x02); /* Enable rtc */
+	SANDPOINT_87308_DEV_ENABLE(0x03); /* Enable fdc (floppy) */
+	SANDPOINT_87308_DEV_ENABLE(0x04); /* Enable parallel */
+	SANDPOINT_87308_DEV_ENABLE(0x05); /* Enable UART 2 */
+	SANDPOINT_87308_CFG_OUTB(0xf0, 0x82); /* Enable bank select regs */
+	SANDPOINT_87308_DEV_ENABLE(0x06); /* Enable UART 1 */
+	SANDPOINT_87308_CFG_OUTB(0xf0, 0x82); /* Enable bank select regs */
+
+	/* Set up floppy in PS/2 mode */
+	outb(0x09, SIO_CONFIG_RA);
+	reg = inb(SIO_CONFIG_RD);
+	reg = (reg & 0x3F) | 0x40;
+	outb(reg, SIO_CONFIG_RD);
+	outb(reg, SIO_CONFIG_RD);	/* Have to write twice to change! */
+
+	return 0;
+}
Looks like zImage wrapper work to me... Unless we ever get booted by a
firmware that both provides a flat DT and doesn't do the above....

Ok, I'm not _ABSOLUTELY_ trying to remove the need for a myboar.c file
here ... but it might be worth doing something generic enough so that
it's either reduced to the strict minimum in many cases _or_ made
completely unnecessary if we can...
OK

+arch_initcall(sandpoint_setup_natl_87308);
+
+static int __init
+sandpoint_request_io(void)
+{
+	request_region(0x00,0x20,"dma1");
+	request_region(0x20,0x20,"pic1");
+	request_region(0x40,0x20,"timer");
+	request_region(0x80,0x10,"dma page reg");
+	request_region(0xa0,0x20,"pic2");
+	request_region(0xc0,0x20,"dma2");
+
+	return 0;
+}
The above has to go... not sure yet what is the best way but it's just a
pain... Could be made dependent on the OFDT too
OK

+arch_initcall(sandpoint_request_io);
+#endif
+
+static void __init
+sandpoint_map_io(void)
+{
+	io_block_mapping(0xfe000000, 0xfe000000, 0x02000000, _PAGE_IO);
+}
To kill absolutely. What is it there for ? At the very least, make
io_block_mapping allocate virtual space as discussed earlier. I'd like
to keep BATs for RAM anyway
It'll go away eventually.

+static void
+sandpoint_restart(char *cmd)
+{
+	local_irq_disable();
+
+	/* Set exception prefix high - to the firmware */
+	_nmask_and_or_msr(0, MSR_IP);
+
+	/* Reset system via Port 92 */
+	outb(0x00, 0x92);
+	outb(0x01, 0x92);
+
+	for(;;);	/* Spin until reset happens */
+}
The above is fairly common. (Or variations of it). In the context of a
"generic" board, we could imagine having a list of known "reboot
methods" and have a DT node indicating which one to use...
Yep.

+static void
+sandpoint_power_off(void)
+{
+	local_irq_disable();
+	for(;;);	/* No way to shut power off with software */
+	/* NOTREACHED */
+}
The above is _VERY_ commmon :) probably worth just not having a callback
in ppc_md. at all...

+static void
+sandpoint_halt(void)
+{
+	sandpoint_power_off();
+	/* NOTREACHED */
+}
Same comment as before.
I do see a couple in arch/ppc/platforms that do something different so
how about keeping ppc_md.power_off/_halt but making the above the default
(with only 1 implementation) which can be changed by board code?

+static void
+sandpoint_show_cpuinfo(struct seq_file *m)
+{
+	seq_printf(m, "vendor\t\t: Freescale\n");
+	seq_printf(m, "machine\t\t: Sandpoint\n");
+}
Do we need that ? We could define standard DT properties that get
printed in cpuinfo and have that in setup-common.c ...
That would be better.

 .../... (commented out IDE code)

This is a windbond ? Can't it just be probed/detected using normal PCI
probing ?
I have to revisit all of the IDE stuff in that file and figure out
what's really needed (if any).  That's why I have it all '#if 0' right
now.  I will get to it eventually.

+
+/*
+ * Set BAT 3 to map 0xf8000000 to end of physical memory space 1-to-1.
+ */
+static __inline__ void
+sandpoint_set_bat(void)
+{
+	unsigned long bat3u, bat3l;
+
+	__asm__ __volatile__(
+			" lis %0,0xf800\n	\
+			ori %1,%0,0x002a\n	\
+			ori %0,%0,0x0ffe\n	\
+			mtspr 0x21e,%0\n	\
+			mtspr 0x21f,%1\n	\
+			isync\n			\
+			sync "
+			: "=r" (bat3u), "=r" (bat3l));
+}
WTF ?
It'll disappear.  The sandpoint.c is copied from arch/ppc and hacked.
Its pretty old so there's lots of code that can probably be weeded out.

+TODC_ALLOC();
+
+static int __init
+sandpoint_probe(void)
+{
+	return 1;
+}
WTF (bis) ? :)
bis?

Its just a hack to get it going.

+define_machine(sandpoint) {
+	.name			= "Sandpoint",
+	.probe			= sandpoint_probe,
+	.setup_arch		= sandpoint_setup_arch,
+	.setup_io_mappings	= sandpoint_map_io,
+	.init_IRQ		= sandpoint_init_IRQ,
+	.show_cpuinfo		= sandpoint_show_cpuinfo,
+	.get_irq		= mpic_get_irq,
+	.restart		= sandpoint_restart,
+	.power_off		= sandpoint_power_off,
+	.halt			= sandpoint_halt,
+	.calibrate_decr		= generic_calibrate_decr,
+	/*
+	.progress		= udbg_progress,
+	*/
+};
So on the TODO list, replace as much as we can of the above with
generic_* equivalents until nothing remains :)
Yep, that's the goal.

diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 7dcdfcb..1badbec 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -629,6 +629,13 @@ #endif /* CONFIG_SMP */
 			mb();
 	}
 
+#ifdef CONFIG_MPIC_SERIAL
+	/* For serial interrupts & set clock ratio */
+	mpic_write(mpic->gregs, MPIC_GREG_GLOBAL_CONF_1,
+		mpic_read(mpic->gregs, MPIC_GREG_GLOBAL_CONF_1)
+			| (1<<27) | (0x7<<28));
+#endif
This should not be an #ifdef.... this should be an MPIC init flag passed
when initializing it. (and if we do generic device-tree instanciation of
PIC's, we could define standard properties to put in the MPIC node to
enable those flags).
Okay, I'll move it into the dt.

Keep up the good work ! :)
Heh.

Cheers,
Ben.
Thanks for the comments.

Mark
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
---
Guennadi Liakhovetski

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help