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
