[PATCH 1/4] Made VMALLOC_END of type unsigned long.

[PATCH 1/4] Made VMALLOC_END of type unsigned long.

[Andreas Fenkart]

On Fri, Jun 19, 2009 at 10:46:40AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 16, 2009 at 02:38:07PM +0200, Andreas Fenkart wrote:
> > This makes VMALLOC_END consistent with VMALLOC_START
> > diff --git a/arch/arm/mach-davinci/include/mach/io.h b/arch/arm/mach-davinci/include/mach/io.h
> > index 2479785..81c4269 100644
> > --- a/arch/arm/mach-davinci/include/mach/io.h
> > +++ b/arch/arm/mach-davinci/include/mach/io.h
> > @@ -18,7 +18,7 @@
> >   * I/O mapping
> >   * ----------------------------------------------------------------------------
> >   */
> > -#define IO_PHYS		0x01c00000
> > +#define IO_PHYS		0x01c00000UL
> 
> Why is this change required?

Davinci defines VMALLOC_END this way:

#define VMALLOC_END   (IO_VIRT - (2<<20))   

The simple solution would be to prefix the VMALLOC_END with UL,
the alternative to make IO_VIRT unsinged long.

IO_VIRT and recurses are defined in mach/hardware.h
#define IO_PHYS   0x01c00000UL
#define IO_OFFSET 0xfd000000 /* Virtual IO = 0xfec00000 */
#define IO_SIZE   0x00400000
#define IO_VIRT   (IO_PHYS + IO_OFFSET)

Changing IO_VIRT to unsigned long should be fine. Virtual
addresses are mostly unsigned long,@least VMALLOC_START,
PAGE_OFFSET are. See below for complete list of IO_VIRT usage.

IO_PHYS should not be changed though, since it is assigned to
unsigned int fields.

So I changed the patch, and made only IO_VIRT unsigned long:

 #define IO_PHYS        0x01c00000
 #define IO_OFFSET      0xfd000000 /* Virtual IO = 0xfec00000 */
 #define IO_SIZE        0x00400000
-#define IO_VIRT        (IO_PHYS + IO_OFFSET)
+#define IO_VIRT        UL(IO_PHYS + IO_OFFSET)


Andreas

----

IO_VIRT:
-assigned to struct map_desc / unsigned long virtual;
-used as return value in void __iomem *davinci_ioremap function;
-used in define DA8XX_CP_INTC_VIRT, which recursively is used as:
--assigned to struct map_desc / unsigned long virtual;
--assigned to struct davinci_soc_info / void __iomem *intc_base;
-used in define of VMALLOC_END

IO_PHYS:
-assigned to struct machine_desc / unsigned int phys_io;
-assigned to struct machine_desc / unsigned int io_pg_offst;
-shifted and assigned to struct map_desc / unsigned long pfn;
-used defines for UART base address, which are:
--via plat_serial8250_port / resource_size_t mapbase
---assigned to type phys_addr_t
-used defines for TIMER base address, which recursively are:
--assigned to struct davinci_timer_instance / void __iomem *base;
-used defines for WDOG base address, which are:
--via struct resource / resource_size_t start
---assigned to type phys_addr_t

[PATCH 1/4] Made VMALLOC_END of type unsigned long.

[Russell King - ARM Linux]

On Thu, Oct 01, 2009 at 12:47:15PM +0200, Andreas Fenkart wrote:
> Davinci defines VMALLOC_END this way:
> 
> #define VMALLOC_END   (IO_VIRT - (2<<20))   
> 
> The simple solution would be to prefix the VMALLOC_END with UL,
> the alternative to make IO_VIRT unsinged long.

Is:

	(IO_VIRT - (2<<20))UL

valid C?  I don't think it is, and neither does my compiler.

> IO_PHYS should not be changed though, since it is assigned to
> unsigned int fields.

I'd actually argue that it's more correct for physical addresses to be
unsigned long rather than unsigned int.

> So I changed the patch, and made only IO_VIRT unsigned long:
> 
>  #define IO_PHYS        0x01c00000
>  #define IO_OFFSET      0xfd000000 /* Virtual IO = 0xfec00000 */
>  #define IO_SIZE        0x00400000
> -#define IO_VIRT        (IO_PHYS + IO_OFFSET)
> +#define IO_VIRT        UL(IO_PHYS + IO_OFFSET)

This doesn't work, for the same reason I point out above.  An expression
suffixed with 'UL' is not valid.  However, a pure number may be suffixed
with 'UL' to make it an unsigned long constant.

> IO_PHYS:
> -assigned to struct machine_desc / unsigned int phys_io;
> -assigned to struct machine_desc / unsigned int io_pg_offst;

These both can become unsigned long without any harm.

> -shifted and assigned to struct map_desc / unsigned long pfn;
> -used defines for UART base address, which are:
> --via plat_serial8250_port / resource_size_t mapbase
> ---assigned to type phys_addr_t
> -used defines for TIMER base address, which recursively are:
> --assigned to struct davinci_timer_instance / void __iomem *base;
> -used defines for WDOG base address, which are:
> --via struct resource / resource_size_t start
> ---assigned to type phys_addr_t

The rest are fine.

[PATCH 1/4] Made VMALLOC_END of type unsigned long.

[Uwe Kleine-König]

Hello,

On Thu, Oct 01, 2009 at 02:03:38PM +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 01, 2009 at 12:47:15PM +0200, Andreas Fenkart wrote:
> > Davinci defines VMALLOC_END this way:
> > 
> > #define VMALLOC_END   (IO_VIRT - (2<<20))   
> > 
> > The simple solution would be to prefix the VMALLOC_END with UL,
> > the alternative to make IO_VIRT unsinged long.
> 
> Is:
> 
> 	(IO_VIRT - (2<<20))UL
> 
> valid C?  I don't think it is, and neither does my compiler.
No, but

	 UL(IO_VIRT - (2<<20))

might work?!  Havn't tried though.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 1/4] Made VMALLOC_END of type unsigned long.

[Russell King - ARM Linux]

On Thu, Oct 01, 2009 at 08:32:48PM +0200, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Thu, Oct 01, 2009 at 02:03:38PM +0100, Russell King - ARM Linux wrote:
> > On Thu, Oct 01, 2009 at 12:47:15PM +0200, Andreas Fenkart wrote:
> > > Davinci defines VMALLOC_END this way:
> > > 
> > > #define VMALLOC_END   (IO_VIRT - (2<<20))   
> > > 
> > > The simple solution would be to prefix the VMALLOC_END with UL,
> > > the alternative to make IO_VIRT unsinged long.
> > 
> > Is:
> > 
> > 	(IO_VIRT - (2<<20))UL
> > 
> > valid C?  I don't think it is, and neither does my compiler.
> No, but
> 
> 	 UL(IO_VIRT - (2<<20))
> 
> might work?!  Havn't tried though.

'UL()' is our own macro which appends UL if building in C mode, otherwise
omits it if assembly:

arch/arm/include/asm/memory.h:#define UL(x) _AC(x, UL)

include/linux/const.h:
#ifdef __ASSEMBLY__
#define _AC(X,Y)        X
#define _AT(T,X)        X
#else
#define __AC(X,Y)       (X##Y)
#define _AC(X,Y)        __AC(X,Y)
#define _AT(T,X)        ((T)(X))
#endif

[PATCH 1/4] Made VMALLOC_END of type unsigned long.

[Andreas Fenkart]

Here it is, rebased against commit
7fa07729e439a6184bd824746d06a49cca553f15.

I added macros simplifying the printout message, as your suggested.
Patches are now split/merged differently so they describe one logical
step.

Unfortunately the macros you suggested trigger a checkpatch.pl
warning:

ERROR: Macros with complex values should be enclosed in parenthesis
#628: FILE: arm/mm/init.c:628:
+#define MLM(b, t) b, t, ((t) - (b)) >> 20

Resolving these results in a compiler warning:
arch/arm/mm/init.c:649: warning: left-hand operand of comma expression
has no effect

I resolved the compiler warning, since it likes an error too.

king regards

Andreas

[PATCH 1/4] Made VMALLOC_END of type unsigned long.

[Russell King - ARM Linux]

On Thu, Oct 01, 2009 at 12:48:55PM +0200, Andreas Fenkart wrote:
> Here it is, rebased against commit
> 7fa07729e439a6184bd824746d06a49cca553f15.
> 
> I added macros simplifying the printout message, as your suggested.
> Patches are now split/merged differently so they describe one logical
> step.
> 
> Unfortunately the macros you suggested trigger a checkpatch.pl
> warning:
> 
> ERROR: Macros with complex values should be enclosed in parenthesis
> #628: FILE: arm/mm/init.c:628:
> +#define MLM(b, t) b, t, ((t) - (b)) >> 20

This is a case where ignoring checkpatch.pl is the right thing to do.
It doesn't know what is trying to be achieved, and is trying to apply
an inappropriate rule.

I'd ignore this warning.