| Message ID | 20191126081602.2264519-3-a.heider@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | arm64: dts: sun50i: Add support for Orange Pi 3 | expand |
Hello Andre, On Tue, Nov 26, 2019 at 09:16:00AM +0100, Andre Heider wrote: > Some Bluetooth controllers, like the BCM4345C5 of the Orange Pi 3, > ship with the controller default address. > > Add a config option to fix it up so it can function properly. I tried it on TBS A711 tablet that also has bluetooth, but that doesn't have ethernet (ethernet alias is not set up) so no ethaddr gets generated and bdaddr setting fails due to dependency on ethernet alias being present. So for this mechanism to be more universal, sid->mac_addr code probably needs to be moved to it's own function, and used both from setup_environment and fixup_bd_address, to avoid dependency on boards having ethernet. Otherwise, on Orange Pi 3: Tested-by: Ondrej Jirman <megous@megous.com> Also see the comment below: > Signed-off-by: Andre Heider <a.heider@gmail.com> > --- > arch/arm/mach-sunxi/Kconfig | 12 ++++++++++++ > board/sunxi/board.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > index 16d41b83af..b41c64870e 100644 > --- a/arch/arm/mach-sunxi/Kconfig > +++ b/arch/arm/mach-sunxi/Kconfig > @@ -1009,4 +1009,16 @@ config PINE64_DT_SELECTION > option, the device tree selection code specific to Pine64 which > utilizes the DRAM size will be enabled. > > +config FIXUP_BDADDR > + string "Fixup the Bluetooth controller address" > + depends on MACH_SUN50I_H6 ^ This should be more sunxi generic or perhaps just removed, because the code is not specific to H6. thank you and regards, o. > + default "" > + help > + This option specifies the DT compatible name of the Bluetooth > + controller for which to set the "local-bd-address" property. > + Set this option if your device ships with the Bluetooth controller > + default address. > + The used address is "bdaddr" if set, and "ethaddr" with the LSB > + flipped elsewise. > + > endif > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > index bb35d6b66e..89851102d1 100644 > --- a/board/sunxi/board.c > +++ b/board/sunxi/board.c > @@ -856,6 +856,34 @@ int misc_init_r(void) > return 0; > } > > +static void fixup_bd_address(void *blob) > +{ > +#ifdef CONFIG_FIXUP_BDADDR > + /* Some devices ship with a Bluetooth controller default address. > + * Set a valid address through the device tree. > + */ > + uchar tmp[ETH_ALEN], bdaddr[ETH_ALEN]; > + int i; > + > + if (strlen(CONFIG_FIXUP_BDADDR) < 1) > + return; > + > + if (!eth_env_get_enetaddr("bdaddr", tmp)) { > + if (!eth_env_get_enetaddr("ethaddr", tmp)) > + return; > + > + tmp[ETH_ALEN - 1] ^= 1; > + } > + > + /* Addresses need to be in the binary format of the corresponding stack */ > + for (i = 0; i < ETH_ALEN; ++i) > + bdaddr[i] = tmp[ETH_ALEN - i - 1]; > + > + do_fixup_by_compat(blob, CONFIG_FIXUP_BDADDR, > + "local-bd-address", bdaddr, ETH_ALEN, 1); > +#endif > +} > + > int ft_board_setup(void *blob, bd_t *bd) > { > int __maybe_unused r; > @@ -866,6 +894,8 @@ int ft_board_setup(void *blob, bd_t *bd) > */ > setup_environment(blob); > > + fixup_bd_address(blob); > + > #ifdef CONFIG_VIDEO_DT_SIMPLEFB > r = sunxi_simplefb_setup(blob); > if (r) > -- > 2.24.0 >
On Tue, Nov 26, 2019 at 11:46:33AM +0100, megous hlavni wrote: > Hello Andre, > > On Tue, Nov 26, 2019 at 09:16:00AM +0100, Andre Heider wrote: > > Some Bluetooth controllers, like the BCM4345C5 of the Orange Pi 3, > > ship with the controller default address. > > > > Add a config option to fix it up so it can function properly. > > I tried it on TBS A711 tablet that also has bluetooth, but that doesn't have > ethernet (ethernet alias is not set up) so no ethaddr gets generated and > bdaddr setting fails due to dependency on ethernet alias being present. > > So for this mechanism to be more universal, sid->mac_addr code probably > needs to be moved to it's own function, and used both from setup_environment > and fixup_bd_address, to avoid dependency on boards having ethernet. Or even simpler, just move the fixup_bd_address() code to setup_environment() regards, o. > Otherwise, on Orange Pi 3: > > Tested-by: Ondrej Jirman <megous@megous.com> > > Also see the comment below: > > > Signed-off-by: Andre Heider <a.heider@gmail.com> > > --- > > arch/arm/mach-sunxi/Kconfig | 12 ++++++++++++ > > board/sunxi/board.c | 30 ++++++++++++++++++++++++++++++ > > 2 files changed, 42 insertions(+) > > > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > > index 16d41b83af..b41c64870e 100644 > > --- a/arch/arm/mach-sunxi/Kconfig > > +++ b/arch/arm/mach-sunxi/Kconfig > > @@ -1009,4 +1009,16 @@ config PINE64_DT_SELECTION > > option, the device tree selection code specific to Pine64 which > > utilizes the DRAM size will be enabled. > > > > +config FIXUP_BDADDR > > + string "Fixup the Bluetooth controller address" > > + depends on MACH_SUN50I_H6 > > ^ This should be more sunxi generic or perhaps just removed, because the code is > not specific to H6. > > thank you and regards, > o. > > > + default "" > > + help > > + This option specifies the DT compatible name of the Bluetooth > > + controller for which to set the "local-bd-address" property. > > + Set this option if your device ships with the Bluetooth controller > > + default address. > > + The used address is "bdaddr" if set, and "ethaddr" with the LSB > > + flipped elsewise. > > + > > endif > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > > index bb35d6b66e..89851102d1 100644 > > --- a/board/sunxi/board.c > > +++ b/board/sunxi/board.c > > @@ -856,6 +856,34 @@ int misc_init_r(void) > > return 0; > > } > > > > +static void fixup_bd_address(void *blob) > > +{ > > +#ifdef CONFIG_FIXUP_BDADDR > > + /* Some devices ship with a Bluetooth controller default address. > > + * Set a valid address through the device tree. > > + */ > > + uchar tmp[ETH_ALEN], bdaddr[ETH_ALEN]; > > + int i; > > + > > + if (strlen(CONFIG_FIXUP_BDADDR) < 1) > > + return; > > + > > + if (!eth_env_get_enetaddr("bdaddr", tmp)) { > > + if (!eth_env_get_enetaddr("ethaddr", tmp)) > > + return; > > + > > + tmp[ETH_ALEN - 1] ^= 1; > > + } > > + > > + /* Addresses need to be in the binary format of the corresponding stack */ > > + for (i = 0; i < ETH_ALEN; ++i) > > + bdaddr[i] = tmp[ETH_ALEN - i - 1]; > > + > > + do_fixup_by_compat(blob, CONFIG_FIXUP_BDADDR, > > + "local-bd-address", bdaddr, ETH_ALEN, 1); > > +#endif > > +} > > + > > int ft_board_setup(void *blob, bd_t *bd) > > { > > int __maybe_unused r; > > @@ -866,6 +894,8 @@ int ft_board_setup(void *blob, bd_t *bd) > > */ > > setup_environment(blob); > > > > + fixup_bd_address(blob); > > + > > #ifdef CONFIG_VIDEO_DT_SIMPLEFB > > r = sunxi_simplefb_setup(blob); > > if (r) > > -- > > 2.24.0 > >
On 26/11/2019 11:46, Ondřej Jirman wrote: > Hello Andre, > > On Tue, Nov 26, 2019 at 09:16:00AM +0100, Andre Heider wrote: >> Some Bluetooth controllers, like the BCM4345C5 of the Orange Pi 3, >> ship with the controller default address. >> >> Add a config option to fix it up so it can function properly. > > I tried it on TBS A711 tablet that also has bluetooth, but that doesn't have > ethernet (ethernet alias is not set up) so no ethaddr gets generated and > bdaddr setting fails due to dependency on ethernet alias being present. > > So for this mechanism to be more universal, sid->mac_addr code probably > needs to be moved to it's own function, and used both from setup_environment > and fixup_bd_address, to avoid dependency on boards having ethernet. Okay, I extracted a helper function locally, so it doesn't rely on "ethaddr". That should work on your tablet then I think. I'll wait a bit for further feedback before sending out v3. > > Otherwise, on Orange Pi 3: > > Tested-by: Ondrej Jirman <megous@megous.com> Nice, thanks! > > Also see the comment below: > >> Signed-off-by: Andre Heider <a.heider@gmail.com> >> --- >> arch/arm/mach-sunxi/Kconfig | 12 ++++++++++++ >> board/sunxi/board.c | 30 ++++++++++++++++++++++++++++++ >> 2 files changed, 42 insertions(+) >> >> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig >> index 16d41b83af..b41c64870e 100644 >> --- a/arch/arm/mach-sunxi/Kconfig >> +++ b/arch/arm/mach-sunxi/Kconfig >> @@ -1009,4 +1009,16 @@ config PINE64_DT_SELECTION >> option, the device tree selection code specific to Pine64 which >> utilizes the DRAM size will be enabled. >> >> +config FIXUP_BDADDR >> + string "Fixup the Bluetooth controller address" >> + depends on MACH_SUN50I_H6 > > ^ This should be more sunxi generic or perhaps just removed, because the code is > not specific to H6. I didn't see a way to exclude fixup_bd_address() from compilation via #ifdefs when FIXUP_BDADDR is empty. And I didn't want to use 2 knobs (one to enable, one for the string). Currently the function body has "if (strlen(CONFIG_FIXUP_BDADDR) < 1) return", and I *think* the compiler should be smart enough to just drop the whole thing if the string is empty. So the "depends on" is just a safety net to protect against code bloat for archs where it's currently unused anyway. But I can drop it if there are no concerns. Thanks, Andre > > thank you and regards, > o. > >> + default "" >> + help >> + This option specifies the DT compatible name of the Bluetooth >> + controller for which to set the "local-bd-address" property. >> + Set this option if your device ships with the Bluetooth controller >> + default address. >> + The used address is "bdaddr" if set, and "ethaddr" with the LSB >> + flipped elsewise. >> + >> endif >> diff --git a/board/sunxi/board.c b/board/sunxi/board.c >> index bb35d6b66e..89851102d1 100644 >> --- a/board/sunxi/board.c >> +++ b/board/sunxi/board.c >> @@ -856,6 +856,34 @@ int misc_init_r(void) >> return 0; >> } >> >> +static void fixup_bd_address(void *blob) >> +{ >> +#ifdef CONFIG_FIXUP_BDADDR >> + /* Some devices ship with a Bluetooth controller default address. >> + * Set a valid address through the device tree. >> + */ >> + uchar tmp[ETH_ALEN], bdaddr[ETH_ALEN]; >> + int i; >> + >> + if (strlen(CONFIG_FIXUP_BDADDR) < 1) >> + return; >> + >> + if (!eth_env_get_enetaddr("bdaddr", tmp)) { >> + if (!eth_env_get_enetaddr("ethaddr", tmp)) >> + return; >> + >> + tmp[ETH_ALEN - 1] ^= 1; >> + } >> + >> + /* Addresses need to be in the binary format of the corresponding stack */ >> + for (i = 0; i < ETH_ALEN; ++i) >> + bdaddr[i] = tmp[ETH_ALEN - i - 1]; >> + >> + do_fixup_by_compat(blob, CONFIG_FIXUP_BDADDR, >> + "local-bd-address", bdaddr, ETH_ALEN, 1); >> +#endif >> +} >> + >> int ft_board_setup(void *blob, bd_t *bd) >> { >> int __maybe_unused r; >> @@ -866,6 +894,8 @@ int ft_board_setup(void *blob, bd_t *bd) >> */ >> setup_environment(blob); >> >> + fixup_bd_address(blob); >> + >> #ifdef CONFIG_VIDEO_DT_SIMPLEFB >> r = sunxi_simplefb_setup(blob); >> if (r) >> -- >> 2.24.0 >>
On Tue, Nov 26, 2019 at 01:15:42PM +0100, Andre Heider wrote: > On 26/11/2019 11:46, Ondřej Jirman wrote: > > Hello Andre, > > > > On Tue, Nov 26, 2019 at 09:16:00AM +0100, Andre Heider wrote: > > > Some Bluetooth controllers, like the BCM4345C5 of the Orange Pi 3, > > > ship with the controller default address. > > > > > > Add a config option to fix it up so it can function properly. > > > > I tried it on TBS A711 tablet that also has bluetooth, but that doesn't have > > ethernet (ethernet alias is not set up) so no ethaddr gets generated and > > bdaddr setting fails due to dependency on ethernet alias being present. > > > > So for this mechanism to be more universal, sid->mac_addr code probably > > needs to be moved to it's own function, and used both from setup_environment > > and fixup_bd_address, to avoid dependency on boards having ethernet. > > Okay, I extracted a helper function locally, so it doesn't rely on > "ethaddr". That should work on your tablet then I think. I'll wait a bit for > further feedback before sending out v3. > > > > > Otherwise, on Orange Pi 3: > > > > Tested-by: Ondrej Jirman <megous@megous.com> > > Nice, thanks! > > > > > Also see the comment below: > > > > > Signed-off-by: Andre Heider <a.heider@gmail.com> > > > --- > > > arch/arm/mach-sunxi/Kconfig | 12 ++++++++++++ > > > board/sunxi/board.c | 30 ++++++++++++++++++++++++++++++ > > > 2 files changed, 42 insertions(+) > > > > > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > > > index 16d41b83af..b41c64870e 100644 > > > --- a/arch/arm/mach-sunxi/Kconfig > > > +++ b/arch/arm/mach-sunxi/Kconfig > > > @@ -1009,4 +1009,16 @@ config PINE64_DT_SELECTION > > > option, the device tree selection code specific to Pine64 which > > > utilizes the DRAM size will be enabled. > > > +config FIXUP_BDADDR > > > + string "Fixup the Bluetooth controller address" > > > + depends on MACH_SUN50I_H6 > > > > ^ This should be more sunxi generic or perhaps just removed, because the code is > > not specific to H6. > > I didn't see a way to exclude fixup_bd_address() from compilation via > #ifdefs when FIXUP_BDADDR is empty. And I didn't want to use 2 knobs (one to > enable, one for the string). > > Currently the function body has "if (strlen(CONFIG_FIXUP_BDADDR) < 1) > return", and I *think* the compiler should be smart enough to just drop the > whole thing if the string is empty. So the "depends on" is just a safety net > to protect against code bloat for archs where it's currently unused anyway. > > But I can drop it if there are no concerns. I understand, but this is useful for many more sunxi SoCs, so I think the small increase in main uboot size for sunxi boards is justified. Anyway, I'd think using `if (CONFIG_FIXUP_BDADDR[0])` would be a safer bet for compiler dropping the if body, rather than strlen. regards, o. > Thanks, > Andre > > > > > thank you and regards, > > o. > > > > > + default "" > > > + help > > > + This option specifies the DT compatible name of the Bluetooth > > > + controller for which to set the "local-bd-address" property. > > > + Set this option if your device ships with the Bluetooth controller > > > + default address. > > > + The used address is "bdaddr" if set, and "ethaddr" with the LSB > > > + flipped elsewise. > > > + > > > endif > > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > > > index bb35d6b66e..89851102d1 100644 > > > --- a/board/sunxi/board.c > > > +++ b/board/sunxi/board.c > > > @@ -856,6 +856,34 @@ int misc_init_r(void) > > > return 0; > > > } > > > +static void fixup_bd_address(void *blob) > > > +{ > > > +#ifdef CONFIG_FIXUP_BDADDR > > > + /* Some devices ship with a Bluetooth controller default address. > > > + * Set a valid address through the device tree. > > > + */ > > > + uchar tmp[ETH_ALEN], bdaddr[ETH_ALEN]; > > > + int i; > > > + > > > + if (strlen(CONFIG_FIXUP_BDADDR) < 1) > > > + return; > > > + > > > + if (!eth_env_get_enetaddr("bdaddr", tmp)) { > > > + if (!eth_env_get_enetaddr("ethaddr", tmp)) > > > + return; > > > + > > > + tmp[ETH_ALEN - 1] ^= 1; > > > + } > > > + > > > + /* Addresses need to be in the binary format of the corresponding stack */ > > > + for (i = 0; i < ETH_ALEN; ++i) > > > + bdaddr[i] = tmp[ETH_ALEN - i - 1]; > > > + > > > + do_fixup_by_compat(blob, CONFIG_FIXUP_BDADDR, > > > + "local-bd-address", bdaddr, ETH_ALEN, 1); > > > +#endif > > > +} > > > + > > > int ft_board_setup(void *blob, bd_t *bd) > > > { > > > int __maybe_unused r; > > > @@ -866,6 +894,8 @@ int ft_board_setup(void *blob, bd_t *bd) > > > */ > > > setup_environment(blob); > > > + fixup_bd_address(blob); > > > + > > > #ifdef CONFIG_VIDEO_DT_SIMPLEFB > > > r = sunxi_simplefb_setup(blob); > > > if (r) > > > -- > > > 2.24.0 > > > >
On 26/11/2019 13:40, Ondřej Jirman wrote: > On Tue, Nov 26, 2019 at 01:15:42PM +0100, Andre Heider wrote: >> On 26/11/2019 11:46, Ondřej Jirman wrote: >>> Hello Andre, >>> >>> On Tue, Nov 26, 2019 at 09:16:00AM +0100, Andre Heider wrote: >>>> Some Bluetooth controllers, like the BCM4345C5 of the Orange Pi 3, >>>> ship with the controller default address. >>>> >>>> Add a config option to fix it up so it can function properly. >>> >>> I tried it on TBS A711 tablet that also has bluetooth, but that doesn't have >>> ethernet (ethernet alias is not set up) so no ethaddr gets generated and >>> bdaddr setting fails due to dependency on ethernet alias being present. >>> >>> So for this mechanism to be more universal, sid->mac_addr code probably >>> needs to be moved to it's own function, and used both from setup_environment >>> and fixup_bd_address, to avoid dependency on boards having ethernet. >> >> Okay, I extracted a helper function locally, so it doesn't rely on >> "ethaddr". That should work on your tablet then I think. I'll wait a bit for >> further feedback before sending out v3. >> >>> >>> Otherwise, on Orange Pi 3: >>> >>> Tested-by: Ondrej Jirman <megous@megous.com> >> >> Nice, thanks! >> >>> >>> Also see the comment below: >>> >>>> Signed-off-by: Andre Heider <a.heider@gmail.com> >>>> --- >>>> arch/arm/mach-sunxi/Kconfig | 12 ++++++++++++ >>>> board/sunxi/board.c | 30 ++++++++++++++++++++++++++++++ >>>> 2 files changed, 42 insertions(+) >>>> >>>> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig >>>> index 16d41b83af..b41c64870e 100644 >>>> --- a/arch/arm/mach-sunxi/Kconfig >>>> +++ b/arch/arm/mach-sunxi/Kconfig >>>> @@ -1009,4 +1009,16 @@ config PINE64_DT_SELECTION >>>> option, the device tree selection code specific to Pine64 which >>>> utilizes the DRAM size will be enabled. >>>> +config FIXUP_BDADDR >>>> + string "Fixup the Bluetooth controller address" >>>> + depends on MACH_SUN50I_H6 >>> >>> ^ This should be more sunxi generic or perhaps just removed, because the code is >>> not specific to H6. >> >> I didn't see a way to exclude fixup_bd_address() from compilation via >> #ifdefs when FIXUP_BDADDR is empty. And I didn't want to use 2 knobs (one to >> enable, one for the string). >> >> Currently the function body has "if (strlen(CONFIG_FIXUP_BDADDR) < 1) >> return", and I *think* the compiler should be smart enough to just drop the >> whole thing if the string is empty. So the "depends on" is just a safety net >> to protect against code bloat for archs where it's currently unused anyway. >> >> But I can drop it if there are no concerns. > > I understand, but this is useful for many more sunxi SoCs, so I think the > small increase in main uboot size for sunxi boards is justified. Okay let me remove it then, but I'll go with what the maintainers deem acceptable. > Anyway, I'd think using `if (CONFIG_FIXUP_BDADDR[0])` would be a safer bet > for compiler dropping the if body, rather than strlen. Yeah, that might just work, thanks! Andre
diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig index 16d41b83af..b41c64870e 100644 --- a/arch/arm/mach-sunxi/Kconfig +++ b/arch/arm/mach-sunxi/Kconfig @@ -1009,4 +1009,16 @@ config PINE64_DT_SELECTION option, the device tree selection code specific to Pine64 which utilizes the DRAM size will be enabled. +config FIXUP_BDADDR + string "Fixup the Bluetooth controller address" + depends on MACH_SUN50I_H6 + default "" + help + This option specifies the DT compatible name of the Bluetooth + controller for which to set the "local-bd-address" property. + Set this option if your device ships with the Bluetooth controller + default address. + The used address is "bdaddr" if set, and "ethaddr" with the LSB + flipped elsewise. + endif diff --git a/board/sunxi/board.c b/board/sunxi/board.c index bb35d6b66e..89851102d1 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -856,6 +856,34 @@ int misc_init_r(void) return 0; } +static void fixup_bd_address(void *blob) +{ +#ifdef CONFIG_FIXUP_BDADDR + /* Some devices ship with a Bluetooth controller default address. + * Set a valid address through the device tree. + */ + uchar tmp[ETH_ALEN], bdaddr[ETH_ALEN]; + int i; + + if (strlen(CONFIG_FIXUP_BDADDR) < 1) + return; + + if (!eth_env_get_enetaddr("bdaddr", tmp)) { + if (!eth_env_get_enetaddr("ethaddr", tmp)) + return; + + tmp[ETH_ALEN - 1] ^= 1; + } + + /* Addresses need to be in the binary format of the corresponding stack */ + for (i = 0; i < ETH_ALEN; ++i) + bdaddr[i] = tmp[ETH_ALEN - i - 1]; + + do_fixup_by_compat(blob, CONFIG_FIXUP_BDADDR, + "local-bd-address", bdaddr, ETH_ALEN, 1); +#endif +} + int ft_board_setup(void *blob, bd_t *bd) { int __maybe_unused r; @@ -866,6 +894,8 @@ int ft_board_setup(void *blob, bd_t *bd) */ setup_environment(blob); + fixup_bd_address(blob); + #ifdef CONFIG_VIDEO_DT_SIMPLEFB r = sunxi_simplefb_setup(blob); if (r)
Some Bluetooth controllers, like the BCM4345C5 of the Orange Pi 3, ship with the controller default address. Add a config option to fix it up so it can function properly. Signed-off-by: Andre Heider <a.heider@gmail.com> --- arch/arm/mach-sunxi/Kconfig | 12 ++++++++++++ board/sunxi/board.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+)