| Message ID | 20190222184708.32004-1-beagleboard@davidjohnsummers.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3,v2] ARM: dts: rk3288-tinker.dtsi: Fix SD card detection | expand |
On 2019-02-22 19:47, David Summers wrote: > The Problem: > > On ASUS Tinker Board S, when booting from the eMMC, and there is no > card sd slot, then there are constant errors. > > Cause: > > Thanks must go to Robin Murphy @ ARM for idenifying the problem. The > rk808 on the Tinker Board and Tinker Board S has many regulators, one > vccio_sd powers the IO for the sd card. Unfortunatly this is also used > in the card detect. Now when no card is install, the regulator is > powered down. This means that the card detect floats, and this means > random card detection. > > The Solution: > > Make sure that the sd IO is always powered, this means card detection > is always active, which is what should be done on a board with an sd > slot, which both the Tinker Board and Tinker Board S are. Hence change > is made to the .dtsi which takes effect on all Tinker Boards as > required. > > The change also adds "regulator-boot-on" the Tinker Board boot from > uboot, and the sd card is always one option. Hence the IO must be > powered in uboot, and so setting this flag. > > Also removed is "disable-wp" the micro sd card which are used have no > write protection, so the concept doesn't mean anything, and the > Tinker Boards work without this. Hence it is removed to simply. > > This change came from ArchLinux Arm, but we note it is the fix also > used by Armbian: > > https://github.com/Miouyouyou/RockMyy/blob/master/patches/kernel/v5.0/DTS/0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch > > Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> > --- > arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi > index aa107ee41b8b..6b7e55085b0c 100644 > --- a/arch/arm/boot/dts/rk3288-tinker.dtsi > +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi > @@ -254,6 +254,8 @@ > }; > > vccio_sd: LDO_REG5 { > + regulator-always-on; > + regulator-boot-on; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <3300000>; > regulator-name = "vccio_sd"; > @@ -431,7 +433,6 @@ > cap-mmc-highspeed; > cap-sd-highspeed; > card-detect-delay = <200>; > - disable-wp; /* wp not hooked up */ I think disable-wp correctly describes that wp is not expected to work and should not be removed. From comment in mmc_sd_get_ro(): "Some systems don't feature a write-protect pin and don't need one. E.g. because they only have micro-SD card slot. For those systems assume that the SD card is always read-write." Without disable-wp core will call dw_mci_get_ro() to get wp status. This patch also fixes reboot when booting from sd-card and having emmc zeroed out. Tested-by: Jonas Karlman <jonas@kwiboo.se> Regards, Jonas > pinctrl-names = "default"; > pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; > status = "okay";
On 24/02/2019 19:26, Jonas Karlman wrote: > On 2019-02-22 19:47, David Summers wrote: >> The Problem: >> >> On ASUS Tinker Board S, when booting from the eMMC, and there is no >> card sd slot, then there are constant errors. >> >> Cause: >> >> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The >> rk808 on the Tinker Board and Tinker Board S has many regulators, one >> vccio_sd powers the IO for the sd card. Unfortunatly this is also used >> in the card detect. Now when no card is install, the regulator is >> powered down. This means that the card detect floats, and this means >> random card detection. >> >> The Solution: >> >> Make sure that the sd IO is always powered, this means card detection >> is always active, which is what should be done on a board with an sd >> slot, which both the Tinker Board and Tinker Board S are. Hence change >> is made to the .dtsi which takes effect on all Tinker Boards as >> required. >> >> The change also adds "regulator-boot-on" the Tinker Board boot from >> uboot, and the sd card is always one option. Hence the IO must be >> powered in uboot, and so setting this flag. >> >> Also removed is "disable-wp" the micro sd card which are used have no >> write protection, so the concept doesn't mean anything, and the >> Tinker Boards work without this. Hence it is removed to simply. >> >> This change came from ArchLinux Arm, but we note it is the fix also >> used by Armbian: >> >> https://github.com/Miouyouyou/RockMyy/blob/master/patches/kernel/v5.0/DTS/0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch >> >> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> >> --- >> arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi >> index aa107ee41b8b..6b7e55085b0c 100644 >> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi >> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi >> @@ -254,6 +254,8 @@ >> }; >> >> vccio_sd: LDO_REG5 { >> + regulator-always-on; >> + regulator-boot-on; >> regulator-min-microvolt = <1800000>; >> regulator-max-microvolt = <3300000>; >> regulator-name = "vccio_sd"; >> @@ -431,7 +433,6 @@ >> cap-mmc-highspeed; >> cap-sd-highspeed; >> card-detect-delay = <200>; >> - disable-wp; /* wp not hooked up */ > I think disable-wp correctly describes that wp is not expected to work and should not be removed. > > From comment in mmc_sd_get_ro(): > "Some systems don't feature a write-protect pin and don't need one. > E.g. because they only have micro-SD card slot. For those systems > assume that the SD card is always read-write." > > Without disable-wp core will call dw_mci_get_ro() to get wp status. > > > This patch also fixes reboot when booting from sd-card and having emmc zeroed out. > > Tested-by: Jonas Karlman <jonas@kwiboo.se> > > Regards, > Jonas > >> pinctrl-names = "default"; >> pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; >> status = "okay"; Hi Jonas, Thanks for testing - did you get an error with "disable-wp" ? We tested this with @TheSaint On ArchLinux Arm, and it worked! With the "regulator-boot-on" and on reboot, it should keep power on the sd card - so reboot should work. Regards, David.
On 2019-02-24 20:35, David Summers wrote: > On 24/02/2019 19:26, Jonas Karlman wrote: >> On 2019-02-22 19:47, David Summers wrote: >>> The Problem: >>> >>> On ASUS Tinker Board S, when booting from the eMMC, and there is no >>> card sd slot, then there are constant errors. >>> >>> Cause: >>> >>> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The >>> rk808 on the Tinker Board and Tinker Board S has many regulators, one >>> vccio_sd powers the IO for the sd card. Unfortunatly this is also used >>> in the card detect. Now when no card is install, the regulator is >>> powered down. This means that the card detect floats, and this means >>> random card detection. >>> >>> The Solution: >>> >>> Make sure that the sd IO is always powered, this means card detection >>> is always active, which is what should be done on a board with an sd >>> slot, which both the Tinker Board and Tinker Board S are. Hence change >>> is made to the .dtsi which takes effect on all Tinker Boards as >>> required. >>> >>> The change also adds "regulator-boot-on" the Tinker Board boot from >>> uboot, and the sd card is always one option. Hence the IO must be >>> powered in uboot, and so setting this flag. >>> >>> Also removed is "disable-wp" the micro sd card which are used have no >>> write protection, so the concept doesn't mean anything, and the >>> Tinker Boards work without this. Hence it is removed to simply. >>> >>> This change came from ArchLinux Arm, but we note it is the fix also >>> used by Armbian: >>> >>> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMiouyouyou%2FRockMyy%2Fblob%2Fmaster%2Fpatches%2Fkernel%2Fv5.0%2FDTS%2F0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch&data=02%7C01%7C%7Cf5937082939e4163c84b08d69a8f3b61%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636866337295767294&sdata=tK8QE3bsG9LW%2FJcvFzLKa8%2BPj5u%2F8exbEn8m2vqKly0%3D&reserved=0 >>> >>> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> >>> --- >>> arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi >>> index aa107ee41b8b..6b7e55085b0c 100644 >>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi >>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi >>> @@ -254,6 +254,8 @@ >>> }; >>> >>> vccio_sd: LDO_REG5 { >>> + regulator-always-on; >>> + regulator-boot-on; >>> regulator-min-microvolt = <1800000>; >>> regulator-max-microvolt = <3300000>; >>> regulator-name = "vccio_sd"; >>> @@ -431,7 +433,6 @@ >>> cap-mmc-highspeed; >>> cap-sd-highspeed; >>> card-detect-delay = <200>; >>> - disable-wp; /* wp not hooked up */ >> I think disable-wp correctly describes that wp is not expected to work and should not be removed. >> >> From comment in mmc_sd_get_ro(): >> "Some systems don't feature a write-protect pin and don't need one. >> E.g. because they only have micro-SD card slot. For those systems >> assume that the SD card is always read-write." >> >> Without disable-wp core will call dw_mci_get_ro() to get wp status. >> >> >> This patch also fixes reboot when booting from sd-card and having emmc zeroed out. >> >> Tested-by: Jonas Karlman <jonas@kwiboo.se> >> >> Regards, >> Jonas >> >>> pinctrl-names = "default"; >>> pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; >>> status = "okay"; > Hi Jonas, > > Thanks for testing - did you get an error with "disable-wp" ? > > We tested this with @TheSaint On ArchLinux Arm, and it worked! > > With the "regulator-boot-on" and on reboot, it should keep power on the > sd card - so reboot should work. I tested both with and without "disable-wp" removed and both works without error. SD-card reboots now works as it should thanks to "regulator-always-on" and "regulator-boot-on", I only tested using both. On a side note I have also been testing [1] to make reboot with UHS signal voltage work and will send out a RFC v2 after I have concluded testing on my other devices supporting UHS signal voltage. [1] https://github.com/Kwiboo/linux-rockchip/compare/patch-rk-5.x-tinker-uhs%5E%5E%5E...patch-rk-5.x-tinker-uhs Regards, Jonas > > Regards, > > David. > >
On 24/02/2019 19:56, Jonas Karlman wrote: > On 2019-02-24 20:35, David Summers wrote: >> On 24/02/2019 19:26, Jonas Karlman wrote: >>> On 2019-02-22 19:47, David Summers wrote: >>>> The Problem: >>>> >>>> On ASUS Tinker Board S, when booting from the eMMC, and there is no >>>> card sd slot, then there are constant errors. >>>> >>>> Cause: >>>> >>>> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The >>>> rk808 on the Tinker Board and Tinker Board S has many regulators, one >>>> vccio_sd powers the IO for the sd card. Unfortunatly this is also used >>>> in the card detect. Now when no card is install, the regulator is >>>> powered down. This means that the card detect floats, and this means >>>> random card detection. >>>> >>>> The Solution: >>>> >>>> Make sure that the sd IO is always powered, this means card detection >>>> is always active, which is what should be done on a board with an sd >>>> slot, which both the Tinker Board and Tinker Board S are. Hence change >>>> is made to the .dtsi which takes effect on all Tinker Boards as >>>> required. >>>> >>>> The change also adds "regulator-boot-on" the Tinker Board boot from >>>> uboot, and the sd card is always one option. Hence the IO must be >>>> powered in uboot, and so setting this flag. >>>> >>>> Also removed is "disable-wp" the micro sd card which are used have no >>>> write protection, so the concept doesn't mean anything, and the >>>> Tinker Boards work without this. Hence it is removed to simply. >>>> >>>> This change came from ArchLinux Arm, but we note it is the fix also >>>> used by Armbian: >>>> >>>> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMiouyouyou%2FRockMyy%2Fblob%2Fmaster%2Fpatches%2Fkernel%2Fv5.0%2FDTS%2F0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch&data=02%7C01%7C%7Cf5937082939e4163c84b08d69a8f3b61%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636866337295767294&sdata=tK8QE3bsG9LW%2FJcvFzLKa8%2BPj5u%2F8exbEn8m2vqKly0%3D&reserved=0 >>>> >>>> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> >>>> --- >>>> arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi >>>> index aa107ee41b8b..6b7e55085b0c 100644 >>>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi >>>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi >>>> @@ -254,6 +254,8 @@ >>>> }; >>>> >>>> vccio_sd: LDO_REG5 { >>>> + regulator-always-on; >>>> + regulator-boot-on; >>>> regulator-min-microvolt = <1800000>; >>>> regulator-max-microvolt = <3300000>; >>>> regulator-name = "vccio_sd"; >>>> @@ -431,7 +433,6 @@ >>>> cap-mmc-highspeed; >>>> cap-sd-highspeed; >>>> card-detect-delay = <200>; >>>> - disable-wp; /* wp not hooked up */ >>> I think disable-wp correctly describes that wp is not expected to work and should not be removed. >>> >>> From comment in mmc_sd_get_ro(): >>> "Some systems don't feature a write-protect pin and don't need one. >>> E.g. because they only have micro-SD card slot. For those systems >>> assume that the SD card is always read-write." >>> >>> Without disable-wp core will call dw_mci_get_ro() to get wp status. >>> >>> >>> This patch also fixes reboot when booting from sd-card and having emmc zeroed out. >>> >>> Tested-by: Jonas Karlman <jonas@kwiboo.se> >>> >>> Regards, >>> Jonas >>> >>>> pinctrl-names = "default"; >>>> pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; >>>> status = "okay"; >> Hi Jonas, >> >> Thanks for testing - did you get an error with "disable-wp" ? >> >> We tested this with @TheSaint On ArchLinux Arm, and it worked! >> >> With the "regulator-boot-on" and on reboot, it should keep power on the >> sd card - so reboot should work. > I tested both with and without "disable-wp" removed and both works without error. > SD-card reboots now works as it should thanks to "regulator-always-on" and "regulator-boot-on", I only tested using both. > > On a side note I have also been testing [1] to make reboot with UHS signal voltage work and will send out a RFC v2 > after I have concluded testing on my other devices supporting UHS signal voltage. > > [1] https://github.com/Kwiboo/linux-rockchip/compare/patch-rk-5.x-tinker-uhs%5E%5E%5E...patch-rk-5.x-tinker-uhs > > Regards, > Jonas Thanks. Glad your are testing - and I hope the uhs tests work. Alas for us to test uhs - and our user on Arch isn't so experienced. I could push him to try different speeds - but to be honest I expect it to be beyond his comfort zone. But its good to here you confirm "disable-wp" works. To my mind it says we are sure that all three changes in the patch are are correct. Hopefully this will be straight forward to Heiko to accept. Its the simple patch of the 3 to accept ;) Regards, David. >> Regards, >> >> David. >> >>
Am Sonntag, 24. Februar 2019, 21:10:50 CET schrieb David Summers: > On 24/02/2019 19:56, Jonas Karlman wrote: > > On 2019-02-24 20:35, David Summers wrote: > >> On 24/02/2019 19:26, Jonas Karlman wrote: > >>> On 2019-02-22 19:47, David Summers wrote: > >>>> The Problem: > >>>> > >>>> On ASUS Tinker Board S, when booting from the eMMC, and there is no > >>>> card sd slot, then there are constant errors. > >>>> > >>>> Cause: > >>>> > >>>> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The > >>>> rk808 on the Tinker Board and Tinker Board S has many regulators, one > >>>> vccio_sd powers the IO for the sd card. Unfortunatly this is also used > >>>> in the card detect. Now when no card is install, the regulator is > >>>> powered down. This means that the card detect floats, and this means > >>>> random card detection. > >>>> > >>>> The Solution: > >>>> > >>>> Make sure that the sd IO is always powered, this means card detection > >>>> is always active, which is what should be done on a board with an sd > >>>> slot, which both the Tinker Board and Tinker Board S are. Hence change > >>>> is made to the .dtsi which takes effect on all Tinker Boards as > >>>> required. > >>>> > >>>> The change also adds "regulator-boot-on" the Tinker Board boot from > >>>> uboot, and the sd card is always one option. Hence the IO must be > >>>> powered in uboot, and so setting this flag. > >>>> > >>>> Also removed is "disable-wp" the micro sd card which are used have no > >>>> write protection, so the concept doesn't mean anything, and the > >>>> Tinker Boards work without this. Hence it is removed to simply. > >>>> > >>>> This change came from ArchLinux Arm, but we note it is the fix also > >>>> used by Armbian: > >>>> > >>>> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu > >>>> b.com%2FMiouyouyou%2FRockMyy%2Fblob%2Fmaster%2Fpatches%2Fkernel%2Fv5.0% > >>>> 2FDTS%2F0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch& > >>>> amp;data=02%7C01%7C%7Cf5937082939e4163c84b08d69a8f3b61%7C84df9e7fe9f640 > >>>> afb435aaaaaaaaaaaa%7C1%7C0%7C636866337295767294&sdata=tK8QE3bsG9LW% > >>>> 2FJcvFzLKa8%2BPj5u%2F8exbEn8m2vqKly0%3D&reserved=0 > >>>> > >>>> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> > >>>> --- > >>>> > >>>> arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi > >>>> b/arch/arm/boot/dts/rk3288-tinker.dtsi index > >>>> aa107ee41b8b..6b7e55085b0c 100644 > >>>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi > >>>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi > >>>> @@ -254,6 +254,8 @@ > >>>> > >>>> }; > >>>> > >>>> vccio_sd: LDO_REG5 { > >>>> > >>>> + regulator-always-on; > >>>> + regulator-boot-on; > >>>> > >>>> regulator-min-microvolt = <1800000>; > >>>> regulator-max-microvolt = <3300000>; > >>>> regulator-name = "vccio_sd"; > >>>> > >>>> @@ -431,7 +433,6 @@ > >>>> > >>>> cap-mmc-highspeed; > >>>> cap-sd-highspeed; > >>>> card-detect-delay = <200>; > >>>> > >>>> - disable-wp; /* wp not hooked up */ > >>> > >>> I think disable-wp correctly describes that wp is not expected to work > >>> and should not be removed.>>> > >>> From comment in mmc_sd_get_ro(): > >>> "Some systems don't feature a write-protect pin and don't need one. > >>> E.g. because they only have micro-SD card slot. For those systems > >>> assume that the SD card is always read-write." > >>> > >>> Without disable-wp core will call dw_mci_get_ro() to get wp status. > >>> > >>> > >>> This patch also fixes reboot when booting from sd-card and having emmc > >>> zeroed out. > >>> > >>> Tested-by: Jonas Karlman <jonas@kwiboo.se> > >>> > >>> Regards, > >>> Jonas > >>> > >>>> pinctrl-names = "default"; > >>>> pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; > >>>> status = "okay"; > >> > >> Hi Jonas, > >> > >> Thanks for testing - did you get an error with "disable-wp" ? > >> > >> We tested this with @TheSaint On ArchLinux Arm, and it worked! > >> > >> With the "regulator-boot-on" and on reboot, it should keep power on the > >> sd card - so reboot should work. > > > > I tested both with and without "disable-wp" removed and both works without > > error. SD-card reboots now works as it should thanks to > > "regulator-always-on" and "regulator-boot-on", I only tested using both. > > > > On a side note I have also been testing [1] to make reboot with UHS signal > > voltage work and will send out a RFC v2 after I have concluded testing on > > my other devices supporting UHS signal voltage. > > > > [1] > > https://github.com/Kwiboo/linux-rockchip/compare/patch-rk-5.x-tinker-uhs% > > 5E%5E%5E...patch-rk-5.x-tinker-uhs > > > > Regards, > > Jonas > > Thanks. > > Glad your are testing - and I hope the uhs tests work. > > Alas for us to test uhs - and our user on Arch isn't so experienced. I > could push him to try different speeds - but to be honest I expect it to > be beyond his comfort zone. > > But its good to here you confirm "disable-wp" works. To my mind it says > we are sure that all three changes in the patch are are correct. > > Hopefully this will be straight forward to Heiko to accept. Its the > simple patch of the 3 to accept ;) From Jonas' explanation, I'd guess we want disable-wp to stay in the node, and not remove it as it tells the system, that "there is no write protect status available" hence it doesn't need to check for it. Heiko
On 2019-02-24 23:54, Heiko Stübner wrote: > Am Sonntag, 24. Februar 2019, 21:10:50 CET schrieb David Summers: >> On 24/02/2019 19:56, Jonas Karlman wrote: >>> On 2019-02-24 20:35, David Summers wrote: >>>> On 24/02/2019 19:26, Jonas Karlman wrote: >>>>> On 2019-02-22 19:47, David Summers wrote: >>>>>> The Problem: >>>>>> >>>>>> On ASUS Tinker Board S, when booting from the eMMC, and there is no >>>>>> card sd slot, then there are constant errors. >>>>>> >>>>>> Cause: >>>>>> >>>>>> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The >>>>>> rk808 on the Tinker Board and Tinker Board S has many regulators, one >>>>>> vccio_sd powers the IO for the sd card. Unfortunatly this is also used >>>>>> in the card detect. Now when no card is install, the regulator is >>>>>> powered down. This means that the card detect floats, and this means >>>>>> random card detection. >>>>>> >>>>>> The Solution: >>>>>> >>>>>> Make sure that the sd IO is always powered, this means card detection >>>>>> is always active, which is what should be done on a board with an sd >>>>>> slot, which both the Tinker Board and Tinker Board S are. Hence change >>>>>> is made to the .dtsi which takes effect on all Tinker Boards as >>>>>> required. >>>>>> >>>>>> The change also adds "regulator-boot-on" the Tinker Board boot from >>>>>> uboot, and the sd card is always one option. Hence the IO must be >>>>>> powered in uboot, and so setting this flag. >>>>>> >>>>>> Also removed is "disable-wp" the micro sd card which are used have no >>>>>> write protection, so the concept doesn't mean anything, and the >>>>>> Tinker Boards work without this. Hence it is removed to simply. >>>>>> >>>>>> This change came from ArchLinux Arm, but we note it is the fix also >>>>>> used by Armbian: >>>>>> >>>>>> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu >>>>>> b.com%2FMiouyouyou%2FRockMyy%2Fblob%2Fmaster%2Fpatches%2Fkernel%2Fv5.0% >>>>>> 2FDTS%2F0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch& >>>>>> amp;data=02%7C01%7C%7Cf5937082939e4163c84b08d69a8f3b61%7C84df9e7fe9f640 >>>>>> afb435aaaaaaaaaaaa%7C1%7C0%7C636866337295767294&sdata=tK8QE3bsG9LW% >>>>>> 2FJcvFzLKa8%2BPj5u%2F8exbEn8m2vqKly0%3D&reserved=0 >>>>>> >>>>>> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> >>>>>> --- >>>>>> >>>>>> arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi >>>>>> b/arch/arm/boot/dts/rk3288-tinker.dtsi index >>>>>> aa107ee41b8b..6b7e55085b0c 100644 >>>>>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi >>>>>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi >>>>>> @@ -254,6 +254,8 @@ >>>>>> >>>>>> }; >>>>>> >>>>>> vccio_sd: LDO_REG5 { >>>>>> >>>>>> + regulator-always-on; >>>>>> + regulator-boot-on; >>>>>> >>>>>> regulator-min-microvolt = <1800000>; >>>>>> regulator-max-microvolt = <3300000>; >>>>>> regulator-name = "vccio_sd"; >>>>>> >>>>>> @@ -431,7 +433,6 @@ >>>>>> >>>>>> cap-mmc-highspeed; >>>>>> cap-sd-highspeed; >>>>>> card-detect-delay = <200>; >>>>>> >>>>>> - disable-wp; /* wp not hooked up */ >>>>> I think disable-wp correctly describes that wp is not expected to work >>>>> and should not be removed.>>> >>>>> From comment in mmc_sd_get_ro(): >>>>> "Some systems don't feature a write-protect pin and don't need one. >>>>> E.g. because they only have micro-SD card slot. For those systems >>>>> assume that the SD card is always read-write." >>>>> >>>>> Without disable-wp core will call dw_mci_get_ro() to get wp status. >>>>> >>>>> >>>>> This patch also fixes reboot when booting from sd-card and having emmc >>>>> zeroed out. >>>>> >>>>> Tested-by: Jonas Karlman <jonas@kwiboo.se> >>>>> >>>>> Regards, >>>>> Jonas >>>>> >>>>>> pinctrl-names = "default"; >>>>>> pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; >>>>>> status = "okay"; >>>> Hi Jonas, >>>> >>>> Thanks for testing - did you get an error with "disable-wp" ? >>>> >>>> We tested this with @TheSaint On ArchLinux Arm, and it worked! >>>> >>>> With the "regulator-boot-on" and on reboot, it should keep power on the >>>> sd card - so reboot should work. >>> I tested both with and without "disable-wp" removed and both works without >>> error. SD-card reboots now works as it should thanks to >>> "regulator-always-on" and "regulator-boot-on", I only tested using both. >>> >>> On a side note I have also been testing [1] to make reboot with UHS signal >>> voltage work and will send out a RFC v2 after I have concluded testing on >>> my other devices supporting UHS signal voltage. >>> >>> [1] >>> https://github.com/Kwiboo/linux-rockchip/compare/patch-rk-5.x-tinker-uhs% >>> 5E%5E%5E...patch-rk-5.x-tinker-uhs >>> >>> Regards, >>> Jonas >> Thanks. >> >> Glad your are testing - and I hope the uhs tests work. >> >> Alas for us to test uhs - and our user on Arch isn't so experienced. I >> could push him to try different speeds - but to be honest I expect it to >> be beyond his comfort zone. >> >> But its good to here you confirm "disable-wp" works. To my mind it says >> we are sure that all three changes in the patch are are correct. >> >> Hopefully this will be straight forward to Heiko to accept. Its the >> simple patch of the 3 to accept ;) > From Jonas' explanation, I'd guess we want disable-wp to stay in the node, > and not remove it as it tells the system, that "there is no write protect > status available" hence it doesn't need to check for it. Correct, I may have been unclear ealier but I think that the disable-wp should stay as it correctly describes that the board has a microSD slot that do not support write protection. Regards, Jonas > > > Heiko > >
Hi, On Fri, Feb 22, 2019 at 10:48 AM David Summers <beagleboard@davidjohnsummers.uk> wrote: > > The Problem: > > On ASUS Tinker Board S, when booting from the eMMC, and there is no > card sd slot, then there are constant errors. > > Cause: > > Thanks must go to Robin Murphy @ ARM for idenifying the problem. The > rk808 on the Tinker Board and Tinker Board S has many regulators, one > vccio_sd powers the IO for the sd card. Unfortunatly this is also used > in the card detect. Now when no card is install, the regulator is > powered down. This means that the card detect floats, and this means > random card detection. Yeah, this is broken on a lot of SoCs that use dw_mmc. :( Really the card detect line needs to be on a different rail and this is why all boards I've worked on recently have a the card detect going to a GPIO instead of the dw_mmc CD. IIRC Rockchip moved the Card Detect to a different rail on newer SoCs (like rk3399) but we still used a GPIO even there since we didn't like the default/automatic muxing of JTAG and SD signals. The one board I was involved in that did it wrong (where we discovered this issue) was exynos5250-snow. You can see some discussion about the issue at: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282474.html In that discussion I'm pretty sure that Ulf suggested that a better way to go was to use something like "broken-cd" which I think was supposed to switch us to use polling. AKA periodically the SD card would be powered on and we'd check for a card, then we'd power everything off. ...but that was never implemented for snow at least so there may be something more than just adding the property. You can read through the whole thread for more details. IIRC leaving the IO rail always on like you're proposing can also work OK but there may be some corner cases, especially if you are trying to reach UHS speeds and/or if the bootloader ever tries to use UHS speeds. It's almost certainly busted if the bootloader did UHS since it will leave the line at ~1.8 V and the kernel will expect it to be at ~3.3 V. ...but maybe you rely on the bootloader not doing UHS and maybe things are generally OK if not? There may also be cases where you can't properly power down / reset a card because the card may be drawing power through the IO lines when you power off its main lines. That's not good for the card and can also put it in a bad state. I haven't done all the research here so this may be a bit of FUD--it's just a vague recollection from many years ago. ...so to make a long story short, a better solution is to allow the IO lines to be powered off but then poll for the card periodically. > The Solution: > > Make sure that the sd IO is always powered, this means card detection > is always active, which is what should be done on a board with an sd > slot, which both the Tinker Board and Tinker Board S are. Hence change > is made to the .dtsi which takes effect on all Tinker Boards as > required. > > The change also adds "regulator-boot-on" the Tinker Board boot from > uboot, and the sd card is always one option. Hence the IO must be > powered in uboot, and so setting this flag. > > Also removed is "disable-wp" the micro sd card which are used have no > write protection, so the concept doesn't mean anything, and the > Tinker Boards work without this. Hence it is removed to simply. As others have said, please leave disable-wp. There's no way for the kernel to know if you have a SD or uSD slot and the only difference between the two (electrically) is that there's no write protect for micro SD. Also: please CC dw_mmc people on future patches in this area. $ ./scripts/get_maintainer.pl -f drivers/mmc/host/dw_mmc.c Jaehoon Chung <jh80.chung@samsung.com> (maintainer:SYNOPSYS DESIGNWARE MMC/SD/SDIO DRIVER) Ulf Hansson <ulf.hansson@linaro.org> (maintainer:MULTIMEDIA CARD (MMC), SECURE DIGITAL (SD) AND...) linux-mmc@vger.kernel.org (open list:SYNOPSYS DESIGNWARE MMC/SD/SDIO DRIVER) -Doug
On 24/02/2019 22:54, Heiko Stübner wrote: > Am Sonntag, 24. Februar 2019, 21:10:50 CET schrieb David Summers: >> On 24/02/2019 19:56, Jonas Karlman wrote: >>> On 2019-02-24 20:35, David Summers wrote: >>>> On 24/02/2019 19:26, Jonas Karlman wrote: >>>>> On 2019-02-22 19:47, David Summers wrote: >>>>>> The Problem: >>>>>> >>>>>> On ASUS Tinker Board S, when booting from the eMMC, and there is no >>>>>> card sd slot, then there are constant errors. >>>>>> >>>>>> Cause: >>>>>> >>>>>> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The >>>>>> rk808 on the Tinker Board and Tinker Board S has many regulators, one >>>>>> vccio_sd powers the IO for the sd card. Unfortunatly this is also used >>>>>> in the card detect. Now when no card is install, the regulator is >>>>>> powered down. This means that the card detect floats, and this means >>>>>> random card detection. >>>>>> >>>>>> The Solution: >>>>>> >>>>>> Make sure that the sd IO is always powered, this means card detection >>>>>> is always active, which is what should be done on a board with an sd >>>>>> slot, which both the Tinker Board and Tinker Board S are. Hence change >>>>>> is made to the .dtsi which takes effect on all Tinker Boards as >>>>>> required. >>>>>> >>>>>> The change also adds "regulator-boot-on" the Tinker Board boot from >>>>>> uboot, and the sd card is always one option. Hence the IO must be >>>>>> powered in uboot, and so setting this flag. >>>>>> >>>>>> Also removed is "disable-wp" the micro sd card which are used have no >>>>>> write protection, so the concept doesn't mean anything, and the >>>>>> Tinker Boards work without this. Hence it is removed to simply. >>>>>> >>>>>> This change came from ArchLinux Arm, but we note it is the fix also >>>>>> used by Armbian: >>>>>> >>>>>> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithu >>>>>> b.com%2FMiouyouyou%2FRockMyy%2Fblob%2Fmaster%2Fpatches%2Fkernel%2Fv5.0% >>>>>> 2FDTS%2F0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch& >>>>>> amp;data=02%7C01%7C%7Cf5937082939e4163c84b08d69a8f3b61%7C84df9e7fe9f640 >>>>>> afb435aaaaaaaaaaaa%7C1%7C0%7C636866337295767294&sdata=tK8QE3bsG9LW% >>>>>> 2FJcvFzLKa8%2BPj5u%2F8exbEn8m2vqKly0%3D&reserved=0 >>>>>> >>>>>> Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> >>>>>> --- >>>>>> >>>>>> arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi >>>>>> b/arch/arm/boot/dts/rk3288-tinker.dtsi index >>>>>> aa107ee41b8b..6b7e55085b0c 100644 >>>>>> --- a/arch/arm/boot/dts/rk3288-tinker.dtsi >>>>>> +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi >>>>>> @@ -254,6 +254,8 @@ >>>>>> >>>>>> }; >>>>>> >>>>>> vccio_sd: LDO_REG5 { >>>>>> >>>>>> + regulator-always-on; >>>>>> + regulator-boot-on; >>>>>> >>>>>> regulator-min-microvolt = <1800000>; >>>>>> regulator-max-microvolt = <3300000>; >>>>>> regulator-name = "vccio_sd"; >>>>>> >>>>>> @@ -431,7 +433,6 @@ >>>>>> >>>>>> cap-mmc-highspeed; >>>>>> cap-sd-highspeed; >>>>>> card-detect-delay = <200>; >>>>>> >>>>>> - disable-wp; /* wp not hooked up */ >>>>> I think disable-wp correctly describes that wp is not expected to work >>>>> and should not be removed.>>> >>>>> From comment in mmc_sd_get_ro(): >>>>> "Some systems don't feature a write-protect pin and don't need one. >>>>> E.g. because they only have micro-SD card slot. For those systems >>>>> assume that the SD card is always read-write." >>>>> >>>>> Without disable-wp core will call dw_mci_get_ro() to get wp status. >>>>> >>>>> >>>>> This patch also fixes reboot when booting from sd-card and having emmc >>>>> zeroed out. >>>>> >>>>> Tested-by: Jonas Karlman <jonas@kwiboo.se> >>>>> >>>>> Regards, >>>>> Jonas >>>>> >>>>>> pinctrl-names = "default"; >>>>>> pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; >>>>>> status = "okay"; >>>> Hi Jonas, >>>> >>>> Thanks for testing - did you get an error with "disable-wp" ? >>>> >>>> We tested this with @TheSaint On ArchLinux Arm, and it worked! >>>> >>>> With the "regulator-boot-on" and on reboot, it should keep power on the >>>> sd card - so reboot should work. >>> I tested both with and without "disable-wp" removed and both works without >>> error. SD-card reboots now works as it should thanks to >>> "regulator-always-on" and "regulator-boot-on", I only tested using both. >>> >>> On a side note I have also been testing [1] to make reboot with UHS signal >>> voltage work and will send out a RFC v2 after I have concluded testing on >>> my other devices supporting UHS signal voltage. >>> >>> [1] >>> https://github.com/Kwiboo/linux-rockchip/compare/patch-rk-5.x-tinker-uhs% >>> 5E%5E%5E...patch-rk-5.x-tinker-uhs >>> >>> Regards, >>> Jonas >> Thanks. >> >> Glad your are testing - and I hope the uhs tests work. >> >> Alas for us to test uhs - and our user on Arch isn't so experienced. I >> could push him to try different speeds - but to be honest I expect it to >> be beyond his comfort zone. >> >> But its good to here you confirm "disable-wp" works. To my mind it says >> we are sure that all three changes in the patch are are correct. >> >> Hopefully this will be straight forward to Heiko to accept. Its the >> simple patch of the 3 to accept ;) > From Jonas' explanation, I'd guess we want disable-wp to stay in the node, > and not remove it as it tells the system, that "there is no write protect > status available" hence it doesn't need to check for it. > > > Heiko > > Hi Heiko, I was going to say that: https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/mmc/mmc.txt says: - disable-wp: When set no physical WP line is present. This property should only be specified when the controller has a dedicated write-protect detection logic. Do we know that dw_mmc-rockchip.c has write-protect detection logic? Its not mentioned in the code, but I don't know my way round the mmc code. However quick grep for other rk3288 dts, and almost all have disable-wp. So in line with this - yes agree to do a patch without that, depending on how another post ... Now I've included Jaehoon and Ulf (and linux-mmc) can you say if the rockchip controller can do write-protect detection logic? Thanks, David.
On 25/02/2019 17:13, Doug Anderson wrote: > Hi, > > On Fri, Feb 22, 2019 at 10:48 AM David Summers > <beagleboard@davidjohnsummers.uk> wrote: >> The Problem: >> >> On ASUS Tinker Board S, when booting from the eMMC, and there is no >> card sd slot, then there are constant errors. >> >> Cause: >> >> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The >> rk808 on the Tinker Board and Tinker Board S has many regulators, one >> vccio_sd powers the IO for the sd card. Unfortunatly this is also used >> in the card detect. Now when no card is install, the regulator is >> powered down. This means that the card detect floats, and this means >> random card detection. > Yeah, this is broken on a lot of SoCs that use dw_mmc. :( Really the > card detect line needs to be on a different rail and this is why all > boards I've worked on recently have a the card detect going to a GPIO > instead of the dw_mmc CD. > > IIRC Rockchip moved the Card Detect to a different rail on newer SoCs > (like rk3399) but we still used a GPIO even there since we didn't like > the default/automatic muxing of JTAG and SD signals. > > The one board I was involved in that did it wrong (where we discovered > this issue) was exynos5250-snow. You can see some discussion about > the issue at: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282474.html > > In that discussion I'm pretty sure that Ulf suggested that a better > way to go was to use something like "broken-cd" which I think was > supposed to switch us to use polling. AKA periodically the SD card > would be powered on and we'd check for a card, then we'd power > everything off. ...but that was never implemented for snow at least > so there may be something more than just adding the property. You can > read through the whole thread for more details. > > > IIRC leaving the IO rail always on like you're proposing can also work > OK but there may be some corner cases, especially if you are trying to > reach UHS speeds and/or if the bootloader ever tries to use UHS > speeds. It's almost certainly busted if the bootloader did UHS since > it will leave the line at ~1.8 V and the kernel will expect it to be > at ~3.3 V. ...but maybe you rely on the bootloader not doing UHS and > maybe things are generally OK if not? There may also be cases where > you can't properly power down / reset a card because the card may be > drawing power through the IO lines when you power off its main lines. > That's not good for the card and can also put it in a bad state. I > haven't done all the research here so this may be a bit of FUD--it's > just a vague recollection from many years ago. > > > ...so to make a long story short, a better solution is to allow the IO > lines to be powered off but then poll for the card periodically. > > >> The Solution: >> >> Make sure that the sd IO is always powered, this means card detection >> is always active, which is what should be done on a board with an sd >> slot, which both the Tinker Board and Tinker Board S are. Hence change >> is made to the .dtsi which takes effect on all Tinker Boards as >> required. >> >> The change also adds "regulator-boot-on" the Tinker Board boot from >> uboot, and the sd card is always one option. Hence the IO must be >> powered in uboot, and so setting this flag. >> >> Also removed is "disable-wp" the micro sd card which are used have no >> write protection, so the concept doesn't mean anything, and the >> Tinker Boards work without this. Hence it is removed to simply. > As others have said, please leave disable-wp. There's no way for the > kernel to know if you have a SD or uSD slot and the only difference > between the two (electrically) is that there's no write protect for > micro SD. > > > Also: please CC dw_mmc people on future patches in this area. > > $ ./scripts/get_maintainer.pl -f drivers/mmc/host/dw_mmc.c > Jaehoon Chung <jh80.chung@samsung.com> (maintainer:SYNOPSYS DESIGNWARE > MMC/SD/SDIO DRIVER) > Ulf Hansson <ulf.hansson@linaro.org> (maintainer:MULTIMEDIA CARD > (MMC), SECURE DIGITAL (SD) AND...) > linux-mmc@vger.kernel.org (open list:SYNOPSYS DESIGNWARE MMC/SD/SDIO DRIVER) > > -Doug I think the possible problem is that without this we were getting a lot of errors. Now as the errors happen when the sd io is power down, and so CD floats; then the IO will be powered back up gain - to access the card, only to find no card. So this means the power line goes up and down a lot. Now if we have broken-cd, and polling has to be used, doesn't this also have to power up the IO so it can poll, and the poll puts a bit more load on the processor. So question is which is better? To keep the IO powered up, or to have it going up and down? Anyway I'm happy with either solution. So if we can agree which is best, I'll do the patch for that. Regards, David.
Hi, On Mon, Feb 25, 2019 at 1:05 PM David Summers <beagleboard@davidjohnsummers.uk> wrote: > I was going to say that: > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/mmc/mmc.txt > says: > > - disable-wp: When set no physical WP line is present. This property should > only be specified when the controller has a dedicated write-protect > detection logic. > > Do we know that dw_mmc-rockchip.c has write-protect detection logic? Its > not mentioned in the code, but I don't know my way round the mmc code. In general dw_mmc has dedicated write protect logic. ...so yes the property is needed. -Doug
Hi, On Mon, Feb 25, 2019 at 1:11 PM David Summers <beagleboard@davidjohnsummers.uk> wrote: > > On 25/02/2019 17:13, Doug Anderson wrote: > > Hi, > > > > On Fri, Feb 22, 2019 at 10:48 AM David Summers > > <beagleboard@davidjohnsummers.uk> wrote: > >> The Problem: > >> > >> On ASUS Tinker Board S, when booting from the eMMC, and there is no > >> card sd slot, then there are constant errors. > >> > >> Cause: > >> > >> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The > >> rk808 on the Tinker Board and Tinker Board S has many regulators, one > >> vccio_sd powers the IO for the sd card. Unfortunatly this is also used > >> in the card detect. Now when no card is install, the regulator is > >> powered down. This means that the card detect floats, and this means > >> random card detection. > > Yeah, this is broken on a lot of SoCs that use dw_mmc. :( Really the > > card detect line needs to be on a different rail and this is why all > > boards I've worked on recently have a the card detect going to a GPIO > > instead of the dw_mmc CD. > > > > IIRC Rockchip moved the Card Detect to a different rail on newer SoCs > > (like rk3399) but we still used a GPIO even there since we didn't like > > the default/automatic muxing of JTAG and SD signals. > > > > The one board I was involved in that did it wrong (where we discovered > > this issue) was exynos5250-snow. You can see some discussion about > > the issue at: > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282474.html > > > > In that discussion I'm pretty sure that Ulf suggested that a better > > way to go was to use something like "broken-cd" which I think was > > supposed to switch us to use polling. AKA periodically the SD card > > would be powered on and we'd check for a card, then we'd power > > everything off. ...but that was never implemented for snow at least > > so there may be something more than just adding the property. You can > > read through the whole thread for more details. > > > > > > IIRC leaving the IO rail always on like you're proposing can also work > > OK but there may be some corner cases, especially if you are trying to > > reach UHS speeds and/or if the bootloader ever tries to use UHS > > speeds. It's almost certainly busted if the bootloader did UHS since > > it will leave the line at ~1.8 V and the kernel will expect it to be > > at ~3.3 V. ...but maybe you rely on the bootloader not doing UHS and > > maybe things are generally OK if not? There may also be cases where > > you can't properly power down / reset a card because the card may be > > drawing power through the IO lines when you power off its main lines. > > That's not good for the card and can also put it in a bad state. I > > haven't done all the research here so this may be a bit of FUD--it's > > just a vague recollection from many years ago. > > > > > > ...so to make a long story short, a better solution is to allow the IO > > lines to be powered off but then poll for the card periodically. > > > > > >> The Solution: > >> > >> Make sure that the sd IO is always powered, this means card detection > >> is always active, which is what should be done on a board with an sd > >> slot, which both the Tinker Board and Tinker Board S are. Hence change > >> is made to the .dtsi which takes effect on all Tinker Boards as > >> required. > >> > >> The change also adds "regulator-boot-on" the Tinker Board boot from > >> uboot, and the sd card is always one option. Hence the IO must be > >> powered in uboot, and so setting this flag. > >> > >> Also removed is "disable-wp" the micro sd card which are used have no > >> write protection, so the concept doesn't mean anything, and the > >> Tinker Boards work without this. Hence it is removed to simply. > > As others have said, please leave disable-wp. There's no way for the > > kernel to know if you have a SD or uSD slot and the only difference > > between the two (electrically) is that there's no write protect for > > micro SD. > > > > > > Also: please CC dw_mmc people on future patches in this area. > > > > $ ./scripts/get_maintainer.pl -f drivers/mmc/host/dw_mmc.c > > Jaehoon Chung <jh80.chung@samsung.com> (maintainer:SYNOPSYS DESIGNWARE > > MMC/SD/SDIO DRIVER) > > Ulf Hansson <ulf.hansson@linaro.org> (maintainer:MULTIMEDIA CARD > > (MMC), SECURE DIGITAL (SD) AND...) > > linux-mmc@vger.kernel.org (open list:SYNOPSYS DESIGNWARE MMC/SD/SDIO DRIVER) > > > > -Doug > > I think the possible problem is that without this we were getting a lot > of errors. Now as the errors happen when the sd io is power down, and so > CD floats; then the IO will be powered back up gain - to access the > card, only to find no card. I definitely haven't thought through all the consequences of adding polling. ...but given that the problem is really common with SoCs using dw_mmc it's probably worth it to figure out out something sane. In theory you could have some code that knows that the card detect becomes reliable once the IOs are powered on... > So this means the power line goes up and down a lot. Now if we have > broken-cd, and polling has to be used, doesn't this also have to power > up the IO so it can poll, and the poll puts a bit more load on the > processor. > > So question is which is better? To keep the IO powered up, or to have it > going up and down? > > Anyway I'm happy with either solution. So if we can agree which is best, > I'll do the patch for that. I don't know which is better. ...but I wouldn't expect that turning on regulators and checking a GPIO ever second or so would burn much power. -Doug
Am Montag, 25. Februar 2019, 22:18:28 CET schrieb Doug Anderson: > Hi, > On Mon, Feb 25, 2019 at 1:11 PM David Summers > > <beagleboard@davidjohnsummers.uk> wrote: > > On 25/02/2019 17:13, Doug Anderson wrote: > > > Hi, > > > > > > On Fri, Feb 22, 2019 at 10:48 AM David Summers > > > > > > <beagleboard@davidjohnsummers.uk> wrote: > > >> The Problem: > > >> > > >> On ASUS Tinker Board S, when booting from the eMMC, and there is no > > >> card sd slot, then there are constant errors. > > >> > > >> Cause: > > >> > > >> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The > > >> rk808 on the Tinker Board and Tinker Board S has many regulators, one > > >> vccio_sd powers the IO for the sd card. Unfortunatly this is also used > > >> in the card detect. Now when no card is install, the regulator is > > >> powered down. This means that the card detect floats, and this means > > >> random card detection. > > > > > > Yeah, this is broken on a lot of SoCs that use dw_mmc. :( Really the > > > card detect line needs to be on a different rail and this is why all > > > boards I've worked on recently have a the card detect going to a GPIO > > > instead of the dw_mmc CD. > > > > > > IIRC Rockchip moved the Card Detect to a different rail on newer SoCs > > > (like rk3399) but we still used a GPIO even there since we didn't like > > > the default/automatic muxing of JTAG and SD signals. > > > > > > The one board I was involved in that did it wrong (where we discovered > > > this issue) was exynos5250-snow. You can see some discussion about > > > the issue at: > > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282474 > > > .html > > > > > > In that discussion I'm pretty sure that Ulf suggested that a better > > > way to go was to use something like "broken-cd" which I think was > > > supposed to switch us to use polling. AKA periodically the SD card > > > would be powered on and we'd check for a card, then we'd power > > > everything off. ...but that was never implemented for snow at least > > > so there may be something more than just adding the property. You can > > > read through the whole thread for more details. > > > > > > > > > IIRC leaving the IO rail always on like you're proposing can also work > > > OK but there may be some corner cases, especially if you are trying to > > > reach UHS speeds and/or if the bootloader ever tries to use UHS > > > speeds. It's almost certainly busted if the bootloader did UHS since > > > it will leave the line at ~1.8 V and the kernel will expect it to be > > > at ~3.3 V. ...but maybe you rely on the bootloader not doing UHS and > > > maybe things are generally OK if not? There may also be cases where > > > you can't properly power down / reset a card because the card may be > > > drawing power through the IO lines when you power off its main lines. > > > That's not good for the card and can also put it in a bad state. I > > > haven't done all the research here so this may be a bit of FUD--it's > > > just a vague recollection from many years ago. > > > > > > > > > ...so to make a long story short, a better solution is to allow the IO > > > lines to be powered off but then poll for the card periodically. > > > > > >> The Solution: > > >> > > >> Make sure that the sd IO is always powered, this means card detection > > >> is always active, which is what should be done on a board with an sd > > >> slot, which both the Tinker Board and Tinker Board S are. Hence change > > >> is made to the .dtsi which takes effect on all Tinker Boards as > > >> required. > > >> > > >> The change also adds "regulator-boot-on" the Tinker Board boot from > > >> uboot, and the sd card is always one option. Hence the IO must be > > >> powered in uboot, and so setting this flag. > > >> > > >> Also removed is "disable-wp" the micro sd card which are used have no > > >> write protection, so the concept doesn't mean anything, and the > > >> Tinker Boards work without this. Hence it is removed to simply. > > > > > > As others have said, please leave disable-wp. There's no way for the > > > kernel to know if you have a SD or uSD slot and the only difference > > > between the two (electrically) is that there's no write protect for > > > micro SD. > > > > > > > > > Also: please CC dw_mmc people on future patches in this area. > > > > > > $ ./scripts/get_maintainer.pl -f drivers/mmc/host/dw_mmc.c > > > Jaehoon Chung <jh80.chung@samsung.com> (maintainer:SYNOPSYS DESIGNWARE > > > MMC/SD/SDIO DRIVER) > > > Ulf Hansson <ulf.hansson@linaro.org> (maintainer:MULTIMEDIA CARD > > > (MMC), SECURE DIGITAL (SD) AND...) > > > linux-mmc@vger.kernel.org (open list:SYNOPSYS DESIGNWARE MMC/SD/SDIO > > > DRIVER) > > > > > > -Doug > > > > I think the possible problem is that without this we were getting a lot > > of errors. Now as the errors happen when the sd io is power down, and so > > CD floats; then the IO will be powered back up gain - to access the > > card, only to find no card. > > I definitely haven't thought through all the consequences of adding > polling. ...but given that the problem is really common with SoCs > using dw_mmc it's probably worth it to figure out out something sane. > In theory you could have some code that knows that the card detect > becomes reliable once the IOs are powered on... > > > So this means the power line goes up and down a lot. Now if we have > > broken-cd, and polling has to be used, doesn't this also have to power > > up the IO so it can poll, and the poll puts a bit more load on the > > processor. > > > > So question is which is better? To keep the IO powered up, or to have it > > going up and down? > > > > Anyway I'm happy with either solution. So if we can agree which is best, > > I'll do the patch for that. > > I don't know which is better. ...but I wouldn't expect that turning > on regulators and checking a GPIO ever second or so would burn much > power. and should also save actual power if the regulator isn't running all the time :-)
On 25/02/2019 22:20, Heiko Stübner wrote: > Am Montag, 25. Februar 2019, 22:18:28 CET schrieb Doug Anderson: >> Hi, >> On Mon, Feb 25, 2019 at 1:11 PM David Summers >> >> <beagleboard@davidjohnsummers.uk> wrote: >>> On 25/02/2019 17:13, Doug Anderson wrote: >>>> Hi, >>>> >>>> On Fri, Feb 22, 2019 at 10:48 AM David Summers >>>> >>>> <beagleboard@davidjohnsummers.uk> wrote: >>>>> The Problem: >>>>> >>>>> On ASUS Tinker Board S, when booting from the eMMC, and there is no >>>>> card sd slot, then there are constant errors. >>>>> >>>>> Cause: >>>>> >>>>> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The >>>>> rk808 on the Tinker Board and Tinker Board S has many regulators, one >>>>> vccio_sd powers the IO for the sd card. Unfortunatly this is also used >>>>> in the card detect. Now when no card is install, the regulator is >>>>> powered down. This means that the card detect floats, and this means >>>>> random card detection. >>>> >>>> Yeah, this is broken on a lot of SoCs that use dw_mmc. :( Really the >>>> card detect line needs to be on a different rail and this is why all >>>> boards I've worked on recently have a the card detect going to a GPIO >>>> instead of the dw_mmc CD. >>>> >>>> IIRC Rockchip moved the Card Detect to a different rail on newer SoCs >>>> (like rk3399) but we still used a GPIO even there since we didn't like >>>> the default/automatic muxing of JTAG and SD signals. >>>> >>>> The one board I was involved in that did it wrong (where we discovered >>>> this issue) was exynos5250-snow. You can see some discussion about >>>> the issue at: >>>> >>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282474 >>>> .html >>>> >>>> In that discussion I'm pretty sure that Ulf suggested that a better >>>> way to go was to use something like "broken-cd" which I think was >>>> supposed to switch us to use polling. AKA periodically the SD card >>>> would be powered on and we'd check for a card, then we'd power >>>> everything off. ...but that was never implemented for snow at least >>>> so there may be something more than just adding the property. You can >>>> read through the whole thread for more details. >>>> >>>> >>>> IIRC leaving the IO rail always on like you're proposing can also work >>>> OK but there may be some corner cases, especially if you are trying to >>>> reach UHS speeds and/or if the bootloader ever tries to use UHS >>>> speeds. It's almost certainly busted if the bootloader did UHS since >>>> it will leave the line at ~1.8 V and the kernel will expect it to be >>>> at ~3.3 V. ...but maybe you rely on the bootloader not doing UHS and >>>> maybe things are generally OK if not? There may also be cases where >>>> you can't properly power down / reset a card because the card may be >>>> drawing power through the IO lines when you power off its main lines. >>>> That's not good for the card and can also put it in a bad state. I >>>> haven't done all the research here so this may be a bit of FUD--it's >>>> just a vague recollection from many years ago. >>>> >>>> >>>> ...so to make a long story short, a better solution is to allow the IO >>>> lines to be powered off but then poll for the card periodically. >>>> >>>>> The Solution: >>>>> >>>>> Make sure that the sd IO is always powered, this means card detection >>>>> is always active, which is what should be done on a board with an sd >>>>> slot, which both the Tinker Board and Tinker Board S are. Hence change >>>>> is made to the .dtsi which takes effect on all Tinker Boards as >>>>> required. >>>>> >>>>> The change also adds "regulator-boot-on" the Tinker Board boot from >>>>> uboot, and the sd card is always one option. Hence the IO must be >>>>> powered in uboot, and so setting this flag. >>>>> >>>>> Also removed is "disable-wp" the micro sd card which are used have no >>>>> write protection, so the concept doesn't mean anything, and the >>>>> Tinker Boards work without this. Hence it is removed to simply. >>>> >>>> As others have said, please leave disable-wp. There's no way for the >>>> kernel to know if you have a SD or uSD slot and the only difference >>>> between the two (electrically) is that there's no write protect for >>>> micro SD. >>>> >>>> >>>> Also: please CC dw_mmc people on future patches in this area. >>>> >>>> $ ./scripts/get_maintainer.pl -f drivers/mmc/host/dw_mmc.c >>>> Jaehoon Chung <jh80.chung@samsung.com> (maintainer:SYNOPSYS DESIGNWARE >>>> MMC/SD/SDIO DRIVER) >>>> Ulf Hansson <ulf.hansson@linaro.org> (maintainer:MULTIMEDIA CARD >>>> (MMC), SECURE DIGITAL (SD) AND...) >>>> linux-mmc@vger.kernel.org (open list:SYNOPSYS DESIGNWARE MMC/SD/SDIO >>>> DRIVER) >>>> >>>> -Doug >>> >>> I think the possible problem is that without this we were getting a lot >>> of errors. Now as the errors happen when the sd io is power down, and so >>> CD floats; then the IO will be powered back up gain - to access the >>> card, only to find no card. >> >> I definitely haven't thought through all the consequences of adding >> polling. ...but given that the problem is really common with SoCs >> using dw_mmc it's probably worth it to figure out out something sane. >> In theory you could have some code that knows that the card detect >> becomes reliable once the IOs are powered on... Right, forcing it at the regulator end is somewhat of a blunt instrument when faced with all possible subtleties (but it is what all the vendor kernels seem to do). My next thought would be some "cd-pulled-up-by-vqmmc" quirk where the only difference is that at the point we know we *don't* have a card present and go to release vqmmc, the quirk turns it back on (or skips turning it off at all). However it seems like that that's almost exactly what was proposed last time - I hadn't seen that thread, so I'll take some time to digest it fully at some point. >>> So this means the power line goes up and down a lot. Now if we have >>> broken-cd, and polling has to be used, doesn't this also have to power >>> up the IO so it can poll, and the poll puts a bit more load on the >>> processor. >>> >>> So question is which is better? To keep the IO powered up, or to have it >>> going up and down? >>> >>> Anyway I'm happy with either solution. So if we can agree which is best, >>> I'll do the patch for that. >> >> I don't know which is better. ...but I wouldn't expect that turning >> on regulators and checking a GPIO ever second or so would burn much >> power. It's not so much power that bothers me here as the general "doing more work" impact of polling - specifically I'm recalling the time we discovered that AMD Overdrive boards gained something ridiculous like 5% performance uplift in hackbench just from having a card in the MMC slot. Admittedly dw_mmc doesn't seem to be *that* bad - a quick play with and without "broken-cd" on my little rk3328 suggests that any difference is probably way down in the noise, so maybe it's OK. What would be really fun, though, would be to take advantage of the fact that CD is only half-broken - it should still detect a card by virtue of the pin being properly pulled to ground, it just can't reliably detect not-a-card. AFAICS we could have a quirk to handle phantom insertions, which double-checks CD after powering up all the regulators to see if it stayed low (hmm, I would hope card-detect-delay might do that anyway...) and ignores the event without error if it didn't. Even if we still use a timer to delay unmasking the interrupt for rate-limiting, that ought to be a fair bit lighter-weight than the rigmarole of trying to initiate communication with a possible card and waiting for it to time out. > and should also save actual power if the regulator isn't running all > the time :-) I'd be surprised if it made any noticable difference in the typical SBC case, but now I'm going to have to have a go at finding some suitable current-measurement points to actually test that assumption ;) Robin.
Hi, On Tue, Feb 26, 2019 at 6:46 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 25/02/2019 22:20, Heiko Stübner wrote: > > Am Montag, 25. Februar 2019, 22:18:28 CET schrieb Doug Anderson: > >> Hi, > >> On Mon, Feb 25, 2019 at 1:11 PM David Summers > >> > >> <beagleboard@davidjohnsummers.uk> wrote: > >>> On 25/02/2019 17:13, Doug Anderson wrote: > >>>> Hi, > >>>> > >>>> On Fri, Feb 22, 2019 at 10:48 AM David Summers > >>>> > >>>> <beagleboard@davidjohnsummers.uk> wrote: > >>>>> The Problem: > >>>>> > >>>>> On ASUS Tinker Board S, when booting from the eMMC, and there is no > >>>>> card sd slot, then there are constant errors. > >>>>> > >>>>> Cause: > >>>>> > >>>>> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The > >>>>> rk808 on the Tinker Board and Tinker Board S has many regulators, one > >>>>> vccio_sd powers the IO for the sd card. Unfortunatly this is also used > >>>>> in the card detect. Now when no card is install, the regulator is > >>>>> powered down. This means that the card detect floats, and this means > >>>>> random card detection. > >>>> > >>>> Yeah, this is broken on a lot of SoCs that use dw_mmc. :( Really the > >>>> card detect line needs to be on a different rail and this is why all > >>>> boards I've worked on recently have a the card detect going to a GPIO > >>>> instead of the dw_mmc CD. > >>>> > >>>> IIRC Rockchip moved the Card Detect to a different rail on newer SoCs > >>>> (like rk3399) but we still used a GPIO even there since we didn't like > >>>> the default/automatic muxing of JTAG and SD signals. > >>>> > >>>> The one board I was involved in that did it wrong (where we discovered > >>>> this issue) was exynos5250-snow. You can see some discussion about > >>>> the issue at: > >>>> > >>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282474 > >>>> .html > >>>> > >>>> In that discussion I'm pretty sure that Ulf suggested that a better > >>>> way to go was to use something like "broken-cd" which I think was > >>>> supposed to switch us to use polling. AKA periodically the SD card > >>>> would be powered on and we'd check for a card, then we'd power > >>>> everything off. ...but that was never implemented for snow at least > >>>> so there may be something more than just adding the property. You can > >>>> read through the whole thread for more details. > >>>> > >>>> > >>>> IIRC leaving the IO rail always on like you're proposing can also work > >>>> OK but there may be some corner cases, especially if you are trying to > >>>> reach UHS speeds and/or if the bootloader ever tries to use UHS > >>>> speeds. It's almost certainly busted if the bootloader did UHS since > >>>> it will leave the line at ~1.8 V and the kernel will expect it to be > >>>> at ~3.3 V. ...but maybe you rely on the bootloader not doing UHS and > >>>> maybe things are generally OK if not? There may also be cases where > >>>> you can't properly power down / reset a card because the card may be > >>>> drawing power through the IO lines when you power off its main lines. > >>>> That's not good for the card and can also put it in a bad state. I > >>>> haven't done all the research here so this may be a bit of FUD--it's > >>>> just a vague recollection from many years ago. > >>>> > >>>> > >>>> ...so to make a long story short, a better solution is to allow the IO > >>>> lines to be powered off but then poll for the card periodically. > >>>> > >>>>> The Solution: > >>>>> > >>>>> Make sure that the sd IO is always powered, this means card detection > >>>>> is always active, which is what should be done on a board with an sd > >>>>> slot, which both the Tinker Board and Tinker Board S are. Hence change > >>>>> is made to the .dtsi which takes effect on all Tinker Boards as > >>>>> required. > >>>>> > >>>>> The change also adds "regulator-boot-on" the Tinker Board boot from > >>>>> uboot, and the sd card is always one option. Hence the IO must be > >>>>> powered in uboot, and so setting this flag. > >>>>> > >>>>> Also removed is "disable-wp" the micro sd card which are used have no > >>>>> write protection, so the concept doesn't mean anything, and the > >>>>> Tinker Boards work without this. Hence it is removed to simply. > >>>> > >>>> As others have said, please leave disable-wp. There's no way for the > >>>> kernel to know if you have a SD or uSD slot and the only difference > >>>> between the two (electrically) is that there's no write protect for > >>>> micro SD. > >>>> > >>>> > >>>> Also: please CC dw_mmc people on future patches in this area. > >>>> > >>>> $ ./scripts/get_maintainer.pl -f drivers/mmc/host/dw_mmc.c > >>>> Jaehoon Chung <jh80.chung@samsung.com> (maintainer:SYNOPSYS DESIGNWARE > >>>> MMC/SD/SDIO DRIVER) > >>>> Ulf Hansson <ulf.hansson@linaro.org> (maintainer:MULTIMEDIA CARD > >>>> (MMC), SECURE DIGITAL (SD) AND...) > >>>> linux-mmc@vger.kernel.org (open list:SYNOPSYS DESIGNWARE MMC/SD/SDIO > >>>> DRIVER) > >>>> > >>>> -Doug > >>> > >>> I think the possible problem is that without this we were getting a lot > >>> of errors. Now as the errors happen when the sd io is power down, and so > >>> CD floats; then the IO will be powered back up gain - to access the > >>> card, only to find no card. > >> > >> I definitely haven't thought through all the consequences of adding > >> polling. ...but given that the problem is really common with SoCs > >> using dw_mmc it's probably worth it to figure out out something sane. > >> In theory you could have some code that knows that the card detect > >> becomes reliable once the IOs are powered on... > > Right, forcing it at the regulator end is somewhat of a blunt instrument > when faced with all possible subtleties (but it is what all the vendor > kernels seem to do). My next thought would be some > "cd-pulled-up-by-vqmmc" quirk where the only difference is that at the > point we know we *don't* have a card present and go to release vqmmc, > the quirk turns it back on (or skips turning it off at all). However it > seems like that that's almost exactly what was proposed last time - I > hadn't seen that thread, so I'll take some time to digest it fully at > some point. I don't remember exactly where we left off last time, but in any case it would probably be worth re-evaluating any conclusions made 4 years ago, especially given that there's definitely more than one board in the same position now. > >>> So this means the power line goes up and down a lot. Now if we have > >>> broken-cd, and polling has to be used, doesn't this also have to power > >>> up the IO so it can poll, and the poll puts a bit more load on the > >>> processor. > >>> > >>> So question is which is better? To keep the IO powered up, or to have it > >>> going up and down? > >>> > >>> Anyway I'm happy with either solution. So if we can agree which is best, > >>> I'll do the patch for that. > >> > >> I don't know which is better. ...but I wouldn't expect that turning > >> on regulators and checking a GPIO ever second or so would burn much > >> power. > > It's not so much power that bothers me here as the general "doing more > work" impact of polling - specifically I'm recalling the time we > discovered that AMD Overdrive boards gained something ridiculous like 5% > performance uplift in hackbench just from having a card in the MMC slot. > Admittedly dw_mmc doesn't seem to be *that* bad - a quick play with and > without "broken-cd" on my little rk3328 suggests that any difference is > probably way down in the noise, so maybe it's OK. > > What would be really fun, though, would be to take advantage of the fact > that CD is only half-broken - it should still detect a card by virtue of > the pin being properly pulled to ground, it just can't reliably detect > not-a-card. AFAICS we could have a quirk to handle phantom insertions, > which double-checks CD after powering up all the regulators to see if it > stayed low (hmm, I would hope card-detect-delay might do that anyway...) > and ignores the event without error if it didn't. Even if we still use a > timer to delay unmasking the interrupt for rate-limiting, that ought to > be a fair bit lighter-weight than the rigmarole of trying to initiate > communication with a possible card and waiting for it to time out. I don't _think_ that would work, but I could be wrong. When you stop powering the IO rails then that stops powering the logic in the SoC. I don't think you can reliably detect interrupts in this case. IMO the ideal case would be to power on the rails periodically and then check the Card Detect. That would be better than trying to talk to a card that doesn't exist. > > and should also save actual power if the regulator isn't running all > > the time :-) > > I'd be surprised if it made any noticable difference in the typical SBC > case, but now I'm going to have to have a go at finding some suitable > current-measurement points to actually test that assumption ;) Yeah, it does seem likely that keeping the IO rails powered up when there's no card there is probably a very small amount of power. -Doug
On 26/02/2019 16:43, Doug Anderson wrote: > Hi, > > On Tue, Feb 26, 2019 at 6:46 AM Robin Murphy <robin.murphy@arm.com> wrote: >> >> On 25/02/2019 22:20, Heiko Stübner wrote: >>> Am Montag, 25. Februar 2019, 22:18:28 CET schrieb Doug Anderson: >>>> Hi, >>>> On Mon, Feb 25, 2019 at 1:11 PM David Summers >>>> >>>> <beagleboard@davidjohnsummers.uk> wrote: >>>>> On 25/02/2019 17:13, Doug Anderson wrote: >>>>>> Hi, >>>>>> >>>>>> On Fri, Feb 22, 2019 at 10:48 AM David Summers >>>>>> >>>>>> <beagleboard@davidjohnsummers.uk> wrote: >>>>>>> The Problem: >>>>>>> >>>>>>> On ASUS Tinker Board S, when booting from the eMMC, and there is no >>>>>>> card sd slot, then there are constant errors. >>>>>>> >>>>>>> Cause: >>>>>>> >>>>>>> Thanks must go to Robin Murphy @ ARM for idenifying the problem. The >>>>>>> rk808 on the Tinker Board and Tinker Board S has many regulators, one >>>>>>> vccio_sd powers the IO for the sd card. Unfortunatly this is also used >>>>>>> in the card detect. Now when no card is install, the regulator is >>>>>>> powered down. This means that the card detect floats, and this means >>>>>>> random card detection. >>>>>> >>>>>> Yeah, this is broken on a lot of SoCs that use dw_mmc. :( Really the >>>>>> card detect line needs to be on a different rail and this is why all >>>>>> boards I've worked on recently have a the card detect going to a GPIO >>>>>> instead of the dw_mmc CD. >>>>>> >>>>>> IIRC Rockchip moved the Card Detect to a different rail on newer SoCs >>>>>> (like rk3399) but we still used a GPIO even there since we didn't like >>>>>> the default/automatic muxing of JTAG and SD signals. >>>>>> >>>>>> The one board I was involved in that did it wrong (where we discovered >>>>>> this issue) was exynos5250-snow. You can see some discussion about >>>>>> the issue at: >>>>>> >>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/282474 >>>>>> .html >>>>>> >>>>>> In that discussion I'm pretty sure that Ulf suggested that a better >>>>>> way to go was to use something like "broken-cd" which I think was >>>>>> supposed to switch us to use polling. AKA periodically the SD card >>>>>> would be powered on and we'd check for a card, then we'd power >>>>>> everything off. ...but that was never implemented for snow at least >>>>>> so there may be something more than just adding the property. You can >>>>>> read through the whole thread for more details. >>>>>> >>>>>> >>>>>> IIRC leaving the IO rail always on like you're proposing can also work >>>>>> OK but there may be some corner cases, especially if you are trying to >>>>>> reach UHS speeds and/or if the bootloader ever tries to use UHS >>>>>> speeds. It's almost certainly busted if the bootloader did UHS since >>>>>> it will leave the line at ~1.8 V and the kernel will expect it to be >>>>>> at ~3.3 V. ...but maybe you rely on the bootloader not doing UHS and >>>>>> maybe things are generally OK if not? There may also be cases where >>>>>> you can't properly power down / reset a card because the card may be >>>>>> drawing power through the IO lines when you power off its main lines. >>>>>> That's not good for the card and can also put it in a bad state. I >>>>>> haven't done all the research here so this may be a bit of FUD--it's >>>>>> just a vague recollection from many years ago. >>>>>> >>>>>> >>>>>> ...so to make a long story short, a better solution is to allow the IO >>>>>> lines to be powered off but then poll for the card periodically. >>>>>> >>>>>>> The Solution: >>>>>>> >>>>>>> Make sure that the sd IO is always powered, this means card detection >>>>>>> is always active, which is what should be done on a board with an sd >>>>>>> slot, which both the Tinker Board and Tinker Board S are. Hence change >>>>>>> is made to the .dtsi which takes effect on all Tinker Boards as >>>>>>> required. >>>>>>> >>>>>>> The change also adds "regulator-boot-on" the Tinker Board boot from >>>>>>> uboot, and the sd card is always one option. Hence the IO must be >>>>>>> powered in uboot, and so setting this flag. >>>>>>> >>>>>>> Also removed is "disable-wp" the micro sd card which are used have no >>>>>>> write protection, so the concept doesn't mean anything, and the >>>>>>> Tinker Boards work without this. Hence it is removed to simply. >>>>>> >>>>>> As others have said, please leave disable-wp. There's no way for the >>>>>> kernel to know if you have a SD or uSD slot and the only difference >>>>>> between the two (electrically) is that there's no write protect for >>>>>> micro SD. >>>>>> >>>>>> >>>>>> Also: please CC dw_mmc people on future patches in this area. >>>>>> >>>>>> $ ./scripts/get_maintainer.pl -f drivers/mmc/host/dw_mmc.c >>>>>> Jaehoon Chung <jh80.chung@samsung.com> (maintainer:SYNOPSYS DESIGNWARE >>>>>> MMC/SD/SDIO DRIVER) >>>>>> Ulf Hansson <ulf.hansson@linaro.org> (maintainer:MULTIMEDIA CARD >>>>>> (MMC), SECURE DIGITAL (SD) AND...) >>>>>> linux-mmc@vger.kernel.org (open list:SYNOPSYS DESIGNWARE MMC/SD/SDIO >>>>>> DRIVER) >>>>>> >>>>>> -Doug >>>>> >>>>> I think the possible problem is that without this we were getting a lot >>>>> of errors. Now as the errors happen when the sd io is power down, and so >>>>> CD floats; then the IO will be powered back up gain - to access the >>>>> card, only to find no card. >>>> >>>> I definitely haven't thought through all the consequences of adding >>>> polling. ...but given that the problem is really common with SoCs >>>> using dw_mmc it's probably worth it to figure out out something sane. >>>> In theory you could have some code that knows that the card detect >>>> becomes reliable once the IOs are powered on... >> >> Right, forcing it at the regulator end is somewhat of a blunt instrument >> when faced with all possible subtleties (but it is what all the vendor >> kernels seem to do). My next thought would be some >> "cd-pulled-up-by-vqmmc" quirk where the only difference is that at the >> point we know we *don't* have a card present and go to release vqmmc, >> the quirk turns it back on (or skips turning it off at all). However it >> seems like that that's almost exactly what was proposed last time - I >> hadn't seen that thread, so I'll take some time to digest it fully at >> some point. > > I don't remember exactly where we left off last time, but in any case > it would probably be worth re-evaluating any conclusions made 4 years > ago, especially given that there's definitely more than one board in > the same position now. > > >>>>> So this means the power line goes up and down a lot. Now if we have >>>>> broken-cd, and polling has to be used, doesn't this also have to power >>>>> up the IO so it can poll, and the poll puts a bit more load on the >>>>> processor. >>>>> >>>>> So question is which is better? To keep the IO powered up, or to have it >>>>> going up and down? >>>>> >>>>> Anyway I'm happy with either solution. So if we can agree which is best, >>>>> I'll do the patch for that. >>>> >>>> I don't know which is better. ...but I wouldn't expect that turning >>>> on regulators and checking a GPIO ever second or so would burn much >>>> power. >> >> It's not so much power that bothers me here as the general "doing more >> work" impact of polling - specifically I'm recalling the time we >> discovered that AMD Overdrive boards gained something ridiculous like 5% >> performance uplift in hackbench just from having a card in the MMC slot. >> Admittedly dw_mmc doesn't seem to be *that* bad - a quick play with and >> without "broken-cd" on my little rk3328 suggests that any difference is >> probably way down in the noise, so maybe it's OK. >> >> What would be really fun, though, would be to take advantage of the fact >> that CD is only half-broken - it should still detect a card by virtue of >> the pin being properly pulled to ground, it just can't reliably detect >> not-a-card. AFAICS we could have a quirk to handle phantom insertions, >> which double-checks CD after powering up all the regulators to see if it >> stayed low (hmm, I would hope card-detect-delay might do that anyway...) >> and ignores the event without error if it didn't. Even if we still use a >> timer to delay unmasking the interrupt for rate-limiting, that ought to >> be a fair bit lighter-weight than the rigmarole of trying to initiate >> communication with a possible card and waiting for it to time out. > > I don't _think_ that would work, but I could be wrong. When you stop > powering the IO rails then that stops powering the logic in the SoC. > I don't think you can reliably detect interrupts in this case. Hmm, it certainly works on RK3288, but in general you do have a point there. As I understand the typical case, the external regulator only powers the I/O pads, while the controller block itself is at the mercy of SoC-internal power domains. On 3288 there's so much other gubbins in PD_PERI that the controller itself will effectively never get powered down, thus the interrupt logic keeps ticking. Comparing RK3399, though, the controller's on its own in the finer-grained PD_SD, which I can see getting aggressively auto-suspended by runtime PM... ...and I think you've just solved the puzzle of why my new board's card detect works as a GPIO but not as the dedicated function (despite having its I/O pad over in the always-on PMU domain away from regulator problems). Thanks! :D > IMO the ideal case would be to power on the rails periodically and > then check the Card Detect. That would be better than trying to talk > to a card that doesn't exist. Yes, that sounds like the most useful compromise - a (possibly dwmmc-specific) property which says that the internal CD *interrupt* is unreliable, but the status bit itself will still be accurate after a proper powerup sequence. Then the existing polling machinery could probably grow an intermediate level wherein it checks ->get_cd() first and only bothers calling ->alive() if it looks like a card's actually turned up. Robin.
Dear All, So lets do a synthesis of where we are, People who have an interest: 1) Me 2) Jonas Karlman - disable-wp should be set, and Tested. But commented "fixes reboot when booting from sd-card and having emmc zeroed out" when regulator-boot-on is set 3) Heiko Stubner (Rockchip maintainer) - agrees disable-wp should set 4) Doug Anderson - Card Detect shouldn't be power by vccio_sd. So this error in board construction, means should do "broken-cd" as Card Detect doesn't work when vccio_sd is down 5) Jaehoon Chung & Ulf Hansson, not posted on this thread - but MMC SD maintainer. In past thread 4-5 years ago, strongly said this is a broken cd, hence use "broken-cd" 6) Robin Murphy - who identified the problem with card detected powered by vccio_sd. So power on vccio_sd always, but can be swayed. Maybe we should have a software correction to this problem. 7) Tony McKahan - involved in Armbian, so interested in Tinker Board S support by mainline Linux And so what to conclude. Well, although Jaehoon and Ulf haven't posted on this thread, the thread from four years ago: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/281153.html And it makes clear that its treated as a broken card detect, and so "broken-cd" is the correct solution, now Robin says on some machines this causes a high load due to the polling; but this hasn't clearly been seen on rk3288 machines, either on this thread or on ArchLinux Arm. To my mind this means that Ulf and Jaehoon as maintainers, even with a 4 year old post, trump and win - and so we go with "broken-cd" over enabling the regulator all the time. Jonas comment though, about warm reboots on sd card. Suggests we do have "regulator-boot-on". Haven't been able to confirm this; so would be good if Jonas could confirm this is *exactly* what is needed? So unless anyone says anything to convince me otherwise, my plan is to submit v3 of this patch in next few days with: 1) "broken-cd" 2) "disable-wp" 3) "regulator-boot-on" Now question for Heiko is what schedule are we looking at? My guess is that the linux 5.1 boot has been missed? So whats the schedule for 5.2? Guess I should also say some words on the talk of should there be a software work around for this common hardware fault, where the CD is powered by vccio_sd. As I see it with "broken-cd" when we want sd status the kernel does: 1) Power on vccio_sd 2) Poll the card with timeouts 3) Power down the vccio_sd Now with an additional software work around, would involve step 2 replaced with "test cd pin" - but still need steps 1 and 3. Now testing the cd pin, should be lower load than polling and timeouts - but still have the regulator on-off overhead. Anyway I don't have time right now to follow such a software work around. So plan this patch to be independent. Any views let me know - otherwise v3 of the patch ~Wednesday. David. On 22/02/2019 18:47, David Summers wrote: > The Problem: > > On ASUS Tinker Board S, when booting from the eMMC, and there is no > card sd slot, then there are constant errors. > > Cause: > > Thanks must go to Robin Murphy @ ARM for idenifying the problem. The > rk808 on the Tinker Board and Tinker Board S has many regulators, one > vccio_sd powers the IO for the sd card. Unfortunatly this is also used > in the card detect. Now when no card is install, the regulator is > powered down. This means that the card detect floats, and this means > random card detection. > > The Solution: > > Make sure that the sd IO is always powered, this means card detection > is always active, which is what should be done on a board with an sd > slot, which both the Tinker Board and Tinker Board S are. Hence change > is made to the .dtsi which takes effect on all Tinker Boards as > required. > > The change also adds "regulator-boot-on" the Tinker Board boot from > uboot, and the sd card is always one option. Hence the IO must be > powered in uboot, and so setting this flag. > > Also removed is "disable-wp" the micro sd card which are used have no > write protection, so the concept doesn't mean anything, and the > Tinker Boards work without this. Hence it is removed to simply. > > This change came from ArchLinux Arm, but we note it is the fix also > used by Armbian: > > https://github.com/Miouyouyou/RockMyy/blob/master/patches/kernel/v5.0/DTS/0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch > > Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> > --- > arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi > index aa107ee41b8b..6b7e55085b0c 100644 > --- a/arch/arm/boot/dts/rk3288-tinker.dtsi > +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi > @@ -254,6 +254,8 @@ > }; > > vccio_sd: LDO_REG5 { > + regulator-always-on; > + regulator-boot-on; > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <3300000>; > regulator-name = "vccio_sd"; > @@ -431,7 +433,6 @@ > cap-mmc-highspeed; > cap-sd-highspeed; > card-detect-delay = <200>; > - disable-wp; /* wp not hooked up */ > pinctrl-names = "default"; > pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; > status = "okay";
diff --git a/arch/arm/boot/dts/rk3288-tinker.dtsi b/arch/arm/boot/dts/rk3288-tinker.dtsi index aa107ee41b8b..6b7e55085b0c 100644 --- a/arch/arm/boot/dts/rk3288-tinker.dtsi +++ b/arch/arm/boot/dts/rk3288-tinker.dtsi @@ -254,6 +254,8 @@ }; vccio_sd: LDO_REG5 { + regulator-always-on; + regulator-boot-on; regulator-min-microvolt = <1800000>; regulator-max-microvolt = <3300000>; regulator-name = "vccio_sd"; @@ -431,7 +433,6 @@ cap-mmc-highspeed; cap-sd-highspeed; card-detect-delay = <200>; - disable-wp; /* wp not hooked up */ pinctrl-names = "default"; pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_cd &sdmmc_bus4>; status = "okay";
The Problem: On ASUS Tinker Board S, when booting from the eMMC, and there is no card sd slot, then there are constant errors. Cause: Thanks must go to Robin Murphy @ ARM for idenifying the problem. The rk808 on the Tinker Board and Tinker Board S has many regulators, one vccio_sd powers the IO for the sd card. Unfortunatly this is also used in the card detect. Now when no card is install, the regulator is powered down. This means that the card detect floats, and this means random card detection. The Solution: Make sure that the sd IO is always powered, this means card detection is always active, which is what should be done on a board with an sd slot, which both the Tinker Board and Tinker Board S are. Hence change is made to the .dtsi which takes effect on all Tinker Boards as required. The change also adds "regulator-boot-on" the Tinker Board boot from uboot, and the sd card is always one option. Hence the IO must be powered in uboot, and so setting this flag. Also removed is "disable-wp" the micro sd card which are used have no write protection, so the concept doesn't mean anything, and the Tinker Boards work without this. Hence it is removed to simply. This change came from ArchLinux Arm, but we note it is the fix also used by Armbian: https://github.com/Miouyouyou/RockMyy/blob/master/patches/kernel/v5.0/DTS/0016-ARM-DTSI-rk3288-tinker-Setting-up-the-SD-regulators.patch Signed-off-by: David Summers <beagleboard@davidjohnsummers.uk> --- arch/arm/boot/dts/rk3288-tinker.dtsi | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)