On 17/10/2018 15:43, Dmitry Osipenko wrote: > On 10/17/18 5:14 PM, Jon Hunter wrote: >> >> On 17/10/2018 14:46, Dmitry Osipenko wrote: >>> On 10/17/18 4:34 PM, Jon Hunter wrote: >>>> >>>> On 17/10/2018 14:07, Dmitry Osipenko wrote: >>>>> On 10/17/18 3:59 PM, Jon Hunter wrote: >>>>>> >>>>>> On 17/10/2018 13:37, Dmitry Osipenko wrote: >>>>>>> On 10/17/18 11:40 AM, Jon Hunter wrote: >>>>>>>> >>>>>>>> On 30/08/2018 20:43, Dmitry Osipenko wrote: >>>>>>>>> Add device-tree binding that describes CPU frequency-scaling hardware >>>>>>>>> found on NVIDIA Tegra20/30 SoC's. >>>>>>>>> >>>>>>>>> Signed-off-by: Dmitry Osipenko <[email protected]> >>>>>>>>> --- >>>>>>>>> .../cpufreq/nvidia,tegra20-cpufreq.txt | 38 >>>>>>>>> +++++++++++++++++++ >>>>>>>>> 1 file changed, 38 insertions(+) >>>>>>>>> create mode 100644 >>>>>>>>> Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt >>>>>>>>> >>>>>>>>> diff --git >>>>>>>>> a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt >>>>>>>>> >>>>>>>>> b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000000..2c51f676e958 >>>>>>>>> --- /dev/null >>>>>>>>> +++ >>>>>>>>> b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt >>>>>>>>> @@ -0,0 +1,38 @@ >>>>>>>>> +Binding for NVIDIA Tegra20 CPUFreq >>>>>>>>> +================================== >>>>>>>>> + >>>>>>>>> +Required properties: >>>>>>>>> +- clocks: Must contain an entry for each entry in clock-names. >>>>>>>>> + See ../clocks/clock-bindings.txt for details. >>>>>>>>> +- clock-names: Must include the following entries: >>>>>>>>> + - pll_x: main-parent for CPU clock, must be the first entry >>>>>>>>> + - backup: intermediate-parent for CPU clock >>>>>>>>> + - cpu: the CPU clock >>>>>>>> >>>>>>>> Is it likely that 'backup' will be anything other that pll_p? If not >>>>>>>> why >>>>>>>> not just call it pll_p? Personally, I don't 'backup' to descriptive >>>>>>>> even >>>>>>>> though I can see what you mean. >>>>>>>> >>>>>>>> I can see that you want to make this flexible, but if the likelihood is >>>>>>>> that we will just use pll_p then I am not sure it is warranted at this >>>>>>>> point. >>>>>>> >>>>>>> That won't describe HW, but software. And device tree should describe >>>>>>> HW. >>>>>> >>>>>> Hmm ... well that's my point exactly. So why call it 'backup'? Sounds >>>>>> like a software description to me. >>>>> >>>>> Because HW is designed the way that CPU parent need to be switched to the >>>>> backup clock source while main clock changes its rate. HW also allow to >>>>> select among different parents, pll_p is one of those parents. >>>> >>>> Yes that part is understood. I am just splitting hairs over the actual >>>> name. We do the same for tegra124 but we just call it 'pll_p'. See ... >>>> >>>> Documentation/devicetree/bindings/cpufreq/nvidia,tegra124-cpufreq.txt >>> >>> tegra124-cpufreq choose to hardwire to the pll_p, but it could be other >>> clocks. Technically abstracting backup clock should be more correct, but >>> result is the same in the end. >> >> Yes and that is probably intentional as that is the configuration that >> has been validated. So unless we are planning to test and verify every >> possibility, my preference is to keep it simple. But yes the result is >> the same. >> >> I was simply curious of your intention here because of the other series >> you posted with regard to the cpu clocks. > > Actually I tried other parents on T30 and they worked with no problems. The > intention of having 'backup' is to describe HW properly in the device tree, > that's it. We'll have backup set to pll_p by default. ACK?
Yes but I don't find the name 'backup' very descriptive because like you say it is an intermediate clock for switching frequency. But at the same time I don't have a good suggestion. Anyway it is more of a bike-shedding comment. Cheers Jon -- nvpublic
