| Message ID | 1486509056-25838-4-git-send-email-eajames@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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 --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__ */