| Message ID | 20231217051210.754832-3-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: imx6: Clean up and add imx95 pci support | expand |
On Sun, Dec 17, 2023 at 12:11:57AM -0500, Frank Li wrote: > Refactors the phy handling logic in the imx6 PCI driver by adding > IMX6_PCIE_FLAG_HAS_PHY bitmask define for drvdata::flags. > > The drvdata::flags and a bitmask ensures a cleaner and more scalable > switch-case structure for handling phy. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > > Notes: > Change from v1 to v3: > - none > > drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 2086214345e9a..91ba26a4b4c3d 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -60,6 +60,9 @@ enum imx6_pcie_variants { > #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0) > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1) > #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2) > +#define IMX6_PCIE_FLAG_HAS_PHY BIT(3) > + > +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val) > > #define IMX6_PCIE_MAX_CLKS 6 > > @@ -1277,6 +1280,13 @@ static int imx6_pcie_probe(struct platform_device *pdev) > if (ret) > return ret; > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY)) { > + imx6_pcie->phy = devm_phy_get(dev, "pcie-phy"); Can't you use devm_phy_optional_get()? This will return NULL if the PHY is not defined in DT. So you can use IS_ERR() to catch error if there are issues in acquiring PHY if defined and NULL can be safely passed to other PHY APIs like phy_init() as well. With this, you won't need a flag in drv_data and can also get rid of the condition around PHY APIs. - Mani > + if (IS_ERR(imx6_pcie->phy)) > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy), > + "failed to get pcie phy\n"); > + } > + > switch (imx6_pcie->drvdata->variant) { > case IMX7D: > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > @@ -1306,11 +1316,6 @@ static int imx6_pcie_probe(struct platform_device *pdev) > return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset), > "failed to get pcie apps reset control\n"); > > - imx6_pcie->phy = devm_phy_get(dev, "pcie-phy"); > - if (IS_ERR(imx6_pcie->phy)) > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy), > - "failed to get pcie phy\n"); > - > break; > default: > break; > @@ -1447,13 +1452,15 @@ static const struct imx6_pcie_drvdata drvdata[] = { > }, > [IMX8MM] = { > .variant = IMX8MM, > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > + IMX6_PCIE_FLAG_HAS_PHY, > .gpr = "fsl,imx8mm-iomuxc-gpr", > .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, > }, > [IMX8MP] = { > .variant = IMX8MP, > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > + IMX6_PCIE_FLAG_HAS_PHY, > .gpr = "fsl,imx8mp-iomuxc-gpr", > .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, > }, > @@ -1465,12 +1472,14 @@ static const struct imx6_pcie_drvdata drvdata[] = { > }, > [IMX8MM_EP] = { > .variant = IMX8MM_EP, > + .flags = IMX6_PCIE_FLAG_HAS_PHY, > .mode = DW_PCIE_EP_TYPE, > .gpr = "fsl,imx8mm-iomuxc-gpr", > .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, > }, > [IMX8MP_EP] = { > .variant = IMX8MP_EP, > + .flags = IMX6_PCIE_FLAG_HAS_PHY, > .mode = DW_PCIE_EP_TYPE, > .gpr = "fsl,imx8mp-iomuxc-gpr", > .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, > -- > 2.34.1 >
On Sun, Dec 17, 2023 at 10:48:11PM +0530, Manivannan Sadhasivam wrote: > On Sun, Dec 17, 2023 at 12:11:57AM -0500, Frank Li wrote: > > Refactors the phy handling logic in the imx6 PCI driver by adding > > IMX6_PCIE_FLAG_HAS_PHY bitmask define for drvdata::flags. > > > > The drvdata::flags and a bitmask ensures a cleaner and more scalable > > switch-case structure for handling phy. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > > > Notes: > > Change from v1 to v3: > > - none > > > > drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > index 2086214345e9a..91ba26a4b4c3d 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -60,6 +60,9 @@ enum imx6_pcie_variants { > > #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0) > > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1) > > #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2) > > +#define IMX6_PCIE_FLAG_HAS_PHY BIT(3) > > + > > +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val) > > > > #define IMX6_PCIE_MAX_CLKS 6 > > > > @@ -1277,6 +1280,13 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > if (ret) > > return ret; > > > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY)) { > > + imx6_pcie->phy = devm_phy_get(dev, "pcie-phy"); > > Can't you use devm_phy_optional_get()? This will return NULL if the PHY is not > defined in DT. So you can use IS_ERR() to catch error if there are issues in > acquiring PHY if defined and NULL can be safely passed to other PHY APIs like > phy_init() as well. > > With this, you won't need a flag in drv_data and can also get rid of the > condition around PHY APIs. The key is the driver need know which platform "pcie-phy" is manditary. devm_phy_optional_get() can work if dts is correct. use HAS_PHY flags just double check if dts correct add "pcie-phy" at dts. This is just one place to check. and Help debug dtb miss match issues. Frank > > - Mani > > > + if (IS_ERR(imx6_pcie->phy)) > > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy), > > + "failed to get pcie phy\n"); > > + } > > + > > switch (imx6_pcie->drvdata->variant) { > > case IMX7D: > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > > @@ -1306,11 +1316,6 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset), > > "failed to get pcie apps reset control\n"); > > > > - imx6_pcie->phy = devm_phy_get(dev, "pcie-phy"); > > - if (IS_ERR(imx6_pcie->phy)) > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy), > > - "failed to get pcie phy\n"); > > - > > break; > > default: > > break; > > @@ -1447,13 +1452,15 @@ static const struct imx6_pcie_drvdata drvdata[] = { > > }, > > [IMX8MM] = { > > .variant = IMX8MM, > > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > > + IMX6_PCIE_FLAG_HAS_PHY, > > .gpr = "fsl,imx8mm-iomuxc-gpr", > > .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, > > }, > > [IMX8MP] = { > > .variant = IMX8MP, > > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > > + IMX6_PCIE_FLAG_HAS_PHY, > > .gpr = "fsl,imx8mp-iomuxc-gpr", > > .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, > > }, > > @@ -1465,12 +1472,14 @@ static const struct imx6_pcie_drvdata drvdata[] = { > > }, > > [IMX8MM_EP] = { > > .variant = IMX8MM_EP, > > + .flags = IMX6_PCIE_FLAG_HAS_PHY, > > .mode = DW_PCIE_EP_TYPE, > > .gpr = "fsl,imx8mm-iomuxc-gpr", > > .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, > > }, > > [IMX8MP_EP] = { > > .variant = IMX8MP_EP, > > + .flags = IMX6_PCIE_FLAG_HAS_PHY, > > .mode = DW_PCIE_EP_TYPE, > > .gpr = "fsl,imx8mp-iomuxc-gpr", > > .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, > > -- > > 2.34.1 > > > > -- > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 2086214345e9a..91ba26a4b4c3d 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -60,6 +60,9 @@ enum imx6_pcie_variants { #define IMX6_PCIE_FLAG_IMX6_PHY BIT(0) #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1) #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2) +#define IMX6_PCIE_FLAG_HAS_PHY BIT(3) + +#define imx6_check_flag(pci, val) (pci->drvdata->flags & val) #define IMX6_PCIE_MAX_CLKS 6 @@ -1277,6 +1280,13 @@ static int imx6_pcie_probe(struct platform_device *pdev) if (ret) return ret; + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY)) { + imx6_pcie->phy = devm_phy_get(dev, "pcie-phy"); + if (IS_ERR(imx6_pcie->phy)) + return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy), + "failed to get pcie phy\n"); + } + switch (imx6_pcie->drvdata->variant) { case IMX7D: if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) @@ -1306,11 +1316,6 @@ static int imx6_pcie_probe(struct platform_device *pdev) return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset), "failed to get pcie apps reset control\n"); - imx6_pcie->phy = devm_phy_get(dev, "pcie-phy"); - if (IS_ERR(imx6_pcie->phy)) - return dev_err_probe(dev, PTR_ERR(imx6_pcie->phy), - "failed to get pcie phy\n"); - break; default: break; @@ -1447,13 +1452,15 @@ static const struct imx6_pcie_drvdata drvdata[] = { }, [IMX8MM] = { .variant = IMX8MM, - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | + IMX6_PCIE_FLAG_HAS_PHY, .gpr = "fsl,imx8mm-iomuxc-gpr", .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, }, [IMX8MP] = { .variant = IMX8MP, - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | + IMX6_PCIE_FLAG_HAS_PHY, .gpr = "fsl,imx8mp-iomuxc-gpr", .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, }, @@ -1465,12 +1472,14 @@ static const struct imx6_pcie_drvdata drvdata[] = { }, [IMX8MM_EP] = { .variant = IMX8MM_EP, + .flags = IMX6_PCIE_FLAG_HAS_PHY, .mode = DW_PCIE_EP_TYPE, .gpr = "fsl,imx8mm-iomuxc-gpr", .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"}, }, [IMX8MP_EP] = { .variant = IMX8MP_EP, + .flags = IMX6_PCIE_FLAG_HAS_PHY, .mode = DW_PCIE_EP_TYPE, .gpr = "fsl,imx8mp-iomuxc-gpr", .clk_names = {IMX6_CLKS_COMMON, "pcie_aux"},
Refactors the phy handling logic in the imx6 PCI driver by adding IMX6_PCIE_FLAG_HAS_PHY bitmask define for drvdata::flags. The drvdata::flags and a bitmask ensures a cleaner and more scalable switch-case structure for handling phy. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- Notes: Change from v1 to v3: - none drivers/pci/controller/dwc/pci-imx6.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)