mbox series

[v4,0/4] Watchdog: sp5100_tco: Replace cd6h/cd7h port I/O accesses with MMIO accesses

Message ID 20220130191225.303115-1-terry.bowman@amd.com
Headers show
Series Watchdog: sp5100_tco: Replace cd6h/cd7h port I/O accesses with MMIO accesses | expand

Message

Terry Bowman Jan. 30, 2022, 7:12 p.m. UTC
This driver uses cd6h/cd7h port I/O to access the FCH::PM::DECODEEN
register during driver initialization. cd6h/cd7h port I/O is no longer
supported on later AMD processors and the recommended method to access
this register is using MMIO. This series will replace the cd6h/cd7h port
I/O with MMIO accesses during initialization.

The first patch refactors watchdog timer initialization into a separate
function. This is needed to add support for new device layouts without
adding complexity.

The second patch moves region request/release into new functions. The
request/release functions provide a location for adding MMIO region
support.

The third patch introduces EFCH initialization using MMIO. This is
required because the registers are no longer accessible using cd6h/cd7h
port I/O.

The fourth patch adds SMBus controller PCI ID check to enable EFCH MMIO
initialization. This eliminates the need for driver updates to support
future processors supporting the same EFCH functionality.

This series includes patches with MMIO accesses to register
FCH::PM::DECODEEN. The same register is also accessed by the piix4_smbus
driver. Both drivers use request_mem_region_muxed() to synchronize the
accesses. request_mem_region_muxed() definition is added in parallel
piix4_smbus patchset review with review URL provided below as a dependency.

Dependency:
Link: https://lore.kernel.org/linux-i2c/20220130184130.176646-2-terry.bowman@amd.com/

Based on v5.16

Testing:
Tested on AMD family 17h and family 19h processors using:

cat  >> /dev/watchdog

Hi Jean,
Please confirm to leave your reviewed-by and tested-by.

Changes in V4:
  - Change to only call devm_ioremap() once. (Guenter Roeck, Jean Delvare)
  - Remove trailing dot for consistency with the other messages.
    (Jean Delvare)
  - Update print formatting in sp5100_tco_prepare_base(). Change period to
    a comma, use '0x%x', and change return code to decimal display.
    (Jean Delvare)
  - Move dev_err() linebreak to 'dev,' in sp5100_tco_prepare_base().
    (Jean Delvare)
  - Remove unused variable. (Andy Shevchenko)
  - Remove unnecessary assignment in sp5100_tco_prepare_base().
    (Andy Shevchenko)
  - Unify comment in sp5100_tco_prepare_base().  (Andy Shevchenko)
  - Fix line break for readability in 'if' in sp5100_tco_prepare_base().
    (Andy Shevchenko)
  - Fix logic issue in 'if' in sp5100_tco_setupdevice(). Added temp
    variable val. (Terry Bowman, Jean Delvare)    
  - Change capitalized letters to lowercase in sp5100_tco_prepare_base().
    (Andy Shevchenko)
  - Add dependency note for piix4_smbus driver. (Andy Shevchenko)
  - Change "SMB" -> "SMBus". (Jean Delvare)
  - Add comment for logic in sp5100_tco_setupdevice_mmio(). (Jean Delvare)
  - Fix 2 locations of line breaks in sp5100_tco_setupdevice_mmio().
    (Jean Delvare)
  
Changes in V3:
  - Remove 'addr' and 'res' variables from struct sp5100_tco.
    (Guenter Roeck)
  - Pass address directly to efch_read_pm_reg8() and
    efch_update_pm_reg8(). (Guenter Roeck)
  - Reword patch descriptions. (Terry Bowman)
  - Change #define AMD_ZEN_SMBUS_PCI_REV value from 0x59 to 0x51. This was
    determined after investigating programmers manual and testing.
    (Robert Richter)
  - Refactor efch_* functions() (Robert Richter)
  - Remove trailing whitespace in patch. (Guenter Roeck)

Changes in V2:
   - Refactor into 4 patch series
   - Move MMIO reservation and mapping into helper functions
   - Combine mmio_addr and alternate mmio_addr base address discovery
   - Replace efch_use_mmio() with efch_mmio layout type
   
Terry Bowman (4):
  Watchdog: sp5100_tco: Move timer initialization into function
  Watchdog: sp5100_tco: Refactor MMIO base address initialization
  Watchdog: sp5100_tco: Add initialization using EFCH MMIO
  Watchdog: sp5100_tco: Enable Family 17h+ CPUs

 drivers/watchdog/sp5100_tco.c | 334 ++++++++++++++++++++++------------
 drivers/watchdog/sp5100_tco.h |   7 +
 2 files changed, 226 insertions(+), 115 deletions(-)

Comments

Jean Delvare Feb. 7, 2022, 12:44 p.m. UTC | #1
Hi Terry,

On Sun, 30 Jan 2022 13:12:21 -0600, Terry Bowman wrote:
> Please confirm to leave your reviewed-by and tested-by.

Confirmed. I reviewed the 4 patches of this version of the series and
I'm fine with them. I also tested the result successfully on my laptop.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jean Delvare <jdelvare@suse.de>

Thanks for your work.
Jean Delvare Feb. 8, 2022, 9:45 a.m. UTC | #2
On Mon, 7 Feb 2022 14:03:08 +0100, Wolfram Sang wrote:
> > Confirmed. I reviewed the 4 patches of this version of the series and
> > I'm fine with them. I also tested the result successfully on my laptop.
> > 
> > Reviewed-by: Jean Delvare <jdelvare@suse.de>
> > Tested-by: Jean Delvare <jdelvare@suse.de>  
> 
> Does that mean you are happy with the i2c-piix4 changes as well?

I'm still reviewing these, sorry. I only picked the first patch of the
series so that the sp5100_tco patches would build so I could test them.
I hope to be done by the end of the day.
Wolfram Sang Feb. 8, 2022, 10 a.m. UTC | #3
> I'm still reviewing these, sorry. I only picked the first patch of the
> series so that the sp5100_tco patches would build so I could test them.

Ah, I see. I thought more than the first patch was needed for testing.

> I hope to be done by the end of the day.

Awesome, thank you!
Jean Delvare Feb. 8, 2022, 2:20 p.m. UTC | #4
On Tue, 8 Feb 2022 11:00:31 +0100, Wolfram Sang wrote:
> > I'm still reviewing these, sorry. I only picked the first patch of the
> > series so that the sp5100_tco patches would build so I could test them.  
> 
> Ah, I see. I thought more than the first patch was needed for testing.

You need the full series to be on the safe side, otherwise there's a
risk that the two drivers will access the same registers using
different methods (legacy I/O vs MMIO) so there's no synchronization
and they could step on each other's toes.

However as I knew about this limitation, I was careful to not use the
SMBus driver while I was testing the watchdog driver :-)