| Message ID | 1327521469-28863-1-git-send-email-swarren@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Stephen, On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren@nvidia.com> wrote: > Document a binding for the Tegra20 CAR (Clock And Reset) Controller, > add it to tegra20.dtsi, and configure it for the board in tegra- > seaboard.dts. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > v2: > * Remove clock-names, clock-output-names properties; Tegra will solely > use integer IDs for clocks in DT. > * Fixed typo in compatible flag > * Resolve FIXME re: multiple clocks with the same "reset ID"; give each > unique clock an ID, and ignore the reset bits, since this is purely a > clock binding. Code (e.g. U-Boot) that wants to use this to determine > CAR reset bit numbers would need a table to convert from the clock IDs > in this binding to the related reset bit number, if any. In general, I > think that's true, and the U-Boot code that handles "peripheral IDs" > should be reworked to handle "clocks", the peripheral clocks being a > subset of all clocks. The clock enable and reset enable bits use the same numbering. I think you have invented a third which is an arbitrary number which doesn't correspond to anything in hardware. That makes sense for pin mux function perhaps, since the indirection provides a useful concept of function number, but here I can't see the benefit. > * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs, > etc. > * Separate tegra-seaboard.dts usage example into a separate patch. > > This patch semantically relies on Grant Likely's common clock binding patch > series. Once that's finalized, this patch could be checked into the kernel > provided there are no relevant changes to Grant's patches. I believe that > Simon Glass wants to start using this within U-Boot ASAP though. As I may have said I am really not keen on the idea of having a table just to use it in U-Boot. > --- > .../bindings/clock/nvidia,tegra20-car.txt | 164 ++++++++++++++++++++ > arch/arm/boot/dts/tegra20.dtsi | 6 + > 2 files changed, 170 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt > > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt > new file mode 100644 > index 0000000..acce2d9 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt > @@ -0,0 +1,164 @@ > +NVIDIA Tegra20 Clock And Reset Controller > + > +This binding uses the common clock binding: > +Documentation/devicetree/bindings/clock/clock-bindings.txt > + > +The CAR (Clock And Reset) Controller on Tegra is the HW module responsible > +for muxing and gating Tegra's clocks, and setting their rates. > + > +Required properties : > +- compatible : Should be "nvidia,<chip>-car" > +- reg : Should contain CAR registers location and length > +- clocks : Should contain phandle and clock specifiers for two clocks: > + the 32 KHz "32k_in", and the board-specific oscillator "osc". > +- #clock-cells : Should be 1. > + In clock consumers, this cell represents the clock ID exposed by the CAR. > + > + The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB > + registers. Later, subsequent IDs will be assigned to all other clocks the > + CAR controls; mainly the PLLs. Are you sure? The ordering doesn't seem to fit with U-Boot's enum periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file follows along with the hardware. > + > + 0 osc > + 1 clk_32k a/k/a clk_s > + 2 clk_m > + 3 sclk > + 4 cclk > + 5 hclk > + 6 pclk > + 7 blink > + 8 pll_a > + 9 pll_a_out0 > + 10 pll_c > + 11 pll_c_out1 > + 12 pll_d > + 13 pll_e > + 14 pll_m > + 15 pll_m_out1 > + 16 pll_p > + 17 pll_p_out1 > + 18 pll_p_out2 > + 19 pll_p_out3 > + 20 pll_p_out4 > + 21 pll_s > + 22 pll_u > + 23 pll_x > + 24 audio a/k/a audio_sync_clk > + 25 audio_2x a/k/a audio_2x_sync_clk > + 26 cpu > + 27 cop a/k/a avp > + 28 ac97 > + 29 rtc > + 30 tmr > + 31 uart1 > + 32 uart2 > + 33 gpio > + 34 sdmmc2 > + 35 spdif_out > + 36 spdif_in > + 37 i2s1 > + 38 i2c1 > + 39 ndflash > + 40 sdmmc1 > + 41 sdmmc4 > + 42 twc > + 43 pwm > + 44 i2s2 > + 45 epp > + 46 vi > + 47 vi_sensor > + 48 2d > + 49 usbd > + 50 isp > + 51 3d > + 52 ide > + 53 disp2 > + 54 disp1 > + 55 host1x > + 56 vcp > + 57 cache2 > + 58 mem > + 59 ahbdma > + 60 apbdma > + 61 kbc > + 62 stat_mon > + 63 pmc > + 64 fuse > + 65 kfuse > + 66 sbc1 > + 67 snor > + 68 spi > + 69 sbc2 > + 70 xio > + 71 sbc3 > + 72 dvc > + 73 dsi > + 74 cve > + 75 tvo > + 76 mipi > + 77 hdmi > + 78 csi > + 79 tvdac > + 80 i2c2 > + 81 uart3 > + 82 emc > + 83 usb2 > + 84 usb3 > + 85 mpe > + 86 vde > + 87 bsea > + 88 bsev > + 89 speedo > + 90 uart4 > + 91 uart5 > + 92 i2c3 > + 93 sbc4 > + 94 sdmmc3 > + 95 pcie > + 96 owr > + 97 afi > + 98 csite > + 99 avpucq > + 100 la > + 101 irama > + 102 iramb > + 103 iramc > + 104 iramd > + 105 cram2 > + 106 clk_d > + 107 sus > + 108 cdev1 > + 109 cdev2 > + > +Example: > + > +clocks { > + #address-cells = <1>; > + #size-cells = <0>; > + > + clk_32k: clock@0 { > + compatible = "fixed-clock"; > + reg = <0>; > + #clock-cells = <0>; > + clock-frequency = <32768>; > + }; > + > + osc: clock@1 { > + compatible = "fixed-clock"; > + reg = <1>; > + #clock-cells = <0>; > + clock-frequency = <12000000>; > + }; > +}; > + > +tegra_car: clock@60006000 { > + compatible = "nvidia,tegra20-car"; > + reg = <0x60006000 1000>; > + clocks = <&clk_32k> <&osc>; > + clock-names = "32k_in", "osc"; > + #clock-cells = <1>; > +}; > + > +usb@c5004000 { > + ... > + clocks = <&tegra_car 58>; /* usb2 */ > +}; > diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi > index 3da7afd..8208660 100644 > --- a/arch/arm/boot/dts/tegra20.dtsi > +++ b/arch/arm/boot/dts/tegra20.dtsi > @@ -4,6 +4,12 @@ > compatible = "nvidia,tegra20"; > interrupt-parent = <&intc>; > > + tegra_car: clock@60006000 { > + compatible = "nvidia,tegra20-car"; > + reg = <0x60006000 1000>; > + #clock-cells = <1>; > + }; > + > intc: interrupt-controller@50041000 { > compatible = "arm,cortex-a9-gic"; > interrupt-controller; > -- > 1.7.0.4 > Regards, Simon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Simon Glass wrote at Wednesday, January 25, 2012 1:14 PM: > On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren@nvidia.com> wrote: > > Document a binding for the Tegra20 CAR (Clock And Reset) Controller, > > add it to tegra20.dtsi, and configure it for the board in tegra- > > seaboard.dts. ... > > v2: ... > > * Resolve FIXME re: multiple clocks with the same "reset ID"; give each > > unique clock an ID, and ignore the reset bits, since this is purely a > > clock binding. Code (e.g. U-Boot) that wants to use this to determine > > CAR reset bit numbers would need a table to convert from the clock IDs > > in this binding to the related reset bit number, if any. In general, I > > think that's true, and the U-Boot code that handles "peripheral IDs" > > should be reworked to handle "clocks", the peripheral clocks being a > > subset of all clocks. > > The clock enable and reset enable bits use the same numbering. I think > you have invented a third which is an arbitrary number which doesn't > correspond to anything in hardware. That makes sense for pin mux > function perhaps, since the indirection provides a useful concept of > function number, but here I can't see the benefit. I didn't do it because there was specific benefit, but simply because we have no choice. There are clocks that don't have reset bits or clock enable bits. There are some clocks that are really different clocks (different mux selection or divider control registers) yet share the same bit for reset and clock enable. Therefore it simply isn't true that there's a 1:1 mapping between clocks and clock-enable/reset bits. I deliberately made this updated binding have a different numbering scheme to the clock-enable/reset bits to make this clear, so that no one would accidentally confuse the two concepts. > > * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs, > > etc. > > * Separate tegra-seaboard.dts usage example into a separate patch. > > > > This patch semantically relies on Grant Likely's common clock binding patch > > series. Once that's finalized, this patch could be checked into the kernel > > provided there are no relevant changes to Grant's patches. I believe that > > Simon Glass wants to start using this within U-Boot ASAP though. > > As I may have said I am really not keen on the idea of having a table > just to use it in U-Boot. I don't see any alternative given the way the HW is designed. We could ignore the way the HW works and assume that clock ID == clock- enable/reset bits is true for many clocks. However, it's not true for all, so I think that'd be too error-prone. Equally, I know that you will need a table sometime in U-Boot to map from clock ID to clock-enable/reset bit and many other per-clock parameters if you're going to be initializing stuff in U-Boot from DT rather than hard-coding it, so I think you may as well just add it now. > > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt ... > > + The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB > > + registers. Later, subsequent IDs will be assigned to all other clocks the > > + CAR controls; mainly the PLLs. > > Are you sure? The ordering doesn't seem to fit with U-Boot's enum > periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file > follows along with the hardware. No, that paragraph is wrong. I simply forgot to remove it.
Hi Stephen, On Wed, Jan 25, 2012 at 12:31 PM, Stephen Warren <swarren@nvidia.com> wrote: > Simon Glass wrote at Wednesday, January 25, 2012 1:14 PM: >> On Wed, Jan 25, 2012 at 11:57 AM, Stephen Warren <swarren@nvidia.com> wrote: >> > Document a binding for the Tegra20 CAR (Clock And Reset) Controller, >> > add it to tegra20.dtsi, and configure it for the board in tegra- >> > seaboard.dts. > ... >> > v2: > ... >> > * Resolve FIXME re: multiple clocks with the same "reset ID"; give each >> > unique clock an ID, and ignore the reset bits, since this is purely a >> > clock binding. Code (e.g. U-Boot) that wants to use this to determine >> > CAR reset bit numbers would need a table to convert from the clock IDs >> > in this binding to the related reset bit number, if any. In general, I >> > think that's true, and the U-Boot code that handles "peripheral IDs" >> > should be reworked to handle "clocks", the peripheral clocks being a >> > subset of all clocks. >> >> The clock enable and reset enable bits use the same numbering. I think >> you have invented a third which is an arbitrary number which doesn't >> correspond to anything in hardware. That makes sense for pin mux >> function perhaps, since the indirection provides a useful concept of >> function number, but here I can't see the benefit. > > I didn't do it because there was specific benefit, but simply because > we have no choice. I was quite happy with your original proposal which made them line up where they could, and used other numbers where they couldn't. > > There are clocks that don't have reset bits or clock enable bits. > > There are some clocks that are really different clocks (different mux > selection or divider control registers) yet share the same bit for reset > and clock enable. > > Therefore it simply isn't true that there's a 1:1 mapping between clocks > and clock-enable/reset bits. > > I deliberately made this updated binding have a different numbering > scheme to the clock-enable/reset bits to make this clear, so that no one > would accidentally confuse the two concepts. I think it makes sense to line them up where you can (all but two cases out of about 60 I think). > >> > * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs, >> > etc. >> > * Separate tegra-seaboard.dts usage example into a separate patch. >> > >> > This patch semantically relies on Grant Likely's common clock binding patch >> > series. Once that's finalized, this patch could be checked into the kernel >> > provided there are no relevant changes to Grant's patches. I believe that >> > Simon Glass wants to start using this within U-Boot ASAP though. >> >> As I may have said I am really not keen on the idea of having a table >> just to use it in U-Boot. > > I don't see any alternative given the way the HW is designed. You had the alternative yourself the first time around. There clearly is an alternative. > > We could ignore the way the HW works and assume that clock ID == clock- > enable/reset bits is true for many clocks. However, it's not true for > all, so I think that'd be too error-prone. > > Equally, I know that you will need a table sometime in U-Boot to map > from clock ID to clock-enable/reset bit and many other per-clock > parameters if you're going to be initializing stuff in U-Boot from DT > rather than hard-coding it, so I think you may as well just add it now. I am happy that there is now a concept of a peripheral number and we don't have to refer to everything with strings. I don't mind if for hardware reasons this peripheral number doesn't always correspond to everything we point it at (clock, reset, pinmux, clock source). But I am uncomfortable with it corresponding to nothing because it might be 'error-prone'. This seems to be introducing a layer of indirection which is not needed at all in U-Boot, for example. I prefer a clock number which corresponds to the clock enable/reset bit positions in all cases it can (all but two) and a different number where it can't. At least this saves one table. Alternatively perhaps these bit numbers should be specified in the device tree also? I was rather hoping that the device tree would take us away from having lots of tables in the C code. > >> > diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt > ... >> > + The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB >> > + registers. Later, subsequent IDs will be assigned to all other clocks the >> > + CAR controls; mainly the PLLs. >> >> Are you sure? The ordering doesn't seem to fit with U-Boot's enum >> periph_id in arch/arm/include/asm/arch-tegra2/clock.h. That file >> follows along with the hardware. > > No, that paragraph is wrong. I simply forgot to remove it. Well I vote for a return :-) > > -- > nvpublic > Regards, Simon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" 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/nvidia,tegra20-car.txt b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt new file mode 100644 index 0000000..acce2d9 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt @@ -0,0 +1,164 @@ +NVIDIA Tegra20 Clock And Reset Controller + +This binding uses the common clock binding: +Documentation/devicetree/bindings/clock/clock-bindings.txt + +The CAR (Clock And Reset) Controller on Tegra is the HW module responsible +for muxing and gating Tegra's clocks, and setting their rates. + +Required properties : +- compatible : Should be "nvidia,<chip>-car" +- reg : Should contain CAR registers location and length +- clocks : Should contain phandle and clock specifiers for two clocks: + the 32 KHz "32k_in", and the board-specific oscillator "osc". +- #clock-cells : Should be 1. + In clock consumers, this cell represents the clock ID exposed by the CAR. + + The first 96 clocks are numbered to match the bits in the CAR's CLK_OUT_ENB + registers. Later, subsequent IDs will be assigned to all other clocks the + CAR controls; mainly the PLLs. + + 0 osc + 1 clk_32k a/k/a clk_s + 2 clk_m + 3 sclk + 4 cclk + 5 hclk + 6 pclk + 7 blink + 8 pll_a + 9 pll_a_out0 + 10 pll_c + 11 pll_c_out1 + 12 pll_d + 13 pll_e + 14 pll_m + 15 pll_m_out1 + 16 pll_p + 17 pll_p_out1 + 18 pll_p_out2 + 19 pll_p_out3 + 20 pll_p_out4 + 21 pll_s + 22 pll_u + 23 pll_x + 24 audio a/k/a audio_sync_clk + 25 audio_2x a/k/a audio_2x_sync_clk + 26 cpu + 27 cop a/k/a avp + 28 ac97 + 29 rtc + 30 tmr + 31 uart1 + 32 uart2 + 33 gpio + 34 sdmmc2 + 35 spdif_out + 36 spdif_in + 37 i2s1 + 38 i2c1 + 39 ndflash + 40 sdmmc1 + 41 sdmmc4 + 42 twc + 43 pwm + 44 i2s2 + 45 epp + 46 vi + 47 vi_sensor + 48 2d + 49 usbd + 50 isp + 51 3d + 52 ide + 53 disp2 + 54 disp1 + 55 host1x + 56 vcp + 57 cache2 + 58 mem + 59 ahbdma + 60 apbdma + 61 kbc + 62 stat_mon + 63 pmc + 64 fuse + 65 kfuse + 66 sbc1 + 67 snor + 68 spi + 69 sbc2 + 70 xio + 71 sbc3 + 72 dvc + 73 dsi + 74 cve + 75 tvo + 76 mipi + 77 hdmi + 78 csi + 79 tvdac + 80 i2c2 + 81 uart3 + 82 emc + 83 usb2 + 84 usb3 + 85 mpe + 86 vde + 87 bsea + 88 bsev + 89 speedo + 90 uart4 + 91 uart5 + 92 i2c3 + 93 sbc4 + 94 sdmmc3 + 95 pcie + 96 owr + 97 afi + 98 csite + 99 avpucq + 100 la + 101 irama + 102 iramb + 103 iramc + 104 iramd + 105 cram2 + 106 clk_d + 107 sus + 108 cdev1 + 109 cdev2 + +Example: + +clocks { + #address-cells = <1>; + #size-cells = <0>; + + clk_32k: clock@0 { + compatible = "fixed-clock"; + reg = <0>; + #clock-cells = <0>; + clock-frequency = <32768>; + }; + + osc: clock@1 { + compatible = "fixed-clock"; + reg = <1>; + #clock-cells = <0>; + clock-frequency = <12000000>; + }; +}; + +tegra_car: clock@60006000 { + compatible = "nvidia,tegra20-car"; + reg = <0x60006000 1000>; + clocks = <&clk_32k> <&osc>; + clock-names = "32k_in", "osc"; + #clock-cells = <1>; +}; + +usb@c5004000 { + ... + clocks = <&tegra_car 58>; /* usb2 */ +}; diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi index 3da7afd..8208660 100644 --- a/arch/arm/boot/dts/tegra20.dtsi +++ b/arch/arm/boot/dts/tegra20.dtsi @@ -4,6 +4,12 @@ compatible = "nvidia,tegra20"; interrupt-parent = <&intc>; + tegra_car: clock@60006000 { + compatible = "nvidia,tegra20-car"; + reg = <0x60006000 1000>; + #clock-cells = <1>; + }; + intc: interrupt-controller@50041000 { compatible = "arm,cortex-a9-gic"; interrupt-controller;
Document a binding for the Tegra20 CAR (Clock And Reset) Controller, add it to tegra20.dtsi, and configure it for the board in tegra- seaboard.dts. Signed-off-by: Stephen Warren <swarren@nvidia.com> --- v2: * Remove clock-names, clock-output-names properties; Tegra will solely use integer IDs for clocks in DT. * Fixed typo in compatible flag * Resolve FIXME re: multiple clocks with the same "reset ID"; give each unique clock an ID, and ignore the reset bits, since this is purely a clock binding. Code (e.g. U-Boot) that wants to use this to determine CAR reset bit numbers would need a table to convert from the clock IDs in this binding to the related reset bit number, if any. In general, I think that's true, and the U-Boot code that handles "peripheral IDs" should be reworked to handle "clocks", the peripheral clocks being a subset of all clocks. * Define clock IDs for all the non-peripheral clocks too; inputs, PLLs, etc. * Separate tegra-seaboard.dts usage example into a separate patch. This patch semantically relies on Grant Likely's common clock binding patch series. Once that's finalized, this patch could be checked into the kernel provided there are no relevant changes to Grant's patches. I believe that Simon Glass wants to start using this within U-Boot ASAP though. --- .../bindings/clock/nvidia,tegra20-car.txt | 164 ++++++++++++++++++++ arch/arm/boot/dts/tegra20.dtsi | 6 + 2 files changed, 170 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/nvidia,tegra20-car.txt