| Message ID | 20240226080433.3307154-5-sumit.garg@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | imx8mp: Enable PCIe/NVMe support | expand |
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 . > + /* 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 ? [...] The series is starting to look much better compared to V1 btw , these ^ are only nitpicks .
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
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, 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 >>> + /* 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 } [...]
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 > [...]
On 2/26/24 2:43 PM, Sumit Garg wrote: > 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 See ^ >> 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. Frankly, all the PLLs in the MX8M are very much similar, so either wait for someone from NXP to confirm this, or try and find some statement in the MX8MPRM . >>>>> +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? This is the enable part which is done in sequence, the part I quoted is the disable part.
diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c index 58cc3f63bb56..3e514a603578 100644 --- a/drivers/power/domain/imx8mp-hsiomix.c +++ b/drivers/power/domain/imx8mp-hsiomix.c @@ -6,9 +6,15 @@ #include <common.h> #include <asm/io.h> #include <clk.h> +#include <clk-uclass.h> #include <dm.h> #include <dm/device.h> #include <dm/device_compat.h> +#include <dm/device-internal.h> +#include <dm/lists.h> +#include <linux/bitfield.h> +#include <linux/delay.h> +#include <linux/iopoll.h> #include <power-domain-uclass.h> #include <dt-bindings/power/imx8mp-power.h> @@ -18,6 +24,15 @@ #define USB_CLOCK_MODULE_EN BIT(1) #define PCIE_PHY_APB_RST BIT(4) #define PCIE_PHY_INIT_RST BIT(5) +#define GPR_REG1 0x4 +#define PLL_LOCK BIT(13) +#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 GPR_REG3 0xc +#define PLL_CKE BIT(17) +#define PLL_RST BIT(31) struct imx8mp_hsiomix_priv { void __iomem *base; @@ -137,6 +152,68 @@ static int imx8mp_hsiomix_of_xlate(struct power_domain *power_domain, return 0; } +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)); + + /* 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); + + return 0; +} + +static const struct clk_ops hsio_pll_clk_ops = { + .enable = hsio_pll_clk_enable, + .disable = hsio_pll_clk_disable, +}; + +U_BOOT_DRIVER(hsio_pll) = { + .name = "hsio-pll", + .id = UCLASS_CLK, + .ops = &hsio_pll_clk_ops, +}; + +int imx8mp_hsiomix_bind(struct udevice *dev) +{ + struct driver *drv; + + drv = lists_driver_lookup_name("hsio-pll"); + if (!drv) + return -ENOENT; + + return device_bind_with_driver_data(dev, drv, "hsio-pll", + (ulong)dev_read_addr_ptr(dev), + dev_ofnode(dev), NULL); +} + static int imx8mp_hsiomix_probe(struct udevice *dev) { struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev); @@ -207,6 +284,7 @@ U_BOOT_DRIVER(imx8mp_hsiomix) = { .id = UCLASS_POWER_DOMAIN, .of_match = imx8mp_hsiomix_ids, .probe = imx8mp_hsiomix_probe, + .bind = imx8mp_hsiomix_bind, .priv_auto = sizeof(struct imx8mp_hsiomix_priv), .ops = &imx8mp_hsiomix_ops, };
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 Signed-off-by: Sumit Garg <sumit.garg@linaro.org> --- drivers/power/domain/imx8mp-hsiomix.c | 78 +++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)