Re: [PATCH 5/5] xen: arm: handle PCI DT node ranges and interrupt-map properties

From: Julien Grall <julien.grall@linaro.org>
To: Ian Campbell <Ian.Campbell@citrix.com>
Cc: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>,
	Clark Laughlin <clark.laughlin@linaro.org>,
	tim@xen.org, stefano.stabellini@eu.citrix.com,
	xen-devel@lists.xen.org
Subject: Re: [PATCH 5/5] xen: arm: handle PCI DT node ranges and interrupt-map properties
Date: Wed, 18 Feb 2015 14:19:01 +0000	[thread overview]
Message-ID: <54E49F55.2010402@linaro.org> (raw)
In-Reply-To: <1424267412.27775.62.camel@citrix.com>

Hi Ian,

On 18/02/2015 13:50, Ian Campbell wrote:
> On Tue, 2015-02-17 at 17:33 +0000, Julien Grall wrote:
>> Hi Ian,
>>
>> On 24/10/14 10:58, Ian Campbell wrote:
>>> These properties are defined in ePAPR and the OpenFirmware PCI Bus Binding
>>> Specification (IEEE Std 1275-1994).
>>>
>>> This replaces the xgene specific mapping. Tested on Mustang and on a model with
>>> a PCI virtio controller.
>>
>> I'm wondering why you choose to map everything at Xen boot time rather
>> than implementing PHYSDEVOP_pci_device_add to do the job?
>
> Does pci_device_add contain sufficient information to do so?

Hmmm... for the interrupt the SBDF is enough. Although for the MMIO it 
looks like there is no difference between PCI bars.

> The regions which are being mapped are essentially the PCI host
> controllers MMIO, IO and CFG "windows" which are then consumed by the
> various bars of the devices on the bus.
>
> So mapping based on pci_device_add would require us to go from the SBDF
> to a set of BARS which need mapping, which is a whole lot more complex
> than just mapping all of the resources owned by the root complex through
> to the h/w domain.

I gave a look to the code which parse the host bridge resource (see 
of_pci_get_host_bridge_resources). They seem to re-use to the 
of_translate_* function. Would not it be possible to do the same?

> Or perhaps I've misunderstood what you were suggesting?

I was suggesting to do it via pci_add_device but it looks like it's only 
possible for IRQ not MMIO.

>> This would allow us to re-use most of the interrupts/mmio decoding
>> provided in the device tree library. It would also avoid missing support
>> of cascade ranges/interrupt-map.
>
> I *think* (if I'm remembering right) I decided we don't need to worry
> about cascades of these things because the second level resources are
> all fully contained within the first (top level) one and so with the
> approach I've taken here are all fully mapped already. That's why I made
> this patch stop descending into children when such a "bus node" is
> found.

I don't understand this paragraph, sorry.

The address range you decoded via the PCI bus may be an intermediate 
address which needs to be translated in the physical hardware address.

Regards,

-- 
Julien Grall