RE: [PATCH] ARM: dts: Add missing GPIO entries for sd_bus in

RE: [PATCH] ARM: dts: Add missing GPIO entries for sd_bus in

[Kukjin Kim]

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

Re: [PATCH] ARM: dts: Add missing GPIO entries for sd_bus in

[Leela Krishna Amudala]

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

Re: [PATCH] ARM: dts: Add missing GPIO entries for sd_bus in

[Sachin Kamat]

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?

-- 
With warm regards,
Sachin

Re: [PATCH] ARM: dts: Add missing GPIO entries for sd_bus in

[Tomasz Figa]

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

Re: [PATCH] ARM: dts: Add missing GPIO entries for sd_bus in

[Sachin Kamat]

Hi Tomasz, Leela,

On 19 December 2013 00:13, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

-- 
With warm regards,
Sachin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html