[PATCH RFC] sunxi: pinephone: detect existed magnetometer and fixup dtb

Sun Feb 11 23:38:36 CET 2024

Hi Andre,

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 at 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:

1. integrated magnetometer is part of the device and making it
extension with separate dtbo sounds a bit strange.

2. 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?

3. 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.

4. 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.

5. When distribution supplies fdt with extlinux, when supplied fdt is
used and no extensions are applied to it. This is my case as well.

6. 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.

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.

> 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.

> 
> > +	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.

1. upstream AF8133J driver to the Linux kernel (on-going)
2. find acceptable solution for u-boot to handle different
   magnetometers (on-going in this thread) 
3. upstream necessary dts changes to the Linux kernel
4. 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
> 

1. https://github.com/AndreySV/u-boot/commit/84a9f1c14d1850c5559c5d888c814eee8886b04f

-- 
Best regards,
Andrey Skvortsov