On Mon, 26 Feb 2024 at 14:24, Marek Vasut marex@denx.de wrote:
On 2/26/24 9:04 AM, Sumit Garg wrote:
Expose the high performance PLL as a regular Linux clock, so the PCIe PHY can use it when there is no external refclock provided.
Inspired from counterpart Linux kernel v6.8-rc3 driver: drivers/pmdomain/imx/imx8mp-blk-ctrl.c
Commit ID, please, see previous comments in this series.
[...]
+static int hsio_pll_clk_enable(struct clk *clk) +{
void *base = (void *)dev_get_driver_data(clk->dev);u32 val;int ret;/* Setup HSIO PLL */clrsetbits_le32(base + GPR_REG2,P_PLL_MASK | M_PLL_MASK | S_PLL_MASK,FIELD_PREP(P_PLL_MASK, 12) |FIELD_PREP(M_PLL_MASK, 800) |FIELD_PREP(S_PLL_MASK, 4));These magic numbers 12, 800, 4 could use explanation (why these numbers?) and a dedicated macro .
I can't find any information in TRM as to how HSIO PLL computation is done. However, I can extend the comment above to say that this configuration leads to a 100 MHz PHY clock as per clk_hsio_pll_recalc_rate() in Linux kernel driver.
/* de-assert PLL reset */setbits_le32(base + GPR_REG3, PLL_RST);/* enable PLL */setbits_le32(base + GPR_REG3, PLL_CKE);/* Check if PLL is locked */ret = readl_poll_sleep_timeout(base + GPR_REG1, val,val & PLL_LOCK, 10, 100000);if (ret)dev_err(clk->dev, "failed to lock HSIO PLL\n");return ret;+}
+static int hsio_pll_clk_disable(struct clk *clk) +{
void *base = (void *)dev_get_driver_data(clk->dev);clrbits_le32(base + GPR_REG3, PLL_CKE);clrbits_le32(base + GPR_REG3, PLL_RST);Can you clear both at once, or do they have to be cleared in sequence ?
I generally follow the reverse sequence of how the configuration is done during hsio_pll_clk_enable(). These bits have seperate functions related to clock and reset. So I would prefer to keep them as they are.
[...]
The series is starting to look much better compared to V1 btw , these ^ are only nitpicks .
Thanks for your detailed review, I appreciate it.
-Sumit