[PATCH v6 0/5] support rockchip dwc3 driver

[PATCH v6 0/5] support rockchip dwc3 driver

[William Wu]

This series add support for rockchip dwc3 driver,
and add additional optional properties for specific
platforms (e.g., rockchip rk3399 platform).

William Wu (5):
  usb: dwc3: of-simple: add compatible for rockchip rk3399
  usb: dwc3: add dis_u2_freeclk_exists_quirk
  usb: dwc3: add phyif_utmi_quirk
  usb: dwc3: add dis_del_phy_power_chg_quirk
  usb: dwc3: rockchip: add devicetree bindings documentation

 Documentation/devicetree/bindings/usb/dwc3.txt     |  9 ++++
 .../devicetree/bindings/usb/rockchip,dwc3.txt      | 59 ++++++++++++++++++++++
 drivers/usb/dwc3/core.c                            | 29 +++++++++++
 drivers/usb/dwc3/core.h                            | 20 ++++++++
 drivers/usb/dwc3/dwc3-of-simple.c                  |  1 +
 5 files changed, 118 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/rockchip,dwc3.txt

-- 
1.9.1

[PATCH v6 1/5] usb: dwc3: of-simple: add compatible for rockchip rk3399

[William Wu]

Rockchip platform merely enable usb3 clocks and
populate its children. So we can use this generic
glue layer to support Rockchip dwc3.

Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v6:
- None

Changes in v5:
- change compatible from "rockchip,dwc3" to "rockchip,rk3399-dwc3" (Heiko)

Changes in v4:
- None

Changes in v3:
- None

Changes in v2:
- sort the list of_dwc3_simple_match (Doug)

 drivers/usb/dwc3/dwc3-of-simple.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index 9743353..05c9349 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -161,6 +161,7 @@ static const struct dev_pm_ops dwc3_of_simple_dev_pm_ops = {
 
 static const struct of_device_id of_dwc3_simple_match[] = {
 	{ .compatible = "qcom,dwc3" },
+	{ .compatible = "rockchip,rk3399-dwc3" },
 	{ .compatible = "xlnx,zynqmp-dwc3" },
 	{ /* Sentinel */ }
 };
-- 
1.9.1

[PATCH v6 2/5] usb: dwc3: add dis_u2_freeclk_exists_quirk

[William Wu]

Add a quirk to clear the GUSB2PHYCFG.U2_FREECLK_EXISTS bit,
which specifies whether the USB2.0 PHY provides a free-running
PHY clock, which is active when the clock control input is active.

Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v6:
- use '-' instead of '_' in dts (Rob Herring)

Changes in v5:
- None

Changes in v4:
- rebase on top of balbi testing/next, remove pdata (balbi)

Changes in v3:
- None

Changes in v2:
- None

 Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
 drivers/usb/dwc3/core.c                        | 5 +++++
 drivers/usb/dwc3/core.h                        | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 7d7ce08..020b0e9 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -39,6 +39,9 @@ Optional properties:
 			disabling the suspend signal to the PHY.
  - snps,dis_rxdet_inp3_quirk: when set core will disable receiver detection
 			in PHY P3 power state.
+ - snps,dis-u2-freeclk-exists-quirk: when set, clear the u2_freeclk_exists
+			in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
+			a free-running PHY clock.
  - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
 			utmi_l1_suspend_n, false when asserts utmi_sleep_n
  - snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 9466431..0b7bfd2 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -500,6 +500,9 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	if (dwc->dis_enblslpm_quirk)
 		reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
 
+	if (dwc->dis_u2_freeclk_exists_quirk)
+		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
+
 	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
 
 	return 0;
@@ -924,6 +927,8 @@ static int dwc3_probe(struct platform_device *pdev)
 				"snps,dis_enblslpm_quirk");
 	dwc->dis_rxdet_inp3_quirk = device_property_read_bool(dev,
 				"snps,dis_rxdet_inp3_quirk");
+	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
+				"snps,dis-u2-freeclk-exists-quirk");
 
 	dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
 				"snps,tx_de_emphasis_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 45d6de5..f321a5c 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -199,6 +199,7 @@
 
 /* Global USB2 PHY Configuration Register */
 #define DWC3_GUSB2PHYCFG_PHYSOFTRST	(1 << 31)
+#define DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS	(1 << 30)
 #define DWC3_GUSB2PHYCFG_SUSPHY		(1 << 6)
 #define DWC3_GUSB2PHYCFG_ULPI_UTMI	(1 << 4)
 #define DWC3_GUSB2PHYCFG_ENBLSLPM	(1 << 8)
@@ -799,6 +800,9 @@ struct dwc3_scratchpad_array {
  * @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
  * @dis_enblslpm_quirk: set if we clear enblslpm in GUSB2PHYCFG,
  *                      disabling the suspend signal to the PHY.
+ * @dis_u2_freeclk_exists_quirk : set if we clear u2_freeclk_exists
+ *			in GUSB2PHYCFG, specify that USB2 PHY doesn't
+ *			provide a free-running PHY clock.
  * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
  * @tx_de_emphasis: Tx de-emphasis value
  * 	0	- -6dB de-emphasis
@@ -942,6 +946,7 @@ struct dwc3 {
 	unsigned		dis_u2_susphy_quirk:1;
 	unsigned		dis_enblslpm_quirk:1;
 	unsigned		dis_rxdet_inp3_quirk:1;
+	unsigned		dis_u2_freeclk_exists_quirk:1;
 
 	unsigned		tx_de_emphasis_quirk:1;
 	unsigned		tx_de_emphasis:2;
-- 
1.9.1

Re: [PATCH v6 2/5] usb: dwc3: add dis_u2_freeclk_exists_quirk

[Rob Herring]

On Thu, Jul 07, 2016 at 10:54:23AM +0800, William Wu wrote:
> Add a quirk to clear the GUSB2PHYCFG.U2_FREECLK_EXISTS bit,
> which specifies whether the USB2.0 PHY provides a free-running
> PHY clock, which is active when the clock control input is active.
> 
> Signed-off-by: William Wu <william.wu@rock-chips.com>
> ---
> Changes in v6:
> - use '-' instead of '_' in dts (Rob Herring)
> 
> Changes in v5:
> - None
> 
> Changes in v4:
> - rebase on top of balbi testing/next, remove pdata (balbi)
> 
> Changes in v3:
> - None
> 
> Changes in v2:
> - None
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
>  drivers/usb/dwc3/core.c                        | 5 +++++
>  drivers/usb/dwc3/core.h                        | 5 +++++
>  3 files changed, 13 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[William Wu]

Add a quirk to configure the core to support the
UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
interface is hardware property, and it's platform
dependent. Normall, the PHYIf can be configured
during coreconsultant. But for some specific usb
cores(e.g. rk3399 soc dwc3), the default PHYIf
configuration value is fault, so we need to
reconfigure it by software.

And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
must be set to the corresponding value according to
the UTMI+ PHY interface.

Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v6:
- use '-' instead of '_' in dts (Rob Herring)

Changes in v5:
- None

Changes in v4:
- rebase on top of balbi testing/next, remove pdata (balbi)

Changes in v3:
- None

Changes in v2:
- add a quirk for phyif_utmi (balbi)

 Documentation/devicetree/bindings/usb/dwc3.txt |  4 ++++
 drivers/usb/dwc3/core.c                        | 19 +++++++++++++++++++
 drivers/usb/dwc3/core.h                        | 12 ++++++++++++
 3 files changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 020b0e9..8d7317d 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -42,6 +42,10 @@ Optional properties:
  - snps,dis-u2-freeclk-exists-quirk: when set, clear the u2_freeclk_exists
 			in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
 			a free-running PHY clock.
+ - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface.
+ - snps,phyif-utmi: the value to configure the core to support a UTMI+ PHY
+			with an 8- or 16-bit interface. Value 0 select 8-bit
+			interface, value 1 select 16-bit interface.
  - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
 			utmi_l1_suspend_n, false when asserts utmi_sleep_n
  - snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 0b7bfd2..94036b1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
 static int dwc3_phy_setup(struct dwc3 *dwc)
 {
 	u32 reg;
+	u32 usbtrdtim;
 	int ret;
 
 	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
@@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	if (dwc->dis_u2_freeclk_exists_quirk)
 		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
 
+	if (dwc->phyif_utmi_quirk) {
+		reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK |
+		       DWC3_GUSB2PHYCFG_USBTRDTIM_MASK);
+		usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT :
+			    USBTRDTIM_UTMI_8_BIT;
+		reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) |
+		       DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim);
+	}
+
 	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
 
 	return 0;
@@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev)
 	struct resource		*res;
 	struct dwc3		*dwc;
 	u8			lpm_nyet_threshold;
+	u8			phyif_utmi;
 	u8			tx_de_emphasis;
 	u8			hird_threshold;
 
@@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev)
 	/* default to highest possible threshold */
 	lpm_nyet_threshold = 0xff;
 
+	/* default to UTMI+ 8-bit interface */
+	phyif_utmi = 0;
+
 	/* default to -3.5dB de-emphasis */
 	tx_de_emphasis = 1;
 
@@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev)
 				"snps,dis_rxdet_inp3_quirk");
 	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
 				"snps,dis-u2-freeclk-exists-quirk");
+	dwc->phyif_utmi_quirk = device_property_read_bool(dev,
+				"snps,phyif-utmi-quirk");
+	 device_property_read_u8(dev, "snps,phyif-utmi",
+				 &phyif_utmi);
 
 	dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
 				"snps,tx_de_emphasis_quirk");
@@ -940,6 +958,7 @@ static int dwc3_probe(struct platform_device *pdev)
 				 &dwc->fladj);
 
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
+	dwc->phyif_utmi = phyif_utmi;
 	dwc->tx_de_emphasis = tx_de_emphasis;
 
 	dwc->hird_threshold = hird_threshold
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index f321a5c..cf6696c 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -203,6 +203,12 @@
 #define DWC3_GUSB2PHYCFG_SUSPHY		(1 << 6)
 #define DWC3_GUSB2PHYCFG_ULPI_UTMI	(1 << 4)
 #define DWC3_GUSB2PHYCFG_ENBLSLPM	(1 << 8)
+#define DWC3_GUSB2PHYCFG_PHYIF(n)	(n << 3)
+#define DWC3_GUSB2PHYCFG_PHYIF_MASK	DWC3_GUSB2PHYCFG_PHYIF(1)
+#define DWC3_GUSB2PHYCFG_USBTRDTIM(n)	(n << 10)
+#define DWC3_GUSB2PHYCFG_USBTRDTIM_MASK	DWC3_GUSB2PHYCFG_USBTRDTIM(0xf)
+#define USBTRDTIM_UTMI_8_BIT		9
+#define USBTRDTIM_UTMI_16_BIT		5
 
 /* Global USB2 PHY Vendor Control Register */
 #define DWC3_GUSB2PHYACC_NEWREGREQ	(1 << 25)
@@ -803,6 +809,10 @@ struct dwc3_scratchpad_array {
  * @dis_u2_freeclk_exists_quirk : set if we clear u2_freeclk_exists
  *			in GUSB2PHYCFG, specify that USB2 PHY doesn't
  *			provide a free-running PHY clock.
+ * @phyif_utmi_quirk: set if we enable phyif UTMI+ quirk
+ * @phyif_utmi: UTMI+ PHY interface value
+ *	0	- 8 bits
+ *	1	- 16 bits
  * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
  * @tx_de_emphasis: Tx de-emphasis value
  * 	0	- -6dB de-emphasis
@@ -948,6 +958,8 @@ struct dwc3 {
 	unsigned		dis_rxdet_inp3_quirk:1;
 	unsigned		dis_u2_freeclk_exists_quirk:1;
 
+	unsigned		phyif_utmi_quirk:1;
+	unsigned		phyif_utmi:1;
 	unsigned		tx_de_emphasis_quirk:1;
 	unsigned		tx_de_emphasis:2;
 };
-- 
1.9.1

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[Heiko Stuebner]

Hi William,

Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:
> Add a quirk to configure the core to support the
> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
> interface is hardware property, and it's platform
> dependent. Normall, the PHYIf can be configured
> during coreconsultant. But for some specific usb
> cores(e.g. rk3399 soc dwc3), the default PHYIf
> configuration value is fault, so we need to
> reconfigure it by software.
> 
> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
> must be set to the corresponding value according to
> the UTMI+ PHY interface.
> 
> Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
[...]
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d
> 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -42,6 +42,10 @@ Optional properties:
>   - snps,dis-u2-freeclk-exists-quirk: when set, clear the
> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
>  			a free-running PHY clock.
> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface.
> + - snps,phyif-utmi: the value to configure the core to support a UTMI+
> PHY +			with an 8- or 16-bit interface. Value 0 select 8-bit
> +			interface, value 1 select 16-bit interface.

maybe
	snps,phyif-utmi-width = <8> or <16>;

devicetree is about describing the hardware, not the things that get written 
to registers :-) . The conversion from the described width to the register 
value can easily be done in the driver.


Also I don't think you need two properties for this. If the snps,phyif-utmi 
property is specified it indicates that you want to manually set the width 
and if it is absent you want to use the IC default. All functions reading 
property-values indicate if the property is missing.

But it looks like there is already a precendence in 
snps,tx_de_emphasis(_quirk), so maybe Felipe has a different opinion here?



>   - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
>  			utmi_l1_suspend_n, false when asserts utmi_sleep_n
>   - snps,hird-threshold: HIRD threshold
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 0b7bfd2..94036b1 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
>  static int dwc3_phy_setup(struct dwc3 *dwc)
>  {
>  	u32 reg;
> +	u32 usbtrdtim;
>  	int ret;
> 
>  	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> @@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  	if (dwc->dis_u2_freeclk_exists_quirk)
>  		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
> 
> +	if (dwc->phyif_utmi_quirk) {
> +		reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK |
> +		       DWC3_GUSB2PHYCFG_USBTRDTIM_MASK);
> +		usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT :
> +			    USBTRDTIM_UTMI_8_BIT;
> +		reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) |
> +		       DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim);
> +	}
> +
>  	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> 
>  	return 0;
> @@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	struct resource		*res;
>  	struct dwc3		*dwc;
>  	u8			lpm_nyet_threshold;
> +	u8			phyif_utmi;
>  	u8			tx_de_emphasis;
>  	u8			hird_threshold;
> 
> @@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev)
>  	/* default to highest possible threshold */
>  	lpm_nyet_threshold = 0xff;
> 
> +	/* default to UTMI+ 8-bit interface */
> +	phyif_utmi = 0;
> +
>  	/* default to -3.5dB de-emphasis */
>  	tx_de_emphasis = 1;
> 
> @@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev)
>  				"snps,dis_rxdet_inp3_quirk");
>  	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
>  				"snps,dis-u2-freeclk-exists-quirk");
> +	dwc->phyif_utmi_quirk = device_property_read_bool(dev,
> +				"snps,phyif-utmi-quirk");
> +	 device_property_read_u8(dev, "snps,phyif-utmi",
> +				 &phyif_utmi);


As described above device_property_read_u8 will return an error if the 
property is not present, so you could fill your dwc->phyif_utmi_quirk from 
that:

	ret = device_property_read_u8(dev, "snps,phyif-utmi",
				 &phyif_utmi);
	dwc->phyif_utmi_quirk = (ret == 0) ? true : false;


Heiko

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4819 bytes --]


Hi,

Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> writes:
> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:
>> Add a quirk to configure the core to support the
>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
>> interface is hardware property, and it's platform
>> dependent. Normall, the PHYIf can be configured
>> during coreconsultant. But for some specific usb
>> cores(e.g. rk3399 soc dwc3), the default PHYIf
>> configuration value is fault, so we need to
>> reconfigure it by software.
>> 
>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
>> must be set to the corresponding value according to
>> the UTMI+ PHY interface.
>> 
>> Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
> [...]
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>> b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d
>> 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -42,6 +42,10 @@ Optional properties:
>>   - snps,dis-u2-freeclk-exists-quirk: when set, clear the
>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
>>  			a free-running PHY clock.
>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface.
>> + - snps,phyif-utmi: the value to configure the core to support a UTMI+
>> PHY +			with an 8- or 16-bit interface. Value 0 select 8-bit
>> +			interface, value 1 select 16-bit interface.
>
> maybe
> 	snps,phyif-utmi-width = <8> or <16>;
>
> devicetree is about describing the hardware, not the things that get written 
> to registers :-) . The conversion from the described width to the register 
> value can easily be done in the driver.
>
>
> Also I don't think you need two properties for this. If the snps,phyif-utmi 
> property is specified it indicates that you want to manually set the width 
> and if it is absent you want to use the IC default. All functions reading 
> property-values indicate if the property is missing.
>
> But it looks like there is already a precendence in 
> snps,tx_de_emphasis(_quirk), so maybe Felipe has a different opinion here?
>
>
>
>>   - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
>>  			utmi_l1_suspend_n, false when asserts utmi_sleep_n
>>   - snps,hird-threshold: HIRD threshold
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 0b7bfd2..94036b1 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
>>  static int dwc3_phy_setup(struct dwc3 *dwc)
>>  {
>>  	u32 reg;
>> +	u32 usbtrdtim;
>>  	int ret;
>> 
>>  	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>> @@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>  	if (dwc->dis_u2_freeclk_exists_quirk)
>>  		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
>> 
>> +	if (dwc->phyif_utmi_quirk) {
>> +		reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK |
>> +		       DWC3_GUSB2PHYCFG_USBTRDTIM_MASK);
>> +		usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT :
>> +			    USBTRDTIM_UTMI_8_BIT;
>> +		reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) |
>> +		       DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim);
>> +	}
>> +
>>  	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> 
>>  	return 0;
>> @@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	struct resource		*res;
>>  	struct dwc3		*dwc;
>>  	u8			lpm_nyet_threshold;
>> +	u8			phyif_utmi;
>>  	u8			tx_de_emphasis;
>>  	u8			hird_threshold;
>> 
>> @@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev)
>>  	/* default to highest possible threshold */
>>  	lpm_nyet_threshold = 0xff;
>> 
>> +	/* default to UTMI+ 8-bit interface */
>> +	phyif_utmi = 0;
>> +
>>  	/* default to -3.5dB de-emphasis */
>>  	tx_de_emphasis = 1;
>> 
>> @@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev)
>>  				"snps,dis_rxdet_inp3_quirk");
>>  	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
>>  				"snps,dis-u2-freeclk-exists-quirk");
>> +	dwc->phyif_utmi_quirk = device_property_read_bool(dev,
>> +				"snps,phyif-utmi-quirk");
>> +	 device_property_read_u8(dev, "snps,phyif-utmi",
>> +				 &phyif_utmi);
>
>
> As described above device_property_read_u8 will return an error if the 
> property is not present, so you could fill your dwc->phyif_utmi_quirk from 
> that:
>
> 	ret = device_property_read_u8(dev, "snps,phyif-utmi",
> 				 &phyif_utmi);
> 	dwc->phyif_utmi_quirk = (ret == 0) ? true : false;

I like this much better :-) Unfortunately can't fix tx_deemphasis due to
backwards compatibility :-s

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[William.wu]

Dear Heiko & Balbi，


On 2016/7/8 21:29, Felipe Balbi wrote:
> Hi,
>
> Heiko Stuebner <heiko@sntech.de> writes:
>> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:
>>> Add a quirk to configure the core to support the
>>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
>>> interface is hardware property, and it's platform
>>> dependent. Normall, the PHYIf can be configured
>>> during coreconsultant. But for some specific usb
>>> cores(e.g. rk3399 soc dwc3), the default PHYIf
>>> configuration value is fault, so we need to
>>> reconfigure it by software.
>>>
>>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
>>> must be set to the corresponding value according to
>>> the UTMI+ PHY interface.
>>>
>>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>>> ---
>> [...]
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d
>>> 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -42,6 +42,10 @@ Optional properties:
>>>    - snps,dis-u2-freeclk-exists-quirk: when set, clear the
>>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
>>>   			a free-running PHY clock.
>>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface.
>>> + - snps,phyif-utmi: the value to configure the core to support a UTMI+
>>> PHY +			with an 8- or 16-bit interface. Value 0 select 8-bit
>>> +			interface, value 1 select 16-bit interface.
>> maybe
>> 	snps,phyif-utmi-width = <8> or <16>;
>>
>> devicetree is about describing the hardware, not the things that get written
>> to registers :-) . The conversion from the described width to the register
>> value can easily be done in the driver.
Thanks for your suggestion:-)
Yes， “snps,phyif-utmi-width = <8> or <16>” is much clearer and easier to 
understand.
And I have considered the same dts property for phyif-utmi, but I have 
no good idea about
the conversion from described width to the registers value for the time 
being.

About phyif utmi width configuration， we need to set two places in 
GUSB2PHYCFG register，
according to DWC3 USB3.0 controller databook version3.00a，6.3.46 
GUSB2PHYCFG

----------------------------------------------------------------------------------------------
     Bits   |  Name                 |     Description
----------------------------------------------------------------------------------------------
   13:10  |   USBTRDTIM       |     Sets the turnaround time in PHY clocks.
             |                            |     4'h5: When the MAC 
interface is 16-bit UTMI+
             |                            |     4'h9: When the MAC 
interface is 8-bit UTMI+/ULPI.
----------------------------------------------------------------------------------------------
   3        |   PHYIF                |    If UTMI+ is selected, the 
application uses this bit to configure
             |                            |    core to support a UTMI+ 
PHY with an 8- or 16-bit interface.
             |                            |    1'b0: 8 bits
             |                            |    1'b1: 16 bits
----------------------------------------------------------------------------------------------

And I think maybe I can try to do this:
change it in dts:
         snps,phyif-utmi-width = <8> or <16>;

Then convert to register value like this：
        device_property_read_u8(dev, "snps,phyif-utmi-width",
                                              &phyif_utmi_width);

        dwc->phyif_utmi = phyif_utmi_width >> 4;

  Ater the conversion， dwc->phyif_utmi value 0 means 8 bits, value 1 
means 16 bits,
  and it's easier for us to config GUSB2PHYCFG.

Is it OK?

>>
>>
>> Also I don't think you need two properties for this. If the snps,phyif-utmi
>> property is specified it indicates that you want to manually set the width
>> and if it is absent you want to use the IC default. All functions reading
>> property-values indicate if the property is missing.
Ah, it seems very good to me. One dts property "snps,phyif-utmi" can help to
reconfig phyif utmi width. And it seems that Felipe likes it very much 
too. :-D
>> But it looks like there is already a precendence in
>> snps,tx_de_emphasis(_quirk), so maybe Felipe has a different opinion here?
>>
>>
>>
>>>    - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
>>>   			utmi_l1_suspend_n, false when asserts utmi_sleep_n
>>>    - snps,hird-threshold: HIRD threshold
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 0b7bfd2..94036b1 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
>>>   static int dwc3_phy_setup(struct dwc3 *dwc)
>>>   {
>>>   	u32 reg;
>>> +	u32 usbtrdtim;
>>>   	int ret;
>>>
>>>   	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>>> @@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>>   	if (dwc->dis_u2_freeclk_exists_quirk)
>>>   		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
>>>
>>> +	if (dwc->phyif_utmi_quirk) {
>>> +		reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK |
>>> +		       DWC3_GUSB2PHYCFG_USBTRDTIM_MASK);
>>> +		usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT :
>>> +			    USBTRDTIM_UTMI_8_BIT;
>>> +		reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) |
>>> +		       DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim);
>>> +	}
>>> +
>>>   	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>>>
>>>   	return 0;
>>> @@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>   	struct resource		*res;
>>>   	struct dwc3		*dwc;
>>>   	u8			lpm_nyet_threshold;
>>> +	u8			phyif_utmi;
>>>   	u8			tx_de_emphasis;
>>>   	u8			hird_threshold;
>>>
>>> @@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev)
>>>   	/* default to highest possible threshold */
>>>   	lpm_nyet_threshold = 0xff;
>>>
>>> +	/* default to UTMI+ 8-bit interface */
>>> +	phyif_utmi = 0;
>>> +
>>>   	/* default to -3.5dB de-emphasis */
>>>   	tx_de_emphasis = 1;
>>>
>>> @@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev)
>>>   				"snps,dis_rxdet_inp3_quirk");
>>>   	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
>>>   				"snps,dis-u2-freeclk-exists-quirk");
>>> +	dwc->phyif_utmi_quirk = device_property_read_bool(dev,
>>> +				"snps,phyif-utmi-quirk");
>>> +	 device_property_read_u8(dev, "snps,phyif-utmi",
>>> +				 &phyif_utmi);
>>
>> As described above device_property_read_u8 will return an error if the
>> property is not present, so you could fill your dwc->phyif_utmi_quirk from
>> that:
>>
>> 	ret = device_property_read_u8(dev, "snps,phyif-utmi",
>> 				 &phyif_utmi);
>> 	dwc->phyif_utmi_quirk = (ret == 0) ? true : false;
> I like this much better :-) Unfortunately can't fix tx_deemphasis due to
> backwards compatibility :-s
I'll try to follow Heiko's suggestion and fix the dwc->phyif_utmi_quirk 
next patch.

Best regards,
        William.wu
>

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[Heiko Stuebner]

Am Samstag, 9. Juli 2016, 11:38:00 schrieb William.wu:
> Dear Heiko & Balbi，
> 
> On 2016/7/8 21:29, Felipe Balbi wrote:
> > Hi,
> > 
> > Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> writes:
> >> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:
> >>> Add a quirk to configure the core to support the
> >>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
> >>> interface is hardware property, and it's platform
> >>> dependent. Normall, the PHYIf can be configured
> >>> during coreconsultant. But for some specific usb
> >>> cores(e.g. rk3399 soc dwc3), the default PHYIf
> >>> configuration value is fault, so we need to
> >>> reconfigure it by software.
> >>> 
> >>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
> >>> must be set to the corresponding value according to
> >>> the UTMI+ PHY interface.
> >>> 
> >>> Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >>> ---
> >> 
> >> [...]
> >> 
> >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> b/Documentation/devicetree/bindings/usb/dwc3.txt index
> >>> 020b0e9..8d7317d
> >>> 100644
> >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>> 
> >>> @@ -42,6 +42,10 @@ Optional properties:
> >>>    - snps,dis-u2-freeclk-exists-quirk: when set, clear the
> >>> 
> >>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't
> >>> provide
> >>> 
> >>>   			a free-running PHY clock.
> >>> 
> >>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+
> >>> interface.
> >>> + - snps,phyif-utmi: the value to configure the core to support a
> >>> UTMI+
> >>> PHY +			with an 8- or 16-bit interface. Value 0 
select 8-bit
> >>> +			interface, value 1 select 16-bit interface.
> >> 
> >> maybe
> >> 
> >> 	snps,phyif-utmi-width = <8> or <16>;
> >> 
> >> devicetree is about describing the hardware, not the things that get
> >> written to registers :-) . The conversion from the described width to
> >> the register value can easily be done in the driver.
> 
> Thanks for your suggestion:-)
> Yes， “snps,phyif-utmi-width = <8> or <16>” is much clearer and easier to
> understand.
> And I have considered the same dts property for phyif-utmi, but I have
> no good idea about
> the conversion from described width to the registers value for the time
> being.
> 
> About phyif utmi width configuration， we need to set two places in
> GUSB2PHYCFG register，
> according to DWC3 USB3.0 controller databook version3.00a，6.3.46
> GUSB2PHYCFG
> 
> --------------------------------------------------------------------------
> -------------------- Bits   |  Name                 |     Description
> --------------------------------------------------------------------------
> -------------------- 13:10  |   USBTRDTIM       |     Sets the turnaround
> time in PHY clocks.
>              |                            |     4'h5: When the MAC
> 
> interface is 16-bit UTMI+
> 
>              |                            |     4'h9: When the MAC
> 
> interface is 8-bit UTMI+/ULPI.
> --------------------------------------------------------------------------
> -------------------- 3        |   PHYIF                |    If UTMI+ is
> selected, the application uses this bit to configure
> 
>              |                            |    core to support a UTMI+
> 
> PHY with an 8- or 16-bit interface.
> 
>              |                            |    1'b0: 8 bits
>              |                            |    1'b1: 16 bits
> 
> --------------------------------------------------------------------------
> --------------------
> 
> And I think maybe I can try to do this:
> change it in dts:
>          snps,phyif-utmi-width = <8> or <16>;
> 
> Then convert to register value like this：
>         device_property_read_u8(dev, "snps,phyif-utmi-width",
>                                               &phyif_utmi_width);
> 
>         dwc->phyif_utmi = phyif_utmi_width >> 4;
> 
>   Ater the conversion， dwc->phyif_utmi value 0 means 8 bits, value 1
> means 16 bits,
>   and it's easier for us to config GUSB2PHYCFG.
> 
> Is it OK?

or you could just store the actual width value read from the dts and make 
the core handle accordingly, making everything a bit more explicit.

I guess personally I'd do something like:

make dwc->phyif_utmi a regular unsigned int

in probe:
      	ret = device_property_read_u8(dev, "snps,phyif-utmi-width",
                                              &dwc->phyif_utmi);
	if (ret < 0) {
		dwc->phyif_utmi = 0;
	else if (dwc->phyif_utmi != 16 && dwc->phyif_utmi != 8) {
		dev_err(dev, "unsupported utmi interface width %d\n",
			dwc->phyif_utmi);
		return -EINVAL;
	}


when setting your GUSB2PHYCFG register:

       if (dwc->phyif_utmi > 0) {
               reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK |
                      DWC3_GUSB2PHYCFG_USBTRDTIM_MASK);
               usbtrdtim = (dwc->phyif_utmi == 16) ? USBTRDTIM_UTMI_16_BIT :
                           USBTRDTIM_UTMI_8_BIT;
		phyif = (dwc->phyif_utmi == 16) ? 1 : 0;
               reg |= DWC3_GUSB2PHYCFG_PHYIF(phyif) |
                      DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim) |
       }


> >> Also I don't think you need two properties for this. If the
> >> snps,phyif-utmi property is specified it indicates that you want to
> >> manually set the width and if it is absent you want to use the IC
> >> default. All functions reading property-values indicate if the
> >> property is missing.
> 
> Ah, it seems very good to me. One dts property "snps,phyif-utmi" can help
> to reconfig phyif utmi width. And it seems that Felipe likes it very much
> too. :-D

yes, but please name it snps,phyif-utmi-width :-)


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[William.wu]

Dear Heiko,


On 2016/7/10 7:47, Heiko Stuebner wrote:
> Am Samstag, 9. Juli 2016, 11:38:00 schrieb William.wu:
>> Dear Heiko & Balbi，
>>
>> On 2016/7/8 21:29, Felipe Balbi wrote:
>>> Hi,
>>>
>>> Heiko Stuebner <heiko@sntech.de> writes:
>>>> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:
>>>>> Add a quirk to configure the core to support the
>>>>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
>>>>> interface is hardware property, and it's platform
>>>>> dependent. Normall, the PHYIf can be configured
>>>>> during coreconsultant. But for some specific usb
>>>>> cores(e.g. rk3399 soc dwc3), the default PHYIf
>>>>> configuration value is fault, so we need to
>>>>> reconfigure it by software.
>>>>>
>>>>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
>>>>> must be set to the corresponding value according to
>>>>> the UTMI+ PHY interface.
>>>>>
>>>>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>>>>> ---
>>>> [...]
>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt index
>>>>> 020b0e9..8d7317d
>>>>> 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>
>>>>> @@ -42,6 +42,10 @@ Optional properties:
>>>>>     - snps,dis-u2-freeclk-exists-quirk: when set, clear the
>>>>>
>>>>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't
>>>>> provide
>>>>>
>>>>>    			a free-running PHY clock.
>>>>>
>>>>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+
>>>>> interface.
>>>>> + - snps,phyif-utmi: the value to configure the core to support a
>>>>> UTMI+
>>>>> PHY +			with an 8- or 16-bit interface. Value 0
> select 8-bit
>>>>> +			interface, value 1 select 16-bit interface.
>>>> maybe
>>>>
>>>> 	snps,phyif-utmi-width = <8> or <16>;
>>>>
>>>> devicetree is about describing the hardware, not the things that get
>>>> written to registers :-) . The conversion from the described width to
>>>> the register value can easily be done in the driver.
>> Thanks for your suggestion:-)
>> Yes， “snps,phyif-utmi-width = <8> or <16>” is much clearer and easier to
>> understand.
>> And I have considered the same dts property for phyif-utmi, but I have
>> no good idea about
>> the conversion from described width to the registers value for the time
>> being.
>>
>> About phyif utmi width configuration， we need to set two places in
>> GUSB2PHYCFG register，
>> according to DWC3 USB3.0 controller databook version3.00a，6.3.46
>> GUSB2PHYCFG
>>
>> --------------------------------------------------------------------------
>> -------------------- Bits   |  Name                 |     Description
>> --------------------------------------------------------------------------
>> -------------------- 13:10  |   USBTRDTIM       |     Sets the turnaround
>> time in PHY clocks.
>>               |                            |     4'h5: When the MAC
>>
>> interface is 16-bit UTMI+
>>
>>               |                            |     4'h9: When the MAC
>>
>> interface is 8-bit UTMI+/ULPI.
>> --------------------------------------------------------------------------
>> -------------------- 3        |   PHYIF                |    If UTMI+ is
>> selected, the application uses this bit to configure
>>
>>               |                            |    core to support a UTMI+
>>
>> PHY with an 8- or 16-bit interface.
>>
>>               |                            |    1'b0: 8 bits
>>               |                            |    1'b1: 16 bits
>>
>> --------------------------------------------------------------------------
>> --------------------
>>
>> And I think maybe I can try to do this:
>> change it in dts:
>>           snps,phyif-utmi-width = <8> or <16>;
>>
>> Then convert to register value like this：
>>          device_property_read_u8(dev, "snps,phyif-utmi-width",
>>                                                &phyif_utmi_width);
>>
>>          dwc->phyif_utmi = phyif_utmi_width >> 4;
>>
>>    Ater the conversion， dwc->phyif_utmi value 0 means 8 bits, value 1
>> means 16 bits,
>>    and it's easier for us to config GUSB2PHYCFG.
>>
>> Is it OK?
> or you could just store the actual width value read from the dts and make
> the core handle accordingly, making everything a bit more explicit.
>
> I guess personally I'd do something like:
>
> make dwc->phyif_utmi a regular unsigned int
>
> in probe:
>        	ret = device_property_read_u8(dev, "snps,phyif-utmi-width",
>                                                &dwc->phyif_utmi);
> 	if (ret < 0) {
> 		dwc->phyif_utmi = 0;
> 	else if (dwc->phyif_utmi != 16 && dwc->phyif_utmi != 8) {
> 		dev_err(dev, "unsupported utmi interface width %d\n",
> 			dwc->phyif_utmi);
> 		return -EINVAL;
> 	}
>
>
> when setting your GUSB2PHYCFG register:
>
>         if (dwc->phyif_utmi > 0) {
>                 reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK |
>                        DWC3_GUSB2PHYCFG_USBTRDTIM_MASK);
>                 usbtrdtim = (dwc->phyif_utmi == 16) ? USBTRDTIM_UTMI_16_BIT :
>                             USBTRDTIM_UTMI_8_BIT;
> 		phyif = (dwc->phyif_utmi == 16) ? 1 : 0;
>                 reg |= DWC3_GUSB2PHYCFG_PHYIF(phyif) |
>                        DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim) |
>         }
>
Ah yes， it seems very good to me, very explicit.
I'll upload a new patch according to your suggestion today.
Thanks a lot~:-D
>>>> Also I don't think you need two properties for this. If the
>>>> snps,phyif-utmi property is specified it indicates that you want to
>>>> manually set the width and if it is absent you want to use the IC
>>>> default. All functions reading property-values indicate if the
>>>> property is missing.
>> Ah, it seems very good to me. One dts property "snps,phyif-utmi" can help
>> to reconfig phyif utmi width. And it seems that Felipe likes it very much
>> too. :-D
> yes, but please name it snps,phyif-utmi-width :-)

Yeah, thank you very much:-)

Best Regards
         William Wu
>
>
> Heiko
>
>
>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[Rob Herring]

On Fri, Jul 08, 2016 at 02:33:09PM +0200, Heiko Stuebner wrote:
> Hi William,
> 
> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:
> > Add a quirk to configure the core to support the
> > UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
> > interface is hardware property, and it's platform
> > dependent. Normall, the PHYIf can be configured
> > during coreconsultant. But for some specific usb
> > cores(e.g. rk3399 soc dwc3), the default PHYIf
> > configuration value is fault, so we need to
> > reconfigure it by software.
> > 
> > And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
> > must be set to the corresponding value according to
> > the UTMI+ PHY interface.
> > 
> > Signed-off-by: William Wu <william.wu@rock-chips.com>
> > ---
> [...]
> > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
> > b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d
> > 100644
> > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > @@ -42,6 +42,10 @@ Optional properties:
> >   - snps,dis-u2-freeclk-exists-quirk: when set, clear the
> > u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
> >  			a free-running PHY clock.
> > + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface.
> > + - snps,phyif-utmi: the value to configure the core to support a UTMI+
> > PHY +			with an 8- or 16-bit interface. Value 0 select 8-bit
> > +			interface, value 1 select 16-bit interface.
> 
> maybe
> 	snps,phyif-utmi-width = <8> or <16>;

Seems like this could be common. Any other bindings have something 
similar already? If not "utmi-width" is fine.

> 
> devicetree is about describing the hardware, not the things that get written 
> to registers :-) . The conversion from the described width to the register 
> value can easily be done in the driver.
> 
> 
> Also I don't think you need two properties for this. If the snps,phyif-utmi 
> property is specified it indicates that you want to manually set the width 
> and if it is absent you want to use the IC default. All functions reading 
> property-values indicate if the property is missing.

Agreed.

Rob

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[William.wu]

Dear Rob,


On 2016/7/11 22:54, Rob Herring wrote:
> On Fri, Jul 08, 2016 at 02:33:09PM +0200, Heiko Stuebner wrote:
>> Hi William,
>>
>> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu:
>>> Add a quirk to configure the core to support the
>>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
>>> interface is hardware property, and it's platform
>>> dependent. Normall, the PHYIf can be configured
>>> during coreconsultant. But for some specific usb
>>> cores(e.g. rk3399 soc dwc3), the default PHYIf
>>> configuration value is fault, so we need to
>>> reconfigure it by software.
>>>
>>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
>>> must be set to the corresponding value according to
>>> the UTMI+ PHY interface.
>>>
>>> Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>> ---
>> [...]
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d
>>> 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -42,6 +42,10 @@ Optional properties:
>>>    - snps,dis-u2-freeclk-exists-quirk: when set, clear the
>>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
>>>   			a free-running PHY clock.
>>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface.
>>> + - snps,phyif-utmi: the value to configure the core to support a UTMI+
>>> PHY +			with an 8- or 16-bit interface. Value 0 select 8-bit
>>> +			interface, value 1 select 16-bit interface.
>> maybe
>> 	snps,phyif-utmi-width = <8> or <16>;
> Seems like this could be common. Any other bindings have something
> similar already? If not "utmi-width" is fine.

It seems that there's not any dwc3 binding similar to this.
So I prefer to use “utmi-width”. :-)

>
>> devicetree is about describing the hardware, not the things that get written
>> to registers :-) . The conversion from the described width to the register
>> value can easily be done in the driver.
>>
>>
>> Also I don't think you need two properties for this. If the snps,phyif-utmi
>> property is specified it indicates that you want to manually set the width
>> and if it is absent you want to use the IC default. All functions reading
>> property-values indicate if the property is missing.
> Agreed.
>
> Rob
>
>
>

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[Rob Herring]

On Thu, Jul 07, 2016 at 10:54:24AM +0800, William Wu wrote:
> Add a quirk to configure the core to support the
> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
> interface is hardware property, and it's platform
> dependent. Normall, the PHYIf can be configured

s/Normall/Normally/
s/PHYIf/PHYIF/

> during coreconsultant. But for some specific usb

s/usb/USB/

> cores(e.g. rk3399 soc dwc3), the default PHYIf
> configuration value is fault, so we need to
> reconfigure it by software.
> 
> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM

s/dwc3/DWC3/

> must be set to the corresponding value according to
> the UTMI+ PHY interface.

And wrap your lines at 70-74 characters.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 3/5] usb: dwc3: add phyif_utmi_quirk

[William.wu]

Dear Rob,


On 2016/7/11 22:58, Rob Herring wrote:
> On Thu, Jul 07, 2016 at 10:54:24AM +0800, William Wu wrote:
>> Add a quirk to configure the core to support the
>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY
>> interface is hardware property, and it's platform
>> dependent. Normall, the PHYIf can be configured
> s/Normall/Normally/
Yeah，I'll fix it.:-)
> s/PHYIf/PHYIF/
Refer to DWC3 controller databook, "PHY Interface" called "PHYIf",
so I describe "PHYIf" here. However， "PHYIF”seems more the norm,
I'll fix it.:-)
>
>> during coreconsultant. But for some specific usb
> s/usb/USB/

Thanks, I'll fix it.:-)

>
>> cores(e.g. rk3399 soc dwc3), the default PHYIf
>> configuration value is fault, so we need to
>> reconfigure it by software.
>>
>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM
> s/dwc3/DWC3/
Thanks, I'll fix it too.
>
>> must be set to the corresponding value according to
>> the UTMI+ PHY interface.
> And wrap your lines at 70-74 characters.

Thanks for your suggestion, I'll pay attention to this problem next 
patch.:-)

Best Regards，
         William Wu
>
> Rob
>
>
>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v6 4/5] usb: dwc3: add dis_del_phy_power_chg_quirk

[William Wu]

Add a quirk to clear the GUSB3PIPECTL.DELAYP1TRANS bit,
which specifies whether disable delay PHY power change
from P0 to P1/P2/P3 when link state changing from U0
to U1/U2/U3 respectively.

Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v6:
- use '-' instead of '_' in dts (Rob Herring)

Changes in v5:
- None

Changes in v4:
- rebase on top of balbi testing/next, remove pdata (balbi)

Changes in v3:
- None

Changes in v2:
- None

 Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
 drivers/usb/dwc3/core.c                        | 5 +++++
 drivers/usb/dwc3/core.h                        | 3 +++
 3 files changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 8d7317d..1c140df 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -42,6 +42,8 @@ Optional properties:
  - snps,dis-u2-freeclk-exists-quirk: when set, clear the u2_freeclk_exists
 			in GUSB2PHYCFG, specify that USB2 PHY doesn't provide
 			a free-running PHY clock.
+ - snps,dis-del-phy-power-chg-quirk: when set core will change PHY power
+			from P0 to P1/P2/P3 without delay.
  - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface.
  - snps,phyif-utmi: the value to configure the core to support a UTMI+ PHY
 			with an 8- or 16-bit interface. Value 0 select 8-bit
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 94036b1..e79d6a4 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -449,6 +449,9 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	if (dwc->dis_u3_susphy_quirk)
 		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
 
+	if (dwc->dis_del_phy_power_chg_quirk)
+		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
+
 	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
 
 	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
@@ -943,6 +946,8 @@ static int dwc3_probe(struct platform_device *pdev)
 				"snps,dis_rxdet_inp3_quirk");
 	dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev,
 				"snps,dis-u2-freeclk-exists-quirk");
+	dwc->dis_del_phy_power_chg_quirk = device_property_read_bool(dev,
+				"snps,dis-del-phy-power-chg-quirk");
 	dwc->phyif_utmi_quirk = device_property_read_bool(dev,
 				"snps,phyif-utmi-quirk");
 	 device_property_read_u8(dev, "snps,phyif-utmi",
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index cf6696c..55e136d 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -809,6 +809,8 @@ struct dwc3_scratchpad_array {
  * @dis_u2_freeclk_exists_quirk : set if we clear u2_freeclk_exists
  *			in GUSB2PHYCFG, specify that USB2 PHY doesn't
  *			provide a free-running PHY clock.
+ * @dis_del_phy_power_chg_quirk: set if we disable delay phy power
+ *			change quirk.
  * @phyif_utmi_quirk: set if we enable phyif UTMI+ quirk
  * @phyif_utmi: UTMI+ PHY interface value
  *	0	- 8 bits
@@ -957,6 +959,7 @@ struct dwc3 {
 	unsigned		dis_enblslpm_quirk:1;
 	unsigned		dis_rxdet_inp3_quirk:1;
 	unsigned		dis_u2_freeclk_exists_quirk:1;
+	unsigned		dis_del_phy_power_chg_quirk:1;
 
 	unsigned		phyif_utmi_quirk:1;
 	unsigned		phyif_utmi:1;
-- 
1.9.1

Re: [PATCH v6 4/5] usb: dwc3: add dis_del_phy_power_chg_quirk

[Rob Herring]

On Thu, Jul 07, 2016 at 10:54:25AM +0800, William Wu wrote:
> Add a quirk to clear the GUSB3PIPECTL.DELAYP1TRANS bit,
> which specifies whether disable delay PHY power change
> from P0 to P1/P2/P3 when link state changing from U0
> to U1/U2/U3 respectively.
> 
> Signed-off-by: William Wu <william.wu@rock-chips.com>
> ---
> Changes in v6:
> - use '-' instead of '_' in dts (Rob Herring)
> 
> Changes in v5:
> - None
> 
> Changes in v4:
> - rebase on top of balbi testing/next, remove pdata (balbi)
> 
> Changes in v3:
> - None
> 
> Changes in v2:
> - None
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
>  drivers/usb/dwc3/core.c                        | 5 +++++
>  drivers/usb/dwc3/core.h                        | 3 +++
>  3 files changed, 10 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v6 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William Wu]

This patch adds the devicetree documentation required for Rockchip
USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.

It supports DRD mode, and could operate in device mode (SS, HS, FS)
and host mode (SS, HS, FS, LS).

Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v6:
- rename bus_clk, and add usbdrd3_1 node as an example (Heiko)

Changes in v5:
- rename clock-names, and remove unnecessary clocks (Heiko)

Changes in v4:
- modify commit log, and add phy documentation location (Sergei)

Changes in v3:
- add dwc3 address (balbi)

Changes in v2:
- add rockchip,dwc3.txt to Documentation/devicetree/bindings/ (balbi, Brian)

 .../devicetree/bindings/usb/rockchip,dwc3.txt      | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/rockchip,dwc3.txt

diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
new file mode 100644
index 0000000..0536a93
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
@@ -0,0 +1,59 @@
+Rockchip SuperSpeed DWC3 USB SoC controller
+
+Required properties:
+- compatible:	should contain "rockchip,rk3399-dwc3" for rk3399 SoC
+- clocks:	A list of phandle + clock-specifier pairs for the
+		clocks listed in clock-names
+- clock-names:	Should contain the following:
+  "ref_clk"	Controller reference clk, have to be 24 MHz
+  "suspend_clk"	Controller suspend clk, have to be 24 MHz or 32 KHz
+  "bus_clk"	Master/Core clock, have to be >= 62.5 MHz for SS
+		operation and >= 30MHz for HS operation
+  "grf_clk"	Controller grf clk
+
+Required child node:
+A child node must exist to represent the core DWC3 IP block. The name of
+the node is not important. The content of the node is defined in dwc3.txt.
+
+Phy documentation is provided in the following places:
+Documentation/devicetree/bindings/phy/rockchip,dwc3-usb-phy.txt
+
+Example device nodes:
+
+	usbdrd3_0: usb@fe800000 {
+		compatible = "rockchip,rk3399-dwc3";
+		clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>,
+			 <&cru ACLK_USB3OTG0>, <&cru ACLK_USB3_GRF>;
+		clock-names = "ref_clk", "suspend_clk",
+			      "bus_clk", "grf_clk";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		status = "disabled";
+		usbdrd_dwc3_0: dwc3@fe800000 {
+			compatible = "snps,dwc3";
+			reg = <0x0 0xfe800000 0x0 0x100000>;
+			interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
+			dr_mode = "otg";
+			status = "disabled";
+		};
+	};
+
+	usbdrd3_1: usb@fe900000 {
+		compatible = "rockchip,rk3399-dwc3";
+		clocks = <&cru SCLK_USB3OTG1_REF>, <&cru SCLK_USB3OTG1_SUSPEND>,
+			 <&cru ACLK_USB3OTG1>, <&cru ACLK_USB3_GRF>;
+		clock-names = "ref_clk", "suspend_clk",
+			      "bus_clk", "grf_clk";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		status = "disabled";
+		usbdrd_dwc3_1: dwc3@fe900000 {
+			compatible = "snps,dwc3";
+			reg = <0x0 0xfe900000 0x0 0x100000>;
+			interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+			dr_mode = "otg";
+			status = "disabled";
+		};
+	};
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Rob Herring]

On Thu, Jul 07, 2016 at 10:58:44AM +0800, William Wu wrote:
> This patch adds the devicetree documentation required for Rockchip
> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
> 
> It supports DRD mode, and could operate in device mode (SS, HS, FS)
> and host mode (SS, HS, FS, LS).
> 
> Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> Changes in v6:
> - rename bus_clk, and add usbdrd3_1 node as an example (Heiko)
> 
> Changes in v5:
> - rename clock-names, and remove unnecessary clocks (Heiko)
> 
> Changes in v4:
> - modify commit log, and add phy documentation location (Sergei)
> 
> Changes in v3:
> - add dwc3 address (balbi)
> 
> Changes in v2:
> - add rockchip,dwc3.txt to Documentation/devicetree/bindings/ (balbi, Brian)
> 
>  .../devicetree/bindings/usb/rockchip,dwc3.txt      | 59 ++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/rockchip,dwc3.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William.wu]

Dear Rob,


On 2016/7/11 23:13, Rob Herring wrote:
> On Thu, Jul 07, 2016 at 10:58:44AM +0800, William Wu wrote:
>> This patch adds the devicetree documentation required for Rockchip
>> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
>>
>> It supports DRD mode, and could operate in device mode (SS, HS, FS)
>> and host mode (SS, HS, FS, LS).
>>
>> Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> ---
>> Changes in v6:
>> - rename bus_clk, and add usbdrd3_1 node as an example (Heiko)
>>
>> Changes in v5:
>> - rename clock-names, and remove unnecessary clocks (Heiko)
>>
>> Changes in v4:
>> - modify commit log, and add phy documentation location (Sergei)
>>
>> Changes in v3:
>> - add dwc3 address (balbi)
>>
>> Changes in v2:
>> - add rockchip,dwc3.txt to Documentation/devicetree/bindings/ (balbi, Brian)
>>
>>   .../devicetree/bindings/usb/rockchip,dwc3.txt      | 59 ++++++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks a lot. I'll add Acked-by next patch.

>
>
>