From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 40188C4332F for ; Mon, 29 Nov 2021 16:08:11 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id CBA9882FAB; Mon, 29 Nov 2021 17:08:08 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1638202089; bh=lnKJcrB3Wy4Fx5PjcH+XCY2K/GZOcQfvpVAeg6KQkfw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=L9p4VLAKt5j5lGIuzgXGM7ufGBlywapN8BwLtEPmZcJ0DwK23ap82sa15/t9jPa1o ap0MlkL3WBIuhf6+o4lJ54EeWmxlY7krj1Ji5dKzUKx36XcnyNL/pPyeERdwhBA0ZB kja0nNgjhN6ROWcYgj2K1vH9yZnszlD5GrBRFgxo914alpaCu3ITYDD7ixgqXdrLe9 ftgoAiHL9dIxrycHBww3sf/tqL3rpD0Hz6XxTM04qgmPDgBvQR+u+yn6Gx8ldWSuPA GgEl/66cUSVXoBsZol+1FA2CVa/4gH7/g5HFEpfpnp7IPsVhwXeipKajRagJkI+XS6 xWgW0+SC8zDNg== Received: by phobos.denx.de (Postfix, from userid 109) id BF4A982FE7; Mon, 29 Nov 2021 17:08:06 +0100 (CET) Received: from mout-u-107.mailbox.org (mout-u-107.mailbox.org [91.198.250.252]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 8FD3D82F8B for ; Mon, 29 Nov 2021 17:08:02 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=fail smtp.mailfrom=sr@denx.de Received: from smtp2.mailbox.org (unknown [91.198.250.124]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-384) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-u-107.mailbox.org (Postfix) with ESMTPS id 4J2qyB1pZTzQlJ3; Mon, 29 Nov 2021 17:08:02 +0100 (CET) Message-ID: <1e69be57-ca7a-9b96-de47-7f3faf146a64@denx.de> Date: Mon, 29 Nov 2021 17:07:54 +0100 MIME-Version: 1.0 Subject: Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c Content-Language: en-US To: =?UTF-8?Q?Pali_Roh=c3=a1r?= Cc: =?UTF-8?Q?Marek_Beh=c3=ban?= , u-boot@lists.denx.de, =?UTF-8?Q?Marek_Beh=c3=ban?= References: <20211118180103.wewgff3pqqrwqjxr@pali> <5ea0641f-cde6-e553-dfb8-993ab6daff67@denx.de> <20211123155953.4cuju6mtwgmrzumq@pali> <20211129090612.q3pdg64bhhk4gnvz@pali> <83bd5ea0-46ce-39be-d566-46c397db2037@denx.de> <20211129114701.evkol44t6l3rvdpf@pali> <20211129132748.5tt4x5rnoxusk5eu@pali> <20211129142845.c7keue6h5fvcnypj@pali> From: Stefan Roese In-Reply-To: <20211129142845.c7keue6h5fvcnypj@pali> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.37 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On 11/29/21 15:28, Pali Rohár wrote: > On Monday 29 November 2021 14:27:48 Pali Rohár wrote: >> On Monday 29 November 2021 13:30:45 Stefan Roese wrote: >>> Hi Pali, >>> >>> On 11/29/21 12:47, Pali Rohár wrote: >>>> Hello! >>>> >>>> On Monday 29 November 2021 10:22:58 Stefan Roese wrote: >>>>> On 11/29/21 10:06, Pali Rohár wrote: >>>>> >>>>> >>>>> >>>>>>>> After this DTS change, pci-mvebu.c will just replace value of current >>>>>>>> number of lanes (which is set to 4 by serdes code) to value from DTS, >>>>>>>> which is 4. Therefore there should be no change. >>>>>>>> >>>>>>>> Could you test whole patch series with above DTS change if it works >>>>>>>> properly on Theadorable board? >>>>>>> >>>>>>> Yes, I don't see any issues with this patchset applied plus this DT >>>>>>> patch on theadorable. The PCIe links are up and with the correct width. >>>>>>> >>>>>>> What I'm wondering is, when exactly does the PCIe RP start the link >>>>>>> establishment. In my case with AXP this is still in the AXP serdes code >>>>>>> of course. But in the A38x code, it should be in the PCIe controller >>>>>>> driver now AFAIU. I see that you configure the link width in the >>>>>>> controller and do some other configuration (address windows etc), but >>>>>>> at the end you "simply" wait for the link to come up via >>>>>>> mvebu_pcie_wait_for_link(). I would have expected here some special >>>>>>> command (config bit?) to the PCIe controller to start the link >>>>>>> establishment. So when exactly does the A38x start this action? >>>>>> >>>>>> That is interesting question... While I'm reading it again, I really do >>>>>> not know. Because you are right that mvebu_pcie_wait_for_link() is just >>>>>> waiting for a link and it "magically" comes up. I have tested it on A385 >>>>>> and it is stable with different Compex Atheros cards which caused issues >>>>>> in past also on A3720. >>>>> >>>>> I would prefer, to fully understand when exactly the link establishment >>>>> is started. Since this is crucial for the setup of the controller that >>>>> needs to be done *before* the link starts to come up. >>>> >>>> I try to dig as more information as possible and finally I find out that >>>> important information is available also in now removed, but originally >>>> public A38x documentation. Thankfully web archive has copy of it: >>>> >>>> https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf >>>> >>>> 17.3 Link Initialization >>>> >>>> In case the initialization fails and no link is established, the PHY >>>> will keep on trying to initiate a link forever unless the port is >>>> disabled. As long as the port is enabled, the PHY will continue trying >>>> to establish a link; once the PHY identifies that a device is >>>> connected to it, a link will be established. >>>> >>>> PCIe port is enabled by bits in SoC Control 1 Register, which is done in >>>> U-Boot SerDes initialization code. This is IIRC SoC specific, and reason >>>> why every Armada SoC has own SerDes init code. >>>> >>>> And looks like that due to "the PHY will keep on trying to initiate a >>>> link forever", the PCIe link comes up when pci-mvebu.c sets all required >>>> registers to correct values. E.g. set correct mode (RC vs endpoint), >>>> link width (x1 vs x4), etc... >>>> >>>>> Could you perhaps try to remove some of the register configurations in >>>>> the MVEBU PCIe driver to see, if the link establishment relies on this >>>>> register to be written to (e.g. PCI_EXP_LNKCAP)? >>>> >>>> First port on A385 is by default set to X4 width, other ports to X1 >>>> width. Without updating LNKCAP to correct width, card in first PCIe port >>>> never initialize. And cards in other ports are initialized even before >>>> pci-mvebu.c starts configuration. >>> >>> So the PCIe ports are now trying to establish the links, even when the >>> correct configuration is not yet done. This might work but sound far >>> from perfect to me IMHO. >> >> Yes, it looks like (based on behavior of the first port). And it is not >> perfect, just another mess :-( >> >>>> So seems that this matches above behavior. SerDes init code enabled all >>>> PCIe ports. Ports which are using default configuration (second, third) >>>> are immediately initialized and link is established. Port which requires >>>> additional configuration (first port, for switching from X4 to X1) just >>>> stay in that "keep on trying to initiate a link forever" state until >>>> pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1 >>>> width and success. And seems that this is the reason why 100ms timeout >>>> is needed... As at this stage when pci-mvebu.c switches X4 to X1, init >>>> timeout as defined in PCIe spec (that 100ms) starts ticking. For other >>>> ports it starts ticking when serdes init code enables ports. >>>> >>>> >>>> I have looked into all PCIe registers which are present in functional >>>> spec, but it looks like that there is no pci-mvebu register which can >>>> turn of LTSSM and link training, like it is in other PCIe controllers. >>>> It looks like that only SoC-specific port enable bits are there. >>>> >>>> It is starting to be bigger mess as before... Any suggestion how to >>>> continue with it? >>>> >>>> We cannot (easily) move that code which flips PCIe bits in SoC Control 1 >>>> Register from SerDes init code to pci-mvebu.c as this is outside of >>>> pci-mvebu.c address space and also it is different on every SoC. >>>> pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion >>>> up to the A39x. >>> >>> One idea would be, to use a "reset-controller" driver on the Armada >>> platforms, that is capable to at least reset and release the PCIe ports. >>> Via the SoC Control 1 reg on A38x and via the SoC Control Register >>> on AXP. >> >> In that specification is also written: >> >> Enable the PCI Express interface by setting the , , >> , or field in the SoC Control 1 Register (Table >> 1888 p. 1395). This allows programming of link parameters before the >> start of link initialization. The highest common link width is >> established according to the following order: x4 to x1. >> >> So I think the correct behavior should be: >> >> 1. pci-mvebu.c configures all controller registers to correct values >> 2. PCIe port is enabled via SoC-specific register >> 3. pci-mvebu.c waits for link up >> >> I guess that reset-controller does not help, as core release this reset >> prior starting driver initialization. > > Ok, it looks like that reset controller API allows to do this. It would > mean to define that "system-controller@18200" as reset controller, > exports from it for each PCIe port reset functionality and implements > assert and deassert functions which disable and enable port. > > And because DTS for pci-mvebu.c driver is defined as multi-root-port, > "resets" property would need to be defined for each port separately. Okay. Sounds like a plan to me. > Just I'm not sure if this "enable port functionality" should be > implemented via Reset Controller API... How else should / could this be done then? Do you have alterative ideas? Thanks, Stefan >> Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is >> part of following device defined in kernel DTS file: >> >> systemc: system-controller@18200 { >> compatible = "marvell,armada-380-system-controller", >> "marvell,armada-370-xp-system-controller"; >> reg = <0x18200 0x100>; >> }; >> >> Linux kernel has driver for this DTS device is file: >> arch/arm/mach-mvebu/system-controller.c >> >> U-Boot does not have any driver for this compatible string. >> >> So PCIe port enable/disable should be in this driver. I can write simple >> driver also for U-Boot which can control this register. But I really do >> not know which interface should it use. >> >> Has somebody else any idea? >> >>> I just looked into some Linux PCIe DT bindings and found e.g. this in >>> the mediatek spec: >>> >>> Documentation/devicetree/bindings/pci/mediatek-pcie.txt >>> ... >>> Required properties for MT7623/MT2701: >>> ... >>> - resets: Must contain an entry for each entry in reset-names. >>> See ../reset/reset.txt for details. >>> - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the >>> number of root ports. >>> ... >>> resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>, >>> <&hifsys MT2701_HIFSYS_PCIE1_RST>, >>> <&hifsys MT2701_HIFSYS_PCIE2_RST>; >>> reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2"; >>> phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>, >>> <&pcie2_phy PHY_TYPE_PCIE>; >>> phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2"; >>> >>> >>> And make sure in the serdes code keeps (or actively sets?) these PCIe >>> ports into the reset state. The PCIe driver would then release the ports >>> out of reset after their configuration. >>> >>> Or is some other serdes code missing in between "get PCIe port out of >>> reset" and the MVEBU PCIe driver taking over the control? >>> >>> What do you think? I might be missing something here. >>> >>> Thanks, >>> Stefan