[PATCH v3 03/10] dt-bindings: PCI: Add ASPEED PCIe RC support

[Jacky Chou]

Hi Mani

Thank you for your reply.

> > > +
> > > +  aspeed,pciecfg:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      Phandle to the ASPEED PCIe configuration syscon node.
> > > +      This reference allows the PCIe controller to access
> > > +      SoC-specific PCIe configuration registers. There are the others
> > > +      functions such PCIe RC and PCIe EP will use this common register
> > > +      to configure the SoC interfaces.
> > > +
> 
> So these config registers are part of the PCIe domain? Is so, accessing them as
> syscon is wrong. You should configure the registers directly from the RC and EP
> controller drivers.
> 

Yes, it is part of the PCIe domain, but some drivers will use the common registers, if
I do not use syscon, how do I add the register range to node?
Use the "reg" property in all pcie node?

> > > +  interrupt-controller:
> > > +    description: Interrupt controller node for handling legacy PCI
> interrupts.
> 
> s/legacy PCI interrupts/INTx
> 

Agreed

> > > +    type: object
> > > +    properties:
> > > +      '#address-cells':
> > > +        const: 0
> > > +      '#interrupt-cells':
> > > +        const: 1
> > > +      interrupt-controller: true
> > > +
> > > +    required:
> > > +      - '#address-cells'
> > > +      - '#interrupt-cells'
> > > +      - interrupt-controller
> > > +
> > > +    additionalProperties: false
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/pci/pci-host-bridge.yaml#
> > > +  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: aspeed,ast2600-pcie
> > > +    then:
> > > +      required:
> > > +        - aspeed,ahbc
> > > +    else:
> > > +      properties:
> > > +        aspeed,ahbc: false
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: aspeed,ast2700-pcie
> > > +    then:
> > > +      required:
> > > +        - aspeed,pciecfg
> > > +    else:
> > > +      properties:
> > > +        aspeed,pciecfg: false
> > > +
> > > +required:
> > > +  - reg
> > > +  - interrupts
> > > +  - bus-range
> > > +  - ranges
> > > +  - resets
> > > +  - reset-names
> > > +  - msi-parent
> > > +  - msi-controller
> > > +  - interrupt-map-mask
> > > +  - interrupt-map
> > > +  - interrupt-controller
> > > +
> > > +unevaluatedProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/clock/ast2600-clock.h>
> > > +
> > > +    apb {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <1>;
> >
> > No need to show this node.
> >

Agreed.

> > > +
> > > +      pcie0: pcie at 1e770000 {
> > > +        compatible = "aspeed,ast2600-pcie";
> > > +        device_type = "pci";
> > > +        reg = <0x1e770000 0x100>;
> > > +        linux,pci-domain = <0>;
> > > +        #address-cells = <3>;
> > > +        #size-cells = <2>;
> > > +        interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>;
> > > +        bus-range = <0x80 0xff>;
> 
> Why bus number starts from 128?
> 

On AST2600, the root bus must start from 128. It is fixed on design.

> > > +
> > > +        ranges = <0x01000000 0x0 0x00018000 0x00018000 0x0
> 0x00008000
> > > +            0x02000000 0x0 0x70000000 0x70000000 0x0
> 0x10000000>;
> > > +
> > > +        status = "disabled";
> >
> > Examples should be enabled. Drop.
> >
> > > +
> > > +        resets = <&syscon ASPEED_RESET_H2X>;
> > > +        reset-names = "h2x";
> > > +
> > > +        #interrupt-cells = <1>;
> > > +        msi-parent = <&pcie0>;
> >
> > There shouldn't be any need to point to yourself.
> >

Agreed.

> > > +        msi-controller;
> > > +
> > > +        aspeed,ahbc = <&ahbc>;
> > > +
> > > +        interrupt-map-mask = <0 0 0 7>;
> > > +        interrupt-map = <0 0 0 1 &pcie_intc0 0>,
> > > +                        <0 0 0 2 &pcie_intc0 1>,
> > > +                        <0 0 0 3 &pcie_intc0 2>,
> > > +                        <0 0 0 4 &pcie_intc0 3>;
> > > +        pcie_intc0: interrupt-controller {
> > > +          interrupt-controller;
> > > +          #address-cells = <0>;
> > > +          #interrupt-cells = <1>;
> > > +        };
> > > +
> > > +        pcie at 8,0 {
> > > +          reg = <0x804000 0 0 0 0>;
> 
> Why the device number starts from 8?
> 
> If there are platform specific reasons behind this numbering scheme, it should
> be mentioned in the description.
> 

Same as above. On AST2600, the device number of root port must be 8.
Will add this information in the description in next version.

Thanks,

Jacky