| Message ID | 1492935543-18190-3-git-send-email-ryder.lee@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > Add documentation for PCIe host driver available in MT7623 > series SoCs. > > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com> > --- > .../bindings/pci/mediatek,mt7623-pcie.txt | 153 +++++++++++++++++++++ > 1 file changed, 153 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt > > diff --git a/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt > new file mode 100644 > index 0000000..ee93ba2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt > @@ -0,0 +1,153 @@ > +Mediatek MT7623 PCIe controller > + > +Required properties: > +- compatible: Should contain "mediatek,mt7623-pcie". Did mediatek license the IP block from someone else or was it developed in-house? Is there a name and/or version identifier for the block itself other than identifying it as the one in mt7623? > +- device_type: Must be "pci" > +- reg: Base addresses and lengths of the pcie controller. > +- interrupts: A list of interrupt outputs of the controller. Please be more specific about what each interrupt is for, and how many there are. > +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 > +- ranges: Sub-ranges distributed from the PCIe controller node. An empty > + property is sufficient. > +- clocks: Must contain an entry for each entry in clock-names. > + See ../clocks/clock-bindings.txt for details. > +- clock-names: Must include the following entries: > + - sys_ck > +- resets: Must contain an entry for each entry in reset-names. > + See ../reset/reset.txt for details. This seems odd: you have a device that is simply identified as "pci" without any more specific ID, but you require additional properties (clocks, reset, ...) that are not part of the standard PCI binding. Can you clarify how the port devices related to the root device in this hardware design? Have you considered moving the nonstandard properties into the host bridge node and having that device deal with setting up the links to the other drivers? That way we could use the regular pcie port driver for the children. > +- reset-names: Must include the following entries: > + - pcie-reset > +- num-lanes: Number of lanes to use for this port. > +- phys: Must contain an entry for each entry in phy-names. > +- phy-names: Must include an entry for each sub node. Entries are of the form > + "pcie-phyN": where N ranges from 0 to the value specified for port number. > + See ../phy/phy-mt7623-pcie.txt for details. I think the name should not include the number of the port but rather be always the same here. Arnd
On Thu, 2017-04-27 at 21:06 +0200, Arnd Bergmann wrote: > On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > > Hi > > > > On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote: > >> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > >> > Add documentation for PCIe host driver available in MT7623 > >> > series SoCs. > >> > > >> > Signed-off-by: Ryder Lee <ryder.lee@mediatek.com> > >> > --- > >> > .../bindings/pci/mediatek,mt7623-pcie.txt | 153 +++++++++++++++++++++ > >> > 1 file changed, 153 insertions(+) > >> > create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt > >> > > >> > diff --git a/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt > >> > new file mode 100644 > >> > index 0000000..ee93ba2 > >> > --- /dev/null > >> > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt > >> > @@ -0,0 +1,153 @@ > >> > +Mediatek MT7623 PCIe controller > >> > + > >> > +Required properties: > >> > +- compatible: Should contain "mediatek,mt7623-pcie". > >> > >> Did mediatek license the IP block from someone else or was it > >> developed in-house? Is there a name and/or version identifier > >> for the block itself other than identifying it as the one in mt7623? > > > > Originally, it license from synopsys. Our designer add a wrapper to hide > > the DBI detail so that we cannot use them directly. Perhaps I can call > > it "mediatek,gen2v1-pcie", because we have a plan to upstream a in-house > > Gen2 IP in the future. > > Ok, so this is the same hardware that drivers/pci/dwc/ handles, but > it needs a separate driver because the wrapper that was added uses > a completely different register layout, right? Yes, that's what I mean. At first, I really want to base on drivers/pci/dwc/ to implement this driver. Eventually I found it hard to go on, like what I said before. > Are any of the registers the same at all, e.g. for MSI handling? No, It doesn't support MSI. All I can do is using the registers that designer provide to me. The others are inviable for software. So I treat it as different hardware. Furthermore, we hope that we can put all mediatek drivers together regardless of in-house IP or lincense IP We have no particular IP name but just use chip name to call it. So I will temporarily use "mediatek,gen2v1-pcie" in patch v1. > >> > +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 > >> > +- ranges: Sub-ranges distributed from the PCIe controller node. An empty > >> > + property is sufficient. > >> > +- clocks: Must contain an entry for each entry in clock-names. > >> > + See ../clocks/clock-bindings.txt for details. > >> > +- clock-names: Must include the following entries: > >> > + - sys_ck > >> > +- resets: Must contain an entry for each entry in reset-names. > >> > + See ../reset/reset.txt for details. > >> > >> This seems odd: you have a device that is simply identified as "pci" > >> without any more specific ID, but you require additional properties > >> (clocks, reset, ...) that are not part of the standard PCI binding. > >> > >> Can you clarify how the port devices related to the root device in > >> this hardware design? > > > > I will write clarify like this: > > > > PCIe subsys includes one Host/PCI bridge and 3 PCIe MAC port. There > > are 3 bus master for data access and 1 slave for configuration and > > status register access. Each port has PIPE interface to PHY and > > If I understand this right, then each of the ports in your hardware > is what we normally drive using the drivers/pci/dwc/ driver framework, > but your implementation actually made it more PCI standard compliant > by implementing the normal PCIe host bridge registers for all ports > combined, something that most others don't. In my view, it's correct to implement our driver in this way. But I don't really understand the details about other platforms. > >> Have you considered moving the nonstandard properties into the host > >> bridge node and having that device deal with setting up the links > >> to the other drivers? That way we could use the regular pcie > >> port driver for the children. > >> > > > > OK, but I still want to use port->reset to catch reset properties in > > driver. > > Do you mean in drivers/pci/pcie/portdrv_pci.c? I see that it > has a function called pcie_portdrv_slot_reset(), but I don't see > how that relates to your reset line at the moment. Is this > something you have submitted in a different series? > > Or do you mean in this host driver? The problem I see with > that approach is that the port device is owned by portdrv_pci, > so the host bridge driver should not look at the properties of > the port. hifsys: syscon@1a000000 { compatible = "mediatek,mt7623-hifsys", "syscon"; reg = <0 0x1a000000 0 0x1000>; #clock-cells = <1>; #reset-cells = <1>; }; We have a reset controller(hifsys) in our platform. We can just use devm_reset_control_get() to catch it in the host driver to control port reset. After much consideration, I will move all nonstandard properties to root node, let child node cleanable. > >> > +- reset-names: Must include the following entries: > >> > + - pcie-reset > >> > +- num-lanes: Number of lanes to use for this port. > >> > +- phys: Must contain an entry for each entry in phy-names. > >> > +- phy-names: Must include an entry for each sub node. Entries are of the form > >> > + "pcie-phyN": where N ranges from 0 to the value specified for port number. > >> > + See ../phy/phy-mt7623-pcie.txt for details. > >> > >> I think the name should not include the number of the port but rather > >> be always the same here. > >> > > > > Hmm, I think it's better to keep the name here. It's more readable for > > user to understand the relationship between port0 and phy0. > > No, I would argue that it's confusing for the reader because it > is different from how most other DT bindings work: In each device > node, you tend to have a set of properties with well-known names > that are documented. When your reference is called "pcie-phy1" > in one node and "pcie-phy2", I would interpret that as both ports > having two phys each, but only one of them being used. Okay I will write it more clearly - phys: list of PHY specifiers (used by generic PHY framework) - phy-names : must be "pcie-phy0", "pcie-phy1", "pcie-phyN".. based on the number of PHYs as specified in *phys* property.
On Fri, Apr 28, 2017 at 4:46 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: > On Thu, 2017-04-27 at 21:06 +0200, Arnd Bergmann wrote: >> On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: >> > On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote: >> >> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote: >> Are any of the registers the same at all, e.g. for MSI handling? > > No, It doesn't support MSI. All I can do is using the registers that designer provide > to me. The others are inviable for software. So I treat it as different hardware. > Furthermore, we hope that we can put all mediatek drivers together > regardless of in-house IP or lincense IP > > We have no particular IP name but just use chip name to call it. So I > will temporarily use "mediatek,gen2v1-pcie" in patch v1. I think using the chip name as in the first version of your patch name is better then, in particular since the 'gen2v1' would not be an actual version number but just say which variant got merged into mainline first. A related question would be on how general we want the binding to be. Your binding text starts out by describing that there are three root ports and what their capabilities are. If you think there might be other (existing or future) chips that use the same binding and driver, then being a little more abstract could help in the long run. Arnd
Hi Arnd, > 2017-04-28 19:41 GMT+08:00 Arnd Bergmann <arnd@arndb.de>: > > On Fri, Apr 28, 2017 at 4:46 AM, Ryder Lee > <ryder.lee@mediatek.com> wrote: > > On Thu, 2017-04-27 at 21:06 +0200, Arnd Bergmann wrote: > >> On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee > <ryder.lee@mediatek.com> wrote: > >> > On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote: > >> >> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee > <ryder.lee@mediatek.com> wrote: > >> Are any of the registers the same at all, e.g. for MSI > handling? > > > > No, It doesn't support MSI. All I can do is using the > registers that designer provide to me. The others are inviable > for software. So I treat it as different hardware. > Furthermore, we hope that we can put all mediatek drivers > together regardless of in-house IP or lincense IP > > > > We have no particular IP name but just use chip name to call > it. So I will temporarily use "mediatek,gen2v1-pcie" in patch > v1. > > I think using the chip name as in the first version of your > patch name is better then, in particular since the 'gen2v1' > would not be an actual version number but just say which > variant got merged into mainline first. Okay, i will correct it. > A related question would be on how general we want the binding > to be. > Your binding text starts out by describing that there are > three root ports and what their capabilities are. > > If you think there might be other (existing or future) chips > that use the same binding and driver, then being a little more > abstract could help in the long run. Thanks for reminding me. If we decide to use the same driver in the future, we will have a internal discussion about it. Ryder.
diff --git a/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt new file mode 100644 index 0000000..ee93ba2 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt @@ -0,0 +1,153 @@ +Mediatek MT7623 PCIe controller + +Required properties: +- compatible: Should contain "mediatek,mt7623-pcie". +- device_type: Must be "pci" +- reg: Base addresses and lengths of the pcie controller. +- interrupts: A list of interrupt outputs of the controller. +- #address-cells: Address representation for root ports (must be 3) + - cell 0 specifies the bus and device numbers of the root port: + [23:16]: bus number + [15:11]: device number + - cell 1 denotes the upper 32 address bits and should be 0 + - cell 2 contains the lower 32 address bits and is used to translate to the + CPU address space +- #size-cells: Size representation for root ports (must be 2) +- clocks: Must contain an entry for each entry in clock-names. + See ../clocks/clock-bindings.txt for details. +- clock-names: Must include the following entries: + - free_ck +- power-domains: A phandle and power domain specifier pair to the power domain + which is responsible for collapsing and restoring power to the peripheral +- bus-range: Range of bus numbers associated with this controller +- ranges: Describes the translation of addresses for root ports and standard + PCI regions. The entries must be 6 cells each, where the first three cells + correspond to the address as described for the #address-cells property + above, the fourth cell is the physical CPU address to translate to and the + fifth and six cells are as described for the #size-cells property above. + - 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). + - The remaining entries setup the mapping for the standard I/O and memory + regions. + Please refer to the standard PCI bus binding document for a more detailed + explanation. + +In addition, the device tree node must have sub-nodes describing each +PCIe interface, having the following mandatory properties: + +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 +- ranges: Sub-ranges distributed from the PCIe controller node. An empty + property is sufficient. +- clocks: Must contain an entry for each entry in clock-names. + See ../clocks/clock-bindings.txt for details. +- clock-names: Must include the following entries: + - sys_ck +- resets: Must contain an entry for each entry in reset-names. + See ../reset/reset.txt for details. +- reset-names: Must include the following entries: + - pcie-reset +- num-lanes: Number of lanes to use for this port. +- phys: Must contain an entry for each entry in phy-names. +- phy-names: Must include an entry for each sub node. Entries are of the form + "pcie-phyN": where N ranges from 0 to the value specified for port number. + See ../phy/phy-mt7623-pcie.txt for details. + +Examples: + +SoC dtsi: + + pcie: pcie@1a140000 { + compatible = "mediatek,mt7623-pcie"; + device_type = "pci"; + reg = <0 0x1a140000 0 0x1000>; /* PCIe shared registers */ + interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_LOW>, + <GIC_SPI 194 IRQ_TYPE_LEVEL_LOW>, + <GIC_SPI 195 IRQ_TYPE_LEVEL_LOW>; + #address-cells = <3>; + #size-cells = <2>; + clocks = <&topckgen CLK_TOP_ETHIF_SEL>; + clock-names = "free_ck"; + power-domains = <&scpsys MT2701_POWER_DOMAIN_HIF>; + + bus-range = <0x00 0xff>; + ranges = <0x82000000 0 0x1a142000 0 0x1a142000 0 0x1000 /* Por0 registers */ + 0x82000000 0 0x1a143000 0 0x1a143000 0 0x1000 /* Por1 registers */ + 0x82000000 0 0x1a144000 0 0x1a144000 0 0x1000 /* Por2 registers */ + 0x81000000 0 0x1a160000 0 0x1a160000 0 0x00010000 /* I/O space */ + 0x83000000 0 0x60000000 0 0x60000000 0 0x10000000>; /* memory space */ + status = "disabled"; + + pcie@1,0 { + device_type = "pci"; + assigned-addresses = <0x82000800 0 0x1a142000 0 0x1000>; + reg = <0x0800 0 0 0 0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; + status = "disabled"; + + clocks = <&hifsys CLK_HIFSYS_PCIE0>; + clock-names = "sys_ck"; + resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>; + reset-names = "pcie-reset"; + + num-lanes = <1>; + phys = <&pcie0_phy>; + phy-names = "pcie-phy0"; + }; + + pcie@2,0 { + device_type = "pci"; + assigned-addresses = <0x82001000 0 0x1a143000 0 0x1000>; + reg = <0x1000 0 0 0 0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; + status = "disabled"; + + clocks = <&hifsys CLK_HIFSYS_PCIE1>; + clock-names = "sys_ck"; + resets = <&hifsys MT2701_HIFSYS_PCIE1_RST>; + reset-names = "pcie-reset"; + + num-lanes = <1>; + phys = <&pcie1_phy>; + phy-names = "pcie-phy1"; + }; + + pcie@3,0 { + device_type = "pci"; + assigned-addresses = <0x82001800 0 0x1a144000 0 0x1000>; + reg = <0x1800 0 0 0 0>; + #address-cells = <3>; + #size-cells = <2>; + ranges; + status = "disabled"; + + clocks = <&hifsys CLK_HIFSYS_PCIE2>; + clock-names = "sys_ck"; + resets = <&hifsys MT2701_HIFSYS_PCIE2_RST>; + reset-names = "pcie-reset"; + + num-lanes = <1>; + phys = <&pcie2_phy>; + phy-names = "pcie-phy2"; + }; + }; + +Board dts: + + &pcie { + status = "okay"; + + pcie@1,0 { + status = "okay"; + }; + }; \ No newline at end of file
Add documentation for PCIe host driver available in MT7623 series SoCs. Signed-off-by: Ryder Lee <ryder.lee@mediatek.com> --- .../bindings/pci/mediatek,mt7623-pcie.txt | 153 +++++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7623-pcie.txt