| Message ID | 1429538553-17366-2-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 20 April 2015 at 20:26, Lee Jones <lee@kernel.org> wrote: > On Mon, 20 Apr 2015, Ulf Hansson wrote: > >> The GPIO regulator for the SD-card isn't a ux500 SOC configuration, but >> instead it's specific to the board. Move the definition of it, into the >> board DTSs. > > What makes you think that? Because of how it was structured today. ste-dbx5x0.dtsi - common for all ux500 boards, thus I considered this as the SoC configuration. Then below are board configs which uses the above dtsi: ste-href.dtsi - common for href boards (used by ste-hrefprev60.dtsi and ste-hrefv60plus.dtsi), have vmmci ste-snowball.dts, have vmmci ste-ccu8540.dts, don't have vmmci ste-ccu9540.dts, don't have vmmci > > We normally place the common pieces (of which there are many in this > node) in the highest level DTSI file, then add the platform specific > ones in the DTS files. Okay, so maybe it's due to the naming of ste-dbx5x0.dtsi, that I thought this was intended to cover the SoC configuration and not any of the platform specific stuff. So what your advise of doing this? Kind regards Uffe > >> Fixes: c94a4ab7af3f ("ARM: ux500: Disable the MMCI gpio-regulator by default") >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> arch/arm/boot/dts/ste-dbx5x0.dtsi | 17 ----------------- >> arch/arm/boot/dts/ste-href.dtsi | 17 +++++++++++++++++ >> arch/arm/boot/dts/ste-snowball.dts | 15 +++++++++++++++ >> 3 files changed, 32 insertions(+), 17 deletions(-) >> >> diff --git a/arch/arm/boot/dts/ste-dbx5x0.dtsi b/arch/arm/boot/dts/ste-dbx5x0.dtsi >> index bfd3f1c..2201cd5 100644 >> --- a/arch/arm/boot/dts/ste-dbx5x0.dtsi >> +++ b/arch/arm/boot/dts/ste-dbx5x0.dtsi >> @@ -1017,23 +1017,6 @@ >> status = "disabled"; >> }; >> >> - vmmci: regulator-gpio { >> - compatible = "regulator-gpio"; >> - >> - regulator-min-microvolt = <1800000>; >> - regulator-max-microvolt = <2900000>; >> - regulator-name = "mmci-reg"; >> - regulator-type = "voltage"; >> - >> - startup-delay-us = <100>; >> - enable-active-high; >> - >> - states = <1800000 0x1 >> - 2900000 0x0>; >> - >> - status = "disabled"; >> - }; >> - >> mcde@a0350000 { >> compatible = "stericsson,mcde"; >> reg = <0xa0350000 0x1000>, /* MCDE */ >> diff --git a/arch/arm/boot/dts/ste-href.dtsi b/arch/arm/boot/dts/ste-href.dtsi >> index bf8f0ed..8cf499a 100644 >> --- a/arch/arm/boot/dts/ste-href.dtsi >> +++ b/arch/arm/boot/dts/ste-href.dtsi >> @@ -111,6 +111,23 @@ >> pinctrl-1 = <&i2c3_sleep_mode>; >> }; >> >> + vmmci: regulator-gpio { >> + compatible = "regulator-gpio"; >> + >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <2900000>; >> + regulator-name = "mmci-reg"; >> + regulator-type = "voltage"; >> + >> + startup-delay-us = <100>; >> + enable-active-high; >> + >> + states = <1800000 0x1 >> + 2900000 0x0>; >> + >> + status = "disabled"; >> + }; >> + >> // External Micro SD slot >> sdi0_per1@80126000 { >> arm,primecell-periphid = <0x10480180>; >> diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts >> index 206826a..65a7f63 100644 >> --- a/arch/arm/boot/dts/ste-snowball.dts >> +++ b/arch/arm/boot/dts/ste-snowball.dts >> @@ -146,8 +146,23 @@ >> }; >> >> vmmci: regulator-gpio { >> + compatible = "regulator-gpio"; >> + >> gpios = <&gpio7 4 0x4>; >> enable-gpio = <&gpio6 25 0x4>; >> + >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <2900000>; >> + regulator-name = "mmci-reg"; >> + regulator-type = "voltage"; >> + >> + startup-delay-us = <100>; >> + enable-active-high; >> + >> + states = <1800000 0x1 >> + 2900000 0x0>; >> + >> + status = "disabled"; >> }; >> >> // External Micro SD slot -- 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
On 21 April 2015 at 09:33, Lee Jones <lee@kernel.org> wrote: > On Tue, 21 Apr 2015, Ulf Hansson wrote: > >> On 20 April 2015 at 20:26, Lee Jones <lee@kernel.org> wrote: >> > On Mon, 20 Apr 2015, Ulf Hansson wrote: >> > >> >> The GPIO regulator for the SD-card isn't a ux500 SOC configuration, but >> >> instead it's specific to the board. Move the definition of it, into the >> >> board DTSs. >> > >> > What makes you think that? >> >> Because of how it was structured today. >> >> ste-dbx5x0.dtsi - common for all ux500 boards, thus I considered this >> as the SoC configuration. > > ste-dbx5x0.dtsi is common for all ux500 and ux540 boards. > >> Then below are board configs which uses the above dtsi: >> ste-href.dtsi - common for href boards (used by ste-hrefprev60.dtsi >> and ste-hrefv60plus.dtsi), have vmmci >> ste-snowball.dts, have vmmci >> ste-ccu8540.dts, don't have vmmci >> ste-ccu9540.dts, don't have vmmci > > Ah, got you. In which case it doesn't belong in ste-dbx5x0.dtsi. > >> > We normally place the common pieces (of which there are many in this >> > node) in the highest level DTSI file, then add the platform specific >> > ones in the DTS files. >> >> Okay, so maybe it's due to the naming of ste-dbx5x0.dtsi, that I >> thought this was intended to cover the SoC configuration and not any >> of the platform specific stuff. > > ste-dbx5x0.dtsi should cover all pieces which are common to all ux500 > and ux540 devices. Then the lower level file ste-href-ab8500.dtsi > should cover all pieces which are common to ux500 devices and finally > the DTS files should add board specific information. Duplicate > nodes and properties are frowned upon. > >> So what your advise of doing this? > > So the file which covers the x500 boards is ste-href-ab8500.dtsi. I > would move the node into there instead. Keep it disabled and enable > the associated nodes in the 2 DTS files. Why ste-href-ab8500.dtsi? Isn't that suppose to cover configurations common to the ab8500 subsystem? The vmmci models a board specific mounted circuit (aka level-shifter). Thus it exist on some boards but not on others. Kind regards Uffe -- 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
On Tue, 21 Apr 2015, Ulf Hansson wrote: > On 21 April 2015 at 09:33, Lee Jones <lee@kernel.org> wrote: > > On Tue, 21 Apr 2015, Ulf Hansson wrote: > > > >> On 20 April 2015 at 20:26, Lee Jones <lee@kernel.org> wrote: > >> > On Mon, 20 Apr 2015, Ulf Hansson wrote: > >> > > >> >> The GPIO regulator for the SD-card isn't a ux500 SOC configuration, but > >> >> instead it's specific to the board. Move the definition of it, into the > >> >> board DTSs. > >> > > >> > What makes you think that? > >> > >> Because of how it was structured today. > >> > >> ste-dbx5x0.dtsi - common for all ux500 boards, thus I considered this > >> as the SoC configuration. > > > > ste-dbx5x0.dtsi is common for all ux500 and ux540 boards. > > > >> Then below are board configs which uses the above dtsi: > >> ste-href.dtsi - common for href boards (used by ste-hrefprev60.dtsi > >> and ste-hrefv60plus.dtsi), have vmmci > >> ste-snowball.dts, have vmmci > >> ste-ccu8540.dts, don't have vmmci > >> ste-ccu9540.dts, don't have vmmci > > > > Ah, got you. In which case it doesn't belong in ste-dbx5x0.dtsi. > > > >> > We normally place the common pieces (of which there are many in this > >> > node) in the highest level DTSI file, then add the platform specific > >> > ones in the DTS files. > >> > >> Okay, so maybe it's due to the naming of ste-dbx5x0.dtsi, that I > >> thought this was intended to cover the SoC configuration and not any > >> of the platform specific stuff. > > > > ste-dbx5x0.dtsi should cover all pieces which are common to all ux500 > > and ux540 devices. Then the lower level file ste-href-ab8500.dtsi > > should cover all pieces which are common to ux500 devices and finally > > the DTS files should add board specific information. Duplicate > > nodes and properties are frowned upon. > > > >> So what your advise of doing this? > > > > So the file which covers the x500 boards is ste-href-ab8500.dtsi. I > > would move the node into there instead. Keep it disabled and enable > > the associated nodes in the 2 DTS files. > > Why ste-href-ab8500.dtsi? Isn't that suppose to cover configurations > common to the ab8500 subsystem? Only because up until now that has been what is a) different from the abx5{40,05} platforms and b) common on abx500 ones. However, the point of the DTS(I) hierarchy is to prevent duplication. Lower level DTSI files contain nodes which are similar to a sub-set of platforms, whereas the highest level DTSI files contain nodes which are shared between all associated platforms. > The vmmci models a board specific mounted circuit (aka level-shifter). > Thus it exist on some boards but not on others. Many of the peripherals we use on the boards are 'off-chip'. That does not preclude them from DTSI files if they are shared among various platforms.
diff --git a/arch/arm/boot/dts/ste-dbx5x0.dtsi b/arch/arm/boot/dts/ste-dbx5x0.dtsi index bfd3f1c..2201cd5 100644 --- a/arch/arm/boot/dts/ste-dbx5x0.dtsi +++ b/arch/arm/boot/dts/ste-dbx5x0.dtsi @@ -1017,23 +1017,6 @@ status = "disabled"; }; - vmmci: regulator-gpio { - compatible = "regulator-gpio"; - - regulator-min-microvolt = <1800000>; - regulator-max-microvolt = <2900000>; - regulator-name = "mmci-reg"; - regulator-type = "voltage"; - - startup-delay-us = <100>; - enable-active-high; - - states = <1800000 0x1 - 2900000 0x0>; - - status = "disabled"; - }; - mcde@a0350000 { compatible = "stericsson,mcde"; reg = <0xa0350000 0x1000>, /* MCDE */ diff --git a/arch/arm/boot/dts/ste-href.dtsi b/arch/arm/boot/dts/ste-href.dtsi index bf8f0ed..8cf499a 100644 --- a/arch/arm/boot/dts/ste-href.dtsi +++ b/arch/arm/boot/dts/ste-href.dtsi @@ -111,6 +111,23 @@ pinctrl-1 = <&i2c3_sleep_mode>; }; + vmmci: regulator-gpio { + compatible = "regulator-gpio"; + + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <2900000>; + regulator-name = "mmci-reg"; + regulator-type = "voltage"; + + startup-delay-us = <100>; + enable-active-high; + + states = <1800000 0x1 + 2900000 0x0>; + + status = "disabled"; + }; + // External Micro SD slot sdi0_per1@80126000 { arm,primecell-periphid = <0x10480180>; diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts index 206826a..65a7f63 100644 --- a/arch/arm/boot/dts/ste-snowball.dts +++ b/arch/arm/boot/dts/ste-snowball.dts @@ -146,8 +146,23 @@ }; vmmci: regulator-gpio { + compatible = "regulator-gpio"; + gpios = <&gpio7 4 0x4>; enable-gpio = <&gpio6 25 0x4>; + + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <2900000>; + regulator-name = "mmci-reg"; + regulator-type = "voltage"; + + startup-delay-us = <100>; + enable-active-high; + + states = <1800000 0x1 + 2900000 0x0>; + + status = "disabled"; }; // External Micro SD slot
The GPIO regulator for the SD-card isn't a ux500 SOC configuration, but instead it's specific to the board. Move the definition of it, into the board DTSs. Fixes: c94a4ab7af3f ("ARM: ux500: Disable the MMCI gpio-regulator by default") Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- arch/arm/boot/dts/ste-dbx5x0.dtsi | 17 ----------------- arch/arm/boot/dts/ste-href.dtsi | 17 +++++++++++++++++ arch/arm/boot/dts/ste-snowball.dts | 15 +++++++++++++++ 3 files changed, 32 insertions(+), 17 deletions(-)