| Message ID | 20240205044557.3340848-1-u-kumar1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] clk: keystone: sci-clk: Adding support for non contiguous clocks | expand |
On 10:15-20240205, 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 assigns > accordingly. > > New firmware started returning error in case of > non-available clock id. Therefore drivers throws error while > re-calculate and other functions. What changed here? started returning error for what API? also please fix up 70 char alignment -> there extra spaces in your commit message. > > In this fix, before assigning and adding clock in list, > driver checks if given clock is valid or not. > > 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 and 18-19 not present > > Signed-off-by: Udit Kumar <u-kumar1@ti.com> > --- > Original logs > https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs > Line 2630 for error > > Logs with fix > https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-with-fix > Line 2594 > > drivers/clk/keystone/sci-clk.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c > index 35fe197dd303..d417ec018d82 100644 > --- a/drivers/clk/keystone/sci-clk.c > +++ b/drivers/clk/keystone/sci-clk.c > @@ -517,6 +517,8 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > int num_clks = 0; > int num_parents; > int clk_id; > + int max_clk_id; > + u64 freq; > const char * const clk_names[] = { > "clocks", "assigned-clocks", "assigned-clock-parents", NULL > }; > @@ -584,6 +586,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > } > > clk_id = args.args[1] + 1; > + max_clk_id = clk_id + num_parents; > > while (num_parents--) { > sci_clk = devm_kzalloc(dev, > @@ -592,11 +595,20 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > if (!sci_clk) > return -ENOMEM; > sci_clk->dev_id = args.args[0]; > - sci_clk->clk_id = clk_id++; > - sci_clk->provider = provider; > - list_add_tail(&sci_clk->node, &clks); > + /* check if given clock id is valid by calling get_freq */ > + /* loop over max possible ids */ > + do { > + sci_clk->clk_id = clk_id++; > > - num_clks++; > + ret = provider->ops->get_freq(provider->sci, > + sci_clk->dev_id, sci_clk->clk_id, &freq); > + } while (ret != 0 && clk_id < max_clk_id); take clock ids 0 1 2 3 -> Say 2 is reserved. num_parents = 4 while(num_parents) Loop 1 -> clk ID 0 is valid, list_add_tail while(num_parents) Loop 2 -> clk ID 1 is valid, list_add_tail while(num_parents) Loop 3 -> clk ID 2 is invalid.. so we scan forward to clk ID 3 -> list_add_tail while(num_parents) Loop 4 -> clk ID 4 is invalid.. but 5 is out of range, so we break off loop. sci_clk is still devm_kzalloced -> but since clk_id > max_clk_id, we jump off loop, and we dont add it to tail. so one extra allocation? If we have multiple reserved intermediate ones, then we'd have as many allocations that aren't linked? Could we not improve the logic a bit to allocate just what is necessary? > + > + sci_clk->provider = provider; > + if (ret == 0) { > + list_add_tail(&sci_clk->node, &clks); > + num_clks++; > + } > } > } > > -- > 2.34.1 >
On 2/5/2024 7:34 PM, Nishanth Menon wrote: > On 10:15-20240205, 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 assigns >> accordingly. >> >> New firmware started returning error in case of >> non-available clock id. Therefore drivers throws error while >> re-calculate and other functions. > What changed here? started returning error for what API? also please fix > up 70 char alignment -> there extra spaces in your commit message. will address in v2 >> In this fix, before assigning and adding clock in list, >> driver checks if given clock is valid or not. >> >> 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 and 18-19 not present >> >> Signed-off-by: Udit Kumar <u-kumar1@ti.com> >> --- >> Original logs >> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs >> Line 2630 for error >> >> Logs with fix >> https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-with-fix >> Line 2594 >> >> drivers/clk/keystone/sci-clk.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c >> index 35fe197dd303..d417ec018d82 100644 >> --- a/drivers/clk/keystone/sci-clk.c >> +++ b/drivers/clk/keystone/sci-clk.c >> @@ -517,6 +517,8 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) >> int num_clks = 0; >> int num_parents; >> [..] >> - num_clks++; >> + ret = provider->ops->get_freq(provider->sci, >> + sci_clk->dev_id, sci_clk->clk_id, &freq); >> + } while (ret != 0 && clk_id < max_clk_id); > take clock ids 0 1 2 3 -> Say 2 is reserved. > num_parents = 4 > while(num_parents) Loop 1 -> clk ID 0 is valid, list_add_tail > while(num_parents) Loop 2 -> clk ID 1 is valid, list_add_tail > while(num_parents) Loop 3 -> clk ID 2 is invalid.. so we scan forward > to clk ID 3 -> list_add_tail > while(num_parents) Loop 4 -> clk ID 4 is invalid.. but 5 is out of > range, so we break off loop. sci_clk is still devm_kzalloced -> > but since clk_id > max_clk_id, we jump off loop, and we dont add > it to tail. so one extra allocation? Thanks for catching this. > If we have multiple reserved intermediate ones, then we'd have as many > allocations that aren't linked? Could we not improve the logic a bit to > allocate just what is necessary? Sure, will change in v2. to check clock validity first and then allocate, add >> + >> + sci_clk->provider = provider; >> + if (ret == 0) { >> + list_add_tail(&sci_clk->node, &clks); >> + num_clks++; >> + } >> } >> } >> >> -- >> 2.34.1 >>
diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c index 35fe197dd303..d417ec018d82 100644 --- a/drivers/clk/keystone/sci-clk.c +++ b/drivers/clk/keystone/sci-clk.c @@ -517,6 +517,8 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) int num_clks = 0; int num_parents; int clk_id; + int max_clk_id; + u64 freq; const char * const clk_names[] = { "clocks", "assigned-clocks", "assigned-clock-parents", NULL }; @@ -584,6 +586,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) } clk_id = args.args[1] + 1; + max_clk_id = clk_id + num_parents; while (num_parents--) { sci_clk = devm_kzalloc(dev, @@ -592,11 +595,20 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) if (!sci_clk) return -ENOMEM; sci_clk->dev_id = args.args[0]; - sci_clk->clk_id = clk_id++; - sci_clk->provider = provider; - list_add_tail(&sci_clk->node, &clks); + /* check if given clock id is valid by calling get_freq */ + /* loop over max possible ids */ + do { + sci_clk->clk_id = clk_id++; - num_clks++; + ret = provider->ops->get_freq(provider->sci, + sci_clk->dev_id, sci_clk->clk_id, &freq); + } while (ret != 0 && clk_id < max_clk_id); + + sci_clk->provider = provider; + if (ret == 0) { + list_add_tail(&sci_clk->node, &clks); + num_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 assigns accordingly. New firmware started returning error in case of non-available clock id. Therefore drivers throws error while re-calculate and other functions. In this fix, before assigning and adding clock in list, driver checks if given clock is valid or not. 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 and 18-19 not present Signed-off-by: Udit Kumar <u-kumar1@ti.com> --- Original logs https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-original-logs Line 2630 for error Logs with fix https://gist.github.com/uditkumarti/de4b36b21247fb36725ad909ce4812f6#file-with-fix Line 2594 drivers/clk/keystone/sci-clk.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)