[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ce766986-79cb-4c7d-9acc-845ef1db9ad1@gmail.com>
Date: Sun, 16 Mar 2025 02:05:09 +0530
From: Sahil Siddiq <icegambit91@...il.com>
To: Stafford Horne <shorne@...il.com>
Cc: jonas@...thpole.se, stefan.kristiansson@...nalahti.fi,
sahilcdq@...ton.me, linux-openrisc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC] openrisc: Add cacheinfo support
Hi,
On 3/15/25 2:26 AM, Stafford Horne wrote:
> On Sat, Mar 15, 2025 at 01:34:03AM +0530, Sahil Siddiq wrote:
>> On 3/14/25 2:54 AM, Stafford Horne wrote:
>>> On Tue, Mar 11, 2025 at 12:43:57AM +0530, Sahil Siddiq wrote:
>>>> Add cacheinfo support for OpenRISC.
>>>>
>>>> [...]
>>>> None of the functions in drivers/base/cacheinfo.c that are capable of
>>>> pulling these details (e.g.: cache_size) have been used. This is because
>>>> they pull these details by reading properties present in the device tree
>>>> file. In setup.c, for example, the value of "clock-frequency" is pulled
>>>> from the device tree file.
>>>>
>>>> Cache related properties are currently not present in OpenRISC's device
>>>> tree files.
>>>
>>> If we want to add L2 caches and define them in the device tree would
>>> it "just work" or is more work needed?
>>>
>>
>> A little more work will have to be done. The implementation of "init_cache_level"
>> and "populate_cache_leaves" will have to be extended. To pull L2 cache attributes,
>> they'll need to make calls to the "of_get_property" family of functions similar to
>> what's being done for RISC-V and PowerPC.
>>
>> Shall I resubmit this patch with those extensions? I think I'll be able to test
>> those changes with a modified device tree file that has an L2 cache component.
>
> Since we don't have any such hardware now I don't think its needed.
> > [...]
>>> Currently this is the case, but it could be possible to create an SoC with L2
>>> caches. I could imagine these would be outside of the CPU and we could define
>>> them with the device tree.
>>
>> In this case, some extra work will have to be done to set the "shared_cpu_map"
>> appropriately. But I think the modifications will be quite small. If the L2 cache
>> is external to all CPUs, then all online CPUs will have their corresponding bit
>> set in the "shared_cpu_map".
>
> Yes, it could be so. For now, let's not do this as no such hardware exists.
Understood.
>>>> [...]
>>>> +
>>>> +int init_cache_level(unsigned int cpu)
>>>> +{
>>>> + struct cpuinfo_or1k *cpuinfo = &cpuinfo_or1k[smp_processor_id()];
>>>> + struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>>> + int leaves = 0, levels = 0;
>>>> + unsigned long upr = mfspr(SPR_UPR);
>>>> + unsigned long iccfgr, dccfgr;
>>>> +
>>>> + if (!(upr & SPR_UPR_UP)) {
>>>> + printk(KERN_INFO
>>>> + "-- no UPR register... unable to detect configuration\n");
>>>> + return -ENOENT;
>>>> + }
>>>> +
>>>> + if (upr & SPR_UPR_DCP) {
>>>> + dccfgr = mfspr(SPR_DCCFGR);
>>>> + cpuinfo->dcache.ways = 1 << (dccfgr & SPR_DCCFGR_NCW);
>>>> + cpuinfo->dcache.sets = 1 << ((dccfgr & SPR_DCCFGR_NCS) >> 3);
>>>> + cpuinfo->dcache.block_size = 16 << ((dccfgr & SPR_DCCFGR_CBS) >> 7);
>>>> + cpuinfo->dcache.size =
>>>> + cpuinfo->dcache.sets * cpuinfo->dcache.ways * cpuinfo->dcache.block_size;
>>>> + leaves += 1;
>>>> + printk(KERN_INFO
>>>> + "-- dcache: %4d bytes total, %2d bytes/line, %d way(s)\n",
>>>> + cpuinfo->dcache.size, cpuinfo->dcache.block_size,
>>>> + cpuinfo->dcache.ways);
>>>
>>> Can we print the number of sets here too? Also is there a reason to pad these
>>> int's with 4 and 2 spaces? I am not sure the padding is needed.
>>>
>>>> + } else
>>>> + printk(KERN_INFO "-- dcache disabled\n");
>>>> +
>>>> + if (upr & SPR_UPR_ICP) {
>>>> + iccfgr = mfspr(SPR_ICCFGR);
>>>> + cpuinfo->icache.ways = 1 << (iccfgr & SPR_ICCFGR_NCW);
>>>> + cpuinfo->icache.sets = 1 << ((iccfgr & SPR_ICCFGR_NCS) >> 3);
>>>> + cpuinfo->icache.block_size = 16 << ((iccfgr & SPR_ICCFGR_CBS) >> 7);
>>>> + cpuinfo->icache.size =
>>>> + cpuinfo->icache.sets * cpuinfo->icache.ways * cpuinfo->icache.block_size;
>>>> + leaves += 1;
>>>> + printk(KERN_INFO
>>>> + "-- icache: %4d bytes total, %2d bytes/line, %d way(s)\n",
>>>> + cpuinfo->icache.size, cpuinfo->icache.block_size,
>>>> + cpuinfo->icache.ways);
>>>
>>> Same here.
>>
>>
>> Sure, I'll print the number of sets as well.
>>
>> I don't think there's any reason for the padding. It was part of the original
>> implementation in setup.c. There shouldn't be any issues in removing them.
>
> Right, it would be good to fix.
>
I have incorporated these changes in v2 of the patch.
I realized that the kernel hangs during booting when using QEMU without the DC
and IC config register changes. I have added a few more changes so the kernel
can be booted with and without these QEMU changes.
Thanks,
Sahil
Powered by blists - more mailing lists