Re: [RFC PATCH 05/10] ppc/pnv: Extend chip_pir class method to TIR as well
On 5/30/24 08:38, Nicholas Piggin wrote:
On Wed May 29, 2024 at 4:30 PM AEST, Cédric Le Goater wrote:
On 5/29/24 02:24, Nicholas Piggin wrote:
On Tue May 28, 2024 at 6:32 PM AEST, Harsh Prateek Bora wrote:
On 5/26/24 17:56, Nicholas Piggin wrote:
The chip_pir chip class method allows the platform to set the PIR
processor identification register. Extend this to a more general
ID function which also allows the TIR to be set. This is in
preparation for "big core", which is a more complicated topology
of cores and threads.
Signed-off-by: Nicholas Piggin
---
include/hw/ppc/pnv_chip.h | 3 +-
hw/ppc/pnv.c | 61 ---
hw/ppc/pnv_core.c | 10 ---
3 files changed, 45 insertions(+), 29 deletions(-)
diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
index 8589f3291e..679723926a 100644
--- a/include/hw/ppc/pnv_chip.h
+++ b/include/hw/ppc/pnv_chip.h
@@ -147,7 +147,8 @@ struct PnvChipClass {
DeviceRealize parent_realize;
-uint32_t (*chip_pir)(PnvChip *chip, uint32_t core_id, uint32_t thread_id);
+void (*processor_id)(PnvChip *chip, uint32_t core_id, uint32_t thread_id,
+ uint32_t *pir, uint32_t *tir);
Should it be named get_chip_core_thread_regs() ?
Yeah, the name isn't great. It is getting the regs, but the regs are the
"pervasive id" used as well... but maybe that's not too relevant here.
What about we drop chip_ since we have the chip and no other methods use
such prefix, then call it get_thread_pir_tir()?
processor relates to chip and so, processor_id() is not great indeed.
get_pir_tir() would be enough I think.
What would be good though, since pnv is growing, is to start adding
documentation to these common helpers.
Okay we'll use that name.
You mean just a comment them in the header? Might as well do that for
new ones at least.
Yes. All class handlers should have a minimal description.
Thanks,
C.
Re: [RFC PATCH 05/10] ppc/pnv: Extend chip_pir class method to TIR as well
On Wed May 29, 2024 at 4:30 PM AEST, Cédric Le Goater wrote:
> On 5/29/24 02:24, Nicholas Piggin wrote:
> > On Tue May 28, 2024 at 6:32 PM AEST, Harsh Prateek Bora wrote:
> >>
> >>
> >> On 5/26/24 17:56, Nicholas Piggin wrote:
> >>> The chip_pir chip class method allows the platform to set the PIR
> >>> processor identification register. Extend this to a more general
> >>> ID function which also allows the TIR to be set. This is in
> >>> preparation for "big core", which is a more complicated topology
> >>> of cores and threads.
> >>>
> >>> Signed-off-by: Nicholas Piggin
> >>> ---
> >>>include/hw/ppc/pnv_chip.h | 3 +-
> >>>hw/ppc/pnv.c | 61 ---
> >>>hw/ppc/pnv_core.c | 10 ---
> >>>3 files changed, 45 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
> >>> index 8589f3291e..679723926a 100644
> >>> --- a/include/hw/ppc/pnv_chip.h
> >>> +++ b/include/hw/ppc/pnv_chip.h
> >>> @@ -147,7 +147,8 @@ struct PnvChipClass {
> >>>
> >>>DeviceRealize parent_realize;
> >>>
> >>> -uint32_t (*chip_pir)(PnvChip *chip, uint32_t core_id, uint32_t
> >>> thread_id);
> >>> +void (*processor_id)(PnvChip *chip, uint32_t core_id, uint32_t
> >>> thread_id,
> >>> + uint32_t *pir, uint32_t *tir);
> >>
> >> Should it be named get_chip_core_thread_regs() ?
> >
> > Yeah, the name isn't great. It is getting the regs, but the regs are the
> > "pervasive id" used as well... but maybe that's not too relevant here.
> > What about we drop chip_ since we have the chip and no other methods use
> > such prefix, then call it get_thread_pir_tir()?
>
> processor relates to chip and so, processor_id() is not great indeed.
> get_pir_tir() would be enough I think.
>
> What would be good though, since pnv is growing, is to start adding
> documentation to these common helpers.
Okay we'll use that name.
You mean just a comment them in the header? Might as well do that for
new ones at least.
Thanks,
Nick
Re: [RFC PATCH 05/10] ppc/pnv: Extend chip_pir class method to TIR as well
On 5/29/24 02:24, Nicholas Piggin wrote:
On Tue May 28, 2024 at 6:32 PM AEST, Harsh Prateek Bora wrote:
On 5/26/24 17:56, Nicholas Piggin wrote:
The chip_pir chip class method allows the platform to set the PIR
processor identification register. Extend this to a more general
ID function which also allows the TIR to be set. This is in
preparation for "big core", which is a more complicated topology
of cores and threads.
Signed-off-by: Nicholas Piggin
---
include/hw/ppc/pnv_chip.h | 3 +-
hw/ppc/pnv.c | 61 ---
hw/ppc/pnv_core.c | 10 ---
3 files changed, 45 insertions(+), 29 deletions(-)
diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
index 8589f3291e..679723926a 100644
--- a/include/hw/ppc/pnv_chip.h
+++ b/include/hw/ppc/pnv_chip.h
@@ -147,7 +147,8 @@ struct PnvChipClass {
DeviceRealize parent_realize;
-uint32_t (*chip_pir)(PnvChip *chip, uint32_t core_id, uint32_t thread_id);
+void (*processor_id)(PnvChip *chip, uint32_t core_id, uint32_t thread_id,
+ uint32_t *pir, uint32_t *tir);
Should it be named get_chip_core_thread_regs() ?
Yeah, the name isn't great. It is getting the regs, but the regs are the
"pervasive id" used as well... but maybe that's not too relevant here.
What about we drop chip_ since we have the chip and no other methods use
such prefix, then call it get_thread_pir_tir()?
processor relates to chip and so, processor_id() is not great indeed.
get_pir_tir() would be enough I think.
What would be good though, since pnv is growing, is to start adding
documentation to these common helpers.
Thanks,
C.
@@ -155,7 +155,7 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void
*fdt)
char *nodename;
int cpus_offset = get_cpus_node(fdt);
-pir = pnv_cc->chip_pir(chip, pc->hwid, 0);
+pnv_cc->processor_id(chip, pc->hwid, 0, , );
As a generic helper API and potentially expandable, it should allow
passing NULL for registers whose values are not really sought to avoid
having to create un-necessary local variables by the caller.
I'll do that.
Thanks,
Nick
Re: [RFC PATCH 05/10] ppc/pnv: Extend chip_pir class method to TIR as well
On Tue May 28, 2024 at 6:32 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 5/26/24 17:56, Nicholas Piggin wrote:
> > The chip_pir chip class method allows the platform to set the PIR
> > processor identification register. Extend this to a more general
> > ID function which also allows the TIR to be set. This is in
> > preparation for "big core", which is a more complicated topology
> > of cores and threads.
> >
> > Signed-off-by: Nicholas Piggin
> > ---
> > include/hw/ppc/pnv_chip.h | 3 +-
> > hw/ppc/pnv.c | 61 ---
> > hw/ppc/pnv_core.c | 10 ---
> > 3 files changed, 45 insertions(+), 29 deletions(-)
> >
> > diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
> > index 8589f3291e..679723926a 100644
> > --- a/include/hw/ppc/pnv_chip.h
> > +++ b/include/hw/ppc/pnv_chip.h
> > @@ -147,7 +147,8 @@ struct PnvChipClass {
> >
> > DeviceRealize parent_realize;
> >
> > -uint32_t (*chip_pir)(PnvChip *chip, uint32_t core_id, uint32_t
> > thread_id);
> > +void (*processor_id)(PnvChip *chip, uint32_t core_id, uint32_t
> > thread_id,
> > + uint32_t *pir, uint32_t *tir);
>
> Should it be named get_chip_core_thread_regs() ?
Yeah, the name isn't great. It is getting the regs, but the regs are the
"pervasive id" used as well... but maybe that's not too relevant here.
What about we drop chip_ since we have the chip and no other methods use
such prefix, then call it get_thread_pir_tir()?
> > @@ -155,7 +155,7 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void
> > *fdt)
> > char *nodename;
> > int cpus_offset = get_cpus_node(fdt);
> >
> > -pir = pnv_cc->chip_pir(chip, pc->hwid, 0);
> > +pnv_cc->processor_id(chip, pc->hwid, 0, , );
>
> As a generic helper API and potentially expandable, it should allow
> passing NULL for registers whose values are not really sought to avoid
> having to create un-necessary local variables by the caller.
I'll do that.
Thanks,
Nick
Re: [RFC PATCH 05/10] ppc/pnv: Extend chip_pir class method to TIR as well
On 5/26/24 17:56, Nicholas Piggin wrote:
The chip_pir chip class method allows the platform to set the PIR
processor identification register. Extend this to a more general
ID function which also allows the TIR to be set. This is in
preparation for "big core", which is a more complicated topology
of cores and threads.
Signed-off-by: Nicholas Piggin
---
include/hw/ppc/pnv_chip.h | 3 +-
hw/ppc/pnv.c | 61 ---
hw/ppc/pnv_core.c | 10 ---
3 files changed, 45 insertions(+), 29 deletions(-)
diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
index 8589f3291e..679723926a 100644
--- a/include/hw/ppc/pnv_chip.h
+++ b/include/hw/ppc/pnv_chip.h
@@ -147,7 +147,8 @@ struct PnvChipClass {
DeviceRealize parent_realize;
-uint32_t (*chip_pir)(PnvChip *chip, uint32_t core_id, uint32_t thread_id);
+void (*processor_id)(PnvChip *chip, uint32_t core_id, uint32_t thread_id,
+ uint32_t *pir, uint32_t *tir);
Should it be named get_chip_core_thread_regs() ?
void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index a706de2e36..7d062ec16c 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -144,7 +144,7 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void
*fdt)
PnvChipClass *pnv_cc = PNV_CHIP_GET_CLASS(chip);
g_autofree uint32_t *servers_prop = g_new(uint32_t, smt_threads);
int i;
-uint32_t pir;
+uint32_t pir, tir;
uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
0x, 0x};
uint32_t tbfreq = PNV_TIMEBASE_FREQ;
@@ -155,7 +155,7 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void
*fdt)
char *nodename;
int cpus_offset = get_cpus_node(fdt);
-pir = pnv_cc->chip_pir(chip, pc->hwid, 0);
+pnv_cc->processor_id(chip, pc->hwid, 0, , );
As a generic helper API and potentially expandable, it should allow
passing NULL for registers whose values are not really sought to avoid
having to create un-necessary local variables by the caller.
regards,
Harsh
nodename = g_strdup_printf("%s@%x", dc->fw_name, pir);
offset = fdt_add_subnode(fdt, cpus_offset, nodename);
@@ -237,7 +237,8 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void
*fdt)
/* Build interrupt servers properties */
for (i = 0; i < smt_threads; i++) {
-servers_prop[i] = cpu_to_be32(pnv_cc->chip_pir(chip, pc->hwid, i));
+pnv_cc->processor_id(chip, pc->hwid, i, , );
+servers_prop[i] = cpu_to_be32(pir);
}
_FDT((fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
servers_prop, sizeof(*servers_prop) * smt_threads)));
@@ -249,14 +250,17 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t
hwid,
uint32_t nr_threads)
{
PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
-uint32_t pir = pcc->chip_pir(chip, hwid, 0);
-uint64_t addr = PNV_ICP_BASE(chip) | (pir << 12);
+uint32_t pir, tir;
+uint64_t addr;
char *name;
const char compat[] = "IBM,power8-icp\0IBM,ppc-xicp";
uint32_t irange[2], i, rsize;
uint64_t *reg;
int offset;
+pcc->processor_id(chip, hwid, 0, , );
+addr = PNV_ICP_BASE(chip) | (pir << 12);
+
irange[0] = cpu_to_be32(pir);
irange[1] = cpu_to_be32(nr_threads);
@@ -1104,10 +1108,12 @@ static void pnv_power10_init(MachineState *machine)
* 25:28 Core number
* 29:31 Thread ID
*/
-static uint32_t pnv_chip_pir_p8(PnvChip *chip, uint32_t core_id,
-uint32_t thread_id)
+static void pnv_processor_id_p8(PnvChip *chip,
+uint32_t core_id, uint32_t thread_id,
+uint32_t *pir, uint32_t *tir)
{
-return (chip->chip_id << 7) | (core_id << 3) | thread_id;
+*pir = (chip->chip_id << 7) | (core_id << 3) | thread_id;
+*tir = thread_id;
}
static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
@@ -1159,15 +1165,17 @@ static void pnv_chip_power8_intc_print_info(PnvChip
*chip, PowerPCCPU *cpu,
*
* We only care about the lower bits. uint32_t is fine for the moment.
*/
-static uint32_t pnv_chip_pir_p9(PnvChip *chip, uint32_t core_id,
-uint32_t thread_id)
+static void pnv_processor_id_p9(PnvChip *chip,
+uint32_t core_id, uint32_t thread_id,
+uint32_t *pir, uint32_t *tir)
{
if (chip->nr_threads == 8) {
-return (chip->chip_id << 8) | ((thread_id & 1) << 2) | (core_id << 3) |
+*pir = (chip->chip_id << 8) | ((thread_id & 1) << 2) | (core_id << 3) |
(thread_id >> 1);
} else {
-
[RFC PATCH 05/10] ppc/pnv: Extend chip_pir class method to TIR as well
The chip_pir chip class method allows the platform to set the PIR
processor identification register. Extend this to a more general
ID function which also allows the TIR to be set. This is in
preparation for "big core", which is a more complicated topology
of cores and threads.
Signed-off-by: Nicholas Piggin
---
include/hw/ppc/pnv_chip.h | 3 +-
hw/ppc/pnv.c | 61 ---
hw/ppc/pnv_core.c | 10 ---
3 files changed, 45 insertions(+), 29 deletions(-)
diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
index 8589f3291e..679723926a 100644
--- a/include/hw/ppc/pnv_chip.h
+++ b/include/hw/ppc/pnv_chip.h
@@ -147,7 +147,8 @@ struct PnvChipClass {
DeviceRealize parent_realize;
-uint32_t (*chip_pir)(PnvChip *chip, uint32_t core_id, uint32_t thread_id);
+void (*processor_id)(PnvChip *chip, uint32_t core_id, uint32_t thread_id,
+ uint32_t *pir, uint32_t *tir);
void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp);
void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu);
void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index a706de2e36..7d062ec16c 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -144,7 +144,7 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void
*fdt)
PnvChipClass *pnv_cc = PNV_CHIP_GET_CLASS(chip);
g_autofree uint32_t *servers_prop = g_new(uint32_t, smt_threads);
int i;
-uint32_t pir;
+uint32_t pir, tir;
uint32_t segs[] = {cpu_to_be32(28), cpu_to_be32(40),
0x, 0x};
uint32_t tbfreq = PNV_TIMEBASE_FREQ;
@@ -155,7 +155,7 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void
*fdt)
char *nodename;
int cpus_offset = get_cpus_node(fdt);
-pir = pnv_cc->chip_pir(chip, pc->hwid, 0);
+pnv_cc->processor_id(chip, pc->hwid, 0, , );
nodename = g_strdup_printf("%s@%x", dc->fw_name, pir);
offset = fdt_add_subnode(fdt, cpus_offset, nodename);
@@ -237,7 +237,8 @@ static int pnv_dt_core(PnvChip *chip, PnvCore *pc, void
*fdt)
/* Build interrupt servers properties */
for (i = 0; i < smt_threads; i++) {
-servers_prop[i] = cpu_to_be32(pnv_cc->chip_pir(chip, pc->hwid, i));
+pnv_cc->processor_id(chip, pc->hwid, i, , );
+servers_prop[i] = cpu_to_be32(pir);
}
_FDT((fdt_setprop(fdt, offset, "ibm,ppc-interrupt-server#s",
servers_prop, sizeof(*servers_prop) * smt_threads)));
@@ -249,14 +250,17 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t
hwid,
uint32_t nr_threads)
{
PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
-uint32_t pir = pcc->chip_pir(chip, hwid, 0);
-uint64_t addr = PNV_ICP_BASE(chip) | (pir << 12);
+uint32_t pir, tir;
+uint64_t addr;
char *name;
const char compat[] = "IBM,power8-icp\0IBM,ppc-xicp";
uint32_t irange[2], i, rsize;
uint64_t *reg;
int offset;
+pcc->processor_id(chip, hwid, 0, , );
+addr = PNV_ICP_BASE(chip) | (pir << 12);
+
irange[0] = cpu_to_be32(pir);
irange[1] = cpu_to_be32(nr_threads);
@@ -1104,10 +1108,12 @@ static void pnv_power10_init(MachineState *machine)
* 25:28 Core number
* 29:31 Thread ID
*/
-static uint32_t pnv_chip_pir_p8(PnvChip *chip, uint32_t core_id,
-uint32_t thread_id)
+static void pnv_processor_id_p8(PnvChip *chip,
+uint32_t core_id, uint32_t thread_id,
+uint32_t *pir, uint32_t *tir)
{
-return (chip->chip_id << 7) | (core_id << 3) | thread_id;
+*pir = (chip->chip_id << 7) | (core_id << 3) | thread_id;
+*tir = thread_id;
}
static void pnv_chip_power8_intc_create(PnvChip *chip, PowerPCCPU *cpu,
@@ -1159,15 +1165,17 @@ static void pnv_chip_power8_intc_print_info(PnvChip
*chip, PowerPCCPU *cpu,
*
* We only care about the lower bits. uint32_t is fine for the moment.
*/
-static uint32_t pnv_chip_pir_p9(PnvChip *chip, uint32_t core_id,
-uint32_t thread_id)
+static void pnv_processor_id_p9(PnvChip *chip,
+uint32_t core_id, uint32_t thread_id,
+uint32_t *pir, uint32_t *tir)
{
if (chip->nr_threads == 8) {
-return (chip->chip_id << 8) | ((thread_id & 1) << 2) | (core_id << 3) |
+*pir = (chip->chip_id << 8) | ((thread_id & 1) << 2) | (core_id << 3) |
(thread_id >> 1);
} else {
-return (chip->chip_id << 8) | (core_id << 2) | thread_id;
+*pir = (chip->chip_id << 8) | (core_id << 2) | thread_id;
}
+*tir = thread_id;
}
/*
@@ -1181,15 +1189,17 @@ static uint32_t pnv_chip_pir_p9(PnvChip *chip, uint32_t
core_id,
*
* We only care about the lower bits. uint32_t is fine for the moment.
*/
-static uint32_t pnv_chip_pir_p10(PnvChip *chip,
