Re: [PATCH] Preliminary MPIC MSI backend

Re: [PATCH] Preliminary MPIC MSI backend

[Segher Boessenkool]

> A pretty hackish MPIC backend, just enough to flesh out the design.
> Based on code from Segher.

It's pretty alright, and very hackish ;-)  I'll sign off on it,
but some comments...

Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>

> +static int msi_mpic_check(struct pci_dev *pdev, int num,
> +			struct msix_entry *entries, int type)
> +{
> +	/* The irq allocator needs more work to support MSI-X/multi-MSI */
> +	if (type == PCI_CAP_ID_MSIX || num != 1)
> +		return 1;

I never tested any MSI-X, so maybe keep MSI-X disabled completely
for now?

> +static int msi_mpic_alloc(struct pci_dev *pdev, int num,
> +			struct msix_entry *entries, int type)
> +{
> +	irq_hw_number_t hwirq;
> +	unsigned int virq;
> +
> +	/* We need a smarter allocator for MSI-X/multi-MSI */
> +	hwirq = irq_map[pdev->irq].hwirq;
> +	hwirq += 100;

Yep, that's the main problem with this code.  A sanity check to
make sure the number isn't >= 120 would be good, too.

> +	set_irq_type(virq, IRQ_TYPE_EDGE_RISING);

I also had some code to show MSI IRQs as "MSI" instead of "EDGE"
in /proc/interrupts, maybe you want to add a generic version of
that?  Or maybe you have, and I judt didn't see it.

> +static int msi_mpic_setup_msi_msg(struct pci_dev *pdev,
> +		struct msix_entry *entry, struct msi_msg *msg, int type)
> +{
> +	msg->address_lo = 0xfee00000;	/* XXX What is this value? */
> +	msg->address_hi = 0;
> +	msg->data = pdev->irq | 0x8000;

Lose the | 0x8000 part, that was an old experiment to work around
U3/U4 MPIC brokenness (and it didn't work).

> +static int msi_mpic_init(void)
> +{
> +	/* XXX Do this in mpic_init ? */
> +	pr_debug("mpic_msi_init: Registering MPIC MSI ops.\n");
> +	ppc_md.get_msi_ops = mpic_get_msi_ops;

It's best to do this in the platform code I think.


Segher

Re: [PATCH] Preliminary MPIC MSI backend

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2686 bytes --]

On Sat, 2006-09-30 at 10:43 +0200, Segher Boessenkool wrote:
> > A pretty hackish MPIC backend, just enough to flesh out the design.
> > Based on code from Segher.
> 
> It's pretty alright, and very hackish ;-)  I'll sign off on it,
> but some comments...
> 
> Signed-off-by: Segher Boessenkool <segher@kernel.crashing.org>
> > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> 
> > +static int msi_mpic_check(struct pci_dev *pdev, int num,
> > +			struct msix_entry *entries, int type)
> > +{
> > +	/* The irq allocator needs more work to support MSI-X/multi-MSI */
> > +	if (type == PCI_CAP_ID_MSIX || num != 1)
> > +		return 1;
> 
> I never tested any MSI-X, so maybe keep MSI-X disabled completely
> for now?

Yeah it is, that's an ||.

> > +static int msi_mpic_alloc(struct pci_dev *pdev, int num,
> > +			struct msix_entry *entries, int type)
> > +{
> > +	irq_hw_number_t hwirq;
> > +	unsigned int virq;
> > +
> > +	/* We need a smarter allocator for MSI-X/multi-MSI */
> > +	hwirq = irq_map[pdev->irq].hwirq;
> > +	hwirq += 100;
> 
> Yep, that's the main problem with this code.  A sanity check to
> make sure the number isn't >= 120 would be good, too.

Talking to Ben, we decided for the moment we'll just reuse the currently
assigned irq, in the medium term we'll come up with some way to find the
unassigned irqs on mpic and write an allocator.

> > +	set_irq_type(virq, IRQ_TYPE_EDGE_RISING);
> 
> I also had some code to show MSI IRQs as "MSI" instead of "EDGE"
> in /proc/interrupts, maybe you want to add a generic version of
> that?  Or maybe you have, and I judt didn't see it.

I lost that along the way somewhere, I'll try and find it and resurrect
it.

> > +static int msi_mpic_setup_msi_msg(struct pci_dev *pdev,
> > +		struct msix_entry *entry, struct msi_msg *msg, int type)
> > +{
> > +	msg->address_lo = 0xfee00000;	/* XXX What is this value? */
> > +	msg->address_hi = 0;
> > +	msg->data = pdev->irq | 0x8000;
> 
> Lose the | 0x8000 part, that was an old experiment to work around
> U3/U4 MPIC brokenness (and it didn't work).

OK.

> > +static int msi_mpic_init(void)
> > +{
> > +	/* XXX Do this in mpic_init ? */
> > +	pr_debug("mpic_msi_init: Registering MPIC MSI ops.\n");
> > +	ppc_md.get_msi_ops = mpic_get_msi_ops;
> 
> It's best to do this in the platform code I think.

Yeah probably, I'll leave that up to Ben.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 191 bytes --]