OpenSBI Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned
       [not found] <0250109233241.2491-1-cfu@mips.com>
@ 2025-02-13 19:52 ` Raj Vishwanathan
  2025-02-13 22:58   ` Samuel Holland
                     ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Raj Vishwanathan @ 2025-02-13 19:52 UTC (permalink / raw)
  To: opensbi

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.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@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;
 	}
 
 	return 0;
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned
  2025-02-13 19:52 ` [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned Raj Vishwanathan
@ 2025-02-13 22:58   ` Samuel Holland
  2025-02-14  6:01     ` Raj Vishwanathan
  2025-02-25 21:16   ` [PATCH v3] " Raj Vishwanathan
  2025-02-25 22:57   ` Raj Vishwanathan
  2 siblings, 1 reply; 9+ messages in thread
From: Samuel Holland @ 2025-02-13 22:58 UTC (permalink / raw)
  To: opensbi

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.

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@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;
>  	}
>  
>  	return 0;



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned
  2025-02-13 22:58   ` Samuel Holland
@ 2025-02-14  6:01     ` Raj Vishwanathan
  2025-02-20 22:43       ` Raj Vishwanathan
  0 siblings, 1 reply; 9+ messages in thread
From: Raj Vishwanathan @ 2025-02-14  6:01 UTC (permalink / raw)
  To: opensbi

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@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.
>
> 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@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;
> >       }
> >
> >       return 0;
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned
  2025-02-14  6:01     ` Raj Vishwanathan
@ 2025-02-20 22:43       ` Raj Vishwanathan
  2025-02-25  2:00         ` Samuel Holland
  0 siblings, 1 reply; 9+ messages in thread
From: Raj Vishwanathan @ 2025-02-20 22:43 UTC (permalink / raw)
  To: opensbi

Hi Samuel

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.

>>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@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@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.
> >
> > 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@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;
> > >       }
> > >
> > >       return 0;
> >


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned
  2025-02-20 22:43       ` Raj Vishwanathan
@ 2025-02-25  2:00         ` Samuel Holland
  2025-02-25  5:44           ` Raj Vishwanathan
  0 siblings, 1 reply; 9+ messages in thread
From: Samuel Holland @ 2025-02-25  2:00 UTC (permalink / raw)
  To: opensbi

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@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@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@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@sifive.com>

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned
  2025-02-25  2:00         ` Samuel Holland
@ 2025-02-25  5:44           ` Raj Vishwanathan
  0 siblings, 0 replies; 9+ messages in thread
From: Raj Vishwanathan @ 2025-02-25  5:44 UTC (permalink / raw)
  To: opensbi

Samuel

I appreciate your response. I will make the changes and send out the
changes soon.

Many thanks

Raj

On Mon, Feb 24, 2025 at 6:00?PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> 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@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@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@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@sifive.com>
>
> >>>>       }
> >>>>
> >>>>       return 0;
> >>>
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3] lib: utils: Make sure that hartid and the scratch are aligned
  2025-02-13 19:52 ` [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned Raj Vishwanathan
  2025-02-13 22:58   ` Samuel Holland
@ 2025-02-25 21:16   ` Raj Vishwanathan
  2025-02-25 21:41     ` Samuel Holland
  2025-02-25 22:57   ` Raj Vishwanathan
  2 siblings, 1 reply; 9+ messages in thread
From: Raj Vishwanathan @ 2025-02-25 21:16 UTC (permalink / raw)
  To: opensbi

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 hart-ids' generate hart-count.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..ccf2d5f 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -986,7 +986,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 {
 	const fdt32_t *val;
 	int i, rc, count, cpu_offset, cpu_intc_offset;
-	u32 phandle, hwirq, hartid, first_hartid, last_hartid, hart_count;
+	u32 phandle, hwirq, hartid, first_hartid, last_hartid;
 	u32 match_hwirq = (for_timer) ? IRQ_M_TIMER : IRQ_M_SOFT;
 
 	if (nodeoffset < 0 || !fdt ||
@@ -1015,7 +1015,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 	count = count / sizeof(fdt32_t);
 
 	first_hartid = -1U;
-	hart_count = last_hartid = 0;
+	last_hartid = 0;
 	for (i = 0; i < (count / 2); i++) {
 		phandle = fdt32_to_cpu(val[2 * i]);
 		hwirq = fdt32_to_cpu(val[(2 * i) + 1]);
@@ -1040,16 +1040,13 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 				first_hartid = hartid;
 			if (hartid > last_hartid)
 				last_hartid = hartid;
-			hart_count++;
 		}
 	}
 
 	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;
 	}
-
 	return 0;
 }
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3] lib: utils: Make sure that hartid and the scratch are aligned
  2025-02-25 21:16   ` [PATCH v3] " Raj Vishwanathan
@ 2025-02-25 21:41     ` Samuel Holland
  0 siblings, 0 replies; 9+ messages in thread
From: Samuel Holland @ 2025-02-25 21:41 UTC (permalink / raw)
  To: opensbi

On 2025-02-25 3:16 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 hart-ids' generate hart-count.
> 
> Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@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 | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index cb350e5..ccf2d5f 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -986,7 +986,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
>  {
>  	const fdt32_t *val;
>  	int i, rc, count, cpu_offset, cpu_intc_offset;
> -	u32 phandle, hwirq, hartid, first_hartid, last_hartid, hart_count;
> +	u32 phandle, hwirq, hartid, first_hartid, last_hartid;
>  	u32 match_hwirq = (for_timer) ? IRQ_M_TIMER : IRQ_M_SOFT;
>  
>  	if (nodeoffset < 0 || !fdt ||
> @@ -1015,7 +1015,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
>  	count = count / sizeof(fdt32_t);
>  
>  	first_hartid = -1U;
> -	hart_count = last_hartid = 0;
> +	last_hartid = 0;
>  	for (i = 0; i < (count / 2); i++) {
>  		phandle = fdt32_to_cpu(val[2 * i]);
>  		hwirq = fdt32_to_cpu(val[(2 * i) + 1]);
> @@ -1040,16 +1040,13 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
>  				first_hartid = hartid;
>  			if (hartid > last_hartid)
>  				last_hartid = hartid;
> -			hart_count++;
>  		}
>  	}
>  
>  	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;

You still have an extra space before the assignment here.

>  	}
> -

Unrelated change.

With those style issues fixed:

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

>  	return 0;
>  }
>  



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3] lib: utils: Make sure that hartid and the scratch are aligned
  2025-02-13 19:52 ` [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned Raj Vishwanathan
  2025-02-13 22:58   ` Samuel Holland
  2025-02-25 21:16   ` [PATCH v3] " Raj Vishwanathan
@ 2025-02-25 22:57   ` Raj Vishwanathan
  2 siblings, 0 replies; 9+ messages in thread
From: Raj Vishwanathan @ 2025-02-25 22:57 UTC (permalink / raw)
  To: opensbi

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 hart-ids' generate hart-count.

Signed-off-by: Raj Vishwanathan <Raj.Vishwanathan@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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index cb350e5..80fe4e8 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -986,7 +986,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 {
 	const fdt32_t *val;
 	int i, rc, count, cpu_offset, cpu_intc_offset;
-	u32 phandle, hwirq, hartid, first_hartid, last_hartid, hart_count;
+	u32 phandle, hwirq, hartid, first_hartid, last_hartid;
 	u32 match_hwirq = (for_timer) ? IRQ_M_TIMER : IRQ_M_SOFT;
 
 	if (nodeoffset < 0 || !fdt ||
@@ -1015,7 +1015,7 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 	count = count / sizeof(fdt32_t);
 
 	first_hartid = -1U;
-	hart_count = last_hartid = 0;
+	last_hartid = 0;
 	for (i = 0; i < (count / 2); i++) {
 		phandle = fdt32_to_cpu(val[2 * i]);
 		hwirq = fdt32_to_cpu(val[(2 * i) + 1]);
@@ -1040,16 +1040,13 @@ int fdt_parse_aclint_node(const void *fdt, int nodeoffset,
 				first_hartid = hartid;
 			if (hartid > last_hartid)
 				last_hartid = hartid;
-			hart_count++;
 		}
 	}
 
 	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;
 	}
-
 	return 0;
 }
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-02-25 22:57 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <0250109233241.2491-1-cfu@mips.com>
2025-02-13 19:52 ` [PATCH v2] lib: utils: Make sure that hartid and the scratch are aligned Raj Vishwanathan
2025-02-13 22:58   ` Samuel Holland
2025-02-14  6:01     ` Raj Vishwanathan
2025-02-20 22:43       ` Raj Vishwanathan
2025-02-25  2:00         ` Samuel Holland
2025-02-25  5:44           ` Raj Vishwanathan
2025-02-25 21:16   ` [PATCH v3] " Raj Vishwanathan
2025-02-25 21:41     ` Samuel Holland
2025-02-25 22:57   ` Raj Vishwanathan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox