Hi,
On 3/18/24 7:52 PM, Peter Collingbourne wrote:
On Mon, Mar 18, 2024 at 3:36 AM Andy Shevchenko andriy.shevchenko@linux.intel.com 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@linux.intel.com
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