[PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned

[Samuel Holland]

Hi Raj,

On 2025-02-20 4:43 PM, Raj Vishwanathan wrote:
> I reviewed the two files for mstimer and mswi_ipi and they seem to be
> correct. We use the same "hartid - first_hartid" to access MMIO
> addresses. So, the current  mtimer_event_start() and mswi_ipi_send()
> implementations work as is.

OK, I looked at the MIPS P8700 reference manual and see what you mean: "The MSIP
register for hart mhartid is accessed at GCR_BASE + 0x50000 + 4 *
mhartid[11:0]." So there is no gap in the mapping, instead there is a gap in the
register space.

>>> Either each device instance needs an array of [ACLINT index] => [hart index]
>>> mappings, or the ACLINT index needs to be recorded separately in a new per-hart
>>> scratch variable.
> 
> ACLINT index is actually "hartid - first_hartid"
> 
> Let me know if you have any further comments.
> 
> Raj
> 
> On Thu, Feb 13, 2025 at 10:01 PM Raj Vishwanathan
> <raj.vishwanathan at gmail.com> wrote:
>>
>> Samuel
>>
>> Thanks for the quick review. I took a quick look and in our case,
>> where the hart-id's are not sequential,  I see that there will be gaps
>> in the MMIO space but nothing should break
>>
>> I will do a deeper study and respond specifically to your questions.
>>
>> Raj
>>
>> On Thu, Feb 13, 2025 at 2:58 PM Samuel Holland
>> <samuel.holland at sifive.com> wrote:
>>>
>>> Hi Raj,
>>>
>>> On 2025-02-13 1:52 PM, Raj Vishwanathan wrote:
>>>> Harts associated with an ACLINT_MSWI need not have sequential hartids.
>>>> It is insufficient to use first_hartid and hart_count. To account for
>>>> non-sequential hart ids, include the empty hard-ids' generate hart-count.

s/hard/hart/

>>>
>>> This doesn't help, because the expression (hartid - first_hartid) is still used
>>> to calculate MMIO addresses. See for example mtimer_event_start() and
>>> mswi_ipi_send(). If there is a gap in the hartids mapped to the ACLINT
>>> MTIMER/MSWI indexes, these functions will compute the wrong MMIO address and
>>> fail to set the timer or send an IPI.
>>>
>>> Either each device instance needs an array of [ACLINT index] => [hart index]
>>> mappings, or the ACLINT index needs to be recorded separately in a new per-hart
>>> scratch variable.
>>>
>>> Regards,
>>> Samuel
>>>
>>>> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan at gmail.com>
>>>> ---
>>>>
>>>> hart_count = last_hart_id - first_hart_id +1
>>>> count = number of harts
>>>> For sequential hart_id's, count = hart_count
>>>> For non-sequential hart_id's, count  < hart_count
>>>> ---
>>>>  lib/utils/fdt/fdt_helper.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
>>>> index cb350e5..bb3fddd 100644
>>>> --- a/lib/utils/fdt/fdt_helper.c
>>>> +++ b/lib/utils/fdt/fdt_helper.c
>>>> @@ -1046,8 +1046,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
>>>>
>>>>       if ((last_hartid >= first_hartid) && first_hartid != -1U) {
>>>>               *out_first_hartid = first_hartid;
>>>> -             count = last_hartid - first_hartid + 1;
>>>> -             *out_hart_count = (hart_count < count) ? hart_count : count;
>>>> +             *out_hart_count  = last_hartid - first_hartid + 1;

Now that I understand the context, this looks like the right change to make.

Please remove the hart_count variable entirely, since it is now unused. Also
please fix the spacing in this line. With those changes:

Reviewed-by: Samuel Holland <samuel.holland at sifive.com>

>>>>       }
>>>>
>>>>       return 0;
>>>