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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7BE5FC433EF for ; Thu, 18 Nov 2021 18:01:20 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 8759861288 for ; Thu, 18 Nov 2021 18:01:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 8759861288 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7B20B82F91; Thu, 18 Nov 2021 19:01:16 +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="jtBcSCoR"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id D677C82F96; Thu, 18 Nov 2021 19:01:13 +0100 (CET) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (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 6257381BC8 for ; Thu, 18 Nov 2021 19:01:08 +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: by mail.kernel.org (Postfix) with ESMTPSA id C9F8361288; Thu, 18 Nov 2021 18:01:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1637258466; bh=PDWDlKvdpqLSSbnd4fs3DzW8UVp8LYd5J898rjHuqD8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jtBcSCoReY4VURikPurYgfKxwxUwIqyRgQLUG1JZb9El2eFKxLhBdeqEMz61ANAhe M+g76I9enO1NF/X8ZVJxQRFhg2zCAUCKwvdkXFFvB+EOUpbgUMT/jHHpXoa+uHgciR IpFEBSRIzBC8W7yz4VT7T3H5+bWm10TT/IDzNHM6na5ATAUFp4bx2IaPO3GFqC3k5y fHwL4WSuWUGd7Sj4MiZzdFHYOATmMEgOWqPLhD9w4iZ+OK5Ge1WGbtGAkbDaGDVshG fJCPQpsp7k+QyMh3FU2u/tMr3qexco4+/snTcxPrYkApR5l/ApciFq/mUo9lVjchc6 VsKgFJNLs0zww== Received: by pali.im (Postfix) id 6EEE1799; Thu, 18 Nov 2021 19:01:03 +0100 (CET) Date: Thu, 18 Nov 2021 19:01:03 +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: <20211118180103.wewgff3pqqrwqjxr@pali> References: <20211111153549.29111-1-kabel@kernel.org> <20211111153549.29111-3-kabel@kernel.org> 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.35 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 Friday 12 November 2021 15:01:57 Stefan Roese wrote: > On 11/11/21 16:35, Marek Behún wrote: > > From: Pali Rohár > > > > As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't > > overwrite read-only SAR PCIe registers") it is required to set Maximum Link > > Width bits of PCIe Root Port Link Capabilities Register depending of number > > of used serdes lanes. As this register is part of PCIe address space and > > not serdes address space, move it into pci_mvebu.c driver. > > > > Read number of PCIe lanes from DT propery "num-lanes" which is used also by > > other PCIe controller drivers in Linux kernel. If this property is absent. > > default to 1. This property needs to be set to 4 for every mvebu board > > which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board. > > > > Signed-off-by: Pali Rohár > > Signed-off-by: Marek Behún > > --- > > arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h | 4 ---- > > .../serdes/a38x/high_speed_env_spec.c | 15 --------------- > > drivers/pci/pci_mvebu.c | 18 ++++++++++++++++++ > > 3 files changed, 18 insertions(+), 19 deletions(-) > > I'm wondering now, if and how this works on Armada XP, which uses the > same PCIe driver but a different serdes/axp/*. Did you take this into > account? It looks like that axp serdes code also touches register of PCIe Root Ports, e.g. in section /* 6.2 PCI Express Link Capabilities */. So it is something which could be improved and cleaned. But it should not cause any issue if registers are configures two times, once in serdes code and once in pci-mvebu.c code. Moreover registers in pci-mvebu space, including config space of PCIe Root Ports are reconfigured by kernel, so I think that it should not cause any issue. But what could cause issues is X1 vs X4 configuration in pci-mvebu.c if "num-lanes" property is not properly set. As is mentioned in commit message there is no A38x board in U-Boot which uses X4. Now with your comment for Armada XP I checked that serdes code and find out that AXP uses macro 'PEX_BUS_MODE_X4' for X4 mode (A38x serdes uses PEX_ROOT_COMPLEX_X4). And in U-Boot there are two boards which sets this macro: board/Synology/ds414/ds414.c and board/theadorable/theadorable.c So it would be needed to adjust this patch for these two boards. Please hold this one patch for now. I will try to prepare new fixed version. Other patches should be OK and independent of this one. > Thanks, > Stefan > > > diff --git a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h > > index 64193d5288..0df898c625 100644 > > --- a/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h > > +++ b/arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h > > @@ -6,12 +6,8 @@ > > #ifndef _CTRL_PEX_H > > #define _CTRL_PEX_H > > -#include > > #include "high_speed_env_spec.h" > > -/* Direct access to PEX0 Root Port's PCIe Capability structure */ > > -#define PEX0_RP_PCIE_CFG_OFFSET (0x00080000 + 0x60) > > - > > /* SOC_CONTROL_REG1 fields */ > > #define PCIE0_ENABLE_OFFS 0 > > #define PCIE0_ENABLE_MASK (0x1 << PCIE0_ENABLE_OFFS) > > diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c > > index d2bc3ab25c..ef4b89c96a 100644 > > --- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c > > +++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c > > @@ -1720,21 +1720,6 @@ int serdes_power_up_ctrl(u32 serdes_num, int serdes_power_up, > > else > > reg_data &= ~0x4000; > > reg_write(SOC_CONTROL_REG1, reg_data); > > - > > - /* > > - * Set Maximum Link Width to X1 or X4 in Root > > - * Port's PCIe Link Capability register. > > - * This register is read-only but if is not set > > - * correctly then access to PCI config space of > > - * endpoint card behind this Root Port does not > > - * work. > > - */ > > - reg_data = reg_read(PEX0_RP_PCIE_CFG_OFFSET + > > - PCI_EXP_LNKCAP); > > - reg_data &= ~PCI_EXP_LNKCAP_MLW; > > - reg_data |= (is_pex_by1 ? 1 : 4) << 4; > > - reg_write(PEX0_RP_PCIE_CFG_OFFSET + > > - PCI_EXP_LNKCAP, reg_data); > > } > > CHECK_STATUS(mv_seq_exec(serdes_num, PEX_POWER_UP_SEQ)); > > diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c > > index a3364d5a59..278dc2756f 100644 > > --- a/drivers/pci/pci_mvebu.c > > +++ b/drivers/pci/pci_mvebu.c > > @@ -83,6 +83,7 @@ struct mvebu_pcie { > > struct resource io; > > u32 port; > > u32 lane; > > + bool is_x4; > > int devfn; > > u32 lane_mask; > > int first_busno; > > @@ -399,6 +400,18 @@ static int mvebu_pcie_probe(struct udevice *dev) > > reg |= PCIE_CTRL_RC_MODE; > > writel(reg, pcie->base + PCIE_CTRL_OFF); > > + /* > > + * Set Maximum Link Width to X1 or X4 in Root Port's PCIe Link > > + * Capability register. This register is defined by PCIe specification > > + * as read-only but this mvebu controller has it as read-write and must > > + * be set to number of SerDes PCIe lanes (1 or 4). If this register is > > + * not set correctly then link with endpoint card is not established. > > + */ > > + reg = readl(pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP); > > + reg &= ~PCI_EXP_LNKCAP_MLW; > > + reg |= (pcie->is_x4 ? 4 : 1) << 4; > > + writel(reg, pcie->base + PCIE_CAPAB_OFF + PCI_EXP_LNKCAP); > > + > > /* > > * Change Class Code of PCI Bridge device to PCI Bridge (0x600400) > > * because default value is Memory controller (0x508000) which > > @@ -582,6 +595,7 @@ static int mvebu_get_tgt_attr(ofnode node, int devfn, > > static int mvebu_pcie_of_to_plat(struct udevice *dev) > > { > > struct mvebu_pcie *pcie = dev_get_plat(dev); > > + u32 num_lanes = 1; > > int ret = 0; > > /* Get port number, lane number and memory target / attr */ > > @@ -594,6 +608,10 @@ static int mvebu_pcie_of_to_plat(struct udevice *dev) > > if (ofnode_read_u32(dev_ofnode(dev), "marvell,pcie-lane", &pcie->lane)) > > pcie->lane = 0; > > + if (!ofnode_read_u32(dev_ofnode(dev), "num-lanes", &num_lanes) && > > + num_lanes == 4) > > + pcie->is_x4 = true; > > + > > sprintf(pcie->name, "pcie%d.%d", pcie->port, pcie->lane); > > /* pci_get_devfn() returns devfn in bits 15..8, see PCI_DEV usage */ > > > > Viele Grüße, > Stefan Roese > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de