| Message ID | 1565150941-27297-1-git-send-email-rayagonda.kokatanur@broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/1] i2c: iproc: Add i2c repeated start capability | expand |
Hi Wolfram, On 8/6/19 9:09 PM, Rayagonda Kokatanur wrote: > From: Lori Hikichi <lori.hikichi@broadcom.com> > > Enable handling of i2c repeated start. The current code > handles a multi msg i2c transfer as separate i2c bus > transactions. This change will now handle this case > using the i2c repeated start protocol. The number of msgs > in a transfer is limited to two, and must be a write > followed by a read. > > Signed-off-by: Lori Hikichi <lori.hikichi@broadcom.com> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com> > Signed-off-by: Icarus Chau <icarus.chau@broadcom.com> > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > Signed-off-by: Shivaraj Shetty <sshetty1@broadcom.com> > --- Note this patch has gone through internal review and testing on various I2C slave devices. It is introduced to work around limitation of our I2C controller and allows it to work on certain I2C slave devices that are sensitive and requires repeated start between transactions instead of a stop. Given that my name is also on the Signed-off-by since I helped to rewrite part of the patch, I'm not going to add my Reviewed-by tag here. Please help to review. Thanks, Ray > drivers/i2c/busses/i2c-bcm-iproc.c | 70 +++++++++++++++++++++++++++++++------- > 1 file changed, 57 insertions(+), 13 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c > index d7fd76b..15fedcf 100644 > --- a/drivers/i2c/busses/i2c-bcm-iproc.c > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c > @@ -81,6 +81,7 @@ > #define M_CMD_PROTOCOL_MASK 0xf > #define M_CMD_PROTOCOL_BLK_WR 0x7 > #define M_CMD_PROTOCOL_BLK_RD 0x8 > +#define M_CMD_PROTOCOL_PROCESS 0xa > #define M_CMD_PEC_SHIFT 8 > #define M_CMD_RD_CNT_SHIFT 0 > #define M_CMD_RD_CNT_MASK 0xff > @@ -675,13 +676,20 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c, > return 0; > } > > -static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, > - struct i2c_msg *msg) > +/* > + * If 'process_call' is true, then this is a multi-msg transfer that requires > + * a repeated start between the messages. > + * More specifically, it must be a write (reg) followed by a read (data). > + * The i2c quirks are set to enforce this rule. > + */ > +static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c, > + struct i2c_msg *msgs, bool process_call) > { > int i; > u8 addr; > u32 val, tmp, val_intr_en; > unsigned int tx_bytes; > + struct i2c_msg *msg = &msgs[0]; > > /* check if bus is busy */ > if (!!(iproc_i2c_rd_reg(iproc_i2c, > @@ -707,14 +715,29 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, > val = msg->buf[i]; > > /* mark the last byte */ > - if (i == msg->len - 1) > - val |= BIT(M_TX_WR_STATUS_SHIFT); > + if (!process_call && (i == msg->len - 1)) > + val |= 1 << M_TX_WR_STATUS_SHIFT; > > iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val); > } > iproc_i2c->tx_bytes = tx_bytes; > } > > + /* Process the read message if this is process call */ > + if (process_call) { > + msg++; > + iproc_i2c->msg = msg; /* point to second msg */ > + > + /* > + * The last byte to be sent out should be a slave > + * address with read operation > + */ > + addr = msg->addr << 1 | 1; > + /* mark it the last byte out */ > + val = addr | (1 << M_TX_WR_STATUS_SHIFT); > + iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val); > + } > + > /* mark as incomplete before starting the transaction */ > if (iproc_i2c->irq) > reinit_completion(&iproc_i2c->done); > @@ -733,7 +756,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, > * underrun interrupt, which will be triggerred when the TX FIFO is > * empty. When that happens we can then pump more data into the FIFO > */ > - if (!(msg->flags & I2C_M_RD) && > + if (!process_call && !(msg->flags & I2C_M_RD) && > msg->len > iproc_i2c->tx_bytes) > val_intr_en |= BIT(IE_M_TX_UNDERRUN_SHIFT); > > @@ -743,6 +766,8 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, > */ > val = BIT(M_CMD_START_BUSY_SHIFT); > if (msg->flags & I2C_M_RD) { > + u32 protocol; > + > iproc_i2c->rx_bytes = 0; > if (msg->len > M_RX_FIFO_MAX_THLD_VALUE) > iproc_i2c->thld_bytes = M_RX_FIFO_THLD_VALUE; > @@ -758,7 +783,10 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, > /* enable the RX threshold interrupt */ > val_intr_en |= BIT(IE_M_RX_THLD_SHIFT); > > - val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | > + protocol = process_call ? > + M_CMD_PROTOCOL_PROCESS : M_CMD_PROTOCOL_BLK_RD; > + > + val |= (protocol << M_CMD_PROTOCOL_SHIFT) | > (msg->len << M_CMD_RD_CNT_SHIFT); > } else { > val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); > @@ -774,17 +802,31 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter, > struct i2c_msg msgs[], int num) > { > struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adapter); > - int ret, i; > + bool process_call = false; > + int ret; > > - /* go through all messages */ > - for (i = 0; i < num; i++) { > - ret = bcm_iproc_i2c_xfer_single_msg(iproc_i2c, &msgs[i]); > - if (ret) { > - dev_dbg(iproc_i2c->device, "xfer failed\n"); > - return ret; > + if (num > 2) { > + dev_err(iproc_i2c->device, > + "Only support up to 2 messages. Current msg count %d\n", > + num); > + return -EOPNOTSUPP; > + } > + > + if (num == 2) { > + /* Repeated start, use process call */ > + process_call = true; > + if (msgs[1].flags & I2C_M_NOSTART) { > + dev_err(iproc_i2c->device, "Invalid repeated start\n"); > + return -EOPNOTSUPP; > } > } > > + ret = bcm_iproc_i2c_xfer_internal(iproc_i2c, msgs, process_call); > + if (ret) { > + dev_dbg(iproc_i2c->device, "xfer failed\n"); > + return ret; > + } > + > return num; > } > > @@ -806,6 +848,8 @@ static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap) > }; > > static struct i2c_adapter_quirks bcm_iproc_i2c_quirks = { > + .flags = I2C_AQ_COMB_WRITE_THEN_READ, > + .max_comb_1st_msg_len = M_TX_RX_FIFO_SIZE, > .max_read_len = M_RX_MAX_READ_LEN, > }; > >
> Given that my name is also on the Signed-off-by since I helped to rewrite > part of the patch, I'm not going to add my Reviewed-by tag here. > > Please help to review. Outstanding iproc patches are next in my queue.
Hi everyone, > +/* > + * If 'process_call' is true, then this is a multi-msg transfer that requires > + * a repeated start between the messages. > + * More specifically, it must be a write (reg) followed by a read (data). > + * The i2c quirks are set to enforce this rule. > + */ With all the limitations in place, I wonder if it might be easier to implement an smbus_xfer callback instead? What is left that makes this controller more than SMBus and real I2C? > + /* Process the read message if this is process call */ Also, the term "process call" here seriously sounds like SMBus. > + addr = msg->addr << 1 | 1; addr = i2c_8bit_addr_from_msg(msg); > + u32 protocol; Hmm, another SMBus terminology. > + if (num > 2) { > + dev_err(iproc_i2c->device, > + "Only support up to 2 messages. Current msg count %d\n", > + num); > + return -EOPNOTSUPP; > + } With your quirks flags set, the core checks it for you. Kind regards, Wolfram
On 8/30/19 5:56 AM, Wolfram Sang wrote: > Hi everyone, > >> +/* >> + * If 'process_call' is true, then this is a multi-msg transfer that requires >> + * a repeated start between the messages. >> + * More specifically, it must be a write (reg) followed by a read (data). >> + * The i2c quirks are set to enforce this rule. >> + */ > > With all the limitations in place, I wonder if it might be easier to > implement an smbus_xfer callback instead? What is left that makes this > controller more than SMBus and real I2C? > Right. But what is the implication of using smbus_xfer instead of master_xfer in our driver? Does it mean it will break existing functions of the i2c app that our customers developed based on i2cdev (e.g., I2C_RDWR)? 1) Does >> + /* Process the read message if this is process call */ > > Also, the term "process call" here seriously sounds like SMBus. > >> + addr = msg->addr << 1 | 1; > > addr = i2c_8bit_addr_from_msg(msg); > >> + u32 protocol; > > Hmm, another SMBus terminology. > > >> + if (num > 2) { >> + dev_err(iproc_i2c->device, >> + "Only support up to 2 messages. Current msg count %d\n", >> + num); >> + return -EOPNOTSUPP; >> + } > > With your quirks flags set, the core checks it for you. > > Kind regards, > > Wolfram >
Hi Ray, > > With all the limitations in place, I wonder if it might be easier to > > implement an smbus_xfer callback instead? What is left that makes this > > controller more than SMBus and real I2C? > > > > Right. But what is the implication of using smbus_xfer instead of > master_xfer in our driver? > > Does it mean it will break existing functions of the i2c app that our > customers developed based on i2cdev (e.g., I2C_RDWR)? If the customers uses I2C_RDWR (and it cannot be mapped to i2c_smbus_* calls) then this is an indication that there is some I2C functionality left which the HW can provide. I'd be interested which one, though. > > 1) Does Maybe you wanted to describe it here and it got accidently cut off? Regards, Wolfram
On 8/31/19 2:49 AM, Wolfram Sang wrote: > Hi Ray, > >>> With all the limitations in place, I wonder if it might be easier to >>> implement an smbus_xfer callback instead? What is left that makes this >>> controller more than SMBus and real I2C? >>> >> >> Right. But what is the implication of using smbus_xfer instead of >> master_xfer in our driver? >> >> Does it mean it will break existing functions of the i2c app that our >> customers developed based on i2cdev (e.g., I2C_RDWR)? > > If the customers uses I2C_RDWR (and it cannot be mapped to i2c_smbus_* > calls) then this is an indication that there is some I2C functionality > left which the HW can provide. I'd be interested which one, though. > >> >> 1) Does > > Maybe you wanted to describe it here and it got accidently cut off? > I think you are right that the controller does not seem to support additional I2C features in addition to SMBUS. However, my concern of switching to the smbus_xfer API is: 1) Some customers might have used I2C_RDWR based API from i2cdev. Changing from master_xfer to smbus_xfer may break the existing applications that are already developed. 2) The sound subsystem I2C regmap based implementation seems to be using i2c_ based API instead of smbus_ based API. Does this mean this will also break most of the audio codec drivers with I2C regmap API based usage? Thanks, Ray > Regards, > > Wolfram >
> I think you are right that the controller does not seem to support > additional I2C features in addition to SMBUS. > > However, my concern of switching to the smbus_xfer API is: > > 1) Some customers might have used I2C_RDWR based API from i2cdev. Changing > from master_xfer to smbus_xfer may break the existing applications that are > already developed. Well, given that you add new quirks in the original patch here, you are kind of breaking it already. Most transfers which are not SMBus-alike transfers would now be rejected. For SMBus-alike transfers which are sent via I2C_RDWR (which is ugly), I have to think about it. > 2) The sound subsystem I2C regmap based implementation seems to be using > i2c_ based API instead of smbus_ based API. Does this mean this will also > break most of the audio codec drivers with I2C regmap API based usage? I don't think so. If you check regmap_get_i2c_bus() then it checks the adapter functionality and chooses the best transfer option then. I may be missing something but I would wonder if the sound system does something special and different.
Hi Wolfram, On 9/4/19 2:37 PM, Wolfram Sang wrote: > >> I think you are right that the controller does not seem to support >> additional I2C features in addition to SMBUS. >> >> However, my concern of switching to the smbus_xfer API is: >> >> 1) Some customers might have used I2C_RDWR based API from i2cdev. Changing >> from master_xfer to smbus_xfer may break the existing applications that are >> already developed. > > Well, given that you add new quirks in the original patch here, you are > kind of breaking it already. Most transfers which are not SMBus-alike > transfers would now be rejected. For SMBus-alike transfers which are > sent via I2C_RDWR (which is ugly), I have to think about it. > >> 2) The sound subsystem I2C regmap based implementation seems to be using >> i2c_ based API instead of smbus_ based API. Does this mean this will also >> break most of the audio codec drivers with I2C regmap API based usage? > > I don't think so. If you check regmap_get_i2c_bus() then it checks the > adapter functionality and chooses the best transfer option then. I may > be missing something but I would wonder if the sound system does > something special and different. > We did more investigation on this. First of all, like you said, there's no concern on regmap based API, the smbus_xfer only based approach should just work. Secondly, for most i2ctools like i2cget, i2cset, i2cdump, there's no concern either, given that they already use I2C_SMBUS based IOCTL. However, for i2ctransfer or any customer applications that use I2C_RDWR IOCTL, i2c_transfer (master_xfer) is the only supported function. And we can confirm we do have at least one customer using i2ctransfer for EEPROM access on their system, e.g., i2ctransfer 1 w2@0x50 0x00 0x00 r64. In my opinion, it's probably better to continue to support master_xfer in our driver (with obvious limitations), in order to allow i2ctransfer (or any apps that use I2C RDWR) to continue to work. What do you think? Regards, Ray
> In my opinion, it's probably better to continue to support master_xfer in > our driver (with obvious limitations), in order to allow i2ctransfer (or any > apps that use I2C RDWR) to continue to work. > > What do you think? Yes, don't break it for users. We should have paid more attention to it in the beginning. But, while not ideal, it is not such a big deal to keep it like this. Thanks for your investigations!
On 9/24/19 11:57 AM, Wolfram Sang wrote: > >> In my opinion, it's probably better to continue to support master_xfer in >> our driver (with obvious limitations), in order to allow i2ctransfer (or any >> apps that use I2C RDWR) to continue to work. >> >> What do you think? > > Yes, don't break it for users. We should have paid more attention to it > in the beginning. But, while not ideal, it is not such a big deal to > keep it like this. > > Thanks for your investigations! > Thanks, Wolfram. Let's please continue with the review process on the current patch then? Note the patch was already internally reviewed by me. Please help to review it and let us know if there's any change that needs to be made? Regards, Ray
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c index d7fd76b..15fedcf 100644 --- a/drivers/i2c/busses/i2c-bcm-iproc.c +++ b/drivers/i2c/busses/i2c-bcm-iproc.c @@ -81,6 +81,7 @@ #define M_CMD_PROTOCOL_MASK 0xf #define M_CMD_PROTOCOL_BLK_WR 0x7 #define M_CMD_PROTOCOL_BLK_RD 0x8 +#define M_CMD_PROTOCOL_PROCESS 0xa #define M_CMD_PEC_SHIFT 8 #define M_CMD_RD_CNT_SHIFT 0 #define M_CMD_RD_CNT_MASK 0xff @@ -675,13 +676,20 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c, return 0; } -static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, - struct i2c_msg *msg) +/* + * If 'process_call' is true, then this is a multi-msg transfer that requires + * a repeated start between the messages. + * More specifically, it must be a write (reg) followed by a read (data). + * The i2c quirks are set to enforce this rule. + */ +static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c, + struct i2c_msg *msgs, bool process_call) { int i; u8 addr; u32 val, tmp, val_intr_en; unsigned int tx_bytes; + struct i2c_msg *msg = &msgs[0]; /* check if bus is busy */ if (!!(iproc_i2c_rd_reg(iproc_i2c, @@ -707,14 +715,29 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, val = msg->buf[i]; /* mark the last byte */ - if (i == msg->len - 1) - val |= BIT(M_TX_WR_STATUS_SHIFT); + if (!process_call && (i == msg->len - 1)) + val |= 1 << M_TX_WR_STATUS_SHIFT; iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val); } iproc_i2c->tx_bytes = tx_bytes; } + /* Process the read message if this is process call */ + if (process_call) { + msg++; + iproc_i2c->msg = msg; /* point to second msg */ + + /* + * The last byte to be sent out should be a slave + * address with read operation + */ + addr = msg->addr << 1 | 1; + /* mark it the last byte out */ + val = addr | (1 << M_TX_WR_STATUS_SHIFT); + iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val); + } + /* mark as incomplete before starting the transaction */ if (iproc_i2c->irq) reinit_completion(&iproc_i2c->done); @@ -733,7 +756,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, * underrun interrupt, which will be triggerred when the TX FIFO is * empty. When that happens we can then pump more data into the FIFO */ - if (!(msg->flags & I2C_M_RD) && + if (!process_call && !(msg->flags & I2C_M_RD) && msg->len > iproc_i2c->tx_bytes) val_intr_en |= BIT(IE_M_TX_UNDERRUN_SHIFT); @@ -743,6 +766,8 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, */ val = BIT(M_CMD_START_BUSY_SHIFT); if (msg->flags & I2C_M_RD) { + u32 protocol; + iproc_i2c->rx_bytes = 0; if (msg->len > M_RX_FIFO_MAX_THLD_VALUE) iproc_i2c->thld_bytes = M_RX_FIFO_THLD_VALUE; @@ -758,7 +783,10 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c, /* enable the RX threshold interrupt */ val_intr_en |= BIT(IE_M_RX_THLD_SHIFT); - val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) | + protocol = process_call ? + M_CMD_PROTOCOL_PROCESS : M_CMD_PROTOCOL_BLK_RD; + + val |= (protocol << M_CMD_PROTOCOL_SHIFT) | (msg->len << M_CMD_RD_CNT_SHIFT); } else { val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT); @@ -774,17 +802,31 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg msgs[], int num) { struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adapter); - int ret, i; + bool process_call = false; + int ret; - /* go through all messages */ - for (i = 0; i < num; i++) { - ret = bcm_iproc_i2c_xfer_single_msg(iproc_i2c, &msgs[i]); - if (ret) { - dev_dbg(iproc_i2c->device, "xfer failed\n"); - return ret; + if (num > 2) { + dev_err(iproc_i2c->device, + "Only support up to 2 messages. Current msg count %d\n", + num); + return -EOPNOTSUPP; + } + + if (num == 2) { + /* Repeated start, use process call */ + process_call = true; + if (msgs[1].flags & I2C_M_NOSTART) { + dev_err(iproc_i2c->device, "Invalid repeated start\n"); + return -EOPNOTSUPP; } } + ret = bcm_iproc_i2c_xfer_internal(iproc_i2c, msgs, process_call); + if (ret) { + dev_dbg(iproc_i2c->device, "xfer failed\n"); + return ret; + } + return num; } @@ -806,6 +848,8 @@ static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap) }; static struct i2c_adapter_quirks bcm_iproc_i2c_quirks = { + .flags = I2C_AQ_COMB_WRITE_THEN_READ, + .max_comb_1st_msg_len = M_TX_RX_FIFO_SIZE, .max_read_len = M_RX_MAX_READ_LEN, };