| Message ID | 20240206104357.3803517-1-u-kumar1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] clk: keystone: sci-clk: Adding support for non contiguous clocks | expand |
On 16:13-20240206, 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 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 v2 > https://gist.github.com/uditkumarti/94e3e28d62282fd708dbfe37435ce1d9 > Line 2591 > > > 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..ff249cbd54a1 100644 > --- a/drivers/clk/keystone/sci-clk.c > +++ b/drivers/clk/keystone/sci-clk.c > @@ -517,6 +517,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > int num_clks = 0; > int num_parents; > int clk_id; > + u64 freq; > const char * const clk_names[] = { > "clocks", "assigned-clocks", "assigned-clock-parents", NULL > }; > @@ -586,16 +587,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > clk_id = args.args[1] + 1; > > while (num_parents--) { > + /* Check if this clock id is valid */ > + ret = provider->ops->get_freq(provider->sci, > + sci_clk->dev_id, clk_id, &freq); get_freq is a bit expensive as it has to walk the clock tree to find the clock frequency (at least the first time?). just wondering if there is lighter alternative here? > + > + clk_id++; > + 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 - 1; > sci_clk->provider = provider; > list_add_tail(&sci_clk->node, &clks); > - Spurious change. > num_clks++; > } > } > -- > 2.34.1 >
Nishanth Menon <nm@ti.com> writes: > On 16:13-20240206, 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> >> while (num_parents--) { >> + /* Check if this clock id is valid */ >> + ret = provider->ops->get_freq(provider->sci, >> + sci_clk->dev_id, clk_id, &freq); > > get_freq is a bit expensive as it has to walk the clock tree to find > the clock frequency (at least the first time?). just wondering if > there is lighter alternative here? > How about get_clock? Doesn't read the registers at least. Regards, Kamlesh
On 2/6/2024 6:44 PM, Nishanth Menon wrote: > On 16:13-20240206, 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 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 v2 >> https://gist.github.com/uditkumarti/94e3e28d62282fd708dbfe37435ce1d9 >> Line 2591 >> >> >> 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..ff249cbd54a1 100644 >> --- a/drivers/clk/keystone/sci-clk.c >> +++ b/drivers/clk/keystone/sci-clk.c >> @@ -517,6 +517,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) >> int num_clks = 0; >> int num_parents; >> int clk_id; >> + u64 freq; >> const char * const clk_names[] = { >> "clocks", "assigned-clocks", "assigned-clock-parents", NULL >> }; >> @@ -586,16 +587,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) >> clk_id = args.args[1] + 1; >> >> while (num_parents--) { >> + /* Check if this clock id is valid */ >> + ret = provider->ops->get_freq(provider->sci, >> + sci_clk->dev_id, clk_id, &freq); > get_freq is a bit expensive as it has to walk the clock tree to find > the clock frequency (at least the first time?). just wondering if > there is lighter alternative here? Let me check , if we have some other alternative here >> + >> + clk_id++; >> + 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 - 1; >> sci_clk->provider = provider; >> list_add_tail(&sci_clk->node, &clks); >> - > Spurious change. I think, you meant by deleting the line ? If yes then will address in next version >> num_clks++; >> } >> } >> -- >> 2.34.1 >>
On 2/6/2024 7:24 PM, Kamlesh Gurudasani wrote: > Nishanth Menon <nm@ti.com> writes: > >> On 16:13-20240206, 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> >>> while (num_parents--) { >>> + /* Check if this clock id is valid */ >>> + ret = provider->ops->get_freq(provider->sci, >>> + sci_clk->dev_id, clk_id, &freq); >> get_freq is a bit expensive as it has to walk the clock tree to find >> the clock frequency (at least the first time?). just wondering if >> there is lighter alternative here? >> > How about get_clock? Doesn't read the registers at least. Said API needs, some flags to be passed, Can those flag be set to zero, Chandru ? > Regards, > Kamlesh
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> ... > @@ -586,16 +587,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) > clk_id = args.args[1] + 1; > > while (num_parents--) { > + /* Check if this clock id is valid */ > + ret = provider->ops->get_freq(provider->sci, > + sci_clk->dev_id, clk_id, &freq); > + > + clk_id++; Why increment it here and subtract if get_freq succeeds (sci_clk->clk_id = clk_id - 1;), rather if(ret) { clk_id++; continue; } > + 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 - 1; and keep sci_clk->clk_id = clk_id++; intact saves one subtraction or even better - clk_id = args.args[1] + 1; + clk_id = args.args[1]; while (num_parents--) { + /* Check if this clock id is valid */ + ret = provider->ops->get_freq(provider->sci, + sci_clk->dev_id, ++clk_id, &freq); and then no increments after, for clk_id Regards, Kamlesh
On 06/02/24 19:45, Kumar, Udit wrote: > > On 2/6/2024 7:24 PM, Kamlesh Gurudasani wrote: >> Nishanth Menon <nm@ti.com> writes: >> >>> On 16:13-20240206, 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> >>>> while (num_parents--) { >>>> + /* Check if this clock id is valid */ >>>> + ret = provider->ops->get_freq(provider->sci, >>>> + sci_clk->dev_id, clk_id, &freq); >>> get_freq is a bit expensive as it has to walk the clock tree to find >>> the clock frequency (at least the first time?). just wondering if >>> there is lighter alternative here? >>> >> How about get_clock? Doesn't read the registers at least. > > Said API needs, some flags to be passed, > > Can those flag be set to zero, Chandru ? get_clock doesn't require any flags to be passed. > > >> Regards, >> Kamlesh
On 2/6/2024 7:56 PM, CHANDRU DHAVAMANI wrote: > > On 06/02/24 19:45, Kumar, Udit wrote: >> >> On 2/6/2024 7:24 PM, Kamlesh Gurudasani wrote: >>> Nishanth Menon <nm@ti.com> writes: >>> >>>> On 16:13-20240206, 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> >>>>> while (num_parents--) { >>>>> + /* Check if this clock id is valid */ >>>>> + ret = provider->ops->get_freq(provider->sci, >>>>> + sci_clk->dev_id, clk_id, &freq); >>>> get_freq is a bit expensive as it has to walk the clock tree to find >>>> the clock frequency (at least the first time?). just wondering if >>>> there is lighter alternative here? >>>> >>> How about get_clock? Doesn't read the registers at least. >> >> Said API needs, some flags to be passed, >> >> Can those flag be set to zero, Chandru ? > > > get_clock doesn't require any flags to be passed. May be firmware does not need it but I was referring to https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 > > >> >> >>> Regards, >>> Kamlesh
"Kumar, Udit" <u-kumar1@ti.com> writes: >>>>> get_freq is a bit expensive as it has to walk the clock tree to find >>>>> the clock frequency (at least the first time?). just wondering if >>>>> there is lighter alternative here? >>>>> >>>> How about get_clock? Doesn't read the registers at least. >>> >>> Said API needs, some flags to be passed, >>> >>> Can those flag be set to zero, Chandru ? >> >> >> get_clock doesn't require any flags to be passed. > > > May be firmware does not need it but I was referring to > > https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 Just took a look, I now understand the reason for confusion, #define TI_SCI_MSG_SET_CLOCK_STATE 0x0100 #define TI_SCI_MSG_GET_CLOCK_STATE 0x0101 cops->get_clock = ti_sci_cmd_get_clock; --> refers to TI_SCI_MSG_SET_CLOCK_STATE That's why we are passing the flag from linux for get_clock Linux is using terminology of get/put. As Chandru pointed, we don't have to pass flags, cause he is refering to TI_SCI_MSG_GET_CLOCK_STATE Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what we actually want. cops->is_auto = ti_sci_cmd_clk_is_auto; cops->is_on = ti_sci_cmd_clk_is_on; cops->is_off = ti_sci_cmd_clk_is_off; Which should be safe to call, Chandru can confirm. Regards, Kamlesh > > > >> >> >>> >>> >>>> Regards, >>>> Kamlesh
On 2/6/2024 9:24 PM, Kamlesh Gurudasani wrote: > "Kumar, Udit" <u-kumar1@ti.com> writes: > >>>>>> get_freq is a bit expensive as it has to walk the clock tree to find >>>>>> the clock frequency (at least the first time?). just wondering if >>>>>> there is lighter alternative here? >>>>>> >>>>> How about get_clock? Doesn't read the registers at least. >>>> Said API needs, some flags to be passed, >>>> >>>> Can those flag be set to zero, Chandru ? >>> >>> get_clock doesn't require any flags to be passed. >> >> May be firmware does not need it but I was referring to >> >> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 > Just took a look, > > I now understand the reason for confusion, > > #define TI_SCI_MSG_SET_CLOCK_STATE 0x0100 > #define TI_SCI_MSG_GET_CLOCK_STATE 0x0101 > > cops->get_clock = ti_sci_cmd_get_clock; --> refers to > TI_SCI_MSG_SET_CLOCK_STATE > That's why we are passing the flag from linux for get_clock > > Linux is using terminology of get/put. > > As Chandru pointed, we don't have to pass flags, cause he is refering > to TI_SCI_MSG_GET_CLOCK_STATE > > Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what > we actually want. > cops->is_auto = ti_sci_cmd_clk_is_auto; > cops->is_on = ti_sci_cmd_clk_is_on; > cops->is_off = ti_sci_cmd_clk_is_off; I think calling ti_sci_cmd_clk_is_auto should be good . other functions needs current state and requested state. Chandru ? > > Which should be safe to call, Chandru can confirm. > > Regards, > Kamlesh >> >> >>> >>>> >>>>> Regards, >>>>> Kamlesh
On 07/02/24 11:03, Kumar, Udit wrote: > > On 2/6/2024 9:24 PM, Kamlesh Gurudasani wrote: >> "Kumar, Udit" <u-kumar1@ti.com> writes: >> >>>>>>> get_freq is a bit expensive as it has to walk the clock tree to >>>>>>> find >>>>>>> the clock frequency (at least the first time?). just wondering if >>>>>>> there is lighter alternative here? >>>>>>> >>>>>> How about get_clock? Doesn't read the registers at least. >>>>> Said API needs, some flags to be passed, >>>>> >>>>> Can those flag be set to zero, Chandru ? >>>> >>>> get_clock doesn't require any flags to be passed. >>> >>> May be firmware does not need it but I was referring to >>> >>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 >>> >> Just took a look, >> >> I now understand the reason for confusion, >> >> #define TI_SCI_MSG_SET_CLOCK_STATE 0x0100 >> #define TI_SCI_MSG_GET_CLOCK_STATE 0x0101 >> >> cops->get_clock = ti_sci_cmd_get_clock; --> refers to >> TI_SCI_MSG_SET_CLOCK_STATE >> That's why we are passing the flag from linux for get_clock >> >> Linux is using terminology of get/put. >> >> As Chandru pointed, we don't have to pass flags, cause he is refering >> to TI_SCI_MSG_GET_CLOCK_STATE >> >> Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what >> we actually want. >> cops->is_auto = ti_sci_cmd_clk_is_auto; >> cops->is_on = ti_sci_cmd_clk_is_on; >> cops->is_off = ti_sci_cmd_clk_is_off; > > > I think calling ti_sci_cmd_clk_is_auto should be good . other > functions needs current state and requested state. > > Chandru ? > ti_sci_cmd_clk_is_auto is internal function to linux. For TI_SCI_MSG_GET_CLOCK_STATE, linux only needs to pass pointers to the variables where result will be stored. >> >> Which should be safe to call, Chandru can confirm. >> >> Regards, >> Kamlesh >>> >>> >>>> >>>>> >>>>>> Regards, >>>>>> Kamlesh
On 2/7/2024 12:53 PM, CHANDRU DHAVAMANI wrote: > > On 07/02/24 11:03, Kumar, Udit wrote: >> >> On 2/6/2024 9:24 PM, Kamlesh Gurudasani wrote: >>> "Kumar, Udit" <u-kumar1@ti.com> writes: >>> >>>>>>>> get_freq is a bit expensive as it has to walk the clock tree to >>>>>>>> find >>>>>>>> the clock frequency (at least the first time?). just wondering if >>>>>>>> there is lighter alternative here? >>>>>>>> >>>>>>> How about get_clock? Doesn't read the registers at least. >>>>>> Said API needs, some flags to be passed, >>>>>> >>>>>> Can those flag be set to zero, Chandru ? >>>>> >>>>> get_clock doesn't require any flags to be passed. >>>> >>>> May be firmware does not need it but I was referring to >>>> >>>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 >>>> >>> Just took a look, >>> >>> I now understand the reason for confusion, >>> >>> #define TI_SCI_MSG_SET_CLOCK_STATE 0x0100 >>> #define TI_SCI_MSG_GET_CLOCK_STATE 0x0101 >>> >>> cops->get_clock = ti_sci_cmd_get_clock; --> refers to >>> TI_SCI_MSG_SET_CLOCK_STATE >>> That's why we are passing the flag from linux for get_clock >>> >>> Linux is using terminology of get/put. >>> >>> As Chandru pointed, we don't have to pass flags, cause he is refering >>> to TI_SCI_MSG_GET_CLOCK_STATE >>> >>> Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what >>> we actually want. >>> cops->is_auto = ti_sci_cmd_clk_is_auto; >>> cops->is_on = ti_sci_cmd_clk_is_on; >>> cops->is_off = ti_sci_cmd_clk_is_off; >> >> >> I think calling ti_sci_cmd_clk_is_auto should be good . other >> functions needs current state and requested state. >> >> Chandru ? >> > > ti_sci_cmd_clk_is_auto is internal function to linux. > For TI_SCI_MSG_GET_CLOCK_STATE, linux only needs to pass pointers to > the variables where result will be stored. Yes this internal function calls TI_SCI_MSG_GET_CLOCK_STATE > > >>> >>> Which should be safe to call, Chandru can confirm. >>> >>> Regards, >>> Kamlesh >>>> >>>> >>>>> >>>>>> >>>>>>> Regards, >>>>>>> Kamlesh
On 07/02/24 13:03, Kumar, Udit wrote: > > On 2/7/2024 12:53 PM, CHANDRU DHAVAMANI wrote: >> >> On 07/02/24 11:03, Kumar, Udit wrote: >>> >>> On 2/6/2024 9:24 PM, Kamlesh Gurudasani wrote: >>>> "Kumar, Udit" <u-kumar1@ti.com> writes: >>>> >>>>>>>>> get_freq is a bit expensive as it has to walk the clock tree >>>>>>>>> to find >>>>>>>>> the clock frequency (at least the first time?). just wondering if >>>>>>>>> there is lighter alternative here? >>>>>>>>> >>>>>>>> How about get_clock? Doesn't read the registers at least. >>>>>>> Said API needs, some flags to be passed, >>>>>>> >>>>>>> Can those flag be set to zero, Chandru ? >>>>>> >>>>>> get_clock doesn't require any flags to be passed. >>>>> >>>>> May be firmware does not need it but I was referring to >>>>> >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/clk/keystone/sci-clk.c#L78 >>>>> >>>> Just took a look, >>>> >>>> I now understand the reason for confusion, >>>> >>>> #define TI_SCI_MSG_SET_CLOCK_STATE 0x0100 >>>> #define TI_SCI_MSG_GET_CLOCK_STATE 0x0101 >>>> >>>> cops->get_clock = ti_sci_cmd_get_clock; --> refers to >>>> TI_SCI_MSG_SET_CLOCK_STATE >>>> That's why we are passing the flag from linux for get_clock >>>> >>>> Linux is using terminology of get/put. >>>> >>>> As Chandru pointed, we don't have to pass flags, cause he is refering >>>> to TI_SCI_MSG_GET_CLOCK_STATE >>>> >>>> Below functions passes TI_SCI_MSG_GET_CLOCK_STATE to DM, which is what >>>> we actually want. >>>> cops->is_auto = ti_sci_cmd_clk_is_auto; >>>> cops->is_on = ti_sci_cmd_clk_is_on; >>>> cops->is_off = ti_sci_cmd_clk_is_off; >>> >>> >>> I think calling ti_sci_cmd_clk_is_auto should be good . other >>> functions needs current state and requested state. >>> >>> Chandru ? >>> >> >> ti_sci_cmd_clk_is_auto is internal function to linux. >> For TI_SCI_MSG_GET_CLOCK_STATE, linux only needs to pass pointers to >> the variables where result will be stored. > > > Yes this internal function calls TI_SCI_MSG_GET_CLOCK_STATE > Okay. We can use TI_SCI_MSG_GET_CLOCK_STATE to check to if clock is valid or not. > >> >> >>>> >>>> Which should be safe to call, Chandru can confirm. >>>> >>>> Regards, >>>> Kamlesh >>>>> >>>>> >>>>>> >>>>>>> >>>>>>>> Regards, >>>>>>>> Kamlesh
diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c index 35fe197dd303..ff249cbd54a1 100644 --- a/drivers/clk/keystone/sci-clk.c +++ b/drivers/clk/keystone/sci-clk.c @@ -517,6 +517,7 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) int num_clks = 0; int num_parents; int clk_id; + u64 freq; const char * const clk_names[] = { "clocks", "assigned-clocks", "assigned-clock-parents", NULL }; @@ -586,16 +587,23 @@ static int ti_sci_scan_clocks_from_dt(struct sci_clk_provider *provider) clk_id = args.args[1] + 1; while (num_parents--) { + /* Check if this clock id is valid */ + ret = provider->ops->get_freq(provider->sci, + sci_clk->dev_id, clk_id, &freq); + + clk_id++; + 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 - 1; sci_clk->provider = provider; 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 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 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 v2 https://gist.github.com/uditkumarti/94e3e28d62282fd708dbfe37435ce1d9 Line 2591 drivers/clk/keystone/sci-clk.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)