Date:   Tue, 7 Jan 2020 07:35:01 +0000
From:   Mike Looijmans <mike.looijmans@...ic.nl>
To:     Stephen Boyd <sboyd@...nel.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mturquette@...libre.com" <mturquette@...libre.com>
Subject: Re: [PATCH] clk, clk-si5341: Support multiple input ports


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@...ic.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@...ic.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)

Powered by blists - more mailing lists