| Message ID | 1428669271-11032-1-git-send-email-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/10/15 05:34, Srinivas Kandagatla wrote: > @@ -250,6 +265,18 @@ > }; > }; > > + ext_3p3v: regulator-fixed@1 { > + compatible = "regulator-fixed"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + regulator-name = "ext_3p3v"; > + regulator-type = "voltage"; > + startup-delay-us = <0>; > + gpio = <&tlmm_pinmux 77 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + regulator-boot-on; > + }; This shouldn't be inside the SoC node because it doesn't have a reg property. It should be in a 'regulators' node that's in the root of the tree: regulators { compatible = "simple-bus"; ext_3p3v: fixedregulator@0 { compatible = "regulator-fixed"; ... }; }; > + > qcom,ssbi@500000 { > compatible = "qcom,ssbi"; > reg = <0x00500000 0x1000>; > @@ -522,5 +549,82 @@ > compatible = "qcom,tcsr-apq8064", "syscon"; > reg = <0x1a400000 0x100>; > }; > + > + hdmi: qcom,hdmi-tx@4a00000 { > + compatible = "qcom,hdmi-tx-8960"; > + reg-names = "core_physical"; > + reg = <0x04a00000 0x1000>; > + interrupts = <GIC_SPI 79 IRQ_TYPE_NONE>; > + clock-names = > + "core_clk", > + "master_iface_clk", > + "slave_iface_clk"; > + clocks = > + <&mmcc HDMI_APP_CLK>, > + <&mmcc HDMI_M_AHB_CLK>, > + <&mmcc HDMI_S_AHB_CLK>; > + qcom,hdmi-tx-ddc-clk = <&tlmm_pinmux 70 > + GPIO_ACTIVE_HIGH>; > + qcom,hdmi-tx-ddc-data = <&tlmm_pinmux 71 > + GPIO_ACTIVE_HIGH>; > + qcom,hdmi-tx-hpd = <&tlmm_pinmux 72 > + GPIO_ACTIVE_HIGH>; This should be done via the *-gpios method. i.e. hdmi-tx-ddc-clk-gpios, hdmi-tx-ddc-data-gpios, etc. > + core-vdda-supply = <&pm8921_hdmi_switch>; > + hdmi-mux-supply = <&ext_3p3v>; > + pinctrl-names = "default"; > + pinctrl-0 = <&hdmi_pinctrl>; > + }; > + > + gpu: qcom,adreno-3xx@4300000 { > + compatible = "qcom,adreno-3xx"; > + reg = <0x04300000 0x20000>; > + reg-names = "kgsl_3d0_reg_memory"; > + interrupts = <GIC_SPI 80 IRQ_TYPE_NONE>; > + interrupt-names = "kgsl_3d0_irq"; > + clock-names = > + "core_clk", > + "iface_clk", > + "mem_clk", > + "mem_iface_clk"; > + clocks = > + <&mmcc GFX3D_CLK>, > + <&mmcc GFX3D_AHB_CLK>, > + <&mmcc GFX3D_AXI_CLK>, > + <&mmcc MMSS_IMEM_AHB_CLK>; > + qcom,chipid = <0x03020002>; > + qcom,gpu-pwrlevels { > + compatible = "qcom,gpu-pwrlevels"; > + qcom,gpu-pwrlevel@0 { > + qcom,gpu-freq = <450000000>; > + }; > + qcom,gpu-pwrlevel@1 { > + qcom,gpu-freq = <27000000>; > + }; > + }; This should be an OPP.
On 10/04/15 18:04, Stephen Boyd wrote: > On 04/10/15 05:34, Srinivas Kandagatla wrote: >> @@ -250,6 +265,18 @@ >> }; >> }; >> >> + ext_3p3v: regulator-fixed@1 { >> + compatible = "regulator-fixed"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + regulator-name = "ext_3p3v"; >> + regulator-type = "voltage"; >> + startup-delay-us = <0>; >> + gpio = <&tlmm_pinmux 77 GPIO_ACTIVE_HIGH>; >> + enable-active-high; >> + regulator-boot-on; >> + }; > > This shouldn't be inside the SoC node because it doesn't have a reg > property. It should be in a 'regulators' node that's in the root of the > tree: Is this new DT requirement/style? I have not noticed such a dt style in the past. I might have missed it. Any advantage of doing this way? > > regulators { > compatible = "simple-bus"; > > ext_3p3v: fixedregulator@0 { > compatible = "regulator-fixed"; > ... > }; > }; > I will move this to the suggested style in next version. > >> + >> qcom,ssbi@500000 { >> compatible = "qcom,ssbi"; >> reg = <0x00500000 0x1000>; >> @@ -522,5 +549,82 @@ >> compatible = "qcom,tcsr-apq8064", "syscon"; >> reg = <0x1a400000 0x100>; >> }; >> + >> + hdmi: qcom,hdmi-tx@4a00000 { >> + compatible = "qcom,hdmi-tx-8960"; >> + reg-names = "core_physical"; >> + reg = <0x04a00000 0x1000>; >> + interrupts = <GIC_SPI 79 IRQ_TYPE_NONE>; >> + clock-names = >> + "core_clk", >> + "master_iface_clk", >> + "slave_iface_clk"; >> + clocks = >> + <&mmcc HDMI_APP_CLK>, >> + <&mmcc HDMI_M_AHB_CLK>, >> + <&mmcc HDMI_S_AHB_CLK>; >> + qcom,hdmi-tx-ddc-clk = <&tlmm_pinmux 70 >> + GPIO_ACTIVE_HIGH>; >> + qcom,hdmi-tx-ddc-data = <&tlmm_pinmux 71 >> + GPIO_ACTIVE_HIGH>; >> + qcom,hdmi-tx-hpd = <&tlmm_pinmux 72 >> + GPIO_ACTIVE_HIGH>; > > This should be done via the *-gpios method. i.e. hdmi-tx-ddc-clk-gpios, > hdmi-tx-ddc-data-gpios, etc. > Thanks for the inputs, That's true, These are existing bindings, so I can't change it as part of this patch, However I will make another patch to fix this in both drivers and DT for good reasons. Just noticed that bindings are not consistent with the examples and the driver, which I guess is another issue. >> + core-vdda-supply = <&pm8921_hdmi_switch>; >> + hdmi-mux-supply = <&ext_3p3v>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&hdmi_pinctrl>; >> + }; >> + >> + gpu: qcom,adreno-3xx@4300000 { >> + compatible = "qcom,adreno-3xx"; >> + reg = <0x04300000 0x20000>; >> + reg-names = "kgsl_3d0_reg_memory"; >> + interrupts = <GIC_SPI 80 IRQ_TYPE_NONE>; >> + interrupt-names = "kgsl_3d0_irq"; >> + clock-names = >> + "core_clk", >> + "iface_clk", >> + "mem_clk", >> + "mem_iface_clk"; >> + clocks = >> + <&mmcc GFX3D_CLK>, >> + <&mmcc GFX3D_AHB_CLK>, >> + <&mmcc GFX3D_AXI_CLK>, >> + <&mmcc MMSS_IMEM_AHB_CLK>; >> + qcom,chipid = <0x03020002>; >> + qcom,gpu-pwrlevels { >> + compatible = "qcom,gpu-pwrlevels"; >> + qcom,gpu-pwrlevel@0 { >> + qcom,gpu-freq = <450000000>; >> + }; >> + qcom,gpu-pwrlevel@1 { >> + qcom,gpu-freq = <27000000>; >> + }; >> + }; > > This should be an OPP. Yes, that looks reasonable approch, But as I said before the driver and the bindings are still using this style, so I cant change this as part of this patch. I will create another patch to for better bindings with driver fixes too. >
On 04/10/15 12:39, Srinivas Kandagatla wrote: > > > On 10/04/15 18:04, Stephen Boyd wrote: >> On 04/10/15 05:34, Srinivas Kandagatla wrote: >>> @@ -250,6 +265,18 @@ >>> }; >>> }; >>> >>> + ext_3p3v: regulator-fixed@1 { >>> + compatible = "regulator-fixed"; >>> + regulator-min-microvolt = <3300000>; >>> + regulator-max-microvolt = <3300000>; >>> + regulator-name = "ext_3p3v"; >>> + regulator-type = "voltage"; >>> + startup-delay-us = <0>; >>> + gpio = <&tlmm_pinmux 77 GPIO_ACTIVE_HIGH>; >>> + enable-active-high; >>> + regulator-boot-on; >>> + }; >> >> This shouldn't be inside the SoC node because it doesn't have a reg >> property. It should be in a 'regulators' node that's in the root of the >> tree: > > Is this new DT requirement/style? I have not noticed such a dt style > in the past. I might have missed it. Any advantage of doing this way? It's a style. I'm not sure if it's new, but I feel like I've seen mention of it before more than a year ago (see arch/arm/boot/dts/tegra30-beaver.dts for an example). The advantage of doing it this way is we can see all the gpio/fixed regulators in one place and they're physically placed on a separate bus from the SoC bus. Typically nodes have reg properties too, so making up fake reg properties for the regulator nodes when they're on the SoC bus would be wrong and confusing. If they're under some regulators container node we can number them from 0 to N and use that for the reg property. >> >> regulators { >> compatible = "simple-bus"; >> >> ext_3p3v: fixedregulator@0 { >> compatible = "regulator-fixed"; >> ... >> }; >> }; >> > I will move this to the suggested style in next version. Thanks. >> >>> + >>> qcom,ssbi@500000 { >>> compatible = "qcom,ssbi"; >>> reg = <0x00500000 0x1000>; >>> @@ -522,5 +549,82 @@ >>> compatible = "qcom,tcsr-apq8064", "syscon"; >>> reg = <0x1a400000 0x100>; >>> }; >>> + >>> + hdmi: qcom,hdmi-tx@4a00000 { >>> + compatible = "qcom,hdmi-tx-8960"; >>> + reg-names = "core_physical"; >>> + reg = <0x04a00000 0x1000>; >>> + interrupts = <GIC_SPI 79 IRQ_TYPE_NONE>; >>> + clock-names = >>> + "core_clk", >>> + "master_iface_clk", >>> + "slave_iface_clk"; >>> + clocks = >>> + <&mmcc HDMI_APP_CLK>, >>> + <&mmcc HDMI_M_AHB_CLK>, >>> + <&mmcc HDMI_S_AHB_CLK>; >>> + qcom,hdmi-tx-ddc-clk = <&tlmm_pinmux 70 >>> + GPIO_ACTIVE_HIGH>; >>> + qcom,hdmi-tx-ddc-data = <&tlmm_pinmux 71 >>> + GPIO_ACTIVE_HIGH>; >>> + qcom,hdmi-tx-hpd = <&tlmm_pinmux 72 >>> + GPIO_ACTIVE_HIGH>; >> >> This should be done via the *-gpios method. i.e. hdmi-tx-ddc-clk-gpios, >> hdmi-tx-ddc-data-gpios, etc. >> > Thanks for the inputs, > > That's true, These are existing bindings, so I can't change it as part > of this patch, However I will make another patch to fix this in both > drivers and DT for good reasons. Just noticed that bindings are not > consistent with the examples and the driver, which I guess is another > issue. Yes, the driver/binding should be fixed and then this patch can be corrected and applied. There are no implementations of the DT for this device upstream in the dts directory so there's no breakage or backwards incompatibility problem by fixing the driver/binding.
On 10/04/15 21:21, Stephen Boyd wrote: > On 04/10/15 12:39, Srinivas Kandagatla wrote: >> >> >> On 10/04/15 18:04, Stephen Boyd wrote: >>> On 04/10/15 05:34, Srinivas Kandagatla wrote: >>>> @@ -250,6 +265,18 @@ >>>> }; >>>> }; >>>> >>>> + ext_3p3v: regulator-fixed@1 { >>>> + compatible = "regulator-fixed"; >>>> + regulator-min-microvolt = <3300000>; >>>> + regulator-max-microvolt = <3300000>; >>>> + regulator-name = "ext_3p3v"; >>>> + regulator-type = "voltage"; >>>> + startup-delay-us = <0>; >>>> + gpio = <&tlmm_pinmux 77 GPIO_ACTIVE_HIGH>; >>>> + enable-active-high; >>>> + regulator-boot-on; >>>> + }; >>> >>> This shouldn't be inside the SoC node because it doesn't have a reg >>> property. It should be in a 'regulators' node that's in the root of the >>> tree: >> >> Is this new DT requirement/style? I have not noticed such a dt style >> in the past. I might have missed it. Any advantage of doing this way? > > It's a style. I'm not sure if it's new, but I feel like I've seen > mention of it before more than a year ago (see > arch/arm/boot/dts/tegra30-beaver.dts for an example). The advantage of > doing it this way is we can see all the gpio/fixed regulators in one > place and they're physically placed on a separate bus from the SoC bus. > Typically nodes have reg properties too, so making up fake reg > properties for the regulator nodes when they're on the SoC bus would be > wrong and confusing. If they're under some regulators container node we > can number them from 0 to N and use that for the reg property. > Thanks for explaining. >>> >>>> + >>>> + hdmi: qcom,hdmi-tx@4a00000 { >>>> + compatible = "qcom,hdmi-tx-8960"; >>>> + reg-names = "core_physical"; >>>> + reg = <0x04a00000 0x1000>; >>>> + interrupts = <GIC_SPI 79 IRQ_TYPE_NONE>; >>>> + clock-names = >>>> + "core_clk", >>>> + "master_iface_clk", >>>> + "slave_iface_clk"; >>>> + clocks = >>>> + <&mmcc HDMI_APP_CLK>, >>>> + <&mmcc HDMI_M_AHB_CLK>, >>>> + <&mmcc HDMI_S_AHB_CLK>; >>>> + qcom,hdmi-tx-ddc-clk = <&tlmm_pinmux 70 >>>> + GPIO_ACTIVE_HIGH>; >>>> + qcom,hdmi-tx-ddc-data = <&tlmm_pinmux 71 >>>> + GPIO_ACTIVE_HIGH>; >>>> + qcom,hdmi-tx-hpd = <&tlmm_pinmux 72 >>>> + GPIO_ACTIVE_HIGH>; >>> >>> This should be done via the *-gpios method. i.e. hdmi-tx-ddc-clk-gpios, >>> hdmi-tx-ddc-data-gpios, etc. >>> >> Thanks for the inputs, >> >> That's true, These are existing bindings, so I can't change it as part >> of this patch, However I will make another patch to fix this in both >> drivers and DT for good reasons. Just noticed that bindings are not >> consistent with the examples and the driver, which I guess is another >> issue. > > Yes, the driver/binding should be fixed and then this patch can be > corrected and applied. There are no implementations of the DT for this > device upstream in the dts directory so there's no breakage or backwards > incompatibility problem by fixing the driver/binding. > Yep, In that case, I should pull this patch out of this series just to avoid any delays and create a new patchset for fixing bindings + driver + DT.
On Fri, Apr 10, 2015 at 4:21 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> That's true, These are existing bindings, so I can't change it as part >> of this patch, However I will make another patch to fix this in both >> drivers and DT for good reasons. Just noticed that bindings are not >> consistent with the examples and the driver, which I guess is another >> issue. > > Yes, the driver/binding should be fixed and then this patch can be > corrected and applied. There are no implementations of the DT for this > device upstream in the dts directory so there's no breakage or backwards > incompatibility problem by fixing the driver/binding. jfyi, some of the current bindings are the way they are to simplify compatibility w/ downstream kernels which I unfortunately still have to deal with.. ofc, the sooner I don't have to deal w/ downstream kernels, the easier it gets to clean up ;-) ofc, since the number of devices that can run upstream kernel but don't have upstream dts files is approximately zero, this would be a case were it would be useful to be able to mark certain bindings as non-abi and refactor them later. BR, -R
diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi index 56cc65e..c88470c 100644 --- a/arch/arm/boot/dts/qcom-apq8064.dtsi +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi @@ -1,6 +1,7 @@ /dts-v1/; #include "skeleton.dtsi" +#include <dt-bindings/gpio/gpio.h> #include <dt-bindings/clock/qcom,gcc-msm8960.h> #include <dt-bindings/reset/qcom,gcc-msm8960.h> #include <dt-bindings/clock/qcom,mmcc-msm8960.h> @@ -107,6 +108,20 @@ }; }; + hdmi_pinctrl: hdmi-pinctrl { + mux1 { + pins = "gpio69", "gpio70", "gpio71"; + function = "hdmi"; + bias-pull-up; + drive-strength = <2>; + }; + mux2 { + pins = "gpio72"; + function = "hdmi"; + bias-pull-down; + drive-strength = <16>; + }; + }; ps_hold: ps_hold { mux { pins = "gpio78"; @@ -250,6 +265,18 @@ }; }; + ext_3p3v: regulator-fixed@1 { + compatible = "regulator-fixed"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + regulator-name = "ext_3p3v"; + regulator-type = "voltage"; + startup-delay-us = <0>; + gpio = <&tlmm_pinmux 77 GPIO_ACTIVE_HIGH>; + enable-active-high; + regulator-boot-on; + }; + qcom,ssbi@500000 { compatible = "qcom,ssbi"; reg = <0x00500000 0x1000>; @@ -522,5 +549,82 @@ compatible = "qcom,tcsr-apq8064", "syscon"; reg = <0x1a400000 0x100>; }; + + hdmi: qcom,hdmi-tx@4a00000 { + compatible = "qcom,hdmi-tx-8960"; + reg-names = "core_physical"; + reg = <0x04a00000 0x1000>; + interrupts = <GIC_SPI 79 IRQ_TYPE_NONE>; + clock-names = + "core_clk", + "master_iface_clk", + "slave_iface_clk"; + clocks = + <&mmcc HDMI_APP_CLK>, + <&mmcc HDMI_M_AHB_CLK>, + <&mmcc HDMI_S_AHB_CLK>; + qcom,hdmi-tx-ddc-clk = <&tlmm_pinmux 70 + GPIO_ACTIVE_HIGH>; + qcom,hdmi-tx-ddc-data = <&tlmm_pinmux 71 + GPIO_ACTIVE_HIGH>; + qcom,hdmi-tx-hpd = <&tlmm_pinmux 72 + GPIO_ACTIVE_HIGH>; + core-vdda-supply = <&pm8921_hdmi_switch>; + hdmi-mux-supply = <&ext_3p3v>; + pinctrl-names = "default"; + pinctrl-0 = <&hdmi_pinctrl>; + }; + + gpu: qcom,adreno-3xx@4300000 { + compatible = "qcom,adreno-3xx"; + reg = <0x04300000 0x20000>; + reg-names = "kgsl_3d0_reg_memory"; + interrupts = <GIC_SPI 80 IRQ_TYPE_NONE>; + interrupt-names = "kgsl_3d0_irq"; + clock-names = + "core_clk", + "iface_clk", + "mem_clk", + "mem_iface_clk"; + clocks = + <&mmcc GFX3D_CLK>, + <&mmcc GFX3D_AHB_CLK>, + <&mmcc GFX3D_AXI_CLK>, + <&mmcc MMSS_IMEM_AHB_CLK>; + qcom,chipid = <0x03020002>; + qcom,gpu-pwrlevels { + compatible = "qcom,gpu-pwrlevels"; + qcom,gpu-pwrlevel@0 { + qcom,gpu-freq = <450000000>; + }; + qcom,gpu-pwrlevel@1 { + qcom,gpu-freq = <27000000>; + }; + }; + }; + + mdp: qcom,mdp@5100000 { + compatible = "qcom,mdp"; + reg = <0x05100000 0xf0000>; + interrupts = <GIC_SPI 75 IRQ_TYPE_NONE>; + connectors = <&hdmi>; + gpus = <&gpu>; + clock-names = + "core_clk", + "iface_clk", + "lut_clk", + "src_clk", + "hdmi_clk", + "mdp_clk", + "mdp_axi_clk"; + clocks = + <&mmcc MDP_CLK>, + <&mmcc MDP_AHB_CLK>, + <&mmcc MDP_LUT_CLK>, + <&mmcc TV_SRC>, + <&mmcc HDMI_TV_CLK>, + <&mmcc MDP_TV_CLK>, + <&mmcc MDP_AXI_CLK>; + }; }; };