Re: [PATCH 24/32] pci: PCIe driver for Marvell Armada 370/XP systems

[Rob Herring <robherring2@gmail.com>]

On 03/07/2013 02:47 PM, Thierry Reding wrote:
> On Thu, Mar 07, 2013 at 01:02:35PM -0700, Jason Gunthorpe wrote:
>> On Thu, Mar 07, 2013 at 08:48:30PM +0100, Thierry Reding wrote:
>>> On Thu, Mar 07, 2013 at 10:49:55AM -0700, Jason Gunthorpe wrote:
>>>> On Thu, Mar 07, 2013 at 09:08:32AM +0100, Thierry Reding wrote:
>>> [...]
>>>>>> Both have various problems, but I think I prefer the first one as it
>>>>>> doesn't conflate the contoller registers and host apertures in a
>>>>>> single ranges..
>>>>>
>>>>> I think a better alternative would be (and this matches what Thomas has
>>>>> said elsewhere) to use something like the first alternative but move the
>>>>> regs property into the pcie@0,X nodes. That would save us from having to
>>>>> index a property in the parent. At least from a DT point of view I find
>>>>> that to be a more consistent representation.
>>>>
>>>> You are thinking a new property 'host-controller-regs' or the like?
>>>
>>> Well, something shorter would be nice, but that's the general idea, yes.
>>> As I mentioned before, for Tegra these registers aren't actually any
>>> controller specific registers but rather a window to access the PCI
>>> configuration space for the root ports.
>>
>> Yes, I understand - but in this DT model configuration space access is
>> a host controller function, not a PCI-device function. Anyhow I was
>> also thinking that by the choice of the name it could do translation
>> from the host-controller scope, not from the bridge scope - so the
>> extra elements in ranges could be avoided as well. Hence the name..
>>
>>> I don't think assigned-addresses is a good fit either. The PCI binding
>>> document is equally specific about it as it is about the reg property.
>>> So in my opinion a separate property would be a better choice. The only
>>> big obstacle is that it needs to be somehow hooked up with the OF core
>>> so that proper address translation can be performed.
>>
>> Yes, agreed.
>>
>> My suggestion is to get the OF experts like GKL/Rob H/etc to weigh in
>> on a preferred approach to this problem with the goal of standardizing
>> across all PCI host drivers. Seems like there are 2 main options
>> (outside regs + regnames/etc or new 'regs' in the bridge) and 1 hacky
>> one (assigned addresses)
> 
> Arnd is already Cc'ed on this thread, adding Grant, Rob and the
> devicetree-discuss ML.
> 
> In a nutshell (since some of the context isn't quoted anymore) the
> problem that we're trying to solve is that some of the embedded SoCs
> require per-root-port registers for configuration. The PCI DT
> specification doesn't make any provisions for this. A few alternatives
> have been discussed so far:

I'm not sure I follow. This is different than the host controller
registers? Why would this not just be multiple entries in the reg property?

Rob

> 
> 	1) Use a "regs" property outside of the root port nodes with
> 	   some mechanism to index into them from within the root port
> 	   nodes. Conceptually somewhat like this:
> 
> 		pcie-controller {
> 			...
> 			regs = <0x80000000 0x00001000
> 			        0x80001000 0x00001000>;
> 
> 			pci@0,1 {
> 				...
> 				port-index = <0>;
> 			};
> 
> 			pci@0,2 {
> 				...
> 				port-index = <1>;
> 			};
> 		};
> 
> 	2) Use a "regs" property inside of the root part nodes, along
> 	   the following lines:
> 
> 		pcie-controller {
> 			...
> 			pci@0,1 {
> 				...
> 				reg = <0x00000800 0 0 0 0>;
> 
> 				regs = <0x80000000 0x00001000>;
> 			};
> 
> 			pci@0,2 {
> 				...
> 				reg = <0x00001000 0 0 0 0>;
> 
> 				regs = <0x80001000 0x00001000>;
> 			};
> 		};
> 
> 	3) Repurpose the "assigned-addresses" property to achieve the
> 	   same. This should work out-of-the-box but isn't a good fit
> 	   because it conflicts with the OF PCI specification which
> 	   defines this property to contain the addresses assigned to
> 	   the base address registers.
> 
> Options 1 and 2 above require changes to the OF core to allow proper
> address translation, but the changes shouldn't be very big.
> 
>>> One possible solution that wouldn't be too hard to implement is to
>>> provide a new function (say of_get_named_address()) similar to
>>> of_get_address() which doesn't get the name of the register property
>>> from the struct of_bus but from a parameter and call that function from
>>> another new function similar to of_address_to_resource() that also gets
>>> the property name from a parameter. I can't think of a better name for
>>> the latter than of_named_address_to_resource(), which is rather long.
>>
>> Seems like a reasonable API, maybe pass in a be32*/length pointer
>> instead of a name to be more flexible?
> 
> There's already __of_address_to_resource() which takes a be32 * and size
> but I thought it might be easier to wrap that to make it easier on the
> drivers to use the API.
> 
> Thierry
>