Message ID | 20201006203951.18040-1-alpernebiyasak@gmail.com |
---|---|
State | Accepted |
Commit | 127c8d85cf4420167ed577121ea62f56ce13635e |
Delegated to: | Kever Yang |
Headers | show |
Series | video: rockchip: Add missing dpcd_write() call to link_train_ce() | expand |
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=-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(+) > 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.)
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 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/drivers/video/drm
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? [2] https://github.com/rockchip-linux/u-boot/
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.
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 <alpernebiyasak@gmail.com> Reviewed-by: Kever Yang<kever.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) {
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
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) {
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(+)