Re: [PATCH 4/7] pnv/chiptod: Add POWER9/10 chiptod model

[Nicholas Piggin]

On Fri Nov 24, 2023 at 3:49 AM AEST, Cédric Le Goater wrote:
> On 11/23/23 11:30, Nicholas Piggin wrote:
> > The ChipTOD (for Time-Of-Day) is a pervasive facility that keeps the
> > clocks on multiple chips consistent which can keep TOD (time-of-day),
> > synchronise it across multiple chips, and can move that TOD to or from
> > the core timebase units.
>
> May be rephrase a bit the sentence above. I find it difficult to read.
>
> > This driver implements basic emulation of chiptod registers sufficient
>
> it's a model.

+1

> > +static void chiptod_power9_send_remotes(PnvChipTOD *chiptod)
> > +{
> > +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
>
> Using qdev_get_machine() in a model is always a bit annoying. There is
> work in progress currently to have a single QEMU binary for all arches
> and when done, the "machine" would encompass something bigger including
> the OCC, BMC, etc.

We need a way to get from one chip to another, fundamentally. I
didn't see a better way, I suppose we could build a PnvHost container
that includes all PnvChips or something.

> Do we know how XSCOM propagates the new state to the chiptop on other
> chips ? is it some sort of broadcast on the bus ? Could we model it ?
> I am only asking, not to be done now.

It's a bit hard to work out. Real ChipTOD is vastly more complicated
than this model :P

It seems that ChipTOD can master PIB scoms to other chips, but also
this TTYPE transactions look like they have a command that goes
on the powerbus via ADU.


> > +static void chiptod_power10_invalidate_remotes(PnvChipTOD *chiptod)
> > +{
> > +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> > +    int i;
> > +
> > +    for (i = 0; i < pnv->num_chips; i++) {
> > +        Pnv10Chip *chip10 = PNV10_CHIP(pnv->chips[i]);
> > +        if (&chip10->chiptod != chiptod) {
> > +            chip10->chiptod.tod_state = tod_not_set;
> > +        }
> > +    }
> > +}
>
> Could we avoid 4 routines doing the same thing and introduce :
>
>    chiptod_power*_set_tod_state(PnvChipTOD *chiptod, enum tod_state new)
>
> ?
>
> We could even introcude a PnvChipClass::get_chiptod handler. Might be
> overkill though.

Good idea...

> > +    case TOD_TX_TTYPE_5_REG:
> > +        if (is_power9) {
> > +            chiptod_power9_invalidate_remotes(chiptod);
> > +        } else {
> > +            chiptod_power10_invalidate_remotes(chiptod);
> > +        }
>
> With PnvChipTODClass::invalidate_remotes and PnvChipTODClass::send_remotes
> handlers, or ::set_state, this routine would not need a 'is_power9' parameter
> and the model would not need 2 different xscom_ops. Can it be done ?

... yes! ChipTODClass seems to be a good place for it.

> > +static void pnv_chiptod_realize(DeviceState *dev, Error **errp)
> > +{
> > +    PnvChipTOD *chiptod = PNV_CHIPTOD(dev);
> > +    PnvChipTODClass *pctc = PNV_CHIPTOD_GET_CLASS(chiptod);
> > +
> > +    if (chiptod->primary) {
> > +        chiptod->pss_mss_ctrl_reg |= PPC_BIT(1); /* TOD is master */
> > +    }
> > +
> > +    /* Drawer is master (we do not simulate multi-drawer) */
> > +    chiptod->pss_mss_ctrl_reg |= PPC_BIT(2);
> > +    chiptod->tod_state = tod_running;
>
> Shouldn't these initial values be set in a reset handler ?

Yes, and actually reset state is tod_error so fixed that too.

Thanks,
Nick