[PATCH v3] console: usb: kbd: Limit poll frequency to improve performance

[Filip Žaludek]

Hi Simon,


On 4/19/23 03:49, Simon Glass wrote:
> Hi Filip,
> 
> On Tue, 11 Apr 2023 at 14:24, Filip Žaludek <filip.zaludek at oracle.com> wrote:
>>
>>
>>
>> On 2/8/23 20:01, Mark Kettenis wrote:
>>>> Date: Wed, 8 Feb 2023 19:45:36 +0100
>>>> From: Michal Suchánek <msuchanek at suse.de>
>>>>
>>>> Hello,
>>>>
>>>> On Wed, Jan 18, 2023 at 05:01:12PM +0100, Filip Žaludek wrote:
>>>>>
>>>>>
>>>>> Hi Michal,
>>>>>
>>>>>    thanks for testing! Do you consider keyboard as working once it is detected without
>>>>> 'usb_kbd usb_kbd: Timeout poll on interrupt endpoint', or judging from subsequent
>>>>> typing? Note that issue is reproducible only in about 20% of reboots.
>>>>
>>>> I rely on keyboard input to boot so if it was 20% broken I would notice.
>>>> I don't use the rPi all that much so if it was broken only a few
>>>> % of the time there is a chance I would miss it.
>>>>
>>>> However, for me not typing on the keyboard during usb detection it is
>>>> 100% not detected, typing on it during usb detection it is 100%
>>>> detected.
>>>>
>>>> The timeout is limitation of the dwc2 controller handling of usb hubs.
>>>>
>>>> There might be a possibility to improve the driver so that it handles
>>>> the condition but it might be that the Linux driver relies on a separate
>>>> thread handling the controller which is not acceptable for u-boot.
>>>>
>>>> I am not usb expert and definitely not dwc2 expert so I cannot do more
>>>> than workaround the current driver limitation.
>>>>
>>>>> For me I can always enter 'U-Boot>' shell, but then keyboard usually does not work.
>>>>> And yes, resetting the usb controller with pressing a key afterwards will
>>>>> finally break the keyboard. ('usb reset' typed from keyboard)
>>>>> If you are Prague located I am ready to demonstrate what I am talking about.
>>>>>
>>>>>    Simon's keyboard detection is somewhat interfered by 'SanDisk USB Extreme Pro' detection,
>>>>> printed complaints but keyboard still works..
>>>>> 'usb_kbd usb_kbd: Timeout poll on interrupt endpoint' and 'Failed to get keyboard state from device 0c40:8000'
>>>>> Btw. why from 0c40:8000 (ELMCU 2.4GHz receiver) when wired keyboard is 046d:c31c (Logitech Keyboard K120)?
>>>>>
>>>>>    What is supposed scenario for RPi3/u-boot/grub usb keyboard equipped users wanting to boot non-default?
>>>>> Enter 'U-Boot>' shell to detect keyboard; type boot; select desired grub entry..?
>>>>>
>>>>> Reverting either from the two makes it non issue for me:
>>>>> 'dwc2: use the nonblock argument in submit_int_msg'
>>>>> commit 9dcab2c4d2cb50ab1864c818b82a72393c160236
>>>>
>>>> Without this booting from USB is not feasible because reading every
>>>> block from the USB drive waits for the keyboard to time out.
>>>>
>>>>> 'console: usb: kbd: Limit poll frequency to improve performance'
>>>>> commit 96991e652f541323a03c5b7e075d54a117091618
>>>>
>>>> No idea about this one, for me it doea not give any substantial
>>>> difference in behavior.
>>>
>>> Reverting that commit leads to a significant slowdown loading a kernel
>>> from disk with a usb keyboard connected.  The slowdown is somewhat
>>> hardware dependent but on some systems loading the OpenBSD/arm64
>>> kernel would take minutes instead of seconds.
>>
>>
>> Hello,
>>     I am about to dig more into this issue with proper tools, but failed to
>> configure/compile trace functionality on RPi3 due to missing references
>> to timer_early_get_count() and timer_early_get_rate().
> 
> You could implement a proper timer driver for rpi.
> 
>>
>>    Is it possible/feasible to implement calls in CONFIG_SYS_ARCH_TIMER
>> and/or CONFIG_SP804_TIMER?
> 
> Yes


  I am little bit missing here secret sauce, timer_early_get_count() and timer_early_get_rate()
are not supposed to be implemented in arch/arm/cpu/armv8/generic_timer.c? But predestined for
drivers/timer/sp804_timer.c?

TIMER is required for common/board_f.c and common/board_r.c but it disables generic_timer..
%< -----------------------------------------
ifndef CONFIG_$(SPL_TPL_)TIMER
obj-$(CONFIG_SYS_ARCH_TIMER) += generic_timer.o
endif
%< -----------------------------------------
And obviously multiple definition of get_tbclk and get_ticks when forced to compile/link.




>>
>>    I would be grateful even for trace to generate function traces without
>> timestamps. Is such nasty hack without timestamping supposed to work?
>> Basically my intention is to trace 'usb reset'.
>>
>> Appreciate any hints/outlines how to proceed.
> 
> I assume you mean CONFIG_TRACE. Yes, you could update it to support
> writing a zero timestamp. See the add_ftrace() function.
> 
> But better to add a driver if you can. It should not be difficult.
> 
> Regards,
> Simon

  I am already happily timestamp tracing with borrowed functionality from generic_timer.c,
albeit bypassing kbuild mechanism. It did not yet answered my usb polling questions,
tracing report is polluted/overfilled.



Instrumenting code thoughts:
* It would be handy to -finstrument-functions only for desired objects.
* It would be handy to have macro inverse to 'notrace' to mark only desired functions. Feasible?
* gcc -finstrument-functions-exclude-file-list still pollutes tracing buffer.

More Tracing in U-Boot thoughts:
* There is proftool options discrepancy, documented {-c, -f, -m, -o, -t, -v}, implemented {-m, -p, -t, -v}.
* Both types FUNCF_ENTRY and FUNCF_EXIT are marked as " <- " by proftool.



Thanks,
Filip