Thanks for the suggestions. I will try remove the extra lines . Add
changes you suggested.
-Marri

-----Original Message-----
From: linuxppc-dev-bounces+tmarri=3Damcc.com@lists.ozlabs.org
[mailto:linuxppc-dev-bounces+tmarri=3Damcc.com@lists.ozlabs.org] On =
Behalf
Of Stefan Roese
Sent: Wednesday, December 23, 2009 12:19 AM
To: linuxppc-dev@lists.ozlabs.org
Cc: linuxppc-dev@ozlabs.org; writetomarri@yahoo.com; Tirumala Reddy
Marri
Subject: Re: [PATCH 2/2] Adding PCI-E MSI support for PowerPC 460SX SOC.

On Wednesday 23 December 2009 08:52:23 tmarri@amcc.com wrote:

From: Tirumala Marri <redacted>

Please find some mostly nitpicking comments below.

BTW: Did you already test this on other 4xx platforms, like 440SPe or
405EX?=20
What are your plans here?
=20

Signed-off-by: Tirumala Marri <redacted>
---
Kernel version: 2.6.33-rc1
Testing:
	When 460SX configured as root as a end point E1000(Intell

Ethernet card)

	was plugged into the one of the PCI-E ports. I was able to run

the traffic

	with MSI interrupts.
---
 arch/powerpc/boot/dts/redwood.dts          |   15 ++
 arch/powerpc/configs/44x/redwood_defconfig |    5 +-
 arch/powerpc/platforms/44x/Kconfig         |    1 +
 arch/powerpc/sysdev/Kconfig                |    7 +
 arch/powerpc/sysdev/Makefile               |    1 +
 arch/powerpc/sysdev/ppc4xx_msi.c           |  342
 ++++++++++++++++++++++++++++ arch/powerpc/sysdev/ppc4xx_msi.h

|=20

swizzled int C */

 				0x0 0x0 0x0 0x4 &UIC3 0xb 0x4 /*

swizzled int D */>;

 		};
+  		MSI: ppc4xx-msi@400300000 {
+  			compatible =3D "amcc,ppc4xx-msi", "ppc4xx-msi";

Better use something like this:

  			compatible =3D "amcc,ppc4xx-msi-ppc460sx",
"amcc,ppc4xx-msi";

This way you could check for 460SX specials in the driver if needed.

100644

board.

b/arch/powerpc/sysdev/Makefile

Nitpicking: Remove one empty line here.

+static struct ppc4xx_msi *ppc4xx_msi;
+
+struct ppc4xx_msi_feature {
+	u32 ppc4xx_pic_ip;
+	u32 msiir_offset;
+};
+
+static int ppc4xx_msi_init_allocator(struct ppc4xx_msi *msi_data)
+{
+	int rc;
+
+	rc =3D msi_bitmap_alloc(&msi_data->bitmap, NR_MSI_IRQS,
+				msi_data->irqhost->of_node);
+	if (rc)
+		return rc;
+	rc =3D msi_bitmap_reserve_dt_hwirqs(&msi_data->bitmap);
+	if (rc < 0) {
+		msi_bitmap_free(&msi_data->bitmap);
+		return rc;
+	}
+	return 0;
+}
+
+

Nitpicking: Remove one empty line here.

+static void ppc4xx_msi_cascade(unsigned int irq, struct irq_desc

*desc)

+{
+	unsigned int cascade_irq;
+	struct ppc4xx_msi *msi_data =3D ppc4xx_msi;
+	int msir_index =3D -1;
+
+	raw_spin_lock(&desc->lock);
+	if (desc->chip->mask_ack) {
+		desc->chip->mask_ack(irq);
+	} else {
+		desc->chip->mask(irq);
+		desc->chip->ack(irq);
+	}
+
+	if (unlikely(desc->status & IRQ_INPROGRESS))
+		goto unlock;
+
+	msir_index =3D (int)desc->handler_data;
+
+	if (msir_index >=3D NR_MSI_IRQS)
+		cascade_irq =3D NO_IRQ;
+
+	desc->status |=3D IRQ_INPROGRESS;
+
+	cascade_irq =3D irq_linear_revmap(msi_data->irqhost, msir_index);
+	if (cascade_irq !=3D NO_IRQ)
+		generic_handle_irq(cascade_irq);
+	desc->status &=3D ~IRQ_INPROGRESS;
+
+	if (!(desc->status & IRQ_DISABLED) && desc->chip->unmask)
+		desc->chip->unmask(irq);
+unlock:
+	raw_spin_unlock(&desc->lock);
+}

Nitpicking: Add one empty line here.

+static void ppc4xx_compose_msi_msg(struct pci_dev *pdev, int hwirq,
+					struct msi_msg *msg)
+{
+	struct ppc4xx_msi *msi_data =3D ppc4xx_msi;
+
+	msg->address_lo =3D msi_data->msi_addr_lo;
+	msg->address_hi =3D msi_data->msi_addr_hi;
+	msg->data =3D hwirq;
+}
+
+

Nitpicking: Remove one empty line here.

+int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
+{
+	struct msi_desc *entry;
+	int rc, hwirq;
+	unsigned int virq;
+	struct msi_msg msg;
+	struct ppc4xx_msi *msi_data =3D ppc4xx_msi;
+
+
+	list_for_each_entry(entry, &dev->msi_list, list) {
+		hwirq =3D msi_bitmap_alloc_hwirqs(&msi_data->bitmap, 1);
+		if (hwirq < 0) {
+			rc =3D hwirq;
+			dev_err(&dev->dev, "%s: fail allocating msi\
+					interrupt\n",	__func__);
+			goto out_free;
+		}
+
+		pr_debug(KERN_INFO"mis is %p\n", msi_data->irqhost);
+		virq =3D irq_create_mapping(msi_data->irqhost, hwirq);
+		if (virq =3D=3D NO_IRQ) {
+			dev_err(&dev->dev, "%s: fail mapping irq\n",

__func__);

+			rc =3D -ENOSPC;
+			goto out_free;
+		}
+
+		set_irq_msi(virq, entry);
+		ppc4xx_compose_msi_msg(dev, hwirq, &msg);
+		write_msi_msg(virq, &msg);
+	}
+
+	return 0;
+out_free:
+	return rc;
+}
+
+void ppc4xx_teardown_msi_irqs(struct pci_dev *dev)
+{
+	struct msi_desc *entry;
+	struct ppc4xx_msi *msi_data =3D ppc4xx_msi;
+	 dev_dbg(&dev->dev, "PCIE-MSI: tearing down msi irqs\n");
+
+	list_for_each_entry(entry, &dev->msi_list, list) {
+		if (entry->irq =3D=3D NO_IRQ)
+			continue;
+		set_irq_msi(entry->irq, NULL);
+		msi_bitmap_free_hwirqs(&msi_data->bitmap,
+				       virq_to_hw(entry->irq), 1);
+		irq_dispose_mapping(entry->irq);
+

Nitpicking: Remove one empty line here.

+	}
+
+	return;
+}
+
+static int ppc4xx_msi_check_device(struct pci_dev *pdev, int nvec,

int

 type) +{
+	pr_debug(KERN_INFO"PCIE-MSI:%s called. vec %x type %d\n",
+					__func__, nvec, type);
+	return 0;
+}
+
+/*
+ * We do not need this actually. The MSIR register has been read once
+ * in the cascade interrupt. So, this MSI interrupt has been acked
+*/
+static void ppc4xx_msi_end_irq(unsigned int virq)
+{
+}
+
+

Nitpicking: Remove one empty line here. I'll stop mentioning this here.
Please=20
check the whole file for this.

+static struct irq_chip ppc4xx_msi_chip =3D {
+	.mask           =3D mask_msi_irq,
+	.unmask         =3D unmask_msi_irq,
+	.ack            =3D ppc4xx_msi_end_irq,
+	.name       =3D " UIC",
+};
+
+static int ppc4xx_msi_host_map(struct irq_host *h, unsigned int virq,
+				irq_hw_number_t hw)
+{
+	struct irq_chip *chip =3D &ppc4xx_msi_chip;
+
+	irq_to_desc(virq)->status |=3D IRQ_TYPE_EDGE_RISING;
+
+	set_irq_chip_and_handler(virq, chip, handle_edge_irq);
+
+	return 0;
+}
+
+static struct irq_host_ops ppc4xx_msi_host_ops =3D {
+	.map =3D ppc4xx_msi_host_map,
+};
+
+
+static int __devinit ppc4xx_msi_probe(struct of_device *dev,
+					const struct of_device_id

*match)

+{
+	struct ppc4xx_msi *msi;
+	struct resource res, rmsi;
+	int i, count;
+	int rc;
+	int virt_msir;
+	const u32 *p;
+	const u32 *sdr_base;
+	u32 *msi_virt =3D NULL;
+	dma_addr_t msi_phys;
+
+
+	msi =3D kzalloc(sizeof(struct ppc4xx_msi), GFP_KERNEL);
+	if (!msi) {
+		dev_err(&dev->dev, "No memory for MSI structure\n");
+		rc =3D -ENOMEM;
+		goto error_out;
+	}
+
+	msi->irqhost =3D irq_alloc_host(dev->node, IRQ_HOST_MAP_LINEAR,
+				      NR_MSI_IRQS, &ppc4xx_msi_host_ops,

0);

+	if (msi->irqhost =3D=3D NULL) {
+		dev_err(&dev->dev, "No memory for MSI irqhost\n");
+		rc =3D -ENOMEM;
+		goto error_out;
+	}
+
+
+	/* Get MSI ranges */
+	rc =3D of_address_to_resource(dev->node, 0, &rmsi);
+	if (rc) {
+		dev_err(&dev->dev, "%s resource error!\n",
+				dev->node->full_name);
+		goto error_out;
+	}
+
+
+	/* Get the MSI reg base */
+	rc =3D of_address_to_resource(dev->node, 1, &res);
+	if (rc) {
+		dev_err(&dev->dev, "%s resource error!\n",
+				dev->node->full_name);
+		goto error_out;
+	}
+	/* Get the sdr-base */
+	sdr_base =3D (u32 *)of_get_property(dev->node, "sdr-base", NULL);
+	if (sdr_base =3D=3D NULL) {
+		dev_err(&dev->dev, "%s resource error!\n",
+				dev->node->full_name);
+		goto error_out;
+	}
+	msi->sdr_base =3D *sdr_base;
+#if  defined(CONFIG_460SX)
+	mtdcri(SDR0, msi->sdr_base, res.start >> 32);
+	mtdcri(SDR0, msi->sdr_base + 1, res.start & 0xFFFFFFFF);
+	msi->msi_regs =3D ioremap(res.start, res.end - res.start + 1);

	msi->msi_regs =3D ioremap(res.start, resource_size(&res));

+#else
+	dev_err(&dev->dev, " Invalid Device \n");
+	goto error_out;
+#endif

Please don't introduce such a compile-time selection here. You don't
even need=20
it, since the register bases are provided via the device-tree. So just
remove=20
the #if and it's #else part here.

+	if (!msi->msi_regs) {
+		dev_err(&dev->dev, "ioremap problem failed\n");
+		goto error_out;
+	}
+	/* MSI region always mapped in 4GB region*/
+	msi->msi_addr_hi =3D 0x0;
+	msi_virt =3D dma_alloc_coherent(&dev->dev, 64, &msi_phys,
+			GFP_KERNEL);
+	if (msi_virt =3D=3D NULL) {
+		dev_err(&dev->dev, "No memory for MSI mem space\n");
+		rc =3D -ENOMEM;
+		goto error_out;
+	}
+	msi->msi_addr_lo =3D (u32)msi_phys;
+
+	/* Progam the Interrupt handler Termination addr registers */
+	out_be32(msi->msi_regs + PEIH_TERMADH, msi->msi_addr_hi);
+	out_be32(msi->msi_regs + PEIH_TERMADL, msi->msi_addr_lo);
+
+	/* Program MSI Expected data and Mask bits */
+	out_be32(msi->msi_regs + PEIH_MSIED, MSI_DATA_PATTERN);
+	out_be32(msi->msi_regs + PEIH_MSIMK, MSI_DATA_PATTERN);
+
+	msi->irqhost->host_data =3D msi;
+
+	if (ppc4xx_msi_init_allocator(msi)) {
+		dev_err(&dev->dev, "Error allocating MSI bitmap\n");
+		goto error_out;
+	}
+
+	p =3D of_get_property(dev->node, "interrupts", &count);
+	if (!p) {
+		dev_err(&dev->dev, "no interrupts property found on

%s\n",

+				dev->node->full_name);
+		rc =3D -ENODEV;
+		goto error_out;
+	}
+	if (count =3D=3D 0) {
+		dev_err(&dev->dev, "Malformed interrupts property on

%s\n",

+				dev->node->full_name);
+		rc =3D -EINVAL;
+		goto error_out;
+	}
+
+	for (i =3D 0; i < NR_MSI_IRQS; i++) {
+		virt_msir =3D irq_of_parse_and_map(dev->node, i);
+		if (virt_msir !=3D NO_IRQ) {
+			set_irq_data(virt_msir, (void *)i);
+			set_irq_chained_handler(virt_msir,

ppc4xx_msi_cascade);

Cheers,
Stefan

--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev