| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <865d0c3c-666a-2b2e-585f-24bd3522e530@topic.nl>
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