| Message ID | 20220118202234.410555-1-terry.bowman@amd.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Watchdog: sp5100_tco: Replace cd6h/cd7h port I/O accesses with MMIO accesses | expand |
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.
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
> Also, if the immutable branch only includes the patch introducing > request_muxed_mem_region(), the pull order should not really matter. Right, good point!
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.
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