Message ID | 1454966902-15228-1-git-send-email-joshua.henderson@microchip.com |
---|---|
State | Not Applicable, archived |
Headers | show |
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@900000 { 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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joshua, Quoting Joshua Henderson (2016-02-08 13:28:17) > +static const struct of_device_id pic32_clk_match[] __initconst = { > + { > + .compatible = "microchip,pic32mzda-refoclk", > + .data = of_refo_clk_setup, > + }, > + { > + .compatible = "microchip,pic32mzda-pbclk", > + .data = of_periph_clk_setup, > + }, > + { > + .compatible = "microchip,pic32mzda-syspll", > + .data = of_sys_pll_setup, > + }, > + { > + .compatible = "microchip,pic32mzda-sosc", > + .data = of_sosc_clk_setup, > + }, > + { > + .compatible = "microchip,pic32mzda-frcdivclk", > + .data = of_frcdiv_setup, > + }, > + { > + .compatible = "microchip,pic32mzda-sysclk-v2", > + .data = of_sys_mux_setup, > + }, > + {} > +}; As I mentioned in my review of the DT binding, instead of having a bunch of compatible strings for stuff that is inside of your SoC, you should have clock generator nodes in DT, and put the actual clock data here in your driver. You might only need one node, perhaps a second for the USBPLL. In your case this would result in static inits of struct pic32_spll, struct pic32_sclk, etc. I'm guessing you have a lot of derivative chips with slightly different clock trees? If so you should create a drivers/clk/pic32 directory. The code in this patch could be common code re-used by your derivatives and then you could store your clock data as a platform_driver in the same directory that inits the static data and uses these common clk_ops functions. Each chip derivative would have a new compatible string, instead of each clock node having a compatible string, which makes no sense. > +static void __init of_pic32_soc_clock_init(struct device_node *np) > +{ > + void (*clk_setup)(struct device_node *); > + const struct of_device_id *clk_id; > + struct device_node *childnp; > + > + pic32_clk_regbase = of_io_request_and_map(np, 0, of_node_full_name(np)); > + if (IS_ERR(pic32_clk_regbase)) > + panic("pic32-clk: failed to map registers\n"); > + > + for_each_child_of_node(np, childnp) { > + clk_id = of_match_node(pic32_clk_match, childnp); > + if (!clk_id) > + continue; > + clk_setup = clk_id->data; > + clk_setup(childnp); > + } > + > + /* register failsafe-clock-monitor NMI */ > + register_nmi_notifier(&failsafe_clk_notifier); > +} > + > +CLK_OF_DECLARE(pic32_soc_clk, "microchip,pic32mzda-clk", > + of_pic32_soc_clock_init); I hate CLK_OF_DECLARE. Sometimes we absolutely must live with it, but most of the time you can create a proper platform_driver that gets probed and registers its clocks from within the Linux Driver Model. Please try to make this into a platform_driver. You requested an example in V5, so please see: drivers/clk/qcom/gcc-apq8084.c Best regards, Mike -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/clock/microchip,pic32.txt b/Documentation/devicetree/bindings/clock/microchip,pic32.txt new file mode 100644 index 0000000..06540e4 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/microchip,pic32.txt @@ -0,0 +1,257 @@ +Binding for a Clock hardware block found on +certain Microchip PIC32 MCU devices. + +Microchip SoC clocks-node consists of few oscillators, PLL, multiplexer +and few divider nodes. + +We will find only the base address of the clock tree, this base +address is common for some of the subnodes, not all. If no address is +specified for any of subnode base address of the clock tree will be +treated as its base. Each of subnodes follow the same common clock +binding with some additional optional properties. + + clocks_node { + reg = <>; + + spll_node { + ... + }; + + frcdiv_node { + ... + }; + + sysclk_mux_node { + ... + }; + + pbdiv_node { + ... + }; + + refoclk_node { + ... + }; + ... + }; + +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". +- 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. +- microchip,slew-step: enable frequency slewing(stepping) during rate change; + applicable only to sys-clock subnode. + +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"; + }; +}; + +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>; +}