| Message ID | 1645760667-10510-7-git-send-email-hongxing.zhu@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: imx6: refine codes and add compliance tests mode support | expand |
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
> -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: 2022年4月2日 4:44 > To: Hongxing 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; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() > callback > > 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. Hi Rob: Thanks a lot for your review comments. Understand what you mean. i.MX PCIe doesn't support hot-plug from chip design view. The ops.host_exit() callback is invoked only when the iMX PCIe driver hooked callback ops->start_link return an error. For the platforms, that support the hot-plug feature, they can just return one zero from their own ops->start_link. In the current situation, i.MX PCIe does just return one zero when probe failed. See the discussion and commit issued by Fabio below. https://patchwork.kernel.org/project/linux-pci/patch/1641368602-20401-6-git-send-email-hongxing.zhu@nxp.com/ https://patchwork.ozlabs.org/project/linux-pci/patch/20220106103645.2790803-1-festevam@gmail.com/ > > > 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. Understand. i.MX PCIe can't handle the error exit properly in this case by itself. So I add one more ops.host_exit() in this series. Best Regards Richard Zhu > > Rob
On Sat, Apr 02, 2022 at 03:03:00AM +0000, Hongxing Zhu wrote: > > -----Original Message----- > > From: Rob Herring <robh@kernel.org> > > Sent: 2022年4月2日 4:44 > > To: Hongxing 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; dl-linux-imx <linux-imx@nxp.com> > > Subject: Re: [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() > > callback > > > > 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. > Hi Rob: > Thanks a lot for your review comments. > Understand what you mean. > i.MX PCIe doesn't support hot-plug from chip design view. The scenario I described is not hotplug. > The ops.host_exit() callback is invoked only when the iMX PCIe driver hooked > callback ops->start_link return an error. > For the platforms, that support the hot-plug feature, they can just return one > zero from their own ops->start_link. You cannot have a per board start_link(). > In the current situation, i.MX PCIe does just return one zero when probe failed. > See the discussion and commit issued by Fabio below. > https://patchwork.kernel.org/project/linux-pci/patch/1641368602-20401-6-git-send-email-hongxing.zhu@nxp.com/ > https://patchwork.ozlabs.org/project/linux-pci/patch/20220106103645.2790803-1-festevam@gmail.com/ That's a stable fix and different discussion. > > > 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. > Understand. You don't seem to... > i.MX PCIe can't handle the error exit properly in this case by itself. So I > add one more ops.host_exit() in this series. > > Best Regards > Richard Zhu > > > > Rob
> -----Original Message----- > From: Rob Herring <robh@kernel.org> > Sent: 2022年4月4日 22:40 > To: Hongxing 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; dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH v8 6/8] PCI: dwc: Add dw_pcie_host_ops.host_exit() > callback > > On Sat, Apr 02, 2022 at 03:03:00AM +0000, Hongxing Zhu wrote: > > > -----Original Message----- > > > From: Rob Herring <robh@kernel.org> > > > Sent: 2022年4月2日 4:44 > > > To: Hongxing 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; dl-linux-imx <linux-imx@nxp.com> > > > Subject: Re: [PATCH v8 6/8] PCI: dwc: Add > > > dw_pcie_host_ops.host_exit() callback > > > > > > 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. > > Hi Rob: > > Thanks a lot for your review comments. > > Understand what you mean. > > i.MX PCIe doesn't support hot-plug from chip design view. > > The scenario I described is not hotplug. Hi Rob: Sorry, I didn't get what you means before. In the hotplug is not supported scenario. Refer to my understand, both pci_host_probe->pci_scan_root_bus_bridge and LTSSM training enable are one shot callback during i.MX PCIe probe procedure. How the example you described in the previous emails can work well? Please correct me if I'm wrong. Best Regards Richard Zhu > > > > The ops.host_exit() callback is invoked only when the iMX PCIe driver > > hooked callback ops->start_link return an error. > > For the platforms, that support the hot-plug feature, they can just > > return one zero from their own ops->start_link. > > You cannot have a per board start_link(). > > > In the current situation, i.MX PCIe does just return one zero when probe > failed. > > See the discussion and commit issued by Fabio below. > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > > hwork.kernel.org%2Fproject%2Flinux-pci%2Fpatch%2F1641368602-20401-6-g > i > > > t-send-email-hongxing.zhu%40nxp.com%2F&data=04%7C01%7Chongxing > .zhu > > %40nxp.com%7Cb23fff6b2c264410da0a08da1648f105%7C686ea1d3bc2b4 > c6fa92cd9 > > > 9c5c301635%7C0%7C1%7C637846799780646784%7CUnknown%7CTWFpbG > Zsb3d8eyJWIj > > > oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30 > 00&am > > > p;sdata=fE9UxCq4S5K%2BUTPWfFR5h5r56%2B26g7anlPKvklo9sKw%3D& > reserve > > d=0 > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > > hwork.ozlabs.org%2Fproject%2Flinux-pci%2Fpatch%2F20220106103645.2790 > 80 > > > 3-1-festevam%40gmail.com%2F&data=04%7C01%7Chongxing.zhu%40nx > p.com% > > > 7Cb23fff6b2c264410da0a08da1648f105%7C686ea1d3bc2b4c6fa92cd99c5c3 > 01635% > > > 7C0%7C1%7C637846799780646784%7CUnknown%7CTWFpbGZsb3d8eyJWIj > oiMC4wLjAwM > > > DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata > =OG4 > > > 5XQBOdqkH6B9KYmqU83mNs1wuHiOCb%2B3oL9ad6SQ%3D&reserved= > 0 > > That's a stable fix and different discussion. > > > > > 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. > > Understand. > > You don't seem to... > > > i.MX PCIe can't handle the error exit properly in this case by itself. > > So I add one more ops.host_exit() in this series. > > > > Best Regards > > Richard Zhu > > > > > > Rob
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index f4755f3a03be..461863bde3c9 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -405,7 +405,7 @@ int dw_pcie_host_init(struct pcie_port *pp) if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) { ret = pci->ops->start_link(pci); if (ret) - goto err_free_msi; + goto err_host_init; } /* Ignore errors, the link may come up later */ @@ -417,6 +417,9 @@ int dw_pcie_host_init(struct pcie_port *pp) if (!ret) return 0; +err_host_init: + if (pp->ops->host_exit) + pp->ops->host_exit(pp); err_free_msi: if (pp->has_msi_ctrl) dw_pcie_free_msi(pp); diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 7d6e9b7576be..1153687ea9a6 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -174,6 +174,7 @@ enum dw_pcie_device_mode { struct dw_pcie_host_ops { int (*host_init)(struct pcie_port *pp); + void (*host_exit)(struct pcie_port *pp); int (*msi_host_init)(struct pcie_port *pp); };
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. Add a new ->host_exit() callback in dw_pcie_host_ops so these drivers can clean up if ->host_init() fails. Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> --- drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-)