[U-Boot,v3,4/6] PPC: 85xx: Generalize DDR TLB mapping function

The DDR mapping function really is just a generic virtual -> physical
mapping function. Generalize it so it can support any virtual starting
offset and IO maps just the same.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/cpu/mpc85xx/tlb.c |   48 ++++++++++++++++++++++++++--------------
 arch/powerpc/include/asm/mmu.h |    3 +++
 2 files changed, 34 insertions(+), 17 deletions(-)

On Tue, 2014-02-11 at 01:10 +0100, Alexander Graf wrote:
> -	if (memsize)
> -		print_size(memsize, " left unmapped\n");
> +	if (size)
> +		print_size(size, " left unmapped\n");
> +}

The print_size should move to the caller, with some way to pass back the
amout left unmapped.  Non-RAM callers would treat a non-zero unmapped
value as an error.

> +unsigned int
> +setup_ddr_tlbs_phys(phys_addr_t p_addr, unsigned int memsize_in_meg)
> +{
> +	unsigned int ram_tlb_address = (unsigned int)CONFIG_SYS_DDR_SDRAM_BASE;
> +	u64 memsize = (u64)memsize_in_meg << 20;
> +
> +	memsize = min(memsize, CONFIG_MAX_MEM_MAPPED);
> +	tlb_map_range(ram_tlb_address, p_addr, memsize, true);
>  	return memsize_in_meg;
>  }

Here you seem to be hiding the message for RAM.

York, are you OK with just removing the message altogether, and having
tlb_map_range return a normal error code if it can't map everything
(with DDR size reduced in advance as above)?

> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
> index cadaeef..5493c51 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -509,6 +509,9 @@ extern void print_tlbcam(void);
>  extern unsigned int setup_ddr_tlbs(unsigned int memsize_in_meg);
>  extern void clear_ddr_tlbs(unsigned int memsize_in_meg);
>  
> +extern void tlb_map_range(ulong v_addr, phys_addr_t p_addr, uint64_t size,
> +			  bool is_ram);

bool arguments tend to be hard to read at call sites -- flags (or enum)
with something like MAP_RAM/MAP_IO would be nicer (I'm not insisting,
though).

-Scott

On 19.02.2014, at 00:21, Scott Wood <scottwood@freescale.com> wrote:

> On Tue, 2014-02-11 at 01:10 +0100, Alexander Graf wrote:
>> -	if (memsize)
>> -		print_size(memsize, " left unmapped\n");
>> +	if (size)
>> +		print_size(size, " left unmapped\n");
>> +}
> 
> The print_size should move to the caller, with some way to pass back the
> amout left unmapped.  Non-RAM callers would treat a non-zero unmapped
> value as an error.
> 
>> +unsigned int
>> +setup_ddr_tlbs_phys(phys_addr_t p_addr, unsigned int memsize_in_meg)
>> +{
>> +	unsigned int ram_tlb_address = (unsigned int)CONFIG_SYS_DDR_SDRAM_BASE;
>> +	u64 memsize = (u64)memsize_in_meg << 20;
>> +
>> +	memsize = min(memsize, CONFIG_MAX_MEM_MAPPED);
>> +	tlb_map_range(ram_tlb_address, p_addr, memsize, true);
>> 	return memsize_in_meg;
>> }
> 
> Here you seem to be hiding the message for RAM.

It could still fail if we're running out of TLB entries, no?

> York, are you OK with just removing the message altogether, and having
> tlb_map_range return a normal error code if it can't map everything
> (with DDR size reduced in advance as above)?

How about we just change the return value of tlb_map_range to uint64_t and return size? That way we can 1:1 move the print code out of the function into the RAM map code and IO callers can just call assert(r != 0).

> 
>> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
>> index cadaeef..5493c51 100644
>> --- a/arch/powerpc/include/asm/mmu.h
>> +++ b/arch/powerpc/include/asm/mmu.h
>> @@ -509,6 +509,9 @@ extern void print_tlbcam(void);
>> extern unsigned int setup_ddr_tlbs(unsigned int memsize_in_meg);
>> extern void clear_ddr_tlbs(unsigned int memsize_in_meg);
>> 
>> +extern void tlb_map_range(ulong v_addr, phys_addr_t p_addr, uint64_t size,
>> +			  bool is_ram);
> 
> bool arguments tend to be hard to read at call sites -- flags (or enum)
> with something like MAP_RAM/MAP_IO would be nicer (I'm not insisting,
> though).

Good point.


Alex

On Thu, 2014-02-20 at 11:25 +0100, Alexander Graf wrote:
> On 19.02.2014, at 00:21, Scott Wood <scottwood@freescale.com> wrote:
> 
> > On Tue, 2014-02-11 at 01:10 +0100, Alexander Graf wrote:
> >> -	if (memsize)
> >> -		print_size(memsize, " left unmapped\n");
> >> +	if (size)
> >> +		print_size(size, " left unmapped\n");
> >> +}
> > 
> > The print_size should move to the caller, with some way to pass back the
> > amout left unmapped.  Non-RAM callers would treat a non-zero unmapped
> > value as an error.
> > 
> >> +unsigned int
> >> +setup_ddr_tlbs_phys(phys_addr_t p_addr, unsigned int memsize_in_meg)
> >> +{
> >> +	unsigned int ram_tlb_address = (unsigned int)CONFIG_SYS_DDR_SDRAM_BASE;
> >> +	u64 memsize = (u64)memsize_in_meg << 20;
> >> +
> >> +	memsize = min(memsize, CONFIG_MAX_MEM_MAPPED);
> >> +	tlb_map_range(ram_tlb_address, p_addr, memsize, true);
> >> 	return memsize_in_meg;
> >> }
> > 
> > Here you seem to be hiding the message for RAM.
> 
> It could still fail if we're running out of TLB entries, no?

That's not the usual reason for that message to be printed.

> > York, are you OK with just removing the message altogether, and having
> > tlb_map_range return a normal error code if it can't map everything
> > (with DDR size reduced in advance as above)?
> 
> How about we just change the return value of tlb_map_range to uint64_t
> and return size? That way we can 1:1 move the print code out of the
> function into the RAM map code and IO callers can just call assert(r !=
> 0).

That's fine.  I was just wondering if the message had value at all,
given that it's expected if you have more than 2 GiB of RAM.
 
-Scott