Re: [PATCH] mmc: card: modify mmc_getgeo function

2011-09-26 Thread Russell King - ARM Linux 
On Mon, Sep 26, 2011 at 03:13:33PM +0200, Uwe Kleine-König wrote:
> 0028 :
>   28:   e3a03004mov r3, #4
>   2c:   e3a02010mov r2, #16
>   30:   e92d4010push{r4, lr}
>   34:   e1a04001mov r4, r1
>   38:   e5c13000strbr3, [r1]
>   3c:   e5c12001strbr2, [r1, #1]
>   40:   e590c050ldr ip, [r0, #80]   ; 0x50
>   44:   e3a02040mov r2, #64 ; 0x40
>   48:   e3a03000mov r3, #0
>   4c:   e1cc04d8ldrdr0, [ip, #72]   ; 0x48
>   50:   ebfebl  0 <__aeabi_uldivmod>
>   54:   e1c400b2strhr0, [r4, #2]

Yes, this is just silly.  A 64-bit by 64-bit division by a power-of-2
value to ultimately store a 16-bit value.

Even truncating the output from get_capacity() would be better and no
less (in)correct (it may look weird but what the assembly shows is that
it really doesn't matter).  u64 / 64 will give the same 16-bit result as
u64-truncated-to-u32 / 64.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: card: modify mmc_getgeo function

2011-09-26 Thread Uwe Kleine-König 
On Mon, Sep 26, 2011 at 08:41:13AM -0400, Chris Ball wrote:
> Hi,
> 
> On Mon, Sep 26 2011, Uwe Kleine-König wrote:
> >> Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
> > This (i.e. ee9e0e0 (mmc: card: Remove duplicated constants) in next)
> > makes gcc emit a reference to __aeabi_uldivmod in one of my nightly
> > builds which isn't defined.
> >
> > The final linking stage fails with:
> >
> >   LD  .tmp_vmlinux1
> > drivers/built-in.o: In function `mmc_blk_getgeo':
> > clkdev.c:(.text+0xd1528): undefined reference to `__aeabi_uldivmod'
> > make[2]: *** [.tmp_vmlinux1] Error 1
> > make[1]: *** [sub-make] Error 2
> > make: *** [all] Error 2
> 
> Interesting, thanks.  It builds fine here on my (gcc-4.6) ARM toolchains.
> Looking online, I think you're hitting an old gcc-4.3 bug?
Yeah, this is gcc 4.3.2. Is it too old to be supported?

Do you think of http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48783? Then
no, that is not the problem. The function is actually used:

0028 :
  28:   e3a03004mov r3, #4
  2c:   e3a02010mov r2, #16
  30:   e92d4010push{r4, lr}
  34:   e1a04001mov r4, r1
  38:   e5c13000strbr3, [r1]
  3c:   e5c12001strbr2, [r1, #1]
  40:   e590c050ldr ip, [r0, #80]   ; 0x50
  44:   e3a02040mov r2, #64 ; 0x40
  48:   e3a03000mov r3, #0
  4c:   e1cc04d8ldrdr0, [ip, #72]   ; 0x48
  50:   ebfebl  0 <__aeabi_uldivmod>
  54:   e1c400b2strhr0, [r4, #2]
  58:   e3a0mov r0, #0
  5c:   e8bd8010pop {r4, pc}

I thought that the problem is more:

> The kernel doesn't support 64-bit by 64-bit division at all then?  
Nope.  64-bit by 64-bit divides are grossly inefficient and should be
avoided.

(taken from http://www.spinics.net/lists/arm/msg13532.html)

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: card: modify mmc_getgeo function

2011-09-26 Thread Russell King - ARM Linux 
On Mon, Sep 26, 2011 at 08:41:13AM -0400, Chris Ball wrote:
> Hi,
> 
> On Mon, Sep 26 2011, Uwe Kleine-König wrote:
> >> Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
> > This (i.e. ee9e0e0 (mmc: card: Remove duplicated constants) in next)
> > makes gcc emit a reference to __aeabi_uldivmod in one of my nightly
> > builds which isn't defined.
> >
> > The final linking stage fails with:
> >
> >   LD  .tmp_vmlinux1
> > drivers/built-in.o: In function `mmc_blk_getgeo':
> > clkdev.c:(.text+0xd1528): undefined reference to `__aeabi_uldivmod'
> > make[2]: *** [.tmp_vmlinux1] Error 1
> > make[1]: *** [sub-make] Error 2
> > make: *** [all] Error 2
> 
> Interesting, thanks.  It builds fine here on my (gcc-4.6) ARM toolchains.
> Looking online, I think you're hitting an old gcc-4.3 bug?

Check your setting of CONFIG_LBDAF - the return type from get_capacity
depends on this (which may be either unsigned long or u64).

Now, the thing about a constant division by (16*4) is that its relatively
easy for gcc to spot that this is the same as a shift - and use a shift
instead of a divide for both the unsigned long and u64 cases.

However, the change may result in gcc no longer realizing that it's a
constant division by a power-of-2, and that optimization can be applied.

If you want to eliminate these constants, I'd suggest two definitions

MMC_GEO_SECTORS
MMC_GEO_HEADS

and just subsituting the '4' and '16' in the function with the appropriate
symbolic constants.  That'd avoid causing this regression.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: card: modify mmc_getgeo function

2011-09-26 Thread Chris Ball 
Hi,

On Mon, Sep 26 2011, Uwe Kleine-König wrote:
>> Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
> This (i.e. ee9e0e0 (mmc: card: Remove duplicated constants) in next)
> makes gcc emit a reference to __aeabi_uldivmod in one of my nightly
> builds which isn't defined.
>
> The final linking stage fails with:
>
> LD  .tmp_vmlinux1
>   drivers/built-in.o: In function `mmc_blk_getgeo':
>   clkdev.c:(.text+0xd1528): undefined reference to `__aeabi_uldivmod'
>   make[2]: *** [.tmp_vmlinux1] Error 1
>   make[1]: *** [sub-make] Error 2
>   make: *** [all] Error 2

Interesting, thanks.  It builds fine here on my (gcc-4.6) ARM toolchains.
Looking online, I think you're hitting an old gcc-4.3 bug?

- Chris.
-- 
Chris Ball  
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: card: modify mmc_getgeo function

2011-09-26 Thread Uwe Kleine-König 
Hello,

On Wed, Sep 21, 2011 at 02:52:08PM -0400, Chris Ball wrote:
> Hi,
> 
> On Tue, Sep 20 2011, Girish K S wrote:
> > In the earlier code the cylinder, sector and head are assigned
> > independently. Current patch generates the cylinder number
> > with the values of sector and head.
> > This patch only makes they cylinder value to be dependent on
> > the sector and head.
> >
> > Signed-off-by: Girish K S 
> > ---
> >  drivers/mmc/card/block.c |3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> > index 1ff5486..bebb13b 100644
> > --- a/drivers/mmc/card/block.c
> > +++ b/drivers/mmc/card/block.c
> > @@ -226,9 +226,10 @@ static int mmc_blk_release(struct gendisk *disk, 
> > fmode_t mode)
> >  static int
> >  mmc_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
> >  {
> > -   geo->cylinders = get_capacity(bdev->bd_disk) / (4 * 16);
> > geo->heads = 4;
> > geo->sectors = 16;
> > +   geo->cylinders = get_capacity(bdev->bd_disk) /
> > +   (geo->heads * geo->sectors);
> > return 0;
> >  }
> 
> Thanks, pushed to mmc-next for 3.2 with a reworded commit message:
This (i.e. ee9e0e0 (mmc: card: Remove duplicated constants) in next)
makes gcc emit a reference to __aeabi_uldivmod in one of my nightly
builds which isn't defined.

The final linking stage fails with:

  LD  .tmp_vmlinux1
drivers/built-in.o: In function `mmc_blk_getgeo':
clkdev.c:(.text+0xd1528): undefined reference to `__aeabi_uldivmod'
make[2]: *** [.tmp_vmlinux1] Error 1
make[1]: *** [sub-make] Error 2
make: *** [all] Error 2

(I don't know why clkdev.c is referenced here. I'm not using ccache.)

It seems gcc isn't smart enough to notice that it can just use the same
generated code ...

Having said that AFAIK the code used before wasn't ok, too. (I.e. an u64
division that was just noticed to be a shift by luck.)

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: card: modify mmc_getgeo function

2011-09-21 Thread Chris Ball 
Hi,

On Tue, Sep 20 2011, Girish K S wrote:
> In the earlier code the cylinder, sector and head are assigned
> independently. Current patch generates the cylinder number
> with the values of sector and head.
> This patch only makes they cylinder value to be dependent on
> the sector and head.
>
> Signed-off-by: Girish K S 
> ---
>  drivers/mmc/card/block.c |3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 1ff5486..bebb13b 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -226,9 +226,10 @@ static int mmc_blk_release(struct gendisk *disk, fmode_t 
> mode)
>  static int
>  mmc_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
>  {
> - geo->cylinders = get_capacity(bdev->bd_disk) / (4 * 16);
>   geo->heads = 4;
>   geo->sectors = 16;
> + geo->cylinders = get_capacity(bdev->bd_disk) /
> + (geo->heads * geo->sectors);
>   return 0;
>  }

Thanks, pushed to mmc-next for 3.2 with a reworded commit message:

Author: Girish K S 
Date:   Tue Sep 20 16:38:49 2011 +0530

mmc: card: Remove duplicated constants

Reuse heads/sectors instead of duplicating them in the cylinders
calculation.

Signed-off-by: Girish K S 
Signed-off-by: Chris Ball 

- Chris.
-- 
Chris Ball  
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mmc: card: modify mmc_getgeo function

2011-09-20 Thread Girish K S 
In the earlier code the cylinder, sector and head are assigned
independently. Current patch generates the cylinder number
with the values of sector and head.
This patch only makes they cylinder value to be dependent on
the sector and head.

Signed-off-by: Girish K S 
---
 drivers/mmc/card/block.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 1ff5486..bebb13b 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -226,9 +226,10 @@ static int mmc_blk_release(struct gendisk *disk, fmode_t 
mode)
 static int
 mmc_blk_getgeo(struct block_device *bdev, struct hd_geometry *geo)
 {
-   geo->cylinders = get_capacity(bdev->bd_disk) / (4 * 16);
geo->heads = 4;
geo->sectors = 16;
+   geo->cylinders = get_capacity(bdev->bd_disk) /
+   (geo->heads * geo->sectors);
return 0;
 }
 
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html