Hi Jon, On 24.11.2020 11:36, Jon Hunter wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 13/11/2020 15:21, Claudiu Beznea wrote: >> There are regulators who's min selector is not zero. Selectors loops >> (looping b/w zero and regulator::desc::n_voltages) might throw errors >> because invalid selectors are used (lower than >> regulator::desc::linear_min_sel). For this situations validate selectors >> against regulator::desc::linear_min_sel. > > > After this commit was merged, I noticed a regression in the DFLL (CPU > clock source) on Tegra124. The DFLL driver > (drivers/clk/tegra/clk-dfll.c) calls regulator_list_voltage() in a loop > to determine the selector for a given voltage (see function > find_vdd_map_entry_exact()). > > Currently, the DFLL driver queries the number of voltages provided by > the regulator by calling regulator_count_voltages() and then starting > from 0, iterates through the number of voltages to find the selector > value for the voltage it is looking for by calling > regulator_list_voltage(). It assumes that any negative value returned by > calling regulator_list_voltage() is an error and this will cause the > loop up to terminate. > > In this case the regulator in question is the as3722 and the > linear_min_sel for this regulator is 1 and so when the DFLL driver calls > regulator_list_voltage() with a selector value of 0 it now returns a > negative error code, as expected by this change, and this terminates the > loop up in the DFLL driver. So I can clearly see why this is happening > and I could fix up the DFLL driver to avoid this. > > Before doing so, I wanted to ask if that is the correct fix here, > because it seems a bit odd that regulator_count_voltages() returns N > voltages, but if the min selector value is greater than 0, then actually > there are less than N. However, changing the number of voltages > supported by the regulator to be N - linear_min_sel does not make sense > either because then we need to know the linear_min_sel in order to > determine the first valid voltage. > > In case of the as3722, the value 0 means that the regulator is powered > down. So it is a valid setting and equates to 0 volts at the output AFAICT. > > Please let me know your thoughts are the correct way to fix this up. I would say that a solution would be to have a new helper to retrieve the linear_min_sel (e.g. regulator_min_sel()) and use this for all the consumers of regulator_list_voltage() and the other APIs that patch "regulator: core: validate selector against linear_min_sel" has changed (regulator_list_voltage_table(), regulator_set_voltage_time()). With this change the loop in find_vdd_map_entry_exact() should be b/w regulator_min_sel() and regulator_count_voltages(). Maybe Mark has a better solution for this. Thank you, Claudiu > > Thanks > Jon > > -- > nvpublic >
