Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Mike Looijmans <mike.looijmans@topic.nl>, linux-clk@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, mturquette@baylibre.com,
	Mike Looijmans <mike.looijmans@topic.nl>
Subject: Re: [PATCH] clk, clk-si5341: Support multiple input ports
Date: Mon, 06 Jan 2020 21:48:37 -0800
Message-ID: <20200107054837.DB91F2075A@mail.kernel.org> (raw)
In-Reply-To: <20191205115734.6987-1-mike.looijmans@topic.nl>

Quoting Mike Looijmans (2019-12-05 03:57:34)
> The Si5341 and Si5340 have multiple input clock options. So far, the driver
> only supported the XTAL input, this adds support for the three external
> clock inputs as well.
> 
> If the clock chip is't programmed at boot, the driver will default to the

isn't

> XTAL input as before. If there is no "xtal" clock input available, it will
> pick the first connected input (e.g. "in0") as the input clock for the PLL.
> One can use clock-assigned-parents to select a particular clock as input.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

> diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c
> index 6e780c2a9e6b..f7dba7698083 100644
> --- a/drivers/clk/clk-si5341.c
> +++ b/drivers/clk/clk-si5341.c
> @@ -4,7 +4,6 @@
>   * Copyright (C) 2019 Topic Embedded Products
>   * Author: Mike Looijmans <mike.looijmans@topic.nl>
>   */
> -
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/delay.h>

I think we can do without this hunk.

> @@ -390,7 +410,112 @@ static unsigned long si5341_clk_recalc_rate(struct clk_hw *hw,
>         return (unsigned long)res;
>  }
>  
> +static int si5341_clk_get_selected_input(struct clk_si5341 *data)
> +{
> +       int err;
> +       u32 val;
> +
> +       err = regmap_read(data->regmap, SI5341_IN_SEL, &val);
> +       if (err < 0)
> +               return err;
> +
> +       return (val & SI5341_IN_SEL_MASK) >> SI5341_IN_SEL_SHIFT;
> +}
> +
> +static unsigned char si5341_clk_get_parent(struct clk_hw *hw)

Please use u8 for now.

> +{
> +       struct clk_si5341 *data = to_clk_si5341(hw);
> +       int res = si5341_clk_get_selected_input(data);
> +
> +       if (res < 0)
> +               return 0; /* Apparently we cannot report errors */

For now this is the case. I'll rekick the series to convert this API to
a function that returns clk_hw pointers.
 
> +
> +       return res;
> +}
> +
[...]
> @@ -985,7 +1110,8 @@ static const struct regmap_range si5341_regmap_volatile_range[] = {
>         regmap_reg_range(0x000C, 0x0012), /* Status */
>         regmap_reg_range(0x001C, 0x001E), /* reset, finc/fdec */
>         regmap_reg_range(0x00E2, 0x00FE), /* NVM, interrupts, device ready */
> -       /* Update bits for synth config */
> +       /* Update bits for P divider and synth config */
> +       regmap_reg_range(SI5341_PX_UPD, SI5341_PX_UPD),
>         regmap_reg_range(SI5341_SYNTH_N_UPD(0), SI5341_SYNTH_N_UPD(0)),
>         regmap_reg_range(SI5341_SYNTH_N_UPD(1), SI5341_SYNTH_N_UPD(1)),
>         regmap_reg_range(SI5341_SYNTH_N_UPD(2), SI5341_SYNTH_N_UPD(2)),
> @@ -1122,6 +1248,7 @@ static int si5341_initialize_pll(struct clk_si5341 *data)
>         struct device_node *np = data->i2c_client->dev.of_node;
>         u32 m_num = 0;
>         u32 m_den = 0;
> +       int sel;
>  
>         if (of_property_read_u32(np, "silabs,pll-m-num", &m_num)) {
>                 dev_err(&data->i2c_client->dev,
> @@ -1135,7 +1262,11 @@ static int si5341_initialize_pll(struct clk_si5341 *data)
>         if (!m_num || !m_den) {
>                 dev_err(&data->i2c_client->dev,
>                         "PLL configuration invalid, assume 14GHz\n");
> -               m_den = clk_get_rate(data->pxtal) / 10;
> +               sel = si5341_clk_get_selected_input(data);
> +               if (sel < 0)
> +                       return sel;
> +
> +               m_den = clk_get_rate(data->input_clk[sel]) / 10;
>                 m_num = 1400000000;
>         }
>  
> @@ -1143,11 +1274,52 @@ static int si5341_initialize_pll(struct clk_si5341 *data)
>                         SI5341_PLL_M_NUM, m_num, m_den);
>  }
>  
> +static int si5341_clk_select_active_input(struct clk_si5341 *data)
> +{
> +       int res;
> +       int err;
> +       int i;
> +
> +       res = si5341_clk_get_selected_input(data);
> +       if (res < 0)
> +               return res;
> +
> +       /* If the current register setting is invalid, pick the first input */
> +       if (!data->input_clk[res]) {
> +               dev_dbg(&data->i2c_client->dev,
> +                       "Input %d not connected, rerouting\n", res);
> +               res = -ENODEV;
> +               for (i = 0; i < SI5341_NUM_INPUTS; ++i) {
> +                       if (data->input_clk[i]) {
> +                               res = i;
> +                               break;
> +                       }
> +               }
> +               if (res < 0) {

What if res is == SI5341_NUM_INPUTS?

> +                       dev_err(&data->i2c_client->dev,
> +                               "No clock input available\n");
> +                       return res;
> +               }
> +       }
> +
> +       /* Make sure the selected clock is also enabled and routed */
> +       err = si5341_clk_reparent(data, res);
> +       if (err < 0)
> +               return err;
> +
> +       err = clk_prepare_enable(data->input_clk[res]);

Is it possible to do this setup and configuration stuff when the clk is
prepared by something? Maybe I've asked this before but I'd prefer that
this driver is a clk provider and not a clk consumer. If some default
setup needs to be done, preferably do that via direct register writes or
by calling the clk_ops functions directly instead of going through the
framework, preferably before registering the clks to the framework.

> +       if (err < 0)
> +               return err;
> +
> +       return res;
> +}
> +

  parent reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-05 11:57 Mike Looijmans
2019-12-18  9:11 ` Mike Looijmans
2020-01-07  5:48 ` Stephen Boyd [this message]
2020-01-07  7:35   ` Mike Looijmans
2020-01-07  7:53   ` [PATCH v2] " Mike Looijmans
2020-01-28 21:06     ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200107054837.DB91F2075A@mail.kernel.org \
    --to=sboyd@kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mike.looijmans@topic.nl \
    --cc=mturquette@baylibre.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git