| Message ID | 1441311613-2681-6-git-send-email-wsa@the-dreams.de |
|---|---|
| State | Accepted |
| Headers | show |
Hi Wolfram, On Thursday 03 September 2015 22:20:09 Wolfram Sang wrote: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Setting up new messages was done in process context while handling a > message was in interrupt context. Because of the HW design, this IP core > is sensitive to timing, so the context switches were too expensive. Move > this setup to interrupt context as well. > > In my test setup, this fixed the occasional 'data byte sent twice' issue > which a number of people have seen. It also fixes to send REP_START > after a read message which was wrongly send as a STOP + START sequence > before. I'm afraid this patch has been found by git bisect to break HDMI on Koelsch :-( The regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val) call in drivers/gpu/drm/i2c/adv7511.c returns -ENXIO. Reverting the patch on top of Geert's current drivers master branch fixes the problem. > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/busses/i2c-rcar.c | 74 +++++++++++++++++++--------------------- > 1 file changed, 37 insertions(+), 37 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c > index 6e459a338ccc75..36c79301044b71 100644 > --- a/drivers/i2c/busses/i2c-rcar.c > +++ b/drivers/i2c/busses/i2c-rcar.c > @@ -266,6 +266,13 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv > *priv) rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND); > } > > +static void rcar_i2c_next_msg(struct rcar_i2c_priv *priv) > +{ > + priv->msg++; > + priv->msgs_left--; > + rcar_i2c_prepare_msg(priv); > +} > + > /* > * interrupt functions > */ > @@ -308,21 +315,17 @@ static int rcar_i2c_irq_send(struct rcar_i2c_priv > *priv, u32 msr) * [ICRXTX] -> [SHIFT] -> [I2C bus] > */ > > - if (priv->flags & ID_LAST_MSG) > + if (priv->flags & ID_LAST_MSG) { > /* > * If current msg is the _LAST_ msg, > * prepare stop condition here. > * ID_DONE will be set on STOP irq. > */ > rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP); > - else > - /* > - * If current msg is _NOT_ last msg, > - * it doesn't call stop phase. > - * thus, there is no STOP irq. > - * return ID_DONE here. > - */ > - return ID_DONE; > + } else { > + rcar_i2c_next_msg(priv); > + return 0; > + } > } > > rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_SEND); > @@ -366,7 +369,10 @@ static int rcar_i2c_irq_recv(struct rcar_i2c_priv > *priv, u32 msr) else > rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA); > > - rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_RECV); > + if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG)) > + rcar_i2c_next_msg(priv); > + else > + rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_RECV); > > return 0; > } > @@ -461,6 +467,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr) > > /* Stop */ > if (msr & MST) { > + priv->msgs_left--; /* The last message also made it */ > rcar_i2c_flags_set(priv, ID_DONE); > goto out; > } > @@ -500,35 +507,28 @@ static int rcar_i2c_master_xfer(struct i2c_adapter > *adap, /* This HW can't send STOP after address phase */ > if (msgs[i].len == 0) { > ret = -EOPNOTSUPP; > - break; > - } > - > - /* init each data */ > - priv->msg = &msgs[i]; > - priv->msgs_left = num - i; > - > - rcar_i2c_prepare_msg(priv); > - > - time_left = wait_event_timeout(priv->wait, > - rcar_i2c_flags_has(priv, ID_DONE), > - adap->timeout); > - if (!time_left) { > - rcar_i2c_init(priv); > - ret = -ETIMEDOUT; > - break; > - } > - > - if (rcar_i2c_flags_has(priv, ID_NACK)) { > - ret = -ENXIO; > - break; > - } > - > - if (rcar_i2c_flags_has(priv, ID_ARBLOST)) { > - ret = -EAGAIN; > - break; > + goto out; > } > + } > > - ret = i + 1; /* The number of transfer */ > + /* init data */ > + priv->msg = msgs; > + priv->msgs_left = num; > + > + rcar_i2c_prepare_msg(priv); > + > + time_left = wait_event_timeout(priv->wait, > + rcar_i2c_flags_has(priv, ID_DONE), > + num * adap->timeout); > + if (!time_left) { > + rcar_i2c_init(priv); > + ret = -ETIMEDOUT; > + } else if (rcar_i2c_flags_has(priv, ID_NACK)) { > + ret = -ENXIO; > + } else if (rcar_i2c_flags_has(priv, ID_ARBLOST)) { > + ret = -EAGAIN; > + } else { > + ret = num - priv->msgs_left; /* The number of transfer */ > } > out: > pm_runtime_put(dev);
On Thu, Oct 22, 2015 at 02:10:52AM +0300, Laurent Pinchart wrote: > Hi Wolfram, > > On Thursday 03 September 2015 22:20:09 Wolfram Sang wrote: > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > > Setting up new messages was done in process context while handling a > > message was in interrupt context. Because of the HW design, this IP core > > is sensitive to timing, so the context switches were too expensive. Move > > this setup to interrupt context as well. > > > > In my test setup, this fixed the occasional 'data byte sent twice' issue > > which a number of people have seen. It also fixes to send REP_START > > after a read message which was wrongly send as a STOP + START sequence > > before. > > I'm afraid this patch has been found by git bisect to break HDMI on Koelsch > :-( > > The regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val) call in > drivers/gpu/drm/i2c/adv7511.c returns -ENXIO. > > Reverting the patch on top of Geert's current drivers master branch fixes the > problem. But HDMI worked on Koelsch in Dublin??
Hi Wolfram, On Thursday 22 October 2015 13:05:05 Wolfram Sang wrote: > On Thu, Oct 22, 2015 at 02:10:52AM +0300, Laurent Pinchart wrote: > > On Thursday 03 September 2015 22:20:09 Wolfram Sang wrote: > > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > > > > > Setting up new messages was done in process context while handling a > > > message was in interrupt context. Because of the HW design, this IP core > > > is sensitive to timing, so the context switches were too expensive. Move > > > this setup to interrupt context as well. > > > > > > In my test setup, this fixed the occasional 'data byte sent twice' issue > > > which a number of people have seen. It also fixes to send REP_START > > > after a read message which was wrongly send as a STOP + START sequence > > > before. > > > > I'm afraid this patch has been found by git bisect to break HDMI on > > Koelsch > > > > :-( > > > > The regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val) call in > > drivers/gpu/drm/i2c/adv7511.c returns -ENXIO. > > > > Reverting the patch on top of Geert's current drivers master branch fixes > > the problem. > > But HDMI worked on Koelsch in Dublin?? I know :-) Do you have a Koelsch board now ? Could you try b9653e9c000dc2ebd9c8712442c659ccd1586e22 from Geert's drivers tree ? On my board the adv7511 fails to probe completely due to the regmap_read() failure.
> Do you have a Koelsch board now ? Could you try Nope, I only have Lager. I'd be surprised if it really was a Koelsch only issue, though... > b9653e9c000dc2ebd9c8712442c659ccd1586e22 from Geert's drivers tree ? On my Does that build for you? drivers/gpu/drm/drm_fb_helper.c: In function 'restore_fbdev_mode': drivers/gpu/drm/drm_fb_helper.c:448:5: error: 'error' undeclared (first use in this function) ?? I fixed it locally and will see if I see ADV7511 problems. I cannot fully test HDMI probably, because it seems that this HDMI->DVI chain I have does not work. Tested that with a kernel which used to work in Dublin. Need to get another HDMI device. > board the adv7511 fails to probe completely due to the regmap_read() failure. Okay, so before any 'modetest' activity. Which kernelconfig? shmobile_defconfig? Thanks, Wolfram
Hi Wolfram, On Friday 23 October 2015 10:06:02 Wolfram Sang wrote: > > Do you have a Koelsch board now ? Could you try > > Nope, I only have Lager. I'd be surprised if it really was a Koelsch > only issue, though... > > > b9653e9c000dc2ebd9c8712442c659ccd1586e22 from Geert's drivers tree ? On my > > Does that build for you? > > drivers/gpu/drm/drm_fb_helper.c: In function 'restore_fbdev_mode': > drivers/gpu/drm/drm_fb_helper.c:448:5: error: 'error' undeclared (first use > in this function) > > ?? I had CONFIG_DRM_FBDEV_EMULATION disabled. I've then enabled it but also merged git://people.freedesktop.org/~airlied/linux next in my branch, which probably fixes the problem. > I fixed it locally and will see if I see ADV7511 problems. I cannot > fully test HDMI probably, because it seems that this HDMI->DVI chain I > have does not work. It's supposed to be supported, so that might be something we need to fix. More work for me \o/ :-) > Tested that with a kernel which used to work in Dublin. Need to get another > HDMI device. > > > board the adv7511 fails to probe completely due to the regmap_read() > > failure. > > Okay, so before any 'modetest' activity. Which kernelconfig? > shmobile_defconfig? .config attached.
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index 6e459a338ccc75..36c79301044b71 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -266,6 +266,13 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv) rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND); } +static void rcar_i2c_next_msg(struct rcar_i2c_priv *priv) +{ + priv->msg++; + priv->msgs_left--; + rcar_i2c_prepare_msg(priv); +} + /* * interrupt functions */ @@ -308,21 +315,17 @@ static int rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr) * [ICRXTX] -> [SHIFT] -> [I2C bus] */ - if (priv->flags & ID_LAST_MSG) + if (priv->flags & ID_LAST_MSG) { /* * If current msg is the _LAST_ msg, * prepare stop condition here. * ID_DONE will be set on STOP irq. */ rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP); - else - /* - * If current msg is _NOT_ last msg, - * it doesn't call stop phase. - * thus, there is no STOP irq. - * return ID_DONE here. - */ - return ID_DONE; + } else { + rcar_i2c_next_msg(priv); + return 0; + } } rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_SEND); @@ -366,7 +369,10 @@ static int rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) else rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA); - rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_RECV); + if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG)) + rcar_i2c_next_msg(priv); + else + rcar_i2c_write(priv, ICMSR, RCAR_IRQ_ACK_RECV); return 0; } @@ -461,6 +467,7 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr) /* Stop */ if (msr & MST) { + priv->msgs_left--; /* The last message also made it */ rcar_i2c_flags_set(priv, ID_DONE); goto out; } @@ -500,35 +507,28 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, /* This HW can't send STOP after address phase */ if (msgs[i].len == 0) { ret = -EOPNOTSUPP; - break; - } - - /* init each data */ - priv->msg = &msgs[i]; - priv->msgs_left = num - i; - - rcar_i2c_prepare_msg(priv); - - time_left = wait_event_timeout(priv->wait, - rcar_i2c_flags_has(priv, ID_DONE), - adap->timeout); - if (!time_left) { - rcar_i2c_init(priv); - ret = -ETIMEDOUT; - break; - } - - if (rcar_i2c_flags_has(priv, ID_NACK)) { - ret = -ENXIO; - break; - } - - if (rcar_i2c_flags_has(priv, ID_ARBLOST)) { - ret = -EAGAIN; - break; + goto out; } + } - ret = i + 1; /* The number of transfer */ + /* init data */ + priv->msg = msgs; + priv->msgs_left = num; + + rcar_i2c_prepare_msg(priv); + + time_left = wait_event_timeout(priv->wait, + rcar_i2c_flags_has(priv, ID_DONE), + num * adap->timeout); + if (!time_left) { + rcar_i2c_init(priv); + ret = -ETIMEDOUT; + } else if (rcar_i2c_flags_has(priv, ID_NACK)) { + ret = -ENXIO; + } else if (rcar_i2c_flags_has(priv, ID_ARBLOST)) { + ret = -EAGAIN; + } else { + ret = num - priv->msgs_left; /* The number of transfer */ } out: pm_runtime_put(dev);