| Message ID | Yj19RH3qpzQsIV/O@shikoro |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [PULL,REQUEST] i2c for v5.18 | expand |
On Fri, Mar 25, 2022 at 1:28 AM Wolfram Sang <wsa@kernel.org> wrote: > > I2C has for 5.18: tracepoints when Linux acts as an I2C client, added > support for AMD PSP, whole subsytsem now uses generic_handle_irq_safe(), > piix4 driver gained MMIO access enabling so far missed controllers with > AMD chipsets, plus a bulk of device driver updates, refactorization, and > fixes. It feels odd/wrong to use the piix4 driver for the AMD MMIO case on SB800. Would it not have made more sense to just make that a separate driver? It feels like now the piix4 driver has a lot of "if SB800" for the probing code, and then a lot of "if (mmio)" at runtime. I've pulled this, but just wanted to mention this "that looks a bit odd". How much code is actually _shared_ in the SB800 case? I'm not insisting on splitting this up - maybe it all makes sense. I'm just questioning it. Linus
The pull request you sent on Fri, 25 Mar 2022 09:28:52 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-mergewindow
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5627ecb8374a00163d90bc92c33f829ac27895b2
Thank you!
On Fri, Mar 25, 2022 at 1:28 AM Wolfram Sang <wsa@kernel.org> wrote: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-mergewindow Oh, and another comment about this pull request: you are one of five people whose pull requests remain unsigned. So you're not exactly alone, but it's a (happily) shrinking group of kernel maingainers that don't use signed tags for pulls. Of course, I haven't checked the ones that are still pending in linux-next, but I've done 127 merges so far this merge window, and 93% of them have been using signed tags (and three of them have been the Andrew Morton email patch-bomb merges). Yes, that's a good percentage, but it could be even better. Hint hint. Linus
On Sat, Mar 26, 2022 at 12:58:36PM -0700, Linus Torvalds wrote: > On Fri, Mar 25, 2022 at 1:28 AM Wolfram Sang <wsa@kernel.org> wrote: > > > > I2C has for 5.18: tracepoints when Linux acts as an I2C client, added > > support for AMD PSP, whole subsytsem now uses generic_handle_irq_safe(), > > piix4 driver gained MMIO access enabling so far missed controllers with > > AMD chipsets, plus a bulk of device driver updates, refactorization, and > > fixes. > > It feels odd/wrong to use the piix4 driver for the AMD MMIO case on SB800. > > Would it not have made more sense to just make that a separate driver? > > It feels like now the piix4 driver has a lot of "if SB800" for the > probing code, and then a lot of "if (mmio)" at runtime. > > I've pulled this, but just wanted to mention this "that looks a bit > odd". How much code is actually _shared_ in the SB800 case? > > I'm not insisting on splitting this up - maybe it all makes sense. I'm > just questioning it. Adding Jean to CC, he maintains the PC-style drivers.
> > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-mergewindow > > Oh, and another comment about this pull request: you are one of five > people whose pull requests remain unsigned. Challenge to be not the last one accepted!
Hi Linus, Wolfram, On Mon, 28 Mar 2022 21:01:08 +0200, Wolfram Sang wrote: > On Sat, Mar 26, 2022 at 12:58:36PM -0700, Linus Torvalds wrote: > > It feels odd/wrong to use the piix4 driver for the AMD MMIO case on SB800. > > > > Would it not have made more sense to just make that a separate driver? > > > > It feels like now the piix4 driver has a lot of "if SB800" for the > > probing code, and then a lot of "if (mmio)" at runtime. > > > > I've pulled this, but just wanted to mention this "that looks a bit > > odd". How much code is actually _shared_ in the SB800 case? > > > > I'm not insisting on splitting this up - maybe it all makes sense. I'm > > just questioning it. > > Adding Jean to CC, he maintains the PC-style drivers. Well, that's a legitimate question, that I asked myself before many times to be honest. To understand why things are the way they are, you have to dig through the history of the driver. Originally it was a driver for Intel chipsets (82371 aka PIIX4, then 82443 aka 440BX). Then support was added for various clones (Victory66 and several ServerWorks chipsets) which were fully compatible (modulo the srvrworks_csb5_delay quirk). Then ATI came with compatible chipsets as well (IXP200, IXP300 and IXP400). These were still very similar to the original Intel design, with a single SMBus controller driving a single SMBus port. So far so good. Where things started diverging is with the ATI SB700, which introduced a second SMBus controller. Then came the ATI SB800, which introduced a 4-port multiplexer on top of the main SMBus controller. Then AMD bought ATI and the new chipsets came with new PCI device IDs and a slightly different configuration procedure, plus a potential conflict with the IMC which require extra care. The move of the latest AMD chipsets to MMIO is only one more diverging step in this list. The reason why I find a driver split difficult is because there's no clear line where to cut. We could have a driver with MMIO support and one without, as suggested by Linus. But we could also move the line and have a driver with multiplexer + MMIO support, and one with neither [1]. Or a driver with aux port + multiplexer + MMIO support, and one with neither. None of these cuts is obviously "the good one", they are all pretty arbitrary. In all cases, that's going to duplicate a fair amount of code, as the SMBus block itself is still exactly the same. So at least piix4_transaction(), piix4_access(), piix4_add_adapter() and piix4_func() would be needed by both drivers. If we don't want to duplicate the code, we'd have to create a shared module that both drivers would rely on. While a clean design, it does not really go in the direction of simplification. If we split on MMIO support then the amount of duplicated (or shared) code would be even larger, as it would also include support of the aux port, multiplexing and IMC conflict workaround. The real question here is, what do we win by having 2 drivers? We better win something, because that's a large amount of work, and renaming a driver can make life difficult for downstream (it breaks blacklisting, preset module parameters, requires kernel configuration and packaging adjustments, etc). And a split is even worse than a rename, as some of these changes then become conditional. In the end, the only benefit I can see is a reduced memory footprint on old systems, which could use the "simple" driver which would be very close to what the i2c-piix4 driver looked like 15 years ago. I don't think that's a goal worth pursuing though, as the number of users of these old chipsets must very small by now. On the other hand, the benefit for the users of recent hardware is marginal, as removing support for the oldest chipsets from the current driver wouldn't remove much code in the end. A rough estimation would be between 50 and 100 lines removed, which for a 1159-line driver isn't really meaningful. Plus, from a maintenance perspective, two drivers instead of one will automatically mean more work (maybe not much, but still). And this is how I came to the conclusion that, despite the weird feeling that there are too many conditionals in the i2c-piix4 driver, there's nothing smart that can be done to get rid of them, and we just have to live with them. [1] That would put the support of the SB700 in one driver, and the support of the SB800 in another, while they share the same PCI device ID (but with a different revision range). So both drivers would load on such systems by default, wasting memory.
On Tue, Mar 29, 2022 at 5:26 AM Jean Delvare <jdelvare@suse.de> wrote: > > And this is how I came to the conclusion that, despite the weird > feeling that there are too many conditionals in the i2c-piix4 driver, > there's nothing smart that can be done to get rid of them, and we just > have to live with them. Thanks for the very complete response. Color me convinced. Linus
Linus, I2C has for 5.18: tracepoints when Linux acts as an I2C client, added support for AMD PSP, whole subsytsem now uses generic_handle_irq_safe(), piix4 driver gained MMIO access enabling so far missed controllers with AMD chipsets, plus a bulk of device driver updates, refactorization, and fixes. Please pull. Thanks, Wolfram The following changes since commit cfb92440ee71adcc2105b0890bb01ac3cddb8507: Linux 5.17-rc5 (2022-02-20 13:07:20 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-mergewindow for you to fetch changes up to 1a22aabf20adf89cb216f566913196128766f25b: i2c: mux: demux-pinctrl: do not deactivate a master that is not active (2022-03-20 00:49:43 +0100) ---------------------------------------------------------------- Akhil R (4): device property: Add fwnode_irq_get_byname docs: firmware-guide: ACPI: Add named interrupt doc i2c: smbus: Use device_*() functions instead of of_*() i2c: tegra: Add SMBus block read function Andy Shevchenko (6): i2c: Introduce common module to instantiate CCGx UCSI i2c: nvidia-gpu: Switch to use i2c_new_ccgx_ucsi() i2c: nvidia-gpu: Use temporary variable for struct device i2c: nvidia-gpu: Convert to use dev_err_probe() i2c: designware-pci: Switch to use i2c_new_ccgx_ucsi() i2c: smbus: Check for parent device before dereference AngeloGioacchino Del Regno (1): i2c: mt65xx: Simplify with clk-bulk Christophe JAILLET (2): i2c: amd-mp2: Remove useless DMA-32 fallback configuration i2c: bcm2835: Fix the error handling in 'bcm2835_i2c_probe()' Conor Dooley (1): dt-bindings: i2c: add bindings for microchip mpfs i2c Geert Uytterhoeven (3): dt-bindings: i2c: renesas,rcar-i2c: Add r8a779f0 support i2c: rcar: Add R-Car Gen4 support dt-bindings: i2c: microchip,corei2c: Fix indentation of compatible items Hans de Goede (2): i2c: designware: Lock the adapter while setting the suspended flag i2c: designware: Use the i2c_mark_adapter_suspended/resumed() helpers Jae Hyun Yoo (1): i2c: add tracepoints for I2C slave events Jan Dabros (4): i2c: designware: Add missing locks i2c: designware: Add AMD PSP I2C bus support i2c: designware: Fix improper usage of readl i2c: designware: Remove code duplication Jarkko Nikula (1): i2c: i801: Add support for Intel Raptor Lake PCH-S Jean Delvare (3): i2c: i801: Drop useless masking in i801_access i2c: i801: Add support for the Process Call command i2c: i801: Drop two outdated comments Jonathan Neuschäfer (1): i2c: npcm7xx: Fix typos Kewei Xu (5): dt-bindings: i2c: update bindings for MT8186 SoC i2c: mediatek: Add i2c compatible for Mediatek MT8186 i2c: mediatek: modify bus speed calculation formula dt-bindings: i2c: update bindings for MT8168 SoC i2c: mediatek: Add i2c compatible for Mediatek MT8168 Lad Prabhakar (1): i2c: riic: Simplify reset handling Lucas Tanure (1): i2c: meson: Fix wrong speed use from probe Lukas Bulwahn (1): MAINTAINERS: adjust XLP9XX I2C DRIVER after removing the devicetree binding Martin Povišer (1): i2c: pasemi: Drop I2C classes from platform driver variant Nathan Chancellor (1): i2c: designware: Mark dw_i2c_plat_{suspend,resume}() as __maybe_unused Peter Rosin (1): i2c: mux: demux-pinctrl: do not deactivate a master that is not active Rafael J. Wysocki (1): i2c: ACPI: Replace acpi_bus_get_device() Rafał Miłecki (1): i2c: brcmstb: allow compiling on BCM4908 Robert Hancock (1): i2c: xiic: Make bus names unique Sebastian Andrzej Siewior (3): genirq: Provide generic_handle_irq_safe() i2c: core: Use generic_handle_irq_safe() in i2c_handle_smbus_host_notify(). i2c: cht-wc: Use generic_handle_irq_safe(). 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+ Vinod Koul (1): i2c: qcom-geni: Add support for GPI DMA Vladimir Zapolskiy (2): dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible i2c: qcom-cci: add sm8450 compatible Wolfram Sang (3): Merge branch 'i2c/add-request_mem_region_muxed' into i2c/for-mergewindow i2c: don't expose function which is only used internally Merge tag 'irq-api-2022-02-21' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip into i2c/for-mergewindow Xiang wangx (1): i2c: cros-ec-tunnel: Fix syntax errors in comments Xu Wang (1): i2c: mediatek: remove redundant null check Yang Li (1): i2c: designware: remove unneeded semicolon with much appreciated quality assurance from ---------------------------------------------------------------- Andy Shevchenko (20): (Rev.) i2c: designware: Remove code duplication (Rev.) i2c: designware: Mark dw_i2c_plat_{suspend,resume}() as __maybe_unused (Rev.) i2c: designware: Use the i2c_mark_adapter_suspended/resumed() helpers (Rev.) i2c: designware: Lock the adapter while setting the suspended flag (Rev.) i2c: designware: Fix improper usage of readl (Rev.) i2c: designware: Add AMD PSP I2C bus support (Rev.) i2c: designware: Add missing locks (Rev.) i2c: piix4: Enable EFCH MMIO for Family 17h+ (Rev.) i2c: piix4: Add EFCH MMIO support for SMBus port select (Rev.) i2c: piix4: Add EFCH MMIO support to SMBus base address detect (Rev.) i2c: piix4: Add EFCH MMIO support to region request and release (Rev.) i2c: piix4: Move SMBus port selection into function (Rev.) i2c: piix4: Move SMBus controller base address detect into function (Rev.) i2c: piix4: Move port I/O region request/release code into functions (Rev.) i2c: piix4: Replace hardcoded memory map size with a #define (Rev.) kernel/resource: Introduce request_mem_region_muxed() (Rev.) i2c: ACPI: Replace acpi_bus_get_device() (Rev.) i2c: smbus: Use device_*() functions instead of of_*() (Rev.) docs: firmware-guide: ACPI: Add named interrupt doc (Rev.) device property: Add fwnode_irq_get_byname AngeloGioacchino Del Regno (3): (Rev.) i2c: mediatek: Add i2c compatible for Mediatek MT8168 (Rev.) dt-bindings: i2c: update bindings for MT8168 SoC (Rev.) i2c: mediatek: modify bus speed calculation formula Bjorn Andersson (1): (Rev.) i2c: qcom-geni: Add support for GPI DMA Dmitry Osipenko (1): (Rev.) i2c: tegra: Add SMBus block read function Geert Uytterhoeven (1): (Rev.) i2c: riic: Simplify reset handling Hans de Goede (4): (Rev.) i2c: designware: Mark dw_i2c_plat_{suspend,resume}() as __maybe_unused (Rev.) i2c: cht-wc: Use generic_handle_irq_safe(). (Rev.) genirq: Provide generic_handle_irq_safe() (Rev.) i2c: designware: Add missing locks Jan Dabros (1): (Rev.) i2c: designware: remove unneeded semicolon Jarkko Nikula (5): (Rev.) i2c: i801: Drop two outdated comments (Rev.) i2c: i801: Add support for the Process Call command (Rev.) i2c: i801: Drop useless masking in i801_access (Test) i2c: designware: Add AMD PSP I2C bus support (Test) i2c: designware: Add missing locks Jean Delvare (9): (Rev.) i2c: i801: Add support for Intel Raptor Lake PCH-S (Rev.) i2c: piix4: Enable EFCH MMIO for Family 17h+ (Rev.) i2c: piix4: Add EFCH MMIO support for SMBus port select (Rev.) i2c: piix4: Add EFCH MMIO support to SMBus base address detect (Rev.) i2c: piix4: Add EFCH MMIO support to region request and release (Rev.) i2c: piix4: Move SMBus port selection into function (Rev.) i2c: piix4: Move SMBus controller base address detect into function (Rev.) i2c: piix4: Move port I/O region request/release code into functions (Rev.) i2c: piix4: Replace hardcoded memory map size with a #define Michal Simek (1): (Test) i2c: xiic: Make bus names unique Neil Armstrong (1): (Rev.) i2c: meson: Fix wrong speed use from probe Niklas Söderlund (1): (Rev.) i2c: don't expose function which is only used internally Oleksandr Natalenko (2): (Rev.) i2c: core: Use generic_handle_irq_safe() in i2c_handle_smbus_host_notify(). (Rev.) genirq: Provide generic_handle_irq_safe() Qii Wang (7): (Rev.) i2c: mediatek: Add i2c compatible for Mediatek MT8168 (Rev.) dt-bindings: i2c: update bindings for MT8168 SoC (Rev.) i2c: mt65xx: Simplify with clk-bulk (Rev.) i2c: mediatek: remove redundant null check (Rev.) i2c: mediatek: modify bus speed calculation formula (Rev.) i2c: mediatek: Add i2c compatible for Mediatek MT8186 (Rev.) dt-bindings: i2c: update bindings for MT8186 SoC Randy Dunlap (1): (Rev.) i2c: npcm7xx: Fix typos Rob Herring (1): (Rev.) dt-bindings: i2c: add bindings for microchip mpfs i2c Robert Foss (2): (Rev.) i2c: qcom-cci: add sm8450 compatible (Rev.) dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible Sven Peter (1): (Rev.) i2c: pasemi: Drop I2C classes from platform driver variant Wolfram Sang (3): (Rev.) genirq: Provide generic_handle_irq_safe() (Rev.) i2c: rcar: Add R-Car Gen4 support (Rev.) dt-bindings: i2c: renesas,rcar-i2c: Add r8a779f0 support tali.perry@nuvoton.com (1): (Rev.) i2c: npcm7xx: Fix typos .../devicetree/bindings/i2c/i2c-mt65xx.txt | 2 + .../devicetree/bindings/i2c/i2c-qcom-cci.txt | 4 +- .../devicetree/bindings/i2c/microchip,corei2c.yaml | 56 +++ .../devicetree/bindings/i2c/renesas,rcar-i2c.yaml | 6 + Documentation/firmware-guide/acpi/enumeration.rst | 39 +++ Documentation/i2c/busses/i2c-i801.rst | 1 + MAINTAINERS | 2 +- drivers/acpi/acpi_apd.c | 7 +- drivers/base/property.c | 29 ++ drivers/i2c/busses/Kconfig | 25 +- drivers/i2c/busses/Makefile | 4 + drivers/i2c/busses/i2c-amd-mp2-pci.c | 7 +- drivers/i2c/busses/i2c-bcm2835.c | 21 +- drivers/i2c/busses/i2c-ccgx-ucsi.c | 30 ++ drivers/i2c/busses/i2c-ccgx-ucsi.h | 11 + drivers/i2c/busses/i2c-cht-wc.c | 11 +- drivers/i2c/busses/i2c-cros-ec-tunnel.c | 4 +- drivers/i2c/busses/i2c-designware-amdpsp.c | 388 +++++++++++++++++++++ drivers/i2c/busses/i2c-designware-baytrail.c | 12 +- drivers/i2c/busses/i2c-designware-common.c | 12 + drivers/i2c/busses/i2c-designware-core.h | 20 +- drivers/i2c/busses/i2c-designware-master.c | 11 +- drivers/i2c/busses/i2c-designware-pcidrv.c | 61 ++-- drivers/i2c/busses/i2c-designware-platdrv.c | 88 ++++- drivers/i2c/busses/i2c-i801.c | 24 +- drivers/i2c/busses/i2c-meson.c | 12 +- drivers/i2c/busses/i2c-mt65xx.c | 206 ++++++----- drivers/i2c/busses/i2c-npcm7xx.c | 16 +- drivers/i2c/busses/i2c-nvidia-gpu.c | 62 ++-- drivers/i2c/busses/i2c-pasemi-core.c | 1 - drivers/i2c/busses/i2c-pasemi-pci.c | 1 + drivers/i2c/busses/i2c-piix4.c | 213 ++++++++--- drivers/i2c/busses/i2c-qcom-cci.c | 3 +- drivers/i2c/busses/i2c-qcom-geni.c | 308 ++++++++++++++-- drivers/i2c/busses/i2c-rcar.c | 1 + drivers/i2c/busses/i2c-riic.c | 34 +- drivers/i2c/busses/i2c-tegra.c | 18 +- drivers/i2c/busses/i2c-xiic.c | 3 +- drivers/i2c/i2c-core-acpi.c | 17 +- drivers/i2c/i2c-core-base.c | 4 +- drivers/i2c/i2c-core-slave.c | 15 + drivers/i2c/i2c-core-smbus.c | 14 +- drivers/i2c/i2c-core.h | 9 + drivers/i2c/i2c-smbus.c | 5 +- drivers/i2c/muxes/i2c-demux-pinctrl.c | 5 +- include/linux/i2c-smbus.h | 8 - include/linux/i2c.h | 8 +- include/linux/ioport.h | 2 + include/linux/irqdesc.h | 1 + include/linux/property.h | 1 + include/trace/events/i2c_slave.h | 67 ++++ kernel/irq/irqdesc.c | 23 ++ 52 files changed, 1569 insertions(+), 363 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/microchip,corei2c.yaml create mode 100644 drivers/i2c/busses/i2c-ccgx-ucsi.c create mode 100644 drivers/i2c/busses/i2c-ccgx-ucsi.h create mode 100644 drivers/i2c/busses/i2c-designware-amdpsp.c create mode 100644 include/trace/events/i2c_slave.h