Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630442 > unrolled thread

Re: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe Arnd Bergmann <arnd@arndb.de> - 2017-04-25 14:20 +0200
    Re: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek  PCIe Ryder Lee <ryder.lee@mediatek.com> - 2017-04-28 04:50 +0200
      Re: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe Arnd Bergmann <arnd@arndb.de> - 2017-04-28 13:50 +0200
        Re: FW: [PATCH 2/2] dt-bindings: pcie: Add documentation for  Mediatek PCIe Ryder Lee <ryder.lee@mediatek.com> - 2017-05-02 09:20 +0200

#1630442 — Re: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

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

[toc] | [next] | [standalone]

#1632505 — Re: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

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.

[toc] | [prev] | [next] | [standalone]

#1632817

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

[toc] | [prev] | [next] | [standalone]

#1634181 — Re: FW: [PATCH 2/2] dt-bindings: pcie: Add documentation for Mediatek PCIe

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web