diff mbox

[linux,v7,3/6] hwmon: occ: Add I2C transport implementation for SCOM operations

Message ID 1486509056-25838-4-git-send-email-eajames@linux.vnet.ibm.com (mailing list archive)
State Superseded
Headers show

Commit Message

Eddie James Feb. 7, 2017, 11:10 p.m. UTC
From: "Edward A. James" <eajames@us.ibm.com>

Add functions to send SCOM operations over I2C bus. The BMC can
communicate with the Power8 host processor over I2C, but needs to use SCOM
operations in order to access the OCC register space.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/occ/occ_scom_i2c.c | 77 ++++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++
 2 files changed, 103 insertions(+)
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
 create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h

Comments

Joel Stanley Feb. 10, 2017, 5:31 a.m. UTC | #1
On Wed, Feb 8, 2017 at 9:40 AM,  <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add functions to send SCOM operations over I2C bus. The BMC can
> communicate with the Power8 host processor over I2C, but needs to use SCOM
> operations in order to access the OCC register space.

This doesn't need to be separate from the p8_occ_i2c.c file. You can
remove a layer of function calls by merging this in and having these
be your getscom putscom bus_ops callbacks.

> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/hwmon/occ/occ_scom_i2c.c | 77 ++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++
>  2 files changed, 103 insertions(+)
>  create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
>  create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
>
> diff --git a/drivers/hwmon/occ/occ_scom_i2c.c b/drivers/hwmon/occ/occ_scom_i2c.c
> new file mode 100644
> index 0000000..74bd6ff
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_scom_i2c.c
> @@ -0,0 +1,77 @@

> +
> +int occ_i2c_getscom(void *bus, u32 address, u64 *data)
> +{
> +       ssize_t rc;
> +       u64 buf;

If you add endianness annotations sparse can check your types are
consistent. The warning looks like this:

make C=2 drivers/hwmon/occ/occ_scom_i2c.o
drivers/hwmon/occ/occ_scom_i2c.c:48:17: warning: cast to restricted __be64

Which tells you it expects the type you pass to be64_to_cpu to be __be64.


> +       struct i2c_client *client = bus;
> +       struct i2c_msg msgs[2];
> +
> +       msgs[0].addr = client->addr;
> +       msgs[0].flags = client->flags & I2C_M_TEN;
> +       msgs[0].len = sizeof(u32);
> +       msgs[0].buf = (char *)&address;
> +
> +       msgs[1].addr = client->addr;
> +       msgs[1].flags = client->flags & I2C_M_TEN;
> +       msgs[1].flags |= I2C_M_RD;

I first thought you had made a mistake here. Instead you could do:

       msgs[1].flags = client->flags & I2C_M_TEN | I2C_M_RD;

> +       msgs[1].len = sizeof(u64);
> +       msgs[1].buf = (char *)&buf;
> +
> +       rc = i2c_transfer(client->adapter, msgs, 2);
> +       if (rc < 0)
> +               return rc;
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eddie James Feb. 10, 2017, 9:05 p.m. UTC | #2
On 02/09/2017 11:31 PM, Joel Stanley wrote:
> On Wed, Feb 8, 2017 at 9:40 AM,<eajames@linux.vnet.ibm.com>  wrote:
>> From: "Edward A. James"<eajames@us.ibm.com>
>>
>> Add functions to send SCOM operations over I2C bus. The BMC can
>> communicate with the Power8 host processor over I2C, but needs to use SCOM
>> operations in order to access the OCC register space.
> This doesn't need to be separate from the p8_occ_i2c.c file. You can
> remove a layer of function calls by merging this in and having these
> be your getscom putscom bus_ops callbacks.

The purpose of having this separate was so that we could do the scom 
address shift for p8 separately.

>> Signed-off-by: Edward A. James<eajames@us.ibm.com>
>> Signed-off-by: Andrew Jeffery<andrew@aj.id.au>
>> ---
>>   drivers/hwmon/occ/occ_scom_i2c.c | 77 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++
>>   2 files changed, 103 insertions(+)
>>   create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
>>   create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
>>
>> diff --git a/drivers/hwmon/occ/occ_scom_i2c.c b/drivers/hwmon/occ/occ_scom_i2c.c
>> new file mode 100644
>> index 0000000..74bd6ff
>> --- /dev/null
>> +++ b/drivers/hwmon/occ/occ_scom_i2c.c
>> @@ -0,0 +1,77 @@
>> +
>> +int occ_i2c_getscom(void *bus, u32 address, u64 *data)
>> +{
>> +       ssize_t rc;
>> +       u64 buf;
> If you add endianness annotations sparse can check your types are
> consistent. The warning looks like this:
>
> make C=2 drivers/hwmon/occ/occ_scom_i2c.o
> drivers/hwmon/occ/occ_scom_i2c.c:48:17: warning: cast to restricted __be64
>
> Which tells you it expects the type you pass to be64_to_cpu to be __be64.
>
>
>> +       struct i2c_client *client = bus;
>> +       struct i2c_msg msgs[2];
>> +
>> +       msgs[0].addr = client->addr;
>> +       msgs[0].flags = client->flags & I2C_M_TEN;
>> +       msgs[0].len = sizeof(u32);
>> +       msgs[0].buf = (char *)&address;
>> +
>> +       msgs[1].addr = client->addr;
>> +       msgs[1].flags = client->flags & I2C_M_TEN;
>> +       msgs[1].flags |= I2C_M_RD;
> I first thought you had made a mistake here. Instead you could do:
>
>         msgs[1].flags = client->flags & I2C_M_TEN | I2C_M_RD;

Sure. Was just copying i2c_master_recv.

>> +       msgs[1].len = sizeof(u64);
>> +       msgs[1].buf = (char *)&buf;
>> +
>> +       rc = i2c_transfer(client->adapter, msgs, 2);
>> +       if (rc < 0)
>> +               return rc;
>> +

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Jeffery Feb. 13, 2017, 1:12 a.m. UTC | #3
On Fri, 2017-02-10 at 15:05 -0600, Eddie James wrote:
> On 02/09/2017 11:31 PM, Joel Stanley wrote:
> > > On Wed, Feb 8, 2017 at 9:40 AM,<eajames@linux.vnet.ibm.com>  wrote:
> > >> From: "Edward A. James"<eajames@us.ibm.com>
> >>
> >> Add functions to send SCOM operations over I2C bus. The BMC can
> >> communicate with the Power8 host processor over I2C, but needs to use SCOM
> >> operations in order to access the OCC register space.
> > This doesn't need to be separate from the p8_occ_i2c.c file. You can
> > remove a layer of function calls by merging this in and having these
> > be your getscom putscom bus_ops callbacks.
> 
> The purpose of having this separate was so that we could do the scom 
> address shift for p8 separately.

Separation was my suggestion. It makes the I2C transport implementation
independent of any P8 details, and having it separate* is no more
abstract than the core performing SCOMs through the FSI interface when
that's available. I feel like it's muddying the waters conceptually to
bury P8 details in the implementation of a SCOM transport layer.

However, in our less abstract world the P8 will be the only system that
uses the I2C transport, so while I don't think merging the functions is
a good idea from an abstraction perspective it won't have a big impact.

Andrew

* Maybe the I2C SCOM transport even deserves to live outside
drivers/hwmon/occ?
diff mbox

Patch

diff --git a/drivers/hwmon/occ/occ_scom_i2c.c b/drivers/hwmon/occ/occ_scom_i2c.c
new file mode 100644
index 0000000..74bd6ff
--- /dev/null
+++ b/drivers/hwmon/occ/occ_scom_i2c.c
@@ -0,0 +1,77 @@ 
+/*
+ * occ_scom_i2c.c - hwmon OCC driver
+ *
+ * This file contains the functions for performing SCOM operations over I2C bus
+ * to access the OCC.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include "occ_scom_i2c.h"
+#include "scom.h"
+
+int occ_i2c_getscom(void *bus, u32 address, u64 *data)
+{
+	ssize_t rc;
+	u64 buf;
+	struct i2c_client *client = bus;
+	struct i2c_msg msgs[2];
+
+	msgs[0].addr = client->addr;
+	msgs[0].flags = client->flags & I2C_M_TEN;
+	msgs[0].len = sizeof(u32);
+	msgs[0].buf = (char *)&address;
+
+	msgs[1].addr = client->addr;
+	msgs[1].flags = client->flags & I2C_M_TEN;
+	msgs[1].flags |= I2C_M_RD;
+	msgs[1].len = sizeof(u64);
+	msgs[1].buf = (char *)&buf;
+
+	rc = i2c_transfer(client->adapter, msgs, 2);
+	if (rc < 0)
+		return rc;
+
+	*data = be64_to_cpu(buf);
+
+	return 0;
+}
+EXPORT_SYMBOL(occ_i2c_getscom);
+
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
+{
+	u32 buf[3];
+	ssize_t rc;
+	struct i2c_client *client = bus;
+
+	/* send raw data, user can handle endian */
+	buf[0] = address;
+	buf[1] = data1;
+	buf[2] = data0;
+
+	rc = i2c_master_send(client, (const char *)buf, sizeof(u32) * 3);
+	if (rc < 0)
+		return rc;
+	else if (rc != sizeof(u32) * 3)
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL(occ_i2c_putscom);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("I2C OCC SCOM transport");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/occ/occ_scom_i2c.h b/drivers/hwmon/occ/occ_scom_i2c.h
new file mode 100644
index 0000000..945739c
--- /dev/null
+++ b/drivers/hwmon/occ/occ_scom_i2c.h
@@ -0,0 +1,26 @@ 
+/*
+ * occ_scom_i2c.h - hwmon OCC driver
+ *
+ * This file contains function protoypes for peforming SCOM operations over I2C
+ * bus to access the OCC.
+ *
+ * Copyright 2016 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __OCC_SCOM_I2C_H__
+#define __OCC_SCOM_I2C_H__
+
+int occ_i2c_getscom(void *bus, u32 address, u64 *data);
+int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
+
+#endif /* __OCC_SCOM_I2C_H__ */