Re: [PATCH v1 5/8] clk: imx: Add initial support for i.MXRT1170 clock driver

[Sean Anderson <seanga2@gmail.com>]

On 3/20/22 6:45 PM, Jesse Taube wrote:
> 
> 
> On 3/20/22 15:17, Sean Anderson wrote:
>> On 3/17/22 2:32 PM, Jesse Taube wrote:
>>> Add clock driver support for i.MXRT1170.
>>>
>>> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
>>> ---
>>>    drivers/clk/imx/Kconfig         |  16 +++
>>>    drivers/clk/imx/Makefile        |   1 +
>>>    drivers/clk/imx/clk-imxrt1170.c | 215 ++++++++++++++++++++++++++++++++
>>>    3 files changed, 232 insertions(+)
>>>    create mode 100644 drivers/clk/imx/clk-imxrt1170.c
>>>
>>> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
>>> index 96721bcbf3..ae56603194 100644
>>> --- a/drivers/clk/imx/Kconfig
>>> +++ b/drivers/clk/imx/Kconfig
>>> @@ -100,3 +100,19 @@ config CLK_IMXRT1050
>>>        select CLK_CCF
>>>        help
>>>          This enables support clock driver for i.MXRT1050 platforms.
>>> +
>>> +config SPL_CLK_IMXRT1170
>>> +    bool "SPL clock support for i.MXRT1170"
>>> +    depends on ARCH_IMXRT && SPL
>>> +    select SPL_CLK
>>> +    select SPL_CLK_CCF
>>> +    help
>>> +      This enables SPL DM/DTS support for clock driver in i.MXRT1170.
>>> +
>>> +config CLK_IMXRT1170
>>> +    bool "Clock support for i.MXRT1170"
>>> +    depends on ARCH_IMXRT
>>> +    select CLK
>>> +    select CLK_CCF
>>> +    help
>>> +      This enables support clock driver for i.MXRT1170 platforms.
>>> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
>>> index 01bbbdf3ae..3ed326739a 100644
>>> --- a/drivers/clk/imx/Makefile
>>> +++ b/drivers/clk/imx/Makefile
>>> @@ -19,3 +19,4 @@ obj-$(CONFIG_$(SPL_TPL_)CLK_IMX8MP) += clk-imx8mp.o clk-pll14xx.o \
>>>    obj-$(CONFIG_$(SPL_TPL_)CLK_IMXRT1020) += clk-imxrt1020.o
>>>    obj-$(CONFIG_$(SPL_TPL_)CLK_IMXRT1050) += clk-imxrt1050.o
>>> +obj-$(CONFIG_$(SPL_TPL_)CLK_IMXRT1170) += clk-imxrt1170.o
>>> diff --git a/drivers/clk/imx/clk-imxrt1170.c b/drivers/clk/imx/clk-imxrt1170.c
>>> new file mode 100644
>>> index 0000000000..6ea46b6a52
>>> --- /dev/null
>>> +++ b/drivers/clk/imx/clk-imxrt1170.c
>>> @@ -0,0 +1,215 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (C) 2022
>>> + * Author(s): Jesse Taube <Mr.Bossman075@gmail.com>
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <clk.h>
>>> +#include <clk-uclass.h>
>>> +#include <dm.h>
>>> +#include <log.h>
>>> +#include <asm/arch/clock.h>
>>> +#include <asm/arch/imx-regs.h>
>>> +#include <dt-bindings/clock/imxrt1170-clock.h>
>>> +
>>> +#include "clk.h"
>>> +
>>> +static ulong imxrt1170_clk_get_rate(struct clk *clk)
>>> +{
>>> +    struct clk *c;
>>> +    int ret;
>>> +
>>> +    debug("%s(#%lu)\n", __func__, clk->id);
>>
>> Consider dev_dbg() if you do a v2.
>>
>>> +
>>> +    ret = clk_get_by_id(clk->id, &c);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return clk_get_rate(c);
>>> +}
>>> +
>>> +static ulong imxrt1170_clk_set_rate(struct clk *clk, ulong rate)
>>> +{
>>> +    struct clk *c;
>>> +    int ret;
>>> +
>>> +    debug("%s(#%lu), rate: %lu\n", __func__, clk->id, rate);
>>> +
>>> +    ret = clk_get_by_id(clk->id, &c);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return clk_set_rate(c, rate);
>>> +}
>>> +
>>> +static int __imxrt1170_clk_enable(struct clk *clk, bool enable)
>>> +{
>>> +    struct clk *c;
>>> +    int ret;
>>> +
>>> +    debug("%s(#%lu) en: %d\n", __func__, clk->id, enable);
>>> +
>>> +    ret = clk_get_by_id(clk->id, &c);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    if (enable)
>>> +        ret = clk_enable(c);
>>> +    else
>>> +        ret = clk_disable(c);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int imxrt1170_clk_disable(struct clk *clk)
>>> +{
>>> +    return __imxrt1170_clk_enable(clk, 0);
>>> +}
>>> +
>>> +static int imxrt1170_clk_enable(struct clk *clk)
>>> +{
>>> +    return __imxrt1170_clk_enable(clk, 1);
>>> +}
>>> +
>>> +static int imxrt1170_clk_set_parent(struct clk *clk, struct clk *parent)
>>> +{
>>> +    struct clk *c, *cp;
>>> +    int ret;
>>> +
>>> +    debug("%s(#%lu), parent: %lu\n", __func__, clk->id, parent->id);
>>> +
>>> +    ret = clk_get_by_id(clk->id, &c);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = clk_get_by_id(parent->id, &cp);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    return clk_set_parent(c, cp);
>>> +}
>>> +
>>> +static struct clk_ops imxrt1170_clk_ops = {
>>> +    .set_rate = imxrt1170_clk_set_rate,
>>> +    .get_rate = imxrt1170_clk_get_rate,
>>> +    .enable = imxrt1170_clk_enable,
>>> +    .disable = imxrt1170_clk_disable,
>>> +    .set_parent = imxrt1170_clk_set_parent,
>>> +};
>>> +
>>> +static const char * const lpuart1_sels[] = {"rcosc48M_div2", "osc", "rcosc400M", "rcosc16M",
>>> +"pll3_div2", "pll1_div5", "pll2_sys", "pll2_pfd3"};
>>> +static const char * const gpt1_sels[] = {"rcosc48M_div2", "osc", "rcosc400M", "rcosc16M",
>>> +"pll3_div2", "pll1_div5", "pll3_pfd2", "pll3_pfd3"};
>>> +static const char * const usdhc1_sels[] = {"rcosc48M_div2", "osc", "rcosc400M", "rcosc16M",
>>> +"pll2_pfd2", "pll2_pfd0", "pll1_div5", "pll_arm"};
>>> +static const char * const semc_sels[] = {"rcosc48M_div2", "osc", "rcosc400M", "rcosc16M",
>>> +"pll1_div5", "pll2_sys", "pll2_pfd2", "pll3_pfd0"};
>>> +
>>> +static int imxrt1170_clk_probe(struct udevice *dev)
>>> +{
>>> +    void *base;
>>> +
>>> +    /* Anatop clocks */
>>> +    base = (void *)ofnode_get_addr(ofnode_by_compatible(ofnode_null(), "fsl,imxrt-anatop"));
>>
>> Where does this compatible come from? I don't see it in Linux or U-Boot.
> It is slowly being merged into mainline it should be in linux-next soon
> Here is the imx8mm one in mainline
> https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/freescale/imx8mm.dtsi#L577
> The rest of the imx family should move to this at some point.

Hm, this sort of thing should really be a reference of some kind, but it
appears that this is what all the other imx clocks do.

--Sean

>>> +
>>> +    clk_dm(IMXRT1170_CLK_PLL_ARM,
>>> +           imx_clk_pllv3(IMX_PLLV3_SYS, "pll_arm", "osc",
>>> +                 base + 0x200, 0xff));
>>> +    clk_dm(IMXRT1170_CLK_PLL3,
>>> +           imx_clk_pllv3(IMX_PLLV3_GENERICV2, "pll3_sys", "osc",
>>> +                 base + 0x210, 1));
>>> +    clk_dm(IMXRT1170_CLK_PLL2,
>>> +           imx_clk_pllv3(IMX_PLLV3_GENERICV2, "pll2_sys", "osc",
>>> +                 base + 0x240, 1));
>>> +
>>> +    clk_dm(IMXRT1170_CLK_PLL3_PFD0,
>>> +           imx_clk_pfd("pll3_pfd0", "pll3_sys", base + 0x230, 0));
>>> +    clk_dm(IMXRT1170_CLK_PLL3_PFD1,
>>> +           imx_clk_pfd("pll3_pfd1", "pll3_sys", base + 0x230, 1));
>>> +    clk_dm(IMXRT1170_CLK_PLL3_PFD2,
>>> +           imx_clk_pfd("pll3_pfd2", "pll3_sys", base + 0x230, 2));
>>> +    clk_dm(IMXRT1170_CLK_PLL3_PFD3,
>>> +           imx_clk_pfd("pll3_pfd3", "pll3_sys", base + 0x230, 3));
>>> +
>>> +    clk_dm(IMXRT1170_CLK_PLL2_PFD0,
>>> +           imx_clk_pfd("pll2_pfd0", "pll2_sys", base + 0x270, 0));
>>> +    clk_dm(IMXRT1170_CLK_PLL2_PFD1,
>>> +           imx_clk_pfd("pll2_pfd1", "pll2_sys", base + 0x270, 1));
>>> +    clk_dm(IMXRT1170_CLK_PLL2_PFD2,
>>> +           imx_clk_pfd("pll2_pfd2", "pll2_sys", base + 0x270, 2));
>>> +    clk_dm(IMXRT1170_CLK_PLL2_PFD3,
>>> +           imx_clk_pfd("pll2_pfd3", "pll2_sys", base + 0x270, 3));
>>> +
>>> +    clk_dm(IMXRT1170_CLK_PLL3_DIV2,
>>> +           imx_clk_fixed_factor("pll3_div2", "pll3_sys", 1, 2));
>>> +
>>> +    /* CCM clocks */
>>> +    base = dev_read_addr_ptr(dev);
>>> +    if (base == (void *)FDT_ADDR_T_NONE)
>>> +        return -EINVAL;
>>> +
>>> +    clk_dm(IMXRT1170_CLK_LPUART1_SEL,
>>> +           imx_clk_mux("lpuart1_sel", base + (25 * 0x80), 8, 3,
>>> +               lpuart1_sels, ARRAY_SIZE(lpuart1_sels)));
>>> +    clk_dm(IMXRT1170_CLK_LPUART1,
>>> +           imx_clk_divider("lpuart1", "lpuart1_sel",
>>> +                   base + (25 * 0x80), 0, 8));
>>> +
>>> +    clk_dm(IMXRT1170_CLK_USDHC1_SEL,
>>> +           imx_clk_mux("usdhc1_sel", base + (58 * 0x80), 8, 3,
>>> +               usdhc1_sels, ARRAY_SIZE(usdhc1_sels)));
>>> +    clk_dm(IMXRT1170_CLK_USDHC1,
>>> +           imx_clk_divider("usdhc1", "usdhc1_sel",
>>> +                   base + (58 * 0x80), 0, 8));
>>> +
>>> +    clk_dm(IMXRT1170_CLK_GPT1_SEL,
>>> +           imx_clk_mux("gpt1_sel", base + (14 * 0x80), 8, 3,
>>> +               gpt1_sels, ARRAY_SIZE(gpt1_sels)));
>>> +    clk_dm(IMXRT1170_CLK_GPT1,
>>> +           imx_clk_divider("gpt1", "gpt1_sel",
>>> +                   base + (14 * 0x80), 0, 8));
>>> +
>>> +    clk_dm(IMXRT1170_CLK_SEMC_SEL,
>>> +           imx_clk_mux("semc_sel", base + (4 * 0x80), 8, 3,
>>> +               semc_sels, ARRAY_SIZE(semc_sels)));
>>> +    clk_dm(IMXRT1170_CLK_SEMC,
>>> +           imx_clk_divider("semc", "semc_sel",
>>> +                   base + (4 * 0x80), 0, 8));
>>> +    struct clk *clk, *clk1;
>>> +
>>> +    clk_get_by_id(IMXRT1170_CLK_PLL2, &clk);
>>> +    clk_enable(clk);
>>> +    clk_set_rate(clk, 528000000UL);
>>> +
>>> +    clk_get_by_id(IMXRT1170_CLK_PLL2_PFD2, &clk);
>>> +
>>> +    clk_get_by_id(IMXRT1170_CLK_SEMC_SEL, &clk1);
>>> +    clk_enable(clk1);
>>> +    clk_set_parent(clk1, clk);
>>> +
>>> +    clk_get_by_id(IMXRT1170_CLK_SEMC, &clk);
>>> +    clk_enable(clk);
>>> +    clk_set_rate(clk, 132000000UL);
>>> +
>>> +    clk_get_by_id(IMXRT1170_CLK_PLL3, &clk);
>>> +    clk_enable(clk);
>>> +    clk_set_rate(clk, 480000000UL);
>>
>> Can this be done using assigned-clock-(frequency|parent)?
> Ah didn't relize U-Boot had this aswell.
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct udevice_id imxrt1170_clk_ids[] = {
>>> +    { .compatible = "fsl,imxrt1170-ccm" },
>>> +    { },
>>> +};
>>> +
>>> +U_BOOT_DRIVER(imxrt1170_clk) = {
>>> +    .name = "clk_imxrt1170",
>>> +    .id = UCLASS_CLK,
>>> +    .of_match = imxrt1170_clk_ids,
>>> +    .ops = &imxrt1170_clk_ops,
>>> +    .probe = imxrt1170_clk_probe,
>>> +    .flags = DM_FLAG_PRE_RELOC,
>>> +};
>>>
>>
>> --Sean