Re: [PATCH v3] clk: keystone: sci-clk: Adding support for non contiguous clocks

[Nishanth Menon]

On 19:53-20240207, Kumar, Udit wrote:
> Hi Nishanth,
> 
> On 2/7/2024 6:24 PM, Nishanth Menon wrote:
> > 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@xxxxxx>
> > > ---
> > > 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@xxxxxx/
> > > 
> > > 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@xxxxxx/
> > > 
> > > 
> > > 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;
> > > [..]					/* 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)?
> 
> No warm in using original code but want to avoid, two statement for
> increment in case of failure and success.
> 
> Let me know, if i need to add few comments around this
> 
> or if you think, code is confusing I can move to original one

Yes, please drop the un-necessary changes. In this case, original
increment code should work just fine.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D