linux-kernel - Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 25 Feb 2015 11:02:43 +0100
From:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
To:	Doug Anderson <dianders@...omium.org>
CC:	Jaehoon Chung <jh80.chung@...sung.com>,
	Seungwon Jeon <tgih.jun@...sung.com>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Alim Akhtar <alim.akhtar@...sung.com>,
	Sonny Rao <sonnyrao@...omium.org>,
	Andrew Bresticker <abrestic@...omium.org>,
	Heiko Stuebner <heiko@...ech.de>,
	Addy Ke <addy.ke@...k-chips.com>,
	Alexandru Stan <amstan@...omium.org>,
	Chris Ball <chris@...ntf.net>,
	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] mmc: dw_mmc: Don't start commands while busy

Hello Doug,

On 02/25/2015 06:43 AM, Doug Anderson wrote:
> 
> OK, so looking through things I _think_ I found another difference
> that _might_ matter...
>

Yes it does! when adding the "sd1_bus1" the slot pinctrl I do have
the WiFi module working, thanks a lot for your help!
 
> Upstream (arch/arm/boot/dts/exynos5420-pinctrl.dtsi):
>                 sd1_bus1: sd1-bus-width1 {
>                         samsung,pins = "gpc1-3";
>                         ...
>                 };
> 
>                 sd1_bus4: sd1-bus-width4 {
>                         samsung,pins = "gpc1-4", "gpc1-5", "gpc1-6";
>                         ...
>                };
> 
>                 sd1_bus8: sd1-bus-width8 {
>                         samsung,pins = "gpd1-4", "gpd1-5", "gpd1-6", "gpd1-7";
>                         ...
>                };
> 
> Upsttream (arch/arm/boot/dts/exynos5420-peach-pit.dts) -- your patch:
>        pinctrl-0 = <&sd1_clk>, <&sd1_cmd>, <&sd1_int>, <&sd1_bus4>,
>                    <&sd1_bus8>, <&wifi_en>, <&wifi_rst>;
> 
> Downstream (arch/arm/boot/dts/exynos542x-pinctrl.dtsi):
>                 sd1_bus1: sd1-bus-width1 {
>                         samsung,pins = "gpc1-3";
>                         ...
>                };
> 
>                 sd1_bus4: sd1-bus-width4 {
>                         samsung,pins = "gpc1-3", "gpc1-4", "gpc1-5", "gpc1-6";
>                         ...
>                };
> 
>                 sd1_bus8: sd1-bus-width8 {
>                         samsung,pins = "gpd1-4", "gpd1-5", "gpd1-6", "gpd1-7";
>                         ...
>                };
> 
> Downstream (arch/arm/boot/dts/exynos542x-peach.dtsi):
>                 pinctrl-0 = <&sd1_clk &sd1_cmd &sd1_int &sd1_bus4 &sd1_bus8>;
> 
> 
> Notice the difference?  You need to add "sd1_bus1" to the pinctrl for
> upstream.  The upstream DTS makes more sense.  I think I remember
> discussing this in the past (finding the conversation on the lists is
> left as an exercise to the reader) and you can in fact see that the
> upstream 5250 pinctrl file is like the downstream 5420 pinctrl...
>

Yeah, I didn't notice that there was an inconsistency in the pinctrl
defines in mainline around the different SoCs. So for 5250 you need:

* only sd1_bus1 for bus-width = <1>
* only sd1_bus4 for bus-width = <4>
* sd1_bus4 + sd1_bus8 for bus-width = <8>

and for 5440 you need:

* only sd1_bus1 for bus-width = <1>
* only sd1_bus1 + sd1_bus4 for bus-width = <4>
* sd1_bus1 + sd1_bus4 + sd1_bus8 for bus-width = <8>

Is true that 5440 is at least more consistent in the sense that you
have to add all the pins but IMHO this approach is very error prone.

I would preferred if sd1_busN included all pins needed for width N
so you only had to define a single pinctrl group but for now I'll
include "sd1_bus1" to fix the SDIO WiFi issue.

> I think the same bug is present in eMMC and SD but possibly the
> bootloader inits the pinctrl properly there?
>

Correct, I'll fix those too so the kernel does not rely on the boot
loader to setup those pins.

> 
> Crossing my fingers that's your bug, but I can't say for sure why
> adding a tons of resets would somehow make it better?
>

It is, thanks a lot again for finding this issue. I feel very dumb for
not realizing there was a difference in the included pinctrl definition.

> -Doug
>

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/