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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 336B0C48291 for ; Fri, 2 Feb 2024 22:51:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Message-ID: Subject:Cc: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=kGhc+0sPjBqB99+yNlhONxAsRtLzxJFNHKM0oFj3//g=; b=fIrYmS38QeXivY yMbgy4uhJgkEZXLfNTBoFwP99IYe8X9M16T44RG24q26+YGXg9gB/HiPaRUrkZd+O0SeFrAtR9NQu RrKp6NJD+i89ewb/OskcIr8wWISNT2bA+7NXPtg50B/xIZqFlrCPE0inPQ5kU++7iJFwzGd5+vpay 6TXwuRpLRhiRbJfkNQRQYqhtVYL4ciSsI1jBYc/+wfqys3zrR9jQlPtxLSbQugUD+RMO+B7PO+j2S z3cByogFahE/Na+gceYpO3l9rAl4dSiqI2WcrqPl6oEN6Ex0Njsvh0MJnpNUfF3wEW3E/JUdaGj9s 7pnOvqXFGmMcvUe/T1xw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rW2NY-0000000DimD-2wqp; Fri, 02 Feb 2024 22:51:24 +0000 Received: from sin.source.kernel.org ([145.40.73.55]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rW2NU-0000000Dijw-3cVD for linux-arm-kernel@lists.infradead.org; Fri, 02 Feb 2024 22:51:22 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 3EDF4CE2EA2; Fri, 2 Feb 2024 22:51:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 286A6C43390; Fri, 2 Feb 2024 22:51:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1706914278; bh=qyAXgTBLLnh8SxD11iUAbf5QXHShn7TzaQqXX7GyvVU=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=su95Moq88Nxy71XnTsLYPUhqiJNf7+84hTe2WOhFiR/k43Bz3yuNIyS0Emk0N+SxK kfN3BgtafQxLef9wu0PDcnYnKyemL5IwEd40YuxbCMdOZqry7M9o++6WdqrmK6SAiu wMr8enCyRbRG/R6QDVLtswIRVSHsiFP32V7+3rX6BlekOXDlSBCxvw5TFMOyZp2vKi AgxOIIurWCjKnZcYYQzBFyI5/64jy/bNw1CFzvkMY/gBRSzg+iSTA1F28muCmzkoBQ 200pSm+umS39YPymHDGC4V+D4Qk/wua+6ieG9b6oNPq55uHUiRn+dbf+4lWkawEWxZ iKAhbDKVxmOsw== Date: Fri, 2 Feb 2024 16:51:16 -0600 From: Bjorn Helgaas To: Frank Li , Rob Herring Cc: manivannan.sadhasivam@linaro.org, bhelgaas@google.com, conor+dt@kernel.org, devicetree@vger.kernel.org, festevam@gmail.com, hongxing.zhu@nxp.com, imx@lists.linux.dev, kernel@pengutronix.de, krzysztof.kozlowski+dt@linaro.org, krzysztof.kozlowski@linaro.org, kw@linux.com, l.stach@pengutronix.de, linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, lpieralisi@kernel.org, s.hauer@pengutronix.de, shawnguo@kernel.org Subject: Re: [PATCH v9 05/16] PCI: imx6: Using "linux,pci-domain" as slot ID Message-ID: <20240202225116.GA732628@bhelgaas> 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-20240202_145121_300705_EABCCE5A X-CRM114-Status: GOOD ( 40.01 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org [Rob to to: line] On Fri, Feb 02, 2024 at 05:22:07PM -0500, Frank Li wrote: > On Fri, Feb 02, 2024 at 03:54:31PM -0600, Bjorn Helgaas wrote: > > On Fri, Jan 19, 2024 at 12:11:11PM -0500, Frank Li wrote: > > > Avoid use get slot id by compared with register physical address. If there > > > are more than 2 slots, compared logic will become complex. > > > > > > "linux,pci-domain" already exist at dts since commit: > > > commit (c0b70f05c87f3b arm64: dts: imx8mq: use_dt_domains for pci node). > > > > > > So it is safe to remove compare basic address code: > > > ... > > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > > > imx6_pcie->controller_id = 1; > > > ... > > > > I have no idea what this is telling me. I guess you don't want to use > > IMX8MQ_PCIE2_BASE_ADDR to decide something? That much sounds good: > > the *address* of some MMIO space doesn't tell us anything about the > > function of that space. > > You are right. If there are more than two controller. The check logic > will be extremely complex. > > There are some discussin at below thread about linux,pci-domain > https://lore.kernel.org/imx/20231206165953.GA717921@bhelgaas/ My response here was too low level, just about trivial syntactic and style issues. I should have seen the larger issue at the time; sorry about that. > https://lore.kernel.org/imx/20231217175158.GF6748@thinkpad/ That's a good response from Mani, but again not relevant to my point. My point here is that "compatible" should tell the driver how to operate the device, i.e., the driver knows what registers are present and how they work. If you have two variant devices that both implement a register that can be used to distinguish them, a single "compatible" string might be enough because the driver can use that register to tell the difference. If the driver can't tell the difference by looking at the hardware itself, I think you need a separate "compatible" string for it. Of course I'm far from a DT expert, so please correct this if necessary, Rob, et al. > > I expect the "compatible" string to tell the driver what the > > programming model of the device is. > > > > > + /* Using linux,pci-domain as PCI slot id */ > > > + imx6_pcie->controller_id = of_get_pci_domain_nr(node); > > > + /* > > > + * If there are no "linux,pci-domain" property specified in DT, then assume only one > > > + * controller is available. > > > + */ > > > + if (imx6_pcie->controller_id == -EINVAL) > > > + imx6_pcie->controller_id = 0; > > > + else if (imx6_pcie->controller_id < 0) > > > + return dev_err_probe(dev, imx6_pcie->controller_id, > > > + "linux,pci-domain have wrong value\n"); > > > > Maybe I'm missing something here. It looks like this driver uses > > controller_id to distinguish between hardware variants or maybe > > between two Root Ports (slots?) in the same SoC? > > Yes! > > > imx6_pcie_grp_offset > > return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14; > > > > imx6_pcie_configure_type > > id = imx6_pcie->controller_id > > if (!drvdata->mode_mask[id]) # <-- looks unsafe > > I can add safe check here. > > > id = 0; > > regmap_update_bits(drvdata->mode_off[id], ...) > > > > (This "mode_mask[id]" looks like it will reference garbage if the DT > > supplies "linux,pci-domain = <2>". A bogus DT shouldn't be able to > > cause a driver to misbehave like that.) > > Suppose I can use dt-bind doc to force to 0,1 and safe check here. Nope. The driver must protect itself from garbage in the DT. > > That doesn't seem related to "linux,pci-domain" at all. > > I added comments about > /* Using linux,pci-domain as PCI slot id */ That doesn't make it related :) > We may add new property about controller-id, but there already have common > one "linux,pci-domain", which value in upstreamed dts exactly match our > expection, I also found other platform use it as slot id in kernel tree. > > Any way, we can continue discuss the better solution here. But I hope > it was not block whole 16 patches. we can skip this one firstly. > > I still have more than 10 clean up patches my local tree. > > > > > Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel