lkml.org 
[lkml]   [2014]   [Jan]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH 3/3] dt-bindings: pci: xgene pcie device tree bindings
From
On Fri, Jan 3, 2014 at 1:49 AM, Arnd Bergmann <arnd@arndb.de> 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@apm.com>
>> ---
>> .../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


\
 
 \ /
  Last update: 2014-01-07 04:41    [W:0.169 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site