From: Christian Zigotzky <chzigotzky@xenosoft.de>
To: Arminder Singh <arminders208@outlook.com>, linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org,
Darren Stevens <darren@stevens-zone.net>,
Sven Peter <sven@svenpeter.dev>, Hector Martin <marcan@marcan.st>,
Paul Mackerras <paulus@samba.org>,
linux-i2c@vger.kernel.org, "R.T.Dickinson" <rtd2@xtra.co.nz>,
Christian Zigotzky <info@xenosoft.de>,
linuxppc-dev@lists.ozlabs.org,
Alyssa Rosenzweig <alyssa@rosenzweig.io>,
asahi@lists.linux.dev
Subject: Re: [PATCH v2] i2c/pasemi: PASemi I2C controller IRQ enablement
Date: Sun, 2 Oct 2022 17:28:35 +0200 [thread overview]
Message-ID: <1ad42694-a002-2108-37b5-ffcd8f08d442@xenosoft.de> (raw)
In-Reply-To: <MN2PR01MB535821C8058C7814B2F8EEDF9F599@MN2PR01MB5358.prod.exchangelabs.com>
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.
prev parent reply other threads:[~2022-10-02 15:30 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-01 22:25 [PATCH v2] i2c/pasemi: PASemi I2C controller IRQ enablement Arminder Singh
2022-10-02 11:21 ` Sven Peter
2022-10-02 11:28 ` Wolfram Sang
2022-10-02 14:07 ` [PATCH v2] i2c-pasemi: " Arminder Singh
2022-10-02 15:02 ` Sven Peter
2022-10-04 1:27 ` Michael Ellerman
2022-10-05 19:04 ` Wolfram Sang
2022-10-02 15:28 ` Christian Zigotzky [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1ad42694-a002-2108-37b5-ffcd8f08d442@xenosoft.de \
--to=chzigotzky@xenosoft.de \
--cc=alyssa@rosenzweig.io \
--cc=arminders208@outlook.com \
--cc=asahi@lists.linux.dev \
--cc=darren@stevens-zone.net \
--cc=info@xenosoft.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=marcan@marcan.st \
--cc=paulus@samba.org \
--cc=rtd2@xtra.co.nz \
--cc=sven@svenpeter.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).