Message-ID: <CAHp75VdOLDCMzS+uRxLY1=0CX8tD4sfkeE6=5Sk2quNTn8z12A@mail.gmail.com>
Date:   Mon, 31 Jan 2022 13:01:35 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Terry Bowman <terry.bowman@....com>
Cc:     Guenter Roeck <linux@...ck-us.net>, linux-watchdog@...r.kernel.org,
        Jean Delvare <jdelvare@...e.com>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Wolfram Sang <wsa@...nel.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Robert Richter <rrichter@....com>,
        Tom Lendacky <thomas.lendacky@....com>,
        sudheesh.mavila@....com,
        "Shah, Nehal-bakulchandra" <Nehal-bakulchandra.Shah@....com>,
        Basavaraj Natikar <Basavaraj.Natikar@....com>,
        Shyam Sundar S K <Shyam-sundar.S-k@....com>,
        Mario Limonciello <Mario.Limonciello@....com>
Subject: Re: [PATCH v4 0/9] i2c: piix4: Replace cd6h/cd7h port I/O accesses
 with MMIO accesses

On Sun, Jan 30, 2022 at 8:41 PM Terry Bowman <terry.bowman@....com> wrote:
>
> This series changes the piix4_smbus driver's cd6h/cd7h port I/O accesses
> to use MMIO instead. This is necessary because cd6h/cd7h port I/O may be
> disabled on later AMD processors.
>
> This series includes patches with MMIO accesses to register
> FCH::PM::DECODEEN. The same register is also accessed by the sp5100_tco
> driver.[1] Synchronization to the MMIO register is required in both
> drivers.
>
> The first patch creates a macro to request MMIO region using the 'muxed'
> retry logic. This is used in patch 6 to synchronize accesses to EFCH MMIO.
>
> The second patch replaces a hardcoded region size with a #define. This is
> to improve maintainability and was requested from v2 review.
>
> The third patch moves duplicated region request/release code into
> functions. This locates related code into functions and reduces code line
> count. This will also make adding MMIO support in patch 6 easier.
>
> The fourth patch moves SMBus controller address detection into a function.
> This is in preparation for adding MMIO region support.
>
> The fifth patch moves EFCH port selection into a function. This is in
> preparation for adding MMIO region support.
>
> The sixth patch adds MMIO support for region requesting/releasing and
> mapping. This is necessary for using MMIO to detect SMBus controller
> address, enable SMBbus controller region, and control the port select.
>
> The seventh patch updates the SMBus controller address detection to support
> using MMIO. This is necessary because the driver accesses register
> FCH::PM::DECODEEN during initialization and only available using MMIO on
> later AMD processors.
>
> The eighth patch updates the SMBus port selection to support MMIO. This is
> required because port selection control resides in the
> FCH::PM::DECODEEN[smbus0sel] and is only accessible using MMIO on later AMD
> processors.
>
> The ninth patch enables the EFCH MMIO functionality added earlier in this
> series. The SMBus controller's PCI revision ID is used to check if EFCH
> MMIO is supported by HW and should be enabled in the driver.

I'm not against the series, but I am still concerned that we are using
_p IO without clear understanding why. With cleaning them up, this can
be simpler and cleaner.

For the i2c patches:
Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
if it helps somebody.

> Based on v5.16.

v5.17-rc2 is out :-)

>
> Testing:
>   Tested on family 19h using:
>     i2cdetect -y 0
>     i2cdetect -y 2
>
>   - Results using v5.16 and this pachset applied. Below
>     shows the devices detected on the busses:
>
>     # i2cdetect -y 0
>          0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>     00:                         -- -- -- -- -- -- -- --
>     10: 10 11 -- -- -- -- -- -- 18 -- -- -- -- -- -- --
>     20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>     30: 30 -- -- -- -- 35 36 -- -- -- -- -- -- -- -- --
>     40: -- -- -- -- -- -- -- -- -- -- 4a -- -- -- -- --
>     50: 50 -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>     60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>     70: -- -- -- 73 -- -- -- --
>     # i2cdetect -y 2
>          0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
>     00:                         -- -- -- -- -- -- -- --
>     10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>     20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>     30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
>     40: -- -- -- -- -- -- -- -- -- -- -- -- 4c -- -- --
>     50: -- 51 -- -- 54 -- -- -- -- -- -- -- -- -- -- --
>     60: 60 -- -- 63 -- -- 66 -- -- -- -- 6b -- -- 6e --
>     70: 70 71 72 73 74 75 -- 77
>
>   Also tested using sp5100_tco submitted series listed below.[1]
>   I applied the sp5100_tco v4 series and ran:
>     cat  >> /dev/watchdog
>
> [1] sp5100_tco v4 patchset is in process but not sent yet. Below is v3
>     upstream review:
> Link: https://lore.kernel.org/linux-watchdog/20220118202234.410555-1-terry.bowman@amd.com/
>
> Changes in v4:
>  - Changed request_muxed_mem_region() macro to request_mem_region_muxed()
>    in patch 1. (Andy Shevchenko)
>  - Removed unnecessary newline where possible in calls to
>    request_muxed_region() in patch 2. (Terry Bowman)
>  - Changed piix4_sb800_region_setup() to piix4_sb800_region_request().
>    Patch 3. (Jean Delvare)
>  - Reordered piix4_setup_sb800() local variables from longest name length
>    to shortest name length. Patch 4. (Terry Bowman)
>  - Changed piix4_sb800_region_request() and piix4_sb800_region_release() by
>    adding early return() to remove 'else' improving readability. Patch 6.
>    (Terry Bowman)
>  - Removed iowrite32(ioread32(...), ...). Unnecessary because MMIO is
>    already enabled. (Terry Bowman)
>  - Refactored piix4_sb800_port_sel() to simplify the 'if' statement using
>    temp variable. Patch 8. (Terry Bowman)
>  - Added mmio_cfg.use_mmio assignment in piix4_add_adapter(). This is
>    needed for calls to piix4_sb800_port_sel() after initialization during
>    normal operation. Patch 9. (Terry Bowman)
>
> Changes in v3:
>  - Added request_muxed_mem_region() patch (Wolfram, Guenter)
>  - Reduced To/Cc list length. (Andy)
>
> Changes in v2:
>  - Split single patch. (Jean Delvare)
>  - Replace constant 2 with SB800_PIIX4_SMB_MAP_SIZE where appropriate.
>    (Jean Delvare)
>  - Shorten SB800_PIIX4_FCH_PM_DECODEEN_MMIO_EN name length to
>    SB800_PIIX4_FCH_PM_DECODEEN_MMIO. (Jean Delvare)
>  - Change AMD_PCI_SMBUS_REVISION_MMIO from 0x59 to 0x51. (Terry Bowman)
>  - Change piix4_sb800_region_setup() to piix4_sb800_region_request().
>    (Jean Delvare)
>  - Change 'SMB' text in  logging to 'SMBus' (Jean Delvare)
>  - Remove unnecessary NULL assignment in piix4_sb800_region_release().
>    (Jean Delvare)
>  - Move 'u8' variable definitions to single line. (Jean Delvare)
>  - Hardcode piix4_setup_sb800_smba() return value to 0 since it is always
>    0. (Jean Delvare)
>
> Terry Bowman (9):
>   kernel/resource: Introduce request_mem_region_muxed()
>   i2c: piix4: Replace hardcoded memory map size with a #define
>   i2c: piix4: Move port I/O region request/release code into functions
>   i2c: piix4: Move SMBus controller base address detect into function
>   i2c: piix4: Move SMBus port selection into function
>   i2c: piix4: Add EFCH MMIO support to region request and release
>   i2c: piix4: Add EFCH MMIO support to SMBus base address detect
>   i2c: piix4: Add EFCH MMIO support for SMBus port select
>   i2c: piix4: Enable EFCH MMIO for Family 17h+
>
>  drivers/i2c/busses/i2c-piix4.c | 207 ++++++++++++++++++++++++++-------
>  include/linux/ioport.h         |   2 +
>  2 files changed, 164 insertions(+), 45 deletions(-)
>
> --
> 2.30.2



-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists