[PATCH v2] soc: imx: imx8mp-blk-ctrl: Add PCIe SYSPLL configurations

[PATCH v2] soc: imx: imx8mp-blk-ctrl: Add PCIe SYSPLL configurations

[Richard Zhu]

Add PCIe SYSPLL configurations, thus the internal SYSPLL can be used as
i.MX8MP PCIe reference clock.

The following properties of PHY dts node should be changed accordingly.
  - Set 'fsl,refclk-pad-mode' as '<IMX8_PCIE_REFCLK_PAD_OUTPUT>'.
  - Change 'clocks' to '<&clk IMX8MP_CLK_HSIO_AXI>'.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
v1->v2:
Refer to Lucas' comments, don't expose IMX8MP_CLK_HSIO_ROOT to dts node.
https://patchwork.ozlabs.org/project/linux-pci/patch/1666590189-1364-1-git-send-email-hongxing.zhu@nxp.com/

Use <&clk IMX8MP_CLK_HSIO_AXI> as referrence clock source when internal
clock mode is used by i.MX8MP PCIe module.

Verified on i.MX8MP EVK board with removing R131/R132/R137/R138, and
populating R135/R136.
---
 drivers/soc/imx/imx8mp-blk-ctrl.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c
index 0e3b6ba22f94..5ad20a8ea25e 100644
--- a/drivers/soc/imx/imx8mp-blk-ctrl.c
+++ b/drivers/soc/imx/imx8mp-blk-ctrl.c
@@ -21,6 +21,16 @@
 #define  USB_CLOCK_MODULE_EN	BIT(1)
 #define  PCIE_PHY_APB_RST	BIT(4)
 #define  PCIE_PHY_INIT_RST	BIT(5)
+#define GPR_REG2		0x8
+#define  P_PLL_MASK		GENMASK(5, 0)
+#define  M_PLL_MASK		GENMASK(15, 6)
+#define  S_PLL_MASK		GENMASK(18, 16)
+#define  P_PLL			(0xc << 0)
+#define  M_PLL			(0x320 << 6)
+#define  S_PLL			(0x4 << 16)
+#define GPR_REG3		0xc
+#define  PLL_CKE		BIT(17)
+#define  PLL_RST		BIT(31)
 
 struct imx8mp_blk_ctrl_domain;
 
@@ -86,6 +96,18 @@ static void imx8mp_hsio_blk_ctrl_power_on(struct imx8mp_blk_ctrl *bc,
 	case IMX8MP_HSIOBLK_PD_PCIE_PHY:
 		regmap_set_bits(bc->regmap, GPR_REG0,
 				PCIE_PHY_APB_RST | PCIE_PHY_INIT_RST);
+
+		/* Set the PLL configurations, P = 12, M = 800, S = 4. */
+		regmap_update_bits(bc->regmap, GPR_REG2,
+				   P_PLL_MASK | M_PLL_MASK | S_PLL_MASK,
+				   P_PLL | M_PLL | S_PLL);
+		udelay(1);
+
+		regmap_update_bits(bc->regmap, GPR_REG3, PLL_RST, PLL_RST);
+		udelay(10);
+
+		/* Set 1b'1 to pll_cke of GPR_REG3 */
+		regmap_update_bits(bc->regmap, GPR_REG3, PLL_CKE, PLL_CKE);
 		break;
 	default:
 		break;
-- 
2.25.1

Re: [PATCH v2] soc: imx: imx8mp-blk-ctrl: Add PCIe SYSPLL configurations

[Marcel Ziswiler]

Hi Richard

On Fri, 2022-11-18 at 10:56 +0800, Richard Zhu wrote:
> Add PCIe SYSPLL configurations, thus the internal SYSPLL can be used as
> i.MX8MP PCIe reference clock.
> 
> The following properties of PHY dts node should be changed accordingly.
>   - Set 'fsl,refclk-pad-mode' as '<IMX8_PCIE_REFCLK_PAD_OUTPUT>'.
>   - Change 'clocks' to '<&clk IMX8MP_CLK_HSIO_AXI>'.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

This works perfectly fine on Verdin iMX8M Plus as well. I will update my previous PCIe-enablement commit
(device tree part) in that respect and (re-)submit it.

Thanks!

Cheers

Marcel

> ---
> v1->v2:
> Refer to Lucas' comments, don't expose IMX8MP_CLK_HSIO_ROOT to dts node.
> https://patchwork.ozlabs.org/project/linux-pci/patch/1666590189-1364-1-git-send-email-hongxing.zhu@nxp.com/
> 
> Use <&clk IMX8MP_CLK_HSIO_AXI> as referrence clock source when internal
> clock mode is used by i.MX8MP PCIe module.
> 
> Verified on i.MX8MP EVK board with removing R131/R132/R137/R138, and
> populating R135/R136.
> ---
>  drivers/soc/imx/imx8mp-blk-ctrl.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c
> index 0e3b6ba22f94..5ad20a8ea25e 100644
> --- a/drivers/soc/imx/imx8mp-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c
> @@ -21,6 +21,16 @@
>  #define  USB_CLOCK_MODULE_EN   BIT(1)
>  #define  PCIE_PHY_APB_RST      BIT(4)
>  #define  PCIE_PHY_INIT_RST     BIT(5)
> +#define GPR_REG2               0x8
> +#define  P_PLL_MASK            GENMASK(5, 0)
> +#define  M_PLL_MASK            GENMASK(15, 6)
> +#define  S_PLL_MASK            GENMASK(18, 16)
> +#define  P_PLL                 (0xc << 0)
> +#define  M_PLL                 (0x320 << 6)
> +#define  S_PLL                 (0x4 << 16)
> +#define GPR_REG3               0xc
> +#define  PLL_CKE               BIT(17)
> +#define  PLL_RST               BIT(31)
>  
>  struct imx8mp_blk_ctrl_domain;
>  
> @@ -86,6 +96,18 @@ static void imx8mp_hsio_blk_ctrl_power_on(struct imx8mp_blk_ctrl *bc,
>         case IMX8MP_HSIOBLK_PD_PCIE_PHY:
>                 regmap_set_bits(bc->regmap, GPR_REG0,
>                                 PCIE_PHY_APB_RST | PCIE_PHY_INIT_RST);
> +
> +               /* Set the PLL configurations, P = 12, M = 800, S = 4. */
> +               regmap_update_bits(bc->regmap, GPR_REG2,
> +                                  P_PLL_MASK | M_PLL_MASK | S_PLL_MASK,
> +                                  P_PLL | M_PLL | S_PLL);
> +               udelay(1);
> +
> +               regmap_update_bits(bc->regmap, GPR_REG3, PLL_RST, PLL_RST);
> +               udelay(10);
> +
> +               /* Set 1b'1 to pll_cke of GPR_REG3 */
> +               regmap_update_bits(bc->regmap, GPR_REG3, PLL_CKE, PLL_CKE);
>                 break;
>         default:
>                 break;

Re: [PATCH v2] soc: imx: imx8mp-blk-ctrl: Add PCIe SYSPLL configurations

[Lucas Stach]

Am Freitag, dem 18.11.2022 um 10:56 +0800 schrieb Richard Zhu:
> Add PCIe SYSPLL configurations, thus the internal SYSPLL can be used as
> i.MX8MP PCIe reference clock.
> 
> The following properties of PHY dts node should be changed accordingly.
>   - Set 'fsl,refclk-pad-mode' as '<IMX8_PCIE_REFCLK_PAD_OUTPUT>'.
>   - Change 'clocks' to '<&clk IMX8MP_CLK_HSIO_AXI>'.

This is still not what I meant. There is no direct relation between the
PCIe PHY domain being powered up and the PCIe PLL being needed. The PLL
really only needs to be active when the reference clock isn't sourced
externally. So the HSIO blk-ctrl should expose a proper clock for the
PLL. As the PLL is effectively fixed rate it should be enough to expose
get_rate, prepare and unprepare clk ops.

The PHY should then point at the blk-ctrl as the reference clock
source.

Regards,
Lucas

> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---
> v1->v2:
> Refer to Lucas' comments, don't expose IMX8MP_CLK_HSIO_ROOT to dts node.
> https://patchwork.ozlabs.org/project/linux-pci/patch/1666590189-1364-1-git-send-email-hongxing.zhu@nxp.com/
> 
> Use <&clk IMX8MP_CLK_HSIO_AXI> as referrence clock source when internal
> clock mode is used by i.MX8MP PCIe module.
> 
> Verified on i.MX8MP EVK board with removing R131/R132/R137/R138, and
> populating R135/R136.
> ---
>  drivers/soc/imx/imx8mp-blk-ctrl.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c b/drivers/soc/imx/imx8mp-blk-ctrl.c
> index 0e3b6ba22f94..5ad20a8ea25e 100644
> --- a/drivers/soc/imx/imx8mp-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c
> @@ -21,6 +21,16 @@
>  #define  USB_CLOCK_MODULE_EN	BIT(1)
>  #define  PCIE_PHY_APB_RST	BIT(4)
>  #define  PCIE_PHY_INIT_RST	BIT(5)
> +#define GPR_REG2		0x8
> +#define  P_PLL_MASK		GENMASK(5, 0)
> +#define  M_PLL_MASK		GENMASK(15, 6)
> +#define  S_PLL_MASK		GENMASK(18, 16)
> +#define  P_PLL			(0xc << 0)
> +#define  M_PLL			(0x320 << 6)
> +#define  S_PLL			(0x4 << 16)
> +#define GPR_REG3		0xc
> +#define  PLL_CKE		BIT(17)
> +#define  PLL_RST		BIT(31)
>  
>  struct imx8mp_blk_ctrl_domain;
>  
> @@ -86,6 +96,18 @@ static void imx8mp_hsio_blk_ctrl_power_on(struct imx8mp_blk_ctrl *bc,
>  	case IMX8MP_HSIOBLK_PD_PCIE_PHY:
>  		regmap_set_bits(bc->regmap, GPR_REG0,
>  				PCIE_PHY_APB_RST | PCIE_PHY_INIT_RST);
> +
> +		/* Set the PLL configurations, P = 12, M = 800, S = 4. */
> +		regmap_update_bits(bc->regmap, GPR_REG2,
> +				   P_PLL_MASK | M_PLL_MASK | S_PLL_MASK,
> +				   P_PLL | M_PLL | S_PLL);
> +		udelay(1);
> +
> +		regmap_update_bits(bc->regmap, GPR_REG3, PLL_RST, PLL_RST);
> +		udelay(10);
> +
> +		/* Set 1b'1 to pll_cke of GPR_REG3 */
> +		regmap_update_bits(bc->regmap, GPR_REG3, PLL_CKE, PLL_CKE);
>  		break;
>  	default:
>  		break;

RE: [PATCH v2] soc: imx: imx8mp-blk-ctrl: Add PCIe SYSPLL configurations

[Hongxing Zhu]

> -----Original Message-----
> From: Lucas Stach <l.stach@pengutronix.de>
> Sent: 2022年11月18日 19:02
> To: Hongxing Zhu <hongxing.zhu@nxp.com>; Marcel Ziswiler
> <marcel.ziswiler@toradex.com>; marex@denx.de; tharvey@gateworks.com;
> bhelgaas@google.com; lorenzo.pieralisi@arm.com; shawnguo@kernel.org;
> alexander.stein@ew.tq-group.com; richard.leitner@linux.dev
> Cc: devicetree@vger.kernel.org; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH v2] soc: imx: imx8mp-blk-ctrl: Add PCIe SYSPLL
> configurations
> 
> Am Freitag, dem 18.11.2022 um 10:56 +0800 schrieb Richard Zhu:
> > Add PCIe SYSPLL configurations, thus the internal SYSPLL can be used
> > as i.MX8MP PCIe reference clock.
> >
> > The following properties of PHY dts node should be changed accordingly.
> >   - Set 'fsl,refclk-pad-mode' as '<IMX8_PCIE_REFCLK_PAD_OUTPUT>'.
> >   - Change 'clocks' to '<&clk IMX8MP_CLK_HSIO_AXI>'.
> 
> This is still not what I meant. There is no direct relation between the PCIe PHY
> domain being powered up and the PCIe PLL being needed. The PLL really only
> needs to be active when the reference clock isn't sourced externally. So the
> HSIO blk-ctrl should expose a proper clock for the PLL. As the PLL is effectively
> fixed rate it should be enough to expose get_rate, prepare and unprepare clk
> ops.
> 
> The PHY should then point at the blk-ctrl as the reference clock source.
Hi Lucas:
Thanks for your comments.
Understand what're your means.
Can you help to provide some specific pseudo example codes to let blk-ctrl
 expose such kind of clock to PCIe PHY module?

Best Regards
Richard Zhu
> 
> Regards,
> Lucas
> 
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> > v1->v2:
> > Refer to Lucas' comments, don't expose IMX8MP_CLK_HSIO_ROOT to dts
> node.
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.ozlabs.org%2Fproject%2Flinux-pci%2Fpatch%2F1666590189-1364-1-gi
> t
> >
> -send-email-hongxing.zhu%40nxp.com%2F&amp;data=05%7C01%7Chongxing.
> zhu%
> >
> 40nxp.com%7C4e559030b13d408f66c108dac9546393%7C686ea1d3bc2b4c6
> fa92cd99
> >
> c5c301635%7C0%7C0%7C638043661537710772%7CUnknown%7CTWFpbGZ
> sb3d8eyJWIjo
> >
> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300
> 0%7C%
> >
> 7C%7C&amp;sdata=GN5WnzNW1oclh7ZJFUUK3B68QYAqRMw6kPv0oEgPmFY
> %3D&amp;res
> > erved=0
> >
> > Use <&clk IMX8MP_CLK_HSIO_AXI> as referrence clock source when
> > internal clock mode is used by i.MX8MP PCIe module.
> >
> > Verified on i.MX8MP EVK board with removing R131/R132/R137/R138, and
> > populating R135/R136.
> > ---
> >  drivers/soc/imx/imx8mp-blk-ctrl.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c
> > b/drivers/soc/imx/imx8mp-blk-ctrl.c
> > index 0e3b6ba22f94..5ad20a8ea25e 100644
> > --- a/drivers/soc/imx/imx8mp-blk-ctrl.c
> > +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c
> > @@ -21,6 +21,16 @@
> >  #define  USB_CLOCK_MODULE_EN	BIT(1)
> >  #define  PCIE_PHY_APB_RST	BIT(4)
> >  #define  PCIE_PHY_INIT_RST	BIT(5)
> > +#define GPR_REG2		0x8
> > +#define  P_PLL_MASK		GENMASK(5, 0)
> > +#define  M_PLL_MASK		GENMASK(15, 6)
> > +#define  S_PLL_MASK		GENMASK(18, 16)
> > +#define  P_PLL			(0xc << 0)
> > +#define  M_PLL			(0x320 << 6)
> > +#define  S_PLL			(0x4 << 16)
> > +#define GPR_REG3		0xc
> > +#define  PLL_CKE		BIT(17)
> > +#define  PLL_RST		BIT(31)
> >
> >  struct imx8mp_blk_ctrl_domain;
> >
> > @@ -86,6 +96,18 @@ static void imx8mp_hsio_blk_ctrl_power_on(struct
> imx8mp_blk_ctrl *bc,
> >  	case IMX8MP_HSIOBLK_PD_PCIE_PHY:
> >  		regmap_set_bits(bc->regmap, GPR_REG0,
> >  				PCIE_PHY_APB_RST | PCIE_PHY_INIT_RST);
> > +
> > +		/* Set the PLL configurations, P = 12, M = 800, S = 4. */
> > +		regmap_update_bits(bc->regmap, GPR_REG2,
> > +				   P_PLL_MASK | M_PLL_MASK | S_PLL_MASK,
> > +				   P_PLL | M_PLL | S_PLL);
> > +		udelay(1);
> > +
> > +		regmap_update_bits(bc->regmap, GPR_REG3, PLL_RST, PLL_RST);
> > +		udelay(10);
> > +
> > +		/* Set 1b'1 to pll_cke of GPR_REG3 */
> > +		regmap_update_bits(bc->regmap, GPR_REG3, PLL_CKE, PLL_CKE);
> >  		break;
> >  	default:
> >  		break;
>