Hi Mathieu,
thanks for doing the review!
> I was just about to send a patch for supporting iTCO v2. I will wait until
> you push your changes before rebasing my work on this refact. Until then I
> got some interrogations/questions.
>
> >+#define PMBASE_ADDRMASK 0x0000ff80
> >+#define PMCBASE_ADDRMASK 0xfffffe00
> >
> >-#define PMBASE_REG 0x40
> >-# define PMBASE_ADDRMASK 0xff00
>
> Was this intentional? Changing PMBASE_ADDRMASK from 0xff00 to 0x0000ff80?
Yes, but I can't remember the exact reason :)
Just speculating, but I think it was because of some bits needed to be
masked for the Apollo Lake and this did also apply to the others as per
spec. But as said, I'm not really sure now, it definitely needs either
a confirmation or revert.
> >+typedef struct {
> > >+ CHAR16 name[16];
> > >+ UINT32 pci_id;
> > >+ UINT32 pmbase;
> > >+ UINT32 pmcbasereg;
> > >+ UINT32 pmcreg;
> > >+ UINT32 smireg;
> > >+} iTCO_info;
> >
>
> This is just a thought here. The iTCO v2 vs v3 uses differents registers.
> - PMC_NO_REBOOT : GCS_NO_REBOOT
> - PMCBASE_ADDRMASK : RCBABASE_ADDRMASK
> - PMCBASE_REG : RCBABASE_REG
> - PMC_REG : GCS_REG
>
> For the reboot flag, I would suggest that we add a field no_reboo_flag. For
> the other three, I am not sure what would be the best between choosing a
> generic term that would fit both iTCO version or adding the iTCO v2
> register names.
Hm, the NO_REBOOT handling has specific implementations, as you can see
in the switch/case routing to update_no_reboot_flag{,_apl}(). Instead of
the switch/case, you may, e.g., add a field holding the respective
function pointers. It's not a change in semantics but more a change in
style then. I opted for the switch/case since it's more explicit but
I don't have strong opinions about this...
For the struct iTCO_info naming, I would prefer to try to use generic
terms that fit all iTCOs. The concrete values are then set in struct
iTCO_info iTCO_chipset_info[] for the respective chipsets. That said,
we may rename them to, e.g., get rid of the pmc prefix though.
Adding the specifics of the iTCO v2 register names and having to do
a special implementation for this may end up qualifying as another
watchdog driver module particularly for iTCO v2 if there are not enough
synergies with the other iTCO support implementations to justify keeping
them all in one module.
> > >- value |= timeout & 0x3ff;
> > >- status = uefi_call_wrapper(pci_io->Io.Write, 6, pci_io,
> > >- EfiPciIoWidthUint16,
> > >- EFI_PCI_IO_PASS_THROUGH_BAR,
> > >- tcobase + TCO_TMR_REG, 1, &value);
> > >+ value |= ((timeout * 10) / 6) & 0x3ff;
> >
> This should be value |= timeout & 0x3ff;
True, I did confuse the patch while rebasing on yours, good spotting!
Will make it into a V2 of the patch I guess...
Kind regards,
Christian
--
Dr. Christian Storm
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany
--
You received this message because you are subscribed to the Google Groups "EFI
Boot Guard" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/efibootguard-dev/20191010081533.6nlifdgy53z7vogk%40MD1ZFJVC.ad001.siemens.net.
