Messages in this thread | | | Date | Wed, 7 Feb 2024 06:54:10 -0600 | From | Nishanth Menon <> | Subject | Re: [PATCH v3] clk: keystone: sci-clk: Adding support for non contiguous clocks |
| |
On 14:41-20240207, Udit Kumar wrote: > Most of clocks and their parents are defined in contiguous range, > But in few cases, there is gap in clock numbers[0]. > Driver assumes clocks to be in contiguous range, and add their clock > ids incrementally. > > New firmware started returning error while calling get_freq and is_on > API for non-available clock ids. > > In this fix, driver checks and adds only valid clock ids. > > Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks") > > [0] https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html > Section Clocks for NAVSS0_CPTS_0 Device, > clock id 12-15 not present. > > Signed-off-by: Udit Kumar <u-kumar1@ti.com> > --- > Changelog > Changes in v3 > - instead of get_freq, is_auto API is used to check validilty of clock > - Address comments of v2, to have preindex increment > Link to v2 https://lore.kernel.org/all/20240206104357.3803517-1-u-kumar1@ti.com/ > > Changes in v2 > - Updated commit message > - Simplified logic for valid clock id > link to v1 https://lore.kernel.org/all/20240205044557.3340848-1-u-kumar1@ti.com/ > > > P.S > Firmawre returns total num_parents count including non available ids. > For above device id NAVSS0_CPTS_0, number of parents clocks are 16 > i.e from id 2 to 17. But out of these ids few are not valid. > So driver adds only valid clock ids out ot total. > > Original logs > https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs > Line 2630 for error > > Logs with fix v3 > https://gist.github.com/uditkumarti/94e3e28d62282fd708dbfe37435ce1d9#file-v3 > Line 2586 > > > drivers/clk/keystone/sci-clk.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c > index 35fe197dd303..31b7df05d7bb 100644 > --- a/drivers/clk/keystone/sci-clk.c > +++ b/drivers/clk/keystone/sci-clk.c > @@ -516,6 +516,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > struct sci_clk *sci_clk, *prev; > int num_clks = 0; > int num_parents; > + bool state; > int clk_id; > const char * const clk_names[] = { > "clocks", "assigned-clocks", "assigned-clock-parents", NULL > @@ -583,16 +584,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > num_parents = 255; > } > > - clk_id = args.args[1] + 1; > + clk_id = args.args[1]; > > while (num_parents--) { > + /* Check if this clock id is valid */ > + ret = provider->ops->is_auto(provider->sci, > + sci_clk->dev_id, ++clk_id, &state);
A bit too nice coding ;) => I had been confused momentarily by clk_id = args.args[1] change just above till I saw that you are pre-incrementing clk_id - Is there a harm in leaving the original clk_id increment logic alone (it was much simpler to read up)?
> + > + if (ret) > + continue; > + > sci_clk = devm_kzalloc(dev, > sizeof(*sci_clk), > GFP_KERNEL); > if (!sci_clk) > return -ENOMEM; > sci_clk->dev_id = args.args[0]; > - sci_clk->clk_id = clk_id++; > + sci_clk->clk_id = clk_id; > sci_clk->provider = provider; > list_add_tail(&sci_clk->node, &clks); > > -- > 2.34.1 >
-- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
| |