* [RFC PATCH 0/2] Proof of concept device tree overlays @ 2021-06-29 12:31 Michael Riesch 2021-06-29 12:31 ` [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection Michael Riesch 2021-06-29 12:31 ` [RFC PATCH 2/2] arm: rockchip-rk3568-evb: apply dt overlay for evb7 Michael Riesch 0 siblings, 2 replies; 8+ messages in thread From: Michael Riesch @ 2021-06-29 12:31 UTC (permalink / raw) To: barebox; +Cc: Michael Riesch Hi all, This is a kind request for comments with regard to the new feature that enables barebox to provide device tree overlays to the kernel. The first patch adds support for the hardware id detection on the Rockchip RK3568 EVB. The voltage tresholds are taken from the EVB1 schematic. The second patch applies a fictional device tree overlay (that is assumed to reside in rootfs/boot/) to the kernel for the EVB7. Is this how you all envisaged the usage of this new feature? Or do you have any suggestions? Thanks and best regards, Michael Michael Riesch (2): arm: rockchip-rk3568-evb: add hardware id detection arm: rockchip-rk3568-evb: apply dt overlay for evb7 arch/arm/boards/rockchip-rk3568-evb/board.c | 40 +++++++++++++++++++++ 1 file changed, 40 insertions(+) -- 2.20.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection 2021-06-29 12:31 [RFC PATCH 0/2] Proof of concept device tree overlays Michael Riesch @ 2021-06-29 12:31 ` Michael Riesch 2021-06-30 10:17 ` Ahmad Fatoum 2021-06-29 12:31 ` [RFC PATCH 2/2] arm: rockchip-rk3568-evb: apply dt overlay for evb7 Michael Riesch 1 sibling, 1 reply; 8+ messages in thread From: Michael Riesch @ 2021-06-29 12:31 UTC (permalink / raw) To: barebox; +Cc: Michael Riesch Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> --- arch/arm/boards/rockchip-rk3568-evb/board.c | 36 +++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/arch/arm/boards/rockchip-rk3568-evb/board.c b/arch/arm/boards/rockchip-rk3568-evb/board.c index 57c24ed3c..ee8e1b979 100644 --- a/arch/arm/boards/rockchip-rk3568-evb/board.c +++ b/arch/arm/boards/rockchip-rk3568-evb/board.c @@ -2,6 +2,7 @@ #include <common.h> #include <init.h> #include <mach/bbu.h> +#include <aiodev.h> #include <bootsource.h> static int rk3568_evb_probe(struct device_d *dev) @@ -34,3 +35,38 @@ static struct driver_d rk3568_evb_board_driver = { .of_compatible = rk3568_evb_of_match, }; coredevice_platform_driver(rk3568_evb_board_driver); + +static int rk3568_evb_detect_version(void) +{ + int ret = 0; + int evb_hw_id = 0; + int evb_hw_id_voltage = 1800; + struct aiochannel *evb_hw_id_chan; + + evb_hw_id_chan = aiochannel_by_name("aiodev0.in_value1_mV"); + if (!IS_ERR(evb_hw_id_chan)) + ret = aiochannel_get_value(evb_hw_id_chan, &evb_hw_id_voltage); + if (ret || IS_ERR(evb_hw_id_chan)) + pr_warn("couldn't retrieve hardware ID"); + + if (evb_hw_id_voltage > 1650) { + evb_hw_id = 1; + } else if (evb_hw_id_voltage > 1350) { + evb_hw_id = 2; + } else if (evb_hw_id_voltage > 1050) { + evb_hw_id = 3; + } else if (evb_hw_id_voltage > 750) { + evb_hw_id = 4; + } else if (evb_hw_id_voltage > 450) { + evb_hw_id = 5; + } else if (evb_hw_id_voltage > 150) { + evb_hw_id = 6; + } else { + evb_hw_id = 7; + } + + pr_info("Detected RK3568 EVB%d\n", evb_hw_id); + + return 0; +} +late_initcall(rk3568_evb_detect_version); -- 2.20.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection 2021-06-29 12:31 ` [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection Michael Riesch @ 2021-06-30 10:17 ` Ahmad Fatoum 2021-07-01 7:19 ` Michael Riesch 2021-07-03 20:49 ` Sascha Hauer 0 siblings, 2 replies; 8+ messages in thread From: Ahmad Fatoum @ 2021-06-30 10:17 UTC (permalink / raw) To: Michael Riesch, barebox Hello Michael, Trent already commented on the device tree overlay part. This commit here seems applicable regardless, so in case you want to resend it for upstream inclusion, some comments are inline. On 29.06.21 14:31, Michael Riesch wrote: > Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> A short commit message here would be nice, e.g.: The rk3568 EVB uses a voltage divider to ... etc. > --- > arch/arm/boards/rockchip-rk3568-evb/board.c | 36 +++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/arch/arm/boards/rockchip-rk3568-evb/board.c b/arch/arm/boards/rockchip-rk3568-evb/board.c > index 57c24ed3c..ee8e1b979 100644 > --- a/arch/arm/boards/rockchip-rk3568-evb/board.c > +++ b/arch/arm/boards/rockchip-rk3568-evb/board.c > @@ -2,6 +2,7 @@ > #include <common.h> > #include <init.h> > #include <mach/bbu.h> > +#include <aiodev.h> > #include <bootsource.h> > > static int rk3568_evb_probe(struct device_d *dev) > @@ -34,3 +35,38 @@ static struct driver_d rk3568_evb_board_driver = { > .of_compatible = rk3568_evb_of_match, > }; > coredevice_platform_driver(rk3568_evb_board_driver); > + > +static int rk3568_evb_detect_version(void) > +{ Once more 64-bit rockchip boards are added, they would all execute this initcall if this board is enabled. For this reason, you need a compatible check here. > + int ret = 0; > + int evb_hw_id = 0; Nitpick: Initializing variables with values that are never read risks introducing silent bugs once the code that uses it later on changes. If you leave it uninitialized you get a compiler warning if it's referenced without initialization. > + int evb_hw_id_voltage = 1800; I am not sure it's a good idea to report the EVB is v1 when the driver isn't enabled. > + struct aiochannel *evb_hw_id_chan; > + > + evb_hw_id_chan = aiochannel_by_name("aiodev0.in_value1_mV"); > + if (!IS_ERR(evb_hw_id_chan)) In the error case it makes no sense to continue here, you should early exit. > + ret = aiochannel_get_value(evb_hw_id_chan, &evb_hw_id_voltage); > + if (ret || IS_ERR(evb_hw_id_chan)) > + pr_warn("couldn't retrieve hardware ID"); early exit > + > + if (evb_hw_id_voltage > 1650) { > + evb_hw_id = 1; > + } else if (evb_hw_id_voltage > 1350) { > + evb_hw_id = 2; > + } else if (evb_hw_id_voltage > 1050) { > + evb_hw_id = 3; > + } else if (evb_hw_id_voltage > 750) { > + evb_hw_id = 4; > + } else if (evb_hw_id_voltage > 450) { > + evb_hw_id = 5; > + } else if (evb_hw_id_voltage > 150) { > + evb_hw_id = 6; > + } else { > + evb_hw_id = 7; > + } > + > + pr_info("Detected RK3568 EVB%d\n", evb_hw_id); You could populate a variable, e.g. global.board.revision here. That would allow using this info in scripts as Trent has described. > + > + return 0; > +} > +late_initcall(rk3568_evb_detect_version); Optimally, you would call the function from the board driver probe above. At postcore initcall, you wouldn't find the ADC yet, but if you return EPROBE_DEFER on error from the probe, the probe will be retried later on at which time it will succeed. With the deep probe patches Sascha just merged, it is possible to directly probe dependencies instead of retrying later and hoping it was probed in-between. It's still not enabled for the EVB here though and it also needs some code added to aiodev, so for now you could go the EPROBE_DEFER route (or add a compatible check). Cheers, Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection 2021-06-30 10:17 ` Ahmad Fatoum @ 2021-07-01 7:19 ` Michael Riesch 2021-07-03 20:49 ` Sascha Hauer 1 sibling, 0 replies; 8+ messages in thread From: Michael Riesch @ 2021-07-01 7:19 UTC (permalink / raw) To: Ahmad Fatoum, barebox Hello Ahmad, On 6/30/21 12:17 PM, Ahmad Fatoum wrote: > Hello Michael, > > Trent already commented on the device tree overlay part. > This commit here seems applicable regardless, > so in case you want to resend it for upstream inclusion, > some comments are inline. Thanks for your feedback. OK, I'll separate this part and resend a non-RFC patch (not sure when, though). > On 29.06.21 14:31, Michael Riesch wrote: >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> > > A short commit message here would be nice, e.g.: > > The rk3568 EVB uses a voltage divider to ... etc. > >> --- >> arch/arm/boards/rockchip-rk3568-evb/board.c | 36 +++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> >> diff --git a/arch/arm/boards/rockchip-rk3568-evb/board.c b/arch/arm/boards/rockchip-rk3568-evb/board.c >> index 57c24ed3c..ee8e1b979 100644 >> --- a/arch/arm/boards/rockchip-rk3568-evb/board.c >> +++ b/arch/arm/boards/rockchip-rk3568-evb/board.c >> @@ -2,6 +2,7 @@ >> #include <common.h> >> #include <init.h> >> #include <mach/bbu.h> >> +#include <aiodev.h> >> #include <bootsource.h> >> >> static int rk3568_evb_probe(struct device_d *dev) >> @@ -34,3 +35,38 @@ static struct driver_d rk3568_evb_board_driver = { >> .of_compatible = rk3568_evb_of_match, >> }; >> coredevice_platform_driver(rk3568_evb_board_driver); >> + >> +static int rk3568_evb_detect_version(void) >> +{ > > Once more 64-bit rockchip boards are added, they would all execute this > initcall if this board is enabled. For this reason, you need a compatible > check here. > >> + int ret = 0; >> + int evb_hw_id = 0; > > Nitpick: Initializing variables with values that are never read risks introducing > silent bugs once the code that uses it later on changes. If you leave > it uninitialized you get a compiler warning if it's referenced without > initialization. > >> + int evb_hw_id_voltage = 1800; > > I am not sure it's a good idea to report the EVB is v1 when the > driver isn't enabled. This default is based on the assumption that the EVB1 device tree works for all variants and does not require any overlays. The other variants do require a specific overlay. This assumption was somewhat valid in the scope of this RFC patch series but cannot be applied in general... > >> + struct aiochannel *evb_hw_id_chan; >> + >> + evb_hw_id_chan = aiochannel_by_name("aiodev0.in_value1_mV"); >> + if (!IS_ERR(evb_hw_id_chan)) > > In the error case it makes no sense to continue here, you should early exit. > >> + ret = aiochannel_get_value(evb_hw_id_chan, &evb_hw_id_voltage); >> + if (ret || IS_ERR(evb_hw_id_chan)) >> + pr_warn("couldn't retrieve hardware ID"); > > early exit > >> + >> + if (evb_hw_id_voltage > 1650) { >> + evb_hw_id = 1; >> + } else if (evb_hw_id_voltage > 1350) { >> + evb_hw_id = 2; >> + } else if (evb_hw_id_voltage > 1050) { >> + evb_hw_id = 3; >> + } else if (evb_hw_id_voltage > 750) { >> + evb_hw_id = 4; >> + } else if (evb_hw_id_voltage > 450) { >> + evb_hw_id = 5; >> + } else if (evb_hw_id_voltage > 150) { >> + evb_hw_id = 6; >> + } else { >> + evb_hw_id = 7; >> + } >> + >> + pr_info("Detected RK3568 EVB%d\n", evb_hw_id); > > You could populate a variable, e.g. global.board.revision > here. That would allow using this info in scripts as Trent has described. ... so what should happen if the board variant cannot be detected? Should the variable be empty? Not set at all? Set to "unknown"? >> + >> + return 0; >> +} >> +late_initcall(rk3568_evb_detect_version); > > Optimally, you would call the function from the board driver probe above. > At postcore initcall, you wouldn't find the ADC yet, but if you > return EPROBE_DEFER on error from the probe, the probe will be retried > later on at which time it will succeed. > > With the deep probe patches Sascha just merged, it is possible to directly > probe dependencies instead of retrying later and hoping it was probed > in-between. It's still not enabled for the EVB here though and it also > needs some code added to aiodev, so for now you could go the EPROBE_DEFER > route (or add a compatible check). I think I'll stick to the late_initcall with compatible check as this seems to be the most straightforward solution right now. Regards, Michael _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection 2021-06-30 10:17 ` Ahmad Fatoum 2021-07-01 7:19 ` Michael Riesch @ 2021-07-03 20:49 ` Sascha Hauer 1 sibling, 0 replies; 8+ messages in thread From: Sascha Hauer @ 2021-07-03 20:49 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: Michael Riesch, barebox On Wed, Jun 30, 2021 at 12:17:59PM +0200, Ahmad Fatoum wrote: > Hello Michael, > > Trent already commented on the device tree overlay part. > This commit here seems applicable regardless, > so in case you want to resend it for upstream inclusion, > some comments are inline. > > > On 29.06.21 14:31, Michael Riesch wrote: > > Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> > > A short commit message here would be nice, e.g.: > > The rk3568 EVB uses a voltage divider to ... etc. > > > --- > > arch/arm/boards/rockchip-rk3568-evb/board.c | 36 +++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/arch/arm/boards/rockchip-rk3568-evb/board.c b/arch/arm/boards/rockchip-rk3568-evb/board.c > > index 57c24ed3c..ee8e1b979 100644 > > --- a/arch/arm/boards/rockchip-rk3568-evb/board.c > > +++ b/arch/arm/boards/rockchip-rk3568-evb/board.c > > @@ -2,6 +2,7 @@ > > #include <common.h> > > #include <init.h> > > #include <mach/bbu.h> > > +#include <aiodev.h> > > #include <bootsource.h> > > > > static int rk3568_evb_probe(struct device_d *dev) > > @@ -34,3 +35,38 @@ static struct driver_d rk3568_evb_board_driver = { > > .of_compatible = rk3568_evb_of_match, > > }; > > coredevice_platform_driver(rk3568_evb_board_driver); > > + > > +static int rk3568_evb_detect_version(void) > > +{ > > Once more 64-bit rockchip boards are added, they would all execute this > initcall if this board is enabled. For this reason, you need a compatible > check here. Instead of a compatible check I'd prefer a static bool machine_is_rk3568_evb; Set it to true in the driver above and test for it in additional initcalls. Testing for a variable is cheaper in terms of binary size and runtime penalty than compatible checks. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH 2/2] arm: rockchip-rk3568-evb: apply dt overlay for evb7 2021-06-29 12:31 [RFC PATCH 0/2] Proof of concept device tree overlays Michael Riesch 2021-06-29 12:31 ` [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection Michael Riesch @ 2021-06-29 12:31 ` Michael Riesch 2021-06-29 21:58 ` Trent Piepho 1 sibling, 1 reply; 8+ messages in thread From: Michael Riesch @ 2021-06-29 12:31 UTC (permalink / raw) To: barebox; +Cc: Michael Riesch Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net> --- arch/arm/boards/rockchip-rk3568-evb/board.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arch/arm/boards/rockchip-rk3568-evb/board.c b/arch/arm/boards/rockchip-rk3568-evb/board.c index ee8e1b979..5b20e016a 100644 --- a/arch/arm/boards/rockchip-rk3568-evb/board.c +++ b/arch/arm/boards/rockchip-rk3568-evb/board.c @@ -4,6 +4,7 @@ #include <mach/bbu.h> #include <aiodev.h> #include <bootsource.h> +#include <environment.h> static int rk3568_evb_probe(struct device_d *dev) { @@ -63,6 +64,9 @@ static int rk3568_evb_detect_version(void) evb_hw_id = 6; } else { evb_hw_id = 7; + setenv("global.of.overlay.dir", "boot"); + setenv("global.of.overlay.filepattern", + "rk3568-evb7-overlay.dtb*"); } pr_info("Detected RK3568 EVB%d\n", evb_hw_id); -- 2.20.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] arm: rockchip-rk3568-evb: apply dt overlay for evb7 2021-06-29 12:31 ` [RFC PATCH 2/2] arm: rockchip-rk3568-evb: apply dt overlay for evb7 Michael Riesch @ 2021-06-29 21:58 ` Trent Piepho 2021-06-30 9:18 ` Michael Riesch 0 siblings, 1 reply; 8+ messages in thread From: Trent Piepho @ 2021-06-29 21:58 UTC (permalink / raw) To: Michael Riesch; +Cc: Barebox List On Tue, Jun 29, 2021 at 5:32 AM Michael Riesch <michael.riesch@wolfvision.net> wrote: > @@ -63,6 +64,9 @@ static int rk3568_evb_detect_version(void) > evb_hw_id = 6; > } else { > evb_hw_id = 7; > + setenv("global.of.overlay.dir", "boot"); Hardcoding the location of boot files in the rootfs into board code doesn't seem right to me. Seems like either creating a nv/global.of.overlay.dir file in the default env for the board or creating an init script that sets that variable would be the way to set this. > + setenv("global.of.overlay.filepattern", > + "rk3568-evb7-overlay.dtb*"); What if one had a board magicvar, something like: globalvar_add_simple("board.variant", "rk3568-evb7"); // Or just evb7 if rk3568 is already somewhere else BAREBOX_MAGICVAR(global.board.variant, "The board variant"); Then an init script could easily construct the overlay: env/init/board-overlay: global of.overlay.filepattern="${global.of.overlay.filepattern} ${global.board.variant}-overlay.dtbo" Since every overlay file is added to the same variable, some care needs to be taken to ensure different initscripts work together. Thus the append in the above script. _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] arm: rockchip-rk3568-evb: apply dt overlay for evb7 2021-06-29 21:58 ` Trent Piepho @ 2021-06-30 9:18 ` Michael Riesch 0 siblings, 0 replies; 8+ messages in thread From: Michael Riesch @ 2021-06-30 9:18 UTC (permalink / raw) To: Trent Piepho; +Cc: Barebox List Hello Trent, Thanks for your comments, much appreciated! On 6/29/21 11:58 PM, Trent Piepho wrote: > On Tue, Jun 29, 2021 at 5:32 AM Michael Riesch > <michael.riesch@wolfvision.net> wrote: >> @@ -63,6 +64,9 @@ static int rk3568_evb_detect_version(void) >> evb_hw_id = 6; >> } else { >> evb_hw_id = 7; >> + setenv("global.of.overlay.dir", "boot"); > > Hardcoding the location of boot files in the rootfs into board code > doesn't seem right to me. > > Seems like either creating a nv/global.of.overlay.dir file in the > default env for the board or creating an init script that sets that > variable would be the way to set this. Right, this approach seems to be more flexible. >> + setenv("global.of.overlay.filepattern", >> + "rk3568-evb7-overlay.dtb*"); > > What if one had a board magicvar, something like: > > globalvar_add_simple("board.variant", "rk3568-evb7"); // Or just evb7 > if rk3568 is already somewhere else > BAREBOX_MAGICVAR(global.board.variant, "The board variant"); > > Then an init script could easily construct the overlay: > > env/init/board-overlay: > global of.overlay.filepattern="${global.of.overlay.filepattern} > ${global.board.variant}-overlay.dtbo" > > Since every overlay file is added to the same variable, some care > needs to be taken to ensure different initscripts work together. Thus > the append in the above script. Thanks for the hint, I think I'll go along this direction. Regards, Michael _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-07-03 20:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-29 12:31 [RFC PATCH 0/2] Proof of concept device tree overlays Michael Riesch 2021-06-29 12:31 ` [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection Michael Riesch 2021-06-30 10:17 ` Ahmad Fatoum 2021-07-01 7:19 ` Michael Riesch 2021-07-03 20:49 ` Sascha Hauer 2021-06-29 12:31 ` [RFC PATCH 2/2] arm: rockchip-rk3568-evb: apply dt overlay for evb7 Michael Riesch 2021-06-29 21:58 ` Trent Piepho 2021-06-30 9:18 ` Michael Riesch
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox