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 B87BAC433EF for ; Mon, 29 Nov 2021 13:28:05 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 300B782F70; Mon, 29 Nov 2021 14:28:02 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="DbOX73rQ"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 3518281918; Mon, 29 Nov 2021 14:28:00 +0100 (CET) Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 6ADD282F9D for ; Mon, 29 Nov 2021 14:27:54 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=pali@kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 6A61D613F1; Mon, 29 Nov 2021 13:27:52 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 71D68C004E1; Mon, 29 Nov 2021 13:27:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1638192471; bh=9FrXzDFq+Q3xYDfYsGm8FmDVOHA2+ElTceyIyhAvDYM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=DbOX73rQgGIWQLURkbrhW4QhLkSBWyXo9JBpvwE+21XDF2vOgR/EgLg56q7duRI06 iuLZZu05ZWUM/nO3GBMrqhl5MqBHz9FuTwRo1xZUwv9kyVZrTH4pnTj3n1VYm37+iL ilQo0OZVNCqdVNZHt0QwGvGEW9bwnq/znU9C7lIb/aHxgx1hztRZFwr5SVVyo9KXcQ +cUqQfEzMntLpOwMFslQV/LBnIFZxOGgawRRS/yWcROAr/AkK9OHlES9k/eHkQwMad KhAKGQvIKKvW4oF8VgqG+w+tB87OlLslehcu4/FYPIjyoet6zFeE20/gySpB55eHgt Vahu/jNttSSYQ== Received: by pali.im (Postfix) id A5532EAA; Mon, 29 Nov 2021 14:27:48 +0100 (CET) Date: Mon, 29 Nov 2021 14:27:48 +0100 From: Pali =?utf-8?B?Um9ow6Fy?= To: Stefan Roese Cc: Marek =?utf-8?B?QmVow7pu?= , u-boot@lists.denx.de, Marek =?utf-8?B?QmVow7pu?= Subject: Re: [PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c Message-ID: <20211129132748.5tt4x5rnoxusk5eu@pali> References: <20211111153549.29111-3-kabel@kernel.org> <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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20180716 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 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. 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