[2/3] add i2cm from bmc

Signed-off-by: Rashmica Gupta <rashmica.g@gmail.com>
---
 libpdbg/i2cm.c   | 323 ++++++++++++++++++++++++++++++++++++++++++++++-
 libpdbg/i2cm.h   | 117 +++++++++++++++++
 libpdbg/kernel.c |   8 +-
 p9-fsi.dtsi.m4   |   7 +
 p9-kernel.dts.m4 |   2 +-
 src/i2c.c        |   2 +-
 6 files changed, 452 insertions(+), 7 deletions(-)
 create mode 100644 libpdbg/i2cm.h

On Friday, 5 April 2019 4:57:16 PM AEST Rashmica Gupta wrote:
> 
> +
> +static void _i2c_write(struct i2cm *i2cm, uint32_t addr, uint32_t data)

Would it be clearer to call this _i2cm_reg_write() to emphasize that it's 
accessing registers on the I2C master rather than on the I2C bus itself?

> +{
> +	fsi_write(&i2cm->target, addr, data);
> +	printf("writing 0x%16" PRIx64 "\n", (uint64_t)data << 32);
> +}
> +
> +static void _i2c_read(struct i2cm *i2cm, uint32_t addr, uint32_t *data)

Ditto.

> +{
> +	fsi_read(&i2cm->target, addr, data);
> +}
> +
> +static void debug_print_reg(struct i2cm *i2cm)
> +{
> +	uint32_t fsidata = 0;
> +
> +	PR_INFO("\t --------\n");
> +	_i2c_read(i2cm,  I2C_STATUS_REG, &fsidata);
> +	PR_INFO("\t status reg \t has value 0x%x \n", fsidata);
> +	if (fsidata & I2C_STAT_INV_CMD)
> +		 PR_INFO("\t\tinvalid cmd\n");
> +	if (fsidata & I2C_STAT_PARITY)
> +		 PR_INFO("\t\tparity\n");
> +	if (fsidata & I2C_STAT_BE_OVERRUN)
> +		 PR_INFO("\t\tback endoverrun\n");
> +	if (fsidata & I2C_STAT_BE_ACCESS)
> +		 PR_INFO("\t\tback end access error\n");
> +	if (fsidata & I2C_STAT_LOST_ARB)
> +		 PR_INFO("\t\tarbitration lost\n");
> +	if (fsidata & I2C_STAT_NACK)
> +		 PR_INFO("\t\tnack\n");
> +	if (fsidata & I2C_STAT_DAT_REQ)
> +		 PR_INFO("\t\tdata request\n");
> +	if (fsidata & I2C_STAT_STOP_ERR)
> +		 PR_INFO("\t\tstop error\n");
> +	if (fsidata & I2C_STAT_PORT_BUSY)
> +		 PR_INFO("\t\ti2c busy\n");
> +	PR_INFO("\t\tfifo entry count: %li \n",fsidata&I2C_STAT_FIFO_COUNT);
> +
> +	_i2c_read(i2cm,  I2C_ESTAT_REG, &fsidata);
> +	PR_INFO("\t extended status reg has value 0x%x \n", fsidata);
> +	if (fsidata & I2C_ESTAT_HIGH_WATER)
> +		PR_INFO("\t\thigh water mark reached\n");
> +	if (fsidata & I2C_ESTAT_LOW_WATER)
> +		PR_INFO("\t\tlow water mark reached\n");
> +
> +
> +	_i2c_read(i2cm,  I2C_WATERMARK_REG, &fsidata);
> +	PR_INFO("\t watermark reg  has value 0x%x \n", fsidata);
> +	PR_INFO("\t\twatermark high: %li \n",fsidata&I2C_WATERMARK_HIGH);
> +	PR_INFO("\t\twatermark low: %li \n",fsidata&I2C_WATERMARK_LOW);
> +
> +	_i2c_read(i2cm,  I2C_RESIDUAL_REG, &fsidata);
> +	PR_INFO("\t residual reg  has value 0x%x \n", fsidata);
> +	PR_INFO("\t\tfrontend_len: %li \n",fsidata&I2C_RESID_FRONT);
> +	PR_INFO("\t\tbackend_len: %li \n",fsidata&I2C_RESID_BACK);
> +
> +	_i2c_read(i2cm,  I2C_PORT_BUSY_REG, &fsidata);
> +	PR_INFO("\t port busy reg  has value 0x%x \n", fsidata);
> +	PR_INFO("\t -------\n");
> +
> +}
> +
> +static void i2c_mode_write(struct i2cm *i2cm, uint16_t port)
> +{
> +	uint32_t data = I2C_MODE_PACING;
> +
> +	// hardcoding bit rate divisor because not too important
> +	data = SETFIELD(I2C_MODE_BIT_RATE_DIV, data, 28);
> +	data = SETFIELD(I2C_MODE_PORT_NUM, data, port);
> +	PR_INFO("setting mode to %x\n", data);
> +	_i2c_write(i2cm, I2C_MODE_REG, data);
> +}
> +
> +static void i2c_watermark_write(struct i2cm *i2cm)
> +{
> +	uint32_t data = 0;
> +
> +	data = SETFIELD(I2C_WATERMARK_HIGH, data, 4);
> +	data = SETFIELD(I2C_WATERMARK_LOW, data, 4);
> +	PR_INFO("setting watermark (0x%x) to: %x\n", I2C_WATERMARK_REG, data);
> +	_i2c_write(i2cm, I2C_WATERMARK_REG, data);
> +}
> +
> +static int i2c_reset(struct i2cm *i2cm, uint16_t port)
> +{
> +	uint32_t fsidata = 0;
> +	debug_print_reg(i2cm);
> +	PR_INFO("### RESETING \n");
> +
> +	fsidata = 0xB;
> +	_i2c_write(i2cm, I2C_IMD_RESET_REG, fsidata);
> +	_i2c_write(i2cm, I2C_IMD_RESET_ERR_REG, fsidata);
> +
> +	usleep(10000);
> +	debug_print_reg(i2cm);
> +	return 0;
> +}
> +
> +static int i2c_status_read(struct i2cm *i2cm, uint32_t *data)

This function looks like it could be dropped, it's clearer to call _i2c_read() 
directly from code. You would also get the actual return value too :-)

As an aside the justification for having a _i2c_read()/write() function is that 
the HW register interface to the I2CM is the same regardless of FSI vs. SCOM 
access so some future work can add `if (scom) ... else ...` in those functions 
to abstract I2CM register access.

> +{
> +	_i2c_read(i2cm, I2C_STATUS_REG, data);
> +	return 0;
> +}
> +
> +/*
> + *	If there are errors in the status register, redo the whole
> + *	operation after restting the i2cm.
> +*/
> +static int i2c_poll_status(struct i2cm *i2cm, uint32_t *data)
> +{
> +	int loop;
> +
> +	for (loop = 0; loop < 10; loop++)
> +	{
> +		usleep(10000);
> +		i2c_status_read(i2cm, data);
> +
> +		if (((*data) & I2C_STAT_CMD_COMP))
> +			break;
> +	}
> +	if ((*data) & I2C_STAT_PORT_BUSY)
> +		PR_INFO("portbusy\n");
> +	if ((*data) & I2C_STAT_ERR) {
> +		PR_INFO("ERROR IN STATUS\n");
> +		debug_print_reg(i2cm);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +static int i2c_fifo_write_byte(struct i2cm *i2cm, uint8_t data)
> +{

Similar comment to i2c_status_read(), although understandable for debug.

> +	PR_INFO("\twriting: %x  to FIFO\n", data);
> +	_i2c_write(i2cm, I2C_FIFO_REG, data);
> +	return 0;
> +}
> +
> +/* Make sure this is only called if the fifo has bytes to read */
> +static int i2c_fifo_read_byte(struct i2cm *i2cm, uint8_t *data)
> +{
> +	uint32_t tmp = 0xeeeeeeee;
> +
> +	_i2c_read(i2cm, I2C_FIFO_REG, &tmp);
> +	PR_INFO("\tread byte: %x \n", tmp);
> +	if (tmp == 0xeeeeeeee)

It would be simpler just to return a fail code from _i2c_read() here - ie. the 
fsi_read() function should detect failure and return an error code rather than 
checking for magic values in tmp which could lead to weirdness if for example 
the value returned from an actual I2C bus read is 0xeeeeeeee.

> +		return 1;
> +	*data = (uint8_t )tmp;
> +	return 0;
> +}
> +
> +static int i2c_fifo_read(struct i2cm *i2cm, uint8_t *data, uint16_t size)
> +{
> +	int bytes_to_read = 1;
> +	int rc = 0, bytes_read = 0;
> +	uint8_t tmp;
> +	uint32_t status;
> +
> +	while (bytes_to_read) {
> +
> +		if (bytes_read > size)
> +			return 0;
> +
> +		/*	Poll status register's FIFO_ENTRY_COUNT to get to know that FIFO 
has
> sufficient number of data. +		Make sure that FIFO never overflows and
> underflows. */
> +
> +		rc = i2c_poll_status(i2cm, &status);
> +		bytes_to_read = status & I2C_STAT_FIFO_COUNT;
> +		PR_INFO("%x bytes in fifo \n", bytes_to_read);
> +
> +		if (rc)
> +			return 0;
> +
> +		if (!bytes_to_read)
> +			continue;
> +
> +		rc = i2c_fifo_read_byte(i2cm, &tmp);

I wonder ... does this actually only read a single byte or is it actually 
reading 4 bytes at a time from the FIFO and only returning a single byte?

> +		if (rc)
> +			return bytes_read;
> +		data[bytes_read] = tmp;
> +		PR_INFO(" %x \n", data[bytes_read]);
> +		bytes_read++;
> +	}
> +	return bytes_read;
> +}
> +
> +static int i2cm_ok_to_use(struct i2cm *i2cm, uint16_t port)
> +{
> +	uint32_t data;
> +
> +	i2c_status_read(i2cm, &data);
> +
> +	if (!(data & I2C_STAT_CMD_COMP) || (data & I2C_STAT_ERR))
> +	{
> +		PR_ERROR("ERROR IN STATUS, attempting to reset %x \n", data);
> +		i2c_reset(i2cm, port);
> +		i2c_status_read(i2cm, &data);
> +	}
> +	if (data & I2C_STAT_ERR) {
> +		PR_ERROR("ERROR IN STATUS %x\n", data);
> +		return 0;
> +	}
> +	i2c_watermark_write(i2cm);
> +	i2c_mode_write(i2cm, port);
> +	PR_INFO("OK\n");
> +	return 1;
> +}
> +
> +static int i2c_get(struct i2cm *i2cm, uint16_t port, uint32_t addr,
> uint32_t offset, uint16_t size, uint64_t *d) +{
> +	uint32_t fsidata = 0xdeadbeef;
> +	uint32_t data = 0xdeadbeef;
> +	int rc = 0;
> +	uint8_t *bytes;
> +
> +	PR_INFO("### attempting to read\n");
> +	// only deal with 8 bytes for now
> +	if (size > 8)
> +		size = 8;
> +	bytes = calloc(size, sizeof(uint8_t));
> +	if (bytes == NULL) {
> +		PR_ERROR("FAILED TO ALLOC MEM\n");
> +		return 1;
> +	}
> +
> +	if(!i2cm_ok_to_use(i2cm, port)) {
> +		rc = 1;
> +		goto out;
> +	}
> +
> +	/*
> +	 *	Tell i2c master which slave device we want to read from
> + 	 */
> +	fsidata = I2C_CMD_WITH_START | I2C_CMD_WITH_ADDR | I2C_CMD_READ_CONTINUE;
> +	fsidata = SETFIELD(I2C_CMD_DEV_ADDR, fsidata, addr);
> +	fsidata = SETFIELD(I2C_CMD_LENGTH, fsidata, 4);
> +	PR_INFO("tell i2c master which device to read from %x\n", fsidata);
> +	_i2c_write(i2cm, I2C_CMD_REG, fsidata);
> +
> +
> +	rc = i2c_poll_status(i2cm, &data);
> +	if (rc) {
> +		PR_ERROR("FAILED to set i2c device\n");
> +		goto out;
> +	}
> +	/*
> +	 * Write the byte address into the FIFO of the slave device
> +	 */
> +	PR_INFO("write the byte address into the fifo %x\n", offset);
> +	i2c_fifo_write_byte(i2cm, offset);

I will catch you offline but we don't want to be writing to the I2C bus during a 
read, although I think I know why you are :-)

> +	rc = i2c_poll_status(i2cm, &data);
> +	if (rc) {
> +		PR_ERROR("FAILED to write byte address\n");
> +		goto out;
> +	}
> +	/*
> +	 * Tell i2c master to read from slave device's fifo
> +	 */
> +	fsidata = I2C_CMD_WITH_START | I2C_CMD_WITH_STOP | I2C_CMD_WITH_ADDR |
> I2C_CMD_READ_NOT_WRITE; +	fsidata = SETFIELD(I2C_CMD_DEV_ADDR, fsidata,
> addr);
> +	fsidata = SETFIELD(I2C_CMD_LENGTH, fsidata, size);
> +	PR_INFO("tell i2c master to read from slave's fifo %x\n", fsidata);
> +	_i2c_write(i2cm, I2C_CMD_REG, fsidata);
> +
> +
> +	int bytes_read;
> +	bytes_read = i2c_fifo_read(i2cm, bytes, size);
> +
> +	rc = i2c_poll_status(i2cm, &data);
> +	if (rc) {
> +		PR_ERROR("ERROR while reading FIFO\n");
> +		goto out;
> +	}
> +	int i;
> +	*d = 0;
> +	for (i = 0; i < bytes_read; i++) {
> +		*d =  *d | (uint64_t)bytes[i] << i*8;
> +	}
> +	PR_INFO("we read: 0x%016" PRIx64 "\n", (*d));
> +
> +	if (bytes_read < size) {
> +		PR_ERROR("ERROR didn't read all bytes\n");
> +		debug_print_reg(i2cm);
> +		goto out;
> +
> +	}
> +
> +
> +	if (data & I2C_STAT_CMD_COMP)
> +		PR_INFO(" cmd completed!\n");
> +
> +out:
> +	free(bytes);
> +	return rc;
> +}
> +
> +int i2cm_target_probe(struct pdbg_target *target)
> +{
> +	return 0;
> +}
> +
> +static struct i2cm i2c_fsi = {
> +	.target = {
> +		.name =	"CFAM I2C Master",
> +		.compatible = "ibm,fsi-i2c-master",
> +		.class = "i2cm",
> +		.probe = i2cm_target_probe,
> +	},
> +	.read = i2c_get,
> +};
> +DECLARE_HW_UNIT(i2c_fsi);
> +
> +///////////////////////////////////////////////////////////////////////////
> // +
>  #ifndef DISABLE_I2CLIB
>  #include <i2c/smbus.h>
> 
>  #define MAX_I2C_BUSES 100
>  static int i2c_buses[100] = {0};
> 
> -static int kernel_i2c_get(struct i2cm *i2cm, uint32_t addr, uint32_t
> offset, uint16_t size, uint64_t *data) +
> +static int kernel_i2c_get(struct i2cm *i2cm, uint16_t port, uint32_t addr,
> uint32_t offset, uint16_t size, uint64_t *data) {
>  	int res = 0;
>  	char i2c_path[NAME_MAX];
> @@ -67,7 +382,7 @@ static int kernel_i2c_get(struct i2cm *i2cm, uint32_t
> addr, uint32_t offset, uin
> 
>  		res = i2c_smbus_read_byte_data(i2c_fd, offset);
>  		if (res >= 0) {
> -			PR_DEBUG("read %x from device %x on bus %s\n", res, addr, 
i2c_path);
> +			PR_INFO("read %x from device %x on bus %s\n", res, addr, 
i2c_path);
>  			*data = (uint64_t)res;
>  			return res;
>  		}
> diff --git a/libpdbg/i2cm.h b/libpdbg/i2cm.h
> new file mode 100644
> index 0000000..d17931c
> --- /dev/null
> +++ b/libpdbg/i2cm.h

Do these definitions actually need to be in a seperate header file? If they are 
only used from the i2cm.c just put them directly in there.

> @@ -0,0 +1,117 @@
> +/* Copyright 2016 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + * 	http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +#ifndef __I2CMASTER_H
> +#define __I2CMASTER_H
> +
> +#include "bitutils.h"
> +
> +/* I2C common registers */
> +#define I2C_FIFO_REG 		0x0
> +#define I2C_CMD_REG 		0x1
> +#define I2C_MODE_REG 		0x2
> +#define I2C_WATERMARK_REG	0x3
> +#define I2C_INT_MASK_REG	0x4
> +#define I2C_INT_COND_REG	0x5
> +#define I2C_STATUS_REG		0x7
> +#define I2C_IMD_RESET_REG	0x7
> +#define I2C_IMD_RESET_ERR_REG	0x8
> +#define I2C_ESTAT_REG		0x8
> +#define I2C_RESIDUAL_REG	0x9
> +#define I2C_PORT_BUSY_REG	0xA
> +
> +#define I2C_PIB_OFFSET 		0x4
> +#define I2C_PIB_ENGINE_0 	0x0000
> +#define I2C_PIB_ENGINE_1 	0x1000
> +#define I2C_PIB_ENGINE_2 	0x2000
> +#define I2C_PIB_ENGINE_3 	0x3000
> +
> +/* I2C command register bits */
> +#define I2C_CMD_WITH_START		PPC_BIT32(0)
> +#define I2C_CMD_WITH_ADDR		PPC_BIT32(1)
> +#define I2C_CMD_READ_CONTINUE	PPC_BIT32(2)
> +#define I2C_CMD_WITH_STOP		PPC_BIT32(3)
> +#define I2C_CMD_INT_STEER		PPC_BITMASK32(6, 7)
> +#define I2C_CMD_DEV_ADDR		PPC_BITMASK32(8, 14)
> +#define I2C_CMD_READ_NOT_WRITE	PPC_BIT32(15)
> +#define I2C_CMD_LENGTH			PPC_BITMASK32(16, 31)
> +
> +/* I2C mode register bits */
> +#define I2C_MODE_BIT_RATE_DIV	PPC_BITMASK32(0, 15)
> +#define I2C_MODE_PORT_NUM		PPC_BITMASK32(16, 21)
> +#define I2C_ENHANCED_MODE		PPC_BIT32(28)
> +#define I2C_MODE_PACING			PPC_BIT32(30)
> +
> +/* watermark */
> +#define I2C_WATERMARK_HIGH		PPC_BITMASK32(16,19)
> +#define I2C_WATERMARK_LOW		PPC_BITMASK32(24,27)
> +
> +/* I2C status register */
> +#define I2C_STAT_INV_CMD		PPC_BIT32(0)
> +#define I2C_STAT_PARITY			PPC_BIT32(1)
> +#define I2C_STAT_BE_OVERRUN		PPC_BIT32(2)
> +#define I2C_STAT_BE_ACCESS		PPC_BIT32(3)
> +#define I2C_STAT_LOST_ARB		PPC_BIT32(4)
> +#define I2C_STAT_NACK			PPC_BIT32(5)
> +#define I2C_STAT_DAT_REQ		PPC_BIT32(6)
> +#define I2C_STAT_CMD_COMP		PPC_BIT32(7)
> +#define I2C_STAT_STOP_ERR		PPC_BIT32(8)
> +#define I2C_STAT_MAX_PORT		PPC_BITMASK32(9, 15)
> +#define I2C_STAT_ANY_INT		PPC_BIT32(16)
> +#define I2C_STAT_WAIT_BUSY		PPC_BIT32(17)
> +#define I2C_STAT_ERR_IN			PPC_BIT32(18)
> +#define I2C_STAT_PORT_HIST_BUSY	PPC_BIT32(19)
> +#define I2C_STAT_SCL_IN			PPC_BIT32(20)
> +#define I2C_STAT_SDA_IN			PPC_BIT32(21)
> +#define I2C_STAT_PORT_BUSY		PPC_BIT32(22)
> +#define I2C_STAT_SELF_BUSY		PPC_BIT32(23)
> +#define I2C_STAT_FIFO_COUNT		PPC_BITMASK32(24, 31)
> +
> +#define I2C_STAT_ERR		(I2C_STAT_INV_CMD |			\
> +						 	 I2C_STAT_PARITY |			\
> +			 				 I2C_STAT_BE_OVERRUN |		\
> +			 				 I2C_STAT_BE_ACCESS |		\
> +			 				 I2C_STAT_LOST_ARB |		\
> +			 				 I2C_STAT_NACK |			\
> +			 				 I2C_STAT_STOP_ERR)
> +
> +#define I2C_STAT_ANY_RESP	(I2C_STAT_ERR |				\
> +						 	I2C_STAT_DAT_REQ |			\
> +							I2C_STAT_CMD_COMP)
> +
> +/* I2C extended status register */
> +#define I2C_ESTAT_FIFO_SIZE PPC_BITMASK32(0,7)
> +#define I2C_ESTAT_MSM_STATE PPC_BITMASK32(11,15)
> +#define I2C_ESTAT_HIGH_WATER 	PPC_BIT32(22)
> +#define I2C_ESTAT_LOW_WATER 	PPC_BIT32(23)
> +
> +/* I2C interrupt mask register */
> +#define I2C_INT_INV_CMD		PPC_BIT32(16)
> +#define I2C_INT_PARITY		PPC_BIT32(17)
> +#define I2C_INT_BE_OVERRUN	PPC_BIT32(18)
> +#define I2C_INT_BE_ACCESS	PPC_BIT32(19)
> +#define I2C_INT_LOST_ARB	PPC_BIT32(20)
> +#define I2C_INT_NACK		PPC_BIT32(21)
> +#define I2C_INT_DAT_REQ		PPC_BIT32(22)
> +#define I2C_INT_CMD_COMP	PPC_BIT32(23)
> +#define I2C_INT_STOP_ERR	PPC_BIT32(24)
> +#define I2C_INT_BUSY		PPC_BIT32(25)
> +#define I2C_INT_IDLE		PPC_BIT32(26)
> +
> +/* I2C residual  register */
> +#define I2C_RESID_FRONT		PPC_BITMASK32(0,15)
> +#define I2C_RESID_BACK		PPC_BITMASK32(16,31)
> +
> +#endif
> diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
> index 8b597ef..00a25b4 100644
> --- a/libpdbg/kernel.c
> +++ b/libpdbg/kernel.c
> @@ -39,6 +39,7 @@ static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t
> addr64, uint32_t *value) {
>  	int rc;
>  	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) << 2);
> +	uint8_t tmp2;
> 
>  	rc = lseek(fsi_fd, addr, SEEK_SET);
>  	if (rc < 0) {
> @@ -46,7 +47,10 @@ static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t
> addr64, uint32_t *value) return errno;
>  	}
> 
> -	rc = read(fsi_fd, &tmp, 4);
> +	if (addr == 0x1800)
> +		rc = read(fsi_fd, &tmp2, 1);

So this might clarify my comment above - I wonder what this read actually does 
at the hardware level. In this case it depends on the OpenFSI kernel driver.

Joel do you know if the OpenFSI read() call supports reading a single physical 
byte from the FSI bus? (rather than reading 4 bytes from the HW and throwing 
away 3 bytes).

> +	else
> +		rc = read(fsi_fd, &tmp, 4);
>  	if (rc < 0) {
>  		if ((addr64 & 0xfff) != 0xc09)
>  			/* We expect reads of 0xc09 to occasionally
> @@ -56,6 +60,8 @@ static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t
> addr64, uint32_t *value) return errno;
>  	}
>  	*value = be32toh(tmp);
> +	if (addr == 0x1800)
> +		*value = tmp2;
> 
>  	return 0;
>  }
> diff --git a/p9-fsi.dtsi.m4 b/p9-fsi.dtsi.m4
> index afa7d39..89ba387 100644
> --- a/p9-fsi.dtsi.m4
> +++ b/p9-fsi.dtsi.m4
> @@ -21,6 +21,13 @@
>  			 include(p9-pib.dts.m4)dnl
>  		};
> 
> +		i2cm@1800 {
> +			#address-cells = <0x2>;
> +			#size-cells = <0x1>;
> +			reg = <0x0 0x1800 0x400>;
> +			compatible = "ibm,fsi-i2c-master";
> +		};
> +
>  		hmfsi@100000 {
>  			#address-cells = <0x2>;
>  			#size-cells = <0x1>;
> diff --git a/p9-kernel.dts.m4 b/p9-kernel.dts.m4
> index 474beca..dacea83 100644
> --- a/p9-kernel.dts.m4
> +++ b/p9-kernel.dts.m4
> @@ -26,7 +26,7 @@
>  			#address-cells = <0x2>;
>  			#size-cells = <0x1>;
>  			reg = <0x0 0x1800 0x400>;
> -			compatible = "ibm,kernel-i2c-master";
> +			compatible = "ibm,fsi-i2c-master";
>  			include(p9-i2c.dts.m4)dnl
>  		};
> 
> diff --git a/src/i2c.c b/src/i2c.c
> index 2a32ec2..32c4edd 100644
> --- a/src/i2c.c
> +++ b/src/i2c.c
> @@ -27,7 +27,7 @@ static int geti2c(uint16_t port, uint32_t addr, uint32_t
> offset, uint16_t size) uint64_t data = 0xc0ffee;
>  	struct pdbg_target *target, *selected = NULL;
> 
> -	for_each_path_target_class("i2cbus", target) {
> +	for_each_path_target_class("i2cm", target) {
>  		if (pdbg_target_probe(target) != PDBG_TARGET_ENABLED)
>  			continue;
>  		selected = target;

On Wed, 2019-04-10 at 12:31 +1000, Alistair Popple wrote:
> On Friday, 5 April 2019 4:57:16 PM AEST Rashmica Gupta wrote:
> > 
> > +
> > +static void _i2c_write(struct i2cm *i2cm, uint32_t addr, uint32_t
> > data)
> 
> Would it be clearer to call this _i2cm_reg_write() to emphasize that
> it's 
> accessing registers on the I2C master rather than on the I2C bus
> itself?
> 
yes it would

> > +{
> > +	fsi_write(&i2cm->target, addr, data);
> > +	printf("writing 0x%16" PRIx64 "\n", (uint64_t)data << 32);
> > +}
> > +
> > +static void _i2c_read(struct i2cm *i2cm, uint32_t addr, uint32_t
> > *data)
> 
> Ditto.
> 
> > +{
> > +	fsi_read(&i2cm->target, addr, data);
> > +}
> > +
> > +static void debug_print_reg(struct i2cm *i2cm)
> > +{
> > +	uint32_t fsidata = 0;
> > +
> > +	PR_INFO("\t --------\n");
> > +	_i2c_read(i2cm,  I2C_STATUS_REG, &fsidata);
> > +	PR_INFO("\t status reg \t has value 0x%x \n", fsidata);
> > +	if (fsidata & I2C_STAT_INV_CMD)
> > +		 PR_INFO("\t\tinvalid cmd\n");
> > +	if (fsidata & I2C_STAT_PARITY)
> > +		 PR_INFO("\t\tparity\n");
> > +	if (fsidata & I2C_STAT_BE_OVERRUN)
> > +		 PR_INFO("\t\tback endoverrun\n");
> > +	if (fsidata & I2C_STAT_BE_ACCESS)
> > +		 PR_INFO("\t\tback end access error\n");
> > +	if (fsidata & I2C_STAT_LOST_ARB)
> > +		 PR_INFO("\t\tarbitration lost\n");
> > +	if (fsidata & I2C_STAT_NACK)
> > +		 PR_INFO("\t\tnack\n");
> > +	if (fsidata & I2C_STAT_DAT_REQ)
> > +		 PR_INFO("\t\tdata request\n");
> > +	if (fsidata & I2C_STAT_STOP_ERR)
> > +		 PR_INFO("\t\tstop error\n");
> > +	if (fsidata & I2C_STAT_PORT_BUSY)
> > +		 PR_INFO("\t\ti2c busy\n");
> > +	PR_INFO("\t\tfifo entry count: %li
> > \n",fsidata&I2C_STAT_FIFO_COUNT);
> > +
> > +	_i2c_read(i2cm,  I2C_ESTAT_REG, &fsidata);
> > +	PR_INFO("\t extended status reg has value 0x%x \n", fsidata);
> > +	if (fsidata & I2C_ESTAT_HIGH_WATER)
> > +		PR_INFO("\t\thigh water mark reached\n");
> > +	if (fsidata & I2C_ESTAT_LOW_WATER)
> > +		PR_INFO("\t\tlow water mark reached\n");
> > +
> > +
> > +	_i2c_read(i2cm,  I2C_WATERMARK_REG, &fsidata);
> > +	PR_INFO("\t watermark reg  has value 0x%x \n", fsidata);
> > +	PR_INFO("\t\twatermark high: %li
> > \n",fsidata&I2C_WATERMARK_HIGH);
> > +	PR_INFO("\t\twatermark low: %li \n",fsidata&I2C_WATERMARK_LOW);
> > +
> > +	_i2c_read(i2cm,  I2C_RESIDUAL_REG, &fsidata);
> > +	PR_INFO("\t residual reg  has value 0x%x \n", fsidata);
> > +	PR_INFO("\t\tfrontend_len: %li \n",fsidata&I2C_RESID_FRONT);
> > +	PR_INFO("\t\tbackend_len: %li \n",fsidata&I2C_RESID_BACK);
> > +
> > +	_i2c_read(i2cm,  I2C_PORT_BUSY_REG, &fsidata);
> > +	PR_INFO("\t port busy reg  has value 0x%x \n", fsidata);
> > +	PR_INFO("\t -------\n");
> > +
> > +}
> > +
> > +static void i2c_mode_write(struct i2cm *i2cm, uint16_t port)
> > +{
> > +	uint32_t data = I2C_MODE_PACING;
> > +
> > +	// hardcoding bit rate divisor because not too important
> > +	data = SETFIELD(I2C_MODE_BIT_RATE_DIV, data, 28);
> > +	data = SETFIELD(I2C_MODE_PORT_NUM, data, port);
> > +	PR_INFO("setting mode to %x\n", data);
> > +	_i2c_write(i2cm, I2C_MODE_REG, data);
> > +}
> > +
> > +static void i2c_watermark_write(struct i2cm *i2cm)
> > +{
> > +	uint32_t data = 0;
> > +
> > +	data = SETFIELD(I2C_WATERMARK_HIGH, data, 4);
> > +	data = SETFIELD(I2C_WATERMARK_LOW, data, 4);
> > +	PR_INFO("setting watermark (0x%x) to: %x\n", I2C_WATERMARK_REG,
> > data);
> > +	_i2c_write(i2cm, I2C_WATERMARK_REG, data);
> > +}
> > +
> > +static int i2c_reset(struct i2cm *i2cm, uint16_t port)
> > +{
> > +	uint32_t fsidata = 0;
> > +	debug_print_reg(i2cm);
> > +	PR_INFO("### RESETING \n");
> > +
> > +	fsidata = 0xB;
> > +	_i2c_write(i2cm, I2C_IMD_RESET_REG, fsidata);
> > +	_i2c_write(i2cm, I2C_IMD_RESET_ERR_REG, fsidata);
> > +
> > +	usleep(10000);
> > +	debug_print_reg(i2cm);
> > +	return 0;
> > +}
> > +
> > +static int i2c_status_read(struct i2cm *i2cm, uint32_t *data)
> 
> This function looks like it could be dropped, it's clearer to call
> _i2c_read() 
> directly from code. You would also get the actual return value too :-
> )
> 

True

> As an aside the justification for having a _i2c_read()/write()
> function is that 
> the HW register interface to the I2CM is the same regardless of FSI
> vs. SCOM 
> access so some future work can add `if (scom) ... else ...` in those
> functions 
> to abstract I2CM register access.

that's what I did in the third patch with i2cm->host?

> 
> > +{
> > +	_i2c_read(i2cm, I2C_STATUS_REG, data);
> > +	return 0;
> > +}
> > +
> > +/*
> > + *	If there are errors in the status register, redo the whole
> > + *	operation after restting the i2cm.
> > +*/
> > +static int i2c_poll_status(struct i2cm *i2cm, uint32_t *data)
> > +{
> > +	int loop;
> > +
> > +	for (loop = 0; loop < 10; loop++)
> > +	{
> > +		usleep(10000);
> > +		i2c_status_read(i2cm, data);
> > +
> > +		if (((*data) & I2C_STAT_CMD_COMP))
> > +			break;
> > +	}
> > +	if ((*data) & I2C_STAT_PORT_BUSY)
> > +		PR_INFO("portbusy\n");
> > +	if ((*data) & I2C_STAT_ERR) {
> > +		PR_INFO("ERROR IN STATUS\n");
> > +		debug_print_reg(i2cm);
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int i2c_fifo_write_byte(struct i2cm *i2cm, uint8_t data)
> > +{
> 
> Similar comment to i2c_status_read(), although understandable for
> debug.
> 
> > +	PR_INFO("\twriting: %x  to FIFO\n", data);
> > +	_i2c_write(i2cm, I2C_FIFO_REG, data);
> > +	return 0;
> > +}
> > +
> > +/* Make sure this is only called if the fifo has bytes to read */
> > +static int i2c_fifo_read_byte(struct i2cm *i2cm, uint8_t *data)
> > +{
> > +	uint32_t tmp = 0xeeeeeeee;
> > +
> > +	_i2c_read(i2cm, I2C_FIFO_REG, &tmp);
> > +	PR_INFO("\tread byte: %x \n", tmp);
> > +	if (tmp == 0xeeeeeeee)
> 
> It would be simpler just to return a fail code from _i2c_read() here
> - ie. the 
> fsi_read() function should detect failure and return an error code
> rather than 
> checking for magic values in tmp which could lead to weirdness if for
> example 
> the value returned from an actual I2C bus read is 0xeeeeeeee.
> 

Derp!

> > +		return 1;
> > +	*data = (uint8_t )tmp;
> > +	return 0;
> > +}
> > +
> > +static int i2c_fifo_read(struct i2cm *i2cm, uint8_t *data,
> > uint16_t size)
> > +{
> > +	int bytes_to_read = 1;
> > +	int rc = 0, bytes_read = 0;
> > +	uint8_t tmp;
> > +	uint32_t status;
> > +
> > +	while (bytes_to_read) {
> > +
> > +		if (bytes_read > size)
> > +			return 0;
> > +
> > +		/*	Poll status register's FIFO_ENTRY_COUNT to get to
> > know that FIFO 
> 
> has
> > sufficient number of data. +		Make sure that FIFO
> > never overflows and
> > underflows. */
> > +
> > +		rc = i2c_poll_status(i2cm, &status);
> > +		bytes_to_read = status & I2C_STAT_FIFO_COUNT;
> > +		PR_INFO("%x bytes in fifo \n", bytes_to_read);
> > +
> > +		if (rc)
> > +			return 0;
> > +
> > +		if (!bytes_to_read)
> > +			continue;
> > +
> > +		rc = i2c_fifo_read_byte(i2cm, &tmp);
> 
> I wonder ... does this actually only read a single byte or is it
> actually 
> reading 4 bytes at a time from the FIFO and only returning a single
> byte?
> 
> > +		if (rc)
> > +			return bytes_read;
> > +		data[bytes_read] = tmp;
> > +		PR_INFO(" %x \n", data[bytes_read]);
> > 

...

> > +#endif
> > diff --git a/libpdbg/kernel.c b/libpdbg/kernel.c
> > index 8b597ef..00a25b4 100644
> > --- a/libpdbg/kernel.c
> > +++ b/libpdbg/kernel.c
> > @@ -39,6 +39,7 @@ static int kernel_fsi_getcfam(struct fsi *fsi,
> > uint32_t
> > addr64, uint32_t *value) {
> >  	int rc;
> >  	uint32_t tmp, addr = (addr64 & 0x7ffc00) | ((addr64 & 0x3ff) <<
> > 2);
> > +	uint8_t tmp2;
> > 
> >  	rc = lseek(fsi_fd, addr, SEEK_SET);
> >  	if (rc < 0) {
> > @@ -46,7 +47,10 @@ static int kernel_fsi_getcfam(struct fsi *fsi,
> > uint32_t
> > addr64, uint32_t *value) return errno;
> >  	}
> > 
> > -	rc = read(fsi_fd, &tmp, 4);
> > +	if (addr == 0x1800)
> > +		rc = read(fsi_fd, &tmp2, 1);
> 
> So this might clarify my comment above - I wonder what this read
> actually does 
> at the hardware level. In this case it depends on the OpenFSI kernel
> driver.
> 
> Joel do you know if the OpenFSI read() call supports reading a single
> physical 
> byte from the FSI bus? (rather than reading 4 bytes from the HW and
> throwing 
> away 3 bytes).
> 

It does seem to read a single byte. For example if I read "/pdbg -D3 -P
i2cm geti2c 0 0x50 10 8" with this hack then I get:

writing 0xd0a1000800000000
8 bytes in fifo
        read byte: 0
7 bytes in fifo
        read byte: ff
6 bytes in fifo
        read byte: ff
5 bytes in fifo
        read byte: ff
4 bytes in fifo
        read byte: db
3 bytes in fifo
        read byte: c0
2 bytes in fifo
        read byte: e7
1 bytes in fifo
        read byte: 26
0 bytes in fifo

and if I remove it then I get:

writing 0xd0a1000800000000
8 bytes in fifo
        read byte: ffffff
4 bytes in fifo
        read byte: dbc0e726
0 bytes in fifo



> > +	else
> > +		rc = read(fsi_fd, &tmp, 4);
> >  	if (rc < 0) {
> >  		if ((addr64 & 0xfff) != 0xc09)
> >  			/* We expect reads of 0xc09 to occasionally
> > @@ -56,6 +60,8 @@ static int kernel_fsi_getcfam(struct fsi *fsi,
> > uint32_t
> > addr64, uint32_t *value) return errno;
> >  	}
> >  	*value = be32toh(tmp);
> > +	if (addr == 0x1800)
> > +		*value = tmp2;
> > 
> >  	return 0;
> >  }
> > diff --git a/p9-fsi.dtsi.m4 b/p9-fsi.dtsi.m4
> > index afa7d39..89ba387 100644
> > --- a/p9-fsi.dtsi.m4
> > +++ b/p9-fsi.dtsi.m4
> > @@ -21,6 +21,13 @@
> >  			 include(p9-pib.dts.m4)dnl
> >  		};
> > 
> > +		i2cm@1800 {
> > +			#address-cells = <0x2>;
> > +			#size-cells = <0x1>;
> > +			reg = <0x0 0x1800 0x400>;
> > +			compatible = "ibm,fsi-i2c-master";
> > +		};
> > +
> >  		hmfsi@100000 {
> >  			#address-cells = <0x2>;
> >  			#size-cells = <0x1>;
> > diff --git a/p9-kernel.dts.m4 b/p9-kernel.dts.m4
> > index 474beca..dacea83 100644
> > --- a/p9-kernel.dts.m4
> > +++ b/p9-kernel.dts.m4
> > @@ -26,7 +26,7 @@
> >  			#address-cells = <0x2>;
> >  			#size-cells = <0x1>;
> >  			reg = <0x0 0x1800 0x400>;
> > -			compatible = "ibm,kernel-i2c-master";
> > +			compatible = "ibm,fsi-i2c-master";
> >  			include(p9-i2c.dts.m4)dnl
> >  		};
> > 
> > diff --git a/src/i2c.c b/src/i2c.c
> > index 2a32ec2..32c4edd 100644
> > --- a/src/i2c.c
> > +++ b/src/i2c.c
> > @@ -27,7 +27,7 @@ static int geti2c(uint16_t port, uint32_t addr,
> > uint32_t
> > offset, uint16_t size) uint64_t data = 0xc0ffee;
> >  	struct pdbg_target *target, *selected = NULL;
> > 
> > -	for_each_path_target_class("i2cbus", target) {
> > +	for_each_path_target_class("i2cm", target) {
> >  		if (pdbg_target_probe(target) != PDBG_TARGET_ENABLED)
> >  			continue;
> >  		selected = target;
> 
>

On Wed, 10 Apr 2019 at 02:31, Alistair Popple <alistair@popple.id.au> wrote:

> > @@ -46,7 +47,10 @@ static int kernel_fsi_getcfam(struct fsi *fsi, uint32_t
> > addr64, uint32_t *value) return errno;
> >       }
> >
> > -     rc = read(fsi_fd, &tmp, 4);
> > +     if (addr == 0x1800)
> > +             rc = read(fsi_fd, &tmp2, 1);
>
> So this might clarify my comment above - I wonder what this read actually does
> at the hardware level. In this case it depends on the OpenFSI kernel driver.
>
> Joel do you know if the OpenFSI read() call supports reading a single physical
> byte from the FSI bus? (rather than reading 4 bytes from the HW and throwing
> away 3 bytes).

I had to double check the code. Yes, AFAICT we read a single byte. The
read is the size requested, we don't throw away bytes.

Cheers,

Joel

On Thursday, 11 April 2019 2:30:32 AM AEST Joel Stanley wrote:
> On Wed, 10 Apr 2019 at 02:31, Alistair Popple <alistair@popple.id.au> wrote:
> > > @@ -46,7 +47,10 @@ static int kernel_fsi_getcfam(struct fsi *fsi,
> > > uint32_t
> > > addr64, uint32_t *value) return errno;
> > > 
> > >       }
> > > 
> > > -     rc = read(fsi_fd, &tmp, 4);
> > > +     if (addr == 0x1800)
> > > +             rc = read(fsi_fd, &tmp2, 1);
> > 
> > So this might clarify my comment above - I wonder what this read actually
> > does at the hardware level. In this case it depends on the OpenFSI kernel
> > driver.
> > 
> > Joel do you know if the OpenFSI read() call supports reading a single
> > physical byte from the FSI bus? (rather than reading 4 bytes from the HW
> > and throwing away 3 bytes).
> 
> I had to double check the code. Yes, AFAICT we read a single byte. The
> read is the size requested, we don't throw away bytes.

Interesting, had no idea. We never did make that work for pdbg as I couldn't 
figure out the application.  I suppose reading a single byte from the I2C is 
valid though.
 
- Alistair

> Cheers,
> 
> Joel