[PATCH v3] console: usb: kbd: Limit poll frequency to improve performance
Filip Žaludek
filip.zaludek at oracle.com
Mon May 15 17:00:14 CEST 2023
Hi Simon,
On 5/3/23 03:28, Simon Glass wrote:
> Hi Filip,
>
> On Tue, 2 May 2023 at 12:43, Filip Žaludek <filip.zaludek at oracle.com> wrote:
>>
>>
>>
>> Hi Simon, Michal, Marek,
>>
>>
>>
>> On 4/26/23 03:04, Simon Glass wrote:
>>> Hi Filip,
>>>
>>> On Tue, 25 Apr 2023 at 06:36, Filip Žaludek <filip.zaludek at oracle.com> wrote:
>>>>
>>>>
>>>>
>>>> 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.
>>
>>
>>
>> More updates to usb keyboard/RPi3/dwc2 controller issue:
>>
>> I was following my former observation about printing characters from semi
>> random places [usb.c, usb_hub.c, device.c, usb-uclass.c, dwc2.c] what
>> works as workaround. I realized this is only when printing to vidconsole,
>> not to serial. After disabling video_sync() and/or flush_dcache_range()
>> from corresponding vidconsole print functions, printing is no longer
>> workaround. This behavior seem to be due to cache coherency.
>>
>>
>>
>> Do you have any objections against elephant in porcelain proposal?
>> Not able to narrow it down more to single source code line.
>> With this keyboard works for me even when touching it only during 15s grub timeout.
>> It is not for sure that cache coherency problem is from dwc2, but afaik there
>> are no other complaints to usb keyboard.
>> Performance degradation not observed..
>>
>>
>> %< -------------------------------------------------------------
>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
>> index 23060fc369..f95314ff1b 100644
>> --- a/drivers/usb/host/dwc2.c
>> +++ b/drivers/usb/host/dwc2.c
>> @@ -814,6 +814,7 @@ static int dwc_otg_submit_rh_msg(struct dwc2_priv *priv, struct usb_device *dev,
>> else
>> stat = dwc_otg_submit_rh_msg_out(priv, dev, buffer, txlen, cmd);
>>
>> + flush_dcache_all();
>> mdelay(1);
>
> We have dma_map_single() and dma_unmap_single() which are designed for
> this. If you put these into the two functions that are called
> immediately above, perhaps that will solve the problem.
>
After more experiments I finally concluded issue is not due to cache coherency at all.
Workaround actually profited from side effect, time spent cleaning&invalidating dcache.
Flushing dcache works for 512M but not for 256M regardless of target address.
flush_dcache_range(random_addr, random_addr + 256M); // ~45us
flush_dcache_range(random_addr, random_addr + 512M); // ~75us
flush_dcache_all(); // ~115us
Please see..
'[PATCH] usb: kbd: dwc2: Increase wait for dwc2 controller reset by 125us'
>>
>> return stat;
>> %< -------------------------------------------------------------
>>
>>
>>
>>
>>
>>>>>>
>>>>>>
>>>>>> 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.
>>>
>>> It seems from the above that if TIMER is enabled for a particular
>>> U-Boot build phase, then generic_timer is not, and vice versa. I
>>> suppose that is fair enough.
>>
>>
>> Sorry, it was imprecisely formulated question from me. I was expecting answer
>> confirming and advocating sp804 is superior to System Timer. Implementing
>> EARLY_TIMER for System Timer is trivial, sp804 requires research from my side.
>> Skimming TF-A project sp804 seems superior.
>
> Yes, implementing that as a timer driver seems fine to me. But we
> often have multiple drivers so I suppose some people will use the
> generic one if sp804 is not available.
>
Simon, could you please outline proper approach how to implement early timer for RPi,
as all current timer_early_get_*() functions are implemented in 'drivers/timer/',
and early timer functionality still rely on dm_timer_init?
1/ Implement timer_early_get_*() in drivers/timer/sp804_timer.c once underestood superior sp804?
2a/ Implement dm version of System Timer, extending arch/arm/cpu/armv8/generic_timer.c?
2b/ Re-implement dm version of System Timer in drivers/timer/?
>>>>>>
>>>>>> 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.
>>>
>>> The compiler probably doesn't support that, or at least we don't
>>> support passing different compiler args to each file in U-Boot, other
>>> than by manually hacking the Makefiles.
>>>
>>> But I wonder if we could have a list of wildcards to match against the
>>> function names?
>>>
>>>> * 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.
>>>
>>> I wonder why? You could check whether the filename includes a full
>>> path, or something like that, so that it doesn't match. There will be
>>> a reason.
>>>
>>
>>
>> Please take my Instrumenting code comments with grain of salt, only as user report.
>> Some doc pages track suggestions and whishlists how to make life easier..
>
> Sure...people using a feature are always going to have ways to tidy it
> up / improve it. So I encourage you to take a look.
>
>>>>
>>>> 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.
>>>
>>> Yes, please send a patch or two to clean these up.
>>>
>>> Regards,
>>> Simon
>>
>>
>> I will try to allocate spare time cycles to work on this, albeit with low priority.
>
> OK
>
Regards,
Filip
More information about the U-Boot
mailing list