Message-ID: <CAGETcx8JDYUT2bdDhJ0PN8_CPmHJ37jCfnuoav6CGFJoovfSqA@mail.gmail.com>
Date:   Fri, 21 Apr 2023 15:45:52 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Jonathan McDowell <noodles@...th.li>
Cc:     Andre Przywara <andre.przywara@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] ARM: dts: sun5i: chip: Enable bluetooth

On Fri, Apr 21, 2023 at 1:28 AM Jonathan McDowell <noodles@...th.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@...th.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@...th.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@...th.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@...p:~# 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

Powered by blists - more mailing lists