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

[Guenter Roeck]

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