On Mon, 26 Feb 2024 at 18:31, Marek Vasut marex@denx.de wrote:
On 2/26/24 1:39 PM, Sumit Garg wrote:
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.
Try the CCM section 5.1.5.4.4 SSCG and Fractional PLLs I think,
That section also does mention following:
"The ARM PLL, GPU PLL, VPU PLL, DRAM PLL, Audio PLL1/2, and Video PLL1 are fractional PLLs. The frequency on these can be tuned to be very accurate to meet audio and video interface requirements."
without any mention of HSIO PLL.
this is likely PLL14xxx from Samsung, so
f_out = f_in * M / P * 2^S
I think f_in is something like 24 MHz, so 24 * 800 / 12 * 16 = 100 MHz
P - predivider S - subdivider (output divider) M - multiplier
IMO, we shouldn't document something based on our analysis but rather it should be based on facts.
/* 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.
Linux does this in drivers/pmdomain/imx/imx8mp-blk-ctrl.c :
120 static void clk_hsio_pll_unprepare(struct clk_hw *hw) 121 { 122 struct clk_hsio_pll *clk = to_clk_hsio_pll(hw); 123 124 regmap_update_bits(clk->regmap, GPR_REG3, PLL_RST | PLL_CKE, 0); 125 }
And just above it:
static int clk_hsio_pll_prepare(struct clk_hw *hw) { ... /* de-assert PLL reset */ regmap_update_bits(clk->regmap, GPR_REG3, PLL_RST, PLL_RST);
/* enable PLL */ regmap_update_bits(clk->regmap, GPR_REG3, PLL_CKE, PLL_CKE); ... }
That's an anti-pattern in the Linux kernel driver which we shouldn't copy, right?
-Sumit
[...]