diff mbox series

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

Message ID 20190506234631.113226-2-chris.brandt@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series usb: Add host and device support for RZ/A2 | expand

Commit Message

Chris Brandt May 6, 2019, 11:46 p.m. UTC
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(+)

Comments

Geert Uytterhoeven May 7, 2019, 8:01 a.m. UTC | #1
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
Chris Brandt May 7, 2019, 11 a.m. UTC | #2
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
Geert Uytterhoeven May 7, 2019, 11:26 a.m. UTC | #3
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
Chris Brandt May 7, 2019, 3:43 p.m. UTC | #4
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
Geert Uytterhoeven May 7, 2019, 4:32 p.m. UTC | #5
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
Chris Brandt May 7, 2019, 8:27 p.m. UTC | #6
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
Geert Uytterhoeven May 8, 2019, 6:59 a.m. UTC | #7
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
diff mbox series

Patch

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.