Re: [PATCH v6 09/10] PCI: imx6: Call: Common PHY API to set mode, speed, and submode

[Frank Li]

On Sun, Jun 30, 2024 at 09:53:37PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jun 17, 2024 at 04:16:45PM -0400, Frank Li wrote:
> 
> You don't need the colon after 'Call' in subject.
> 
> > Invoke the common PHY API to configure mode, speed, and submode. While
> > these functions are optional in the PHY interface, they are necessary for
> > certain PHY drivers. Lack of support for these functions in a PHY driver
> > does not cause harm.
> > 
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index ab0ed7ab3007a..18c133f5a56fc 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -30,6 +30,7 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/reset.h>
> >  #include <linux/phy/phy.h>
> > +#include <linux/phy/pcie.h>
> 
> This should be moved one entry above.
> 
> >  #include <linux/pm_domain.h>
> >  #include <linux/pm_runtime.h>
> >  
> > @@ -229,6 +230,10 @@ static void imx_pcie_configure_type(struct imx_pcie *imx_pcie)
> >  
> >  	id = imx_pcie->controller_id;
> >  
> > +	/* If mode_mask[0] is 0, means use phy driver to set mode */
> 
> /* If mode_mask is 0, then generic PHY driver is used to set the mode */
> 
> > +	if (!drvdata->mode_mask[0])
> > +		return;
> > +
> >  	/* If mode_mask[id] is zero, means each controller have its individual gpr */
> >  	if (!drvdata->mode_mask[id])
> >  		id = 0;
> > @@ -808,6 +813,7 @@ static void imx_pcie_ltssm_enable(struct device *dev)
> >  	struct imx_pcie *imx_pcie = dev_get_drvdata(dev);
> >  	const struct imx_pcie_drvdata *drvdata = imx_pcie->drvdata;
> >  
> > +	phy_set_speed(imx_pcie->phy, PCI_EXP_LNKCAP_SLS_2_5GB);
> 
> Is this setting really universal? This looks like applicable only to specific
> platforms supporting this link speed.

phy layer just ignore it if phy driver don't support set_speed, like other
phy API. Check platform will make code complex and without benefit.

But it should be better to set SPEED as the same as CAP register.

> 
> >  	if (drvdata->ltssm_mask)
> >  		regmap_update_bits(imx_pcie->iomuxc_gpr, drvdata->ltssm_off, drvdata->ltssm_mask,
> >  				   drvdata->ltssm_mask);
> > @@ -820,6 +826,7 @@ static void imx_pcie_ltssm_disable(struct device *dev)
> >  	struct imx_pcie *imx_pcie = dev_get_drvdata(dev);
> >  	const struct imx_pcie_drvdata *drvdata = imx_pcie->drvdata;
> >  
> > +	phy_set_speed(imx_pcie->phy, 0);
> >  	if (drvdata->ltssm_mask)
> >  		regmap_update_bits(imx_pcie->iomuxc_gpr, drvdata->ltssm_off,
> >  				   drvdata->ltssm_mask, 0);
> > @@ -955,6 +962,12 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
> >  			goto err_clk_disable;
> >  		}
> >  
> > +		ret = phy_set_mode_ext(imx_pcie->phy, PHY_MODE_PCIE, PHY_MODE_PCIE_RC);
> > +		if (ret) {
> > +			dev_err(dev, "unable to set pcie PHY mode\n");
> 
> s/pcie/PCIe
> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்