Re: [PATCH v3 2/4] cacheinfo: Check cache properties are present in DT

From: Florian Fainelli <f.fainelli@gmail.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Pierre Gondois <pierre.gondois@arm.com>,
	linux-kernel@vger.kernel.org, Radu Rendec <rrendec@redhat.com>,
	Alexandre Ghiti <alexghiti@rivosinc.com>,
	Conor Dooley <conor.dooley@microchip.com>,
	Will Deacon <will@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Palmer Dabbelt <palmer@rivosinc.com>,
	Gavin Shan <gshan@redhat.com>
Subject: Re: [PATCH v3 2/4] cacheinfo: Check cache properties are present in DT
Date: Thu, 13 Apr 2023 13:06:46 -0700	[thread overview]
Message-ID: <d44baa58-8c39-407c-db94-390bc0d12dbe@gmail.com> (raw)
In-Reply-To: <20230413195032.bw3yu7a6puqziinx@bogus>

On 4/13/23 12:50, Sudeep Holla wrote:
> On Thu, Apr 13, 2023 at 11:16:37AM -0700, Florian Fainelli wrote:
>> On 4/13/23 02:14, Pierre Gondois wrote:
>>> If a Device Tree (DT) is used, the presence of cache properties is
>>> assumed. Not finding any is not considered. For arm64 platforms,
>>> cache information can be fetched from the clidr_el1 register.
>>> Checking whether cache information is available in the DT
>>> allows to switch to using clidr_el1.
>>>
>>> init_of_cache_level()
>>> \-of_count_cache_leaves()
>>> will assume there a 2 cache leaves (L1 data/instruction caches), which
>>> can be different from clidr_el1 information.
>>>
>>> cache_setup_of_node() tries to read cache properties in the DT.
>>> If there are none, this is considered a success. Knowing no
>>> information was available would allow to switch to using clidr_el1.
>>>
>>> Fixes: de0df442ee49 ("cacheinfo: Check 'cache-unified' property to count cache leaves")
>>> Reported-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>>> Link: https://lore.kernel.org/all/20230404-hatred-swimmer-6fecdf33b57a@spud/
>>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>>
>> Humm, it would appear that the cache levels and topology is still provided,
>> despite the lack of cache properties in the Device Tree which is intended by
>> this patch set however we lost the size/ways/sets information, could we not
>> complement the missing properties here?
>>
> 
> I am confused. How and from where the information was fetched before this
> change ?

I applied Pierre's patches to my tree and then did the following:

- before means booting with the patches applied and the Device Tree 
providing cache information: {d,i}-cache-{size,line-size,sets} and 
next-level-cache

- after means removing all of those properties still with the patches 
applied

My expectation is that if we omit the properties in the Device Tree, we 
will fallback to reading that information out of clidr_el1. However as 
can be seen from the "before" and "after" outputs, there is loss of 
information, as we no longer have the cacheline size, number of 
sets/ways, the rest is valid though.

So my question is whether this is expected and in scope of what is being 
done here, or not.

> 
>> If this is out of the scope of what you are doing:
>>
>> Tested-by: Florian Fainelli <f.fainelli@gmail.com>
>>
> 
> Just looking at the lscpu output before and after, it looks something is
> broken. What am I missing here ?
> 

What is broken in the "before" output? It contains the entire set of 
possible information we know about the caches. As for the "after", well 
yes there is information missing, the whole point of my email actually...
-- 
Florian