| Message ID | 1382700168-12527-1-git-send-email-sachin.kamat@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sachin Kamat wrote: > Following is more clear? "ARM: dts: Add missing GPIO entries for sd_bus_width4 in exynos5420-pinctrl" > Adds missing GPIO entries for sd_bus nodes in exynos5420-pinctrl. > > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> > Signed-off-by: Tushar Behera <tushar.behera@linaro.org> > --- > arch/arm/boot/dts/exynos5420-pinctrl.dtsi | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi > b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi > index e695aba..fcb8206 100644 > --- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi > +++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi > @@ -181,7 +181,7 @@ > }; > > sd0_bus4: sd0-bus-width4 { > - samsung,pins = "gpc0-4", "gpc0-5", "gpc0-6"; > + samsung,pins = "gpc0-3", "gpc0-4", "gpc0-5", "gpc0-6"; > samsung,pin-function = <2>; > samsung,pin-pud = <3>; > samsung,pin-drv = <3>; > @@ -230,7 +230,7 @@ > }; > > sd1_bus4: sd1-bus-width4 { > - samsung,pins = "gpc1-4", "gpc1-5", "gpc1-6"; > + samsung,pins = "gpc1-3", "gpc1-4", "gpc1-5", "gpc1-6"; > samsung,pin-function = <2>; > samsung,pin-pud = <3>; > samsung,pin-drv = <3>; > @@ -272,7 +272,7 @@ > }; > > sd2_bus4: sd2-bus-width4 { > - samsung,pins = "gpc2-4", "gpc2-5", "gpc2-6"; > + samsung,pins = "gpc2-3", "gpc2-4", "gpc2-5", "gpc2-6"; > samsung,pin-function = <2>; > samsung,pin-pud = <3>; > samsung,pin-drv = <3>; > -- > 1.7.9.5 Looks ok to me, applied with updating title. Thanks, Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sachin, On Tue, Nov 12, 2013 at 3:53 PM, Kukjin Kim <kgene@kernel.org> wrote: > Sachin Kamat wrote: >> > > Following is more clear? > > "ARM: dts: Add missing GPIO entries for sd_bus_width4 in exynos5420-pinctrl" > >> Adds missing GPIO entries for sd_bus nodes in exynos5420-pinctrl. >> This is not a missing stuff, I did this purposefully. Myself and Doug anderson discussed on this and the objective of doing this is to eliminate the gpio pin configuration multiple times. Please see the below explanation The current code in main line kernel shows like sd0_bus1: sd0-bus-width1 { samsung,pins = "gpc0-3"; }; sd0_bus4: sd0-bus-width4 { samsung,pins = "gpc0-4", "gpc0-5", "gpc0-6"; }; sd0_bus8: sd0-bus-width8 { samsung,pins = "gpc3-0", "gpc3-1", "gpc3-2", "gpc3-3"; }; and lets mmc wants to use 8 bit width then node should be like below dwmmc0@12200000 { [snip] pinctrl-names = "default"; pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4 &sd0_bus8>; Here it will configure all the 8 pins ("gpc0-3", "gpc0-4", "gpc0-5", "gpc0-6", "gpc3-0", "gpc3-1", "gpc3-2", "gpc3-3") slot@0 { reg = <0>; bus-width = <8>; }; }; Similarly for using 1 bit width the property value should be like below pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1>; and if other mmc wants to use 4 bit width the property value should be like below pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4>; But with the changes you made and if one mmc wants to use 1 bit width and other wants to use 4 bit width the pin "gpc0-3" will be configured twice which is wrong. So, I strongly feel it is better to change the existing mmc node instead of changing the "sd0_bus" entries Best Wishes, Leela Krishna. >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> >> Signed-off-by: Tushar Behera <tushar.behera@linaro.org> >> --- >> arch/arm/boot/dts/exynos5420-pinctrl.dtsi | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi >> b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi >> index e695aba..fcb8206 100644 >> --- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi >> +++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi >> @@ -181,7 +181,7 @@ >> }; >> >> sd0_bus4: sd0-bus-width4 { >> - samsung,pins = "gpc0-4", "gpc0-5", "gpc0-6"; >> + samsung,pins = "gpc0-3", "gpc0-4", "gpc0-5", > "gpc0-6"; >> samsung,pin-function = <2>; >> samsung,pin-pud = <3>; >> samsung,pin-drv = <3>; >> @@ -230,7 +230,7 @@ >> }; >> >> sd1_bus4: sd1-bus-width4 { >> - samsung,pins = "gpc1-4", "gpc1-5", "gpc1-6"; >> + samsung,pins = "gpc1-3", "gpc1-4", "gpc1-5", > "gpc1-6"; >> samsung,pin-function = <2>; >> samsung,pin-pud = <3>; >> samsung,pin-drv = <3>; >> @@ -272,7 +272,7 @@ >> }; >> >> sd2_bus4: sd2-bus-width4 { >> - samsung,pins = "gpc2-4", "gpc2-5", "gpc2-6"; >> + samsung,pins = "gpc2-3", "gpc2-4", "gpc2-5", > "gpc2-6"; >> samsung,pin-function = <2>; >> samsung,pin-pud = <3>; >> samsung,pin-drv = <3>; >> -- >> 1.7.9.5 > > Looks ok to me, applied with updating title. > > Thanks, > Kukjin > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Leela, Thanks for the detailed explanation. On 12 November 2013 17:20, Leela Krishna Amudala <leelakrishna.a@gmail.com> wrote: > Hi Sachin, > > On Tue, Nov 12, 2013 at 3:53 PM, Kukjin Kim <kgene@kernel.org> wrote: >> Sachin Kamat wrote: >>> >> >> Following is more clear? >> >> "ARM: dts: Add missing GPIO entries for sd_bus_width4 in exynos5420-pinctrl" >> >>> Adds missing GPIO entries for sd_bus nodes in exynos5420-pinctrl. >>> > > This is not a missing stuff, I did this purposefully. > Myself and Doug anderson discussed on this and the objective of doing this is to > eliminate the gpio pin configuration multiple times. > > Please see the below explanation > > The current code in main line kernel shows like > sd0_bus1: sd0-bus-width1 { > samsung,pins = "gpc0-3"; > }; > > sd0_bus4: sd0-bus-width4 { > samsung,pins = "gpc0-4", "gpc0-5", "gpc0-6"; > }; > > sd0_bus8: sd0-bus-width8 { > samsung,pins = "gpc3-0", "gpc3-1", "gpc3-2", "gpc3-3"; > }; > > and lets mmc wants to use 8 bit width then node should be like below > > dwmmc0@12200000 { > [snip] > pinctrl-names = "default"; > pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4 &sd0_bus8>; > > Here it will configure all the 8 pins ("gpc0-3", "gpc0-4", "gpc0-5", > "gpc0-6", "gpc3-0", "gpc3-1", "gpc3-2", "gpc3-3") > > slot@0 { > reg = <0>; > bus-width = <8>; > }; > }; > > Similarly for using 1 bit width the property value should be like below > pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1>; > > and if other mmc wants to use 4 bit width the property value should be > like below > pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4>; > > But with the changes you made and if one mmc wants to use 1 bit width > and other wants to use 4 bit width > the pin "gpc0-3" will be configured twice which is wrong. I think this is the case with current code too. From your above example, for 2 slots: For 1 bit width: pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1>; "gpc0-3" For 4 bit width: pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4>; "gpc0-3", "gpc0-4", "gpc0-5", "gpc0-6" > So, I strongly feel it is better to change the existing mmc node > instead of changing the "sd0_bus" entries I think this is already taken care of in the mmc nodes. For slot with bus-width of 8, pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus4 &sd0_bus8>; Similarly for slot with bus width of 4, pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>; Do you still see the problem here?
Hi Kukjin, On 12 November 2013 15:53, Kukjin Kim <kgene@kernel.org> wrote: > Sachin Kamat wrote: >> > > Following is more clear? > > "ARM: dts: Add missing GPIO entries for sd_bus_width4 in exynos5420-pinctrl" > >> Adds missing GPIO entries for sd_bus nodes in exynos5420-pinctrl. >> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> >> Signed-off-by: Tushar Behera <tushar.behera@linaro.org> >> --- >> arch/arm/boot/dts/exynos5420-pinctrl.dtsi | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi >> b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi >> index e695aba..fcb8206 100644 >> --- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi >> +++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi >> @@ -181,7 +181,7 @@ >> }; >> >> sd0_bus4: sd0-bus-width4 { >> - samsung,pins = "gpc0-4", "gpc0-5", "gpc0-6"; >> + samsung,pins = "gpc0-3", "gpc0-4", "gpc0-5", > "gpc0-6"; >> samsung,pin-function = <2>; >> samsung,pin-pud = <3>; >> samsung,pin-drv = <3>; >> @@ -230,7 +230,7 @@ >> }; >> >> sd1_bus4: sd1-bus-width4 { >> - samsung,pins = "gpc1-4", "gpc1-5", "gpc1-6"; >> + samsung,pins = "gpc1-3", "gpc1-4", "gpc1-5", > "gpc1-6"; >> samsung,pin-function = <2>; >> samsung,pin-pud = <3>; >> samsung,pin-drv = <3>; >> @@ -272,7 +272,7 @@ >> }; >> >> sd2_bus4: sd2-bus-width4 { >> - samsung,pins = "gpc2-4", "gpc2-5", "gpc2-6"; >> + samsung,pins = "gpc2-3", "gpc2-4", "gpc2-5", > "gpc2-6"; >> samsung,pin-function = <2>; >> samsung,pin-pud = <3>; >> samsung,pin-drv = <3>; >> -- >> 1.7.9.5 > > Looks ok to me, applied with updating title. Didn't find this in your tree.
Hi Sachin, 2013/11/13 Sachin Kamat <sachin.kamat@linaro.org>: > Hi Leela, > > Thanks for the detailed explanation. > > On 12 November 2013 17:20, Leela Krishna Amudala > <leelakrishna.a@gmail.com> wrote: >> Hi Sachin, >> >> On Tue, Nov 12, 2013 at 3:53 PM, Kukjin Kim <kgene@kernel.org> wrote: >>> Sachin Kamat wrote: >>>> >>> >>> Following is more clear? >>> >>> "ARM: dts: Add missing GPIO entries for sd_bus_width4 in exynos5420-pinctrl" >>> >>>> Adds missing GPIO entries for sd_bus nodes in exynos5420-pinctrl. >>>> >> >> This is not a missing stuff, I did this purposefully. >> Myself and Doug anderson discussed on this and the objective of doing this is to >> eliminate the gpio pin configuration multiple times. >> >> Please see the below explanation >> >> The current code in main line kernel shows like >> sd0_bus1: sd0-bus-width1 { >> samsung,pins = "gpc0-3"; >> }; >> >> sd0_bus4: sd0-bus-width4 { >> samsung,pins = "gpc0-4", "gpc0-5", "gpc0-6"; >> }; >> >> sd0_bus8: sd0-bus-width8 { >> samsung,pins = "gpc3-0", "gpc3-1", "gpc3-2", "gpc3-3"; >> }; >> >> and lets mmc wants to use 8 bit width then node should be like below >> >> dwmmc0@12200000 { >> [snip] >> pinctrl-names = "default"; >> pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4 &sd0_bus8>; >> >> Here it will configure all the 8 pins ("gpc0-3", "gpc0-4", "gpc0-5", >> "gpc0-6", "gpc3-0", "gpc3-1", "gpc3-2", "gpc3-3") >> >> slot@0 { >> reg = <0>; >> bus-width = <8>; >> }; >> }; >> >> Similarly for using 1 bit width the property value should be like below >> pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1>; >> >> and if other mmc wants to use 4 bit width the property value should be >> like below >> pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4>; >> >> But with the changes you made and if one mmc wants to use 1 bit width >> and other wants to use 4 bit width >> the pin "gpc0-3" will be configured twice which is wrong. > > I think this is the case with current code too. > > From your above example, for 2 slots: > For 1 bit width: pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1>; > "gpc0-3" > > For 4 bit width: pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus1 &sd0_bus4>; > "gpc0-3", "gpc0-4", "gpc0-5", "gpc0-6" No, this way you will have gpc0-3 in two groups - sd0_bus1 and sd0_bus4, which probably won't even work as the pinctrl subsystem probably doesn't allow claiming one pin twice. > > >> So, I strongly feel it is better to change the existing mmc node >> instead of changing the "sd0_bus" entries > > I think this is already taken care of in the mmc nodes. > For slot with bus-width of 8, > pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_bus4 &sd0_bus8>; > > Similarly for slot with bus width of 4, > pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>; This is inconsistent: - for 1-bit bus you would have to specify just sdX_bus1, - for 4-bit just sdX_bus4, but - for 8-bit you need to specify both sdX_bus4 and sdX_bus8. The change Leela mentioned was about making this consistent, so you need to always specify all possible working widths on given board. Other Exynos SoCs should be changed to use the same convention as well. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz, Leela, On 19 December 2013 00:13, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Hi Sachin, > > This is inconsistent: > - for 1-bit bus you would have to specify just sdX_bus1, > - for 4-bit just sdX_bus4, but > - for 8-bit you need to specify both sdX_bus4 and sdX_bus8. > > The change Leela mentioned was about making this consistent, > so you need to always specify all possible working widths on > given board. > > Other Exynos SoCs should be changed to use the same > convention as well. With the above explanation, it makes more sense now to specify all possible bus widths. I will drop this patch and change it in other places (SoCs) to make it consistent.
diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi index e695aba..fcb8206 100644 --- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi +++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi @@ -181,7 +181,7 @@ }; sd0_bus4: sd0-bus-width4 { - samsung,pins = "gpc0-4", "gpc0-5", "gpc0-6"; + samsung,pins = "gpc0-3", "gpc0-4", "gpc0-5", "gpc0-6"; samsung,pin-function = <2>; samsung,pin-pud = <3>; samsung,pin-drv = <3>; @@ -230,7 +230,7 @@ }; sd1_bus4: sd1-bus-width4 { - samsung,pins = "gpc1-4", "gpc1-5", "gpc1-6"; + samsung,pins = "gpc1-3", "gpc1-4", "gpc1-5", "gpc1-6"; samsung,pin-function = <2>; samsung,pin-pud = <3>; samsung,pin-drv = <3>; @@ -272,7 +272,7 @@ }; sd2_bus4: sd2-bus-width4 { - samsung,pins = "gpc2-4", "gpc2-5", "gpc2-6"; + samsung,pins = "gpc2-3", "gpc2-4", "gpc2-5", "gpc2-6"; samsung,pin-function = <2>; samsung,pin-pud = <3>; samsung,pin-drv = <3>;