[1/3] ARM: dts: sun5i: chip: Enable bluetooth

  The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
up on UART3. Support for this didn't exist in mainline when the DTS was
initially added, but it does now, so enable it.

Signed-off-by: Jonathan McDowell <noodles@earth.li>
---
 arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
 1 file changed, 4 insertions(+)
  

Hi Jonathan,

kernel test robot noticed the following build errors:

[auto build test ERROR on sunxi/sunxi/for-next]
[also build test ERROR on robh/for-next krzk/for-next krzk-dt/for-next krzk-mem-ctrl/for-next linus/master v6.3-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jonathan-McDowell/ARM-dts-sun5i-chip-Enable-bluetooth/20230416-014856
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
patch link:    https://lore.kernel.org/r/f26d11e613df7bd55822ff3fb7689e36bf9e4f7a.1681580558.git.noodles%40earth.li
patch subject: [PATCH 1/3] ARM: dts: sun5i: chip: Enable bluetooth
config: arm-defconfig (https://download.01.org/0day-ci/archive/20230416/202304160423.TaGIRb5X-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/7eb91ccc0e287519d5df2e97e81c3bee553e3535
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jonathan-McDowell/ARM-dts-sun5i-chip-Enable-bluetooth/20230416-014856
        git checkout 7eb91ccc0e287519d5df2e97e81c3bee553e3535
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304160423.TaGIRb5X-lkp@intel.com/

All errors (new ones prefixed by >>):

>> Error: arch/arm/boot/dts/sun5i-r8-chip.dts:262.1-2 syntax error
   FATAL ERROR: Unable to parse input tree
  

On Sat, 15 Apr 2023 18:46:03 +0100
Jonathan McDowell <noodles@earth.li> wrote:

> The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
> up on UART3. Support for this didn't exist in mainline when the DTS was
> initially added, but it does now, so enable it.
> 
> Signed-off-by: Jonathan McDowell <noodles@earth.li>
> ---
>  arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
> index fd37bd1f3920..4d72a181d8aa 100644
> --- a/arch/arm/boot/dts/sun5i-r8-chip.dts
> +++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
> @@ -255,6 +255,10 @@ &uart3 {
>  	pinctrl-0 = <&uart3_pg_pins>,
>  		    <&uart3_cts_rts_pg_pins>;
>  	status = "okay";
> +
> +	bluetooth {
> +		compatible = "realtek,rtl8723bs-bt";
> +	}

As the kernel test robot already pointed out, there is a semicolon
missing here.
Otherwise looks good (dt-validate passes), but don't know if there are
any wakeup GPIOs connected (can't seem to find a schematic?).

Cheers,
Andre


>  };
>  
>  &usb_otg {
  

On Sun, Apr 16, 2023 at 01:24:21AM +0100, Andre Przywara wrote:
> On Sat, 15 Apr 2023 18:46:03 +0100
> Jonathan McDowell <noodles@earth.li> wrote:
> 
> > The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
> > up on UART3. Support for this didn't exist in mainline when the DTS was
> > initially added, but it does now, so enable it.
> > 
> > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > ---
> >  arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > index fd37bd1f3920..4d72a181d8aa 100644
> > --- a/arch/arm/boot/dts/sun5i-r8-chip.dts
> > +++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > @@ -255,6 +255,10 @@ &uart3 {
> >  	pinctrl-0 = <&uart3_pg_pins>,
> >  		    <&uart3_cts_rts_pg_pins>;
> >  	status = "okay";
> > +
> > +	bluetooth {
> > +		compatible = "realtek,rtl8723bs-bt";
> > +	}
> 
> As the kernel test robot already pointed out, there is a semicolon
> missing here.
> Otherwise looks good (dt-validate passes), but don't know if there are
> any wakeup GPIOs connected (can't seem to find a schematic?).

So there are wakeups, but if I add:

	device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
	host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */

then some odd sort of dependency issue happens where the serial port
load is deferred waiting for the GPIO to appear, and then the device
doesn't work. Error in dmesg is:

serial serial0-0: deferred probe pending

on 6.3-rc and on 6.1 I get:

dw-apb-uart 1c28c00.serial: Failed to create device link (0x180) with axp20x-gpio

I'm not clear why it's trying to link the serial port to the GPIO; it
seems that it should be the bluetooth device that depends on both the
UART and the GPIO, and that the GPIO is actually optional so shouldn't
hold up loading, but I can't see how that should be represented.

Adding Greg + Saravana in the hope they can tell me what I've done wrong
here. The LED driver using a different GPIO line on the axp209 works
fine, so the device is definitely loaded and working ok.

> >  };
> >  
> >  &usb_otg {
> 

J.
  

On Thu, Apr 20, 2023 at 12:12 PM Jonathan McDowell <noodles@earth.li> wrote:
>
> On Sun, Apr 16, 2023 at 01:24:21AM +0100, Andre Przywara wrote:
> > On Sat, 15 Apr 2023 18:46:03 +0100
> > Jonathan McDowell <noodles@earth.li> wrote:
> >
> > > The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
> > > up on UART3. Support for this didn't exist in mainline when the DTS was
> > > initially added, but it does now, so enable it.
> > >
> > > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > > ---
> > >  arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > index fd37bd1f3920..4d72a181d8aa 100644
> > > --- a/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > +++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > @@ -255,6 +255,10 @@ &uart3 {
> > >     pinctrl-0 = <&uart3_pg_pins>,
> > >                 <&uart3_cts_rts_pg_pins>;
> > >     status = "okay";
> > > +
> > > +   bluetooth {
> > > +           compatible = "realtek,rtl8723bs-bt";
> > > +   }
> >
> > As the kernel test robot already pointed out, there is a semicolon
> > missing here.
> > Otherwise looks good (dt-validate passes), but don't know if there are
> > any wakeup GPIOs connected (can't seem to find a schematic?).
>
> So there are wakeups, but if I add:
>
>         device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
>         host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
>
> then some odd sort of dependency issue happens where the serial port
> load is deferred waiting for the GPIO to appear, and then the device
> doesn't work.

When you say your device doesn't work, are you saying it never probes?
<debugfs>/devices_deferred should tell you what devices have deferred and why.

> Error in dmesg is:
>
> serial serial0-0: deferred probe pending
>
> on 6.3-rc and on 6.1 I get:
>
> dw-apb-uart 1c28c00.serial: Failed to create device link (0x180) with axp20x-gpio

This error message doesn't block anything. So I don't think this is
the cause of your blocking issue. But I still want to understand why
this error message is showing up.

> I'm not clear why it's trying to link the serial port to the GPIO; it
> seems that it should be the bluetooth device that depends on both the
> UART and the GPIO,

A fix for the device link error message went in on v6.3-rc3. Is that
the 6.3 version you tested this on?

Also, I tried looking into the UART driver
(drivers/tty/serial/8250/8250_dw.c) but it wasn't clear how it ends up
populating the bluetooth serial device. If you can point that out,
that'd be helpful (assuming 6.3-rc3 still shows that error message).

> and that the GPIO is actually optional so shouldn't
> hold up loading, but I can't see how that should be represented.

Optional dependencies should get ignored after the default
deferred_probe_timeout runs out and the supplier driver hasn't been
loaded yet.

-Saravana

> Adding Greg + Saravana in the hope they can tell me what I've done wrong
> here. The LED driver using a different GPIO line on the axp209 works
> fine, so the device is definitely loaded and working ok.
  

On Thu, Apr 20, 2023 at 06:43:06PM -0700, Saravana Kannan wrote:
> On Thu, Apr 20, 2023 at 12:12 PM Jonathan McDowell <noodles@earth.li> wrote:
> > On Sun, Apr 16, 2023 at 01:24:21AM +0100, Andre Przywara wrote:
> > > On Sat, 15 Apr 2023 18:46:03 +0100
> > > Jonathan McDowell <noodles@earth.li> wrote:
> > >
> > > > The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
> > > > up on UART3. Support for this didn't exist in mainline when the DTS was
> > > > initially added, but it does now, so enable it.
> > > >
> > > > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > > > ---
> > > >  arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > index fd37bd1f3920..4d72a181d8aa 100644
> > > > --- a/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > +++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > @@ -255,6 +255,10 @@ &uart3 {
> > > >     pinctrl-0 = <&uart3_pg_pins>,
> > > >                 <&uart3_cts_rts_pg_pins>;
> > > >     status = "okay";
> > > > +
> > > > +   bluetooth {
> > > > +           compatible = "realtek,rtl8723bs-bt";
> > > > +   }
> > >
> > > As the kernel test robot already pointed out, there is a semicolon
> > > missing here.
> > > Otherwise looks good (dt-validate passes), but don't know if there are
> > > any wakeup GPIOs connected (can't seem to find a schematic?).
> >
> > So there are wakeups, but if I add:
> >
> >         device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
> >         host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> >
> > then some odd sort of dependency issue happens where the serial port
> > load is deferred waiting for the GPIO to appear, and then the device
> > doesn't work.
> 
> When you say your device doesn't work, are you saying it never probes?

The bluetooth device (realtek,rtl8723bs-bt) never appears, apparently
because the UART it's attached to never loads - it doesn't even try to
load the firmware.

> <debugfs>/devices_deferred should tell you what devices have deferred and why.

root@chip:~# cat /sys/kernel/debug/devices_deferred
serial0-0

> > Error in dmesg is:
> >
> > serial serial0-0: deferred probe pending
> >
> > on 6.3-rc and on 6.1 I get:
> >
> > dw-apb-uart 1c28c00.serial: Failed to create device link (0x180) with axp20x-gpio
> 
> This error message doesn't block anything. So I don't think this is
> the cause of your blocking issue. But I still want to understand why
> this error message is showing up.
> 
> > I'm not clear why it's trying to link the serial port to the GPIO; it
> > seems that it should be the bluetooth device that depends on both the
> > UART and the GPIO,
> 
> A fix for the device link error message went in on v6.3-rc3. Is that
> the 6.3 version you tested this on?

I originally tried on 6.1.21, which is where I got the "Failed to create
device link" message. I then moved to 6.3-rc7 as I saw there had been
further changes recently. There I just get the:

serial serial0-0: deferred probe pending

message.

> Also, I tried looking into the UART driver
> (drivers/tty/serial/8250/8250_dw.c) but it wasn't clear how it ends up
> populating the bluetooth serial device. If you can point that out,
> that'd be helpful (assuming 6.3-rc3 still shows that error message).

I have the following in my device tree:

&uart3 {
        pinctrl-names = "default";
        pinctrl-0 = <&uart3_pg_pins>,
                    <&uart3_cts_rts_pg_pins>;
        status = "okay";

        bluetooth {
                compatible = "realtek,rtl8723bs-bt";
                device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
                host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
        };
};

uart3 is a snps,dw-apb-uart, defined in arch/arm/boot/dts/sun5i.dtsi

The UART and AXP209 device drivers are compiled into the kernel:

CONFIG_PINCTRL_AXP209=y
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_DW=y

The bluetooth bits are modules (btrtl, hci_uart).

If I remove the device-wake-gpios line then the Bluetooth device works
fine, and /sys/kernel/debug/devices_deferred is empty.

Somehow it seems like the GPIO is being parsed as a dependency for the
serial port, even though the serial port + GPIO are both dependencies
for the bluetooth device. Even with that, given both are built-in I
don't understand why the serial port never completes setup.

> > and that the GPIO is actually optional so shouldn't
> > hold up loading, but I can't see how that should be represented.
> 
> Optional dependencies should get ignored after the default
> deferred_probe_timeout runs out and the supplier driver hasn't been
> loaded yet.

When I say it's optional I mean if it's not listed everything works
fine, but I don't believe there's anyway to express that in the DTS.
It's certainly not required for the serial port, just the bluetooth
device.

J.
  

On Fri, Apr 21, 2023 at 1:28 AM Jonathan McDowell <noodles@earth.li> wrote:
>
> On Thu, Apr 20, 2023 at 06:43:06PM -0700, Saravana Kannan wrote:
> > On Thu, Apr 20, 2023 at 12:12 PM Jonathan McDowell <noodles@earth.li> wrote:
> > > On Sun, Apr 16, 2023 at 01:24:21AM +0100, Andre Przywara wrote:
> > > > On Sat, 15 Apr 2023 18:46:03 +0100
> > > > Jonathan McDowell <noodles@earth.li> wrote:
> > > >
> > > > > The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
> > > > > up on UART3. Support for this didn't exist in mainline when the DTS was
> > > > > initially added, but it does now, so enable it.
> > > > >
> > > > > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > > > > ---
> > > > >  arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > index fd37bd1f3920..4d72a181d8aa 100644
> > > > > --- a/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > +++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > @@ -255,6 +255,10 @@ &uart3 {
> > > > >     pinctrl-0 = <&uart3_pg_pins>,
> > > > >                 <&uart3_cts_rts_pg_pins>;
> > > > >     status = "okay";
> > > > > +
> > > > > +   bluetooth {
> > > > > +           compatible = "realtek,rtl8723bs-bt";
> > > > > +   }
> > > >
> > > > As the kernel test robot already pointed out, there is a semicolon
> > > > missing here.
> > > > Otherwise looks good (dt-validate passes), but don't know if there are
> > > > any wakeup GPIOs connected (can't seem to find a schematic?).
> > >
> > > So there are wakeups, but if I add:
> > >
> > >         device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
> > >         host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> > >
> > > then some odd sort of dependency issue happens where the serial port
> > > load is deferred waiting for the GPIO to appear, and then the device
> > > doesn't work.
> >
> > When you say your device doesn't work, are you saying it never probes?

Read your whole email and it's a strange issue. Also, going forward to
avoid confusion, only reply to questions with respect to 6.3-rc7.

> The bluetooth device (realtek,rtl8723bs-bt) never appears, apparently
> because the UART it's attached to never loads - it doesn't even try to
> load the firmware.
>
> > <debugfs>/devices_deferred should tell you what devices have deferred and why.
>
> root@chip:~# cat /sys/kernel/debug/devices_deferred
> serial0-0

Do you see this in 6.3-rc7 too?

> > > Error in dmesg is:
> > >
> > > serial serial0-0: deferred probe pending
> > >
> > > on 6.3-rc and on 6.1 I get:
> > >
> > > dw-apb-uart 1c28c00.serial: Failed to create device link (0x180) with axp20x-gpio
> >
> > This error message doesn't block anything. So I don't think this is
> > the cause of your blocking issue. But I still want to understand why
> > this error message is showing up.
> >
> > > I'm not clear why it's trying to link the serial port to the GPIO; it
> > > seems that it should be the bluetooth device that depends on both the
> > > UART and the GPIO,
> >
> > A fix for the device link error message went in on v6.3-rc3. Is that
> > the 6.3 version you tested this on?
>
> I originally tried on 6.1.21, which is where I got the "Failed to create
> device link" message. I then moved to 6.3-rc7 as I saw there had been
> further changes recently. There I just get the:
>
> serial serial0-0: deferred probe pending

If the deferral is related to fw_devlink, you should see the reason
for deferring in the devices_deferred file. So I don't think the issue
is related to fw_devlink.

> message.
>
> > Also, I tried looking into the UART driver
> > (drivers/tty/serial/8250/8250_dw.c) but it wasn't clear how it ends up
> > populating the bluetooth serial device. If you can point that out,
> > that'd be helpful (assuming 6.3-rc3 still shows that error message).
>
> I have the following in my device tree:
>
> &uart3 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&uart3_pg_pins>,
>                     <&uart3_cts_rts_pg_pins>;
>         status = "okay";
>
>         bluetooth {
>                 compatible = "realtek,rtl8723bs-bt";
>                 device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
>                 host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
>         };
> };
>
> uart3 is a snps,dw-apb-uart, defined in arch/arm/boot/dts/sun5i.dtsi
>
> The UART and AXP209 device drivers are compiled into the kernel:
>
> CONFIG_PINCTRL_AXP209=y
> CONFIG_SERIAL_8250=y
> CONFIG_SERIAL_8250_DW=y
>
> The bluetooth bits are modules (btrtl, hci_uart).
>
> If I remove the device-wake-gpios line then the Bluetooth device works
> fine, and /sys/kernel/debug/devices_deferred is empty.
>
> Somehow it seems like the GPIO is being parsed as a dependency for the
> serial port, even though the serial port + GPIO are both dependencies
> for the bluetooth device.

I'm fairly sure that fw_devlink isn't causing that. Because even
without bluetooth, fw_devlink doesn't consider any suppliers listed in
child DT nodes as mandatory suppliers. That has been the case since
the beginning.

> Even with that, given both are built-in I
> don't understand why the serial port never completes setup.

My guess is that the driver itself has some bug that's sensitive to
device probe order even though it shouldn't.

Can you add #define DEBUG 1 to the top of drivers/base/core.c and
share the boot log? I can try and help debug it.

-Saravana

> > > and that the GPIO is actually optional so shouldn't
> > > hold up loading, but I can't see how that should be represented.
> >
> > Optional dependencies should get ignored after the default
> > deferred_probe_timeout runs out and the supplier driver hasn't been
> > loaded yet.
>
> When I say it's optional I mean if it's not listed everything works
> fine, but I don't believe there's anyway to express that in the DTS.
> It's certainly not required for the serial port, just the bluetooth
> device.
>
> J.
>
> --
> Web [                     Don't be a stranger.                     ]
> site: https:// [                                          ]      Made by
> www.earth.li/~noodles/  [                      ]         HuggieTag 0.0.24
  

On Fri, Apr 21, 2023 at 03:45:52PM -0700, Saravana Kannan wrote:
> On Fri, Apr 21, 2023 at 1:28 AM Jonathan McDowell <noodles@earth.li> wrote:
> >
> > On Thu, Apr 20, 2023 at 06:43:06PM -0700, Saravana Kannan wrote:
> > > On Thu, Apr 20, 2023 at 12:12 PM Jonathan McDowell <noodles@earth.li> wrote:
> > > > On Sun, Apr 16, 2023 at 01:24:21AM +0100, Andre Przywara wrote:
> > > > > On Sat, 15 Apr 2023 18:46:03 +0100
> > > > > Jonathan McDowell <noodles@earth.li> wrote:
> > > > >
> > > > > > The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
> > > > > > up on UART3. Support for this didn't exist in mainline when the DTS was
> > > > > > initially added, but it does now, so enable it.
> > > > > >
> > > > > > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > > > > > ---
> > > > > >  arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > index fd37bd1f3920..4d72a181d8aa 100644
> > > > > > --- a/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > +++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > @@ -255,6 +255,10 @@ &uart3 {
> > > > > >     pinctrl-0 = <&uart3_pg_pins>,
> > > > > >                 <&uart3_cts_rts_pg_pins>;
> > > > > >     status = "okay";
> > > > > > +
> > > > > > +   bluetooth {
> > > > > > +           compatible = "realtek,rtl8723bs-bt";
> > > > > > +   }
> > > > >
> > > > > As the kernel test robot already pointed out, there is a semicolon
> > > > > missing here.
> > > > > Otherwise looks good (dt-validate passes), but don't know if there are
> > > > > any wakeup GPIOs connected (can't seem to find a schematic?).
> > > >
> > > > So there are wakeups, but if I add:
> > > >
> > > >         device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
> > > >         host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> > > >
> > > > then some odd sort of dependency issue happens where the serial port
> > > > load is deferred waiting for the GPIO to appear, and then the device
> > > > doesn't work.
> > >
> > > When you say your device doesn't work, are you saying it never probes?
> 
> Read your whole email and it's a strange issue. Also, going forward to
> avoid confusion, only reply to questions with respect to 6.3-rc7.

Just to be clear, in my initial mail I referred to 6.1.21 as that's
where I started, but in my reply to you all output was quoted from
6.3-rc7. 6.3 has been released since, so all details below are based on
that.

> > The bluetooth device (realtek,rtl8723bs-bt) never appears, apparently
> > because the UART it's attached to never loads - it doesn't even try to
> > load the firmware.
> >
> > > <debugfs>/devices_deferred should tell you what devices have deferred and why.
> >
> > root@chip:~# cat /sys/kernel/debug/devices_deferred
> > serial0-0
> 
> Do you see this in 6.3-rc7 too?

That was under 6.3-rc7. I see it on 6.3 too:

root@chip:~# cat /sys/kernel/debug/devices_deferred
serial0-0

Without the device-wake-gpios line in the device tree it's empty.

> > > > Error in dmesg is:
> > > >
> > > > serial serial0-0: deferred probe pending
> > > >
> > > > on 6.3-rc and on 6.1 I get:
> > > >
> > > > dw-apb-uart 1c28c00.serial: Failed to create device link (0x180) with axp20x-gpio
> > >
> > > This error message doesn't block anything. So I don't think this is
> > > the cause of your blocking issue. But I still want to understand why
> > > this error message is showing up.
> > >
> > > > I'm not clear why it's trying to link the serial port to the GPIO; it
> > > > seems that it should be the bluetooth device that depends on both the
> > > > UART and the GPIO,
> > >
> > > A fix for the device link error message went in on v6.3-rc3. Is that
> > > the 6.3 version you tested this on?
> >
> > I originally tried on 6.1.21, which is where I got the "Failed to create
> > device link" message. I then moved to 6.3-rc7 as I saw there had been
> > further changes recently. There I just get the:
> >
> > serial serial0-0: deferred probe pending
> 
> If the deferral is related to fw_devlink, you should see the reason
> for deferring in the devices_deferred file. So I don't think the issue
> is related to fw_devlink.

Ok.

> > message.
> >
> > > Also, I tried looking into the UART driver
> > > (drivers/tty/serial/8250/8250_dw.c) but it wasn't clear how it ends up
> > > populating the bluetooth serial device. If you can point that out,
> > > that'd be helpful (assuming 6.3-rc3 still shows that error message).
> >
> > I have the following in my device tree:
> >
> > &uart3 {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&uart3_pg_pins>,
> >                     <&uart3_cts_rts_pg_pins>;
> >         status = "okay";
> >
> >         bluetooth {
> >                 compatible = "realtek,rtl8723bs-bt";
> >                 device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
> >                 host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> >         };
> > };
> >
> > uart3 is a snps,dw-apb-uart, defined in arch/arm/boot/dts/sun5i.dtsi
> >
> > The UART and AXP209 device drivers are compiled into the kernel:
> >
> > CONFIG_PINCTRL_AXP209=y
> > CONFIG_SERIAL_8250=y
> > CONFIG_SERIAL_8250_DW=y
> >
> > The bluetooth bits are modules (btrtl, hci_uart).
> >
> > If I remove the device-wake-gpios line then the Bluetooth device works
> > fine, and /sys/kernel/debug/devices_deferred is empty.
> >
> > Somehow it seems like the GPIO is being parsed as a dependency for the
> > serial port, even though the serial port + GPIO are both dependencies
> > for the bluetooth device.
> 
> I'm fairly sure that fw_devlink isn't causing that. Because even
> without bluetooth, fw_devlink doesn't consider any suppliers listed in
> child DT nodes as mandatory suppliers. That has been the case since
> the beginning.

Ok, I just got confused with the 6.1 message about the device link and
thought that might be the rough area.

> > Even with that, given both are built-in I
> > don't understand why the serial port never completes setup.
> 
> My guess is that the driver itself has some bug that's sensitive to
> device probe order even though it shouldn't.
> 
> Can you add #define DEBUG 1 to the top of drivers/base/core.c and
> share the boot log? I can try and help debug it.

Thanks for any input you're able to provide. dmesg from 6.3 with a
failure to complete the probe:

https://the.earth.li/~noodles/chip-bluetooth/6.3-not-working

and just for completeness, without the device-wake-gpios line:

https://the.earth.li/~noodles/chip-bluetooth/6.3-working

J.
  

On Mon, Apr 24, 2023 at 10:34 AM Jonathan McDowell <noodles@earth.li> wrote:
>
> On Fri, Apr 21, 2023 at 03:45:52PM -0700, Saravana Kannan wrote:
> > On Fri, Apr 21, 2023 at 1:28 AM Jonathan McDowell <noodles@earth.li> wrote:
> > >
> > > On Thu, Apr 20, 2023 at 06:43:06PM -0700, Saravana Kannan wrote:
> > > > On Thu, Apr 20, 2023 at 12:12 PM Jonathan McDowell <noodles@earth.li> wrote:
> > > > > On Sun, Apr 16, 2023 at 01:24:21AM +0100, Andre Przywara wrote:
> > > > > > On Sat, 15 Apr 2023 18:46:03 +0100
> > > > > > Jonathan McDowell <noodles@earth.li> wrote:
> > > > > >
> > > > > > > The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
> > > > > > > up on UART3. Support for this didn't exist in mainline when the DTS was
> > > > > > > initially added, but it does now, so enable it.
> > > > > > >
> > > > > > > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > > > > > > ---
> > > > > > >  arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
> > > > > > >  1 file changed, 4 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > > index fd37bd1f3920..4d72a181d8aa 100644
> > > > > > > --- a/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > > +++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > > @@ -255,6 +255,10 @@ &uart3 {
> > > > > > >     pinctrl-0 = <&uart3_pg_pins>,
> > > > > > >                 <&uart3_cts_rts_pg_pins>;
> > > > > > >     status = "okay";
> > > > > > > +
> > > > > > > +   bluetooth {
> > > > > > > +           compatible = "realtek,rtl8723bs-bt";
> > > > > > > +   }
> > > > > >
> > > > > > As the kernel test robot already pointed out, there is a semicolon
> > > > > > missing here.
> > > > > > Otherwise looks good (dt-validate passes), but don't know if there are
> > > > > > any wakeup GPIOs connected (can't seem to find a schematic?).
> > > > >
> > > > > So there are wakeups, but if I add:
> > > > >
> > > > >         device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
> > > > >         host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> > > > >
> > > > > then some odd sort of dependency issue happens where the serial port
> > > > > load is deferred waiting for the GPIO to appear, and then the device
> > > > > doesn't work.
> > > >
> > > > When you say your device doesn't work, are you saying it never probes?
> >
> > Read your whole email and it's a strange issue. Also, going forward to
> > avoid confusion, only reply to questions with respect to 6.3-rc7.

Sorry it took a while to respond. Life got busy.

> Just to be clear, in my initial mail I referred to 6.1.21 as that's
> where I started, but in my reply to you all output was quoted from
> 6.3-rc7. 6.3 has been released since, so all details below are based on
> that.
>
> > > The bluetooth device (realtek,rtl8723bs-bt) never appears, apparently
> > > because the UART it's attached to never loads - it doesn't even try to
> > > load the firmware.
> > >
> > > > <debugfs>/devices_deferred should tell you what devices have deferred and why.
> > >
> > > root@chip:~# cat /sys/kernel/debug/devices_deferred
> > > serial0-0
> >
> > Do you see this in 6.3-rc7 too?
>
> That was under 6.3-rc7. I see it on 6.3 too:
>
> root@chip:~# cat /sys/kernel/debug/devices_deferred
> serial0-0

I somehow didn't connect the dots earlier... but serial0-0 is NOT the
uart/serial device. It's the child device of serial0 and in this case,
it's the bluetooth device.

So adding those gpios in DT is not breaking serial. It's just
preventing the BT device from probing.

Looking at the logs in the non-working case:

[    0.715083] 1c28c00.serial: ttyS1 at MMIO 0x1c28c00 (irq = 53,
base_baud = 1500000) is a U6_16550A
[    0.724132] device: 'serial0': device_add

I don't know why all of the ttySx are showing up as serial0, but this
is the serial port. As you can see 1c28c00 is already probing.

[    0.724228] device: 'serial0-0': device_add

This is the child devices getting populated. In this case this is the BT device.

[    0.724311] device: 'platform:1c20800.pinctrl--serial:serial0-0': device_add

I can tell it's the BT device because we see a device link being
created between pinctrl and serial:serial0-0. So it's a device sitting
on the serial bus.

[    0.724378] devices_kset: Moving serial0-0 to end of list
[    0.724390] serial serial0-0: Linked as a consumer to 1c20800.pinctrl
[    0.724401] /soc/serial@1c28c00/bluetooth Dropping the fwnode link
to /soc/pinctrl@1c20800

And the fwnode like that was converted to device link clearly shows
that the serial0-0 corresponds to the bluetooth node.

[    0.724441] serial serial0: tty port ttyS1 registered

Serial port works.

>
> Without the device-wake-gpios line in the device tree it's empty.

I think the issue is at the BT driver level or some other framework.

Add a print at the start of the BT driver to see if the probe() is
actually getting called. I'm guessing it is and it's returning an
error from within.

If you don't see that print, then debug the really_probe() function to
see how far within it the BT device goes through before it errors out.
It's possible pinctrl_bind_pins() in really_probe() fails for the BT
device because of how the GPIO pins are configured in your DT.

Thanks,
Saravana

>
> > > > > Error in dmesg is:
> > > > >
> > > > > serial serial0-0: deferred probe pending
> > > > >
> > > > > on 6.3-rc and on 6.1 I get:
> > > > >
> > > > > dw-apb-uart 1c28c00.serial: Failed to create device link (0x180) with axp20x-gpio
> > > >
> > > > This error message doesn't block anything. So I don't think this is
> > > > the cause of your blocking issue. But I still want to understand why
> > > > this error message is showing up.
> > > >
> > > > > I'm not clear why it's trying to link the serial port to the GPIO; it
> > > > > seems that it should be the bluetooth device that depends on both the
> > > > > UART and the GPIO,
> > > >
> > > > A fix for the device link error message went in on v6.3-rc3. Is that
> > > > the 6.3 version you tested this on?
> > >
> > > I originally tried on 6.1.21, which is where I got the "Failed to create
> > > device link" message. I then moved to 6.3-rc7 as I saw there had been
> > > further changes recently. There I just get the:
> > >
> > > serial serial0-0: deferred probe pending
> >
> > If the deferral is related to fw_devlink, you should see the reason
> > for deferring in the devices_deferred file. So I don't think the issue
> > is related to fw_devlink.
>
> Ok.
>
> > > message.
> > >
> > > > Also, I tried looking into the UART driver
> > > > (drivers/tty/serial/8250/8250_dw.c) but it wasn't clear how it ends up
> > > > populating the bluetooth serial device. If you can point that out,
> > > > that'd be helpful (assuming 6.3-rc3 still shows that error message).
> > >
> > > I have the following in my device tree:
> > >
> > > &uart3 {
> > >         pinctrl-names = "default";
> > >         pinctrl-0 = <&uart3_pg_pins>,
> > >                     <&uart3_cts_rts_pg_pins>;
> > >         status = "okay";
> > >
> > >         bluetooth {
> > >                 compatible = "realtek,rtl8723bs-bt";
> > >                 device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
> > >                 host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> > >         };
> > > };
> > >
> > > uart3 is a snps,dw-apb-uart, defined in arch/arm/boot/dts/sun5i.dtsi
> > >
> > > The UART and AXP209 device drivers are compiled into the kernel:
> > >
> > > CONFIG_PINCTRL_AXP209=y
> > > CONFIG_SERIAL_8250=y
> > > CONFIG_SERIAL_8250_DW=y
> > >
> > > The bluetooth bits are modules (btrtl, hci_uart).
> > >
> > > If I remove the device-wake-gpios line then the Bluetooth device works
> > > fine, and /sys/kernel/debug/devices_deferred is empty.
> > >
> > > Somehow it seems like the GPIO is being parsed as a dependency for the
> > > serial port, even though the serial port + GPIO are both dependencies
> > > for the bluetooth device.
> >
> > I'm fairly sure that fw_devlink isn't causing that. Because even
> > without bluetooth, fw_devlink doesn't consider any suppliers listed in
> > child DT nodes as mandatory suppliers. That has been the case since
> > the beginning.
>
> Ok, I just got confused with the 6.1 message about the device link and
> thought that might be the rough area.
>
> > > Even with that, given both are built-in I
> > > don't understand why the serial port never completes setup.
> >
> > My guess is that the driver itself has some bug that's sensitive to
> > device probe order even though it shouldn't.
> >
> > Can you add #define DEBUG 1 to the top of drivers/base/core.c and
> > share the boot log? I can try and help debug it.
>
> Thanks for any input you're able to provide. dmesg from 6.3 with a
> failure to complete the probe:
>
> https://the.earth.li/~noodles/chip-bluetooth/6.3-not-working
>
> and just for completeness, without the device-wake-gpios line:
>
> https://the.earth.li/~noodles/chip-bluetooth/6.3-working
>
> J.
>
> --
> Most people are descended from apes. Redheads are descended from cats.
  

On Mon, May 01, 2023 at 02:12:05PM -0700, Saravana Kannan wrote:
> On Mon, Apr 24, 2023 at 10:34 AM Jonathan McDowell <noodles@earth.li> wrote:
> >
> > On Fri, Apr 21, 2023 at 03:45:52PM -0700, Saravana Kannan wrote:
> > > On Fri, Apr 21, 2023 at 1:28 AM Jonathan McDowell <noodles@earth.li> wrote:
> > > >
> > > > On Thu, Apr 20, 2023 at 06:43:06PM -0700, Saravana Kannan wrote:
> > > > > On Thu, Apr 20, 2023 at 12:12 PM Jonathan McDowell <noodles@earth.li> wrote:
> > > > > > On Sun, Apr 16, 2023 at 01:24:21AM +0100, Andre Przywara wrote:
> > > > > > > On Sat, 15 Apr 2023 18:46:03 +0100
> > > > > > > Jonathan McDowell <noodles@earth.li> wrote:
> > > > > > >
> > > > > > > > The C.H.I.P has an rtl8723bs device with the bluetooth interface hooked
> > > > > > > > up on UART3. Support for this didn't exist in mainline when the DTS was
> > > > > > > > initially added, but it does now, so enable it.
> > > > > > > >
> > > > > > > > Signed-off-by: Jonathan McDowell <noodles@earth.li>
> > > > > > > > ---
> > > > > > > >  arch/arm/boot/dts/sun5i-r8-chip.dts | 4 ++++
> > > > > > > >  1 file changed, 4 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/arch/arm/boot/dts/sun5i-r8-chip.dts b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > > > index fd37bd1f3920..4d72a181d8aa 100644
> > > > > > > > --- a/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > > > +++ b/arch/arm/boot/dts/sun5i-r8-chip.dts
> > > > > > > > @@ -255,6 +255,10 @@ &uart3 {
> > > > > > > >     pinctrl-0 = <&uart3_pg_pins>,
> > > > > > > >                 <&uart3_cts_rts_pg_pins>;
> > > > > > > >     status = "okay";
> > > > > > > > +
> > > > > > > > +   bluetooth {
> > > > > > > > +           compatible = "realtek,rtl8723bs-bt";
> > > > > > > > +   }
> > > > > > >
> > > > > > > As the kernel test robot already pointed out, there is a semicolon
> > > > > > > missing here.
> > > > > > > Otherwise looks good (dt-validate passes), but don't know if there are
> > > > > > > any wakeup GPIOs connected (can't seem to find a schematic?).
> > > > > >
> > > > > > So there are wakeups, but if I add:
> > > > > >
> > > > > >         device-wake-gpios = <&axp_gpio 3 GPIO_ACTIVE_LOW>;
> > > > > >         host-wake-gpios = <&pio 1 3 GPIO_ACTIVE_HIGH>; /* PB3 */
> > > > > >
> > > > > > then some odd sort of dependency issue happens where the serial port
> > > > > > load is deferred waiting for the GPIO to appear, and then the device
> > > > > > doesn't work.
> > > > >
> > > > > When you say your device doesn't work, are you saying it never probes?
> > >
> > > Read your whole email and it's a strange issue. Also, going forward to
> > > avoid confusion, only reply to questions with respect to 6.3-rc7.
> 
> Sorry it took a while to respond. Life got busy.

No problem, I appreciate you looking into this.

> > Just to be clear, in my initial mail I referred to 6.1.21 as that's
> > where I started, but in my reply to you all output was quoted from
> > 6.3-rc7. 6.3 has been released since, so all details below are based on
> > that.
> >
> > > > The bluetooth device (realtek,rtl8723bs-bt) never appears, apparently
> > > > because the UART it's attached to never loads - it doesn't even try to
> > > > load the firmware.
> > > >
> > > > > <debugfs>/devices_deferred should tell you what devices have deferred and why.
> > > >
> > > > root@chip:~# cat /sys/kernel/debug/devices_deferred
> > > > serial0-0
> > >
> > > Do you see this in 6.3-rc7 too?
> >
> > That was under 6.3-rc7. I see it on 6.3 too:
> >
> > root@chip:~# cat /sys/kernel/debug/devices_deferred
> > serial0-0
> 
> I somehow didn't connect the dots earlier... but serial0-0 is NOT the
> uart/serial device. It's the child device of serial0 and in this case,
> it's the bluetooth device.
> 
> So adding those gpios in DT is not breaking serial. It's just
> preventing the BT device from probing.

Aaaaah.

> Looking at the logs in the non-working case:
> 
> [    0.715083] 1c28c00.serial: ttyS1 at MMIO 0x1c28c00 (irq = 53,
> base_baud = 1500000) is a U6_16550A
> [    0.724132] device: 'serial0': device_add
> 
> I don't know why all of the ttySx are showing up as serial0, but this
> is the serial port. As you can see 1c28c00 is already probing.
> 
> [    0.724228] device: 'serial0-0': device_add
> 
> This is the child devices getting populated. In this case this is the BT device.
> 
> [    0.724311] device: 'platform:1c20800.pinctrl--serial:serial0-0': device_add
> 
> I can tell it's the BT device because we see a device link being
> created between pinctrl and serial:serial0-0. So it's a device sitting
> on the serial bus.
> 
> [    0.724378] devices_kset: Moving serial0-0 to end of list
> [    0.724390] serial serial0-0: Linked as a consumer to 1c20800.pinctrl
> [    0.724401] /soc/serial@1c28c00/bluetooth Dropping the fwnode link
> to /soc/pinctrl@1c20800
> 
> And the fwnode like that was converted to device link clearly shows
> that the serial0-0 corresponds to the bluetooth node.
> 
> [    0.724441] serial serial0: tty port ttyS1 registered
> 
> Serial port works.
> 
> >
> > Without the device-wake-gpios line in the device tree it's empty.
> 
> I think the issue is at the BT driver level or some other framework.
> 
> Add a print at the start of the BT driver to see if the probe() is
> actually getting called. I'm guessing it is and it's returning an
> error from within.
> 
> If you don't see that print, then debug the really_probe() function to
> see how far within it the BT device goes through before it errors out.
> It's possible pinctrl_bind_pins() in really_probe() fails for the BT
> device because of how the GPIO pins are configured in your DT.

Thank you! That was exactly the pointer I needed to go and find the
actual issue, which is a real facepalm moment. The AXP209 driver does
not have support for GPIO3 (which is in a different register and needs a
bunch of special casing). So the Bluetooth driver tries to request it
and gets failures, so can't complete the probe.

I've added support to the pinctrl-axp209 driver for GPIO3 and everything
is working just fine now. I'll get that cleaned up and a v2 patch set
submitted. Thanks for all your help.

J.