Re: [PATCH 1/1] ARM: LPAE: use phys_addr_t instead of unsigned long in outercache hooks

[Leizhen (ThunderTown)]

On 2020/12/28 15:00, Arnd Bergmann wrote:
> On Fri, Dec 25, 2020 at 12:48 PM Zhen Lei <thunder.leizhen@xxxxxxxxxx> wrote:
>>
>> The outercache of some Hisilicon SOCs support physical addresses wider
>> than 32-bits. The unsigned long datatype is not sufficient for mapping
>> physical addresses >= 4GB. The commit ad6b9c9d78b9 ("ARM: 6671/1: LPAE:
>> use phys_addr_t instead of unsigned long in outercache functions") has
>> already modified the outercache functions. But the parameters of the
>> outercache hooks are not changed. This patch use phys_addr_t instead of
>> unsigned long in outercache hooks: inv_range, clean_range, flush_range.
>>
>> To ensure the outercache that does not support LPAE works properly, do
>> cast phys_addr_t to unsigned long by adding a middle-tier function.
>> For example:
>> -static void l2c220_inv_range(unsigned long start, unsigned long end)
>> +static void __l2c220_inv_range(unsigned long start, unsigned long end)
>>  {
>>         ...
>>  }
>> +static void l2c220_inv_range(phys_addr_t start, phys_addr_t end)
>> +{
>> +  __l2c220_inv_range(start, end);
>> +}
>>
>> Note that the outercache functions have been doing this cast before this
>> patch. So now, the cast is just moved to the middle-tier function.
>>
>> No functional change.
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
> 
> This looks reasonable in principle, but it would be helpful to
> understand better which SoCs are affected. In which way is
> this specific to Hisilicon implementations, and why would others
> not need this?

I answered at the end.

> 
> Wouldn't this also be needed by an Armada XP that supports
> more than 4GB of RAM but has an outer cache?

I don't know about the armada XP environment.

> 
> I suppose those SoCs using off-the-shelf Arm cores are either
> pre-LPAE and cannot address memory above 4GB, or they do
> not need the outer_cache interfaces.

I think so.

> 
>> diff --git a/arch/arm/mm/cache-feroceon-l2.c b/arch/arm/mm/cache-feroceon-l2.c
>> index 5c1b7a7b9af6300..ab1d8051bf832c9 100644
>> --- a/arch/arm/mm/cache-feroceon-l2.c
>> +++ b/arch/arm/mm/cache-feroceon-l2.c
>> @@ -168,7 +168,7 @@ static unsigned long calc_range_end(unsigned long start, unsigned long end)
>>         return range_end;
>>  }
>>
>> -static void feroceon_l2_inv_range(unsigned long start, unsigned long end)
>> +static void __feroceon_l2_inv_range(unsigned long start, unsigned long end)
>>  {
>>         /*
>>          * Clean and invalidate partial first cache line.
>> @@ -198,7 +198,12 @@ static void feroceon_l2_inv_range(unsigned long start, unsigned long end)
>>         dsb();
>>  }
>>
>> -static void feroceon_l2_clean_range(unsigned long start, unsigned long end)
>> +static void feroceon_l2_inv_range(phys_addr_t start, phys_addr_t end)
>> +{
>> +       __feroceon_l2_inv_range(start, end);
>> +}
>> +
> 
> What is this indirection for? It looks like you do this for all implementations,
> so the actual address gets truncated here.

Because these environments are all 32-bit physical addresses or only the lower
32-bit physical addresses need to be operated. But my environment operates 64-bit
physical address and sizeof(long) is 32. So need to change the datatype of the
outchache hooks.

 struct outer_cache_fns {
-	void (*inv_range)(unsigned long, unsigned long);
-	void (*clean_range)(unsigned long, unsigned long);
-	void (*flush_range)(unsigned long, unsigned long);
+	void (*inv_range)(phys_addr_t, phys_addr_t);
+	void (*clean_range)(phys_addr_t, phys_addr_t);
+	void (*flush_range)(phys_addr_t, phys_addr_t);
 	void (*flush_all)(void);

I added middle-tier function for all implementations, just to ensure that the
above changes do not have side effects on them.

> 
>        Arnd
> 
> .
>