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 X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CA36DC433DF for ; Thu, 18 Jun 2020 03:01:56 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 96E7D20CC7 for ; Thu, 18 Jun 2020 03:01:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="hbfawIau"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="rvspTLk/" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 96E7D20CC7 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=EZZAs8oOqmU0EeDgyv3SzT6QZAe14vihljpn3hEl3VE=; b=hbfawIau4W/m+K bqDWem25V01BeOPzfg/eo0X4QTRJL7TyR4a1YYndAOY+VOHyEFQkQfkf78d5koM25sjHfzhmLkSCA SE7qrjDgac7Q5Z2U5hvKBGQAq9dOMSq+TjbR43JsYMTYvhIK1uA+83pPwYH+JtPywBXpFBhnegLKv nv5atg4CrGjbG7/588LpEYMXmidW0EP6ufzgwHT6PetCPzhByrQzi7dEv9jwP8qipa2Dq3pD+dngl nJlJ4NaURPq1OyTQdETcRc/lPHyEDhmbO9ioJvwcPjdS7PkXheT+fom6jCFfo9zLa6sROdcBfkrui gM6B6LyZVUiR5Lg5CI4g==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jlkoG-0002RZ-U8; Thu, 18 Jun 2020 03:01:48 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jlkoD-0002QP-GY; Thu, 18 Jun 2020 03:01:47 +0000 Received: from localhost (mobile-166-170-222-206.mycingular.net [166.170.222.206]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id F3240208C7; Thu, 18 Jun 2020 03:01:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1592449303; bh=D+RIBaE1e99GexBp+YDaMWHaXlTMDUSMPXDMbSLsLhU=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=rvspTLk/7iukyPQFUzJrN85r6rr4OtLWCMuGAvhlQQgoCThyT5PA5LDTn4FYkkLfF Dhz/y4545V7+UmxzWJxFbRbIXf8ZXXZwvcNKQgfAZ/6UR5RD69gDLOBtFHpP48Gblt Kn4t75RU6lP8zLcyLuU06yvIETn4t/g8p0HJDz1I= Date: Wed, 17 Jun 2020 22:01:41 -0500 From: Bjorn Helgaas To: Jim Quinlan Subject: Re: [PATCH v5 09/12] PCI: brcmstb: Set internal memory viewport sizes Message-ID: <20200618030141.GA2041805@bjorn-Precision-5520> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200617_200145_673207_09E710D0 X-CRM114-Status: GOOD ( 36.02 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , Rob Herring , Lorenzo Pieralisi , "open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS" , open list , Florian Fainelli , "maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE" , "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" , Bjorn Helgaas , Christoph Hellwig , Nicolas Saenz Julienne Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Jun 17, 2020 at 01:28:12PM -0400, Jim Quinlan wrote: > Hello Bjorn, > > On Tue, Jun 16, 2020 at 6:05 PM Bjorn Helgaas wrote: > > > > On Tue, Jun 16, 2020 at 04:55:16PM -0400, Jim Quinlan wrote: > > > BrcmSTB PCIe controllers are intimately connected to the memory > > > controller(s) on the SOC. There is a "viewport" for each memory controller > > > that allows inbound accesses to CPU memory. Each viewport's size must be > > > set to a power of two, and that size must be equal to or larger than the > > > amount of memory each controller supports. > > > > This describes some requirements, but doesn't actually say what this > > patch *does*. > > > > I *think* it reads the viewport sizes from the "brcm,scb-sizes" DT > > property instead of computing something from "dma-ranges". Looks like > > it also adds support for SCB1 and SCB2. > > > > Those seem interesting, but don't really come through in the subject > > or even the commit log. > > > > If I understand correctly, this is all for DMA ("inbound accesses to > > CPU memory"). I think it would be worth mentioning "DMA", since > > that's the common term for this. > > > I have changed the commit message to the text below. Please let me > know if it requires more work > Thanks, Jim > > PCI: brcmstb: Set internal memory DMA viewport sizes Did you not set the viewport sizes before? > BrcmSTB PCIe controllers are intimately connected to the memory > controller(s) on the SOC. There is a "viewport" for each memory controller > that allows inbound DMA acceses to CPU memory. Each viewport's size must > be set to a power of two, and that size must be equal to or larger than the > amount of memory each controller supports. Unfortunately the viewport > sizes cannot be ascertained from the "dma-ranges" property so they have > their own property, "brcm,scb-sizes". s/inbound DMA acceses to CPU memory/DMA/ "Accesses" is redundant since the "A" in "DMA" stands for "access". I'm not sure "inbound" adds anything and might confuse since DMA may be either a read or write of CPU memory. I assume *all* drivers need to know the address and size of regions in "dma-ranges". Is there something special about this device that means it needs something different? I guess it's the base/extension split? That couldn't be described as two separate DMA ranges? Could/should the new property have a name somehow related to "dma-ranges"? Should "dma-ranges" be documented in Documentation/devicetree/bindings/pci/pci.txt instead of the individual device bindings? > There may be one to three memory controllers; they are indicated by the > term SCBi. Each controller has a base region and an optional extension > region. In physical memory, the base and extension regions are not > adjacent, but in PCIe-space they are. Further, the 1-3 viewports are also > adjacent in PCIe-space. > > The SCB settings work in conjunction with the "dma-ranges' offsets to > enable non-identity mappings between system memory and PCIe space. s/ranges'/ranges"/ (mismatched quotes) This describes the hardware, but still doesn't actually say what this patch *does*. If I'm a user, why do I want this patch? Does it fix something that didn't work before? Does it increase the amount of DMA-able memory? What does this mean in terms of backwards compatibility with old DTs? Does this work with old DTs that don't have "brcm,scb-sizes"? Maybe this is all related to specific devices that weren't supported before, so there *are* no old DTs for them? I can't tell from the binding update or the patch that this is related to specific devices. > > > Signed-off-by: Jim Quinlan > > > Acked-by: Florian Fainelli > > > --- > > > drivers/pci/controller/pcie-brcmstb.c | 68 ++++++++++++++++++++------- > > > 1 file changed, 50 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c > > > index 9189406fd35c..39f77709c6a2 100644 > > > --- a/drivers/pci/controller/pcie-brcmstb.c > > > +++ b/drivers/pci/controller/pcie-brcmstb.c > > > @@ -57,6 +57,8 @@ > > > #define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK 0x300000 > > > #define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_128 0x0 > > > #define PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK 0xf8000000 > > > +#define PCIE_MISC_MISC_CTRL_SCB1_SIZE_MASK 0x07c00000 > > > +#define PCIE_MISC_MISC_CTRL_SCB2_SIZE_MASK 0x0000001f > > > > > > #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO 0x400c > > > #define PCIE_MEM_WIN0_LO(win) \ > > > @@ -154,6 +156,7 @@ > > > #define SSC_STATUS_OFFSET 0x1 > > > #define SSC_STATUS_SSC_MASK 0x400 > > > #define SSC_STATUS_PLL_LOCK_MASK 0x800 > > > +#define PCIE_BRCM_MAX_MEMC 3 > > > > > > #define IDX_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_INDEX]) > > > #define DATA_ADDR(pcie) (pcie->reg_offsets[EXT_CFG_DATA]) > > > @@ -260,6 +263,8 @@ struct brcm_pcie { > > > const int *reg_field_info; > > > enum pcie_type type; > > > struct reset_control *rescal; > > > + int num_memc; > > > + u64 memc_size[PCIE_BRCM_MAX_MEMC]; > > > }; > > > > > > /* > > > @@ -715,22 +720,44 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie, > > > u64 *rc_bar2_offset) > > > { > > > struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie); > > > - struct device *dev = pcie->dev; > > > struct resource_entry *entry; > > > + struct device *dev = pcie->dev; > > > + u64 lowest_pcie_addr = ~(u64)0; > > > + int ret, i = 0; > > > + u64 size = 0; > > > > > > - entry = resource_list_first_type(&bridge->dma_ranges, IORESOURCE_MEM); > > > - if (!entry) > > > - return -ENODEV; > > > + resource_list_for_each_entry(entry, &bridge->dma_ranges) { > > > + u64 pcie_beg = entry->res->start - entry->offset; > > > > > > + size += entry->res->end - entry->res->start + 1; > > > + if (pcie_beg < lowest_pcie_addr) > > > + lowest_pcie_addr = pcie_beg; > > > + } > > > > > > - /* > > > - * The controller expects the inbound window offset to be calculated as > > > - * the difference between PCIe's address space and CPU's. The offset > > > - * provided by the firmware is calculated the opposite way, so we > > > - * negate it. > > > - */ > > > - *rc_bar2_offset = -entry->offset; > > > - *rc_bar2_size = 1ULL << fls64(entry->res->end - entry->res->start); > > > + if (lowest_pcie_addr == ~(u64)0) { > > > + dev_err(dev, "DT node has no dma-ranges\n"); > > > + return -EINVAL; > > > + } > > > + > > > + ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1, > > > + PCIE_BRCM_MAX_MEMC); > > > + > > > + if (ret <= 0) { > > > + /* Make an educated guess */ > > > + pcie->num_memc = 1; > > > + pcie->memc_size[0] = 1 << fls64(size - 1); > > > + } else { > > > + pcie->num_memc = ret; > > > + } > > > + > > > + /* Each memc is viewed through a "port" that is a power of 2 */ > > > + for (i = 0, size = 0; i < pcie->num_memc; i++) > > > + size += pcie->memc_size[i]; > > > + > > > + /* System memory starts at this address in PCIe-space */ > > > + *rc_bar2_offset = lowest_pcie_addr; > > > + /* The sum of all memc views must also be a power of 2 */ > > > + *rc_bar2_size = 1ULL << fls64(size - 1); > > > > > > /* > > > * We validate the inbound memory view even though we should trust > > > @@ -782,12 +809,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > > void __iomem *base = pcie->base; > > > struct device *dev = pcie->dev; > > > struct resource_entry *entry; > > > - unsigned int scb_size_val; > > > bool ssc_good = false; > > > struct resource *res; > > > int num_out_wins = 0; > > > u16 nlw, cls, lnksta; > > > - int i, ret; > > > + int i, ret, memc; > > > u32 tmp, aspm_support; > > > > > > /* Reset the bridge */ > > > @@ -824,11 +850,17 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie) > > > writel(upper_32_bits(rc_bar2_offset), > > > base + PCIE_MISC_RC_BAR2_CONFIG_HI); > > > > > > - scb_size_val = rc_bar2_size ? > > > - ilog2(rc_bar2_size) - 15 : 0xf; /* 0xf is 1GB */ > > > tmp = readl(base + PCIE_MISC_MISC_CTRL); > > > - u32p_replace_bits(&tmp, scb_size_val, > > > - PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK); > > > + for (memc = 0; memc < pcie->num_memc; memc++) { > > > + u32 scb_size_val = ilog2(pcie->memc_size[memc]) - 15; > > > + > > > + if (memc == 0) > > > + u32p_replace_bits(&tmp, scb_size_val, PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK); > > > + else if (memc == 1) > > > + u32p_replace_bits(&tmp, scb_size_val, PCIE_MISC_MISC_CTRL_SCB1_SIZE_MASK); > > > + else if (memc == 2) > > > + u32p_replace_bits(&tmp, scb_size_val, PCIE_MISC_MISC_CTRL_SCB2_SIZE_MASK); > > > + } > > > writel(tmp, base + PCIE_MISC_MISC_CTRL); > > > > > > /* > > > -- > > > 2.17.1 > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel