Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 1 Apr 2022 15:43:38 -0500
From:   Rob Herring <robh@kernel.org>
To:     Richard Zhu <hongxing.zhu@nxp.com>
Cc:     l.stach@pengutronix.de, bhelgaas@google.com, broonie@kernel.org,
        lorenzo.pieralisi@arm.com, jingoohan1@gmail.com,
        festevam@gmail.com, francesco.dolcini@toradex.com,
        linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, kernel@pengutronix.de,
        linux-imx@nxp.com
Subject: Re: [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit()
 callback
Message-ID: <Ykdj+oJ+0k8UZCwK@robh.at.kernel.org>
References: <1645760667-10510-1-git-send-email-hongxing.zhu@nxp.com>
 <1645760667-10510-7-git-send-email-hongxing.zhu@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1645760667-10510-7-git-send-email-hongxing.zhu@nxp.com>
Precedence: bulk

On Fri, Feb 25, 2022 at 11:44:25AM +0800, Richard Zhu wrote:
> When the link never comes up after ->host_init(), some drivers,
> especially those that don't support hotplug, want to turn off clocks
> and power supplies.

Isn't supporting hotplug or not a board level decision? And hotplug 
doesn't have to mean physical plug/unplug. For example, you could have a 
soldered down PCIe device which needs regulators, resets, clocks, etc. 
for that device to be initialized before the link comes up. If that 
device is handled by a module loaded some time later, then the link may 
be down when you probe.

I think the way this all needs to work is with runtime PM. If that's all 
in place, then either you shutdown clocks/power on timeout or via sysfs 
suspend. If there's a child device, then that should prevent suspending.

> Add a new ->host_exit() callback in dw_pcie_host_ops so these
> drivers can clean up if ->host_init() fails.

I'm not really a fan of adding more ops nor the ops which aren't too 
specific about what they do. 'init' and 'exit' can be anything. I'd 
rather see more specific ops with the DWC core driver in charge of 
sequence of operations and the state. 

Rob