* [RFC PATCH 0/1] i2c: rcar: handle RXDMA HW bug on Gen3 @ 2018-05-29 10:58 Wolfram Sang 2018-05-29 10:58 ` [RFC PATCH 1/1] " Wolfram Sang 0 siblings, 1 reply; 19+ messages in thread From: Wolfram Sang @ 2018-05-29 10:58 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, Yoshihiro Shimoda, Hiromitsu Yamasaki, Wolfram Sang As mentioned in the patch description, this patch works around a HW bug on Renesas R-Car Gen3 SoCs. It had not been discovered yet because it only happens when clearing DMA enable bits is too late because of latency. To reproduce the problem reliably, the below patch was used [1]. To the best of my knowledge, all Gen3 SoCs are affected, so I didn't add any soc_device_match kind of matching for now. If the situation changes, we can add it. For now, priv->devtype is good. The improvement can be shown by using the following command. Before the patch, executed on a Salvator-X / R-Car M3-W ES1.0: # i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00 0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 # i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16 0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0xfe 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 Note the broken first byte (0xfe) after the first successful RXDMA transfer. After the patch (tested with and without CONFIG_RESET_CONTROLLER): # i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00 # i2ctransfer -y 2 w1@0x10 0 r16 w1 0 r16 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00 0x0f 0x07 0x3f 0x20 0x20 0x55 0x05 0x07 0x0f 0x07 0x3f 0x00 0x00 0x00 0x00 0x00 Looks way better! Patch was tested with current renesas-drivers/master, but should also apply cleanly on i2c/for-next or v4.17-rc7. Please test and comment. Kind regards, Wolfram Wolfram Sang (1): i2c: rcar: handle RXDMA HW bug on Gen3 drivers/i2c/busses/i2c-rcar.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) [1] Testcase patch: diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index d445b5891280..e42cd0121862 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -353,6 +353,7 @@ static void rcar_i2c_dma_unmap(struct rcar_i2c_priv *priv) struct dma_chan *chan = priv->dma_direction == DMA_FROM_DEVICE ? priv->dma_rx : priv->dma_tx; + mdelay(10); /* Disable DMA Master Received/Transmitted */ rcar_i2c_write(priv, ICDMAER, 0); -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-05-29 10:58 [RFC PATCH 0/1] i2c: rcar: handle RXDMA HW bug on Gen3 Wolfram Sang @ 2018-05-29 10:58 ` Wolfram Sang 2018-05-30 8:35 ` Yoshihiro Shimoda 0 siblings, 1 reply; 19+ messages in thread From: Wolfram Sang @ 2018-05-29 10:58 UTC (permalink / raw) To: linux-i2c Cc: linux-renesas-soc, Yoshihiro Shimoda, Hiromitsu Yamasaki, Wolfram Sang 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(-) 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; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-05-29 10:58 ` [RFC PATCH 1/1] " Wolfram Sang @ 2018-05-30 8:35 ` Yoshihiro Shimoda 2018-05-31 8:31 ` Wolfram Sang 2018-05-31 8:45 ` Geert Uytterhoeven 0 siblings, 2 replies; 19+ messages in thread From: Yoshihiro Shimoda @ 2018-05-30 8:35 UTC (permalink / raw) To: Wolfram Sang, linux-i2c; +Cc: linux-renesas-soc, Hiromitsu Yamasaki 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-05-30 8:35 ` Yoshihiro Shimoda @ 2018-05-31 8:31 ` Wolfram Sang 2018-05-31 9:22 ` Yoshihiro Shimoda 2018-05-31 8:45 ` Geert Uytterhoeven 1 sibling, 1 reply; 19+ messages in thread From: Wolfram Sang @ 2018-05-31 8:31 UTC (permalink / raw) To: Yoshihiro Shimoda, Geert Uytterhoeven Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Hiromitsu Yamasaki [-- Attachment #1: Type: text/plain, Size: 1599 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-05-31 8:31 ` Wolfram Sang @ 2018-05-31 9:22 ` Yoshihiro Shimoda 0 siblings, 0 replies; 19+ messages in thread From: Yoshihiro Shimoda @ 2018-05-31 9:22 UTC (permalink / raw) To: Wolfram Sang, Geert Uytterhoeven Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Hiromitsu Yamasaki 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-05-30 8:35 ` Yoshihiro Shimoda 2018-05-31 8:31 ` Wolfram Sang @ 2018-05-31 8:45 ` Geert Uytterhoeven 2018-05-31 9:12 ` Yoshihiro Shimoda 1 sibling, 1 reply; 19+ messages in thread From: Geert Uytterhoeven @ 2018-05-31 8:45 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Hiromitsu Yamasaki 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 -- 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-05-31 8:45 ` Geert Uytterhoeven @ 2018-05-31 9:12 ` Yoshihiro Shimoda 2018-06-07 5:36 ` Yoshihiro Shimoda 0 siblings, 1 reply; 19+ messages in thread From: Yoshihiro Shimoda @ 2018-05-31 9:12 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Hiromitsu Yamasaki 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-05-31 9:12 ` Yoshihiro Shimoda @ 2018-06-07 5:36 ` Yoshihiro Shimoda 2018-06-07 10:08 ` Geert Uytterhoeven 0 siblings, 1 reply; 19+ messages in thread From: Yoshihiro Shimoda @ 2018-06-07 5:36 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Hiromitsu Yamasaki 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-07 5:36 ` Yoshihiro Shimoda @ 2018-06-07 10:08 ` Geert Uytterhoeven 2018-06-26 3:05 ` Wolfram Sang 0 siblings, 1 reply; 19+ messages in thread From: Geert Uytterhoeven @ 2018-06-07 10:08 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Hiromitsu Yamasaki 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 -- 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-07 10:08 ` Geert Uytterhoeven @ 2018-06-26 3:05 ` Wolfram Sang 2018-06-26 8:38 ` Yoshihiro Shimoda 0 siblings, 1 reply; 19+ messages in thread From: Wolfram Sang @ 2018-06-26 3:05 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Yoshihiro Shimoda, Wolfram Sang, linux-i2c, linux-renesas-soc, Hiromitsu Yamasaki [-- Attachment #1: Type: text/plain, Size: 2288 bytes --] > > 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... [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-26 3:05 ` Wolfram Sang @ 2018-06-26 8:38 ` Yoshihiro Shimoda 2018-06-26 8:58 ` Geert Uytterhoeven 0 siblings, 1 reply; 19+ messages in thread From: Yoshihiro Shimoda @ 2018-06-26 8:38 UTC (permalink / raw) To: Wolfram Sang, Geert Uytterhoeven Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, Hiromitsu Yamasaki 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-26 8:38 ` Yoshihiro Shimoda @ 2018-06-26 8:58 ` Geert Uytterhoeven 2018-06-26 9:26 ` Yoshihiro Shimoda ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Geert Uytterhoeven @ 2018-06-26 8:58 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Wolfram Sang, Wolfram Sang, Linux I2C, Linux-Renesas, Hiromitsu Yamasaki 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 -- 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-26 8:58 ` Geert Uytterhoeven @ 2018-06-26 9:26 ` Yoshihiro Shimoda 2018-06-27 8:44 ` Wolfram Sang 2018-06-28 7:33 ` Wolfram Sang 2 siblings, 0 replies; 19+ messages in thread From: Yoshihiro Shimoda @ 2018-06-26 9:26 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Wolfram Sang, Wolfram Sang, Linux I2C, Linux-Renesas, Hiromitsu Yamasaki 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-26 8:58 ` Geert Uytterhoeven 2018-06-26 9:26 ` Yoshihiro Shimoda @ 2018-06-27 8:44 ` Wolfram Sang 2018-06-27 8:51 ` Geert Uytterhoeven 2018-06-27 9:48 ` Yoshihiro Shimoda 2018-06-28 7:33 ` Wolfram Sang 2 siblings, 2 replies; 19+ messages in thread From: Wolfram Sang @ 2018-06-27 8:44 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Yoshihiro Shimoda, Wolfram Sang, Linux I2C, Linux-Renesas, Hiromitsu Yamasaki [-- Attachment #1: Type: text/plain, Size: 1798 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-27 8:44 ` Wolfram Sang @ 2018-06-27 8:51 ` Geert Uytterhoeven 2018-06-27 9:10 ` Wolfram Sang 2018-06-27 9:48 ` Yoshihiro Shimoda 1 sibling, 1 reply; 19+ messages in thread From: Geert Uytterhoeven @ 2018-06-27 8:51 UTC (permalink / raw) To: Wolfram Sang Cc: Yoshihiro Shimoda, Wolfram Sang, Linux I2C, Linux-Renesas, Hiromitsu Yamasaki 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 -- 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-27 8:51 ` Geert Uytterhoeven @ 2018-06-27 9:10 ` Wolfram Sang 0 siblings, 0 replies; 19+ messages in thread From: Wolfram Sang @ 2018-06-27 9:10 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Yoshihiro Shimoda, Wolfram Sang, Linux I2C, Linux-Renesas, Hiromitsu Yamasaki [-- Attachment #1: Type: text/plain, Size: 1346 bytes --] > > 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! [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-27 8:44 ` Wolfram Sang 2018-06-27 8:51 ` Geert Uytterhoeven @ 2018-06-27 9:48 ` Yoshihiro Shimoda 2018-06-28 5:13 ` Wolfram Sang 1 sibling, 1 reply; 19+ messages in thread From: Yoshihiro Shimoda @ 2018-06-27 9:48 UTC (permalink / raw) To: Wolfram Sang, Geert Uytterhoeven Cc: Wolfram Sang, Linux I2C, Linux-Renesas, Hiromitsu Yamasaki 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-27 9:48 ` Yoshihiro Shimoda @ 2018-06-28 5:13 ` Wolfram Sang 0 siblings, 0 replies; 19+ messages in thread From: Wolfram Sang @ 2018-06-28 5:13 UTC (permalink / raw) To: Yoshihiro Shimoda Cc: Geert Uytterhoeven, Wolfram Sang, Linux I2C, Linux-Renesas, Hiromitsu Yamasaki [-- Attachment #1: Type: text/plain, Size: 309 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [RFC PATCH 1/1] i2c: rcar: handle RXDMA HW bug on Gen3 2018-06-26 8:58 ` Geert Uytterhoeven 2018-06-26 9:26 ` Yoshihiro Shimoda 2018-06-27 8:44 ` Wolfram Sang @ 2018-06-28 7:33 ` Wolfram Sang 2 siblings, 0 replies; 19+ messages in thread From: Wolfram Sang @ 2018-06-28 7:33 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Yoshihiro Shimoda, Wolfram Sang, Linux I2C, Linux-Renesas, Hiromitsu Yamasaki [-- Attachment #1: Type: text/plain, Size: 663 bytes --] > 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. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-06-28 7:33 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-29 10:58 [RFC PATCH 0/1] i2c: rcar: handle RXDMA HW bug on Gen3 Wolfram Sang 2018-05-29 10:58 ` [RFC PATCH 1/1] " Wolfram Sang 2018-05-30 8:35 ` Yoshihiro Shimoda 2018-05-31 8:31 ` Wolfram Sang 2018-05-31 9:22 ` Yoshihiro Shimoda 2018-05-31 8:45 ` Geert Uytterhoeven 2018-05-31 9:12 ` Yoshihiro Shimoda 2018-06-07 5:36 ` Yoshihiro Shimoda 2018-06-07 10:08 ` Geert Uytterhoeven 2018-06-26 3:05 ` Wolfram Sang 2018-06-26 8:38 ` Yoshihiro Shimoda 2018-06-26 8:58 ` Geert Uytterhoeven 2018-06-26 9:26 ` Yoshihiro Shimoda 2018-06-27 8:44 ` Wolfram Sang 2018-06-27 8:51 ` Geert Uytterhoeven 2018-06-27 9:10 ` Wolfram Sang 2018-06-27 9:48 ` Yoshihiro Shimoda 2018-06-28 5:13 ` Wolfram Sang 2018-06-28 7:33 ` Wolfram Sang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.