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

[Simon Glass]

Hi Benjamin,

On 29 July 2016 at 12:34, Benjamin Tietz <uboot at dresden.micronet24.de> wrote:
> Hello Stephen,
>
> On Fri, Jul 29, 2016 at 12:02:02PM -0600, Stephen Warren wrote:
>> On 07/29/2016 11:26 AM, Benjamin Tietz wrote:
>> >Hello Stephen,
>
> [snip]
>
>> >>>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.
>
> Just because they are defined in a single place doesn't make that
> numbers less magic. They are choosen just for a specific software
> component. And U-Boot isn't the only user of device-trees. See: Linux
> Kernel.
>
> Syncing files of magic values between different projects is a pain and
> should be reduced to a bare minimum. As part of that, choosing those
> values should be avoided. If they are choosen once, modification isn't
> really possible anymore. If one must be choosen, this should be done
> carefully.

Note though that the numbers are part of the device-tree binding for
an SoC. They are actually fixed in value. So the shared header file
(between C code and DT) is pretty-much essential. This is the way it
works at present.

>
>>
>> >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, ...
>
> The device-tree was introduced as an additional abstraction layer
> between the hardware in use and the software running on this hardware,
> esp. the bootloader and the OS kernel. One goal was to have a single
> firmware running on different boards, based on the device-tree flashed
> on that board. This should improve the ways to reuse code written once.
>
> If the hardware changes there must be some way to represent this change
> in software - either by having a new driver ( involving dubled code ),
> or by a compatibility layer ( adding additional complexity ) or
> sometimes by a reasonable decision on the design or some simple IDs in
> the beginning. For the stm32-clk driver the currently implemented logic
> does need some kind of decision. Please don't interprete this as
> criticism on your work, but as I am, as everyone else, not perfect
> and there is no obvious solution I wrote down my complaints, asking for
> some comment or hint.
>
> To discuss a solution and not a problem, I would prefer to take my mail
> from this morning at 07:22 as base.
> In any case, have a nice weekend.

As to your problem, can you not have separate clock drivers? For the
ones that don't have multiple clocks, they can ignore the id. For the
others, they can use the ID from the device tree.

Regards,
Simon