2011-02-12 10:28:34

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH v4 15/19] ARM: LPAE: use phys_addr_t instead of unsigned long for physical addresses

On Mon, Jan 24, 2011 at 05:55:57PM +0000, Catalin Marinas wrote:
> arch/arm/include/asm/memory.h | 17 +++++++++--------
> arch/arm/include/asm/outercache.h | 14 ++++++++------
> arch/arm/include/asm/pgtable.h | 2 +-
> arch/arm/include/asm/setup.h | 2 +-
> arch/arm/kernel/setup.c | 5 +++--
> arch/arm/mm/init.c | 6 +++---
> arch/arm/mm/mmu.c | 7 ++++---
> 7 files changed, 29 insertions(+), 24 deletions(-)

If this is split up into four separate patches, we can probably sort out
merging this for the upcoming window.

asm/memory.h will conflict non-trivially with p2v patch set, but I think
we can merge the changes to everything but __virt_to_phys/__phys_to_virt.

asm/outercache.h changes are stand-alone.

asm/pgtable.h looks like it could use __pfn_to_phys(pfn) rather than
adding the cast, and can be combined with mm/init.c and mm/mmu.c.

asm/setup.h and arch/arm/kernel/setup.c form another logical group.


2011-02-15 11:52:33

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v4 15/19] ARM: LPAE: use phys_addr_t instead of unsigned long for physical addresses

Hi Russell,

On Sat, 2011-02-12 at 10:28 +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 24, 2011 at 05:55:57PM +0000, Catalin Marinas wrote:
> > arch/arm/include/asm/memory.h | 17 +++++++++--------
> > arch/arm/include/asm/outercache.h | 14 ++++++++------
> > arch/arm/include/asm/pgtable.h | 2 +-
> > arch/arm/include/asm/setup.h | 2 +-
> > arch/arm/kernel/setup.c | 5 +++--
> > arch/arm/mm/init.c | 6 +++---
> > arch/arm/mm/mmu.c | 7 ++++---
> > 7 files changed, 29 insertions(+), 24 deletions(-)
>
> If this is split up into four separate patches, we can probably sort out
> merging this for the upcoming window.
>
Excellent! I've split the patch up into four distinct parts, as per your
suggestions. I've submitted these to your patch system (6670/1-6673/1)
alongside a fixed version of the printf format patch (6669/1) because
without that, you get a bunch of compiler warnings.

Will

2011-02-15 12:35:38

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH v4 15/19] ARM: LPAE: use phys_addr_t instead of unsigned long for physical addresses

On Tue, Feb 15, 2011 at 11:52:22AM +0000, Will Deacon wrote:
> Excellent! I've split the patch up into four distinct parts, as per your
> suggestions. I've submitted these to your patch system (6670/1-6673/1)
> alongside a fixed version of the printf format patch (6669/1) because
> without that, you get a bunch of compiler warnings.

Except 6669/1 still suffers from "%#08llx". For a value of one, that prints:

0x000001

five zeros following the 0x rather than seven. The width in the format
string includes the 0x prefix.

2011-02-15 12:39:38

by Catalin Marinas

[permalink] [raw]
Subject: Re: [PATCH v4 15/19] ARM: LPAE: use phys_addr_t instead of unsigned long for physical addresses

On Tue, 2011-02-15 at 12:35 +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 15, 2011 at 11:52:22AM +0000, Will Deacon wrote:
> > Excellent! I've split the patch up into four distinct parts, as per your
> > suggestions. I've submitted these to your patch system (6670/1-6673/1)
> > alongside a fixed version of the printf format patch (6669/1) because
> > without that, you get a bunch of compiler warnings.
>
> Except 6669/1 still suffers from "%#08llx". For a value of one, that prints:
>
> 0x000001
>
> five zeros following the 0x rather than seven. The width in the format
> string includes the 0x prefix.

Ah, sorry, I only fixed one case and forgot about the rest (and
misleading Will).

--
Catalin

2011-02-15 13:37:17

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v4 15/19] ARM: LPAE: use phys_addr_t instead of unsigned long for physical addresses

On Tue, 2011-02-15 at 12:39 +0000, Catalin Marinas wrote:
> On Tue, 2011-02-15 at 12:35 +0000, Russell King - ARM Linux wrote:
> > On Tue, Feb 15, 2011 at 11:52:22AM +0000, Will Deacon wrote:
> > > Excellent! I've split the patch up into four distinct parts, as per your
> > > suggestions. I've submitted these to your patch system (6670/1-6673/1)
> > > alongside a fixed version of the printf format patch (6669/1) because
> > > without that, you get a bunch of compiler warnings.
> >
> > Except 6669/1 still suffers from "%#08llx". For a value of one, that prints:
> >
> > 0x000001
> >
> > five zeros following the 0x rather than seven. The width in the format
> > string includes the 0x prefix.
>
> Ah, sorry, I only fixed one case and forgot about the rest (and
> misleading Will).
>

I should've spotted this either way. I've superseded the old patch with
6674/1.

Apologies for the confusion,

Will


2011-02-15 14:31:16

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH v4 15/19] ARM: LPAE: use phys_addr_t instead of unsigned long for physical addresses

On Tue, Feb 15, 2011 at 01:37:07PM +0000, Will Deacon wrote:
> I should've spotted this either way. I've superseded the old patch with
> 6674/1.

One additional thing that I think has been lost. I said in the original
reply to Catalin:
| asm/memory.h will conflict non-trivially with p2v patch set, but I think
| we can merge the changes to everything but __virt_to_phys/__phys_to_virt.

So 6670/1 which I'm intending to apply to the p2v branch can't be merged
as-is. The ideal solution would be a version of 6670/1 to apply on top
of the existing p2v branch.

Thanks.

2011-02-15 15:27:01

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH v4 15/19] ARM: LPAE: use phys_addr_t instead of unsigned long for physical addresses

Hi Russell,

On Tue, 2011-02-15 at 14:23 +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 15, 2011 at 01:37:07PM +0000, Will Deacon wrote:
> > I should've spotted this either way. I've superseded the old patch with
> > 6674/1.
>
> One additional thing that I think has been lost. I said in the original
> reply to Catalin:
> | asm/memory.h will conflict non-trivially with p2v patch set, but I think
> | we can merge the changes to everything but __virt_to_phys/__phys_to_virt.
>
> So 6670/1 which I'm intending to apply to the p2v branch can't be merged
> as-is. The ideal solution would be a version of 6670/1 to apply on top
> of the existing p2v branch.
>

The conflict with the p2v branch is fairly hefty, but something like
this should do (if you're happy I'll submit it to replace 6670/1):

Note that because the v2p macros only work for lowmem, I've not bothered
to add casts for the __v2p macros (rather, I've just changed the types
of the static inline functions). This simplifies the code and means we
can stay clear of the runtime fixup stuff.


diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 015fd5e..791cb3e 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -15,6 +15,7 @@

#include <linux/compiler.h>
#include <linux/const.h>
+#include <linux/types.h>
#include <mach/memory.h>
#include <asm/sizes.h>

@@ -135,8 +136,8 @@
/*
* Convert a physical address to a Page Frame Number and back
*/
-#define __phys_to_pfn(paddr) ((paddr) >> PAGE_SHIFT)
-#define __pfn_to_phys(pfn) ((pfn) << PAGE_SHIFT)
+#define __phys_to_pfn(paddr) ((unsigned long)((paddr) >> PAGE_SHIFT))
+#define __pfn_to_phys(pfn) ((phys_addr_t)(pfn) << PAGE_SHIFT)

/*
* Convert a page to/from a physical address
@@ -234,12 +235,12 @@ static inline unsigned long __phys_to_virt(unsigned long x)
* translation for translating DMA addresses. Use the driver
* DMA support - see dma-mapping.h.
*/
-static inline unsigned long virt_to_phys(void *x)
+static inline phys_addr_t virt_to_phys(void *x)
{
return __virt_to_phys((unsigned long)(x));
}

-static inline void *phys_to_virt(unsigned long x)
+static inline void *phys_to_virt(phys_addr_t x)
{
return (void *)(__phys_to_virt((unsigned long)(x)));
}


Cheers,

Will

2011-02-15 15:48:44

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH v4 15/19] ARM: LPAE: use phys_addr_t instead of unsigned long for physical addresses

On Tue, Feb 15, 2011 at 03:26:49PM +0000, Will Deacon wrote:
> The conflict with the p2v branch is fairly hefty, but something like
> this should do (if you're happy I'll submit it to replace 6670/1):

I was thinking that was the case - which is why I wanted this split up
so this can be tackled separately.

> Note that because the v2p macros only work for lowmem, I've not bothered
> to add casts for the __v2p macros (rather, I've just changed the types
> of the static inline functions). This simplifies the code and means we
> can stay clear of the runtime fixup stuff.

This patch looks good enough, thanks.