Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: Mike Looijmans <mike.looijmans@topic.nl>
To: Stephen Boyd <sboyd@kernel.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"mturquette@baylibre.com" <mturquette@baylibre.com>
Subject: Re: [PATCH] clk, clk-si5341: Support multiple input ports
Date: Tue, 7 Jan 2020 07:35:01 +0000
Message-ID: <865d0c3c-666a-2b2e-585f-24bd3522e530@topic.nl> (raw)
In-Reply-To: <20200107054837.DB91F2075A@mail.kernel.org>


On 07-01-2020 06:48, Stephen Boyd wrote:
> 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?

Cannot happen, "i" can be SI5341_NUM_INPUTS but "res" is only set to 
non-negative if a match was found.

> 
>> +                       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.

I suppose it would be possible to do so, and I do recall some discussion in 
the past.

The reason for the call here is that the input clock must be up and running at 
this point, because it drives the ~14GHz PLL that drives all other clocks. The 
input clock will undoubtedly be a crystal or fixed-frequency oscillator.

My main reason for doing this here is that the clock chip is often 
pre-programmed and already up and running at boot. It might even be providing 
the CPU clock...

Can we postpone this to a new patch? It'd be quite a big change to alter this, 
and the clk_prepare_enable was already there.

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

I'll send a v2 with the above changes (except that last one)


  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
2020-01-07  7:35   ` Mike Looijmans [this message]
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=865d0c3c-666a-2b2e-585f-24bd3522e530@topic.nl \
    --to=mike.looijmans@topic.nl \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=sboyd@kernel.org \
    /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