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 <[email protected]>
---
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/MN2PR01MB535821C8058C7814B2F8EEDF9F599@MN2PR01MB5358.prod.exchangelabs.com/
v1 linked here: https://lore.kernel.org/linux-i2c/MN2PR01MB535838492432C910F2381F929F6F9@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_MTXFIFO 0x00
#define REG_MRXFIFO 0x04
#define REG_SMSTA 0x14
+#define REG_IMASK 0x18
#define REG_CTL 0x1c
#define REG_REV 0x28
@@ -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, void *dev_id)
+{
+ struct pasemi_smbus *smbus = dev_id;
+
+ 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..88821f4e8a9f 100644
--- a/drivers/i2c/busses/i2c-pasemi-core.h
+++ b/drivers/i2c/busses/i2c-pasemi-core.h
@@ -7,6 +7,7 @@
#include <linux/i2c-smbus.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/completion.h>
#define PASEMI_HW_REV_PCI -1
@@ -16,6 +17,10 @@ struct pasemi_smbus {
void __iomem *ioaddr;
unsigned int clk_div;
int hw_rev;
+ int use_irq;
+ struct completion irq_completion;
};
int pasemi_i2c_common_probe(struct pasemi_smbus *smbus);
+
+irqreturn_t pasemi_irq_handler(int irq, void *dev_id);
diff --git a/drivers/i2c/busses/i2c-pasemi-platform.c b/drivers/i2c/busses/i2c-pasemi-platform.c
index 88a54aaf7e3c..e35945a91dbe 100644
--- a/drivers/i2c/busses/i2c-pasemi-platform.c
+++ b/drivers/i2c/busses/i2c-pasemi-platform.c
@@ -49,6 +49,7 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
struct pasemi_smbus *smbus;
u32 frequency;
int error;
+ int irq_num;
data = devm_kzalloc(dev, sizeof(struct pasemi_platform_i2c_data),
GFP_KERNEL);
@@ -82,6 +83,11 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
if (error)
goto out_clk_disable;
+ irq_num = platform_get_irq(pdev, 0);
+ error = devm_request_irq(smbus->dev, irq_num, pasemi_irq_handler, 0, "pasemi_apple_i2c", (void *)smbus);
+
+ if (!error)
+ smbus->use_irq = 1;
platform_set_drvdata(pdev, data);
return 0;
--
2.34.1
Hi,
> On 7. Oct 2022, at 02:43, Arminder Singh <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> 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.
I think Wolfram suggested to keep this in the commit message. If in doubt listen to him and not me because he’s much more experienced with the kernel than I am ;)
>
> 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)
You can just collect and keep those tags above your signed off by if you only change things like whitespaces.
>
> 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/MN2PR01MB535821C8058C7814B2F8EEDF9F599@MN2PR01MB5358.prod.exchangelabs.com/
> v1 linked here: https://lore.kernel.org/linux-i2c/MN2PR01MB535838492432C910F2381F929F6F9@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_MTXFIFO 0x00
> #define REG_MRXFIFO 0x04
> #define REG_SMSTA 0x14
> +#define REG_IMASK 0x18
> #define REG_CTL 0x1c
> #define REG_REV 0x28
>
> @@ -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, void *dev_id)
> +{
> + struct pasemi_smbus *smbus = dev_id;
> +
> + complete(&smbus->irq_completion);
I only realized just now that you also want to disable the interrupt right here by writing to IMASK. This is a level sensitive interrupt at AIC level so the moment this handler returns it will fire again until you reach the write above after the completion wait a bit later.
> + return IRQ_HANDLED;
> +}
> diff --git a/drivers/i2c/busses/i2c-pasemi-core.h b/drivers/i2c/busses/i2c-pasemi-core.h
> index 4655124a37f3..88821f4e8a9f 100644
> --- a/drivers/i2c/busses/i2c-pasemi-core.h
> +++ b/drivers/i2c/busses/i2c-pasemi-core.h
> @@ -7,6 +7,7 @@
> #include <linux/i2c-smbus.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> +#include <linux/completion.h>
>
> #define PASEMI_HW_REV_PCI -1
>
> @@ -16,6 +17,10 @@ struct pasemi_smbus {
> void __iomem *ioaddr;
> unsigned int clk_div;
> int hw_rev;
> + int use_irq;
> + struct completion irq_completion;
> };
>
> int pasemi_i2c_common_probe(struct pasemi_smbus *smbus);
> +
> +irqreturn_t pasemi_irq_handler(int irq, void *dev_id);
> diff --git a/drivers/i2c/busses/i2c-pasemi-platform.c b/drivers/i2c/busses/i2c-pasemi-platform.c
> index 88a54aaf7e3c..e35945a91dbe 100644
> --- a/drivers/i2c/busses/i2c-pasemi-platform.c
> +++ b/drivers/i2c/busses/i2c-pasemi-platform.c
> @@ -49,6 +49,7 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
> struct pasemi_smbus *smbus;
> u32 frequency;
> int error;
> + int irq_num;
>
> data = devm_kzalloc(dev, sizeof(struct pasemi_platform_i2c_data),
> GFP_KERNEL);
> @@ -82,6 +83,11 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
> if (error)
> goto out_clk_disable;
>
> + irq_num = platform_get_irq(pdev, 0);
> + error = devm_request_irq(smbus->dev, irq_num, pasemi_irq_handler, 0, "pasemi_apple_i2c", (void *)smbus);
> +
> + if (!error)
> + smbus->use_irq = 1;
> platform_set_drvdata(pdev, data);
>
> return 0;
> --
> 2.34.1
>
Sven
> > + complete(&smbus->irq_completion);
>
> I only realized just now that you also want to disable the interrupt
> right here by writing to IMASK. This is a level sensitive interrupt at
> AIC level so the moment this handler returns it will fire again until
> you reach the write above after the completion wait a bit later.
This seems like a valid request. Any chance for a v4? We are so close to
being good here...
On 07/10/2022 09.42, Arminder Singh wrote:
> 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 <[email protected]>
> ---
> 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.
>
[...]
Please increase the timeout to 100ms for v4. 10ms was always wrong (the
datasheet says the hardware clock stretching timeout is 25ms, and most
i2c drivers have much larger timeouts), and with the tighter timing
achievable with the IRQ patchset we are seeing timeouts in tipd
controller requests which can clock-stretch for ~10ms themselves,
followed by a spiral of errors as the driver has pretty poor error
recovery. Increasing the timeout fixes the immediate problem/regression.
Other than that, I now have a patch that makes the whole timeout/error
detection/recovery much more robust, but I can submit it after this goes
in :)
- Hector
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
Thanks Hector! Acknowledged the need to change to a 100ms delay, will be
addressed/changed in v4 of the patch.
Thanks,
Arminder