[PATCH] mini2440 MMC correct write protect detection

[PATCH] mini2440 MMC correct write protect detection

[Cedric Roux]

mini2440 MMC correct write protect detection

The mini2440 computer uses "active high" to signal that the "write protect"
of the inserted MMC is set. The current code uses the opposite, leading to
a wrong detection of write protection. The solution is simply to use
".wprotect_invert = 1" in the description of the MMC.

Signed-off-by: Cedric Roux <sed@free.fr>

--- arch/arm/mach-s3c24xx/mach-mini2440.c.orig	2018-09-04 22:15:20.696087528 +0200
+++ arch/arm/mach-s3c24xx/mach-mini2440.c	2018-09-04 22:15:32.708088023 +0200
@@ -232,6 +232,7 @@ static struct s3c2410fb_mach_info mini24
 /* MMC/SD  */
 
 static struct s3c24xx_mci_pdata mini2440_mmc_cfg __initdata = {
+   .wprotect_invert = 1,
    .gpio_detect   = S3C2410_GPG(8),
    .gpio_wprotect = S3C2410_GPH(8),
    .set_power     = NULL,

[PATCH] mini2440 MMC correct write protect detection

[Krzysztof Kozlowski]

On Tue, Sep 04, 2018 at 10:36:35PM +0200, Cedric Roux wrote:
> mini2440 MMC correct write protect detection
> 
> The mini2440 computer uses "active high" to signal that the "write protect"
> of the inserted MMC is set. The current code uses the opposite, leading to
> a wrong detection of write protection. The solution is simply to use
> ".wprotect_invert = 1" in the description of the MMC.

I looked at Mini2440 schematics found on the net and it looks like the
pin (just like CD) is active low. However I might be looking at wrong
schematics or missing some things.

This is really an old code so I am just quite surprised that it was not
reported before. Not able to write to SD card (for example if it is
rootfs) should be spotted quite early.

Best regards,
Krzysztof


> 
> Signed-off-by: Cedric Roux <sed@free.fr>
> 
> --- arch/arm/mach-s3c24xx/mach-mini2440.c.orig	2018-09-04 22:15:20.696087528 +0200
> +++ arch/arm/mach-s3c24xx/mach-mini2440.c	2018-09-04 22:15:32.708088023 +0200
> @@ -232,6 +232,7 @@ static struct s3c2410fb_mach_info mini24
>  /* MMC/SD  */
>  
>  static struct s3c24xx_mci_pdata mini2440_mmc_cfg __initdata = {
> +   .wprotect_invert = 1,
>     .gpio_detect   = S3C2410_GPG(8),
>     .gpio_wprotect = S3C2410_GPH(8),
>     .set_power     = NULL,

[PATCH] mini2440 MMC correct write protect detection

[Cedric Roux]

On 09/05/2018 07:45 PM, Krzysztof Kozlowski wrote:
> I looked at Mini2440 schematics found on the net and it looks like the
> pin (just like CD) is active low. However I might be looking at wrong
> schematics or missing some things.

I have the same schematics I think.
But I have a real mini2440 and I am positively sure that WP
is active high. CD is active low, yes.

Do you know someone that you trust that could confirm this?

> 
> This is really an old code so I am just quite surprised that it was not
> reported before. Not able to write to SD card (for example if it is
> rootfs) should be spotted quite early.

I don't think anyone tried a recent kernel, that's why.
And the mini2440 has been kind of replaced by the mini6410,
I'm not sure it's still in production. Apart from me, I
don't think anyone still uses it. If someone does, I
heavily doubt that person will try to boot a 4.something
kernel on it.

In the 2.6.32.63 kernel it's all different.

We have s3cmci_get_ro in drivers/mmc/host/s3cmci.c
that calls s3c2410_gpio_getpin in arch/arm/plat-s3c24xx/gpio.c
that returns the WP bit as is read from the GPIO.
s3cmci_get_ro will return > 0 if the bit is set and
wprotect_invert is not set. For mini2440, wprotect_invert
is not set (actually it does not seem to be set anywhere
in the kernel tree, is it used at all in 2.6.32.63?). So
it's returned as is. Getting 1 from GPIO means Read Only.

Then we have s3cmci_card_present, also in drivers/mmc/host/s3cmci.c,
that reads the CD bit from the GPIO, but take the opposite value
and then returns the opposite of the result if detect_invert
is 1. For mini2440 it's not 1, but 0. So we return the opposite
of the CD bit read from the GPIO, which means active low.

I'm not an electronic professional so I don't know how to interpret
the schematics. My experiment with my mini2440 says that:
- no card:         WP bit = 1, CD bit = 1
- card read/write: WP bit = 0, CD bit = 0
- card read only:  WP bit = 1, CD bit = 0
By "WP bit" I mean the value read from GPH8.
By "CD bit" I mean the value read from GPG8.

So I don't know what to do. If you don't trust me and you know
no one that you trust that would confirm my findings, then it's
game over.

I can do things that you ask me to do to prove my claims.

I can provide a simple userland program to be run on a mini2440
that demonstrates this as well (open /dev/mem, mmap 0x56000000,
reads GPG and GPH and prints the values).

Regards,
C?dric.

[PATCH] mini2440 MMC correct write protect detection

[Krzysztof Kozlowski]

On Wed, Sep 05, 2018 at 09:45:15PM +0200, Cedric Roux wrote:
> On 09/05/2018 07:45 PM, Krzysztof Kozlowski wrote:
> > I looked at Mini2440 schematics found on the net and it looks like the
> > pin (just like CD) is active low. However I might be looking at wrong
> > schematics or missing some things.
> 
> I have the same schematics I think.
> But I have a real mini2440 and I am positively sure that WP
> is active high. CD is active low, yes.
> 
> Do you know someone that you trust that could confirm this?
> 
> > 
> > This is really an old code so I am just quite surprised that it was not
> > reported before. Not able to write to SD card (for example if it is
> > rootfs) should be spotted quite early.
> 
> I don't think anyone tried a recent kernel, that's why.
> And the mini2440 has been kind of replaced by the mini6410,
> I'm not sure it's still in production. Apart from me, I
> don't think anyone still uses it. If someone does, I
> heavily doubt that person will try to boot a 4.something
> kernel on it.
> 
> In the 2.6.32.63 kernel it's all different.
> 
> We have s3cmci_get_ro in drivers/mmc/host/s3cmci.c
> that calls s3c2410_gpio_getpin in arch/arm/plat-s3c24xx/gpio.c
> that returns the WP bit as is read from the GPIO.
> s3cmci_get_ro will return > 0 if the bit is set and
> wprotect_invert is not set. For mini2440, wprotect_invert
> is not set (actually it does not seem to be set anywhere
> in the kernel tree, is it used at all in 2.6.32.63?). So
> it's returned as is. Getting 1 from GPIO means Read Only.
> 
> Then we have s3cmci_card_present, also in drivers/mmc/host/s3cmci.c,
> that reads the CD bit from the GPIO, but take the opposite value
> and then returns the opposite of the result if detect_invert
> is 1. For mini2440 it's not 1, but 0. So we return the opposite
> of the CD bit read from the GPIO, which means active low.
> 
> I'm not an electronic professional so I don't know how to interpret
> the schematics. My experiment with my mini2440 says that:
> - no card:         WP bit = 1, CD bit = 1
> - card read/write: WP bit = 0, CD bit = 0
> - card read only:  WP bit = 1, CD bit = 0
> By "WP bit" I mean the value read from GPH8.
> By "CD bit" I mean the value read from GPG8.
> 
> So I don't know what to do. If you don't trust me and you know
> no one that you trust that would confirm my findings, then it's
> game over.
> 
> I can do things that you ask me to do to prove my claims.
> 
> I can provide a simple userland program to be run on a mini2440
> that demonstrates this as well (open /dev/mem, mmap 0x56000000,
> reads GPG and GPH and prints the values).

You provided really good explanation so let's go with your patch.

Thanks, applied.

Best regards,
Krzysztof

[PATCH] mini2440 MMC correct write protect detection

[Krzysztof Kozlowski]

On Thu, Sep 06, 2018 at 05:53:39PM +0200, Krzysztof Kozlowski wrote:
> On Wed, Sep 05, 2018 at 09:45:15PM +0200, Cedric Roux wrote:
> > On 09/05/2018 07:45 PM, Krzysztof Kozlowski wrote:
> > > I looked at Mini2440 schematics found on the net and it looks like the
> > > pin (just like CD) is active low. However I might be looking at wrong
> > > schematics or missing some things.
> > 
> > I have the same schematics I think.
> > But I have a real mini2440 and I am positively sure that WP
> > is active high. CD is active low, yes.
> > 
> > Do you know someone that you trust that could confirm this?
> > 
> > > 
> > > This is really an old code so I am just quite surprised that it was not
> > > reported before. Not able to write to SD card (for example if it is
> > > rootfs) should be spotted quite early.
> > 
> > I don't think anyone tried a recent kernel, that's why.
> > And the mini2440 has been kind of replaced by the mini6410,
> > I'm not sure it's still in production. Apart from me, I
> > don't think anyone still uses it. If someone does, I
> > heavily doubt that person will try to boot a 4.something
> > kernel on it.
> > 
> > In the 2.6.32.63 kernel it's all different.
> > 
> > We have s3cmci_get_ro in drivers/mmc/host/s3cmci.c
> > that calls s3c2410_gpio_getpin in arch/arm/plat-s3c24xx/gpio.c
> > that returns the WP bit as is read from the GPIO.
> > s3cmci_get_ro will return > 0 if the bit is set and
> > wprotect_invert is not set. For mini2440, wprotect_invert
> > is not set (actually it does not seem to be set anywhere
> > in the kernel tree, is it used at all in 2.6.32.63?). So
> > it's returned as is. Getting 1 from GPIO means Read Only.
> > 
> > Then we have s3cmci_card_present, also in drivers/mmc/host/s3cmci.c,
> > that reads the CD bit from the GPIO, but take the opposite value
> > and then returns the opposite of the result if detect_invert
> > is 1. For mini2440 it's not 1, but 0. So we return the opposite
> > of the CD bit read from the GPIO, which means active low.
> > 
> > I'm not an electronic professional so I don't know how to interpret
> > the schematics. My experiment with my mini2440 says that:
> > - no card:         WP bit = 1, CD bit = 1
> > - card read/write: WP bit = 0, CD bit = 0
> > - card read only:  WP bit = 1, CD bit = 0
> > By "WP bit" I mean the value read from GPH8.
> > By "CD bit" I mean the value read from GPG8.
> > 
> > So I don't know what to do. If you don't trust me and you know
> > no one that you trust that would confirm my findings, then it's
> > game over.
> > 
> > I can do things that you ask me to do to prove my claims.
> > 
> > I can provide a simple userland program to be run on a mini2440
> > that demonstrates this as well (open /dev/mem, mmap 0x56000000,
> > reads GPG and GPH and prints the values).
> 
> You provided really good explanation so let's go with your patch.
> 
> Thanks, applied.

... and dropped. Checkpatch fails:
https://krzk.eu/#/builders/26/builds/121

Please fix checkpatch errors and resubmit with proper patch style and
adjusted title: "ARM: s3c24xx: Correct SD card write protect detection
on Mini2440".


Best regards,
Krzysztof

[PATCH] mini2440 MMC correct write protect detection

[Krzysztof Kozlowski]

On Wed, Sep 05, 2018 at 09:45:15PM +0200, Cedric Roux wrote:
> On 09/05/2018 07:45 PM, Krzysztof Kozlowski wrote:
> > I looked at Mini2440 schematics found on the net and it looks like the
> > pin (just like CD) is active low. However I might be looking at wrong
> > schematics or missing some things.
> 
> I have the same schematics I think.
> But I have a real mini2440 and I am positively sure that WP
> is active high. CD is active low, yes.
> 
> Do you know someone that you trust that could confirm this?
> 
> > 
> > This is really an old code so I am just quite surprised that it was not
> > reported before. Not able to write to SD card (for example if it is
> > rootfs) should be spotted quite early.
> 
> I don't think anyone tried a recent kernel, that's why.
> And the mini2440 has been kind of replaced by the mini6410,
> I'm not sure it's still in production. Apart from me, I
> don't think anyone still uses it. If someone does, I
> heavily doubt that person will try to boot a 4.something
> kernel on it.
> 
> In the 2.6.32.63 kernel it's all different.
> 
> We have s3cmci_get_ro in drivers/mmc/host/s3cmci.c
> that calls s3c2410_gpio_getpin in arch/arm/plat-s3c24xx/gpio.c
> that returns the WP bit as is read from the GPIO.
> s3cmci_get_ro will return > 0 if the bit is set and
> wprotect_invert is not set. For mini2440, wprotect_invert
> is not set (actually it does not seem to be set anywhere
> in the kernel tree, is it used at all in 2.6.32.63?). So
> it's returned as is. Getting 1 from GPIO means Read Only.
> 
> Then we have s3cmci_card_present, also in drivers/mmc/host/s3cmci.c,
> that reads the CD bit from the GPIO, but take the opposite value
> and then returns the opposite of the result if detect_invert
> is 1. For mini2440 it's not 1, but 0. So we return the opposite
> of the CD bit read from the GPIO, which means active low.
> 
> I'm not an electronic professional so I don't know how to interpret
> the schematics. My experiment with my mini2440 says that:
> - no card:         WP bit = 1, CD bit = 1
> - card read/write: WP bit = 0, CD bit = 0
> - card read only:  WP bit = 1, CD bit = 0
> By "WP bit" I mean the value read from GPH8.
> By "CD bit" I mean the value read from GPG8.
> 
> So I don't know what to do. If you don't trust me and you know
> no one that you trust that would confirm my findings, then it's
> game over.
> 
> I can do things that you ask me to do to prove my claims.
> 
> I can provide a simple userland program to be run on a mini2440
> that demonstrates this as well (open /dev/mem, mmap 0x56000000,
> reads GPG and GPH and prints the values).

The patch was not patchwork compatible:
    Applying patch #10587863 using 'git am -s'
    Description: mini2440 MMC correct write protect detection
    Applying: mini2440 MMC correct write protect detection
    error: arm/mach-s3c24xx/mach-mini2440.c: does not exist in index

git-am by default expects -p1.

The preferred and easiest way to generate patches is to use git for your
work and then just:
    $ git format-patch -1
    $ scripts/get_maintainer 000*
    $ git send-email --to ........  000*

(the last commands can be even squashed into one but that is just
optimization).

If you do not use git, please pay attention to generate proper
applicable patches.

Best regards,
Krzysztof

[PATCH] mini2440 MMC correct write protect detection

[Cedric Roux]

On 09/06/2018 06:01 PM, Krzysztof Kozlowski wrote:
> The patch was not patchwork compatible:
>     Applying patch #10587863 using 'git am -s'
>     Description: mini2440 MMC correct write protect detection
>     Applying: mini2440 MMC correct write protect detection
>     error: arm/mach-s3c24xx/mach-mini2440.c: does not exist in index

My bad, sorry. I didn't read carefully
Documentation/process/submitting-patches.rst

> 
> git-am by default expects -p1.
> 
> The preferred and easiest way to generate patches is to use git for your
> work and then just:
>     $ git format-patch -1
>     $ scripts/get_maintainer 000*
>     $ git send-email --to ........  000*
> 
> (the last commands can be even squashed into one but that is just
> optimization).
> 
> If you do not use git, please pay attention to generate proper
> applicable patches.

I'll stick to git for the next ones. Thanks for the quick
tutorial. I'll also try to use the correct prefix in my emails.

Best regards,
C?dric.

> 
> Best regards,
> Krzysztof
>