[U-Boot] [PATCH v2 07/22] clock-uclass: allow disabling a peripheral clock

[Stephen Warren]

On 07/29/2016 11:26 AM, Benjamin Tietz wrote:
> Hello Stephen,
>
> On Fri, Jul 29, 2016 at 10:04:56AM -0600, Stephen Warren wrote:
>> On 07/28/2016 01:28 PM, Benjamin Tietz wrote:
>>> Hello Vikas, hello Simon,
>>>
>>> the new clk-API leaves me with a problem. Previously there was a
>>> seperate way to access the clock-device itself (using clk_[gs]et_rate) and
>>> the peripherals connected (clk_[gs]et_periph_rate). The former case now isn't
>>> available no more. But the system clock in STM32 doesn't have a separate ID.
>>> According to the device-tree the kernel doesn't care about that - or
>>> uses special logic.
>>
>> I don't understand the issue here. The device tree model is that every clock
>> has some provider (a node/phandle) and some ID (a sequence of integer
>> values). There's no such thing as "the clock-device itself".
>
> For the current STM32 DT exactly that is the problem. The phandle is
> available and the set of IDs is defined. There is - at least at the
> moment - no ID defined for the system clock itself, but only for the
> derived clocks for the individual peripherals.
>
> The existing peripherals IDs even start to count at 0, so the "intuitive"
> solution to use that ID, doesn't work either.
>
>>
>> The kernel is consistent with that model; each client executes clk_get(),
>> which for a DT-based system parses the phandle+clock_specifier and returns a
>> clock handle, and then the client just uses that handle. That's exactly how
>> U-Boot works too.
>
> I must admit, that I haven't had an in-depth look on the STM32 clocking
> kernel sources yet. From other architectures I've seen, the system clock
> is either accessed by a certain ID, based on the underlying hardware -
> which isn't available on STM32 - or assumed to be "just there". On a
> first glance, the kernel STM32 driver seems to fall into the second category.
>
>>
>> Perhaps you can show the portion of DT that's causing the issue?
>
> It is the not existing potion of the DT ;)
>
>>
>> Is the issue that there are clocks that your code needs to use that haven't
>> yet been assigned a clock ID (clock specifier value) for use in DT (i.e. you
>> have SoC-specific code that's calling clk_get() and the mapping isn't via
>> DT)? If so, you simply need to assign one and everything will work fine.
>> High numbers work fine for this.
>>
>>> Possible solutions:
>>>
>>> a) Using a magic ID. Unfortunately, the peripheral used in the current
>>> device-tree are 0-based (and 0 is already in use), so this number isn't
>>> available. Moving the IDs around would break compatibility to the linux
>>> kernel.
>>>
>>> Negative numbers look like errors.
>>>
>>> Using a special high number looks unintuitive. And often result in
>>> additional work-arounds in the future.
>>
>> What specific issues are you thinking of? I haven't experienced any when
>> assigning IDs on Tegra, and I haven't observed anyone having issues doing
>> this on any platform within the Linux kernel, where the exact same thing
>> would be required.
>
> I'm no friend of magic numbers. Experience had shown, that - especially
> those introduced as a work-around - generate complicate problems at a
> later point of time.

Well, the numbers aren't magic. For DT to work in any way at all, the 
numbering used by DT properties must match the numbering implemented by 
the clock driver code. The only way to do that is to use a shared header 
files that defines what those IDs are. That header is included by both 
the DT compiler and the C compiler, so there's no chance of them getting 
out of sync. In the U-Boot source code, look at 
include/dt-bindings/clock/tegra20-car.h for example.

> The first thing I can think of is to run into a hardware, having more
> peripheral clocks than expected. Nevertheless, in a follow-up mail I
> made a suggestion interpreting part of the ID as a bit-field - with
> enough room for bigger hardware.

Every piece of different HW requires a different DT binding (or at least 
a different compatible value; compatible values that represent similar 
HW can share a binding definition file). Each compatible value defines a 
separate set of clock IDs, and hence there's potentially a separate 
header file for each HW (although of course where different HW just 
happens to use the same set of IDs, they can use the same header file to 
define them). As such, it's easy to assign different "virtual" IDs for 
each different HW. For example, there's a separate clock ID header for 
Tegra20, Tegra30, etc. That's because all of (a) there are more clocks 
on Tegra30 thus requiring some additional IDs to be defined for it (b) 
Tegra30 probably moved some clock IDs around in HW thus requiring some 
clock IDs to be changed (c) virtual IDs had to be changed to leave space 
for the new clocks.

Note that the clock IDs defined by the DT binding should match HW where 
it makes sense. For example on Tegra20, there are 96 clocks whose IDs 
correspond exactly to bit positions in a nice regular set of HW 
registers. However there are many clocks (mainly SoC clock inputs, PLLs, 
etc.) that aren't covered by that main register set, and hence have IDs 
assigned that are entirely arbitrary.

With DT, if you have any clocks that don't have an obvious HW-defined 
ID, you have no choice but to assign them some arbitrary/chosen ID, and 
have a mapping table that converts DT IDs into information about those 
clocks; their HW identity, their type (PLL, peripheral, ...), register 
address, ...