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

[Benjamin Tietz]

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.

> 
> >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.

regards
Benjamin