Re: [PATCH 2/5] PCI: intel-gw: Enable clock before phy init

[Manivannan Sadhasivam]

On Fri, Mar 27, 2026 at 08:42:21AM +0100, Florian Eckert wrote:
> Hello Mani,
> 
> Thanks for your review.
> 
> On 2026-03-26 17:08, Manivannan Sadhasivam wrote:
> > On Tue, Mar 17, 2026 at 11:12:50AM +0100, Florian Eckert wrote:
> > > To ensure that the boot sequence is correct, the dwc pcie core clock
> > > must
> > > be switched on before phy init call.
> > > 
> > > This changes are based on patched kernel sources of the MaxLinear SDK,
> > > which can be found at https://github.com/maxlinear/linux
> > > 
> > 
> > How can we treat Maxlinear kernel source as a reference for Intel
> > driver?
> > Atleast, you need to provide some info onto why this should be trusted
> > as a
> > reference like used a product etc...
> 
> As far as I know, this driver is only used by Maxlinear’s URX851 and URX850
> SoCs. However, the chip was originally developed by Intel when they
> acquired Lantiq’s home networking division in 2015 [1] for this SoCs.
> In 2020 the home network division was sold to Maxlinear [2].
> 
> Since then, Maxlinear has been responsible for the driver. However, their
> SDK is outdated and based on kernel 5.15. Other than that, not much is
> happening! Even the developers listed as maintainers can no longer be
> reached. When it came to the patch set, the email couldn't be delivered to
> the responsible developer 'Chuanhua Lei <lchuanhua@xxxxxxxxxxxxx>' either.
> The email bounced back.
> 
> The company I work for is using the chip and is currently in the process of
> extracting the key components from the SDK so that the SoC can work again
> with
> a mainline kernel again.
> 

Ok, thanks for the background.

- Mani

> [1] https://www.intc.com/news-events/press-releases/detail/364/intel-to-acquire-lantiq-advancing-the-connected-home
> [2] https://investors.maxlinear.com/press-releases/detail/395/maxlinear-to-acquire-intels-home-gateway-platform
> 
> > > Signed-off-by: Florian Eckert <fe@xxxxxxxxxx>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-intel-gw.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > index 3a85bd0ef1b7f9414ce19fe56d82a78e34e9b648..6110a8adb8732dbbd5e9e2db68a0606ccf032ae1
> > > 100644
> > > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > > @@ -292,13 +292,9 @@ static int intel_pcie_host_setup(struct
> > > intel_pcie *pcie)
> > > 
> > >  	intel_pcie_core_rst_assert(pcie);
> > >  	intel_pcie_device_rst_assert(pcie);
> > > -
> > > -	ret = phy_init(pcie->phy);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > >  	intel_pcie_core_rst_deassert(pcie);
> > > 
> > > +	/* Controller clock must be provided earlier than PHY */
> > >  	ret = clk_prepare_enable(pcie->core_clk);
> > >  	if (ret) {
> > >  		dev_err(pcie->pci.dev, "Core clock enable failed: %d\n", ret);
> > > @@ -307,13 +303,17 @@ static int intel_pcie_host_setup(struct
> > > intel_pcie *pcie)
> > > 
> > >  	pci->atu_base = pci->dbi_base + 0xC0000;
> > > 
> > > +	ret = phy_init(pcie->phy);
> > > +	if (ret)
> > > +		goto phy_err;
> > 
> > You just messed up the whole error path. This one should branch to
> > err_disable_core_clk that disables core_clk and deasserts reset.
> > 
> > > +
> > >  	intel_pcie_ltssm_disable(pcie);
> > >  	intel_pcie_link_setup(pcie);
> > >  	intel_pcie_init_n_fts(pci);
> > > 
> > >  	ret = dw_pcie_setup_rc(&pci->pp);
> > >  	if (ret)
> > > -		goto app_init_err;
> > > +		goto phy_err;
> > 
> > And this to err_phy_exit that does phy_exit().
> > 
> > > 
> > >  	dw_pcie_upconfig_setup(pci);
> > > 
> > > @@ -322,13 +322,13 @@ static int intel_pcie_host_setup(struct
> > > intel_pcie *pcie)
> > > 
> > >  	ret = dw_pcie_wait_for_link(pci);
> > >  	if (ret)
> > > -		goto app_init_err;
> > > +		goto phy_err;
> > > 
> > >  	intel_pcie_core_irq_enable(pcie);
> > > 
> > >  	return 0;
> > > 
> > 
> > Here, you should add shuffle phy_exit() accordingly.
> 
> I'll take another look at that.
> 
> Thanks
> 
> - Florian

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