diff mbox series

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

Message ID 20240617-pci2_upstream-v6-9-e0821238f997@nxp.com (mailing list archive)
State Superseded
Headers show
Series PCI: imx6: Fix\rename\clean up and add lut information for imx95 | expand

Commit Message

Frank Li June 17, 2024, 8:16 p.m. UTC
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@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Manivannan Sadhasivam June 30, 2024, 4:23 p.m. UTC | #1
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@nxp.com>
> ---
>  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.

>  	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
Frank Li July 8, 2024, 4:01 p.m. UTC | #2
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@nxp.com>
> > ---
> >  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
> 
> -- 
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

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>
 #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 (!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);
 	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");
+			goto err_phy_off;
+		}
+
 		ret = phy_power_on(imx_pcie->phy);
 		if (ret) {
 			dev_err(dev, "waiting for PHY ready timeout!\n");