Re: [PATCH v10 2/2] PCI: eic7700: Add Eswin PCIe host controller driver

[Manivannan Sadhasivam]

On Thu, Feb 26, 2026 at 04:09:44PM +0800, zhangsenchuan wrote:
> 
> > > > Subject: Re: [PATCH v10 2/2] PCI: eic7700: Add Eswin PCIe host controller driver
> > > > 
> > > > On Thu, Jan 29, 2026 at 05:29:00PM +0800, zhangsenchuan@xxxxxxxxxxxxxxxxxx wrote:
> > > > > From: Senchuan Zhang <zhangsenchuan@xxxxxxxxxxxxxxxxxx>
> > > > >
> > > > > +static int eic7700_pcie_suspend_noirq(struct device *dev)
> > > > > +{
> > > > > +	struct eic7700_pcie *pcie = dev_get_drvdata(dev);
> > > > > +
> > > > > +	/*
> > > > > +	 * The ESWIN EIC7700 SoC lacks hardware support for the L2/L3 low-power
> > > > > +	 * link states. It cannot enter the L2/L3 Ready state through the
> > > > > +	 * PME_Turn_Off/PME_To_Ack handshake protocol. To avoid this problem,
> > > > > +	 * the dw_pcie_suspend_noirq API is not used.
> > > > > +	 */
> > > > 
> > > > With 7.0, you can provide a dummy pme_turn_off() API and set
> > > > 'pci->pp.skip_l23_ready' to reuse the dw_pcie_{suspend/resume}_noirq APIs.
> > > > 
> > > 
> > > Hi Mani,
> > > 
> > > Setting pci->pp.skip_l23_ready does indeed allow us to reuse the
> > > dw_pcie_suspend_noirq function. However, for the dw_pcie_resume_noirq
> > > function, if the dw_pcie_start_link and dw_pcie_wait_for_link APIs fail to
> > > execute, the clk/reset resources in the pci->pp.ops->init function cannot
> > > be released. Perhaps the dw_pcie_resume_noirq function needs to be optimized.
> > 
> > Will this help?
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 6ae6189e9b8a..38ad79bbeab1 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -1300,15 +1300,24 @@ int dw_pcie_resume_noirq(struct dw_pcie *pci)
> >  
> >         ret = dw_pcie_start_link(pci);
> >         if (ret)
> > -               return ret;
> > +               goto err_deinit;
> >  
> >         ret = dw_pcie_wait_for_link(pci);
> >         if (ret)
> > -               return ret;
> > +               goto err_stop_link;
> >  
> >         if (pci->pp.ops->post_init)
> >                 pci->pp.ops->post_init(&pci->pp);
> >  
> > +       return 0;
> > +
> > +err_stop_link:
> > +       dw_pcie_stop_link(pci);
> > +
> > +err_deinit:
> > +       if (pci->pp.ops->deinit)
> > +               pci->pp.ops->deinit(&pci->pp);
> > +
> >         return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_resume_noirq);
> > 
> 
> Yes, this can release the resources after init, after optimizing the 
> resume function, i can reuse the dw_pcie_{suspend/resume}_noirq APIs.
> 
> I noticed that the dw_pcie_wait_for_link function has been optimized. Is 
> it necessary to release the resources only when it return -ETIMEOUT?
> Perhaps it needs to be slightly improved：
> 
>  ret = dw_pcie_wait_for_link(pci);
>  if (ret == -ETIMEOUT)
>          goto err_stop_link;
> 
> What about your suggestion?
> 

Absolutely! I forgot my own rework ;) I'll cook a patch for the above. Then if
you base your controller driver patch on top of it, we can merge both in a
single tree (if Bjorn agrees).

- Mani

-- 
மணிவண்ணன் சதாசிவம்