Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Wed, 6 Dec 2023 11:50:16 -0500
From:   Frank Li <Frank.li@nxp.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     imx@lists.linux.dev, Richard Zhu <hongxing.zhu@nxp.com>,
        Lucas Stach <l.stach@pengutronix.de>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        "open list:PCI DRIVER FOR IMX6" <linux-pci@vger.kernel.org>,
        "moderated list:PCI DRIVER FOR IMX6" 
        <linux-arm-kernel@lists.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/9] PCI: imx6: Using "linux,pci-domain" as slot ID
Message-ID: <ZXCmSOwTWR6AVpGB@lizhi-Precision-Tower-5810>
References: <20231206155903.566194-5-Frank.Li@nxp.com>
 <20231206163656.GA717052@bhelgaas>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231206163656.GA717052@bhelgaas>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?WjhjdBp1TIjcFHQneBxTkKOkjZXyRJ0yByLCXUJutuHsKELCvt23AdRBbtaA?=
 =?us-ascii?Q?TeS8H44L3qva6pzJk3aCf0ktJGUeZRtfl0d8AL5zoeLAGALcrfH/qfrRMn7Z?=
 =?us-ascii?Q?sIkVi//pkhcCaGA+b2T1zQOXYLKg4acLWPL2cnUVvoFhNojZKl0vcJQxHABl?=
 =?us-ascii?Q?TIygwD2OZWICKKTCvwEOWe9ZAkSHUZVFzz8lsQ26JvCTtrRQ48wQu3t14uEL?=
 =?us-ascii?Q?bmOOB9BdvsmgAyLeWIvkyNa7sK7ajhbvLVnyc6wScZ+t2f829hdtQIIC6p/L?=
 =?us-ascii?Q?J6BN9cQvK7fBOokxxzqI4DJDwkjdB2fauRPC53pYT+ebWPQpMQMarhybqgNG?=
 =?us-ascii?Q?yefVEVghcD5xVrzvrnGYkK5G40jweJWTQxCCYATyLWJqX1r6jfnUMtdIkGyn?=
 =?us-ascii?Q?TDxi12cljA8sH71obmyl368++KdxkzwaIZfLwrGUwEovxkSqkaXNOgWQlArK?=
 =?us-ascii?Q?x5hewAc3hXIzdHOoH12vWVUK88Yv7vJAPoFsmf9Bey/QDdY6Op+z5REie5Uu?=
 =?us-ascii?Q?N35vbvOz6zs19HGrLQvPLU7Ux2UI+mNweMcDZ3O9ho395cX+gA17EWkTOpNY?=
 =?us-ascii?Q?lExC5MGU6GvjgNPEAlKQyZGT3gB3WMO633vHRlsj36rtbZL11zb1Wbeu/JEc?=
 =?us-ascii?Q?YjVk2G/UuCabSmSXTYo+ggFUp6UXgCKkYod+8v3hq71+rcgDeEIiloA3gnye?=
 =?us-ascii?Q?G2M3nkzzpw5unF+fFQkyOj5R3MRQyvHSxOLSYykNjvLG6inBf9BdQZP7VlIT?=
 =?us-ascii?Q?s6IfrLqnSQzXW9NzcWyPbi816uX9/UOvRubj+xCZQ+ZiALNkVojpGuErJu/t?=
 =?us-ascii?Q?tm1JIhjUcD60NuZ5AtnAIO87RCfJ7ffBTwZ78F6ZG7i4eyuxsPSIkZllnolG?=
 =?us-ascii?Q?C4H4sv++RMb8B00HdO9s+wrglMajXnVLAHpi7BWSChhsLdxAYhmR1J/cTozJ?=
 =?us-ascii?Q?WuanTXyUfwUkMVFrGOj3DYAa4fq08yvUyrxPLo9w+L4EWsyj3kHXbafQXl9s?=
 =?us-ascii?Q?Lm/FNP2KhRRYF0mN7f0UnKf1wb3R8WZ1lIACxGxH0cLGcWrBuhPd+HzyBTdy?=
 =?us-ascii?Q?8Jn55SoxLAGU+cB5lnTXNG0C2l80PbZO01yybK4Flun9A8eg6lwDZA+fbKQS?=
 =?us-ascii?Q?tC3PQfGg+L21egJQ2Ru74ZvXIY2NhkvuelnoB/3jiLhq2y0zL9BUH/ZQemYV?=
 =?us-ascii?Q?jezxZAQWkIVyTUQRemGb4aNGu9SAYthlSp3S400Sll6Q+MeTDblt3pGBRxyS?=
 =?us-ascii?Q?dfnw7/hMKbFOYCazegamvdaNBGZc9K0Wrb2PGR6I9bePnKsnUqZcTS/q1Zu0?=
 =?us-ascii?Q?FR6oaa6cs++1LYEGzKlWHp7xYSxarHnigcK7ztQNeuFVYOZVf0XfRWmODjp2?=
 =?us-ascii?Q?JvjQ9v8ep89WrS3TFFoQxAVW97mpILvjrOVecqFfVvXQq26F0CAf+au4ihUo?=
 =?us-ascii?Q?muBIbRhYs9Zeg/d5TB0xwfk3V4546cxR7IEtpcRP6cAmYGq+y/y0vMoWcDwy?=
 =?us-ascii?Q?YQDHrX+oT1KOA5dxqgtbA6Sgc8WcD8YlZ7IbWoJPhcOIRys4VDffx2a7K3iY?=
 =?us-ascii?Q?jV7hRutPVXuSiZqH5Bc=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: eb3f90e2-686b-4113-bc5a-08dbf67b7248
X-MS-Exchange-CrossTenant-AuthSource: AM6PR04MB4838.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2023 16:50:26.5427
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yme4eOKs1lPU5NWRe8cTTRzWNbJWStJ6c0zPvpe99hmlJ7x8uwlCs2I2mTOunvwd/VBqWoxi4kQF5RQV7JNR3w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR04MB9256
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Wed, 06 Dec 2023 08:51:32 -0800 (PST)

On Wed, Dec 06, 2023 at 10:36:56AM -0600, Bjorn Helgaas wrote:
> In subject, maybe you mean "Use 'linux,pci-domain' as slot ID"?
> "Using" is the wrong verb form here.
> 
> On Wed, Dec 06, 2023 at 10:58:58AM -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.
> 
> But this doesn't say anything about "linux,pci-domain", and I don't
> see anything about a register physical address in the patch.
> 
> Maybe this commit log was meant for a different patch?  I'm confused.
> 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 62d77fabd82a..239ef439ba70 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/pm_domain.h>
> >  #include <linux/pm_runtime.h>
> >  
> > +#include "../../pci.h"
> >  #include "pcie-designware.h"
> >  
> >  #define IMX8MQ_GPR_PCIE_REF_USE_PAD		BIT(9)
> > @@ -1333,6 +1334,11 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> >  					     "Failed to get PCIEPHY reset control\n");
> >  	}
> >  
> > +	/* Using linux,pci-domain as PCI slot id */
> > +	imx6_pcie->controller_id = of_get_pci_domain_nr(node);
> > +	if (imx6_pcie->controller_id)
> > +		imx6_pcie->controller_id = 0;
> 
> I don't understand what this is doing.  It looks the same as just:

Good capture. It should be 
if (imx6_pcie->controller_id < 0)
	imx6_pcie->controller_id = 0;

for only one PCI controller case. I just tested first one slot before send
patch, so not met problem.

Previously, we use below logic
	if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
		imx6_pcie->controller_id = 1;

It is not good to depend on register's base address. If there are 3
controllers, check logic will becomoe ugly.

Frank
> 
>   imx6_pcie->controller_id = 0;
> 
> Maybe this is a typo?  As written, it doesn't look like there's any
> point in calling of_get_pci_domain_nr().
> 
> >  	switch (imx6_pcie->drvdata->variant) {
> >  	case IMX7D:
> >  		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > -- 
> > 2.34.1
> >