Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

From: Arnd Bergmann <arnd@arndb.de>
To: Ryder Lee <ryder.lee@mediatek.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-pci <linux-pci@vger.kernel.org>,
	devicetree@vger.kernel.org, linux-mediatek@lists.infradead.org,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Red Hung <red.hung@mediatek.com>
Subject: Re: [PATCH v3 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe
Date: Wed, 10 May 2017 12:01:49 +0200	[thread overview]
Message-ID: <CAK8P3a1AUsUTNDnN+_9251sxC7R4vvkQRr37dvAke6ooCJ_oxg@mail.gmail.com> (raw)
In-Reply-To: <1494408707.31307.32.camel@mtkswgap22>

On Wed, May 10, 2017 at 11:31 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> On Wed, 2017-05-10 at 10:08 +0200, Arnd Bergmann wrote:
>> On Wed, May 10, 2017 at 4:07 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
>>
>> > +- ranges:
>> > +  - The first three entries are expected to translate the addresses for the root
>> > +    port registers, which are referenced by the assigned-addresses property of
>> > +    the root port nodes (see below).
>>
>> I don't understand this part. Why do you need a static translation for these?
>> Shouldn't they just be listed in the 'reg' property of the parent node now that
>> you have the clk/reset/phy properties in the parent as well?
>
> At first, I did like that. But I noticed that someone suggest it's
> better to use 'assigned-addresses' to handle per-port registers, the
> same path as tegra and marvell did, in other platform discussion thread.
> So I just put shared register in root node. It could be rolled back if
> you feel this is inappropriate.

The marvell case is not a good example for your case: their top-level
device is made up by the OS to help with the shared resource allocation,
while in your case the bus bridge actually exists in hardware.

I'm not too familiar with the Tegra case, and haven't looked at that here,
but it could be an artifact of how for a while we used to list the config
space access in the top-level "ranges" instead of the "reg" property.

I'd vote for moving it back, for consistency with the other port specific
properties that are now in the root node. Once you do that, the port
nodes can be removed completely, which is what I was aiming for with
the comments on the previous version.

>> > +Required properties:
>> > +- device_type: Must be "pci"
>> > +- assigned-addresses: Address and size of the port configuration registers
>> > +- reg: Only the first four bytes are used to refer to the correct bus number
>> > +  and device number.
>> > +- #address-cells: Must be 3
>> > +- #size-cells: Must be 2
>> > +- #interrupt-cells: Must be 1
>> > +- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties
>> > +  Please refer to the standard PCI bus binding document for a more detailed
>> > +  explanation.
>>
>> Child nodes do not normally have interrupt-map properties. Isn't this
>> already covered by the interrupt-map in the parent?
>>
>
> I have one Intel 4 port ethernet card(0000:00:01) and MTK WLAN card
> (0000:00:02), probe message looks good to me.
>
> pci 0000:00:01.0: fixup irq: got 224
> pci 0000:00:01.0: assigning IRQ 224
> pci 0000:00:02.0: fixup irq: got 225
> pci 0000:00:02.0: assigning IRQ 225
>
> pci 0000:01:00.0: fixup irq: got 224
> pci 0000:01:00.0: assigning IRQ 224
> pci 0000:01:00.1: fixup irq: got 224
> pci 0000:01:00.1: assigning IRQ 224
> pci 0000:01:00.2: fixup irq: got 224
> pci 0000:01:00.2: assigning IRQ 224
> pci 0000:01:00.3: fixup irq: got 224
> pci 0000:01:00.3: assigning IRQ 224
>
> pci 0000:02:00.0: fixup irq: got 225
> pci 0000:02:00.0: assigning IRQ 225
>
>
> But child nodes without interrupt-map properties:
> It seems incorrect.
>
> pci 0000:00:01.0: fixup irq: got 224
> pci 0000:00:01.0: assigning IRQ 224
> pci 0000:00:02.0: fixup irq: got 225
> pci 0000:00:02.0: assigning IRQ 225
>
> pci 0000:01:00.0: fixup irq: got 223
> pci 0000:01:00.0: assigning IRQ 223

Not entirely sure what happens here, but I guess the problem
is that the 'reg' portion of the parent interrupt-map refers to
the port devices, not the devices attached the devices behind
them.

On a related note, I see that you still list

> +- interrupts: Three interrupt outputs of the controller. Must contain an
> +  entry for each entry in the interrupt-names property.
> +- interrupt-names: Must include the following names
> +  - "pcie-int0"
> +  - "pcie-int1"
> +  - "pcie-int2"

This seems to be an artifact from the older version and should be
removed as the driver correctly ignores the properties now.

      Arnd