| Message ID | 20180112231002.26213-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/13/2018 2:10 AM, Laurent Pinchart wrote: > From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS > must be enabled and the bias crcuit enabled after the LVDS I/O pins are > enabled, not before. Fix the gen2 LVDS startup sequence accordingly. > > While at it, also fix the comment preceding the first LVDCR0 write that > still talks about hardcoding the LVDS mode 0. Please do this in a separate commit then... > Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support") You forgot to specify the other commit this one fixes -- I mean the comment fix. > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > [Set the mode and input at the same time as the BEN and LVEN bits] > Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > Hi Sergei, > > For your convenience (and if you agree with bundling mode setup with the first > write as explained in my review of patch 1/2), here's the updated version of > patch 2/2 that I have taken in my development branch. If you're fine with it > I'll keep it, otherwise we can continue the review discussion. As I said, I don't know how to interpret the note 3 in either manual. [...] MBR, Sergei
Hi Sergei, On Saturday, 13 January 2018 11:33:55 EET Sergei Shtylyov wrote: > On 1/13/2018 2:10 AM, Laurent Pinchart wrote: > > From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > > > According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS > > must be enabled and the bias crcuit enabled after the LVDS I/O pins are > > enabled, not before. Fix the gen2 LVDS startup sequence accordingly. > > > > While at it, also fix the comment preceding the first LVDCR0 write that > > still talks about hardcoding the LVDS mode 0. > > Please do this in a separate commit then... The reason I added it here is that I think we don't need patch 1/2 from this series, and I found a bit overkill to split a comment fix to a separate patch when we have a patch that touches the code around the comment. > > Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support") > > You forgot to specify the other commit this one fixes -- I mean the comment > fix. Do we need to for a comment update ? It doesn't affect fix the behaviour of the driver or device, and I'd thus prefer to avoid giving the wrong impression that this patch fixes an bug introduced in a previous commit, otherwise it might end up being backported unnecessarily. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > [Set the mode and input at the same time as the BEN and LVEN bits] > > Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > Hi Sergei, > > > > For your convenience (and if you agree with bundling mode setup with the > > first write as explained in my review of patch 1/2), here's the updated > > version of patch 2/2 that I have taken in my development branch. If > > you're fine with it I'll keep it, otherwise we can continue the review > > discussion. > > As I said, I don't know how to interpret the note 3 in either manual. As explained in my latest reply to patch 1/2, my understanding is that the parameters can be programmed at any time before step 6. The fact that the current code works seems to confirm that interpretation. We could ask Renesas for a confirmation if you want.
On 01/16/2018 06:46 PM, Laurent Pinchart wrote: >>> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> >>> According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS >>> must be enabled and the bias crcuit enabled after the LVDS I/O pins are Oops, this needs fixing (note the typo!). Could you please change this passage to: and the bias circuit must be enabled after the LVDS I/O pins are ? >>> enabled, not before. Fix the gen2 LVDS startup sequence accordingly. >>> >>> While at it, also fix the comment preceding the first LVDCR0 write that >>> still talks about hardcoding the LVDS mode 0. >> >> Please do this in a separate commit then... > > The reason I added it here is that I think we don't need patch 1/2 from this > series, and I found a bit overkill to split a comment fix to a separate patch > when we have a patch that touches the code around the comment. OK, you're the maintainer of this driver, you know better. :-) >>> Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support") >> >> You forgot to specify the other commit this one fixes -- I mean the comment >> fix. > > Do we need to for a comment update ? It doesn't affect fix the behaviour of > the driver or device, and I'd thus prefer to avoid giving the wrong impression > that this patch fixes an bug introduced in a previous commit, otherwise it > might end up being backported unnecessarily. OK, no dire need to backport indeed... >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>> [Set the mode and input at the same time as the BEN and LVEN bits] >>> Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>> Signed-off-by: Laurent Pinchart >>> <laurent.pinchart+renesas@ideasonboard.com> >>> --- >>> >>> drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> Hi Sergei, >>> >>> For your convenience (and if you agree with bundling mode setup with the >>> first write as explained in my review of patch 1/2), here's the updated >>> version of patch 2/2 that I have taken in my development branch. If >>> you're fine with it I'll keep it, otherwise we can continue the review >>> discussion. >> >> As I said, I don't know how to interpret the note 3 in either manual. Moreover, it seems to me that the notes don't match the start-up procedure anymore... > As explained in my latest reply to patch 1/2, my understanding is that the > parameters can be programmed at any time before step 6. The fact that the > current code works seems to confirm that interpretation. We could ask Renesas > for a confirmation if you want. Would be good to ask, indeed. MBR, Sergei
Hi Sergei, On Tuesday, 16 January 2018 22:17:31 EET Sergei Shtylyov wrote: > On 01/16/2018 06:46 PM, Laurent Pinchart wrote: > >>> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >>> > >>> According to the latest revision 2.00 of the R-Car gen2 manual, the LVDS > >>> must be enabled and the bias crcuit enabled after the LVDS I/O pins are > > Oops, this needs fixing (note the typo!). Could you please change this > passage to: > > and the bias circuit must be enabled after the LVDS I/O pins are > > ? Sure I'll fix that. > >>> enabled, not before. Fix the gen2 LVDS startup sequence accordingly. > >>> > >>> While at it, also fix the comment preceding the first LVDCR0 write that > >>> still talks about hardcoding the LVDS mode 0. > >> > >> Please do this in a separate commit then... > > > > The reason I added it here is that I think we don't need patch 1/2 from > > this series, and I found a bit overkill to split a comment fix to a > > separate patch when we have a patch that touches the code around the > > comment. > > OK, you're the maintainer of this driver, you know better. :-) > > >>> Fixes: 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support") > >> > >> You forgot to specify the other commit this one fixes -- I mean the > >> comment fix. > > > > Do we need to for a comment update ? It doesn't affect fix the behaviour > > of the driver or device, and I'd thus prefer to avoid giving the wrong > > impression that this patch fixes an bug introduced in a previous commit, > > otherwise it might end up being backported unnecessarily. > > OK, no dire need to backport indeed... > > >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >>> Reviewed-by: Laurent Pinchart > >>> <laurent.pinchart+renesas@ideasonboard.com> > >>> [Set the mode and input at the same time as the BEN and LVEN bits] > >>> Tested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >>> Signed-off-by: Laurent Pinchart > >>> <laurent.pinchart+renesas@ideasonboard.com> > >>> --- > >>> > >>> drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++------- > >>> 1 file changed, 7 insertions(+), 7 deletions(-) > >>> > >>> Hi Sergei, > >>> > >>> For your convenience (and if you agree with bundling mode setup with the > >>> first write as explained in my review of patch 1/2), here's the updated > >>> version of patch 2/2 that I have taken in my development branch. If > >>> you're fine with it I'll keep it, otherwise we can continue the review > >>> discussion. > >> > >> As I said, I don't know how to interpret the note 3 in either manual. > > Moreover, it seems to me that the notes don't match the start-up procedure > anymore... How so ? > > As explained in my latest reply to patch 1/2, my understanding is that the > > parameters can be programmed at any time before step 6. The fact that the > > current code works seems to confirm that interpretation. We could ask > > Renesas for a confirmation if you want. > > Would be good to ask, indeed. I'll ask.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c b/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c index abbb7b25129a..b37c255c3d93 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c @@ -59,20 +59,20 @@ static void rcar_du_lvdsenc_start_gen2(struct rcar_du_lvdsenc *lvds, rcar_lvds_write(lvds, LVDPLLCR, pllcr); + /* Turn all the channels on. */ + rcar_lvds_write(lvds, LVDCR1, + LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) | + LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY); + /* - * Select the input, hardcode mode 0, enable LVDS operation and turn - * bias circuitry on. + * Set the LVDS mode, select the input, enable LVDS operation, + * and turn bias circuitry on. */ lvdcr0 = (lvds->mode << LVDCR0_LVMD_SHIFT) | LVDCR0_BEN | LVDCR0_LVEN; if (rcrtc->index == 2) lvdcr0 |= LVDCR0_DUSEL; rcar_lvds_write(lvds, LVDCR0, lvdcr0); - /* Turn all the channels on. */ - rcar_lvds_write(lvds, LVDCR1, - LVDCR1_CHSTBY(3) | LVDCR1_CHSTBY(2) | - LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY); - /* * Turn the PLL on, wait for the startup delay, and turn the output * on.