| 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
| ||
|
Message-ID: <20160209004513.12298.5063@quark.deferred.io>
Date: Mon, 08 Feb 2016 16:45:13 -0800
From: Michael Turquette <mturquette@...libre.com>
To: Joshua Henderson <joshua.henderson@...rochip.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: "Purna Chandra Mandal" <purna.mandal@...rochip.com>,
"Joshua Henderson" <joshua.henderson@...rochip.com>,
"Ralf Baechle" <ralf@...ux-mips.org>,
"Stephen Boyd" <sboyd@...eaurora.org>,
"Rob Herring" <robh+dt@...nel.org>,
"Pawel Moll" <pawel.moll@....com>,
"Mark Rutland" <mark.rutland@....com>,
"Ian Campbell" <ijc+devicetree@...lion.org.uk>,
"Kumar Gala" <galak@...eaurora.org>, linux-clk@...r.kernel.org
Subject: Re: [PATCH v6 1/2] dt/bindings: Add PIC32 clock binding documentation
Hi Joshua,
Quoting Joshua Henderson (2016-02-08 13:28:16)
> + clocks_node {
> + reg = <>;
> +
> + spll_node {
> + ...
> + };
> +
> + frcdiv_node {
> + ...
> + };
> +
> + sysclk_mux_node {
> + ...
> + };
> +
> + pbdiv_node {
> + ...
> + };
> +
> + refoclk_node {
> + ...
> + };
Why the _node suffix on everything?
> + ...
> + };
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : should be one of "microchip,pic32mzda-clk",
> + "microchip,pic32mzda-sosc", "microchip,pic32mzda-frcdivclk",
> + "microchip,pic32mzda-syspll", "microchip,pic32mzda-sysclk-v2",
> + "microchip,pic32mzda-pbclk", "microchip,pic32mzda-refoclk".
This seems like a lot of compatible strings. I took a very quick look at
the pic32mz reference manual and it seems like all of those clock nodes
are on-chip, correct? The only thing that would be off-chip would the
crystal that supplies Posc (OSC1 and OSC2) as well as the (mandatory?)
32k clock for Sosc (SOSCI and SOSCO).
Since most of those clocks are on-chip, can't you just group them all
into a single clock generator node? E.g. everything that's in the same
address range probably belongs in one node, the USB PLL should be a
fixed-rate clock stuffed inside of the USB host controller DT node, etc.
More to the point this DT binding description + driver is clearly
following the Device Tree Data Driven™ way of doing things, which isn't
necessarily a good thing. Putting hardware details into drivers (in the
form of static struct initialization) is a good thing.
> +- reg : A Base address and length of the register set.
> +- interrupts : source of interrupt.
> +
> +Optional properties (for subnodes):
> +- #clock-cells: From common clock binding, should be 0.
> +- microchip,clock-indices: in multiplexer node clock sources always aren't linear
> + and contiguous. This property helps define clock-sources with respect to
> + the mux clock node.
> +- microchip,ignore-unused : ignore gate request even if the gated clock is unused.
> +- microchip,status-bit-mask: bitmask for status check. This will be used to confirm
> + particular operation by clock sub-node is completed. It is dependent sub-node.
> +- microchip,bit-mask: enable mask, similar to microchip,status-bit-mask.
Generally it is bad to put register-level details into DT. Static clock
data is where this level detail belongs.
> +- microchip,slew-step: enable frequency slewing(stepping) during rate change;
> + applicable only to sys-clock subnode.
Does this change from board to board or from chip-to-chip? Should it
belong in DT or in the driver?
> +
> +Example:
> +
> +/* PIC32 specific clks */
> +pic32_clktree {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x1f801200 0x200>;
> + compatible = "microchip,pic32mzda-clk";
> + ranges = <0 0x1f801200 0x200>;
> +
> + /* secondary oscillator; external input on SOSCI pin */
> + SOSC:sosc_clk@0 {
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-sosc";
> + clock-frequency = <32768>;
> + reg = <0x000 0x10>, /* enable reg */
> + <0x1d0 0x10>; /* status reg */
> + microchip,bit-mask = <0x02>; /* enable mask */
> + microchip,status-bit-mask = <0x10>; /* status-mask*/
> + };
> +
> + FRCDIV:frcdiv_clk {
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-frcdivclk";
> + clocks = <&FRC>;
> + clock-output-names = "frcdiv_clk";
> + };
> +
> + /* System PLL clock */
> + SYSPLL:spll_clk@20 {
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-syspll";
> + reg = <0x020 0x10>, /* SPLL register */
> + <0x1d0 0x10>; /* CLKSTAT register */
> + clocks = <&POSC>, <&FRC>;
> + clock-output-names = "sys_pll";
> + microchip,status-bit-mask = <0x80>; /* SPLLRDY */
> + };
> +
> + /* system clock; mux with postdiv & slew */
> + SYSCLK:sys_clk@1c0 {
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-sysclk-v2";
> + reg = <0x1c0 0x04>; /* SLEWCON */
> + clocks = <&FRCDIV>, <&SYSPLL>, <&POSC>, <&SOSC>,
> + <&LPRC>, <&FRCDIV>;
> + microchip,clock-indices = <0>, <1>, <2>, <4>,
> + <5>, <7>;
> + clock-output-names = "sys_clk";
> + };
> +
> + /* UPLL is integral part of USB PHY; UTMI clk for USBCORE */
> + UPLL:usb_phy_clk {
> + #clock-cells = <0>;
> + compatible = "fixed-clocks";
> + clock-frequency = <24000000>;
> + clock-output-names = "usbphy_clk";
> + };
> +
> + /* Peripheral bus1 clock */
> + PBCLK1:pb1_clk@140 {
> + reg = <0x140 0x10>;
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-pbclk";
> + clocks = <&SYSCLK>;
> + clock-output-names = "pb1_clk";
> + /* used by system modules, not gateable */
> + microchip,ignore-unused;
> + };
> +
> + /* Peripheral bus2 clock */
> + PBCLK2:pb2_clk@150 {
> + reg = <0x150 0x10>;
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-pbclk";
> + clocks = <&SYSCLK>;
> + clock-output-names = "pb2_clk";
> + /* avoid gating even if unused */
> + microchip,ignore-unused;
> + };
> +
> + /* Peripheral bus3 clock */
> + PBCLK3:pb3_clk@160 {
> + reg = <0x160 0x10>;
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-pbclk";
> + clocks = <&SYSCLK>;
> + clock-output-names = "pb3_clk";
> + };
> +
> + /* Peripheral bus4 clock(I/O ports, GPIO) */
> + PBCLK4:pb4_clk@170 {
> + reg = <0x170 0x10>;
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-pbclk";
> + clocks = <&SYSCLK>;
> + clock-output-names = "pb4_clk";
> + };
> +
> + /* Peripheral bus clock */
> + PBCLK5:pb5_clk@180 {
> + reg = <0x180 0x10>;
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-pbclk";
> + clocks = <&SYSCLK>;
> + clock-output-names = "pb5_clk";
> + };
> +
> + /* Peripheral Bus6 clock; */
> + PBCLK6:pb6_clk@190 {
> + reg = <0x190 0x10>;
> + compatible = "microchip,pic32mzda-pbclk";
> + clocks = <&SYSCLK>;
> + #clock-cells = <0>;
> + };
> +
> + /* Peripheral bus7 clock */
> + PBCLK7:pb7_clk@1a0 {
> + reg = <0x1a0 0x10>;
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-pbclk";
> + /* CPU is driven by this clock; so named */
> + clock-output-names = "cpu_clk";
> + clocks = <&SYSCLK>;
> + };
> +
> + /* Reference Oscillator clock for SPI/I2S */
> + REFCLKO1:refo1_clk@80 {
> + reg = <0x080 0x20>;
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-refoclk";
> + clocks = <&SYSCLK>, <&PBCLK1>, <&POSC>, <&FRC>, <&LPRC>,
> + <&SOSC>, <&SYSPLL>, <&REFIx>, <&BFRC>;
> + microchip,clock-indices = <0>, <1>, <2>, <3>, <4>,
> + <5>, <7>, <8>, <9>;
> + clock-output-names = "refo1_clk";
> + };
> +
> + /* Reference Oscillator clock for SQI */
> + REFCLKO2:refo2_clk@a0 {
> + reg = <0x0a0 0x20>;
> + #clock-cells = <0>;
> + compatible = "microchip,pic32mzda-refoclk";
> + clocks = <&SYSCLK>, <&PBCLK1>, <&POSC>, <&FRC>, <&LPRC>,
> + <&SOSC>, <&SYSPLL>, <&REFIx>, <&BFRC>;
> + microchip,clock-indices = <0>, <1>, <2>, <3>, <4>,
> + <5>, <7>, <8>, <9>;
> + clock-output-names = "refo2_clk";
> + };
> +
> + /* Reference Oscillator clock, ADC */
> + REFCLKO3:refo3_clk@c0 {
> + reg = <0x0c0 0x20>;
> + compatible = "microchip,pic32mzda-refoclk";
> + clocks = <&SYSCLK>, <&PBCLK1>, <&POSC>, <&FRC>, <&LPRC>,
> + <&SOSC>, <&SYSPLL>, <&REFIx>, <&BFRC>;
> + microchip,clock-indices = <0>, <1>, <2>, <3>, <4>,
> + <5>, <7>, <8>, <9>;
> + #clock-cells = <0>;
> + clock-output-names = "refo3_clk";
> + };
> +
> + /* Reference Oscillator clock */
> + REFCLKO4:refo4_clk@e0 {
> + reg = <0x0e0 0x20>;
> + compatible = "microchip,pic32mzda-refoclk";
> + clocks = <&SYSCLK>, <&PBCLK1>, <&POSC>, <&FRC>, <&LPRC>,
> + <&SOSC>, <&SYSPLL>, <&REFIx>, <&BFRC>;
> + microchip,clock-indices = <0>,<1>,<2>,<3>,<4>,
> + <5>,<7>,<8>,<9>;
> + #clock-cells = <0>;
> + clock-output-names = "refo4_clk";
> + };
> +
> + /* Reference Oscillator clock, LCD */
> + REFCLKO5:refo5_clk@100 {
> + reg = <0x100 0x20>;
> + compatible = "microchip,pic32mzda-refoclk";
> + clocks = <&SYSCLK>,<&PBCLK1>,<&POSC>,<&FRC>,<&LPRC>,
> + <&SOSC>,<&SYSPLL>,<&REFIx>,<&BFRC>;
> + microchip,clock-indices = <0>, <1>, <2>, <3>, <4>,
> + <5>, <7>, <8>,<9>;
> + #clock-cells = <0>;
> + clock-output-names = "refo5_clk";
> + };
Essentially all of the above information can live in the driver as
static data. Then you can create a single clock generator node like you
see in the qcom binding:
An example from Documentation/devicetree/bindings/clock/qcom,gcc.txt and
arch/arm/boot/dts/qcom-apq8064.dtsi:
gcc: clock-controller@...000 {
compatible = "qcom,gcc-apq8064";
reg = <0x00900000 0x4000>;
#clock-cells = <1>;
#reset-cells = <1>;
};
If you have off-chip fixed-rate or fixed-factor clocks (such as XTALs or
the USBPLL in your USB IP block) then go ahead and put those in Device
Tree. Board specific details belong in DT (and we make exception for
simple fixed-rate clocks like your USBPLL one).
Regards,
Mike
> +};
> +
> +The clock consumer should specify the desired clock by having the clocks in its
> +"clock" phandle cell. For example for UART:
> +
> +uart2: serial@<> {
> + compatible = "microchip,pic32mzda-uart";
> + reg = <>;
> + interrupts = <>;
> + clocks = <&PBCLK2>;
> +}
> --
> 1.7.9.5
>
Powered by blists - more mailing lists