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

[Jonathan McDowell <noodles@earth.li>]

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.

-- 
101 things you can't have too much of : 49 - Bandwidth.
This .sig brought to you by the letter V and the number  8
Product of the Republic of HuggieTag