On Fri, Jan 3, 2014 at 1:49 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Monday 23 December 2013, Tanmay Inamdar wrote:
>> This patch adds the bindings for X-Gene PCIe driver. The driver resides
>> under 'drivers/pci/host/pcie-xgene.c' file.
>>
>> Signed-off-by: Tanmay Inamdar <tinamdar@xxxxxxx>
>> ---
>> .../devicetree/bindings/pci/xgene-pcie.txt | 43 ++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pci/xgene-pcie.txt b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
>> new file mode 100644
>> index 0000000..d92da4f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
>> @@ -0,0 +1,43 @@
>> +* AppliedMicro X-Gene PCIe interface
>> +
>> +Required properties:
>> +- status: Either "ok" or "disabled".
>> +- device_type: set to "pci"
>> +- compatible: should contain "xgene,pcie" to identify the core.
>> +- reg: base addresses and lengths of the pcie controller configuration
>> + space register.
>> +- #address-cells: set to <3>
>> +- #size-cells: set to <2>
>> +- ranges: ranges for the PCI memory, I/O regions, config and MSI regions
>> +- #interrupt-cells: set to <1>
>> +- interrupt-map-mask and interrupt-map: standard PCI properties
>> + to define the mapping of the PCIe interface to interrupt
>> + numbers.
>> +- clocks: from common clock binding: handle to pci clock.
>> +- clock-names: from common clock binding. Should be "pcieclk".
>> +
>
> Better use an anonymous clock?
Sorry. Can you please elaborate?
>
> The driver also seems to use a phy that is not defined here.
>
>> +Example:
>> +
>> +SoC specific DT Entry:
>> + pcie0: pcie@1f2b0000 {
>> + status = "disabled";
>> + device_type = "pci";
>> + compatible = "xgene,pcie";
>> + #interrupt-cells = <1>;
>> + #size-cells = <2>;
>> + #address-cells = <3>;
>> + reg = < 0x00 0x1f2b0000 0x0 0x00010000>;
>> + ranges = <0x02000000 0x0 0x00000000 0xe0 0x00000000 0x0 0x10000000 /* mem*/
>
> This is an awfully small memory space for a 64 bit machine. Is that a hardware bug you
> are working around? If not, please make it as large as you can to allow for arbitrary
> extension cards with large BARs, at least 4GB.
HW does support a lot more than 256MB. I will change it.
>
> Also, do you support no prefetchable memory?
HW has either IO or Memory regions for mapping device's memory space.
There is no separate prefetchable memory space.
>
>> + 0x01000000 0x0 0x80000000 0xe0 0x80000000 0x0 0x00010000 /* io */
>
> I/O space at 0x80000000? That won't work with a lot of devices. Can you make it start
> at bus address 0?
Ok.
>
>> + 0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */
>
> config space is not normally in the ranges property, and I think you will need
> it in the pcie node itself as a 'reg' property so the code can access it.
pcie-designware.c does it that way. I just followed their implementation.
>
>> + 0x00000000 0x0 0x79000000 0x00 0x79000000 0x0 0x00800000>; /* msi */
>
> Same here. Also I suspect this is a separate irqchip that isn't actually part
> of the PCIe host. If they are separate devices, I'd strongly recommend modeling
> them as separate device-nodes in DT to make reuse easier.
You are right. However MSI irqchip needs PCIe core to map above
mentioned space. I will remove it for now and will send it along with
MSI patch.
>
>> + interrupt-map-mask = <0x0 0x0 0x0 0x7>;
>> + interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>;
>
> Only one IRQ for all devices?
The node represents a port. I believe that Linux framework uses only
one of the legacy IRQs per port. Rest all remain unused. Hence I
removed them. Please correct me if I am wrong.
>
>> + clocks = <&pcie0clk 0>;
>> + clock-names = "pcieclk"
>> + };
>> +
>> +Board specific DT Entry:
>> + &pcie0 {
>> + status = "ok";
>> + };
>
> Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
