RE: [PATCH v2] phy: rcar-gen3-usb3: add support for R-Car Gen3 USB 3.0 PHY

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven
> Sent: Wednesday, May 24, 2017 9:39 PM
> 
> Hi Shimoda-san,
> 
> On Wed, May 24, 2017 at 2:17 PM, Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > The USB 3.0 PHY modules of R-Car Gen3 SoCs have:
> >  - Spread spectrum clock (ssc).
> >  - Using USB 2.0 EXTAL clock instead of USB 3.0 clock.
> >  - Enabling VBUS detection for usb3.0 peripheral.
> >
> > So, this driver supports these features.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > ---
<snip>
> > diff --git a/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> > new file mode 100644
> > index 0000000..e58ba6b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> > +* Renesas R-Car generation 3 USB 3.0 PHY
> > +
> > +This file provides information on what the device node for the R-Car generation
> > +3 USB 3.0 PHY contains.
> > +If you want to enable spread spectrum clock (ssc), you should use USB_EXTAL
> > +instead of USB3_CLK. However, if you don't want to these features, you don't
> > +need this driver.
> > +
> > +Required properties:
> > +- compatible: "renesas,r8a7795-usb3-phy" if the device is a part of an R8A7795
> > +             SoC.
> > +             "renesas,r8a7796-usb3-phy" if the device is a part of an R8A7796
> > +             SoC.
> > +             "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 compatible
> > +             device.
> > +
> > +             When compatible with the generic version, nodes must list the
> > +             SoC-specific version corresponding to the platform first
> > +             followed by the generic version.
> > +
> > +- reg: offset and length of the USB 3.0 PHY register block.
> > +- clocks: A list of phandles and clock-specifier pairs.
> > +- clock-names: Name of the clocks.
> > +  - The funcional clock must be "usb3-if".
> > +  - The usb3's external clock must be "usb3s_clk". If you want to use the ssc,
> > +    the clock-frequency must be 0.
> 
> Given you have "renesas,ssc-range" to enable ssc, I think it doesn't matter
> if the clock frequency is 0 or not, so the "if you..." part can be removed.

I see. If both usb3s_clk and usb_extal are not 0 and "ssc-range" is enabled,
the driver should choose usb_extal and enable the ssc.
So, I understand "if you.." part can be removed. Thanks!

> > +  - The usb2's external clock must be "usb_extal". If you want to use the ssc,
> > +    the clock-frequenvy must not be 0.
> 
> frequency

Oops, I will fix it.

> Here it does matter, as you need this clock for ssc.
> BTW, the driver can fallback to the other clock if ssc cannot be used.
> 
> > +Optional properties:
> > +- renesas,ssc-range: Enable/disable spread spectrum clock (ssc) by using
> > +                    the following values as u32:
> > +                       - 0 (or the property doesn't exist): disable the ssc
> > +                       - 4980: enable the ssc as -4980 ppm
> > +                       - 4492: enable the ssc as -4492 ppm
> > +                       - 4003: enable the ssc as -4003 ppm
> 
> Using ssc or not sounds like software policy, not hardware description?
> Can this be decided at runtime?

I'm thinking this is hardware description :)
This setting needs before usb3.0 controller starts.
So, this cannot be decided at runtime.

I found phy/brcm-sata-phy.txt also had a similar property "brcm,enable-ssc".

> > --- /dev/null
> > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb3.c
> 
> > +static const struct of_device_id rcar_gen3_phy_usb3_match_table[] = {
> > +       { .compatible = "renesas,r8a7795-usb3-phy" },
> > +       { .compatible = "renesas,r8a7796-usb3-phy" },
> 
> As the driver matches against the generic compatible property, the above two
> lines can be removed.

I got it. I will remove it.

Best regards,
Yoshihiro Shimoda

> > +       { .compatible = "renesas,rcar-gen3-usb3-phy" },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, rcar_gen3_phy_usb3_match_table);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> 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