Skip to contentSkip to conversation

Marvell host bridge support (mv64x60)

PATCH In review Mark A. Greer 3 participants 7 messages Nov 19, 2004 – Nov 23, 2004

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Marvell host bridge support (mv64x60)

This patch adds core support for a line of host bridges from Marvell (formerly Galileo). This code has been tested with a GT64260a, GT64260b, MV64360, and MV64460. Patches for platforms that use these bridges will be sent separately.

The patch is rather large so a link is provided.

--

ftp://source.mvista.com/pub/mgreer/mv64x60.patch

#1 lore raw
Quoted 6 lines · Mark A. Greer

This patch adds core support for a line of host bridges from Marvell (formerly Galileo). This code has been tested with a GT64260a, GT64260b, MV64360, and MV64460. Patches for platforms that use these bridges will be sent separately.

Shouldn't these guys:

+ u32 cpu2mem_tab[MV64x60_CPU2MEM_WINDOWS][2] = {

+                       { MV64x60_CPU2MEM_0_BASE, MV64x60_CPU2MEM_0_SIZE },
+                       { MV64x60_CPU2MEM_1_BASE, MV64x60_CPU2MEM_1_SIZE },
+                       { MV64x60_CPU2MEM_2_BASE, MV64x60_CPU2MEM_2_SIZE },
+                       { MV64x60_CPU2MEM_3_BASE, MV64x60_CPU2MEM_3_SIZE }
+       };
+       u32     com2mem_tab[MV64x60_CPU2MEM_WINDOWS][2] = {
+                       { MV64360_MPSC2MEM_0_BASE, MV64360_MPSC2MEM_0_SIZE },
+                       { MV64360_MPSC2MEM_1_BASE, MV64360_MPSC2MEM_1_SIZE },
+                       { MV64360_MPSC2MEM_2_BASE, MV64360_MPSC2MEM_2_SIZE },
+                       { MV64360_MPSC2MEM_3_BASE, MV64360_MPSC2MEM_3_SIZE }

+ };
+ u32 dram_selects[MV64x60_CPU2MEM_WINDOWS] = { 0xe, 0xd, 0xb, 0x7 };

be static, and maybe __devinitdata? Right now, the CPU has to populate them by hand at runtime.

+wait_for_ownership(int chan)
+{
+ int i;
+
+ for (i=0; i<MAX_TX_WAIT; i++) {
+ if ((MV64x60_REG_READ(sdma_regs[chan].sdcm) &
+ SDMA_SDCM_TXD) == 0)
+ break;
+
+ udelay(1000);

ow, big busywait. Can't use a sleep in here? I guess not :(

+ * arch/ppc/boot/simple/mv64x60_tty.c

hm. Normally we put arch-specific drivers like this into drivers/serial and do the appropriate Kconfig work. Is there a reason why this serial driver is buried under arch/ppc?

+#include "../../../../drivers/serial/mpsc_defs.h"

erk.

+struct mv64x60_rx_desc {
+ volatile u16 bufsize;
+ volatile u16 bytecnt;
+ volatile u32 cmd_stat;
+ volatile u32 next_desc_ptr;
+ volatile u32 buffer;
+};
+
+struct mv64x60_tx_desc {
+ volatile u16 bytecnt;
+ volatile u16 shadow;
+ volatile u32 cmd_stat;
+ volatile u32 next_desc_ptr;
+ volatile u32 buffer;
+};

Do these need to be volatile? If so, it indicates that the driver is doing something wrong.

+gt64260_register_hdlrs(void)
+{
+ /* Register CPU interface error interrupt handler */
+ request_irq(MV64x60_IRQ_CPU_ERR, gt64260_cpu_error_int_handler,
+ SA_INTERRUPT, CPU_INTR_STR, 0);

request_irq() can fail.

+int
+mv64360_get_irq(struct pt_regs *regs)
+{
+ int irq;
+ int irq_gpp;
+
+#ifdef CONFIG_SMP
+ /*
+ * Second CPU gets only doorbell (message) interrupts.
+ * The doorbell interrupt is BIT28 in the main interrupt low cause reg.
+ */
+ int cpu_nr = smp_processor_id();

This function has no callers, so I am unable to determine whether it is called from non-preemptible code. If it is called from preemptible code then that smp_processor_id() is buggy, because you can switch CPUs at any time.

+static struct platform_device mpsc_shared_device = { /* Shared device */
+ .name = MPSC_SHARED_NAME,
+ .id = 0,
+ .num_resources = ARRAY_SIZE(mv64x60_mpsc_shared_resources),
+ .resource = mv64x60_mpsc_shared_resources,
+ .dev = {
+ .driver_data = (void *)&mv64x60_mpsc_shared_pd_dd,
+ },
+};

The cast to void* is unnecessary.

+ (void)mv64x60_setup_for_chip(&bh);

how come you always stick that (void) in there?

+mv64x60_config_cpu2mem_windows(struct mv64x60_handle *bh,
+ struct mv64x60_setup_info *si,
+ u32 mem_windows[MV64x60_CPU2MEM_WINDOWS][2])
+{
+ u32 i, win;
+ u32 prot_tab[] = {
+ MV64x60_CPU_PROT_0_WIN, MV64x60_CPU_PROT_1_WIN,
+ MV64x60_CPU_PROT_2_WIN, MV64x60_CPU_PROT_3_WIN
+ };
+ u32 cpu_snoop_tab[] = {
+ MV64x60_CPU_SNOOP_0_WIN, MV64x60_CPU_SNOOP_1_WIN,
+ MV64x60_CPU_SNOOP_2_WIN, MV64x60_CPU_SNOOP_3_WIN
+ };

static initialisation?

+mv64x60_config_cpu2pci_windows(struct mv64x60_handle *bh,
+ struct mv64x60_pci_info *pi, u32 bus)
+{
+ int i;
+ u32 win_tab[2][4] = {
+ { MV64x60_CPU2PCI0_IO_WIN, MV64x60_CPU2PCI0_MEM_0_WIN,
+ MV64x60_CPU2PCI0_MEM_1_WIN,
+ MV64x60_CPU2PCI0_MEM_2_WIN },
+ { MV64x60_CPU2PCI1_IO_WIN, MV64x60_CPU2PCI1_MEM_0_WIN,
+ MV64x60_CPU2PCI1_MEM_1_WIN,
+ MV64x60_CPU2PCI1_MEM_2_WIN },
+ };
+ u32 remap_tab[2][4] = {
+ { MV64x60_CPU2PCI0_IO_REMAP_WIN,

+			  MV64x60_CPU2PCI0_MEM_0_REMAP_WIN,
+			  MV64x60_CPU2PCI0_MEM_1_REMAP_WIN,
+			  MV64x60_CPU2PCI0_MEM_2_REMAP_WIN },
+			{ MV64x60_CPU2PCI1_IO_REMAP_WIN,
+			  MV64x60_CPU2PCI1_MEM_0_REMAP_WIN,
+			  MV64x60_CPU2PCI1_MEM_1_REMAP_WIN,
+			  MV64x60_CPU2PCI1_MEM_2_REMAP_WIN }

+ };
+

ditto

+mv64x60_config_pci2mem_windows(struct mv64x60_handle *bh,

and here

+mv64360_set_pci2mem_window(struct pci_controller *hose, u32 bus, u32 window,

and here

+mv64360_config_io2mem_windows(struct mv64x60_handle *bh,

and here

Anyway, I'll stick this as-is in -mm. Feel free to send an incremental patch, or a replacement.

#2 lore raw
Quoted 25 lines · Andrew Morton
Quoted 4 lines · Mark A. Greer

This patch adds core support for a line of host bridges from Marvell (formerly Galileo). This code has been tested with a GT64260a, GT64260b, MV64360, and MV64460. Patches for platforms that use these bridges will be sent separately.

Shouldn't these guys:

(quoted diff) +13 -0
+       u32     cpu2mem_tab[MV64x60_CPU2MEM_WINDOWS][2] = {
+                       { MV64x60_CPU2MEM_0_BASE, MV64x60_CPU2MEM_0_SIZE },
+                       { MV64x60_CPU2MEM_1_BASE, MV64x60_CPU2MEM_1_SIZE },
+                       { MV64x60_CPU2MEM_2_BASE, MV64x60_CPU2MEM_2_SIZE },
+                       { MV64x60_CPU2MEM_3_BASE, MV64x60_CPU2MEM_3_SIZE }
+       };
+       u32     com2mem_tab[MV64x60_CPU2MEM_WINDOWS][2] = {
+                       { MV64360_MPSC2MEM_0_BASE, MV64360_MPSC2MEM_0_SIZE },
+                       { MV64360_MPSC2MEM_1_BASE, MV64360_MPSC2MEM_1_SIZE },
+                       { MV64360_MPSC2MEM_2_BASE, MV64360_MPSC2MEM_2_SIZE },
+                       { MV64360_MPSC2MEM_3_BASE, MV64360_MPSC2MEM_3_SIZE }
+       };
+       u32     dram_selects[MV64x60_CPU2MEM_WINDOWS] = { 0xe, 0xd, 0xb, 0x7 };
Quoted 3 lines

be static, and maybe __devinitdata? Right now, the CPU has to populate them by hand at runtime.

Yes. I'll fix that.

Quoted 12 lines
(quoted diff) +10 -0
+wait_for_ownership(int chan)
+{
+	int	i;
+
+	for (i=0; i<MAX_TX_WAIT; i++) {
+		if ((MV64x60_REG_READ(sdma_regs[chan].sdcm) &
+				SDMA_SDCM_TXD) == 0)
+			break;
+
+		udelay(1000);
Quoted 2 lines

ow, big busywait. Can't use a sleep in here? I guess not :(

This code is in the ppc bootwrapper which is glue code to get the hardware (and bd_info, etc.) from whatever state the f/w left it in into something the kernel can work with. IOW, its not in the kernel and there is no sleep mechanism...or even a thread/process entity to put to sleep.

Quoted 6 lines

+ * arch/ppc/boot/simple/mv64x60_tty.c

hm. Normally we put arch-specific drivers like this into drivers/serial and do the appropriate Kconfig work. Is there a reason why this serial driver is buried under arch/ppc?

It isn't a part of the kernel so I don't think it belongs in drivers/serial. This particular serial driver is required for cmd_line editing when booting a zImage.

Quoted 4 lines

+#include "../../../../drivers/serial/mpsc_defs.h"

erk.

Ah, yeah, I'll fix that too :)

Quoted 17 lines
(quoted diff) +15 -0
+struct mv64x60_rx_desc {
+	volatile u16 bufsize;
+	volatile u16 bytecnt;
+	volatile u32 cmd_stat;
+	volatile u32 next_desc_ptr;
+	volatile u32 buffer;
+};
+
+struct mv64x60_tx_desc {
+	volatile u16 bytecnt;
+	volatile u16 shadow;
+	volatile u32 cmd_stat;
+	volatile u32 next_desc_ptr;
+	volatile u32 buffer;
+};
Quoted 3 lines

Do these need to be volatile? If so, it indicates that the driver is doing something wrong.

I didn't spend much time looking at this code. I'll clean it up.

Quoted 9 lines
(quoted diff) +5 -0
+gt64260_register_hdlrs(void)
+{
+	/* Register CPU interface error interrupt handler */
+	request_irq(MV64x60_IRQ_CPU_ERR, gt64260_cpu_error_int_handler,
+		    SA_INTERRUPT, CPU_INTR_STR, 0);

request_irq() can fail.

OK.

Quoted 13 lines
(quoted diff) +12 -0
+int
+mv64360_get_irq(struct pt_regs *regs)
+{
+	int irq;
+	int irq_gpp;
+
+#ifdef CONFIG_SMP
+	/*
+	 * Second CPU gets only doorbell (message) interrupts.
+	 * The doorbell interrupt is BIT28 in the main interrupt low cause reg.
+	 */
+	int cpu_nr = smp_processor_id();
Quoted 5 lines

This function has no callers, so I am unable to determine whether it is called from non-preemptible code. If it is called from preemptible code then that smp_processor_id() is buggy, because you can switch CPUs at any time.

Its called via ppc_md.get_irq() (see arch/ppc/kernel/irq.c:do_IRQ()). ppc_md.get_irq is set up in the platform files.

Quoted 10 lines
(quoted diff) +9 -0
+static struct platform_device mpsc_shared_device = { /* Shared device */
+	.name		= MPSC_SHARED_NAME,
+	.id		= 0,
+	.num_resources	= ARRAY_SIZE(mv64x60_mpsc_shared_resources),
+	.resource	= mv64x60_mpsc_shared_resources,
+	.dev = {
+		.driver_data = (void *)&mv64x60_mpsc_shared_pd_dd,
+	},
+};
Quoted 2 lines

The cast to void* is unnecessary.

OK.

Quoted 4 lines
(quoted diff) +1 -0
+	(void)mv64x60_setup_for_chip(&bh);

how come you always stick that (void) in there?

I did that b/c the routine returns an 'int' but I'm ignoring it. I probably shouldn't be ignoring it...

Quoted 15 lines
(quoted diff) +13 -0
+mv64x60_config_cpu2mem_windows(struct mv64x60_handle *bh,
+	struct mv64x60_setup_info *si,
+	u32 mem_windows[MV64x60_CPU2MEM_WINDOWS][2])
+{
+	u32	i, win;
+	u32	prot_tab[] = {
+			MV64x60_CPU_PROT_0_WIN, MV64x60_CPU_PROT_1_WIN,
+			MV64x60_CPU_PROT_2_WIN, MV64x60_CPU_PROT_3_WIN
+		};
+	u32	cpu_snoop_tab[] = {
+			MV64x60_CPU_SNOOP_0_WIN, MV64x60_CPU_SNOOP_1_WIN,
+			MV64x60_CPU_SNOOP_2_WIN, MV64x60_CPU_SNOOP_3_WIN
+		};
Quoted 2 lines

static initialisation?

Yep.

Quoted 32 lines
(quoted diff) +23 -0
+mv64x60_config_cpu2pci_windows(struct mv64x60_handle *bh,
+	struct mv64x60_pci_info *pi, u32 bus)
+{
+	int	i;
+	u32	win_tab[2][4] = {
+			{ MV64x60_CPU2PCI0_IO_WIN, MV64x60_CPU2PCI0_MEM_0_WIN,
+			  MV64x60_CPU2PCI0_MEM_1_WIN,
+			  MV64x60_CPU2PCI0_MEM_2_WIN },
+			{ MV64x60_CPU2PCI1_IO_WIN, MV64x60_CPU2PCI1_MEM_0_WIN,
+			  MV64x60_CPU2PCI1_MEM_1_WIN,
+			  MV64x60_CPU2PCI1_MEM_2_WIN },
+		};
+	u32	remap_tab[2][4] = {
+			{ MV64x60_CPU2PCI0_IO_REMAP_WIN,
+			  MV64x60_CPU2PCI0_MEM_0_REMAP_WIN,
+			  MV64x60_CPU2PCI0_MEM_1_REMAP_WIN,
+			  MV64x60_CPU2PCI0_MEM_2_REMAP_WIN },
+			{ MV64x60_CPU2PCI1_IO_REMAP_WIN,
+			  MV64x60_CPU2PCI1_MEM_0_REMAP_WIN,
+			  MV64x60_CPU2PCI1_MEM_1_REMAP_WIN,
+			  MV64x60_CPU2PCI1_MEM_2_REMAP_WIN }
+		};
+

ditto

+mv64x60_config_pci2mem_windows(struct mv64x60_handle *bh,

and here

Quoted 7 lines

+mv64360_set_pci2mem_window(struct pci_controller *hose, u32 bus, u32 window,

and here

+mv64360_config_io2mem_windows(struct mv64x60_handle *bh,

and here

Yes, several times. I'll fix it.

Quoted 4 lines

Anyway, I'll stick this as-is in -mm. Feel free to send an incremental patch, or a replacement.

Thanks for the feedback. I'll clean it up & resend.

Mark

#3 lore raw
In reply to Andrew Morton
Quoted 3 lines · Andrew Morton

Anyway, I'll stick this as-is in -mm. Feel free to send an incremental patch, or a replacement.

Here is an incremental patch [hopefully] with your concerns addressed. Note that the arch/ppc/boot code is not kernel code and only exists for a short period of time before execution jumps to the kernel. Please let me know if you have any more concerns.

--

arch/ppc/boot/include/mpsc_defs.h new file +146 -0
diff -Nru a/arch/ppc/boot/include/mpsc_defs.h b/arch/ppc/boot/include/mpsc_defs.h
@@ -0,0 +1,146 @@
+/*
+ * drivers/serial/mpsc/mpsc_defs.h
+ * 
+ * Register definitions for the Marvell Multi-Protocol Serial Controller (MPSC),
+ * Serial DMA Controller (SDMA), and Baud Rate Generator (BRG).
+ *
+ * Author: Mark A. Greer <[email protected]>
+ *
+ * 2004 (c) MontaVista, Software, Inc.  This file is licensed under
+ * the terms of the GNU General Public License version 2.  This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+#ifndef	_PPC_BOOT_MPSC_DEFS_H__
+#define	_PPC_BOOT_MPSC_DEFS_H__
+
+#define	MPSC_NUM_CTLRS		2
+
+/*
+ *****************************************************************************
+ *
+ *	Multi-Protocol Serial Controller Interface Registers
+ *
+ *****************************************************************************
+ */
+
+/* Main Configuratino Register Offsets */
+#define	MPSC_MMCRL			0x0000
+#define	MPSC_MMCRH			0x0004
+#define	MPSC_MPCR			0x0008
+#define	MPSC_CHR_1			0x000c
+#define	MPSC_CHR_2			0x0010
+#define	MPSC_CHR_3			0x0014
+#define	MPSC_CHR_4			0x0018
+#define	MPSC_CHR_5			0x001c
+#define	MPSC_CHR_6			0x0020
+#define	MPSC_CHR_7			0x0024
+#define	MPSC_CHR_8			0x0028
+#define	MPSC_CHR_9			0x002c
+#define	MPSC_CHR_10			0x0030
+#define	MPSC_CHR_11			0x0034
+
+#define	MPSC_MPCR_CL_5			0
+#define	MPSC_MPCR_CL_6			1
+#define	MPSC_MPCR_CL_7			2
+#define	MPSC_MPCR_CL_8			3
+#define	MPSC_MPCR_SBL_1			0
+#define	MPSC_MPCR_SBL_2			3
+
+#define	MPSC_CHR_2_TEV			(1<<1)
+#define	MPSC_CHR_2_TA			(1<<7)
+#define	MPSC_CHR_2_TTCS			(1<<9)
+#define	MPSC_CHR_2_REV			(1<<17)
+#define	MPSC_CHR_2_RA			(1<<23)
+#define	MPSC_CHR_2_CRD			(1<<25)
+#define	MPSC_CHR_2_EH			(1<<31)
+#define	MPSC_CHR_2_PAR_ODD		0
+#define	MPSC_CHR_2_PAR_SPACE		1
+#define	MPSC_CHR_2_PAR_EVEN		2
+#define	MPSC_CHR_2_PAR_MARK		3
+
+/* MPSC Signal Routing */
+#define	MPSC_MRR			0x0000
+#define	MPSC_RCRR			0x0004
+#define	MPSC_TCRR			0x0008
+
+/*
+ *****************************************************************************
+ *
+ *	Serial DMA Controller Interface Registers
+ *
+ *****************************************************************************
+ */
+
+#define	SDMA_SDC			0x0000
+#define	SDMA_SDCM			0x0008
+#define	SDMA_RX_DESC			0x0800
+#define	SDMA_RX_BUF_PTR			0x0808
+#define	SDMA_SCRDP			0x0810
+#define	SDMA_TX_DESC			0x0c00
+#define	SDMA_SCTDP			0x0c10
+#define	SDMA_SFTDP			0x0c14
+
+#define	SDMA_DESC_CMDSTAT_PE		(1<<0)
+#define	SDMA_DESC_CMDSTAT_CDL		(1<<1)
+#define	SDMA_DESC_CMDSTAT_FR		(1<<3)
+#define	SDMA_DESC_CMDSTAT_OR		(1<<6)
+#define	SDMA_DESC_CMDSTAT_BR		(1<<9)
+#define	SDMA_DESC_CMDSTAT_MI		(1<<10)
+#define	SDMA_DESC_CMDSTAT_A		(1<<11)
+#define	SDMA_DESC_CMDSTAT_AM		(1<<12)
+#define	SDMA_DESC_CMDSTAT_CT		(1<<13)
+#define	SDMA_DESC_CMDSTAT_C		(1<<14)
+#define	SDMA_DESC_CMDSTAT_ES		(1<<15)
+#define	SDMA_DESC_CMDSTAT_L		(1<<16)
+#define	SDMA_DESC_CMDSTAT_F		(1<<17)
+#define	SDMA_DESC_CMDSTAT_P		(1<<18)
+#define	SDMA_DESC_CMDSTAT_EI		(1<<23)
+#define	SDMA_DESC_CMDSTAT_O		(1<<31)
+
+#define SDMA_DESC_DFLT			(SDMA_DESC_CMDSTAT_O |	\
+					SDMA_DESC_CMDSTAT_EI)
+
+#define	SDMA_SDC_RFT			(1<<0)
+#define	SDMA_SDC_SFM			(1<<1)
+#define	SDMA_SDC_BLMR			(1<<6)
+#define	SDMA_SDC_BLMT			(1<<7)
+#define	SDMA_SDC_POVR			(1<<8)
+#define	SDMA_SDC_RIFB			(1<<9)
+
+#define	SDMA_SDCM_ERD			(1<<7)
+#define	SDMA_SDCM_AR			(1<<15)
+#define	SDMA_SDCM_STD			(1<<16)
+#define	SDMA_SDCM_TXD			(1<<23)
+#define	SDMA_SDCM_AT			(1<<31)
+
+#define	SDMA_0_CAUSE_RXBUF		(1<<0)
+#define	SDMA_0_CAUSE_RXERR		(1<<1)
+#define	SDMA_0_CAUSE_TXBUF		(1<<2)
+#define	SDMA_0_CAUSE_TXEND		(1<<3)
+#define	SDMA_1_CAUSE_RXBUF		(1<<8)
+#define	SDMA_1_CAUSE_RXERR		(1<<9)
+#define	SDMA_1_CAUSE_TXBUF		(1<<10)
+#define	SDMA_1_CAUSE_TXEND		(1<<11)
+
+#define	SDMA_CAUSE_RX_MASK	(SDMA_0_CAUSE_RXBUF | SDMA_0_CAUSE_RXERR | \
+	SDMA_1_CAUSE_RXBUF | SDMA_1_CAUSE_RXERR)
+#define	SDMA_CAUSE_TX_MASK	(SDMA_0_CAUSE_TXBUF | SDMA_0_CAUSE_TXEND | \
+	SDMA_1_CAUSE_TXBUF | SDMA_1_CAUSE_TXEND)
+
+/* SDMA Interrupt registers */
+#define	SDMA_INTR_CAUSE			0x0000
+#define	SDMA_INTR_MASK			0x0080
+
+/*
+ *****************************************************************************
+ *
+ *	Baud Rate Generator Interface Registers
+ *
+ *****************************************************************************
+ */
+
+#define	BRG_BCR				0x0000
+#define	BRG_BTR				0x0004
+
+#endif /*_PPC_BOOT_MPSC_DEFS_H__ */
arch/ppc/boot/simple/mv64x60_tty.c +29 -24

80 lines not on this page · raw message

arch/ppc/syslib/gt64260_pic.c +16 -6
diff -Nru a/arch/ppc/syslib/gt64260_pic.c b/arch/ppc/syslib/gt64260_pic.c
@@ -294,21 +294,31 @@
 static int __init
 gt64260_register_hdlrs(void)
 {
+	int rc;
+
 	/* Register CPU interface error interrupt handler */
-	request_irq(MV64x60_IRQ_CPU_ERR, gt64260_cpu_error_int_handler,
-		    SA_INTERRUPT, CPU_INTR_STR, 0);
+	if ((rc = request_irq(MV64x60_IRQ_CPU_ERR,
+		gt64260_cpu_error_int_handler, SA_INTERRUPT, CPU_INTR_STR, 0)))
+		printk(KERN_WARNING "Can't register cpu error handler: %d", rc);
+
 	mv64x60_write(&bh, MV64x60_CPU_ERR_MASK, 0);
 	mv64x60_write(&bh, MV64x60_CPU_ERR_MASK, 0x000000fe);
 
 	/* Register PCI 0 error interrupt handler */
-	request_irq(MV64360_IRQ_PCI0, gt64260_pci_error_int_handler,
-		    SA_INTERRUPT, PCI0_INTR_STR, (void *)0);
+	if ((rc = request_irq(MV64360_IRQ_PCI0, gt64260_pci_error_int_handler,
+		    SA_INTERRUPT, PCI0_INTR_STR, (void *)0)))
+		printk(KERN_WARNING "Can't register pci 0 error handler: %d",
+			rc);
+
 	mv64x60_write(&bh, MV64x60_PCI0_ERR_MASK, 0);
 	mv64x60_write(&bh, MV64x60_PCI0_ERR_MASK, 0x003c0c24);
 
 	/* Register PCI 1 error interrupt handler */
-	request_irq(MV64360_IRQ_PCI1, gt64260_pci_error_int_handler,
-		    SA_INTERRUPT, PCI1_INTR_STR, (void *)1);
+	if ((rc = request_irq(MV64360_IRQ_PCI1, gt64260_pci_error_int_handler,
+		    SA_INTERRUPT, PCI1_INTR_STR, (void *)1)))
+		printk(KERN_WARNING "Can't register pci 1 error handler: %d",
+			rc);
+
 	mv64x60_write(&bh, MV64x60_PCI1_ERR_MASK, 0);
 	mv64x60_write(&bh, MV64x60_PCI1_ERR_MASK, 0x003c0c24);
 
arch/ppc/syslib/mv64360_pic.c +18 -8
diff -Nru a/arch/ppc/syslib/mv64360_pic.c b/arch/ppc/syslib/mv64360_pic.c
@@ -367,16 +367,20 @@
 mv64360_register_hdlrs(void)
 {
 	u32	mask;
+	int	rc;
 
 	/* Register CPU interface error interrupt handler */
-	request_irq(MV64x60_IRQ_CPU_ERR, mv64360_cpu_error_int_handler,
-		    SA_INTERRUPT, CPU_INTR_STR, 0);
+	if ((rc = request_irq(MV64x60_IRQ_CPU_ERR,
+		mv64360_cpu_error_int_handler, SA_INTERRUPT, CPU_INTR_STR, 0)))
+		printk(KERN_WARNING "Can't register cpu error handler: %d", rc);
+
 	mv64x60_write(&bh, MV64x60_CPU_ERR_MASK, 0);
 	mv64x60_write(&bh, MV64x60_CPU_ERR_MASK, 0x000000ff);
 
 	/* Register internal SRAM error interrupt handler */
-	request_irq(MV64360_IRQ_SRAM_PAR_ERR, mv64360_sram_error_int_handler,
-		    SA_INTERRUPT, SRAM_INTR_STR, 0);
+	if ((rc = request_irq(MV64360_IRQ_SRAM_PAR_ERR,
+		mv64360_sram_error_int_handler,SA_INTERRUPT,SRAM_INTR_STR, 0)))
+		printk(KERN_WARNING "Can't register SRAM error handler: %d",rc);
 
 	/*
 	 * Bit 0 reserved on 64360 and erratum FEr PCI-#11 (PCI internal
@@ -390,14 +394,20 @@
 		mask |= 0x1;	/* enable DPErr on 64460 */
 
 	/* Register PCI 0 error interrupt handler */
-	request_irq(MV64360_IRQ_PCI0, mv64360_pci_error_int_handler,
-		    SA_INTERRUPT, PCI0_INTR_STR, (void *)0);
+	if ((rc = request_irq(MV64360_IRQ_PCI0, mv64360_pci_error_int_handler,
+		    SA_INTERRUPT, PCI0_INTR_STR, (void *)0)))
+		printk(KERN_WARNING "Can't register pci 0 error handler: %d",
+			rc);
+
 	mv64x60_write(&bh, MV64x60_PCI0_ERR_MASK, 0);
 	mv64x60_write(&bh, MV64x60_PCI0_ERR_MASK, mask);
 
 	/* Register PCI 1 error interrupt handler */
-	request_irq(MV64360_IRQ_PCI1, mv64360_pci_error_int_handler,
-		    SA_INTERRUPT, PCI1_INTR_STR, (void *)1);
+	if ((rc = request_irq(MV64360_IRQ_PCI1, mv64360_pci_error_int_handler,
+		    SA_INTERRUPT, PCI1_INTR_STR, (void *)1)))
+		printk(KERN_WARNING "Can't register pci 1 error handler: %d",
+			rc);
+
 	mv64x60_write(&bh, MV64x60_PCI1_ERR_MASK, 0);
 	mv64x60_write(&bh, MV64x60_PCI1_ERR_MASK, mask);
 
arch/ppc/syslib/mv64x60.c +104 -96

350 lines not on this page · raw message

#4 lore raw
Quoted 8 lines · Mark A. Greer
Quoted 3 lines · Andrew Morton

Anyway, I'll stick this as-is in -mm. Feel free to send an incremental patch, or a replacement.

Here is an incremental patch [hopefully] with your concerns addressed.

OK, thanks. I added this to the queue for post-2.6.10. I'll assume that silence means assent from the rest of the ppc team ;)

(Patches are at

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc2/2.6.10-rc2-mm3/broken-out/ppc32-marvell-host-bridge-support-mv64x60.patch ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc2/2.6.10-rc2-mm3/broken-out/ppc32-support-for-marvell-ev-64260-bp-eval-platform.patch ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10-rc2/2.6.10-rc2-mm3/broken-out/ppc32-support-for-artesyn-katana-cpci-boards.patch

#5 lore raw
In reply to Mark A. Greer

Just looking through this and you should share some more code with mips, e.g. the mavell register layout should move from asm-mips/marvell.h and your file to linux/marvell.h or something, especially as another ppc plattform (pegasosII) needs it aswell.

#6 lore raw
Quoted 5 lines · Christoph Hellwig

Just looking through this and you should share some more code with mips, e.g. the mavell register layout should move from asm-mips/marvell.h and your file to linux/marvell.h or something, especially as another ppc plattform (pegasosII) needs it aswell.

Yes, and there is already include/linux/mv643xx.h that has a bunch of 64340 definitions which share offset with the 64360 (and some with the 64260). However, to be correct, there needs to be a lot of munging. mv643xx.h should be renamed to mv64xxx.h and all of the MV64340 macros need to be renamed as well. This ripples into the mips code which I didn't want to battle with right now. I would prefer that this patch go in and then submit a separate patch that fixes and combines the hdr files & macros they contain. Doing it that way, there is one patch for the ppc bridge support and one patch for the hdr file clean up. That's cleaner than mixing the two (IMHO). It is on the todo list.

Mark