Messages in this thread | | | From | Mike Looijmans <> | Subject | Re: [PATCH] clk, clk-si5341: Support multiple input ports | Date | Tue, 7 Jan 2020 07:35:01 +0000 |
| |
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)
| |