drm: rcar-du: lvds: fix LVDS startup on R-Car gen2
Subsystems (3)
PATCH 2/2 drm: rcar-du: lvds: fix LVDS startup on R-Car gen2
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.
--- drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c +6 -5
@@ -59,11 +59,8 @@ static void rcar_du_lvdsenc_start_gen2(srcar_lvds_write (lvds,LVDPLLCR , pllcr); -/* -* 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 ; +/* Select the input and set the LVDS mode. */ + lvdcr0 = lvds->mode <<LVDCR0_LVMD_SHIFT ;if (rcrtc->index ==2 ) lvdcr0 |=LVDCR0_DUSEL ;rcar_lvds_write (lvds,LVDCR0 , lvdcr0);
@@ -73,6 +70,10 @@ static void rcar_du_lvdsenc_start_gen2(sLVDCR1_CHSTBY (3 ) |LVDCR1_CHSTBY (2 ) | LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY); +/* Enable LVDS operation and turn bias circuitry on. */ + lvdcr0 |=LVDCR0_BEN |LVDCR0_LVEN ; +rcar_lvds_write (lvds,LVDCR0 , lvdcr0); +/* * Turn the PLL on, wait for the startup delay, and turn the output * on.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Sergei,
Thank you for the patch.
Quoted 30 lines · On Friday, 12 January 2018 22:12:05 EET Sergei Shtylyov wrote:
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.
Fixes 90374b5c25c9 ("drm/rcar-du: Add internal LVDS encoder support")Signed-off-by Sergei Shtylyov <[email protected]>--- drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)Index:
linux/drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c
===================================================================drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c +2
-5@@ -59,11 +59,8 @@ static void rcar_du_lvdsenc_start_gen2(srcar_lvds_write (lvds,LVDPLLCR , pllcr); -/* -* 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 ; +/* Select the input and set the LVDS mode. */ + lvdcr0 = lvds->mode <<LVDCR0_LVMD_SHIFT ;if (rcrtc->index ==2 ) lvdcr0 |=LVDCR0_DUSEL ;rcar_lvds_write (lvds,LVDCR0 , lvdcr0);
drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c static void rcar_du_lvdsenc_start_gen2(s · line 75
Quoted 6 lines
drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c +3
-0@@ -73,6 +70,10 @@ static void rcar_du_lvdsenc_start_gen2(sLVDCR1_CHSTBY (3 ) |LVDCR1_CHSTBY (2 ) | LVDCR1_CHSTBY(1) | LVDCR1_CHSTBY(0) | LVDCR1_CLKSTBY); +/* Enable LVDS operation and turn bias circuitry on. */ + lvdcr0 |=LVDCR0_BEN |LVDCR0_LVEN ; +rcar_lvds_write (lvds,LVDCR0 , lvdcr0);
This change looks good, but for the reasons explained in my replies to patch 1/2, I would set the mode here along with the BEN and LVEN bits.
Quoted 3 lines
drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c +0
-0/* * Turn the PLL on, wait for the startup delay, and turn the output * on.
Signature · 3 lines
Regards, Laurent Pinchart
PATCH v2 2/2 drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2
From: Sergei Shtylyov <[email protected]>
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.
--- 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.
drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c +7 -7
@@ -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.
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2
Quoted 8 lines · Laurent Pinchart
From: Sergei Shtylyov <[email protected]>
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...
Quoted 1 line
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.
Quoted 9 lines
Signed-off-by Sergei Shtylyov <[email protected]>Reviewed-by Laurent Pinchart <[email protected]>[Set the mode and input at the same time as the BEN and LVEN bits]Tested-by Laurent Pinchart <[email protected]>Signed-off-by Laurent Pinchart <[email protected]>--- drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Quoted 6 lines
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
Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2
Hi Sergei,
Quoted 11 lines · On Saturday, 13 January 2018 11:33:55 EET Sergei Shtylyov wrote:
Quoted 8 lines · Laurent Pinchart
From: Sergei Shtylyov <[email protected]>
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.
Quoted 4 lines
Quoted 1 line
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.
Quoted 13 lines
Quoted 13 lines
Signed-off-by Sergei Shtylyov <[email protected]>Reviewed-by Laurent Pinchart <[email protected]>[Set the mode and input at the same time as the BEN and LVEN bits]Tested-by Laurent Pinchart <[email protected]>Signed-off-by Laurent Pinchart<[email protected]>
---drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)Hi Sergei,
Quoted 7 lines
Quoted 5 lines
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.
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2
Quoted 4 lines · Laurent Pinchart
Quoted 4 lines
Quoted 4 lines
From: Sergei Shtylyov <[email protected]>
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
?
Quoted 10 lines
Quoted 6 lines
Quoted 4 lines
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. :-)
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
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...
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Signed-off-by Sergei Shtylyov <[email protected]>Reviewed-by Laurent Pinchart <[email protected]>[Set the mode and input at the same time as the BEN and LVEN bits]Tested-by Laurent Pinchart <[email protected]>Signed-off-by Laurent Pinchart<[email protected]>
---drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)Hi Sergei,
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
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...
Quoted 4 lines
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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm: rcar-du: lvds: Fix LVDS startup on R-Car gen2
Hi Sergei,
Quoted 6 lines · On Tuesday, 16 January 2018 22:17:31 EET Sergei Shtylyov wrote:
Quoted 4 lines · Laurent Pinchart
Quoted 4 lines
Quoted 4 lines
From: Sergei Shtylyov <[email protected]>
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
Quoted 6 lines
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.
Quoted 46 lines
Quoted 11 lines
Quoted 6 lines
Quoted 4 lines
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. :-)
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
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...
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
Signed-off-by Sergei Shtylyov <[email protected]>Reviewed-by Laurent Pinchart[Set the mode and input at the same time as the BEN and LVEN bits]Tested-by Laurent Pinchart <[email protected]>Signed-off-by Laurent Pinchart<[email protected]>
---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.
Quoted 4 lines
Quoted 1 line
Quoted 1 line
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 ?
Quoted 6 lines
Quoted 4 lines
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.
Signature · 3 lines
Regards, Laurent Pinchart