Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] PPC: E500: Generate dt pci irq map dynamically

[Scott Wood <scottwood@freescale.com>]

On 12/12/2012 06:04:11 PM, Alexander Graf wrote:
> 
> On 13.12.2012, at 00:43, Scott Wood wrote:
> 
> > On 12/12/2012 05:38:32 PM, Alexander Graf wrote:
> >> On 12.12.2012, at 19:40, Scott Wood wrote:
> >> > On 12/12/2012 08:09:56 AM, Alexander Graf wrote:
> >> >> +    for (slot = first_slot; slot < last_slot; slot++) {
> >> >> +        for (pci_irq = 0; pci_irq < 4; pci_irq++) {
> >> >> +            pci_map[i++] = cpu_to_be32(slot << 11);
> >> >> +            pci_map[i++] = cpu_to_be32(0x0);
> >> >> +            pci_map[i++] = cpu_to_be32(0x0);
> >> >> +            pci_map[i++] = cpu_to_be32(pci_irq + 1);
> >> >> +            pci_map[i++] = cpu_to_be32(mpic);
> >> >> +            pci_map[i++] = cpu_to_be32(((pci_irq + slot) % 4)  
> + 1);
> >> >> +            pci_map[i++] = cpu_to_be32(0x1);
> >> >> +        }
> >> >>     }
> >> >
> >> > It would be nice if the slot-to-IRQ calculation were done in  
> only one place rather than duplicated here.
> >> Sure, what exactly would you suggest to do? :)
> >
> > Have a common function to calculate the IRQ given the slot number,  
> and call that both from here and from mpc85xx_pci_map_irq().
> >
> >> We can move the whole function to ppce500_pci.c.
> >> We could export the function(slot, pci_irq) through the header of  
> ppce500_pci.c.
> >
> > Either works, though I'd lean towards moving this function into  
> ppce500_pci.c.
> 
> Well, I'm not sure Anthony would be happy about that. He wanted to  
> keep device tree generation inside the machine files.

Sigh.  I don't understand the hostility to device tree generation, to  
the point of enforcing unnatural code grouping and possibly even  
duplication.

> But this one might be an exception, because it's not a generic device.

So what happens when we do have a generic device?  Duplicate the code  
in every machine that uses it, or have a parallel "hw/device_dt.c" (or  
maybe some better hidden place) to factor out common code while (sort  
of) complying with Anthony's mandate? :-P

> >> We could also try and traverse the pci bus to find the function  
> that is actually called to convert irq numbers internally, so we  
> potentially support other pci host controllers.
> >
> > Not sure what you mean here.
> 
> We could call bus->map_irq(...) with an artificially created  
> PCIDevice struct ;). But that's pretty hacky.

If we do anything like that, it should probably be to iterate over the  
devices that actually exist and add interrupt-map entries only for  
those.

> So you're indicating you'd like the below patch?

I think you pasted a bit more than one patch, but yes.

> Do you think it's worth the additional code for a simple + and & 3?

It's not about duplicating "+ and & 3" so much as having only one place  
where the relationship is defined, in case someone wants to alter it  
(e.g. for adding some other board where the mapping is done  
differently).

> agraf@lychee:/home/agraf/release/qemu> git add hw/ppce500_pci.h
> agraf@lychee:/home/agraf/release/qemu> git diff HEAD
> agraf@lychee:/home/agraf/release/qemu> git diff HEAD | cat

What does piping through cat get you?

-Scott