Re: [PATCH v5 1/4] usb: dbc: early driver for xhci debug capability

[Lu Baolu <baolu.lu@linux.intel.com>]

Hi Ingo,

On 01/24/2017 04:20 PM, Ingo Molnar wrote:
> * Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
>> Hi Ingo,
>>
>> On 01/22/2017 05:04 PM, Ingo Molnar wrote:
>>> * Lu Baolu <baolu.lu@linux.intel.com> 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