diff mbox series

[v3,5/7] usb: dwc3: Add snps,ref-clock-frequency-hz property for ACPI

Message ID 20220127200636.1456175-6-sean.anderson@seco.com
State New
Headers show
Series usb: dwc3: Calculate REFCLKPER et. al. from reference clock | expand

Commit Message

Sean Anderson Jan. 27, 2022, 8:06 p.m. UTC
This property allows setting the reference clock frequency properly for
ACPI-based systems. It is not documented under dt-bindings, since it is
not intended for use on DT-based systems. DT-based systems should use
the clocks property instead.

Frequency is preferred over period since it has greater precision when
used in calculations.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

(no changes since v2)

Changes in v2:
- New

 drivers/usb/dwc3/core.c | 6 ++++--
 drivers/usb/dwc3/core.h | 4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

Comments

Thinh Nguyen Feb. 3, 2022, 3:11 a.m. UTC | #1
Sean Anderson wrote:
> This property allows setting the reference clock frequency properly for
> ACPI-based systems. It is not documented under dt-bindings, since it is
> not intended for use on DT-based systems. DT-based systems should use
> the clocks property instead.
> 
> Frequency is preferred over period since it has greater precision when
> used in calculations.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - New
> 
>  drivers/usb/dwc3/core.c | 6 ++++--
>  drivers/usb/dwc3/core.h | 4 +++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 18adddfba3da..c1b045121672 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -353,8 +353,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>  	unsigned long rate;
>  	u32 reg;
>  
> -	if (dwc->ref_clk) {
> -		rate = clk_get_rate(dwc->ref_clk);
> +	if (dwc->ref_clk || dwc->ref_clk_freq) {
> +		rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>  		if (!rate)
>  			return;
>  		period = NSEC_PER_SEC / rate;
> @@ -1497,6 +1497,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  				 &dwc->fladj);
>  	device_property_read_u32(dev, "snps,ref-clock-period-ns",
>  				 &dwc->ref_clk_per);
> +	device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
> +				 &dwc->ref_clk_freq);
>  
>  	dwc->dis_metastability_quirk = device_property_read_bool(dev,
>  				"snps,dis_metastability_quirk");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index eb9c1efced05..00a792459fec 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -988,7 +988,8 @@ struct dwc3_scratchpad_array {
>   * @regs: base address for our registers
>   * @regs_size: address space size
>   * @fladj: frame length adjustment
> - * @ref_clk_per: reference clock period configuration
> + * @ref_clk_per: reference clock period; deprecated in favor of @ref_clk_freq
> + * @ref_clk_freq: reference clock frequency to use if @ref_clk is missing
>   * @irq_gadget: peripheral controller's IRQ number
>   * @otg_irq: IRQ number for OTG IRQs
>   * @current_otg_role: current role of operation while using the OTG block
> @@ -1171,6 +1172,7 @@ struct dwc3 {
>  
>  	u32			fladj;
>  	u32			ref_clk_per;
> +	u32			ref_clk_freq;
>  	u32			irq_gadget;
>  	u32			otg_irq;
>  	u32			current_otg_role;

Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh
Sean Anderson Feb. 4, 2022, 4 p.m. UTC | #2
Hi Andy,

On 2/4/22 7:54 AM, Andy Shevchenko wrote:
> 
> 
> On Thursday, January 27, 2022, Sean Anderson <sean.anderson@seco.com <mailto:sean.anderson@seco.com>> wrote:
> 
>     This property allows setting the reference clock frequency properly for
>     ACPI-based systems. It is not documented under dt-bindings, since it is
>     not intended for use on DT-based systems. DT-based systems should use
>     the clocks property instead.
> 
>     Frequency is preferred over period since it has greater precision when
>     used in 
> 
> Is it function or interface clock? 
> 
> We have standard property for the functional clock rate, I.e. “clock-frequency” (in Hz), can it be used here?

I believe this is a "functional" clock. It it is a reference for the USB
signals. I'm not sure exactly what the purpose of this clock is, since I
do not have access to the databook for this IP. I considered using
"clock-frequency", but I am concerned about ambiguity because there is a
second "suspend" clock which is also a "functional" clock. The latter
clock appears to be used when the PHY is shut down (and not necessarily
corresponding to Linux's notion of a suspended device). If it is
necessary in the future to configure that clock on ACPI platforms (e.g.
to set GCTL.PWRDNSCALE) I think it is clear what the property name would
be (snps,susp-clock-frequency-hz).

--Sean

>     Signed-off-by: Sean Anderson <sean.anderson@seco.com <mailto:sean.anderson@seco.com>>
>     ---
> 
>     (no changes since v2)
> 
>     Changes in v2:
>     - New
> 
>      drivers/usb/dwc3/core.c | 6 ++++--
>      drivers/usb/dwc3/core.h | 4 +++-
>      2 files changed, 7 insertions(+), 3 deletions(-)
> 
>     diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>     index 18adddfba3da..c1b045121672 100644
>     --- a/drivers/usb/dwc3/core.c
>     +++ b/drivers/usb/dwc3/core.c
>     @@ -353,8 +353,8 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>             unsigned long rate;
>             u32 reg;
> 
>     -       if (dwc->ref_clk) {
>     -               rate = clk_get_rate(dwc->ref_clk);
>     +       if (dwc->ref_clk || dwc->ref_clk_freq) {
>     +               rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
>                     if (!rate)
>                             return;
>                     period = NSEC_PER_SEC / rate;
>     @@ -1497,6 +1497,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>                                      &dwc->fladj);
>             device_property_read_u32(dev, "snps,ref-clock-period-ns",
>                                      &dwc->ref_clk_per);
>     +       device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
>     +                                &dwc->ref_clk_freq);
> 
>             dwc->dis_metastability_quirk = device_property_read_bool(dev,
>                                     "snps,dis_metastability_quirk");
>     diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>     index eb9c1efced05..00a792459fec 100644
>     --- a/drivers/usb/dwc3/core.h
>     +++ b/drivers/usb/dwc3/core.h
>     @@ -988,7 +988,8 @@ struct dwc3_scratchpad_array {
>       * @regs: base address for our registers
>       * @regs_size: address space size
>       * @fladj: frame length adjustment
>     - * @ref_clk_per: reference clock period configuration
>     + * @ref_clk_per: reference clock period; deprecated in favor of @ref_clk_freq
>     + * @ref_clk_freq: reference clock frequency to use if @ref_clk is missing
>       * @irq_gadget: peripheral controller's IRQ number
>       * @otg_irq: IRQ number for OTG IRQs
>       * @current_otg_role: current role of operation while using the OTG block
>     @@ -1171,6 +1172,7 @@ struct dwc3 {
> 
>             u32                     fladj;
>             u32                     ref_clk_per;
>     +       u32                     ref_clk_freq;
>             u32                     irq_gadget;
>             u32                     otg_irq;
>             u32                     current_otg_role;
>     -- 
>     2.25.1
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Andy Shevchenko Feb. 4, 2022, 6:38 p.m. UTC | #3
On Fri, Feb 4, 2022 at 6:00 PM Sean Anderson <sean.anderson@seco.com> wrote:
> On 2/4/22 7:54 AM, Andy Shevchenko wrote:

...

> > On Thursday, January 27, 2022, Sean Anderson <sean.anderson@seco.com <mailto:sean.anderson@seco.com>> wrote:

> > Is it function or interface clock?
> >
> > We have standard property for the functional clock rate, I.e. “clock-frequency” (in Hz), can it be used here?
>
> I believe this is a "functional" clock. It it is a reference for the USB
> signals. I'm not sure exactly what the purpose of this clock is, since I
> do not have access to the databook for this IP. I considered using
> "clock-frequency", but I am concerned about ambiguity because there is a
> second "suspend" clock which is also a "functional" clock. The latter
> clock appears to be used when the PHY is shut down (and not necessarily
> corresponding to Linux's notion of a suspended device). If it is
> necessary in the future to configure that clock on ACPI platforms (e.g.
> to set GCTL.PWRDNSCALE) I think it is clear what the property name would
> be (snps,susp-clock-frequency-hz).

In order to have more or less unified APIs in the future I would
suggest using 'clock-frequency' for the "main" functional clock. For
example, 8250_dw uses it for the baud rate generator, while it also
utilizes auxiliary clock(s) on some platforms.
Sean Anderson Feb. 4, 2022, 6:44 p.m. UTC | #4
On 2/4/22 1:38 PM, Andy Shevchenko wrote:
> On Fri, Feb 4, 2022 at 6:00 PM Sean Anderson <sean.anderson@seco.com> wrote:
>> On 2/4/22 7:54 AM, Andy Shevchenko wrote:
> 
> ...
> 
>> > On Thursday, January 27, 2022, Sean Anderson <sean.anderson@seco.com <mailto:sean.anderson@seco.com>> wrote:
> 
>> > Is it function or interface clock?
>> >
>> > We have standard property for the functional clock rate, I.e. “clock-frequency” (in Hz), can it be used here?
>>
>> I believe this is a "functional" clock. It it is a reference for the USB
>> signals. I'm not sure exactly what the purpose of this clock is, since I
>> do not have access to the databook for this IP. I considered using
>> "clock-frequency", but I am concerned about ambiguity because there is a
>> second "suspend" clock which is also a "functional" clock. The latter
>> clock appears to be used when the PHY is shut down (and not necessarily
>> corresponding to Linux's notion of a suspended device). If it is
>> necessary in the future to configure that clock on ACPI platforms (e.g.
>> to set GCTL.PWRDNSCALE) I think it is clear what the property name would
>> be (snps,susp-clock-frequency-hz).
> 
> In order to have more or less unified APIs in the future I would
> suggest using 'clock-frequency' for the "main" functional clock. For
> example, 8250_dw uses it for the baud rate generator, while it also
> utilizes auxiliary clock(s) on some platforms.
> 

OK, I had a look though that driver, and it seems like it uses
clock-frequency only for the baudclk, and e.g. apb_pclk has no
corresponding frequency property. For this driver, it would mean that
the suspend clock would only be configurable through device tree. Is
that the approach you recommend?

--Sean
Andy Shevchenko Feb. 4, 2022, 7:24 p.m. UTC | #5
On Fri, Feb 4, 2022 at 8:44 PM Sean Anderson <sean.anderson@seco.com> wrote:
> On 2/4/22 1:38 PM, Andy Shevchenko wrote:
> > On Fri, Feb 4, 2022 at 6:00 PM Sean Anderson <sean.anderson@seco.com> wrote:
> >> On 2/4/22 7:54 AM, Andy Shevchenko wrote:
> >> > On Thursday, January 27, 2022, Sean Anderson <sean.anderson@seco.com <mailto:sean.anderson@seco.com>> wrote:

...

> > In order to have more or less unified APIs in the future I would
> > suggest using 'clock-frequency' for the "main" functional clock. For
> > example, 8250_dw uses it for the baud rate generator, while it also
> > utilizes auxiliary clock(s) on some platforms.
>
> OK, I had a look though that driver, and it seems like it uses
> clock-frequency only for the baudclk, and e.g. apb_pclk has no
> corresponding frequency property. For this driver, it would mean that
> the suspend clock would only be configurable through device tree. Is
> that the approach you recommend?


What I meant is to use the "clock-frequency" property as it is kinda
standard de facto for the "main'' functional clock. The rest is up to
the individual drivers. From the API perspective I would expect a
common helper in the future that takes clock name and returns rate
based on clock (if found) or "clock-frequency" property. We may also
extend in far future to support any combinations of the [clock name,
property name] to get a clock rated either via Device Tree or via
ACPI.
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 18adddfba3da..c1b045121672 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -353,8 +353,8 @@  static void dwc3_ref_clk_period(struct dwc3 *dwc)
 	unsigned long rate;
 	u32 reg;
 
-	if (dwc->ref_clk) {
-		rate = clk_get_rate(dwc->ref_clk);
+	if (dwc->ref_clk || dwc->ref_clk_freq) {
+		rate = clk_get_rate(dwc->ref_clk) ?: dwc->ref_clk_freq;
 		if (!rate)
 			return;
 		period = NSEC_PER_SEC / rate;
@@ -1497,6 +1497,8 @@  static void dwc3_get_properties(struct dwc3 *dwc)
 				 &dwc->fladj);
 	device_property_read_u32(dev, "snps,ref-clock-period-ns",
 				 &dwc->ref_clk_per);
+	device_property_read_u32(dev, "snps,ref-clock-frequency-hz",
+				 &dwc->ref_clk_freq);
 
 	dwc->dis_metastability_quirk = device_property_read_bool(dev,
 				"snps,dis_metastability_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index eb9c1efced05..00a792459fec 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -988,7 +988,8 @@  struct dwc3_scratchpad_array {
  * @regs: base address for our registers
  * @regs_size: address space size
  * @fladj: frame length adjustment
- * @ref_clk_per: reference clock period configuration
+ * @ref_clk_per: reference clock period; deprecated in favor of @ref_clk_freq
+ * @ref_clk_freq: reference clock frequency to use if @ref_clk is missing
  * @irq_gadget: peripheral controller's IRQ number
  * @otg_irq: IRQ number for OTG IRQs
  * @current_otg_role: current role of operation while using the OTG block
@@ -1171,6 +1172,7 @@  struct dwc3 {
 
 	u32			fladj;
 	u32			ref_clk_per;
+	u32			ref_clk_freq;
 	u32			irq_gadget;
 	u32			otg_irq;
 	u32			current_otg_role;