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