回覆: [PATCH v2 06/10] ARM: dts: aspeed-g6: Add PCIe RC node

[Jacky Chou <jacky_chou@aspeedtech.com>]

> > > >                                 quality = <100>;
> > > >                         };
> > > >
> > > > +                       pcie_phy1: syscon@1e6ed200 {
> > > > +                               compatible = "aspeed,pcie-phy",
> > > "syscon";
> > > > +                               reg = <0x1e6ed200 0x100>;
> > >
> > > This looks like part of something else? It should be a child of that.
> > >
> > > If this is the controls for the PCIe PHY, then use the PHY binding
> > > instead of your own custom phandle property.
> > >
> >
> > Our PCIe design has multiple functions. And the series of patches are
> > submitted for PCIe RC. The other PCIe functions also use this phy node.
> > I traced the PHY driver interface, it cannot meet our usage.
> 
> Why not?
> 
> There is also no requirement that using the DT PHY binding means you have to
> use the Linux PHY subsystem.
> 

Got it. I always focused on when using the "phys" property, I must use the Linux PHY
subsystem. I will change this part to use the "phys" property instead of our definition
property.
Thank you for your comments.

> > Therefore, the RC driver uses the phandle property to configure.
> > And this syscon also is used by the other PCIe functions.
> 
> Like what?
> 

Other PCIe functions such as MCTP also use the PHY interface.

> > > > +                       };
> > > > +
> > > > +                       pcie_cfg: syscon@1e770000 {
> > > > +                               compatible = "aspeed,pcie-cfg",
> > > "syscon";
> > > > +                               reg = <0x1e770000 0x80>;
> > >
> > > Looks like this is really part of the PCIe block as a h/w block
> > > isn't going to start at offset 0xc0.
> > >
> > >
> >
> > Actually.
> > There are two PCIe bus in AST2600
> > We use the other one PCIe to EP mode, here I call PCIe A.
> > I call the pcie0 node as PCIe B.
> > We do not provide PCIe A to RC mode for usage, just EP mode.
> > But, when PCIe A is used as RC, it reg mapping is starting from 0x1e770080.
> > I list there mapping.
> >
> > 0x1e77_0000 ~ 0x1e77_007f : common usage
> > 0x1e77_0080 ~ 0x1e77_00bf : PCIE A
> > 0x1e77_00C0 ~ 0x1e77_00ff : PCIE B
> >
> > So, it is why we create one node to describe common usage for PCIe A and B.
> > And, why the pcie0 reg mapping is starting from 0x1e77_00c0.
> 
> In that case, maybe you need a common parent node with 2 child nodes for
> each bus.

Got it. But we may remove the pcie_cfg node and merge these register regions.

> >
> > > > +                       };
> > > > +
> > > > +                       pcie0: pcie@1e7700c0 {
> > > > +                               compatible =
> "aspeed,ast2600-pcie";
> > > > +                               device_type = "pci";
> > > > +                               reg = <0x1e7700c0 0x40>;
> > > > +                               linux,pci-domain = <0>;
> > >
> > > No need for this. You only have 1 PCI host.
> > >
> >
> > Agreed.
> > We only provide one RC.
> >
> > > > +                               #address-cells = <3>;
> > > > +                               #size-cells = <2>;
> > > > +                               interrupts = <GIC_SPI 168
> > > IRQ_TYPE_LEVEL_HIGH>;
> > > > +                               bus-range = <0x80 0xff>;
> > >
> > > Does this h/w not support bus 0-0x7f for some reason?
> > >
> >
> > List:
> > PCIE A: 0-0x7f
> > PCIE B: 0x80-0xff
> >
> > It is our design on PCIe B to use bus-range 0x80-0xff.
> 
> That's a policy or h/w limitation?
> 

It is a hardware limitation of this PCIe RC.

Thanks,
Jacky