> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> > Sent: Thursday, June 2, 2022 2:12 AM > To: Jun Li (OSS) <jun.li@xxxxxxxxxxx>; Thinh Nguyen > <Thinh.Nguyen@xxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; > balbi@xxxxxxxxxx > Cc: linux-usb@xxxxxxxxxxxxxxx; J.D. Yue <jindong.yue@xxxxxxx> > Subject: Re: [PATCH] usb: dwc3: add power down scale setting > > Jun Li (OSS) wrote: > > Hi, > > > >> -----Original Message----- > >> From: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> > >> Sent: Wednesday, June 1, 2022 12:48 AM > >> To: Jun Li (OSS) <jun.li@xxxxxxxxxxx>; Thinh Nguyen > >> <Thinh.Nguyen@xxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; > >> balbi@xxxxxxxxxx > >> Cc: linux-usb@xxxxxxxxxxxxxxx; J.D. Yue <jindong.yue@xxxxxxx> > >> Subject: Re: [PATCH] usb: dwc3: add power down scale setting > >> > >> Jun Li (OSS) wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> > >>>> Sent: Saturday, May 28, 2022 9:46 AM > >>>> To: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; > >>>> balbi@xxxxxxxxxx > >>>> Cc: linux-usb@xxxxxxxxxxxxxxx; J.D. Yue <jindong.yue@xxxxxxx> > >>>> Subject: Re: [PATCH] usb: dwc3: add power down scale setting > >>>> > >>>> Thinh Nguyen wrote: > >>>>> Li Jun wrote: > >>>>>> Some SoC(e.g NXP imx8MQ) may have a wrong default power down > >>>>>> scale setting so need init it to be the correct value, the power > >>>>>> down scale setting description in DWC3 databook: > >>>>>> > >>>>>> Power Down Scale (PwrDnScale) > >>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock > >>>>>> source to a small part of the USB3 core that operates when the SS > >>>>>> PHY is in its lowest power (P3) state, and therefore does not provide > a clock. > >>>>>> The Power Down Scale field specifies how many suspend_clk periods > >>>>>> fit into a 16 kHz clock period. When performing the division, > >>>>>> round up the remainder. > >>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz > >>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 > >>>>>> (rounder up) > >>>>>> > >>>>>> So use the suspend clock rate to calculate it. > >>>> > >>>> Also, shouldn't the value selected be the max_rate of the suspend > >>>> clock and not just the value from clk_get_rate()? > >>> > >>> In my case, the suspend_clk is fixed, seems max rate is only Used by > >>> clock provider and there is no API to get max_rate for clock users. > >>> > >> > >> Seems like only the user/designer of the device knows the max rate > >> for the suspend_clk if the clock frequency varies. We may not want to > >> inadvertently overwrite the correct/tested default value for all > >> devices. Maybe we also need a new device property to inform the > >> controller of the power down scale value and give the user an option > >> to override the calculated value here. > > > > Understood, considering this is a rare case(wrong default value), and > > DT maintainer Rob does not like continue expand dwc3 huge property > > list > > Last I check, Rob does not like expanding quirks in dwc3 only (but I could > be wrong here). I don't think this is a quirk as nothing is really broken. > It's just something the user needs to inform the controller. > > > for this kind of soc level thing, instead, propose implies by > > compatible, do you think is it acceptable to use compatible check like > below for this? > > > > if (of_device_is_compatible(node, "fsl,imx8mq-dwc3")) > > > > I don't think we should do that. This is a common calculation for dwc3x > controller and not unique to just your platform. > > How about this: check the default setting to see if it makes sense before > overwriting it. That is, only overwrite it if the default value of > GCTL.PWRDNSCALE is > > * Less than the calculated value from clk_get_rate() > * Ridiculously high that's (maybe 3x) more than the calculated value > > Let me know what you think. This makes sense to me, I will send out v2. Thanks Li Jun > > Thanks, > Thinh
