Message-ID: <39619170-117d-2504-4816-ff40c398ec36@nvidia.com>
Date:   Thu, 5 Dec 2019 15:27:55 +0530
From:   Vidya Sagar <vidyas@...dia.com>
To:     Rob Herring <robh+dt@...nel.org>
CC:     Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jingoo Han <jingoohan1@...il.com>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "jonathanh@...dia.com" <jonathanh@...dia.com>,
        "andrew.murray@....com" <andrew.murray@....com>,
        "kishon@...com" <kishon@...com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "kthota@...dia.com" <kthota@...dia.com>,
        "mmaddireddy@...dia.com" <mmaddireddy@...dia.com>,
        "sagar.tv@...il.com" <sagar.tv@...il.com>
Subject: Re: [PATCH 2/6] dt-bindings: PCI: tegra: Add DT support for PCIe EP
 nodes in Tegra194

On 11/29/2019 6:56 PM, Vidya Sagar wrote:

Rob, Can you please update your comments on this?

Thanks,
Vidya Sagar

> On 11/25/2019 5:22 PM, Gustavo Pimentel wrote:
>> On Mon, Nov 25, 2019 at 7:33:59, Thierry Reding
>> <thierry.reding@...il.com> wrote:
>>
>>> On Mon, Nov 25, 2019 at 12:53:42PM +0530, Vidya Sagar wrote:
>>>> On 11/22/2019 6:49 PM, Thierry Reding wrote:
>>>>> On Fri, Nov 22, 2019 at 04:15:01PM +0530, Vidya Sagar wrote:
>>>>>> Add support for PCIe controllers that can operate in endpoint mode
>>>>>> in Tegra194.
>>>>>>
>>>>>> Signed-off-by: Vidya Sagar <vidyas@...dia.com>
>>>>>> ---
>>>>>>    .../bindings/pci/nvidia,tegra194-pcie-ep.txt  | 138 ++++++++++++++++++
>>>>>>    1 file changed, 138 insertions(+)
>>>>>>    create mode 100644 Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt
>>>>>
>>>>> The vast majority of this is a duplication of the host mode device tree
>>>>> bindings. I think it'd be best to combine both and only highlight where
>>>>> both modes differ.
>>>>>
>>>>> The designware-pcie.txt binding does something similar.
>>>> Ok. I'll merge this into the host mode bindings file and in that differentiate between
>>>> root mode and endpoint mode.
>>>>
>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..4676ccf7dfa5
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie-ep.txt
>>>>>> @@ -0,0 +1,138 @@
>>>>>> +NVIDIA Tegra PCIe Endpoint mode controller (Synopsys DesignWare Core based)
>>>>>> +
>>>>>> +Some of the PCIe controllers which are based on Synopsys DesignWare PCIe IP
>>>>>> +are dual mode i.e. they can work in root port mode or endpoint mode but one
>>>>>> + at a time. Since they are based on DesignWare IP, they inherit all the common
>>>>>> +properties defined in designware-pcie.txt.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: For Tegra19x, must contain "nvidia,tegra194-pcie".
>>>>>
>>>>> The device tree snippets that you added have "nvidia,tegra194-pcie-ep"
>>>>> for EP mode controllers. So either this is wrong or the DTS files are
>>>>> wrong.
>>>> DTS file are correct. This is a mistake in this file. I'll correct this.
>>>>
>>>>>
>>>>> This device tree binding describes the exact same hardware, so I don't
>>>>> think we necessarily need two different compatible strings. It's fairly
>>>>> easy to distinguish between which mode to run in by looking at which
>>>>> properties exist. EP mode for example is the only one that uses the
>>>>> "addr_space" reg entry.
>>>>>
>>>>> Rob, do you know why a different compatible string was chosen for the EP
>>>>> mode? Looking at the driver, there are only a handful of differences in
>>>>> the programming, but most of the driver remains identical. An extra DT
>>>>> compatible string seems a bit exaggerated since it suggests that this is
>>>>> actually different hardware, where it clearly isn't.
>>>> Since all other implementations have done it this way, I just followed to be in sync
>>>> with them. Even I would also like to hear from Rob on the rationale behind this.
> Rob, Could you please update on this?
> 
>>>>
>>>>>
>>>>>> +  Tegra194: Only C0, C4 & C5 controllers are dual mode controllers.
>>>>>> +- power-domains: A phandle to the node that controls power to the respective
>>>>>> +  PCIe controller and a specifier name for the PCIe controller. Following are
>>>>>> +  the specifiers for the different PCIe controllers
>>>>>> +    TEGRA194_POWER_DOMAIN_PCIEX8B: C0
>>>>>> +    TEGRA194_POWER_DOMAIN_PCIEX4A: C4
>>>>>> +    TEGRA194_POWER_DOMAIN_PCIEX8A: C5
>>>>>> +  these specifiers are defined in
>>>>>> +  "include/dt-bindings/power/tegra194-powergate.h" file.
>>>>>> +- reg: A list of physical base address and length pairs for each set of
>>>>>> +  controller registers. Must contain an entry for each entry in the reg-names
>>>>>> +  property.
>>>>>> +- reg-names: Must include the following entries:
>>>>>> +  "appl": Controller's application logic registers
>>>>>> +  "atu_dma": iATU and DMA registers. This is where the iATU (internal Address
>>>>>> +             Translation Unit) registers of the PCIe core are made available
>>>>>> +             for SW access.
>>>>>> +  "dbi": The aperture where root port's own configuration registers are
>>>>>> +         available
>>>>>> +  "addr_space": Used to map remote RC address space
>>>>>> +- interrupts: A list of interrupt outputs of the controller. Must contain an
>>>>>> +  entry for each entry in the interrupt-names property.
>>>>>> +- interrupt-names: Must include the following entry:
>>>>>> +  "intr": The Tegra interrupt that is asserted for controller interrupts
>>>>>> +- 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:
>>>>>> +  - core
>>>>>> +- resets: Must contain an entry for each entry in reset-names.
>>>>>> +  See ../reset/reset.txt for details.
>>>>>> +- reset-names: Must include the following entries:
>>>>>> +  - apb
>>>>>> +  - core
>>>>>> +- phys: Must contain a phandle to P2U PHY for each entry in phy-names.
>>>>>> +- phy-names: Must include an entry for each active lane.
>>>>>> +  "p2u-N": where N ranges from 0 to one less than the total number of lanes
>>>>>> +- nvidia,bpmp: Must contain a pair of phandle to BPMP controller node followed
>>>>>> +  by controller-id. Following are the controller ids for each controller.
>>>>>> +    0: C0
>>>>>> +    4: C4
>>>>>> +    5: C5
>>>>>> +- vddio-pex-ctl-supply: Regulator supply for PCIe side band signals
>>>>>> +- nvidia,pex-rst-gpio: Must contain a phandle to a GPIO controller followed by
>>>>>> +  GPIO that is being used as PERST signal
>>>>>
>>>>> Why is this NVIDIA specific? Do other instantiations of the DW IP not
>>>>> also need a means to define which GPIO is the reset?
>>>> I'm not sure. At least I didn't find anything like this in other implementations.
>>>> My understanding is that, controller handles assert/de-assert on the PERST line
>>>> automatically without SW intervention. I think it is for the same reason that other
>>>> implementations don't wait for the REFCLK to flow in from host to configure the IP.
>>>> I think they just use some internal clock for the configuration and switch to
>>>> running the core based on REFCLK as and when it is available
>>>> (i.e. whenever a de-assert is perceived on PERST line by the controller)
>>>
>>> That would be somewhat surprising, though. The IP used in Tegra must be
>>> pretty close to the IP used in other SoCs, and the code that we need in
>>> pex_ep_event_pex_rst_{assert,deassert}() is pretty significant. Why the
>>> other instantiations wouldn't need something similar seems unlikely to
>>> me.
>>>
>>> Perhaps Jingoo or Gustavo can shed some light on this.
>>
>> On my current FPGA prototyping solution, I don't need to control the
>> PERST line and it's very likely that I don't even have access to control
>> it. I guess due to some particularity of my solution, the HW team
>> probably has decided to wire it up directly for some unknown reason to
>> me.
>>
>> However, It seems to me that exynos, imx6, keystone, meson, al, histb,
>> kirin, and qcom drivers controls the PERST line in spite of others driver
>> that doesn't do it like in my prototype solution.
>> In the end I'd says that depends of how the IP solution of design by the
>> HW team.
>>
>> Gustavo
>>
>>>
>>> Thierry
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- pinctrl-names: A list of pinctrl state names.
>>>>>> +  It is mandatory for C5 controller and optional for other controllers.
>>>>>> +  - "default": Configures PCIe I/O for proper operation.
>>>>>> +- pinctrl-0: phandle for the 'default' state of pin configuration.
>>>>>> +  It is mandatory for C5 controller and optional for other controllers.
>>>>>> +- supports-clkreq: Refer to Documentation/devicetree/bindings/pci/pci.txt
>>>>>> +- nvidia,update-fc-fixup: This is a boolean property and needs to be present to
>>>>>> +    improve performance when a platform is designed in such a way that it
>>>>>> +    satisfies at least one of the following conditions thereby enabling root
>>>>>> +    port to exchange optimum number of FC (Flow Control) credits with
>>>>>> +    downstream devices
>>>>>> +    1. If C0/C4/C5 run at x1/x2 link widths (irrespective of speed and MPS)
>>>>>> +    2. If C0/C4/C5 operate at their respective max link widths and
>>>>>> +       a) speed is Gen-2 and MPS is 256B
>>>>>> +       b) speed is >= Gen-3 with any MPS
>>>>>> +- nvidia,aspm-cmrt-us: Common Mode Restore Time for proper operation of ASPM
>>>>>> +   to be specified in microseconds
>>>>>> +- nvidia,aspm-pwr-on-t-us: Power On time for proper operation of ASPM to be
>>>>>> +   specified in microseconds
>>>>>> +- nvidia,aspm-l0s-entrance-latency-us: ASPM L0s entrance latency to be
>>>>>> +   specified in microseconds
>>>>>> +
>>>>>> +NOTE:- On Tegra194's P2972-0000 platform, only C5 controller can be enabled to
>>>>>> +operate in the endpoint mode because of the way the platform is designed.
>>>>>> +There is a mux that needs to be programmed to let the REFCLK from the host to
>>>>>> +flow into C5 controller when it operates in the endpoint mode. This mux is
>>>>>> +controlled by the GPIO (AA, 5) and it needs to be driven 'high'. For this to
>>>>>> +happen, set status of "pex-refclk-sel-high" node under "gpio@...0000" node to
>>>>>> +'okay'.
>>>>>> +    When any dual mode controller is made to operate in the endpoint mode,
>>>>>> +please make sure that its respective root port node's status is set to
>>>>>> +'disabled'.
>>>>>
>>>>> This seems very brittle to me. There's no good way how we can detect
>>>>> such misconfigurations. If instead we only have one node describing the
>>>>> hardware fully, the chances of configuring things wrong (by for example
>>>>> enabling both the host and EP mode device tree nodes) can be reduced.
>>>>>
>>>>> So I think instead of duplicating all of the device tree content to have
>>>>> both a host and an EP node for each controller, it'd be better to just
>>>>> have a single node and let the device tree bindings specify which
>>>>> changes to apply to switch into EP mode.
>>>>>
>>>>> For example, there should be nothing wrong with specifying some of the
>>>>> EP-only properties (like num-ib-windows and num-ob-windows) all the time
>>>>> and only use them when we actually run in EP mode.
>>>>>
>>>>> As I mentioned earlier, there are a couple of easy ways to distinguish
>>>>> the modes. The presence of the "addr_space" reg entry is one example,
>>>>> but we could also key off the nvidia,pex-rst-gpio property, since that
>>>>> (presumably) wouldn't be needed for host mode.
>>>>>
>>>>> That way we can just add default, host mode entries to tegra194.dtsi and
>>>>> whenever somebody wants to enable EP mode, they can just override the
>>>>> node in the board-level DTS file, like so:
>>>>>
>>>>>     pcie@...a0000 {
>>>>>         reg = <0x00 0x141a0000 0x0 0x00020000   /* appl registers (128K)      */
>>>>>                0x00 0x3a040000 0x0 0x00040000   /* iATU_DMA reg space (256K)  */
>>>>>                0x00 0x3a080000 0x0 0x00040000   /* DBI reg space (256K)       */
>>>>>                0x1c 0x00000000 0x4 0x00000000>; /* Address Space (16G)        */
>>>>>         reg-names = "appl", "atu_dma", "dbi", "addr_space";
>>>>>
>>>>>         nvidia,pex-rst-gpio = <&gpio TEGRA194_MAIN_GPIO(GG, 1) GPIO_ACTIVE_LOW>;
>>>>>     };
>>>>>
>>>>> Thierry
>>>> I like it and fine with making these modifications also but would like to hear from Rob
>>>> also on this.
>>>>
>>>> - Vidya Sagar
>>>>>
>>>>>> +
>>>>>> +Examples:
>>>>>> +=========
>>>>>> +
>>>>>> +Tegra194:
>>>>>> +--------
>>>>>> +
>>>>>> +    pcie_ep@...a0000 {
>>>>>> +        compatible = "nvidia,tegra194-pcie-ep", "snps,dw-pcie-ep";
>>>>>> +        power-domains = <&bpmp TEGRA194_POWER_DOMAIN_PCIEX8A>;
>>>>>> +        reg = <0x00 0x141a0000 0x0 0x00020000   /* appl registers (128K)      */
>>>>>> +               0x00 0x3a040000 0x0 0x00040000   /* iATU_DMA reg space (256K)  */
>>>>>> +               0x00 0x3a080000 0x0 0x00040000   /* DBI reg space (256K)       */
>>>>>> +               0x1c 0x00000000 0x4 0x00000000>; /* Address Space (16G)        */
>>>>>> +        reg-names = "appl", "atu_dma", "dbi", "addr_space";
>>>>>> +
>>>>>> +        num-lanes = <8>;
>>>>>> +        num-ib-windows = <2>;
>>>>>> +        num-ob-windows = <8>;
>>>>>> +
>>>>>> +        pinctrl-names = "default";
>>>>>> +        pinctrl-0 = <&clkreq_c5_bi_dir_state>;
>>>>>> +
>>>>>> +        clocks = <&bpmp TEGRA194_CLK_PEX1_CORE_5>;
>>>>>> +        clock-names = "core";
>>>>>> +
>>>>>> +        resets = <&bpmp TEGRA194_RESET_PEX1_CORE_5_APB>,
>>>>>> +             <&bpmp TEGRA194_RESET_PEX1_CORE_5>;
>>>>>> +        reset-names = "apb", "core";
>>>>>> +
>>>>>> +        interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>;    /* controller interrupt */
>>>>>> +        interrupt-names = "intr";
>>>>>> +
>>>>>> +        nvidia,bpmp = <&bpmp 5>;
>>>>>> +
>>>>>> +        nvidia,aspm-cmrt-us = <60>;
>>>>>> +        nvidia,aspm-pwr-on-t-us = <20>;
>>>>>> +        nvidia,aspm-l0s-entrance-latency-us = <3>;
>>>>>> +
>>>>>> +        vddio-pex-ctl-supply = <&vdd_1v8ao>;
>>>>>> +
>>>>>> +        nvidia,pex-rst-gpio = <&gpio TEGRA194_MAIN_GPIO(GG, 1)
>>>>>> +                    GPIO_ACTIVE_LOW>;
>>>>>> +
>>>>>> +        phys = <&p2u_nvhs_0>, <&p2u_nvhs_1>, <&p2u_nvhs_2>,
>>>>>> +               <&p2u_nvhs_3>, <&p2u_nvhs_4>, <&p2u_nvhs_5>,
>>>>>> +               <&p2u_nvhs_6>, <&p2u_nvhs_7>;
>>>>>> +
>>>>>> +        phy-names = "p2u-0", "p2u-1", "p2u-2", "p2u-3", "p2u-4",
>>>>>> +                "p2u-5", "p2u-6", "p2u-7";
>>>>>> +    };
>>>>>> -- 
>>>>>> 2.17.1
>>>>>>
>>>>
>>
>>
> 

Powered by blists - more mailing lists