Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Thu, 7 Dec 2023 12:10:26 -0500
From:   Frank Li <Frank.li@nxp.com>
To:     Philipp Zabel <p.zabel@pengutronix.de>
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 3/9] PCI: imx6: Simplify reset handling by using by using
 *_FLAG_HAS_*_RESET
Message-ID: <ZXH8guDh+pLbg0Tn@lizhi-Precision-Tower-5810>
References: <20231206155903.566194-1-Frank.Li@nxp.com>
 <20231206155903.566194-4-Frank.Li@nxp.com>
 <e314466b31dd8e88212ae5d7ac2fecf26b851829.camel@pengutronix.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e314466b31dd8e88212ae5d7ac2fecf26b851829.camel@pengutronix.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?d0IvYq7HsFzD2hUoN4PrijusWMUZIg8c7+4wbWu6pjkWemtJzecePr1aQWai?=
 =?us-ascii?Q?mgpo7OpMK5VU+Dv1O2XtDq4iXoqHi3eLUxOIX2BtPNXJ9X3Os+mer9H3HRBz?=
 =?us-ascii?Q?4076hHn5BIx7JFLxt5/nhI4rAieyZubGa8l5b5dLUQ6TaVDDpZM3r5P+NWeF?=
 =?us-ascii?Q?Ot5j8kH0sblDNmjeDbg5lq/SvkdCS/aYIMLNnxI+C306AMpao/eO16eS2UMP?=
 =?us-ascii?Q?2q4j6wBOYN+6QLc+sYkNgfzitYaadrcEfW8TG/8gosEkJJghIM/8vTm5Sax5?=
 =?us-ascii?Q?/CXtc1eRFDgfqaeN+HSuz4khdl4q02mQ4Gl2QRzdueiPFyfOkua5NaKA9ASz?=
 =?us-ascii?Q?jRgSBuHW5dspUPXINW8cK/k2p542+nDAZTbRU4XPEJFZ+A0ysRJ8bZVSy6un?=
 =?us-ascii?Q?Z3bxTHPifOqLBOw3iQPrDUv4m8+4zT7R/0XP6JZEbwIe9TEuKdmwN0eq5Q32?=
 =?us-ascii?Q?BSi58AsASD89Zmvmz8vE/F7FlGE/Sl6xaSfsaauqQ6EJj+PkAFtN1tGzh9IR?=
 =?us-ascii?Q?Wg4zqqDDx3iwZMv8rJ2v1GApdA5S1EQmiu4STNtSbb6i6KF6k/C/wGe81OPb?=
 =?us-ascii?Q?wn4o7Mwlr1W70Z7DzggSncjgIphiICxyRpB0QQuFWQNgucFnyKAtF3wmSVBt?=
 =?us-ascii?Q?tvHEDi7P7NR2Vkk6VflsjWT24uJxTgReCkcMRF83y50eW6kJqBkX5PePaqa9?=
 =?us-ascii?Q?T7y6xO6k88omly+iNVGZ2wZU3Y80pJ3nnQ9/1RExu3VgZhC87B3mfnEu02Lt?=
 =?us-ascii?Q?RFU1v7CDeHE6udNH53CKgyJorCEiF2bd82ZjMibdIizOflm6/NQoUTrT8aC2?=
 =?us-ascii?Q?llo7zEY+F8DWObcxST+PH0TkzNessmt/WVtkJapD5zi/cfogxTq6nqs4i/VT?=
 =?us-ascii?Q?K6zHXi2xKCzl6vI1sEbBwTDyf55fGOn25sEYJZvGsKJSzVZkiTyrzeKGmX7Q?=
 =?us-ascii?Q?NGV4YhAWWgVIN5bGNLhOcY24C7Cka3s/c/YirFh7J/8az3yz8ekoUN/TvMM9?=
 =?us-ascii?Q?h3DeW2rbelCDyUgfIP9lMDghD+XvOPvZ/JggJgIZ4xu85tp20urmY7R6PCXh?=
 =?us-ascii?Q?cnlGcV2Z85x5VEYoYqEe7OOQoJrZSlX87r3D8D1NSjqxTEx86WkKj+eK/0tK?=
 =?us-ascii?Q?dFuMlI6W/iAsC8t/i2vdjlNtdbWlWbc6o1oGeiSE0YKgTJrPWBv20ziJCWLr?=
 =?us-ascii?Q?IWz2h6QA8CPXkanv703ETO1+/0PXiaRWhR4DDivAAnn4SQAH7nV3uZDA4i/M?=
 =?us-ascii?Q?a3bd2TbULYwRlW+Xa2I2IePh656oVlZ7cNKXHKhe7Z2ExGNYEQtMcPMhkCSc?=
 =?us-ascii?Q?MSATCnaUyij4Z0HLaHFOBBa9mKipKi+XpuzBxlFm7i9DOKOSFF3zGnQtQrff?=
 =?us-ascii?Q?PG953AxLYyIDrlTNRvneoL8CoMTLsR6bP17rC/NZSRwEZ/IoT25Kq3FV0YQ+?=
 =?us-ascii?Q?D1piaYCCdC2y4DkRkQGGePq7UUduyKjGgrJiatc70rjq9mOHypQLyr84Y2kU?=
 =?us-ascii?Q?nZZC7A6UVUUIzibS5FjerWkL980wwCkfzasA2fRbiQBnAFSrvMuxr6aDeOTD?=
 =?us-ascii?Q?Ss08Bxh4b+3/wInGGJk=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9f34e547-4264-4559-1912-08dbf7476dca
X-MS-Exchange-CrossTenant-AuthSource: AM6PR04MB4838.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Dec 2023 17:10:36.2509
 (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: bjre/QxGhbIBuSk/UzCtNz/YpBmABdNpTgE5oV6/XOWMp33GynwVBd0C+g7OXTQbvO3EWMVSokAocuPvz6noMA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR04MB9423
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 pete.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 (pete.vger.email [0.0.0.0]); Thu, 07 Dec 2023 09:11:15 -0800 (PST)

On Wed, Dec 06, 2023 at 05:52:23PM +0100, Philipp Zabel wrote:
> Hi Frank,
> 
> On Mi, 2023-12-06 at 10:58 -0500, Frank Li wrote:
> > Refactors the reset handling logic in the imx6 PCI driver by adding
> > IMX6_PCIE_FLAG_HAS_*_RESET bitmask define for drvdata::flags.
> > 
> > The drvdata::flags and a bitmask ensures a cleaner and more scalable
> > switch-case structure for handling reset.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 115 +++++++++++---------------
> >  1 file changed, 47 insertions(+), 68 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index bcf52aa86462..62d77fabd82a 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> [...]
> > @@ -696,18 +698,13 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie)
> >  
> >  static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> >  {
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX7D:
> > -	case IMX8MQ:
> > -	case IMX8MQ_EP:
> > +	if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY_RESET))
> 
> This check is not strictly necessary. If the flag is not set,
> imx6_pcie->pciephy_reset is guaranteed to be NULL, and then
> reset_control_assert() is a no-op. Same for the other (de)assert
> calls below.
> 
> [...]
> 
> > @@ -1335,36 +1319,24 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> >  					     "failed to get pcie phy\n");
> >  	}
> >  
> > -	switch (imx6_pcie->drvdata->variant) {
> > -	case IMX7D:
> > -		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > -			imx6_pcie->controller_id = 1;
> > -
> > -		imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev,
> > -									    "pciephy");
> > -		if (IS_ERR(imx6_pcie->pciephy_reset)) {
> > -			dev_err(dev, "Failed to get PCIEPHY reset control\n");
> > -			return PTR_ERR(imx6_pcie->pciephy_reset);
> > -		}
> > -
> > -		imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> > -									 "apps");
> > -		if (IS_ERR(imx6_pcie->apps_reset)) {
> > -			dev_err(dev, "Failed to get PCIE APPS reset control\n");
> > -			return PTR_ERR(imx6_pcie->apps_reset);
> > -		}
> > -		break;
> > -	case IMX8MM:
> > -	case IMX8MM_EP:
> > -	case IMX8MP:
> > -	case IMX8MP_EP:
> > -		imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> > -									 "apps");
> > +	if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_APP_RESET)) {
> > +		imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev, "apps");
> [...]
> 
> I wonder whether we should just defer the check whether apps/pciephy
> resets should be used to the device tree validation, and make this an
> unconditional call to get an optional reset control:
> 
> 	imx6_pcie->apps_reset = devm_reset_control_get_optional_exclusive(dev, "apps");

I think double check here is neccesary. No sure if dts file version
binding yaml was exactly matched driver. Sometime user use old version dts. 
Double check here will help identify the dts problem. 

Frank 
> 
> regards
> Philipp