[PATCH v4] i2c/pasemi: PASemi I2C controller IRQ enablement
This patch adds IRQ support to the PASemi I2C controller driver to increase the performace of I2C transactions on platforms with PASemi I2C controllers. While primarily intended for Apple silicon platforms, this patch should also help in enabling IRQ support for older PASemi hardware as well should the need arise. This version of the patch has been tested on an M1 Ultra Mac Studio, as well as an M1 MacBook Pro, and userspace launches successfully while using the IRQ path for I2C transactions. Signed-off-by: Arminder Singh --- This version of the patch fixes some reliability issues brought up by Hector and Sven in the v3 patch email thread. First, this patch increases the timeout value in pasemi_smb_waitready to 100ms from 10ms, as the original 10ms timeout in the driver was incorrect according to the controller's datasheet as Hector pointed out in the v3 patch email thread. This incorrect timeout had caused some issues with the tps6598x controller on Apple silicon platforms. This version of the patch also adds a reg_write to REG_IMASK in the IRQ handler, because as Sven pointed out in the previous thread, the I2C transaction interrupt is level sensitive so not masking the interrupt in REG_IMASK will cause the interrupt to trigger again when it leaves the IRQ handler until it reaches the call to reg_write after the completion expires. Patch changelog: v3 to v4 changes: - Increased the timeout value for I2C transactions to 100ms, as the original 10ms timeout in the driver was incorrect according to the I2C chip's datasheet. Mitigates an issue with the tps6598x controller on Apple silicon platforms. - Added a reg_write to REG_IMASK inside the IRQ handler, which prevents the IRQ from triggering again after leaving the IRQ handler, as the IRQ is level-sensitive. v2 to v3 changes: - Fixed some whitespace and alignment issues found in v2 of the patch v1 to v2 changes: - moved completion setup from pasemi_platform_i2c_probe to pasemi_i2c_common_probe to allow PASemi and Apple platforms to share common completion setup code in case PASemi hardware gets IRQ support added - initialized the status variable in pasemi_smb_waitready when going down the non-IRQ path - removed an unnecessary cast of dev_id in the IRQ handler - fixed alignment of struct member names in i2c-pasemi-core.h (addresses Christophe's feedback in the original submission) - IRQs are now disabled after the wait_for_completion_timeout call instead of inside the IRQ handler (prevents the IRQ from going off after the completion times out) - changed the request_irq call to a devm_request_irq call to obviate the need for a remove function and a free_irq call (thanks to Sven for pointing this out in the original submission) - added a reinit_completion call to pasemi_reset as a failsafe to prevent missed interrupts from causing the completion to never complete (thanks to Arnd Bergmann for pointing this out) - removed the bitmask variable in favor of just using the value directly (it wasn't used anywhere else) v3: https://lore.kernel.org/linux-i2c/mn2pr01mb5358ed8fc32c0cfaebd4a0e19f...@mn2pr01mb5358.prod.exchangelabs.com/T/ v2: https://lore.kernel.org/linux-i2c/mn2pr01mb535821c8058c7814b2f8eedf9f...@mn2pr01mb5358.prod.exchangelabs.com/T/ v1: https://lore.kernel.org/linux-i2c/mn2pr01mb535838492432c910f2381f929f...@mn2pr01mb5358.prod.exchangelabs.com/T/ drivers/i2c/busses/i2c-pasemi-core.c | 32 drivers/i2c/busses/i2c-pasemi-core.h | 5 drivers/i2c/busses/i2c-pasemi-platform.c | 6 + 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c index 9028ffb58cc0..7d54a9f34c74 100644 --- a/drivers/i2c/busses/i2c-pasemi-core.c +++ b/drivers/i2c/busses/i2c-pasemi-core.c @@ -21,6 +21,7 @@ #define REG_MTXFIFO0x00 #define REG_MRXFIFO0x04 #define REG_SMSTA 0x14 +#define REG_IMASK 0x18 #define REG_CTL0x1c #define REG_REV0x28 @@ -66,6 +67,7 @@ static void pasemi_reset(struct pasemi_smbus *smbus) val |= CTL_EN; reg_write(smbus, REG_CTL, val); + reinit_completion(&smbus->irq_completion); } static void pasemi_smb_clear(struct pasemi_smbus *smbus) @@ -78,14 +80,21 @@ static void pasemi_smb_clear(struct pasemi_smbus *smbus) static int pasemi_smb_waitready(struct pasemi_smbus *smbus) { - int timeout = 10; + int timeout = 100; unsigned int status; - status = reg_read(smbus, REG_SMSTA); - - while (!(status & SMSTA_XEN) && timeout--) { - msleep(1); + if (smbus->use_irq) { + reinit_completion(&smbus->irq_completion); + reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN); + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_j
Re: [PATCH v3] i2c/pasemi: PASemi I2C controller IRQ enablement
Thanks Hector! Acknowledged the need to change to a 100ms delay, will be addressed/changed in v4 of the patch. Thanks, Arminder
Re: [PATCH v3] i2c/pasemi: PASemi I2C controller IRQ enablement
Don't worry I have not forgotten about the patch! I am working on a v4, however I just got pretty busy with real life things so I ended up putting this aside for a bit. v4 of the patch should be ready by the end of the week assuming everything goes well. Thanks, Arminder
[PATCH v3] i2c/pasemi: PASemi I2C controller IRQ enablement
This patch adds IRQ support to the PASemi I2C controller driver to increase the performace of I2C transactions on platforms with PASemi I2C controllers. While primarily intended for Apple silicon platforms, this patch should also help in enabling IRQ support for older PASemi hardware as well should the need arise. Signed-off-by: Arminder Singh --- This version of the patch has been tested on an M1 Ultra Mac Studio, as well as an M1 MacBook Pro, and userspace launches successfully while using the IRQ path for I2C transactions. This version of the patch only contains fixes to the whitespace and alignment issues found in v2 of the patch, and as such the testing that Christian Zigotsky did on PASemi hardware for v2 of the patch also applies to this version of the patch as well. (See v2 patch email thread for the "Tested-by" tag) v2 to v3 changes: - Fixed some whitespace and alignment issues found in v2 of the patch v1 to v2 changes: - moved completion setup from pasemi_platform_i2c_probe to pasemi_i2c_common_probe to allow PASemi and Apple platforms to share common completion setup code in case PASemi hardware gets IRQ support added - initialized the status variable in pasemi_smb_waitready when going down the non-IRQ path - removed an unnecessary cast of dev_id in the IRQ handler - fixed alignment of struct member names in i2c-pasemi-core.h (addresses Christophe's feedback in the original submission) - IRQs are now disabled after the wait_for_completion_timeout call instead of inside the IRQ handler (prevents the IRQ from going off after the completion times out) - changed the request_irq call to a devm_request_irq call to obviate the need for a remove function and a free_irq call (thanks to Sven for pointing this out in the original submission) - added a reinit_completion call to pasemi_reset as a failsafe to prevent missed interrupts from causing the completion to never complete (thanks to Arnd Bergmann for pointing this out) - removed the bitmask variable in favor of just using the value directly (it wasn't used anywhere else) v2 linked here: https://lore.kernel.org/linux-i2c/mn2pr01mb535821c8058c7814b2f8eedf9f...@mn2pr01mb5358.prod.exchangelabs.com/ v1 linked here: https://lore.kernel.org/linux-i2c/mn2pr01mb535838492432c910f2381f929f...@mn2pr01mb5358.prod.exchangelabs.com/T/#m11b3504c2667517aad7521514c99ca0e07a9381f Hopefully the patch is good to go this time around! drivers/i2c/busses/i2c-pasemi-core.c | 29 drivers/i2c/busses/i2c-pasemi-core.h | 5 drivers/i2c/busses/i2c-pasemi-platform.c | 6 + 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c index 9028ffb58cc0..4855144b370e 100644 --- a/drivers/i2c/busses/i2c-pasemi-core.c +++ b/drivers/i2c/busses/i2c-pasemi-core.c @@ -21,6 +21,7 @@ #define REG_MTXFIFO0x00 #define REG_MRXFIFO0x04 #define REG_SMSTA 0x14 +#define REG_IMASK 0x18 #define REG_CTL0x1c #define REG_REV0x28 @@ -66,6 +67,7 @@ static void pasemi_reset(struct pasemi_smbus *smbus) val |= CTL_EN; reg_write(smbus, REG_CTL, val); + reinit_completion(&smbus->irq_completion); } static void pasemi_smb_clear(struct pasemi_smbus *smbus) @@ -81,11 +83,18 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus) int timeout = 10; unsigned int status; - status = reg_read(smbus, REG_SMSTA); - - while (!(status & SMSTA_XEN) && timeout--) { - msleep(1); + if (smbus->use_irq) { + reinit_completion(&smbus->irq_completion); + reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN); + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10)); + reg_write(smbus, REG_IMASK, 0); status = reg_read(smbus, REG_SMSTA); + } else { + status = reg_read(smbus, REG_SMSTA); + while (!(status & SMSTA_XEN) && timeout--) { + msleep(1); + status = reg_read(smbus, REG_SMSTA); + } } /* Got NACK? */ @@ -344,10 +353,14 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus) /* set up the sysfs linkage to our parent device */ smbus->adapter.dev.parent = smbus->dev; + smbus->use_irq = 0; + init_completion(&smbus->irq_completion); if (smbus->hw_rev != PASEMI_HW_REV_PCI) smbus->hw_rev = reg_read(smbus, REG_REV); + reg_write(smbus, REG_IMASK, 0); + pasemi_reset(smbus); error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter); @@ -356,3 +369,11 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus) return 0; } + +
Re: [PATCH v2] i2c-pasemi: PASemi I2C controller IRQ enablement
Hi, > #define REG_MTXFIFO 0x00 > #define REG_MRXFIFO 0x04 > #define REG_SMSTA0x14 > +#define REG_IMASK 0x18 > This doesn't seem to be aligned correctly, this file seems to use a tab > to separate the register name and the offset and you used spaces here. > @@ -15,7 +16,11 @@ struct pasemi_smbus { > struct i2c_adapter adapter; > void __iomem*ioaddr; > unsigned int clk_div; > - int hw_rev; > + int hw_rev; > + int use_irq; > + struct completionirq_completion; > This doesn't seem to be aligned correctly and the hw_rev line > doesn't have to be changed. I'm sorry for the alignment issues in the patch, I genuinely didn't notice them as from the perspective of my primary editor (Visual Studio Code) the entries were aligned. I just saw them when opening the files in nano. Does fixing the alignment issues and the commit description justify a v3 of the patch or should the minor fixes go out as a "resend"? Just not sure in this particular case as the fixes seem to be very minor ones. Thanks, Arminder
[PATCH v2] i2c/pasemi: PASemi I2C controller IRQ enablement
Hello, This is v2 of the PASemi I2C controller IRQ enablement patch. This patch adds IRQ support to the PASemi I2C controller driver to increase the performace of I2C transactions on platforms with PASemi I2C controllers. While the patch is primarily intended for Apple silicon platforms, this patch should also help in enabling IRQ support for older PASemi hardware as well should the need arise. This version of the patch has been tested on an M1 Ultra Mac Studio, as well as an M1 MacBook Pro, and userspace launches successfully while using the IRQ path for I2C transactions. Tested-by: Arminder Singh Signed-off-by: Arminder Singh --- Changes from v1: - moved completion setup from pasemi_platform_i2c_probe to pasemi_i2c_common_probe to allow PASemi and Apple platforms to share common completion setup code in case PASemi hardware gets IRQ support added - initialized the status variable in pasemi_smb_waitready when going down the non-IRQ path - removed an unnecessary cast of dev_id in the IRQ handler - fixed alignment of struct member names in i2c-pasemi-core.h (addresses Christophe's feedback in the original submission) - IRQs are now disabled after the wait_for_completion_timeout call instead of inside the IRQ handler (prevents the IRQ from going off after the completion times out) - changed the request_irq call to a devm_request_irq call to obviate the need for a remove function and a free_irq call (thanks to Sven for pointing this out in the original submission) - added a reinit_completion call to pasemi_reset as a failsafe to prevent missed interrupts from causing the completion to never complete (thanks to Arnd Bergmann for pointing this out) - removed the bitmask variable in favor of just using the value directly (it wasn't used anywhere else) v1 linked here: https://lore.kernel.org/linux-i2c/mn2pr01mb535838492432c910f2381f929f...@mn2pr01mb5358.prod.exchangelabs.com/T/#m11b3504c2667517aad7521514c99ca0e07a9381f Thanks for all the feedback on the previous submission, I'm sorry I wasn't able to answer everyone's emails, was just pretty busy, I'll make sure to be more responsive this time around! Also wasn't sure whether the v1 changelog belonged before or after the '---' so I put it after to keep the commit changelog short and concise. (This is just one patch, didn't think it needed a cover letter) drivers/i2c/busses/i2c-pasemi-core.c | 29 drivers/i2c/busses/i2c-pasemi-core.h | 7 +- drivers/i2c/busses/i2c-pasemi-platform.c | 6 + 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c index 9028ffb58cc0..05af8f3575bc 100644 --- a/drivers/i2c/busses/i2c-pasemi-core.c +++ b/drivers/i2c/busses/i2c-pasemi-core.c @@ -21,6 +21,7 @@ #define REG_MTXFIFO0x00 #define REG_MRXFIFO0x04 #define REG_SMSTA 0x14 +#define REG_IMASK 0x18 #define REG_CTL0x1c #define REG_REV0x28 @@ -66,6 +67,7 @@ static void pasemi_reset(struct pasemi_smbus *smbus) val |= CTL_EN; reg_write(smbus, REG_CTL, val); + reinit_completion(&smbus->irq_completion); } static void pasemi_smb_clear(struct pasemi_smbus *smbus) @@ -81,11 +83,18 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus) int timeout = 10; unsigned int status; - status = reg_read(smbus, REG_SMSTA); - - while (!(status & SMSTA_XEN) && timeout--) { - msleep(1); + if (smbus->use_irq) { + reinit_completion(&smbus->irq_completion); + reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN); + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10)); + reg_write(smbus, REG_IMASK, 0); status = reg_read(smbus, REG_SMSTA); + } else { + status = reg_read(smbus, REG_SMSTA); + while (!(status & SMSTA_XEN) && timeout--) { + msleep(1); + status = reg_read(smbus, REG_SMSTA); + } } /* Got NACK? */ @@ -344,10 +353,14 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus) /* set up the sysfs linkage to our parent device */ smbus->adapter.dev.parent = smbus->dev; + smbus->use_irq = 0; + init_completion(&smbus->irq_completion); if (smbus->hw_rev != PASEMI_HW_REV_PCI) smbus->hw_rev = reg_read(smbus, REG_REV); + reg_write(smbus, REG_IMASK, 0); + pasemi_reset(smbus); error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter); @@ -356,3 +369,11 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus) return 0; } + +irqreturn_t pasemi_irq_handler(int irq, vo
[PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
This is the first time I'm interacting with the Linux mailing lists, so please don't eviscerate me *too much* if I get the formatting wrong. Of course I'm always willing to take criticism and improve my formatting in the future. This patch adds support for IRQs to the PASemi I2C controller driver. This will allow for faster performing I2C transactions on Apple Silicon hardware, as previously, the driver was forced to poll the SMSTA register for a set amount of time. With this patchset the driver on Apple silicon hardware will instead wait for an interrupt which will signal the completion of the I2C transaction. The timeout value for this completion will be the same as the current amount of time the I2C driver polls for. This will result in some performance improvement since the driver will be waiting for less time than it does right now on Apple Silicon hardware. The patch right now will only enable IRQs for Apple Silicon I2C chips, and only if it's able to successfully request the IRQ from the kernel. === Testing === This patch has been tested on both the mainline Linux kernel tree and the Asahi branch (https://github.com/AsahiLinux/linux.git) on both an M1 and M2 MacBook Air, and it compiles successfully as both a module and built-in to the kernel itself. The patch in both trees successfully boots to userspace without any hitch. I do not have PASemi hardware on hand unfortunately, so I'm unable to test the impact of this patch on old PASemi hardware. This is also why I've elected to do the IRQ request and enablement on the Apple platform driver and not in the common file, as I'm not sure if PASemi hardware supports IRQs. I also fixed a quick checkpatch warning on line 303. "i ++" is now "i++". Any and all critiques of the patch would be well appreciated. Signed-off-by: Arminder Singh --- drivers/i2c/busses/i2c-pasemi-core.c | 29 drivers/i2c/busses/i2c-pasemi-core.h | 5 drivers/i2c/busses/i2c-pasemi-platform.c | 8 +++ 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c index 9028ffb58cc0..375aa9528233 100644 --- a/drivers/i2c/busses/i2c-pasemi-core.c +++ b/drivers/i2c/busses/i2c-pasemi-core.c @@ -21,6 +21,7 @@ #define REG_MTXFIFO0x00 #define REG_MRXFIFO0x04 #define REG_SMSTA 0x14 +#define REG_IMASK 0x18 #define REG_CTL0x1c #define REG_REV0x28 @@ -80,14 +81,21 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus) { int timeout = 10; unsigned int status; + unsigned int bitmask = SMSTA_XEN | SMSTA_MTN; - status = reg_read(smbus, REG_SMSTA); - - while (!(status & SMSTA_XEN) && timeout--) { - msleep(1); + if (smbus->use_irq) { + reinit_completion(&smbus->irq_completion); + reg_write(smbus, REG_IMASK, bitmask); + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10)); status = reg_read(smbus, REG_SMSTA); + } else { + while (!(status & SMSTA_XEN) && timeout--) { + msleep(1); + status = reg_read(smbus, REG_SMSTA); + } } + /* Got NACK? */ if (status & SMSTA_MTN) return -ENXIO; @@ -300,7 +308,7 @@ static int pasemi_smb_xfer(struct i2c_adapter *adapter, case I2C_SMBUS_BLOCK_DATA: case I2C_SMBUS_BLOCK_PROC_CALL: data->block[0] = len; - for (i = 1; i <= len; i ++) { + for (i = 1; i <= len; i++) { rd = RXFIFO_RD(smbus); if (rd & MRXFIFO_EMPTY) { err = -ENODATA; @@ -348,6 +356,8 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus) if (smbus->hw_rev != PASEMI_HW_REV_PCI) smbus->hw_rev = reg_read(smbus, REG_REV); + reg_write(smbus, REG_IMASK, 0); + pasemi_reset(smbus); error = devm_i2c_add_adapter(smbus->dev, &smbus->adapter); @@ -356,3 +366,12 @@ int pasemi_i2c_common_probe(struct pasemi_smbus *smbus) return 0; } + +irqreturn_t pasemi_irq_handler(int irq, void *dev_id) +{ + struct pasemi_smbus *smbus = (struct pasemi_smbus *)dev_id; + + reg_write(smbus, REG_IMASK, 0); + complete(&smbus->irq_completion); + return IRQ_HANDLED; +} diff --git a/drivers/i2c/busses/i2c-pasemi-core.h b/drivers/i2c/busses/i2c-pasemi-core.h index 4655124a37f3..045e4a9a3d13 100644 --- a/drivers/i2c/busses/i2c-pasemi-core.h +++ b/drivers/i2c/busses/i2c-pasemi-core.h @@ -7,6 +7,7 @@ #include #include #include +#include #define PASEMI_HW_REV_PCI -1 @@ -16,6 +17,10 @@ struct pasemi_smbus { void __i