Re: [RFC PATCH 05/10] ppc/pnv: Extend chip_pir class method to TIR as well

[Cédric Le Goater]

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 <npiggin@gmail.com>
> > > ---
> > >    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, &pir, &tir);
> >
> > 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