Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi Ingo, On 01/26/2017 03:22 PM, Ingo Molnar wrote: > * Lu Baolu wrote: > >> Hi, >> >> On 01/25/2017 10:38 PM, Peter Zijlstra wrote: >>> On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote: In my driver, udelay() is mostly used to handle time out. Xdbc hides most USB things in its firmware. Early printk driver only needs to setup the registers/data structures and wait until link ready or time out. Without udelay(), I have no means to convert the polling times into waiting time. >>> What is timeout and why? >> Put it in simple: >> >> The driver sets the RUN bit in control register and polls READY >> bit in status register for the successful USB device enumeration. >> As the USB device enumeration might fail and the READY bit will >> never be set, the driver must have a timeout logic to avoid >> endless loop. > Is there any error status available in the host registers anywhere that tells > us > that enumeration did not succeed? No, there isn't. The xhci spec requires software to impose a timeout. Page 425, xhci specification: " Software shall impose a timeout between the detection of the Debug Host connection and the DbC Run transition to ‘1’. " Best regards, Lu Baolu
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi Ingo, On 01/26/2017 03:19 PM, Ingo Molnar wrote: > * Lu Baolu wrote: > >> Fair enough. >> >> USB connection is stable enough, unless the user unplugs the >> USB cable during debugging. > What does the hardware do in this case? The XHCI registers are in the host > hardware, so they won't disappear, right? Is there some cable connection > status > bit we can extract without interrupts? The hardware has a register for the debug port status. Software is able to check cable unplugging through this register. > > I.e. if there's any polling component then it would be reasonable to add an > error > component: poll the status and if it goes 'disconnected' then disable > early-printk > altogether in this case and trigger an emergency printk() so that there's > chance > that the user notices [if the system does not misbehave otherwise]. Sure. I will add a kernel thread to polling the errors. I will show you the code in the new upcoming version. Best regards, Lu Baolu > > I.e. try to be as robust and informative as lockdep - yet don't lock up the > host > kernel: lockdep too is called from very deep internals, there are various > conditions where it sees corrupt data structures (i.e. a 'disconnect' - a > system > environment outside the normal bounds of operation), yet of the kernel and > over > the last 10+ years of lockdep's existence we had very, very few cases of > lockdep > itself locking up and behaving unpredictably. > > Thanks, > > Ingo >
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Peter Zijlstra wrote:
> On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote:
> > > >
> > > > I.e. if there's any polling component then it would be reasonable to
> > > > add an error
> > > > component: poll the status and if it goes 'disconnected' then disable
> > > > early-printk
> > > > altogether in this case and trigger an emergency printk() so that
> > > > there's chance
> > > > that the user notices [if the system does not misbehave otherwise].
> > >
> > > That'll be fun when printk() == early_printk() :-)
> >
> > My suggestion would be to just print into the printk buffer directly in
> > this case,
> > without console output - the developer will notice it in 'dmesg'.
>
> When you map printk() onto early_printk() dmesg will be empty, there
> will be nothing there, and therefore no reason what so ever to look
> there.
Unless you want a third layer of a console driver putting the debug message
into
dmesg isn't all that bad of a solution.
Let's admit it: something like USB that involves external pieces of hardware
_does_ have failure modes, and troubleshooting messages instead of indefinite
hangs are obviously more robust.
> I certainly don't ever look there.
You'll have to teach yourself that if the box boots up fine but there are no
messages whatsoever from the early-printk console that you'll need to look at
dmesg output or the syslog for more clues.
This should not be a common occurrance in any case - but when it happens it's
very
useful to have diagnostic messages. I don't think this is a controversial point
in
any fashion.
> Note that the printk buffer itself is a major part of why printk sucks donkey
> balls. Not to mention that you really cannot have an early_printk()
> implementation that depends on printk().
There are several easy solutions to do that, my favorite would be to put it
into
the printk buffer totally unlocked. When your early-printk is active it's
unused
and in the end it's a known data structure after all:
/*
* Just zap whatever's in the printk buffer and put your emergency message into
* it, prominently. No locking, no worries - don't generate emergency messages
* while printk is active and syslogd is running - this facility is a poor
man's
* fallback printk() when early-printk has taken over all kernel logging:
*/
void printk_emergency_puts(const char *str)
{
struct printk_log *msg, *msg_end;
msg = log_buf;
memset(msg, 0, sizeof(*msg));
msg.text_len = strlen(str);
msg_end = (void *)msg + sizeof(*msg) + msg->text_len;
/* Zero ->len denotes end of log buffer: */
memset(msg_end, 0, sizeof(*msg_end));
snprintf(ptr, str);
}
...
printk_emergency_puts"earlyprintk emergency: Hardware timed out,
shutting down. Fix your debug cable?\n");
...
(Or so - totally untested, some details might be wrong.)
But yes, I agree with your wider point, I just looked at kernel/printk/printk.c
and puked. Why did we merge that crappy piece of binary logging code, when we
already have two other binary logging facilities in the kernel already, both of
them better and cleaner than this?? Why did we mess up our nicely readable,
simple, reliable ASCII log buffer printk code? :-(
> > > I myself wouldn't mind the system getting stuck until the link is
> > > re-established. My own damn fault for taking that cable out etc.
> >
> > That's fine too, although beyond the obvious "yanked the cable without
> > realizing it" case there are corner cases where usability is increased
> > massively if the kernel is more proactive about error conditions: for
> > example
> > there are sub-standard USB cables and there are too long USB pathways from
> > overloaded USB hubs which can result in intermittent behavior, etc.
> >
> > A clear diagnostic message in 'dmesg' that the USB host controller is
> > unhappy
> > about the USB-debug dongle device is a _lot_ more useful when
> > troubleshooting
> > such problems than the occasional weird, non-deterministic hang...
>
> Sure, I'm just not sure what or where makes sense.
>
> If your serial cable is bad you notice because you don't receive the right
> amount of characters and or stuff gets mangled. You chuck the cable and get a
> new one.
>
> I think the most important part is re-establishing the link when the cable
> gets
> re-inserted. Maybe we should just drop all characters written when there's no
> link and leave it at that, same as serial.
That would be fine with me too - but even in this case there should be a stat
counter somewhere (in /proc or /debug) that counts the number of characters
dropped. Maybe that file could also display an emergency string - avoiding the
interaction with the printk buffer.
We can do better than passive-aggressive logging behavior...
Thanks,
Ingo
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
On Thu, Jan 26, 2017 at 05:01:05PM +0100, Ingo Molnar wrote: > > > > > > I.e. if there's any polling component then it would be reasonable to add > > > an error > > > component: poll the status and if it goes 'disconnected' then disable > > > early-printk > > > altogether in this case and trigger an emergency printk() so that there's > > > chance > > > that the user notices [if the system does not misbehave otherwise]. > > > > That'll be fun when printk() == early_printk() :-) > > My suggestion would be to just print into the printk buffer directly in this > case, > without console output - the developer will notice it in 'dmesg'. When you map printk() onto early_printk() dmesg will be empty, there will be nothing there, and therefore no reason what so ever to look there. I certainly don't ever look there. Note that the printk buffer itself is a major part of why printk sucks donkey balls. Not to mention that you really cannot have an early_printk() implementation that depends on printk(). > > I myself wouldn't mind the system getting stuck until the link is > > re-established. My own damn fault for taking that cable out etc. > > That's fine too, although beyond the obvious "yanked the cable without > realizing > it" case there are corner cases where usability is increased massively if the > kernel is more proactive about error conditions: for example there are > sub-standard USB cables and there are too long USB pathways from overloaded > USB > hubs which can result in intermittent behavior, etc. > > A clear diagnostic message in 'dmesg' that the USB host controller is unhappy > about the USB-debug dongle device is a _lot_ more useful when troubleshooting > such > problems than the occasional weird, non-deterministic hang... Sure, I'm just not sure what or where makes sense. If your serial cable is bad you notice because you don't receive the right amount of characters and or stuff gets mangled. You chuck the cable and get a new one. I think the most important part is re-establishing the link when the cable gets re-inserted. Maybe we should just drop all characters written when there's no link and leave it at that, same as serial.
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Peter Zijlstra wrote: > On Thu, Jan 26, 2017 at 08:19:37AM +0100, Ingo Molnar wrote: > > > > * Lu Baolu wrote: > > > > > Fair enough. > > > > > > USB connection is stable enough, unless the user unplugs the > > > USB cable during debugging. > > > > What does the hardware do in this case? The XHCI registers are in the host > > hardware, so they won't disappear, right? Is there some cable connection > > status > > bit we can extract without interrupts? > > > > I.e. if there's any polling component then it would be reasonable to add an > > error > > component: poll the status and if it goes 'disconnected' then disable > > early-printk > > altogether in this case and trigger an emergency printk() so that there's > > chance > > that the user notices [if the system does not misbehave otherwise]. > > That'll be fun when printk() == early_printk() :-) My suggestion would be to just print into the printk buffer directly in this case, without console output - the developer will notice it in 'dmesg'. > I myself wouldn't mind the system getting stuck until the link is > re-established. My own damn fault for taking that cable out etc. That's fine too, although beyond the obvious "yanked the cable without realizing it" case there are corner cases where usability is increased massively if the kernel is more proactive about error conditions: for example there are sub-standard USB cables and there are too long USB pathways from overloaded USB hubs which can result in intermittent behavior, etc. A clear diagnostic message in 'dmesg' that the USB host controller is unhappy about the USB-debug dongle device is a _lot_ more useful when troubleshooting such problems than the occasional weird, non-deterministic hang... Thanks, Ingo
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
On Thu, Jan 26, 2017 at 08:19:37AM +0100, Ingo Molnar wrote: > > * Lu Baolu wrote: > > > Fair enough. > > > > USB connection is stable enough, unless the user unplugs the > > USB cable during debugging. > > What does the hardware do in this case? The XHCI registers are in the host > hardware, so they won't disappear, right? Is there some cable connection > status > bit we can extract without interrupts? > > I.e. if there's any polling component then it would be reasonable to add an > error > component: poll the status and if it goes 'disconnected' then disable > early-printk > altogether in this case and trigger an emergency printk() so that there's > chance > that the user notices [if the system does not misbehave otherwise]. That'll be fun when printk() == early_printk() :-) I myself wouldn't mind the system getting stuck until the link is re-established. My own damn fault for taking that cable out etc.
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Lu Baolu wrote: > Hi Ingo, > > On 01/26/2017 03:19 PM, Ingo Molnar wrote: > > * Lu Baolu wrote: > > > >> Fair enough. > >> > >> USB connection is stable enough, unless the user unplugs the > >> USB cable during debugging. > > What does the hardware do in this case? The XHCI registers are in the host > > hardware, so they won't disappear, right? Is there some cable connection > > status > > bit we can extract without interrupts? > > Yes, there are register bits for us to know the cable status. I will go > through the spec again and give you more accurate answer later. Ok, that's good news - so we don't really have to time out and we don't have to rely on the user holding the phone right either. > I'm sorry. I will be off during the next 7 days for Chinese New Year > holiday. My email access will be very limited during this time. I will > revisit this thread after I am back from holiday. > > Sorry for the inconvenience. No problem, have fun! Thanks, Ingo
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi Ingo, On 01/26/2017 03:19 PM, Ingo Molnar wrote: > * Lu Baolu wrote: > >> Fair enough. >> >> USB connection is stable enough, unless the user unplugs the >> USB cable during debugging. > What does the hardware do in this case? The XHCI registers are in the host > hardware, so they won't disappear, right? Is there some cable connection > status > bit we can extract without interrupts? Yes, there are register bits for us to know the cable status. I will go through the spec again and give you more accurate answer later. I'm sorry. I will be off during the next 7 days for Chinese New Year holiday. My email access will be very limited during this time. I will revisit this thread after I am back from holiday. Sorry for the inconvenience. Best regards, Lu Baolu > I.e. if there's any polling component then it would be reasonable to add an > error > component: poll the status and if it goes 'disconnected' then disable > early-printk > altogether in this case and trigger an emergency printk() so that there's > chance > that the user notices [if the system does not misbehave otherwise]. > > I.e. try to be as robust and informative as lockdep - yet don't lock up the > host > kernel: lockdep too is called from very deep internals, there are various > conditions where it sees corrupt data structures (i.e. a 'disconnect' - a > system > environment outside the normal bounds of operation), yet of the kernel and > over > the last 10+ years of lockdep's existence we had very, very few cases of > lockdep > itself locking up and behaving unpredictably. > > Thanks, > > Ingo >
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Lu Baolu wrote: > > Hi, > > On 01/25/2017 10:38 PM, Peter Zijlstra wrote: > > On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote: > >> In my driver, udelay() is mostly used to handle time out. > >> > >> Xdbc hides most USB things in its firmware. Early printk driver only needs > >> to setup the registers/data structures and wait until link ready or time > >> out. > >> Without udelay(), I have no means to convert the polling times into waiting > >> time. > > What is timeout and why? > > Put it in simple: > > The driver sets the RUN bit in control register and polls READY > bit in status register for the successful USB device enumeration. > As the USB device enumeration might fail and the READY bit will > never be set, the driver must have a timeout logic to avoid > endless loop. Is there any error status available in the host registers anywhere that tells us that enumeration did not succeed? Thanks, Ingo
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Lu Baolu wrote: > Fair enough. > > USB connection is stable enough, unless the user unplugs the > USB cable during debugging. What does the hardware do in this case? The XHCI registers are in the host hardware, so they won't disappear, right? Is there some cable connection status bit we can extract without interrupts? I.e. if there's any polling component then it would be reasonable to add an error component: poll the status and if it goes 'disconnected' then disable early-printk altogether in this case and trigger an emergency printk() so that there's chance that the user notices [if the system does not misbehave otherwise]. I.e. try to be as robust and informative as lockdep - yet don't lock up the host kernel: lockdep too is called from very deep internals, there are various conditions where it sees corrupt data structures (i.e. a 'disconnect' - a system environment outside the normal bounds of operation), yet of the kernel and over the last 10+ years of lockdep's existence we had very, very few cases of lockdep itself locking up and behaving unpredictably. Thanks, Ingo
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi, On 01/26/2017 12:16 AM, Peter Zijlstra wrote: > On Wed, Jan 25, 2017 at 11:51:34PM +0800, Lu Baolu wrote: > >>> What is timeout and why? >> Put it in simple: >> >> The driver sets the RUN bit in control register and polls READY >> bit in status register for the successful USB device enumeration. >> As the USB device enumeration might fail and the READY bit will >> never be set, the driver must have a timeout logic to avoid >> endless loop. >> >> More details: >> >> The operational model is that driver sets up all necessary registers >> and data structures, and then starts the debug engine by setting >> the RUN/STOP bit in the control register. >> >> The debug engine then brings up itself as a ready-for-enumeration >> USB device. The USB link between host and device starts link training >> and then host will detect the connected device. The hub driver in >> host will then starts the USB device enumeration processes (as defined >> in USB spec). If everything goes smoothly, the device gets enumerated >> and host can talk with the debug device. >> >> After that, xdbc firmware will set the READY bit in status register. And >> the driver can go ahead with data transfer over USB. > I have vague memories from a prior discussion where you said this READY > state can be lost at any time (cable unplug or whatnot) and at that > point the driver should re-start the setup, right? Yes. So the documentation requires users not to unplug the usb cable during debugging. This rule applies to other debug methods as well. > >>> If there is an error other than !ready, I would >>> expect the hardware to inform you of this through another status bit, >>> no? >> Yeah, this might be another choice of hardware design. But it's not a >> topic for this driver. > So is there really no way to way to distinguish between "I did setup and > am waiting for READY", "I did setup, am waiting for READY, but things > got hosed" and "I was READY, things be hosed" ? > > I suppose the first and last can be distinguished by remembering if you > ever saw READY, but the first and second are the interesting case I > think. > >>> So why can't you poll indefinitely for either ready or error? >>> >> Even if the hardware has both ready and error status bits, it's still >> nice to have a time out watch dog. Buggy hardware or firmware >> might not set any of these bits. Polling indefinitely might result in >> a endless loop. > Loosing output, esp. without indication, is very _very_ annoying when > you're debugging things. Its just about on par with a stuck system, at > least then you know something bad happened. Fair enough. USB connection is stable enough, unless the user unplugs the USB cable during debugging. Best regards, Lu Baolu
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi Ingo,
On 01/25/2017 05:23 PM, Ingo Molnar wrote:
> * Lu Baolu wrote:
>
>>> Hiding essentially an early udelay() implementation in an early-printk
>>> driver is
>>> ugly and counterproductive.
>> Sure. How about below change?
>>
>> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
>> index d3f0c84..940989e 100644
>> --- a/drivers/usb/early/xhci-dbc.c
>> +++ b/drivers/usb/early/xhci-dbc.c
>> @@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size,
>> bool read)
>> return size;
>> }
>>
>> +static void __init xdbc_udelay_calibration(void)
>> +{
>> + unsigned long lpj = 0;
>> + unsigned int tsc_khz, cpu_khz;
>> +
>> + if (!boot_cpu_has(X86_FEATURE_TSC))
>> + goto calibration_out;
>> +
>> + cpu_khz = x86_platform.calibrate_cpu();
>> + tsc_khz = x86_platform.calibrate_tsc();
>> +
>> + if (tsc_khz == 0)
>> + tsc_khz = cpu_khz;
>> + else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
>> + cpu_khz = tsc_khz;
>> +
>> + if (!tsc_khz)
>> + goto calibration_out;
>> +
>> + lpj = tsc_khz * 1000;
>> + do_div(lpj, HZ);
>> +
>> +calibration_out:
>> + if (!lpj)
>> + lpj = 1 << 22;
>> +
>> + loops_per_jiffy = lpj;
>> +}
>> +
>> static int __init xdbc_early_setup(void)
>> {
>> int ret;
>> @@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
>> }
>> xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base +
>> offset);
>>
>> + xdbc_udelay_calibration();
>> +
>> return 0;
>> }
> Yeah - so could we do this in a more generic fashion, not in the early-printk
> driver but in core x86 code?
How about below changes?
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 4cfba94..aab7faa 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -835,6 +835,26 @@ dump_kernel_offset(struct notifier_block *self, unsigned
long v, void *p)
return 0;
}
+static void __init simple_udelay_calibration(void)
+{
+ unsigned int tsc_khz, cpu_khz;
+ unsigned long lpj;
+
+ if (!boot_cpu_has(X86_FEATURE_TSC))
+ return;
+
+ cpu_khz = x86_platform.calibrate_cpu();
+ tsc_khz = x86_platform.calibrate_tsc();
+
+ tsc_khz = tsc_khz ? : cpu_khz;
+ if (!tsc_khz)
+ return;
+
+ lpj = tsc_khz * 1000;
+ do_div(lpj, HZ);
+ loops_per_jiffy = lpj;
+}
+
/*
* Determine if we were loaded by an EFI loader. If so, then we have also been
* passed the efi memmap, systab, etc., so we should use these data structures
@@ -983,6 +1003,8 @@ void __init setup_arch(char **cmdline_p)
*/
x86_configure_nx();
+ simple_udelay_calibration();
+
parse_early_param();
If it's okay for you, do you want it in a separated patch or part of patch 2/4?
Best regards,
Lu Baolu
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
On Wed, Jan 25, 2017 at 11:51:34PM +0800, Lu Baolu wrote: > > What is timeout and why? > > Put it in simple: > > The driver sets the RUN bit in control register and polls READY > bit in status register for the successful USB device enumeration. > As the USB device enumeration might fail and the READY bit will > never be set, the driver must have a timeout logic to avoid > endless loop. > > More details: > > The operational model is that driver sets up all necessary registers > and data structures, and then starts the debug engine by setting > the RUN/STOP bit in the control register. > > The debug engine then brings up itself as a ready-for-enumeration > USB device. The USB link between host and device starts link training > and then host will detect the connected device. The hub driver in > host will then starts the USB device enumeration processes (as defined > in USB spec). If everything goes smoothly, the device gets enumerated > and host can talk with the debug device. > > After that, xdbc firmware will set the READY bit in status register. And > the driver can go ahead with data transfer over USB. I have vague memories from a prior discussion where you said this READY state can be lost at any time (cable unplug or whatnot) and at that point the driver should re-start the setup, right? > > If there is an error other than !ready, I would > > expect the hardware to inform you of this through another status bit, > > no? > > Yeah, this might be another choice of hardware design. But it's not a > topic for this driver. So is there really no way to way to distinguish between "I did setup and am waiting for READY", "I did setup, am waiting for READY, but things got hosed" and "I was READY, things be hosed" ? I suppose the first and last can be distinguished by remembering if you ever saw READY, but the first and second are the interesting case I think. > > So why can't you poll indefinitely for either ready or error? > > > > Even if the hardware has both ready and error status bits, it's still > nice to have a time out watch dog. Buggy hardware or firmware > might not set any of these bits. Polling indefinitely might result in > a endless loop. Loosing output, esp. without indication, is very _very_ annoying when you're debugging things. Its just about on par with a stuck system, at least then you know something bad happened.
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi, On 01/25/2017 10:38 PM, Peter Zijlstra wrote: > On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote: >> In my driver, udelay() is mostly used to handle time out. >> >> Xdbc hides most USB things in its firmware. Early printk driver only needs >> to setup the registers/data structures and wait until link ready or time out. >> Without udelay(), I have no means to convert the polling times into waiting >> time. > What is timeout and why? Put it in simple: The driver sets the RUN bit in control register and polls READY bit in status register for the successful USB device enumeration. As the USB device enumeration might fail and the READY bit will never be set, the driver must have a timeout logic to avoid endless loop. More details: The operational model is that driver sets up all necessary registers and data structures, and then starts the debug engine by setting the RUN/STOP bit in the control register. The debug engine then brings up itself as a ready-for-enumeration USB device. The USB link between host and device starts link training and then host will detect the connected device. The hub driver in host will then starts the USB device enumeration processes (as defined in USB spec). If everything goes smoothly, the device gets enumerated and host can talk with the debug device. After that, xdbc firmware will set the READY bit in status register. And the driver can go ahead with data transfer over USB. > If there is an error other than !ready, I would > expect the hardware to inform you of this through another status bit, > no? Yeah, this might be another choice of hardware design. But it's not a topic for this driver. > > So why can't you poll indefinitely for either ready or error? > > Even if the hardware has both ready and error status bits, it's still nice to have a time out watch dog. Buggy hardware or firmware might not set any of these bits. Polling indefinitely might result in a endless loop. Best regards, Lu Baolu
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
On Wed, Jan 25, 2017 at 08:27:38PM +0800, Lu Baolu wrote: > In my driver, udelay() is mostly used to handle time out. > > Xdbc hides most USB things in its firmware. Early printk driver only needs > to setup the registers/data structures and wait until link ready or time out. > Without udelay(), I have no means to convert the polling times into waiting > time. What is timeout and why? If there is an error other than !ready, I would expect the hardware to inform you of this through another status bit, no? So why can't you poll indefinitely for either ready or error?
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi, On 01/25/2017 05:57 PM, Peter Zijlstra wrote: > On Wed, Jan 25, 2017 at 10:23:55AM +0100, Ingo Molnar wrote: >> * Lu Baolu wrote: >> Hiding essentially an early udelay() implementation in an early-printk driver is ugly and counterproductive. >> Yeah - so could we do this in a more generic fashion, not in the >> early-printk >> driver but in core x86 code? > So ideally early_printk() would not depend on udelay() being setup. > > In fact, ideally early_printk() wouldn't even use udelay -- this very > much includes its own copy. > > Why is udelay() required? Can't the thing simply poll its own register > state to wait for completion? In my driver, udelay() is mostly used to handle time out. Xdbc hides most USB things in its firmware. Early printk driver only needs to setup the registers/data structures and wait until link ready or time out. Without udelay(), I have no means to convert the polling times into waiting time. Best regards, Lu Baolu > > This all sounds like xdbc cruft is still unreliably garbage.. >
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi,
On 01/25/2017 05:23 PM, Ingo Molnar wrote:
> * Lu Baolu wrote:
>
>>> Hiding essentially an early udelay() implementation in an early-printk
>>> driver is
>>> ugly and counterproductive.
>> Sure. How about below change?
>>
>> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
>> index d3f0c84..940989e 100644
>> --- a/drivers/usb/early/xhci-dbc.c
>> +++ b/drivers/usb/early/xhci-dbc.c
>> @@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size,
>> bool read)
>> return size;
>> }
>>
>> +static void __init xdbc_udelay_calibration(void)
>> +{
>> + unsigned long lpj = 0;
>> + unsigned int tsc_khz, cpu_khz;
>> +
>> + if (!boot_cpu_has(X86_FEATURE_TSC))
>> + goto calibration_out;
>> +
>> + cpu_khz = x86_platform.calibrate_cpu();
>> + tsc_khz = x86_platform.calibrate_tsc();
>> +
>> + if (tsc_khz == 0)
>> + tsc_khz = cpu_khz;
>> + else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
>> + cpu_khz = tsc_khz;
>> +
>> + if (!tsc_khz)
>> + goto calibration_out;
>> +
>> + lpj = tsc_khz * 1000;
>> + do_div(lpj, HZ);
>> +
>> +calibration_out:
>> + if (!lpj)
>> + lpj = 1 << 22;
>> +
>> + loops_per_jiffy = lpj;
>> +}
>> +
>> static int __init xdbc_early_setup(void)
>> {
>> int ret;
>> @@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
>> }
>> xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base +
>> offset);
>>
>> + xdbc_udelay_calibration();
>> +
>> return 0;
>> }
> Yeah - so could we do this in a more generic fashion, not in the early-printk
> driver but in core x86 code?
>
Sure. I will move this to arch/x86/kernel/setup.c.
Best regards,
Lu Baolu
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
On Wed, Jan 25, 2017 at 10:23:55AM +0100, Ingo Molnar wrote: > > * Lu Baolu wrote: > > > > Hiding essentially an early udelay() implementation in an early-printk > > > driver is > > > ugly and counterproductive. > Yeah - so could we do this in a more generic fashion, not in the early-printk > driver but in core x86 code? So ideally early_printk() would not depend on udelay() being setup. In fact, ideally early_printk() wouldn't even use udelay -- this very much includes its own copy. Why is udelay() required? Can't the thing simply poll its own register state to wait for completion? This all sounds like xdbc cruft is still unreliably garbage..
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Lu Baolu wrote:
> > Hiding essentially an early udelay() implementation in an early-printk
> > driver is
> > ugly and counterproductive.
>
> Sure. How about below change?
>
> diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
> index d3f0c84..940989e 100644
> --- a/drivers/usb/early/xhci-dbc.c
> +++ b/drivers/usb/early/xhci-dbc.c
> @@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool
> read)
> return size;
> }
>
> +static void __init xdbc_udelay_calibration(void)
> +{
> + unsigned long lpj = 0;
> + unsigned int tsc_khz, cpu_khz;
> +
> + if (!boot_cpu_has(X86_FEATURE_TSC))
> + goto calibration_out;
> +
> + cpu_khz = x86_platform.calibrate_cpu();
> + tsc_khz = x86_platform.calibrate_tsc();
> +
> + if (tsc_khz == 0)
> + tsc_khz = cpu_khz;
> + else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
> + cpu_khz = tsc_khz;
> +
> + if (!tsc_khz)
> + goto calibration_out;
> +
> + lpj = tsc_khz * 1000;
> + do_div(lpj, HZ);
> +
> +calibration_out:
> + if (!lpj)
> + lpj = 1 << 22;
> +
> + loops_per_jiffy = lpj;
> +}
> +
> static int __init xdbc_early_setup(void)
> {
> int ret;
> @@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
> }
> xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
>
> + xdbc_udelay_calibration();
> +
> return 0;
> }
Yeah - so could we do this in a more generic fashion, not in the early-printk
driver but in core x86 code?
Thanks,
Ingo
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi Ingo,
On 01/24/2017 04:20 PM, Ingo Molnar wrote:
> * Lu Baolu wrote:
>
>> Hi Ingo,
>>
>> On 01/22/2017 05:04 PM, Ingo Molnar wrote:
>>> * Lu Baolu wrote:
>>>
>> +static void xdbc_runtime_delay(unsigned long count)
>> +{
>> +udelay(count);
>> +}
>> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
> Is this udelay() complication really necessary? udelay() should work fine
> even in
> early code. It might not be precisely calibrated, but should be good
> enough.
I tried udelay() in the early code. It's not precise enough for the
hardware handshaking.
>>> Possibly because on x86 early udelay() did not work at all - i.e. there's
>>> no delay
>>> whatsoever.
>> Yes.
>>
>>> Could you try it on top of this commit in tip:timers/core:
>>>
>>> 4c45c5167c95 x86/timer: Make delay() work during early bootup
>>>
>>> ?
>> I tried tip:timers/core. It's not precise enough for my context either.
>>
>> __const_udelay().
>>
>> 157 inline void __const_udelay(unsigned long xloops)
>> 158 {
>> 159 unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? :
>> loops_per_jiffy;
>> 160 int d0;
>> 161
>> 162 xloops *= 4;
>> 163 asm("mull %%edx"
>> 164 :"=d" (xloops), "=&a" (d0)
>> 165 :"1" (xloops), "0" (lpj * (HZ / 4)));
>> 166
>> 167 __delay(++xloops);
>> 168 }
>>
>>
>> In my early code, loops_per_jiffy is not initialized yet. Hence "lpj" for
>> the asm line
>> is 4096 (default value).
>>
>> The cpu_info.loops_per_jiffy actually reads 8832000 after initialization.
>> They are
>> about 2000 times different.
>>
>> I did a hacky test in kernel to check the difference between these two
>> different
>> "lpj" values. (The hacky patch is attached.) Below is the output for 100ms
>> delay.
>>
>> [2.494751] udelay_test uninitialized >start
>> [2.494820] udelay_test uninitialized >end
>> [2.494828] udelay_test initialized >start
>> [2.595234] udelay_test initialized >end
>>
>> For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a
>> delay of
>> only 69us.
> Ok, then could we add some simple calibration to make udelay work much better
> - or
> perhaps move the udelay calibration up earlier?
>
> Hiding essentially an early udelay() implementation in an early-printk driver
> is
> ugly and counterproductive.
Sure. How about below change?
diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index d3f0c84..940989e 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -587,6 +587,35 @@ static int xdbc_bulk_transfer(void *data, int size, bool
read)
return size;
}
+static void __init xdbc_udelay_calibration(void)
+{
+ unsigned long lpj = 0;
+ unsigned int tsc_khz, cpu_khz;
+
+ if (!boot_cpu_has(X86_FEATURE_TSC))
+ goto calibration_out;
+
+ cpu_khz = x86_platform.calibrate_cpu();
+ tsc_khz = x86_platform.calibrate_tsc();
+
+ if (tsc_khz == 0)
+ tsc_khz = cpu_khz;
+ else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
+ cpu_khz = tsc_khz;
+
+ if (!tsc_khz)
+ goto calibration_out;
+
+ lpj = tsc_khz * 1000;
+ do_div(lpj, HZ);
+
+calibration_out:
+ if (!lpj)
+ lpj = 1 << 22;
+
+ loops_per_jiffy = lpj;
+}
+
static int __init xdbc_early_setup(void)
{
int ret;
@@ -686,6 +715,8 @@ int __init early_xdbc_parse_parameter(char *s)
}
xdbc.xdbc_reg = (struct xdbc_regs __iomem *)(xdbc.xhci_base + offset);
+ xdbc_udelay_calibration();
+
return 0;
}
Best regards,
Lu Baolu
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Lu Baolu wrote:
> Hi Ingo,
>
> On 01/22/2017 05:04 PM, Ingo Molnar wrote:
> > * Lu Baolu wrote:
> >
> +static void xdbc_runtime_delay(unsigned long count)
> +{
> +udelay(count);
> +}
> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
> >>> Is this udelay() complication really necessary? udelay() should work fine
> >>> even in
> >>> early code. It might not be precisely calibrated, but should be good
> >>> enough.
> >> I tried udelay() in the early code. It's not precise enough for the
> >> hardware handshaking.
> > Possibly because on x86 early udelay() did not work at all - i.e. there's
> > no delay
> > whatsoever.
>
> Yes.
>
> >
> > Could you try it on top of this commit in tip:timers/core:
> >
> > 4c45c5167c95 x86/timer: Make delay() work during early bootup
> >
> > ?
>
> I tried tip:timers/core. It's not precise enough for my context either.
>
> __const_udelay().
>
> 157 inline void __const_udelay(unsigned long xloops)
> 158 {
> 159 unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? :
> loops_per_jiffy;
> 160 int d0;
> 161
> 162 xloops *= 4;
> 163 asm("mull %%edx"
> 164 :"=d" (xloops), "=&a" (d0)
> 165 :"1" (xloops), "0" (lpj * (HZ / 4)));
> 166
> 167 __delay(++xloops);
> 168 }
>
>
> In my early code, loops_per_jiffy is not initialized yet. Hence "lpj" for
> the asm line
> is 4096 (default value).
>
> The cpu_info.loops_per_jiffy actually reads 8832000 after initialization.
> They are
> about 2000 times different.
>
> I did a hacky test in kernel to check the difference between these two
> different
> "lpj" values. (The hacky patch is attached.) Below is the output for 100ms
> delay.
>
> [2.494751] udelay_test uninitialized >start
> [2.494820] udelay_test uninitialized >end
> [2.494828] udelay_test initialized >start
> [2.595234] udelay_test initialized >end
>
> For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a
> delay of
> only 69us.
Ok, then could we add some simple calibration to make udelay work much better -
or
perhaps move the udelay calibration up earlier?
Hiding essentially an early udelay() implementation in an early-printk driver
is
ugly and counterproductive.
Thanks,
Ingo
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi Ingo,
On 01/22/2017 05:04 PM, Ingo Molnar wrote:
> * Lu Baolu wrote:
>
+static void xdbc_runtime_delay(unsigned long count)
+{
+ udelay(count);
+}
+static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
>>> Is this udelay() complication really necessary? udelay() should work fine
>>> even in
>>> early code. It might not be precisely calibrated, but should be good enough.
>> I tried udelay() in the early code. It's not precise enough for the
>> hardware handshaking.
> Possibly because on x86 early udelay() did not work at all - i.e. there's no
> delay
> whatsoever.
Yes.
>
> Could you try it on top of this commit in tip:timers/core:
>
> 4c45c5167c95 x86/timer: Make delay() work during early bootup
>
> ?
I tried tip:timers/core. It's not precise enough for my context either.
__const_udelay().
157 inline void __const_udelay(unsigned long xloops)
158 {
159 unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy) ? :
loops_per_jiffy;
160 int d0;
161
162 xloops *= 4;
163 asm("mull %%edx"
164 :"=d" (xloops), "=&a" (d0)
165 :"1" (xloops), "0" (lpj * (HZ / 4)));
166
167 __delay(++xloops);
168 }
In my early code, loops_per_jiffy is not initialized yet. Hence "lpj" for the
asm line
is 4096 (default value).
The cpu_info.loops_per_jiffy actually reads 8832000 after initialization. They
are
about 2000 times different.
I did a hacky test in kernel to check the difference between these two different
"lpj" values. (The hacky patch is attached.) Below is the output for 100ms
delay.
[2.494751] udelay_test uninitialized >start
[2.494820] udelay_test uninitialized >end
[2.494828] udelay_test initialized >start
[2.595234] udelay_test initialized >end
For 100ms delay, udelay() with uninitialized loops_per_jiffy only gives a delay
of
only 69us.
Best regards,
Lu Baolu
diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
index a8e91ae..ffc2874 100644
--- a/arch/x86/lib/delay.c
+++ b/arch/x86/lib/delay.c
@@ -168,6 +168,36 @@ inline void __const_udelay(unsigned long xloops)
}
EXPORT_SYMBOL(__const_udelay);
+void udelay_uninitialized(unsigned long xloops)
+{
+ unsigned long lpj = (1<<12);
+ int d0;
+
+ xloops *= 0x10c7ul;
+ xloops *= 4;
+ asm("mull %%edx"
+ :"=d" (xloops), "=&a" (d0)
+ :"1" (xloops), "0" (lpj * (HZ / 4)));
+
+ delay_loop(++xloops);
+}
+EXPORT_SYMBOL(udelay_uninitialized);
+
+void udelay_initialized(unsigned long xloops)
+{
+ unsigned long lpj = this_cpu_read(cpu_info.loops_per_jiffy);
+ int d0;
+
+ xloops *= 0x10c7ul;
+ xloops *= 4;
+ asm("mull %%edx"
+ :"=d" (xloops), "=&a" (d0)
+ :"1" (xloops), "0" (lpj * (HZ / 4)));
+
+ delay_loop(++xloops);
+}
+EXPORT_SYMBOL(udelay_initialized);
+
void __udelay(unsigned long usecs)
{
__const_udelay(usecs * 0x10c7); /* 2**32 / 100 (rounded up) */
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 954abfd..b6a7437 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -302,6 +302,21 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
/* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
pm_runtime_put_noidle(&dev->dev);
+ do {
+ int count = 1000;
+
+ pr_notice("udelay_test uninitialized >start\n");
+ while (count-- > 0)
+ udelay_uninitialized(100);
+ pr_notice("udelay_test uninitialized >end\n");
+
+ count = 1000;
+ pr_notice("udelay_test initialized >start\n");
+ while (count-- > 0)
+ udelay_initialized(100);
+ pr_notice("udelay_test initialized >end\n");
+ } while (0);
+
return 0;
put_usb3_hcd:
diff --git a/include/asm-generic/delay.h b/include/asm-generic/delay.h
index 0f79054..200ab55 100644
--- a/include/asm-generic/delay.h
+++ b/include/asm-generic/delay.h
@@ -9,6 +9,8 @@ extern void __udelay(unsigned long usecs);
extern void __ndelay(unsigned long nsecs);
extern void __const_udelay(unsigned long xloops);
extern void __delay(unsigned long loops);
+extern void udelay_uninitialized(unsigned long xloops);
+extern void udelay_initialized(unsigned long xloops);
/*
* The weird n/2 thing suppresses a "comparison is always false due to
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Lu Baolu wrote:
> >
> >> +static void xdbc_runtime_delay(unsigned long count)
> >> +{
> >> + udelay(count);
> >> +}
> >> +static void (*xdbc_delay)(unsigned long) = xdbc_early_delay;
> > Is this udelay() complication really necessary? udelay() should work fine
> > even in
> > early code. It might not be precisely calibrated, but should be good enough.
>
> I tried udelay() in the early code. It's not precise enough for the
> hardware handshaking.
Possibly because on x86 early udelay() did not work at all - i.e. there's no
delay
whatsoever.
Could you try it on top of this commit in tip:timers/core:
4c45c5167c95 x86/timer: Make delay() work during early bootup
?
Thanks,
Ingo
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
Hi Ingo,
I'm very appreciated for your review comments. I've put my
replies in lines.
On 01/19/2017 05:37 PM, Ingo Molnar wrote:
> * Lu Baolu wrote:
>
>> xHCI debug capability (DbC) is an optional but standalone
>> functionality provided by an xHCI host controller. Software
>> learns this capability by walking through the extended
>> capability list of the host. xHCI specification describes
>> DbC in section 7.6.
>>
>> This patch introduces the code to probe and initialize the
>> debug capability hardware during early boot. With hardware
>> initialized, the debug target (system on which this code is
>> running) will present a debug device through the debug port
>> (normally the first USB3 port). The debug device is fully
>> compliant with the USB framework and provides the equivalent
>> of a very high performance (USB3) full-duplex serial link
>> between the debug host and target. The DbC functionality is
>> independent of xHCI host. There isn't any precondition from
>> xHCI host side for DbC to work.
>>
>> This patch also includes bulk out and bulk in interfaces.
>> These interfaces could be used to implement early printk
>> bootconsole or hook to various system debuggers.
>>
>> This code is designed to be only used for kernel debugging
>> when machine crashes very early before the console code is
>> initialized. For normal operation it is not recommended.
>>
>> Cc: Mathias Nyman
>> Signed-off-by: Lu Baolu
>> ---
>> arch/x86/Kconfig.debug| 14 +
>> drivers/usb/Kconfig |3 +
>> drivers/usb/Makefile |2 +-
>> drivers/usb/early/Makefile|1 +
>> drivers/usb/early/xhci-dbc.c | 1068
>> +
>> drivers/usb/early/xhci-dbc.h | 205
>> include/linux/usb/xhci-dbgp.h | 22 +
>> 7 files changed, 1314 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/usb/early/xhci-dbc.c
>> create mode 100644 drivers/usb/early/xhci-dbc.h
>> create mode 100644 include/linux/usb/xhci-dbgp.h
>>
>> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
>> index 67eec55..13e85b7 100644
>> --- a/arch/x86/Kconfig.debug
>> +++ b/arch/x86/Kconfig.debug
>> @@ -29,6 +29,7 @@ config EARLY_PRINTK
>> config EARLY_PRINTK_DBGP
>> bool "Early printk via EHCI debug port"
>> depends on EARLY_PRINTK && PCI
>> +select USB_EARLY_PRINTK
>> ---help---
>>Write kernel log output directly into the EHCI debug port.
>>
>> @@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
>>This is useful for kernel debugging when your machine crashes very
>>early before the console code is initialized.
>>
>> +config EARLY_PRINTK_XDBC
>> +bool "Early printk via xHCI debug port"
>> +depends on EARLY_PRINTK && PCI
>> +select USB_EARLY_PRINTK
>> +---help---
>> + Write kernel log output directly into the xHCI debug port.
>> +
>> + This is useful for kernel debugging when your machine crashes very
>> + early before the console code is initialized. For normal operation
>> + it is not recommended because it looks ugly and doesn't cooperate
>> + with klogd/syslogd or the X server. You should normally N here,
>> + unless you want to debug such a crash.
> Could we please do this rename:
>
> s/EARLY_PRINTK_XDBC
>EARLY_PRINTK_USB_XDBC
>
> ?
>
> As many people will not realize what 'xdbc' means, standalone - while "it's
> an
> USB serial logging variant" is a lot more natural.
>
>
>> +config USB_EARLY_PRINTK
>> +bool
> Also, could we standardize the nomencalture to not be a mixture of prefixes
> and
> postfixes - i.e. standardize on postfixes (as commonly done in the Kconfig
> space)
> and rename this one to EARLY_PRINTK_USB or so?
>
> You can see the prefix/postfix inconsistency here already:
Sure. I will fix the names. Thanks.
>
>> -obj-$(CONFIG_EARLY_PRINTK_DBGP) += early/
>> +obj-$(CONFIG_USB_EARLY_PRINTK) += early/
>> +obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
>> +static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
>> +{
>> +u32 val, sz;
>> +u64 val64, sz64, mask64;
>> +u8 byte;
>> +void __iomem *base;
>> +
>> +val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
>> +write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, ~0);
>> +sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
>> +write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, val);
>> +if (val == 0x || sz == 0x) {
>> +pr_notice("invalid mmio bar\n");
>> +return NULL;
>> +}
>> +if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
>> +PCI_BASE_ADDRESS_MEM_TYPE_64) {
> Please don't break the line here.
Sure. Will fix it.
>
>> +val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
>> +write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, ~0);
>> +sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
>> +write_pci_
Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
* Lu Baolu wrote:
> xHCI debug capability (DbC) is an optional but standalone
> functionality provided by an xHCI host controller. Software
> learns this capability by walking through the extended
> capability list of the host. xHCI specification describes
> DbC in section 7.6.
>
> This patch introduces the code to probe and initialize the
> debug capability hardware during early boot. With hardware
> initialized, the debug target (system on which this code is
> running) will present a debug device through the debug port
> (normally the first USB3 port). The debug device is fully
> compliant with the USB framework and provides the equivalent
> of a very high performance (USB3) full-duplex serial link
> between the debug host and target. The DbC functionality is
> independent of xHCI host. There isn't any precondition from
> xHCI host side for DbC to work.
>
> This patch also includes bulk out and bulk in interfaces.
> These interfaces could be used to implement early printk
> bootconsole or hook to various system debuggers.
>
> This code is designed to be only used for kernel debugging
> when machine crashes very early before the console code is
> initialized. For normal operation it is not recommended.
>
> Cc: Mathias Nyman
> Signed-off-by: Lu Baolu
> ---
> arch/x86/Kconfig.debug| 14 +
> drivers/usb/Kconfig |3 +
> drivers/usb/Makefile |2 +-
> drivers/usb/early/Makefile|1 +
> drivers/usb/early/xhci-dbc.c | 1068
> +
> drivers/usb/early/xhci-dbc.h | 205
> include/linux/usb/xhci-dbgp.h | 22 +
> 7 files changed, 1314 insertions(+), 1 deletion(-)
> create mode 100644 drivers/usb/early/xhci-dbc.c
> create mode 100644 drivers/usb/early/xhci-dbc.h
> create mode 100644 include/linux/usb/xhci-dbgp.h
>
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 67eec55..13e85b7 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -29,6 +29,7 @@ config EARLY_PRINTK
> config EARLY_PRINTK_DBGP
> bool "Early printk via EHCI debug port"
> depends on EARLY_PRINTK && PCI
> + select USB_EARLY_PRINTK
> ---help---
> Write kernel log output directly into the EHCI debug port.
>
> @@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI
> This is useful for kernel debugging when your machine crashes very
> early before the console code is initialized.
>
> +config EARLY_PRINTK_XDBC
> + bool "Early printk via xHCI debug port"
> + depends on EARLY_PRINTK && PCI
> + select USB_EARLY_PRINTK
> + ---help---
> + Write kernel log output directly into the xHCI debug port.
> +
> + This is useful for kernel debugging when your machine crashes very
> + early before the console code is initialized. For normal operation
> + it is not recommended because it looks ugly and doesn't cooperate
> + with klogd/syslogd or the X server. You should normally N here,
> + unless you want to debug such a crash.
Could we please do this rename:
s/EARLY_PRINTK_XDBC
EARLY_PRINTK_USB_XDBC
?
As many people will not realize what 'xdbc' means, standalone - while "it's an
USB serial logging variant" is a lot more natural.
> +config USB_EARLY_PRINTK
> + bool
Also, could we standardize the nomencalture to not be a mixture of prefixes and
postfixes - i.e. standardize on postfixes (as commonly done in the Kconfig
space)
and rename this one to EARLY_PRINTK_USB or so?
You can see the prefix/postfix inconsistency here already:
> -obj-$(CONFIG_EARLY_PRINTK_DBGP) += early/
> +obj-$(CONFIG_USB_EARLY_PRINTK) += early/
> +obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o
> +static void __iomem * __init xdbc_map_pci_mmio(u32 bus, u32 dev, u32 func)
> +{
> + u32 val, sz;
> + u64 val64, sz64, mask64;
> + u8 byte;
> + void __iomem *base;
> +
> + val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, ~0);
> + sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0, val);
> + if (val == 0x || sz == 0x) {
> + pr_notice("invalid mmio bar\n");
> + return NULL;
> + }
> + if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
> + PCI_BASE_ADDRESS_MEM_TYPE_64) {
Please don't break the line here.
> + val = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, ~0);
> + sz = read_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4);
> + write_pci_config(bus, dev, func, PCI_BASE_ADDRESS_0 + 4, val);
> +
> + val64 |= ((u64)val << 32);
> + sz64 |= ((u64)sz << 32);
> + mask64 |= ((u64)~0 << 32);
Unnecessary parentheses.
> + }
> +
> + sz64 &= mask64;
> +
> + if (sizeof(dma_a
[PATCH v5 1/4] usb: dbc: early driver for xhci debug capability
xHCI debug capability (DbC) is an optional but standalone functionality provided by an xHCI host controller. Software learns this capability by walking through the extended capability list of the host. xHCI specification describes DbC in section 7.6. This patch introduces the code to probe and initialize the debug capability hardware during early boot. With hardware initialized, the debug target (system on which this code is running) will present a debug device through the debug port (normally the first USB3 port). The debug device is fully compliant with the USB framework and provides the equivalent of a very high performance (USB3) full-duplex serial link between the debug host and target. The DbC functionality is independent of xHCI host. There isn't any precondition from xHCI host side for DbC to work. This patch also includes bulk out and bulk in interfaces. These interfaces could be used to implement early printk bootconsole or hook to various system debuggers. This code is designed to be only used for kernel debugging when machine crashes very early before the console code is initialized. For normal operation it is not recommended. Cc: Mathias Nyman Signed-off-by: Lu Baolu --- arch/x86/Kconfig.debug| 14 + drivers/usb/Kconfig |3 + drivers/usb/Makefile |2 +- drivers/usb/early/Makefile|1 + drivers/usb/early/xhci-dbc.c | 1068 + drivers/usb/early/xhci-dbc.h | 205 include/linux/usb/xhci-dbgp.h | 22 + 7 files changed, 1314 insertions(+), 1 deletion(-) create mode 100644 drivers/usb/early/xhci-dbc.c create mode 100644 drivers/usb/early/xhci-dbc.h create mode 100644 include/linux/usb/xhci-dbgp.h diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug index 67eec55..13e85b7 100644 --- a/arch/x86/Kconfig.debug +++ b/arch/x86/Kconfig.debug @@ -29,6 +29,7 @@ config EARLY_PRINTK config EARLY_PRINTK_DBGP bool "Early printk via EHCI debug port" depends on EARLY_PRINTK && PCI + select USB_EARLY_PRINTK ---help--- Write kernel log output directly into the EHCI debug port. @@ -48,6 +49,19 @@ config EARLY_PRINTK_EFI This is useful for kernel debugging when your machine crashes very early before the console code is initialized. +config EARLY_PRINTK_XDBC + bool "Early printk via xHCI debug port" + depends on EARLY_PRINTK && PCI + select USB_EARLY_PRINTK + ---help--- + Write kernel log output directly into the xHCI debug port. + + This is useful for kernel debugging when your machine crashes very + early before the console code is initialized. For normal operation + it is not recommended because it looks ugly and doesn't cooperate + with klogd/syslogd or the X server. You should normally N here, + unless you want to debug such a crash. + config X86_PTDUMP_CORE def_bool n diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig index fbe493d..9313fff 100644 --- a/drivers/usb/Kconfig +++ b/drivers/usb/Kconfig @@ -19,6 +19,9 @@ config USB_EHCI_BIG_ENDIAN_MMIO config USB_EHCI_BIG_ENDIAN_DESC bool +config USB_EARLY_PRINTK + bool + menuconfig USB_SUPPORT bool "USB support" depends on HAS_IOMEM diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile index 7791af6..0c37838 100644 --- a/drivers/usb/Makefile +++ b/drivers/usb/Makefile @@ -49,7 +49,7 @@ obj-$(CONFIG_USB_MICROTEK)+= image/ obj-$(CONFIG_USB_SERIAL) += serial/ obj-$(CONFIG_USB) += misc/ -obj-$(CONFIG_EARLY_PRINTK_DBGP)+= early/ +obj-$(CONFIG_USB_EARLY_PRINTK) += early/ obj-$(CONFIG_USB_ATM) += atm/ obj-$(CONFIG_USB_SPEEDTOUCH) += atm/ diff --git a/drivers/usb/early/Makefile b/drivers/usb/early/Makefile index 24bbe51..2db5906 100644 --- a/drivers/usb/early/Makefile +++ b/drivers/usb/early/Makefile @@ -3,3 +3,4 @@ # obj-$(CONFIG_EARLY_PRINTK_DBGP) += ehci-dbgp.o +obj-$(CONFIG_EARLY_PRINTK_XDBC) += xhci-dbc.o diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c new file mode 100644 index 000..5ac4223 --- /dev/null +++ b/drivers/usb/early/xhci-dbc.c @@ -0,0 +1,1068 @@ +/** + * xhci-dbc.c - xHCI debug capability early driver + * + * Copyright (C) 2016 Intel Corporation + * + * Author: Lu Baolu + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#define pr_fmt(fmt)KBUILD_MODNAME ":%s: " fmt, __func__ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "../host/xhci.h" +#include "xhci-dbc.h" + +static struct xdbc_state xdbc; +static int early_console_keep; + +#ifdef XDBC_TRACE +#definexdbc_trace trace_printk +#else +static inline void xdbc_trace(
