Hi,
On 3/18/24 7:52 PM, Peter Collingbourne wrote:
> On Mon, Mar 18, 2024 at 3:36 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>>
>> On Sun, Mar 17, 2024 at 10:41:23PM +0100, Hans de Goede wrote:
>>> Commit e5d6bd25f93d ("serial: 8250_dw: Do not reclock if already at
>>> correct rate") breaks the dw UARTs on Intel Bay Trail (BYT) and
>>> Cherry Trail (CHT) SoCs.
>>>
>>> Before this change the RTL8732BS Bluetooth HCI which is found
>>> connected over the dw UART on both BYT and CHT boards works properly:
>>>
>>> Bluetooth: hci0: RTL: examining hci_ver=06 hci_rev=000b lmp_ver=06 lmp_subver=8723
>>> Bluetooth: hci0: RTL: rom_version status=0 version=1
>>> Bluetooth: hci0: RTL: loading rtl_bt/rtl8723bs_fw.bin
>>> Bluetooth: hci0: RTL: loading rtl_bt/rtl8723bs_config-OBDA8723.bin
>>> Bluetooth: hci0: RTL: cfg_sz 64, total sz 24508
>>> Bluetooth: hci0: RTL: fw version 0x365d462e
>>>
>>> where as after this change probing it fails:
>>>
>>> Bluetooth: hci0: RTL: examining hci_ver=06 hci_rev=000b lmp_ver=06 lmp_subver=8723
>>> Bluetooth: hci0: RTL: rom_version status=0 version=1
>>> Bluetooth: hci0: RTL: loading rtl_bt/rtl8723bs_fw.bin
>>> Bluetooth: hci0: RTL: loading rtl_bt/rtl8723bs_config-OBDA8723.bin
>>> Bluetooth: hci0: RTL: cfg_sz 64, total sz 24508
>>> Bluetooth: hci0: command 0xfc20 tx timeout
>>> Bluetooth: hci0: RTL: download fw command failed (-110)
>>>
>>> Revert the changes to fix this regression.
>>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>>
>>> Note it is not entirely clear to me why this commit is causing
>>> this issue. Maybe probe() needs to explicitly set the clk rate
>>> which it just got (that feels like a clk driver issue) or maybe
>>> the issue is that unless setup before hand by firmware /
>>> the bootloader serial8250_update_uartclk() needs to be called
>>> at least once to setup things ? Note that probe() does not call
>>> serial8250_update_uartclk(), this is only called from the
>>> dw8250_clk_notifier_cb()
>>>
>>> This requires more debugging which is why I'm proposing
>>> a straight revert to fix the regression ASAP and then this
>>> can be investigated further.
>>
>> Yep. When I reviewed the original submission I was got puzzled with
>> the CLK APIs. Now I might remember that ->set_rate() can't be called
>> on prepared/enabled clocks and it's possible the same limitation
>> is applied to ->round_rate().
>>
>> I also tried to find documentation about the requirements for those
>> APIs, but failed (maybe was not pursuing enough, dunno). If you happen
>> to know the one, can you point on it?
>
> To me it seems to be unlikely to be related to round_rate(). It seems
> more likely that my patch causes us to never actually set the clock
> rate (e.g. because uartclk was initialized to the intended clock rate
> instead of the current actual clock rate).
I agree that the likely cause is that we never set the clk-rate. I'm not
sure if the issue is us never actually calling clk_set_rate() or if
the issue is that by never calling clk_set_rate() dw8250_clk_notifier_cb()
never gets called and thus we never call serial8250_update_uartclk()
> It should be possible to
> confirm by checking the behavior with my patch with `&& p->uartclk !=
> rate` removed, which I would expect to unbreak Hans's scenario. If my
> hypothesis is correct, the fix might involve querying the clock with
> clk_get_rate() in the if instead of reading from uartclk.
Querying the clk with clk_get_rate() instead of reading it from
uartclk will not help as uartclk gets initialized with clk_get_rate()
in dw8250_probe(). So I believe that in my scenario clk_get_rate()
already returns the desired rate causing us to never call clk_set_rate()
at all which leaves 2 possible root causes for the regressions:
1. The clk generator has non readable registers and the returned
rate from clk_get_rate() is a default rate and the actual hw is
programmed differently, iow we need to call clk_set_rate() at
least once on this hw to ensure that the clk generator is prggrammed
properly.
2. The 8250 code is not working as it should because
serial8250_update_uartclk() has never been called.
I would be happy to test patches to try and fix this. But in the mean
time 6.8 has been released with dw_uart-s on Intel Bay Trail and
Cherry Trail SoCs completely broken, so can we please move forward
with this revert to unbreak 6.8 now ?
Regards,
Hans
>
> Peter
>
