Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Asmaa Mnebhi <Asmaa@mellanox.com>
To:     "minyard@acm.org" <minyard@acm.org>,
        Vijay Khemka <vijaykhemka@fb.com>
CC:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "openipmi-developer@lists.sourceforge.net" 
        <openipmi-developer@lists.sourceforge.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "cminyard@mvista.com" <cminyard@mvista.com>,
        "joel@jms.id.au" <joel@jms.id.au>,
        "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
        "sdasari@fb.com" <sdasari@fb.com>
Subject: RE: [PATCH v4] drivers: ipmi: Support raw i2c packet in IPMB
Thread-Topic: [PATCH v4] drivers: ipmi: Support raw i2c packet in IPMB
Thread-Index: AQHVmnw2jQvA7h95R066pXtlC6V86aeKtZqAgAACfbA=
Date:   Thu, 14 Nov 2019 14:21:48 +0000
Message-ID: <DB6PR0501MB27121BF4E680F14A119232B4DA710@DB6PR0501MB2712.eurprd05.prod.outlook.com>
References: <20191113234133.3790374-1-vijaykhemka@fb.com>
 <20191114141037.GP2882@minyard.net>
In-Reply-To: <20191114141037.GP2882@minyard.net>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c0c8c76b-52da-4a71-b677-08d7690dfc27
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Nov 2019 14:21:48.1167
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: ep/XmNs3DQ6Jr48JAiRJ8cDP2vUMyLFev+9NFvkO9nxU8DwXdsWq9yDUnsbheEVr/QgO/RDVh4HzWK5XY/Qmlw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0501MB2855
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Vijay, could you update the existing ipmb documentation to list and describ=
e 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@linuxfoundati=
on.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel=
.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.a=
u; 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=20
> support only smbus devices. Added support for raw i2c packets.
>=20
> User can define i2c-protocol in device tree to use i2c raw transfer.
>=20
> 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 dev=
ice 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 r=
eally, 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=20
> ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>=20
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c=20
> 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;
>  };
> =20
>  static inline struct ipmb_dev *to_ipmb_dev(struct file *file) @@=20
> -112,6 +113,25 @@ static ssize_t ipmb_read(struct file *file, char __user=
 *buf, size_t count,
>  	return ret < 0 ? ret : count;
>  }
> =20
> +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 =3D msg[IPMB_MSG_LEN_IDX] - 1;
> +
> +	/* Assign message to buffer except first 2 bytes (length and address) *=
/
> +	i2c_msg.buf =3D msg + 2;
> +
> +	i2c_msg.addr =3D GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
> +	i2c_msg.flags =3D 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 c=
har __user *buf,
>  	rq_sa =3D GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
>  	netf_rq_lun =3D msg[NETFN_LUN_IDX];
> =20
> +	/* Check i2c block transfer vs smbus */
> +	if (ipmb_dev->is_i2c_protocol) {
> +		ret =3D ipmb_i2c_write(ipmb_dev->client, msg);
> +		return (ret =3D=3D 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;
> =20
>  	ipmb_dev =3D devm_kzalloc(&client->dev, sizeof(*ipmb_dev), @@ -302,6=20
> +329,11 @@ static int ipmb_probe(struct i2c_client *client,
>  	if (ret)
>  		return ret;
> =20
> +	/* Check if i2c block xmit needs to use instead of smbus */
> +	np =3D client->dev.of_node;
> +	if (np && of_get_property(np, "i2c-protocol", NULL))
> +		ipmb_dev->is_i2c_protocol =3D true;
> +
>  	ipmb_dev->client =3D client;
>  	i2c_set_clientdata(client, ipmb_dev);
>  	ret =3D i2c_slave_register(client, ipmb_slave_cb);
> --
> 2.17.1
>=20