Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
Hi,
On 2020/10/16 上午2:29, Tom Rini wrote:
On Wed, Oct 14, 2020 at 11:39:40PM +0300, Alper Nebi Yasak wrote:
On 14/10/2020 22:31, Tom Rini wrote:
On Wed, Oct 14, 2020 at 09:58:28PM +0300, Alper Nebi Yasak wrote:
On 14/10/2020 18:24, Tom Rini wrote:
Ugh. In so far as anything can be re-licensed, who did it all
originally? I suspect coreboot isn't interested in 2.0+ but we can do
2.0-only.
For this patch, coreboot commit b9a7877568cf ("rockchip/*: refactor edp
driver") introduces the related change to src/soc/rockchip/common/edp.c
renamed from .../rk3288/edp.c, which was introduced at coreboot commit
40f558e8f4f7 ("rockchip: support display").
Right, sorry. I mean, on the U-Boot side, where did things come from?
I wonder how we got a different license text, and perhaps if we
shouldn't just re-port the coreboot code over as a clean/clear way to
re-license it to GPL-2.0-only.
I'm not sure re-porting is a great idea from the technical perspective.
I've been reading both drivers to compare them, there are also things in
U-Boot that're missing from coreboot. Things like DM integration are
also not there as they're U-Boot specific.
I checked some files with git log and checked the first commit that
showed up for each.
Simon Glass added:
- drivers/video/rockchip/rk_edp.c
- drivers/video/rockchip/rk_hdmi.c
- drivers/video/rockchip/rk_vop.c
- arch/arm/include/asm/arch-rockchip/vop_rk3288.h
- arch/arm/include/asm/arch-rockchip/edp_rk3288.h
as Copyright (c) 2015 Google, Inc
Copyright 2014 Rockchip Inc.
Philipp Tomsich added:
- drivers/video/rockchip/rk3288_hdmi.c
- drivers/video/rockchip/rk3399_hdmi.c
- drivers/video/rockchip/rk_hdmi.h
- drivers/video/rockchip/rk_vop.h
as Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
- drivers/video/rockchip/rk3288_vop.c
- drivers/video/rockchip/rk3399_vop.c
as Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
Copyright (c) 2015 Google, Inc
Copyright 2014 Rockchip Inc.
Eric Gao added:
- drivers/video/rockchip/rk3288_mipi.c
- drivers/video/rockchip/rk3399_mipi.c
- drivers/video/rockchip/rk_mipi.c
- drivers/video/rockchip/rk_mipi.h
- arch/arm/include/asm/arch-rockchip/rockchip_mipi_dsi.h
as Copyright (C) 2017 Fuzhou Rockchip Electronics Co., Ltd
Jacob Chen added:
- drivers/video/rockchip/rk_lvds.c
- arch/arm/include/asm/arch-rockchip/lvds_rk3288.h
as Copyright 2016 Rockchip Inc.
Probably then the best "I am not a lawyer" answer is to have Philipp
Tomsich, Eric Gao and Jacob Chen all acked-by a patch to mark our driver
as GPL-2.0-only so that it's very clear that we can take improvements
from other GPL-2.0-only sources.
It's OK for Rockchip to make this change update license to GPL-2.0+.
Thanks,
- Kever
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On 2020/10/7 上午4:39, Alper Nebi Yasak wrote:
Found this by comparing it to the coreboot driver, a form of this call
was introduced there in their commit b9a7877568cf ("rockchip/*: refactor
edp driver"). This is copy-pasted from U-Boot's link_train_cr() slightly
above it.
Without this on a gru-kevin chromebook, I have:
clock recovery at voltage 0 pre-emphasis 0
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
channel eq failed, ret=-5
link train failed!
rk_vop_probe() Device failed: ret=-5
With this, it looks like training succeeds:
clock recovery at voltage 0 pre-emphasis 0
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 6dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 6dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 6dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 6dB
using signal parameters: voltage 0.4V pre_emph 6dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 0dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 0dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 0dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 0dB
using signal parameters: voltage 0.4V pre_emph 0dB
channel eq at voltage 0 pre-emphasis 0
config video failed
rk_vop_probe() Device failed: ret=-110
The "config video failed" error also goes away when I disable higher
log levels, and it claims to have successfully probed the device.
Signed-off-by: Alper Nebi Yasak
Reviewed-by: Kever Yang
Thanks,
- Kever
---
I'm testing this with a lot of other patches to make the board work. The
actual tree I'm using is available here:
https://github.com/alpernebbi/u-boot/commits/rk3399-gru-kevin/wip
(currently at commit c0dc4b42afe770671ce7bb0dd519d894a3acdea0)
drivers/video/rockchip/rk_edp.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
index 000bd48140..a032eb6889 100644
--- a/drivers/video/rockchip/rk_edp.c
+++ b/drivers/video/rockchip/rk_edp.c
@@ -559,6 +559,12 @@ static int rk_edp_link_train_ce(struct rk_edp_priv *edp)
channel_eq = 0;
for (tries = 0; tries < 5; tries++) {
rk_edp_set_link_training(edp, edp->train_set);
+ ret = rk_edp_dpcd_write(regs, DPCD_TRAINING_LANE0_SET,
+ edp->train_set,
+ edp->link_train.lane_count);
+ if (ret)
+ return ret;
+
udelay(400);
if (rk_edp_dpcd_read_link_status(edp, status) < 0) {
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On Wed, Oct 14, 2020 at 11:39:40PM +0300, Alper Nebi Yasak wrote:
> On 14/10/2020 22:31, Tom Rini wrote:
> > On Wed, Oct 14, 2020 at 09:58:28PM +0300, Alper Nebi Yasak wrote:
> >> On 14/10/2020 18:24, Tom Rini wrote:
> >>> Ugh. In so far as anything can be re-licensed, who did it all
> >>> originally? I suspect coreboot isn't interested in 2.0+ but we can do
> >>> 2.0-only.
> >>
> >> For this patch, coreboot commit b9a7877568cf ("rockchip/*: refactor edp
> >> driver") introduces the related change to src/soc/rockchip/common/edp.c
> >> renamed from .../rk3288/edp.c, which was introduced at coreboot commit
> >> 40f558e8f4f7 ("rockchip: support display").
> >
> > Right, sorry. I mean, on the U-Boot side, where did things come from?
> > I wonder how we got a different license text, and perhaps if we
> > shouldn't just re-port the coreboot code over as a clean/clear way to
> > re-license it to GPL-2.0-only.
>
> I'm not sure re-porting is a great idea from the technical perspective.
> I've been reading both drivers to compare them, there are also things in
> U-Boot that're missing from coreboot. Things like DM integration are
> also not there as they're U-Boot specific.
>
> I checked some files with git log and checked the first commit that
> showed up for each.
>
> Simon Glass added:
> - drivers/video/rockchip/rk_edp.c
> - drivers/video/rockchip/rk_hdmi.c
> - drivers/video/rockchip/rk_vop.c
> - arch/arm/include/asm/arch-rockchip/vop_rk3288.h
> - arch/arm/include/asm/arch-rockchip/edp_rk3288.h
> as Copyright (c) 2015 Google, Inc
>Copyright 2014 Rockchip Inc.
>
> Philipp Tomsich added:
> - drivers/video/rockchip/rk3288_hdmi.c
> - drivers/video/rockchip/rk3399_hdmi.c
> - drivers/video/rockchip/rk_hdmi.h
> - drivers/video/rockchip/rk_vop.h
> as Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
> - drivers/video/rockchip/rk3288_vop.c
> - drivers/video/rockchip/rk3399_vop.c
> as Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
>Copyright (c) 2015 Google, Inc
>Copyright 2014 Rockchip Inc.
>
> Eric Gao added:
> - drivers/video/rockchip/rk3288_mipi.c
> - drivers/video/rockchip/rk3399_mipi.c
> - drivers/video/rockchip/rk_mipi.c
> - drivers/video/rockchip/rk_mipi.h
> - arch/arm/include/asm/arch-rockchip/rockchip_mipi_dsi.h
> as Copyright (C) 2017 Fuzhou Rockchip Electronics Co., Ltd
>
> Jacob Chen added:
> - drivers/video/rockchip/rk_lvds.c
> - arch/arm/include/asm/arch-rockchip/lvds_rk3288.h
> as Copyright 2016 Rockchip Inc.
Probably then the best "I am not a lawyer" answer is to have Philipp
Tomsich, Eric Gao and Jacob Chen all acked-by a patch to mark our driver
as GPL-2.0-only so that it's very clear that we can take improvements
from other GPL-2.0-only sources.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On 15/10/2020 10:19, Arnaud Patard (Rtp) wrote: > Alper Nebi Yasak writes: >> I'm not sure re-porting is a great idea from the technical perspective. >> I've been reading both drivers to compare them, there are also things in >> U-Boot that're missing from coreboot. Things like DM integration are >> also not there as they're U-Boot specific. > > I think it would be better to use fixes from the kernel or coreboot (if > license allows it) than copying coreboot blindly. Doing that will > possibly regress support from some systems as I fear that coreboot is > missing some HW support. I was planning on picking changes that look like improvements, yeah. > tbh, I've not looked at the coreboot code but given most HW vendor > history, it would be not so surprising that only the support for what's > needed on kevin or bob (EDP and CDN DP or HDMI on rk3399) has been added > to coreboot. I could only find veyron-based and gru-based Chrome OS devices (but the latter also includes gru-scarlet w/ MIPI), as their vendor firmware is coreboot-based. I'd guess other vendors are more focused on U-Boot. > I've also seen some uboot sources with rockchip linux drm code > [1]. I've no idea if it's used in practice but this means that people may > even ask "Why merging coreboot code while it's possible to use linux drm > code ?" ... AFAIK, Rockchip-downstream U-Boot [2] does it that way. Maybe that's not being done upstream for e.g. code size reasons? [2] https://github.com/rockchip-linux/u-boot/
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
Alper Nebi Yasak writes:
> On 14/10/2020 22:31, Tom Rini wrote:
>> On Wed, Oct 14, 2020 at 09:58:28PM +0300, Alper Nebi Yasak wrote:
>>> On 14/10/2020 18:24, Tom Rini wrote:
Ugh. In so far as anything can be re-licensed, who did it all
originally? I suspect coreboot isn't interested in 2.0+ but we can do
2.0-only.
>>>
>>> For this patch, coreboot commit b9a7877568cf ("rockchip/*: refactor edp
>>> driver") introduces the related change to src/soc/rockchip/common/edp.c
>>> renamed from .../rk3288/edp.c, which was introduced at coreboot commit
>>> 40f558e8f4f7 ("rockchip: support display").
>>
>> Right, sorry. I mean, on the U-Boot side, where did things come from?
>> I wonder how we got a different license text, and perhaps if we
>> shouldn't just re-port the coreboot code over as a clean/clear way to
>> re-license it to GPL-2.0-only.
>
> I'm not sure re-porting is a great idea from the technical perspective.
> I've been reading both drivers to compare them, there are also things in
> U-Boot that're missing from coreboot. Things like DM integration are
> also not there as they're U-Boot specific.
>
I think it would be better to use fixes from the kernel or coreboot (if
license allows it) than copying coreboot blindly. Doing that will
possibly regress support from some systems as I fear that coreboot is
missing some HW support.
tbh, I've not looked at the coreboot code but given most HW vendor
history, it would be not so surprising that only the support for what's
needed on kevin or bob (EDP and CDN DP or HDMI on rk3399) has been added
to coreboot.
I've also seen some uboot sources with rockchip linux drm code
[1]. I've no idea if it's used in practice but this means that people may
even ask "Why merging coreboot code while it's possible to use linux drm
code ?" ...
Arnaud
[1]
https://gitlab.collabora.com/nicolas/rockchip-uboot/-/tree/rk3399-roc-pc/drivers/video/drm
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On 14/10/2020 22:31, Tom Rini wrote:
> On Wed, Oct 14, 2020 at 09:58:28PM +0300, Alper Nebi Yasak wrote:
>> On 14/10/2020 18:24, Tom Rini wrote:
>>> Ugh. In so far as anything can be re-licensed, who did it all
>>> originally? I suspect coreboot isn't interested in 2.0+ but we can do
>>> 2.0-only.
>>
>> For this patch, coreboot commit b9a7877568cf ("rockchip/*: refactor edp
>> driver") introduces the related change to src/soc/rockchip/common/edp.c
>> renamed from .../rk3288/edp.c, which was introduced at coreboot commit
>> 40f558e8f4f7 ("rockchip: support display").
>
> Right, sorry. I mean, on the U-Boot side, where did things come from?
> I wonder how we got a different license text, and perhaps if we
> shouldn't just re-port the coreboot code over as a clean/clear way to
> re-license it to GPL-2.0-only.
I'm not sure re-porting is a great idea from the technical perspective.
I've been reading both drivers to compare them, there are also things in
U-Boot that're missing from coreboot. Things like DM integration are
also not there as they're U-Boot specific.
I checked some files with git log and checked the first commit that
showed up for each.
Simon Glass added:
- drivers/video/rockchip/rk_edp.c
- drivers/video/rockchip/rk_hdmi.c
- drivers/video/rockchip/rk_vop.c
- arch/arm/include/asm/arch-rockchip/vop_rk3288.h
- arch/arm/include/asm/arch-rockchip/edp_rk3288.h
as Copyright (c) 2015 Google, Inc
Copyright 2014 Rockchip Inc.
Philipp Tomsich added:
- drivers/video/rockchip/rk3288_hdmi.c
- drivers/video/rockchip/rk3399_hdmi.c
- drivers/video/rockchip/rk_hdmi.h
- drivers/video/rockchip/rk_vop.h
as Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
- drivers/video/rockchip/rk3288_vop.c
- drivers/video/rockchip/rk3399_vop.c
as Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
Copyright (c) 2015 Google, Inc
Copyright 2014 Rockchip Inc.
Eric Gao added:
- drivers/video/rockchip/rk3288_mipi.c
- drivers/video/rockchip/rk3399_mipi.c
- drivers/video/rockchip/rk_mipi.c
- drivers/video/rockchip/rk_mipi.h
- arch/arm/include/asm/arch-rockchip/rockchip_mipi_dsi.h
as Copyright (C) 2017 Fuzhou Rockchip Electronics Co., Ltd
Jacob Chen added:
- drivers/video/rockchip/rk_lvds.c
- arch/arm/include/asm/arch-rockchip/lvds_rk3288.h
as Copyright 2016 Rockchip Inc.
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On Wed, Oct 14, 2020 at 09:58:28PM +0300, Alper Nebi Yasak wrote:
> On 14/10/2020 18:24, Tom Rini wrote:
> > On Tue, Oct 13, 2020 at 09:54:55AM -0600, Simon Glass wrote:
> >> I think it is OK to change the file to GPL2. I'm not sure if changing
> >> coreboot parts to 2.0+ is an option. I believe the use of 2+ in U-Boot
> >> is for fairly narrow reasons, but I'm not sure if that is documented
> >> anywhere.
> >>
> >> +Tom Rini might have a comment
> >
> > Ugh. In so far as anything can be re-licensed, who did it all
> > originally? I suspect coreboot isn't interested in 2.0+ but we can do
> > 2.0-only.
>
> For this patch, coreboot commit b9a7877568cf ("rockchip/*: refactor edp
> driver") introduces the related change to src/soc/rockchip/common/edp.c
> renamed from .../rk3288/edp.c, which was introduced at coreboot commit
> 40f558e8f4f7 ("rockchip: support display").
>
> $ git shortlog -s -e b9a7877568cf --
> src/soc/rockchip/{common,rk3288}/edp.c
> > 2 Julius Werner
> > 1 Lin Huang
> > 1 Patrick Georgi
> > 1 Patrick Georgi
> > 4 huang lin
>
> The sign-offs are:
>
> $ git log b9a7877568cf -- src/soc/rockchip/{common,rk3288}/edp.c \
> | grep -i "Signed-off-by:" | sort -u
> >Original-Signed-off-by: huang lin
> >Original-Signed-off-by: Julius Werner
> >Original-Signed-off-by: Lin Huang
> >Signed-off-by: Patrick Georgi
> >Signed-off-by: Stefan Reinauer
>
> That file at that refactor-commit has two more fixes I'm interested in,
> and it's not the only file things could be ported from. If I run the
> above on a wider list of files upto current master I get 16 authors or
> 20 signoffs with duplicates (including e.g. original-signed-off-by),
> most of them either @google.com, @chromium.org, or @rock-chips.com.
>
> $ git shortlog -s -e --
> src/soc/rockchip/{common,rk3288,rk3399}/{include/soc/,include/,}{edp,vop,display,mipi}{.c,.h}
> > 1 Angel Pons
> > 1 Arthur Heymans
> > 3 David Hendricks
> > 1 Ege Mihmanli
> >15 Elyes HAOUAS
> > 1 Jacob Garber
> > 7 Julius Werner
> > 1 Kyösti Mälkki
> >13 Lin Huang
> > 1 Martin Roth
> > 2 Nickey Yang
> > 1 Patrick Georgi
> > 3 Patrick Georgi
> > 2 Shunqian Zheng
> > 2 Yakir Yang
> > 5 huang lin
>
> $ git log --
> src/soc/rockchip/{common,rk3288,rk3399}/{include/soc/,include/,}{edp,vop,display,mipi}{.c,.h}
> \
> | grep -i "Signed-off-by:" | sort -u
> >Original-Signed-off-by: David Hendricks
> >Original-Signed-off-by: huang lin
> >Original-Signed-off-by: Julius Werner
> >Original-Signed-off-by: Lin Huang
> >Original-Signed-off-by: Shunqian Zheng
> >Original-Signed-off-by: Yakir Yang
> >Signed-off-by: Angel Pons
> >Signed-off-by: Arthur Heymans
> >Signed-off-by: Ege Mihmanli
> >Signed-off-by: Elyes HAOUAS
> >Signed-off-by: Jacob Garber
> >Signed-off-by: Julius Werner
> >Signed-off-by: Kyösti Mälkki
> >Signed-off-by: Lin Huang
> >Signed-off-by: Martin Roth
> >Signed-off-by: Nickey Yang
> >Signed-off-by: Patrick Georgi
> >Signed-off-by: Patrick Georgi
> >Signed-off-by: Patrick Georgi
> >Signed-off-by: Stefan Reinauer
>
> (There's also hdmi{.c,.h} licensed w/ GPL-2.0-or-later, and clock{.c,.h}
> for which the U-Boot counterpart is already "GPL-2.0" assuming thats
> GPL-2.0-only, so I've excluded both.)
Right, sorry. I mean, on the U-Boot side, where did things come from?
I wonder how we got a different license text, and perhaps if we
shouldn't just re-port the coreboot code over as a clean/clear way to
re-license it to GPL-2.0-only.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
Hi,
On Wed, 14 Oct 2020 at 09:24, Tom Rini wrote:
>
> On Tue, Oct 13, 2020 at 09:54:55AM -0600, Simon Glass wrote:
> > Hi Alper,
> >
> > On Tue, 13 Oct 2020 at 09:01, Alper Nebi Yasak
> > wrote:
> > >
> > > On 12/10/2020 06:34, Simon Glass wrote:
> > > > On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak
> > > > wrote:
> > > >>
> > > >> Found this by comparing it to the coreboot driver, a form of this call
> > > >> was introduced there in their commit b9a7877568cf ("rockchip/*:
> > > >> refactor
> > > >> edp driver"). This is copy-pasted from U-Boot's link_train_cr()
> > > >> slightly
> > > >> above it.
> > > >>
> > > >> Signed-off-by: Alper Nebi Yasak
> > > >
> > > > Reviewed-by: Simon Glass
> > >
> > > Simon, I noticed the coreboot driver is GPL-2.0-only, but the U-Boot
> > > driver is GPL-2.0+, is that a problem for this patch?
> > >
> > > They also seem to be almost the same especially in their earlier
> > > revisions (even had the same typos in some comments). It could be good
> > > to sync the two drivers to pick improvements from it e.g. support for
> > > rk3399 (though there's an RFC series for that [1]), but the license
> > > difference makes it difficult. Could the coreboot parts be relicensed to
> > > GPL-2.0+ by Google and/or Rockchip? Alternatively, is it OK to change
> > > the U-Boot one to GPL-2.0-only to sync things from coreboot?
> >
> > I think it is OK to change the file to GPL2. I'm not sure if changing
> > coreboot parts to 2.0+ is an option. I believe the use of 2+ in U-Boot
> > is for fairly narrow reasons, but I'm not sure if that is documented
> > anywhere.
> >
> > +Tom Rini might have a comment
>
> Ugh. In so far as anything can be re-licensed, who did it all
> originally? I suspect coreboot isn't interested in 2.0+ but we can do
> 2.0-only.
Well I think the Rockchip engineers wrote it originally, so perhaps
they can just relicense when contributing to another project.
Regards,
Simon
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On 14/10/2020 18:24, Tom Rini wrote:
> On Tue, Oct 13, 2020 at 09:54:55AM -0600, Simon Glass wrote:
>> I think it is OK to change the file to GPL2. I'm not sure if changing
>> coreboot parts to 2.0+ is an option. I believe the use of 2+ in U-Boot
>> is for fairly narrow reasons, but I'm not sure if that is documented
>> anywhere.
>>
>> +Tom Rini might have a comment
>
> Ugh. In so far as anything can be re-licensed, who did it all
> originally? I suspect coreboot isn't interested in 2.0+ but we can do
> 2.0-only.
For this patch, coreboot commit b9a7877568cf ("rockchip/*: refactor edp
driver") introduces the related change to src/soc/rockchip/common/edp.c
renamed from .../rk3288/edp.c, which was introduced at coreboot commit
40f558e8f4f7 ("rockchip: support display").
$ git shortlog -s -e b9a7877568cf -- src/soc/rockchip/{common,rk3288}/edp.c
> 2 Julius Werner
> 1 Lin Huang
> 1 Patrick Georgi
> 1 Patrick Georgi
> 4 huang lin
The sign-offs are:
$ git log b9a7877568cf -- src/soc/rockchip/{common,rk3288}/edp.c \
| grep -i "Signed-off-by:" | sort -u
>Original-Signed-off-by: huang lin
>Original-Signed-off-by: Julius Werner
>Original-Signed-off-by: Lin Huang
>Signed-off-by: Patrick Georgi
>Signed-off-by: Stefan Reinauer
That file at that refactor-commit has two more fixes I'm interested in,
and it's not the only file things could be ported from. If I run the
above on a wider list of files upto current master I get 16 authors or
20 signoffs with duplicates (including e.g. original-signed-off-by),
most of them either @google.com, @chromium.org, or @rock-chips.com.
$ git shortlog -s -e --
src/soc/rockchip/{common,rk3288,rk3399}/{include/soc/,include/,}{edp,vop,display,mipi}{.c,.h}
> 1 Angel Pons
> 1 Arthur Heymans
> 3 David Hendricks
> 1 Ege Mihmanli
>15 Elyes HAOUAS
> 1 Jacob Garber
> 7 Julius Werner
> 1 Kyösti Mälkki
>13 Lin Huang
> 1 Martin Roth
> 2 Nickey Yang
> 1 Patrick Georgi
> 3 Patrick Georgi
> 2 Shunqian Zheng
> 2 Yakir Yang
> 5 huang lin
$ git log --
src/soc/rockchip/{common,rk3288,rk3399}/{include/soc/,include/,}{edp,vop,display,mipi}{.c,.h}
\
| grep -i "Signed-off-by:" | sort -u
>Original-Signed-off-by: David Hendricks
>Original-Signed-off-by: huang lin
>Original-Signed-off-by: Julius Werner
>Original-Signed-off-by: Lin Huang
>Original-Signed-off-by: Shunqian Zheng
>Original-Signed-off-by: Yakir Yang
>Signed-off-by: Angel Pons
>Signed-off-by: Arthur Heymans
>Signed-off-by: Ege Mihmanli
>Signed-off-by: Elyes HAOUAS
>Signed-off-by: Jacob Garber
>Signed-off-by: Julius Werner
>Signed-off-by: Kyösti Mälkki
>Signed-off-by: Lin Huang
>Signed-off-by: Martin Roth
>Signed-off-by: Nickey Yang
>Signed-off-by: Patrick Georgi
>Signed-off-by: Patrick Georgi
>Signed-off-by: Patrick Georgi
>Signed-off-by: Stefan Reinauer
(There's also hdmi{.c,.h} licensed w/ GPL-2.0-or-later, and clock{.c,.h}
for which the U-Boot counterpart is already "GPL-2.0" assuming thats
GPL-2.0-only, so I've excluded both.)
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On Tue, Oct 13, 2020 at 09:54:55AM -0600, Simon Glass wrote:
> Hi Alper,
>
> On Tue, 13 Oct 2020 at 09:01, Alper Nebi Yasak
> wrote:
> >
> > On 12/10/2020 06:34, Simon Glass wrote:
> > > On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak
> > > wrote:
> > >>
> > >> Found this by comparing it to the coreboot driver, a form of this call
> > >> was introduced there in their commit b9a7877568cf ("rockchip/*: refactor
> > >> edp driver"). This is copy-pasted from U-Boot's link_train_cr() slightly
> > >> above it.
> > >>
> > >> Signed-off-by: Alper Nebi Yasak
> > >
> > > Reviewed-by: Simon Glass
> >
> > Simon, I noticed the coreboot driver is GPL-2.0-only, but the U-Boot
> > driver is GPL-2.0+, is that a problem for this patch?
> >
> > They also seem to be almost the same especially in their earlier
> > revisions (even had the same typos in some comments). It could be good
> > to sync the two drivers to pick improvements from it e.g. support for
> > rk3399 (though there's an RFC series for that [1]), but the license
> > difference makes it difficult. Could the coreboot parts be relicensed to
> > GPL-2.0+ by Google and/or Rockchip? Alternatively, is it OK to change
> > the U-Boot one to GPL-2.0-only to sync things from coreboot?
>
> I think it is OK to change the file to GPL2. I'm not sure if changing
> coreboot parts to 2.0+ is an option. I believe the use of 2+ in U-Boot
> is for fairly narrow reasons, but I'm not sure if that is documented
> anywhere.
>
> +Tom Rini might have a comment
Ugh. In so far as anything can be re-licensed, who did it all
originally? I suspect coreboot isn't interested in 2.0+ but we can do
2.0-only.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
Hi Alper,
On Tue, 13 Oct 2020 at 09:01, Alper Nebi Yasak wrote:
>
> On 12/10/2020 06:34, Simon Glass wrote:
> > On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak
> > wrote:
> >>
> >> Found this by comparing it to the coreboot driver, a form of this call
> >> was introduced there in their commit b9a7877568cf ("rockchip/*: refactor
> >> edp driver"). This is copy-pasted from U-Boot's link_train_cr() slightly
> >> above it.
> >>
> >> Signed-off-by: Alper Nebi Yasak
> >
> > Reviewed-by: Simon Glass
>
> Simon, I noticed the coreboot driver is GPL-2.0-only, but the U-Boot
> driver is GPL-2.0+, is that a problem for this patch?
>
> They also seem to be almost the same especially in their earlier
> revisions (even had the same typos in some comments). It could be good
> to sync the two drivers to pick improvements from it e.g. support for
> rk3399 (though there's an RFC series for that [1]), but the license
> difference makes it difficult. Could the coreboot parts be relicensed to
> GPL-2.0+ by Google and/or Rockchip? Alternatively, is it OK to change
> the U-Boot one to GPL-2.0-only to sync things from coreboot?
I think it is OK to change the file to GPL2. I'm not sure if changing
coreboot parts to 2.0+ is an option. I believe the use of 2+ in U-Boot
is for fairly narrow reasons, but I'm not sure if that is documented
anywhere.
+Tom Rini might have a comment
>
> [1] https://patchwork.ozlabs.org/project/uboot/list/?series=204229=*
Regards,
Simon
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On 12/10/2020 06:34, Simon Glass wrote:
> On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak
> wrote:
>>
>> Found this by comparing it to the coreboot driver, a form of this call
>> was introduced there in their commit b9a7877568cf ("rockchip/*: refactor
>> edp driver"). This is copy-pasted from U-Boot's link_train_cr() slightly
>> above it.
>>
>> Signed-off-by: Alper Nebi Yasak
>
> Reviewed-by: Simon Glass
Simon, I noticed the coreboot driver is GPL-2.0-only, but the U-Boot
driver is GPL-2.0+, is that a problem for this patch?
They also seem to be almost the same especially in their earlier
revisions (even had the same typos in some comments). It could be good
to sync the two drivers to pick improvements from it e.g. support for
rk3399 (though there's an RFC series for that [1]), but the license
difference makes it difficult. Could the coreboot parts be relicensed to
GPL-2.0+ by Google and/or Rockchip? Alternatively, is it OK to change
the U-Boot one to GPL-2.0-only to sync things from coreboot?
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=204229=*
Re: [PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak wrote:
>
> Found this by comparing it to the coreboot driver, a form of this call
> was introduced there in their commit b9a7877568cf ("rockchip/*: refactor
> edp driver"). This is copy-pasted from U-Boot's link_train_cr() slightly
> above it.
>
> Without this on a gru-kevin chromebook, I have:
>
> clock recovery at voltage 0 pre-emphasis 0
> requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
> using signal parameters: voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
> using signal parameters: voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
> using signal parameters: voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
> using signal parameters: voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
> using signal parameters: voltage 0.4V pre_emph 3.5dB
> channel eq failed, ret=-5
> link train failed!
> rk_vop_probe() Device failed: ret=-5
>
> With this, it looks like training succeeds:
>
> clock recovery at voltage 0 pre-emphasis 0
> requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
> using signal parameters: voltage 0.4V pre_emph 3.5dB
> requested signal parameters: lane 0 voltage 0.4V pre_emph 6dB
> requested signal parameters: lane 1 voltage 0.4V pre_emph 6dB
> requested signal parameters: lane 2 voltage 0.4V pre_emph 6dB
> requested signal parameters: lane 3 voltage 0.4V pre_emph 6dB
> using signal parameters: voltage 0.4V pre_emph 6dB
> requested signal parameters: lane 0 voltage 0.4V pre_emph 0dB
> requested signal parameters: lane 1 voltage 0.4V pre_emph 0dB
> requested signal parameters: lane 2 voltage 0.4V pre_emph 0dB
> requested signal parameters: lane 3 voltage 0.4V pre_emph 0dB
> using signal parameters: voltage 0.4V pre_emph 0dB
> channel eq at voltage 0 pre-emphasis 0
> config video failed
> rk_vop_probe() Device failed: ret=-110
>
> The "config video failed" error also goes away when I disable higher
> log levels, and it claims to have successfully probed the device.
>
> Signed-off-by: Alper Nebi Yasak
> ---
> I'm testing this with a lot of other patches to make the board work. The
> actual tree I'm using is available here:
>
> https://github.com/alpernebbi/u-boot/commits/rk3399-gru-kevin/wip
> (currently at commit c0dc4b42afe770671ce7bb0dd519d894a3acdea0)
>
> drivers/video/rockchip/rk_edp.c | 6 ++
> 1 file changed, 6 insertions(+)
>
Reviewed-by: Simon Glass
[PATCH] video: rockchip: Add missing dpcd_write() call to link_train_ce()
Found this by comparing it to the coreboot driver, a form of this call
was introduced there in their commit b9a7877568cf ("rockchip/*: refactor
edp driver"). This is copy-pasted from U-Boot's link_train_cr() slightly
above it.
Without this on a gru-kevin chromebook, I have:
clock recovery at voltage 0 pre-emphasis 0
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
channel eq failed, ret=-5
link train failed!
rk_vop_probe() Device failed: ret=-5
With this, it looks like training succeeds:
clock recovery at voltage 0 pre-emphasis 0
requested signal parameters: lane 0 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 3.5dB
using signal parameters: voltage 0.4V pre_emph 3.5dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 6dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 6dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 6dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 6dB
using signal parameters: voltage 0.4V pre_emph 6dB
requested signal parameters: lane 0 voltage 0.4V pre_emph 0dB
requested signal parameters: lane 1 voltage 0.4V pre_emph 0dB
requested signal parameters: lane 2 voltage 0.4V pre_emph 0dB
requested signal parameters: lane 3 voltage 0.4V pre_emph 0dB
using signal parameters: voltage 0.4V pre_emph 0dB
channel eq at voltage 0 pre-emphasis 0
config video failed
rk_vop_probe() Device failed: ret=-110
The "config video failed" error also goes away when I disable higher
log levels, and it claims to have successfully probed the device.
Signed-off-by: Alper Nebi Yasak
---
I'm testing this with a lot of other patches to make the board work. The
actual tree I'm using is available here:
https://github.com/alpernebbi/u-boot/commits/rk3399-gru-kevin/wip
(currently at commit c0dc4b42afe770671ce7bb0dd519d894a3acdea0)
drivers/video/rockchip/rk_edp.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/video/rockchip/rk_edp.c b/drivers/video/rockchip/rk_edp.c
index 000bd48140..a032eb6889 100644
--- a/drivers/video/rockchip/rk_edp.c
+++ b/drivers/video/rockchip/rk_edp.c
@@ -559,6 +559,12 @@ static int rk_edp_link_train_ce(struct rk_edp_priv *edp)
channel_eq = 0;
for (tries = 0; tries < 5; tries++) {
rk_edp_set_link_training(edp, edp->train_set);
+ ret = rk_edp_dpcd_write(regs, DPCD_TRAINING_LANE0_SET,
+ edp->train_set,
+ edp->link_train.lane_count);
+ if (ret)
+ return ret;
+
udelay(400);
if (rk_edp_dpcd_read_link_status(edp, status) < 0) {
--
2.28.0
