| Message ID | 20180529105847.15663-2-wsa+renesas@sang-engineering.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,1/1] i2c: rcar: handle RXDMA HW bug on Gen3 | expand |
Hi Wolfram-san, Thank you for the patch! > From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM > Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 If possible, I'd like to replace "bug" with "specification" or other words :) <snip> > @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, > > pm_runtime_get_sync(dev); > > + /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */ > + if (priv->devtype == I2C_RCAR_GEN3) { > + priv->flags |= ID_P_NO_RXDMA; > + if (!IS_ERR(priv->rstc)) { > + ret = reset_control_reset(priv->rstc); According to the datasheet Rev.1.00 page 57-69, we should do: reset_control_assert(); udelay(1); reset_control_deassert(); while (reset_control_status()) ; instead of reset_control_reset(), I think. Best regards, Yoshihiro Shimoda
Hello Shimoda-san, > > Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 > > If possible, I'd like to replace "bug" with "specification" or other words :) "behaviour" maybe is a better word? > > + /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */ > > + if (priv->devtype == I2C_RCAR_GEN3) { > > + priv->flags |= ID_P_NO_RXDMA; > > + if (!IS_ERR(priv->rstc)) { > > + ret = reset_control_reset(priv->rstc); > > According to the datasheet Rev.1.00 page 57-69, we should do: > reset_control_assert(); > udelay(1); > reset_control_deassert(); > while (reset_control_status()) > ; > instead of reset_control_reset(), I think. I was following Geert's suggestion here from the mail thread '[periperi] About BSP patch "i2c: rcar: Fix I2C DMA reception by adding reset': === >> reset_control_assert() + reset_control_deassert() can be replaced by >> reset_control_assert(). > > Do you mean 'reset_control_reset' maybe? I am not sure, I don't know > this API very well... but two time *_assert looks suspicious. Of course. Silly c&p. >> In addition, that will make sure the delay of one cycle of the RCLK clock >> is taken into account, which is not the case with the current code. > > I guess this is why there is now this patch in the BSP which Shimoda-san > mentioned in a later mail: > > f0cd22525f73 ("i2c: rcar: Fix module reset function for hardware specification") Exactly. === As far as I understood it takes care of the proper reset mechanism with the delay? Kind regards, Wolfram
Hi Shimoda-san, On Wed, May 30, 2018 at 10:35 AM, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: >> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM >> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 > > If possible, I'd like to replace "bug" with "specification" or other words :) > > <snip> >> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, >> >> pm_runtime_get_sync(dev); >> >> + /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */ >> + if (priv->devtype == I2C_RCAR_GEN3) { >> + priv->flags |= ID_P_NO_RXDMA; >> + if (!IS_ERR(priv->rstc)) { >> + ret = reset_control_reset(priv->rstc); > > According to the datasheet Rev.1.00 page 57-69, we should do: > reset_control_assert(); > udelay(1); > reset_control_deassert(); > while (reset_control_status()) > ; > instead of reset_control_reset(), I think. The i2c-specific procedure documented at page 57-69 thus differs from the generic one at page 8A-58, which is what cpg_mssr_reset() implements. The latter waits 35µs instead of 1µs, so that should be safe. But it doesn't check the status bit. Is the longer delay sufficient, or should a check polling the status bit be added to cpg_mssr_reset()? Gr{oetje,eeting}s, Geert
Hi Geert-san, Wolfram-san, > From: Geert Uytterhoeven, Sent: Thursday, May 31, 2018 5:45 PM > > Hi Shimoda-san, > > On Wed, May 30, 2018 at 10:35 AM, Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > >> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM > >> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 > > > > If possible, I'd like to replace "bug" with "specification" or other words :) > > > > <snip> > >> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, > >> > >> pm_runtime_get_sync(dev); > >> > >> + /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */ > >> + if (priv->devtype == I2C_RCAR_GEN3) { > >> + priv->flags |= ID_P_NO_RXDMA; > >> + if (!IS_ERR(priv->rstc)) { > >> + ret = reset_control_reset(priv->rstc); > > > > According to the datasheet Rev.1.00 page 57-69, we should do: > > reset_control_assert(); > > udelay(1); > > reset_control_deassert(); > > while (reset_control_status()) > > ; > > instead of reset_control_reset(), I think. > > The i2c-specific procedure documented at page 57-69 thus differs from > the generic one at page 8A-58, which is what cpg_mssr_reset() implements. > > The latter waits 35µs instead of 1µs, so that should be safe. > But it doesn't check the status bit. Is the longer delay sufficient, or should > a check polling the status bit be added to cpg_mssr_reset()? Thank you for the pointed out. I agree we should wait 35us for safe. But, anyway I'll ask HW team about this contradiction and really need polling the status. Best regards, Yoshihiro Shimoda > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hello Wolfram-san, > From: Wolfram Sang, Sent: Thursday, May 31, 2018 5:31 PM > > Hello Shimoda-san, > > > > Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 > > > > If possible, I'd like to replace "bug" with "specification" or other words :) > > "behaviour" maybe is a better word? It sounds good to me. > > > + /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */ > > > + if (priv->devtype == I2C_RCAR_GEN3) { > > > + priv->flags |= ID_P_NO_RXDMA; > > > + if (!IS_ERR(priv->rstc)) { > > > + ret = reset_control_reset(priv->rstc); > > > > According to the datasheet Rev.1.00 page 57-69, we should do: > > reset_control_assert(); > > udelay(1); > > reset_control_deassert(); > > while (reset_control_status()) > > ; > > instead of reset_control_reset(), I think. > > I was following Geert's suggestion here from the mail thread '[periperi] About > BSP patch "i2c: rcar: Fix I2C DMA reception by adding reset': > > === > > >> reset_control_assert() + reset_control_deassert() can be replaced by > >> reset_control_assert(). > > > > Do you mean 'reset_control_reset' maybe? I am not sure, I don't know > > this API very well... but two time *_assert looks suspicious. > > Of course. Silly c&p. > > >> In addition, that will make sure the delay of one cycle of the RCLK clock > >> is taken into account, which is not the case with the current code. > > > > I guess this is why there is now this patch in the BSP which Shimoda-san > > mentioned in a later mail: > > > > f0cd22525f73 ("i2c: rcar: Fix module reset function for hardware specification") > > Exactly. > > === > > As far as I understood it takes care of the proper reset mechanism with the delay? I missed this email... As I replied to Geert-san on other email thread, I'll ask HW team about this topic. Best regards, Yoshihiro Shimoda > Kind regards, > > Wolfram
Hi Geert-san, > From: Yoshihiro Shimoda, Sent: Thursday, May 31, 2018 6:12 PM > > Hi Geert-san, Wolfram-san, > > > From: Geert Uytterhoeven, Sent: Thursday, May 31, 2018 5:45 PM > > > > Hi Shimoda-san, > > > > On Wed, May 30, 2018 at 10:35 AM, Yoshihiro Shimoda > > <yoshihiro.shimoda.uh@renesas.com> wrote: > > >> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM > > >> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 > > > > > > If possible, I'd like to replace "bug" with "specification" or other words :) > > > > > > <snip> > > >> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, > > >> > > >> pm_runtime_get_sync(dev); > > >> > > >> + /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */ > > >> + if (priv->devtype == I2C_RCAR_GEN3) { > > >> + priv->flags |= ID_P_NO_RXDMA; > > >> + if (!IS_ERR(priv->rstc)) { > > >> + ret = reset_control_reset(priv->rstc); > > > > > > According to the datasheet Rev.1.00 page 57-69, we should do: > > > reset_control_assert(); > > > udelay(1); > > > reset_control_deassert(); > > > while (reset_control_status()) > > > ; > > > instead of reset_control_reset(), I think. > > > > The i2c-specific procedure documented at page 57-69 thus differs from > > the generic one at page 8A-58, which is what cpg_mssr_reset() implements. > > > > The latter waits 35µs instead of 1µs, so that should be safe. > > But it doesn't check the status bit. Is the longer delay sufficient, or should > > a check polling the status bit be added to cpg_mssr_reset()? > > Thank you for the pointed out. > I agree we should wait 35us for safe. > But, anyway I'll ask HW team about this contradiction and really need polling the status. I got information about this topic. < In CPG / MSSR point of view > - This needs 35 usec wait while asserting. - After deasserted a reset, no wait needs. - In other words, there is each hardware IP dependence. < In I2C point of view > - After deasserted the reset, this nees SRCR register polling. So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset. But, what do you think? Best regards, Yoshihiro Shimoda
Hi Shimoda-san, On Thu, Jun 7, 2018 at 7:36 AM, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: >> From: Yoshihiro Shimoda, Sent: Thursday, May 31, 2018 6:12 PM >> > From: Geert Uytterhoeven, Sent: Thursday, May 31, 2018 5:45 PM >> > On Wed, May 30, 2018 at 10:35 AM, Yoshihiro Shimoda >> > <yoshihiro.shimoda.uh@renesas.com> wrote: >> > >> From: Wolfram Sang, Sent: Tuesday, May 29, 2018 7:59 PM >> > >> Subject: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 >> > > >> > > If possible, I'd like to replace "bug" with "specification" or other words :) >> > > >> > > <snip> >> > >> @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, >> > >> >> > >> pm_runtime_get_sync(dev); >> > >> >> > >> + /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */ >> > >> + if (priv->devtype == I2C_RCAR_GEN3) { >> > >> + priv->flags |= ID_P_NO_RXDMA; >> > >> + if (!IS_ERR(priv->rstc)) { >> > >> + ret = reset_control_reset(priv->rstc); >> > > >> > > According to the datasheet Rev.1.00 page 57-69, we should do: >> > > reset_control_assert(); >> > > udelay(1); >> > > reset_control_deassert(); >> > > while (reset_control_status()) >> > > ; >> > > instead of reset_control_reset(), I think. >> > >> > The i2c-specific procedure documented at page 57-69 thus differs from >> > the generic one at page 8A-58, which is what cpg_mssr_reset() implements. >> > >> > The latter waits 35µs instead of 1µs, so that should be safe. >> > But it doesn't check the status bit. Is the longer delay sufficient, or should >> > a check polling the status bit be added to cpg_mssr_reset()? >> >> Thank you for the pointed out. >> I agree we should wait 35us for safe. >> But, anyway I'll ask HW team about this contradiction and really need polling the status. > > I got information about this topic. > > < In CPG / MSSR point of view > > - This needs 35 usec wait while asserting. > - After deasserted a reset, no wait needs. > - In other words, there is each hardware IP dependence. Let's call the above procedure A. > < In I2C point of view > > - After deasserted the reset, this nees SRCR register polling. Let's call the above procedure B. > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset. > But, what do you think? cpg_mssr_reset() indeed does not check the status bit after deasserting the reset, as it follows procedure A. Such a check could be added, though. Then it'll become (let's call this procedure C): /* Reset module */ spin_lock_irqsave(&priv->rmw_lock, flags); value = readl(priv->base + SRCR(reg)); value |= bitmask; writel(value, priv->base + SRCR(reg)); spin_unlock_irqrestore(&priv->rmw_lock, flags); /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ udelay(35); /* Release module from reset state */ writel(bitmask, priv->base + SRSTCLR(reg)); + /* Wait until deassertion has completed */ + while (readl(priv->base + SRCR(reg)) & bitmask) + cpu_relax(); Probably we need an upper limit on the number of loops, and call udelay(1) after each iteration? for (i 0; i < 35; i++) { if (!(readl(priv->base + SRCR(reg)) & bitmask)) return 0; udelay(1); } return -EIO; Any advice from the hardware team about that? But according to procedure A, the check is not needed? Probably because 35µs is an upper limit satisfying all individual hardware modules? I'm wondering whether we could use procedure B in the general case, as it explicitly checks for completion? Procedure C is safest, though, so probably the way to go. Thanks! Gr{oetje,eeting}s, Geert
> > I got information about this topic. > > > > < In CPG / MSSR point of view > > > - This needs 35 usec wait while asserting. > > - After deasserted a reset, no wait needs. > > - In other words, there is each hardware IP dependence. > > Let's call the above procedure A. > > > < In I2C point of view > > > - After deasserted the reset, this nees SRCR register polling. > > Let's call the above procedure B. > > > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset. > > But, what do you think? > > cpg_mssr_reset() indeed does not check the status bit after deasserting > the reset, as it follows procedure A. > > Such a check could be added, though. Then it'll become > (let's call this procedure C): > > /* Reset module */ > spin_lock_irqsave(&priv->rmw_lock, flags); > value = readl(priv->base + SRCR(reg)); > value |= bitmask; > writel(value, priv->base + SRCR(reg)); > spin_unlock_irqrestore(&priv->rmw_lock, flags); > > /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ > udelay(35); > > /* Release module from reset state */ > writel(bitmask, priv->base + SRSTCLR(reg)); > > + /* Wait until deassertion has completed */ > + while (readl(priv->base + SRCR(reg)) & bitmask) > + cpu_relax(); > > Probably we need an upper limit on the number of loops, and call udelay(1) > after each iteration? > > for (i 0; i < 35; i++) { > if (!(readl(priv->base + SRCR(reg)) & bitmask)) > return 0; > udelay(1); > } > return -EIO; > > Any advice from the hardware team about that? > > But according to procedure A, the check is not needed? > Probably because 35µs is an upper limit satisfying all individual hardware > modules? > > I'm wondering whether we could use procedure B in the general case, > as it explicitly checks for completion? > > Procedure C is safest, though, so probably the way to go. Any news about this topic? And how to upstream all this? My I2C patch is clearly a bugfix, but the necessary CPG update technically isn't? Not sure about this one...
Hi Wolfram-san, Geert-san, I'm sorry for delayed response. I completely overlooked this email... > From: Wolfram Sang <wsa@the-dreams.de>, Sent: Tuesday, June 26, 2018 12:05 PM > > > > I got information about this topic. > > > > > > < In CPG / MSSR point of view > > > > - This needs 35 usec wait while asserting. > > > - After deasserted a reset, no wait needs. > > > - In other words, there is each hardware IP dependence. > > > > Let's call the above procedure A. > > > > > < In I2C point of view > > > > - After deasserted the reset, this nees SRCR register polling. > > > > Let's call the above procedure B. > > > > > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset. > > > But, what do you think? > > > > cpg_mssr_reset() indeed does not check the status bit after deasserting > > the reset, as it follows procedure A. > > > > Such a check could be added, though. Then it'll become > > (let's call this procedure C): > > > > /* Reset module */ > > spin_lock_irqsave(&priv->rmw_lock, flags); > > value = readl(priv->base + SRCR(reg)); > > value |= bitmask; > > writel(value, priv->base + SRCR(reg)); > > spin_unlock_irqrestore(&priv->rmw_lock, flags); > > > > /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ > > udelay(35); > > > > /* Release module from reset state */ > > writel(bitmask, priv->base + SRSTCLR(reg)); > > > > + /* Wait until deassertion has completed */ > > + while (readl(priv->base + SRCR(reg)) & bitmask) > > + cpu_relax(); > > > > Probably we need an upper limit on the number of loops, and call udelay(1) > > after each iteration? > > > > for (i 0; i < 35; i++) { > > if (!(readl(priv->base + SRCR(reg)) & bitmask)) > > return 0; > > udelay(1); > > } > > return -EIO; > > > > Any advice from the hardware team about that? The hardware team said: - In CPG point of view: - such polling doesn't need (because the reset pulse is generated correctly). - About the interval after deassert the reset, this is depend on each hardware module. (In other words, if each hardware module has a special handling about after the deassert interval, we should follow the special handling.) - The I2C controller needs an interval of reading SRCR at least (this is a special handling). So, I think adding this code is not good fit in CPG point of view. > > But according to procedure A, the check is not needed? As hardware team said, the check (that means procedure C) is not needed. > > Probably because 35µs is an upper limit satisfying all individual hardware > > modules? > > > > I'm wondering whether we could use procedure B in the general case, > > as it explicitly checks for completion? > > > > Procedure C is safest, though, so probably the way to go. > > Any news about this topic? > > And how to upstream all this? My I2C patch is clearly a bugfix, but the > necessary CPG update technically isn't? Not sure about this one... I think we have to add reset_control_status() calling into the i2c-rcar.c to follow procedure B. But, Geert-san, what do you think? Best regards, Yoshihiro Shimoda
Hi Shimoda-san, On Tue, Jun 26, 2018 at 10:38 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Wolfram Sang <wsa@the-dreams.de>, Sent: Tuesday, June 26, 2018 12:05 PM > > > > I got information about this topic. > > > > > > > > < In CPG / MSSR point of view > > > > > - This needs 35 usec wait while asserting. > > > > - After deasserted a reset, no wait needs. > > > > - In other words, there is each hardware IP dependence. > > > > > > Let's call the above procedure A. > > > > > > > < In I2C point of view > > > > > - After deasserted the reset, this nees SRCR register polling. > > > > > > Let's call the above procedure B. > > > > > > > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset. > > > > But, what do you think? > > > > > > cpg_mssr_reset() indeed does not check the status bit after deasserting > > > the reset, as it follows procedure A. > > > > > > Such a check could be added, though. Then it'll become > > > (let's call this procedure C): > > > > > > /* Reset module */ > > > spin_lock_irqsave(&priv->rmw_lock, flags); > > > value = readl(priv->base + SRCR(reg)); > > > value |= bitmask; > > > writel(value, priv->base + SRCR(reg)); > > > spin_unlock_irqrestore(&priv->rmw_lock, flags); > > > > > > /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ > > > udelay(35); > > > > > > /* Release module from reset state */ > > > writel(bitmask, priv->base + SRSTCLR(reg)); > > > > > > + /* Wait until deassertion has completed */ > > > + while (readl(priv->base + SRCR(reg)) & bitmask) > > > + cpu_relax(); > > > > > > Probably we need an upper limit on the number of loops, and call udelay(1) > > > after each iteration? > > > > > > for (i 0; i < 35; i++) { > > > if (!(readl(priv->base + SRCR(reg)) & bitmask)) > > > return 0; > > > udelay(1); > > > } > > > return -EIO; > > > > > > Any advice from the hardware team about that? > > The hardware team said: > - In CPG point of view: > - such polling doesn't need (because the reset pulse is generated correctly). > - About the interval after deassert the reset, this is depend on each hardware module. > (In other words, if each hardware module has a special handling about after the deassert interval, > we should follow the special handling.) > - The I2C controller needs an interval of reading SRCR at least (this is a special handling). > > So, I think adding this code is not good fit in CPG point of view. > > > > But according to procedure A, the check is not needed? > > As hardware team said, the check (that means procedure C) is not needed. > > > > Probably because 35µs is an upper limit satisfying all individual hardware > > > modules? > > > > > > I'm wondering whether we could use procedure B in the general case, > > > as it explicitly checks for completion? > > > > > > Procedure C is safest, though, so probably the way to go. > > > > Any news about this topic? > > > > And how to upstream all this? My I2C patch is clearly a bugfix, but the > > necessary CPG update technically isn't? Not sure about this one... > > I think we have to add reset_control_status() calling into the i2c-rcar.c > to follow procedure B. > But, Geert-san, what do you think? Calling reset_control_status() from i2c-rcar is fine for me. Note that reset controller support is optional, so we want to add select RESET_CONTROLLER if ARCH_RENESAS && ARM64 to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail silently. This hardware bug is restricted to R-Car Gen3? If it applies to R-Car Gen2, too, the "&& ARM64" has to be dropped. If it applies to R-Car Gen1, too, we have to write an R-Car Gen1 reset controller driver (R-Car Gen1 uses the legacy CPG and MSTP clock drivers, and cannot be migrated to the CPG/MSSR driver, as its MSTP and RESET functionalities are in separate modules). Gr{oetje,eeting}s, Geert
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Tuesday, June 26, 2018 5:58 PM > > Hi Shimoda-san, > > On Tue, Jun 26, 2018 at 10:38 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > From: Wolfram Sang <wsa@the-dreams.de>, Sent: Tuesday, June 26, 2018 12:05 PM > > > > > I got information about this topic. > > > > > > > > > > < In CPG / MSSR point of view > > > > > > - This needs 35 usec wait while asserting. > > > > > - After deasserted a reset, no wait needs. > > > > > - In other words, there is each hardware IP dependence. > > > > > > > > Let's call the above procedure A. > > > > > > > > > < In I2C point of view > > > > > > - After deasserted the reset, this nees SRCR register polling. > > > > > > > > Let's call the above procedure B. > > > > > > > > > So, I don't think cpg_mssr_reset() checks the status bit after deasserted a reset. > > > > > But, what do you think? > > > > > > > > cpg_mssr_reset() indeed does not check the status bit after deasserting > > > > the reset, as it follows procedure A. > > > > > > > > Such a check could be added, though. Then it'll become > > > > (let's call this procedure C): > > > > > > > > /* Reset module */ > > > > spin_lock_irqsave(&priv->rmw_lock, flags); > > > > value = readl(priv->base + SRCR(reg)); > > > > value |= bitmask; > > > > writel(value, priv->base + SRCR(reg)); > > > > spin_unlock_irqrestore(&priv->rmw_lock, flags); > > > > > > > > /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ > > > > udelay(35); > > > > > > > > /* Release module from reset state */ > > > > writel(bitmask, priv->base + SRSTCLR(reg)); > > > > > > > > + /* Wait until deassertion has completed */ > > > > + while (readl(priv->base + SRCR(reg)) & bitmask) > > > > + cpu_relax(); > > > > > > > > Probably we need an upper limit on the number of loops, and call udelay(1) > > > > after each iteration? > > > > > > > > for (i 0; i < 35; i++) { > > > > if (!(readl(priv->base + SRCR(reg)) & bitmask)) > > > > return 0; > > > > udelay(1); > > > > } > > > > return -EIO; > > > > > > > > Any advice from the hardware team about that? > > > > The hardware team said: > > - In CPG point of view: > > - such polling doesn't need (because the reset pulse is generated correctly). > > - About the interval after deassert the reset, this is depend on each hardware module. > > (In other words, if each hardware module has a special handling about after the deassert interval, > > we should follow the special handling.) > > - The I2C controller needs an interval of reading SRCR at least (this is a special handling). > > > > So, I think adding this code is not good fit in CPG point of view. > > > > > > But according to procedure A, the check is not needed? > > > > As hardware team said, the check (that means procedure C) is not needed. > > > > > > Probably because 35µs is an upper limit satisfying all individual hardware > > > > modules? > > > > > > > > I'm wondering whether we could use procedure B in the general case, > > > > as it explicitly checks for completion? > > > > > > > > Procedure C is safest, though, so probably the way to go. > > > > > > Any news about this topic? > > > > > > And how to upstream all this? My I2C patch is clearly a bugfix, but the > > > necessary CPG update technically isn't? Not sure about this one... > > > > I think we have to add reset_control_status() calling into the i2c-rcar.c > > to follow procedure B. > > But, Geert-san, what do you think? > > Calling reset_control_status() from i2c-rcar is fine for me. Thank you for your comment! > Note that reset controller support is optional, so we want to add > > select RESET_CONTROLLER if ARCH_RENESAS && ARM64 > > to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail > silently. Thanks for this information. It's helpful to me because I'll add reset controller support on renesas_usbhs driver later. > This hardware bug is restricted to R-Car Gen3? Yes, this is hardware "behaviour" on R-Car Gen3 :) And older SoCs' I2C doesn't support DMA transfer. > If it applies to R-Car Gen2, too, the "&& ARM64" has to be dropped. > If it applies to R-Car Gen1, too, we have to write an R-Car Gen1 reset > controller driver (R-Car Gen1 uses the legacy CPG and MSTP clock drivers, > and cannot be migrated to the CPG/MSSR driver, as its MSTP and RESET > functionalities are in separate modules). So, they are not needed, I think. Best regards, Yoshihiro Shimoda > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi, thanks for the discussion on this topic! > > The hardware team said: > > - In CPG point of view: > > - such polling doesn't need (because the reset pulse is generated correctly). > > - About the interval after deassert the reset, this is depend on each hardware module. > > (In other words, if each hardware module has a special handling about after the deassert interval, > > we should follow the special handling.) > > - The I2C controller needs an interval of reading SRCR at least (this is a special handling). > > > > So, I think adding this code is not good fit in CPG point of view. > > Calling reset_control_status() from i2c-rcar is fine for me. > Doesn't make this writing device drivers a bit harder, I wonder? If we follow the above, we need to know per driver (like i2c-rcar.c) if reset_control_reset() is enough or if we need to call reset_control_status() additionally. For a driver author, it would be much less error prone IMHO, if reset_control_reset() just does the right thing. It has also the advantage that we can handle special handling on different SoCs differently (if that is ever needed) because MSSR driver is per SoC. Where is this special handling documented BTW, I can't find it. The BSP waits for maximum 1024ms which seems excessive to me. > Note that reset controller support is optional, so we want to add > > select RESET_CONTROLLER if ARCH_RENESAS && ARM64 > > to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail > silently. Technically, it should also be "&& HAS_DMA". But this is maybe a tad too defensive? > This hardware bug is restricted to R-Car Gen3? As already mentioned by Shimoda-san, only Gen3 has DMA support. Kind regards, Wolfram
Hi Wolfram, On Wed, Jun 27, 2018 at 10:44 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > > The hardware team said: > > > - In CPG point of view: > > > - such polling doesn't need (because the reset pulse is generated correctly). > > > - About the interval after deassert the reset, this is depend on each hardware module. > > > (In other words, if each hardware module has a special handling about after the deassert interval, > > > we should follow the special handling.) > > > - The I2C controller needs an interval of reading SRCR at least (this is a special handling). > > > > > > So, I think adding this code is not good fit in CPG point of view. > > > > Calling reset_control_status() from i2c-rcar is fine for me. > > > > Doesn't make this writing device drivers a bit harder, I wonder? If we > follow the above, we need to know per driver (like i2c-rcar.c) if > reset_control_reset() is enough or if we need to call > reset_control_status() additionally. For a driver author, it would be > much less error prone IMHO, if reset_control_reset() just does the right > thing. It has also the advantage that we can handle special handling on > different SoCs differently (if that is ever needed) because MSSR driver > is per SoC. True. But this seems to be a bug in the R-Car Gen3 I2C controller implementation. > > Note that reset controller support is optional, so we want to add > > > > select RESET_CONTROLLER if ARCH_RENESAS && ARM64 > > > > to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail > > silently. > > Technically, it should also be "&& HAS_DMA". But this is maybe a tad too > defensive? s/HAS_DMA/RCAR_DMAC/ :-) Gr{oetje,eeting}s, Geert
> > Doesn't make this writing device drivers a bit harder, I wonder? If we > > follow the above, we need to know per driver (like i2c-rcar.c) if > > reset_control_reset() is enough or if we need to call > > reset_control_status() additionally. For a driver author, it would be > > much less error prone IMHO, if reset_control_reset() just does the right > > thing. It has also the advantage that we can handle special handling on > > different SoCs differently (if that is ever needed) because MSSR driver > > is per SoC. > > True. > > But this seems to be a bug in the R-Car Gen3 I2C controller implementation. I see. If this is more of "a bug in the IP core", then I can live with having the workaround in the I2C driver. With a comment explaining the situation, of course. If it was more like "intended" special handling needed for different IP cores (not only I2C), then we should maybe reconsider. > > > Note that reset controller support is optional, so we want to add > > > > > > select RESET_CONTROLLER if ARCH_RENESAS && ARM64 > > > > > > to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail > > > silently. > > > > Technically, it should also be "&& HAS_DMA". But this is maybe a tad too > > defensive? > > s/HAS_DMA/RCAR_DMAC/ :-) Yes, this is better. Thanks!
Hi Wolfram-san, > From: Wolfram Sang, Sent: Wednesday, June 27, 2018 5:44 PM > > Hi, > > thanks for the discussion on this topic! > > > > The hardware team said: > > > - In CPG point of view: > > > - such polling doesn't need (because the reset pulse is generated correctly). > > > - About the interval after deassert the reset, this is depend on each hardware module. > > > (In other words, if each hardware module has a special handling about after the deassert interval, > > > we should follow the special handling.) > > > - The I2C controller needs an interval of reading SRCR at least (this is a special handling). > > > > > > So, I think adding this code is not good fit in CPG point of view. > > > > Calling reset_control_status() from i2c-rcar is fine for me. > > < snip > > > Where is this special handling documented BTW, I can't find it. Please refer to "57.5.4 Usage note for the transmission and reception procedure" in the datasheet Rev. 1.00. I agree waiting for maximum 1024ms > The BSP waits for maximum 1024ms which seems excessive to me. The BSP waits for maximum 1024us, not 1024ms. Best regards, Yoshihiro Shimoda
Shimoda-san, > Please refer to "57.5.4 Usage note for the transmission and reception > procedure" in the datasheet Rev. 1.00. Thank you. I'll send a new version with the workaround in the i2c driver. > The BSP waits for maximum 1024us, not 1024ms. You are right. Sorry, my mistake. Regards, Wolfram
> Note that reset controller support is optional, so we want to add > > select RESET_CONTROLLER if ARCH_RENESAS && ARM64 > > to the I2C_RCAR section drivers/i2c/busses/Kconfig. Else reset will fail > silently. Actually, no. I use a non-optional reset_controller_get[1], so I will get an ERR_PTR when !RESET_CONTROLLER. And I check for this ERR_PTR. So, it will work with !RESET_CONTROLLER, just without RXDMA always then. That worked fine with the old patch, need to test my new version now. [1] In fact, this function is deprecated and I replaced it with the now more explicit reset_control_get_exclusive(). Plus devm_*, of course.
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index e42cd0121862..27277f42710d 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -24,6 +24,7 @@ #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/reset.h> #include <linux/slab.h> /* register offsets */ @@ -103,8 +104,9 @@ #define ID_ARBLOST (1 << 3) #define ID_NACK (1 << 4) /* persistent flags */ +#define ID_P_NO_RXDMA (1 << 30) /* HW bug forbids RXDMA sometimes */ #define ID_P_PM_BLOCKED (1 << 31) -#define ID_P_MASK ID_P_PM_BLOCKED +#define ID_P_MASK (ID_P_PM_BLOCKED | ID_P_NO_RXDMA) enum rcar_i2c_type { I2C_RCAR_GEN1, @@ -133,6 +135,8 @@ struct rcar_i2c_priv { struct dma_chan *dma_rx; struct scatterlist sg; enum dma_data_direction dma_direction; + + struct reset_control *rstc; }; #define rcar_i2c_priv_to_dev(p) ((p)->adap.dev.parent) @@ -363,6 +367,11 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv) dma_unmap_single(chan->device->dev, sg_dma_address(&priv->sg), sg_dma_len(&priv->sg), priv->dma_direction); + /* Gen3 can only do one RXDMA per transfer */ + if (priv->devtype == I2C_RCAR_GEN3 && + priv->dma_direction == DMA_FROM_DEVICE) + priv->flags |= ID_P_NO_RXDMA; + priv->dma_direction = DMA_NONE; } @@ -400,8 +409,9 @@ static void rcar_i2c_dma(struct rcar_i2c_priv *priv) unsigned char *buf; int len; - /* Do not use DMA if it's not available or for messages < 8 bytes */ - if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE)) + /* Do various checks to see if DMA is feasible at all */ + if (IS_ERR(chan) || msg->len < 8 || !(msg->flags & I2C_M_DMA_SAFE) || + (read && priv->flags & ID_P_NO_RXDMA)) return; if (read) { @@ -743,6 +753,16 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, pm_runtime_get_sync(dev); + /* Gen3 has a HW bug which needs a reset before allowing RX DMA once */ + if (priv->devtype == I2C_RCAR_GEN3) { + priv->flags |= ID_P_NO_RXDMA; + if (!IS_ERR(priv->rstc)) { + ret = reset_control_reset(priv->rstc); + if (ret == 0) + priv->flags &= ~ID_P_NO_RXDMA; + } + } + rcar_i2c_init(priv); ret = rcar_i2c_bus_barrier(priv); @@ -913,6 +933,9 @@ static int rcar_i2c_probe(struct platform_device *pdev) if (ret < 0) goto out_pm_put; + if (priv->devtype == I2C_RCAR_GEN3) + priv->rstc = devm_reset_control_get(&pdev->dev, NULL); + /* Stay always active when multi-master to keep arbitration working */ if (of_property_read_bool(dev->of_node, "multi-master")) priv->flags |= ID_P_PM_BLOCKED;
On Gen3, we can only do RXDMA once per transfer reliably. For that, we must reset the device, then we can have RXDMA once. This patch implements this. When there is no reset controller or the reset fails, RXDMA will be blocked completely. Otherwise, it will be disabled after the first RXDMA transfer. Based on a commit from the BSP by Hiromitsu Yamasaki, yet completely refactored to handle multiple read messages within one transfer. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/busses/i2c-rcar.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-)