Hi Simon,


On 5/3/23 03:28, Simon Glass wrote:
Hi Filip,

On Tue, 2 May 2023 at 12:43, Filip Žaludek <filip.zalu...@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.zalu...@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.zalu...@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 <msucha...@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


Reply via email to