diff mbox series

usb: dwc3: add power down scale setting

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

Commit Message

Jun Li May 27, 2022, 9:03 a.m. UTC
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(+)

Comments

Thinh Nguyen May 28, 2022, 1:05 a.m. UTC | #1
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 May 28, 2022, 1:45 a.m. UTC | #2
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
Jun Li (OSS) May 30, 2022, 9:09 a.m. UTC | #3
> -----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
Jun Li (OSS) May 30, 2022, 9:43 a.m. UTC | #4
> -----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
Thinh Nguyen May 31, 2022, 4:47 p.m. UTC | #5
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
Jun Li (OSS) June 1, 2022, 2:14 a.m. UTC | #6
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
Thinh Nguyen June 1, 2022, 6:11 p.m. UTC | #7
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
Jun Li (OSS) June 6, 2022, 8:53 a.m. UTC | #8
> -----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 mbox series

Patch

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)