Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Rob Herring <robh@kernel.org>
To: "Wan Mohamad,
	Wan Ahmad Zainie"  <wan.ahmad.zainie.wan.mohamad@intel.com>
Cc: "bhelgaas@google.com" <bhelgaas@google.com>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"andriy.shevchenko@linux.intel.com" 
	<andriy.shevchenko@linux.intel.com>,
	"mgross@linux.intel.com" <mgross@linux.intel.com>,
	"Raja Subramanian,
	Lakshmi Bai"  <lakshmi.bai.raja.subramanian@intel.com>
Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller
Date: Fri, 30 Oct 2020 09:55:37 -0500
Message-ID: <CAL_Jsq+1HiYK09+piSqJz0Jo+F3XXfg0+qpKQSDL7G32c2P4Eg@mail.gmail.com> (raw)
In-Reply-To: <DM6PR11MB3721FA210CD596C4C64306F8DD150@DM6PR11MB3721.namprd11.prod.outlook.com>

On Fri, Oct 30, 2020 at 8:05 AM Wan Mohamad, Wan Ahmad Zainie
<wan.ahmad.zainie.wan.mohamad@intel.com> wrote:
>
> Hi Rob.
>
> Thanks for the review.
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Wednesday, October 28, 2020 10:42 PM
> > To: Wan Mohamad, Wan Ahmad Zainie
> > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > Cc: bhelgaas@google.com; lorenzo.pieralisi@arm.com; linux-
> > pci@vger.kernel.org; devicetree@vger.kernel.org;
> > andriy.shevchenko@linux.intel.com; mgross@linux.intel.com; Raja
> > Subramanian, Lakshmi Bai <lakshmi.bai.raja.subramanian@intel.com>
> > Subject: Re: [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller
> >
> > On Tue, Oct 27, 2020 at 02:00:10PM +0800, Wan Ahmad Zainie wrote:
> > > Document DT bindings for PCIe controller found on Intel Keem Bay SoC.
> > >
> > > Signed-off-by: Wan Ahmad Zainie
> > > <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > ---
> > >  .../bindings/pci/intel,keembay-pcie-ep.yaml   |  86 +++++++++++++
> > >  .../bindings/pci/intel,keembay-pcie.yaml      | 120 ++++++++++++++++++
> > >  2 files changed, 206 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > >  create mode 100644
> > > Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-ep.yaml
> > > new file mode 100644
> > > index 000000000000..11962c205744
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie-
> > ep.yaml
> > > @@ -0,0 +1,86 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie-ep.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Intel Keem Bay PCIe EP controller
> > > +
> > > +maintainers:
> > > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +      const: intel,keembay-pcie-ep
>
> Fixed in v2, wrong indentation as per report.
>
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: DesignWare PCIe registers
> > > +      - description: PCIe configuration space
> > > +      - description: Keem Bay specific registers
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: dbi
> > > +      - const: addr_space
> > > +      - const: apb
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: PCIe interrupt
> > > +      - description: PCIe event interrupt
> > > +      - description: PCIe error interrupt
> > > +      - description: PCIe memory access interrupt
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: intr
> > > +      - const: ev_intr
> > > +      - const: err_intr
> > > +      - const: mem_access_intr
> >
> > '_intr' is redundant. Drop it. You'll need a better name for the first one
> > though.
>
> I will drop _intr in v2.
> I will send out once I get suitable name from Keem Bay data book.
>
> >
> > > +
> > > +  num-ib-windows:
> > > +    description: Number of inbound address translation windows
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  num-ob-windows:
> > > +    description: Number of outbound address translation windows
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +
> > > +  num-lanes:
> > > +    description: Number of lanes to use.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [ 1, 2, 4, 8 ]
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - reg-names
> > > +  - interrupts
> > > +  - interrupt-names
> > > +  - num-ib-windows
> > > +  - num-ob-windows
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +  - |
> > > +    pcie-ep@37000000 {
> > > +          compatible = "intel,keembay-pcie-ep";
> > > +          reg = <0x37000000 0x00800000>,
> > > +                <0x36000000 0x01000000>,
> > > +                <0x37800000 0x00000200>;
> > > +          reg-names = "dbi", "addr_space", "apb";
> > > +          interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL_HIGH>,
> > > +                       <GIC_SPI 108 IRQ_TYPE_EDGE_RISING>,
> > > +                       <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>,
> > > +                       <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> > > +          interrupt-names = "intr", "ev_intr", "err_intr",
> > > +                       "mem_access_intr";
> > > +          num-ib-windows = <4>;
> > > +          num-ob-windows = <4>;
> > > +          num-lanes = <2>;
> > > +    };
> > > diff --git
> > > a/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > new file mode 100644
> > > index 000000000000..49e5d3d35bd4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/intel,keembay-pcie.yaml
> > > @@ -0,0 +1,120 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/pci/intel,keembay-pcie.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Intel Keem Bay PCIe RC controller
> > > +
> > > +maintainers:
> > > +  - Wan Ahmad Zainie <wan.ahmad.zainie.wan.mohamad@intel.com>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/pci/pci-bus.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +      const: intel,keembay-pcie
>
> Wrong indentation as per report.
> I will fix in v2.
>
> >
> > > +
> > > +  device_type:
> > > +    const: pci
> > > +
> > > +  "#address-cells":
> > > +    const: 3
> > > +
> > > +  "#size-cells":
> > > +    const: 2
> >
> > Can drop these 3 as pci-bus.yaml defines them.
>
> I will drop these 3 in v2.
>
> >
> > > +
> > > +  ranges:
> > > +    maxItems: 1
> > > +
> > > +  reset-gpios:
> > > +    maxItems: 1
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: DesignWare PCIe registers
> > > +      - description: PCIe configuration space
> > > +      - description: Keem Bay specific registers
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: dbi
> > > +      - const: config
> > > +      - const: apb
> > > +
> > > +  clocks:
> > > +    items:
> > > +      - description: bus clock
> > > +      - description: auxiliary clock
> >
> > The EP doesn't have clocks? You should have roughly the same resources for
> > RC and EP modes.
>
> For Keem Bay, EP mode link initialization is done in boot firmware.
> This include setup the clocks.
> That's why I do not include clocks for EP.
>
> >
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: master
> > > +      - const: aux
> > > +
> > > +  interrupts:
> > > +    items:
> > > +      - description: PCIe interrupt
> > > +      - description: PCIe event interrupt
> > > +      - description: PCIe error interrupt
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: intr
> > > +      - const: ev_intr
> > > +      - const: err_intr
> > > +
> > > +  num-lanes:
> > > +    description: Number of lanes to use.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [ 1, 2, 4, 8 ]
> > > +
> > > +  num-viewport:
> > > +    description: Number of view ports configured in hardware.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    default: 2
> >
> > Pretty sure it's not 2 if num-ib-windows and num-ob-windows are 4.
>
> As per pcie-designware-host.c, default value is 2, if it is not set.

Yes, that's true.

> My example and the DT in my system is 4.
> I will fix in v2, by using const: 4.
> Should I drop default?

Yes.

BTW, I'm going to make all 3 properties obsolete. I'm working on a
patch to detect all this. It's pretty straight-forward, just see how
many registers are writable. The WIP patch is on my for-kernelci
branch.

The problem with these properties is they are defined as RC and EP
specific, but they are really fixed h/w config independent of the
mode. And num-viewport is incomplete because the inbound and outbound
sizes are independent. The driver just currently doesn't use inbound
windows for RC mode. Also, the driver claims there can be up to 256
windows, but I'm not really sure that's right. There's 2 platforms
upstream (ls1088a and ls208xa) claiming 256 windows in DT, but testing
with the detection code indicates they only have 16 IB and 16 OB
windows. Perhaps if you have the DWC manual you could confirm what's
possible.

Rob

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-27  6:00 [PATCH 0/2] PCI: keembay: Add support for Intel Keem Bay Wan Ahmad Zainie
2020-10-27  6:00 ` [PATCH 1/2] dt-bindings: PCI: Add Intel Keem Bay PCIe controller Wan Ahmad Zainie
2020-10-28 13:57   ` Rob Herring
2020-10-28 14:42   ` Rob Herring
2020-10-30 13:04     ` Wan Mohamad, Wan Ahmad Zainie
2020-10-30 14:55       ` Rob Herring [this message]
2020-11-03  6:01         ` Wan Mohamad, Wan Ahmad Zainie
2020-10-27  6:00 ` [PATCH 2/2] PCI: keembay: Add support for Intel Keem Bay Wan Ahmad Zainie
2020-10-28 14:22   ` Rob Herring
2020-10-28 15:34     ` Andy Shevchenko
2020-11-03  4:58     ` Wan Mohamad, Wan Ahmad Zainie
2020-11-03 22:22   ` Bjorn Helgaas
2020-11-04  9:36     ` Andy Shevchenko
2020-11-04 12:03     ` Wan Mohamad, Wan Ahmad Zainie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAL_Jsq+1HiYK09+piSqJz0Jo+F3XXfg0+qpKQSDL7G32c2P4Eg@mail.gmail.com \
    --to=robh@kernel.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=lakshmi.bai.raja.subramanian@intel.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mgross@linux.intel.com \
    --cc=wan.ahmad.zainie.wan.mohamad@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git