linux-kernel - Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <522110AA.7040700@wwwdotorg.org>
Date:	Fri, 30 Aug 2013 15:37:46 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Mike Turquette <mturquette@...aro.org>
CC:	Kumar Gala <galak@...eaurora.org>, devicetree@...r.kernel.org,
	Heiko Stübner <heiko@...ech.de>,
	Stephen Boyd <sboyd@...eaurora.org>,
	linux-kernel@...r.kernel.org, Tero Kristo <t-kristo@...com>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Matt Sealey <neko@...uhatsu.net>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock

On 08/30/2013 02:33 PM, Mike Turquette wrote:
> Quoting Kumar Gala (2013-08-30 13:02:42)
>> On Aug 28, 2013, at 8:14 PM, Mike Turquette wrote:
>>
>>> Quoting Kumar Gala (2013-08-28 08:50:10)
>>>> On Aug 22, 2013, at 12:53 AM, Mike Turquette wrote:
>>>>
>>>>> Device Tree binding for the basic clock multiplexer, plus the setup
>>>>> function to register the clock.  Based on the existing fixed-clock
>>>>> binding.
>>>>>
>>>>> Includes minor beautification of clk-provider.h where some whitespace is
>>>>> added and of_fixed_factor_clock_setup is relocated to maintain a
>>>>> consistent style.
>>>>>
>>>>> Tero Kristo contributed helpful bug fixes to this patch.
>>>>>
>>>>> Signed-off-by: Mike Turquette <mturquette@...aro.org>
>>>>> Tested-by: Heiko Stuebner <heiko@...ech.de>
>>>>> Reviewed-by: Heiko Stuebner <heiko@...ech.de>
>>>>> ---
>>>>> Changes since v3:
>>>>> * replaced underscores with dashes in DT property names
>>>>> * bail from of clock setup function early if of_iomap fails
>>>>> * removed unecessary explict cast
>>>>>
>>>>> .../devicetree/bindings/clock/mux-clock.txt        | 79 ++++++++++++++++++++++
>>>>> drivers/clk/clk-mux.c                              | 68 ++++++++++++++++++-
>>>>> include/linux/clk-provider.h                       |  5 +-
>>>>> 3 files changed, 150 insertions(+), 2 deletions(-)
>>>>> create mode 100644 Documentation/devicetree/bindings/clock/mux-clock.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/mux-clock.txt b/Documentation/devicetree/bindings/clock/mux-clock.txt
>>>>> new file mode 100644
>>>>> index 0000000..21eb3b3
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/mux-clock.txt
>>>>> @@ -0,0 +1,79 @@
>>>>> +Binding for simple mux clock.
>>>>> +
>>>>> +This binding uses the common clock binding[1].  It assumes a
>>>>> +register-mapped multiplexer with multiple input clock signals or
>>>>> +parents, one of which can be selected as output.  This clock does not
>>>>> +gate or adjust the parent rate via a divider or multiplier.
>>>>> +
>>>>> +By default the "clocks" property lists the parents in the same order
>>>>> +as they are programmed into the regster.  E.g:
>>>>> +
>>>>> +     clocks = <&foo_clock>, <&bar_clock>, <&baz_clock>;
>>>>> +
>>>>> +results in programming the register as follows:
>>>>> +
>>>>> +register value               selected parent clock
>>>>> +0                    foo_clock
>>>>> +1                    bar_clock
>>>>> +2                    baz_clock
>>>>> +
>>>>> +Some clock controller IPs do not allow a value of zero to be programmed
>>>>> +into the register, instead indexing begins at 1.  The optional property
>>>>> +"index-starts-at-one" modified the scheme as follows:
>>>>> +
>>>>> +register value               selected clock parent
>>>>> +1                    foo_clock
>>>>> +2                    bar_clock
>>>>> +3                    baz_clock
>>>>> +
>>>>> +Additionally an optional table of bit and parent pairs may be supplied
>>>>> +like so:
>>>>> +
>>>>> +     table = <&foo_clock 0x0>, <&bar_clock, 0x2>, <&baz_clock, 0x4>;
>>>>> +
>>>>
>>>> I think this is going way too far for what should be in a device tree.  Why should this not just be encoded in tables in the code?
>>>
>>> To reduce kernel data bloat.
>>
>> Why is this really an issue, for systems that are concerned about kernel size, they are going to be built targeting the HW platform of choice, for one's like a server, the bloat isn't going to be significant, and if so we can look at mechanisms like __init section that would free up after boot.
> 
> s/bloat/churn/
> 
> The clock _data_ seems to always have some churn to it. Moving it out to
> DT reduces that churn from Linux. My concern above is not about kernel
> data size.

That sounds like the opposite of what we should be doing.

It's fine for kernel code/data to change; that's a natural part of
development. Obviously, we should minimize churn, through thorough
review, domain knowledge, etc.

Saying that we must shove the data out into DT because it changes
pre-supposes that we should be changing the DT!

We should strive to write the DT for a particular piece of HW once, and
have it be a complete and accurate description of the HW. That's hard
enough to do with small amounts of simple data. Deliberately pushing
data that's known to be hard to get right and churn a lot into DT seems
to be destined to make most DTs contain incorrect data.

DT-as-an-ABI works both ways; the binding definition needs to stay
constant /and/ the data in any particular DT needs to be accurate too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/