| Message ID | 1653642195-23889-1-git-send-email-jun.li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: add power down scale setting | expand |
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. > > Signed-off-by: Li Jun <jun.li@nxp.com> > --- > drivers/usb/dwc3/core.c | 22 ++++++++++++++++++++++ > drivers/usb/dwc3/core.h | 1 + > 2 files changed, 23 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index e027c0420dc3..16d441dbc28b 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1029,6 +1029,25 @@ static void dwc3_set_incr_burst_type(struct dwc3 *dwc) > dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg); > } > > +static void dwc3_set_power_down_clk_scale(struct dwc3 *dwc) > +{ > + u32 reg, scale; Can we declare these variables in separate lines? Preferably reverse Christmas tree style. > + > + if (!dwc->susp_clk) > + return; > + > + /* > + * The power down scale field specifies how many suspend_clk > + * periods fit into a 16KHz clock period. When performing > + * the division, round up the remainder. > + */ > + scale = DIV_ROUND_UP(clk_get_rate(dwc->susp_clk), 16384); 16kHz == 16000Hz right? > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > + reg &= ~(DWC3_GCTL_PWRDNSCALE_MASK); > + reg |= DWC3_GCTL_PWRDNSCALE(scale); > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > +} > + > /** > * dwc3_core_init - Low-level initialization of DWC3 Core > * @dwc: Pointer to our controller context structure > @@ -1105,6 +1124,9 @@ static int dwc3_core_init(struct dwc3 *dwc) > if (ret) > goto err1; > > + /* Set power down scale of suspend_clk */ > + dwc3_set_power_down_clk_scale(dwc); > + > /* Adjust Frame Length */ > dwc3_frame_length_adjustment(dwc); > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 81c486b3941c..722808d8c0af 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -231,6 +231,7 @@ > > /* Global Configuration Register */ > #define DWC3_GCTL_PWRDNSCALE(n) ((n) << 19) > +#define DWC3_GCTL_PWRDNSCALE_MASK GENMASK(31, 19) > #define DWC3_GCTL_U2RSTECN BIT(16) > #define DWC3_GCTL_RAMCLKSEL(x) (((x) & DWC3_GCTL_CLK_MASK) << 6) > #define DWC3_GCTL_CLK_BUS (0) Thanks, Thinh
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()? >> >> Signed-off-by: Li Jun <jun.li@nxp.com> >> --- >> drivers/usb/dwc3/core.c | 22 ++++++++++++++++++++++ >> drivers/usb/dwc3/core.h | 1 + >> 2 files changed, 23 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index e027c0420dc3..16d441dbc28b 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1029,6 +1029,25 @@ static void dwc3_set_incr_burst_type(struct dwc3 *dwc) >> dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg); >> } >> >> +static void dwc3_set_power_down_clk_scale(struct dwc3 *dwc) >> +{ >> + u32 reg, scale; > > Can we declare these variables in separate lines? Preferably reverse > Christmas tree style. > >> + >> + if (!dwc->susp_clk) >> + return; >> + >> + /* >> + * The power down scale field specifies how many suspend_clk >> + * periods fit into a 16KHz clock period. When performing >> + * the division, round up the remainder. >> + */ >> + scale = DIV_ROUND_UP(clk_get_rate(dwc->susp_clk), 16384); > > 16kHz == 16000Hz right? > >> + reg = dwc3_readl(dwc->regs, DWC3_GCTL); >> + reg &= ~(DWC3_GCTL_PWRDNSCALE_MASK); >> + reg |= DWC3_GCTL_PWRDNSCALE(scale); >> + dwc3_writel(dwc->regs, DWC3_GCTL, reg); >> +} >> + >> /** >> * dwc3_core_init - Low-level initialization of DWC3 Core >> * @dwc: Pointer to our controller context structure >> @@ -1105,6 +1124,9 @@ static int dwc3_core_init(struct dwc3 *dwc) >> if (ret) >> goto err1; >> >> + /* Set power down scale of suspend_clk */ >> + dwc3_set_power_down_clk_scale(dwc); >> + >> /* Adjust Frame Length */ >> dwc3_frame_length_adjustment(dwc); >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 81c486b3941c..722808d8c0af 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -231,6 +231,7 @@ >> >> /* Global Configuration Register */ >> #define DWC3_GCTL_PWRDNSCALE(n) ((n) << 19) >> +#define DWC3_GCTL_PWRDNSCALE_MASK GENMASK(31, 19) >> #define DWC3_GCTL_U2RSTECN BIT(16) >> #define DWC3_GCTL_RAMCLKSEL(x) (((x) & DWC3_GCTL_CLK_MASK) << 6) >> #define DWC3_GCTL_CLK_BUS (0) > BR, Thinh
> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: Saturday, May 28, 2022 9:06 AM > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; balbi@kernel.org > Cc: linux-usb@vger.kernel.org; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; > J.D. Yue <jindong.yue@nxp.com> > Subject: Re: [PATCH] usb: dwc3: add power down scale setting > > 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. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/usb/dwc3/core.c | 22 ++++++++++++++++++++++ > > drivers/usb/dwc3/core.h | 1 + > > 2 files changed, 23 insertions(+) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index e027c0420dc3..16d441dbc28b 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1029,6 +1029,25 @@ static void dwc3_set_incr_burst_type(struct dwc3 > *dwc) > > dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg); > > } > > > > +static void dwc3_set_power_down_clk_scale(struct dwc3 *dwc) > > +{ > > + u32 reg, scale; > > Can we declare these variables in separate lines? Preferably reverse > Christmas tree style. Okay, will update in v2. > > > + > > + if (!dwc->susp_clk) > > + return; > > + > > + /* > > + * The power down scale field specifies how many suspend_clk > > + * periods fit into a 16KHz clock period. When performing > > + * the division, round up the remainder. > > + */ > > + scale = DIV_ROUND_UP(clk_get_rate(dwc->susp_clk), 16384); > > 16kHz == 16000Hz right? yes, I misunderstood it, will update in v2. Thanks Li Jun > > > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > > + reg &= ~(DWC3_GCTL_PWRDNSCALE_MASK); > > + reg |= DWC3_GCTL_PWRDNSCALE(scale); > > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > > +} > > + > > /** > > * dwc3_core_init - Low-level initialization of DWC3 Core > > * @dwc: Pointer to our controller context structure > > @@ -1105,6 +1124,9 @@ static int dwc3_core_init(struct dwc3 *dwc) > > if (ret) > > goto err1; > > > > + /* Set power down scale of suspend_clk */ > > + dwc3_set_power_down_clk_scale(dwc); > > + > > /* Adjust Frame Length */ > > dwc3_frame_length_adjustment(dwc); > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index 81c486b3941c..722808d8c0af 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -231,6 +231,7 @@ > > > > /* Global Configuration Register */ > > #define DWC3_GCTL_PWRDNSCALE(n) ((n) << 19) > > +#define DWC3_GCTL_PWRDNSCALE_MASK GENMASK(31, 19) > > #define DWC3_GCTL_U2RSTECN BIT(16) > > #define DWC3_GCTL_RAMCLKSEL(x) (((x) & DWC3_GCTL_CLK_MASK) << 6) > > #define DWC3_GCTL_CLK_BUS (0) > > Thanks, > Thinh
> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: Saturday, May 28, 2022 9:46 AM > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; balbi@kernel.org > Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> > 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. Thanks Li Jun > > >> > >> Signed-off-by: Li Jun <jun.li@nxp.com> > >> --- > >> drivers/usb/dwc3/core.c | 22 ++++++++++++++++++++++ > >> drivers/usb/dwc3/core.h | 1 + > >> 2 files changed, 23 insertions(+) > >> > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >> index e027c0420dc3..16d441dbc28b 100644 > >> --- a/drivers/usb/dwc3/core.c > >> +++ b/drivers/usb/dwc3/core.c > >> @@ -1029,6 +1029,25 @@ static void dwc3_set_incr_burst_type(struct dwc3 > *dwc) > >> dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg); > >> } > >> > >> +static void dwc3_set_power_down_clk_scale(struct dwc3 *dwc) > >> +{ > >> + u32 reg, scale; > > > > Can we declare these variables in separate lines? Preferably reverse > > Christmas tree style. > > > >> + > >> + if (!dwc->susp_clk) > >> + return; > >> + > >> + /* > >> + * The power down scale field specifies how many suspend_clk > >> + * periods fit into a 16KHz clock period. When performing > >> + * the division, round up the remainder. > >> + */ > >> + scale = DIV_ROUND_UP(clk_get_rate(dwc->susp_clk), 16384); > > > > 16kHz == 16000Hz right? > > > >> + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > >> + reg &= ~(DWC3_GCTL_PWRDNSCALE_MASK); > >> + reg |= DWC3_GCTL_PWRDNSCALE(scale); > >> + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > >> +} > >> + > >> /** > >> * dwc3_core_init - Low-level initialization of DWC3 Core > >> * @dwc: Pointer to our controller context structure > >> @@ -1105,6 +1124,9 @@ static int dwc3_core_init(struct dwc3 *dwc) > >> if (ret) > >> goto err1; > >> > >> + /* Set power down scale of suspend_clk */ > >> + dwc3_set_power_down_clk_scale(dwc); > >> + > >> /* Adjust Frame Length */ > >> dwc3_frame_length_adjustment(dwc); > >> > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >> index 81c486b3941c..722808d8c0af 100644 > >> --- a/drivers/usb/dwc3/core.h > >> +++ b/drivers/usb/dwc3/core.h > >> @@ -231,6 +231,7 @@ > >> > >> /* Global Configuration Register */ > >> #define DWC3_GCTL_PWRDNSCALE(n) ((n) << 19) > >> +#define DWC3_GCTL_PWRDNSCALE_MASK GENMASK(31, 19) > >> #define DWC3_GCTL_U2RSTECN BIT(16) > >> #define DWC3_GCTL_RAMCLKSEL(x) (((x) & DWC3_GCTL_CLK_MASK) << 6) > >> #define DWC3_GCTL_CLK_BUS (0) > > > > BR, > Thinh
Jun Li (OSS) wrote: > > >> -----Original Message----- >> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Sent: Saturday, May 28, 2022 9:46 AM >> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; balbi@kernel.org >> Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> >> 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. Thanks, Thinh
Hi, > -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: Wednesday, June 1, 2022 12:48 AM > To: Jun Li (OSS) <jun.li@oss.nxp.com>; Thinh Nguyen > <Thinh.Nguyen@synopsys.com>; gregkh@linuxfoundation.org; > balbi@kernel.org > Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> > Subject: Re: [PATCH] usb: dwc3: add power down scale setting > > Jun Li (OSS) wrote: > > > > > >> -----Original Message----- > >> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > >> Sent: Saturday, May 28, 2022 9:46 AM > >> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; > >> balbi@kernel.org > >> Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> > >> 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 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")) thanks Li Jun > > Thanks, > Thinh
Jun Li (OSS) wrote: > Hi, > >> -----Original Message----- >> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Sent: Wednesday, June 1, 2022 12:48 AM >> To: Jun Li (OSS) <jun.li@oss.nxp.com>; Thinh Nguyen >> <Thinh.Nguyen@synopsys.com>; gregkh@linuxfoundation.org; >> balbi@kernel.org >> Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> >> Subject: Re: [PATCH] usb: dwc3: add power down scale setting >> >> Jun Li (OSS) wrote: >>> >>> >>>> -----Original Message----- >>>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>>> Sent: Saturday, May 28, 2022 9:46 AM >>>> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; >>>> balbi@kernel.org >>>> Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> >>>> 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
> -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: Thursday, June 2, 2022 2:12 AM > To: Jun Li (OSS) <jun.li@oss.nxp.com>; Thinh Nguyen > <Thinh.Nguyen@synopsys.com>; gregkh@linuxfoundation.org; > balbi@kernel.org > Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> > Subject: Re: [PATCH] usb: dwc3: add power down scale setting > > Jun Li (OSS) wrote: > > Hi, > > > >> -----Original Message----- > >> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > >> Sent: Wednesday, June 1, 2022 12:48 AM > >> To: Jun Li (OSS) <jun.li@oss.nxp.com>; Thinh Nguyen > >> <Thinh.Nguyen@synopsys.com>; gregkh@linuxfoundation.org; > >> balbi@kernel.org > >> Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> > >> Subject: Re: [PATCH] usb: dwc3: add power down scale setting > >> > >> Jun Li (OSS) wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > >>>> Sent: Saturday, May 28, 2022 9:46 AM > >>>> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org; > >>>> balbi@kernel.org > >>>> Cc: linux-usb@vger.kernel.org; J.D. Yue <jindong.yue@nxp.com> > >>>> 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
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index e027c0420dc3..16d441dbc28b 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1029,6 +1029,25 @@ static void dwc3_set_incr_burst_type(struct dwc3 *dwc) dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg); } +static void dwc3_set_power_down_clk_scale(struct dwc3 *dwc) +{ + u32 reg, scale; + + if (!dwc->susp_clk) + return; + + /* + * The power down scale field specifies how many suspend_clk + * periods fit into a 16KHz clock period. When performing + * the division, round up the remainder. + */ + scale = DIV_ROUND_UP(clk_get_rate(dwc->susp_clk), 16384); + reg = dwc3_readl(dwc->regs, DWC3_GCTL); + reg &= ~(DWC3_GCTL_PWRDNSCALE_MASK); + reg |= DWC3_GCTL_PWRDNSCALE(scale); + dwc3_writel(dwc->regs, DWC3_GCTL, reg); +} + /** * dwc3_core_init - Low-level initialization of DWC3 Core * @dwc: Pointer to our controller context structure @@ -1105,6 +1124,9 @@ static int dwc3_core_init(struct dwc3 *dwc) if (ret) goto err1; + /* Set power down scale of suspend_clk */ + dwc3_set_power_down_clk_scale(dwc); + /* Adjust Frame Length */ dwc3_frame_length_adjustment(dwc); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 81c486b3941c..722808d8c0af 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -231,6 +231,7 @@ /* Global Configuration Register */ #define DWC3_GCTL_PWRDNSCALE(n) ((n) << 19) +#define DWC3_GCTL_PWRDNSCALE_MASK GENMASK(31, 19) #define DWC3_GCTL_U2RSTECN BIT(16) #define DWC3_GCTL_RAMCLKSEL(x) (((x) & DWC3_GCTL_CLK_MASK) << 6) #define DWC3_GCTL_CLK_BUS (0)
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. Signed-off-by: Li Jun <jun.li@nxp.com> --- drivers/usb/dwc3/core.c | 22 ++++++++++++++++++++++ drivers/usb/dwc3/core.h | 1 + 2 files changed, 23 insertions(+)