mbox series

[v2,0/2] Add driver for Intel Over-Clocking Watchdog

Message ID 20250312-ivo-intel_oc_wdt-v2-0-52d09738cd0b@siemens.com
Headers show
Series Add driver for Intel Over-Clocking Watchdog | expand

Message

Diogo Ivo March 12, 2025, 3:46 p.m. UTC
This series adds a driver for the Intel Over-Clocking Watchdog found in
the Intel Platform Controller Hub (PCH).

This watchdog is controlled via a simple single-register interface and
would otherwise be standard except for the presence of a LOCK bit that
can only be set once per power cycle, needing extra handling around it.

Due to the way these devices are described in ACPI tables with both the
generic PNP0C02 CID and a more detailed ACPI HID we also need to add
their HIDs to the list of known non-PNP devices. As there are several HIDs
for what seems to be essentially the same hardware but I don't know all
the possible HIDs this series does not include an exhaustive list of all
such HIDs, only those that I could personally test.

Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
---
Changes in v2:
- Split v1 into two patches, adding the ACPI IDs in a separate patch
- Initialize hearbeat module parameter to zero
- Clarify wording around lock handling
- Properly print resource with %pR when failing to obtain it
- Enable compile testing and add dependency on HAS_IOPORT
- Drop unneeded ACPI_PTR() and MODULE_ALIAS()
- Link to v1: https://lore.kernel.org/r/20250311-ivo-intel_oc_wdt-v1-1-fd470460d9f5@siemens.com

---
Diogo Ivo (2):
      watchdog: Add driver for Intel OC WDT
      ACPI: PNP: Add Intel OC Watchdog IDs to non-PNP device list

 drivers/acpi/acpi_pnp.c         |   2 +
 drivers/watchdog/Kconfig        |  11 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/intel_oc_wdt.c | 233 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 247 insertions(+)
---
base-commit: 4d872d51bc9d7b899c1f61534e3dbde72613f627
change-id: 20250227-ivo-intel_oc_wdt-7a483a4d6a04

Best regards,

Comments

Rafael J. Wysocki March 12, 2025, 7:31 p.m. UTC | #1
On Wed, Mar 12, 2025 at 4:46 PM Diogo Ivo <diogo.ivo@siemens.com> wrote:
>
> With the kernel having an ACPI driver for these watchdog devices add
> their IDs to the known non-PNP device list. Note that this commit is
> not a complete list of all the possible watchdog IDs.
>
> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
> ---
>  drivers/acpi/acpi_pnp.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
> index 01abf26764b00c86f938dea2ed138424f041f880..3f5a1840f573303c71f5d579e32963a5b29d2587 100644
> --- a/drivers/acpi/acpi_pnp.c
> +++ b/drivers/acpi/acpi_pnp.c
> @@ -355,8 +355,10 @@ static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matc
>   * device represented by it.
>   */
>  static const struct acpi_device_id acpi_nonpnp_device_ids[] = {
> +       {"INT3F0D"},
>         {"INTC1080"},
>         {"INTC1081"},
> +       {"INTC1099"},
>         {""},
>  };
>
>
> --

Is there a particular reason for this patch?
Diogo Ivo March 13, 2025, 10:28 a.m. UTC | #2
Hi Rafael,

On 3/12/25 7:31 PM, Rafael J. Wysocki wrote:
> On Wed, Mar 12, 2025 at 4:46 PM Diogo Ivo <diogo.ivo@siemens.com> wrote:
>>
>> With the kernel having an ACPI driver for these watchdog devices add
>> their IDs to the known non-PNP device list. Note that this commit is
>> not a complete list of all the possible watchdog IDs.
>>
>> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
>> ---
>>   drivers/acpi/acpi_pnp.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
>> index 01abf26764b00c86f938dea2ed138424f041f880..3f5a1840f573303c71f5d579e32963a5b29d2587 100644
>> --- a/drivers/acpi/acpi_pnp.c
>> +++ b/drivers/acpi/acpi_pnp.c
>> @@ -355,8 +355,10 @@ static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matc
>>    * device represented by it.
>>    */
>>   static const struct acpi_device_id acpi_nonpnp_device_ids[] = {
>> +       {"INT3F0D"},
>>          {"INTC1080"},
>>          {"INTC1081"},
>> +       {"INTC1099"},
>>          {""},
>>   };
>>
>>
>> --
> 
> Is there a particular reason for this patch?

Yes, since the ACPI tables for these watchdogs have both a PNP0C02 CID and
and then an HID (such as INT3F0D or INTC1099) without this patch the driver
in patch 01 will not bind to the device because PNP will bind to it first.
My understanding is that this table was added to solve exactly this problem
so I added these HIDs here, but if this is wrong and I misunderstood
please let me know.

Best regards,
Diogo
Rafael J. Wysocki March 13, 2025, 3:29 p.m. UTC | #3
Hi,

On Thu, Mar 13, 2025 at 11:28 AM Diogo Ivo <diogo.ivo@siemens.com> wrote:
>
> Hi Rafael,
>
> On 3/12/25 7:31 PM, Rafael J. Wysocki wrote:
> > On Wed, Mar 12, 2025 at 4:46 PM Diogo Ivo <diogo.ivo@siemens.com> wrote:
> >>
> >> With the kernel having an ACPI driver for these watchdog devices add
> >> their IDs to the known non-PNP device list. Note that this commit is
> >> not a complete list of all the possible watchdog IDs.
> >>
> >> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
> >> ---
> >>   drivers/acpi/acpi_pnp.c | 2 ++
> >>   1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
> >> index 01abf26764b00c86f938dea2ed138424f041f880..3f5a1840f573303c71f5d579e32963a5b29d2587 100644
> >> --- a/drivers/acpi/acpi_pnp.c
> >> +++ b/drivers/acpi/acpi_pnp.c
> >> @@ -355,8 +355,10 @@ static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matc
> >>    * device represented by it.
> >>    */
> >>   static const struct acpi_device_id acpi_nonpnp_device_ids[] = {
> >> +       {"INT3F0D"},
> >>          {"INTC1080"},
> >>          {"INTC1081"},
> >> +       {"INTC1099"},
> >>          {""},
> >>   };
> >>
> >>
> >> --
> >
> > Is there a particular reason for this patch?
>
> Yes, since the ACPI tables for these watchdogs have both a PNP0C02 CID and
> and then an HID (such as INT3F0D or INTC1099) without this patch the driver
> in patch 01 will not bind to the device because PNP will bind to it first.
> My understanding is that this table was added to solve exactly this problem
> so I added these HIDs here, but if this is wrong and I misunderstood
> please let me know.

You are right, but the above information is missing from the
changelog.  Please add it there.
Diogo Ivo March 13, 2025, 7:59 p.m. UTC | #4
On 3/13/25 3:29 PM, Rafael J. Wysocki wrote:
> Hi,
> 
> On Thu, Mar 13, 2025 at 11:28 AM Diogo Ivo <diogo.ivo@siemens.com> wrote:
>>
>> Hi Rafael,
>>
>> On 3/12/25 7:31 PM, Rafael J. Wysocki wrote:
>>> On Wed, Mar 12, 2025 at 4:46 PM Diogo Ivo <diogo.ivo@siemens.com> wrote:
>>>>
>>>> With the kernel having an ACPI driver for these watchdog devices add
>>>> their IDs to the known non-PNP device list. Note that this commit is
>>>> not a complete list of all the possible watchdog IDs.
>>>>
>>>> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
>>>> ---
>>>>    drivers/acpi/acpi_pnp.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/acpi_pnp.c b/drivers/acpi/acpi_pnp.c
>>>> index 01abf26764b00c86f938dea2ed138424f041f880..3f5a1840f573303c71f5d579e32963a5b29d2587 100644
>>>> --- a/drivers/acpi/acpi_pnp.c
>>>> +++ b/drivers/acpi/acpi_pnp.c
>>>> @@ -355,8 +355,10 @@ static bool acpi_pnp_match(const char *idstr, const struct acpi_device_id **matc
>>>>     * device represented by it.
>>>>     */
>>>>    static const struct acpi_device_id acpi_nonpnp_device_ids[] = {
>>>> +       {"INT3F0D"},
>>>>           {"INTC1080"},
>>>>           {"INTC1081"},
>>>> +       {"INTC1099"},
>>>>           {""},
>>>>    };
>>>>
>>>>
>>>> --
>>>
>>> Is there a particular reason for this patch?
>>
>> Yes, since the ACPI tables for these watchdogs have both a PNP0C02 CID and
>> and then an HID (such as INT3F0D or INTC1099) without this patch the driver
>> in patch 01 will not bind to the device because PNP will bind to it first.
>> My understanding is that this table was added to solve exactly this problem
>> so I added these HIDs here, but if this is wrong and I misunderstood
>> please let me know.
> 
> You are right, but the above information is missing from the
> changelog.  Please add it there.

I'll add it and send v3 after receiving feedback on the other patch in
the series.

Best regards,
Diogo