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. Thanks, Thinh
