Hi Balbi,
If no other doubts, please help apply it.
Thanks,
Regards,
Yinbo Zhu.
-----Original Message-----
From: Felipe Balbi <ba...@kernel.org>
Sent: 2019年8月27日 19:55
To: Ran Wang <ran.wan...@nxp.com>; Yinbo Zhu <yinbo....@nxp.com>; Greg
Kroah-Hartman <gre...@linuxfoundation.org>; linux-usb@vger.kernel.org; open
list <linux-ker...@vger.kernel.org>
Cc: Xiaobo Xie <xiaobo....@nxp.com>; Jiafei Pan <jiafei....@nxp.com>
Subject: RE: [EXT] Re: [PATCH v1] usb: dwc3: remove the call trace of
USBx_GFLADJ
Hi,
Ran Wang <ran.wan...@nxp.com> writes:
>> Yinbo Zhu <yinbo....@nxp.com> writes:
>> >> Yinbo Zhu <yinbo....@nxp.com> writes:
>> >> >> > diff --git a/drivers/usb/dwc3/core.c
>> >> >> > b/drivers/usb/dwc3/core.c index
>> >> >> > 98bce85c29d0..a133d8490322 100644
>> >> >> > --- a/drivers/usb/dwc3/core.c
>> >> >> > +++ b/drivers/usb/dwc3/core.c
>> >> >> > @@ -300,8 +300,7 @@ static void
>> >> >> > dwc3_frame_length_adjustment(struct
>> >> >> > dwc3 *dwc)
>> >> >> >
>> >> >> > reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
>> >> >> > dft = reg & DWC3_GFLADJ_30MHZ_MASK;
>> >> >> > - if (!dev_WARN_ONCE(dwc->dev, dft == dwc->fladj,
>> >> >> > - "request value same as default, ignoring\n")) {
>> >> >> > + if (dft != dwc->fladj) {
>> >> >>
>> >> >> if the value isn't different, why do you want to change it?
>> >> >>
>> >> >> --
>> >> >> Balbi
>> >> > Hi Balbi,
>> >> >
>> >> > I don't change any value. I was remove that call trace.
>> >>
>> >> Sure you do. The splat only shows when you request a FLADJ value
>> >> that's the same as the one already in the register. The reason you
>> >> see the splat is because your requested value is what's already in the HW.
>> >>
>> >> So, again, why are you adding this device tree property if the
>> >> value is already the correct one?
>> >>
>> >> > In addition that GFLADJ_30MHZ value intial value is 0, and it's
>> >> > value must be 0x20, if not, usb will not work.
>> >>
>> >> it's not zero, otherwise the splat wouldn't trigger. You're
>> >> requesting the value that's already in your register by default.
>> >>
>> >> --
>> >> Balbi
>> >
>> > Hi Balbi,
>> >
>> > According that rm doc that GFLADJ_30MHZ has a default value is
>> > 0x20, when GFLADJ_30MHZ_REG_SEL is 0, this 0x20 is a hard-coded value.
>> >
>> > But in fact, that default value is 0, please you note!
>> >
>> > Then according that xhci spec 5.2.4, that register the sixth bit if
>> > is 0, then that can support Frame Lenth Timing value.
>> >
>> > So set GFLADJ_30MHZ_REG_SEL to 1 for use FLADJ, then I find that it
>> > must use 0x20 usb will work well, even thoug xhci can permit
>> > GFLADJ_30MHZ use other value
>>
>> You only get the splat because you try to sent GFLADJ to 0x20 and
>> it's ALREADY 0x20. This means that you don't need the property in DTS.
>>
>> > In addition about what you said is about dts patch, and that patch
>> > had merged by upstream, patch owner isn't me,
>>
>> Well, then remove the setting from DTS, since clearly it's not needed.
>
> Please considering below scenarios on the same board which needs GFLADJ
> property on kernel DTS:
>
> 1. Board boot to U-Boot first, then load kernel. In this case, we need kernel
> DTS
> help to get GFLADJ setting right, everything is as expected.
>
> 2. Board boot to U-Boot console, then execute 'usb start' under U-Boot
> console to init
> DWC3 controller, then load kernel. In this case, actually GFLADJ is
> correctly
> configured already, and the GFLADJ config double-checking is fine
> (because kernel
> cannot know if U-Boot has initialized it or not), but warning looks not
> necessary.
>
> 3. Board boot to kernel, GFLADJ get set from DTS, then system suspend &
> resume. In this case
> when resuming, GFLADJ setting has been restored correctly, so here we
> might not need
> send out the warning message (double-checking might be fine).
>
> So, what's your suggestion to remove this looks non-necessary warning message?
now this is well explained! So the value in the register is *NOT* 0x20 by
default, however, u-boot _can_ use dwc3 if we're flashing, then it'll result in
the splat.
Okay, this is a valid scenario that the kernel should consider. I agree that we
should remove the WARN() from there.
Thanks
--
balbi
