On Wed, Apr 1, 2009 at 1:16 PM, Roderick Colenbrander
<thunderbir...@gmail.com> wrote:
> Hi,
>
> As requested by Grant Likely here the same patch but now inlined.
word wrapped. neener neener.
Mostly looks good to me. It would help to split the PCI driver out
into a separate patch from the board support. Comments below.
> diff -urN -X linux-2.6.29/Documentation/dontdiff
> linux-2.6.29/arch/powerpc/boot/dts/virtex440-ml510.dts
> ml510-dev/linux-2.6.29/arch/powerpc/boot/dts/virtex440-ml510.dts
> --- linux-2.6.29/arch/powerpc/boot/dts/virtex440-ml510.dts
> +++ ml510-dev/linux-2.6.29/arch/powerpc/boot/dts/virtex440-ml510.dts
A bunch of oddities in this .dts file; but that's not your fault. The
xilinx device tree generator needs some TLC.
> +/dts-v1/;
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "xlnx,ml510-ref-design";
I still haven't decided how best to handle the board level compatible
value for the Virtex, but this will do for now.
> + plbv46_pci_0: plbv46-...@85e00000 {
> + #size-cells = <2>;
> + #address-cells = <3>;
> + compatible = "xlnx,plbv46-pci-1.03.a";
> + device_type = "pci";
> + reg = < 0x85e00000 0x10000 >; /* addr is at +0x10c,
> data at +0x110
> */
> +
> + /* The PCI bus is implemented by a soft-core which is
> connected to
> the PLB bus which is seen by the CPU at 0xa0000000.
> + * Both the PLB and PCI have their own address
> domain. The PCI
> soft-core performs this translation. The Xilinx plbpci doc
> + * mentions 'the number of high-order bits
> substituted in the PLB
> address presented to the bridge is given by the number
> + * of bits that are the SAME between C_IPIFBAR_N and
> C_IPIF_HIGHADDR_N.'
> + *
> + * For the default ml510_bsb1_pcores_ppc440 reference
> design this
> means:
> + * C_IPIFBAR_0 = 0xa0000000
> + * C_IPIF_HIGHADDR_0 = 0xbfffffff <- only the last 3
> bits of
> (0xa=1010b, 0xb=1011b) are similar
> + * C_IPIFBAR2PCIBAR_0 = 0x00000000
> + *
> + * C_IPIFBAR_1 = 0x94000000
> + * C_IPIF_HIGHADDR_1 = 0x97ffffff <- only the last 6
> bits are
> similar
> + * C_IPIFBAR2PCIBAR_1 = 0x00000000
> + *
> + * This means that a CPU write to 0xa0001234
> translates to
> 0x00001234 on the PCI bus and that
> + * the pcibar_0 base and pcibar_1 base are zero. In
> order to prevent
> collision between inbound
> + * and outbound memory reads/writes
> C_IPIFBAR2PCIBAR_0 needs to be
> set to 0x80000000.
> + */
Line lengths, especially in comment blocks, should be restricted to 80
characters.
This description really belongs in the device tree binding
documentation in Documentation/powerpc/dts-bindings because it is not
ml510 specific.
> + ranges = <0x02000000 0x00000000 0x80000000 0xa0000000
> 0x00000000
> 0x20000000
I think we've been over this before, but why do the PLB and PCI
addresses differ here? For mem regions I:q
t is legal for them to be different, but things are simpler if they match.
> + 0x01000000 0x00000000 0x00000000 0x94000000
> 0x00000000
> 0x00010000>;
> +
> + #interrupt-cells = <1>;
> + interrupt-parent = <&xps_intc_0>;
> + interrupt-map-mask = <0xff00 0x0 0x0 0x7>;
> + interrupt-map = <
> + /* IDSEL 0x15 / dev=5, bus=0 / PCI slot 5 */
> + /* pci irq a is connected to xintc irq 5, b
> to 4, c to 3 and d to 2
> */
> + /* According to the datasheet + schematic
> ABCD [FPGA] of slot 5 is
> mapped to DABC, testing showed that at least A maps to B */
> + 0x2800 0 0 1 &xps_intc_0 4 2
> +// 0x2800 0 0 2 &xps_intc_0 5 2
> +// 0x2800 0 0 3 &xps_intc_0 4 2
> +// 0x2800 0 0 4 &xps_intc_0 3 2
> +
> + /* IDSEL 0x16 / dev=6, bus=0 / PCI slot 3 */
> + 0x3000 0 0 1 &xps_intc_0 3 2
> + 0x3000 0 0 2 &xps_intc_0 2 2
> + 0x3000 0 0 3 &xps_intc_0 5 2
> + 0x3000 0 0 4 &xps_intc_0 4 2
> +
> + /* IDSEL 0x11 / dev=1, bus=0 / AC97 audio */
> + 0x0800 0 0 1 &i8259 7 2
> +
> + /* IDSEL 0x1b / dev=11, bus=0 / IDE */
> + 0x5800 0 0 1 &i8259 14 2
> +
> + /* IDSEL 0x1f / dev 15, bus=0 / USB */
> + 0x7800 0 0 1 &i8259 7 2
What about the other ALI devices? May as well fill those in too.
> + >;
> + ali_m1533 {
> + #size-cells = <1>;
> + #address-cells = <2>;
> + i8259: interrupt-control...@20 {
> + reg = <1 0x20 2
> + 1 0xa0 2
> + 1 0x4d0 2>;
> + interrupt-controller;
> + device_type = "interrupt-controller";
You should be able to drop device_type here
> + #address-cells = <0>;
> + #interrupt-cells = <2>;
> + compatible = "chrp,iic";
> +
> + interrupts = <1 2>;
> + interrupt-parent = <&xps_intc_0>;
> + };
> + };
> + } ;
> + xps_bram_if_cntlr_1: xps-bram-if-cn...@ffff0000 {
> + compatible = "xlnx,xps-bram-if-cntlr-1.00.a";
> + reg = < 0xffff0000 0x10000 >;
> + xlnx,family = "virtex5";
> + } ;
> + xps_intc_0: interrupt-control...@81800000 {
> + #interrupt-cells = <0x2>;
> + compatible = "xlnx,xps-intc-1.00.a";
> + interrupt-controller ;
> + reg = < 0x81800000 0x10000 >;
> + xlnx,num-intr-inputs = <0xc>;
> + } ;
> + } ;
> +} ;
> linux-2.6.29/arch/powerpc/configs/44x/virtex5_defconfig
> ml510-dev/linux-2.6.29/arch/powerpc/configs/44x/virtex5_defconfig
> --- linux-2.6.29/arch/powerpc/configs/44x/virtex5_defconfig 2009-03-24
> 00:12:14.000000000 +0100
> +++ ml510-dev/linux-2.6.29/arch/powerpc/configs/44x/virtex5_defconfig
Put the defconfig changes into a separate patch.
> diff -urN -X linux-2.6.29/Documentation/dontdiff
> linux-2.6.29/arch/powerpc/platforms/44x/Kconfig
> ml510-dev/linux-2.6.29/arch/powerpc/platforms/44x/Kconfig
> --- linux-2.6.29/arch/powerpc/platforms/44x/Kconfig 2009-03-24
> 00:12:14.000000000 +0100
> +++ ml510-dev/linux-2.6.29/arch/powerpc/platforms/44x/Kconfig 2009-03-27
> 15:50:28.000000000 +0100
> @@ -160,6 +160,16 @@
> Most Virtex 5 designs should use this unless it needs to do some
> special configuration at board probe time.
>
> +config XILINX_ML510
> + bool "Xilinx ML510 Reference Design support"
> + depends on 44x
> + default n
> + select XILINX_VIRTEX_5_FXT
> + select PPC_PCI_CHOICE
> + select XILINX_VIRTEX_PCI if PCI
> + select PPC_INDIRECT_PCI if PCI
> + select PPC_I8259 if PCI
> +
> config PPC44x_SIMPLE
> bool "Simple PowerPC 44x board support"
> depends on 44x
> @@ -232,4 +242,3 @@
> config XILINX_VIRTEX_5_FXT
> bool
> select XILINX_VIRTEX
> -
^^^^^^^^^^^^^^^^^^^^^^
unrelated whitespace change
> diff -urN -X linux-2.6.29/Documentation/dontdiff
> linux-2.6.29/arch/powerpc/platforms/44x/Makefile
> ml510-dev/linux-2.6.29/arch/powerpc/platforms/44x/Makefile
> --- linux-2.6.29/arch/powerpc/platforms/44x/Makefile 2009-03-24
> 00:12:14.000000000 +0100
> +++ ml510-dev/linux-2.6.29/arch/powerpc/platforms/44x/Makefile
> 2009-04-01 11:59:28.000000000 +0200
> @@ -4,3 +4,4 @@
> obj-$(CONFIG_SAM440EP) += sam440ep.o
> obj-$(CONFIG_WARP) += warp.o
> obj-$(CONFIG_XILINX_VIRTEX_5_FXT) += virtex.o
> +obj-$(CONFIG_XILINX_ML510) += ml510.o
> \ No newline at end of file
^^^^^^^^^^^^^^^^^^^^^^^
Fix this.
> diff -urN -X linux-2.6.29/Documentation/dontdiff
> linux-2.6.29/arch/powerpc/platforms/44x/ml510.c
> ml510-dev/linux-2.6.29/arch/powerpc/platforms/44x/ml510.c
> --- linux-2.6.29/arch/powerpc/platforms/44x/ml510.c 1970-01-01
> 01:00:00.000000000 +0100
> +++ ml510-dev/linux-2.6.29/arch/powerpc/platforms/44x/ml510.c 2009-04-01
> 12:42:28.000000000 +0200
> @@ -0,0 +1,161 @@
> +/*
> + * Xilinx ML510 Reference Design support, derived from
> + * the generic Xilinx Virtex 5 board support
> + *
> + * Copyright 2007 Secret Lab Technologies Ltd.
> + * Copyright 2008 Xilinx, Inc.
> + * Copyright 2009 Roderick Colenbrander
> + *
> + * The i8259 cascade code was derived from 86xx/pic.c which is
> copyrighted by Freescale Semiconductor, Inc.
> + * Xilinx ML510 PCI initialization code, derived from the Xilinx
> ML300/ML410 based board support.
Keep line lengths under 80 characters.
> + *
> + * 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.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/of_platform.h>
> +#include <asm/machdep.h>
> +#include <asm/prom.h>
> +#include <asm/time.h>
> +#include <asm/xilinx_intc.h>
> +#include <asm/reg.h>
> +#include <asm/ppc4xx.h>
> +#ifdef CONFIG_PPC_I8259
> +#include <asm/i8259.h>
> +#endif
> +#ifdef CONFIG_PCI
> +#include <linux/pci.h>
> +#endif
> +#include "44x.h"
> +
> +static struct of_device_id xilinx_of_bus_ids[] __initdata = {
> + { .compatible = "simple-bus", },
> + { .compatible = "xlnx,plb-v46-1.00.a", },
> + { .compatible = "xlnx,plb-v46-1.02.a", },
> + { .compatible = "xlnx,plb-v34-1.01.a", },
> + { .compatible = "xlnx,plb-v34-1.02.a", },
> + { .compatible = "xlnx,opb-v20-1.10.c", },
> + { .compatible = "xlnx,dcr-v29-1.00.a", },
> + { .compatible = "xlnx,compound", },
> + {}
> +};
> +
> +#ifdef CONFIG_PPC_I8259
> +static void ml510_8259_cascade(unsigned int irq, struct irq_desc *desc)
> +{
> + unsigned int cascade_irq = i8259_irq();
> + if (cascade_irq != NO_IRQ)
> + generic_handle_irq(cascade_irq);
> +
> + /* Let xilinx_intc end the interrupt */
> + desc->chip->ack(irq);
> + desc->chip->unmask(irq);
> +}
> +
> +static void __init ml510_setup_i8259_cascade(void)
> +{
> + struct device_node *np, *cascade_node = NULL;
> + int cascade_irq;
> +
> + /* Initialize i8259 controller */
> + for_each_node_by_type(np, "interrupt-controller")
> + if (of_device_is_compatible(np, "chrp,iic")) {
> + cascade_node = np;
> + break;
> + }
> +
> + if (cascade_node == NULL) {
> + printk(KERN_DEBUG "Could not find i8259 PIC\n");
> + return;
> + }
> +
> + cascade_irq = irq_of_parse_and_map(cascade_node, 0);
> + if (cascade_irq == NO_IRQ) {
> + printk(KERN_ERR "Failed to map cascade interrupt\n");
> + return;
> + }
> +
> + i8259_init(cascade_node, 0);
> +
> + of_node_put(cascade_node);
> + set_irq_chained_handler(cascade_irq, ml510_8259_cascade);
> +}
> +#endif /* CONFIG_PPC_I8259 */
> +
> +#ifdef CONFIG_PCI
> +static void __devinit ali_quirk(struct pci_dev *dev)
> +{
> + /* Enable the IDE controller */
> + pci_write_config_byte(dev, 0x58, 0x4c);
> + /* Assign irq 14 to the primary ide channel */
> + pci_write_config_byte(dev, 0x44, 0x0d);
> + /* Assign irq 15 to the secondary ide channel */
> + pci_write_config_byte(dev, 0x75, 0x0f);
> + /* Set the ide controller in native mode */
> + pci_write_config_byte(dev, 0x09, 0xff);
> +
> + pci_write_config_byte(dev, 0x48, 0x00); // INTB = disabled, INTA =
> disabled
> + pci_write_config_byte(dev, 0x4a, 0x00); // INTD = disabled, INTC =
> disabled
> + pci_write_config_byte(dev, 0x4b, 0x00); // Audio = INT7, Modem =
> disabled.
> + pci_write_config_byte(dev, 0x74, 0x06); // USB = INT7
> +}
> +DECLARE_PCI_FIXUP_EARLY(0x10b9, 0x1533, ali_quirk);
> +#endif /* CONFIG_PCI */
> +
> +static int __init ml510_device_probe(void)
> +{
> + of_platform_bus_probe(NULL, xilinx_of_bus_ids, NULL);
> +
> + return 0;
> +}
> +machine_device_initcall(ml510, ml510_device_probe);
> +
> +static int __init ml510_probe(void)
> +{
> + unsigned long root = of_get_flat_dt_root();
> +
> + if (!of_flat_dt_is_compatible(root, "xlnx,ml510-ref-design"))
> + return 0;
> +
> + return 1;
or just: return of_flat_dt_is_compatible(root, "xlnx,ml510-ref-design");
> +}
> +
> +void virtex_pci_init(void);
> +static void __init ml510_setup_arch(void)
> +{
> + struct device_node *pci_node = of_find_compatible_node(NULL, NULL,
> "xlnx,plbv46-pci-1.03.a");
> +
> +#ifdef CONFIG_PCI
> + if(pci_node)
> + {
> +//Is this the right way or should this be done using OF?
> + /* Register the host bridge */
> + virtex_pci_init();
> + }
No, you don't need to use of_platform bus here. Doing it with
discrete reads of the device tree is fine. However, you should move
the of_find_compatible_node() call into virtex_pci_init() itself.
Oh, and don't use '//' style comments.
> +#endif /* CONFIG_PCI */
> +}
> +
> +static void ml510_init_IRQ(void)
> +{
> + xilinx_intc_init_tree();
> +
> +#ifdef CONFIG_PPC_I8259
> + /* The devices on the ALI M1553 south bridge are connected to an
> internal i8259 */
> + ml510_setup_i8259_cascade();
> + /* Program irq 7 (usb/audio), 14/15 (ide) to level sensitive */
> + outb(0xc0, 0x4d0);
> + outb(0xc0, 0x4d1);
> +#endif /* CONFIG_PPC_I8259 */
> +}
> +
> +define_machine(ml510) {
> + .name = "Xilinx ML510 Reference Design support",
> + .probe = ml510_probe,
> + .setup_arch = ml510_setup_arch,
> + .init_IRQ = ml510_init_IRQ,
> + .get_irq = xilinx_intc_get_irq,
> + .calibrate_decr = generic_calibrate_decr,
> + .restart = ppc4xx_reset_system,
> +};
> diff -urN -X linux-2.6.29/Documentation/dontdiff
> linux-2.6.29/arch/powerpc/sysdev/Kconfig
> ml510-dev/linux-2.6.29/arch/powerpc/sysdev/Kconfig
> --- linux-2.6.29/arch/powerpc/sysdev/Kconfig 2009-03-24
> 00:12:14.000000000 +0100
> +++ ml510-dev/linux-2.6.29/arch/powerpc/sysdev/Kconfig 2009-03-27
> 12:50:29.000000000 +0100
> @@ -12,3 +12,7 @@
> depends on PCI_MSI
> default y if MPIC
> default y if FSL_PCI
> +
> +config XILINX_VIRTEX_PCI
> + bool
> + depends on PCI
> diff -urN -X linux-2.6.29/Documentation/dontdiff
> linux-2.6.29/arch/powerpc/sysdev/Makefile
> ml510-dev/linux-2.6.29/arch/powerpc/sysdev/Makefile
> --- linux-2.6.29/arch/powerpc/sysdev/Makefile 2009-03-24
> 00:12:14.000000000 +0100
> +++ ml510-dev/linux-2.6.29/arch/powerpc/sysdev/Makefile 2009-03-27
> 11:57:45.000000000 +0100
> @@ -34,6 +34,7 @@
> obj-$(CONFIG_4xx) += uic.o
> obj-$(CONFIG_4xx_SOC) += ppc4xx_soc.o
> obj-$(CONFIG_XILINX_VIRTEX) += xilinx_intc.o
> +obj-$(CONFIG_XILINX_VIRTEX_PCI) += virtex_pci.o
> obj-$(CONFIG_OF_RTC) += of_rtc.o
> ifeq ($(CONFIG_PCI),y)
> obj-$(CONFIG_4xx) += ppc4xx_pci.o
> diff -urN -X linux-2.6.29/Documentation/dontdiff
> linux-2.6.29/arch/powerpc/sysdev/virtex_pci.c
> ml510-dev/linux-2.6.29/arch/powerpc/sysdev/virtex_pci.c
> --- linux-2.6.29/arch/powerpc/sysdev/virtex_pci.c 1970-01-01
> 01:00:00.000000000 +0100
> +++ ml510-dev/linux-2.6.29/arch/powerpc/sysdev/virtex_pci.c 2009-04-01
> 12:54:42.000000000 +0200
> @@ -0,0 +1,95 @@
> +/*
> + * PCI support for Xilinx plbv46_pci soft-core which can be used on
> Xilinx Virtex ML410 / ML510 boards.
> + *
> + * Copyright 2009 Roderick Colenbrander
> + *
> + * The pci bridge fixup code was copied from ppc4xx_pci.c and was
> written by Benjamin Herrenschmidt.
> + * Copyright 2007 Ben. Herrenschmidt <b...@kernel.crashing.org>, IBM
> Corp.
> + *
> + * 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.
> + */
> +
> +#include <linux/pci.h>
> +#include <mm/mmu_decl.h>
> +#include <asm/io.h>
> +
> +#define XPLB_PCI_ADDR 0x10c
> +#define XPLB_PCI_DATA 0x110
> +#define XPLB_PCI_BUS 0x114
> +
> +#define PCI_HOST_ENABLE_CMD PCI_COMMAND_SERR | PCI_COMMAND_PARITY |
> PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY
> +
> +static void fixup_virtex_pci_bridge(struct pci_dev *dev)
> +{
> + struct pci_controller *hose;
> + int i;
> +
> + if (dev->devfn != 0 || dev->bus->self != NULL)
> + return;
Could simply be: if (dev->devfn || dev->bus->self)
> +
> + hose = pci_bus_to_host(dev->bus);
> + if (hose == NULL)
if (!hose)
> + return;
> +
> + if(!of_device_is_compatible(hose->dn, "xlnx,plbv46-pci-1.03.a"))
> + return;
> +
> + /* Hide the PCI host BARs from the kernel as their content doesn't
> + * fit well in the resource management
> + */
> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
> + dev->resource[i].start = dev->resource[i].end = 0;
Chaining up start = end = 0 like this is discouraged. Use one
assignment per line.
> + dev->resource[i].flags = 0;
> + }
> +
> + printk(KERN_INFO "PCI: Hiding Xilinx plb-pci host bridge resources %s
> \n",
> + pci_name(dev));
replace printk(KERN_INFO "..."); with dev_info();
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID,
> fixup_virtex_pci_bridge);
> +
> +void virtex_pci_init(void)
> +{
> + struct device_node *pci_node = of_find_compatible_node(NULL, NULL,
> "xlnx,plbv46-pci-1.03.a");
> +
> + if(pci_node)
> + {
if (!pci_node)
return;
> + struct pci_controller *hose;
> + struct resource r;
> + void __iomem *pci_reg;
> +
> + printk("Found a Xilinx plb-pci host bridge\n");
ditto here
> +
> + if(of_address_to_resource(pci_node, 0, &r))
> + {
> + printk("No address for Xilinx plb-pci host bridge\n");
> + return;
> + }
> +
> + hose = pcibios_alloc_controller(pci_node);
> + if (!hose)
> + return;
> +
> + hose->first_busno = 0;
> + hose->last_busno = 1; /* there are two slots behind a TI2250
> pci-to-pci bridge */
> +
> + /* Setup config space */
> + setup_indirect_pci(hose, r.start + XPLB_PCI_ADDR, r.start +
> XPLB_PCI_DATA, 0);
> +
> + /* According to the xilinx plbv46_pci documentation the
> soft-core
> starts a self-init when the bus master enable bit is set.
> + * Without this bit set the pci bus can't be scanned. */
> + early_write_config_word(hose, 0, 0, PCI_COMMAND,
> PCI_HOST_ENABLE_CMD);
> +
> + /* Set the max latency timer to 255 */
> + early_write_config_byte(hose, 0, 0, PCI_LATENCY_TIMER, 0xff);
> +
> + /* Set the max bus number to 255 */
> + pci_reg = of_iomap(pci_node, 0);
> + out_8(pci_reg + XPLB_PCI_BUS, 0xff);
> + iounmap(pci_reg);
> +
> + /* Register the host bridge with the linux kernel! */
> + pci_process_bridge_OF_ranges(hose, pci_node, 1 /* primary=yes
> */);
Drop the comment in the parameters. Readers will go and look at the
declaration of pci_process_bridge_OF_ranges()
> + }
> +}
> diff -urN -X linux-2.6.29/Documentation/dontdiff
> linux-2.6.29/drivers/ide/alim15x3.c
> ml510-dev/linux-2.6.29/drivers/ide/alim15x3.c
> --- linux-2.6.29/drivers/ide/alim15x3.c 2009-03-24 00:12:14.000000000
> +0100
> +++ ml510-dev/linux-2.6.29/drivers/ide/alim15x3.c 2009-03-27
> 14:47:50.000000000 +0100
> @@ -401,7 +401,8 @@
> return cbl;
> }
>
> -#if !defined(CONFIG_SPARC64) && !defined(CONFIG_PPC)
> +#if !defined(CONFIG_SPARC64)
> +// && !defined(CONFIG_PPC)
heh, I think you know what you need to do here.
> /**
> * init_hwif_ali15x3 - Initialize the ALI IDE x86 stuff
> * @hwif: interface to configure
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
