[PATCH v14 14/17] PCI: imx6: Do not hide phy driver callbacks and refine the error handling

[Lucas Stach]

Am Mittwoch, dem 13.07.2022 um 10:57 +0000 schrieb Hongxing Zhu:
> > -----Original Message-----
> > From: Lucas Stach <l.stach at pengutronix.de>
> > Sent: 2022年7月13日 16:59
> > To: Hongxing Zhu <hongxing.zhu at nxp.com>; bhelgaas at google.com;
> > robh+dt at kernel.org; broonie at kernel.org; lorenzo.pieralisi at arm.com;
> > festevam at gmail.com; francesco.dolcini at toradex.com
> > Cc: linux-pci at vger.kernel.org;
> > linux-arm-kernel at lists.infradead.org;
> > linux-kernel at vger.kernel.org; kernel at pengutronix.de; dl-linux-imx
> > <linux-imx at nxp.com>
> > Subject: Re: [PATCH v14 14/17] PCI: imx6: Do not hide phy driver
> > callbacks and
> > refine the error handling
> > 
> > Am Freitag, dem 01.07.2022 um 11:25 +0800 schrieb Richard Zhu:
> > > - Move the phy_power_on() to host_init from
> > > imx6_pcie_clk_enable().
> > > - Move the phy_init() to host_init from
> > > imx6_pcie_deassert_core_reset().
> > > 
> > > Refine the error handling in imx6_pcie_host_init() accordingly.
> > > 
> > > Signed-off-by: Richard Zhu <hongxing.zhu at nxp.com>
> > > Signed-off-by: Bjorn Helgaas <bhelgaas at google.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-imx6.c | 34 +++++++++++++++++----
> > > ------
> > >  1 file changed, 21 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 5a06fbca82d6..0b2a5256fb0d 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -639,14 +639,6 @@ static int imx6_pcie_clk_enable(struct
> > > imx6_pcie
> > *imx6_pcie)
> > >  		goto err_ref_clk;
> > >  	}
> > > 
> > > -	switch (imx6_pcie->drvdata->variant) {
> > > -	case IMX8MM:
> > > -		if (phy_power_on(imx6_pcie->phy))
> > > -			dev_err(dev, "unable to power on
> > > PHY\n");
> > > -		break;
> > > -	default:
> > > -		break;
> > > -	}
> > >  	/* allow the clocks to stabilize */
> > >  	usleep_range(200, 500);
> > >  	return 0;
> > > @@ -723,10 +715,6 @@ static int
> > > imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  	case IMX8MQ:
> > >  		reset_control_deassert(imx6_pcie-
> > > >pciephy_reset);
> > >  		break;
> > > -	case IMX8MM:
> > > -		if (phy_init(imx6_pcie->phy))
> > > -			dev_err(dev, "waiting for phy ready
> > > timeout!\n");
> > > -		break;
> > >  	case IMX7D:
> > >  		reset_control_deassert(imx6_pcie-
> > > >pciephy_reset);
> > > 
> > > @@ -762,6 +750,7 @@ static int
> > > imx6_pcie_deassert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> > >  		usleep_range(200, 500);
> > >  		break;
> > >  	case IMX6Q:		/* Nothing to do */
> > > +	case IMX8MM:
> > >  		break;
> > >  	}
> > > 
> > > @@ -913,17 +902,36 @@ static int imx6_pcie_host_init(struct
> > > pcie_port
> > *pp)
> > >  			return ret;
> > >  		}
> > >  	}
> > > +	if (imx6_pcie->phy) {
> > > +		ret = phy_power_on(imx6_pcie->phy);
> > > +		if (ret) {
> > > +			dev_err(dev, "pcie phy power up
> > > failed.\n");
> > > +			goto err_reg_disable;
> > > +		}
> > > +	}
> > > 
> > >  	ret = imx6_pcie_deassert_core_reset(imx6_pcie);
> > >  	if (ret < 0) {
> > >  		dev_err(dev, "pcie deassert core reset failed:
> > > %d\n", ret);
> > > -		goto err_reg_disable;
> > > +		goto err_phy_off;
> > >  	}
> > > 
> > > +	if (imx6_pcie->phy) {
> > > +		ret = phy_init(imx6_pcie->phy);
> > > +		if (ret) {
> > > +			dev_err(dev, "waiting for phy ready
> > > timeout!\n");
> > > +			goto err_clk_disable;
> > > +		}
> > > +	}
> > 
> > Wouldn't it be more logical to put this into imx6_pcie_init_phy()?
> > 
> Before adding i.MX8MM PCIe support, the imx6_pcie_init_phy() only
> touches the
>  GPR registers. PCIe clocks and so on are not required in this case.
> But phy_init() used by i.MX8MM PCIe touches not only the GPR
> registers but
>  also the PHY's registers.
> The clocks should be on and resets of PHY should be configured
> properly when
>  phy_init() is invoked.
> So, phy_init() is placed behind of imx6_pcie_deassert_core_reset()
> here.

The PHY driver should be self-contained enough to not care about the
state of the controller here, no? It should set all the necessary GPRs
and enable clocks as needed on its own. Is this not the case with the
current code?

Also PHY init should be called before PHY power-on, to make things
symmetric with the shutdown paths which do phy_power_off() first, then
phy_exit().

Regards,
Lucas