This driver uses cd6h/cd7h port I/O to access the FCH::PM::DECODEEN and
FCH::PM::ISACONTROL registers during driver initialization. cd6h/cd7h port
I/O is no longer supported on later AMD processors and the recommended
method to access these registers 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.

Important:
This series includes patches with MMIO accesses to registers
FCH::PM::DECODEEN and FCH::PM::ISACONTROL. The same registers are also
accessed by the piix4_smbus driver. The registers are currently accessed
indirectly through cd6h/cd7h port I/O and both drivers use
request_muxed_region() to synchronize the accesses. It should be noted the
request_muxed_region() uses a wait queue to sleep and retry taking
exclusive access if the port I/O region is busy.

This series uses request_mem_region() to synchronize accesses to the MMIO
registers mentioned above. request_mem_region() is missing the retry
logic in the case the resource is busy. As a result, request_mem_region()
will fail immediately if the resource is busy. The 'muxed' variant is
needed here but request_muxed_mem_region() is not defined to use.  I will
follow up with another patch series to define the
request_muxed_mem_region() and use in both drivers.

The piix4_smbus driver or the sp5100_tco driver can potentialy fail until
the muxed mem synchronization series is present in the tree. The potential
for failure is not likely because the sp5100_tco driver only accesses the
FCH::PM::DECODEEN and FCH::PM::ISACONTROL registers during driver
initialization.

Link: https://lore.kernel.org/all/20210715221828.244536-1-Terry.Bowman@amd.com/#t

Based on v5.16

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

cat  >> /dev/watchdog

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)

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 | 335 ++++++++++++++++++++++------------
 drivers/watchdog/sp5100_tco.h |   6 +
 2 files changed, 227 insertions(+), 114 deletions(-)

--
2.30.2

Hi Terry,

On Tue, 18 Jan 2022 14:22:30 -0600, Terry Bowman wrote:
> This series uses request_mem_region() to synchronize accesses to the MMIO
> registers mentioned above. request_mem_region() is missing the retry
> logic in the case the resource is busy. As a result, request_mem_region()
> will fail immediately if the resource is busy. The 'muxed' variant is
> needed here but request_muxed_mem_region() is not defined to use.  I will
> follow up with another patch series to define the
> request_muxed_mem_region() and use in both drivers.

Shouldn't this be done the other way around, first introducing
request_muxed_mem_region() and then using it directly in both drivers,
rather than having a temporary situation where a failure can happen?

As far as I'm concerned, the patch series you just posted are
acceptable only if request_muxed_mem_region() gets accepted too.
Otherwise we end up with the situation where a driver could randomly
fail.

-- 
Jean Delvare
SUSE L3 Support

On 1/19/22 9:30 AM, Jean Delvare wrote:
> Hi Terry,
> 
> On Tue, 18 Jan 2022 14:22:30 -0600, Terry Bowman wrote:
>> This series uses request_mem_region() to synchronize accesses to the MMIO
>> registers mentioned above. request_mem_region() is missing the retry
>> logic in the case the resource is busy. As a result, request_mem_region()
>> will fail immediately if the resource is busy. The 'muxed' variant is
>> needed here but request_muxed_mem_region() is not defined to use.  I will
>> follow up with another patch series to define the
>> request_muxed_mem_region() and use in both drivers.
> 
> Shouldn't this be done the other way around, first introducing
> request_muxed_mem_region() and then using it directly in both drivers,
> rather than having a temporary situation where a failure can happen?
> 
> As far as I'm concerned, the patch series you just posted are
> acceptable only if request_muxed_mem_region() gets accepted too.
> Otherwise we end up with the situation where a driver could randomly
> fail.
> 

Hi Jean,
                                                                      
I considered sending the request_muxed_mem_region() patch series first but 
was concerned the patch might not be accepted without a need or usage. I 
didn't see an obvious path forward for the order of submissions because of 
the dependencies. 

I need to make the review easy for you and the other maintainers. I can 
send the request_muxed_mem_region() single patch series ASAP if you like. 
Then I change the request_mem_region() -> request_muxed_mem_region() as 
needed in the piix4_smbus v3 and sp5100_tco v4 and add dependency line as 
well? Is their a risk the driver patches will take 2 merge windows before 
added to the tree ? Is there anything I can do to avoid this?

Regards,
Terry

> I considered sending the request_muxed_mem_region() patch series first but 
> was concerned the patch might not be accepted without a need or usage. I 
> didn't see an obvious path forward for the order of submissions because of 
> the dependencies. 

My suggestion: make the request_muxed_mem_region() patch the new patch 1
of the piix4 series. Then, the user will directly come in the following
patches. From this series, I will create an immutable branch which can
be pulled in by the watchdog tree. It will then have the dependency for
your watchdog series. During next merge window, we (the maintainers)
will make sure that I2C will hit Linus' tree before the watchdog tree.

This works the other way around as well, if needed. Make
request_muxed_mem_region() the first patch of the watchdog series and
let me pull an immutable branch from watchdog into I2C.

On 1/19/22 9:47 AM, Wolfram Sang wrote:
> 
>> I considered sending the request_muxed_mem_region() patch series first but
>> was concerned the patch might not be accepted without a need or usage. I
>> didn't see an obvious path forward for the order of submissions because of
>> the dependencies.
> 
> My suggestion: make the request_muxed_mem_region() patch the new patch 1
> of the piix4 series. Then, the user will directly come in the following
> patches. From this series, I will create an immutable branch which can
> be pulled in by the watchdog tree. It will then have the dependency for
> your watchdog series. During next merge window, we (the maintainers)
> will make sure that I2C will hit Linus' tree before the watchdog tree.
> 
> This works the other way around as well, if needed. Make
> request_muxed_mem_region() the first patch of the watchdog series and
> let me pull an immutable branch from watchdog into I2C.
> 

Creating an immutable branch from i2c is fine. Also, typically Wim sends
his pull request late in the commit window, so i2c first should be no
problem either.

Also, if the immutable branch only includes the patch introducing
request_muxed_mem_region(), the pull order should not really matter.

Guenter

On 1/19/22 12:39 PM, Guenter Roeck wrote:
> On 1/19/22 9:47 AM, Wolfram Sang wrote:
>>
>>> I considered sending the request_muxed_mem_region() patch series first but
>>> was concerned the patch might not be accepted without a need or usage. I
>>> didn't see an obvious path forward for the order of submissions because of
>>> the dependencies.
>>
>> My suggestion: make the request_muxed_mem_region() patch the new patch 1
>> of the piix4 series. Then, the user will directly come in the following
>> patches. From this series, I will create an immutable branch which can
>> be pulled in by the watchdog tree. It will then have the dependency for
>> your watchdog series. During next merge window, we (the maintainers)
>> will make sure that I2C will hit Linus' tree before the watchdog tree.
>>
>> This works the other way around as well, if needed. Make
>> request_muxed_mem_region() the first patch of the watchdog series and
>> let me pull an immutable branch from watchdog into I2C.
>>
> 
> Creating an immutable branch from i2c is fine. Also, typically Wim sends
> his pull request late in the commit window, so i2c first should be no
> problem either.
> 
> Also, if the immutable branch only includes the patch introducing
> request_muxed_mem_region(), the pull order should not really matter.
> 
> Guenter

Ok, I'll add the request_muxed_mem_region() patch to the i2c v3 series
as the first patch.

Reqards,
Terry

On Tue, 18 Jan 2022 14:22:30 -0600, Terry Bowman wrote:
> 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 | 335 ++++++++++++++++++++++------------
>  drivers/watchdog/sp5100_tco.h |   6 +
>  2 files changed, 227 insertions(+), 114 deletions(-)

FWIW, I tested this patch series successfully on my AMD Ryzen 7 PRO
3700U-based laptop.

-- 
Jean Delvare
SUSE L3 Support