[PATCH 0/6] Add RZ/G2L USB2.0 phy and host support

[PATCH 0/6] Add RZ/G2L USB2.0 phy and host support

[Biju Das]

This patch series adds USB2.0 phy and Host support for RZ/G2L SoC.

A new driver introduced for RZ/G2L USB phy control block,
which mainly controls reset and power down of the USB2.0/PHY.

This patchset is based on master branch [1]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/

Biju Das (6):
  dt-bindings: phy: renesas: Document RZ/G2L USB PHY Control bindings
  drivers: clk: renesas: r9a07g044-cpg: Add USB clocks
  phy: renesas: Add RZ/G2L usb phy control driver
  arm64: configs: defconfig: Enable RZ/G2L USB PHY control driver
  dt-bindings: phy: renesas,usb2-phy: Document RZ/G2L phy bindings
  arm64: dts: renesas: r9a07g044: Add USB2.0 phy and host support

 .../phy/renesas,rzg2l-usbphyctrl.yaml         |  50 ++++++
 .../bindings/phy/renesas,usb2-phy.yaml        |   1 +
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi    |  81 +++++++++
 arch/arm64/configs/defconfig                  |   1 +
 drivers/clk/renesas/r9a07g044-cpg.c           |   6 +
 drivers/phy/renesas/Kconfig                   |   7 +
 drivers/phy/renesas/Makefile                  |   1 +
 drivers/phy/renesas/phy-rzg2l-usbphyctrl.c    | 163 ++++++++++++++++++
 8 files changed, 310 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
 create mode 100644 drivers/phy/renesas/phy-rzg2l-usbphyctrl.c

-- 
2.17.1

[PATCH 1/6] dt-bindings: phy: renesas: Document RZ/G2L USB PHY Control bindings

[Biju Das]

Add device tree binding document for RZ/G2L USB PHY control driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../phy/renesas,rzg2l-usbphyctrl.yaml         | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml

diff --git a/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml b/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
new file mode 100644
index 000000000000..5fd316a2e79e
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/renesas,rzg2l-usbphyctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/G2L USB2.0 PHY Control
+
+maintainers:
+  - Biju Das <biju.das.jz@bp.renesas.com>
+
+description:
+  The RZ/G2L USB2.0 PHY Control mainly controls reset and power down of the
+  USB/PHY.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a07g044-usbphyctrl # RZ/G2{L,LC}
+      - const: renesas,rzg2l-usbphyctrl
+
+  reg:
+    maxItems: 1
+
+  '#phy-cells':
+    # see phy-bindings.txt in the same directory
+    const: 1
+    description: |
+      The phandle's argument in the PHY specifier is the phy reset control bit
+      of usb phy control.
+      0 = Port 1 Phy reset
+      1 = Port 2 Phy reset
+    enum: [ 0, 1 ]
+
+required:
+  - compatible
+  - reg
+  - '#phy-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    usbphyctrl@11c40000 {
+        compatible = "renesas,r9a07g044-usbphyctrl",
+                     "renesas,rzg2l-usbphyctrl";
+        reg = <0x11c40000 0x10000>;
+        #phy-cells = <1>;
+    };
-- 
2.17.1

Re: [PATCH 1/6] dt-bindings: phy: renesas: Document RZ/G2L USB PHY Control bindings

[Vinod Koul]

On 11-06-21, 14:46, Biju Das wrote:
> Add device tree binding document for RZ/G2L USB PHY control driver.

Rob ?

> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../phy/renesas,rzg2l-usbphyctrl.yaml         | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml b/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
> new file mode 100644
> index 000000000000..5fd316a2e79e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/renesas,rzg2l-usbphyctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas RZ/G2L USB2.0 PHY Control
> +
> +maintainers:
> +  - Biju Das <biju.das.jz@bp.renesas.com>
> +
> +description:
> +  The RZ/G2L USB2.0 PHY Control mainly controls reset and power down of the
> +  USB/PHY.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - renesas,r9a07g044-usbphyctrl # RZ/G2{L,LC}
> +      - const: renesas,rzg2l-usbphyctrl
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#phy-cells':
> +    # see phy-bindings.txt in the same directory
> +    const: 1
> +    description: |
> +      The phandle's argument in the PHY specifier is the phy reset control bit
> +      of usb phy control.
> +      0 = Port 1 Phy reset
> +      1 = Port 2 Phy reset
> +    enum: [ 0, 1 ]
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#phy-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    usbphyctrl@11c40000 {
> +        compatible = "renesas,r9a07g044-usbphyctrl",
> +                     "renesas,rzg2l-usbphyctrl";
> +        reg = <0x11c40000 0x10000>;
> +        #phy-cells = <1>;
> +    };
> -- 
> 2.17.1

-- 
~Vinod

RE: [PATCH 1/6] dt-bindings: phy: renesas: Document RZ/G2L USB PHY Control bindings

[Biju Das]

Hi All,

As per the clock list document, I need to update the bindings.

I will send V2 this changes.

Regards,
Biju

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: 21 June 2021 05:02
> To: Biju Das <biju.das.jz@bp.renesas.com>; Rob Herring
> <robh+dt@kernel.org>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>; linux-phy@lists.infradead.org;
> devicetree@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>;
> Chris Paterson <Chris.Paterson2@renesas.com>; Biju Das
> <biju.das@bp.renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org
> Subject: Re: [PATCH 1/6] dt-bindings: phy: renesas: Document RZ/G2L USB
> PHY Control bindings
> 
> On 11-06-21, 14:46, Biju Das wrote:
> > Add device tree binding document for RZ/G2L USB PHY control driver.
> 
> Rob ?
> 
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../phy/renesas,rzg2l-usbphyctrl.yaml         | 50 +++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
> > b/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.yaml
> > new file mode 100644
> > index 000000000000..5fd316a2e79e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/renesas,rzg2l-usbphyctrl.y
> > +++ aml
> > @@ -0,0 +1,50 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fschemas%2Fphy%2Frenesas%2Crzg2l-usbphyctrl.yaml%23&amp;d
> > +ata=04%7C01%7Cbiju.das.jz%40bp.renesas.com%7C28c0b4be786244eaef1008d9
> > +346949a2%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C637598449085051
> > +587%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBT
> > +iI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WtOl%2BWHTYN4TuB21rTKdJjdx
> > +bKLK96N7NkiSiRT65U0%3D&amp;reserved=0
> > +$schema:
> > +https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=04%7C01%7Cbiju.das.
> > +jz%40bp.renesas.com%7C28c0b4be786244eaef1008d9346949a2%7C53d82571da19
> > +47e49cb4625a166a4a2a%7C0%7C0%7C637598449085051587%7CUnknown%7CTWFpbGZ
> > +sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> > +3D%7C1000&amp;sdata=Y%2FVo3raj5AqUIv%2BaN0Vavdph6cHdWf1a7Imq6chC2KQ%3
> > +D&amp;reserved=0
> > +
> > +title: Renesas RZ/G2L USB2.0 PHY Control
> > +
> > +maintainers:
> > +  - Biju Das <biju.das.jz@bp.renesas.com>
> > +
> > +description:
> > +  The RZ/G2L USB2.0 PHY Control mainly controls reset and power down
> > +of the
> > +  USB/PHY.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - renesas,r9a07g044-usbphyctrl # RZ/G2{L,LC}
> > +      - const: renesas,rzg2l-usbphyctrl
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#phy-cells':
> > +    # see phy-bindings.txt in the same directory
> > +    const: 1
> > +    description: |
> > +      The phandle's argument in the PHY specifier is the phy reset
> control bit
> > +      of usb phy control.
> > +      0 = Port 1 Phy reset
> > +      1 = Port 2 Phy reset
> > +    enum: [ 0, 1 ]
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#phy-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    usbphyctrl@11c40000 {
> > +        compatible = "renesas,r9a07g044-usbphyctrl",
> > +                     "renesas,rzg2l-usbphyctrl";
> > +        reg = <0x11c40000 0x10000>;
> > +        #phy-cells = <1>;
> > +    };
> > --
> > 2.17.1
> 
> --
> ~Vinod

[PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Biju Das]

Add clock entries for USB{0,1}.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/clk/renesas/r9a07g044-cpg.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clk/renesas/r9a07g044-cpg.c b/drivers/clk/renesas/r9a07g044-cpg.c
index 04123908511c..2d2bc78b84a2 100644
--- a/drivers/clk/renesas/r9a07g044-cpg.c
+++ b/drivers/clk/renesas/r9a07g044-cpg.c
@@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
 	DEF_MOD("dmac",		R9A07G044_CLK_DMAC,
 				R9A07G044_CLK_P1,
 				0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
+	DEF_MOD("usb0",		R9A07G044_CLK_USB0,
+				R9A07G044_CLK_P1,
+				0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
+	DEF_MOD("usb1",		R9A07G044_CLK_USB1,
+				R9A07G044_CLK_P1,
+				0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
 	DEF_MOD("scif0",	R9A07G044_CLK_SCIF0,
 				R9A07G044_CLK_P0,
 				0x584, BIT(0), BIT(0)),
-- 
2.17.1

Re: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Geert Uytterhoeven]

Hi Biju,

On Fri, Jun 11, 2021 at 3:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add clock entries for USB{0,1}.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a07g044-cpg.c
> +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
>         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
>                                 R9A07G044_CLK_P1,
>                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
> +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> +                               R9A07G044_CLK_P1,
> +                               0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> +                               R9A07G044_CLK_P1,
> +                               0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
>         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
>                                 R9A07G044_CLK_P0,
>                                 0x584, BIT(0), BIT(0)),

While the above matches the datasheet, I see a problem with the
implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared by
the two USB2.0 channels, disabling USB_PCLK or asserting USB_PRESETN
will affect both channels.  So it looks like you need special handling
to make sure that doesn't happen while the other channel is in use.

Or am I missing something?

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 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Geert Uytterhoeven]

Hi Biju,

On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Jun 11, 2021 at 3:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Add clock entries for USB{0,1}.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> >                                 R9A07G044_CLK_P1,
> >                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
> > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > +                               R9A07G044_CLK_P1,
> > +                               0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > +                               R9A07G044_CLK_P1,
> > +                               0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
> >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> >                                 R9A07G044_CLK_P0,
> >                                 0x584, BIT(0), BIT(0)),
>
> While the above matches the datasheet, I see a problem with the
> implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared by
> the two USB2.0 channels, disabling USB_PCLK or asserting USB_PRESETN
> will affect both channels.  So it looks like you need special handling
> to make sure that doesn't happen while the other channel is in use.
>
> Or am I missing something?

I'm getting the impression we do have to model the individual bits
as separate clocks (and resets).  That would solve the problem with
the shared USB_PCLK, as the clock framework will take care of keeping
it enabled when at least one channel is in use.

Besides USB, SDHI has 4 clock bits, which we definitely don't want
to control together, as the card detect clock must not be stopped
while suspended.
However, the exception to the rule is Ethernet: each channel has
2 clocks, but only a single bit to control, so this needs a custom
single-gate-for-dual-clock driver.

Perhaps merging the clock binding definitions and initial driver for
v5.14 was a bit premature...
Anyway, we'll have 6 rcs after v5.14-rc1 to get it right ;-)

What do you think?

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 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Laurent Pinchart]

Hi Geert,

On Tue, Jun 15, 2021 at 10:58:57AM +0200, Geert Uytterhoeven wrote:
> On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Jun 11, 2021 at 3:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Add clock entries for USB{0,1}.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> > >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> > >                                 R9A07G044_CLK_P1,
> > >                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
> > > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > > +                               R9A07G044_CLK_P1,
> > > +                               0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> > > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > > +                               R9A07G044_CLK_P1,
> > > +                               0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
> > >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> > >                                 R9A07G044_CLK_P0,
> > >                                 0x584, BIT(0), BIT(0)),
> >
> > While the above matches the datasheet, I see a problem with the
> > implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared by
> > the two USB2.0 channels, disabling USB_PCLK or asserting USB_PRESETN
> > will affect both channels.  So it looks like you need special handling
> > to make sure that doesn't happen while the other channel is in use.
> >
> > Or am I missing something?
> 
> I'm getting the impression we do have to model the individual bits
> as separate clocks (and resets).  That would solve the problem with
> the shared USB_PCLK, as the clock framework will take care of keeping
> it enabled when at least one channel is in use.
> 
> Besides USB, SDHI has 4 clock bits, which we definitely don't want
> to control together, as the card detect clock must not be stopped
> while suspended.
> However, the exception to the rule is Ethernet: each channel has
> 2 clocks, but only a single bit to control, so this needs a custom
> single-gate-for-dual-clock driver.

Does it ? Can't the same clock be referenced twice in DT ?

> Perhaps merging the clock binding definitions and initial driver for
> v5.14 was a bit premature...
> Anyway, we'll have 6 rcs after v5.14-rc1 to get it right ;-)
> 
> What do you think?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Geert Uytterhoeven]

Hi Laurent,

On Tue, Jun 15, 2021 at 11:49 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, Jun 15, 2021 at 10:58:57AM +0200, Geert Uytterhoeven wrote:
> > On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Fri, Jun 11, 2021 at 3:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Add clock entries for USB{0,1}.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> > > >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> > > >                                 R9A07G044_CLK_P1,
> > > >                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
> > > > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > > > +                               R9A07G044_CLK_P1,
> > > > +                               0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> > > > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > > > +                               R9A07G044_CLK_P1,
> > > > +                               0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
> > > >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> > > >                                 R9A07G044_CLK_P0,
> > > >                                 0x584, BIT(0), BIT(0)),
> > >
> > > While the above matches the datasheet, I see a problem with the
> > > implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared by
> > > the two USB2.0 channels, disabling USB_PCLK or asserting USB_PRESETN
> > > will affect both channels.  So it looks like you need special handling
> > > to make sure that doesn't happen while the other channel is in use.
> > >
> > > Or am I missing something?
> >
> > I'm getting the impression we do have to model the individual bits
> > as separate clocks (and resets).  That would solve the problem with
> > the shared USB_PCLK, as the clock framework will take care of keeping
> > it enabled when at least one channel is in use.
> >
> > Besides USB, SDHI has 4 clock bits, which we definitely don't want
> > to control together, as the card detect clock must not be stopped
> > while suspended.
> > However, the exception to the rule is Ethernet: each channel has
> > 2 clocks, but only a single bit to control, so this needs a custom
> > single-gate-for-dual-clock driver.
>
> Does it ? Can't the same clock be referenced twice in DT ?

Yes, you can reference the same clock twice. But what's the point?
If they're two different clocks, DT should reference two different
clocks.  But the single bit should correspond to the ORed value of
the two clock enable states.

Or do you mean something different?

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 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Laurent Pinchart]

On Tue, Jun 15, 2021 at 11:53:37AM +0200, Geert Uytterhoeven wrote:
> Hi Laurent,
> 
> On Tue, Jun 15, 2021 at 11:49 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Tue, Jun 15, 2021 at 10:58:57AM +0200, Geert Uytterhoeven wrote:
> > > On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Fri, Jun 11, 2021 at 3:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > Add clock entries for USB{0,1}.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > > > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> > > > >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> > > > >                                 R9A07G044_CLK_P1,
> > > > >                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
> > > > > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > > > > +                               R9A07G044_CLK_P1,
> > > > > +                               0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> > > > > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > > > > +                               R9A07G044_CLK_P1,
> > > > > +                               0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
> > > > >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> > > > >                                 R9A07G044_CLK_P0,
> > > > >                                 0x584, BIT(0), BIT(0)),
> > > >
> > > > While the above matches the datasheet, I see a problem with the
> > > > implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared by
> > > > the two USB2.0 channels, disabling USB_PCLK or asserting USB_PRESETN
> > > > will affect both channels.  So it looks like you need special handling
> > > > to make sure that doesn't happen while the other channel is in use.
> > > >
> > > > Or am I missing something?
> > >
> > > I'm getting the impression we do have to model the individual bits
> > > as separate clocks (and resets).  That would solve the problem with
> > > the shared USB_PCLK, as the clock framework will take care of keeping
> > > it enabled when at least one channel is in use.
> > >
> > > Besides USB, SDHI has 4 clock bits, which we definitely don't want
> > > to control together, as the card detect clock must not be stopped
> > > while suspended.
> > > However, the exception to the rule is Ethernet: each channel has
> > > 2 clocks, but only a single bit to control, so this needs a custom
> > > single-gate-for-dual-clock driver.
> >
> > Does it ? Can't the same clock be referenced twice in DT ?
> 
> Yes, you can reference the same clock twice. But what's the point?
> If they're two different clocks, DT should reference two different
> clocks.  But the single bit should correspond to the ORed value of
> the two clock enable states.
> 
> Or do you mean something different?

If the device has two clock inputs, I'd model the two clocks separately
in the DT bindings. If those two clocks are gated by the same bit in an
SoC, we have two options to model the integration:

- Create a driver that registers different clocks with the same gating
  bit. We'll have two clocks to reference in DT.

- Model both clocks as a single clock in the clock driver, and reference
  that clock twice in DT. This is simpler, but only works if the
  consumer doesn't need to query the clock rate.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Geert Uytterhoeven]

Hi Laurent,

On Tue, Jun 15, 2021 at 11:57 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, Jun 15, 2021 at 11:53:37AM +0200, Geert Uytterhoeven wrote:
> > On Tue, Jun 15, 2021 at 11:49 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > > On Tue, Jun 15, 2021 at 10:58:57AM +0200, Geert Uytterhoeven wrote:
> > > > On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Fri, Jun 11, 2021 at 3:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > Add clock entries for USB{0,1}.
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > > > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > > > > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> > > > > >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> > > > > >                                 R9A07G044_CLK_P1,
> > > > > >                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
> > > > > > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > > > > > +                               R9A07G044_CLK_P1,
> > > > > > +                               0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> > > > > > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > > > > > +                               R9A07G044_CLK_P1,
> > > > > > +                               0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
> > > > > >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> > > > > >                                 R9A07G044_CLK_P0,
> > > > > >                                 0x584, BIT(0), BIT(0)),
> > > > >
> > > > > While the above matches the datasheet, I see a problem with the
> > > > > implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared by
> > > > > the two USB2.0 channels, disabling USB_PCLK or asserting USB_PRESETN
> > > > > will affect both channels.  So it looks like you need special handling
> > > > > to make sure that doesn't happen while the other channel is in use.
> > > > >
> > > > > Or am I missing something?
> > > >
> > > > I'm getting the impression we do have to model the individual bits
> > > > as separate clocks (and resets).  That would solve the problem with
> > > > the shared USB_PCLK, as the clock framework will take care of keeping
> > > > it enabled when at least one channel is in use.
> > > >
> > > > Besides USB, SDHI has 4 clock bits, which we definitely don't want
> > > > to control together, as the card detect clock must not be stopped
> > > > while suspended.
> > > > However, the exception to the rule is Ethernet: each channel has
> > > > 2 clocks, but only a single bit to control, so this needs a custom
> > > > single-gate-for-dual-clock driver.
> > >
> > > Does it ? Can't the same clock be referenced twice in DT ?
> >
> > Yes, you can reference the same clock twice. But what's the point?
> > If they're two different clocks, DT should reference two different
> > clocks.  But the single bit should correspond to the ORed value of
> > the two clock enable states.
> >
> > Or do you mean something different?
>
> If the device has two clock inputs, I'd model the two clocks separately
> in the DT bindings. If those two clocks are gated by the same bit in an
> SoC, we have two options to model the integration:
>
> - Create a driver that registers different clocks with the same gating
>   bit. We'll have two clocks to reference in DT.

OK, that's what I suggested.

> - Model both clocks as a single clock in the clock driver, and reference
>   that clock twice in DT. This is simpler, but only works if the
>   consumer doesn't need to query the clock rate.

Modelling them as a single clock is how the current RZ/G2L clock
driver would implement it. But why bother referencing it twice in DT?
renesas,ether*.yaml (assuming the Ethernet block is compatible)
documents a single clock only (ignoring optional refclk), and the driver
doesn't care about the clock rate.

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 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Biju Das]

Hi Geert and Laurent,

Thanks for the feedback.

I have gone through the below discussion and agreeing for two clock entries in dt bindings
and implement a gate for controlling this clocks.

Regards,
Biju

> Subject: Re: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB
> clocks
> 
> Hi Laurent,
> 
> On Tue, Jun 15, 2021 at 11:57 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Tue, Jun 15, 2021 at 11:53:37AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Jun 15, 2021 at 11:49 AM Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com> wrote:
> > > > On Tue, Jun 15, 2021 at 10:58:57AM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven <geert@linux-
> m68k.org> wrote:
> > > > > > On Fri, Jun 11, 2021 at 3:46 PM Biju Das
> <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > Add clock entries for USB{0,1}.
> > > > > > >
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > Reviewed-by: Lad Prabhakar
> > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > >
> > > > > > Thanks for your patch!
> > > > > >
> > > > > > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > > > > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > > > > > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk
> r9a07g044_mod_clks[] = {
> > > > > > >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> > > > > > >                                 R9A07G044_CLK_P1,
> > > > > > >                                 0x52c, (BIT(0) | BIT(1)),
> > > > > > > (BIT(0) | BIT(1))),
> > > > > > > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > > > > > > +                               R9A07G044_CLK_P1,
> > > > > > > +                               0x578, (BIT(0) | BIT(2) |
> BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> > > > > > > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > > > > > > +                               R9A07G044_CLK_P1,
> > > > > > > +                               0x578, (BIT(1) | BIT(3)),
> > > > > > > + (BIT(1) | BIT(3))),
> > > > > > >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> > > > > > >                                 R9A07G044_CLK_P0,
> > > > > > >                                 0x584, BIT(0), BIT(0)),
> > > > > >
> > > > > > While the above matches the datasheet, I see a problem with
> > > > > > the implementation. As BIT(3) of the
> > > > > > CPG_{CLKON,CLKMON,RST}_USB is shared by the two USB2.0
> > > > > > channels, disabling USB_PCLK or asserting USB_PRESETN will
> > > > > > affect both channels.  So it looks like you need special
> handling to make sure that doesn't happen while the other channel is in
> use.
> > > > > >
> > > > > > Or am I missing something?
> > > > >
> > > > > I'm getting the impression we do have to model the individual
> > > > > bits as separate clocks (and resets).  That would solve the
> > > > > problem with the shared USB_PCLK, as the clock framework will
> > > > > take care of keeping it enabled when at least one channel is in
> use.
> > > > >
> > > > > Besides USB, SDHI has 4 clock bits, which we definitely don't
> > > > > want to control together, as the card detect clock must not be
> > > > > stopped while suspended.
> > > > > However, the exception to the rule is Ethernet: each channel has
> > > > > 2 clocks, but only a single bit to control, so this needs a
> > > > > custom single-gate-for-dual-clock driver.
> > > >
> > > > Does it ? Can't the same clock be referenced twice in DT ?
> > >
> > > Yes, you can reference the same clock twice. But what's the point?
> > > If they're two different clocks, DT should reference two different
> > > clocks.  But the single bit should correspond to the ORed value of
> > > the two clock enable states.
> > >
> > > Or do you mean something different?
> >
> > If the device has two clock inputs, I'd model the two clocks
> > separately in the DT bindings. If those two clocks are gated by the
> > same bit in an SoC, we have two options to model the integration:
> >
> > - Create a driver that registers different clocks with the same gating
> >   bit. We'll have two clocks to reference in DT.
> 
> OK, that's what I suggested.
> 
> > - Model both clocks as a single clock in the clock driver, and reference
> >   that clock twice in DT. This is simpler, but only works if the
> >   consumer doesn't need to query the clock rate.
> 
> Modelling them as a single clock is how the current RZ/G2L clock driver
> would implement it. But why bother referencing it twice in DT?
> renesas,ether*.yaml (assuming the Ethernet block is compatible) documents
> a single clock only (ignoring optional refclk), and the driver doesn't
> care about the clock rate.
> 
> 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 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Laurent Pinchart]

Hi Geert,

On Tue, Jun 15, 2021 at 12:24:52PM +0200, Geert Uytterhoeven wrote:
> On Tue, Jun 15, 2021 at 11:57 AM Laurent Pinchart wrote:
> > On Tue, Jun 15, 2021 at 11:53:37AM +0200, Geert Uytterhoeven wrote:
> > > On Tue, Jun 15, 2021 at 11:49 AM Laurent Pinchart wrote:
> > > > On Tue, Jun 15, 2021 at 10:58:57AM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven wrote:
> > > > > > On Fri, Jun 11, 2021 at 3:46 PM Biju Das wrote:
> > > > > > > Add clock entries for USB{0,1}.
> > > > > > >
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > >
> > > > > > Thanks for your patch!
> > > > > >
> > > > > > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > > > > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > > > > > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {
> > > > > > >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> > > > > > >                                 R9A07G044_CLK_P1,
> > > > > > >                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) | BIT(1))),
> > > > > > > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > > > > > > +                               R9A07G044_CLK_P1,
> > > > > > > +                               0x578, (BIT(0) | BIT(2) | BIT(3)), (BIT(0) | BIT(2) | BIT(3))),
> > > > > > > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > > > > > > +                               R9A07G044_CLK_P1,
> > > > > > > +                               0x578, (BIT(1) | BIT(3)), (BIT(1) | BIT(3))),
> > > > > > >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> > > > > > >                                 R9A07G044_CLK_P0,
> > > > > > >                                 0x584, BIT(0), BIT(0)),
> > > > > >
> > > > > > While the above matches the datasheet, I see a problem with the
> > > > > > implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared by
> > > > > > the two USB2.0 channels, disabling USB_PCLK or asserting USB_PRESETN
> > > > > > will affect both channels.  So it looks like you need special handling
> > > > > > to make sure that doesn't happen while the other channel is in use.
> > > > > >
> > > > > > Or am I missing something?
> > > > >
> > > > > I'm getting the impression we do have to model the individual bits
> > > > > as separate clocks (and resets).  That would solve the problem with
> > > > > the shared USB_PCLK, as the clock framework will take care of keeping
> > > > > it enabled when at least one channel is in use.
> > > > >
> > > > > Besides USB, SDHI has 4 clock bits, which we definitely don't want
> > > > > to control together, as the card detect clock must not be stopped
> > > > > while suspended.
> > > > > However, the exception to the rule is Ethernet: each channel has
> > > > > 2 clocks, but only a single bit to control, so this needs a custom
> > > > > single-gate-for-dual-clock driver.
> > > >
> > > > Does it ? Can't the same clock be referenced twice in DT ?
> > >
> > > Yes, you can reference the same clock twice. But what's the point?
> > > If they're two different clocks, DT should reference two different
> > > clocks.  But the single bit should correspond to the ORed value of
> > > the two clock enable states.
> > >
> > > Or do you mean something different?
> >
> > If the device has two clock inputs, I'd model the two clocks separately
> > in the DT bindings. If those two clocks are gated by the same bit in an
> > SoC, we have two options to model the integration:
> >
> > - Create a driver that registers different clocks with the same gating
> >   bit. We'll have two clocks to reference in DT.
> 
> OK, that's what I suggested.
> 
> > - Model both clocks as a single clock in the clock driver, and reference
> >   that clock twice in DT. This is simpler, but only works if the
> >   consumer doesn't need to query the clock rate.
> 
> Modelling them as a single clock is how the current RZ/G2L clock
> driver would implement it. But why bother referencing it twice in DT?
> renesas,ether*.yaml (assuming the Ethernet block is compatible)
> documents a single clock only (ignoring optional refclk), and the driver
> doesn't care about the clock rate.

The idea here is that if the device has two clock inputs, its driver
could handle clocks without knowing that in some SoCs they are handled
by a single gate (or rather two gates with the same control signal).

-- 
Regards,

Laurent Pinchart

RE: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB clocks

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/6] drivers: clk: renesas: r9a07g044-cpg: Add USB
> clocks
> 
> Hi Biju,
> 
> On Mon, Jun 14, 2021 at 2:26 PM Geert Uytterhoeven <geert@linux-m68k.org>
> wrote:
> > On Fri, Jun 11, 2021 at 3:46 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Add clock entries for USB{0,1}.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/clk/renesas/r9a07g044-cpg.c
> > > +++ b/drivers/clk/renesas/r9a07g044-cpg.c
> > > @@ -88,6 +88,12 @@ static struct rzg2l_mod_clk r9a07g044_mod_clks[] =
> {
> > >         DEF_MOD("dmac",         R9A07G044_CLK_DMAC,
> > >                                 R9A07G044_CLK_P1,
> > >                                 0x52c, (BIT(0) | BIT(1)), (BIT(0) |
> > > BIT(1))),
> > > +       DEF_MOD("usb0",         R9A07G044_CLK_USB0,
> > > +                               R9A07G044_CLK_P1,
> > > +                               0x578, (BIT(0) | BIT(2) | BIT(3)),
> (BIT(0) | BIT(2) | BIT(3))),
> > > +       DEF_MOD("usb1",         R9A07G044_CLK_USB1,
> > > +                               R9A07G044_CLK_P1,
> > > +                               0x578, (BIT(1) | BIT(3)), (BIT(1) |
> > > + BIT(3))),
> > >         DEF_MOD("scif0",        R9A07G044_CLK_SCIF0,
> > >                                 R9A07G044_CLK_P0,
> > >                                 0x584, BIT(0), BIT(0)),
> >
> > While the above matches the datasheet, I see a problem with the
> > implementation. As BIT(3) of the CPG_{CLKON,CLKMON,RST}_USB is shared
> > by the two USB2.0 channels, disabling USB_PCLK or asserting
> > USB_PRESETN will affect both channels.  So it looks like you need
> > special handling to make sure that doesn't happen while the other
> channel is in use.
> >
> > Or am I missing something?
> 
> I'm getting the impression we do have to model the individual bits as
> separate clocks (and resets).  That would solve the problem with the
> shared USB_PCLK, as the clock framework will take care of keeping it
> enabled when at least one channel is in use.
> 
> Besides USB, SDHI has 4 clock bits, which we definitely don't want to
> control together, as the card detect clock must not be stopped while
> suspended.
> However, the exception to the rule is Ethernet: each channel has
> 2 clocks, but only a single bit to control, so this needs a custom single-
> gate-for-dual-clock driver.
> 
> Perhaps merging the clock binding definitions and initial driver for
> v5.14 was a bit premature...
> Anyway, we'll have 6 rcs after v5.14-rc1 to get it right ;-)
> 
> What do you think?

I am ok with this approach for USB, SDHI and Ethernet.

Regards,
Biju

[PATCH 3/6] phy: renesas: Add RZ/G2L usb phy control driver

[Biju Das]

Add support for RZ/G2L USB PHY Control driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/phy/renesas/Kconfig                |   7 +
 drivers/phy/renesas/Makefile               |   1 +
 drivers/phy/renesas/phy-rzg2l-usbphyctrl.c | 163 +++++++++++++++++++++
 3 files changed, 171 insertions(+)
 create mode 100644 drivers/phy/renesas/phy-rzg2l-usbphyctrl.c

diff --git a/drivers/phy/renesas/Kconfig b/drivers/phy/renesas/Kconfig
index 111bdcae775c..2184fba12255 100644
--- a/drivers/phy/renesas/Kconfig
+++ b/drivers/phy/renesas/Kconfig
@@ -32,3 +32,10 @@ config PHY_RCAR_GEN3_USB3
 	select GENERIC_PHY
 	help
 	  Support for USB 3.0 PHY found on Renesas R-Car generation 3 SoCs.
+
+config PHY_RZG2L_USBPHYCTRL
+	tristate "Renesas RZ/G2L USB PHY control driver"
+	depends on ARCH_RENESAS || COMPILE_TEST
+	select GENERIC_PHY
+	help
+	  Support for USB PHY Control found on RZ/G2L SoCs.
diff --git a/drivers/phy/renesas/Makefile b/drivers/phy/renesas/Makefile
index b599ff8a4349..62acc6bde5cb 100644
--- a/drivers/phy/renesas/Makefile
+++ b/drivers/phy/renesas/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_PHY_RCAR_GEN2)		+= phy-rcar-gen2.o
 obj-$(CONFIG_PHY_RCAR_GEN3_PCIE)	+= phy-rcar-gen3-pcie.o
 obj-$(CONFIG_PHY_RCAR_GEN3_USB2)	+= phy-rcar-gen3-usb2.o
 obj-$(CONFIG_PHY_RCAR_GEN3_USB3)	+= phy-rcar-gen3-usb3.o
+obj-$(CONFIG_PHY_RZG2L_USBPHYCTRL)	+= phy-rzg2l-usbphyctrl.o
diff --git a/drivers/phy/renesas/phy-rzg2l-usbphyctrl.c b/drivers/phy/renesas/phy-rzg2l-usbphyctrl.c
new file mode 100644
index 000000000000..7176f18b28d2
--- /dev/null
+++ b/drivers/phy/renesas/phy-rzg2l-usbphyctrl.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L USB PHY control driver
+ *
+ * Copyright (C) 2021 Renesas Electronics Corporation
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+
+#define RESET			0x000
+
+#define SEL_PLLRESET		BIT(12)
+#define PLL_RESET		BIT(8)
+
+#define PHY_RESET_PORT2		(BIT(1) | BIT(5))
+#define PHY_RESET_PORT1		(BIT(0) | BIT(4))
+
+#define NUM_PORTS		2
+
+struct rzg2l_usbphycontrol_drv;
+
+struct rzg2l_phyctrl {
+	struct phy *phy;
+	struct rzg2l_usbphycontrol_drv *drv;
+	u32 phy_reset_port_mask;
+};
+
+struct rzg2l_usbphycontrol_drv {
+	void __iomem *base;
+	struct rzg2l_phyctrl phyctrl[NUM_PORTS];
+};
+
+static int rzg2l_usbphyctrl_init(struct phy *p)
+{
+	struct rzg2l_phyctrl *r = phy_get_drvdata(p);
+	struct rzg2l_usbphycontrol_drv *drv = r->drv;
+	void __iomem *base = drv->base;
+	u32 val = readl(base + RESET);
+
+	val |= SEL_PLLRESET;
+	val &= ~(PLL_RESET | r->phy_reset_port_mask);
+	writel(val, base + RESET);
+
+	return 0;
+}
+
+static int rzg2l_usbphyctrl_exit(struct phy *p)
+{
+	struct rzg2l_phyctrl *r = phy_get_drvdata(p);
+	struct rzg2l_usbphycontrol_drv *drv = r->drv;
+	void __iomem *base = drv->base;
+	u32 val = readl(base + RESET);
+
+	val |= r->phy_reset_port_mask;
+	if ((val & 0xFF) == (PHY_RESET_PORT1 | PHY_RESET_PORT2))
+		val |= PLL_RESET;
+	writel(val, base + RESET);
+	return 0;
+}
+
+static const struct phy_ops rzg2l_usbphyctrl_ops = {
+	.init		= rzg2l_usbphyctrl_init,
+	.exit		= rzg2l_usbphyctrl_exit,
+	.owner		= THIS_MODULE,
+};
+
+static const struct of_device_id rzg2l_usbphyctrl_match_table[] = {
+	{ .compatible = "renesas,rzg2l-usbphyctrl" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzg2l_usbphyctrl_match_table);
+
+static struct phy *rzg2l_usbphycontrol_xlate(struct device *dev,
+					     struct of_phandle_args *args)
+{
+	struct rzg2l_usbphycontrol_drv *drv;
+	struct rzg2l_phyctrl *r;
+
+	drv = dev_get_drvdata(dev);
+	if (!drv)
+		return ERR_PTR(-EINVAL);
+
+	if (args->args[0] >= NUM_PORTS)
+		return ERR_PTR(-ENODEV);
+
+	r = &drv->phyctrl[args->args[0]];
+
+	return r->phy;
+}
+
+static int rzg2l_usbphycontrol_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rzg2l_usbphycontrol_drv *drv;
+	struct phy_provider *provider;
+	u32 val;
+	int n;
+
+	if (!dev->of_node) {
+		dev_err(dev, "device tree not found\n");
+		return -EINVAL;
+	}
+
+	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	drv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(drv->base))
+		return PTR_ERR(drv->base);
+
+	for (n = 0; n < NUM_PORTS; n++) {
+		struct rzg2l_phyctrl *phy = &drv->phyctrl[n];
+
+		phy->phy = devm_phy_create(dev, NULL, &rzg2l_usbphyctrl_ops);
+		if (IS_ERR(phy->phy)) {
+			dev_err(dev, "Failed to create USBPHY Control\n");
+			return PTR_ERR(phy->phy);
+		}
+
+		if (n == 1)
+			phy->phy_reset_port_mask = PHY_RESET_PORT2;
+		else
+			phy->phy_reset_port_mask = PHY_RESET_PORT1;
+
+		phy->drv = drv;
+		phy_set_drvdata(phy->phy, phy);
+	};
+
+	provider = devm_of_phy_provider_register(dev,
+						 rzg2l_usbphycontrol_xlate);
+	if (IS_ERR(provider)) {
+		dev_err(dev, "Failed to register PHY provider\n");
+		return PTR_ERR(provider);
+	}
+
+	dev_set_drvdata(dev, drv);
+
+	/* put pll and phy into reset state */
+	val = readl(drv->base + RESET);
+	val |= SEL_PLLRESET | PLL_RESET | PHY_RESET_PORT2 | PHY_RESET_PORT1;
+	writel(val, drv->base + RESET);
+
+	return 0;
+}
+
+static struct platform_driver rzg2l_usbphyctrl_driver = {
+	.driver = {
+		.name		= "rzg2l_usbphyctrl",
+		.of_match_table	= rzg2l_usbphyctrl_match_table,
+	},
+	.probe	= rzg2l_usbphycontrol_probe,
+};
+module_platform_driver(rzg2l_usbphyctrl_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Renesas RZ/G2L USBPHYControl");
+MODULE_AUTHOR("biju.das.jz@bp.renesas.com>");
-- 
2.17.1

Re: [PATCH 3/6] phy: renesas: Add RZ/G2L usb phy control driver

[Vinod Koul]

On 11-06-21, 14:46, Biju Das wrote:
> +static int rzg2l_usbphycontrol_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rzg2l_usbphycontrol_drv *drv;
> +	struct phy_provider *provider;
> +	u32 val;
> +	int n;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "device tree not found\n");
> +		return -EINVAL;
> +	}

why do you think this would happen?

> +
> +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	drv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(drv->base))
> +		return PTR_ERR(drv->base);
> +
> +	for (n = 0; n < NUM_PORTS; n++) {
> +		struct rzg2l_phyctrl *phy = &drv->phyctrl[n];
> +
> +		phy->phy = devm_phy_create(dev, NULL, &rzg2l_usbphyctrl_ops);
> +		if (IS_ERR(phy->phy)) {
> +			dev_err(dev, "Failed to create USBPHY Control\n");
> +			return PTR_ERR(phy->phy);
> +		}
> +
> +		if (n == 1)
> +			phy->phy_reset_port_mask = PHY_RESET_PORT2;

this looks inverted, should this logically not be:
                if (n == 0)
                        phy->phy_reset_port_mask = PHY_RESET_PORT1;
?

> +		else
> +			phy->phy_reset_port_mask = PHY_RESET_PORT1;
> +
> +		phy->drv = drv;
> +		phy_set_drvdata(phy->phy, phy);
> +	};
> +
> +	provider = devm_of_phy_provider_register(dev,
> +						 rzg2l_usbphycontrol_xlate);

single line pls

-- 
~Vinod

RE: [PATCH 3/6] phy: renesas: Add RZ/G2L usb phy control driver

[Biju Das]

Hi Vinod,

Thanks for the feedback.

> Subject: Re: [PATCH 3/6] phy: renesas: Add RZ/G2L usb phy control driver
> 
> On 11-06-21, 14:46, Biju Das wrote:
> > +static int rzg2l_usbphycontrol_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct rzg2l_usbphycontrol_drv *drv;
> > +	struct phy_provider *provider;
> > +	u32 val;
> > +	int n;
> > +
> > +	if (!dev->of_node) {
> > +		dev_err(dev, "device tree not found\n");
> > +		return -EINVAL;
> > +	}
> 
> why do you think this would happen?

Not needed. Will take out.

> 
> > +
> > +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> > +	if (!drv)
> > +		return -ENOMEM;
> > +
> > +	drv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(drv->base))
> > +		return PTR_ERR(drv->base);
> > +
> > +	for (n = 0; n < NUM_PORTS; n++) {
> > +		struct rzg2l_phyctrl *phy = &drv->phyctrl[n];
> > +
> > +		phy->phy = devm_phy_create(dev, NULL, &rzg2l_usbphyctrl_ops);
> > +		if (IS_ERR(phy->phy)) {
> > +			dev_err(dev, "Failed to create USBPHY Control\n");
> > +			return PTR_ERR(phy->phy);
> > +		}
> > +
> > +		if (n == 1)
> > +			phy->phy_reset_port_mask = PHY_RESET_PORT2;
> 
> this looks inverted, should this logically not be:
Agreed. Will change this in v2.

>                 if (n == 0)
>                         phy->phy_reset_port_mask = PHY_RESET_PORT1; ?
> 
> > +		else
> > +			phy->phy_reset_port_mask = PHY_RESET_PORT1;
> > +
> > +		phy->drv = drv;
> > +		phy_set_drvdata(phy->phy, phy);
> > +	};
> > +
> > +	provider = devm_of_phy_provider_register(dev,
> > +						 rzg2l_usbphycontrol_xlate);
> 
> single line pls
OK.

Regards,
Biju

> 
> --
> ~Vinod

[PATCH 4/6] arm64: configs: defconfig: Enable RZ/G2L USB PHY control driver

[Biju Das]

RZ/G2L SMARC EVK supports USB PHY control,so enable it in ARM64 defconfig.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 91eb7e7fa654..b6f755b225af 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1107,6 +1107,7 @@ CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2=y
 CONFIG_PHY_RCAR_GEN3_PCIE=y
 CONFIG_PHY_RCAR_GEN3_USB2=y
 CONFIG_PHY_RCAR_GEN3_USB3=m
+CONFIG_PHY_RZG2L_USBPHYCTRL=y
 CONFIG_PHY_ROCKCHIP_EMMC=y
 CONFIG_PHY_ROCKCHIP_INNO_HDMI=m
 CONFIG_PHY_ROCKCHIP_INNO_USB2=y
-- 
2.17.1

[PATCH 5/6] dt-bindings: phy: renesas,usb2-phy: Document RZ/G2L phy bindings

[Biju Das]

Document USB phy bindings for RZ/G2L SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml b/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
index 0f358d5b84ef..2425295941d1 100644
--- a/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/renesas,usb2-phy.yaml
@@ -28,6 +28,7 @@ properties:
               - renesas,usb2-phy-r8a77965 # R-Car M3-N
               - renesas,usb2-phy-r8a77990 # R-Car E3
               - renesas,usb2-phy-r8a77995 # R-Car D3
+              - renesas,usb2-phy-r9a07g044 # RZ/G2{L,LC}
           - const: renesas,rcar-gen3-usb2-phy
 
   reg:
-- 
2.17.1

[PATCH 6/6] arm64: dts: renesas: r9a07g044: Add USB2.0 phy and host support

[Biju Das]

Add USB2.0 phy and host support to SoC DT.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
This patch depend on [1]
[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210609153230.6967-11-prabhakar.mahadev-lad.rj@bp.renesas.com/
---
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 81 ++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 47f9fafd6c06..2ffdaed6c9a5 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -158,6 +158,87 @@
 			      <0x0 0x11940000 0 0x60000>;
 			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
 		};
+
+		usbphyctrl: usbphyctrl@11c40000 {
+			compatible = "renesas,r9a07g044-usbphyctrl",
+				     "renesas,rzg2l-usbphyctrl";
+			reg = <0 0x11c40000 0 0x10000>;
+			#phy-cells = <1>;
+		};
+
+		ohci0: usb@11c50000 {
+			compatible = "generic-ohci";
+			reg = <0 0x11c50000 0 0x100>;
+			interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_CLK_USB0>;
+			resets = <&cpg R9A07G044_CLK_USB0>;
+			phys = <&usbphyctrl 0>, <&usb2_phy0 1>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		ohci1: usb@11c70000 {
+			compatible = "generic-ohci";
+			reg = <0 0x11c70000 0 0x100>;
+			interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_CLK_USB1>;
+			resets = <&cpg R9A07G044_CLK_USB1>;
+			phys = <&usbphyctrl 1>, <&usb2_phy1 1>;
+			phy-names = "usb";
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		ehci0: usb@11c50100 {
+			compatible = "generic-ehci";
+			reg = <0 0x11c50100 0 0x100>;
+			interrupts = <GIC_SPI 92 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_CLK_USB0>;
+			resets = <&cpg R9A07G044_CLK_USB0>;
+			phys = <&usbphyctrl 0>, <&usb2_phy0 2>;
+			phy-names = "usb";
+			companion = <&ohci0>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		ehci1: usb@11c70100 {
+			compatible = "generic-ehci";
+			reg = <0 0x11c70100 0 0x100>;
+			interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_CLK_USB1>;
+			resets = <&cpg R9A07G044_CLK_USB1>;
+			phys = <&usbphyctrl 1>, <&usb2_phy1 2>;
+			phy-names = "usb";
+			companion = <&ohci1>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		usb2_phy0: usb-phy@11c50200 {
+			compatible = "renesas,usb2-phy-r9a07g044",
+				     "renesas,rcar-gen3-usb2-phy";
+			reg = <0 0x11c50200 0 0x700>;
+			interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_CLK_USB0>;
+			resets = <&cpg R9A07G044_CLK_USB0>;
+			#phy-cells = <1>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
+
+		usb2_phy1: usb-phy@11c70200 {
+			compatible = "renesas,usb2-phy-r9a07g044",
+				     "renesas,rcar-gen3-usb2-phy";
+			reg = <0 0x11c70200 0 0x700>;
+			interrupts = <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_CLK_USB1>;
+			resets = <&cpg R9A07G044_CLK_USB1>;
+			#phy-cells = <1>;
+			power-domains = <&cpg>;
+			status = "disabled";
+		};
 	};
 
 	timer {
-- 
2.17.1