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 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=154031133019835) by > > Elie M.: > > > > -> fix missing typecast warning > > -> removed the
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 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=154031133019835) by > > Elie M.: > > > > -> fix missing typecast warning > > -> removed the
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
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
Re: [PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
I've tested v7 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
Re: [PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
I've tested v7 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
Re: [PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
[+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 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=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 ++ >
Re: [PATCH v7] i2c: Add PCI and platform drivers for the AMD MP2 I2C controller
[+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 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=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 ++ >
[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=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" + 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
[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=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" + 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
