[2/2] fsi: Add IBM I2C Responder virtual FSI master
Commit Message
The I2C Responder (I2CR) is an I2C device that translates I2C commands
to CFAM or SCOM operations, effectively implementing an FSI master and
bus.
Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
drivers/fsi/Kconfig | 9 +
drivers/fsi/Makefile | 1 +
drivers/fsi/fsi-master-i2cr.c | 225 +++++++++++++++++++++++++
include/trace/events/fsi_master_i2cr.h | 96 +++++++++++
4 files changed, 331 insertions(+)
create mode 100644 drivers/fsi/fsi-master-i2cr.c
create mode 100644 include/trace/events/fsi_master_i2cr.h
Comments
On Fri, 20 Jan 2023, at 04:17, Eddie James wrote:
> The I2C Responder (I2CR) is an I2C device that translates I2C commands
> to CFAM or SCOM operations, effectively implementing an FSI master and
> bus.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
> drivers/fsi/Kconfig | 9 +
> drivers/fsi/Makefile | 1 +
> drivers/fsi/fsi-master-i2cr.c | 225 +++++++++++++++++++++++++
> include/trace/events/fsi_master_i2cr.h | 96 +++++++++++
> 4 files changed, 331 insertions(+)
> create mode 100644 drivers/fsi/fsi-master-i2cr.c
> create mode 100644 include/trace/events/fsi_master_i2cr.h
>
> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
> index e6668a869913..999be82720c5 100644
> --- a/drivers/fsi/Kconfig
> +++ b/drivers/fsi/Kconfig
> @@ -62,6 +62,15 @@ config FSI_MASTER_ASPEED
>
> Enable it for your BMC kernel in an OpenPower or IBM Power system.
>
> +config FSI_MASTER_I2CR
> + tristate "IBM I2C Responder virtual FSI master"
> + depends on I2C
> + help
> + This option enables a virtual FSI master in order to access a CFAM
> + behind an IBM I2C Responder (I2CR) chip. The I2CR is an I2C device
> + that translates I2C commands to CFAM or SCOM operations, effectively
> + implementing an FSI master and bus.
> +
> config FSI_SCOM
> tristate "SCOM FSI client device driver"
> help
> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
> index da218a1ad8e1..34dbaa1c452e 100644
> --- a/drivers/fsi/Makefile
> +++ b/drivers/fsi/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_FSI) += fsi-core.o
> obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
> obj-$(CONFIG_FSI_MASTER_ASPEED) += fsi-master-aspeed.o
> obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
> +obj-$(CONFIG_FSI_MASTER_I2CR) += fsi-master-i2cr.o
> obj-$(CONFIG_FSI_MASTER_AST_CF) += fsi-master-ast-cf.o
> obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
> obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
> diff --git a/drivers/fsi/fsi-master-i2cr.c
> b/drivers/fsi/fsi-master-i2cr.c
> new file mode 100644
> index 000000000000..d19ac96c0a83
> --- /dev/null
> +++ b/drivers/fsi/fsi-master-i2cr.c
> @@ -0,0 +1,225 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) IBM Corporation 2023 */
> +
> +#include <linux/device.h>
> +#include <linux/fsi.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +
> +#include "fsi-master.h"
> +
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/fsi_master_i2cr.h>
> +
> +#define I2CR_ADDRESS_CFAM(a) ((a) >> 2)
> +#define I2CR_STATUS 0x30001
> +#define I2CR_STATUS_ERR BIT_ULL(61)
> +#define I2CR_ERROR 0x30002
> +
> +struct fsi_master_i2cr {
> + struct fsi_master master;
> + struct mutex lock; /* protect HW access */
> + struct i2c_client *client;
> +};
> +
> +static bool i2cr_check_parity(u32 v, bool parity)
> +{
> + u32 i;
> +
> + for (i = 0; i < 32; ++i) {
> + if (v & (1 << i))
> + parity = !parity;
> + }
> +
> + return parity;
> +}
> +
> +static __be32 i2cr_get_command(u32 address, bool parity)
> +{
> + __be32 command;
> +
> + address <<= 1;
> +
> + if (i2cr_check_parity(address, parity))
> + address |= 1;
> +
> + command = cpu_to_be32(address);
> + trace_i2cr_command((__force uint32_t)command);
> +
> + return command;
> +}
> +
> +static int i2cr_transfer(struct i2c_client *client, u32 address,
> __be64 *data)
Is there a reason to use __be64 *data here and not `void *data, size_t
len`? We never actually use it as the declared type internally, only
cast it to __u8 *.
> +{
> + struct i2c_msg msgs[2];
> + __be32 command;
> + int ret;
> +
> + command = i2cr_get_command(address, true);
> + msgs[0].addr = client->addr;
> + msgs[0].flags = 0;
> + msgs[0].len = sizeof(command);
> + msgs[0].buf = (__u8 *)&command;
> + msgs[1].addr = client->addr;
> + msgs[1].flags = I2C_M_RD;
> + msgs[1].len = sizeof(*data);
> + msgs[1].buf = (__u8 *)data;
> +
> + ret = i2c_transfer(client->adapter, msgs, 2);
> + if (ret == 2)
> + return 0;
> +
> + trace_i2cr_i2c_error(ret);
> +
> + if (ret < 0)
> + return ret;
> +
> + return -EIO;
> +}
> +
> +static int i2cr_check_status(struct i2c_client *client)
> +{
> + __be64 status_be = 0;
> + u64 status;
> + int ret;
> +
> + ret = i2cr_transfer(client, I2CR_STATUS, &status_be);
> + if (ret)
> + return ret;
> +
> + status = be64_to_cpu(status_be);
> + if (status & I2CR_STATUS_ERR) {
> + __be64 error_be = 0;
> + u64 error;
> +
> + i2cr_transfer(client, I2CR_ERROR, &error_be);
> + error = be64_to_cpu(error_be);
> + trace_i2cr_status_error(status, error);
> + dev_err(&client->dev, "status:%016llx error:%016llx\n", status,
> error);
> + return -EREMOTEIO;
> + }
> +
> + trace_i2cr_status(status);
> + return 0;
> +}
> +
> +static int i2cr_read(struct fsi_master *master, int link, uint8_t id,
> uint32_t addr, void *val,
> + size_t size)
> +{
> + struct fsi_master_i2cr *i2cr = container_of(master, struct
> fsi_master_i2cr, master);
> + __be64 data = 0;
> + int ret;
> +
> + if (link || id || (addr & 0xffff0000) || !size || size > 4 || size ==
> 3)
These size constraints are a bit funky. Instead of `!size || size > 4 ||
size == 3` we write `!(size == 1 || size == 2 || size == 4)`?
> + return -EINVAL;
> +
> + mutex_lock(&i2cr->lock);
> +
> + ret = i2cr_transfer(i2cr->client, I2CR_ADDRESS_CFAM(addr), &data);
> + if (ret)
> + goto unlock;
> +
> + ret = i2cr_check_status(i2cr->client);
> + if (ret)
> + goto unlock;
> +
> + trace_i2cr_read(addr, size, (__force uint32_t)data);
> + memcpy(val, &data, size);
> +
> +unlock:
> + mutex_unlock(&i2cr->lock);
> + return ret;
> +}
> +
> +static int i2cr_write(struct fsi_master *master, int link, uint8_t id,
> uint32_t addr,
> + const void *val, size_t size)
> +{
> + struct fsi_master_i2cr *i2cr = container_of(master, struct
> fsi_master_i2cr, master);
> + __be32 data[3];
> + int ret;
> +
> + if (link || id || (addr & 0xffff0000) || !size || size > 4 || size ==
> 3)
As above
> + return -EINVAL;
> +
> + data[1] = 0;
> + memcpy(&data[1], val, size);
> + data[0] = i2cr_get_command(I2CR_ADDRESS_CFAM(addr),
> + i2cr_check_parity((__force u32)data[1], true));
> + data[2] = 0;
> +
> + mutex_lock(&i2cr->lock);
> +
> + ret = i2c_master_send(i2cr->client, (const char *)data, sizeof(data));
> + if (ret == sizeof(data)) {
> + ret = i2cr_check_status(i2cr->client);
> + if (!ret)
> + trace_i2cr_write(addr, size, (__force uint32_t)data[1]);
I think we can reduce the amount of __force if we flip the endianness
of the data variable?
```
u32 data[3];
__be32 cmd_be;
data[1] = 0;
memcpy(&data[1], val, size);
cmd_be = i2cr_get_command(I2CR_ADDRESS_CFAM(addr),
i2cr_check_parity(data[1], true));
data[0] = (__force u32)cmd_be;
data[2] = 0;
....
trace_i2cr_write(addr, size, data[1]);
```
?
Or define i2cr_check_parity() and the tracepoint in terms of big-endian?
> + } else {
> + trace_i2cr_i2c_error(ret);
> +
> + if (ret >= 0)
> + ret = -EIO;
> + }
> +
> + mutex_unlock(&i2cr->lock);
> + return ret;
> +}
> +
> +static int i2cr_probe(struct i2c_client *client)
> +{
> + struct fsi_master_i2cr *i2cr;
> + int ret;
> +
> + i2cr = devm_kzalloc(&client->dev, sizeof(*i2cr), GFP_KERNEL);
> + if (!i2cr)
> + return -ENOMEM;
> +
> + i2cr->master.dev.parent = &client->dev;
> + i2cr->master.dev.of_node = of_node_get(dev_of_node(&client->dev));
> +
> + i2cr->master.n_links = 1;
> + i2cr->master.read = i2cr_read;
> + i2cr->master.write = i2cr_write;
> +
> + mutex_init(&i2cr->lock);
> + i2cr->client = client;
> +
> + ret = fsi_master_register(&i2cr->master);
> + if (ret)
> + return ret;
> +
> + i2c_set_clientdata(client, i2cr);
> + return 0;
> +}
> +
> +static int i2cr_remove(struct i2c_client *client)
> +{
> + struct fsi_master_i2cr *i2cr = i2c_get_clientdata(client);
> +
> + fsi_master_unregister(&i2cr->master);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id i2cr_i2c_ids[] = {
> + { .compatible = "ibm,i2cr", },
This may need an update after discussion on the binding patch.
Andrew
Hi Eddie,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.2-rc4 next-20230120]
[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/Eddie-James/dt-bindings-fsi-Document-the-IBM-I2C-Responder-virtual-FSI-master/20230120-014831
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230119174714.1486042-3-eajames%40linux.ibm.com
patch subject: [PATCH 2/2] fsi: Add IBM I2C Responder virtual FSI master
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230120/202301201849.I88UJen6-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.1.0
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
# https://github.com/intel-lab-lkp/linux/commit/1312ba80b81ef02457d213ee6bc6ee80739c3e01
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Eddie-James/dt-bindings-fsi-Document-the-IBM-I2C-Responder-virtual-FSI-master/20230120-014831
git checkout 1312ba80b81ef02457d213ee6bc6ee80739c3e01
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/fsi/fsi-master-i2cr.c:214:19: error: initialization of 'void (*)(struct i2c_client *)' from incompatible pointer type 'int (*)(struct i2c_client *)' [-Werror=incompatible-pointer-types]
214 | .remove = i2cr_remove,
| ^~~~~~~~~~~
drivers/fsi/fsi-master-i2cr.c:214:19: note: (near initialization for 'i2cr_driver.remove')
cc1: some warnings being treated as errors
vim +214 drivers/fsi/fsi-master-i2cr.c
211
212 static struct i2c_driver i2cr_driver = {
213 .probe_new = i2cr_probe,
> 214 .remove = i2cr_remove,
215 .driver = {
216 .name = "i2cr",
217 .of_match_table = i2cr_i2c_ids,
218 },
219 };
220
Hi Eddie,
I love your patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.2-rc4 next-20230120]
[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/Eddie-James/dt-bindings-fsi-Document-the-IBM-I2C-Responder-virtual-FSI-master/20230120-014831
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230119174714.1486042-3-eajames%40linux.ibm.com
patch subject: [PATCH 2/2] fsi: Add IBM I2C Responder virtual FSI master
config: arm-randconfig-c002-20230120 (https://download.01.org/0day-ci/archive/20230121/202301210758.mY8JSNOf-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
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 arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/1312ba80b81ef02457d213ee6bc6ee80739c3e01
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Eddie-James/dt-bindings-fsi-Document-the-IBM-I2C-Responder-virtual-FSI-master/20230120-014831
git checkout 1312ba80b81ef02457d213ee6bc6ee80739c3e01
# 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=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/fsi/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/fsi/fsi-master-i2cr.c:214:12: error: incompatible function pointer types initializing 'void (*)(struct i2c_client *)' with an expression of type 'int (struct i2c_client *)' [-Wincompatible-function-pointer-types]
.remove = i2cr_remove,
^~~~~~~~~~~
1 error generated.
vim +214 drivers/fsi/fsi-master-i2cr.c
211
212 static struct i2c_driver i2cr_driver = {
213 .probe_new = i2cr_probe,
> 214 .remove = i2cr_remove,
215 .driver = {
216 .name = "i2cr",
217 .of_match_table = i2cr_i2c_ids,
218 },
219 };
220
On 1/19/23 19:09, Andrew Jeffery wrote:
>
> On Fri, 20 Jan 2023, at 04:17, Eddie James wrote:
>> The I2C Responder (I2CR) is an I2C device that translates I2C commands
>> to CFAM or SCOM operations, effectively implementing an FSI master and
>> bus.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>> drivers/fsi/Kconfig | 9 +
>> drivers/fsi/Makefile | 1 +
>> drivers/fsi/fsi-master-i2cr.c | 225 +++++++++++++++++++++++++
>> include/trace/events/fsi_master_i2cr.h | 96 +++++++++++
>> 4 files changed, 331 insertions(+)
>> create mode 100644 drivers/fsi/fsi-master-i2cr.c
>> create mode 100644 include/trace/events/fsi_master_i2cr.h
>>
>> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
>> index e6668a869913..999be82720c5 100644
>> --- a/drivers/fsi/Kconfig
>> +++ b/drivers/fsi/Kconfig
>> @@ -62,6 +62,15 @@ config FSI_MASTER_ASPEED
>>
>> Enable it for your BMC kernel in an OpenPower or IBM Power system.
>>
>> +config FSI_MASTER_I2CR
>> + tristate "IBM I2C Responder virtual FSI master"
>> + depends on I2C
>> + help
>> + This option enables a virtual FSI master in order to access a CFAM
>> + behind an IBM I2C Responder (I2CR) chip. The I2CR is an I2C device
>> + that translates I2C commands to CFAM or SCOM operations, effectively
>> + implementing an FSI master and bus.
>> +
>> config FSI_SCOM
>> tristate "SCOM FSI client device driver"
>> help
>> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
>> index da218a1ad8e1..34dbaa1c452e 100644
>> --- a/drivers/fsi/Makefile
>> +++ b/drivers/fsi/Makefile
>> @@ -4,6 +4,7 @@ obj-$(CONFIG_FSI) += fsi-core.o
>> obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
>> obj-$(CONFIG_FSI_MASTER_ASPEED) += fsi-master-aspeed.o
>> obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
>> +obj-$(CONFIG_FSI_MASTER_I2CR) += fsi-master-i2cr.o
>> obj-$(CONFIG_FSI_MASTER_AST_CF) += fsi-master-ast-cf.o
>> obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
>> obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
>> diff --git a/drivers/fsi/fsi-master-i2cr.c
>> b/drivers/fsi/fsi-master-i2cr.c
>> new file mode 100644
>> index 000000000000..d19ac96c0a83
>> --- /dev/null
>> +++ b/drivers/fsi/fsi-master-i2cr.c
>> @@ -0,0 +1,225 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (C) IBM Corporation 2023 */
>> +
>> +#include <linux/device.h>
>> +#include <linux/fsi.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +
>> +#include "fsi-master.h"
>> +
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/fsi_master_i2cr.h>
>> +
>> +#define I2CR_ADDRESS_CFAM(a) ((a) >> 2)
>> +#define I2CR_STATUS 0x30001
>> +#define I2CR_STATUS_ERR BIT_ULL(61)
>> +#define I2CR_ERROR 0x30002
>> +
>> +struct fsi_master_i2cr {
>> + struct fsi_master master;
>> + struct mutex lock; /* protect HW access */
>> + struct i2c_client *client;
>> +};
>> +
>> +static bool i2cr_check_parity(u32 v, bool parity)
>> +{
>> + u32 i;
>> +
>> + for (i = 0; i < 32; ++i) {
>> + if (v & (1 << i))
>> + parity = !parity;
>> + }
>> +
>> + return parity;
>> +}
>> +
>> +static __be32 i2cr_get_command(u32 address, bool parity)
>> +{
>> + __be32 command;
>> +
>> + address <<= 1;
>> +
>> + if (i2cr_check_parity(address, parity))
>> + address |= 1;
>> +
>> + command = cpu_to_be32(address);
>> + trace_i2cr_command((__force uint32_t)command);
>> +
>> + return command;
>> +}
>> +
>> +static int i2cr_transfer(struct i2c_client *client, u32 address,
>> __be64 *data)
> Is there a reason to use __be64 *data here and not `void *data, size_t
> len`? We never actually use it as the declared type internally, only
> cast it to __u8 *.
Well, its mostly to ensure the user buffer is at least 8 bytes. We have
to read 8 bytes of data, so passing in a length doesn't really make sense?
>
>> +{
>> + struct i2c_msg msgs[2];
>> + __be32 command;
>> + int ret;
>> +
>> + command = i2cr_get_command(address, true);
>> + msgs[0].addr = client->addr;
>> + msgs[0].flags = 0;
>> + msgs[0].len = sizeof(command);
>> + msgs[0].buf = (__u8 *)&command;
>> + msgs[1].addr = client->addr;
>> + msgs[1].flags = I2C_M_RD;
>> + msgs[1].len = sizeof(*data);
>> + msgs[1].buf = (__u8 *)data;
>> +
>> + ret = i2c_transfer(client->adapter, msgs, 2);
>> + if (ret == 2)
>> + return 0;
>> +
>> + trace_i2cr_i2c_error(ret);
>> +
>> + if (ret < 0)
>> + return ret;
>> +
>> + return -EIO;
>> +}
>> +
>> +static int i2cr_check_status(struct i2c_client *client)
>> +{
>> + __be64 status_be = 0;
>> + u64 status;
>> + int ret;
>> +
>> + ret = i2cr_transfer(client, I2CR_STATUS, &status_be);
>> + if (ret)
>> + return ret;
>> +
>> + status = be64_to_cpu(status_be);
>> + if (status & I2CR_STATUS_ERR) {
>> + __be64 error_be = 0;
>> + u64 error;
>> +
>> + i2cr_transfer(client, I2CR_ERROR, &error_be);
>> + error = be64_to_cpu(error_be);
>> + trace_i2cr_status_error(status, error);
>> + dev_err(&client->dev, "status:%016llx error:%016llx\n", status,
>> error);
>> + return -EREMOTEIO;
>> + }
>> +
>> + trace_i2cr_status(status);
>> + return 0;
>> +}
>> +
>> +static int i2cr_read(struct fsi_master *master, int link, uint8_t id,
>> uint32_t addr, void *val,
>> + size_t size)
>> +{
>> + struct fsi_master_i2cr *i2cr = container_of(master, struct
>> fsi_master_i2cr, master);
>> + __be64 data = 0;
>> + int ret;
>> +
>> + if (link || id || (addr & 0xffff0000) || !size || size > 4 || size ==
>> 3)
> These size constraints are a bit funky. Instead of `!size || size > 4 ||
> size == 3` we write `!(size == 1 || size == 2 || size == 4)`?
Good idea, thanks.
>
>> + return -EINVAL;
>> +
>> + mutex_lock(&i2cr->lock);
>> +
>> + ret = i2cr_transfer(i2cr->client, I2CR_ADDRESS_CFAM(addr), &data);
>> + if (ret)
>> + goto unlock;
>> +
>> + ret = i2cr_check_status(i2cr->client);
>> + if (ret)
>> + goto unlock;
>> +
>> + trace_i2cr_read(addr, size, (__force uint32_t)data);
>> + memcpy(val, &data, size);
>> +
>> +unlock:
>> + mutex_unlock(&i2cr->lock);
>> + return ret;
>> +}
>> +
>> +static int i2cr_write(struct fsi_master *master, int link, uint8_t id,
>> uint32_t addr,
>> + const void *val, size_t size)
>> +{
>> + struct fsi_master_i2cr *i2cr = container_of(master, struct
>> fsi_master_i2cr, master);
>> + __be32 data[3];
>> + int ret;
>> +
>> + if (link || id || (addr & 0xffff0000) || !size || size > 4 || size ==
>> 3)
> As above
>
>> + return -EINVAL;
>> +
>> + data[1] = 0;
>> + memcpy(&data[1], val, size);
>> + data[0] = i2cr_get_command(I2CR_ADDRESS_CFAM(addr),
>> + i2cr_check_parity((__force u32)data[1], true));
>> + data[2] = 0;
>> +
>> + mutex_lock(&i2cr->lock);
>> +
>> + ret = i2c_master_send(i2cr->client, (const char *)data, sizeof(data));
>> + if (ret == sizeof(data)) {
>> + ret = i2cr_check_status(i2cr->client);
>> + if (!ret)
>> + trace_i2cr_write(addr, size, (__force uint32_t)data[1]);
> I think we can reduce the amount of __force if we flip the endianness
> of the data variable?
>
> ```
> u32 data[3];
> __be32 cmd_be;
>
> data[1] = 0;
> memcpy(&data[1], val, size);
> cmd_be = i2cr_get_command(I2CR_ADDRESS_CFAM(addr),
> i2cr_check_parity(data[1], true));
> data[0] = (__force u32)cmd_be;
> data[2] = 0;
> ....
> trace_i2cr_write(addr, size, data[1]);
> ```
>
> ?
>
> Or define i2cr_check_parity() and the tracepoint in terms of big-endian?
I think I'll define a struct with the command as __be32 and the data as
u32. That should clean it up.
>
>> + } else {
>> + trace_i2cr_i2c_error(ret);
>> +
>> + if (ret >= 0)
>> + ret = -EIO;
>> + }
>> +
>> + mutex_unlock(&i2cr->lock);
>> + return ret;
>> +}
>> +
>> +static int i2cr_probe(struct i2c_client *client)
>> +{
>> + struct fsi_master_i2cr *i2cr;
>> + int ret;
>> +
>> + i2cr = devm_kzalloc(&client->dev, sizeof(*i2cr), GFP_KERNEL);
>> + if (!i2cr)
>> + return -ENOMEM;
>> +
>> + i2cr->master.dev.parent = &client->dev;
>> + i2cr->master.dev.of_node = of_node_get(dev_of_node(&client->dev));
>> +
>> + i2cr->master.n_links = 1;
>> + i2cr->master.read = i2cr_read;
>> + i2cr->master.write = i2cr_write;
>> +
>> + mutex_init(&i2cr->lock);
>> + i2cr->client = client;
>> +
>> + ret = fsi_master_register(&i2cr->master);
>> + if (ret)
>> + return ret;
>> +
>> + i2c_set_clientdata(client, i2cr);
>> + return 0;
>> +}
>> +
>> +static int i2cr_remove(struct i2c_client *client)
>> +{
>> + struct fsi_master_i2cr *i2cr = i2c_get_clientdata(client);
>> +
>> + fsi_master_unregister(&i2cr->master);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id i2cr_i2c_ids[] = {
>> + { .compatible = "ibm,i2cr", },
> This may need an update after discussion on the binding patch.
Yep.
Thanks for the review!
Eddie
>
> Andrew
@@ -62,6 +62,15 @@ config FSI_MASTER_ASPEED
Enable it for your BMC kernel in an OpenPower or IBM Power system.
+config FSI_MASTER_I2CR
+ tristate "IBM I2C Responder virtual FSI master"
+ depends on I2C
+ help
+ This option enables a virtual FSI master in order to access a CFAM
+ behind an IBM I2C Responder (I2CR) chip. The I2CR is an I2C device
+ that translates I2C commands to CFAM or SCOM operations, effectively
+ implementing an FSI master and bus.
+
config FSI_SCOM
tristate "SCOM FSI client device driver"
help
@@ -4,6 +4,7 @@ obj-$(CONFIG_FSI) += fsi-core.o
obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
obj-$(CONFIG_FSI_MASTER_ASPEED) += fsi-master-aspeed.o
obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
+obj-$(CONFIG_FSI_MASTER_I2CR) += fsi-master-i2cr.o
obj-$(CONFIG_FSI_MASTER_AST_CF) += fsi-master-ast-cf.o
obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
new file mode 100644
@@ -0,0 +1,225 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) IBM Corporation 2023 */
+
+#include <linux/device.h>
+#include <linux/fsi.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+
+#include "fsi-master.h"
+
+#define CREATE_TRACE_POINTS
+#include <trace/events/fsi_master_i2cr.h>
+
+#define I2CR_ADDRESS_CFAM(a) ((a) >> 2)
+#define I2CR_STATUS 0x30001
+#define I2CR_STATUS_ERR BIT_ULL(61)
+#define I2CR_ERROR 0x30002
+
+struct fsi_master_i2cr {
+ struct fsi_master master;
+ struct mutex lock; /* protect HW access */
+ struct i2c_client *client;
+};
+
+static bool i2cr_check_parity(u32 v, bool parity)
+{
+ u32 i;
+
+ for (i = 0; i < 32; ++i) {
+ if (v & (1 << i))
+ parity = !parity;
+ }
+
+ return parity;
+}
+
+static __be32 i2cr_get_command(u32 address, bool parity)
+{
+ __be32 command;
+
+ address <<= 1;
+
+ if (i2cr_check_parity(address, parity))
+ address |= 1;
+
+ command = cpu_to_be32(address);
+ trace_i2cr_command((__force uint32_t)command);
+
+ return command;
+}
+
+static int i2cr_transfer(struct i2c_client *client, u32 address, __be64 *data)
+{
+ struct i2c_msg msgs[2];
+ __be32 command;
+ int ret;
+
+ command = i2cr_get_command(address, true);
+ msgs[0].addr = client->addr;
+ msgs[0].flags = 0;
+ msgs[0].len = sizeof(command);
+ msgs[0].buf = (__u8 *)&command;
+ msgs[1].addr = client->addr;
+ msgs[1].flags = I2C_M_RD;
+ msgs[1].len = sizeof(*data);
+ msgs[1].buf = (__u8 *)data;
+
+ ret = i2c_transfer(client->adapter, msgs, 2);
+ if (ret == 2)
+ return 0;
+
+ trace_i2cr_i2c_error(ret);
+
+ if (ret < 0)
+ return ret;
+
+ return -EIO;
+}
+
+static int i2cr_check_status(struct i2c_client *client)
+{
+ __be64 status_be = 0;
+ u64 status;
+ int ret;
+
+ ret = i2cr_transfer(client, I2CR_STATUS, &status_be);
+ if (ret)
+ return ret;
+
+ status = be64_to_cpu(status_be);
+ if (status & I2CR_STATUS_ERR) {
+ __be64 error_be = 0;
+ u64 error;
+
+ i2cr_transfer(client, I2CR_ERROR, &error_be);
+ error = be64_to_cpu(error_be);
+ trace_i2cr_status_error(status, error);
+ dev_err(&client->dev, "status:%016llx error:%016llx\n", status, error);
+ return -EREMOTEIO;
+ }
+
+ trace_i2cr_status(status);
+ return 0;
+}
+
+static int i2cr_read(struct fsi_master *master, int link, uint8_t id, uint32_t addr, void *val,
+ size_t size)
+{
+ struct fsi_master_i2cr *i2cr = container_of(master, struct fsi_master_i2cr, master);
+ __be64 data = 0;
+ int ret;
+
+ if (link || id || (addr & 0xffff0000) || !size || size > 4 || size == 3)
+ return -EINVAL;
+
+ mutex_lock(&i2cr->lock);
+
+ ret = i2cr_transfer(i2cr->client, I2CR_ADDRESS_CFAM(addr), &data);
+ if (ret)
+ goto unlock;
+
+ ret = i2cr_check_status(i2cr->client);
+ if (ret)
+ goto unlock;
+
+ trace_i2cr_read(addr, size, (__force uint32_t)data);
+ memcpy(val, &data, size);
+
+unlock:
+ mutex_unlock(&i2cr->lock);
+ return ret;
+}
+
+static int i2cr_write(struct fsi_master *master, int link, uint8_t id, uint32_t addr,
+ const void *val, size_t size)
+{
+ struct fsi_master_i2cr *i2cr = container_of(master, struct fsi_master_i2cr, master);
+ __be32 data[3];
+ int ret;
+
+ if (link || id || (addr & 0xffff0000) || !size || size > 4 || size == 3)
+ return -EINVAL;
+
+ data[1] = 0;
+ memcpy(&data[1], val, size);
+ data[0] = i2cr_get_command(I2CR_ADDRESS_CFAM(addr),
+ i2cr_check_parity((__force u32)data[1], true));
+ data[2] = 0;
+
+ mutex_lock(&i2cr->lock);
+
+ ret = i2c_master_send(i2cr->client, (const char *)data, sizeof(data));
+ if (ret == sizeof(data)) {
+ ret = i2cr_check_status(i2cr->client);
+ if (!ret)
+ trace_i2cr_write(addr, size, (__force uint32_t)data[1]);
+ } else {
+ trace_i2cr_i2c_error(ret);
+
+ if (ret >= 0)
+ ret = -EIO;
+ }
+
+ mutex_unlock(&i2cr->lock);
+ return ret;
+}
+
+static int i2cr_probe(struct i2c_client *client)
+{
+ struct fsi_master_i2cr *i2cr;
+ int ret;
+
+ i2cr = devm_kzalloc(&client->dev, sizeof(*i2cr), GFP_KERNEL);
+ if (!i2cr)
+ return -ENOMEM;
+
+ i2cr->master.dev.parent = &client->dev;
+ i2cr->master.dev.of_node = of_node_get(dev_of_node(&client->dev));
+
+ i2cr->master.n_links = 1;
+ i2cr->master.read = i2cr_read;
+ i2cr->master.write = i2cr_write;
+
+ mutex_init(&i2cr->lock);
+ i2cr->client = client;
+
+ ret = fsi_master_register(&i2cr->master);
+ if (ret)
+ return ret;
+
+ i2c_set_clientdata(client, i2cr);
+ return 0;
+}
+
+static int i2cr_remove(struct i2c_client *client)
+{
+ struct fsi_master_i2cr *i2cr = i2c_get_clientdata(client);
+
+ fsi_master_unregister(&i2cr->master);
+
+ return 0;
+}
+
+static const struct of_device_id i2cr_i2c_ids[] = {
+ { .compatible = "ibm,i2cr", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, i2cr_i2c_ids);
+
+static struct i2c_driver i2cr_driver = {
+ .probe_new = i2cr_probe,
+ .remove = i2cr_remove,
+ .driver = {
+ .name = "i2cr",
+ .of_match_table = i2cr_i2c_ids,
+ },
+};
+
+module_i2c_driver(i2cr_driver)
+
+MODULE_AUTHOR("Eddie James <eajames@linux.ibm.com>");
+MODULE_DESCRIPTION("IBM I2C Responder virtual FSI master driver");
+MODULE_LICENSE("GPL");
new file mode 100644
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM fsi_master_i2cr
+
+#if !defined(_TRACE_FSI_MASTER_I2CR_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_FSI_MASTER_I2CR_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(i2cr_command,
+ TP_PROTO(uint32_t command),
+ TP_ARGS(command),
+ TP_STRUCT__entry(
+ __field(uint32_t, command)
+ ),
+ TP_fast_assign(
+ __entry->command = command;
+ ),
+ TP_printk("command:%08x", __entry->command)
+);
+
+TRACE_EVENT(i2cr_i2c_error,
+ TP_PROTO(int rc),
+ TP_ARGS(rc),
+ TP_STRUCT__entry(
+ __field(int, rc)
+ ),
+ TP_fast_assign(
+ __entry->rc = rc;
+ ),
+ TP_printk("rc:%d", __entry->rc)
+);
+
+TRACE_EVENT(i2cr_read,
+ TP_PROTO(uint32_t addr, size_t size, uint64_t result),
+ TP_ARGS(addr, size, result),
+ TP_STRUCT__entry(
+ __field(uint32_t, addr)
+ __field(size_t, size)
+ __field(uint64_t, result)
+ ),
+ TP_fast_assign(
+ __entry->addr = addr;
+ __entry->size = size;
+ __entry->result = result;
+ ),
+ TP_printk("addr:%08x size:%zu result:%016llx", __entry->addr, __entry->size,
+ __entry->result)
+);
+
+TRACE_EVENT(i2cr_status,
+ TP_PROTO(uint64_t status),
+ TP_ARGS(status),
+ TP_STRUCT__entry(
+ __field(uint32_t, status)
+ ),
+ TP_fast_assign(
+ __entry->status = status >> 32;
+ ),
+ TP_printk("status:%08x", __entry->status)
+);
+
+TRACE_EVENT(i2cr_status_error,
+ TP_PROTO(uint64_t status, uint64_t error),
+ TP_ARGS(status, error),
+ TP_STRUCT__entry(
+ __field(uint64_t, error)
+ __field(uint32_t, status)
+ ),
+ TP_fast_assign(
+ __entry->error = error;
+ __entry->status = status >> 32;
+ ),
+ TP_printk("status:%08x error:%016llx", __entry->status, __entry->error)
+);
+
+TRACE_EVENT(i2cr_write,
+ TP_PROTO(uint32_t addr, uint32_t val, size_t size),
+ TP_ARGS(addr, val, size),
+ TP_STRUCT__entry(
+ __field(uint32_t, addr)
+ __field(uint32_t, val)
+ __field(size_t, size)
+ ),
+ TP_fast_assign(
+ __entry->addr = addr;
+ __entry->val = val;
+ __entry->size = size;
+ ),
+ TP_printk("addr:%08x val:%08x size:%zu", __entry->addr, __entry->val, __entry->size)
+);
+
+#endif
+
+#include <trace/define_trace.h>