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