| Message ID | 20210412000813.13071-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: allwinner: Revert SD card CD GPIO for Pine64-LTS | expand |
Hi, On Mon, Apr 12, 2021 at 8:08 AM Andre Przywara <andre.przywara@arm.com> wrote: > > Commit 941432d00768 ("arm64: dts: allwinner: Drop non-removable from > SoPine/LTS SD card") enabled the card detect GPIO for the SOPine module, > along the way with the Pine64-LTS, which share the same base .dtsi. > > This was based on the observation that the Pine64-LTS has as "push-push" > SD card socket, and that the schematic mentions the card detect GPIO. > > After having received two reports about failing SD card access with that > patch, some more research and polls on that subject revealed that there > are at least two different versions of the Pine64-LTS out there: > - On some boards (including mine) the card detect pin is "stuck" at > high, regardless of an microSD card being inserted or not. > - On other boards the card-detect is working, but is active-high, by > virtue of an explicit inverter circuit, as shown in the schematic. > > To cover all versions of the board out there, and don't take any chances, > let's revert the introduction of the CD GPIO, and go back to the > non-removable property for the Pine64-LTS. That should avoid regressions > and should work for everyone. > The SOPine card detect has proven to be working, so let's keep that > GPIO in place. > > Fixes: 941432d00768 ("arm64: dts: allwinner: Drop non-removable from SoPine/LTS SD card") > Reported-by: Michael Weiser <michael.weiser@gmx.de> > Reported-by: Daniel Kulesz <kuleszdl@posteo.org> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > index e79ce49e7e6a..843338e19694 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > @@ -21,5 +21,5 @@ > }; > > &mmc0 { > - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 push-push switch */ > + non-removable; /* card detect is broken on some boards */ So a revert is good, but has anyone tried using the "broken-cd" instead? That way, at least on Linux, the mmc core resorts to polling for a card. At least this way the card is still removable. ChenYu
On Mon, 12 Apr 2021 14:20:41 +0800 Chen-Yu Tsai <wens@csie.org> wrote: > Hi, > > On Mon, Apr 12, 2021 at 8:08 AM Andre Przywara <andre.przywara@arm.com> wrote: > > > > Commit 941432d00768 ("arm64: dts: allwinner: Drop non-removable from > > SoPine/LTS SD card") enabled the card detect GPIO for the SOPine module, > > along the way with the Pine64-LTS, which share the same base .dtsi. > > > > This was based on the observation that the Pine64-LTS has as "push-push" > > SD card socket, and that the schematic mentions the card detect GPIO. > > > > After having received two reports about failing SD card access with that > > patch, some more research and polls on that subject revealed that there > > are at least two different versions of the Pine64-LTS out there: > > - On some boards (including mine) the card detect pin is "stuck" at > > high, regardless of an microSD card being inserted or not. > > - On other boards the card-detect is working, but is active-high, by > > virtue of an explicit inverter circuit, as shown in the schematic. > > > > To cover all versions of the board out there, and don't take any chances, > > let's revert the introduction of the CD GPIO, and go back to the > > non-removable property for the Pine64-LTS. That should avoid regressions > > and should work for everyone. > > The SOPine card detect has proven to be working, so let's keep that > > GPIO in place. > > > > Fixes: 941432d00768 ("arm64: dts: allwinner: Drop non-removable from SoPine/LTS SD card") > > Reported-by: Michael Weiser <michael.weiser@gmx.de> > > Reported-by: Daniel Kulesz <kuleszdl@posteo.org> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > index e79ce49e7e6a..843338e19694 100644 > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > @@ -21,5 +21,5 @@ > > }; > > > > &mmc0 { > > - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 push-push switch */ > > + non-removable; /* card detect is broken on some boards */ > > So a revert is good, but has anyone tried using the "broken-cd" instead? Ha, that's a good idea, I totally forgot about this property! > That way, at least on Linux, the mmc core resorts to polling for a card. > At least this way the card is still removable. Yes indeed, I tested it on my "stuck at 1" Pine64-LTS, and it works like a charm! Daniel, Michael, can you test this on your boards? So removing the cd-gpios property, and adding "broken-cd;" instead? Cheers, Andre
Hi Andre, ChenYu, On Mon, Apr 12, 2021 at 05:45:58PM +0100, Andre Przywara wrote: > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > index e79ce49e7e6a..843338e19694 100644 > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > @@ -21,5 +21,5 @@ > > > }; > > > > > > &mmc0 { > > > - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 push-push switch */ > > > + non-removable; /* card detect is broken on some boards */ > > > > So a revert is good, but has anyone tried using the "broken-cd" instead? > Ha, that's a good idea, I totally forgot about this property! > > That way, at least on Linux, the mmc core resorts to polling for a card. > > At least this way the card is still removable. > Yes indeed, I tested it on my "stuck at 1" Pine64-LTS, and it works like > a charm! > Daniel, Michael, can you test this on your boards? So removing the > cd-gpios property, and adding "broken-cd;" instead? Yes, it works fine. What flummoxed me at first was that obviously I also have to disable the ACTIVE_LOW definition in sun50i-a64-sopine.dtsi after having added and disabled an ACTIVE_HIGH definition in sun50i-a64-pine64-lts.dts. BTW: My boards have a marking "PINE64-R18-V1_1" and below it "2017-08-03" on their upper side. On the back it says on one sticker "Model:PineA64 2GB LTS" and on another "2O1-PINE64R18-00" and "PINE64-R18-V1.1 2G". Is CD being stuck at 1 a bug of revision 1.0 perhaps? Is there a way to detect this difference in software and add some sort of quirk handler for it?
Dne ponedeljek, 12. april 2021 ob 19:41:25 CEST je Michael Weiser napisal(a): > Hi Andre, ChenYu, > > On Mon, Apr 12, 2021 at 05:45:58PM +0100, Andre Przywara wrote: > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts b/ arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > > index e79ce49e7e6a..843338e19694 100644 > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > > @@ -21,5 +21,5 @@ > > > > }; > > > > > > > > &mmc0 { > > > > - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 push-push switch */ > > > > + non-removable; /* card detect is broken on some boards */ > > > > > > So a revert is good, but has anyone tried using the "broken-cd" instead? > > Ha, that's a good idea, I totally forgot about this property! > > > > That way, at least on Linux, the mmc core resorts to polling for a card. > > > At least this way the card is still removable. > > Yes indeed, I tested it on my "stuck at 1" Pine64-LTS, and it works like > > a charm! > > > Daniel, Michael, can you test this on your boards? So removing the > > cd-gpios property, and adding "broken-cd;" instead? > > Yes, it works fine. What flummoxed me at first was that obviously I also > have to disable the ACTIVE_LOW definition in sun50i-a64-sopine.dtsi > after having added and disabled an ACTIVE_HIGH definition in > sun50i-a64-pine64-lts.dts. > > BTW: My boards have a marking "PINE64-R18-V1_1" and below it > "2017-08-03" on their upper side. On the back it says on one sticker > "Model:PineA64 2GB LTS" and on another "2O1-PINE64R18-00" and > "PINE64-R18-V1.1 2G". Is CD being stuck at 1 a bug of revision 1.0 > perhaps? Is there a way to detect this difference in software and add > some sort of quirk handler for it? This is job for bootloader (U-Boot) which can patch DT. Most Allwinner boards have no reliable way to be distinguished, except from Olimex. So I would say it's not possible. Best regards, Jernej
On Mon, 12 Apr 2021 19:41:25 +0200 Michael Weiser <michael.weiser@gmx.de> wrote: Hi Michael, > Hi Andre, ChenYu, > > On Mon, Apr 12, 2021 at 05:45:58PM +0100, Andre Przywara wrote: > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > > index e79ce49e7e6a..843338e19694 100644 > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > > > > @@ -21,5 +21,5 @@ > > > > }; > > > > > > > > &mmc0 { > > > > - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 push-push switch */ > > > > + non-removable; /* card detect is broken on some boards */ > > > > > > So a revert is good, but has anyone tried using the "broken-cd" instead? > > Ha, that's a good idea, I totally forgot about this property! > > > > That way, at least on Linux, the mmc core resorts to polling for a card. > > > At least this way the card is still removable. > > Yes indeed, I tested it on my "stuck at 1" Pine64-LTS, and it works like > > a charm! > > > Daniel, Michael, can you test this on your boards? So removing the > > cd-gpios property, and adding "broken-cd;" instead? > > Yes, it works fine. What flummoxed me at first was that obviously I also > have to disable the ACTIVE_LOW definition in sun50i-a64-sopine.dtsi > after having added and disabled an ACTIVE_HIGH definition in > sun50i-a64-pine64-lts.dts. Why? From my experiments it should not matter, the actual card presence is typically detected via the SD bus anyway (if I understand the code correctly). broken-cd just prevents installation of an interrupt handler, so it's less efficient and prevents wakeup on card detect, AFAICS. But also: ACTIVE_HIGH *is* the right polarity, even for the Pine64-LTS. At least that's what an earlier report from Daniel suggested: => gpio input pf6 card inserted: value is 1 card removed: value is 0 So for your particular board (version) you could actually remove the whole &mmc0 node override, use the same node as the SOPine (working active-high CD) and it should work. > BTW: My boards have a marking "PINE64-R18-V1_1" and below it > "2017-08-03" on their upper side. On the back it says on one sticker > "Model:PineA64 2GB LTS" and on another "2O1-PINE64R18-00" and > "PINE64-R18-V1.1 2G". Is CD being stuck at 1 a bug of revision 1.0 > perhaps? Is there a way to detect this difference in software and add > some sort of quirk handler for it? As Jernej mentioned, this would be U-Boot's task, but I don't see a good reason for it. Firstly, you would need to find a good automatic way of determining the board revision, which I doubt there is. And secondly, I don't see the benefit: It works quite nicely with broken-cd: card removals and insertions are detected automatically, it's just not as efficient (interrupt-driven) as it could be. Or do you see any problems with broken-cd? Cheers, Andre
Hi Jernej and Andre, On Tue, Apr 13, 2021 at 11:58:37AM +0100, Andre Przywara wrote: > > > Daniel, Michael, can you test this on your boards? So removing the > > > cd-gpios property, and adding "broken-cd;" instead? > > Yes, it works fine. What flummoxed me at first was that obviously I also > > have to disable the ACTIVE_LOW definition in sun50i-a64-sopine.dtsi > > after having added and disabled an ACTIVE_HIGH definition in > > sun50i-a64-pine64-lts.dts. > Why? From my experiments it should not matter, the actual card presence > is typically detected via the SD bus anyway (if I understand the code > correctly). broken-cd just prevents installation of an interrupt > handler, so it's less efficient and prevents wakeup on card detect, > AFAICS. I just retested to be sure: At least with 5.11.11 and on my boards, cd-gpio ACTIVE_LOW being specified in the sopine.dtsi takes precedence over broken-cd being specified in pine64-lts.dts. Could that spoil the plan of disabling cd-gpio for the LTS while leaving it enabled for Sopine and baseboard? Behaviour is as such: With cd-gpios ACTIVE_LOW in sopine.dtsi and broken-cd in pine64-lts.dts, card insertion, removal and reinsertion is not detected after booting the kernel without a card in the slot. With cd-gpios ACTIVE_LOW removed from sopine.dtsi it starts working. In diffs for added clarity: PAGER= git log --pretty=oneline HEAD~1..HEAD aa7258f8f3d48a29bc024ea8c5145bdc4a980e4d (HEAD, tag: v5.11.11) Linux 5.11.11 - not working on its own: index 302e24be0a31..5b0c21e68352 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts @@ -8,3 +8,7 @@ / { compatible = "pine64,pine64-lts", "allwinner,sun50i-r18", "allwinner,sun50i-a64"; }; + +&mmc0 { + broken-cd; +}; - working with this additional change: index 3402cec87035..ba2b7398993b 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi @@ -34,7 +34,6 @@ &mmc0 { vmmc-supply = <®_dcdc1>; disable-wp; bus-width = <4>; - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */ status = "okay"; }; > So for your particular board (version) you could actually remove the > whole &mmc0 node override, use the same node as the SOPine (working > active-high CD) and it should work. Correct. Again for reasons of laziness I tested with the dts from 5.11.11 which is currently running on the board and which still has ACTIVE_LOW in sopine.dtsi. Sorry for not being clearer about that. But somewhat lucky as well because without ACTIVE_LOW still being set in sopine.dtsi I wouldn't have had a way to tell that broken-cd was not taking effect and silently have tested the working ACTIVE_HIGH definition from sopine.dtsi. Or am I somehow making a mess of this? > > BTW: My boards have a marking "PINE64-R18-V1_1" and below it > > "2017-08-03" on their upper side. On the back it says on one sticker > > "Model:PineA64 2GB LTS" and on another "2O1-PINE64R18-00" and > > "PINE64-R18-V1.1 2G". Is CD being stuck at 1 a bug of revision 1.0 > > perhaps? Is there a way to detect this difference in software and add > > some sort of quirk handler for it? > As Jernej mentioned, this would be U-Boot's task, but I don't see a > good reason for it. Firstly, you would need to find a good automatic > way of determining the board revision, which I doubt there is. And > secondly, I don't see the benefit: It works quite nicely with > broken-cd: card removals and insertions are detected automatically, > it's just not as efficient (interrupt-driven) as it could be. > Or do you see any problems with broken-cd? Of course not. My boards have their rootfs on mmc0, so the card is never removed and replaced during operation anyway. I was just asking out of curiosity. And out of curiosity again: Could one have a device tree overlay configured manually to be loaded by the bootloader that adds cd-gpio ACTIVE_HIGH for mmc0 and disables/overrules broken-cd? Somewhat like so (untested): /dts-v1/; /plugin/; #include <dt-bindings/gpio/gpio.h> / { fragment@0 { target = <&mmc0>; __overlay__ { cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 push-push switch */ }; }; }; Here it would be useful if cd-gpios indeed took precedence over broken-cd because my grepping of the code can't find a way to un-specify it once set.
Hi folks, I have just made the two changes suggested by Michael to my A64-LTS. My unit is running kernel 5.10.28-1 from Debian unstable (Package "linux-image-5.10.0-6-arm64"). The DTB was built from the one supplied with kernel 5.10.29 (from kernel.org). Using the modified dtb (with the changes applied), the machine *DOES* boot fine for me. Cheers, Daniel On Tue, 13 Apr 2021 20:48:24 +0200 Michael Weiser <michael.weiser@gmx.de> wrote: > Hi Jernej and Andre, > > On Tue, Apr 13, 2021 at 11:58:37AM +0100, Andre Przywara wrote: > > > > > Daniel, Michael, can you test this on your boards? So removing the > > > > cd-gpios property, and adding "broken-cd;" instead? > > > Yes, it works fine. What flummoxed me at first was that obviously I also > > > have to disable the ACTIVE_LOW definition in sun50i-a64-sopine.dtsi > > > after having added and disabled an ACTIVE_HIGH definition in > > > sun50i-a64-pine64-lts.dts. > > Why? From my experiments it should not matter, the actual card presence > > is typically detected via the SD bus anyway (if I understand the code > > correctly). broken-cd just prevents installation of an interrupt > > handler, so it's less efficient and prevents wakeup on card detect, > > AFAICS. > > I just retested to be sure: At least with 5.11.11 and on my boards, > cd-gpio ACTIVE_LOW being specified in the sopine.dtsi takes precedence > over broken-cd being specified in pine64-lts.dts. Could that spoil the > plan of disabling cd-gpio for the LTS while leaving it enabled for > Sopine and baseboard? > > Behaviour is as such: With cd-gpios ACTIVE_LOW in sopine.dtsi and > broken-cd in pine64-lts.dts, card insertion, removal and reinsertion is > not detected after booting the kernel without a card in the slot. With > cd-gpios ACTIVE_LOW removed from sopine.dtsi it starts working. > > In diffs for added clarity: > > PAGER= git log --pretty=oneline HEAD~1..HEAD > aa7258f8f3d48a29bc024ea8c5145bdc4a980e4d (HEAD, tag: v5.11.11) Linux 5.11.11 > > - not working on its own: > > index 302e24be0a31..5b0c21e68352 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > @@ -8,3 +8,7 @@ / { > compatible = "pine64,pine64-lts", "allwinner,sun50i-r18", > "allwinner,sun50i-a64"; > }; > + > +&mmc0 { > + broken-cd; > +}; > > - working with this additional change: > > index 3402cec87035..ba2b7398993b 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > @@ -34,7 +34,6 @@ &mmc0 { > vmmc-supply = <®_dcdc1>; > disable-wp; > bus-width = <4>; > - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */ > status = "okay"; > }; > > > So for your particular board (version) you could actually remove the > > whole &mmc0 node override, use the same node as the SOPine (working > > active-high CD) and it should work. > > Correct. Again for reasons of laziness I tested with the dts from > 5.11.11 which is currently running on the board and which still has > ACTIVE_LOW in sopine.dtsi. Sorry for not being clearer about that. > > But somewhat lucky as well because without ACTIVE_LOW still being set in > sopine.dtsi I wouldn't have had a way to tell that broken-cd was not > taking effect and silently have tested the working ACTIVE_HIGH > definition from sopine.dtsi. > > Or am I somehow making a mess of this? > > > > BTW: My boards have a marking "PINE64-R18-V1_1" and below it > > > "2017-08-03" on their upper side. On the back it says on one sticker > > > "Model:PineA64 2GB LTS" and on another "2O1-PINE64R18-00" and > > > "PINE64-R18-V1.1 2G". Is CD being stuck at 1 a bug of revision 1.0 > > > perhaps? Is there a way to detect this difference in software and add > > > some sort of quirk handler for it? > > As Jernej mentioned, this would be U-Boot's task, but I don't see a > > good reason for it. Firstly, you would need to find a good automatic > > way of determining the board revision, which I doubt there is. And > > secondly, I don't see the benefit: It works quite nicely with > > broken-cd: card removals and insertions are detected automatically, > > it's just not as efficient (interrupt-driven) as it could be. > > Or do you see any problems with broken-cd? > > Of course not. My boards have their rootfs on mmc0, so the card is never > removed and replaced during operation anyway. I was just asking out of > curiosity. > > And out of curiosity again: Could one have a device tree overlay > configured manually to be loaded by the bootloader that adds cd-gpio > ACTIVE_HIGH for mmc0 and disables/overrules broken-cd? Somewhat like so > (untested): > > /dts-v1/; > /plugin/; > > #include <dt-bindings/gpio/gpio.h> > > / { > fragment@0 { > target = <&mmc0>; > __overlay__ { > cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 push-push switch */ > }; > }; > }; > > Here it would be useful if cd-gpios indeed took precedence over > broken-cd because my grepping of the code can't find a way to un-specify > it once set. > -- > Michael
On Tue, 13 Apr 2021 20:48:24 +0200 Michael Weiser <michael.weiser@gmx.de> wrote: Hi Michael, thanks for the reply and your testing! > On Tue, Apr 13, 2021 at 11:58:37AM +0100, Andre Przywara wrote: > > > > > Daniel, Michael, can you test this on your boards? So removing the > > > > cd-gpios property, and adding "broken-cd;" instead? > > > Yes, it works fine. What flummoxed me at first was that obviously I also > > > have to disable the ACTIVE_LOW definition in sun50i-a64-sopine.dtsi > > > after having added and disabled an ACTIVE_HIGH definition in > > > sun50i-a64-pine64-lts.dts. > > Why? From my experiments it should not matter, the actual card presence > > is typically detected via the SD bus anyway (if I understand the code > > correctly). broken-cd just prevents installation of an interrupt > > handler, so it's less efficient and prevents wakeup on card detect, > > AFAICS. > > I just retested to be sure: At least with 5.11.11 and on my boards, > cd-gpio ACTIVE_LOW being specified in the sopine.dtsi takes precedence > over broken-cd being specified in pine64-lts.dts. Could that spoil the > plan of disabling cd-gpio for the LTS while leaving it enabled for > Sopine and baseboard? I am not sure what you mean with "takes precedence"? And there should be no active-low anymore, after that other patch of mine[1]. The CD GPIO and broken-cd are somewhat independent: Whenever a CD GPIO is specified in the DT, the code will pick it up and use any level changes, but only to trigger the actual card detection routine (over the SD bus). If "broken-cd" is NOT specified, it will also try to register an interrupt handler for any pin change, so it doesn't need to poll and can use it as a wakeup source. So I specified a different GPIO and connected a switch to that pin, to simulate the different cases: - Having broken-cd didn't hurt in any way, maybe apart from the missing wakeup source (which I didn't test). - Having no CD GPIO property, but broken-cd worked equally fine, every card insertion or removal was detected. - Having the CD GPIO at somewhat random (switch not corresponding to actual card insertion state), but still having broken-cd, did not do any harm either: it always looked at the actual card state and still detected any card change, even without a pin change. - The only problematic situation is "no broken-cd, but wrong CD GPIO": card removals are detected, but the card insertion checks would rely on the CD pin reading "card inserted". Not sure if there is a way to force card detection from the prompt somehow. This last case however is only a problem is the CD GPIO never reads "card inserted". In our case (active high, as inherited from the SOPine), it works even without broken-cd on those broken boards: the "stuck at 1" means "card always inserted", which correctly detects every removal and insertion. So given the above (broken-cd not doing any harm, but correctly describing the situation on some boards), I would like to see both an active-high CD GPIO and the broken-cd property to be in. I will send a v2 in a minute. > > Behaviour is as such: With cd-gpios ACTIVE_LOW in sopine.dtsi and Yes, that is wrong, and I sent a patch already [1]. > broken-cd in pine64-lts.dts, card insertion, removal and reinsertion is > not detected after booting the kernel without a card in the slot. With > cd-gpios ACTIVE_LOW removed from sopine.dtsi it starts working. Yes, that confirms my findings above. ACTIVE_LOW is the culprit here, with ACTIVE_HIGH it should work. > > In diffs for added clarity: > > PAGER= git log --pretty=oneline HEAD~1..HEAD > aa7258f8f3d48a29bc024ea8c5145bdc4a980e4d (HEAD, tag: v5.11.11) Linux 5.11.11 > > - not working on its own: > > index 302e24be0a31..5b0c21e68352 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts > @@ -8,3 +8,7 @@ / { > compatible = "pine64,pine64-lts", "allwinner,sun50i-r18", > "allwinner,sun50i-a64"; > }; > + > +&mmc0 { > + broken-cd; > +}; > > - working with this additional change: > > index 3402cec87035..ba2b7398993b 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi > @@ -34,7 +34,6 @@ &mmc0 { > vmmc-supply = <®_dcdc1>; > disable-wp; > bus-width = <4>; > - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 */ It should work with this line still in, but reading ACTIVE_HIGH, as per [1]. > status = "okay"; > }; > > > So for your particular board (version) you could actually remove the > > whole &mmc0 node override, use the same node as the SOPine (working > > active-high CD) and it should work. > > Correct. Again for reasons of laziness I tested with the dts from > 5.11.11 which is currently running on the board and which still has > ACTIVE_LOW in sopine.dtsi. Sorry for not being clearer about that. > > But somewhat lucky as well because without ACTIVE_LOW still being set in > sopine.dtsi I wouldn't have had a way to tell that broken-cd was not > taking effect and silently have tested the working ACTIVE_HIGH > definition from sopine.dtsi. > > Or am I somehow making a mess of this? > > > > BTW: My boards have a marking "PINE64-R18-V1_1" and below it > > > "2017-08-03" on their upper side. On the back it says on one sticker > > > "Model:PineA64 2GB LTS" and on another "2O1-PINE64R18-00" and > > > "PINE64-R18-V1.1 2G". Is CD being stuck at 1 a bug of revision 1.0 > > > perhaps? Is there a way to detect this difference in software and add > > > some sort of quirk handler for it? > > As Jernej mentioned, this would be U-Boot's task, but I don't see a > > good reason for it. Firstly, you would need to find a good automatic > > way of determining the board revision, which I doubt there is. And > > secondly, I don't see the benefit: It works quite nicely with > > broken-cd: card removals and insertions are detected automatically, > > it's just not as efficient (interrupt-driven) as it could be. > > Or do you see any problems with broken-cd? > > Of course not. My boards have their rootfs on mmc0, so the card is never > removed and replaced during operation anyway. I was just asking out of > curiosity. Ah, OK. I am running with initrds for those tests, so SD card status is not fatal. > And out of curiosity again: Could one have a device tree overlay > configured manually to be loaded by the bootloader that adds cd-gpio > ACTIVE_HIGH for mmc0 and disables/overrules broken-cd? Somewhat like so > (untested): ACTIVE_HIGH should be merged anytime soon, so we don't need that. And yes, you can remove broken-cd, but as shown above, I don't see why and it's certainly not worth the hassle from my point of view. > > /dts-v1/; > /plugin/; > > #include <dt-bindings/gpio/gpio.h> > > / { > fragment@0 { > target = <&mmc0>; > __overlay__ { > cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 push-push switch */ > }; > }; > }; If you really want to remove broken-cd from the DT, you can do much easier in U-Boot: => fdt addr $fdt_addr_r => fdt rm /soc/mmc broken-cd > > Here it would be useful if cd-gpios indeed took precedence over > broken-cd because my grepping of the code can't find a way to un-specify > it once set. See above, shouldn't be a problem. Cheers, Andre [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2021-March/645191.html
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts index e79ce49e7e6a..843338e19694 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts @@ -21,5 +21,5 @@ }; &mmc0 { - cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>; /* PF6 push-push switch */ + non-removable; /* card detect is broken on some boards */ };
Commit 941432d00768 ("arm64: dts: allwinner: Drop non-removable from SoPine/LTS SD card") enabled the card detect GPIO for the SOPine module, along the way with the Pine64-LTS, which share the same base .dtsi. This was based on the observation that the Pine64-LTS has as "push-push" SD card socket, and that the schematic mentions the card detect GPIO. After having received two reports about failing SD card access with that patch, some more research and polls on that subject revealed that there are at least two different versions of the Pine64-LTS out there: - On some boards (including mine) the card detect pin is "stuck" at high, regardless of an microSD card being inserted or not. - On other boards the card-detect is working, but is active-high, by virtue of an explicit inverter circuit, as shown in the schematic. To cover all versions of the board out there, and don't take any chances, let's revert the introduction of the CD GPIO, and go back to the non-removable property for the Pine64-LTS. That should avoid regressions and should work for everyone. The SOPine card detect has proven to be working, so let's keep that GPIO in place. Fixes: 941432d00768 ("arm64: dts: allwinner: Drop non-removable from SoPine/LTS SD card") Reported-by: Michael Weiser <michael.weiser@gmx.de> Reported-by: Daniel Kulesz <kuleszdl@posteo.org> Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-lts.dts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)