| Message ID | 54EB5658.6080004@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Javier, On Mon, Feb 23, 2015 at 8:33 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > Hello Doug, > > On 02/20/2015 09:31 PM, Doug Anderson wrote: >> We've seen problems on some WiFi modules where we seem to send a CMD53 >> (which requires the data lines) while the module is asserting busy. >> We shouldn't do that. >> >> The Designware Databook says that before issuing a new data transfer >> command we should check for busy, so that's what we'll do. >> > > I tried $subject along with patches: > > * mmc: dw_mmc: Make sure we only adjust the clock when power is on > https://patchwork.kernel.org/patch/5858261/ > > * mmc: dw_mmc: Give a good reset after we give power > https://patchwork.kernel.org/patch/5858281/ > > but unfortunately these don't solve the "Timeout sending command" error > that I got when trying to enable the WiFi module on the Peach Pit and Pi: > > [ 5.332103] dwmmc_exynos 12210000.mmc: Busy; trying anyway > [ 5.336110] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0 status 0x0) > >> We'll leverage the existing dw_mmc knowledge about whether it should >> wait for the previous command to finish to know whether we should >> check for busy before sending the command. This means we won't end up >> incorrectly waiting for things like CMD52 (SDIO) or CMD13 (SD) which >> don't use the data line. >> >> Note that this also has the advantage of making sure that we don't >> change the clock while the card is busy, too. >> > > The timeout happens in this case: > > mmc_rescan() > mmc_attach_sdio() > mmc_sdio_init_card() > dw_mci_init_card() > mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | > SDMMC_CMD_PRV_DAT_WAIT, 0); > > which should be covered by your patch since the SDMMC_CMD_PRV_DAT_WAIT flag > is set but SDMMC_STATUS_BUSY bit is not cleared and mci_send_cmd() timeouts > when sending the command to the controller. > >> >> +static void dw_mci_wait_while_busy(struct dw_mci *host, u32 cmd_flags) >> +{ >> + unsigned long timeout = jiffies + msecs_to_jiffies(500); >> + >> + /* >> + * Databook says that before issuing a new data transfer command >> + * we need to check to see if the card is busy. Data transfer commands >> + * all have SDMMC_CMD_PRV_DAT_WAIT set, so we'll key off that. >> + * >> + * ...also allow sending for SDMMC_CMD_VOLT_SWITCH where busy is >> + * expected. >> + */ >> + if ((cmd_flags & SDMMC_CMD_PRV_DAT_WAIT) && >> + !(cmd_flags & SDMMC_CMD_VOLT_SWITCH)) { >> + while (mci_readl(host, STATUS) & SDMMC_STATUS_BUSY) { >> + if (time_after(jiffies, timeout)) { >> + /* Command will fail; we'll pass error then */ >> + dev_err(host->dev, "Busy; trying anyway\n"); > > Addy's "mmc: dw_mmc: fix bug that cause 'Timeout sending command" [0] patch > reset the controller if it was busy for more than 500ms while your patch > doesn't. I don't mean that your patch is not correct, I'm just mentioning > because calling dw_mci_ctrl_reset() does makes the command to succeed the > next time and I can see the SDIO devices enumerated in /sys/bus/sdio/devices: > > [ 5.892124] dwmmc_exynos 12210000.mmc: Busy; trying anyway > [ 5.892135] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0 status 0x0) > [ 5.913885] mmc2: new high speed SDIO card at address 0001 > [ 6.582133] dwmmc_exynos 12210000.mmc: Busy; trying anyway Hmmmmm. In the cases I was seeing I didn't need the "reset" since the "SDMMC_CMD_UPD_CLK" was the one from dw_mci_set_ios() and my patch: * mmc: dw_mmc: Give a good reset after we give power https://patchwork.kernel.org/patch/5858281/ ...gave the needed reset after vqmmc power was applied. Then dw_mmc never got wedged and didn't need the reset to get it unwedged. In your care you're getting called from dw_mci_init_card(). That should happen _after_ dw_mci_set_ios() as far as I know. Can you put a printout in the reset in dw_mci_set_ios() and make sure it runs? How many resets do you need before things work? If just one then something must be happening between the initial "set_ios" and the "init_card". Any chance you could figure out what that is? If it needs multiple resets then it seems like something is fishy... > but that reset may not be right since the WiFi chip does not work because > the firmware later fails to load: > > [ 240.273352] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > [ 240.281159] kworker/2:1 D c04b3340 0 1260 2 0x00000000 > [ 240.287487] Workqueue: events request_firmware_work_func > [ 240.292763] [<c04b3340>] (__schedule) from [<c04b36f0>] (schedule+0x34/0x98) > [ 240.299815] [<c04b36f0>] (schedule) from [<c04b7198>] (schedule_timeout+0x120/0x16c) > [ 240.307537] [<c04b7198>] (schedule_timeout) from [<c04b41b4>] (wait_for_common+0xb0/0x154) > [ 240.315783] [<c04b41b4>] (wait_for_common) from [<c037abb8>] (mmc_wait_for_req+0xa0/0x140) > [ 240.324020] [<c037abb8>] (mmc_wait_for_req) from [<c0384b04>] (mmc_io_rw_extended+0x304/0x34c) > [ 240.332607] [<c0384b04>] (mmc_io_rw_extended) from [<c0385a90>] (sdio_io_rw_ext_helper+0x138/0x1a4) > [ 240.341630] [<c0385a90>] (sdio_io_rw_ext_helper) from [<c0385b1c>] (sdio_writesb+0x20/0x28) > [ 240.349962] [<c0385b1c>] (sdio_writesb) from [<c02e60d4>] (mwifiex_write_data_sync+0x4c/0x80) > [ 240.358460] [<c02e60d4>] (mwifiex_write_data_sync) from [<c02e68e4>] (mwifiex_prog_fw_w_helper+0x1b0/0x2c4) > [ 240.368176] [<c02e68e4>] (mwifiex_prog_fw_w_helper) from [<c02c7d6c>] (mwifiex_dnld_fw+0x58/0x10c) > [ 240.377127] [<c02c7d6c>] (mwifiex_dnld_fw) from [<c02c5f10>] (mwifiex_fw_dpc+0x264/0x408) > [ 240.385263] [<c02c5f10>] (mwifiex_fw_dpc) from [<c0291b28>] (request_firmware_work_func+0x30/0x50) > [ 240.394200] [<c0291b28>] (request_firmware_work_func) from [<c0032ae8>] (process_one_work+0x120/0x324) > [ 240.403482] [<c0032ae8>] (process_one_work) from [<c0032e58>] (worker_thread+0x138/0x464) > [ 240.411635] [<c0032e58>] (worker_thread) from [<c0037470>] (kthread+0xd8/0xf4) > [ 240.418837] [<c0037470>] (kthread) from [<c000e680>] (ret_from_fork+0x14/0x34) > > The DTS changes on top of linux-next to add WiFi support is [1] in case you can > find something that is wrong with my patch. I still haven't had time to try using the new MMC power sequencing :( so I can't say for sure, but one thought below... > I also checked that the external reference clock for the WiFi module is enabled > correctly according to /sys/kernel/debug/clk/clk_summary and also by looking > at the value in the max77802 i2c register address that enables that clock. > > Any hints will be highly appreciated. > > Thanks a lot and best regards, > Javier > > [0]: https://lkml.org/lkml/2015/2/5/222 > [1]: > From ced0974472d109019e1ee551a6dd08f16ae5cfc2 Mon Sep 17 00:00:00 2001 > From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Date: Mon, 23 Feb 2014 15:42:15 +0100 > Subject: [PATCH] ARM: dts: Add Peach Pit board WiFi support > > Peach boards have an SDIO WiFi module that is always powered but it > needs a power sequence involving the reset and enable pins and also > a 32kHz reference clock. > > Add a dev node for the SDIO slot and the MMC power sequence provider. > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > arch/arm/boot/dts/exynos5420-peach-pit.dts | 59 ++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts > index ec86d9523935..26df041e46e7 100644 > --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts > +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts > @@ -125,6 +125,14 @@ > }; > }; > }; > + > + mmc1_pwrseq: mmc1_pwrseq { > + compatible = "mmc-pwrseq-simple"; > + reset-gpios = <&gpx0 1 GPIO_ACTIVE_LOW>, /* WIFI_RSTn */ > + <&gpx0 0 GPIO_ACTIVE_LOW>; /* WIFI_EN */ Any chance you want "WIFI_EN" to actually be specified as "vmmc" for the slot? ...possibly with a 'regulator-enable-ramp-delay' specified? I know that with SD Cards on an rk3288 board I needed to make sure there was a bit of delay after "vqmmc" came up... BTW: IIRC the "WIFI_RSTn" ended up being totally unused. It was used in earlier revs of the board that had a different rev of the WiFi chip... > + clocks = <&max77802 MAX77802_CLK_32K_CP>; > + clock-names = "ext_clock"; > + }; > }; > > &adc { > @@ -691,6 +699,24 @@ > bus-width = <8>; > }; > > +&mmc_1 { > + status = "okay"; > + num-slots = <1>; > + broken-cd; > + cap-sdio-irq; > + card-detect-delay = <200>; > + clock-frequency = <400000000>; > + samsung,dw-mshc-ciu-div = <1>; > + samsung,dw-mshc-sdr-timing = <0 1>; Just for kicks, does this help if you do: ciu-div = 3 sdr-timing = 2 3 ...I know we have ciu-div = 1 downstream, but we also have tuning... > + samsung,dw-mshc-ddr-timing = <0 2>; > + pinctrl-names = "default"; > + pinctrl-0 = <&sd1_clk>, <&sd1_cmd>, <&sd1_int>, <&sd1_bus4>, > + <&sd1_bus8>, <&wifi_en>, <&wifi_rst>; > + bus-width = <4>; > + cap-sd-highspeed; > + mmc-pwrseq = <&mmc1_pwrseq>; Should you be specifying a "vqmmc-supply" here? I know that we don't change voltages for WiFi (starts at 1.8V and ends up at 1.8V) but still might be good to specify it... > +}; > + > &mmc_2 { > status = "okay"; > num-slots = <1>; > @@ -710,6 +736,20 @@ > pinctrl-names = "default"; > pinctrl-0 = <&mask_tpm_reset>; > > + wifi_en: wifi-en { > + samsung,pins = "gpx0-0"; > + samsung,pin-function = <1>; > + samsung,pin-pud = <0>; > + samsung,pin-drv = <0>; > + }; > + > + wifi_rst: wifi-rst { > + samsung,pins = "gpx0-1"; > + samsung,pin-function = <1>; > + samsung,pin-pud = <0>; > + samsung,pin-drv = <0>; > + }; > + > max98090_irq: max98090-irq { > samsung,pins = "gpx0-2"; > samsung,pin-function = <0>; > @@ -797,6 +837,25 @@ > }; > }; > > +&pinctrl_1 { > + /* Adjust WiFi drive strengths lower for EMI */ > + sd1_clk: sd1-clk { > + samsung,pin-drv = <2>; > + }; > + > + sd1_cmd: sd1-cmd { > + samsung,pin-drv = <2>; > + }; > + > + sd1_bus4: sd1-bus-width4 { > + samsung,pin-drv = <2>; > + }; > + > + sd1_bus8: sd1-bus-width8 { > + samsung,pin-drv = <2>; > + }; > +}; > + > &pinctrl_2 { > pmic_dvs_2: pmic-dvs-2 { > samsung,pins = "gpj4-2"; > -- > 2.1.3 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Doug, On 02/23/2015 08:45 PM, Doug Anderson wrote: > On Mon, Feb 23, 2015 at 8:33 AM, Javier Martinez Canillas > <javier.martinez@collabora.co.uk> wrote: >> >> Addy's "mmc: dw_mmc: fix bug that cause 'Timeout sending command" [0] patch >> reset the controller if it was busy for more than 500ms while your patch >> doesn't. I don't mean that your patch is not correct, I'm just mentioning >> because calling dw_mci_ctrl_reset() does makes the command to succeed the >> next time and I can see the SDIO devices enumerated in /sys/bus/sdio/devices: >> >> [ 5.892124] dwmmc_exynos 12210000.mmc: Busy; trying anyway >> [ 5.892135] mmc_host mmc2: Timeout sending command (cmd 0x202000 arg 0x0 status 0x0) >> [ 5.913885] mmc2: new high speed SDIO card at address 0001 >> [ 6.582133] dwmmc_exynos 12210000.mmc: Busy; trying anyway > > Hmmmmm. In the cases I was seeing I didn't need the "reset" since the > "SDMMC_CMD_UPD_CLK" was the one from dw_mci_set_ios() and my patch: > > * mmc: dw_mmc: Give a good reset after we give power > https://patchwork.kernel.org/patch/5858281/ > > ...gave the needed reset after vqmmc power was applied. Then dw_mmc > never got wedged and didn't need the reset to get it unwedged. In > your care you're getting called from dw_mci_init_card(). That should > happen _after_ dw_mci_set_ios() as far as I know. Can you put a > printout in the reset in dw_mci_set_ios() and make sure it runs? > Added a printout in dw_mci_set_ios() and I see that the card is reset at MMC_POWER_ON. So you are right that it gets wedged somehow between dw_mci_set_ios() and dw_mci_init_card(). This only happens when the slot is attached to a SDIO device though since the controller neither gets busy nor a command timeouts for the eMMC and uSD. > How many resets do you need before things work? If just one then > something must be happening between the initial "set_ios" and the The card is reseted 3 times to make it "work", that is to avoid being blocked constantly with "Timeout sending command" errors. But as I said before, even when the reset in dw_mci_wait_while_busy(), the firmware fails to load into the WiFi module so is not in the best state. > "init_card". Any chance you could figure out what that is? If it > needs multiple resets then it seems like something is fishy... > Sure, I'll investigate more what happens between dw_mci_set_ios() and dw_mci_init_card(). Thanks for the suggestion. > >> but that reset may not be right since the WiFi chip does not work because >> the firmware later fails to load: >> >> [ 240.273352] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. >> [ 240.281159] kworker/2:1 D c04b3340 0 1260 2 0x00000000 >> [ 240.287487] Workqueue: events request_firmware_work_func >> [ 240.292763] [<c04b3340>] (__schedule) from [<c04b36f0>] (schedule+0x34/0x98) >> [ 240.299815] [<c04b36f0>] (schedule) from [<c04b7198>] (schedule_timeout+0x120/0x16c) >> [ 240.307537] [<c04b7198>] (schedule_timeout) from [<c04b41b4>] (wait_for_common+0xb0/0x154) >> [ 240.315783] [<c04b41b4>] (wait_for_common) from [<c037abb8>] (mmc_wait_for_req+0xa0/0x140) >> [ 240.324020] [<c037abb8>] (mmc_wait_for_req) from [<c0384b04>] (mmc_io_rw_extended+0x304/0x34c) >> [ 240.332607] [<c0384b04>] (mmc_io_rw_extended) from [<c0385a90>] (sdio_io_rw_ext_helper+0x138/0x1a4) >> [ 240.341630] [<c0385a90>] (sdio_io_rw_ext_helper) from [<c0385b1c>] (sdio_writesb+0x20/0x28) >> [ 240.349962] [<c0385b1c>] (sdio_writesb) from [<c02e60d4>] (mwifiex_write_data_sync+0x4c/0x80) >> [ 240.358460] [<c02e60d4>] (mwifiex_write_data_sync) from [<c02e68e4>] (mwifiex_prog_fw_w_helper+0x1b0/0x2c4) >> [ 240.368176] [<c02e68e4>] (mwifiex_prog_fw_w_helper) from [<c02c7d6c>] (mwifiex_dnld_fw+0x58/0x10c) >> [ 240.377127] [<c02c7d6c>] (mwifiex_dnld_fw) from [<c02c5f10>] (mwifiex_fw_dpc+0x264/0x408) >> [ 240.385263] [<c02c5f10>] (mwifiex_fw_dpc) from [<c0291b28>] (request_firmware_work_func+0x30/0x50) >> [ 240.394200] [<c0291b28>] (request_firmware_work_func) from [<c0032ae8>] (process_one_work+0x120/0x324) >> [ 240.403482] [<c0032ae8>] (process_one_work) from [<c0032e58>] (worker_thread+0x138/0x464) >> [ 240.411635] [<c0032e58>] (worker_thread) from [<c0037470>] (kthread+0xd8/0xf4) >> [ 240.418837] [<c0037470>] (kthread) from [<c000e680>] (ret_from_fork+0x14/0x34) >> >> The DTS changes on top of linux-next to add WiFi support is [1] in case you can >> find something that is wrong with my patch. > > I still haven't had time to try using the new MMC power sequencing :( > so I can't say for sure, but one thought below... > > >> I also checked that the external reference clock for the WiFi module is enabled >> correctly according to /sys/kernel/debug/clk/clk_summary and also by looking >> at the value in the max77802 i2c register address that enables that clock. >> >> Any hints will be highly appreciated. >> >> Thanks a lot and best regards, >> Javier >> >> [0]: https://lkml.org/lkml/2015/2/5/222 >> [1]: >> From ced0974472d109019e1ee551a6dd08f16ae5cfc2 Mon Sep 17 00:00:00 2001 >> From: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> Date: Mon, 23 Feb 2014 15:42:15 +0100 >> Subject: [PATCH] ARM: dts: Add Peach Pit board WiFi support >> >> Peach boards have an SDIO WiFi module that is always powered but it >> needs a power sequence involving the reset and enable pins and also >> a 32kHz reference clock. >> >> Add a dev node for the SDIO slot and the MMC power sequence provider. >> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> >> --- >> arch/arm/boot/dts/exynos5420-peach-pit.dts | 59 ++++++++++++++++++++++++++++++ >> 1 file changed, 59 insertions(+) >> >> diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts >> index ec86d9523935..26df041e46e7 100644 >> --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts >> +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts >> @@ -125,6 +125,14 @@ >> }; >> }; >> }; >> + >> + mmc1_pwrseq: mmc1_pwrseq { >> + compatible = "mmc-pwrseq-simple"; >> + reset-gpios = <&gpx0 1 GPIO_ACTIVE_LOW>, /* WIFI_RSTn */ >> + <&gpx0 0 GPIO_ACTIVE_LOW>; /* WIFI_EN */ > > Any chance you want "WIFI_EN" to actually be specified as "vmmc" for > the slot? ...possibly with a 'regulator-enable-ramp-delay' specified? > I know that with SD Cards on an rk3288 board I needed to make sure > there was a bit of delay after "vqmmc" came up... > If that's the case then we would had a bug in the MMC power sequencing support but I don't think that is needed because there is a mmc_delay(10) in mmc_power_up() between mmc_pwrseq_pre_power_on() and mmc_set_ios(). But just in case, I tried removing the "reset-gpios" property and adding "WIFI_EN" as a fixed regulator with a "regulator-enable-ramp-delay" but the behavior is the same. > BTW: IIRC the "WIFI_RSTn" ended up being totally unused. It was used > in earlier revs of the board that had a different rev of the WiFi > chip... > You are right, I removed WIFI_RSTn and the SDIO devices are still enumerated. >> + clocks = <&max77802 MAX77802_CLK_32K_CP>; >> + clock-names = "ext_clock"; >> + }; >> }; >> >> &adc { >> @@ -691,6 +699,24 @@ >> bus-width = <8>; >> }; >> >> +&mmc_1 { >> + status = "okay"; >> + num-slots = <1>; >> + broken-cd; >> + cap-sdio-irq; >> + card-detect-delay = <200>; >> + clock-frequency = <400000000>; >> + samsung,dw-mshc-ciu-div = <1>; >> + samsung,dw-mshc-sdr-timing = <0 1>; > > Just for kicks, does this help if you do: > > ciu-div = 3 > sdr-timing = 2 3 > > ...I know we have ciu-div = 1 downstream, but we also have tuning... > This makes it even worst since with those values the boot hangs after a "Timeout sending command" error even with the reset in wait while busy. >> + samsung,dw-mshc-ddr-timing = <0 2>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&sd1_clk>, <&sd1_cmd>, <&sd1_int>, <&sd1_bus4>, >> + <&sd1_bus8>, <&wifi_en>, <&wifi_rst>; >> + bus-width = <4>; >> + cap-sd-highspeed; >> + mmc-pwrseq = <&mmc1_pwrseq>; > > Should you be specifying a "vqmmc-supply" here? I know that we don't > change voltages for WiFi (starts at 1.8V and ends up at 1.8V) but > still might be good to specify it... > Sure, I added a "vqmmc-supply = <&buck10_reg>" here. It does not have an effect though but is true that is better to specify it. Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Javier, On Tue, Feb 24, 2015 at 3:20 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: >> Hmmmmm. In the cases I was seeing I didn't need the "reset" since the >> "SDMMC_CMD_UPD_CLK" was the one from dw_mci_set_ios() and my patch: >> >> * mmc: dw_mmc: Give a good reset after we give power >> https://patchwork.kernel.org/patch/5858281/ >> >> ...gave the needed reset after vqmmc power was applied. Then dw_mmc >> never got wedged and didn't need the reset to get it unwedged. In >> your care you're getting called from dw_mci_init_card(). That should >> happen _after_ dw_mci_set_ios() as far as I know. Can you put a >> printout in the reset in dw_mci_set_ios() and make sure it runs? >> > > Added a printout in dw_mci_set_ios() and I see that the card is reset > at MMC_POWER_ON. So you are right that it gets wedged somehow between > dw_mci_set_ios() and dw_mci_init_card(). Actually, if your code needs 3 resets then maybe there's something else wrong... > This only happens when the slot is attached to a SDIO device though > since the controller neither gets busy nor a command timeouts for > the eMMC and uSD. It sounds like there's something else going on here. >> How many resets do you need before things work? If just one then >> something must be happening between the initial "set_ios" and the > > The card is reseted 3 times to make it "work", that is to avoid being > blocked constantly with "Timeout sending command" errors. But as I said > before, even when the reset in dw_mci_wait_while_busy(), the firmware > fails to load into the WiFi module so is not in the best state. 3 times? ...but that doesn't make a lot of sense to me. Is it simply the delay that makes it work, or do you actually need the 3 resets? Is anything else happening between all the resets? I guess you keep trying to send the command and resetting between? Hrmmm... Seems like there has got to be something else going on in this case... >> BTW: IIRC the "WIFI_RSTn" ended up being totally unused. It was used >> in earlier revs of the board that had a different rev of the WiFi >> chip... >> > > You are right, I removed WIFI_RSTn and the SDIO devices are still enumerated. I just checked. WIFI_RSTn goes to the 3G connector which (as far as I know) was never hooked up to anything. ...so yeah, you can safely leave that out. >>> +&mmc_1 { >>> + status = "okay"; >>> + num-slots = <1>; >>> + broken-cd; >>> + cap-sdio-irq; >>> + card-detect-delay = <200>; >>> + clock-frequency = <400000000>; >>> + samsung,dw-mshc-ciu-div = <1>; >>> + samsung,dw-mshc-sdr-timing = <0 1>; >> >> Just for kicks, does this help if you do: >> >> ciu-div = 3 >> sdr-timing = 2 3 >> >> ...I know we have ciu-div = 1 downstream, but we also have tuning... >> > > This makes it even worst since with those values the boot hangs after a > "Timeout sending command" error even with the reset in wait while busy. I don't have a pit or pi hooked up on my desk right now, but I will try to give it a shot soon and see what it ends up at. ...of course you might still be at 400kHz mode in which case the divs just have to not be totally wrong, I think... >>> + samsung,dw-mshc-ddr-timing = <0 2>; >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&sd1_clk>, <&sd1_cmd>, <&sd1_int>, <&sd1_bus4>, >>> + <&sd1_bus8>, <&wifi_en>, <&wifi_rst>; >>> + bus-width = <4>; >>> + cap-sd-highspeed; >>> + mmc-pwrseq = <&mmc1_pwrseq>; >> >> Should you be specifying a "vqmmc-supply" here? I know that we don't >> change voltages for WiFi (starts at 1.8V and ends up at 1.8V) but >> still might be good to specify it... >> > > Sure, I added a "vqmmc-supply = <&buck10_reg>" here. It does not have an > effect though but is true that is better to specify it. Yeah, makes sense. --- OK, so looking through things I _think_ I found another difference that _might_ matter... 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... I think the same bug is present in eMMC and SD but possibly the bootloader inits the pinctrl properly there? 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? -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/boot/dts/exynos5420-peach-pit.dts b/arch/arm/boot/dts/exynos5420-peach-pit.dts index ec86d9523935..26df041e46e7 100644 --- a/arch/arm/boot/dts/exynos5420-peach-pit.dts +++ b/arch/arm/boot/dts/exynos5420-peach-pit.dts @@ -125,6 +125,14 @@ }; }; }; + + mmc1_pwrseq: mmc1_pwrseq { + compatible = "mmc-pwrseq-simple"; + reset-gpios = <&gpx0 1 GPIO_ACTIVE_LOW>, /* WIFI_RSTn */ + <&gpx0 0 GPIO_ACTIVE_LOW>; /* WIFI_EN */ + clocks = <&max77802 MAX77802_CLK_32K_CP>; + clock-names = "ext_clock"; + }; }; &adc { @@ -691,6 +699,24 @@ bus-width = <8>; }; +&mmc_1 { + status = "okay"; + num-slots = <1>; + broken-cd; + cap-sdio-irq; + card-detect-delay = <200>; + clock-frequency = <400000000>; + samsung,dw-mshc-ciu-div = <1>; + samsung,dw-mshc-sdr-timing = <0 1>; + samsung,dw-mshc-ddr-timing = <0 2>; + pinctrl-names = "default"; + pinctrl-0 = <&sd1_clk>, <&sd1_cmd>, <&sd1_int>, <&sd1_bus4>, + <&sd1_bus8>, <&wifi_en>, <&wifi_rst>; + bus-width = <4>; + cap-sd-highspeed; + mmc-pwrseq = <&mmc1_pwrseq>; +}; + &mmc_2 { status = "okay"; num-slots = <1>; @@ -710,6 +736,20 @@ pinctrl-names = "default"; pinctrl-0 = <&mask_tpm_reset>; + wifi_en: wifi-en { + samsung,pins = "gpx0-0"; + samsung,pin-function = <1>; + samsung,pin-pud = <0>; + samsung,pin-drv = <0>; + }; + + wifi_rst: wifi-rst { + samsung,pins = "gpx0-1"; + samsung,pin-function = <1>; + samsung,pin-pud = <0>; + samsung,pin-drv = <0>; + }; + max98090_irq: max98090-irq { samsung,pins = "gpx0-2"; samsung,pin-function = <0>; @@ -797,6 +837,25 @@ }; }; +&pinctrl_1 { + /* Adjust WiFi drive strengths lower for EMI */ + sd1_clk: sd1-clk { + samsung,pin-drv = <2>; + }; + + sd1_cmd: sd1-cmd { + samsung,pin-drv = <2>; + }; + + sd1_bus4: sd1-bus-width4 { + samsung,pin-drv = <2>; + }; + + sd1_bus8: sd1-bus-width8 { + samsung,pin-drv = <2>; + }; +}; + &pinctrl_2 { pmic_dvs_2: pmic-dvs-2 { samsung,pins = "gpj4-2";