[PATCH 0/2] ARM: mvebu: fix DT def. of PCIe units for mv78230 and mv78260

[arno@natisbad.org (Arnaud Ebalard)]

Hi Jason,

Jason Gunthorpe <jgunthorpe@obsidianresearch.com> writes:

> On Wed, Nov 06, 2013 at 07:22:33PM +0100, Thomas Petazzoni wrote:
>> Dear Arnaud Ebalard,
>> 
>> On Wed, 06 Nov 2013 19:14:42 +0100, Arnaud Ebalard wrote:
>> 
>> > > As a side note, Thomas, I noticed one more thing, this time in mv78460
>> > > .dtsi when comparing it w/ mv78230 and mv78260 ones. The first address
>> > > of "assigned-address" property varies in the former for each pcie
>> > > node:
>> > >
>> > > $ grep assigned-address armada-xp-mv78230.dtsi 
>> > >          assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x44000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x48000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x4c000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x80000 0 0x2000>;
>> > > $ grep assigned-address armada-xp-mv78260.dtsi 
>> > >          assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x44000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x48000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x4c000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x80000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x84000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x88000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x8c000 0 0x2000>;
>> > >          assigned-addresses = <0x82000800 0 0x42000 0 0x2000>;
>> > > $ grep assigned-address armada-xp-mv78460.dtsi 
>> > >          assigned-addresses = <0x82000800 0 0x40000 0 0x2000>;
>> > >          assigned-addresses = <0x82001000 0 0x44000 0 0x2000>;
>> > >          assigned-addresses = <0x82001800 0 0x48000 0 0x2000>;
>> > >          assigned-addresses = <0x82002000 0 0x4c000 0 0x2000>;
>> > >          assigned-addresses = <0x82002800 0 0x80000 0 0x2000>;
>> > >          assigned-addresses = <0x82003000 0 0x84000 0 0x2000>;
>> > >          assigned-addresses = <0x82003800 0 0x88000 0 0x2000>;
>> > >          assigned-addresses = <0x82004000 0 0x8c000 0 0x2000>;
>> > >          assigned-addresses = <0x82004800 0 0x42000 0 0x2000>;
>> > >          assigned-addresses = <0x82005000 0 0x82000 0 0x2000>;
>> > >
>> > > I took at Documentation/devicetree/bindings/pci/mvebu-pci.txt but
>> > > failed to find an answer. Can you explain where the difference comes
>> > > from?
>> > 
>> > Sorry to bother you again with the one above (it was in the cover
>> > letter) but the difference looks quite odd for someone not familiar
>> > with the syntax and the semantic.
>> 
>> I do still have this question in mind, I wanted to reply to it today
>> but just hadn't had the time to do it. I'm adding Jason Gunthorpe in Cc
>> because he knows better than me the precise meaning of
>> assigned-addresses. If it doesn't reply, I'll try to have a look
>> tomorrow. This is the kind of stuff I can't answer without
>> concentrating for a bit of time to re-understand again how it all fits
>> together :-)
>
> In this context the assigned-addresses encodes the PCI device bus
> function as well as the BAR #.
>
> This PCI stuff would be 100% more readable with some macros ..
>
> #define PCI_REG_BDF(bus,device,function) (....)
> #define PCI_MEM_BAR_BDF(bus,device,function,bar) \
>    (0x82000000 | PCI_REG_BDF(bus,device,function) + bar*XX) 0)
>
> So that:
>  reg = <PCI_REG_BDF(0,1,0) 0 0>
>  assigned-addresses = <PCI_MEM_BAR_BDF(0,1,0,0)  0x40000  0 0x2000>
>
> When done properly the assigned-address and reg should have the same
> BDF. As you've noticed there is a copy and paste mistake in some of
> the dts's:
>
> eg:
>  pcie at 2,0 {
>      assigned-addresses = <0x82000800 0 0x44000 0 0x2000>;
>      reg = <0x1000 0 0 0 0>;
>
> The assigned-addresses in that instance should be 0x82001000 to match
> the reg.
>
> It probably works because the Linux machinery doesn't check the BDF
> value?

ok.

> The purpose of assigned-address in this specific context is to tell
> the driver how to find its control registers. So the 0x44000 above is
> the base for the PEX register. The "pci at 3,0", assigned-addresses, and
> reg are encoding this statement: "Create a PCI bridge at B:D.F using
> the PEX registers at 0x44000" The B:D.F in all three places should be
> the same.

Thanks for the explanation.

> I recommend adding some macros and constants derived from the OF PCI
> spec. It will save us all headaches :)

Too bad it was not that clear in mvebu-pci.txt; I could have taken the
opportunity to fix mv782{30,60} .dtsi in two previous patches.

Cheers,

a+