Re: [PATCH] Watchdog: sp5100_tco: Replace watchdog cd6h/cd7h port I/O accesses with MMIO accesses

[Guenter Roeck <linux@roeck-us.net>]

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