| Message ID | 20240207091100.4001428-1-u-kumar1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] clk: keystone: sci-clk: Adding support for non contiguous clocks | expand |
Udit Kumar <u-kumar1@ti.com> writes: > 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); > + > + 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); > Looks good to me. Reviewed-by: Kamlesh Gurudasani <kamlesh@ti.com> > -- > 2.34.1
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 >
Nishanth Menon <nm@ti.com> writes: > > 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)? > Personlly, I think this is simpler as this keeps everything related to parents inside while loop and increment only at one place. The other logic will have increment inside condition and also at 2 other places. Let's Udit take a call. Kamlesh
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@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; >> [..] /* 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 >> + >> + 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 >>
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@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; > > > [..] /* 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.
Nishanth Menon <nm@ti.com> writes: > 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@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; >> > > [..] /* 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. I wouldn't call it unnecessary, If I have to track increment/addition at 3 different places just to understand the loop, it is hard. On other hand, pre-increment code is solving the problem by having increment at only one place(easier to track). On the plus side, every clk_id belonging to parent is handled completely inside the loop. For a new person looking at this code, pre-increment code would be actually easier to undertsand. Also, Udit feels the same. Would you please explain why do you think the original increment code make more sense? It's not simple to understand or track, that's for sure. Kamlesh >\ > -- > Regards, > Nishanth Menon > Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
On 00:25-20240210, Kamlesh Gurudasani wrote: > >> > > > >> > > 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. > I wouldn't call it unnecessary, If I have to track increment/addition at > 3 different places just to understand the loop, it is hard. On other > hand, pre-increment code is solving the problem by having increment at > only one place(easier to track). On the plus side, every clk_id belonging to > parent is handled completely inside the loop. > > For a new person looking at this code, pre-increment code would be > actually easier to undertsand. > > Also, Udit feels the same. > > Would you please explain why do you think the original increment code > make more sense? It's not simple to understand or track, that's for sure. the context of the fix is the is_auto call to know what parent options are valid or not. Do the absolutely what is necessary in the change. if you want to beautify etc, move it to some other patch and debate about it. So, this is un-necessary change in this patch.
Nishanth Menon <nm@ti.com> writes: > On 00:25-20240210, Kamlesh Gurudasani wrote: >> >> > > >> >> > > 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. >> I wouldn't call it unnecessary, If I have to track increment/addition at >> 3 different places just to understand the loop, it is hard. On other >> hand, pre-increment code is solving the problem by having increment at >> only one place(easier to track). On the plus side, every clk_id belonging to >> parent is handled completely inside the loop. >> >> For a new person looking at this code, pre-increment code would be >> actually easier to undertsand. >> >> Also, Udit feels the same. >> >> Would you please explain why do you think the original increment code >> make more sense? It's not simple to understand or track, that's for sure. > > the context of the fix is the is_auto call to know what parent options > are valid or not. Do the absolutely what is necessary in the change. if > you want to beautify etc, move it to some other patch and debate about > it. So, this is un-necessary change in this patch. The context of the fix i.e. handling non contiguous parents is making the loop logic complex. Before this patch, i.e. contiguous clock handling was simple. In this fix, we are solving the problem as well as keeping the loop simple. clk_id is basically the part of the same loop and it's affecting nothing but the loop count. This is not just beautification, this is also simplifying the logic and improving the readibility. If the patch can provide the solution and avoid the complexity, then I don't understand why we need a patch that introduce the complexity and another patch to solve the complexity. My original question below is still not answered so I guess there is no debate here actually. "Would you please explain why do you think the original increment code make more sense? It's not simple to understand or track, that's for sure." Kamlesh > > -- > Regards, > Nishanth Menon > Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
On Wed, Feb 07, 2024 at 02:41:00PM +0530, 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> no empty lines in between tags and only tags at the end of the commit message, this [0] reference needs to be before or moved to a `Link:` tag, whatever works best for you. Francesco
On 2/11/2024 9:24 PM, Francesco Dolcini wrote: > On Wed, Feb 07, 2024 at 02:41:00PM +0530, 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> > no empty lines in between tags and only tags at the end of the commit > message, this [0] reference needs to be before or moved to a `Link:` tag, > whatever works best for you. Thanks will below works ? [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 Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically probing clocks") Udit
On Mon, Feb 12, 2024 at 10:06:30AM +0530, Kumar, Udit wrote: > > On 2/11/2024 9:24 PM, Francesco Dolcini wrote: > > On Wed, Feb 07, 2024 at 02:41:00PM +0530, Udit Kumar wrote: > > > 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> > > no empty lines in between tags and only tags at the end of the commit > > message, this [0] reference needs to be before or moved to a `Link:` tag, > > whatever works best for you. > will below works ? No, it does not fullfil the expectation to have only tags at the end. > [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 > Fixes: 3c13933c6033 ("clk: keystone: sci-clk: add support for dynamically > probing clocks")
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); + + 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);
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(-)