Hi Marek,
Thank you for the patch.
On Wednesday 11 Jan 2017 15:37:11 Marek Vasut wrote:
> On 01/10/2017 07:50 PM, Marek Vasut wrote:
> > Add driver for IDT VersaClock 5 5P49V5923 and 5P49V5933 chips. These
> > chips have two clock inputs, XTAL or CLK, which are muxed into single
> > PLL/VCO input. In case of 5P49V5923, the XTAL in built into the chip
> > while the 5P49V5923 requires external XTAL.
> >
> > The PLL feeds two fractional dividers. Each fractional divider feeds
> > output mux, which allows selecting between clock from the fractional
> > divider itself or from output mux on output N-1. In case of output
> > mux 0, the output N-1 is instead connected to the output from the mux
> > feeding the PLL.
> >
> > The driver thus far supports only the 5P49V5923 and 5P49V5933, while
> > it should be easily extensible to the whole 5P49V59xx family of chips
> > as they are all pretty similar.
> >
> > Signed-off-by: Marek Vasut <marek.vasut@xxxxxxxxx>
> > Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
>
> +Cc Laurent, linux-renesas
>
> > ---
> > V2: - Drop address-cells and size-cells from the binding
> > - Add clock-names to the binding
> > - Drop vc5_input_names
> > - Fix assortment of spelling mistakes
> > - Switch div_frc and div_int datatype to uXX
> > - Switch f_in to u32
> > - Add missing remove
> > - Define macros for handling XIN and CLKIN
> > - Rework the FOD fractional divider calculation, this was wrong
> > and made the output clock be off considerably under certain
> > circumstances (when the fractional part was large).
> >
> > V3: - Rework the MUX frequency recalculation and divider configration
> > so that it fits into the clock framework
> > - Add support for 5P49V5933 chip to lay groundwork for adding more
> > chips easily.
> > - Export the OUT0_SEL_I2CB output into the clock framework, so it
> > can be accessed from DT as well. WARNING: This does change the
> > bindings, clock0 is now the OUT0_SEL_I2CB, clock1 is OUT1 and
> > clock2 is OUT2 (or OUT4 on the 5P49V5933).
> > - Drop unnecessary caching of pin_*_name clock name and clk_hw
> > structures in driver data.
> > - Add missing MAINTAINERS entry
> >
> > V4: - Support also 5P49V5923A by dropping the B from the bindings and
> > code. According to the update notice, the chips are identical
> > except for disabling the VCO monitoring, which is internal
> > factory-defined bit and has no impact on silicon performance.
> >
> > ---
> >
> > .../devicetree/bindings/clock/idt,versaclock5.txt | 43 ++
If you decide to split the bindings in a separate patch as often requested by
the DT reviewers,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
for them. Please see below for more comments.
> > MAINTAINERS | 5 +
> > drivers/clk/Kconfig | 10 +
> > drivers/clk/Makefile | 1 +
> > drivers/clk/clk-versaclock5.c | 821 ++++++++++++++++
> > 5 files changed, 880 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/clock/idt,versaclock5.txt create mode
> > 100644 drivers/clk/clk-versaclock5.c
[snip]
> > diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> > new file mode 100644
> > index 000000000000..14f518e84d6d
> > --- /dev/null
> > +++ b/drivers/clk/clk-versaclock5.c
[snip]
> > +/*
> > + * VersaClock5 i2c regmap
> > + */
> > +static bool vc5_regmap_is_writeable(struct device *dev, unsigned int reg)
> > +{
> > + /* Factory reserved regs, make them read-only */
> > + if (reg >= 0 && reg <= 0xf)
reg is unsigned, it will always be >= 0. You can drop the first part of the
condition.
> > + return false;
> > +
> > + /* Factory reserved regs, make them read-only */
> > + if (reg == 0x14 || reg == 0x1c || reg == 0x1d)
> > + return false;
> > +
> > + return true;
> > +}
[snip]
> > +static long vc5_mux_round_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *parent_rate)
> > +{
> > + unsigned long idiv;
> > +
> > + /* PLL cannot operate with input clock above 50 MHz. */
> > + if (rate > 50000000)
> > + return -EINVAL;
As this is a PLL constraint, shouldn't it be enforced by vc5_pll_round_rate()
instead ?
> > + /* 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 int vc5_mux_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct vc5_driver_data *vc5 =
> > + container_of(hw, struct vc5_driver_data, clk_mux);
> > + unsigned long idiv;
> > + u8 div;
> > +
> > + /* CLKIN within range of PLL input, feed directly to PLL. */
> > + if (parent_rate <= 50000000) {
> > + regmap_update_bits(vc5->regmap, VC5_VCO_CTRL_AND_PREDIV,
> > + VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV,
> > + VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV);
> > + regmap_update_bits(vc5->regmap, 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);
> > +
> > + regmap_update_bits(vc5->regmap, VC5_REF_DIVIDER, 0xff, div);
> > + regmap_update_bits(vc5->regmap, VC5_VCO_CTRL_AND_PREDIV,
> > + VC5_VCO_CTRL_AND_PREDIV_BYPASS_PREDIV, 0);
> > +
> > + return 0;
> > +}
[snip]
> > +static struct clk_hw *
> > +vc5_of_clk_get(struct of_phandle_args *clkspec, void *data)
> > +{
> > + struct vc5_driver_data *vc5 = data;
> > + unsigned int idx = clkspec->args[0];
> > +
> > + if (idx > 2) {
> > + pr_err("%s: invalid index %u\n", __func__, idx);
Does this deserve an error message ?
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + return &vc5->clk_out[idx].hw;
> > +}
> > +
> > +static int vc5_map_index_to_output(const enum vc5_model model,
> > + const unsigned int n)
> > +{
> > + switch (model) {
> > + case IDT_VC5_5P49V5923:
> > + if (n == 0) /* OUT1 */
> > + return 0;
> > + if (n == 1) /* OUT2 */
> > + return 1;
> > + break;
> > + case IDT_VC5_5P49V5933:
> > + if (n == 0) /* OUT1 */
> > + return 0;
> > + if (n == 1) /* OUT4 */
> > + return 3;
> > + break;
> > + }
> > +
> > + /*
> > + * If we got here, there is certainly a bug in the driver,
> > + * but we shouldn't crash the kernel.
> > + */
> > + WARN_ON("Invalid clock index!\n");
> > + return -EINVAL;
I don't think that's needed. The function is called from two locations only,
and they're both very easy to audit. You can just return n in the
IDT_VC5_5P49V5923 case and return n == 0 ? 0 : 3 in the other case.
> > +}
[snip]
> > +static int vc5_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + const struct of_device_id *of_id =
> > + of_match_device(clk_vc5_of_match, &client->dev);
> > + struct vc5_driver_data *vc5;
> > + struct clk_init_data init;
> > + const char *parent_names[2];
> > + unsigned int n, idx;
> > + int ret;
> > +
> > + vc5 = devm_kzalloc(&client->dev, sizeof(*vc5), GFP_KERNEL);
> > + if (vc5 == NULL)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(client, vc5);
> > + vc5->client = client;
> > + vc5->model = (enum vc5_model)of_id->data;
> > +
> > + vc5->pin_xin = devm_clk_get(&client->dev, "xin");
> > + if (PTR_ERR(vc5->pin_xin) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> > +
> > + vc5->pin_clkin = devm_clk_get(&client->dev, "clkin");
> > + if (PTR_ERR(vc5->pin_clkin) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> > +
> > + vc5->regmap = devm_regmap_init_i2c(client, &vc5_regmap_config);
> > + if (IS_ERR(vc5->regmap)) {
> > + dev_err(&client->dev, "failed to allocate register map\n");
> > + return PTR_ERR(vc5->regmap);
> > + }
> > +
> > + /* Register clock input mux */
> > + memset(&init, 0, sizeof(init));
> > +
> > + if (!IS_ERR(vc5->pin_xin)) {
> > + clk_prepare_enable(vc5->pin_xin);
Given that the CCF core will take care of enabling/disabling parents as
needed, do you need this ?
> > + vc5->clk_mux_ins |= VC5_MUX_IN_XIN;
> > + parent_names[init.num_parents++] =
> > __clk_get_name(vc5->pin_xin);
> > + } else if (vc5->model == IDT_VC5_5P49V5933) {
> > + /* IDT VC5 5P49V5933 has built-in oscilator. */
> > + vc5->pin_xin = clk_register_fixed_rate(&client->dev,
> > + "internal-xtal",
> > NULL,
> > + 0, 25000000);
> > + if (IS_ERR(vc5->pin_xin))
> > + return PTR_ERR(vc5->pin_xin);
> > + vc5->clk_mux_ins |= VC5_MUX_IN_XIN;
> > + parent_names[init.num_parents++] =
> > __clk_get_name(vc5->pin_xin); + }
> > +
> > + if (!IS_ERR(vc5->pin_clkin)) {
> > + clk_prepare_enable(vc5->pin_clkin);
> > + vc5->clk_mux_ins |= VC5_MUX_IN_CLKIN;
> > + parent_names[init.num_parents++] =
> > + __clk_get_name(vc5->pin_clkin);
> > + }
> > +
> > + if (!init.num_parents) {
> > + dev_err(&client->dev, "no input clock specified!\n");
> > + return -EINVAL;
> > + }
> > +
> > + init.name = vc5_mux_names[0];
> > + init.ops = &vc5_mux_ops;
> > + init.flags = 0;
> > + init.parent_names = parent_names;
> > + vc5->clk_mux.init = &init;
> > + ret = devm_clk_hw_register(&client->dev, &vc5->clk_mux);
> > + if (ret) {
> > + dev_err(&client->dev, "unable to register %s\n", init.name);
> > + goto err_clk;
> > + }
> > +
> > + /* Register PLL */
> > + memset(&init, 0, sizeof(init));
> > + init.name = vc5_pll_names[0];
> > + init.ops = &vc5_pll_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + init.parent_names = vc5_mux_names;
> > + init.num_parents = 1;
> > + vc5->clk_pll.num = 0;
> > + vc5->clk_pll.vc5 = vc5;
> > + vc5->clk_pll.hw.init = &init;
> > + ret = devm_clk_hw_register(&client->dev, &vc5->clk_pll.hw);
> > + if (ret) {
> > + dev_err(&client->dev, "unable to register %s\n", init.name);
> > + goto err_clk;
> > + }
> > +
> > + /* Register FODs */
> > + for (n = 0; n < 2; n++) {
> > + idx = vc5_map_index_to_output(vc5->model, n);
> > + memset(&init, 0, sizeof(init));
> > + init.name = vc5_fod_names[idx];
> > + init.ops = &vc5_fod_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + init.parent_names = vc5_pll_names;
> > + init.num_parents = 1;
> > + vc5->clk_fod[n].num = idx;
> > + vc5->clk_fod[n].vc5 = vc5;
> > + vc5->clk_fod[n].hw.init = &init;
> > + ret = devm_clk_hw_register(&client->dev,
> > &vc5->clk_fod[n].hw); + if (ret) {
> > + dev_err(&client->dev, "unable to register %s\n",
> > + init.name);
> > + goto err_clk;
> > + }
> > + }
> > +
> > + /* Register MUX-connected OUT0_I2C_SELB output */
> > + memset(&init, 0, sizeof(init));
> > + init.name = vc5_clk_out_names[0];
> > + init.ops = &vc5_clk_out_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + init.parent_names = vc5_mux_names;
> > + init.num_parents = 1;
> > + vc5->clk_out[0].num = idx;
> > + vc5->clk_out[0].vc5 = vc5;
> > + vc5->clk_out[0].hw.init = &init;
> > + ret = devm_clk_hw_register(&client->dev, &vc5->clk_out[0].hw);
> > + if (ret) {
> > + dev_err(&client->dev, "unable to register %s\n",
> > + init.name);
> > + goto err_clk;
> > + }
> > +
> > + /* Register FOD-connected OUTx outputs */
> > + for (n = 1; n < 3; n++) {
> > + idx = vc5_map_index_to_output(vc5->model, n - 1);
> > + parent_names[0] = vc5_fod_names[idx];
> > + if (n == 1)
> > + parent_names[1] = vc5_mux_names[0];
> > + else
> > + parent_names[1] = vc5_clk_out_names[n - 1];
> > +
> > + memset(&init, 0, sizeof(init));
> > + init.name = vc5_clk_out_names[idx + 1];
> > + init.ops = &vc5_clk_out_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + init.parent_names = parent_names;
> > + init.num_parents = 2;
> > + vc5->clk_out[n].num = idx;
> > + vc5->clk_out[n].vc5 = vc5;
> > + vc5->clk_out[n].hw.init = &init;
> > + ret = devm_clk_hw_register(&client->dev,
> > + &vc5->clk_out[n].hw);
> > + if (ret) {
> > + dev_err(&client->dev, "unable to register %s\n",
> > + init.name);
> > + goto err_clk;
> > + }
> > + }
> > +
> > + ret = of_clk_add_hw_provider(client->dev.of_node, vc5_of_clk_get,
> > vc5); + if (ret) {
> > + dev_err(&client->dev, "unable to add clk provider\n");
> > + goto err_clk;
> > + }
> > +
> > + return 0;
> > +
> > +err_clk:
> > + if (!IS_ERR(vc5->pin_xin))
> > + clk_disable_unprepare(vc5->pin_xin);
> > + if (!IS_ERR(vc5->pin_clkin))
> > + clk_disable_unprepare(vc5->pin_clkin);
> > + if (vc5->model == IDT_VC5_5P49V5933)
> > + clk_unregister_fixed_rate(vc5->pin_xin);
> > + return ret;
> > +}
[snip]
--
Regards,
Laurent Pinchart
