Re: [PATCH v5 2/3] hw/ppc: Add nest1 chiplet model

[Chalapathi V <chalapathi.v@linux.ibm.com>]

On 24-11-2023 18:31, Nicholas Piggin wrote:
> On Fri Nov 24, 2023 at 10:19 PM AEST, Cédric Le Goater wrote:
>> On 11/24/23 12:26, Nicholas Piggin wrote:
>>> For this and actually the last patch too, it would be good to mention
>>> (possibly in a header comment in the file too) what actual functionality
>>> is being provided/modeled. It looks like it's just modeling behaviour of
>>> reads and writes for some registers.
>>>
>>> Oh, and sorry I didn't follow development and comments on this too
>>> closely, so forgive me if I've missed things already said. I'll go
>>> back and read through the series.
>>>
>>> On Fri Nov 24, 2023 at 8:15 PM AEST, Chalapathi V wrote:
>>>> The nest1 chiplet handle the high speed i/o traffic over PCIe and others.
>>>> The nest1 chiplet consists of PowerBus Fabric controller,
>>>> nest Memory Management Unit, chiplet control unit and more.
>>>>
>>>> This commit creates a nest1 chiplet model and initialize and realize the
>>>> pervasive chiplet model where chiplet control registers are implemented.
>>>>
>>>> This commit also implement the read/write method for the powerbus scom
>>>> registers
>>> The powerbus scom registers, are those specifically for the PowerBus
>>> Fabric controller mentioned in the first paragraph, or is it a more
>>> general set of registers for the chiplet?
Yes, They are for the PowerBus racetrack unit.
>>>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>>>> ---
>>>>    include/hw/ppc/pnv_nest_chiplet.h |  36 ++++++
>>>>    include/hw/ppc/pnv_xscom.h        |   6 +
>>>>    hw/ppc/pnv_nest1_chiplet.c        | 197 ++++++++++++++++++++++++++++++
>>>>    hw/ppc/meson.build                |   1 +
>>>>    4 files changed, 240 insertions(+)
>>>>    create mode 100644 include/hw/ppc/pnv_nest_chiplet.h
>>>>    create mode 100644 hw/ppc/pnv_nest1_chiplet.c
>>>>
>>>> diff --git a/include/hw/ppc/pnv_nest_chiplet.h b/include/hw/ppc/pnv_nest_chiplet.h
>>>> new file mode 100644
>>>> index 0000000000..845030fb1a
>>>> --- /dev/null
>>>> +++ b/include/hw/ppc/pnv_nest_chiplet.h
>>>> @@ -0,0 +1,36 @@
>>>> +/*
>>>> + * QEMU PowerPC nest chiplet model
>>>> + *
>>>> + * Copyright (c) 2023, IBM Corporation.
>>>> + *
>>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>>> + *
>>>> + * This code is licensed under the GPL version 2 or later. See the
>>>> + * COPYING file in the top-level directory.
>>>> + *
>>>> + */
>>>> +
>>>> +#ifndef PPC_PNV_NEST1_CHIPLET_H
>>>> +#define PPC_PNV_NEST1_CHIPLET_H
>>>> +
>>>> +#include "hw/ppc/pnv_pervasive.h"
>>>> +
>>>> +#define TYPE_PNV_NEST1 "pnv-nest1-chiplet"
>>>> +#define PNV_NEST1(obj) OBJECT_CHECK(PnvNest1, (obj), TYPE_PNV_NEST1)
>>>> +
>>>> +typedef struct pb_scom {
>>>> +    uint64_t mode;
>>>> +    uint64_t hp_mode2_curr;
>>>> +} pb_scom;
>>>> +
>>>> +typedef struct PnvNest1 {
>>> Naming nitpicking again...
>>>
>>> The main ifndef guard for header files should match the file name, so
>>> the file should be called pnv_nest1_chiplet.h (and that matches the .c
>>> file too).
>>>
>>> I think this struct should be called Nest1Chiplet too.
>> I asked Chalapathi to do the exact opposit :)
> Oops :)
>
>> I don't mind really, my argument was that most models represent HW logic
>> units or subunits of a bigger unit. I don't see the point in adding a
>> chip/chiplet suffix apart from PnvChip since it represents a socket or
>> processor.
>>
>> You choose. I will keep quiet :)
> Ah. I can see that side of it. And for many of the nest chiplets (MC,
> PAU, PCI) that makes sense. For Nest0 and Nest1... it's a bit
> overloaded. First of all, all the nest chiplets are "nest". Then
> there is also some nest units inside the processor chiplets (L2, L3,
> NCU are considered to be nest). And then the nest also has a Pervasive
> Chiplet itself, and we also have these pervasive registers in each
> chiplet, etc., etc.
>
> So my worry is we'll run into confusion if we shorten names too much.
>
> We can always rename things, so it won't be the end of the world, but
> thinking about the pervasive chiplet, I think we can already see that
> "PnvPervasive" would not be a good name for it.
>
> The chiplets have short names actually if that would help. Nest 1 is
> called N1, so we could call it PnvN1Chiplet. That seems the usual
> way to refer to them in docs, so I think a better name.
>
> Thanks,
> Nick
Sure. Will rename this to PnvN1Chiplet.