Re: [PATCH] i2c: amd_mp2: handle num is 0 input for i2c_amd_xfer
Acked-by: Elie Morisse Le ven. 4 sept. 2020 à 20:06, a écrit : > > From: Tom Rix > > clang static analyzer reports this problem > > i2c-amd-mp2-plat.c:174:9: warning: Branch condition evaluates > to a garbage value > return err ? err : num; >^~~ > > err is not initialized, it depends on the being set in the > transfer loop which will not happen if num is 0. Surveying > other master_xfer() implementations show all handle a 0 num. > > Because returning 0 is expected, initialize err to 0. > > Signed-off-by: Tom Rix > --- > drivers/i2c/busses/i2c-amd-mp2-plat.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-amd-mp2-plat.c > b/drivers/i2c/busses/i2c-amd-mp2-plat.c > index 17df9e8845b6..506433bc0ff2 100644 > --- a/drivers/i2c/busses/i2c-amd-mp2-plat.c > +++ b/drivers/i2c/busses/i2c-amd-mp2-plat.c > @@ -155,7 +155,7 @@ static int i2c_amd_xfer(struct i2c_adapter *adap, struct > i2c_msg *msgs, int num) > struct amd_i2c_dev *i2c_dev = i2c_get_adapdata(adap); > int i; > struct i2c_msg *pmsg; > - int err; > + int err = 0; > > /* the adapter might have been deleted while waiting for the bus lock > */ > if (unlikely(!i2c_dev->common.mp2_dev)) > -- > 2.18.1 >
[PATCH v17] i2c: Add drivers for the AMD PCIe MP2 I2C controller
MP2 controllers have two separate busses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. Communication with the MP2 takes place through MMIO registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many other improvements. Signed-off-by: Elie Morisse --- Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): -> Add fix for IOMMU -> Add dependency of ACPI -> Add locks to avoid the crash Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers Changes since v9 by Elie M.: -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock -> platform device remove() fixes (protection against the very unlikely case that both the PCI and platform devices get detached manually and simultaneously) -> fix ma
Re: [PATCH v16] i2c: Add drivers for the AMD PCIe MP2 I2C controller
My apologies for the time it took to apply those changes. I will also start working on an alternate (v17?) version with only one module and replacing the platform driver by an ACPI lookup as requested by Bjorn Helgaas, unless Nehal clarifies how AMD plans to expand the platform driver. Elie Morisse Le mar. 26 févr. 2019 à 13:12, Elie Morisse a écrit : > > MP2 controllers have two separate busses, so may accommodate up to two I2C > adapters. Those adapters are listed in the ACPI namespace with the > "AMDI0011" HID, and probed by a platform driver. > > Communication with the MP2 takes place through MMIO registers, or through > DMA for more than 32 bytes transfers. > > This is major rework of the patch submitted by Nehal-bakulchandra Shah from > AMD (https://patchwork.kernel.org/patch/10597369/). > > Most of the event handling of v3 was rewritten to make it work with more > than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version > contains many other improvements. > > Signed-off-by: Elie Morisse > --- > Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): > -> Add fix for IOMMU > -> Add dependency of ACPI > -> Add locks to avoid the crash > > Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): > > -> fix for review comments > -> fix for more than 32 bytes write issue > > Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: > > -> support more than one bus/adapter > -> support more than one slave per bus > -> use the bus speed specified by the slaves declared in the DSDT instead of >assuming speed == 400kbits/s > -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply >use i2c_msg.buf > -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a >multiple of 4 by using memcpy_fromio and memcpy_toio > -> use streaming DMA mappings instead of allocating a coherent DMA buffer for >every >32 bytes read/write > -> properly check for timeouts during i2c_amd_xfer and increase it from 50 >jiffies to 250 msecs (which is more in line with other drivers) > -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success >event, instead of stalling i2c_amd_xfer > -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see >the point since it's already waiting for a i2c_busenable_complete event > -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want >parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) > -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which >are shared across the two busses/adapters > -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT >enumerates devices with the "AMDI0011" HID > -> set maximum length of reads/writes to 4095 (event's length field is 12 > bits) > -> basic PM support > -> style corrections to match the kernel code style, and tried to reduce code >duplication whenever possible > > Changes since v4 by Elie M.: > > -> fix missing typecast warning > -> removed the duplicated entry in Kconfig > > Changes since v5 by Elie M.: > > -> move DMA mapping from the platform driver to the PCI driver > -> attempt to find the platform device's PCI parent through the _DEP ACPI > method >(if not found take the first MP2 device registred in the i2c-amd-pci-mp2 >driver, like before) > -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 >driver > -> address other review comments by Bjorn Helgaas (meant for v3) > > Changes since v6 by Elie M.: > > -> remove unnecessary memcpy from the DMA buffer during > i2c_amd_read_completion > > Changes since v7 by Elie M.: > > -> merge the two modules into one named i2c-amd-mp2, delete now useless > exports > -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the >MP2 controller if a slave doesn't respond to a read or write request > -> unmap the DMA buffer before read/write_complete > -> fix bus_disable commands handling (no more errors during module exit) > -> prefer managed versions of pci_ and dev_ functions whenever possible > -> increase adapter retries from 0 to 3 > -> reduce code duplication in debugfs functions > -> address other review points by Bjorn Helgaas (except regarding the _DEP >hint) > > Changes since v8 by Elie M.: > > -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever > possible > -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf &g
[PATCH v16] i2c: Add drivers for the AMD PCIe MP2 I2C controller
MP2 controllers have two separate busses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. Communication with the MP2 takes place through MMIO registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many other improvements. Signed-off-by: Elie Morisse --- Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): -> Add fix for IOMMU -> Add dependency of ACPI -> Add locks to avoid the crash Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers Changes since v9 by Elie M.: -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock -> platform device remove() fixes (protection against the very unlikely case that both the PCI and platform devices get detached manually and simultaneously) -> fix ma
Re: [PATCH v15] i2c: Add drivers for the AMD PCIe MP2 I2C controller
Hi, Nehal isn't responding, while the end of the merge window is closing in. I addressed his last requests, will this get merged? Elie Le mer. 26 déc. 2018 à 20:23, Elie Morisse a écrit : > > MP2 controllers have two separate busses, so may accommodate up to two I2C > adapters. Those adapters are listed in the ACPI namespace with the > "AMDI0011" HID, and probed by a platform driver. > > Communication with the MP2 takes place through MMIO registers, or through > DMA for more than 32 bytes transfers. > > This is major rework of the patch submitted by Nehal-bakulchandra Shah from > AMD (https://patchwork.kernel.org/patch/10597369/). > > Most of the event handling of v3 was rewritten to make it work with more > than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version > contains many other improvements. > > Signed-off-by: Elie Morisse > --- > Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): > -> Add fix for IOMMU > -> Add dependency of ACPI > -> Add locks to avoid the crash > > Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): > > -> fix for review comments > -> fix for more than 32 bytes write issue > > Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: > > -> support more than one bus/adapter > -> support more than one slave per bus > -> use the bus speed specified by the slaves declared in the DSDT instead of >assuming speed == 400kbits/s > -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply >use i2c_msg.buf > -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a >multiple of 4 by using memcpy_fromio and memcpy_toio > -> use streaming DMA mappings instead of allocating a coherent DMA buffer for >every >32 bytes read/write > -> properly check for timeouts during i2c_amd_xfer and increase it from 50 >jiffies to 250 msecs (which is more in line with other drivers) > -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success >event, instead of stalling i2c_amd_xfer > -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see >the point since it's already waiting for a i2c_busenable_complete event > -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want >parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) > -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which >are shared across the two busses/adapters > -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT >enumerates devices with the "AMDI0011" HID > -> set maximum length of reads/writes to 4095 (event's length field is 12 > bits) > -> basic PM support > -> style corrections to match the kernel code style, and tried to reduce code >duplication whenever possible > > Changes since v4 by Elie M.: > > -> fix missing typecast warning > -> removed the duplicated entry in Kconfig > > Changes since v5 by Elie M.: > > -> move DMA mapping from the platform driver to the PCI driver > -> attempt to find the platform device's PCI parent through the _DEP ACPI > method >(if not found take the first MP2 device registred in the i2c-amd-pci-mp2 >driver, like before) > -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 >driver > -> address other review comments by Bjorn Helgaas (meant for v3) > > Changes since v6 by Elie M.: > > -> remove unnecessary memcpy from the DMA buffer during > i2c_amd_read_completion > > Changes since v7 by Elie M.: > > -> merge the two modules into one named i2c-amd-mp2, delete now useless > exports > -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the >MP2 controller if a slave doesn't respond to a read or write request > -> unmap the DMA buffer before read/write_complete > -> fix bus_disable commands handling (no more errors during module exit) > -> prefer managed versions of pci_ and dev_ functions whenever possible > -> increase adapter retries from 0 to 3 > -> reduce code duplication in debugfs functions > -> address other review points by Bjorn Helgaas (except regarding the _DEP >hint) > > Changes since v8 by Elie M.: > > -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever > possible > -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf > -> defer probe() by the platform driver if no MP2 device has been probed > yet by the PCI driver (which should address Bjorn Helgaas&#x
[PATCH v15] i2c: Add drivers for the AMD PCIe MP2 I2C controller
MP2 controllers have two separate busses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. Communication with the MP2 takes place through MMIO registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many other improvements. Signed-off-by: Elie Morisse --- Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): -> Add fix for IOMMU -> Add dependency of ACPI -> Add locks to avoid the crash Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers Changes since v9 by Elie M.: -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock -> platform device remove() fixes (protection against the very unlikely case that both the PCI and platform devices get detached manually and simultaneously) -> fix ma
[PATCH v14] i2c: Add drivers for the AMD PCIe MP2 I2C controller
MP2 controllers have two separate busses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. Communication with the MP2 takes place through MMIO registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many other improvements. Signed-off-by: Elie Morisse --- Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): -> Add fix for IOMMU -> Add dependency of ACPI -> Add locks to avoid the crash Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers Changes since v9 by Elie M.: -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock -> platform device remove() fixes (protection against the very unlikely case that both the PCI and platform devices get detached manually and simultaneously) -> fix ma
Re: [PATCH v13] i2c: Add drivers for the AMD PCIe MP2 I2C controller
Hi, Having the patch reviewed by Nehal Shah would be great. While waiting to hear from him, he voiced some concerns in the v10 thread: https://lkml.org/lkml/2018/11/27/62 The _DEP hint was removed in v12. Regarding AMD's intention of having two drivers, there are still two drivers, just within the same module. And the comment about where the DMA mapping code should be is minor and it's trivial to move it back if AMD plans to expand the platform driver. Noone reported any issue since v11, I'm done improving the drivers unless it's necessary in order to get it accepted upstream. Please let me know if I still have to change something to get it merged. Elie Morisse Le mar. 11 déc. 2018 à 18:02, Wolfram Sang a écrit : > > On Sun, Dec 09, 2018 at 12:56:02PM -0300, Elie Morisse wrote: > > MP2 controllers have two separate busses, so may accommodate up to two I2C > > adapters. Those adapters are listed in the ACPI namespace with the > > "AMDI0011" HID, and probed by a platform driver. > > > > Communication with the MP2 takes place through iomapped registers, or > > through DMA for more than 32 bytes transfers. > > > > This is major rework of the patch submitted by Nehal-bakulchandra Shah from > > AMD (https://patchwork.kernel.org/patch/10597369/). > > > > Most of the event handling of v3 was rewritten to make it work with more > > than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version > > contains many other improvements. > > > > Signed-off-by: Elie Morisse > > Nehal Shah, are you happy with this driver from your side? >
[PATCH v13] i2c: Add drivers for the AMD PCIe MP2 I2C controller
MP2 controllers have two separate busses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. Communication with the MP2 takes place through iomapped registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many other improvements. Signed-off-by: Elie Morisse --- Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): -> Add fix for IOMMU -> Add dependency of ACPI -> Add locks to avoid the crash Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers Changes since v9 by Elie M.: -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock -> platform device remove() fixes (protection against the very unlikely case that both the PCI and platform devices get detached manually and simultaneously) -> fix ma
[PATCH v12] i2c: Add drivers for the AMD PCIe MP2 I2C controller
MP2 controllers have two separate busses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. Communication with the MP2 takes place through iomapped registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many other improvements. Signed-off-by: Elie Morisse --- Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers Changes since v9 by Elie M.: -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock -> platform device remove() fixes (protection against the very unlikely case that both the PCI and platform devices get detached manually and simultaneously) -> fix ma
[PATCH v11] i2c: Add drivers for the AMD PCIe MP2 I2C controller
MP2 controllers have two separate busses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. Communication with the MP2 takes place through iomapped registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many more improvements. Signed-off-by: Elie Morisse --- Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds (there seems to be an interrupt issue with older Zen microcodes, upgrade your amd microcode package if you experience timeouts) -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers Changes since v9 by Elie M.: -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock -> platform device remove() fixes (protection
Re: [PATCH v10] i2c: Add drivers for the AMD PCIe MP2 I2C controller
Hi Nehal-bakulchandra,
> There was intention behind to make two separate driver for pci and I2c. It
> has future usecase in our platforms and hence we made modular designed. So
> better to make it two separate driver.
I merged the two modules into one because of Bjorn Helgaas' concern
that the platform driver only exists to extract information from the
ACPI namespace.
And that didn't really address it tbh.
Currently the minor advantages of having two drivers is that each
bus/adapter on the MP2 has its entry in sysfs and can be individually
(un-)bound, and less code duplication in the driver. Now if AMD plans
to expand the platform driver, that would probably make more sense to
keep two drivers and two modules.
If the Linux maintainers consent to it I can re-split the module into two.
> Another thing i would like to understand why _DEP method based hint is
> required, i didn't find need of it.
In case there is more than one MP2 PCI device present on the same
system, how do you determine which PCI device is an AMDI0011 ACPI
device related to?
I pasted the Yoga 530's DSDT here: https://paste.kde.org/pjobniftq
The only available hints are the _DEP, method, and also perhaps the UID or _ADR:
Device (I2CB)
{
Name (_HID, "AMDI0011") // _HID: Hardware ID
Name (_UID, One) // _UID: Unique ID
Name (_ADR, One) // _ADR: Address
//...
Name (_DEP, Package (0x01) // _DEP: Dependencies
{
^PCI0.GP17.MP2C
})
If _DEP shouldn't be used, could _UID (or _ADR?) be assumed to refer
to the MP2 device, in the order in which the PCI devices are
enumerated? i.e
UID
0 -> first MP2 bus 0
1 -> first MP2 bus 1
2 -> second MP2 bus 0
3 -> second MP2 bus 1
etc.
There's currently no such computer around with more than one MP2 PCI
device at the moment, but does the documentation say something about
this?
> Indeed PCI device owned by i2c-mp2 device so what is the need of moving DMA
> mapping to PCI driver?
Not sure if I understand correctly, the DMA buffer has to be owned by
the PCI device, not by the platform device/I2C adapter :
i2c_common->dma_addr = dma_map_single(&privdata->pci_dev->dev, ...);
That's why it seemed more natural to move the mapping into the PCI
driver, and this was also the last occurrence of the platform driver
accessing the pci_dev pointer. However if you plan to introduce
non-PCI I2C controllers with the same split between small messages
transferred through registers and large ones going through DMA then
yes it'd make more sense to put it back in the platform driver. But is
that really an issue for now?
Btw I'll submit a v11 version of the patch which definitely fixes the
timeouts on Lenovo Ideapad 530s soon (already fixed in
https://github.com/Syniurge/i2c-amd-mp2).
Elie
Le lun. 26 nov. 2018 à 15:52, Shah, Nehal-bakulchandra
a écrit :
>
>
> Hi Elie Morisse,
>
> On 11/14/2018 10:00 PM, Elie Morisse wrote:
> > I2C communication takes place through iomapped registers, or through DMA
> > for more than 32 bytes transfers.
> >
> > MP2 controllers have two separate buses, so may accommodate up to two I2C
> > adapters. Those adapters are listed in the ACPI namespace with the
> > "AMDI0011" HID, and probed by a platform driver.
> >
> > This is major rework of the patch submitted by Nehal-bakulchandra Shah from
> > AMD (https://patchwork.kernel.org/patch/10597369/).
> >
> > Most of the event handling of v3 was rewritten to make it work with more
> > than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version
> > contains many more improvements.
> >
> > Signed-off-by: Elie Morisse
> > ---
> > Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html):
> > -> Add fix for IOMMU
> > -> Add depedency of ACPI
> > -> Add locks to avoid the crash
> >
> > Changes since v2 (https://patchwork.ozlabs.org/patch/961270/):
> >
> > -> fix for review comments
> > -> fix for more than 32 bytes write issue
> >
> > Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.:
> >
> > -> support more than one bus/adapter
> > -> support more than one slave per bus
> > -> use the bus speed specified by the slaves declared in the DSDT instead of
> >assuming speed == 400kbits/s
> > -> instead of kzalloc'ing a buffer for every less than 32 bytes reads,
> > simply
> >use i2c_msg.buf
> > -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a
> >multiple of 4 by using memcpy_fromio and memcpy_toio
> > -> use streaming DMA mappings instead o
[PATCH v10] i2c: Add drivers for the AMD PCIe MP2 I2C controller
I2C communication takes place through iomapped registers, or through DMA for more than 32 bytes transfers. MP2 controllers have two separate buses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many more improvements. Signed-off-by: Elie Morisse --- Changes since v1 (https://www.spinics.net/lists/linux-i2c/msg34650.html): -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2 (https://patchwork.ozlabs.org/patch/961270/): -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds (there seems to be an interrupt issue with older Zen microcodes, upgrade your amd microcode package if you experience timeouts) -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers Changes since v9 by Elie M.: -> remove the xfer_lock mutex which was redundant with i2c_adapter.bus_lock -> platform device remove() fixes (protection against the v
Re: [PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
Hi, In v9 the platform driver defers probe() if no MP2 devices has been registered in the PCI driver yet, is this an acceptable fix? I've found examples of kernel code extracting info from the ACPI namespace (e.g in i2c-core-acpi.c), but this would require adding more code to the driver and AMDI0011 ACPI devices getting associated to a platform device still seem like a minor but nice bonus to me, but that may be the wrong way of looking at it. So, to get this patch accepted do I keep things this way or do I replace the platform driver by an ACPI namespace walker? Le mar. 30 oct. 2018 à 17:56, Bjorn Helgaas a écrit : > > [+cc Rafael, Len, linux-acpi] > > On Sat, Oct 27, 2018 at 12:09:10PM -0300, Elie Morisse wrote: > > This contains two drivers: > > * i2c-amd-plat-mp2: platform driver managing an i2c adapter (one of > > the two busses of the MP2) and routing any i2c read/write command to > > the PCI driver. > > * i2c-amd-pci-mp2: PCI driver communicating through the C2P/P2C > > mailbox registers, or through DMA for more than 32 bytes transfers. > > I'm dubious about this two-driver structure. If I understand > correctly (and it's very possible that I don't), the PCI driver > (amd_mp2_pci_probe()) is the real owner of the i2c adapter: it > claims the PCI device, claims its BARs, and requests an IRQ. > > The i2c_amd_probe() code *looks* like a platform driver that claims > AMDI0011 devices from the ACPI namespace, but I don't think it's > really a driver. It looks like it exists mainly to extract some > information (bus speed and maybe a bus number?) from the namespace, > then to call i2c_add_adapter(). > > It looks like i2c_amd_probe() must run *after* amd_mp2_pci_probe(), > but there's no way to really enforce that ordering. > > And i2c-amd-plat-mp2 contains the i2c_amd_algorithm functions, which > operate on the PCI device, which requires exported interfaces > (amd_mp2_read(), amd_mp2_write()) that are implemented in the PCI > driver but called from the platform part. > > It seems like there should be a way to put the ACPI lookups into > i2c-amd-pci-mp2 so there's only one driver. > > I only have a couple trivial comments below but I'm not trimming my > response so the ACPI folks can see the whole context. > > > This is major rework of the patch submitted by Nehal-bakulchandra Shah > > from AMD (https://patchwork.kernel.org/patch/10597369/). > > > > Most of the event handling of v2/v3 was rewritten since it couldn't work > > if more than one bus was enabled, and contains many more fixes listed > > in the patch changelog. > > > > With those changes both the touchpad and the touchscreen of the > > Ryzen-based Lenovo Yoga 530 which lie in separate busses work beautifully. > > > > Signed-off-by: Elie Morisse > > --- > > Changes since v1:(https://www.spinics.net/lists/linux-i2c/msg34650.html) > > -> Add fix for IOMMU > > -> Add depedency of ACPI > > -> Add locks to avoid the crash > > > > Changes since v2:(https://patchwork.ozlabs.org/patch/961270/) > > > > -> fix for review comments > > -> fix for more than 32 bytes write issue > > > > Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: > > > > -> support more than one bus/adapter > > -> support more than one slave per bus > > -> use the bus speed specified by the slaves declared in the DSDT instead of > >assuming speed == 400kbits/s > > -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, > > simply > >use i2c_msg.buf > > -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a > >multiple of 4 by using memcpy_fromio and memcpy_toio > > -> use streaming DMA mappings instead of allocating a coherent DMA buffer > > for > >every >32 bytes read/write > > -> properly check for timeouts during i2c_amd_xfer and increase it from 50 > >jiffies to 250 msecs (which is more in line with other drivers) > > -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success > >event, instead of stalling i2c_amd_xfer > > -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't > > see > >the point since it's already waiting for a i2c_busenable_complete event > > -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want > >parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) > > -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, > > which > >are shared acros
[PATCH v9] i2c: Add drivers for the AMD PCIe MP2 I2C controller
I2C communication takes place through iomapped registers, or through DMA for more than 32 bytes transfers. MP2 controllers have two separate buses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID, and probed by a platform driver. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this version contains many more improvements. Signed-off-by: Elie Morisse --- Changes since v1:(https://www.spinics.net/lists/linux-i2c/msg34650.html) -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2:(https://patchwork.ozlabs.org/patch/961270/) -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) Changes since v8 by Elie M.: -> remove mostly redundant amd_i2c_rw_config, refer to i2c_msg whenever possible -> use i2c_get_dma_safe_msg_buf and i2c_put_dma_safe_msg_buf -> defer probe() by the platform driver if no MP2 device has been probed yet by the PCI driver (which should address Bjorn Helgaas' concern while preserving the platform driver) -> if the interrupt following a command doesn't get handled after 250ms, zero AMD_P2C_MSG_INTEN to prevent the MP2 from stalling for a few more seconds (there seems to be an interrupt issue with older Zen microcodes, upgrade your amd microcode package if you experience timeouts) -> include AMD_P2C_MSG3 and fix typo in debugfs output -> cosmetic fixes, code reduction, and better comments -> add Nehal Shah and Shyam Sundar S K from AMD to the list of maintainers MAINTAINERS | 8 + drivers/i2c/busses/Kconfig| 10 + drivers/i2c/busses/Makefile | 2 + drivers/i2c/busses/i2c-amd
Re: [PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
The errors on module exit and the hang after a read/write timeout should be fixed in v8, someone else reported the same issue here: https://github.com/Syniurge/i2c-amd-mp2/issues/1 Weird that despite a lot of trying by doing random stuff with my touchpad and touchscreen neither never ever timed out on my Yoga 530. Le mar. 30 oct. 2018 à 18:41, Tobias Thomer a écrit : > > I've tested this patch on a Yoga 530 and it seems to work (sometimes for > hours) but it randomly dies with a i2c read timeout > > [ 9562.237020] i2c_amd_plat_mp2 AMDI0011:00: i2c read timed out > > and reloading/removing of the module results in this: > > [ 9703.591021] pcie_mp2_amd :03:00.7: length 0 in event doesn't match > buffer length 64! > [ 9703.591030] pcie_mp2_amd :03:00.7: unexpected slave address 0 > (expected: 2c)! > [ 9703.863740] i2c_amd_plat_mp2 AMDI0011:01: i2c connection timed out > [ 9707.596826] INFO: task irq/67-WCOM517E:340 blocked for more than 120 > seconds. > [ 9707.596838] Tainted: G OE 4.19.0-v7 #1 > [ 9707.596841] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables > this message. > [ 9707.596845] irq/67-WCOM517E D0 340 2 0x8080 > [ 9707.596851] Call Trace: > [ 9707.596868] ? __schedule+0x29b/0x8b0 > [ 9707.596873] schedule+0x32/0x90 > [ 9707.596875] schedule_preempt_disabled+0x14/0x20 > [ 9707.596879] __mutex_lock.isra.0+0x217/0x520 > [ 9707.596884] ? __switch_to_asm+0x40/0x70 > [ 9707.596887] ? __switch_to_asm+0x34/0x70 > [ 9707.596890] ? __switch_to_asm+0x34/0x70 > [ 9707.596898] amd_mp2_read+0x4f/0x17b [i2c_amd_pci_mp2] > [ 9707.596912] i2c_amd_xfer+0x61/0x150 [i2c_amd_plat_mp2] > [ 9707.596919] __i2c_transfer+0x142/0x480 > [ 9707.596922] ? _raw_spin_unlock_irq+0x1d/0x30 > [ 9707.596926] ? irq_forced_thread_fn+0x70/0x70 > [ 9707.596929] i2c_transfer+0x51/0xc0 > [ 9707.596932] i2c_transfer_buffer_flags+0x4c/0x70 > [ 9707.596937] i2c_hid_irq+0x3c/0x130 [i2c_hid] > [ 9707.596941] irq_thread_fn+0x1f/0x50 > [ 9707.596944] irq_thread+0xf7/0x1a0 > [ 9707.596946] ? irq_thread_check_affinity.part.3+0xa0/0xa0 > [ 9707.596949] ? irq_thread_dtor+0xb0/0xb0 > [ 9707.596953] kthread+0x112/0x130 > [ 9707.596956] ? kthread_park+0x80/0x80 > [ 9707.596959] ret_from_fork+0x22/0x40
[PATCH v8] i2c: Add driver for the AMD PCIe MP2 I2C controller
I2C communication takes place through the iomapped C2P/P2C mailbox registers, or through DMA for more than 32 bytes transfers. MP2 controllers have two separate buses, so may accommodate up to two I2C adapters. Those adapters are listed in the ACPI namespace with the "AMDI0011" HID. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v3 was rewritten to make it work with more than one bus (e.g on Ryzen-based Lenovo Yoga 530), and this contains many more improvements. Signed-off-by: Elie Morisse --- Changes since v1:(https://www.spinics.net/lists/linux-i2c/msg34650.html) -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2:(https://patchwork.ozlabs.org/patch/961270/) -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 (https://marc.info/?l=linux-kernel&m=154031133019835) by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion Changes since v7 by Elie M.: -> merge the two modules into one named i2c-amd-mp2, delete now useless exports -> unlock the C2P mutex if i2c_amd_xfer_msg timeouts, to prevent stalling the MP2 controller if a slave doesn't respond to a read or write request -> unmap the DMA buffer before read/write_complete -> fix bus_disable commands handling (no more errors during module exit) -> prefer managed versions of pci_ and dev_ functions whenever possible -> increase adapter retries from 0 to 3 -> reduce code duplication in debugfs functions -> address other review points by Bjorn Helgaas (except regarding the _DEP hint) MAINTAINERS | 6 + drivers/i2c/busses/Kconfig| 10 + drivers/i2c/busses/Makefile | 2 + drivers/i2c/busses/i2c-amd-mp2-pci.c | 672 ++ drivers/i2c/busses/i2c-amd-mp2-plat.c | 371 ++ drivers/i2c/busses/i2c-amd-mp2.h | 232 + 6 files changed, 1293 insertions(+) create mode 100644 drivers/i2c/busses/i2c-amd-mp2-pci.c create mode 100644 drivers/i2c/busses/i2c-amd-mp2-plat.c create mode 100644 drivers/i2c/busses/i2c-amd-mp2.h diff --git a/MAINTAINERS b/MAINTAINERS index 6ac000cc006d..8ff2bddc1ac2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -791,6 +791,12 @@ F: drivers/gpu/drm/amd/include/vi_structs.h F: drivers/gpu/drm/amd/include/v9_structs.h F: include/uapi/linux/kfd_ioctl.h +AMD MP2 I2C DRIVER +M: Elie Morisse +L: linux-...@vger.kernel.org +S: Maintained +F: drivers/i2c/busses/i2c-amd-*-mp2* + AMD POWERPLAY M: Rex Zhu M:
[PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
This contains two drivers: * i2c-amd-plat-mp2: platform driver managing an i2c adapter (one of the two busses of the MP2) and routing any i2c read/write command to the PCI driver. * i2c-amd-pci-mp2: PCI driver communicating through the C2P/P2C mailbox registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v2/v3 was rewritten since it couldn't work if more than one bus was enabled, and contains many more fixes listed in the patch changelog. With those changes both the touchpad and the touchscreen of the Ryzen-based Lenovo Yoga 530 which lie in separate busses work beautifully. Signed-off-by: Elie Morisse --- Changes since v1:(https://www.spinics.net/lists/linux-i2c/msg34650.html) -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2:(https://patchwork.ozlabs.org/patch/961270/) -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 (https://marc.info/?l=linux-kernel&m=154031133019835) by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP ACPI method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) Changes since v6 by Elie M.: -> remove unnecessary memcpy from the DMA buffer during i2c_amd_read_completion MAINTAINERS | 6 + drivers/i2c/busses/Kconfig| 15 + drivers/i2c/busses/Makefile | 2 + drivers/i2c/busses/i2c-amd-pci-mp2.c | 706 ++ drivers/i2c/busses/i2c-amd-pci-mp2.h | 224 drivers/i2c/busses/i2c-amd-plat-mp2.c | 373 ++ 6 files changed, 1326 insertions(+) create mode 100644 drivers/i2c/busses/i2c-amd-pci-mp2.c create mode 100644 drivers/i2c/busses/i2c-amd-pci-mp2.h create mode 100644 drivers/i2c/busses/i2c-amd-plat-mp2.c diff --git a/MAINTAINERS b/MAINTAINERS index 6ac000cc006d..8ff2bddc1ac2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -791,6 +791,12 @@ F: drivers/gpu/drm/amd/include/vi_structs.h F: drivers/gpu/drm/amd/include/v9_structs.h F: include/uapi/linux/kfd_ioctl.h +AMD MP2 I2C DRIVER +M: Elie Morisse +L: linux-...@vger.kernel.org +S: Maintained +F: drivers/i2c/busses/i2c-amd-*-mp2* + AMD POWERPLAY M: Rex Zhu M: Evan Quan diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 451d4ae50e66..e20f2d1ce381 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -77,6 +77,21 @@ config I2C_AMD8111 This driver can also be built as a module. If so, the module will be called i2c-amd8111. +config I2C_AMD_MP2_PCI + tristate + depends on PCI + +config I2C_AMD_MP2_PLATFORM + tristate "AMD MP2 Platform" + sele
[PATCH v6] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
This contains two drivers: * i2c-amd-plat-mp2: platform driver managing an i2c adapter (one of the two busses of the MP2) and routing any i2c read/write command to the PCI driver. * i2c-amd-pci-mp2: PCI driver communicating through the C2P/P2C mailbox registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v2/v3 was rewritten since it couldn't work if more than one bus was enabled, and contains many more fixes listed in the patch changelog. With those changes both the touchpad and the touchscreen of the Ryzen-based Lenovo Yoga 530 which lie in separate busses work beautifully. Signed-off-by: Elie Morisse --- Changes since v1:(https://www.spinics.net/lists/linux-i2c/msg34650.html) -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2:(https://patchwork.ozlabs.org/patch/961270/) -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 (https://marc.info/?l=linux-kernel&m=154031133019835) by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig Changes since v5 by Elie M.: -> move DMA mapping from the platform driver to the PCI driver -> attempt to find the platform device's PCI parent through the _DEP method (if not found take the first MP2 device registred in the i2c-amd-pci-mp2 driver, like before) -> do not assume anymore that the PCI device is owned by the i2c-amd-pci-mp2 driver -> address other review comments by Bjorn Helgaas (meant for v3) MAINTAINERS | 6 + drivers/i2c/busses/Kconfig| 15 + drivers/i2c/busses/Makefile | 2 + drivers/i2c/busses/i2c-amd-pci-mp2.c | 705 ++ drivers/i2c/busses/i2c-amd-pci-mp2.h | 224 drivers/i2c/busses/i2c-amd-plat-mp2.c | 384 ++ 6 files changed, 1336 insertions(+) create mode 100644 drivers/i2c/busses/i2c-amd-pci-mp2.c create mode 100644 drivers/i2c/busses/i2c-amd-pci-mp2.h create mode 100644 drivers/i2c/busses/i2c-amd-plat-mp2.c diff --git a/MAINTAINERS b/MAINTAINERS index 6ac000cc006d..8ff2bddc1ac2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -791,6 +791,12 @@ F: drivers/gpu/drm/amd/include/vi_structs.h F: drivers/gpu/drm/amd/include/v9_structs.h F: include/uapi/linux/kfd_ioctl.h +AMD MP2 I2C DRIVER +M: Elie Morisse +L: linux-...@vger.kernel.org +S: Maintained +F: drivers/i2c/busses/i2c-amd-*-mp2* + AMD POWERPLAY M: Rex Zhu M: Evan Quan diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 451d4ae50e66..e20f2d1ce381 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -77,6 +77,21 @@ config I2C_AMD8111 This driver can also be built as a module. If so, the module will be called i2c-amd8111. +config I2C_AMD_MP2_PCI + tristate + depends on PCI + +config I2C_AMD_MP2_PLATFORM + tristate "AMD MP2 Platform" + select I2C_AMD_MP2_PCI + depends on ACPI + help + If you say yes to this option, support will be included fo
Re: [PATCH v3] i2c:amd I2C Driver based on PCI Interface for upcoming, platform
Hi,
I did some work to improve Shah's driver and make it work on laptops
where the two buses of the MP2 are enabled (e.g Lenovo Yoga
530-14ARR):
https://marc.info/?l=linux-i2c&m=154039677815809
v3 had some issues preventing both buses to work:
- enabling the second adapter (bus 1) was overriding amd_mp2_dev.ops
so events intended for the first adapter (bus 0) would be handled by
the second adapter
- in the interrupt handler checking whether the content of
AMD_P2C_MSG1 or AMD_P2C_MSG2 was != zero to determine which bus the
IRQ was for was incorrect since they were not getting zero'd
Tomorrow I'll submit an updated patch that takes Bjorn's remarks that
still apply to my version into account.
Elie Morisse
Le mer. 24 oct. 2018 à 16:16, Bjorn Helgaas a écrit :
>
> On Thu, Oct 25, 2018 at 01:26:51AM +0800, Kai Heng Feng wrote:
> > > On Sep 17, 2018, at 16:19, Kai-Heng Feng
> > > wrote:
> > > at 18:54, Shah, Nehal-bakulchandra
> > > wrote:
> > >
> > >> From: Nehal-bakulchandra Shah
> > >>
> > >> This contains two drivers.
> > >> 1)i2c-amd-platdrv: This is based on I2C framework of
> > >> linux kernel. So any i2c read write call or commands
> > >> to this driver is routed to PCI Interface driver.
> > >> 2) i2c-amd-platdrv: This driver is responsible to
> > >> talk with Mp2 and does all C2P/P2C communication
> > >> or reading/writing from DRAM in case of more
> > >> data.
> > >>
> > >> Reviewed-by: S-k, Shyam-sundar
> > >> Reviewed-by: Sandeep Singh
> > >> Signed-off-by: Nehal-bakulchandra Shah
> > >
> > > The I2C touchpad on Latitude 5495 works with this patch.
> >
> > From PCI’s point of view, do you think this driver is good?
>
> Speaking for myself, I usually only review things that *change* the
> PCI core, not things like drivers that simply *use* the PCI core
> services.
>
> But since you made the mistake of asking, I do have a few comments
> below :)
>
> > >> --- /dev/null
> > >> +++ b/drivers/i2c/busses/i2c-amd-pci-mp2.c
> > >> @@ -0,0 +1,580 @@
> > >> +// SPDX-License-Identifier: GPL-2.0
> > >> +/*
> > >> + * Copyright (C) 2018 Advanced Micro Devices, Inc. All Rights Reserved.
> > >> + *
> > >> + *
>
> Extra blank line above.
>
> > >> + * Author: Shyam Sundar S K
> > >> + * AMD PCIe MP2 Communication Driver
> > >> + */
> > >> +
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +
> > >> +#include "i2c-amd-pci-mp2.h"
> > >> +
> > >> +#define DRIVER_NAME "pcie_mp2_amd"
> > >> +#define DRIVER_DESC "AMD(R) PCI-E MP2 Communication Driver"
>
> "PCIe" is the usual spelling.
>
> > >> +#define DRIVER_VER"1.0"
> > >> +
> > >> +MODULE_DESCRIPTION(DRIVER_DESC);
> > >> +MODULE_VERSION(DRIVER_VER);
> > >> +MODULE_LICENSE("Dual BSD/GPL");
>
> This doesn't match the SPDX tag above.
> See
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/license-rules.rst
>
> > >> +MODULE_AUTHOR("Shyam Sundar S K ");
> > >> +
> > >> +static const struct file_operations amd_mp2_debugfs_info;
> > >> +static struct dentry *debugfs_dir;
> > >> +
> > >> +int amd_mp2_connect(struct pci_dev *dev, struct i2c_config i2c_cfg)
> > >> +{
> > >> + struct amd_mp2_dev *privdata = pci_get_drvdata(dev);
> > >> + union i2c_cmd_base i2c_cmd_base;
> > >> + unsigned long flags;
>
> Extra spaces in the "flags" declaration.
>
> > >> +
> > >> + raw_spin_lock_irqsave(&privdata->lock, flags);
> > >> + dev_dbg(ndev_dev(privdata), "%s addr: %x id: %d\n", __func__,
> > >> + i2c_cfg.dev_addr, i2c_cfg.bus_id);
> > >> +
> > >> + i2c_cmd_base.ul = 0;
> > >> + i2c_cmd_base.s.i2c_cmd = i2c_enable;
> > >> + i2c_cmd_base.s.bus_id = i2c_cfg.bus_id;
> > >> + i2c_cmd_base.s.i2c_speed = i2c_cfg.i2c_speed;
> > >> +
> > >> + if (i2c_cmd_base.s.bus_id == i2c_bus_1) {
> > >> + writel(i2c_cmd_base.ul, privdata->mmio + AMD_C2P_MSG1);
> > >> + } else if (i2c_cmd_base.s.bus_id == i2c_bus_0) {
> > >&
[v5] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
This contains two drivers: * i2c-amd-plat-mp2: platform driver managing an i2c adapter (one of the two busses of the MP2) and routing any i2c read/write command to the PCI driver. * i2c-amd-pci-mp2: PCI driver communicating through the C2P/P2C mailbox registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v2/v3 was rewritten since it couldn't work if more than one bus was enabled, and contains many more fixes listed in the patch changelog. With those changes both the touchpad and the touchscreen of the Ryzen-based Lenovo Yoga 530 which lie in separate busses work beautifully. Signed-off-by: Elie Morisse --- Changes since v1:(https://www.spinics.net/lists/linux-i2c/msg34650.html) -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2:(https://patchwork.ozlabs.org/patch/961270/) -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible Changes since v4 (https://marc.info/?l=linux-kernel&m=154031133019835) by Elie M.: -> fix missing typecast warning -> removed the duplicated entry in Kconfig MAINTAINERS | 6 + drivers/i2c/busses/Kconfig| 15 + drivers/i2c/busses/Makefile | 2 + drivers/i2c/busses/i2c-amd-pci-mp2.c | 633 ++ drivers/i2c/busses/i2c-amd-pci-mp2.h | 219 + drivers/i2c/busses/i2c-amd-plat-mp2.c | 362 +++ 6 files changed, 1237 insertions(+) create mode 100644 drivers/i2c/busses/i2c-amd-pci-mp2.c create mode 100644 drivers/i2c/busses/i2c-amd-pci-mp2.h create mode 100644 drivers/i2c/busses/i2c-amd-plat-mp2.c diff --git a/MAINTAINERS b/MAINTAINERS index 6ac000cc006d..8ff2bddc1ac2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -791,6 +791,12 @@ F: drivers/gpu/drm/amd/include/vi_structs.h F: drivers/gpu/drm/amd/include/v9_structs.h F: include/uapi/linux/kfd_ioctl.h +AMD MP2 I2C DRIVER +M: Elie Morisse +L: linux-...@vger.kernel.org +S: Maintained +F: drivers/i2c/busses/i2c-amd-*-mp2* + AMD POWERPLAY M: Rex Zhu M: Evan Quan diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 451d4ae50e66..e20f2d1ce381 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -77,6 +77,21 @@ config I2C_AMD8111 This driver can also be built as a module. If so, the module will be called i2c-amd8111. +config I2C_AMD_MP2_PCI + tristate + depends on PCI + +config I2C_AMD_MP2_PLATFORM + tristate "AMD MP2 Platform" + select I2C_AMD_MP2_PCI + depends on ACPI + help + If you say yes to this option, support will be included for the AMD MP2 + PCI I2C adapter. + + This driver can also be built as a module. If so, the module + will be called i2c-amd-plat-mp2. + config I2C_HIX5HD2 tristate "Hix5hd2 high-speed I2C driver" depends on ARCH_HISI || ARCH_HIX5HD2 || COMPILE_TEST diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 18b26af82b1c..16ef646d7ef5 100644 --- a/drivers/i2c/
[v4] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
This contains two drivers: * i2c-amd-plat-mp2: platform driver managing an i2c adapter (one of the two busses of the MP2) and routing any i2c read write call or command to the PCI driver. * i2c-amd-pci-mp2: PCI driver communicating through the C2P/P2C mailbox registers, or through DMA for more than 32 bytes transfers. This is major rework of the patch submitted by Nehal-bakulchandra Shah from AMD (https://patchwork.kernel.org/patch/10597369/). Most of the event handling of v2/v3 was rewritten since it couldn't work if more than one bus was enabled, and contains many more fixes listed in the patch changelog. With those changes both the touchpad and the touchscreen of the Ryzen-based Lenovo Yoga 530 which lie in separate busses work beautifully. Signed-off-by: Elie Morisse --- Changes since v1:(https://www.spinics.net/lists/linux-i2c/msg34650.html) -> Add fix for IOMMU -> Add depedency of ACPI -> Add locks to avoid the crash Changes since v2:(https://patchwork.ozlabs.org/patch/961270/) -> fix for review comments -> fix for more than 32 bytes write issue Changes since v3 (https://patchwork.kernel.org/patch/10597369/) by Elie M.: -> support more than one bus/adapter -> support more than one slave per bus -> use the bus speed specified by the slaves declared in the DSDT instead of assuming speed == 400kbits/s -> instead of kzalloc'ing a buffer for every less than 32 bytes reads, simply use i2c_msg.buf -> fix buffer overreads/overflows when (<=32 bytes) message lengths aren't a multiple of 4 by using memcpy_fromio and memcpy_toio -> use streaming DMA mappings instead of allocating a coherent DMA buffer for every >32 bytes read/write -> properly check for timeouts during i2c_amd_xfer and increase it from 50 jiffies to 250 msecs (which is more in line with other drivers) -> complete amd_i2c_dev.msg even if the device doesn't return a xxx_success event, instead of stalling i2c_amd_xfer -> removed the spinlock and mdelay during i2c_amd_pci_configure, I didn't see the point since it's already waiting for a i2c_busenable_complete event -> add an adapter-specific mutex lock for i2c_amd_xfer, since we don't want parallel calls writing to AMD_C2P_MSG0 (or AMD_C2P_MSG1) -> add a global mutex lock for registers AMD_C2P_MSG2 to AMD_C2P_MSG9, which are shared across the two busses/adapters -> add MODULE_DEVICE_TABLE to automatically load i2c-amd-platdrv if the DSDT enumerates devices with the "AMDI0011" HID -> set maximum length of reads/writes to 4095 (event's length field is 12 bits) -> basic PM support -> style corrections to match the kernel code style, and tried to reduce code duplication whenever possible MAINTAINERS | 6 + drivers/i2c/busses/Kconfig| 25 + drivers/i2c/busses/Makefile | 2 + drivers/i2c/busses/i2c-amd-pci-mp2.c | 633 ++ drivers/i2c/busses/i2c-amd-pci-mp2.h | 219 + drivers/i2c/busses/i2c-amd-plat-mp2.c | 362 +++ 6 files changed, 1247 insertions(+) create mode 100644 drivers/i2c/busses/i2c-amd-pci-mp2.c create mode 100644 drivers/i2c/busses/i2c-amd-pci-mp2.h create mode 100644 drivers/i2c/busses/i2c-amd-plat-mp2.c diff --git a/MAINTAINERS b/MAINTAINERS index 6ac000cc006d..8ff2bddc1ac2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -791,6 +791,12 @@ F: drivers/gpu/drm/amd/include/vi_structs.h F: drivers/gpu/drm/amd/include/v9_structs.h F: include/uapi/linux/kfd_ioctl.h +AMD MP2 I2C DRIVER +M: Elie Morisse +L: linux-...@vger.kernel.org +S: Maintained +F: drivers/i2c/busses/i2c-amd-*-mp2* + AMD POWERPLAY M: Rex Zhu M: Evan Quan diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 451d4ae50e66..c35a51fa01c5 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -77,6 +77,31 @@ config I2C_AMD8111 This driver can also be built as a module. If so, the module will be called i2c-amd8111. +config I2C_AMD8111 + tristate "AMD 8111" + depends on PCI + help + If you say yes to this option, support will be included for the + second (SMBus 2.0) AMD 8111 mainboard I2C interface. + + This driver can also be built as a module. If so, the module + will be called i2c-amd8111. + +config I2C_AMD_MP2_PCI + tristate + depends on PCI + +config I2C_AMD_MP2_PLATFORM + tristate "AMD MP2 Platform" + select I2C_AMD_MP2_PCI + depends on ACPI + help + If you say yes to this option, support will be included for the AMD MP2 + PCI I2C adapter. + + This driver can also be built as a module. If so, the module + will be called i2c-amd-plat-mp2. + config I2C_HIX5HD2 tristate "Hix5hd2 high-speed I2C driver"
