[PATCH 00/10] usb: Add host and device support for RZ/A2

[PATCH 00/10] usb: Add host and device support for RZ/A2

[Chris Brandt]

For the most part, the RZ/A2 has the same USB 2.0 host and device
HW as the R-Car Gen3, so we can reuse a lot of the code.

However, there are a couple extra register bits, and the CFIFO
register 8-bit access works a little different (weird, no idea why).

There is a dedicated DMAC for the RZ/A2 USB Device HW, but we
have not been able to reliably get that working yet, so device
operation is pio only at the moment.

On the RZ/A2M eval board, both USB channels can be used as either
host or device. But, it's not set up for otg (ie, there are jumpers
and separate connectors). Therefore, below is an example of what it
would look like to enable USB channel 0 as a device instead of a host.

&usb2_phy0 {
	pinctrl-names = "default";
	pinctrl-0 = <&usb0_pins>;
	renesas,uses_usb_x1;
	dr_mode = "peripheral";
	status = "okay";
};
&usbhs0 {
	status = "okay";
};






Chris Brandt (10):
  phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option
  dt-bindings: rcar-gen3-phy-usb2: Document uses_usb_x1
  phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG
  dt-bindings: rcar-gen3-phy-usb2: Document dr_mode
  dt-bindings: rcar-gen3-phy-usb2: Add r7s9210 support
  usb: renesas_usbhs: Add support for RZ/A2
  dt-bindings: usb: renesas_usbhs: Add support for r7s9210
  ARM: dts: r7s9210: Add USB Host support
  ARM: dts: r7s9210: Add USB Device support
  ARM: dts: r7s9210-rza2mevb: Add USB host support

 .../devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 15 +++-
 .../devicetree/bindings/usb/renesas_usbhs.txt      |  2 +
 arch/arm/boot/dts/r7s9210-rza2mevb.dts             | 37 +++++++++
 arch/arm/boot/dts/r7s9210.dtsi                     | 88 ++++++++++++++++++++++
 drivers/phy/renesas/phy-rcar-gen3-usb2.c           | 16 +++-
 drivers/usb/renesas_usbhs/Makefile                 |  2 +-
 drivers/usb/renesas_usbhs/common.c                 | 27 +++++--
 drivers/usb/renesas_usbhs/common.h                 | 13 ++++
 drivers/usb/renesas_usbhs/fifo.c                   |  8 +-
 drivers/usb/renesas_usbhs/rza.h                    |  1 +
 drivers/usb/renesas_usbhs/rza2.c                   | 82 ++++++++++++++++++++
 include/linux/usb/renesas_usbhs.h                  |  1 +
 12 files changed, 277 insertions(+), 15 deletions(-)
 create mode 100644 drivers/usb/renesas_usbhs/rza2.c

-- 
2.16.1

[PATCH 01/10] phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option

[Chris Brandt]

The RZ/A2 has optional dedicated 48MHz crystal inputs, which adds a new
register setting when used.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 1322185a00a2..218b32e458cb 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -34,6 +34,7 @@
 #define USB2_VBCTRL		0x60c
 #define USB2_LINECTRL1		0x610
 #define USB2_ADPCTRL		0x630
+#define USB2_PHYCLK_CTRL	0x644
 
 /* INT_ENABLE */
 #define USB2_INT_ENABLE_UCOM_INTEN	BIT(3)
@@ -110,6 +111,7 @@ struct rcar_gen3_chan {
 	bool extcon_host;
 	bool is_otg_channel;
 	bool uses_otg_pins;
+	bool uses_usb_x1;
 };
 
 /*
@@ -391,6 +393,9 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
 	void __iomem *usb2_base = channel->base;
 	u32 val;
 
+	if (channel->uses_usb_x1)
+		writel(0x00000001, usb2_base + USB2_PHYCLK_CTRL);
+
 	/* Initialize USB2 part */
 	val = readl(usb2_base + USB2_INT_ENABLE);
 	val |= USB2_INT_ENABLE_UCOM_INTEN | rphy->int_enable_bits;
@@ -630,6 +635,9 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
+		channel->uses_usb_x1 = true;
+
 	/*
 	 * devm_phy_create() will call pm_runtime_enable(&phy->dev);
 	 * And then, phy-core will manage runtime pm for this device.
-- 
2.16.1

Re: [PATCH 01/10] phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option

[Geert Uytterhoeven]

Hi Chris,

On Tue, May 7, 2019 at 1:47 AM Chris Brandt <chris.brandt@renesas.com> wrote:
> The RZ/A2 has optional dedicated 48MHz crystal inputs, which adds a new
> register setting when used.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>

Thanks for your patch!

> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c

> @@ -630,6 +635,9 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>                 }
>         }
>
> +       if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
> +               channel->uses_usb_x1 = true;
> +

Perhaps this can be checked some other way (e.g. by checking for a non-zero
clock rate of the USB_X1 clock referenced from DT), thus removing the need for
adding a custom property?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 01/10] phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option

[Chris Brandt]

Hi Geert,

On Tue, May 07, 2019, Geert Uytterhoeven wrote:
> > +       if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
> > +               channel->uses_usb_x1 = true;
> > +
> 
> Perhaps this can be checked some other way (e.g. by checking for a non-
> zero
> clock rate of the USB_X1 clock referenced from DT), thus removing the need
> for
> adding a custom property?


Good point. I've done that for other drivers before and it worked well.

I'll take this out...one less property to set :)

Question: Since the driver depends on this, should I mention this in the
dt-bindings documentation ?

Thanks,
Chris

Re: [PATCH 01/10] phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option

[Geert Uytterhoeven]

Hi Chris,

On Tue, May 7, 2019 at 1:00 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, May 07, 2019, Geert Uytterhoeven wrote:
> > > +       if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
> > > +               channel->uses_usb_x1 = true;
> > > +
> >
> > Perhaps this can be checked some other way (e.g. by checking for a non-
> > zero
> > clock rate of the USB_X1 clock referenced from DT), thus removing the need
> > for
> > adding a custom property?
>
> Good point. I've done that for other drivers before and it worked well.
>
> I'll take this out...one less property to set :)
>
> Question: Since the driver depends on this, should I mention this in the
> dt-bindings documentation ?

Yes, this should be described in the section about the clocks property.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 01/10] phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option

[Chris Brandt]

Hi Geert,

On Tue, May 07, 2019, Geert Uytterhoeven wrote:
> > +       if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
> > +               channel->uses_usb_x1 = true;
> > +
> 
> Perhaps this can be checked some other way (e.g. by checking for a non-
> zero
> clock rate of the USB_X1 clock referenced from DT), thus removing the need
> for
> adding a custom property?

Currently, there is no USB_X1 in DT like there is for RZ/A1.

For RZ/A2, those are dedicated pins that belong to the USB HW block 
itself. They do not feed into the system CPG or any dividers, so I
never included it in the .dtsi.

So with that said, does a uses-usb-x1 property make more sense?


Chris

Re: [PATCH 01/10] phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option

[Geert Uytterhoeven]

Hi Chris,

On Tue, May 7, 2019 at 5:43 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, May 07, 2019, Geert Uytterhoeven wrote:
> > > +       if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
> > > +               channel->uses_usb_x1 = true;
> > > +
> >
> > Perhaps this can be checked some other way (e.g. by checking for a non-
> > zero
> > clock rate of the USB_X1 clock referenced from DT), thus removing the need
> > for
> > adding a custom property?
>
> Currently, there is no USB_X1 in DT like there is for RZ/A1.
>
> For RZ/A2, those are dedicated pins that belong to the USB HW block
> itself. They do not feed into the system CPG or any dividers, so I
> never included it in the .dtsi.

Like pcie_bus_clk on R-Car?
We do have that in DT, with a "clock" link to it from the PCIe device node.
After all, it is provided by an external clock crystal, and consumed by the
PCIe device.

> So with that said, does a uses-usb-x1 property make more sense?

No ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH 01/10] phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option

[Chris Brandt]

Hi Geert,

On Tue, May 07, 2019 1, Geert Uytterhoeven wrote:
> > So with that said, does a uses-usb-x1 property make more sense?
> 
> No ;-)

So....

I guess the first patch in the series needs to add this to the .dtsi:

	usb_x1_clk: usb_x1 {
		#clock-cells = <0>;
		compatible = "fixed-clock";
		/* If clk present, value must be set by board */
		clock-frequency = <0>;
	};

Then I can reference "usb_x1" in the driver and see if it is set to 
non-zero.

What do you think?


Chris

Re: [PATCH 01/10] phy: renesas: rcar-gen3-usb2: Add uses_usb_x1 option

[Geert Uytterhoeven]

Hi Chris,

On Tue, May 7, 2019 at 10:27 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Tue, May 07, 2019 1, Geert Uytterhoeven wrote:
> > > So with that said, does a uses-usb-x1 property make more sense?
> >
> > No ;-)
>
> So....
>
> I guess the first patch in the series needs to add this to the .dtsi:
>
>         usb_x1_clk: usb_x1 {
>                 #clock-cells = <0>;
>                 compatible = "fixed-clock";
>                 /* If clk present, value must be set by board */
>                 clock-frequency = <0>;
>         };
>
> Then I can reference "usb_x1" in the driver and see if it is set to
> non-zero.
>
> What do you think?

Exactly!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 02/10] dt-bindings: rcar-gen3-phy-usb2: Document uses_usb_x1

[Chris Brandt]

Document the optional renesas,uses_usb_x1 property.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
index d46188f450bf..26bf377102d3 100644
--- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
+++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
@@ -46,6 +46,8 @@ channel as USB OTG:
 	       regulator will be managed during the PHY power on/off sequence.
 - renesas,no-otg-pins: boolean, specify when a board does not provide proper
 		       otg pins.
+- renesas,use_usb_x1: boolean, the dedicated 48MHz crystal inputs USB_X1 are
+                      used for the PLL source
 
 Example (R-Car H3):
 
-- 
2.16.1

Re: [PATCH 02/10] dt-bindings: rcar-gen3-phy-usb2: Document uses_usb_x1

[Rob Herring]

On Mon, May 6, 2019 at 6:47 PM Chris Brandt <chris.brandt@renesas.com> wrote:
>
> Document the optional renesas,uses_usb_x1 property.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> index d46188f450bf..26bf377102d3 100644
> --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> @@ -46,6 +46,8 @@ channel as USB OTG:
>                regulator will be managed during the PHY power on/off sequence.
>  - renesas,no-otg-pins: boolean, specify when a board does not provide proper
>                        otg pins.
> +- renesas,use_usb_x1: boolean, the dedicated 48MHz crystal inputs USB_X1 are
> +                      used for the PLL source

s/_/-/

RE: [PATCH 02/10] dt-bindings: rcar-gen3-phy-usb2: Document uses_usb_x1

[Chris Brandt]

On Mon, May 06, 2019, Rob Herring wrote:
> > +- renesas,use_usb_x1: boolean, the dedicated 48MHz crystal inputs
> USB_X1 are
> > +                      used for the PLL source
> 
> s/_/-/

Good point.
Thanks.


Chris

Re: [PATCH 02/10] dt-bindings: rcar-gen3-phy-usb2: Document uses_usb_x1

[Sergei Shtylyov]

Hello!

On 07.05.2019 2:46, Chris Brandt wrote:

> Document the optional renesas,uses_usb_x1 property.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>   Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> index d46188f450bf..26bf377102d3 100644
> --- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
> @@ -46,6 +46,8 @@ channel as USB OTG:
>   	       regulator will be managed during the PHY power on/off sequence.
>   - renesas,no-otg-pins: boolean, specify when a board does not provide proper
>   		       otg pins.
> +- renesas,use_usb_x1: boolean, the dedicated 48MHz crystal inputs USB_X1 are

    Hyphens are preferred to underscores in the DT property names.

> +                      used for the PLL source
>   
>   Example (R-Car H3):

MBR, Sergei

[PATCH 03/10] phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG

[Chris Brandt]

When not using OTG, the PHY will need to know if it should function as
host or peripheral by checking dr_mode in the PHY node (not the parent
controller node).

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 218b32e458cb..4eaa228ebd30 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
 		if (rcar_gen3_needs_init_otg(channel))
 			rcar_gen3_init_otg(channel);
 		rphy->otg_initialized = true;
-	}
+	} else
+		/* Not OTG, so dr_mode should be set in PHY node */
+		if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL)
+			writel(0x80000000, usb2_base + USB2_COMMCTRL);
+		else
+			writel(0x00000000, usb2_base + USB2_COMMCTRL);
 
 	rphy->initialized = true;
 
@@ -638,6 +643,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
 	if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
 		channel->uses_usb_x1 = true;
 
+
 	/*
 	 * devm_phy_create() will call pm_runtime_enable(&phy->dev);
 	 * And then, phy-core will manage runtime pm for this device.
-- 
2.16.1

Re: [PATCH 03/10] phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG

[Sergei Shtylyov]

On 07.05.2019 2:46, Chris Brandt wrote:

> When not using OTG, the PHY will need to know if it should function as
> host or peripheral by checking dr_mode in the PHY node (not the parent
> controller node).
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>   drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 218b32e458cb..4eaa228ebd30 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
>   		if (rcar_gen3_needs_init_otg(channel))
>   			rcar_gen3_init_otg(channel);
>   		rphy->otg_initialized = true;
> -	}
> +	} else
> +		/* Not OTG, so dr_mode should be set in PHY node */
> +		if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL)
> +			writel(0x80000000, usb2_base + USB2_COMMCTRL);
> +		else
> +			writel(0x00000000, usb2_base + USB2_COMMCTRL);
>   
>   	rphy->initialized = true;
>   
> @@ -638,6 +643,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>   	if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
>   		channel->uses_usb_x1 = true;
>   
> +

    Unrelated whitespace tweaking? :-)

>   	/*
>   	 * devm_phy_create() will call pm_runtime_enable(&phy->dev);
>   	 * And then, phy-core will manage runtime pm for this device.

MBR, Sergei

Re: [PATCH 03/10] phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG

[Sergei Shtylyov]

On 05/07/2019 02:46 AM, Chris Brandt wrote:

> When not using OTG, the PHY will need to know if it should function as
> host or peripheral by checking dr_mode in the PHY node (not the parent
> controller node).
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 218b32e458cb..4eaa228ebd30 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
>  		if (rcar_gen3_needs_init_otg(channel))
>  			rcar_gen3_init_otg(channel);
>  		rphy->otg_initialized = true;
> -	}
> +	} else

   Wait, don't we neeed {} here?

> +		/* Not OTG, so dr_mode should be set in PHY node */
> +		if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL)
> +			writel(0x80000000, usb2_base + USB2_COMMCTRL);
> +		else
> +			writel(0x00000000, usb2_base + USB2_COMMCTRL);
>  
>  	rphy->initialized = true;
>  

MBR, Sergei

RE: [PATCH 03/10] phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG

[Chris Brandt]

On Tue, May 07, 2019, Sergei Shtylyov wrote:
> > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
> >  		if (rcar_gen3_needs_init_otg(channel))
> >  			rcar_gen3_init_otg(channel);
> >  		rphy->otg_initialized = true;
> > -	}
> > +	} else
> 
>    Wait, don't we neeed {} here?
> 
> > +		/* Not OTG, so dr_mode should be set in PHY node */
> > +		if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL)
> > +			writel(0x80000000, usb2_base + USB2_COMMCTRL);
> > +		else
> > +			writel(0x00000000, usb2_base + USB2_COMMCTRL);

Technically there is only 1 statement after the else (the 'if' which 
will also include the 'else') statement. The coding rules say not to use
{ } if there is only 1 statement.


Chris

Re: [PATCH 03/10] phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG

[Sergei Shtylyov]

On 05/07/2019 02:45 PM, Chris Brandt wrote:

>>> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
>>> @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
>>>  		if (rcar_gen3_needs_init_otg(channel))
>>>  			rcar_gen3_init_otg(channel);
>>>  		rphy->otg_initialized = true;
>>> -	}
>>> +	} else
>>
>>    Wait, don't we neeed {} here?
>>
>>> +		/* Not OTG, so dr_mode should be set in PHY node */
>>> +		if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL)
>>> +			writel(0x80000000, usb2_base + USB2_COMMCTRL);
>>> +		else
>>> +			writel(0x00000000, usb2_base + USB2_COMMCTRL);
> 
> Technically there is only 1 statement after the else (the 'if' which 
> will also include the 'else') statement. The coding rules say not to use
> { } if there is only 1 statement.

   Don't you remember another rule: use {} in all branches if at least 
one branch uses {}?

> Chris

MBR, Sergei

RE: [PATCH 03/10] phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG

[Chris Brandt]

On Tue, May 07, 2019 1, Sergei Shtylyov wrote:
>    Don't you remember another rule: use {} in all branches if at least
> one branch uses {}?

Ah, I see it now.

Documentation/process/coding-style.rst:

This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:

.. code-block:: c

	if (condition) {
		do_this();
		do_that();
	} else {
		otherwise();
	}


I will change it.
Thanks!

Chris

RE: [PATCH 03/10] phy: renesas: rcar-gen3-usb2: Check dr_mode when not using OTG

[Yoshihiro Shimoda]

Hi Chrisさん

Thank you for the patch!

> From: Chris Brandt, Sent: Tuesday, May 7, 2019 8:46 AM
> 
> When not using OTG, the PHY will need to know if it should function as
> host or peripheral by checking dr_mode in the PHY node (not the parent
> controller node).
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/phy/renesas/phy-rcar-gen3-usb2.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 218b32e458cb..4eaa228ebd30 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -408,7 +408,12 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
>  		if (rcar_gen3_needs_init_otg(channel))
>  			rcar_gen3_init_otg(channel);
>  		rphy->otg_initialized = true;
> -	}
> +	} else

As Sergei-san said, this should be "} else {"

> +		/* Not OTG, so dr_mode should be set in PHY node */
> +		if (usb_get_dr_mode(channel->dev) == USB_DR_MODE_PERIPHERAL)
> +			writel(0x80000000, usb2_base + USB2_COMMCTRL);
> +		else

I would like to add "else if usb_get_dr_mode(channel->dev) == USB_DR_MODE_HOST)"
for a PHY node without "dr_mode" property. In other words, if the PHY node
doesn't have dr_mode property like R-Car, this condition can be the same behavior as previous.

> +			writel(0x00000000, usb2_base + USB2_COMMCTRL);
> 
>  	rphy->initialized = true;
> 
> @@ -638,6 +643,7 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  	if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
>  		channel->uses_usb_x1 = true;
> 
> +

As Sergei-san said, this is not needed :)

Best regards,
Yoshihiro Shimoda

[PATCH 04/10] dt-bindings: rcar-gen3-phy-usb2: Document dr_mode

[Chris Brandt]

Document the optional dr_mode property

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
index 26bf377102d3..646a1748d3c4 100644
--- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
+++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
@@ -48,6 +48,9 @@ channel as USB OTG:
 		       otg pins.
 - renesas,use_usb_x1: boolean, the dedicated 48MHz crystal inputs USB_X1 are
                       used for the PLL source
+- dr_mode: string, indicates the working mode for the PHY. Can be "host",
+           "peripheral", or "otg". Defaults to "host" if not defined.
+
 
 Example (R-Car H3):
 
-- 
2.16.1

[PATCH 05/10] dt-bindings: rcar-gen3-phy-usb2: Add r7s9210 support

[Chris Brandt]

Document RZ/A2 (R7S9210) SoC bindings.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
index 646a1748d3c4..e7bbee43e268 100644
--- a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
+++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb2.txt
@@ -1,10 +1,12 @@
 * Renesas R-Car generation 3 USB 2.0 PHY
 
 This file provides information on what the device node for the R-Car generation
-3, RZ/G1C and RZ/G2 USB 2.0 PHY contain.
+3, RZ/G1C, RZ/G2 and RZ/A2 USB 2.0 PHY contain.
 
 Required properties:
-- compatible: "renesas,usb2-phy-r8a77470" if the device is a part of an R8A77470
+- compatible: "renesas,usb2-phy-r7s9210" if the device is a part of an R7S9210
+	      SoC.
+	      "renesas,usb2-phy-r8a77470" if the device is a part of an R8A77470
 	      SoC.
 	      "renesas,usb2-phy-r8a774a1" if the device is a part of an R8A774A1
 	      SoC.
@@ -20,8 +22,8 @@ Required properties:
 	      R8A77990 SoC.
 	      "renesas,usb2-phy-r8a77995" if the device is a part of an
 	      R8A77995 SoC.
-	      "renesas,rcar-gen3-usb2-phy" for a generic R-Car Gen3 or RZ/G2
-	      compatible device.
+	      "renesas,rcar-gen3-usb2-phy" for a generic R-Car Gen3, RZ/G2 or
+	      RZ/A2 compatible device.
 
 	      When compatible with the generic version, nodes must list the
 	      SoC-specific version corresponding to the platform first
-- 
2.16.1

[PATCH 06/10] usb: renesas_usbhs: Add support for RZ/A2

[Chris Brandt]

The RZ/A2 is similar to the R-Car Gen3 with some small differences.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 drivers/usb/renesas_usbhs/Makefile |  2 +-
 drivers/usb/renesas_usbhs/common.c | 27 +++++++++----
 drivers/usb/renesas_usbhs/common.h | 13 ++++++
 drivers/usb/renesas_usbhs/fifo.c   |  8 +++-
 drivers/usb/renesas_usbhs/rza.h    |  1 +
 drivers/usb/renesas_usbhs/rza2.c   | 82 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/renesas_usbhs.h  |  1 +
 7 files changed, 124 insertions(+), 10 deletions(-)
 create mode 100644 drivers/usb/renesas_usbhs/rza2.c

diff --git a/drivers/usb/renesas_usbhs/Makefile b/drivers/usb/renesas_usbhs/Makefile
index 5c5b51bb48ef..a1fed56b0957 100644
--- a/drivers/usb/renesas_usbhs/Makefile
+++ b/drivers/usb/renesas_usbhs/Makefile
@@ -5,7 +5,7 @@
 
 obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs.o
 
-renesas_usbhs-y			:= common.o mod.o pipe.o fifo.o rcar2.o rcar3.o rza.o
+renesas_usbhs-y			:= common.o mod.o pipe.o fifo.o rcar2.o rcar3.o rza.o rza2.o
 
 ifneq ($(CONFIG_USB_RENESAS_USBHS_HCD),)
 	renesas_usbhs-y		+= mod_host.o
diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
index 249fbee97f3f..8293f34b964a 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -44,13 +44,6 @@
  */
 
 
-#define USBHSF_RUNTIME_PWCTRL	(1 << 0)
-
-/* status */
-#define usbhsc_flags_init(p)   do {(p)->flags = 0; } while (0)
-#define usbhsc_flags_set(p, b) ((p)->flags |=  (b))
-#define usbhsc_flags_clr(p, b) ((p)->flags &= ~(b))
-#define usbhsc_flags_has(p, b) ((p)->flags &   (b))
 
 /*
  * platform call back
@@ -123,6 +116,12 @@ void usbhs_sys_function_ctrl(struct usbhs_priv *priv, int enable)
 	u16 mask = DCFM | DRPD | DPRPU | HSE | USBE;
 	u16 val  = HSE | USBE;
 
+	/* CNEN bit is required for function operation */
+	if (usbhsc_flags_has(priv, USBHSF_HAS_CNEN)) {
+		mask |= CNEN;
+		val  |= CNEN;
+	}
+
 	/*
 	 * if enable
 	 *
@@ -583,6 +582,10 @@ static const struct of_device_id usbhs_of_match[] = {
 		.compatible = "renesas,rza1-usbhs",
 		.data = (void *)USBHS_TYPE_RZA1,
 	},
+	{
+		.compatible = "renesas,rza2-usbhs",
+		.data = (void *)USBHS_TYPE_RZA2,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, usbhs_of_match);
@@ -620,6 +623,11 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
 		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
 	}
 
+	if (dparam->type == USBHS_TYPE_RZA2) {
+		dparam->pipe_configs = usbhsc_new_pipe;
+		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
+	}
+
 	return info;
 }
 
@@ -689,6 +697,11 @@ static int usbhs_probe(struct platform_device *pdev)
 	case USBHS_TYPE_RZA1:
 		priv->pfunc = usbhs_rza1_ops;
 		break;
+	case USBHS_TYPE_RZA2:
+		priv->pfunc = usbhs_rza2_ops;
+		usbhsc_flags_set(priv, USBHSF_HAS_CNEN);
+		usbhsc_flags_set(priv, USBHSF_CFIFO_BYTE_ADDR);
+		break;
 	default:
 		if (!info->platform_callback.get_id) {
 			dev_err(&pdev->dev, "no platform callbacks");
diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
index 3777af848a35..5978c30c9993 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -104,6 +104,7 @@ struct usbhs_priv;
 
 /* SYSCFG */
 #define SCKE	(1 << 10)	/* USB Module Clock Enable */
+#define CNEN	(1 << 8)	/* Single-ended receiver operation Enable */
 #define HSE	(1 << 7)	/* High-Speed Operation Enable */
 #define DCFM	(1 << 6)	/* Controller Function Select */
 #define DRPD	(1 << 5)	/* D+ Line/D- Line Resistance Control */
@@ -339,4 +340,16 @@ struct usbhs_priv *usbhs_pdev_to_priv(struct platform_device *pdev);
 #define usbhs_priv_to_dev(priv)		(&priv->pdev->dev)
 #define usbhs_priv_to_lock(priv)	(&priv->lock)
 
+/*
+ * flags
+ */
+#define USBHSF_RUNTIME_PWCTRL	(1 << 0)
+#define USBHSF_HAS_CNEN		(1 << 1) /* Single-ended receiver */
+#define USBHSF_CFIFO_BYTE_ADDR	(1 << 2) /* Byte addressable */
+
+#define usbhsc_flags_init(p)   do {(p)->flags = 0; } while (0)
+#define usbhsc_flags_set(p, b) ((p)->flags |=  (b))
+#define usbhsc_flags_clr(p, b) ((p)->flags &= ~(b))
+#define usbhsc_flags_has(p, b) ((p)->flags &   (b))
+
 #endif /* RENESAS_USB_DRIVER_H */
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index 39fa2fc1b8b7..9b8220c2c9cc 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -543,8 +543,12 @@ static int usbhsf_pio_try_push(struct usbhs_pkt *pkt, int *is_done)
 	}
 
 	/* the rest operation */
-	for (i = 0; i < len; i++)
-		iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
+	if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR))
+		for (i = 0; i < len; i++)
+			iowrite8(buf[i], addr + (i & 0x03));
+	else
+		for (i = 0; i < len; i++)
+			iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
 
 	/*
 	 * variable update
diff --git a/drivers/usb/renesas_usbhs/rza.h b/drivers/usb/renesas_usbhs/rza.h
index ca917ca54f6d..073a53d1d442 100644
--- a/drivers/usb/renesas_usbhs/rza.h
+++ b/drivers/usb/renesas_usbhs/rza.h
@@ -2,3 +2,4 @@
 #include "common.h"
 
 extern const struct renesas_usbhs_platform_callback usbhs_rza1_ops;
+extern const struct renesas_usbhs_platform_callback usbhs_rza2_ops;
diff --git a/drivers/usb/renesas_usbhs/rza2.c b/drivers/usb/renesas_usbhs/rza2.c
new file mode 100644
index 000000000000..c0b5dfa4b85d
--- /dev/null
+++ b/drivers/usb/renesas_usbhs/rza2.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas USB driver RZ/A2 initialization and power control
+ *
+ * Copyright (C) 2019 Chris Brandt
+ * Copyright (C) 2019 Renesas Electronics Corporation
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/of_device.h>
+#include <linux/phy/phy.h>
+#include "common.h"
+#include "rza.h"
+
+
+static int usbhs_rza2_hardware_init(struct platform_device *pdev)
+{
+	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+
+	if (IS_ENABLED(CONFIG_GENERIC_PHY)) {
+		struct phy *phy = phy_get(&pdev->dev, "usb");
+
+		if (IS_ERR(phy))
+			return PTR_ERR(phy);
+
+		priv->phy = phy;
+		return 0;
+	}
+	return -ENXIO;
+}
+
+static int usbhs_rza2_hardware_exit(struct platform_device *pdev)
+{
+	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+
+	if (priv->phy) {
+		phy_put(priv->phy);
+		priv->phy = NULL;
+	}
+
+	return 0;
+}
+
+static int usbhs_rza2_power_ctrl(struct platform_device *pdev,
+				void __iomem *base, int enable)
+{
+	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
+	int retval = -ENODEV;
+
+	if (priv->phy) {
+		if (enable) {
+			retval = phy_init(priv->phy);
+			if (enable) {
+				usbhs_bset(priv, SUSPMODE, SUSPM, SUSPM);
+				/* Wait 100 usec for PLL to become stable */
+				udelay(100);
+			} else {
+				usbhs_bset(priv, SUSPMODE, SUSPM, 0);
+			}
+			if (!retval)
+				retval = phy_power_on(priv->phy);
+		} else {
+			phy_power_off(priv->phy);
+			phy_exit(priv->phy);
+			retval = 0;
+		}
+	}
+	return retval;
+}
+
+static int usbhs_rza2_get_id(struct platform_device *pdev)
+{
+	return USBHS_GADGET;
+}
+
+const struct renesas_usbhs_platform_callback usbhs_rza2_ops = {
+	.hardware_init = usbhs_rza2_hardware_init,
+	.hardware_exit = usbhs_rza2_hardware_exit,
+	.power_ctrl = usbhs_rza2_power_ctrl,
+	.get_id = usbhs_rza2_get_id,
+};
diff --git a/include/linux/usb/renesas_usbhs.h b/include/linux/usb/renesas_usbhs.h
index 53924f8e840c..39604c8b1eed 100644
--- a/include/linux/usb/renesas_usbhs.h
+++ b/include/linux/usb/renesas_usbhs.h
@@ -196,6 +196,7 @@ struct renesas_usbhs_driver_param {
 #define USBHS_TYPE_RCAR_GEN3		2
 #define USBHS_TYPE_RCAR_GEN3_WITH_PLL	3
 #define USBHS_TYPE_RZA1			4
+#define USBHS_TYPE_RZA2			5
 
 /*
  * option:
-- 
2.16.1

RE: [PATCH 06/10] usb: renesas_usbhs: Add support for RZ/A2

[Yoshihiro Shimoda]

Hi Chrisさん

Thank you for the patch!

> From: Chris Brandt, Sent: Tuesday, May 7, 2019 8:46 AM
> 
> The RZ/A2 is similar to the R-Car Gen3 with some small differences.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  drivers/usb/renesas_usbhs/Makefile |  2 +-
>  drivers/usb/renesas_usbhs/common.c | 27 +++++++++----
>  drivers/usb/renesas_usbhs/common.h | 13 ++++++
>  drivers/usb/renesas_usbhs/fifo.c   |  8 +++-
>  drivers/usb/renesas_usbhs/rza.h    |  1 +
>  drivers/usb/renesas_usbhs/rza2.c   | 82 ++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/renesas_usbhs.h  |  1 +
>  7 files changed, 124 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/usb/renesas_usbhs/rza2.c
> 
> diff --git a/drivers/usb/renesas_usbhs/Makefile b/drivers/usb/renesas_usbhs/Makefile
> index 5c5b51bb48ef..a1fed56b0957 100644
> --- a/drivers/usb/renesas_usbhs/Makefile
> +++ b/drivers/usb/renesas_usbhs/Makefile
> @@ -5,7 +5,7 @@
> 
>  obj-$(CONFIG_USB_RENESAS_USBHS)	+= renesas_usbhs.o
> 
> -renesas_usbhs-y			:= common.o mod.o pipe.o fifo.o rcar2.o rcar3.o rza.o
> +renesas_usbhs-y			:= common.o mod.o pipe.o fifo.o rcar2.o rcar3.o rza.o rza2.o
> 
>  ifneq ($(CONFIG_USB_RENESAS_USBHS_HCD),)
>  	renesas_usbhs-y		+= mod_host.o
> diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> index 249fbee97f3f..8293f34b964a 100644
> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c
> @@ -44,13 +44,6 @@
>   */
> 
> 
> -#define USBHSF_RUNTIME_PWCTRL	(1 << 0)
> -
> -/* status */
> -#define usbhsc_flags_init(p)   do {(p)->flags = 0; } while (0)
> -#define usbhsc_flags_set(p, b) ((p)->flags |=  (b))
> -#define usbhsc_flags_clr(p, b) ((p)->flags &= ~(b))
> -#define usbhsc_flags_has(p, b) ((p)->flags &   (b))

I would like to separate this patch to some patches like below to review the patch(es) easily:

1. Just move these definitions to common.h.
2. Add USBHSF_HAS_CNEN and related code.
3. Add USBHSF_CFIFO_BYTE_ADDR and related code.
4. Add RZ/A2 support.

<snip>
> @@ -620,6 +623,11 @@ static struct renesas_usbhs_platform_info *usbhs_parse_dt(struct device *dev)
>  		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
>  	}
> 
> +	if (dparam->type == USBHS_TYPE_RZA2) {
> +		dparam->pipe_configs = usbhsc_new_pipe;
> +		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> +	}
> +

It's the same with RZA1. So, I think we can reuse the code like below. What do you think?
+	if (dparam->type == USBHS_TYPE_RZA1 ||
+	    dparam->type == USBHS_TYPE_RZA2) {
		dparam->pipe_configs = usbhsc_new_pipe;
		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
	}

<snip>
> diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
> index 39fa2fc1b8b7..9b8220c2c9cc 100644
> --- a/drivers/usb/renesas_usbhs/fifo.c
> +++ b/drivers/usb/renesas_usbhs/fifo.c
> @@ -543,8 +543,12 @@ static int usbhsf_pio_try_push(struct usbhs_pkt *pkt, int *is_done)
>  	}
> 
>  	/* the rest operation */
> -	for (i = 0; i < len; i++)
> -		iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
> +	if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR))
> +		for (i = 0; i < len; i++)
> +			iowrite8(buf[i], addr + (i & 0x03));
> +	else
> +		for (i = 0; i < len; i++)
> +			iowrite8(buf[i], addr + (0x03 - (i & 0x03)));

I prefer to add "{ }" on "if" and "else" like below.

	if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR)) {
		for (i = 0; i < len; i++)
			iowrite8(buf[i], addr + (i & 0x03));
	} else {
		for (i = 0; i < len; i++)
			iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
	}

>  	/*
>  	 * variable update
> diff --git a/drivers/usb/renesas_usbhs/rza.h b/drivers/usb/renesas_usbhs/rza.h
> index ca917ca54f6d..073a53d1d442 100644
> --- a/drivers/usb/renesas_usbhs/rza.h
> +++ b/drivers/usb/renesas_usbhs/rza.h
> @@ -2,3 +2,4 @@
>  #include "common.h"
> 
>  extern const struct renesas_usbhs_platform_callback usbhs_rza1_ops;
> +extern const struct renesas_usbhs_platform_callback usbhs_rza2_ops;
> diff --git a/drivers/usb/renesas_usbhs/rza2.c b/drivers/usb/renesas_usbhs/rza2.c
> new file mode 100644
> index 000000000000..c0b5dfa4b85d
> --- /dev/null
> +++ b/drivers/usb/renesas_usbhs/rza2.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas USB driver RZ/A2 initialization and power control
> + *
> + * Copyright (C) 2019 Chris Brandt
> + * Copyright (C) 2019 Renesas Electronics Corporation
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/of_device.h>
> +#include <linux/phy/phy.h>
> +#include "common.h"
> +#include "rza.h"
> +
> +
> +static int usbhs_rza2_hardware_init(struct platform_device *pdev)
> +{
> +	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> +
> +	if (IS_ENABLED(CONFIG_GENERIC_PHY)) {
> +		struct phy *phy = phy_get(&pdev->dev, "usb");
> +
> +		if (IS_ERR(phy))
> +			return PTR_ERR(phy);
> +
> +		priv->phy = phy;
> +		return 0;
> +	}
> +	return -ENXIO;
> +}
> +
> +static int usbhs_rza2_hardware_exit(struct platform_device *pdev)
> +{
> +	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> +
> +	if (priv->phy) {
> +		phy_put(priv->phy);
> +		priv->phy = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int usbhs_rza2_power_ctrl(struct platform_device *pdev,
> +				void __iomem *base, int enable)
> +{
> +	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> +	int retval = -ENODEV;
> +
> +	if (priv->phy) {
> +		if (enable) {
> +			retval = phy_init(priv->phy);
> +			if (enable) {
> +				usbhs_bset(priv, SUSPMODE, SUSPM, SUSPM);
> +				/* Wait 100 usec for PLL to become stable */
> +				udelay(100);
> +			} else {

This else code never runs. So,

> +				usbhs_bset(priv, SUSPMODE, SUSPM, 0);

this code should be on the below "here"?

> +			}
> +			if (!retval)
> +				retval = phy_power_on(priv->phy);
> +		} else {

here?

Best regardsm
Yoshihiro Shimoda

RE: [PATCH 06/10] usb: renesas_usbhs: Add support for RZ/A2

[Chris Brandt]

Hi Shimodaさん、

> From: Yoshihiro Shimoda
> Sent: Thursday, May 09, 2019 3:04 AM

> > -/* status */
> > -#define usbhsc_flags_init(p)   do {(p)->flags = 0; } while (0)
> > -#define usbhsc_flags_set(p, b) ((p)->flags |=  (b))
> > -#define usbhsc_flags_clr(p, b) ((p)->flags &= ~(b))
> > -#define usbhsc_flags_has(p, b) ((p)->flags &   (b))
> 
> I would like to separate this patch to some patches like below to review
> the patch(es) easily:
> 
> 1. Just move these definitions to common.h.

FYI, checkpatch.pl says this:

  WARNING: Single statement macros should not use a do {} while (0) loop
  #122: FILE: drivers/usb/renesas_usbhs/common.h:350:
  +#define usbhsc_flags_init(p)   do {(p)->flags = 0; } while (0)

So, I will change this code to:

#define usbhsc_flags_init(p)   {(p)->flags = 0;}



> It's the same with RZA1. So, I think we can reuse the code like below.
> What do you think?
> +	if (dparam->type == USBHS_TYPE_RZA1 ||
> +	    dparam->type == USBHS_TYPE_RZA2) {
> 		dparam->pipe_configs = usbhsc_new_pipe;
> 		dparam->pipe_size = ARRAY_SIZE(usbhsc_new_pipe);
> 	}

OK.

#At first, RZA2 had 'dparam->has_usb_dmac = 1'. But, DMA had some
 issues, so I removed it.



> I prefer to add "{ }" on "if" and "else" like below.
> 
> 	if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR)) {
> 		for (i = 0; i < len; i++)
> 			iowrite8(buf[i], addr + (i & 0x03));
> 	} else {
> 		for (i = 0; i < len; i++)
> 			iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
> 	}

OK.
#I always prefer braces. It is easier to read.


> > +static int usbhs_rza2_power_ctrl(struct platform_device *pdev,
> > +				void __iomem *base, int enable)
> > +{
> > +	struct usbhs_priv *priv = usbhs_pdev_to_priv(pdev);
> > +	int retval = -ENODEV;
> > +
> > +	if (priv->phy) {
> > +		if (enable) {
> > +			retval = phy_init(priv->phy);
> > +			if (enable) {
> > +				usbhs_bset(priv, SUSPMODE, SUSPM, SUSPM);
> > +				/* Wait 100 usec for PLL to become stable */
> > +				udelay(100);
> > +			} else {
> 
> This else code never runs. So,

Yes, thank you.

This code is ugly, so I'm going to change it.

Chris

[PATCH 07/10] dt-bindings: usb: renesas_usbhs: Add support for r7s9210

[Chris Brandt]

Add support for r7s9210 (RZ/A2M) SoC

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 Documentation/devicetree/bindings/usb/renesas_usbhs.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
index b8acc2a994a8..11c99d079dfb 100644
--- a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
+++ b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
@@ -20,9 +20,11 @@ Required properties:
 	- "renesas,usbhs-r8a77990" for r8a77990 (R-Car E3) compatible device
 	- "renesas,usbhs-r8a77995" for r8a77995 (R-Car D3) compatible device
 	- "renesas,usbhs-r7s72100" for r7s72100 (RZ/A1) compatible device
+	- "renesas,usbhs-r7s9210" for r7s72100 (RZ/A2) compatible device
 	- "renesas,rcar-gen2-usbhs" for R-Car Gen2 or RZ/G1 compatible devices
 	- "renesas,rcar-gen3-usbhs" for R-Car Gen3 or RZ/G2 compatible devices
 	- "renesas,rza1-usbhs" for RZ/A1 compatible device
+	- "renesas,rza2-usbhs" for RZ/A2 compatible device
 
 	When compatible with the generic version, nodes must list the
 	SoC-specific version corresponding to the platform first followed
-- 
2.16.1

[PATCH 08/10] ARM: dts: r7s9210: Add USB Host support

[Chris Brandt]

Add EHCI and OHCI host support for RZ/A2.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 arch/arm/boot/dts/r7s9210.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
index 2eaa5eeba509..1a992e6197c3 100644
--- a/arch/arm/boot/dts/r7s9210.dtsi
+++ b/arch/arm/boot/dts/r7s9210.dtsi
@@ -322,6 +322,70 @@
 			status = "disabled";
 		};
 
+		ohci0: usbhcd@e8218000 {
+			compatible = "generic-ohci";
+			reg = <0xe8218000 0x100>;
+			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 61>;
+			phys = <&usb2_phy0>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		ehci0: usbhcd@e8218100 {
+			compatible = "generic-ehci";
+			reg = <0xe8218100 0x100>;
+			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 61>;
+			phys = <&usb2_phy0>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		usb2_phy0: usb-phy@e8218200 {
+			compatible = "renesas,usb2-phy-r7s9210","renesas,rcar-gen3-usb2-phy";
+			reg = <0xe8218200 0x10>;
+			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 61>;
+			power-domains = <&cpg>;
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
+		ohci1: usbhcd@e821a000 {
+			compatible = "generic-ohci";
+			reg = <0xe821a000 0x100>;
+			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 60>;
+			phys = <&usb2_phy1>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		ehci1: usbhcd@e821a100 {
+			compatible = "generic-ehci";
+			reg = <0xe821a100 0x100>;
+			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 60>;
+			phys = <&usb2_phy1>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		usb2_phy1: usb-phy@e821a200 {
+			compatible = "renesas,usb2-phy-r7s9210","renesas,rcar-gen3-usb2-phy";
+			reg = <0xe821a200 0x10>;
+			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 60>;
+			power-domains = <&cpg>;
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
 		sdhi0: sd@e8228000 {
 			compatible = "renesas,sdhi-r7s9210";
 			reg = <0xe8228000 0x8c0>;
-- 
2.16.1

Re: [PATCH 08/10] ARM: dts: r7s9210: Add USB Host support

[Simon Horman]

On Mon, May 06, 2019 at 06:46:29PM -0500, Chris Brandt wrote:
> Add EHCI and OHCI host support for RZ/A2.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  arch/arm/boot/dts/r7s9210.dtsi | 64 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
> index 2eaa5eeba509..1a992e6197c3 100644
> --- a/arch/arm/boot/dts/r7s9210.dtsi
> +++ b/arch/arm/boot/dts/r7s9210.dtsi
> @@ -322,6 +322,70 @@
>  			status = "disabled";
>  		};
>  
> +		ohci0: usbhcd@e8218000 {
> +			compatible = "generic-ohci";
> +			reg = <0xe8218000 0x100>;
> +			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 61>;
> +			phys = <&usb2_phy0>;
> +			phy-names = "usb";
> +			power-domains = <&cpg>;
> +			status = "disabled";
> +		};
> +
> +		ehci0: usbhcd@e8218100 {
> +			compatible = "generic-ehci";
> +			reg = <0xe8218100 0x100>;
> +			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 61>;
> +			phys = <&usb2_phy0>;
> +			phy-names = "usb";
> +			power-domains = <&cpg>;
> +			status = "disabled";
> +		};
> +
> +		usb2_phy0: usb-phy@e8218200 {
> +			compatible = "renesas,usb2-phy-r7s9210","renesas,rcar-gen3-usb2-phy";

Hi Chris,

please add a space after ','. Likewise below.

Otherwise this patch looks good to me.

> +			reg = <0xe8218200 0x10>;
> +			interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 61>;
> +			power-domains = <&cpg>;
> +			#phy-cells = <0>;
> +			status = "disabled";
> +		};
> +
> +		ohci1: usbhcd@e821a000 {
> +			compatible = "generic-ohci";
> +			reg = <0xe821a000 0x100>;
> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 60>;
> +			phys = <&usb2_phy1>;
> +			phy-names = "usb";
> +			power-domains = <&cpg>;
> +			status = "disabled";
> +		};
> +
> +		ehci1: usbhcd@e821a100 {
> +			compatible = "generic-ehci";
> +			reg = <0xe821a100 0x100>;
> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 60>;
> +			phys = <&usb2_phy1>;
> +			phy-names = "usb";
> +			power-domains = <&cpg>;
> +			status = "disabled";
> +		};
> +
> +		usb2_phy1: usb-phy@e821a200 {
> +			compatible = "renesas,usb2-phy-r7s9210","renesas,rcar-gen3-usb2-phy";
> +			reg = <0xe821a200 0x10>;
> +			interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 60>;
> +			power-domains = <&cpg>;
> +			#phy-cells = <0>;
> +			status = "disabled";
> +		};
> +
>  		sdhi0: sd@e8228000 {
>  			compatible = "renesas,sdhi-r7s9210";
>  			reg = <0xe8228000 0x8c0>;
> -- 
> 2.16.1
> 

[PATCH 09/10] ARM: dts: r7s9210: Add USB Device support

[Chris Brandt]

Add USB Device support for RZ/A2.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 arch/arm/boot/dts/r7s9210.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
index 1a992e6197c3..67ac746142d0 100644
--- a/arch/arm/boot/dts/r7s9210.dtsi
+++ b/arch/arm/boot/dts/r7s9210.dtsi
@@ -354,6 +354,18 @@
 			status = "disabled";
 		};
 
+		usbhs0: usbhs@e8219000 {
+			compatible = "renesas,usbhs-r7s9210","renesas,rza2-usbhs";
+			reg = <0xe8219000 0x724>;
+			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 61>;
+			renesas,buswait = <7>;
+			phys = <&usb2_phy0>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
 		ohci1: usbhcd@e821a000 {
 			compatible = "generic-ohci";
 			reg = <0xe821a000 0x100>;
@@ -386,6 +398,18 @@
 			status = "disabled";
 		};
 
+		usbhs1: usbhs@e821b000 {
+			compatible = "renesas,usbhs-r7s9210","renesas,rza2-usbhs";
+			reg = <0xe821b000 0x724>;
+			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD 60>;
+			renesas,buswait = <7>;
+			phys = <&usb2_phy1>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
 		sdhi0: sd@e8228000 {
 			compatible = "renesas,sdhi-r7s9210";
 			reg = <0xe8228000 0x8c0>;
-- 
2.16.1

Re: [PATCH 09/10] ARM: dts: r7s9210: Add USB Device support

[Sergei Shtylyov]

On 07.05.2019 2:46, Chris Brandt wrote:

> Add USB Device support for RZ/A2.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>   arch/arm/boot/dts/r7s9210.dtsi | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
> index 1a992e6197c3..67ac746142d0 100644
> --- a/arch/arm/boot/dts/r7s9210.dtsi
> +++ b/arch/arm/boot/dts/r7s9210.dtsi
> @@ -354,6 +354,18 @@
>   			status = "disabled";
>   		};
>   
> +		usbhs0: usbhs@e8219000 {

   The node names should be generic, i.e. "usb@e8219000".

[...]
> @@ -386,6 +398,18 @@
>   			status = "disabled";
>   		};
>   
> +		usbhs1: usbhs@e821b000 {

    Same here.

[...]

MBR, Sergei

RE: [PATCH 09/10] ARM: dts: r7s9210: Add USB Device support

[Chris Brandt]

On Tue, May 07, 2019, Sergei Shtylyov wrote:
> > +		usbhs0: usbhs@e8219000 {
> 
>    The node names should be generic, i.e. "usb@e8219000".
> 

Good point.

Although it seems like usb-phy is still correct.

Thanks,
Chris

Re: [PATCH 09/10] ARM: dts: r7s9210: Add USB Device support

[Simon Horman]

On Mon, May 06, 2019 at 06:46:30PM -0500, Chris Brandt wrote:
> Add USB Device support for RZ/A2.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  arch/arm/boot/dts/r7s9210.dtsi | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
> index 1a992e6197c3..67ac746142d0 100644
> --- a/arch/arm/boot/dts/r7s9210.dtsi
> +++ b/arch/arm/boot/dts/r7s9210.dtsi
> @@ -354,6 +354,18 @@
>  			status = "disabled";
>  		};
>  
> +		usbhs0: usbhs@e8219000 {
> +			compatible = "renesas,usbhs-r7s9210","renesas,rza2-usbhs";

Hi Chris,

please add a space after ",". Likewise below.

Otherwise this patch looks good to me.

> +			reg = <0xe8219000 0x724>;
> +			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 61>;
> +			renesas,buswait = <7>;
> +			phys = <&usb2_phy0>;
> +			phy-names = "usb";
> +			power-domains = <&cpg>;
> +			status = "disabled";
> +		};
> +
>  		ohci1: usbhcd@e821a000 {
>  			compatible = "generic-ohci";
>  			reg = <0xe821a000 0x100>;
> @@ -386,6 +398,18 @@
>  			status = "disabled";
>  		};
>  
> +		usbhs1: usbhs@e821b000 {
> +			compatible = "renesas,usbhs-r7s9210","renesas,rza2-usbhs";
> +			reg = <0xe821b000 0x724>;
> +			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cpg CPG_MOD 60>;
> +			renesas,buswait = <7>;
> +			phys = <&usb2_phy1>;
> +			phy-names = "usb";
> +			power-domains = <&cpg>;
> +			status = "disabled";
> +		};
> +
>  		sdhi0: sd@e8228000 {
>  			compatible = "renesas,sdhi-r7s9210";
>  			reg = <0xe8228000 0x8c0>;
> -- 
> 2.16.1
> 

Re: [PATCH 09/10] ARM: dts: r7s9210: Add USB Device support

[Simon Horman]

On Wed, May 08, 2019 at 11:43:13AM +0200, Simon Horman wrote:
> On Mon, May 06, 2019 at 06:46:30PM -0500, Chris Brandt wrote:
> > Add USB Device support for RZ/A2.
> > 
> > Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> > ---
> >  arch/arm/boot/dts/r7s9210.dtsi | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/r7s9210.dtsi b/arch/arm/boot/dts/r7s9210.dtsi
> > index 1a992e6197c3..67ac746142d0 100644
> > --- a/arch/arm/boot/dts/r7s9210.dtsi
> > +++ b/arch/arm/boot/dts/r7s9210.dtsi
> > @@ -354,6 +354,18 @@
> >  			status = "disabled";
> >  		};
> >  
> > +		usbhs0: usbhs@e8219000 {
> > +			compatible = "renesas,usbhs-r7s9210","renesas,rza2-usbhs";
> 
> Hi Chris,
> 
> please add a space after ",". Likewise below.
> 
> Otherwise this patch looks good to me.

I meant to say, that it looks good but please address Sergei's feedback.

> 
> > +			reg = <0xe8219000 0x724>;
> > +			interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&cpg CPG_MOD 61>;
> > +			renesas,buswait = <7>;
> > +			phys = <&usb2_phy0>;
> > +			phy-names = "usb";
> > +			power-domains = <&cpg>;
> > +			status = "disabled";
> > +		};
> > +
> >  		ohci1: usbhcd@e821a000 {
> >  			compatible = "generic-ohci";
> >  			reg = <0xe821a000 0x100>;
> > @@ -386,6 +398,18 @@
> >  			status = "disabled";
> >  		};
> >  
> > +		usbhs1: usbhs@e821b000 {
> > +			compatible = "renesas,usbhs-r7s9210","renesas,rza2-usbhs";
> > +			reg = <0xe821b000 0x724>;
> > +			interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&cpg CPG_MOD 60>;
> > +			renesas,buswait = <7>;
> > +			phys = <&usb2_phy1>;
> > +			phy-names = "usb";
> > +			power-domains = <&cpg>;
> > +			status = "disabled";
> > +		};
> > +
> >  		sdhi0: sd@e8228000 {
> >  			compatible = "renesas,sdhi-r7s9210";
> >  			reg = <0xe8228000 0x8c0>;
> > -- 
> > 2.16.1
> > 
> 

[PATCH 10/10] ARM: dts: r7s9210-rza2mevb: Add USB host support

[Chris Brandt]

Enable USB Host support for both the Type-C connector on the CPU board
and the Type-A plug on the sub board.

Both boards are also capable of USB Device operation as well after the
appropriate Device Tree modifications.

Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
---
 arch/arm/boot/dts/r7s9210-rza2mevb.dts | 37 ++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm/boot/dts/r7s9210-rza2mevb.dts b/arch/arm/boot/dts/r7s9210-rza2mevb.dts
index 1eba37db7cdc..79d9d4b4779f 100644
--- a/arch/arm/boot/dts/r7s9210-rza2mevb.dts
+++ b/arch/arm/boot/dts/r7s9210-rza2mevb.dts
@@ -100,6 +100,18 @@
 		pinmux = <RZA2_PINMUX(PORT5, 4, 3)>,	/* SD1_CD */
 			 <RZA2_PINMUX(PORT5, 5, 3)>;	/* SD1_WP */
 	};
+
+	usb0_pins: usb0 {
+		pinmux = <RZA2_PINMUX(PORT5, 2, 3)>,	/* VBUSIN0 */
+			 <RZA2_PINMUX(PORTC, 6, 1)>,	/* VBUSEN0 */
+			 <RZA2_PINMUX(PORTC, 7, 1)>;	/* OVRCUR0 */
+	};
+
+	usb1_pins: usb1 {
+		pinmux = <RZA2_PINMUX(PORTC, 0, 1)>,	/* VBUSIN1 */
+			 <RZA2_PINMUX(PORTC, 5, 1)>,	/* VBUSEN1 */
+			 <RZA2_PINMUX(PORT7, 5, 5)>;	/* OVRCUR1 */
+	};
 };
 
 /* High resolution System tick timers */
@@ -154,3 +166,28 @@
 	bus-width = <4>;
 	status = "okay";
 };
+
+/* USB-0 as Host */
+/* NOTE: Requires JP3 to be fitted */
+&usb2_phy0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&usb0_pins>;
+	renesas,uses_usb_x1;
+	dr_mode = "host";
+	status = "okay";
+};
+&ehci0 {
+	status = "okay";
+};
+
+/* USB-1 as Host */
+&usb2_phy1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&usb1_pins>;
+	renesas,uses_usb_x1;
+	dr_mode = "host";
+	status = "okay";
+};
+&ehci1 {
+	status = "okay";
+};
-- 
2.16.1

Re: [PATCH 10/10] ARM: dts: r7s9210-rza2mevb: Add USB host support

[Simon Horman]

On Mon, May 06, 2019 at 06:46:31PM -0500, Chris Brandt wrote:
> Enable USB Host support for both the Type-C connector on the CPU board
> and the Type-A plug on the sub board.
> 
> Both boards are also capable of USB Device operation as well after the
> appropriate Device Tree modifications.
> 
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> ---
>  arch/arm/boot/dts/r7s9210-rza2mevb.dts | 37 ++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r7s9210-rza2mevb.dts b/arch/arm/boot/dts/r7s9210-rza2mevb.dts
> index 1eba37db7cdc..79d9d4b4779f 100644
> --- a/arch/arm/boot/dts/r7s9210-rza2mevb.dts
> +++ b/arch/arm/boot/dts/r7s9210-rza2mevb.dts
> @@ -100,6 +100,18 @@
>  		pinmux = <RZA2_PINMUX(PORT5, 4, 3)>,	/* SD1_CD */
>  			 <RZA2_PINMUX(PORT5, 5, 3)>;	/* SD1_WP */
>  	};
> +
> +	usb0_pins: usb0 {
> +		pinmux = <RZA2_PINMUX(PORT5, 2, 3)>,	/* VBUSIN0 */
> +			 <RZA2_PINMUX(PORTC, 6, 1)>,	/* VBUSEN0 */
> +			 <RZA2_PINMUX(PORTC, 7, 1)>;	/* OVRCUR0 */
> +	};
> +
> +	usb1_pins: usb1 {
> +		pinmux = <RZA2_PINMUX(PORTC, 0, 1)>,	/* VBUSIN1 */
> +			 <RZA2_PINMUX(PORTC, 5, 1)>,	/* VBUSEN1 */
> +			 <RZA2_PINMUX(PORT7, 5, 5)>;	/* OVRCUR1 */
> +	};
>  };
>  
>  /* High resolution System tick timers */
> @@ -154,3 +166,28 @@
>  	bus-width = <4>;
>  	status = "okay";
>  };
> +
> +/* USB-0 as Host */
> +/* NOTE: Requires JP3 to be fitted */
> +&usb2_phy0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&usb0_pins>;
> +	renesas,uses_usb_x1;
> +	dr_mode = "host";
> +	status = "okay";
> +};

Hi Chris,

Please add a space between the usb2_phy0 and ehci0 nodes.
Likewise below between the usb2_phy1 and ehci1 nodes.

Otherwise this patch looks good to me.

> +&ehci0 {
> +	status = "okay";
> +};
> +
> +/* USB-1 as Host */
> +&usb2_phy1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&usb1_pins>;
> +	renesas,uses_usb_x1;
> +	dr_mode = "host";
> +	status = "okay";
> +};
> +&ehci1 {
> +	status = "okay";
> +};
> -- 
> 2.16.1
> 

RE: [PATCH 10/10] ARM: dts: r7s9210-rza2mevb: Add USB host support

[Chris Brandt]

Hi Simon,

On Wed, May 08, 2019, Simon Horman wrote:
> Please add a space between the usb2_phy0 and ehci0 nodes.
> Likewise below between the usb2_phy1 and ehci1 nodes.
> 
> Otherwise this patch looks good to me.

I also see that you renamed some patch titles from
  "ARM: dts: r7s9210-rza2mevb: xxx" 
to
  "ARM: dts: rza2mevb: xxx"

when you applied them.
So I will make that change as well.

Chris

RE: [PATCH 00/10] usb: Add host and device support for RZ/A2

[Yoshihiro Shimoda]

Hi Chris-san,

> From: Chris Brandt, Sent: Tuesday, May 7, 2019 8:46 AM
> 
> For the most part, the RZ/A2 has the same USB 2.0 host and device
> HW as the R-Car Gen3, so we can reuse a lot of the code.
> 
> However, there are a couple extra register bits, and the CFIFO
> register 8-bit access works a little different (weird, no idea why).

This is just my gut feeling, but if we set the BIGEND bit in the CFIFOSEL
of RZ/A2M (R-Car Gen3 doesn't have such a bit though), could the original
code work correctly?

Best regards,
Yoshihiro Shimoda

RE: [PATCH 00/10] usb: Add host and device support for RZ/A2

[Chris Brandt]

Hi Shimodaさん

> From: Yoshihiro Shimoda
> Sent: Tuesday, May 07, 2019 5:17 AM
> > For the most part, the RZ/A2 has the same USB 2.0 host and device
> > HW as the R-Car Gen3, so we can reuse a lot of the code.
> >
> > However, there are a couple extra register bits, and the CFIFO
> > register 8-bit access works a little different (weird, no idea why).
> 
> This is just my gut feeling, but if we set the BIGEND bit in the CFIFOSEL
> of RZ/A2M (R-Car Gen3 doesn't have such a bit though), could the original
> code work correctly?

I just tried to set CFIFOSEL.BIGEND = 1

 * Set CFIFOSEL.BIGEND = 1
 * Write 8-bit values to CFIFO (same method as R-Car)
 * Set CFIFOSEL.BIGEND = 0

The result is bad.


But, then I tried this:
 * Set CFIFOSEL.MBW = 0   (CFIFO port access = 8-bit)
 * Write 8-bit values to CFIFO
 * Set CFIFOSEL.MBW = 2   (CFIFO port access = 32-bit)

Code:
u16 cfifosel = usbhs_read(priv, fifo->sel);

usbhs_write(priv, fifo->sel, cfifosel & 0xF3FF); // MBW = 8-bit

		for (i = 0; i < len; i++)
			iowrite8(buf[i], addr); //same address each time

usbhs_write(priv, fifo->sel, cfifosel);	// MBW = 32-bit


This method works good.

  (I assume this method would work with R-Car also)

But...then we have extra register reads and writes.
Register accesses are slower, so performance is lower.

So, I prefer my original method:
	if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR))
		for (i = 0; i < len; i++)
			iowrite8(buf[i], addr + (i & 0x03));
	else
		for (i = 0; i < len; i++)
			iowrite8(buf[i], addr + (0x03 - (i & 0x03)));


Do you agree?

Chris

RE: [PATCH 00/10] usb: Add host and device support for RZ/A2

[Yoshihiro Shimoda]

Hi Chrisさん

> From: Chris Brandt, Sent: Thursday, May 9, 2019 3:08 AM
> 
> Hi Shimodaさん
> 
> > From: Yoshihiro Shimoda
> > Sent: Tuesday, May 07, 2019 5:17 AM
> > > For the most part, the RZ/A2 has the same USB 2.0 host and device
> > > HW as the R-Car Gen3, so we can reuse a lot of the code.
> > >
> > > However, there are a couple extra register bits, and the CFIFO
> > > register 8-bit access works a little different (weird, no idea why).
> >
> > This is just my gut feeling, but if we set the BIGEND bit in the CFIFOSEL
> > of RZ/A2M (R-Car Gen3 doesn't have such a bit though), could the original
> > code work correctly?
> 
> I just tried to set CFIFOSEL.BIGEND = 1

Thank you for trying it!

>  * Set CFIFOSEL.BIGEND = 1
>  * Write 8-bit values to CFIFO (same method as R-Car)
>  * Set CFIFOSEL.BIGEND = 0
> 
> The result is bad.

I got it...

> But, then I tried this:
>  * Set CFIFOSEL.MBW = 0   (CFIFO port access = 8-bit)
>  * Write 8-bit values to CFIFO
>  * Set CFIFOSEL.MBW = 2   (CFIFO port access = 32-bit)
> 
> Code:
> u16 cfifosel = usbhs_read(priv, fifo->sel);
> 
> usbhs_write(priv, fifo->sel, cfifosel & 0xF3FF); // MBW = 8-bit
> 
> 		for (i = 0; i < len; i++)
> 			iowrite8(buf[i], addr); //same address each time
> 
> usbhs_write(priv, fifo->sel, cfifosel);	// MBW = 32-bit
> 
> 
> This method works good.

I got it.

>   (I assume this method would work with R-Car also)

Unfortunately, R-Car cannot work with this method...
But, "iowrite8(buf[i], addr + 3);" or "iowrite32(buf[i], addr);" works on the R-Car.
And then, I realized that R-Car CFIFO register allows 32-bit access only...
# So, I'm asking HW guy whether the 8-bit access can be allowed or not now...

> But...then we have extra register reads and writes.
> Register accesses are slower, so performance is lower.
> 
> So, I prefer my original method:
> 	if (usbhsc_flags_has(priv, USBHSF_CFIFO_BYTE_ADDR))
> 		for (i = 0; i < len; i++)
> 			iowrite8(buf[i], addr + (i & 0x03));
> 	else
> 		for (i = 0; i < len; i++)
> 			iowrite8(buf[i], addr + (0x03 - (i & 0x03)));
> 
> 
> Do you agree?

I agree.
However, I have some comments about the patch. So, I'll reply on the patch later.

Best regards,
Yoshihiro Shimoda

> Chris