LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Terry Bowman <Terry.Bowman@amd.com>
To: Guenter Roeck <linux@roeck-us.net>, linux-watchdog@vger.kernel.org
Cc: wim@linux-watchdog.org, linux-kernel@vger.kernel.org,
	rrichter@amd.com, thomas.lendacky@amd.com
Subject: Re: [PATCH] Watchdog: sp5100_tco: Replace watchdog cd6h/cd7h port I/O accesses with MMIO accesses
Date: Mon, 23 Aug 2021 12:58:18 -0500	[thread overview]
Message-ID: <c47b05ee-5e0a-08ee-318a-52e370619d2c@amd.com> (raw)
In-Reply-To: <b32c2263-94a1-fef5-4455-6d7bef40a968@roeck-us.net>



On 8/18/21 3:34 PM, Guenter Roeck wrote:
> On 8/16/21 2:29 PM, Terry Bowman wrote:
>>
>>
>> On 8/13/21 5:37 PM, Guenter Roeck wrote:
>>> On 8/13/21 2:32 PM, Terry Bowman wrote:
>>>> Use MMIO instead of port I/O during SMBus controller address discovery.
>>>> Also, update how EFCH capability is determined by replacing a family check
>>>> with a PCI revision ID check.
>>>>
>>>> cd6h/cd7h port I/O can be disabled on recent AMD hardware. Read accesses to
>>>> disabled cd6h/cd7h port I/O will return F's and written data is dropped.
>>>> The recommended workaround to handle disabled cd6h/cd7h port I/O is
>>>> replacing port I/O with MMIO accesses. The MMIO access method has been
>>>> available since at least SMBus controllers using PCI revision 0x59.
>>>>
>>>> The sp5100_tco driver uses a CPU family match of 17h to determine
>>>> EFCH_PM_DECODEEN_WDT_TMREN register support. Using a family check requires
>>>> driver updates for each new AMD CPU family following 17h. This patch
>>>> replaces the family check with a check for SMBus PCI revision ID 0x59 and
>>>> later. Note: Family 17h processors use SMBus PCI revision ID 0x59. The
>>>> intent is to use the PCI revision ID check to support future AMD processors
>>>> while minimizing required driver changes. The caveat with this change is
>>>> the sp5100_tco driver must be updated if a new AMD processor family changes
>>>> the EFCH design or the SMBus PCI ID value doesn't follow this pattern.
>>>>
>>>> Tested with forced WDT reset using `cat >> /dev/watchdog`.
>>>>
>>>
>>> I am sorry, I don't understand why the new code can not use devm functions,
>>> why the new data structure is necessary in the first place, and why it is
>>> not possible to improve alignment with the existing code. This will require
>>> a substantial amount of time to review to ensure that the changes are not
>>> excessive (at first glance it for sure looks like that to me).
>>>
>>> Guenter
>>>
>>
>> Hi Guenter,
>>
>> I can change the patch to use devm functions as you mentioned. My
>> understanding is the patch's reservation and mapping related functions
>> are the focus. I originally chose not to use devm functions because the
>> patch's MMIO reserved and mapped resources are not held for the driver
>> lifetime as is the case for most device managed resources. The
>> sp5100_tco driver must only hold these MMIO resources briefly because
>> other drivers use the same EFCH MMIO registers. An example of another
>> driver using the same registers is the piix4_smbus driver (drivers/i2c
>> /busses/i2c-piix4.c). This patch can be changed to use the devm
>> functions but the driver may not benefit from the device management.
>>
>> The 'struct efch_cfg' addition is needed for MMIO reads/writes as well
>> as during cleanup when leaving sp5100_region_setup(). This structure was
>> chosen to contain the data instead of passing multiple parameters to
>> each EFCH function called.
>>
>> Do you have any recommendations for how to best improve the alignment?
>>
> 
> Overall it seems to me that it might make more sense to implement this
> as new driver instead of messing with the existing driver. Have you
> thought about that ?
> 
> Guenter

A new driver was initially considered for this patch. I decided to update
the sp5100_tco driver instead because the changes are limited to
initialization and post-initialization code is not modified. Also, updating
the existing sp5100_tco driver allows for code reuse.

This patch can be split into 2 or more patches to simplify review and
testing. Would this help?

Regards,
Terry

  reply	other threads:[~2021-08-23 17:58 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13 21:32 Terry Bowman
2021-08-13 22:37 ` Guenter Roeck
2021-08-16 21:29   ` Terry Bowman
2021-08-18 20:34     ` Guenter Roeck
2021-08-23 17:58       ` Terry Bowman [this message]
2021-09-06 12:05   ` Robert Richter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c47b05ee-5e0a-08ee-318a-52e370619d2c@amd.com \
    --to=terry.bowman@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=rrichter@amd.com \
    --cc=thomas.lendacky@amd.com \
    --cc=wim@linux-watchdog.org \
    --subject='Re: [PATCH] Watchdog: sp5100_tco: Replace watchdog cd6h/cd7h port I/O accesses with MMIO accesses' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).