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
