[PATCH] Add support for iTCOv4 (Sunrise Point-LP)

[Mathieu Tetreault]

Signed-off-by: Mathieu Tetreault <mat...@rmds.ca>
---
drivers/watchdog/itco.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/itco.c b/drivers/watchdog/itco.c
index 9252014..c20b1b6 100644
--- a/drivers/watchdog/itco.c
+++ b/drivers/watchdog/itco.c
@@ -40,6 +40,7 @@ enum iTCO_chipsets {
ITCO_INTEL_WBG,
ITCO_INTEL_EHL,
ITCO_INTEL_TLH,
+ ITCO_INTEL_SUNRISE_LP,
};

enum iTCO_versions {
@@ -92,7 +93,12 @@ static const iTCO_regs iTCO_version_regs[] = {
},
[ITCO_V4] =
{
- /* Not implemented yet */
+ .pmc_base_reg = 0x10,
+ .pmc_reg = 0x1008,
+ .pmc_no_reboot_mask = (1 << 4),
+ .pmc_base_addr_mask = 0xfffffe00,
+ .pm_base_reg = 0x400,
+ .pm_base_addr_mask = 0x0000ff80,
},
[ITCO_V5] =
{
@@ -163,6 +169,12 @@ static const iTCO_info iTCO_chipset_info[] = {
.pci_id = 0x43a3,
.itco_version = ITCO_V6,
},
+ [ITCO_INTEL_SUNRISE_LP] =
+ {
+ .name = L"Sunrise Point-LP",
+ .pci_id = 0x9d23,
+ .itco_version = ITCO_V4,
+ },
};

static BOOLEAN itco_supported(UINT16 pci_device_id, UINT8 *index)
@@ -320,6 +332,7 @@ static EFI_STATUS init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
update_no_reboot_flag_cnt(tco_base);
break;
case ITCO_V5:
+ case ITCO_V4:
update_no_reboot_flag_apl(itco);
break;
case ITCO_V3:
--
2.20.1

[Jan Kiszka]

On 17.04.24 17:38, 'Mathieu Tetreault' via EFI Boot Guard wrote:
> Signed-off-by: Mathieu Tetreault <mat...@rmds.ca>
> ---
> drivers/watchdog/itco.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/itco.c b/drivers/watchdog/itco.c
> index 9252014..c20b1b6 100644
> --- a/drivers/watchdog/itco.c
> +++ b/drivers/watchdog/itco.c
> @@ -40,6 +40,7 @@ enum iTCO_chipsets {
> ITCO_INTEL_WBG,
> ITCO_INTEL_EHL,
> ITCO_INTEL_TLH,
> + ITCO_INTEL_SUNRISE_LP,
> };
>
> enum iTCO_versions {
> @@ -92,7 +93,12 @@ static const iTCO_regs iTCO_version_regs[] = {
> },
> [ITCO_V4] =
> {
> - /* Not implemented yet */
> + .pmc_base_reg = 0x10,
> + .pmc_reg = 0x1008,
> + .pmc_no_reboot_mask = (1 << 4),

Is that correct? I'm comparing things here to
linux/drivers/watchdog/iTCO_wdt.c, no_reboot_bit() which seems to
deliver a mask, not a bit number.

> + .pmc_base_addr_mask = 0xfffffe00,
> + .pm_base_reg = 0x400,

Minor: Indention is off here.

> + .pm_base_addr_mask = 0x0000ff80,
> },
> [ITCO_V5] =
> {
> @@ -163,6 +169,12 @@ static const iTCO_info iTCO_chipset_info[] = {
> .pci_id = 0x43a3,
> .itco_version = ITCO_V6,
> },
> + [ITCO_INTEL_SUNRISE_LP] =
> + {
> + .name = L"Sunrise Point-LP",
> + .pci_id = 0x9d23,
> + .itco_version = ITCO_V4,
> + },
> };
>
> static BOOLEAN itco_supported(UINT16 pci_device_id, UINT8 *index)
> @@ -320,6 +332,7 @@ static EFI_STATUS init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
> update_no_reboot_flag_cnt(tco_base);
> break;
> case ITCO_V5:
> + case ITCO_V4:
> update_no_reboot_flag_apl(itco);
> break;
> case ITCO_V3:

Thanks for enabling this!

Jan

--
Siemens AG, Technology
Linux Expert Center

[Mathieu Alexandre-Tetreault]

> > enum iTCO_versions {
> > @@ -92,7 +93,12 @@ static const iTCO_regs iTCO_version_regs[] = {
> > },
> > [ITCO_V4] =
> > {
> > - /* Not implemented yet */
> > + .pmc_base_reg = 0x10,
> > + .pmc_reg = 0x1008,
> > + .pmc_no_reboot_mask = (1 << 4),
>
> Is that correct? I'm comparing things here to
> linux/drivers/watchdog/iTCO_wdt.c, no_reboot_bit() which seems to deliver a
> mask, not a bit number.

(1<<4) should be equivalent to 0x00000010. However, it seems that you are right the value seems to be wrong. The linux kernel uses 0x00000002(1<<1) for iTCO v4.

I'll get the device and do additional tests.
Do you have some test that you'd like me to do to confirm?

>
> > + .pmc_base_addr_mask = 0xfffffe00,
> > + .pm_base_reg = 0x400,
>
> Minor: Indention is off here.

Good catch, thanks.

[Jan Kiszka]

On 19.04.24 14:24, 'Mathieu Alexandre-Tetreault' via EFI Boot Guard wrote:
>>> enum iTCO_versions {
>>> @@ -92,7 +93,12 @@ static const iTCO_regs iTCO_version_regs[] = {
>>> },
>>> [ITCO_V4] =
>>> {
>>> - /* Not implemented yet */
>>> + .pmc_base_reg = 0x10,
>>> + .pmc_reg = 0x1008,
>>> + .pmc_no_reboot_mask = (1 << 4),
>>
>> Is that correct? I'm comparing things here to
>> linux/drivers/watchdog/iTCO_wdt.c, no_reboot_bit() which seems to deliver a
>> mask, not a bit number.
> (1<<4) should be equivalent to 0x00000010. However, it seems that you are right the value seems to be wrong. The linux kernel uses 0x00000002(1<<1) for iTCO v4.
>
> I'll get the device and do additional tests.
> Do you have some test that you'd like me to do to confirm?
>

Well, the test is simple: Check that the watchdog is firing after the
configured time, rebooting the system. To enforce that, you could load a
Linux kernel that does not contain an iTCO watchdog driver.

[Jan Kiszka]

Already had a chance to check? The Debian community is asking for a new
release as we have a number of build fixes in master for latest gnuefi,
and they like to update that. Would be good to have this feature
included into the next release as well.

[Mathieu Alexandre-Tetreault]

We currently have a limited amount of iTCO v4 devices. I sent a request to have access to the device, I don't know when I'll get access to it unfortunately.

With that said, if someone have an iTCO v4 and would like to test. I wouldn't mind at all.

I know that at some point Arturs Laizans had a device. https://groups.google.com/g/efibootguard-dev/c/vejOTECkE3s/m/4OktAjFHBAAJ

Best,

Mathieu

[Christopher Obbard]

Hi,

I've tried to enable iTCO v4 on a Lenovo Thinkcenter with an i5-6500 using
this patch.


I've modified the patch to update the no_reboot_mask which Jan has already
pointed out (as well as fixing the formatting issues):

@@ -101,9 +111,17 @@ static const iTCO_regs iTCO_version_regs[] = {
.pm_base_reg = 0x40,
.pm_base_addr_mask = 0x0000ff80,

},
[ITCO_V4] =
{
- /* Not implemented yet */
+ .pmc_base_reg = 0x10,
+ .pmc_reg = 0x1008,

+ .pmc_no_reboot_mask = (1 << 1),

+ .pmc_base_addr_mask = 0xfffffe00,
+ .pm_base_reg = 0x400,
+ .pm_base_addr_mask = 0x0000ff80,
},

.. and I've added the PCI ID for this SoC to the device mapping table ...

@@ -240,6 +258,27 @@ static const iTCO_info iTCO_chipset_info[] = {
.pci_id = 0x5796,
.itco_version = ITCO_V6,
},
+ [ITCO_INTEL_SUNRISE_H] =
+ {
+ .name = L"Sunrise Point-H",
+ .pci_id = 0xa123,

+ .itco_version = ITCO_V4,
+ },

but the watchdog doesn't probe on that device and efibootguard simply errors
out with:

Detected Intel TCO Sunrise Point-H watchdog
ERROR: reading PM_BASE: Unsupported


so this mail is a NACK for the original patch, enabling iTCO v4. Did you
manage to test it on real hardware?


Thanks,

Chris

[Mathieu Alexandre-Tetreault]

Hello Christopher,

Thanks for taking the time to test the iTCO v4 patch.

> -----Original Message-----
> From: Christopher Obbard <chris....@collabora.com>
> Sent: Monday, June 10, 2024 5:55 PM
> To: Mathieu Alexandre-Tetreault <mat...@rmds.ca>; efibootguard-
> d...@googlegroups.com
> Subject: Re: [PATCH] Add support for iTCOv4 (Sunrise Point-LP)
>
> [You don't often get email from chris....@collabora.com. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]

It was tested on real hardware and it was working. I'll double-check this patch against the one we use for v0.8. I am starting to think that there was some manipulation errors.
Thanks for reporting the issue.

I asked to get access to the iTCO v4 device in order to test this further.
>
>
> Thanks,
>
> Chris

Regards,

Mathieu

[Jan Kiszka]

Were you able to make progress on this? Unfortunately, I'm also lacking
HW access and can't help directly.