From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ryder Lee Subject: Re: [SPAM]Re: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe Date: Wed, 26 Apr 2017 16:10:05 +0800 Message-ID: <1493194205.27023.80.camel@mtkswgap22> References: <1492935543-18190-1-git-send-email-ryder.lee@mediatek.com> <1492935543-18190-3-git-send-email-ryder.lee@mediatek.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Arnd Bergmann Cc: devicetree@vger.kernel.org, linux-pci , Linux Kernel Mailing List , Rob Herring , linux-mediatek@lists.infradead.org, Bjorn Helgaas , Linux ARM List-Id: devicetree@vger.kernel.org Hi On Tue, 2017-04-25 at 14:18 +0200, Arnd Bergmann wrote: > On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee wrote: > > Add documentation for PCIe host driver available in MT7623 > > series SoCs. > > > > Signed-off-by: Ryder Lee > > --- > > .../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. > > +- 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. OK. > > +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 > 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. > > +- 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. > Arnd > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek