* Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
2022-08-20 19:45 [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon Arminder Singh
@ 2022-08-20 21:39 ` kernel test robot
2022-08-21 7:24 ` Christophe Leroy
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2022-08-20 21:39 UTC (permalink / raw)
To: Arminder Singh, linux-kernel
Cc: llvm, kbuild-all, Sven Peter, Alyssa Rosenzweig, Hector Martin,
Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
linux-arm-kernel, linuxppc-dev, linux-i2c, Arminder Singh
Hi Arminder,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on linus/master v6.0-rc1 next-20220819]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Arminder-Singh/i2c-pasemi-Add-IRQ-support-for-Apple-Silicon/20220821-034703
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: riscv-randconfig-r042-20220821 (https://download.01.org/0day-ci/archive/20220821/202208210548.MCoIwyQl-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project c9a41fe60ab62f7a40049c100adcc8087a47669b)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/98584b2b51d808ab582798c7a50511e8c1889ced
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Arminder-Singh/i2c-pasemi-Add-IRQ-support-for-Apple-Silicon/20220821-034703
git checkout 98584b2b51d808ab582798c7a50511e8c1889ced
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/i2c/busses/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from drivers/i2c/busses/i2c-pasemi-core.c:9:
In file included from include/linux/pci.h:38:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:136:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/i2c/busses/i2c-pasemi-core.c:9:
In file included from include/linux/pci.h:38:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:136:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/i2c/busses/i2c-pasemi-core.c:9:
In file included from include/linux/pci.h:38:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/riscv/include/asm/io.h:136:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:1134:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
~~~~~~~~~~ ^
>> drivers/i2c/busses/i2c-pasemi-core.c:86:6: warning: variable 'status' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (smbus->use_irq) {
^~~~~~~~~~~~~~
drivers/i2c/busses/i2c-pasemi-core.c:92:12: note: uninitialized use occurs here
while (!(status & SMSTA_XEN) && timeout--) {
^~~~~~
drivers/i2c/busses/i2c-pasemi-core.c:86:2: note: remove the 'if' if its condition is always true
if (smbus->use_irq) {
^~~~~~~~~~~~~~~~~~~~
drivers/i2c/busses/i2c-pasemi-core.c:83:21: note: initialize the variable 'status' to silence this warning
unsigned int status;
^
= 0
8 warnings generated.
vim +86 drivers/i2c/busses/i2c-pasemi-core.c
79
80 static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
81 {
82 int timeout = 10;
83 unsigned int status;
84 unsigned int bitmask = SMSTA_XEN | SMSTA_MTN;
85
> 86 if (smbus->use_irq) {
87 reinit_completion(&smbus->irq_completion);
88 reg_write(smbus, REG_IMASK, bitmask);
89 wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
90 status = reg_read(smbus, REG_SMSTA);
91 } else {
92 while (!(status & SMSTA_XEN) && timeout--) {
93 msleep(1);
94 status = reg_read(smbus, REG_SMSTA);
95 }
96 }
97
98
99 /* Got NACK? */
100 if (status & SMSTA_MTN)
101 return -ENXIO;
102
103 if (timeout < 0) {
104 dev_warn(smbus->dev, "Timeout, status 0x%08x\n", status);
105 reg_write(smbus, REG_SMSTA, status);
106 return -ETIME;
107 }
108
109 /* Clear XEN */
110 reg_write(smbus, REG_SMSTA, SMSTA_XEN);
111
112 return 0;
113 }
114
--
0-DAY CI Kernel Test Service
https://01.org/lkp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
2022-08-20 19:45 [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon Arminder Singh
2022-08-20 21:39 ` kernel test robot
@ 2022-08-21 7:24 ` Christophe Leroy
2022-08-21 9:55 ` Sven Peter
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Christophe Leroy @ 2022-08-21 7:24 UTC (permalink / raw)
To: Arminder Singh
Cc: Sven Peter, Hector Martin, Paul Mackerras,
linux-arm-kernel@lists.infradead.org,
linuxppc-dev@lists.ozlabs.org, Alyssa Rosenzweig,
linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Le 20/08/2022 à 21:45, Arminder Singh a écrit :
> 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.
The above text should go after you signed-off-by, after the --- , so
that it get's ignored by 'git am' when applying.
>
> 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.
That as well shouldn't be part of the commit message.
>
>
>
>
> Signed-off-by: Arminder Singh <arminders208@outlook.com>
> ---
> 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_MTXFIFO 0x00
> #define REG_MRXFIFO 0x04
> #define REG_SMSTA 0x14
> +#define REG_IMASK 0x18
> #define REG_CTL 0x1c
> #define REG_REV 0x28
>
> @@ -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;
Used only inside the if (smbus->use_irq), bitmask should be defined only
in that scope.
>
> - status = reg_read(smbus, REG_SMSTA);
This should go in the else block.
> -
> - 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 {
status is uninitialised when entering the while loop.
> + while (!(status & SMSTA_XEN) && timeout--) {
> + msleep(1);
> + status = reg_read(smbus, REG_SMSTA);
> + }
> }
>
> +
Why a second blank line here ?
> /* 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++) {
This shouldn't be part of this patch, it is unrelated.
> 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;
dev_id is void*, the cast is not needed.
> +
> + 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 <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;
Keep same alignment for member name.
> };
>
> 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..ee1c84e7734b 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,13 @@ static int pasemi_platform_i2c_probe(struct platform_device *pdev)
> if (error)
> goto out_clk_disable;
>
> + smbus->use_irq = 0;
> + init_completion(&smbus->irq_completion);
> + irq_num = platform_get_irq(pdev, 0);
> + error = request_irq(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
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
2022-08-20 19:45 [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon Arminder Singh
2022-08-20 21:39 ` kernel test robot
2022-08-21 7:24 ` Christophe Leroy
@ 2022-08-21 9:55 ` Sven Peter
2022-08-22 8:44 ` Dan Carpenter
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Sven Peter @ 2022-08-21 9:55 UTC (permalink / raw)
To: Arminder Singh, linux-kernel
Cc: Alyssa Rosenzweig, Hector Martin, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, linux-arm-kernel,
linuxppc-dev, linux-i2c
Hi,
Thanks for the patch! Some additional comments:
On Sat, Aug 20, 2022, at 21:45, Arminder Singh wrote:
> 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 <arminders208@outlook.com>
> ---
> 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_MTXFIFO 0x00
> #define REG_MRXFIFO 0x04
> #define REG_SMSTA 0x14
> +#define REG_IMASK 0x18
> #define REG_CTL 0x1c
> #define REG_REV 0x28
>
> @@ -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);
s/bitmask/SMSTA_XEN | SMSTA_MTN/ and then you can just drop the bitmask
variable which isn't used anywhere else.
> + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
> status = reg_read(smbus, REG_SMSTA);
If the irq hasn't fired and wait_for_completion_timeout timed out the irq
is still enabled here. I'd put a reg_write(smbus, REG_IMASK, 0); here
to be safe.
> + } else {
You also need status = reg_read(smbus, REG_SMSTA); here.
> + 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 <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..ee1c84e7734b 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,13 @@ static int pasemi_platform_i2c_probe(struct
> platform_device *pdev)
> if (error)
> goto out_clk_disable;
>
> + smbus->use_irq = 0;
> + init_completion(&smbus->irq_completion);
I'd move this into the common probe function. If someone eventually wants
to add irq support to the PASemi boards it'll be required there as well.
> + irq_num = platform_get_irq(pdev, 0);
> + error = request_irq(irq_num, pasemi_irq_handler, 0,
> "pasemi_apple_i2c", (void *)smbus);
If you use request_irq here you'll have to add a remove function and
call free_irq there. I'd just use devm_request_irq which takes care of
that automatically.
> +
> + if (!error)
> + smbus->use_irq = 1;
> platform_set_drvdata(pdev, data);
>
> return 0;
> --
> 2.34.1
Best,
Sven
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
2022-08-20 19:45 [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon Arminder Singh
` (2 preceding siblings ...)
2022-08-21 9:55 ` Sven Peter
@ 2022-08-22 8:44 ` Dan Carpenter
2022-08-23 1:50 ` Michael Ellerman
2022-08-25 12:29 ` Arnd Bergmann
5 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2022-08-22 8:44 UTC (permalink / raw)
To: kbuild, Arminder Singh, linux-kernel
Cc: lkp, kbuild-all, Sven Peter, Alyssa Rosenzweig, Hector Martin,
Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
linux-arm-kernel, linuxppc-dev, linux-i2c, Arminder Singh
Hi Arminder,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Arminder-Singh/i2c-pasemi-Add-IRQ-support-for-Apple-Silicon/20220821-034703
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: parisc-randconfig-m031-20220821 (https://download.01.org/0day-ci/archive/20220822/202208220231.f88sizqa-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/i2c/busses/i2c-pasemi-core.c:92 pasemi_smb_waitready() error: uninitialized symbol 'status'.
vim +/status +92 drivers/i2c/busses/i2c-pasemi-core.c
8032214346c0c8 drivers/i2c/busses/i2c-pasemi.c Julia Lawall 2010-09-05 80 static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 81 {
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 82 int timeout = 10;
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 83 unsigned int status;
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20 84 unsigned int bitmask = SMSTA_XEN | SMSTA_MTN;
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 85
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20 86 if (smbus->use_irq) {
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20 87 reinit_completion(&smbus->irq_completion);
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20 88 reg_write(smbus, REG_IMASK, bitmask);
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20 89 wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(10));
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 90 status = reg_read(smbus, REG_SMSTA);
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20 91 } else {
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 @92 while (!(status & SMSTA_XEN) && timeout--) {
"status" not intialized.
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 93 msleep(1);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 94 status = reg_read(smbus, REG_SMSTA);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 95 }
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20 96 }
98584b2b51d808 drivers/i2c/busses/i2c-pasemi-core.c Arminder Singh 2022-08-20 97
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 98
be8a1f7cd4501c drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-11-15 99 /* Got NACK? */
be8a1f7cd4501c drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-11-15 100 if (status & SMSTA_MTN)
be8a1f7cd4501c drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-11-15 101 return -ENXIO;
be8a1f7cd4501c drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-11-15 102
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 103 if (timeout < 0) {
c06f50ed36cc0a drivers/i2c/busses/i2c-pasemi.c Sven Peter 2021-10-08 104 dev_warn(smbus->dev, "Timeout, status 0x%08x\n", status);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 105 reg_write(smbus, REG_SMSTA, status);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 106 return -ETIME;
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 107 }
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 108
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 109 /* Clear XEN */
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 110 reg_write(smbus, REG_SMSTA, SMSTA_XEN);
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 111
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 112 return 0;
beb58aa39e6e5a drivers/i2c/busses/i2c-pasemi.c Olof Johansson 2007-02-13 113 }
--
0-DAY CI Kernel Test Service
https://01.org/lkp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
2022-08-20 19:45 [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon Arminder Singh
` (3 preceding siblings ...)
2022-08-22 8:44 ` Dan Carpenter
@ 2022-08-23 1:50 ` Michael Ellerman
2022-08-25 14:45 ` Christian Zigotzky
2022-08-25 12:29 ` Arnd Bergmann
5 siblings, 1 reply; 8+ messages in thread
From: Michael Ellerman @ 2022-08-23 1:50 UTC (permalink / raw)
To: Arminder Singh, linux-kernel
Cc: Sven Peter, Alyssa Rosenzweig, Hector Martin,
Benjamin Herrenschmidt, Paul Mackerras, linux-arm-kernel,
linuxppc-dev, linux-i2c, Arminder Singh, Darren Stevens,
Christian Zigotzky
Arminder Singh <arminders208@outlook.com> writes:
> 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've added Darren and Christian to Cc, they have helped with PASemi
development and testing in the past, and may be able to help test this
series on PASemi hardware.
cheers
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
2022-08-23 1:50 ` Michael Ellerman
@ 2022-08-25 14:45 ` Christian Zigotzky
0 siblings, 0 replies; 8+ messages in thread
From: Christian Zigotzky @ 2022-08-25 14:45 UTC (permalink / raw)
To: Michael Ellerman, Arminder Singh, linux-kernel
Cc: Sven Peter, Alyssa Rosenzweig, Hector Martin,
Benjamin Herrenschmidt, Paul Mackerras, linux-arm-kernel,
linuxppc-dev, linux-i2c, Darren Stevens, R.T.Dickinson,
Christian Zigotzky
On 23 August 2022 at 03:50 am, Michael Ellerman wrote:
> Arminder Singh <arminders208@outlook.com> writes:
>> 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've added Darren and Christian to Cc, they have helped with PASemi
> development and testing in the past, and may be able to help test this
> series on PASemi hardware.
>
> cheers
Tested-by: Christian Zigotzky <chzigotzky at xenosoft.de>
on an A-EON AmigaOne X1000 with a PASemi PWRficient PA6T-1682 processor.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon
2022-08-20 19:45 [PATCH] i2c: pasemi: Add IRQ support for Apple Silicon Arminder Singh
` (4 preceding siblings ...)
2022-08-23 1:50 ` Michael Ellerman
@ 2022-08-25 12:29 ` Arnd Bergmann
5 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2022-08-25 12:29 UTC (permalink / raw)
To: Arminder Singh
Cc: linux-kernel, Sven Peter, Alyssa Rosenzweig, Hector Martin,
Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
linux-arm-kernel, linuxppc-dev, linux-i2c
On Sat, Aug 20, 2022 at 9:45 PM Arminder Singh <arminders208@outlook.com> wrote:
>
> I also fixed a quick checkpatch warning on line 303. "i ++" is now "i++".
In general, anything that is mentioned in the changelog as "also done this"
is worth splitting into a separate patch, or dropping from the patch, to
make reviewing easier.
In this case, I would just not do the trivial change. Alternatively you
can consider doing a larger patch for coding style cleanup on the
patch, as there are possibly other issues as well. Usually it's not worth
changing things unless they hurt readability.
> @@ -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;
>
> + 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);
> }
>
> +
> /* Got NACK? */
> if (status & SMSTA_MTN)
> return -ENXIO;
...
> @@ -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;
> +}
I think the completion structure gets out of sync if you run into a
timeout here,
so a subsequent wait_for_completion will never complete after we missed one
interrupt.
Since this already causes a bus reset, I think you can just do
reinit_completion() at the end of pasemi_reset() to avoid this.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread