2021-07-23 11:44:49

by Archie Pusaka

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag

Hi Marcel,

On Thu, 22 Jul 2021 at 22:32, Marcel Holtmann <[email protected]> wrote:
>
> Hi Archie,
>
> > Some RTL chips resets the FW on suspend, so wakeup is disabled on
> > those chips. This patch introduces this WAKEUP_DISABLE flag so that
> > chips that doesn't reset FW on suspend can leave the flag unset and
> > is allowed to wake the host.
> >
> > This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
> > allowing it to wake the host, and preventing reprobing on resume.
> >
> > Signed-off-by: Archie Pusaka <[email protected]>
> > Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
> > Reviewed-by: Hilda Wu <[email protected]>
> >
> > ---
> >
> > Changes in v2:
> > * Remove unnecessary variable
> >
> > drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
> > 1 file changed, 59 insertions(+), 24 deletions(-)
>
> so the set does not apply cleanly to bluetooth-next
>
> Applying: Bluetooth: hci_h5: Add runtime suspend
> error: patch failed: drivers/bluetooth/hci_h5.c:11
> error: drivers/bluetooth/hci_h5.c: patch does not apply

Hmm, it applies cleanly for me. Not sure what's going on.
Anyway I rebased and made a little change as v3, please take a look!

>
>
> And I am really close to not accepting any patches for hci_h5.c anymore. This thing turns into crazy hacking and nobody is taking my hint to redo this as clean H:5 3-Wire serdev standalone driver.

Pardon my unfamiliarity, but could you share more about your vision of
a clean h5 driver? Should the RTL component be moved out to btrtl?
Do we have something as a reference?

>
> Regards
>
> Marcel
>

Thanks,
Archie


2021-07-23 12:18:50

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag

Hi Archie,

>>> Some RTL chips resets the FW on suspend, so wakeup is disabled on
>>> those chips. This patch introduces this WAKEUP_DISABLE flag so that
>>> chips that doesn't reset FW on suspend can leave the flag unset and
>>> is allowed to wake the host.
>>>
>>> This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
>>> allowing it to wake the host, and preventing reprobing on resume.
>>>
>>> Signed-off-by: Archie Pusaka <[email protected]>
>>> Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
>>> Reviewed-by: Hilda Wu <[email protected]>
>>>
>>> ---
>>>
>>> Changes in v2:
>>> * Remove unnecessary variable
>>>
>>> drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
>>> 1 file changed, 59 insertions(+), 24 deletions(-)
>>
>> so the set does not apply cleanly to bluetooth-next
>>
>> Applying: Bluetooth: hci_h5: Add runtime suspend
>> error: patch failed: drivers/bluetooth/hci_h5.c:11
>> error: drivers/bluetooth/hci_h5.c: patch does not apply
>
> Hmm, it applies cleanly for me. Not sure what's going on.
> Anyway I rebased and made a little change as v3, please take a look!

the v3 applied cleanly.

>>
>>
>> And I am really close to not accepting any patches for hci_h5.c anymore. This thing turns into crazy hacking and nobody is taking my hint to redo this as clean H:5 3-Wire serdev standalone driver.
>
> Pardon my unfamiliarity, but could you share more about your vision of
> a clean h5 driver? Should the RTL component be moved out to btrtl?
> Do we have something as a reference?

so a while back I send a bt3wire.c sample driver around. That would be a good starting point.

Anyhow, the problem is that hci_uart.ko is inherent a line discipline driver from 2.4.x kernel days and it has been stacked and hacked on top of it. It has become a burden, especially in the light that you can have clean serdev based drivers now (like btmtkuart.c).

And yes, it would be following the 3-Wire H:5 spec and then deal with vendor specific details like btusb.c for example. And my hope would be that especially in the Realtek and Broadcom (RPi3 etc.) cases this can move into vendor specific blocks and shared between USB and UART transports.

I also send around a btuart.c sample driver that is solely serdev based and should replace all the cases where we have H:4 as transport.

Regards

Marcel

2021-07-26 04:04:40

by Archie Pusaka

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] Bluetooth: hci_h5: add WAKEUP_DISABLE flag

Hi Marcel,

On Fri, 23 Jul 2021 at 20:17, Marcel Holtmann <[email protected]> wrote:
>
> Hi Archie,
>
> >>> Some RTL chips resets the FW on suspend, so wakeup is disabled on
> >>> those chips. This patch introduces this WAKEUP_DISABLE flag so that
> >>> chips that doesn't reset FW on suspend can leave the flag unset and
> >>> is allowed to wake the host.
> >>>
> >>> This patch also left RTL8822 WAKEUP_DISABLE flag unset, therefore
> >>> allowing it to wake the host, and preventing reprobing on resume.
> >>>
> >>> Signed-off-by: Archie Pusaka <[email protected]>
> >>> Reviewed-by: Abhishek Pandit-Subedi <[email protected]>
> >>> Reviewed-by: Hilda Wu <[email protected]>
> >>>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> * Remove unnecessary variable
> >>>
> >>> drivers/bluetooth/hci_h5.c | 83 +++++++++++++++++++++++++++-----------
> >>> 1 file changed, 59 insertions(+), 24 deletions(-)
> >>
> >> so the set does not apply cleanly to bluetooth-next
> >>
> >> Applying: Bluetooth: hci_h5: Add runtime suspend
> >> error: patch failed: drivers/bluetooth/hci_h5.c:11
> >> error: drivers/bluetooth/hci_h5.c: patch does not apply
> >
> > Hmm, it applies cleanly for me. Not sure what's going on.
> > Anyway I rebased and made a little change as v3, please take a look!
>
> the v3 applied cleanly.
>
> >>
> >>
> >> And I am really close to not accepting any patches for hci_h5.c anymore. This thing turns into crazy hacking and nobody is taking my hint to redo this as clean H:5 3-Wire serdev standalone driver.
> >
> > Pardon my unfamiliarity, but could you share more about your vision of
> > a clean h5 driver? Should the RTL component be moved out to btrtl?
> > Do we have something as a reference?
>
> so a while back I send a bt3wire.c sample driver around. That would be a good starting point.
>
> Anyhow, the problem is that hci_uart.ko is inherent a line discipline driver from 2.4.x kernel days and it has been stacked and hacked on top of it. It has become a burden, especially in the light that you can have clean serdev based drivers now (like btmtkuart.c).
>
> And yes, it would be following the 3-Wire H:5 spec and then deal with vendor specific details like btusb.c for example. And my hope would be that especially in the Realtek and Broadcom (RPi3 etc.) cases this can move into vendor specific blocks and shared between USB and UART transports.
>
> I also send around a btuart.c sample driver that is solely serdev based and should replace all the cases where we have H:4 as transport.
>

Thanks for the pointers!

The files you mentioned are rather hard to find, so below I paste the
URL where I found them in case anyone else is also interested.

[RFC v2] Bluetooth: Add new serdev based driver for UART attached controllers
https://www.spinics.net/lists/linux-bluetooth/msg74918.html

[RFC] Bluetooth: Add new serdev based driver for 3-Wire attached controllers
https://www.spinics.net/lists/linux-bluetooth/msg74839.html

Thanks,
Archie