lkml.org 
[lkml]   [2004]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH][PPC32] Marvell host bridge support (mv64x60)
Andrew Morton wrote:

>"Mark A. Greer" <mgreer@mvista.com> wrote:
>
>
>>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.
>

Yes. I'll fix that.

>
>+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 :(
>

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.

>+ * 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.

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

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

>
>+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.
>

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

>
>+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.

>+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.
>

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.

>+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.
>

OK.

>+ (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...

>
>+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?
>

Yep.

>
>+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

Yes, several times. I'll fix it.

>
>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



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:08    [W:0.085 / U:0.460 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site