| Message ID | 20210626102806.15402-4-raviteja.narayanam@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i2c: xiic: Add features, bug fixes. | expand |
W dniu 26.06.2021 o 12:27, Raviteja Narayanam pisze: > Xilinx I2C IP has two modes of operation, both of which implement > I2C transactions. The only difference from sw perspective is the > programming sequence for these modes. > Dynamic mode -> Simple to program, less number of steps in sequence. > Standard mode -> Gives flexibility, more number of steps in sequence. > > In dynamic mode, during the i2c-read transactions, if there is a > delay(> 200us) between the register writes (address & byte count), > read transaction fails. On a system with load, this scenario is > occurring frequently. > To avoid this, switch to standard mode if there is a read request. > > Added a quirk to identify the IP version effected by this and follow > the standard mode. > > Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com> [...] If those two modes only differ in software complexity but we are not able to support only the simpler one and we have support for the more complicated (standard mode) anyways, we know that standard mode can handle or the cases while dynamic mode cannot, we also know that dynamic mode is broken on some versions of the core, why do we actually keep support for dynamic mode? Krzysztof
On 6/29/22 14:47, Krzysztof Adamski wrote: > W dniu 26.06.2021 o 12:27, Raviteja Narayanam pisze: >> Xilinx I2C IP has two modes of operation, both of which implement >> I2C transactions. The only difference from sw perspective is the >> programming sequence for these modes. >> Dynamic mode -> Simple to program, less number of steps in sequence. >> Standard mode -> Gives flexibility, more number of steps in sequence. >> >> In dynamic mode, during the i2c-read transactions, if there is a >> delay(> 200us) between the register writes (address & byte count), >> read transaction fails. On a system with load, this scenario is >> occurring frequently. >> To avoid this, switch to standard mode if there is a read request. >> >> Added a quirk to identify the IP version effected by this and follow >> the standard mode. >> >> Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com> > > [...] > > If those two modes only differ in software complexity but we are not > able to support only the simpler one and we have support for the more > complicated (standard mode) anyways, we know that standard mode > can handle or the cases while dynamic mode cannot, we also know that > dynamic mode is broken on some versions of the core, why do we actually > keep support for dynamic mode? If I recall it right, the dynamic mode was supposed to handle transfers longer than 255 Bytes, which the core cannot do in Standard mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot of time debugging the race conditions in the XIIC, which I ultimately fixed (the patches are upstream), but the long transfers I rather fixed in the MXT driver instead. I also recall there was supposed to be some update for the XIIC core coming with newer vivado, but I might be wrong about that.
Hi Marek, W dniu 29.06.2022 o 16:05, Marek Vasut pisze: >> [...] >> >> If those two modes only differ in software complexity but we are not >> able to support only the simpler one and we have support for the more >> complicated (standard mode) anyways, we know that standard mode >> can handle or the cases while dynamic mode cannot, we also know that >> dynamic mode is broken on some versions of the core, why do we actually >> keep support for dynamic mode? > > If I recall it right, the dynamic mode was supposed to handle > transfers longer than 255 Bytes, which the core cannot do in Standard > mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot > of time debugging the race conditions in the XIIC, which I ultimately > fixed (the patches are upstream), but the long transfers I rather > fixed in the MXT driver instead. > > I also recall there was supposed to be some update for the XIIC core > coming with newer vivado, but I might be wrong about that. It seems to be the other way around - dynamic mode is limited to 255 bytes - when you trigger dynamic mode you first write the address of the slave to the FIFO, then you write the length as one byte so you can't request more than 255 bytes. So *standard* mode is used for those messages. In other words - dynamic mode is the one that is more limited - everything that you can do in dynamic mode you can also do in standard mode. So why don't we use standard mode always for everything? Krzysztof
On 6/29/22 16:09, Krzysztof Adamski wrote: > Hi Marek, > > W dniu 29.06.2022 o 16:05, Marek Vasut pisze: >>> [...] >>> >>> If those two modes only differ in software complexity but we are not >>> able to support only the simpler one and we have support for the more >>> complicated (standard mode) anyways, we know that standard mode >>> can handle or the cases while dynamic mode cannot, we also know that >>> dynamic mode is broken on some versions of the core, why do we actually >>> keep support for dynamic mode? >> >> If I recall it right, the dynamic mode was supposed to handle >> transfers longer than 255 Bytes, which the core cannot do in Standard >> mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot >> of time debugging the race conditions in the XIIC, which I ultimately >> fixed (the patches are upstream), but the long transfers I rather >> fixed in the MXT driver instead. >> >> I also recall there was supposed to be some update for the XIIC core >> coming with newer vivado, but I might be wrong about that. > > It seems to be the other way around - dynamic mode is limited to 255 > bytes - when you trigger dynamic mode you first write the address of the > slave to the FIFO, then you write the length as one byte so you can't > request more than 255 bytes. So *standard* mode is used for those > messages. In other words - dynamic mode is the one that is more limited > - everything that you can do in dynamic mode you can also do in standard > mode. So why don't we use standard mode always for everything? Sigh, it's been a year since I looked into this, sorry. One of the modes is maybe not supported on all the XIIC core instances ?
[AMD Official Use Only - General] Hi Krzysztof, > -----Original Message----- > From: Krzysztof Adamski <krzysztof.adamski@nokia.com> > Sent: Wednesday, June 29, 2022 7:40 PM > To: Marek Vasut <marex@denx.de>; Raviteja Narayanam > <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org; > michal.simek@xilinx.com > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > git@xilinx.com; joe@perches.com > Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c- > read > > [CAUTION: External Email] > > CAUTION: This message has originated from an External Source. Please use > proper judgment and caution when opening attachments, clicking links, or > responding to this email. > > > Hi Marek, > > W dniu 29.06.2022 o 16:05, Marek Vasut pisze: > >> [...] > >> > >> If those two modes only differ in software complexity but we are not > >> able to support only the simpler one and we have support for the more > >> complicated (standard mode) anyways, we know that standard mode can > >> handle or the cases while dynamic mode cannot, we also know that > >> dynamic mode is broken on some versions of the core, why do we > >> actually keep support for dynamic mode? > > > > If I recall it right, the dynamic mode was supposed to handle > > transfers longer than 255 Bytes, which the core cannot do in Standard > > mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot > > of time debugging the race conditions in the XIIC, which I ultimately > > fixed (the patches are upstream), but the long transfers I rather > > fixed in the MXT driver instead. > > > > I also recall there was supposed to be some update for the XIIC core > > coming with newer vivado, but I might be wrong about that. > > It seems to be the other way around - dynamic mode is limited to 255 bytes - > when you trigger dynamic mode you first write the address of the slave to > the FIFO, then you write the length as one byte so you can't request more > than 255 bytes. So *standard* mode is used for those messages. In other > words - dynamic mode is the one that is more limited > - everything that you can do in dynamic mode you can also do in standard > mode. So why don't we use standard mode always for everything? However the current mode is dynamic mode so for less than 255 we can use dynamic mode.(the current behavior will not change) Also the dynamic mode is nicer on the processor resources. We set the bytes and the controller takes care of transferring. However do not have any strong views open to suggestions. > > Krzysztof
W dniu 30.06.2022 o 10:23, Datta, Shubhrajyoti pisze: > [AMD Official Use Only - General] > > Hi Krzysztof, > >> -----Original Message----- >> From: Krzysztof Adamski <krzysztof.adamski@nokia.com> >> Sent: Wednesday, June 29, 2022 7:40 PM >> To: Marek Vasut <marex@denx.de>; Raviteja Narayanam >> <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org; >> michal.simek@xilinx.com >> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; >> git@xilinx.com; joe@perches.com >> Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c- >> read >> >> [CAUTION: External Email] >> >> CAUTION: This message has originated from an External Source. Please use >> proper judgment and caution when opening attachments, clicking links, or >> responding to this email. >> >> >> Hi Marek, >> >> W dniu 29.06.2022 o 16:05, Marek Vasut pisze: >>>> [...] >>>> >>>> If those two modes only differ in software complexity but we are not >>>> able to support only the simpler one and we have support for the more >>>> complicated (standard mode) anyways, we know that standard mode can >>>> handle or the cases while dynamic mode cannot, we also know that >>>> dynamic mode is broken on some versions of the core, why do we >>>> actually keep support for dynamic mode? >>> If I recall it right, the dynamic mode was supposed to handle >>> transfers longer than 255 Bytes, which the core cannot do in Standard >>> mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot >>> of time debugging the race conditions in the XIIC, which I ultimately >>> fixed (the patches are upstream), but the long transfers I rather >>> fixed in the MXT driver instead. >>> >>> I also recall there was supposed to be some update for the XIIC core >>> coming with newer vivado, but I might be wrong about that. >> It seems to be the other way around - dynamic mode is limited to 255 bytes - >> when you trigger dynamic mode you first write the address of the slave to >> the FIFO, then you write the length as one byte so you can't request more >> than 255 bytes. So *standard* mode is used for those messages. In other >> words - dynamic mode is the one that is more limited >> - everything that you can do in dynamic mode you can also do in standard >> mode. So why don't we use standard mode always for everything? > However the current mode is dynamic mode so for less than 255 we can use dynamic mode.(the current behavior will not change) > Also the dynamic mode is nicer on the processor resources. We set the bytes and the controller takes care of > transferring. > > However do not have any strong views open to suggestions. All I'm saying is that before this patchset, the dynamic mode was used in all cases and it made sense - it is easier to work with. But it turned out it has its limitations and support for standard mode was added with several cases that switch to that mode. The commit message suggests that the only difference is in how complicated the code for handling them is, does not say why dynamic mode might still be preferred. And supporting both of them complicates the code noticeably. My understanding now is that the code struggles to use the dynamic mode in all cases that it can because that produces less interrupts and so it is slightly lighter on resources. So it is a code complication vs effectiveness tradeoff. Since this is I2C - a slow bus, I'm not sure it is worth it but also don't have strong opinion on that. If nothing else, I think it would make sense to update the commit message a little bit to better explain why it is worth keeping both modes. Krzysztof
[AMD Official Use Only - General] > -----Original Message----- > From: Krzysztof Adamski <krzysztof.adamski@nokia.com> > Sent: Friday, July 1, 2022 12:32 PM > To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; Marek Vasut > <marex@denx.de>; Raviteja Narayanam <raviteja.narayanam@xilinx.com>; > linux-i2c@vger.kernel.org; michal.simek@xilinx.com > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > git@xilinx.com; joe@perches.com > Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c- > read > > [CAUTION: External Email] > > W dniu 30.06.2022 o 10:23, Datta, Shubhrajyoti pisze: > > [AMD Official Use Only - General] > > > > Hi Krzysztof, > > > >> -----Original Message----- > >> From: Krzysztof Adamski <krzysztof.adamski@nokia.com> > >> Sent: Wednesday, June 29, 2022 7:40 PM > >> To: Marek Vasut <marex@denx.de>; Raviteja Narayanam > >> <raviteja.narayanam@xilinx.com>; linux-i2c@vger.kernel.org; > >> michal.simek@xilinx.com > >> Cc: linux-arm-kernel@lists.infradead.org; > >> linux-kernel@vger.kernel.org; git@xilinx.com; joe@perches.com > >> Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode > >> for i2c- read > >> > >> [CAUTION: External Email] > >> > >> CAUTION: This message has originated from an External Source. Please > >> use proper judgment and caution when opening attachments, clicking > >> links, or responding to this email. > >> > >> > >> Hi Marek, > >> > >> W dniu 29.06.2022 o 16:05, Marek Vasut pisze: > >>>> [...] > >>>> > >>>> If those two modes only differ in software complexity but we are > >>>> not able to support only the simpler one and we have support for > >>>> the more complicated (standard mode) anyways, we know that > standard > >>>> mode can handle or the cases while dynamic mode cannot, we also > >>>> know that dynamic mode is broken on some versions of the core, why > >>>> do we actually keep support for dynamic mode? > >>> If I recall it right, the dynamic mode was supposed to handle > >>> transfers longer than 255 Bytes, which the core cannot do in > >>> Standard mode. It is needed e.g. by Atmel MXT touch controller. I > >>> spent a lot of time debugging the race conditions in the XIIC, which > >>> I ultimately fixed (the patches are upstream), but the long > >>> transfers I rather fixed in the MXT driver instead. > >>> > >>> I also recall there was supposed to be some update for the XIIC core > >>> coming with newer vivado, but I might be wrong about that. > >> It seems to be the other way around - dynamic mode is limited to 255 > >> bytes - when you trigger dynamic mode you first write the address of > >> the slave to the FIFO, then you write the length as one byte so you > >> can't request more than 255 bytes. So *standard* mode is used for > >> those messages. In other words - dynamic mode is the one that is more > >> limited > >> - everything that you can do in dynamic mode you can also do in > >> standard mode. So why don't we use standard mode always for > everything? > > However the current mode is dynamic mode so for less than 255 we can > > use dynamic mode.(the current behavior will not change) Also the > > dynamic mode is nicer on the processor resources. We set the bytes and > the controller takes care of transferring. > > > > However do not have any strong views open to suggestions. > > All I'm saying is that before this patchset, the dynamic mode was used in all > cases and it made sense - it is easier to work with. But it turned out it has its > limitations and support for standard mode was added with several cases that > switch to that mode. The commit message suggests that the only difference is > in how complicated the code for handling them is, does not say why dynamic > mode might still be preferred. And supporting both of them complicates the > code noticeably. > My understanding now is that the code struggles to use the dynamic mode in > all cases that it can because that produces less interrupts and so it is slightly > lighter on resources. So it is a code complication vs effectiveness tradeoff. > Since this is I2C - a slow bus, I'm not sure it is worth it but also don't have > strong opinion on that. If nothing else, I think it would make sense to update > the commit message a little bit to better explain why it is worth keeping both > modes. Will update the commit message in the next version. > > Krzysztof
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index 004103267e9c..c31d0d0a8384 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -33,6 +33,8 @@ #define DRIVER_NAME "xiic-i2c" +#define DYNAMIC_MODE_READ_BROKEN_BIT BIT(0) + enum xilinx_i2c_state { STATE_DONE, STATE_ERROR, @@ -62,6 +64,7 @@ enum xiic_endian { * @singlemaster: Indicates bus is single master * @dynamic: Mode of controller * @prev_msg_tx: Previous message is Tx + * @quirks: To hold platform specific bug info */ struct xiic_i2c { struct device *dev; @@ -80,8 +83,12 @@ struct xiic_i2c { bool singlemaster; bool dynamic; bool prev_msg_tx; + u32 quirks; }; +struct xiic_version_data { + u32 quirks; +}; #define XIIC_MSB_OFFSET 0 #define XIIC_REG_OFFSET (0x100+XIIC_MSB_OFFSET) @@ -963,6 +970,7 @@ static int xiic_start_xfer(struct xiic_i2c *i2c) static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) { + bool broken_read, max_read_len; struct xiic_i2c *i2c = i2c_get_adapdata(adap); int err, count; @@ -986,10 +994,21 @@ static int xiic_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) /* Initialize prev message type */ i2c->prev_msg_tx = false; - /* Enter standard mode only when read length is > 255 bytes */ + /* + * Scan through nmsgs, use dynamic mode when none of the below two + * conditions occur. We need standard mode even if one condition holds + * true in the entire array of messages in a single transfer. + * If read transaction as dynamic mode is broken for delayed reads + * in xlnx,axi-iic-2.0 / xlnx,xps-iic-2.00.a IP versions. + * If read length is > 255 bytes. + */ for (count = 0; count < i2c->nmsgs; count++) { - if ((i2c->tx_msg[count].flags & I2C_M_RD) && - i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC) { + broken_read = (i2c->quirks & DYNAMIC_MODE_READ_BROKEN_BIT) && + (i2c->tx_msg[count].flags & I2C_M_RD); + max_read_len = (i2c->tx_msg[count].flags & I2C_M_RD) && + (i2c->tx_msg[count].len > MAX_READ_LENGTH_DYNAMIC); + + if (broken_read || max_read_len) { i2c->dynamic = false; break; } @@ -1035,11 +1054,23 @@ static const struct i2c_adapter xiic_adapter = { .algo = &xiic_algorithm, }; +static const struct xiic_version_data xiic_2_00 = { + .quirks = DYNAMIC_MODE_READ_BROKEN_BIT, +}; + +#if defined(CONFIG_OF) +static const struct of_device_id xiic_of_match[] = { + { .compatible = "xlnx,xps-iic-2.00.a", .data = &xiic_2_00 }, + {}, +}; +MODULE_DEVICE_TABLE(of, xiic_of_match); +#endif static int xiic_i2c_probe(struct platform_device *pdev) { struct xiic_i2c *i2c; struct xiic_i2c_platform_data *pdata; + const struct of_device_id *match; struct resource *res; int ret, irq; u8 i; @@ -1049,6 +1080,13 @@ static int xiic_i2c_probe(struct platform_device *pdev) if (!i2c) return -ENOMEM; + match = of_match_node(xiic_of_match, pdev->dev.of_node); + if (match && match->data) { + const struct xiic_version_data *data = match->data; + + i2c->quirks = data->quirks; + } + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); i2c->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(i2c->base)) @@ -1129,6 +1167,8 @@ static int xiic_i2c_probe(struct platform_device *pdev) i2c_new_client_device(&i2c->adap, pdata->devices + i); } + dev_info(&pdev->dev, "mmio %08lx irq %d\n", (unsigned long)res->start, irq); + return 0; err_clk_dis: @@ -1160,14 +1200,6 @@ static int xiic_i2c_remove(struct platform_device *pdev) return 0; } -#if defined(CONFIG_OF) -static const struct of_device_id xiic_of_match[] = { - { .compatible = "xlnx,xps-iic-2.00.a", }, - {}, -}; -MODULE_DEVICE_TABLE(of, xiic_of_match); -#endif - static int __maybe_unused xiic_i2c_runtime_suspend(struct device *dev) { struct xiic_i2c *i2c = dev_get_drvdata(dev);
Xilinx I2C IP has two modes of operation, both of which implement I2C transactions. The only difference from sw perspective is the programming sequence for these modes. Dynamic mode -> Simple to program, less number of steps in sequence. Standard mode -> Gives flexibility, more number of steps in sequence. In dynamic mode, during the i2c-read transactions, if there is a delay(> 200us) between the register writes (address & byte count), read transaction fails. On a system with load, this scenario is occurring frequently. To avoid this, switch to standard mode if there is a read request. Added a quirk to identify the IP version effected by this and follow the standard mode. Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com> --- drivers/i2c/busses/i2c-xiic.c | 54 ++++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 11 deletions(-)