[PATCH] usb: dwc3: reference clock configuration

[PATCH] usb: dwc3: reference clock configuration

[Baruch Siach]

From: Balaji Prakash J <bjagadee@codeaurora.org>

DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
to control the behavior of controller with respect to SOF and ITP.
The reset values of these registers are aligned for 19.2 MHz
reference clock source. This change will add option to override
these settings for reference clock other than 19.2 MHz

Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.

Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
[ baruch: mention tested hardware ]
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 .../devicetree/bindings/usb/dwc3.txt          |  5 ++
 drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
 drivers/usb/dwc3/core.h                       | 12 +++++
 3 files changed, 69 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 1aae2b6160c1..4ffa87b697dc 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -89,6 +89,11 @@ Optional properties:
  - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
 	register for post-silicon frame length adjustment when the
 	fladj_30mhz_sdbnd signal is invalid or incorrect.
+ - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields of GFLADJ
+	register for reference clock other than 19.2 MHz is used.
+ - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL. This field
+	indicates in terms of nano seconds the period of ref_clk. To calculate the
+	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
  - snps,rx-thr-num-pkt-prd: periodic ESS RX packet threshold count - host mode
 			only. Set this and rx-max-burst-prd to a valid,
 			non-zero value 1-16 (DWC_usb31 programming guide
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 841daec70b6e..85e40ec8e23b 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -325,6 +325,48 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
 	}
 }
 
+/**
+ * dwc3_ref_clk_adjustment - Reference clock settings for SOF and ITP
+ *		Default reference clock configurations are calculated assuming
+ *		19.2 MHz clock source. For other clock source, this will set
+ *		configuration in DWC3_GFLADJ register
+ * @dwc: Pointer to our controller context structure
+ */
+static void dwc3_ref_clk_adjustment(struct dwc3 *dwc)
+{
+	u32 reg;
+
+	if (dwc->ref_clk_adj == 0)
+		return;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
+	reg &= ~DWC3_GFLADJ_REFCLK_MASK;
+	reg |=  (dwc->ref_clk_adj << DWC3_GFLADJ_REFCLK_SEL);
+	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
+}
+
+/**
+ * dwc3_ref_clk_period - Reference clock period configuration
+ *		Default reference clock period is calculated assuming
+ *		19.2 MHz as clock source. For other clock source, this
+ *		will set clock period in DWC3_GUCTL register
+ * @dwc: Pointer to our controller context structure
+ * @ref_clk_per: reference clock period in ns
+ */
+static void dwc3_ref_clk_period(struct dwc3 *dwc)
+{
+	u32 reg;
+
+	if (dwc->ref_clk_per == 0)
+		return;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
+	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
+	reg |=  (dwc->ref_clk_per << DWC3_GUCTL_REFCLKPER_SEL);
+	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
+}
+
+
 /**
  * dwc3_free_one_event_buffer - Frees one event buffer
  * @dwc: Pointer to our controller context structure
@@ -982,6 +1024,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	/* Adjust Frame Length */
 	dwc3_frame_length_adjustment(dwc);
 
+	/* Adjust Reference Clock Settings */
+	dwc3_ref_clk_adjustment(dwc);
+
+	/* Adjust Reference Clock Period */
+	dwc3_ref_clk_period(dwc);
+
 	dwc3_set_incr_burst_type(dwc);
 
 	usb_phy_set_suspend(dwc->usb2_phy, 0);
@@ -1351,6 +1399,10 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				    &dwc->hsphy_interface);
 	device_property_read_u32(dev, "snps,quirk-frame-length-adjustment",
 				 &dwc->fladj);
+	device_property_read_u32(dev, "snps,quirk-ref-clock-adjustment",
+				 &dwc->ref_clk_adj);
+	device_property_read_u32(dev, "snps,quirk-ref-clock-period",
+				 &dwc->ref_clk_per);
 
 	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 1b241f937d8f..469e94512414 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -379,6 +379,14 @@
 #define DWC3_GFLADJ_30MHZ_SDBND_SEL		BIT(7)
 #define DWC3_GFLADJ_30MHZ_MASK			0x3f
 
+/* Global User Control Register*/
+#define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
+#define DWC3_GUCTL_REFCLKPER_SEL		22
+
+/* Global reference clock Adjustment Register */
+#define DWC3_GFLADJ_REFCLK_MASK			0xffffff00
+#define DWC3_GFLADJ_REFCLK_SEL			8
+
 /* Global User Control Register 2 */
 #define DWC3_GUCTL2_RST_ACTBITLATER		BIT(14)
 
@@ -956,6 +964,8 @@ struct dwc3_scratchpad_array {
  * @regs: base address for our registers
  * @regs_size: address space size
  * @fladj: frame length adjustment
+ * @ref_clk_adj: reference clock adjustment
+ * @ref_clk_per: reference clock period configuration
  * @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
@@ -1118,6 +1128,8 @@ struct dwc3 {
 	enum usb_dr_mode	role_switch_default_mode;
 
 	u32			fladj;
+	u32			ref_clk_adj;
+	u32			ref_clk_per;
 	u32			irq_gadget;
 	u32			otg_irq;
 	u32			current_otg_role;
-- 
2.30.0

Re: [PATCH] usb: dwc3: reference clock configuration

[Greg KH]

On Mon, Feb 08, 2021 at 08:00:06AM +0200, Baruch Siach wrote:
> From: Balaji Prakash J <bjagadee@codeaurora.org>
> 
> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
> to control the behavior of controller with respect to SOF and ITP.
> The reset values of these registers are aligned for 19.2 MHz
> reference clock source. This change will add option to override
> these settings for reference clock other than 19.2 MHz
> 
> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
> 
> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
> [ baruch: mention tested hardware ]
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++

Bindings should be split into a separate patch (1/2) so that the DT
maintainers can review it easier.

Also, always run checkpatch on your submissions before sending them out.

thanks,

greg k-h

Re: [PATCH] usb: dwc3: reference clock configuration

[Bjorn Andersson]

On Mon 08 Feb 00:00 CST 2021, Baruch Siach wrote:

> From: Balaji Prakash J <bjagadee@codeaurora.org>
> 
> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
> to control the behavior of controller with respect to SOF and ITP.
> The reset values of these registers are aligned for 19.2 MHz
> reference clock source. This change will add option to override
> these settings for reference clock other than 19.2 MHz
> 
> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
> 
> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
> [ baruch: mention tested hardware ]
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
>  drivers/usb/dwc3/core.h                       | 12 +++++
>  3 files changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 1aae2b6160c1..4ffa87b697dc 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -89,6 +89,11 @@ Optional properties:
>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>  	register for post-silicon frame length adjustment when the
>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields of GFLADJ
> +	register for reference clock other than 19.2 MHz is used.

What are typical values for this property? What unit does it have? How
does it actually relate to the frequency of the reference clock?

> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL. This field
> +	indicates in terms of nano seconds the period of ref_clk. To calculate the
> +	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.

Can't we make the dwc3 reference this clock and use clk_get_rate() and
then do this math in the driver?

>   - snps,rx-thr-num-pkt-prd: periodic ESS RX packet threshold count - host mode
>  			only. Set this and rx-max-burst-prd to a valid,
>  			non-zero value 1-16 (DWC_usb31 programming guide
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 841daec70b6e..85e40ec8e23b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -325,6 +325,48 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>  	}
>  }
>  
> +/**
> + * dwc3_ref_clk_adjustment - Reference clock settings for SOF and ITP
> + *		Default reference clock configurations are calculated assuming
> + *		19.2 MHz clock source. For other clock source, this will set
> + *		configuration in DWC3_GFLADJ register
> + * @dwc: Pointer to our controller context structure
> + */
> +static void dwc3_ref_clk_adjustment(struct dwc3 *dwc)
> +{
> +	u32 reg;
> +
> +	if (dwc->ref_clk_adj == 0)
> +		return;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> +	reg &= ~DWC3_GFLADJ_REFCLK_MASK;
> +	reg |=  (dwc->ref_clk_adj << DWC3_GFLADJ_REFCLK_SEL);

	reg = FIELD_SET(DWC3_GFLADJ_REFCLK_MASK, adj, reg);

Regards,
Bjorn

> +	dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> +}
> +
> +/**
> + * dwc3_ref_clk_period - Reference clock period configuration
> + *		Default reference clock period is calculated assuming
> + *		19.2 MHz as clock source. For other clock source, this
> + *		will set clock period in DWC3_GUCTL register
> + * @dwc: Pointer to our controller context structure
> + * @ref_clk_per: reference clock period in ns
> + */
> +static void dwc3_ref_clk_period(struct dwc3 *dwc)
> +{
> +	u32 reg;
> +
> +	if (dwc->ref_clk_per == 0)
> +		return;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
> +	reg &= ~DWC3_GUCTL_REFCLKPER_MASK;
> +	reg |=  (dwc->ref_clk_per << DWC3_GUCTL_REFCLKPER_SEL);
> +	dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
> +}
> +
> +
>  /**
>   * dwc3_free_one_event_buffer - Frees one event buffer
>   * @dwc: Pointer to our controller context structure
> @@ -982,6 +1024,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	/* Adjust Frame Length */
>  	dwc3_frame_length_adjustment(dwc);
>  
> +	/* Adjust Reference Clock Settings */
> +	dwc3_ref_clk_adjustment(dwc);
> +
> +	/* Adjust Reference Clock Period */
> +	dwc3_ref_clk_period(dwc);
> +
>  	dwc3_set_incr_burst_type(dwc);
>  
>  	usb_phy_set_suspend(dwc->usb2_phy, 0);
> @@ -1351,6 +1399,10 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  				    &dwc->hsphy_interface);
>  	device_property_read_u32(dev, "snps,quirk-frame-length-adjustment",
>  				 &dwc->fladj);
> +	device_property_read_u32(dev, "snps,quirk-ref-clock-adjustment",
> +				 &dwc->ref_clk_adj);
> +	device_property_read_u32(dev, "snps,quirk-ref-clock-period",
> +				 &dwc->ref_clk_per);
>  
>  	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 1b241f937d8f..469e94512414 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -379,6 +379,14 @@
>  #define DWC3_GFLADJ_30MHZ_SDBND_SEL		BIT(7)
>  #define DWC3_GFLADJ_30MHZ_MASK			0x3f
>  
> +/* Global User Control Register*/
> +#define DWC3_GUCTL_REFCLKPER_MASK		0xffc00000
> +#define DWC3_GUCTL_REFCLKPER_SEL		22
> +
> +/* Global reference clock Adjustment Register */
> +#define DWC3_GFLADJ_REFCLK_MASK			0xffffff00
> +#define DWC3_GFLADJ_REFCLK_SEL			8
> +
>  /* Global User Control Register 2 */
>  #define DWC3_GUCTL2_RST_ACTBITLATER		BIT(14)
>  
> @@ -956,6 +964,8 @@ struct dwc3_scratchpad_array {
>   * @regs: base address for our registers
>   * @regs_size: address space size
>   * @fladj: frame length adjustment
> + * @ref_clk_adj: reference clock adjustment
> + * @ref_clk_per: reference clock period configuration
>   * @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
> @@ -1118,6 +1128,8 @@ struct dwc3 {
>  	enum usb_dr_mode	role_switch_default_mode;
>  
>  	u32			fladj;
> +	u32			ref_clk_adj;
> +	u32			ref_clk_per;
>  	u32			irq_gadget;
>  	u32			otg_irq;
>  	u32			current_otg_role;
> -- 
> 2.30.0
> 

Re: [PATCH] usb: dwc3: reference clock configuration

[Baruch Siach]

Hi Bjorn,

Thanks for your review comments.

On Mon, Feb 08 2021, Bjorn Andersson wrote:
> On Mon 08 Feb 00:00 CST 2021, Baruch Siach wrote:
>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>> 
>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>> to control the behavior of controller with respect to SOF and ITP.
>> The reset values of these registers are aligned for 19.2 MHz
>> reference clock source. This change will add option to override
>> these settings for reference clock other than 19.2 MHz
>> 
>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>> 
>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>> [ baruch: mention tested hardware ]
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>  3 files changed, 69 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 1aae2b6160c1..4ffa87b697dc 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -89,6 +89,11 @@ Optional properties:
>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>  	register for post-silicon frame length adjustment when the
>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields of GFLADJ
>> +	register for reference clock other than 19.2 MHz is used.
>
> What are typical values for this property? What unit does it have? How
> does it actually relate to the frequency of the reference clock?

Downstream codeaurora kernel (fig branch) sets 0xA87F0 for IPQ6018
(24MHz reference clock), and 0x49459 for IPQ5018 (60MHz). So this value
appears to correlates with clock rate. I have no access to DWC3
documentation. I only tested IPQ6018 hardware.

>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL. This field
>> +	indicates in terms of nano seconds the period of ref_clk. To calculate the
>> +	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>
> Can't we make the dwc3 reference this clock and use clk_get_rate() and
> then do this math in the driver?

This is doable, I believe. Though current code does not identify
specific clocks, as far as I can see.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] usb: dwc3: reference clock configuration

[Kathiravan T]

On 2021-02-10 11:40, Baruch Siach wrote:
> Hi Bjorn,
> 
> Thanks for your review comments.
> 
> On Mon, Feb 08 2021, Bjorn Andersson wrote:
>> On Mon 08 Feb 00:00 CST 2021, Baruch Siach wrote:
>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>>> 
>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>>> to control the behavior of controller with respect to SOF and ITP.
>>> The reset values of these registers are aligned for 19.2 MHz
>>> reference clock source. This change will add option to override
>>> these settings for reference clock other than 19.2 MHz
>>> 
>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>>> 
>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>>> [ baruch: mention tested hardware ]
>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>> ---
>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>>  drivers/usb/dwc3/core.c                       | 52 
>>> +++++++++++++++++++
>>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>>  3 files changed, 69 insertions(+)
>>> 
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index 1aae2b6160c1..4ffa87b697dc 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -89,6 +89,11 @@ Optional properties:
>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field 
>>> of GFLADJ
>>>  	register for post-silicon frame length adjustment when the
>>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields 
>>> of GFLADJ
>>> +	register for reference clock other than 19.2 MHz is used.
>> 
>> What are typical values for this property? What unit does it have? How
>> does it actually relate to the frequency of the reference clock?
> 
> Downstream codeaurora kernel (fig branch) sets 0xA87F0 for IPQ6018
> (24MHz reference clock), and 0x49459 for IPQ5018 (60MHz). So this value
> appears to correlates with clock rate. I have no access to DWC3
> documentation. I only tested IPQ6018 hardware.
> 

It will be written as (0xA87F0 << 7) retaining the 0-7 LSB value.
I could see, BIT(23) of GFLADJ register enables the functionality of
running SOF/ITP counters based on the reference clock. Since this bit is 
set, we need to
compute the other fields as well i.e., from 8th bit to 31st bit. Finally 
it is derived to
0xA87F0 for IPQ6018.


>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL. 
>>> This field
>>> +	indicates in terms of nano seconds the period of ref_clk. To 
>>> calculate the
>>> +	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>> 
>> Can't we make the dwc3 reference this clock and use clk_get_rate() and
>> then do this math in the driver?
> 
> This is doable, I believe. Though current code does not identify
> specific clocks, as far as I can see.
> 
> baruch

We can mention one more clock(ref) in the USB device node and do the 
math (NSEC_PER_SEC / clk_get_rate()) in dwc3 driver.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] usb: dwc3: reference clock configuration

[Kathiravan T]

On 2021-02-15 22:28, Kathiravan T wrote:
> On 2021-02-10 11:40, Baruch Siach wrote:
>> Hi Bjorn,
>> 
>> Thanks for your review comments.
>> 
>> On Mon, Feb 08 2021, Bjorn Andersson wrote:
>>> On Mon 08 Feb 00:00 CST 2021, Baruch Siach wrote:
>>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>>>> 
>>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>>>> to control the behavior of controller with respect to SOF and ITP.
>>>> The reset values of these registers are aligned for 19.2 MHz
>>>> reference clock source. This change will add option to override
>>>> these settings for reference clock other than 19.2 MHz
>>>> 
>>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>>>> 
>>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>>>> [ baruch: mention tested hardware ]
>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>> ---
>>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>>>  drivers/usb/dwc3/core.c                       | 52 
>>>> +++++++++++++++++++
>>>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>>>  3 files changed, 69 insertions(+)
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index 1aae2b6160c1..4ffa87b697dc 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -89,6 +89,11 @@ Optional properties:
>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field 
>>>> of GFLADJ
>>>>  	register for post-silicon frame length adjustment when the
>>>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* 
>>>> fields of GFLADJ
>>>> +	register for reference clock other than 19.2 MHz is used.
>>> 
>>> What are typical values for this property? What unit does it have? 
>>> How
>>> does it actually relate to the frequency of the reference clock?
>> 
>> Downstream codeaurora kernel (fig branch) sets 0xA87F0 for IPQ6018
>> (24MHz reference clock), and 0x49459 for IPQ5018 (60MHz). So this 
>> value
>> appears to correlates with clock rate. I have no access to DWC3
>> documentation. I only tested IPQ6018 hardware.
>> 
> 
> It will be written as (0xA87F0 << 7) retaining the 0-7 LSB value.
> I could see, BIT(23) of GFLADJ register enables the functionality of
> running SOF/ITP counters based on the reference clock. Since this bit
> is set, we need to
> compute the other fields as well i.e., from 8th bit to 31st bit.
> Finally it is derived to
> 0xA87F0 for IPQ6018.
> 

Bjorn / All,

Any comments on this? Please do suggest if this can be handled in a 
better way.


> 
>>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL. 
>>>> This field
>>>> +	indicates in terms of nano seconds the period of ref_clk. To 
>>>> calculate the
>>>> +	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>>> 
>>> Can't we make the dwc3 reference this clock and use clk_get_rate() 
>>> and
>>> then do this math in the driver?
>> 
>> This is doable, I believe. Though current code does not identify
>> specific clocks, as far as I can see.
>> 
>> baruch
> 
> We can mention one more clock(ref) in the USB device node and do the
> math (NSEC_PER_SEC / clk_get_rate()) in dwc3 driver.

Thanks,
Kathiravan T.
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] usb: dwc3: reference clock configuration

[Jack Pham]

Hi Kathiravan, Baruch,

On Thu, Feb 25, 2021 at 10:17:49PM +0530, Kathiravan T wrote:
> On 2021-02-15 22:28, Kathiravan T wrote:
> > On 2021-02-10 11:40, Baruch Siach wrote:
> > > Hi Bjorn,
> > > 
> > > Thanks for your review comments.
> > > 
> > > On Mon, Feb 08 2021, Bjorn Andersson wrote:
> > > > On Mon 08 Feb 00:00 CST 2021, Baruch Siach wrote:
> > > > > From: Balaji Prakash J <bjagadee@codeaurora.org>
> > > > > 
> > > > > DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
> > > > > to control the behavior of controller with respect to SOF and ITP.
> > > > > The reset values of these registers are aligned for 19.2 MHz
> > > > > reference clock source. This change will add option to override
> > > > > these settings for reference clock other than 19.2 MHz
> > > > > 
> > > > > Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
> > > > > 
> > > > > Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
> > > > > [ baruch: mention tested hardware ]
> > > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> > > > > ---
> > > > >  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
> > > > >  drivers/usb/dwc3/core.c                       | 52
> > > > > +++++++++++++++++++
> > > > >  drivers/usb/dwc3/core.h                       | 12 +++++
> > > > >  3 files changed, 69 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > > b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > > index 1aae2b6160c1..4ffa87b697dc 100644
> > > > > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > > @@ -89,6 +89,11 @@ Optional properties:
> > > > >   - snps,quirk-frame-length-adjustment: Value for
> > > > > GFLADJ_30MHZ field of GFLADJ
> > > > >  	register for post-silicon frame length adjustment when the
> > > > >  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> > > > > + - snps,quirk-ref-clock-adjustment: Value for
> > > > > GFLADJ_REFCLK_* fields of GFLADJ
> > > > > +	register for reference clock other than 19.2 MHz is used.
> > > > 
> > > > What are typical values for this property? What unit does it
> > > > have? How
> > > > does it actually relate to the frequency of the reference clock?
> > > 
> > > Downstream codeaurora kernel (fig branch) sets 0xA87F0 for IPQ6018
> > > (24MHz reference clock), and 0x49459 for IPQ5018 (60MHz). So this
> > > value
> > > appears to correlates with clock rate. I have no access to DWC3
> > > documentation. I only tested IPQ6018 hardware.
> > > 
> > 
> > It will be written as (0xA87F0 << 7) retaining the 0-7 LSB value.
> > I could see, BIT(23) of GFLADJ register enables the functionality of
> > running SOF/ITP counters based on the reference clock. Since this bit
> > is set, we need to
> > compute the other fields as well i.e., from 8th bit to 31st bit.
> > Finally it is derived to
> > 0xA87F0 for IPQ6018.
> > 
> 
> Bjorn / All,
> 
> Any comments on this? Please do suggest if this can be handled in a better
> way.
> 
> 
> > 
> > > > > + - snps,quirk-ref-clock-period: Value for REFCLKPER filed
> > > > > of GUCTL. This field
> > > > > +	indicates in terms of nano seconds the period of ref_clk.
> > > > > To calculate the
> > > > > +	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
> > > > 
> > > > Can't we make the dwc3 reference this clock and use
> > > > clk_get_rate() and
> > > > then do this math in the driver?
> > > 
> > > This is doable, I believe. Though current code does not identify
> > > specific clocks, as far as I can see.

I agree it should be doable. Looks like prior to 0d3a97083e0c ("usb:
dwc3: Rework clock initialization to be more flexible") the core did
support specific clocks ("ref", "bus_early", "suspend"), but was
changed to use a simpler devm_clk_bulk_get_all() call.

> > We can mention one more clock(ref) in the USB device node and do the
> > math (NSEC_PER_SEC / clk_get_rate()) in dwc3 driver.

Yea, just need to make sure "ref" clk is specified in the DT node. Then
in the driver you can just iterate through dwc->clks and try to find one
with .id=="ref". If clk_get_rate() succeeds then you can use the value
to calculate the GUCTL.REFCLKPER and GFLADJ register fields.

Or perhaps even use a lookup table, since according to the DWC3
programming guide only 6 refclk frequencies (16, 17, 19.2, 20, 24, 39.7
MHz) are supported so that might be simpler than a few integer divide
operations that would otherwise be required.

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] usb: dwc3: reference clock configuration

[Thinh Nguyen]

Jack Pham wrote:
> Hi Kathiravan, Baruch,
> 
> On Thu, Feb 25, 2021 at 10:17:49PM +0530, Kathiravan T wrote:
>> On 2021-02-15 22:28, Kathiravan T wrote:
>>> On 2021-02-10 11:40, Baruch Siach wrote:
>>>> Hi Bjorn,
>>>>
>>>> Thanks for your review comments.
>>>>
>>>> On Mon, Feb 08 2021, Bjorn Andersson wrote:
>>>>> On Mon 08 Feb 00:00 CST 2021, Baruch Siach wrote:
>>>>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>>>
>>>>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>>>>>> to control the behavior of controller with respect to SOF and ITP.
>>>>>> The reset values of these registers are aligned for 19.2 MHz
>>>>>> reference clock source. This change will add option to override
>>>>>> these settings for reference clock other than 19.2 MHz
>>>>>>
>>>>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>>>>>>
>>>>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>>> [ baruch: mention tested hardware ]
>>>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>>>> ---
>>>>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>>>>>  drivers/usb/dwc3/core.c                       | 52
>>>>>> +++++++++++++++++++
>>>>>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>>>>>  3 files changed, 69 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> index 1aae2b6160c1..4ffa87b697dc 100644
>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> @@ -89,6 +89,11 @@ Optional properties:
>>>>>>   - snps,quirk-frame-length-adjustment: Value for
>>>>>> GFLADJ_30MHZ field of GFLADJ
>>>>>>  	register for post-silicon frame length adjustment when the
>>>>>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>>> + - snps,quirk-ref-clock-adjustment: Value for
>>>>>> GFLADJ_REFCLK_* fields of GFLADJ
>>>>>> +	register for reference clock other than 19.2 MHz is used.
>>>>>
>>>>> What are typical values for this property? What unit does it
>>>>> have? How
>>>>> does it actually relate to the frequency of the reference clock?
>>>>
>>>> Downstream codeaurora kernel (fig branch) sets 0xA87F0 for IPQ6018
>>>> (24MHz reference clock), and 0x49459 for IPQ5018 (60MHz). So this
>>>> value
>>>> appears to correlates with clock rate. I have no access to DWC3
>>>> documentation. I only tested IPQ6018 hardware.
>>>>
>>>
>>> It will be written as (0xA87F0 << 7) retaining the 0-7 LSB value.
>>> I could see, BIT(23) of GFLADJ register enables the functionality of
>>> running SOF/ITP counters based on the reference clock. Since this bit
>>> is set, we need to
>>> compute the other fields as well i.e., from 8th bit to 31st bit.
>>> Finally it is derived to
>>> 0xA87F0 for IPQ6018.
>>>
>>
>> Bjorn / All,
>>
>> Any comments on this? Please do suggest if this can be handled in a better
>> way.
>>
>>
>>>
>>>>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed
>>>>>> of GUCTL. This field
>>>>>> +	indicates in terms of nano seconds the period of ref_clk.
>>>>>> To calculate the
>>>>>> +	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>>>>>
>>>>> Can't we make the dwc3 reference this clock and use
>>>>> clk_get_rate() and
>>>>> then do this math in the driver?
>>>>
>>>> This is doable, I believe. Though current code does not identify
>>>> specific clocks, as far as I can see.
> 
> I agree it should be doable. Looks like prior to 0d3a97083e0c ("usb:
> dwc3: Rework clock initialization to be more flexible") the core did
> support specific clocks ("ref", "bus_early", "suspend"), but was
> changed to use a simpler devm_clk_bulk_get_all() call.
> 
>>> We can mention one more clock(ref) in the USB device node and do the
>>> math (NSEC_PER_SEC / clk_get_rate()) in dwc3 driver.
> 
> Yea, just need to make sure "ref" clk is specified in the DT node. Then
> in the driver you can just iterate through dwc->clks and try to find one
> with .id=="ref". If clk_get_rate() succeeds then you can use the value
> to calculate the GUCTL.REFCLKPER and GFLADJ register fields.
> 
> Or perhaps even use a lookup table, since according to the DWC3
> programming guide only 6 refclk frequencies (16, 17, 19.2, 20, 24, 39.7
> MHz) are supported so that might be simpler than a few integer divide
> operations that would otherwise be required.
> 
> Jack
> 

Hi,

Why not create use a DT property instead? This will complicate things
for PCI devices. The designer know what refclk frequencies it is, we
just need to inform the controller via this property in nanosecond. It's
more accurate and you don't have to any calculation or worry about
whether to match the "ref" clock, or worse create a dummy/fake refclk
for a PCI device just to inform the controller this.

BR,
Thinh

Re: [PATCH] usb: dwc3: reference clock configuration

[Thinh Nguyen]

Baruch Siach wrote:
> From: Balaji Prakash J <bjagadee@codeaurora.org>
> 
> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
> to control the behavior of controller with respect to SOF and ITP.
> The reset values of these registers are aligned for 19.2 MHz
> reference clock source. This change will add option to override
> these settings for reference clock other than 19.2 MHz
> 
> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
> 
> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
> [ baruch: mention tested hardware ]
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
>  drivers/usb/dwc3/core.h                       | 12 +++++
>  3 files changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 1aae2b6160c1..4ffa87b697dc 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -89,6 +89,11 @@ Optional properties:
>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>  	register for post-silicon frame length adjustment when the
>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields of GFLADJ
> +	register for reference clock other than 19.2 MHz is used.
> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL. This field
> +	indicates in terms of nano seconds the period of ref_clk. To calculate the
> +	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.

Why is this a quirk? It's not a quirk. The user can inform the ref_clk
period to the controller here.

The default value from GUCTL.REFCLKPER is a value from coreConsultant
setting. The designer knows what period it should be and should properly
set it if the default is not correctly set.

BR,
Thinh

Re: [PATCH] usb: dwc3: reference clock configuration

[Kathiravan T]

On 2021-03-31 06:47, Thinh Nguyen wrote:
> Baruch Siach wrote:
>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>> 
>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>> to control the behavior of controller with respect to SOF and ITP.
>> The reset values of these registers are aligned for 19.2 MHz
>> reference clock source. This change will add option to override
>> these settings for reference clock other than 19.2 MHz
>> 
>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>> 
>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>> [ baruch: mention tested hardware ]
>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>> ---
>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>  drivers/usb/dwc3/core.c                       | 52 
>> +++++++++++++++++++
>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>  3 files changed, 69 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 1aae2b6160c1..4ffa87b697dc 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -89,6 +89,11 @@ Optional properties:
>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field 
>> of GFLADJ
>>  	register for post-silicon frame length adjustment when the
>>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields 
>> of GFLADJ
>> +	register for reference clock other than 19.2 MHz is used.
>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL. 
>> This field
>> +	indicates in terms of nano seconds the period of ref_clk. To 
>> calculate the
>> +	ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
> 
> Why is this a quirk? It's not a quirk. The user can inform the ref_clk
> period to the controller here.
> 
> The default value from GUCTL.REFCLKPER is a value from coreConsultant
> setting. The designer knows what period it should be and should 
> properly
> set it if the default is not correctly set.

Thanks Thinh for your inputs. Can we have the DT property for both the 
GUCTL.REFCLKPER and GFLADJ.REFCLK* fields?
Since GFLADJ.REFCLK* field values derived based on the GUCTL.REFCLKPER. 
In other words, are you fine with the
approach followed here? If so, we can work on the nitpicks and send the 
V2.

Please let us know your thoughts on this.

> BR,
> Thinh


-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] usb: dwc3: reference clock configuration

[Thinh Nguyen]

Kathiravan T wrote:
> On 2021-03-31 06:47, Thinh Nguyen wrote:
>> Baruch Siach wrote:
>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>>>
>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>>> to control the behavior of controller with respect to SOF and ITP.
>>> The reset values of these registers are aligned for 19.2 MHz
>>> reference clock source. This change will add option to override
>>> these settings for reference clock other than 19.2 MHz
>>>
>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>>>
>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>>> [ baruch: mention tested hardware ]
>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>> ---
>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
>>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>>  3 files changed, 69 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index 1aae2b6160c1..4ffa87b697dc 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -89,6 +89,11 @@ Optional properties:
>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field
>>> of GFLADJ
>>>      register for post-silicon frame length adjustment when the
>>>      fladj_30mhz_sdbnd signal is invalid or incorrect.
>>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields
>>> of GFLADJ
>>> +    register for reference clock other than 19.2 MHz is used.
>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL.
>>> This field
>>> +    indicates in terms of nano seconds the period of ref_clk. To
>>> calculate the
>>> +    ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>>
>> Why is this a quirk? It's not a quirk. The user can inform the ref_clk
>> period to the controller here.
>>
>> The default value from GUCTL.REFCLKPER is a value from coreConsultant
>> setting. The designer knows what period it should be and should properly
>> set it if the default is not correctly set.
> 
> Thanks Thinh for your inputs. Can we have the DT property for both the
> GUCTL.REFCLKPER and GFLADJ.REFCLK* fields?
> Since GFLADJ.REFCLK* field values derived based on the GUCTL.REFCLKPER.
> In other words, are you fine with the
> approach followed here? If so, we can work on the nitpicks and send the V2.
> 
> Please let us know your thoughts on this.
> 

Hi Kathiravan,

Yes, IMO, using DT properties work just fine to inform the controller if
the default settings don't match the HW configuration. As I mention in
the separate email thread, using clk_get_rate() doesn't make sense for
PCI devices.

The snps,quirk-ref-clock-adjustment property updates multiple fields of
the GFLADJ and not just GFLADJ_REFCLK_FLADJ. I'd suggest to split them
to different properties for different fields for clarity. If the other
fields of GFLADJ.REFCLK_* are derived based on the GUCTL.REFCLKPER
according to the example of the programming guide, then do that
calculation in the driver as default. However, I'd suggest to create a
separate property (maybe "snps,use-refclk-for-sof-itp"?) to select
GFLADJ.GFLADJ_REFCLK_LPM_SEL or GCTL.SOFITPSYNC depending whether the
controller is operating as host or device mode. Note that this feature
is only available for DWC_usb32 and DWC_usb31 v1.80a or higher. I need
to double check for DWC_usb3 IP, but I believe it's v3.30a or higher.

Btw, we don't need to mention 19.2 MHz since it's the specific default
configuration of your setup. Other setups may not be the same.

BR,
Thinh

Re: [PATCH] usb: dwc3: reference clock configuration

[Bjorn Andersson]

On Wed 07 Apr 20:53 CDT 2021, Thinh Nguyen wrote:

> Kathiravan T wrote:
> > On 2021-03-31 06:47, Thinh Nguyen wrote:
> >> Baruch Siach wrote:
> >>> From: Balaji Prakash J <bjagadee@codeaurora.org>
> >>>
> >>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
> >>> to control the behavior of controller with respect to SOF and ITP.
> >>> The reset values of these registers are aligned for 19.2 MHz
> >>> reference clock source. This change will add option to override
> >>> these settings for reference clock other than 19.2 MHz
> >>>
> >>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
> >>>
> >>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
> >>> [ baruch: mention tested hardware ]
> >>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >>> ---
> >>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
> >>>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
> >>>  drivers/usb/dwc3/core.h                       | 12 +++++
> >>>  3 files changed, 69 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> index 1aae2b6160c1..4ffa87b697dc 100644
> >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> @@ -89,6 +89,11 @@ Optional properties:
> >>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field
> >>> of GFLADJ
> >>>      register for post-silicon frame length adjustment when the
> >>>      fladj_30mhz_sdbnd signal is invalid or incorrect.
> >>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields
> >>> of GFLADJ
> >>> +    register for reference clock other than 19.2 MHz is used.
> >>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL.
> >>> This field
> >>> +    indicates in terms of nano seconds the period of ref_clk. To
> >>> calculate the
> >>> +    ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
> >>
> >> Why is this a quirk? It's not a quirk. The user can inform the ref_clk
> >> period to the controller here.
> >>
> >> The default value from GUCTL.REFCLKPER is a value from coreConsultant
> >> setting. The designer knows what period it should be and should properly
> >> set it if the default is not correctly set.
> > 
> > Thanks Thinh for your inputs. Can we have the DT property for both the
> > GUCTL.REFCLKPER and GFLADJ.REFCLK* fields?
> > Since GFLADJ.REFCLK* field values derived based on the GUCTL.REFCLKPER.
> > In other words, are you fine with the
> > approach followed here? If so, we can work on the nitpicks and send the V2.
> > 
> > Please let us know your thoughts on this.
> > 
> 
> Hi Kathiravan,
> 
> Yes, IMO, using DT properties work just fine to inform the controller if
> the default settings don't match the HW configuration.

I'm not against using a separate DT property if the information it
provides can't be derived from what's already there.

> As I mention in
> the separate email thread, using clk_get_rate() doesn't make sense for
> PCI devices.
> 

I'm sorry, can you help me understand why this relate to PCI?

> The snps,quirk-ref-clock-adjustment property updates multiple fields of
> the GFLADJ and not just GFLADJ_REFCLK_FLADJ. I'd suggest to split them
> to different properties for different fields for clarity. If the other
> fields of GFLADJ.REFCLK_* are derived based on the GUCTL.REFCLKPER
> according to the example of the programming guide, then do that
> calculation in the driver as default.

It sounds to me that rather than saying "refclk is X MHz" you propose a
set or properties in the line of "write X, Y, Z to these registers",
which isn't what we typically put in DT.

Regards,
Bjorn

> However, I'd suggest to create a
> separate property (maybe "snps,use-refclk-for-sof-itp"?) to select
> GFLADJ.GFLADJ_REFCLK_LPM_SEL or GCTL.SOFITPSYNC depending whether the
> controller is operating as host or device mode.
> Note that this feature
> is only available for DWC_usb32 and DWC_usb31 v1.80a or higher. I need
> to double check for DWC_usb3 IP, but I believe it's v3.30a or higher.
> 
> Btw, we don't need to mention 19.2 MHz since it's the specific default
> configuration of your setup. Other setups may not be the same.
> 
> BR,
> Thinh

Re: [PATCH] usb: dwc3: reference clock configuration

[Thinh Nguyen]

Bjorn Andersson wrote:
> On Wed 07 Apr 20:53 CDT 2021, Thinh Nguyen wrote:
> 
>> Kathiravan T wrote:
>>> On 2021-03-31 06:47, Thinh Nguyen wrote:
>>>> Baruch Siach wrote:
>>>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>>
>>>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>>>>> to control the behavior of controller with respect to SOF and ITP.
>>>>> The reset values of these registers are aligned for 19.2 MHz
>>>>> reference clock source. This change will add option to override
>>>>> these settings for reference clock other than 19.2 MHz
>>>>>
>>>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>>>>>
>>>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>> [ baruch: mention tested hardware ]
>>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>>> ---
>>>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>>>>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
>>>>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>>>>  3 files changed, 69 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index 1aae2b6160c1..4ffa87b697dc 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -89,6 +89,11 @@ Optional properties:
>>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field
>>>>> of GFLADJ
>>>>>      register for post-silicon frame length adjustment when the
>>>>>      fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields
>>>>> of GFLADJ
>>>>> +    register for reference clock other than 19.2 MHz is used.
>>>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL.
>>>>> This field
>>>>> +    indicates in terms of nano seconds the period of ref_clk. To
>>>>> calculate the
>>>>> +    ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>>>>
>>>> Why is this a quirk? It's not a quirk. The user can inform the ref_clk
>>>> period to the controller here.
>>>>
>>>> The default value from GUCTL.REFCLKPER is a value from coreConsultant
>>>> setting. The designer knows what period it should be and should properly
>>>> set it if the default is not correctly set.
>>>
>>> Thanks Thinh for your inputs. Can we have the DT property for both the
>>> GUCTL.REFCLKPER and GFLADJ.REFCLK* fields?
>>> Since GFLADJ.REFCLK* field values derived based on the GUCTL.REFCLKPER.
>>> In other words, are you fine with the
>>> approach followed here? If so, we can work on the nitpicks and send the V2.
>>>
>>> Please let us know your thoughts on this.
>>>
>>
>> Hi Kathiravan,
>>
>> Yes, IMO, using DT properties work just fine to inform the controller if
>> the default settings don't match the HW configuration.
> 
> I'm not against using a separate DT property if the information it
> provides can't be derived from what's already there.

That's the issue. That information is not available if dwc3 is using PCI
bus.

> 
>> As I mention in
>> the separate email thread, using clk_get_rate() doesn't make sense for
>> PCI devices.
>>
> 
> I'm sorry, can you help me understand why this relate to PCI?

It's because the clock's attributes are not exposed if we're using the
PCI bus. The driver cannot access this information unless the user
provides it in some other way.

> 
>> The snps,quirk-ref-clock-adjustment property updates multiple fields of
>> the GFLADJ and not just GFLADJ_REFCLK_FLADJ. I'd suggest to split them
>> to different properties for different fields for clarity. If the other
>> fields of GFLADJ.REFCLK_* are derived based on the GUCTL.REFCLKPER
>> according to the example of the programming guide, then do that
>> calculation in the driver as default.
> 
> It sounds to me that rather than saying "refclk is X MHz" you propose a
> set or properties in the line of "write X, Y, Z to these registers",
> which isn't what we typically put in DT.

Different fields of the register control different features and not just
the "refclk is X MHz".

GUCTL register field REFCLKPER is "refclk is X MHz"

GFLADJ register field GFLADJ_REFCLK_LPM_SEL enables a feature to use
refclk to track SOF/ITP counter

GFLADJ register field GFLADJ_REFCLK_FLADJ do adjustments to the frame
length when running on refclk

GFLADJ register field GFLADJ_REFCLK_240MHZ_DECR is another adjustment
for 240MHz

GFLADJ register field GFLADJ_REFCLK_240MHZDECR_PLS1 is another adjustment

My suggestion is to have 2 DT properties:
1) for GUCTL.REFCLKPER for "refclk is X MHz" but in term of period ns
2) for GFLADJ.GFLADJ_REFCLK_LPM_SEL to enable a specific feature of the
controller. The other fields of GFLADJ can be calculated as default
according to the programming guide.

Is it typical that we combine different features in a single DT
property? Which was what this orignal patch did for GFLADJ register with
the fields mentioned above.

BR,
Thinh

> 
> Regards,
> Bjorn
> 
>> However, I'd suggest to create a
>> separate property (maybe "snps,use-refclk-for-sof-itp"?) to select
>> GFLADJ.GFLADJ_REFCLK_LPM_SEL or GCTL.SOFITPSYNC depending whether the
>> controller is operating as host or device mode.
>> Note that this feature
>> is only available for DWC_usb32 and DWC_usb31 v1.80a or higher. I need
>> to double check for DWC_usb3 IP, but I believe it's v3.30a or higher.
>>
>> Btw, we don't need to mention 19.2 MHz since it's the specific default
>> configuration of your setup. Other setups may not be the same.
>>
>> BR,
>> Thinh

Re: [PATCH] usb: dwc3: reference clock configuration

[Thinh Nguyen]

Thinh Nguyen wrote:
> Bjorn Andersson wrote:
>> On Wed 07 Apr 20:53 CDT 2021, Thinh Nguyen wrote:
>>
>>> Kathiravan T wrote:
>>>> On 2021-03-31 06:47, Thinh Nguyen wrote:
>>>>> Baruch Siach wrote:
>>>>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>>>
>>>>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>>>>>> to control the behavior of controller with respect to SOF and ITP.
>>>>>> The reset values of these registers are aligned for 19.2 MHz
>>>>>> reference clock source. This change will add option to override
>>>>>> these settings for reference clock other than 19.2 MHz
>>>>>>
>>>>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>>>>>>
>>>>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>>> [ baruch: mention tested hardware ]
>>>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>>>> ---
>>>>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>>>>>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
>>>>>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>>>>>  3 files changed, 69 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> index 1aae2b6160c1..4ffa87b697dc 100644
>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> @@ -89,6 +89,11 @@ Optional properties:
>>>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field
>>>>>> of GFLADJ
>>>>>>      register for post-silicon frame length adjustment when the
>>>>>>      fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields
>>>>>> of GFLADJ
>>>>>> +    register for reference clock other than 19.2 MHz is used.
>>>>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL.
>>>>>> This field
>>>>>> +    indicates in terms of nano seconds the period of ref_clk. To
>>>>>> calculate the
>>>>>> +    ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>>>>>
>>>>> Why is this a quirk? It's not a quirk. The user can inform the ref_clk
>>>>> period to the controller here.
>>>>>
>>>>> The default value from GUCTL.REFCLKPER is a value from coreConsultant
>>>>> setting. The designer knows what period it should be and should properly
>>>>> set it if the default is not correctly set.
>>>>
>>>> Thanks Thinh for your inputs. Can we have the DT property for both the
>>>> GUCTL.REFCLKPER and GFLADJ.REFCLK* fields?
>>>> Since GFLADJ.REFCLK* field values derived based on the GUCTL.REFCLKPER.
>>>> In other words, are you fine with the
>>>> approach followed here? If so, we can work on the nitpicks and send the V2.
>>>>
>>>> Please let us know your thoughts on this.
>>>>
>>>
>>> Hi Kathiravan,
>>>
>>> Yes, IMO, using DT properties work just fine to inform the controller if
>>> the default settings don't match the HW configuration.
>>
>> I'm not against using a separate DT property if the information it
>> provides can't be derived from what's already there.
> 
> That's the issue. That information is not available if dwc3 is using PCI
> bus.
> 

I forgot to mention. DWC3 creates and use a platform device from PCI.

BR,
Thinh

>>
>>> As I mention in
>>> the separate email thread, using clk_get_rate() doesn't make sense for
>>> PCI devices.
>>>
>>
>> I'm sorry, can you help me understand why this relate to PCI?
> 
> It's because the clock's attributes are not exposed if we're using the
> PCI bus. The driver cannot access this information unless the user
> provides it in some other way.
> 
>>
>>> The snps,quirk-ref-clock-adjustment property updates multiple fields of
>>> the GFLADJ and not just GFLADJ_REFCLK_FLADJ. I'd suggest to split them
>>> to different properties for different fields for clarity. If the other
>>> fields of GFLADJ.REFCLK_* are derived based on the GUCTL.REFCLKPER
>>> according to the example of the programming guide, then do that
>>> calculation in the driver as default.
>>
>> It sounds to me that rather than saying "refclk is X MHz" you propose a
>> set or properties in the line of "write X, Y, Z to these registers",
>> which isn't what we typically put in DT.
> 
> Different fields of the register control different features and not just
> the "refclk is X MHz".
> 
> GUCTL register field REFCLKPER is "refclk is X MHz"
> 
> GFLADJ register field GFLADJ_REFCLK_LPM_SEL enables a feature to use
> refclk to track SOF/ITP counter
> 
> GFLADJ register field GFLADJ_REFCLK_FLADJ do adjustments to the frame
> length when running on refclk
> 
> GFLADJ register field GFLADJ_REFCLK_240MHZ_DECR is another adjustment
> for 240MHz
> 
> GFLADJ register field GFLADJ_REFCLK_240MHZDECR_PLS1 is another adjustment
> 
> My suggestion is to have 2 DT properties:
> 1) for GUCTL.REFCLKPER for "refclk is X MHz" but in term of period ns
> 2) for GFLADJ.GFLADJ_REFCLK_LPM_SEL to enable a specific feature of the
> controller. The other fields of GFLADJ can be calculated as default
> according to the programming guide.
> 
> Is it typical that we combine different features in a single DT
> property? Which was what this orignal patch did for GFLADJ register with
> the fields mentioned above.
> 
> BR,
> Thinh
> 
>>
>> Regards,
>> Bjorn
>>
>>> However, I'd suggest to create a
>>> separate property (maybe "snps,use-refclk-for-sof-itp"?) to select
>>> GFLADJ.GFLADJ_REFCLK_LPM_SEL or GCTL.SOFITPSYNC depending whether the
>>> controller is operating as host or device mode.
>>> Note that this feature
>>> is only available for DWC_usb32 and DWC_usb31 v1.80a or higher. I need
>>> to double check for DWC_usb3 IP, but I believe it's v3.30a or higher.
>>>
>>> Btw, we don't need to mention 19.2 MHz since it's the specific default
>>> configuration of your setup. Other setups may not be the same.
>>>
>>> BR,
>>> Thinh
> 

Re: [PATCH] usb: dwc3: reference clock configuration

[Bjorn Andersson]

On Wed 07 Apr 21:50 CDT 2021, Thinh Nguyen wrote:

> Bjorn Andersson wrote:
> > On Wed 07 Apr 20:53 CDT 2021, Thinh Nguyen wrote:
> > 
> >> Kathiravan T wrote:
> >>> On 2021-03-31 06:47, Thinh Nguyen wrote:
> >>>> Baruch Siach wrote:
> >>>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
> >>>>>
> >>>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
> >>>>> to control the behavior of controller with respect to SOF and ITP.
> >>>>> The reset values of these registers are aligned for 19.2 MHz
> >>>>> reference clock source. This change will add option to override
> >>>>> these settings for reference clock other than 19.2 MHz
> >>>>>
> >>>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
> >>>>>
> >>>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
> >>>>> [ baruch: mention tested hardware ]
> >>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> >>>>> ---
> >>>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
> >>>>>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
> >>>>>  drivers/usb/dwc3/core.h                       | 12 +++++
> >>>>>  3 files changed, 69 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> index 1aae2b6160c1..4ffa87b697dc 100644
> >>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>>> @@ -89,6 +89,11 @@ Optional properties:
> >>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field
> >>>>> of GFLADJ
> >>>>>      register for post-silicon frame length adjustment when the
> >>>>>      fladj_30mhz_sdbnd signal is invalid or incorrect.
> >>>>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields
> >>>>> of GFLADJ
> >>>>> +    register for reference clock other than 19.2 MHz is used.
> >>>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL.
> >>>>> This field
> >>>>> +    indicates in terms of nano seconds the period of ref_clk. To
> >>>>> calculate the
> >>>>> +    ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
> >>>>
> >>>> Why is this a quirk? It's not a quirk. The user can inform the ref_clk
> >>>> period to the controller here.
> >>>>
> >>>> The default value from GUCTL.REFCLKPER is a value from coreConsultant
> >>>> setting. The designer knows what period it should be and should properly
> >>>> set it if the default is not correctly set.
> >>>
> >>> Thanks Thinh for your inputs. Can we have the DT property for both the
> >>> GUCTL.REFCLKPER and GFLADJ.REFCLK* fields?
> >>> Since GFLADJ.REFCLK* field values derived based on the GUCTL.REFCLKPER.
> >>> In other words, are you fine with the
> >>> approach followed here? If so, we can work on the nitpicks and send the V2.
> >>>
> >>> Please let us know your thoughts on this.
> >>>
> >>
> >> Hi Kathiravan,
> >>
> >> Yes, IMO, using DT properties work just fine to inform the controller if
> >> the default settings don't match the HW configuration.
> > 
> > I'm not against using a separate DT property if the information it
> > provides can't be derived from what's already there.
> 
> That's the issue. That information is not available if dwc3 is using PCI
> bus.
> 
> > 
> >> As I mention in
> >> the separate email thread, using clk_get_rate() doesn't make sense for
> >> PCI devices.
> >>
> > 
> > I'm sorry, can you help me understand why this relate to PCI?
> 
> It's because the clock's attributes are not exposed if we're using the
> PCI bus. The driver cannot access this information unless the user
> provides it in some other way.
> 

So we have a DWC3 controller on a PCI bus, somehow described in DT, but
the refclock is derived from something that's not described as a clock
in the OS?

> > 
> >> The snps,quirk-ref-clock-adjustment property updates multiple fields of
> >> the GFLADJ and not just GFLADJ_REFCLK_FLADJ. I'd suggest to split them
> >> to different properties for different fields for clarity. If the other
> >> fields of GFLADJ.REFCLK_* are derived based on the GUCTL.REFCLKPER
> >> according to the example of the programming guide, then do that
> >> calculation in the driver as default.
> > 
> > It sounds to me that rather than saying "refclk is X MHz" you propose a
> > set or properties in the line of "write X, Y, Z to these registers",
> > which isn't what we typically put in DT.
> 
> Different fields of the register control different features and not just
> the "refclk is X MHz".
> 
> GUCTL register field REFCLKPER is "refclk is X MHz"
> 

As long as there's a technical reason why this needs to be described,
this would be a property.

> GFLADJ register field GFLADJ_REFCLK_LPM_SEL enables a feature to use
> refclk to track SOF/ITP counter
> 
> GFLADJ register field GFLADJ_REFCLK_FLADJ do adjustments to the frame
> length when running on refclk
> 
> GFLADJ register field GFLADJ_REFCLK_240MHZ_DECR is another adjustment
> for 240MHz
> 
> GFLADJ register field GFLADJ_REFCLK_240MHZDECR_PLS1 is another adjustment
> 
> My suggestion is to have 2 DT properties:
> 1) for GUCTL.REFCLKPER for "refclk is X MHz" but in term of period ns
> 2) for GFLADJ.GFLADJ_REFCLK_LPM_SEL to enable a specific feature of the
> controller. The other fields of GFLADJ can be calculated as default
> according to the programming guide.
> 

I'm not familiar with the details of these adjustments, but from what
you describe your suggestion sounds very reasonable to me.

> Is it typical that we combine different features in a single DT
> property? Which was what this orignal patch did for GFLADJ register with
> the fields mentioned above.
> 

For things that are generic yes, but otherwise the general guideline is
that things should be human readable with standard units (whenever
possible).

Thank you,
Bjorn

Re: [PATCH] usb: dwc3: reference clock configuration

[Thinh Nguyen]

Bjorn Andersson wrote:
> On Wed 07 Apr 21:50 CDT 2021, Thinh Nguyen wrote:
> 
>> Bjorn Andersson wrote:
>>> On Wed 07 Apr 20:53 CDT 2021, Thinh Nguyen wrote:
>>>
>>>> Kathiravan T wrote:
>>>>> On 2021-03-31 06:47, Thinh Nguyen wrote:
>>>>>> Baruch Siach wrote:
>>>>>>> From: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>>>>
>>>>>>> DWC_USB3_GFLADJ and DWC_USB3_GUCTL registers contain options
>>>>>>> to control the behavior of controller with respect to SOF and ITP.
>>>>>>> The reset values of these registers are aligned for 19.2 MHz
>>>>>>> reference clock source. This change will add option to override
>>>>>>> these settings for reference clock other than 19.2 MHz
>>>>>>>
>>>>>>> Tested on IPQ6018 SoC based CP01 board with 24MHz reference clock.
>>>>>>>
>>>>>>> Signed-off-by: Balaji Prakash J <bjagadee@codeaurora.org>
>>>>>>> [ baruch: mention tested hardware ]
>>>>>>> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/usb/dwc3.txt          |  5 ++
>>>>>>>  drivers/usb/dwc3/core.c                       | 52 +++++++++++++++++++
>>>>>>>  drivers/usb/dwc3/core.h                       | 12 +++++
>>>>>>>  3 files changed, 69 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> index 1aae2b6160c1..4ffa87b697dc 100644
>>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> @@ -89,6 +89,11 @@ Optional properties:
>>>>>>>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field
>>>>>>> of GFLADJ
>>>>>>>      register for post-silicon frame length adjustment when the
>>>>>>>      fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>>>> + - snps,quirk-ref-clock-adjustment: Value for GFLADJ_REFCLK_* fields
>>>>>>> of GFLADJ
>>>>>>> +    register for reference clock other than 19.2 MHz is used.
>>>>>>> + - snps,quirk-ref-clock-period: Value for REFCLKPER filed of GUCTL.
>>>>>>> This field
>>>>>>> +    indicates in terms of nano seconds the period of ref_clk. To
>>>>>>> calculate the
>>>>>>> +    ideal value, REFCLKPER = (1/ref_clk in Hz)*10^9.
>>>>>>
>>>>>> Why is this a quirk? It's not a quirk. The user can inform the ref_clk
>>>>>> period to the controller here.
>>>>>>
>>>>>> The default value from GUCTL.REFCLKPER is a value from coreConsultant
>>>>>> setting. The designer knows what period it should be and should properly
>>>>>> set it if the default is not correctly set.
>>>>>
>>>>> Thanks Thinh for your inputs. Can we have the DT property for both the
>>>>> GUCTL.REFCLKPER and GFLADJ.REFCLK* fields?
>>>>> Since GFLADJ.REFCLK* field values derived based on the GUCTL.REFCLKPER.
>>>>> In other words, are you fine with the
>>>>> approach followed here? If so, we can work on the nitpicks and send the V2.
>>>>>
>>>>> Please let us know your thoughts on this.
>>>>>
>>>>
>>>> Hi Kathiravan,
>>>>
>>>> Yes, IMO, using DT properties work just fine to inform the controller if
>>>> the default settings don't match the HW configuration.
>>>
>>> I'm not against using a separate DT property if the information it
>>> provides can't be derived from what's already there.
>>
>> That's the issue. That information is not available if dwc3 is using PCI
>> bus.
>>
>>>
>>>> As I mention in
>>>> the separate email thread, using clk_get_rate() doesn't make sense for
>>>> PCI devices.
>>>>
>>>
>>> I'm sorry, can you help me understand why this relate to PCI?
>>
>> It's because the clock's attributes are not exposed if we're using the
>> PCI bus. The driver cannot access this information unless the user
>> provides it in some other way.
>>
> 
> So we have a DWC3 controller on a PCI bus, somehow described in DT, but
> the refclock is derived from something that's not described as a clock
> in the OS?
> 

It's not described in DT. We create a platform device in the PCI glue
driver and pass in specific properties as if it's a platform device.
From the PCI function, we have no clue of the refclk. It may be better
if the DWC3 driver can initialize and drive the PCI function without
converting it to a platform device. However, I don't see this will
change any time soon.

BR,
Thinh