| Message ID | MN2PR01MB535821C8058C7814B2F8EEDF9F599@MN2PR01MB5358.prod.exchangelabs.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c/pasemi: PASemi I2C controller IRQ enablement | expand |
Hi, Looks almost good to me, just a few minor things: On Sun, Oct 2, 2022, at 00:25, Arminder Singh wrote: > Hello, > > This is v2 of the PASemi I2C controller IRQ enablement patch. This shouldn't be inside the commit description. > > 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 is probably the only paragraph that should be the entire commit description. > > 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 <arminders208@outlook.com> I think it's usually implied that you tested your own patches ;) > Signed-off-by: Arminder Singh <arminders208@outlook.com> > --- > 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/MN2PR01MB535838492432C910F2381F929F6F9@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_MTXFIFO 0x00 > #define REG_MRXFIFO 0x04 > #define REG_SMSTA 0x14 > +#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. > #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..ba6d6ccf9cdc 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 > > @@ -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 completion irq_completion; This doesn't seem to be aligned correctly and the hw_rev line doesn't have to be changed. > }; > > 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 Best, Sven
On 02 October 2022 at 00:25 am, Arminder Singh wrote: > 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 <arminders208@outlook.com> > Signed-off-by: Arminder Singh <arminders208@outlook.com> > --- > 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/MN2PR01MB535838492432C910F2381F929F6F9@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_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..ba6d6ccf9cdc 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 > > @@ -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 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; Tested-by: Christian Zigotzky <chzigotzky at xenosoft.de> on an A-EON AmigaOne X1000 with a PASemi PWRficient PA6T-1682 processor.
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_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..ba6d6ccf9cdc 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 @@ -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 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;