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 <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware-host.c | 5 ++++-
drivers/pci/controller/dwc/pcie-designware.h | 1 +
2 files changed, 5 insertions(+), 1 deletion(-)
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);
};
--
2.25.1
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
On Sat, Apr 02, 2022 at 03:03:00AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Rob Herring <[email protected]>
> > Sent: 2022年4月2日 4:44
> > To: Hongxing Zhu <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; dl-linux-imx <[email protected]>
> > 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/[email protected]/
> https://patchwork.ozlabs.org/project/linux-pci/patch/[email protected]/
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 <[email protected]>
> Sent: 2022??4??2?? 4:44
> To: Hongxing Zhu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>
> 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/[email protected]/
https://patchwork.ozlabs.org/project/linux-pci/patch/[email protected]/
>
> > 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
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: 2022??4??4?? 22:40
> To: Hongxing Zhu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; dl-linux-imx <[email protected]>
> 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 <[email protected]>
> > > Sent: 2022??4??2?? 4:44
> > > To: Hongxing Zhu <[email protected]>
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; dl-linux-imx <[email protected]>
> > > 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