[Pdbg] [PATCH 2/3] add i2cm from bmc
Rashmica Gupta
rashmica.g at gmail.com
Thu Apr 11 11:12:10 AEST 2019
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 at 1800 {
> > + #address-cells = <0x2>;
> > + #size-cells = <0x1>;
> > + reg = <0x0 0x1800 0x400>;
> > + compatible = "ibm,fsi-i2c-master";
> > + };
> > +
> > hmfsi at 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;
>
>
More information about the Pdbg
mailing list