On Mon, 12 Feb 2024 01:38:36 +0300 Andrey Skvortsov andrej.skvortzov@gmail.com wrote:
Hi Andrey,
thank you for the valuable feedback!
On 24-02-11 13:13, Andre Przywara wrote:
On Sun, 11 Feb 2024 12:28:24 +0300 Andrey Skvortsov andrej.skvortzov@gmail.com wrote:
Hi Andrey,
thanks for taking care of this upstream!
In newer 1.2 PinePhone board revisions LIS3MDL magnetometer was replaced by AF8133J. They use the same PB1 pin in different modes.
AF8133J's driver isn't upstreamed yet, but it will be soon. Therefore this is RFC patch. I'd like to know whether selected approach will be accepted in u-boot before submiting coresponding dts changes to the Linux kernel. Any feedback on this change would be very welcome.
So I think this is the right approach: Since the SPL has no use of this device, and it's a rather small DT change, we definitely want to do this in U-Boot proper. And I believe that U-Boot (proper) is indeed the best place to do those adjustments, since it's built for one board anyway, so anything board specific belongs into there (and not into TF-A or the SPL, which are more tailored to one *SoC*). Also we are not so size sensitive in proper. And I also like the fact that it's protected by a board specific definition, and even better that it's an already existing one.
Oh, and I am not really convinced this is the right approach here, but maybe have a look at doc/usage/cmd/extension.rst, for another related mechanism.
Thanks for the tip. I've looked at it after your suggestion and tried to implement the same logic using extension command. Here are my thoughts about that:
- integrated magnetometer is part of the device and making it
extension with separate dtbo sounds a bit strange.
- if I'd create dtbo for new AF8133J magnetometer, then I'd call it
like other dts files for the device: 'sun50i-a64-pinephone-1.2-af8133j.dtbo. This is 38 characters and currently overlay name is limited to 32 bytes. What do you think about increasing overlay name?
- extensions are not supported for extlinux boot flow at all. This is Debian
case, that I'm working with. And this is a major problem for me.
- I've looked how EFI boot flow is made and I see that extensions are
not applied to fdtcontroladdr, only to loaded fdt to fdt_addr_r. Extlinux uses fdtcontroladdr always.
- When distribution supplies fdt with extlinux, when supplied fdt is
used and no extensions are applied to it. This is my case as well.
- I can't apply extensions to fdtcontroladdr. When I've tried to
apply working extension to fdtcontroladdr, then I get a crash. I have to copy fdt from fdtcontroladdr to fdt_addr_r and then apply extension to fdt_addr_r and when it works. Maybe this is something sunxi-specific.
Not really. The DTB is appended to the end of the U-Boot image, and I believe there is something important immediately after it, like the heap or the gd or something. Some years back this used to work, but not anymore, for quite a while now. So to apply overlays, do a "fdt move $fdtcontroladdr $fdt_addr_r" first, then use $fdt_addr_r. But that's just for clarification, nothing that solves the above problems.
Overall extensions seems like a nice feature for capes, extension boards for pogo pins and so on. But I'm not sure, that it's the right choice in this case.
Yeah, many thanks for the extensive research and nice summary! I was already expecting something along those lines, but wasn't sure. So thanks for saving me some time.
This confirms that we should go the route you already sketched.
Some smaller comments below ...
board/sunxi/board.c | 26 ++++++++++++++++++++++++++ configs/pinephone_defconfig | 1 + 2 files changed, 27 insertions(+)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 1305302060..a4bfa24400 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -15,6 +15,7 @@ #include <dm.h> #include <env.h> #include <hang.h> +#include <i2c.h> #include <image.h> #include <init.h> #include <log.h> @@ -920,6 +921,28 @@ static void bluetooth_dt_fixup(void *blob) "local-bd-address", bdaddr, ETH_ALEN, 1); }
+#ifdef CONFIG_PINEPHONE_DT_SELECTION
Can you move that line into the function? That would for one avoid the protection in the caller below, and secondly make it easier to add other boards, if a need arises for them. The two #defines don't hurt or change the code, so just keep them outside.
+#define PINEPHONE_LIS3MDL_I2C_ADDR 0x1E +#define PINEPHONE_LIS3MDL_I2C_BUS 1 /* I2C1 */
+static void board_dt_fixup(void *blob) +{
- struct udevice *bus, *dev;
(have the #ifdef here)
Ok, then I mark local variables with __maybe_unused and remove #ifdef at board_dt_fixup call.
Ah right, there are warnings. I've got an even better idea, use: if (IS_ENABLED(CONFIG_PINEPHONE_DT_SELECTION) && !fdt_node_check_compatible(blob, 0, "pine64,pinephone-1.2")) {
Then you can keep the variables without the tag, and avoid the #ifdef altogether. The compiler optimises this away, for an unrelated board "size board.o" reports exactly the same numbers.
- if (!fdt_node_check_compatible(blob, 0, "pine64,pinephone-1.2"))
Please have curly braces around that.
I'll fix that in v2.
So I'd say please send the (disabled) DT node addition patch to the kernel MLs, then send this patch to U-Boot.
Do you mean patch with disabled AF8133J DT node? Right? If so, then it was the plan.
- upstream AF8133J driver to the Linux kernel (on-going)
- find acceptable solution for u-boot to handle different magnetometers (on-going in this thread)
- upstream necessary dts changes to the Linux kernel
I'd say we settled 2., so feel free to append the DT change to the series in 1), or send it as a follow-up patch if it's out there already. This gives us also a user in the kernel DT tree. You should add a comment to the disabled node, that this will be fixed up in firmware.
Thanks, Andre
- upstream previously discussed changes to u-boot.
Cheers, Andre
if (!uclass_get_device_by_seq(UCLASS_I2C, PINEPHONE_LIS3MDL_I2C_BUS, &bus)) {dm_i2c_probe(bus, PINEPHONE_LIS3MDL_I2C_ADDR, 0, &dev);fdt_set_status_by_compatible(blob, "st,lis3mdl-magn",dev ? FDT_STATUS_OKAY : FDT_STATUS_DISABLED);fdt_set_status_by_compatible(blob, "voltafield,af8133j",dev ? FDT_STATUS_DISABLED : FDT_STATUS_OKAY);}+} +#endif
int ft_board_setup(void *blob, struct bd_info *bd) { int __maybe_unused r; @@ -934,6 +957,9 @@ int ft_board_setup(void *blob, struct bd_info *bd)
bluetooth_dt_fixup(blob);
+#ifdef CONFIG_PINEPHONE_DT_SELECTION
- board_dt_fixup(blob);
+#endif
I'll remove #ifdef here to make code a bit cleaner. I've applied all your suggestions and make it available here [1].
#ifdef CONFIG_VIDEO_DT_SIMPLEFB r = sunxi_simplefb_setup(blob); if (r) diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig index eebc676901..457e7ee1e7 100644 --- a/configs/pinephone_defconfig +++ b/configs/pinephone_defconfig @@ -12,6 +12,7 @@ CONFIG_MMC_SUNXI_SLOT_EXTRA=2 CONFIG_PINEPHONE_DT_SELECTION=y # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2" +CONFIG_SYS_I2C_MVTWSI=y CONFIG_LED_STATUS=y CONFIG_LED_STATUS_GPIO=y CONFIG_LED_STATUS0=y