diff mbox series

[v4] drivers: ipmi: Support raw i2c packet in IPMB

Message ID 20191113234133.3790374-1-vijaykhemka@fb.com
State Not Applicable, archived
Headers show
Series [v4] drivers: ipmi: Support raw i2c packet in IPMB | expand

Commit Message

Vijay Khemka Nov. 13, 2019, 11:41 p.m. UTC 
Many IPMB devices doesn't support smbus protocol and current driver
support only smbus devices. Added support for raw i2c packets.

User can define i2c-protocol in device tree to use i2c raw transfer.

Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
 drivers/char/ipmi/ipmb_dev_int.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Corey Minyard Nov. 14, 2019, 2:10 p.m. UTC | #1 
On Wed, Nov 13, 2019 at 03:41:33PM -0800, Vijay Khemka wrote:
> Many IPMB devices doesn't support smbus protocol and current driver
> support only smbus devices. Added support for raw i2c packets.
> 
> User can define i2c-protocol in device tree to use i2c raw transfer.
> 
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>

With Aasma's response, I am ok with this.

One thing, though.  This is the slave device on the I2C, not the master
device that has the issue, right?  So it's at least theoretically
possible to have one SMBus and one I2C IPMB device on the same master,
right?

For normal I2C, devices are in the device tree and get added to the
kernel device handling.  It looks like that is not being done in your
case.  But really, the "right" way to do this is to have the IPMB slaves
added as Linux devices and address them that way.  I'm not sure this
will ever be a real issue, but if it is, there will be some work to do
to fix it.

-corey

> ---
>  drivers/char/ipmi/ipmb_dev_int.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
> index ae3bfba27526..10904bec1de0 100644
> --- a/drivers/char/ipmi/ipmb_dev_int.c
> +++ b/drivers/char/ipmi/ipmb_dev_int.c
> @@ -63,6 +63,7 @@ struct ipmb_dev {
>  	spinlock_t lock;
>  	wait_queue_head_t wait_queue;
>  	struct mutex file_mutex;
> +	bool is_i2c_protocol;
>  };
>  
>  static inline struct ipmb_dev *to_ipmb_dev(struct file *file)
> @@ -112,6 +113,25 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
>  	return ret < 0 ? ret : count;
>  }
>  
> +static int ipmb_i2c_write(struct i2c_client *client, u8 *msg)
> +{
> +	struct i2c_msg i2c_msg;
> +
> +	/*
> +	 * subtract 1 byte (rq_sa) from the length of the msg passed to
> +	 * raw i2c_transfer
> +	 */
> +	i2c_msg.len = msg[IPMB_MSG_LEN_IDX] - 1;
> +
> +	/* Assign message to buffer except first 2 bytes (length and address) */
> +	i2c_msg.buf = msg + 2;
> +
> +	i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
> +	i2c_msg.flags = client->flags & I2C_CLIENT_PEC;
> +
> +	return i2c_transfer(client->adapter, &i2c_msg, 1);
> +}
> +
>  static ssize_t ipmb_write(struct file *file, const char __user *buf,
>  			size_t count, loff_t *ppos)
>  {
> @@ -133,6 +153,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
>  	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
>  	netf_rq_lun = msg[NETFN_LUN_IDX];
>  
> +	/* Check i2c block transfer vs smbus */
> +	if (ipmb_dev->is_i2c_protocol) {
> +		ret = ipmb_i2c_write(ipmb_dev->client, msg);
> +		return (ret == 1) ? count : ret;
> +	}
> +
>  	/*
>  	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
>  	 * i2c_smbus_xfer
> @@ -277,6 +303,7 @@ static int ipmb_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
>  	struct ipmb_dev *ipmb_dev;
> +	struct device_node *np;
>  	int ret;
>  
>  	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev),
> @@ -302,6 +329,11 @@ static int ipmb_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
>  
> +	/* Check if i2c block xmit needs to use instead of smbus */
> +	np = client->dev.of_node;
> +	if (np && of_get_property(np, "i2c-protocol", NULL))
> +		ipmb_dev->is_i2c_protocol = true;
> +
>  	ipmb_dev->client = client;
>  	i2c_set_clientdata(client, ipmb_dev);
>  	ret = i2c_slave_register(client, ipmb_slave_cb);
> -- 
> 2.17.1
>
Asmaa Mnebhi Nov. 14, 2019, 2:16 p.m. UTC | #2 
Hi Vijay,

You haven't addressed some of my comments.

-----Original Message-----
From: Vijay Khemka <vijaykhemka@fb.com> 
Sent: Wednesday, November 13, 2019 6:42 PM
To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org
Cc: vijaykhemka@fb.com; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.com
Subject: [PATCH v4] drivers: ipmi: Support raw i2c packet in IPMB

Many IPMB devices doesn't support smbus protocol and current driver support only smbus devices. Added support for raw i2c packets.

User can define i2c-protocol in device tree to use i2c raw transfer.

A>> Please fix the description as suggested in previous comments

Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
 drivers/char/ipmi/ipmb_dev_int.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
index ae3bfba27526..10904bec1de0 100644
--- a/drivers/char/ipmi/ipmb_dev_int.c
+++ b/drivers/char/ipmi/ipmb_dev_int.c
@@ -63,6 +63,7 @@ struct ipmb_dev {
 	spinlock_t lock;
 	wait_queue_head_t wait_queue;
 	struct mutex file_mutex;
+	bool is_i2c_protocol;
 };
 
 static inline struct ipmb_dev *to_ipmb_dev(struct file *file) @@ -112,6 +113,25 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
 	return ret < 0 ? ret : count;
 }
 
+static int ipmb_i2c_write(struct i2c_client *client, u8 *msg) {
+	struct i2c_msg i2c_msg;
+
+	/*
+	 * subtract 1 byte (rq_sa) from the length of the msg passed to
+	 * raw i2c_transfer
+	 */
+	i2c_msg.len = msg[IPMB_MSG_LEN_IDX] - 1;
+
+	/* Assign message to buffer except first 2 bytes (length and address) */
+	i2c_msg.buf = msg + 2;
+
+	i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);

You can have:
i2c_msg.flags = addr;
addr being an argument of the ipmb_i2c_write function and passed in ipmb_write. We already define it.

+	i2c_msg.flags = client->flags & I2C_CLIENT_PEC;
+
+	return i2c_transfer(client->adapter, &i2c_msg, 1); }
+
 static ssize_t ipmb_write(struct file *file, const char __user *buf,
 			size_t count, loff_t *ppos)
 {
@@ -133,6 +153,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
 	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
 	netf_rq_lun = msg[NETFN_LUN_IDX];
 
+	/* Check i2c block transfer vs smbus */
+	if (ipmb_dev->is_i2c_protocol) {
+		ret = ipmb_i2c_write(ipmb_dev->client, msg);
+		return (ret == 1) ? count : ret;
+	}
+
 	/*
 	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
 	 * i2c_smbus_xfer
@@ -277,6 +303,7 @@ static int ipmb_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct ipmb_dev *ipmb_dev;
+	struct device_node *np;
 	int ret;
 
 	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev), @@ -302,6 +329,11 @@ static int ipmb_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	/* Check if i2c block xmit needs to use instead of smbus */
+	np = client->dev.of_node;
+	if (np && of_get_property(np, "i2c-protocol", NULL))
+		ipmb_dev->is_i2c_protocol = true;

I know Corey said that ACPI is not a priority but many companies (including mine) use ACPI and I would prefer if we implement it. All you need to do is use
device_property_read_u32 function instead of of_get_property:
ret = device_property_read_u32(&client->dev, "i2c-protocol", &ipmb_dev->is_i2c_protocol);
This function does the job for both dt and acpi.

+
 	ipmb_dev->client = client;
 	i2c_set_clientdata(client, ipmb_dev);
 	ret = i2c_slave_register(client, ipmb_slave_cb);
--
2.17.1
Asmaa Mnebhi Nov. 14, 2019, 2:21 p.m. UTC | #3 
Vijay, could you update the existing ipmb documentation to list and describe this new device tree/acpi variable i2c-protocol.

-----Original Message-----
From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard
Sent: Thursday, November 14, 2019 9:11 AM
To: Vijay Khemka <vijaykhemka@fb.com>
Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.com
Subject: Re: [PATCH v4] drivers: ipmi: Support raw i2c packet in IPMB

On Wed, Nov 13, 2019 at 03:41:33PM -0800, Vijay Khemka wrote:
> Many IPMB devices doesn't support smbus protocol and current driver 
> support only smbus devices. Added support for raw i2c packets.
> 
> User can define i2c-protocol in device tree to use i2c raw transfer.
> 
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>

With Aasma's response, I am ok with this.

One thing, though.  This is the slave device on the I2C, not the master device that has the issue, right?  So it's at least theoretically possible to have one SMBus and one I2C IPMB device on the same master, right?

For normal I2C, devices are in the device tree and get added to the kernel device handling.  It looks like that is not being done in your case.  But really, the "right" way to do this is to have the IPMB slaves added as Linux devices and address them that way.  I'm not sure this will ever be a real issue, but if it is, there will be some work to do to fix it.

-corey

> ---
>  drivers/char/ipmi/ipmb_dev_int.c | 32 
> ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c 
> b/drivers/char/ipmi/ipmb_dev_int.c
> index ae3bfba27526..10904bec1de0 100644
> --- a/drivers/char/ipmi/ipmb_dev_int.c
> +++ b/drivers/char/ipmi/ipmb_dev_int.c
> @@ -63,6 +63,7 @@ struct ipmb_dev {
>  	spinlock_t lock;
>  	wait_queue_head_t wait_queue;
>  	struct mutex file_mutex;
> +	bool is_i2c_protocol;
>  };
>  
>  static inline struct ipmb_dev *to_ipmb_dev(struct file *file) @@ 
> -112,6 +113,25 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
>  	return ret < 0 ? ret : count;
>  }
>  
> +static int ipmb_i2c_write(struct i2c_client *client, u8 *msg) {
> +	struct i2c_msg i2c_msg;
> +
> +	/*
> +	 * subtract 1 byte (rq_sa) from the length of the msg passed to
> +	 * raw i2c_transfer
> +	 */
> +	i2c_msg.len = msg[IPMB_MSG_LEN_IDX] - 1;
> +
> +	/* Assign message to buffer except first 2 bytes (length and address) */
> +	i2c_msg.buf = msg + 2;
> +
> +	i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
> +	i2c_msg.flags = client->flags & I2C_CLIENT_PEC;
> +
> +	return i2c_transfer(client->adapter, &i2c_msg, 1); }
> +
>  static ssize_t ipmb_write(struct file *file, const char __user *buf,
>  			size_t count, loff_t *ppos)
>  {
> @@ -133,6 +153,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
>  	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
>  	netf_rq_lun = msg[NETFN_LUN_IDX];
>  
> +	/* Check i2c block transfer vs smbus */
> +	if (ipmb_dev->is_i2c_protocol) {
> +		ret = ipmb_i2c_write(ipmb_dev->client, msg);
> +		return (ret == 1) ? count : ret;
> +	}
> +
>  	/*
>  	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
>  	 * i2c_smbus_xfer
> @@ -277,6 +303,7 @@ static int ipmb_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
>  	struct ipmb_dev *ipmb_dev;
> +	struct device_node *np;
>  	int ret;
>  
>  	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev), @@ -302,6 
> +329,11 @@ static int ipmb_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
>  
> +	/* Check if i2c block xmit needs to use instead of smbus */
> +	np = client->dev.of_node;
> +	if (np && of_get_property(np, "i2c-protocol", NULL))
> +		ipmb_dev->is_i2c_protocol = true;
> +
>  	ipmb_dev->client = client;
>  	i2c_set_clientdata(client, ipmb_dev);
>  	ret = i2c_slave_register(client, ipmb_slave_cb);
> --
> 2.17.1
>
Vijay Khemka Nov. 14, 2019, 6:02 p.m. UTC | #4 
Yes Asmaa, That was already in my plan. Do you want to include it in the same patch or separate patch.

On 11/14/19, 6:21 AM, "Asmaa Mnebhi" <asmaa@mellanox.com> wrote:

    Vijay, could you update the existing ipmb documentation to list and describe this new device tree/acpi variable i2c-protocol.
    
    -----Original Message-----
    From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard
    Sent: Thursday, November 14, 2019 9:11 AM
    To: Vijay Khemka <vijaykhemka@fb.com>
    Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.com
    Subject: Re: [PATCH v4] drivers: ipmi: Support raw i2c packet in IPMB
    
    On Wed, Nov 13, 2019 at 03:41:33PM -0800, Vijay Khemka wrote:
    > Many IPMB devices doesn't support smbus protocol and current driver 
    > support only smbus devices. Added support for raw i2c packets.
    > 
    > User can define i2c-protocol in device tree to use i2c raw transfer.
    > 
    > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
    
    With Aasma's response, I am ok with this.
    
    One thing, though.  This is the slave device on the I2C, not the master device that has the issue, right?  So it's at least theoretically possible to have one SMBus and one I2C IPMB device on the same master, right?
    
    For normal I2C, devices are in the device tree and get added to the kernel device handling.  It looks like that is not being done in your case.  But really, the "right" way to do this is to have the IPMB slaves added as Linux devices and address them that way.  I'm not sure this will ever be a real issue, but if it is, there will be some work to do to fix it.
    
    -corey
    
    > ---
    >  drivers/char/ipmi/ipmb_dev_int.c | 32 
    > ++++++++++++++++++++++++++++++++
    >  1 file changed, 32 insertions(+)
    > 
    > diff --git a/drivers/char/ipmi/ipmb_dev_int.c 
    > b/drivers/char/ipmi/ipmb_dev_int.c
    > index ae3bfba27526..10904bec1de0 100644
    > --- a/drivers/char/ipmi/ipmb_dev_int.c
    > +++ b/drivers/char/ipmi/ipmb_dev_int.c
    > @@ -63,6 +63,7 @@ struct ipmb_dev {
    >  	spinlock_t lock;
    >  	wait_queue_head_t wait_queue;
    >  	struct mutex file_mutex;
    > +	bool is_i2c_protocol;
    >  };
    >  
    >  static inline struct ipmb_dev *to_ipmb_dev(struct file *file) @@ 
    > -112,6 +113,25 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
    >  	return ret < 0 ? ret : count;
    >  }
    >  
    > +static int ipmb_i2c_write(struct i2c_client *client, u8 *msg) {
    > +	struct i2c_msg i2c_msg;
    > +
    > +	/*
    > +	 * subtract 1 byte (rq_sa) from the length of the msg passed to
    > +	 * raw i2c_transfer
    > +	 */
    > +	i2c_msg.len = msg[IPMB_MSG_LEN_IDX] - 1;
    > +
    > +	/* Assign message to buffer except first 2 bytes (length and address) */
    > +	i2c_msg.buf = msg + 2;
    > +
    > +	i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
    > +	i2c_msg.flags = client->flags & I2C_CLIENT_PEC;
    > +
    > +	return i2c_transfer(client->adapter, &i2c_msg, 1); }
    > +
    >  static ssize_t ipmb_write(struct file *file, const char __user *buf,
    >  			size_t count, loff_t *ppos)
    >  {
    > @@ -133,6 +153,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
    >  	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
    >  	netf_rq_lun = msg[NETFN_LUN_IDX];
    >  
    > +	/* Check i2c block transfer vs smbus */
    > +	if (ipmb_dev->is_i2c_protocol) {
    > +		ret = ipmb_i2c_write(ipmb_dev->client, msg);
    > +		return (ret == 1) ? count : ret;
    > +	}
    > +
    >  	/*
    >  	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
    >  	 * i2c_smbus_xfer
    > @@ -277,6 +303,7 @@ static int ipmb_probe(struct i2c_client *client,
    >  			const struct i2c_device_id *id)
    >  {
    >  	struct ipmb_dev *ipmb_dev;
    > +	struct device_node *np;
    >  	int ret;
    >  
    >  	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev), @@ -302,6 
    > +329,11 @@ static int ipmb_probe(struct i2c_client *client,
    >  	if (ret)
    >  		return ret;
    >  
    > +	/* Check if i2c block xmit needs to use instead of smbus */
    > +	np = client->dev.of_node;
    > +	if (np && of_get_property(np, "i2c-protocol", NULL))
    > +		ipmb_dev->is_i2c_protocol = true;
    > +
    >  	ipmb_dev->client = client;
    >  	i2c_set_clientdata(client, ipmb_dev);
    >  	ret = i2c_slave_register(client, ipmb_slave_cb);
    > --
    > 2.17.1
    >
Vijay Khemka Nov. 14, 2019, 6:03 p.m. UTC | #5 
Ok I will send another patch with acpi support.

On 11/14/19, 6:16 AM, "Asmaa Mnebhi" <asmaa@mellanox.com> wrote:

    Hi Vijay,
    
    You haven't addressed some of my comments.
    
    -----Original Message-----
    From: Vijay Khemka <vijaykhemka@fb.com> 
    Sent: Wednesday, November 13, 2019 6:42 PM
    To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org
    Cc: vijaykhemka@fb.com; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.com
    Subject: [PATCH v4] drivers: ipmi: Support raw i2c packet in IPMB
    
    Many IPMB devices doesn't support smbus protocol and current driver support only smbus devices. Added support for raw i2c packets.
    
    User can define i2c-protocol in device tree to use i2c raw transfer.
    
    A>> Please fix the description as suggested in previous comments
    
    Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
    ---
     drivers/char/ipmi/ipmb_dev_int.c | 32 ++++++++++++++++++++++++++++++++
     1 file changed, 32 insertions(+)
    
    diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
    index ae3bfba27526..10904bec1de0 100644
    --- a/drivers/char/ipmi/ipmb_dev_int.c
    +++ b/drivers/char/ipmi/ipmb_dev_int.c
    @@ -63,6 +63,7 @@ struct ipmb_dev {
     	spinlock_t lock;
     	wait_queue_head_t wait_queue;
     	struct mutex file_mutex;
    +	bool is_i2c_protocol;
     };
     
     static inline struct ipmb_dev *to_ipmb_dev(struct file *file) @@ -112,6 +113,25 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
     	return ret < 0 ? ret : count;
     }
     
    +static int ipmb_i2c_write(struct i2c_client *client, u8 *msg) {
    +	struct i2c_msg i2c_msg;
    +
    +	/*
    +	 * subtract 1 byte (rq_sa) from the length of the msg passed to
    +	 * raw i2c_transfer
    +	 */
    +	i2c_msg.len = msg[IPMB_MSG_LEN_IDX] - 1;
    +
    +	/* Assign message to buffer except first 2 bytes (length and address) */
    +	i2c_msg.buf = msg + 2;
    +
    +	i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
    
    You can have:
    i2c_msg.flags = addr;
    addr being an argument of the ipmb_i2c_write function and passed in ipmb_write. We already define it.
    
    +	i2c_msg.flags = client->flags & I2C_CLIENT_PEC;
    +
    +	return i2c_transfer(client->adapter, &i2c_msg, 1); }
    +
     static ssize_t ipmb_write(struct file *file, const char __user *buf,
     			size_t count, loff_t *ppos)
     {
    @@ -133,6 +153,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
     	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
     	netf_rq_lun = msg[NETFN_LUN_IDX];
     
    +	/* Check i2c block transfer vs smbus */
    +	if (ipmb_dev->is_i2c_protocol) {
    +		ret = ipmb_i2c_write(ipmb_dev->client, msg);
    +		return (ret == 1) ? count : ret;
    +	}
    +
     	/*
     	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
     	 * i2c_smbus_xfer
    @@ -277,6 +303,7 @@ static int ipmb_probe(struct i2c_client *client,
     			const struct i2c_device_id *id)
     {
     	struct ipmb_dev *ipmb_dev;
    +	struct device_node *np;
     	int ret;
     
     	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev), @@ -302,6 +329,11 @@ static int ipmb_probe(struct i2c_client *client,
     	if (ret)
     		return ret;
     
    +	/* Check if i2c block xmit needs to use instead of smbus */
    +	np = client->dev.of_node;
    +	if (np && of_get_property(np, "i2c-protocol", NULL))
    +		ipmb_dev->is_i2c_protocol = true;
    
    I know Corey said that ACPI is not a priority but many companies (including mine) use ACPI and I would prefer if we implement it. All you need to do is use
    device_property_read_u32 function instead of of_get_property:
    ret = device_property_read_u32(&client->dev, "i2c-protocol", &ipmb_dev->is_i2c_protocol);
    This function does the job for both dt and acpi.
    
    +
     	ipmb_dev->client = client;
     	i2c_set_clientdata(client, ipmb_dev);
     	ret = i2c_slave_register(client, ipmb_slave_cb);
    --
    2.17.1
Vijay Khemka Nov. 14, 2019, 6:05 p.m. UTC | #6 
On 11/14/19, 6:11 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote:

    On Wed, Nov 13, 2019 at 03:41:33PM -0800, Vijay Khemka wrote:
    > Many IPMB devices doesn't support smbus protocol and current driver
    > support only smbus devices. Added support for raw i2c packets.
    > 
    > User can define i2c-protocol in device tree to use i2c raw transfer.
    > 
    > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
    
    With Aasma's response, I am ok with this.
    
    One thing, though.  This is the slave device on the I2C, not the master
    device that has the issue, right?  So it's at least theoretically
    possible to have one SMBus and one I2C IPMB device on the same master,
    right?
    
    For normal I2C, devices are in the device tree and get added to the
    kernel device handling.  It looks like that is not being done in your
    case.  But really, the "right" way to do this is to have the IPMB slaves
    added as Linux devices and address them that way.  I'm not sure this
    will ever be a real issue, but if it is, there will be some work to do
    to fix it.

I will be happy to address this in follow up patch and need some more info.
    
    -corey
    
    > ---
    >  drivers/char/ipmi/ipmb_dev_int.c | 32 ++++++++++++++++++++++++++++++++
    >  1 file changed, 32 insertions(+)
    > 
    > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
    > index ae3bfba27526..10904bec1de0 100644
    > --- a/drivers/char/ipmi/ipmb_dev_int.c
    > +++ b/drivers/char/ipmi/ipmb_dev_int.c
    > @@ -63,6 +63,7 @@ struct ipmb_dev {
    >  	spinlock_t lock;
    >  	wait_queue_head_t wait_queue;
    >  	struct mutex file_mutex;
    > +	bool is_i2c_protocol;
    >  };
    >  
    >  static inline struct ipmb_dev *to_ipmb_dev(struct file *file)
    > @@ -112,6 +113,25 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
    >  	return ret < 0 ? ret : count;
    >  }
    >  
    > +static int ipmb_i2c_write(struct i2c_client *client, u8 *msg)
    > +{
    > +	struct i2c_msg i2c_msg;
    > +
    > +	/*
    > +	 * subtract 1 byte (rq_sa) from the length of the msg passed to
    > +	 * raw i2c_transfer
    > +	 */
    > +	i2c_msg.len = msg[IPMB_MSG_LEN_IDX] - 1;
    > +
    > +	/* Assign message to buffer except first 2 bytes (length and address) */
    > +	i2c_msg.buf = msg + 2;
    > +
    > +	i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
    > +	i2c_msg.flags = client->flags & I2C_CLIENT_PEC;
    > +
    > +	return i2c_transfer(client->adapter, &i2c_msg, 1);
    > +}
    > +
    >  static ssize_t ipmb_write(struct file *file, const char __user *buf,
    >  			size_t count, loff_t *ppos)
    >  {
    > @@ -133,6 +153,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
    >  	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
    >  	netf_rq_lun = msg[NETFN_LUN_IDX];
    >  
    > +	/* Check i2c block transfer vs smbus */
    > +	if (ipmb_dev->is_i2c_protocol) {
    > +		ret = ipmb_i2c_write(ipmb_dev->client, msg);
    > +		return (ret == 1) ? count : ret;
    > +	}
    > +
    >  	/*
    >  	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
    >  	 * i2c_smbus_xfer
    > @@ -277,6 +303,7 @@ static int ipmb_probe(struct i2c_client *client,
    >  			const struct i2c_device_id *id)
    >  {
    >  	struct ipmb_dev *ipmb_dev;
    > +	struct device_node *np;
    >  	int ret;
    >  
    >  	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev),
    > @@ -302,6 +329,11 @@ static int ipmb_probe(struct i2c_client *client,
    >  	if (ret)
    >  		return ret;
    >  
    > +	/* Check if i2c block xmit needs to use instead of smbus */
    > +	np = client->dev.of_node;
    > +	if (np && of_get_property(np, "i2c-protocol", NULL))
    > +		ipmb_dev->is_i2c_protocol = true;
    > +
    >  	ipmb_dev->client = client;
    >  	i2c_set_clientdata(client, ipmb_dev);
    >  	ret = i2c_slave_register(client, ipmb_slave_cb);
    > -- 
    > 2.17.1
    >
Asmaa Mnebhi Nov. 14, 2019, 6:17 p.m. UTC | #7 
Same patch would be good.

-----Original Message-----
From: Vijay Khemka <vijaykhemka@fb.com> 
Sent: Thursday, November 14, 2019 1:03 PM
To: Asmaa Mnebhi <Asmaa@mellanox.com>; minyard@acm.org
Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; Sai Dasari <sdasari@fb.com>
Subject: Re: [PATCH v4] drivers: ipmi: Support raw i2c packet in IPMB

Yes Asmaa, That was already in my plan. Do you want to include it in the same patch or separate patch.

On 11/14/19, 6:21 AM, "Asmaa Mnebhi" <asmaa@mellanox.com> wrote:

    Vijay, could you update the existing ipmb documentation to list and describe this new device tree/acpi variable i2c-protocol.
    
    -----Original Message-----
    From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard
    Sent: Thursday, November 14, 2019 9:11 AM
    To: Vijay Khemka <vijaykhemka@fb.com>
    Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.com
    Subject: Re: [PATCH v4] drivers: ipmi: Support raw i2c packet in IPMB
    
    On Wed, Nov 13, 2019 at 03:41:33PM -0800, Vijay Khemka wrote:
    > Many IPMB devices doesn't support smbus protocol and current driver 
    > support only smbus devices. Added support for raw i2c packets.
    > 
    > User can define i2c-protocol in device tree to use i2c raw transfer.
    > 
    > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
    
    With Aasma's response, I am ok with this.
    
    One thing, though.  This is the slave device on the I2C, not the master device that has the issue, right?  So it's at least theoretically possible to have one SMBus and one I2C IPMB device on the same master, right?
    
    For normal I2C, devices are in the device tree and get added to the kernel device handling.  It looks like that is not being done in your case.  But really, the "right" way to do this is to have the IPMB slaves added as Linux devices and address them that way.  I'm not sure this will ever be a real issue, but if it is, there will be some work to do to fix it.
    
    -corey
    
    > ---
    >  drivers/char/ipmi/ipmb_dev_int.c | 32 
    > ++++++++++++++++++++++++++++++++
    >  1 file changed, 32 insertions(+)
    > 
    > diff --git a/drivers/char/ipmi/ipmb_dev_int.c 
    > b/drivers/char/ipmi/ipmb_dev_int.c
    > index ae3bfba27526..10904bec1de0 100644
    > --- a/drivers/char/ipmi/ipmb_dev_int.c
    > +++ b/drivers/char/ipmi/ipmb_dev_int.c
    > @@ -63,6 +63,7 @@ struct ipmb_dev {
    >  	spinlock_t lock;
    >  	wait_queue_head_t wait_queue;
    >  	struct mutex file_mutex;
    > +	bool is_i2c_protocol;
    >  };
    >  
    >  static inline struct ipmb_dev *to_ipmb_dev(struct file *file) @@ 
    > -112,6 +113,25 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
    >  	return ret < 0 ? ret : count;
    >  }
    >  
    > +static int ipmb_i2c_write(struct i2c_client *client, u8 *msg) {
    > +	struct i2c_msg i2c_msg;
    > +
    > +	/*
    > +	 * subtract 1 byte (rq_sa) from the length of the msg passed to
    > +	 * raw i2c_transfer
    > +	 */
    > +	i2c_msg.len = msg[IPMB_MSG_LEN_IDX] - 1;
    > +
    > +	/* Assign message to buffer except first 2 bytes (length and address) */
    > +	i2c_msg.buf = msg + 2;
    > +
    > +	i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
    > +	i2c_msg.flags = client->flags & I2C_CLIENT_PEC;
    > +
    > +	return i2c_transfer(client->adapter, &i2c_msg, 1); }
    > +
    >  static ssize_t ipmb_write(struct file *file, const char __user *buf,
    >  			size_t count, loff_t *ppos)
    >  {
    > @@ -133,6 +153,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
    >  	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
    >  	netf_rq_lun = msg[NETFN_LUN_IDX];
    >  
    > +	/* Check i2c block transfer vs smbus */
    > +	if (ipmb_dev->is_i2c_protocol) {
    > +		ret = ipmb_i2c_write(ipmb_dev->client, msg);
    > +		return (ret == 1) ? count : ret;
    > +	}
    > +
    >  	/*
    >  	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
    >  	 * i2c_smbus_xfer
    > @@ -277,6 +303,7 @@ static int ipmb_probe(struct i2c_client *client,
    >  			const struct i2c_device_id *id)
    >  {
    >  	struct ipmb_dev *ipmb_dev;
    > +	struct device_node *np;
    >  	int ret;
    >  
    >  	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev), @@ -302,6 
    > +329,11 @@ static int ipmb_probe(struct i2c_client *client,
    >  	if (ret)
    >  		return ret;
    >  
    > +	/* Check if i2c block xmit needs to use instead of smbus */
    > +	np = client->dev.of_node;
    > +	if (np && of_get_property(np, "i2c-protocol", NULL))
    > +		ipmb_dev->is_i2c_protocol = true;
    > +
    >  	ipmb_dev->client = client;
    >  	i2c_set_clientdata(client, ipmb_dev);
    >  	ret = i2c_slave_register(client, ipmb_slave_cb);
    > --
    > 2.17.1
    >
diff mbox series

Patch

diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
index ae3bfba27526..10904bec1de0 100644
--- a/drivers/char/ipmi/ipmb_dev_int.c
+++ b/drivers/char/ipmi/ipmb_dev_int.c
@@ -63,6 +63,7 @@  struct ipmb_dev {
 	spinlock_t lock;
 	wait_queue_head_t wait_queue;
 	struct mutex file_mutex;
+	bool is_i2c_protocol;
 };
 
 static inline struct ipmb_dev *to_ipmb_dev(struct file *file)
@@ -112,6 +113,25 @@  static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
 	return ret < 0 ? ret : count;
 }
 
+static int ipmb_i2c_write(struct i2c_client *client, u8 *msg)
+{
+	struct i2c_msg i2c_msg;
+
+	/*
+	 * subtract 1 byte (rq_sa) from the length of the msg passed to
+	 * raw i2c_transfer
+	 */
+	i2c_msg.len = msg[IPMB_MSG_LEN_IDX] - 1;
+
+	/* Assign message to buffer except first 2 bytes (length and address) */
+	i2c_msg.buf = msg + 2;
+
+	i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
+	i2c_msg.flags = client->flags & I2C_CLIENT_PEC;
+
+	return i2c_transfer(client->adapter, &i2c_msg, 1);
+}
+
 static ssize_t ipmb_write(struct file *file, const char __user *buf,
 			size_t count, loff_t *ppos)
 {
@@ -133,6 +153,12 @@  static ssize_t ipmb_write(struct file *file, const char __user *buf,
 	rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
 	netf_rq_lun = msg[NETFN_LUN_IDX];
 
+	/* Check i2c block transfer vs smbus */
+	if (ipmb_dev->is_i2c_protocol) {
+		ret = ipmb_i2c_write(ipmb_dev->client, msg);
+		return (ret == 1) ? count : ret;
+	}
+
 	/*
 	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
 	 * i2c_smbus_xfer
@@ -277,6 +303,7 @@  static int ipmb_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct ipmb_dev *ipmb_dev;
+	struct device_node *np;
 	int ret;
 
 	ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev),
@@ -302,6 +329,11 @@  static int ipmb_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	/* Check if i2c block xmit needs to use instead of smbus */
+	np = client->dev.of_node;
+	if (np && of_get_property(np, "i2c-protocol", NULL))
+		ipmb_dev->is_i2c_protocol = true;
+
 	ipmb_dev->client = client;
 	i2c_set_clientdata(client, ipmb_dev);
 	ret = i2c_slave_register(client, ipmb_slave_cb);