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 BEE88C433EF for ; Fri, 19 Nov 2021 06:27:22 +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 73E9C61AEC for ; Fri, 19 Nov 2021 06:27:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 73E9C61AEC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=denx.de 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 1FDAD82FCE; Fri, 19 Nov 2021 07:27:19 +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=1637303239; bh=29akfgu11M499eGIcVjBjXXM2b90iSmaEHA2bVUbjns=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=oYY8c+JX1iwjf9QDW5w5AQ6K2TLCZtsMQn8byhyfDrePU9dvgHCcmJ30vYdqKZ2XV BIEGoUXKKp0OC1pdnmefUqhiOYjlRVreCi3A3/vKVoHzCOws6dpewBkmHuftBKyDJJ so5/Fp6IYaBwRvqPk5RzevBGqA/ROROEDvUqHslxSty/sUWa7j94V6ESfEINi+jY81 /3L+esoFzp9JPJoqKDcZpfV45jGZ8mWKLZySUCCCQnkK1wpSfh6sZPqTrtoYzmKyc5 wXOFf6H2gMB4FXl5PYAd9+IOfa7S9M+OyyBAf4OPqdH07TrV91SF7s1nBRBVrcpwKh 96e8UukNU9m8g== Received: by phobos.denx.de (Postfix, from userid 109) id 2320B82FD3; Fri, 19 Nov 2021 07:27:17 +0100 (CET) Received: from mout-u-107.mailbox.org (mout-u-107.mailbox.org [IPv6:2001:67c:2050:1::465:107]) (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 83F6582FC9 for ; Fri, 19 Nov 2021 07:27:13 +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 smtp1.mailbox.org (smtp1.mailbox.org [IPv6:2001:67c:2050:105:465:1:1:0]) (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 4HwRXd38zCzQkFx; Fri, 19 Nov 2021 07:27:13 +0100 (CET) Message-ID: <9fef9511-4f9f-fdcf-8187-ba4612145970@denx.de> Date: Fri, 19 Nov 2021 07:27:09 +0100 MIME-Version: 1.0 Subject: Re: [PATCH u-boot-marvell 07/10] pci: pci_mvebu: Fix PCIe MEM and IO resources assignment and mbus mapping 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: <20211111153549.29111-1-kabel@kernel.org> <20211111153549.29111-8-kabel@kernel.org> <20211118174633.doaoh7bp2pxj57iu@pali> From: Stefan Roese In-Reply-To: <20211118174633.doaoh7bp2pxj57iu@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.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 11/18/21 18:46, Pali Rohár wrote: > On Friday 12 November 2021 15:18:48 Stefan Roese wrote: >> On 11/11/21 16:35, Marek Behún wrote: >>> From: Pali Rohár >>> >>> Do not call pci_set_region() for resources which were not properly mapped. >>> This prevents U-Boot to access unmapped memory space. >>> >>> Update MBUS_PCI_MEM_SIZE and MBUS_PCI_IO_SIZE macros to cover all PCIe MEM >>> and IO ranges. Previously these macros covered only address ranges for the >>> first PCIe port. Between MBUS_PCI_IO_BASE and MBUS_PCI_MEM_BASE there is >>> space for six 128 MB long address ranges. So set MBUS_PCI_MEM_SIZE to value >>> of 6*128 MB. Similarly set MBUS_PCI_IO_SIZE to 6*64 KB. >> >> Perhaps it makes sense to calculate this '6' in some macro? > > Hm... maybe. > > Or maybe better to add "#define maximal_number_of_pcie_ports 6" (but > with better name!) as 6 is the maximal number of PCIe ports on 88f78xx0. > Other SoCs can have maximally only 4 PCIe ports. So even if we change > this default mapping in future and there would be bigger gap between > MBUS_PCI_IO_BASE and MBUS_PCI_MEM_BASE, it does not make sense to change > that '6' to any higher value. > > What do you think? Sounds good to me. Thanks, Stefan >> This makes >> it easier to understand this value at a later time. Something like this: >> >> #define MEM_SIZE_TOTAL (0xf0000000 - MBUS_PCI_MEM_BASE) // or some better >> macro for 0xf000000 here? >> #define MEM_REGION_COUNT (MEM_SIZE_TOTAL / (128 << 20)) >> >> ? >> >>> Function resource_size() returns zero when start address is 0 and end >>> address is -1. So set invalid resources to these values to indicate that >>> resource has no mapping. >>> >>> Split global PCIe MEM and IO resources (defined by MBUS_PCI_*_* macros) >>> into PCIe ports in mvebu_pcie_bind() function which allocates per-port >>> based struct mvebu_pcie, instead of using global state variables >>> mvebu_pcie_membase and mvebu_pcie_iobase. This makes pci_mvebu.c driver >>> independent of global static variables (which store the state of >>> allocation) and allows to bind and unbind the driver more times. >>> >>> Signed-off-by: Pali Rohár >>> Signed-off-by: Marek Behún >>> --- >>> arch/arm/mach-mvebu/include/mach/cpu.h | 4 +- >>> drivers/pci/pci_mvebu.c | 84 ++++++++++++++++++-------- >>> 2 files changed, 61 insertions(+), 27 deletions(-) >>> >>> diff --git a/arch/arm/mach-mvebu/include/mach/cpu.h b/arch/arm/mach-mvebu/include/mach/cpu.h >>> index a7a62c7e7d..4c52a330d9 100644 >>> --- a/arch/arm/mach-mvebu/include/mach/cpu.h >>> +++ b/arch/arm/mach-mvebu/include/mach/cpu.h >>> @@ -75,9 +75,9 @@ enum { >>> * Default Device Address MAP BAR values >>> */ >>> #define MBUS_PCI_MEM_BASE MVEBU_SDRAM_SIZE_MAX >>> -#define MBUS_PCI_MEM_SIZE (128 << 20) >>> +#define MBUS_PCI_MEM_SIZE ((6*128) << 20) >> >> Nitpicking: Space before and after the '*' please. >> >>> #define MBUS_PCI_IO_BASE 0xF1100000 >>> -#define MBUS_PCI_IO_SIZE (64 << 10) >>> +#define MBUS_PCI_IO_SIZE ((6*64) << 10) >> >> Same here. >> >>> #define MBUS_SPI_BASE 0xF4000000 >>> #define MBUS_SPI_SIZE (8 << 20) >>> #define MBUS_DFX_BASE 0xF6000000 >>> diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c >>> index 701a17dfb7..fea32414bf 100644 >>> --- a/drivers/pci/pci_mvebu.c >>> +++ b/drivers/pci/pci_mvebu.c >>> @@ -26,6 +26,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> DECLARE_GLOBAL_DATA_PTR; >>> @@ -96,14 +97,6 @@ struct mvebu_pcie { >>> u32 cfgcache[(0x3c - 0x10) / 4]; >>> }; >>> -/* >>> - * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped >>> - * into SoCs address space. Each controller will map 128M of MEM >>> - * and 64K of I/O space when registered. >>> - */ >>> -static void __iomem *mvebu_pcie_membase = (void __iomem *)MBUS_PCI_MEM_BASE; >>> -static void __iomem *mvebu_pcie_iobase = (void __iomem *)MBUS_PCI_IO_BASE; >>> - >>> static inline bool mvebu_pcie_link_up(struct mvebu_pcie *pcie) >>> { >>> u32 val; >>> @@ -478,26 +471,24 @@ static int mvebu_pcie_probe(struct udevice *dev) >>> mvebu_pcie_set_local_bus_nr(pcie, 0); >>> mvebu_pcie_set_local_dev_nr(pcie, 1); >>> - pcie->mem.start = (u32)mvebu_pcie_membase; >>> - pcie->mem.end = pcie->mem.start + MBUS_PCI_MEM_SIZE - 1; >>> - mvebu_pcie_membase += MBUS_PCI_MEM_SIZE; >>> - >>> - if (mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr, >>> + if (resource_size(&pcie->mem) && >>> + mvebu_mbus_add_window_by_id(pcie->mem_target, pcie->mem_attr, >>> (phys_addr_t)pcie->mem.start, >>> resource_size(&pcie->mem))) { >>> printf("PCIe unable to add mbus window for mem at %08x+%08x\n", >>> (u32)pcie->mem.start, (unsigned)resource_size(&pcie->mem)); >>> + pcie->mem.start = 0; >>> + pcie->mem.end = -1; >>> } >>> - pcie->io.start = (u32)mvebu_pcie_iobase; >>> - pcie->io.end = pcie->io.start + MBUS_PCI_IO_SIZE - 1; >>> - mvebu_pcie_iobase += MBUS_PCI_IO_SIZE; >>> - >>> - if (mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr, >>> + if (resource_size(&pcie->io) && >>> + mvebu_mbus_add_window_by_id(pcie->io_target, pcie->io_attr, >>> (phys_addr_t)pcie->io.start, >>> resource_size(&pcie->io))) { >>> printf("PCIe unable to add mbus window for IO at %08x+%08x\n", >>> (u32)pcie->io.start, (unsigned)resource_size(&pcie->io)); >>> + pcie->io.start = 0; >>> + pcie->io.end = -1; >>> } >>> /* Setup windows and configure host bridge */ >>> @@ -506,13 +497,23 @@ static int mvebu_pcie_probe(struct udevice *dev) >>> /* PCI memory space */ >>> pci_set_region(hose->regions + 0, pcie->mem.start, >>> pcie->mem.start, resource_size(&pcie->mem), PCI_REGION_MEM); >>> - pci_set_region(hose->regions + 1, >>> - 0, 0, >>> - gd->ram_size, >>> - PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); >>> - pci_set_region(hose->regions + 2, pcie->io.start, >>> - pcie->io.start, resource_size(&pcie->io), PCI_REGION_IO); >>> - hose->region_count = 3; >>> + hose->region_count = 1; >>> + >>> + if (resource_size(&pcie->mem)) { >>> + pci_set_region(hose->regions + hose->region_count, >>> + pcie->mem.start, pcie->mem.start, >>> + resource_size(&pcie->mem), >>> + PCI_REGION_MEM); >>> + hose->region_count++; >>> + } >>> + >>> + if (resource_size(&pcie->io)) { >>> + pci_set_region(hose->regions + hose->region_count, >>> + pcie->io.start, pcie->io.start, >>> + resource_size(&pcie->io), >>> + PCI_REGION_IO); >>> + hose->region_count++; >>> + } >>> /* PCI Bridge support 32-bit I/O and 64-bit prefetch mem addressing */ >>> pcie->cfgcache[(PCI_IO_BASE - 0x10) / 4] = >>> @@ -680,6 +681,8 @@ static int mvebu_pcie_bind(struct udevice *parent) >>> struct mvebu_pcie *pcie; >>> struct uclass_driver *drv; >>> struct udevice *dev; >>> + struct resource mem; >>> + struct resource io; >>> ofnode subnode; >>> /* Lookup pci driver */ >>> @@ -689,6 +692,11 @@ static int mvebu_pcie_bind(struct udevice *parent) >>> return -ENOENT; >>> } >>> + mem.start = MBUS_PCI_MEM_BASE; >>> + mem.end = MBUS_PCI_MEM_BASE + MBUS_PCI_MEM_SIZE - 1; >>> + io.start = MBUS_PCI_IO_BASE; >>> + io.end = MBUS_PCI_IO_BASE + MBUS_PCI_IO_SIZE - 1; >>> + >>> ofnode_for_each_subnode(subnode, dev_ofnode(parent)) { >>> if (!ofnode_is_available(subnode)) >>> continue; >>> @@ -697,6 +705,32 @@ static int mvebu_pcie_bind(struct udevice *parent) >>> if (!pcie) >>> return -ENOMEM; >>> + /* >>> + * MVEBU PCIe controller needs MEMORY and I/O BARs to be mapped >>> + * into SoCs address space. Each controller will map 128M of MEM >>> + * and 64K of I/O space when registered. >>> + */ >>> + >>> + if (resource_size(&mem) >= SZ_128M) { >>> + pcie->mem.start = mem.start; >>> + pcie->mem.end = mem.start + SZ_128M - 1; >>> + mem.start += SZ_128M; >>> + } else { >>> + printf("PCIe unable to assign mbus window for mem\n"); >>> + pcie->mem.start = 0; >>> + pcie->mem.end = -1; >>> + } >>> + >>> + if (resource_size(&io) >= SZ_64K) { >>> + pcie->io.start = io.start; >>> + pcie->io.end = io.start + SZ_64K - 1; >>> + io.start += SZ_64K; >>> + } else { >>> + printf("PCIe unable to assign mbus window for io\n"); >>> + pcie->io.start = 0; >>> + pcie->io.end = -1; >>> + } >>> + >>> /* Create child device UCLASS_PCI and bind it */ >>> device_bind(parent, &pcie_mvebu_drv, pcie->name, pcie, subnode, >>> &dev); >>> >> >> 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 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