[RFC][PATCH v2 0/5] dwc3: Changes for HiKey960 support

[RFC][PATCH v2 0/5] dwc3: Changes for HiKey960 support

[John Stultz]

I've been carrying for awhile some patches that Yu Chen was
previously pushing upstream to enable USB on the HiKey960 board
and I wanted to try to nudge them forward as I'm not sure as to
what his plans are.

This series is just the simpler parts of the patch set that I
wanted to send out to see if we could make some progress on
while I continue to work on the more complex bits.

You can find the full set of changes to get USB working on the
board here:
  https://git.linaro.org/people/john.stultz/android-dev.git/log/?id=ef858be80f202b7bffb7d03c168ee72457a0ef3e

This series is just the more trivial changes, along with some
missing binding documentation that I've added.

I'd greatly appreciate any review or feedback on this series!

thanks
-john

New in v2:
* Tweaked binding clock name as clk_usb3phy_ref didn't seem right.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org

John Stultz (2):
  dt-bindings: usb: dwc3: Add a property to do a CGTL soft reset on mode
    switching
  dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

Yu Chen (3):
  usb: dwc3: Execute GCTL Core Soft Reset while switch mdoe for
    Hisilicon Kirin Soc
  usb: dwc3: Increase timeout for CmdAct cleared by device controller
  usb: dwc3: dwc3-of-simple: Add support for dwc3 of Hisilicon Soc
    Platform

 .../devicetree/bindings/usb/dwc3.txt          |  2 +
 .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
 drivers/usb/dwc3/core.c                       | 20 +++++++
 drivers/usb/dwc3/core.h                       |  3 ++
 drivers/usb/dwc3/dwc3-of-simple.c             |  4 +-
 drivers/usb/dwc3/gadget.c                     |  2 +-
 6 files changed, 81 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt

-- 
2.17.1

[RFC][PATCH v2 1/5] dt-bindings: usb: dwc3: Add a property to do a CGTL soft reset on mode switching

[John Stultz]

Provide a dt-binding for quirk needed to do a GCTL soft reset on mode
switching

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 66780a47ad85..cf4ef6c22fb3 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -77,6 +77,8 @@ Optional properties:
 			during HS transmit.
  - snps,dis_metastability_quirk: when set, disable metastability workaround.
 			CAUTION: use only if you are absolutely sure of it.
+ - snps,gctl-reset-quirk: when set, GCTL soft reset will be executed on mode
+			switch.
  - 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
-- 
2.17.1

[RFC][PATCH v2 2/5] usb: dwc3: Execute GCTL Core Soft Reset while switch mdoe for Hisilicon Kirin Soc

[John Stultz]

From: Yu Chen <chenyu56@huawei.com>

A GCTL soft reset should be executed when switch mode for dwc3 core
of Hisilicon Kirin Soc.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/usb/dwc3/core.c | 20 ++++++++++++++++++++
 drivers/usb/dwc3/core.h |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 999ce5e84d3c..440261432421 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
 	dwc->current_dr_role = mode;
 }
 
+static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
+{
+	u32 reg;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+	reg |= DWC3_GCTL_CORESOFTRESET;
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+	reg &= ~DWC3_GCTL_CORESOFTRESET;
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+}
+
 static void __dwc3_set_mode(struct work_struct *work)
 {
 	struct dwc3 *dwc = work_to_dwc(work);
@@ -156,6 +169,10 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
+	/* Execute a GCTL Core Soft Reset when switch mode */
+	if (dwc->gctl_reset_quirk)
+		dwc3_gctl_core_soft_reset(dwc);
+
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	switch (dwc->desired_dr_role) {
@@ -1316,6 +1333,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	dwc->dis_metastability_quirk = device_property_read_bool(dev,
 				"snps,dis_metastability_quirk");
 
+	dwc->gctl_reset_quirk = device_property_read_bool(dev,
+				"snps,gctl-reset-quirk");
+
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
 	dwc->tx_de_emphasis = tx_de_emphasis;
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1c8b349379af..b3cb6eec3f8f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1029,6 +1029,7 @@ struct dwc3_scratchpad_array {
  * 	2	- No de-emphasis
  * 	3	- Reserved
  * @dis_metastability_quirk: set to disable metastability quirk.
+ * @gctl_reset_quirk: set to do a gctl soft-reset while switch operation mode.
  * @imod_interval: set the interrupt moderation interval in 250ns
  *                 increments or 0 to disable.
  */
@@ -1219,6 +1220,8 @@ struct dwc3 {
 
 	unsigned		dis_metastability_quirk:1;
 
+	unsigned		gctl_reset_quirk:1;
+
 	u16			imod_interval;
 };
 
-- 
2.17.1

[RFC][PATCH v2 3/5] usb: dwc3: Increase timeout for CmdAct cleared by device controller

[John Stultz]

From: Yu Chen <chenyu56@huawei.com>

It needs more time for the device controller to clear the CmdAct of
DEPCMD on Hisilicon Kirin Soc.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8adb59f8e4f1..81907e163252 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -270,7 +270,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
 {
 	const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
 	struct dwc3		*dwc = dep->dwc;
-	u32			timeout = 1000;
+	u32			timeout = 5000;
 	u32			saved_config = 0;
 	u32			reg;
 
-- 
2.17.1

[RFC][PATCH v2 4/5] dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

[John Stultz]

Add necessary compatible flag for HiSi's DWC3 so
dwc3-of-simple will probe.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2: Tweaked clock names as clk_usb3phy_ref didn't seem right.
---
 .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt

diff --git a/Documentation/devicetree/bindings/usb/hisi,dwc3.txt b/Documentation/devicetree/bindings/usb/hisi,dwc3.txt
new file mode 100644
index 000000000000..3a3e5c320f2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/hisi,dwc3.txt
@@ -0,0 +1,52 @@
+HiSi SuperSpeed DWC3 USB SoC controller
+
+Required properties:
+- compatible:		should contain "hisilicon,hi3660-dwc3" for HiSi SoC
+- clocks:		A list of phandle + clock-specifier pairs for the
+			clocks listed in clock-names
+- clock-names:		Should contain the following:
+  "clk_abb_usb"		USB reference clk
+  "aclk_usb3otg"	USB3 OTG aclk
+
+- assigned-clocks:	Should be:
+				HI3660_ACLK_GATE_USB3OTG
+- assigned-clock-rates: Should be:
+				229Mhz (229000000) for HI3660_ACLK_GATE_USB3OTG
+
+Optional properties:
+- resets:		Phandle to reset control that resets core and wrapper.
+
+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.
+
+Example device nodes:
+
+	usb3: hisi_dwc3 {
+		compatible = "hisilicon,hi3660-dwc3";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		clocks = <&crg_ctrl HI3660_CLK_ABB_USB>,
+			 <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
+		clock-names = "clk_abb_usb", "aclk_usb3otg";
+
+		assigned-clocks = <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
+		assigned-clock-rates = <229 000 000>;
+		resets = <&crg_rst 0x90 8>,
+			 <&crg_rst 0x90 7>,
+			 <&crg_rst 0x90 6>,
+			 <&crg_rst 0x90 5>;
+
+		dwc3: dwc3@ff100000 {
+			compatible = "snps,dwc3";
+			reg = <0x0 0xff100000 0x0 0x100000>;
+			interrupts = <0 159 4>, <0 161 4>;
+			phys = <&usb_phy>;
+			phy-names = "usb3-phy";
+			dr_mode = "otg";
+
+			...
+		};
+	};
-- 
2.17.1

[RFC][PATCH v2 5/5] usb: dwc3: dwc3-of-simple: Add support for dwc3 of Hisilicon Soc Platform

[John Stultz]

From: Yu Chen <chenyu56@huawei.com>

This patch adds support for the poweron and shutdown of dwc3 core
on Hisilicon Soc Platform.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index bdac3e7d7b18..78617500edee 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -51,7 +51,8 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 	 * Some controllers need to toggle the usb3-otg reset before trying to
 	 * initialize the PHY, otherwise the PHY times out.
 	 */
-	if (of_device_is_compatible(np, "rockchip,rk3399-dwc3"))
+	if (of_device_is_compatible(np, "rockchip,rk3399-dwc3") ||
+	    of_device_is_compatible(np, "hisilicon,hi3660-dwc3"))
 		simple->need_reset = true;
 
 	if (of_device_is_compatible(np, "amlogic,meson-axg-dwc3") ||
@@ -183,6 +184,7 @@ static const struct of_device_id of_dwc3_simple_match[] = {
 	{ .compatible = "amlogic,meson-axg-dwc3" },
 	{ .compatible = "amlogic,meson-gxl-dwc3" },
 	{ .compatible = "allwinner,sun50i-h6-dwc3" },
+	{ .compatible = "hisilicon,hi3660-dwc3" },
 	{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, of_dwc3_simple_match);
-- 
2.17.1

Re: [RFC][PATCH v2 4/5] dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

[Rob Herring]

On Mon, Oct 7, 2019 at 12:56 PM John Stultz <john.stultz@linaro.org> wrote:
>
> Add necessary compatible flag for HiSi's DWC3 so
> dwc3-of-simple will probe.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Yu Chen <chenyu56@huawei.com>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2: Tweaked clock names as clk_usb3phy_ref didn't seem right.
> ---
>  .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt

Can you make this a schema.

> diff --git a/Documentation/devicetree/bindings/usb/hisi,dwc3.txt b/Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> new file mode 100644
> index 000000000000..3a3e5c320f2a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> @@ -0,0 +1,52 @@
> +HiSi SuperSpeed DWC3 USB SoC controller
> +
> +Required properties:
> +- compatible:          should contain "hisilicon,hi3660-dwc3" for HiSi SoC
> +- clocks:              A list of phandle + clock-specifier pairs for the
> +                       clocks listed in clock-names
> +- clock-names:         Should contain the following:
> +  "clk_abb_usb"                USB reference clk
> +  "aclk_usb3otg"       USB3 OTG aclk
> +
> +- assigned-clocks:     Should be:
> +                               HI3660_ACLK_GATE_USB3OTG
> +- assigned-clock-rates: Should be:
> +                               229Mhz (229000000) for HI3660_ACLK_GATE_USB3OTG
> +
> +Optional properties:
> +- resets:              Phandle to reset control that resets core and wrapper.

Looks like 4 resets though.

> +
> +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.
> +
> +Example device nodes:
> +
> +       usb3: hisi_dwc3 {
> +               compatible = "hisilicon,hi3660-dwc3";
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +
> +               clocks = <&crg_ctrl HI3660_CLK_ABB_USB>,
> +                        <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
> +               clock-names = "clk_abb_usb", "aclk_usb3otg";
> +
> +               assigned-clocks = <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
> +               assigned-clock-rates = <229 000 000>;
> +               resets = <&crg_rst 0x90 8>,
> +                        <&crg_rst 0x90 7>,
> +                        <&crg_rst 0x90 6>,
> +                        <&crg_rst 0x90 5>;
> +
> +               dwc3: dwc3@ff100000 {

If it's only clocks and resets for the wrapper node, just make this
all one node.

And 'usb3' for the node name.

> +                       compatible = "snps,dwc3";
> +                       reg = <0x0 0xff100000 0x0 0x100000>;
> +                       interrupts = <0 159 4>, <0 161 4>;
> +                       phys = <&usb_phy>;
> +                       phy-names = "usb3-phy";
> +                       dr_mode = "otg";
> +
> +                       ...
> +               };
> +       };
> --
> 2.17.1
>

Re: [RFC][PATCH v2 4/5] dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

[John Stultz]

On Mon, Oct 7, 2019 at 11:38 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Oct 7, 2019 at 12:56 PM John Stultz <john.stultz@linaro.org> wrote:
> >
> > Add necessary compatible flag for HiSi's DWC3 so
> > dwc3-of-simple will probe.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Felipe Balbi <balbi@kernel.org>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Yu Chen <chenyu56@huawei.com>
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > Cc: linux-usb@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> > v2: Tweaked clock names as clk_usb3phy_ref didn't seem right.
> > ---
> >  .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt
>
> Can you make this a schema.

Sorry, I'm not sure exactly what you're asking. I'm guessing from
grepping around you want the bindings in yaml instead (I see a few
examples)? Is there some pointer to documentation? The
Documentation/devicetree/bindings/writing-bindings.txt file doesn't
seem to say much on it.

> > diff --git a/Documentation/devicetree/bindings/usb/hisi,dwc3.txt b/Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> > new file mode 100644
> > index 000000000000..3a3e5c320f2a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> > @@ -0,0 +1,52 @@
> > +HiSi SuperSpeed DWC3 USB SoC controller
> > +
> > +Required properties:
> > +- compatible:          should contain "hisilicon,hi3660-dwc3" for HiSi SoC
> > +- clocks:              A list of phandle + clock-specifier pairs for the
> > +                       clocks listed in clock-names
> > +- clock-names:         Should contain the following:
> > +  "clk_abb_usb"                USB reference clk
> > +  "aclk_usb3otg"       USB3 OTG aclk
> > +
> > +- assigned-clocks:     Should be:
> > +                               HI3660_ACLK_GATE_USB3OTG
> > +- assigned-clock-rates: Should be:
> > +                               229Mhz (229000000) for HI3660_ACLK_GATE_USB3OTG
> > +
> > +Optional properties:
> > +- resets:              Phandle to reset control that resets core and wrapper.
>
> Looks like 4 resets though.

Good point. I'll fix that up.

> > +
> > +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.
> > +
> > +Example device nodes:
> > +
> > +       usb3: hisi_dwc3 {
> > +               compatible = "hisilicon,hi3660-dwc3";
> > +               #address-cells = <2>;
> > +               #size-cells = <2>;
> > +               ranges;
> > +
> > +               clocks = <&crg_ctrl HI3660_CLK_ABB_USB>,
> > +                        <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
> > +               clock-names = "clk_abb_usb", "aclk_usb3otg";
> > +
> > +               assigned-clocks = <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
> > +               assigned-clock-rates = <229 000 000>;
> > +               resets = <&crg_rst 0x90 8>,
> > +                        <&crg_rst 0x90 7>,
> > +                        <&crg_rst 0x90 6>,
> > +                        <&crg_rst 0x90 5>;
> > +
> > +               dwc3: dwc3@ff100000 {
>
> If it's only clocks and resets for the wrapper node, just make this
> all one node.

Just to make sure I'm following, you're suggesting I put all the
clocks/resets in the dwc3 node (renamed to usb for the node name) and
not add the wrapper?

I'll have to see if that's possible. The generic dwc3 binding wants 3
clocks, but I only have two in the code I've worked with (similarly it
seems to only want two resets, not 4) so I'll have to see if I can
figure out how to adapt that.

If that approach is preferred, do I also no longer need a separate
binding document/schema?

thanks
-john

Re: [RFC][PATCH v2 4/5] dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

[Rob Herring]

On Mon, Oct 7, 2019 at 2:07 PM John Stultz <john.stultz@linaro.org> wrote:
>
> On Mon, Oct 7, 2019 at 11:38 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Oct 7, 2019 at 12:56 PM John Stultz <john.stultz@linaro.org> wrote:
> > >
> > > Add necessary compatible flag for HiSi's DWC3 so
> > > dwc3-of-simple will probe.
> > >
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Felipe Balbi <balbi@kernel.org>
> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Yu Chen <chenyu56@huawei.com>
> > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > Cc: linux-usb@vger.kernel.org
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > ---
> > > v2: Tweaked clock names as clk_usb3phy_ref didn't seem right.
> > > ---
> > >  .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
> > >  1 file changed, 52 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> >
> > Can you make this a schema.
>
> Sorry, I'm not sure exactly what you're asking. I'm guessing from
> grepping around you want the bindings in yaml instead (I see a few
> examples)?

Yes.

> Is there some pointer to documentation? The
> Documentation/devicetree/bindings/writing-bindings.txt file doesn't
> seem to say much on it.

You mean Documentation/devicetree/writing-schemas.rst? There's that
and Documentation/devicetree/bindings/example-schema.yaml which has a
bunch of annotations on what each part means.

> > > diff --git a/Documentation/devicetree/bindings/usb/hisi,dwc3.txt b/Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> > > new file mode 100644
> > > index 000000000000..3a3e5c320f2a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> > > @@ -0,0 +1,52 @@
> > > +HiSi SuperSpeed DWC3 USB SoC controller
> > > +
> > > +Required properties:
> > > +- compatible:          should contain "hisilicon,hi3660-dwc3" for HiSi SoC
> > > +- clocks:              A list of phandle + clock-specifier pairs for the
> > > +                       clocks listed in clock-names
> > > +- clock-names:         Should contain the following:
> > > +  "clk_abb_usb"                USB reference clk

Probably 'ref' from dwc3.txt.

> > > +  "aclk_usb3otg"       USB3 OTG aclk

'bus_early'? IIRC, 'aclk' is the clock name for AXI bus clock.

> > > +
> > > +- assigned-clocks:     Should be:
> > > +                               HI3660_ACLK_GATE_USB3OTG
> > > +- assigned-clock-rates: Should be:
> > > +                               229Mhz (229000000) for HI3660_ACLK_GATE_USB3OTG
> > > +
> > > +Optional properties:
> > > +- resets:              Phandle to reset control that resets core and wrapper.
> >
> > Looks like 4 resets though.
>
> Good point. I'll fix that up.
>
> > > +
> > > +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.
> > > +
> > > +Example device nodes:
> > > +
> > > +       usb3: hisi_dwc3 {
> > > +               compatible = "hisilicon,hi3660-dwc3";
> > > +               #address-cells = <2>;
> > > +               #size-cells = <2>;
> > > +               ranges;
> > > +
> > > +               clocks = <&crg_ctrl HI3660_CLK_ABB_USB>,
> > > +                        <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
> > > +               clock-names = "clk_abb_usb", "aclk_usb3otg";
> > > +
> > > +               assigned-clocks = <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
> > > +               assigned-clock-rates = <229 000 000>;
> > > +               resets = <&crg_rst 0x90 8>,
> > > +                        <&crg_rst 0x90 7>,
> > > +                        <&crg_rst 0x90 6>,
> > > +                        <&crg_rst 0x90 5>;
> > > +
> > > +               dwc3: dwc3@ff100000 {
> >
> > If it's only clocks and resets for the wrapper node, just make this
> > all one node.
>
> Just to make sure I'm following, you're suggesting I put all the
> clocks/resets in the dwc3 node (renamed to usb for the node name) and
> not add the wrapper?

Yes.

> I'll have to see if that's possible. The generic dwc3 binding wants 3
> clocks, but I only have two in the code I've worked with (similarly it
> seems to only want two resets, not 4) so I'll have to see if I can
> figure out how to adapt that.

Possible since commit fe8abf332b8f ("usb: dwc3: support clocks and
resets for DWC3 core").

>
> If that approach is preferred, do I also no longer need a separate
> binding document/schema?

Correct. And then no need to convert to schema yet (though feel free
to convert dwc3.txt :)).

Rob

Re: [RFC][PATCH v2 4/5] dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

[John Stultz]

On Mon, Oct 7, 2019 at 2:11 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Oct 7, 2019 at 2:07 PM John Stultz <john.stultz@linaro.org> wrote:
> >
> > On Mon, Oct 7, 2019 at 11:38 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Mon, Oct 7, 2019 at 12:56 PM John Stultz <john.stultz@linaro.org> wrote:
> > > >
> > > > Add necessary compatible flag for HiSi's DWC3 so
> > > > dwc3-of-simple will probe.
> > > >
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Cc: Felipe Balbi <balbi@kernel.org>
> > > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Yu Chen <chenyu56@huawei.com>
> > > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > Cc: linux-usb@vger.kernel.org
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > ---
> > > > v2: Tweaked clock names as clk_usb3phy_ref didn't seem right.
> > > > ---
> > > >  .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
> > > >  1 file changed, 52 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> > >
> > > Can you make this a schema.
> >
> > Sorry, I'm not sure exactly what you're asking. I'm guessing from
> > grepping around you want the bindings in yaml instead (I see a few
> > examples)?
>
> Yes.
>
> > Is there some pointer to documentation? The
> > Documentation/devicetree/bindings/writing-bindings.txt file doesn't
> > seem to say much on it.
>
> You mean Documentation/devicetree/writing-schemas.rst? There's that
> and Documentation/devicetree/bindings/example-schema.yaml which has a
> bunch of annotations on what each part means.

Ah! Sorry for missing that. Thanks for the pointer, though I may get
away with dropping this one.

> > > If it's only clocks and resets for the wrapper node, just make this
> > > all one node.
> >
> > Just to make sure I'm following, you're suggesting I put all the
> > clocks/resets in the dwc3 node (renamed to usb for the node name) and
> > not add the wrapper?
>
> Yes.
>
> > I'll have to see if that's possible. The generic dwc3 binding wants 3
> > clocks, but I only have two in the code I've worked with (similarly it
> > seems to only want two resets, not 4) so I'll have to see if I can
> > figure out how to adapt that.
>
> Possible since commit fe8abf332b8f ("usb: dwc3: support clocks and
> resets for DWC3 core").

Ok. It *seems* like I can get it working with the existing binding
then. There's a little funkiness with the core expecting three clocks
while I only have two (currently I'm duplicating the "bus_early" clk
for "suspend". Is there a preferred way to do this sort of hack?), and
I'm a little worried that only the first reset is being used (instead
of the 4 specified), but it seems to work so far.

I still suspect the dwc3-of-simple.c method might be better since it
can handle arbitrary sets of clks and resets instead of the fixed 3 &
1 in the dwc3.txt binding.
But if I can get away without having to add an extra binding here, I'd
be happier.

thanks
-john

Re: [RFC][PATCH v2 2/5] usb: dwc3: Execute GCTL Core Soft Reset while switch mdoe for Hisilicon Kirin Soc

[Jack Pham]

Hi John, Yu, Felipe,

On Mon, Oct 07, 2019 at 05:55:50PM +0000, John Stultz wrote:
> From: Yu Chen <chenyu56@huawei.com>
> 
> A GCTL soft reset should be executed when switch mode for dwc3 core
> of Hisilicon Kirin Soc.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Yu Chen <chenyu56@huawei.com>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/usb/dwc3/core.c | 20 ++++++++++++++++++++
>  drivers/usb/dwc3/core.h |  3 +++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 999ce5e84d3c..440261432421 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>  	dwc->current_dr_role = mode;
>  }
>  
> +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
> +{
> +	u32 reg;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg |= DWC3_GCTL_CORESOFTRESET;
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg &= ~DWC3_GCTL_CORESOFTRESET;
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +}
> +
>  static void __dwc3_set_mode(struct work_struct *work)
>  {
>  	struct dwc3 *dwc = work_to_dwc(work);
> @@ -156,6 +169,10 @@ static void __dwc3_set_mode(struct work_struct *work)
>  
>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>  
> +	/* Execute a GCTL Core Soft Reset when switch mode */
> +	if (dwc->gctl_reset_quirk)
> +		dwc3_gctl_core_soft_reset(dwc);
> +

In fact it is mentioned in the Synopsys databook to perform a GCTL
CoreSoftReset when changing the PrtCapDir between device & host modes.
So I think this should apply generally without a quirk. Further, it
states to do this *prior* to writing PrtCapDir, so should it go before
dwc3_set_prtcap() instead?

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

Re: [RFC][PATCH v2 2/5] usb: dwc3: Execute GCTL Core Soft Reset while switch mdoe for Hisilicon Kirin Soc

[John Stultz]

On Mon, Oct 7, 2019 at 4:39 PM Jack Pham <jackp@codeaurora.org> wrote:
>
> Hi John, Yu, Felipe,
>
> On Mon, Oct 07, 2019 at 05:55:50PM +0000, John Stultz wrote:
> > From: Yu Chen <chenyu56@huawei.com>
> >
> > A GCTL soft reset should be executed when switch mode for dwc3 core
> > of Hisilicon Kirin Soc.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Felipe Balbi <balbi@kernel.org>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Yu Chen <chenyu56@huawei.com>
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > Cc: linux-usb@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Yu Chen <chenyu56@huawei.com>
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> >  drivers/usb/dwc3/core.c | 20 ++++++++++++++++++++
> >  drivers/usb/dwc3/core.h |  3 +++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 999ce5e84d3c..440261432421 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
> >       dwc->current_dr_role = mode;
> >  }
> >
> > +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
> > +{
> > +     u32 reg;
> > +
> > +     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > +     reg |= DWC3_GCTL_CORESOFTRESET;
> > +     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > +
> > +     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > +     reg &= ~DWC3_GCTL_CORESOFTRESET;
> > +     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > +}
> > +
> >  static void __dwc3_set_mode(struct work_struct *work)
> >  {
> >       struct dwc3 *dwc = work_to_dwc(work);
> > @@ -156,6 +169,10 @@ static void __dwc3_set_mode(struct work_struct *work)
> >
> >       dwc3_set_prtcap(dwc, dwc->desired_dr_role);
> >
> > +     /* Execute a GCTL Core Soft Reset when switch mode */
> > +     if (dwc->gctl_reset_quirk)
> > +             dwc3_gctl_core_soft_reset(dwc);
> > +
>
> In fact it is mentioned in the Synopsys databook to perform a GCTL
> CoreSoftReset when changing the PrtCapDir between device & host modes.
> So I think this should apply generally without a quirk. Further, it
> states to do this *prior* to writing PrtCapDir, so should it go before
> dwc3_set_prtcap() instead?

Sounds good. I have no such access to the hardware docs, so I really
appreciate your input here!

I'll refactor it as you describe and remove the quirk flag.

thanks
-john

Re: [RFC][PATCH v2 4/5] dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

[Rob Herring]

On Mon, Oct 07, 2019 at 04:00:24PM -0700, John Stultz wrote:
> On Mon, Oct 7, 2019 at 2:11 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Mon, Oct 7, 2019 at 2:07 PM John Stultz <john.stultz@linaro.org> wrote:
> > >
> > > On Mon, Oct 7, 2019 at 11:38 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Mon, Oct 7, 2019 at 12:56 PM John Stultz <john.stultz@linaro.org> wrote:
> > > > >
> > > > > Add necessary compatible flag for HiSi's DWC3 so
> > > > > dwc3-of-simple will probe.
> > > > >
> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Cc: Felipe Balbi <balbi@kernel.org>
> > > > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > Cc: Yu Chen <chenyu56@huawei.com>
> > > > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > Cc: linux-usb@vger.kernel.org
> > > > > Cc: devicetree@vger.kernel.org
> > > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > > ---
> > > > > v2: Tweaked clock names as clk_usb3phy_ref didn't seem right.
> > > > > ---
> > > > >  .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
> > > > >  1 file changed, 52 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> > > >
> > > > Can you make this a schema.
> > >
> > > Sorry, I'm not sure exactly what you're asking. I'm guessing from
> > > grepping around you want the bindings in yaml instead (I see a few
> > > examples)?
> >
> > Yes.
> >
> > > Is there some pointer to documentation? The
> > > Documentation/devicetree/bindings/writing-bindings.txt file doesn't
> > > seem to say much on it.
> >
> > You mean Documentation/devicetree/writing-schemas.rst? There's that
> > and Documentation/devicetree/bindings/example-schema.yaml which has a
> > bunch of annotations on what each part means.
> 
> Ah! Sorry for missing that. Thanks for the pointer, though I may get
> away with dropping this one.
> 
> > > > If it's only clocks and resets for the wrapper node, just make this
> > > > all one node.
> > >
> > > Just to make sure I'm following, you're suggesting I put all the
> > > clocks/resets in the dwc3 node (renamed to usb for the node name) and
> > > not add the wrapper?
> >
> > Yes.
> >
> > > I'll have to see if that's possible. The generic dwc3 binding wants 3
> > > clocks, but I only have two in the code I've worked with (similarly it
> > > seems to only want two resets, not 4) so I'll have to see if I can
> > > figure out how to adapt that.
> >
> > Possible since commit fe8abf332b8f ("usb: dwc3: support clocks and
> > resets for DWC3 core").
> 
> Ok. It *seems* like I can get it working with the existing binding
> then. There's a little funkiness with the core expecting three clocks
> while I only have two (currently I'm duplicating the "bus_early" clk
> for "suspend". Is there a preferred way to do this sort of hack?), and
> I'm a little worried that only the first reset is being used (instead
> of the 4 specified), but it seems to work so far.

I would assume that you simply don't know how the 'suspend' clock is 
connected rather than you don't have one. But that's maybe not a 
problem you can fix.

I would make dwc3 use devm_clk_bulk_get_all and allow for less than 3 
clocks. And do a similar change for resets.


> I still suspect the dwc3-of-simple.c method might be better since it
> can handle arbitrary sets of clks and resets instead of the fixed 3 &
> 1 in the dwc3.txt binding.
> But if I can get away without having to add an extra binding here, I'd
> be happier.

Me too.

Rob

Re: [RFC][PATCH v2 4/5] dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

[John Stultz]

On Fri, Oct 11, 2019 at 8:51 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Oct 07, 2019 at 04:00:24PM -0700, John Stultz wrote:
> > On Mon, Oct 7, 2019 at 2:11 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Mon, Oct 7, 2019 at 2:07 PM John Stultz <john.stultz@linaro.org> wrote:
> > > >
> > > > On Mon, Oct 7, 2019 at 11:38 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Mon, Oct 7, 2019 at 12:56 PM John Stultz <john.stultz@linaro.org> wrote:
> > > > > >
> > > > > > Add necessary compatible flag for HiSi's DWC3 so
> > > > > > dwc3-of-simple will probe.
> > > > > >
> > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > Cc: Felipe Balbi <balbi@kernel.org>
> > > > > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > > Cc: Yu Chen <chenyu56@huawei.com>
> > > > > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > > > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > Cc: linux-usb@vger.kernel.org
> > > > > > Cc: devicetree@vger.kernel.org
> > > > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > > > ---
> > > > > > v2: Tweaked clock names as clk_usb3phy_ref didn't seem right.
> > > > > > ---
> > > > > >  .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
> > > > > >  1 file changed, 52 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> > > > >
> > > > > Can you make this a schema.
> > > >
> > > > Sorry, I'm not sure exactly what you're asking. I'm guessing from
> > > > grepping around you want the bindings in yaml instead (I see a few
> > > > examples)?
> > >
> > > Yes.
> > >
> > > > Is there some pointer to documentation? The
> > > > Documentation/devicetree/bindings/writing-bindings.txt file doesn't
> > > > seem to say much on it.
> > >
> > > You mean Documentation/devicetree/writing-schemas.rst? There's that
> > > and Documentation/devicetree/bindings/example-schema.yaml which has a
> > > bunch of annotations on what each part means.
> >
> > Ah! Sorry for missing that. Thanks for the pointer, though I may get
> > away with dropping this one.
> >
> > > > > If it's only clocks and resets for the wrapper node, just make this
> > > > > all one node.
> > > >
> > > > Just to make sure I'm following, you're suggesting I put all the
> > > > clocks/resets in the dwc3 node (renamed to usb for the node name) and
> > > > not add the wrapper?
> > >
> > > Yes.
> > >
> > > > I'll have to see if that's possible. The generic dwc3 binding wants 3
> > > > clocks, but I only have two in the code I've worked with (similarly it
> > > > seems to only want two resets, not 4) so I'll have to see if I can
> > > > figure out how to adapt that.
> > >
> > > Possible since commit fe8abf332b8f ("usb: dwc3: support clocks and
> > > resets for DWC3 core").
> >
> > Ok. It *seems* like I can get it working with the existing binding
> > then. There's a little funkiness with the core expecting three clocks
> > while I only have two (currently I'm duplicating the "bus_early" clk
> > for "suspend". Is there a preferred way to do this sort of hack?), and
> > I'm a little worried that only the first reset is being used (instead
> > of the 4 specified), but it seems to work so far.
>
> I would assume that you simply don't know how the 'suspend' clock is
> connected rather than you don't have one. But that's maybe not a
> problem you can fix.
>
> I would make dwc3 use devm_clk_bulk_get_all and allow for less than 3
> clocks. And do a similar change for resets.

So got a chance to start implementing this and it seems like it will
work. That said, it feels like I'm duplicating logic already in the
dwc-of-simple.c implementation (which already handles arbitrary clks
and resets), particularly if I try to implement the device specific
need_reset quirk used by HiKey960 (and rk3399).

Do you feel having that logic copied is worth avoiding the extra
bindings? Or is it too duplicative?

thanks
-john

Re: [RFC][PATCH v2 4/5] dt-bindings: usb: dwc3: of-simple: add compatible for HiSi

[Rob Herring]

On Mon, Oct 14, 2019 at 10:57 PM John Stultz <john.stultz@linaro.org> wrote:
>
> On Fri, Oct 11, 2019 at 8:51 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Oct 07, 2019 at 04:00:24PM -0700, John Stultz wrote:
> > > On Mon, Oct 7, 2019 at 2:11 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Mon, Oct 7, 2019 at 2:07 PM John Stultz <john.stultz@linaro.org> wrote:
> > > > >
> > > > > On Mon, Oct 7, 2019 at 11:38 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Oct 7, 2019 at 12:56 PM John Stultz <john.stultz@linaro.org> wrote:
> > > > > > >
> > > > > > > Add necessary compatible flag for HiSi's DWC3 so
> > > > > > > dwc3-of-simple will probe.
> > > > > > >
> > > > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > > Cc: Felipe Balbi <balbi@kernel.org>
> > > > > > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > > > Cc: Yu Chen <chenyu56@huawei.com>
> > > > > > > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > > > > > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > > > > Cc: linux-usb@vger.kernel.org
> > > > > > > Cc: devicetree@vger.kernel.org
> > > > > > > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > > > > > > ---
> > > > > > > v2: Tweaked clock names as clk_usb3phy_ref didn't seem right.
> > > > > > > ---
> > > > > > >  .../devicetree/bindings/usb/hisi,dwc3.txt     | 52 +++++++++++++++++++
> > > > > > >  1 file changed, 52 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/usb/hisi,dwc3.txt
> > > > > >
> > > > > > Can you make this a schema.
> > > > >
> > > > > Sorry, I'm not sure exactly what you're asking. I'm guessing from
> > > > > grepping around you want the bindings in yaml instead (I see a few
> > > > > examples)?
> > > >
> > > > Yes.
> > > >
> > > > > Is there some pointer to documentation? The
> > > > > Documentation/devicetree/bindings/writing-bindings.txt file doesn't
> > > > > seem to say much on it.
> > > >
> > > > You mean Documentation/devicetree/writing-schemas.rst? There's that
> > > > and Documentation/devicetree/bindings/example-schema.yaml which has a
> > > > bunch of annotations on what each part means.
> > >
> > > Ah! Sorry for missing that. Thanks for the pointer, though I may get
> > > away with dropping this one.
> > >
> > > > > > If it's only clocks and resets for the wrapper node, just make this
> > > > > > all one node.
> > > > >
> > > > > Just to make sure I'm following, you're suggesting I put all the
> > > > > clocks/resets in the dwc3 node (renamed to usb for the node name) and
> > > > > not add the wrapper?
> > > >
> > > > Yes.
> > > >
> > > > > I'll have to see if that's possible. The generic dwc3 binding wants 3
> > > > > clocks, but I only have two in the code I've worked with (similarly it
> > > > > seems to only want two resets, not 4) so I'll have to see if I can
> > > > > figure out how to adapt that.
> > > >
> > > > Possible since commit fe8abf332b8f ("usb: dwc3: support clocks and
> > > > resets for DWC3 core").
> > >
> > > Ok. It *seems* like I can get it working with the existing binding
> > > then. There's a little funkiness with the core expecting three clocks
> > > while I only have two (currently I'm duplicating the "bus_early" clk
> > > for "suspend". Is there a preferred way to do this sort of hack?), and
> > > I'm a little worried that only the first reset is being used (instead
> > > of the 4 specified), but it seems to work so far.
> >
> > I would assume that you simply don't know how the 'suspend' clock is
> > connected rather than you don't have one. But that's maybe not a
> > problem you can fix.
> >
> > I would make dwc3 use devm_clk_bulk_get_all and allow for less than 3
> > clocks. And do a similar change for resets.
>
> So got a chance to start implementing this and it seems like it will
> work. That said, it feels like I'm duplicating logic already in the
> dwc-of-simple.c implementation (which already handles arbitrary clks
> and resets), particularly if I try to implement the device specific
> need_reset quirk used by HiKey960 (and rk3399).
>
> Do you feel having that logic copied is worth avoiding the extra
> bindings? Or is it too duplicative?

We already have reset and clock setup in 2 places, so it's already
kind of duplicated.

Why not refactor into a helper that both can use?

Rob