[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 alpernebiyasak@gmail.com --- 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) {
On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak alpernebiyasak@gmail.com 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=-5With 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=-110The "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 alpernebiyasak@gmail.com
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 sjg@chromium.org
On 12/10/2020 06:34, Simon Glass wrote:
On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak alpernebiyasak@gmail.com 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 alpernebiyasak@gmail.com
Reviewed-by: Simon Glass sjg@chromium.org
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&state=*
Hi Alper,
On Tue, 13 Oct 2020 at 09:01, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
On 12/10/2020 06:34, Simon Glass wrote:
On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak alpernebiyasak@gmail.com 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 alpernebiyasak@gmail.com
Reviewed-by: Simon Glass sjg@chromium.org
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&state=*
Regards, Simon
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 alpernebiyasak@gmail.com wrote:
On 12/10/2020 06:34, Simon Glass wrote:
On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak alpernebiyasak@gmail.com 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 alpernebiyasak@gmail.com
Reviewed-by: Simon Glass sjg@chromium.org
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.
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 jwerner@chromium.org > 1 Lin Huang hl@rock-chips.com > 1 Patrick Georgi pgeorgi@chromium.org > 1 Patrick Georgi pgeorgi@google.com > 4 huang lin hl@rock-chips.com
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 hl@rock-chips.com > Original-Signed-off-by: Julius Werner jwerner@chromium.org > Original-Signed-off-by: Lin Huang hl@rock-chips.com > Signed-off-by: Patrick Georgi pgeorgi@chromium.org > Signed-off-by: Stefan Reinauer reinauer@chromium.org
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 th3fanbus@gmail.com > 1 Arthur Heymans arthur@aheymans.xyz > 3 David Hendricks dhendrix@chromium.org > 1 Ege Mihmanli egemih@google.com > 15 Elyes HAOUAS ehaouas@noos.fr > 1 Jacob Garber jgarber1@ualberta.ca > 7 Julius Werner jwerner@chromium.org > 1 Kyösti Mälkki kyosti.malkki@gmail.com > 13 Lin Huang hl@rock-chips.com > 1 Martin Roth martinroth@google.com > 2 Nickey Yang nickey.yang@rock-chips.com > 1 Patrick Georgi pgeorgi@chromium.org > 3 Patrick Georgi pgeorgi@google.com > 2 Shunqian Zheng zhengsq@rock-chips.com > 2 Yakir Yang ykk@rock-chips.com > 5 huang lin hl@rock-chips.com
$ 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 dhendrix@chromium.org > Original-Signed-off-by: huang lin hl@rock-chips.com > Original-Signed-off-by: Julius Werner jwerner@chromium.org > Original-Signed-off-by: Lin Huang hl@rock-chips.com > Original-Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com > Original-Signed-off-by: Yakir Yang ykk@rock-chips.com > Signed-off-by: Angel Pons th3fanbus@gmail.com > Signed-off-by: Arthur Heymans arthur@aheymans.xyz > Signed-off-by: Ege Mihmanli egemih@google.com > Signed-off-by: Elyes HAOUAS ehaouas@noos.fr > Signed-off-by: Jacob Garber jgarber1@ualberta.ca > Signed-off-by: Julius Werner jwerner@chromium.org > Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com > Signed-off-by: Lin Huang hl@rock-chips.com > Signed-off-by: Martin Roth martinroth@google.com > Signed-off-by: Nickey Yang nickey.yang@rock-chips.com > Signed-off-by: Patrick Georgi patrick@georgi-clan.de > Signed-off-by: Patrick Georgi pgeorgi@chromium.org > Signed-off-by: Patrick Georgi pgeorgi@google.com > Signed-off-by: Stefan Reinauer reinauer@chromium.org
(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.)
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 <jwerner@chromium.org> > 1 Lin Huang <hl@rock-chips.com> > 1 Patrick Georgi <pgeorgi@chromium.org> > 1 Patrick Georgi <pgeorgi@google.com> > 4 huang lin <hl@rock-chips.com>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 <hl@rock-chips.com> > Original-Signed-off-by: Julius Werner <jwerner@chromium.org> > Original-Signed-off-by: Lin Huang <hl@rock-chips.com> > Signed-off-by: Patrick Georgi <pgeorgi@chromium.org> > Signed-off-by: Stefan Reinauer <reinauer@chromium.org>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 <th3fanbus@gmail.com> > 1 Arthur Heymans <arthur@aheymans.xyz> > 3 David Hendricks <dhendrix@chromium.org> > 1 Ege Mihmanli <egemih@google.com> > 15 Elyes HAOUAS <ehaouas@noos.fr> > 1 Jacob Garber <jgarber1@ualberta.ca> > 7 Julius Werner <jwerner@chromium.org> > 1 Kyösti Mälkki <kyosti.malkki@gmail.com> > 13 Lin Huang <hl@rock-chips.com> > 1 Martin Roth <martinroth@google.com> > 2 Nickey Yang <nickey.yang@rock-chips.com> > 1 Patrick Georgi <pgeorgi@chromium.org> > 3 Patrick Georgi <pgeorgi@google.com> > 2 Shunqian Zheng <zhengsq@rock-chips.com> > 2 Yakir Yang <ykk@rock-chips.com> > 5 huang lin <hl@rock-chips.com> $ 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 <dhendrix@chromium.org> > Original-Signed-off-by: huang lin <hl@rock-chips.com> > Original-Signed-off-by: Julius Werner <jwerner@chromium.org> > Original-Signed-off-by: Lin Huang <hl@rock-chips.com> > Original-Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> > Original-Signed-off-by: Yakir Yang <ykk@rock-chips.com> > Signed-off-by: Angel Pons <th3fanbus@gmail.com> > Signed-off-by: Arthur Heymans <arthur@aheymans.xyz> > Signed-off-by: Ege Mihmanli <egemih@google.com> > Signed-off-by: Elyes HAOUAS <ehaouas@noos.fr> > Signed-off-by: Jacob Garber <jgarber1@ualberta.ca> > Signed-off-by: Julius Werner <jwerner@chromium.org> > Signed-off-by: Kyösti Mälkki <kyosti.malkki@gmail.com> > Signed-off-by: Lin Huang <hl@rock-chips.com> > Signed-off-by: Martin Roth <martinroth@google.com> > Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com> > Signed-off-by: Patrick Georgi <patrick@georgi-clan.de> > Signed-off-by: Patrick Georgi <pgeorgi@chromium.org> > Signed-off-by: Patrick Georgi <pgeorgi@google.com> > Signed-off-by: Stefan Reinauer <reinauer@chromium.org>(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.
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 sjg@chromium.org 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 philipp.tomsich@theobroma-systems.com 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 eric.gao@rock-chips.com 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 jacob-chen@iotwrt.com added: - drivers/video/rockchip/rk_lvds.c - arch/arm/include/asm/arch-rockchip/lvds_rk3288.h as Copyright 2016 Rockchip Inc.
Alper Nebi Yasak alpernebiyasak@gmail.com 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/dri...
On 15/10/2020 10:19, Arnaud Patard (Rtp) wrote:
Alper Nebi Yasak alpernebiyasak@gmail.com 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?
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 sjg@chromium.org 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 philipp.tomsich@theobroma-systems.com 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 eric.gao@rock-chips.com 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 jacob-chen@iotwrt.com 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.
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 sjg@chromium.org 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 philipp.tomsich@theobroma-systems.com 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 eric.gao@rock-chips.com 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 jacob-chen@iotwrt.com 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
Hi,
On Wed, 14 Oct 2020 at 09:24, Tom Rini trini@konsulko.com 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 alpernebiyasak@gmail.com wrote:
On 12/10/2020 06:34, Simon Glass wrote:
On Tue, 6 Oct 2020 at 14:40, Alper Nebi Yasak alpernebiyasak@gmail.com 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 alpernebiyasak@gmail.com
Reviewed-by: Simon Glass sjg@chromium.org
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
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=-5With 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=-110The "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 alpernebiyasak@gmail.com
Reviewed-by: Kever Yangkever.yang@rock-chips.com
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) {
participants (5)
-
Alper Nebi Yasak -
Arnaud Patard -
Kever Yang -
Simon Glass -
Tom Rini