[PATCH V3] clk: clk_versaclock: Add support for versaclock driver

Adam Ford aford173 at gmail.com
Fri Jun 4 17:56:01 CEST 2021


On Fri, Jun 4, 2021 at 9:29 AM Sean Anderson <sean.anderson at seco.com> wrote:
>
>
>
> On 6/4/21 10:22 AM, Adam Ford wrote:
>  > The driver is based on the Versaclock driver from the Linux code, but
>  > due differences in the clock API between them, some pieces had to be
>  > changed.
>  >
>  > This driver creates a mux, pfd, pll, and a series of fod ouputs.
>  >   Rate               Usecnt      Name
>  > ------------------------------------------
>  >   25000000             0        `-- x304-clock
>  >   25000000             0            `-- clock-controller at 6a.mux
>  >   25000000             0                |-- clock-controller at 6a.pfd
>  >   2800000000           0                |   `-- clock-controller at 6a.pll
>  >   33333333             0                |       |-- clock-controller at 6a.fod0
>  >   33333333             0                |       |   `-- clock-controller at 6a.out1
>  >   33333333             0                |       |-- clock-controller at 6a.fod1
>  >   33333333             0                |       |   `-- clock-controller at 6a.out2
>  >   50000000             0                |       |-- clock-controller at 6a.fod2
>  >   50000000             0                |       |   `-- clock-controller at 6a.out3
>  >   125000000            0                |       `-- clock-controller at 6a.fod3
>  >   125000000            0                |           `-- clock-controller at 6a.out4
>  >   25000000             0                `-- clock-controller at 6a.out0_sel_i2cb
>  >
>  > A translation function is added so the references to <&versaclock X> get routed
>  > to the corresponding clock-controller at 6a.outX.
>  >
>  > Signed-off-by: Adam Ford <aford173 at gmail.com>
>  > ---
>  > V3:  Streamline finding the out OUTx subnodes.
>  >       Add error handling.
>  >       Replace printf and pr_err with dev_dbg.
>  >       Restore registers removed in V2.
>  >
>  > V2:  Remove unused registers.
>  >       Fix spacing in Makefile.
>  >       Make the versaclock driver dependent on CCF.
>  >       Move the versaclock_ids next to U_BOOT_DRIVER(versaclock)
>  >
>  > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>  > index 40a5a5dd88..2a7507ea18 100644
>  > --- a/drivers/clk/Kconfig
>  > +++ b/drivers/clk/Kconfig
>  > @@ -197,4 +197,13 @@ config SANDBOX_CLK_CCF
>  >        Enable this option if you want to test the Linux kernel's Common
>  >        Clock Framework [CCF] code in U-Boot's Sandbox clock driver.
>  >
>  > +config CLK_VERSACLOCK
>  > +    tristate "Enable VersaClock 5/6 devices"
>  > +    depends on CLK
>  > +    depends on CLK_CCF
>  > +    depends on OF_CONTROL
>  > +    help
>  > +      This driver supports the IDT VersaClock 5 and VersaClock 6
>  > +      programmable clock generators.
>  > +
>  >   endmenu
>  > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>  > index 645709b855..6f5ddafd64 100644
>  > --- a/drivers/clk/Makefile
>  > +++ b/drivers/clk/Makefile
>  > @@ -51,3 +51,4 @@ obj-$(CONFIG_SANDBOX_CLK_CCF) += clk_sandbox_ccf.o
>  >   obj-$(CONFIG_STM32H7) += clk_stm32h7.o
>  >   obj-$(CONFIG_CLK_VERSAL) += clk_versal.o
>  >   obj-$(CONFIG_CLK_CDCE9XX) += clk-cdce9xx.o
>  > +obj-$(CONFIG_CLK_VERSACLOCK) += clk_versaclock.o
>  > diff --git a/drivers/clk/clk_versaclock.c b/drivers/clk/clk_versaclock.c
>  > new file mode 100644
>  > index 0000000000..84893d842c
>  > --- /dev/null
>  > +++ b/drivers/clk/clk_versaclock.c
>  > @@ -0,0 +1,1099 @@
>  > +// SPDX-License-Identifier: GPL-2.0-or-later
>  > +/*
>  > + * Driver for IDT Versaclock 5/6
>  > + *
>  > + * Derived from code Copyright (C) 2017 Marek Vasut <marek.vasut at gmail.com>
>  > + */
>  > +
>  > +#include <common.h>
>  > +#include <clk.h>
>  > +#include <clk-uclass.h>
>  > +#include <dm.h>
>  > +#include <errno.h>
>  > +#include <i2c.h>
>  > +#include <dm/device_compat.h>
>  > +#include <log.h>
>  > +#include <linux/clk-provider.h>
>  > +#include <linux/kernel.h>
>  > +#include <linux/math64.h>
>  > +
>  > +#include <dt-bindings/clk/versaclock.h>
>  > +
>  > +/* VersaClock5 registers */
>  > +#define VC5_OTP_CONTROL                             0x00
>  > +
>  > +/* Factory-reserved register block */
>  > +#define VC5_RSVD_DEVICE_ID                  0x01
>  > +#define VC5_RSVD_ADC_GAIN_7_0                       0x02
>  > +#define VC5_RSVD_ADC_GAIN_15_8                      0x03
>  > +#define VC5_RSVD_ADC_OFFSET_7_0                     0x04
>  > +#define VC5_RSVD_ADC_OFFSET_15_8            0x05
>  > +#define VC5_RSVD_TEMPY                              0x06
>  > +#define VC5_RSVD_OFFSET_TBIN                        0x07
>  > +#define VC5_RSVD_GAIN                               0x08
>  > +#define VC5_RSVD_TEST_NP                    0x09
>  > +#define VC5_RSVD_UNUSED                             0x0a
>  > +#define VC5_RSVD_BANDGAP_TRIM_UP            0x0b
>  > +#define VC5_RSVD_BANDGAP_TRIM_DN            0x0c
>  > +#define VC5_RSVD_CLK_R_12_CLK_AMP_4         0x0d
>  > +#define VC5_RSVD_CLK_R_34_CLK_AMP_4         0x0e
>  > +#define VC5_RSVD_CLK_AMP_123                        0x0f
>  > +
>  > +/* Configuration register block */
>  > +#define VC5_PRIM_SRC_SHDN                   0x10
>  > +#define VC5_PRIM_SRC_SHDN_EN_XTAL           BIT(7)
>  > +#define VC5_PRIM_SRC_SHDN_EN_CLKIN          BIT(6)
>  > +#define VC5_PRIM_SRC_SHDN_EN_DOUBLE_XTAL_FREQ       BIT(3)
>  > +#define VC5_PRIM_SRC_SHDN_SP                        BIT(1)
>  > +#define VC5_PRIM_SRC_SHDN_EN_GBL_SHDN               BIT(0)
>  > +
>  > +#define VC5_VCO_BAND                                0x11
>  > +#define VC5_XTAL_X1_LOAD_CAP                        0x12
>  > +#define VC5_XTAL_X2_LOAD_CAP                        0x13
>  > +#define VC5_REF_DIVIDER                             0x15
>  > +#define VC5_REF_DIVIDER_SEL_PREDIV2         BIT(7)
>  > +#define VC5_REF_DIVIDER_REF_DIV(n)          ((n) & 0x3f)
>  > +
>  > +#define VC5_VCO_CTRL_AND_PREDIV                     0x16
>  > +#define VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV       BIT(7)
>  > +
>  > +#define VC5_FEEDBACK_INT_DIV                        0x17
>  > +#define VC5_FEEDBACK_INT_DIV_BITS           0x18
>  > +#define VC5_FEEDBACK_FRAC_DIV(n)            (0x19 + (n))
>  > +#define VC5_RC_CONTROL0                             0x1e
>  > +#define VC5_RC_CONTROL1                             0x1f
>  > +/* Register 0x20 is factory reserved */
>  > +
>  > +/* Output divider control for divider 1,2,3,4 */
>  > +#define VC5_OUT_DIV_CONTROL(idx)    (0x21 + ((idx) * 0x10))
>  > +#define VC5_OUT_DIV_CONTROL_RESET   BIT(7)
>  > +#define VC5_OUT_DIV_CONTROL_SELB_NORM       BIT(3)
>  > +#define VC5_OUT_DIV_CONTROL_SEL_EXT BIT(2)
>  > +#define VC5_OUT_DIV_CONTROL_INT_MODE        BIT(1)
>  > +#define VC5_OUT_DIV_CONTROL_EN_FOD  BIT(0)
>  > +
>  > +#define VC5_OUT_DIV_FRAC(idx, n)    (0x22 + ((idx) * 0x10) + (n))
>  > +#define VC5_OUT_DIV_FRAC4_OD_SCEE   BIT(1)
>  > +
>  > +#define VC5_OUT_DIV_STEP_SPREAD(idx, n)     (0x26 + ((idx) * 0x10) + (n))
>  > +#define VC5_OUT_DIV_SPREAD_MOD(idx, n)      (0x29 + ((idx) * 0x10) + (n))
>  > +#define VC5_OUT_DIV_SKEW_INT(idx, n)        (0x2b + ((idx) * 0x10) + (n))
>  > +#define VC5_OUT_DIV_INT(idx, n)             (0x2d + ((idx) * 0x10) + (n))
>  > +#define VC5_OUT_DIV_SKEW_FRAC(idx)  (0x2f + ((idx) * 0x10))
>  > +/* Registers 0x30, 0x40, 0x50 are factory reserved */
>  > +
>  > +/* Clock control register for clock 1,2 */
>  > +#define VC5_CLK_OUTPUT_CFG(idx, n)  (0x60 + ((idx) * 0x2) + (n))
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_SHIFT       5
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_MASK GENMASK(7, VC5_CLK_OUTPUT_CFG0_CFG_SHIFT)
>  > +
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_LVPECL      (VC5_LVPECL)
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_CMOS                (VC5_CMOS)
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_HCSL33      (VC5_HCSL33)
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_LVDS                (VC5_LVDS)
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_CMOS2               (VC5_CMOS2)
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_CMOSD               (VC5_CMOSD)
>  > +#define VC5_CLK_OUTPUT_CFG0_CFG_HCSL25      (VC5_HCSL25)
>  > +
>  > +#define VC5_CLK_OUTPUT_CFG0_PWR_SHIFT       3
>  > +#define VC5_CLK_OUTPUT_CFG0_PWR_MASK GENMASK(4, VC5_CLK_OUTPUT_CFG0_PWR_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG0_PWR_18  (0 << VC5_CLK_OUTPUT_CFG0_PWR_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG0_PWR_25  (2 << VC5_CLK_OUTPUT_CFG0_PWR_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG0_PWR_33  (3 << VC5_CLK_OUTPUT_CFG0_PWR_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG0_SLEW_SHIFT      0
>  > +#define VC5_CLK_OUTPUT_CFG0_SLEW_MASK GENMASK(1, VC5_CLK_OUTPUT_CFG0_SLEW_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG0_SLEW_80 (0 << VC5_CLK_OUTPUT_CFG0_SLEW_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG0_SLEW_85 (1 << VC5_CLK_OUTPUT_CFG0_SLEW_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG0_SLEW_90 (2 << VC5_CLK_OUTPUT_CFG0_SLEW_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG0_SLEW_100        (3 << VC5_CLK_OUTPUT_CFG0_SLEW_SHIFT)
>  > +#define VC5_CLK_OUTPUT_CFG1_EN_CLKBUF       BIT(0)
>  > +
>  > +#define VC5_CLK_OE_SHDN                             0x68
>  > +#define VC5_CLK_OS_SHDN                             0x69
>  > +
>  > +#define VC5_GLOBAL_REGISTER                 0x76
>  > +#define VC5_GLOBAL_REGISTER_GLOBAL_RESET    BIT(5)
>  > +
>  > +/* PLL/VCO runs between 2.5 GHz and 3.0 GHz */
>  > +#define VC5_PLL_VCO_MIN                             2500000000UL
>  > +#define VC5_PLL_VCO_MAX                             3000000000UL
>  > +
>  > +/* VC5 Input mux settings */
>  > +#define VC5_MUX_IN_XIN              BIT(0)
>  > +#define VC5_MUX_IN_CLKIN    BIT(1)
>  > +
>  > +/* Maximum number of clk_out supported by this driver */
>  > +#define VC5_MAX_CLK_OUT_NUM 5
>  > +
>  > +/* Maximum number of FODs supported by this driver */
>  > +#define VC5_MAX_FOD_NUM     4
>  > +
>  > +/* flags to describe chip features */
>  > +/* chip has built-in oscilator */
>  > +#define VC5_HAS_INTERNAL_XTAL       BIT(0)
>  > +/* chip has PFD requency doubler */
>  > +#define VC5_HAS_PFD_FREQ_DBL        BIT(1)
>  > +
>  > +/* Supported IDT VC5 models. */
>  > +enum vc5_model {
>  > +    IDT_VC5_5P49V5923,
>  > +    IDT_VC5_5P49V5925,
>  > +    IDT_VC5_5P49V5933,
>  > +    IDT_VC5_5P49V5935,
>  > +    IDT_VC6_5P49V6901,
>  > +    IDT_VC6_5P49V6965,
>  > +};
>  > +
>  > +/* Structure to describe features of a particular VC5 model */
>  > +struct vc5_chip_info {
>  > +    const enum vc5_model    model;
>  > +    const unsigned int      clk_fod_cnt;
>  > +    const unsigned int      clk_out_cnt;
>  > +    const u32               flags;
>  > +};
>  > +
>  > +struct vc5_driver_data;
>  > +
>  > +struct vc5_hw_data {
>  > +    struct clk              hw;
>  > +    struct vc5_driver_data  *vc5;
>  > +    u32                     div_int;
>  > +    u32                     div_frc;
>  > +    unsigned int            num;
>  > +};
>  > +
>  > +struct vc5_out_data {
>  > +    struct clk              hw;
>  > +    struct vc5_driver_data  *vc5;
>  > +    unsigned int            num;
>  > +    unsigned int            clk_output_cfg0;
>  > +    unsigned int            clk_output_cfg0_mask;
>  > +};
>  > +
>  > +struct vc5_driver_data {
>  > +    struct udevice          *i2c;
>  > +    const struct vc5_chip_info      *chip_info;
>  > +
>  > +    struct clk              *pin_xin;
>  > +    struct clk              *pin_clkin;
>  > +    unsigned char           clk_mux_ins;
>  > +    struct clk              clk_mux;
>  > +    struct clk              clk_mul;
>  > +    struct clk              clk_pfd;
>  > +    struct vc5_hw_data      clk_pll;
>  > +    struct vc5_hw_data      clk_fod[VC5_MAX_FOD_NUM];
>  > +    struct vc5_out_data     clk_out[VC5_MAX_CLK_OUT_NUM];
>  > +};
>  > +
>  > +static const struct vc5_chip_info idt_5p49v5923_info = {
>  > +    .model = IDT_VC5_5P49V5923,
>  > +    .clk_fod_cnt = 2,
>  > +    .clk_out_cnt = 3,
>  > +    .flags = 0,
>  > +};
>  > +
>  > +static const struct vc5_chip_info idt_5p49v5925_info = {
>  > +    .model = IDT_VC5_5P49V5925,
>  > +    .clk_fod_cnt = 4,
>  > +    .clk_out_cnt = 5,
>  > +    .flags = 0,
>  > +};
>  > +
>  > +static const struct vc5_chip_info idt_5p49v5933_info = {
>  > +    .model = IDT_VC5_5P49V5933,
>  > +    .clk_fod_cnt = 2,
>  > +    .clk_out_cnt = 3,
>  > +    .flags = VC5_HAS_INTERNAL_XTAL,
>  > +};
>  > +
>  > +static const struct vc5_chip_info idt_5p49v5935_info = {
>  > +    .model = IDT_VC5_5P49V5935,
>  > +    .clk_fod_cnt = 4,
>  > +    .clk_out_cnt = 5,
>  > +    .flags = VC5_HAS_INTERNAL_XTAL,
>  > +};
>  > +
>  > +static const struct vc5_chip_info idt_5p49v6901_info = {
>  > +    .model = IDT_VC6_5P49V6901,
>  > +    .clk_fod_cnt = 4,
>  > +    .clk_out_cnt = 5,
>  > +    .flags = VC5_HAS_PFD_FREQ_DBL,
>  > +};
>  > +
>  > +static const struct vc5_chip_info idt_5p49v6965_info = {
>  > +    .model = IDT_VC6_5P49V6965,
>  > +    .clk_fod_cnt = 4,
>  > +    .clk_out_cnt = 5,
>  > +    .flags = 0,
>  > +};
>  > +
>  > +static int vc5_update_bits(struct udevice *dev, unsigned int reg, unsigned int mask,
>  > +                       unsigned int src)
>  > +{
>  > +    int ret;
>  > +    unsigned char cache;
>  > +
>  > +    ret = dm_i2c_read(dev, reg, &cache, 1);
>  > +    if (ret < 0)
>  > +            return ret;
>  > +
>  > +    cache &= ~mask;
>  > +    cache |= mask & src;
>  > +    ret = dm_i2c_write(dev, reg, (uchar *)&cache, 1);
>  > +
>  > +    return ret;
>  > +}
>  > +
>  > +static unsigned long vc5_mux_get_rate(struct clk *hw)
>  > +{
>  > +    return clk_get_rate(clk_get_parent(hw));
>  > +}
>  > +
>  > +static int vc5_mux_set_parent(struct clk *hw, unsigned char index)
>  > +{
>  > +    struct vc5_driver_data *vc5 = container_of(hw, struct vc5_driver_data, clk_mux);
>  > +    const u8 mask = VC5_PRIM_SRC_SHDN_EN_XTAL | VC5_PRIM_SRC_SHDN_EN_CLKIN;
>  > +    u8 src;
>  > +
>  > +    if (index > 1 || !vc5->clk_mux_ins)
>  > +            return -EINVAL;
>  > +
>  > +    if (vc5->clk_mux_ins == (VC5_MUX_IN_CLKIN | VC5_MUX_IN_XIN)) {
>  > +            if (index == 0)
>  > +                    src = VC5_PRIM_SRC_SHDN_EN_XTAL;
>  > +            if (index == 1)
>  > +                    src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
>  > +    } else {
>  > +            if (index != 0)
>  > +                    return -EINVAL;
>  > +
>  > +            if (vc5->clk_mux_ins == VC5_MUX_IN_XIN)
>  > +                    src = VC5_PRIM_SRC_SHDN_EN_XTAL;
>  > +            else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
>  > +                    src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
>  > +            else /* Invalid; should have been caught by vc5_probe() */
>  > +                    return -EINVAL;
>  > +    }
>  > +
>  > +    return vc5_update_bits(vc5->i2c, VC5_PRIM_SRC_SHDN, mask, src);
>  > +}
>  > +
>  > +static const struct clk_ops vc5_mux_ops = {
>  > +    .get_rate       = vc5_mux_get_rate,
>  > +};
>  > +
>  > +static unsigned long vc5_pfd_round_rate(struct clk *hw, unsigned long rate)
>  > +{
>  > +    struct clk *clk_parent = clk_get_parent(hw);
>  > +    unsigned long parent_rate = clk_get_rate(clk_parent);
>  > +    unsigned long idiv;
>  > +
>  > +    /* PLL cannot operate with input clock above 50 MHz. */
>  > +    if (rate > 50000000)
>  > +            return -EINVAL;
>  > +
>  > +    /* CLKIN within range of PLL input, feed directly to PLL. */
>  > +    if (parent_rate <= 50000000)
>  > +            return parent_rate;
>  > +
>  > +    idiv = DIV_ROUND_UP(parent_rate, rate);
>  > +    if (idiv > 127)
>  > +            return -EINVAL;
>  > +
>  > +    return parent_rate / idiv;
>  > +}
>  > +
>  > +static unsigned long vc5_pfd_recalc_rate(struct clk *hw)
>  > +{
>  > +    struct vc5_driver_data *vc5 =
>  > +            container_of(hw, struct vc5_driver_data, clk_pfd);
>  > +    unsigned int prediv, div;
>  > +    struct clk *clk_parent = clk_get_parent(hw);
>  > +    unsigned long parent_rate = clk_get_rate(clk_parent);
>  > +
>  > +    dm_i2c_read(vc5->i2c, VC5_VCO_CTRL_AND_PREDIV, (uchar *)&prediv, 1);
>  > +
>  > +    /* The bypass_prediv is set, PLL fed from Ref_in directly. */
>  > +    if (prediv & VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV)
>  > +            return parent_rate;
>  > +
>  > +    dm_i2c_read(vc5->i2c, VC5_REF_DIVIDER, (uchar *)&div, 1);
>  > +
>  > +    /* The Sel_prediv2 is set, PLL fed from prediv2 (Ref_in / 2) */
>  > +    if (div & VC5_REF_DIVIDER_SEL_PREDIV2)
>  > +            return parent_rate / 2;
>  > +    else
>  > +            return parent_rate / VC5_REF_DIVIDER_REF_DIV(div);
>  > +}
>  > +
>  > +static unsigned long vc5_pfd_set_rate(struct clk *hw, unsigned long rate)
>  > +{
>  > +    struct vc5_driver_data *vc5 =
>  > +            container_of(hw, struct vc5_driver_data, clk_pfd);
>  > +    unsigned long idiv;
>  > +    u8 div;
>  > +    struct clk *clk_parent = clk_get_parent(hw);
>  > +    unsigned long parent_rate = clk_get_rate(clk_parent);
>  > +
>  > +    /* CLKIN within range of PLL input, feed directly to PLL. */
>  > +    if (parent_rate <= 50000000) {
>  > +            vc5_update_bits(vc5->i2c, VC5_VCO_CTRL_AND_PREDIV,
>  > +                            VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV,
>  > +                            VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV);
>  > +            vc5_update_bits(vc5->i2c, VC5_REF_DIVIDER, 0xff, 0x00);
>  > +            return 0;
>  > +    }
>  > +
>  > +    idiv = DIV_ROUND_UP(parent_rate, rate);
>  > +
>  > +    /* We have dedicated div-2 predivider. */
>  > +    if (idiv == 2)
>  > +            div = VC5_REF_DIVIDER_SEL_PREDIV2;
>  > +    else
>  > +            div = VC5_REF_DIVIDER_REF_DIV(idiv);
>  > +
>  > +    vc5_update_bits(vc5->i2c, VC5_REF_DIVIDER, 0xff, div);
>  > +    vc5_update_bits(vc5->i2c, VC5_VCO_CTRL_AND_PREDIV,
>  > +                    VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV, 0);
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +static const struct clk_ops vc5_pfd_ops = {
>  > +    .round_rate     = vc5_pfd_round_rate,
>  > +    .get_rate       = vc5_pfd_recalc_rate,
>  > +    .set_rate       = vc5_pfd_set_rate,
>  > +};
>  > +
>  > +/*
>  > + * VersaClock5 PLL/VCO
>  > + */
>  > +static unsigned long vc5_pll_recalc_rate(struct clk *hw)
>  > +{
>  > +    struct vc5_hw_data *hwdata = container_of(hw, struct vc5_hw_data, hw);
>  > +    struct vc5_driver_data *vc = hwdata->vc5;
>  > +    struct clk *clk_parent = clk_get_parent(hw);
>  > +    unsigned long parent_rate = clk_get_rate(clk_parent);
>  > +    u32 div_int, div_frc;
>  > +    u8 fb[5];
>  > +
>  > +    dm_i2c_read(vc->i2c, VC5_FEEDBACK_INT_DIV, fb, 5);
>  > +
>  > +    div_int = (fb[0] << 4) | (fb[1] >> 4);
>  > +    div_frc = (fb[2] << 16) | (fb[3] << 8) | fb[4];
>  > +
>  > +    /* The PLL divider has 12 integer bits and 24 fractional bits */
>  > +    return (parent_rate * div_int) + ((parent_rate * div_frc) >> 24);
>  > +}
>  > +
>  > +static unsigned long vc5_pll_round_rate(struct clk *hw, unsigned long rate)
>  > +{
>  > +    struct clk *clk_parent = clk_get_parent(hw);
>  > +    unsigned long parent_rate = clk_get_rate(clk_parent);
>  > +    struct vc5_hw_data *hwdata = container_of(hw, struct vc5_hw_data, hw);
>  > +    u32 div_int;
>  > +    u64 div_frc;
>  > +
>  > +    if (rate < VC5_PLL_VCO_MIN)
>  > +            rate = VC5_PLL_VCO_MIN;
>  > +    if (rate > VC5_PLL_VCO_MAX)
>  > +            rate = VC5_PLL_VCO_MAX;
>  > +
>  > +    /* Determine integer part, which is 12 bit wide */
>  > +    div_int = rate / parent_rate;
>  > +    if (div_int > 0xfff)
>  > +            rate = parent_rate * 0xfff;
>  > +
>  > +    /* Determine best fractional part, which is 24 bit wide */
>  > +    div_frc = rate % parent_rate;
>  > +    div_frc *= BIT(24) - 1;
>  > +    do_div(div_frc, parent_rate);
>  > +
>  > +    hwdata->div_int = div_int;
>  > +    hwdata->div_frc = (u32)div_frc;
>  > +
>  > +    return (parent_rate * div_int) + ((parent_rate * div_frc) >> 24);
>  > +}
>  > +
>  > +static unsigned long vc5_pll_set_rate(struct clk *hw, unsigned long rate)
>  > +{
>  > +    struct vc5_hw_data *hwdata = container_of(hw, struct vc5_hw_data, hw);
>  > +    struct vc5_driver_data *vc5 = hwdata->vc5;
>  > +    u8 fb[5];
>  > +
>  > +    fb[0] = hwdata->div_int >> 4;
>  > +    fb[1] = hwdata->div_int << 4;
>  > +    fb[2] = hwdata->div_frc >> 16;
>  > +    fb[3] = hwdata->div_frc >> 8;
>  > +    fb[4] = hwdata->div_frc;
>  > +
>  > +    return dm_i2c_write(vc5->i2c, VC5_FEEDBACK_INT_DIV, fb, 5);
>  > +}
>  > +
>  > +static const struct clk_ops vc5_pll_ops = {
>  > +    .round_rate     = vc5_pll_round_rate,
>  > +    .get_rate       = vc5_pll_recalc_rate,
>  > +    .set_rate       = vc5_pll_set_rate,
>  > +};
>  > +
>  > +static unsigned long vc5_fod_recalc_rate(struct clk *hw)
>  > +{
>  > +    struct vc5_hw_data *hwdata = container_of(hw, struct vc5_hw_data, hw);
>  > +    struct vc5_driver_data *vc = hwdata->vc5;
>  > +    struct clk *parent = &vc->clk_pll.hw;
>  > +    unsigned long parent_rate =  vc5_pll_recalc_rate(parent);
>  > +
>  > +    /* VCO frequency is divided by two before entering FOD */
>  > +    u32 f_in = parent_rate / 2;
>  > +    u32 div_int, div_frc;
>  > +    u8 od_int[2];
>  > +    u8 od_frc[4];
>  > +
>  > +    dm_i2c_read(vc->i2c, VC5_OUT_DIV_INT(hwdata->num, 0), od_int, 2);
>  > +    dm_i2c_read(vc->i2c, VC5_OUT_DIV_FRAC(hwdata->num, 0), od_frc, 4);
>  > +
>  > +    div_int = (od_int[0] << 4) | (od_int[1] >> 4);
>  > +    div_frc = (od_frc[0] << 22) | (od_frc[1] << 14) |
>  > +              (od_frc[2] << 6) | (od_frc[3] >> 2);
>  > +
>  > +    /* Avoid division by zero if the output is not configured. */
>  > +    if (div_int == 0 && div_frc == 0)
>  > +            return 0;
>  > +
>  > +    /* The PLL divider has 12 integer bits and 30 fractional bits */
>  > +    return div64_u64((u64)f_in << 24ULL, ((u64)div_int << 24ULL) + div_frc);
>  > +}
>  > +
>  > +static unsigned long vc5_fod_round_rate(struct clk *hw, unsigned long rate)
>  > +{
>  > +    struct vc5_hw_data *hwdata = container_of(hw, struct vc5_hw_data, hw);
>  > +    struct vc5_driver_data *vc = hwdata->vc5;
>  > +    struct clk *parent = &vc->clk_pll.hw;
>  > +    unsigned long parent_rate =  vc5_pll_recalc_rate(parent);
>  > +
>  > +    /* VCO frequency is divided by two before entering FOD */
>  > +    u32 f_in = parent_rate / 2;
>  > +    u32 div_int;
>  > +    u64 div_frc;
>  > +
>  > +    /* Determine integer part, which is 12 bit wide */
>  > +    div_int = f_in / rate;
>  > +
>  > +    /*
>  > +     * WARNING: The clock chip does not output signal if the integer part
>  > +     *          of the divider is 0xfff and fractional part is non-zero.
>  > +     *          Clamp the divider at 0xffe to keep the code simple.
>  > +     */
>  > +    if (div_int > 0xffe) {
>  > +            div_int = 0xffe;
>  > +            rate = f_in / div_int;
>  > +    }
>  > +
>  > +    /* Determine best fractional part, which is 30 bit wide */
>  > +    div_frc = f_in % rate;
>  > +    div_frc <<= 24;
>  > +    do_div(div_frc, rate);
>  > +
>  > +    hwdata->div_int = div_int;
>  > +    hwdata->div_frc = (u32)div_frc;
>  > +
>  > +    return div64_u64((u64)f_in << 24ULL, ((u64)div_int << 24ULL) + div_frc);
>  > +}
>  > +
>  > +static unsigned long vc5_fod_set_rate(struct clk *hw, unsigned long rate)
>  > +{
>  > +    struct vc5_hw_data *hwdata = container_of(hw, struct vc5_hw_data, hw);
>  > +    struct vc5_driver_data *vc5 = hwdata->vc5;
>  > +
>  > +    u8 data[14] = {
>  > +            hwdata->div_frc >> 22, hwdata->div_frc >> 14,
>  > +            hwdata->div_frc >> 6, hwdata->div_frc << 2,
>  > +            0, 0, 0, 0, 0,
>  > +            0, 0,
>  > +            hwdata->div_int >> 4, hwdata->div_int << 4,
>  > +            0
>  > +    };
>  > +
>  > +    dm_i2c_write(vc5->i2c, VC5_OUT_DIV_FRAC(hwdata->num, 0), data, 14);
>  > +
>  > +    /*
>  > +     * Toggle magic bit in undocumented register for unknown reason.
>  > +     * This is what the IDT timing commander tool does and the chip
>  > +     * datasheet somewhat implies this is needed, but the register
>  > +     * and the bit is not documented.
>  > +     */
>  > +    vc5_update_bits(vc5->i2c, VC5_GLOBAL_REGISTER,
>  > +                    VC5_GLOBAL_REGISTER_GLOBAL_RESET, 0);
>  > +    vc5_update_bits(vc5->i2c, VC5_GLOBAL_REGISTER,
>  > +                    VC5_GLOBAL_REGISTER_GLOBAL_RESET,
>  > +                    VC5_GLOBAL_REGISTER_GLOBAL_RESET);
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +static const struct clk_ops vc5_fod_ops = {
>  > +    .round_rate     = vc5_fod_round_rate,
>  > +    .get_rate       = vc5_fod_recalc_rate,
>  > +    .set_rate       = vc5_fod_set_rate,
>  > +};
>  > +
>  > +static int vc5_clk_out_prepare(struct clk *hw)
>  > +{
>  > +    struct udevice *dev;
>  > +    struct vc5_driver_data *vc5;
>  > +    struct vc5_out_data *hwdata;
>  > +
>  > +    const u8 mask = VC5_OUT_DIV_CONTROL_SELB_NORM |
>  > +                    VC5_OUT_DIV_CONTROL_SEL_EXT |
>  > +                    VC5_OUT_DIV_CONTROL_EN_FOD;
>  > +    unsigned int src;
>  > +    int ret;
>  > +
>  > +    uclass_get_device_by_name(UCLASS_CLK, clk_hw_get_name(hw), &dev);
>  > +    vc5 = dev_get_priv(dev);
>  > +    hwdata = &vc5->clk_out[hw->id];
>  > +
>  > +    /*
>  > +     * If the input mux is disabled, enable it first and
>  > +     * select source from matching FOD.
>  > +     */
>  > +
>  > +    dm_i2c_read(vc5->i2c, VC5_OUT_DIV_CONTROL(hwdata->num), (uchar *)&src, 1);
>  > +
>  > +    if ((src & mask) == 0) {
>  > +            src = VC5_OUT_DIV_CONTROL_RESET | VC5_OUT_DIV_CONTROL_EN_FOD;
>  > +            ret = vc5_update_bits(vc5->i2c,
>  > +                                  VC5_OUT_DIV_CONTROL(hwdata->num),
>  > +                                  mask | VC5_OUT_DIV_CONTROL_RESET, src);
>  > +            if (ret)
>  > +                    return ret;
>  > +    }
>  > +
>  > +    /* Enable the clock buffer */
>  > +    vc5_update_bits(vc5->i2c, VC5_CLK_OUTPUT_CFG(hwdata->num, 1),
>  > +                    VC5_CLK_OUTPUT_CFG1_EN_CLKBUF,
>  > +                    VC5_CLK_OUTPUT_CFG1_EN_CLKBUF);
>  > +    if (hwdata->clk_output_cfg0_mask) {
>  > +            vc5_update_bits(vc5->i2c, VC5_CLK_OUTPUT_CFG(hwdata->num, 0),
>  > +                            hwdata->clk_output_cfg0_mask,
>  > +                            hwdata->clk_output_cfg0);
>  > +    }
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +static int vc5_clk_out_unprepare(struct clk *hw)
>  > +{
>  > +    struct udevice *dev;
>  > +    struct vc5_driver_data *vc5;
>  > +    struct vc5_out_data *hwdata;
>  > +    int ret;
>  > +
>  > +    uclass_get_device_by_name(UCLASS_CLK, clk_hw_get_name(hw), &dev);
>  > +    vc5 = dev_get_priv(dev);
>  > +    hwdata = &vc5->clk_out[hw->id];
>  > +
>  > +    /* Disable the clock buffer */
>  > +    ret = vc5_update_bits(vc5->i2c, VC5_CLK_OUTPUT_CFG(hwdata->num, 1),
>  > +                          VC5_CLK_OUTPUT_CFG1_EN_CLKBUF, 0);
>  > +
>  > +    return ret;
>  > +}
>  > +
>  > +static int vc5_clk_out_set_parent(struct vc5_driver_data *vc, u8 num, u8 index)
>  > +{
>  > +    const u8 mask = VC5_OUT_DIV_CONTROL_RESET |
>  > +                    VC5_OUT_DIV_CONTROL_SELB_NORM |
>  > +                    VC5_OUT_DIV_CONTROL_SEL_EXT |
>  > +                    VC5_OUT_DIV_CONTROL_EN_FOD;
>  > +    const u8 extclk = VC5_OUT_DIV_CONTROL_SELB_NORM |
>  > +                      VC5_OUT_DIV_CONTROL_SEL_EXT;
>  > +    u8 src = VC5_OUT_DIV_CONTROL_RESET;
>  > +
>  > +    if (index == 0)
>  > +            src |= VC5_OUT_DIV_CONTROL_EN_FOD;
>  > +    else
>  > +            src |= extclk;
>  > +
>  > +    return vc5_update_bits(vc->i2c, VC5_OUT_DIV_CONTROL(num), mask, src);
>  > +}
>  > +
>  > +/*
>  > + * The device references to the Versaclock point to the head, so xlate needs to
>  > + * redirect it to clk_out[idx]
>  > + */
>  > +static int vc5_clk_out_xlate(struct clk *hw, struct ofnode_phandle_args *args)
>  > +{
>  > +    unsigned int idx = args->args[0];
>  > +
>  > +    if (args->args_count != 1) {
>  > +            debug("Invaild args_count: %d\n", args->args_count);
>  > +            return -EINVAL;
>  > +    }
>  > +
>  > +    hw->id = idx;
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +static unsigned long vc5_clk_out_set_rate(struct clk *hw, unsigned long rate)
>  > +{
>  > +    struct udevice *dev;
>  > +    struct vc5_driver_data *vc;
>  > +    struct clk *parent;
>  > +
>  > +    uclass_get_device_by_name(UCLASS_CLK, clk_hw_get_name(hw), &dev);
>  > +    vc = dev_get_priv(dev);
>  > +    parent = clk_get_parent(&vc->clk_out[hw->id].hw);
>  > +
>  > +    /* setting the output rate really means setting the parent FOD rate */
>  > +    return clk_set_rate(parent, clk_round_rate(parent, rate));
>  > +}
>  > +
>  > +static unsigned long vc5_clk_out_get_rate(struct clk *hw)
>  > +{
>  > +    return clk_get_parent_rate(hw);
>  > +}
>  > +
>  > +static const struct clk_ops vc5_clk_out_ops = {
>  > +    .enable = vc5_clk_out_prepare,
>  > +    .disable        = vc5_clk_out_unprepare,
>  > +    .set_rate       = vc5_clk_out_set_rate,
>  > +    .get_rate       = vc5_clk_out_get_rate,
>  > +};
>  > +
>  > +static const struct clk_ops vc5_clk_out_sel_ops = {
>  > +    .enable = vc5_clk_out_prepare,
>  > +    .disable        = vc5_clk_out_unprepare,
>  > +    .get_rate       = vc5_clk_out_get_rate,
>  > +};
>  > +
>  > +static const struct clk_ops vc5_clk_ops = {
>  > +    .enable = vc5_clk_out_prepare,
>  > +    .disable        = vc5_clk_out_unprepare,
>  > +    .of_xlate       = vc5_clk_out_xlate,
>  > +    .set_rate       = vc5_clk_out_set_rate,
>  > +    .get_rate       = vc5_clk_out_get_rate,
>  > +};
>  > +
>  > +static int vc5_map_index_to_output(const enum vc5_model model,
>  > +                               const unsigned int n)
>  > +{
>  > +    switch (model) {
>  > +    case IDT_VC5_5P49V5933:
>  > +            return (n == 0) ? 0 : 3;
>  > +    case IDT_VC5_5P49V5923:
>  > +    case IDT_VC5_5P49V5925:
>  > +    case IDT_VC5_5P49V5935:
>  > +    case IDT_VC6_5P49V6901:
>  > +    case IDT_VC6_5P49V6965:
>  > +    default:
>  > +            return n;
>  > +    }
>  > +}
>  > +
>  > +static int vc5_update_mode(ofnode np_output,
>  > +                       struct vc5_out_data *clk_out)
>  > +{
>  > +    u32 value;
>  > +
>  > +    if (!ofnode_read_u32(np_output, "idt,mode", &value)) {
>  > +            clk_out->clk_output_cfg0_mask |= VC5_CLK_OUTPUT_CFG0_CFG_MASK;
>  > +            switch (value) {
>  > +            case VC5_CLK_OUTPUT_CFG0_CFG_LVPECL:
>  > +            case VC5_CLK_OUTPUT_CFG0_CFG_CMOS:
>  > +            case VC5_CLK_OUTPUT_CFG0_CFG_HCSL33:
>  > +            case VC5_CLK_OUTPUT_CFG0_CFG_LVDS:
>  > +            case VC5_CLK_OUTPUT_CFG0_CFG_CMOS2:
>  > +            case VC5_CLK_OUTPUT_CFG0_CFG_CMOSD:
>  > +            case VC5_CLK_OUTPUT_CFG0_CFG_HCSL25:
>  > +                    clk_out->clk_output_cfg0 |=
>  > +                        value << VC5_CLK_OUTPUT_CFG0_CFG_SHIFT;
>  > +                    break;
>  > +            default:
>  > +                    return -EINVAL;
>  > +            }
>  > +    }
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +static int vc5_update_power(ofnode np_output, struct vc5_out_data *clk_out)
>  > +{
>  > +    u32 value;
>  > +
>  > +    if (!ofnode_read_u32(np_output, "idt,voltage-microvolt", &value)) {
>  > +            clk_out->clk_output_cfg0_mask |= VC5_CLK_OUTPUT_CFG0_PWR_MASK;
>  > +            switch (value) {
>  > +            case 1800000:
>  > +                    clk_out->clk_output_cfg0 |= VC5_CLK_OUTPUT_CFG0_PWR_18;
>  > +                    break;
>  > +            case 2500000:
>  > +                    clk_out->clk_output_cfg0 |= VC5_CLK_OUTPUT_CFG0_PWR_25;
>  > +                    break;
>  > +            case 3300000:
>  > +                    clk_out->clk_output_cfg0 |= VC5_CLK_OUTPUT_CFG0_PWR_33;
>  > +                    break;
>  > +            default:
>  > +                    return -EINVAL;
>  > +            }
>  > +    }
>  > +    return 0;
>  > +}
>  > +
>  > +static int vc5_map_cap_value(u32 femtofarads)
>  > +{
>  > +    int mapped_value;
>  > +
>  > +    /*
>  > +     * The datasheet explicitly states 9000 - 25000 with 0.5pF
>  > +     * steps, but the Programmer's guide shows the steps are 0.430pF.
>  > +     * After getting feedback from Renesas, the .5pF steps were the
>  > +     * goal, but 430nF was the actual values.
>  > +     * Because of this, the actual range goes to 22760 instead of 25000
>  > +     */
>  > +    if (femtofarads < 9000 || femtofarads > 22760)
>  > +            return -EINVAL;
>  > +
>  > +    /*
>  > +     * The Programmer's guide shows XTAL[5:0] but in reality,
>  > +     * XTAL[0] and XTAL[1] are both LSB which makes the math
>  > +     * strange.  With clarfication from Renesas, setting the
>  > +     * values should be simpler by ignoring XTAL[0]
>  > +     */
>  > +    mapped_value = DIV_ROUND_CLOSEST(femtofarads - 9000, 430);
>  > +
>  > +    /*
>  > +     * Since the calculation ignores XTAL[0], there is one
>  > +     * special case where mapped_value = 32.  In reality, this means
>  > +     * the real mapped value should be 111111b.  In other cases,
>  > +     * the mapped_value needs to be shifted 1 to the left.
>  > +     */
>  > +    if (mapped_value > 31)
>  > +            mapped_value = 0x3f;
>  > +    else
>  > +            mapped_value <<= 1;
>  > +
>  > +    return mapped_value;
>  > +}
>  > +
>  > +static int vc5_update_cap_load(ofnode node, struct vc5_driver_data *vc5)
>  > +{
>  > +    u32 value;
>  > +    int mapped_value;
>  > +
>  > +    if (!ofnode_read_u32(node, "idt,xtal-load-femtofarads", &value)) {
>  > +            mapped_value = vc5_map_cap_value(value);
>  > +
>  > +            if (mapped_value < 0)
>  > +                    return mapped_value;
>  > +
>  > +            /*
>  > +             * The mapped_value is really the high 6 bits of
>  > +             * VC5_XTAL_X1_LOAD_CAP and VC5_XTAL_X2_LOAD_CAP, so
>  > +             * shift the value 2 places.
>  > +             */
>  > +            vc5_update_bits(vc5->i2c, VC5_XTAL_X1_LOAD_CAP, ~0x03, mapped_value << 2);
>  > +            vc5_update_bits(vc5->i2c, VC5_XTAL_X2_LOAD_CAP, ~0x03, mapped_value << 2);
>  > +    }
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +static int vc5_update_slew(ofnode np_output, struct vc5_out_data *clk_out)
>  > +{
>  > +    u32 value;
>  > +
>  > +    if (!ofnode_read_u32(np_output, "idt,slew-percent", &value)) {
>  > +            clk_out->clk_output_cfg0_mask |= VC5_CLK_OUTPUT_CFG0_SLEW_MASK;
>  > +
>  > +            switch (value) {
>  > +            case 80:
>  > +                    clk_out->clk_output_cfg0 |= VC5_CLK_OUTPUT_CFG0_SLEW_80;
>  > +                    break;
>  > +            case 85:
>  > +                    clk_out->clk_output_cfg0 |= VC5_CLK_OUTPUT_CFG0_SLEW_85;
>  > +                    break;
>  > +            case 90:
>  > +                    clk_out->clk_output_cfg0 |= VC5_CLK_OUTPUT_CFG0_SLEW_90;
>  > +                    break;
>  > +            case 100:
>  > +                    clk_out->clk_output_cfg0 |=
>  > +                        VC5_CLK_OUTPUT_CFG0_SLEW_100;
>  > +                    break;
>  > +            default:
>  > +                    return -EINVAL;
>  > +            }
>  > +    }
>  > +    return 0;
>  > +}
>  > +
>  > +static int vc5_get_output_config(struct udevice *dev,
>  > +                             struct vc5_out_data *clk_out)
>  > +{
>  > +    ofnode np_output;
>  > +    char child_name[5];
>  > +    int ret = 0;
>  > +
>  > +    sprintf(child_name, "OUT%d", clk_out->num + 1);
>  > +
>  > +    np_output = dev_read_subnode(dev, child_name);
>  > +
>  > +    if (!ofnode_valid(np_output)) {
>
> Same comment as last time: use dev_read_u32() and friends instead of
> passing around an ofnode.

Sean,

I am not sure I understand what you're asking here. I will admit I am
fairly new to the U-Boot clock API.

We have multiple child nodes called OUTx where x can vary from chip to
chip and the contents inside these OUTx nodes may also vary depending
on the application.

versaclock: clock-controller at 6a {
    ...
     OUT1 {
          idt,mode = <VC5_CMOS>;
          idt,voltage-microvolt = <1800000>;
          idt,slew-percent = <100>;
     };
    ...
    OUTx {
          idt,mode = <VC5_CMOS>;
          idt,voltage-microvolt = <1800000>;
          idt,slew-percent = <100>;
     };

};

Since the *dev in this case points to the parent, clock-controller at 6a,
 we need to traverse into the corresponding OUTx node based on the
clock index, clk_out_num + 1.
Each OUTx node is independent of the others so searching for idt,mode,
idt,voltage-microvolt, and idt,slew-precent from the device tree may
yield different values depending on which OUTx node is being read.
They may not be the same.

The idea behind this is to identify the OUTx node, and then read the
values accordingly to that node.  Using dev_read32, I can see that we
can read the properties, but it's not clear to me how to specify which
child subnode we'd be reading.
If we eliminate the passing of the subnode, then each function called
would have to determine the subnode and that seems like it would be
duplicating code.

>
>  > +            dev_dbg(dev, "!ofnode_valid(np_output)\n");
>  > +            return 0;
>  > +    }
>  > +
>  > +    ret = vc5_update_mode(np_output, clk_out);
>  > +    if (ret)
>  > +            goto output_error;
>  > +
>  > +    ret = vc5_update_power(np_output, clk_out);
>  > +    if (ret)
>  > +            goto output_error;
>  > +
>  > +    ret = vc5_update_slew(np_output, clk_out);
>  > +
>  > +output_error:
>  > +    if (ret)
>  > +            dev_dbg(dev, "Invalid clock output configuration OUT%d\n", clk_out->num + 1);
>  > +
>  > +    return ret;
>  > +}
>  > +
>  > +static char *versaclock_get_name(const char *dev_name, const char *clk_name, int index)
>  > +{
>  > +    int length;
>  > +    char *buf;
>  > +
>  > +    if (index < 0)
>  > +            length = snprintf(NULL, 0, "%s.%s", dev_name, clk_name) + 1;
>  > +    else
>  > +            length = snprintf(NULL, 0, "%s.%s%d", dev_name, clk_name, index) + 1;
>  > +
>  > +    buf = malloc(length);
>  > +    if (!buf)
>  > +            ERR_PTR(-ENOMEM);
>  > +
>  > +    if (index < 0)
>  > +            snprintf(buf, length, "%s.%s", dev_name, clk_name);
>  > +    else
>  > +            snprintf(buf, length, "%s.%s%d", dev_name, clk_name, index);
>  > +
>  > +    return buf;
>  > +}
>  > +
>  > +int versaclock_probe(struct udevice *dev)
>  > +{
>  > +    struct vc5_driver_data *vc5 = dev_get_priv(dev);
>  > +    struct vc5_chip_info *chip = (void *)dev_get_driver_data(dev);
>  > +    unsigned int n, idx = 0;
>  > +    char *mux_name, *pfd_name, *pll_name, *outsel_name;
>  > +    char *out_name[VC5_MAX_CLK_OUT_NUM];
>  > +    char *fod_name[VC5_MAX_FOD_NUM];
>  > +    int ret;
>  > +    u64 val;
>  > +
>  > +    val = (u64)dev_read_addr_ptr(dev);
>  > +    ret = i2c_get_chip(dev->parent, val, 1, &vc5->i2c);
>  > +
>  > +    if (ret) {
>  > +            dev_err(dev, "I2C probe failed.\n");
>  > +            return ret;
>  > +    }
>  > +
>  > +    vc5->chip_info = chip;
>  > +    vc5->pin_xin = devm_clk_get(dev, "xin");
>  > +
>  > +    if (IS_ERR(vc5->pin_xin))
>  > +            dev_err(dev, "failed to get xin clock\n");
>
> And the dev_dbg/log_msg_ret comments apply to this probe function as
> well.

I'll replace the dev_err with dev_dgb.

>
>  > +
>  > +    ret = clk_enable(vc5->pin_xin);
>  > +    if (ret)
>  > +            dev_err(dev, "failed to enable XIN clock\n");
>  > +
>  > +    vc5->pin_clkin = devm_clk_get(dev, "clkin");
>  > +
>  > +    /* Register clock input mux */
>  > +    if (!IS_ERR(vc5->pin_xin)) {
>  > +            vc5->clk_mux_ins |= VC5_MUX_IN_XIN;
>  > +    } else if (vc5->chip_info->flags & VC5_HAS_INTERNAL_XTAL) {
>  > +            if (IS_ERR(vc5->pin_xin))
>  > +                    return PTR_ERR(vc5->pin_xin);
>  > +            vc5->clk_mux_ins |= VC5_MUX_IN_XIN;
>  > +    }
>  > +
>  > +    mux_name = versaclock_get_name(dev->name, "mux", -1);
>  > +    if (IS_ERR(mux_name))
>  > +            printf("mux_name: %lu\n", PTR_ERR(mux_name));
>  > +
>  > +    clk_register(&vc5->clk_mux, "versaclock-mux", mux_name, vc5->pin_xin->dev->name);
>  > +
>  > +    if (!IS_ERR(vc5->pin_xin))
>  > +            vc5_mux_set_parent(&vc5->clk_mux, 1);
>  > +    else
>  > +            vc5_mux_set_parent(&vc5->clk_mux, 0);
>  > +
>  > +    /* Configure Optional Loading Capacitance for external XTAL */
>  > +    if (!(vc5->chip_info->flags & VC5_HAS_INTERNAL_XTAL)) {
>  > +            ret = vc5_update_cap_load(dev_ofnode(dev), vc5);
>  > +            if (ret)
>  > +                    dev_err(dev, "failed to vc5_update_cap_load\n");
>  > +    }
>  > +
>  > +    /* Register PFD */
>  > +    pfd_name = versaclock_get_name(dev->name, "pfd", -1);
>  > +    if (IS_ERR(pfd_name))
>  > +            printf("pfd_name: %lu\n", PTR_ERR(pfd_name));
>
> In particular, printf is almost always incorrect when used in a driver.

Sorry, I have egg on my face.  I added some new printfs for debugging
my new error handling, and I forgot to remove them.

I will make sure the printfs are gone.  Sorry about that.

>
> --Sean
>
>  > +
>  > +    ret = clk_register(&vc5->clk_pfd, "versaclock-pfd", pfd_name, vc5->clk_mux.dev->name);
>  > +    if (ret)
>  > +            goto free_mux;
>  > +
>  > +    /* Register PLL */
>  > +    vc5->clk_pll.num = 0;
>  > +    vc5->clk_pll.vc5 = vc5;
>  > +    pll_name = versaclock_get_name(dev->name, "pll", -1);
>  > +    if (IS_ERR(pll_name)) {
>  > +            ret = PTR_ERR(pll_name);
>  > +            goto free_pfd;
>  > +    }
>  > +
>  > +    ret = clk_register(&vc5->clk_pll.hw, "versaclock-pll", pll_name, vc5->clk_pfd.dev->name);
>  > +    if (ret)
>  > +            goto free_pll;
>  > +
>  > +    /* Register FODs */
>  > +    for (n = 0; n < vc5->chip_info->clk_fod_cnt; n++) {
>  > +            fod_name[n] = versaclock_get_name(dev->name, "fod", n);
>  > +            if (IS_ERR(pll_name))
>  > +                    printf("fod_name[%d]: %lu\n", n, PTR_ERR(fod_name[n]));
>  > +            idx = vc5_map_index_to_output(vc5->chip_info->model, n);
>  > +            vc5->clk_fod[n].num = idx;
>  > +            vc5->clk_fod[n].vc5 = vc5;
>  > +            ret = clk_register(&vc5->clk_fod[n].hw, "versaclock-fod", fod_name[n],
>  > +                               vc5->clk_pll.hw.dev->name);
>  > +            if (ret)
>  > +                    goto free_fod;
>  > +    }
>  > +
>  > +    /* Register MUX-connected OUT0_I2C_SELB output */
>  > +    vc5->clk_out[0].num = idx;
>  > +    vc5->clk_out[0].vc5 = vc5;
>  > +    outsel_name = versaclock_get_name(dev->name, "out0_sel_i2cb", -1);
>  > +    if (IS_ERR(outsel_name)) {
>  > +            ret = PTR_ERR(outsel_name);
>  > +            goto free_fod;
>  > +    };
>  > +
>  > +    ret = clk_register(&vc5->clk_out[0].hw, "versaclock-outsel",  outsel_name,
>  > +                       vc5->clk_mux.dev->name);
>  > +    if (ret)
>  > +            goto free_selb;
>  > +
>  > +    /* Register FOD-connected OUTx outputs */
>  > +    for (n = 1; n < vc5->chip_info->clk_out_cnt; n++) {
>  > +            idx = vc5_map_index_to_output(vc5->chip_info->model, n - 1);
>  > +            out_name[n] = versaclock_get_name(dev->name, "out", n);
>  > +            if (IS_ERR(out_name[n])) {
>  > +                    ret = PTR_ERR(out_name[n]);
>  > +                    goto free_selb;
>  > +            }
>  > +            vc5->clk_out[n].num = idx;
>  > +            vc5->clk_out[n].vc5 = vc5;
>  > +            ret = clk_register(&vc5->clk_out[n].hw, "versaclock-out", out_name[n],
>  > +                               vc5->clk_fod[idx].hw.dev->name);
>  > +            if (ret)
>  > +                    goto free_out;
>  > +            vc5_clk_out_set_parent(vc5, idx, 0);
>  > +
>  > +            /* Fetch Clock Output configuration from DT (if specified) */
>  > +            ret = vc5_get_output_config(dev, &vc5->clk_out[n]);
>  > +            if (ret) {
>  > +                    dev_err(dev, "failed to vc5_get_output_config()\n");
>  > +                    goto free_out;
>  > +            }
>  > +    }
>  > +
>  > +    return 0;
>  > +
>  > +free_out:
>  > +    for (n = 1; n < vc5->chip_info->clk_out_cnt; n++) {
>  > +            clk_free(&vc5->clk_out[n].hw);
>  > +            free(out_name[n]);
>  > +    }
>  > +free_selb:
>  > +    clk_free(&vc5->clk_out[0].hw);
>  > +    free(outsel_name);
>  > +free_fod:
>  > +    for (n = 0; n < vc5->chip_info->clk_fod_cnt; n++) {
>  > +            clk_free(&vc5->clk_fod[n].hw);
>  > +            free(fod_name[n]);
>  > +    }
>  > +free_pll:
>  > +    clk_free(&vc5->clk_pll.hw);
>  > +    free(pll_name);
>  > +free_pfd:
>  > +    clk_free(&vc5->clk_pfd);
>  > +    free(pfd_name);
>  > +free_mux:
>  > +    clk_free(&vc5->clk_mux);
>  > +    free(mux_name);
>  > +
>  > +    return ret;
>  > +}
>  > +
>  > +static const struct udevice_id versaclock_ids[] = {
>  > +    { .compatible = "idt,5p49v5923", .data = (ulong)&idt_5p49v5923_info },
>  > +    { .compatible = "idt,5p49v5925", .data = (ulong)&idt_5p49v5925_info },
>  > +    { .compatible = "idt,5p49v5933", .data = (ulong)&idt_5p49v5933_info },
>  > +    { .compatible = "idt,5p49v5935", .data = (ulong)&idt_5p49v5935_info },
>  > +    { .compatible = "idt,5p49v6901", .data = (ulong)&idt_5p49v6901_info },
>  > +    { .compatible = "idt,5p49v6965", .data = (ulong)&idt_5p49v6965_info },
>  > +    {},
>  > +};
>  > +
>  > +U_BOOT_DRIVER(versaclock) = {
>  > +    .name           = "versaclock",
>  > +    .id             = UCLASS_CLK,
>  > +    .ops            = &vc5_clk_ops,
>  > +    .of_match       = versaclock_ids,
>  > +    .probe          = versaclock_probe,
>  > +    .priv_auto      = sizeof(struct vc5_driver_data),
>  > +};
>  > +
>  > +U_BOOT_DRIVER(versaclock_mux) = {
>  > +    .name           = "versaclock-mux",
>  > +    .id             = UCLASS_CLK,
>  > +    .ops            = &vc5_mux_ops,
>  > +};
>  > +
>  > +U_BOOT_DRIVER(versaclock_pfd) = {
>  > +    .name           = "versaclock-pfd",
>  > +    .id             = UCLASS_CLK,
>  > +    .ops            = &vc5_pfd_ops,
>  > +};
>  > +
>  > +U_BOOT_DRIVER(versaclock_pll) = {
>  > +    .name           = "versaclock-pll",
>  > +    .id             = UCLASS_CLK,
>  > +    .ops            = &vc5_pll_ops,
>  > +};
>  > +
>  > +U_BOOT_DRIVER(versaclock_fod) = {
>  > +    .name           = "versaclock-fod",
>  > +    .id             = UCLASS_CLK,
>  > +    .ops            = &vc5_fod_ops,
>  > +};
>  > +
>  > +U_BOOT_DRIVER(versaclock_out) = {
>  > +    .name           = "versaclock-out",
>  > +    .id             = UCLASS_CLK,
>  > +    .ops            = &vc5_clk_out_ops,
>  > +};
>  > +
>  > +U_BOOT_DRIVER(versaclock_outsel) = {
>  > +    .name           = "versaclock-outsel",
>  > +    .id             = UCLASS_CLK,
>  > +    .ops            = &vc5_clk_out_sel_ops,
>  > +};
>  >


More information about the U-Boot mailing list