| Message ID | 20231025121725.46028-2-m.szyprowski@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add atomic transfers to s3c24xx i2c driver | expand |
Hi Andi, On 27.10.2023 15:39, Andi Shyti wrote: > On Wed, Oct 25, 2023 at 02:17:23PM +0200, Marek Szyprowski wrote: >> To properly handle read transfers in polling mode, no waiting for the ACK >> state is needed as it will never come. Just wait a bit to ensure start >> state is on the bus and continue processing next bytes. >> >> Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support") >> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com> >> --- >> drivers/i2c/busses/i2c-s3c2410.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c >> index 127eb3805fac..f9dcb1112a61 100644 >> --- a/drivers/i2c/busses/i2c-s3c2410.c >> +++ b/drivers/i2c/busses/i2c-s3c2410.c >> @@ -216,8 +216,13 @@ static bool is_ack(struct s3c24xx_i2c *i2c) >> int tries; >> >> for (tries = 50; tries; --tries) { >> - if (readl(i2c->regs + S3C2410_IICCON) >> - & S3C2410_IICCON_IRQPEND) { >> + unsigned long tmp = readl(i2c->regs + S3C2410_IICCON); >> + >> + if (!(tmp & S3C2410_IICCON_ACKEN)) { >> + usleep_range(100, 200); >> + return true; > What is the real issue here? Is the value of S3C2410_IICCON_ACKEN > enabling/disabling irq's? It is not about the enabling/disabling interrupts, but controlling the bus state. This bit is named as 'Acknowledge generation / I2C-bus acknowledge enable bit' in Exynos reference manual: In Tx mode, the I2CSDA is idle in the ACK time. In Rx mode, the I2CSDA is low in the ACK time. So it is a part of proper controlling the bus state, not the reported interrupts, although the S3C2410_IICCON_ACKEN name is a bit misleading in this case. > Besides, if we use polling mode, shouldn't we disable the acks > already in probe (even though they are disabled by default), > never enable them before starting the message and avoid checking > here everytime? I assume that this polling mode is a special case, so there is no point in optimizing it much. It is used only by the i2c core for some special transfers to the PMIC during system reboot/shutdown or by the s3c24xx i2c controller embedded in SoC for controlling some PHYs. Till now only the second case was actually used. There were only a few single writes done this way, so noone even noticed that the other types of transfers (multi message or read) were broken... I found all those issues by enabling polling mode unconditionally and fixing it to make all my test systems working again. >> + } >> + if (tmp & S3C2410_IICCON_IRQPEND) { >> if (!(readl(i2c->regs + S3C2410_IICSTAT) >> & S3C2410_IICSTAT_LASTBIT)) >> return true; >> -- >> 2.34.1 >> Best regards
On Tue, Oct 31, 2023 at 02:49:04PM +0100, Marek Szyprowski wrote: > On 27.10.2023 15:39, Andi Shyti wrote: > > On Wed, Oct 25, 2023 at 02:17:23PM +0200, Marek Szyprowski wrote: > >> To properly handle read transfers in polling mode, no waiting for the ACK > >> state is needed as it will never come. Just wait a bit to ensure start > >> state is on the bus and continue processing next bytes. > >> > >> Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support") > >> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com> > >> --- > >> drivers/i2c/busses/i2c-s3c2410.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c > >> index 127eb3805fac..f9dcb1112a61 100644 > >> --- a/drivers/i2c/busses/i2c-s3c2410.c > >> +++ b/drivers/i2c/busses/i2c-s3c2410.c > >> @@ -216,8 +216,13 @@ static bool is_ack(struct s3c24xx_i2c *i2c) > >> int tries; > >> > >> for (tries = 50; tries; --tries) { > >> - if (readl(i2c->regs + S3C2410_IICCON) > >> - & S3C2410_IICCON_IRQPEND) { > >> + unsigned long tmp = readl(i2c->regs + S3C2410_IICCON); > >> + > >> + if (!(tmp & S3C2410_IICCON_ACKEN)) { > >> + usleep_range(100, 200); > >> + return true; > > What is the real issue here? Is the value of S3C2410_IICCON_ACKEN > > enabling/disabling irq's? > > It is not about the enabling/disabling interrupts, but controlling the > bus state. This bit is named as 'Acknowledge generation / I2C-bus > acknowledge enable bit' in Exynos reference manual: > > In Tx mode, the I2CSDA is idle in the ACK time. > > In Rx mode, the I2CSDA is low in the ACK time. > > So it is a part of proper controlling the bus state, not the reported > interrupts, although the S3C2410_IICCON_ACKEN name is a bit misleading > in this case. Yes, correct, but I still don't understand this sequence in the message_start: - enable ACKEN in IICCON (enable_ack()) - read IICCON (iiccon = readl(...)) - write what you read in IICCON (writel(iiccon, ...)) - if ACKEN is disabled in IICCON (from your patch) Where is supposed ACKEN to be disabled? > > Besides, if we use polling mode, shouldn't we disable the acks > > already in probe (even though they are disabled by default), > > never enable them before starting the message and avoid checking > > here everytime? > > > I assume that this polling mode is a special case, so there is no point > in optimizing it much. It is used only by the i2c core for some special > transfers to the PMIC during system reboot/shutdown or by the s3c24xx > i2c controller embedded in SoC for controlling some PHYs. Till now only > the second case was actually used. There were only a few single writes > done this way, so noone even noticed that the other types of transfers > (multi message or read) were broken... I found all those issues by > enabling polling mode unconditionally and fixing it to make all my test > systems working again. Yeah, I understand your point here. It would be nice to have a pure polling mode supported though. Thanks, Andi
Hi, > -----Original Message----- > From: Marek Szyprowski <m.szyprowski@samsung.com> > Sent: Wednesday, October 25, 2023 9:17 PM > To: linux-samsung-soc@vger.kernel.org; linux-i2c@vger.kernel.org > Cc: Marek Szyprowski <m.szyprowski@samsung.com>; Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org>; Alim Akhtar <alim.akhtar@samsung.com>; > Andi Shyti <andi.shyti@kernel.org>; Wolfram Sang <wsa@kernel.org> > Subject: [PATCH v2 1/3] i2c: s3c24xx: fix read transfers in polling mode > > To properly handle read transfers in polling mode, no waiting for the ACK > state is needed as it will never come. Just wait a bit to ensure start > state is on the bus and continue processing next bytes. > > Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support") > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/i2c/busses/i2c-s3c2410.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c- > s3c2410.c > index 127eb3805fac..f9dcb1112a61 100644 > --- a/drivers/i2c/busses/i2c-s3c2410.c > +++ b/drivers/i2c/busses/i2c-s3c2410.c > @@ -216,8 +216,13 @@ static bool is_ack(struct s3c24xx_i2c *i2c) > int tries; > > for (tries = 50; tries; --tries) { > - if (readl(i2c->regs + S3C2410_IICCON) > - & S3C2410_IICCON_IRQPEND) { > + unsigned long tmp = readl(i2c->regs + S3C2410_IICCON); > + > + if (!(tmp & S3C2410_IICCON_ACKEN)) { > + usleep_range(100, 200); Trivial question, but is there any hardware specification related to sleeping for 100-200 microseconds? If any then it would be nice to use const variable instead and add some description about why sleeping here is needed. Thanks, Inki Dae > + return true; > + } > + if (tmp & S3C2410_IICCON_IRQPEND) { > if (!(readl(i2c->regs + S3C2410_IICSTAT) > & S3C2410_IICSTAT_LASTBIT)) > return true; > -- > 2.34.1
On 02.11.2023 01:49, Andi Shyti wrote: > On Tue, Oct 31, 2023 at 02:49:04PM +0100, Marek Szyprowski wrote: >> On 27.10.2023 15:39, Andi Shyti wrote: >>> On Wed, Oct 25, 2023 at 02:17:23PM +0200, Marek Szyprowski wrote: >>>> To properly handle read transfers in polling mode, no waiting for the ACK >>>> state is needed as it will never come. Just wait a bit to ensure start >>>> state is on the bus and continue processing next bytes. >>>> >>>> Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support") >>>> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com> >>>> --- >>>> drivers/i2c/busses/i2c-s3c2410.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c >>>> index 127eb3805fac..f9dcb1112a61 100644 >>>> --- a/drivers/i2c/busses/i2c-s3c2410.c >>>> +++ b/drivers/i2c/busses/i2c-s3c2410.c >>>> @@ -216,8 +216,13 @@ static bool is_ack(struct s3c24xx_i2c *i2c) >>>> int tries; >>>> >>>> for (tries = 50; tries; --tries) { >>>> - if (readl(i2c->regs + S3C2410_IICCON) >>>> - & S3C2410_IICCON_IRQPEND) { >>>> + unsigned long tmp = readl(i2c->regs + S3C2410_IICCON); >>>> + >>>> + if (!(tmp & S3C2410_IICCON_ACKEN)) { >>>> + usleep_range(100, 200); >>>> + return true; >>> What is the real issue here? Is the value of S3C2410_IICCON_ACKEN >>> enabling/disabling irq's? >> It is not about the enabling/disabling interrupts, but controlling the >> bus state. This bit is named as 'Acknowledge generation / I2C-bus >> acknowledge enable bit' in Exynos reference manual: >> >> In Tx mode, the I2CSDA is idle in the ACK time. >> >> In Rx mode, the I2CSDA is low in the ACK time. >> >> So it is a part of proper controlling the bus state, not the reported >> interrupts, although the S3C2410_IICCON_ACKEN name is a bit misleading >> in this case. > Yes, correct, but I still don't understand this sequence in the > message_start: > > - enable ACKEN in IICCON (enable_ack()) > - read IICCON (iiccon = readl(...)) > - write what you read in IICCON (writel(iiccon, ...)) > - if ACKEN is disabled in IICCON (from your patch) > > Where is supposed ACKEN to be disabled? It must be cleared by the hardware when read mode is set. Frankly speaking I didn't dig much into the driver internals and respective i2c bus states. I just logged all the call paths and driver states from the driver operating with interrupts enabled and fixed the polling mode to match what I have captured with interrupts enabled. After that changes, all my test boards finally booted properly with polling mode enabled unconditionally. This ACK related logic is a bit strange, but I don't really want to change driver logic and risk other regressions, so my changes were limited only to the code reachable during polling mode. >>> Besides, if we use polling mode, shouldn't we disable the acks >>> already in probe (even though they are disabled by default), >>> never enable them before starting the message and avoid checking >>> here everytime? >> I assume that this polling mode is a special case, so there is no point >> in optimizing it much. It is used only by the i2c core for some special >> transfers to the PMIC during system reboot/shutdown or by the s3c24xx >> i2c controller embedded in SoC for controlling some PHYs. Till now only >> the second case was actually used. There were only a few single writes >> done this way, so noone even noticed that the other types of transfers >> (multi message or read) were broken... I found all those issues by >> enabling polling mode unconditionally and fixing it to make all my test >> systems working again. > Yeah, I understand your point here. > > It would be nice to have a pure polling mode supported though. Well, it is now with this patchset. It is just a matter of core to select it. Best regards
Hi Inki, On 02.11.2023 02:31, 대인기/Tizen Platform Lab(SR)/삼성전자 wrote: > >> -----Original Message----- >> From: Marek Szyprowski<m.szyprowski@samsung.com> >> Sent: Wednesday, October 25, 2023 9:17 PM >> To:linux-samsung-soc@vger.kernel.org;linux-i2c@vger.kernel.org >> Cc: Marek Szyprowski<m.szyprowski@samsung.com>; Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org>; Alim Akhtar<alim.akhtar@samsung.com>; >> Andi Shyti<andi.shyti@kernel.org>; Wolfram Sang<wsa@kernel.org> >> Subject: [PATCH v2 1/3] i2c: s3c24xx: fix read transfers in polling mode >> >> To properly handle read transfers in polling mode, no waiting for the ACK >> state is needed as it will never come. Just wait a bit to ensure start >> state is on the bus and continue processing next bytes. >> >> Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support") >> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com> >> --- >> drivers/i2c/busses/i2c-s3c2410.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c- >> s3c2410.c >> index 127eb3805fac..f9dcb1112a61 100644 >> --- a/drivers/i2c/busses/i2c-s3c2410.c >> +++ b/drivers/i2c/busses/i2c-s3c2410.c >> @@ -216,8 +216,13 @@ static bool is_ack(struct s3c24xx_i2c *i2c) >> int tries; >> >> for (tries = 50; tries; --tries) { >> - if (readl(i2c->regs + S3C2410_IICCON) >> - & S3C2410_IICCON_IRQPEND) { >> + unsigned long tmp = readl(i2c->regs + S3C2410_IICCON); >> + >> + if (!(tmp & S3C2410_IICCON_ACKEN)) { >> + usleep_range(100, 200); > Trivial question, but is there any hardware specification related to sleeping for 100-200 microseconds? If any then it would be nice to use const variable instead and add some description about why sleeping here is needed. Well, this is a bit magic value I got from my experiments. There is some delay needed there to let hardware to clear that bit and the values I proposed worked. If You don't like that, I can reuse the delay value that is already present in that loop: usleep_range(1000, 2000). Best regards
Hi Marek, > >> for (tries = 50; tries; --tries) { > >> - if (readl(i2c->regs + S3C2410_IICCON) > >> - & S3C2410_IICCON_IRQPEND) { > >> + unsigned long tmp = readl(i2c->regs + S3C2410_IICCON); > >> + > >> + if (!(tmp & S3C2410_IICCON_ACKEN)) { > >> + usleep_range(100, 200); > > Trivial question, but is there any hardware specification related to sleeping for 100-200 microseconds? If any then it would be nice to use const variable instead and add some description about why sleeping here is needed. > > > Well, this is a bit magic value I got from my experiments. There is some > delay needed there to let hardware to clear that bit and the values I > proposed worked. If You don't like that, I can reuse the delay value > that is already present in that loop: usleep_range(1000, 2000). I also wanted to comment on this... please, then, add a comment saying how you got to this range. Andi
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c index 127eb3805fac..f9dcb1112a61 100644 --- a/drivers/i2c/busses/i2c-s3c2410.c +++ b/drivers/i2c/busses/i2c-s3c2410.c @@ -216,8 +216,13 @@ static bool is_ack(struct s3c24xx_i2c *i2c) int tries; for (tries = 50; tries; --tries) { - if (readl(i2c->regs + S3C2410_IICCON) - & S3C2410_IICCON_IRQPEND) { + unsigned long tmp = readl(i2c->regs + S3C2410_IICCON); + + if (!(tmp & S3C2410_IICCON_ACKEN)) { + usleep_range(100, 200); + return true; + } + if (tmp & S3C2410_IICCON_IRQPEND) { if (!(readl(i2c->regs + S3C2410_IICSTAT) & S3C2410_IICSTAT_LASTBIT)) return true;
To properly handle read transfers in polling mode, no waiting for the ACK state is needed as it will never come. Just wait a bit to ensure start state is on the bus and continue processing next bytes. Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support") Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/i2c/busses/i2c-s3c2410.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)