fixup_bigphys_addr and ioremap64 question

fixup_bigphys_addr and ioremap64 question

[Benjamin Herrenschmidt]

While merging io.h between 32 and 64 bits arch/powerpc, I stumbled upon
this little gem :-)

So we have these:

 - If CONFIG_PHYS_64BIT is not set, nothing special. phys_addr_t is
defined to be 32 bits.

 - If it is set however, we have phys_addr_t defined to be 64 bits in
asm-ppc/mmu.h. 

So why do we have both ioremap and ioremap64 knowing that the former is
defined to take a phys_addr_t argument ?

Currently, we have both, with the only difference being that ioremap
calls ioremap64 but also passes the argument through a
fixup_bigphys_addr() function first.

It took me a while to find it ... it's not defined in generic code but
in platform code (ugh !). In fact, the only version of it we have in
arch/powerpc is in the 85xx support and does:

phys_addr_t fixup_bigphys_addr(phys_addr_t addr, phys_addr_t size)
{
	return addr;
};

So here's my question: Is it, as I think, some old mecanism that was
useful when ioremap didn't take a phys_addr_t argument and resources
didn't have 64 bits fields and thus we had a way to "remap" IOs from a
32 bits space into a 64 bits space in a platform specific way ?

Now, the big question is: do we still need that ?

If, as I expect, the answer is no, then I'll just remove it. I'll also
remove ioremap64 from arch/powerpc since ioremap can take a 64 bits
value directly.

Now, if the answer is yes, then I'll turn it into a ppc_md. call since
it its current form, it's just broken.

Ben.

Re: fixup_bigphys_addr and ioremap64 question

[Josh Boyer]

On Mon, 2006-11-06 at 14:17 +1100, Benjamin Herrenschmidt wrote:
> While merging io.h between 32 and 64 bits arch/powerpc, I stumbled upon
> this little gem :-)
> 
> So we have these:
> 
>  - If CONFIG_PHYS_64BIT is not set, nothing special. phys_addr_t is
> defined to be 32 bits.
> 
>  - If it is set however, we have phys_addr_t defined to be 64 bits in
> asm-ppc/mmu.h. 
> 
> So why do we have both ioremap and ioremap64 knowing that the former is
> defined to take a phys_addr_t argument ?
> 
> Currently, we have both, with the only difference being that ioremap
> calls ioremap64 but also passes the argument through a
> fixup_bigphys_addr() function first.
> 
> It took me a while to find it ... it's not defined in generic code but
> in platform code (ugh !). In fact, the only version of it we have in
> arch/powerpc is in the 85xx support and does:

It's in arch/ppc/syslib/44x_common.c and it's used to trap the least
significant 32 bits of an address and set the right ERPN for io space on
44x.  Something like that might be needed when 44x merges to
arch/powerpc.

josh

Re: fixup_bigphys_addr and ioremap64 question

[Benjamin Herrenschmidt]

> > So why do we have both ioremap and ioremap64 knowing that the former is
> > defined to take a phys_addr_t argument ?
> > 
> > Currently, we have both, with the only difference being that ioremap
> > calls ioremap64 but also passes the argument through a
> > fixup_bigphys_addr() function first.
> > 
> > It took me a while to find it ... it's not defined in generic code but
> > in platform code (ugh !). In fact, the only version of it we have in
> > arch/powerpc is in the 85xx support and does:
> 
> It's in arch/ppc/syslib/44x_common.c and it's used to trap the least
> significant 32 bits of an address and set the right ERPN for io space on
> 44x.  Something like that might be needed when 44x merges to
> arch/powerpc.

Well, my point is that since nowadays we have 64 bits struct resource
and 64 bits phys_addr_t passed to ioremap, do we still need that ? In
fact, in my upcoming patch merging io.h for 32 and 64 bits in
asm-powerpc, I've simply removed that hook and ioremap64 :-) I can add
it back still, but so far, I yet have to be convinced there is still a
good reason for that hook.

Ben.

Re: fixup_bigphys_addr and ioremap64 question

[Josh Boyer]

On Mon, 2006-11-06 at 15:09 +1100, Benjamin Herrenschmidt wrote:
> > > So why do we have both ioremap and ioremap64 knowing that the former is
> > > defined to take a phys_addr_t argument ?
> > > 
> > > Currently, we have both, with the only difference being that ioremap
> > > calls ioremap64 but also passes the argument through a
> > > fixup_bigphys_addr() function first.
> > > 
> > > It took me a while to find it ... it's not defined in generic code but
> > > in platform code (ugh !). In fact, the only version of it we have in
> > > arch/powerpc is in the 85xx support and does:
> > 
> > It's in arch/ppc/syslib/44x_common.c and it's used to trap the least
> > significant 32 bits of an address and set the right ERPN for io space on
> > 44x.  Something like that might be needed when 44x merges to
> > arch/powerpc.
> 
> Well, my point is that since nowadays we have 64 bits struct resource
> and 64 bits phys_addr_t passed to ioremap, do we still need that ? In
> fact, in my upcoming patch merging io.h for 32 and 64 bits in
> asm-powerpc, I've simply removed that hook and ioremap64 :-) I can add
> it back still, but so far, I yet have to be convinced there is still a
> good reason for that hook.

Hm.. that might be ok.  I'm hoping to get back to working on something
soon so I'll be able to tell more later.  Maybe Matt knows for sure.

It'll probably break out-of-tree drivers though once the merge happens
for 4xx.  Could we leave ioremap64 around for a bit with a deprecation
warning?

josh

Re: fixup_bigphys_addr and ioremap64 question

[Benjamin Herrenschmidt]

> Hm.. that might be ok.  I'm hoping to get back to working on something
> soon so I'll be able to tell more later.  Maybe Matt knows for sure.
> 
> It'll probably break out-of-tree drivers though once the merge happens
> for 4xx.  Could we leave ioremap64 around for a bit with a deprecation
> warning?

I'd rather not... I don't really care about out of tree drivers anyway.

Ben.

Re: fixup_bigphys_addr and ioremap64 question

[Matt Porter]

On Sun, Nov 05, 2006 at 10:16:20PM -0600, Josh Boyer wrote:
> On Mon, 2006-11-06 at 15:09 +1100, Benjamin Herrenschmidt wrote:
> > > > So why do we have both ioremap and ioremap64 knowing that the former is
> > > > defined to take a phys_addr_t argument ?
> > > > 
> > > > Currently, we have both, with the only difference being that ioremap
> > > > calls ioremap64 but also passes the argument through a
> > > > fixup_bigphys_addr() function first.
> > > > 
> > > > It took me a while to find it ... it's not defined in generic code but
> > > > in platform code (ugh !). In fact, the only version of it we have in
> > > > arch/powerpc is in the 85xx support and does:
> > > 
> > > It's in arch/ppc/syslib/44x_common.c and it's used to trap the least
> > > significant 32 bits of an address and set the right ERPN for io space on
> > > 44x.  Something like that might be needed when 44x merges to
> > > arch/powerpc.
> > 
> > Well, my point is that since nowadays we have 64 bits struct resource
> > and 64 bits phys_addr_t passed to ioremap, do we still need that ? In
> > fact, in my upcoming patch merging io.h for 32 and 64 bits in
> > asm-powerpc, I've simply removed that hook and ioremap64 :-) I can add
> > it back still, but so far, I yet have to be convinced there is still a
> > good reason for that hook.
> 
> Hm.. that might be ok.  I'm hoping to get back to working on something
> soon so I'll be able to tell more later.  Maybe Matt knows for sure.

Yes, that's the right way to go.  The history on this is that when
the 64-bit resource change came relatively recently, ioremap64 and
friends weren't killed off in arch/ppc.  ioremap64 only existed because
a long time ago several people blocked the idea of 64-bit resources
(on 32-bit platforms) and we needed a way around it plus all the
fixup garbage. Going forward, we now can handle everything through
ioremap() with no fixup stuff. There's a related issue with mmap and
64-bits but that's a separate issue yet that should now be addressed.

> It'll probably break out-of-tree drivers though once the merge happens
> for 4xx.  Could we leave ioremap64 around for a bit with a deprecation
> warning?

The kernel APIs break all the time. Every out of tree driver has to
have that built into their expectation when moving to a new kernel
so I don't see a good reason for making a special case here.

-Matt

Re: fixup_bigphys_addr and ioremap64 question

[Kumar Gala]

On Nov 6, 2006, at 10:45 AM, Matt Porter wrote:

> On Sun, Nov 05, 2006 at 10:16:20PM -0600, Josh Boyer wrote:
>> On Mon, 2006-11-06 at 15:09 +1100, Benjamin Herrenschmidt wrote:
>>>>> So why do we have both ioremap and ioremap64 knowing that the  
>>>>> former is
>>>>> defined to take a phys_addr_t argument ?
>>>>>
>>>>> Currently, we have both, with the only difference being that  
>>>>> ioremap
>>>>> calls ioremap64 but also passes the argument through a
>>>>> fixup_bigphys_addr() function first.
>>>>>
>>>>> It took me a while to find it ... it's not defined in generic  
>>>>> code but
>>>>> in platform code (ugh !). In fact, the only version of it we  
>>>>> have in
>>>>> arch/powerpc is in the 85xx support and does:
>>>>
>>>> It's in arch/ppc/syslib/44x_common.c and it's used to trap the  
>>>> least
>>>> significant 32 bits of an address and set the right ERPN for io  
>>>> space on
>>>> 44x.  Something like that might be needed when 44x merges to
>>>> arch/powerpc.
>>>
>>> Well, my point is that since nowadays we have 64 bits struct  
>>> resource
>>> and 64 bits phys_addr_t passed to ioremap, do we still need  
>>> that ? In
>>> fact, in my upcoming patch merging io.h for 32 and 64 bits in
>>> asm-powerpc, I've simply removed that hook and ioremap64 :-) I  
>>> can add
>>> it back still, but so far, I yet have to be convinced there is  
>>> still a
>>> good reason for that hook.
>>
>> Hm.. that might be ok.  I'm hoping to get back to working on  
>> something
>> soon so I'll be able to tell more later.  Maybe Matt knows for sure.
>
> Yes, that's the right way to go.  The history on this is that when
> the 64-bit resource change came relatively recently, ioremap64 and
> friends weren't killed off in arch/ppc.  ioremap64 only existed  
> because
> a long time ago several people blocked the idea of 64-bit resources
> (on 32-bit platforms) and we needed a way around it plus all the
> fixup garbage. Going forward, we now can handle everything through
> ioremap() with no fixup stuff. There's a related issue with mmap and
> 64-bits but that's a separate issue yet that should now be addressed.
>
>> It'll probably break out-of-tree drivers though once the merge  
>> happens
>> for 4xx.  Could we leave ioremap64 around for a bit with a  
>> deprecation
>> warning?
>
> The kernel APIs break all the time. Every out of tree driver has to
> have that built into their expectation when moving to a new kernel
> so I don't see a good reason for making a special case here.

On 85xx I just put the place holder in for handling the larger  
address space, but we never did anything real with it.  Now that we  
have 64-bit resources I'm happy to see ioremap() fixed and ioremap64  
go away.

- k