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

[Alexander Graf]

On 13.12.2012, at 01:20, Scott Wood wrote:

> 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

*shrug* I'd say we can try and find out when we hit something we really care 
about. In this case I doubt we do.

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

Right. Though I'm not sure how pci hotplug slots would look like in that model. 
I don't think we have PCIDevice structs there yet, but we would still need to 
keep interrupt maps ready.

> 
>> So you're indicating you'd like the below patch?
> 
> I think you pasted a bit more than one patch, but yes.

Yikes. It's way past midnight after all :).

You mean I messed up and pasted more than I wanted or that I should split the 
patch? :)

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

I totally agree with you. I'm just saying that in this particular case the 
logic is easy enough that I don't necessarily see a big chance for a pitfall. 
But I'm not objecting to moving the logic to a single place - it certainly 
makes the relationship situation clearer.

> 
>> address@hidden:/home/agraf/release/qemu> git add hw/ppce500_pci.h
>> address@hidden:/home/agraf/release/qemu> git diff HEAD
>> address@hidden:/home/agraf/release/qemu> git diff HEAD | cat
> 
> What does piping through cat get you?

Piping through cat gets me that I don't get the patch in less, so I can easily 
copy&paste it from my terminal into the email client even though it's bigger 
than my terminal window. Also not using less means that formatting stays 
consistent.


Alex