RE: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Petre-san,

> From: Petre Pircalabu
> Sent: Friday, May 19, 2017 8:55 AM
> 
> Hi Shimoda-san,
> 
> I just saw that you patch predates mine. I will drop my version and
> integrate yours.

I got it!

Best regards,
Yoshihiro Shimoda

> Many thanks for for your support,
> Petre

RE: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven
> Sent: Thursday, May 18, 2017 9:43 PM
> 
> Hi Shimoda-san,
> 
> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
>  wrote:
> >> From: Geert Uytterhoeven
> >> Sent: Thursday, May 18, 2017 8:41 PM
> >> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
> >>  wrote:
> >> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register 
> >> >> >> block.
> >> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be 
> >> >> >> <0>.
> >> >> >
> >> >> > I think we should add "clocks" property as required.
> >> >> >
> >> >> >> +Optional properties:
> >> >> >
> >> >> > You should add "renesas,use-on-chip-clk" here.
> >> >> > And, the name of "use-on-chip-clk" is not good to me.
> >> >> > FYI, my developing patch names "renesas,usb-extal".
> >
> > I'm sorry for this. I missed description "on-chip clock source is supplied 
> > though
> > USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is 
> > reasonable.
> >
> >> >> Can this be decided at runtime, e.g. by looking at the rates of the 
> >> >> clocks
> >> >> to see which one is available/best suited?
> >> >
> >> > According to the HW manual, this module cannot see which one is 
> >> > available/best suited.
> >> > So, I don't think this can be decided at runtime.
> >>
> >> I mean, can't Linux look at the rates of the two clocks, and if one is 
> >> zero,
> >> use the other?
> >
> > I still misunderstand your question though, but software cannot look at the 
> > rates
> > of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have 
> > registers
> > for indicating the rates.
> 
> If we have "clocks" properties, as you suggested, both clocks will have to
> be described in DT.  Hence Linux will create clock objects for them, and the
> USB PHY driver can call clk_get_rate() on those objects.
> 
> If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
> with zero rate should be described in DT.
> 
> Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.

Thank you for the detailed explanation!
I understood it. I will try to use such properties.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@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] phy: rcar-gen3-usb3: Initial support for xhci phy

[Petre Pircalabu]

Hi Shimoda-san,

I just saw that you patch predates mine. I will drop my version and
integrate yours.

Many thanks for for your support,
Petre

On Fri, May 19, 2017 at 12:50 AM, Petre Pircalabu
 wrote:
> Hi Geert, Shimoda-san,
>
> I have also used initialy the name "use-xtal-clk" to describe the
> flag, but I've changed it to "use-on-chip-clk" to make a clear
> distinction between the on-chip and the external clocks. If you think
> "usb-extal" is more descriptive I can change he name to this value.
>
> Regarding the clock, I suggest the distinction between them could be
> made using predefined names.
> E.g.:
> ..
> clocks = <>, <>
> clock-names = "external', "on-chip"
> ...
>
> The device node can define none, one of both the clocks:
> - If no clock is defined (e.g. backup compatibility with the
> Salvator-X  board), the driver should not set any of the phy registers
> and preserve the hw defaults (selects external)
> - If only one clock is defined the PHY driver should be set-up
> according to the name ("external" and "on-chip")
> - if both clocks are defined (and valid), the driver should choose one
> of them according to the "use-on-chip-clk" parameter (if present or
> not). (the scenario of having 2 valid clocks and selecting the on-chip
> one is possible, although I see no real application).
>
> The default should be "no clocks defined" in order not to break any
> existing (and not yet published) platforms.
>
> Do you think this proposal is feasible?
>
> Also, theoretically the driver should work also with r8a7795 ES2.0 and
> r8a7796, but unfortunately I don't have access to neither a H3 ES2.0
> nor a M3 platform. I can create a patch for these platforms, but I
> would require your assistance in order to test it (I'm feeling a
> little bit queasy in submitting an untested patch).
>
> Best regards,
> Petre
>
> On Thu, May 18, 2017 at 11:21 PM, Petre Pircalabu
>  wrote:
>> Hi Geert, Shimoda-san,
>>
>> First, thank you very much for your comments. I will refactor the
>> patch and send a v2 patchset as soon as possible.
>>
>> To my understanding, although the RCAR Gen3 HW manual references the
>> clk frequencies (50MHz for XTAL and 100MHz for external clock) the
>> actual register description only allows choosing between between an
>> on-chip clock and an external reference signal (no clock rate /
>> divisor is specified in the PHY registers ).
>> In my opinion, the "use-on-chip-clk" parameter was meant to
>> differentiate between the internal/external references, and not to by
>> a specific clock rate. My only concern here is that the device tree
>> description of the hardware might be a little difficult to understand
>> when compared to the actual description from SOC's HW manual.
>>
>> Many thanks for your support,
>> Petre
>>
>>
>>
>> On Thu, May 18, 2017 at 3:42 PM, Geert Uytterhoeven
>>  wrote:
>>> Hi Shimoda-san,
>>>
>>> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
>>>  wrote:
> From: Geert Uytterhoeven
> Sent: Thursday, May 18, 2017 8:41 PM
> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>  wrote:
> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register 
> >> >> block.
> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be 
> >> >> <0>.
> >> >
> >> > I think we should add "clocks" property as required.
> >> >
> >> >> +Optional properties:
> >> >
> >> > You should add "renesas,use-on-chip-clk" here.
> >> > And, the name of "use-on-chip-clk" is not good to me.
> >> > FYI, my developing patch names "renesas,usb-extal".

 I'm sorry for this. I missed description "on-chip clock source is supplied 
 though
 USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is 
 reasonable.

> >> Can this be decided at runtime, e.g. by looking at the rates of the 
> >> clocks
> >> to see which one is available/best suited?
> >
> > According to the HW manual, this module cannot see which one is 
> > available/best suited.
> > So, I don't think this can be decided at runtime.
>
> I mean, can't Linux look at the rates of the two clocks, and if one is 
> zero,
> use the other?

 I still misunderstand your question though, but software cannot look at 
 the rates
 of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't 
 have registers
 for indicating the rates.
>>>
>>> If we have "clocks" properties, as you suggested, both clocks will have to
>>> be described in DT.  Hence Linux will create clock objects for them, and the
>>> USB PHY driver can call clk_get_rate() on those objects.
>>>
>>> If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
>>> with zero rate should be described in DT.
>>>
>>> Cfr. the existing clocks with 

Re: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Petre Pircalabu]

Hi Geert, Shimoda-san,

I have also used initialy the name "use-xtal-clk" to describe the
flag, but I've changed it to "use-on-chip-clk" to make a clear
distinction between the on-chip and the external clocks. If you think
"usb-extal" is more descriptive I can change he name to this value.

Regarding the clock, I suggest the distinction between them could be
made using predefined names.
E.g.:
..
clocks = <>, <>
clock-names = "external', "on-chip"
...

The device node can define none, one of both the clocks:
- If no clock is defined (e.g. backup compatibility with the
Salvator-X  board), the driver should not set any of the phy registers
and preserve the hw defaults (selects external)
- If only one clock is defined the PHY driver should be set-up
according to the name ("external" and "on-chip")
- if both clocks are defined (and valid), the driver should choose one
of them according to the "use-on-chip-clk" parameter (if present or
not). (the scenario of having 2 valid clocks and selecting the on-chip
one is possible, although I see no real application).

The default should be "no clocks defined" in order not to break any
existing (and not yet published) platforms.

Do you think this proposal is feasible?

Also, theoretically the driver should work also with r8a7795 ES2.0 and
r8a7796, but unfortunately I don't have access to neither a H3 ES2.0
nor a M3 platform. I can create a patch for these platforms, but I
would require your assistance in order to test it (I'm feeling a
little bit queasy in submitting an untested patch).

Best regards,
Petre

On Thu, May 18, 2017 at 11:21 PM, Petre Pircalabu
 wrote:
> Hi Geert, Shimoda-san,
>
> First, thank you very much for your comments. I will refactor the
> patch and send a v2 patchset as soon as possible.
>
> To my understanding, although the RCAR Gen3 HW manual references the
> clk frequencies (50MHz for XTAL and 100MHz for external clock) the
> actual register description only allows choosing between between an
> on-chip clock and an external reference signal (no clock rate /
> divisor is specified in the PHY registers ).
> In my opinion, the "use-on-chip-clk" parameter was meant to
> differentiate between the internal/external references, and not to by
> a specific clock rate. My only concern here is that the device tree
> description of the hardware might be a little difficult to understand
> when compared to the actual description from SOC's HW manual.
>
> Many thanks for your support,
> Petre
>
>
>
> On Thu, May 18, 2017 at 3:42 PM, Geert Uytterhoeven
>  wrote:
>> Hi Shimoda-san,
>>
>> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
>>  wrote:
 From: Geert Uytterhoeven
 Sent: Thursday, May 18, 2017 8:41 PM
 On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
  wrote:
 >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register 
 >> >> block.
 >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be 
 >> >> <0>.
 >> >
 >> > I think we should add "clocks" property as required.
 >> >
 >> >> +Optional properties:
 >> >
 >> > You should add "renesas,use-on-chip-clk" here.
 >> > And, the name of "use-on-chip-clk" is not good to me.
 >> > FYI, my developing patch names "renesas,usb-extal".
>>>
>>> I'm sorry for this. I missed description "on-chip clock source is supplied 
>>> though
>>> USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is 
>>> reasonable.
>>>
 >> Can this be decided at runtime, e.g. by looking at the rates of the 
 >> clocks
 >> to see which one is available/best suited?
 >
 > According to the HW manual, this module cannot see which one is 
 > available/best suited.
 > So, I don't think this can be decided at runtime.

 I mean, can't Linux look at the rates of the two clocks, and if one is 
 zero,
 use the other?
>>>
>>> I still misunderstand your question though, but software cannot look at the 
>>> rates
>>> of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have 
>>> registers
>>> for indicating the rates.
>>
>> If we have "clocks" properties, as you suggested, both clocks will have to
>> be described in DT.  Hence Linux will create clock objects for them, and the
>> USB PHY driver can call clk_get_rate() on those objects.
>>
>> If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
>> with zero rate should be described in DT.
>>
>> Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.
>>
>> Gr{oetje,eeting}s,
>>
>> Geert
>>
>> --
>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
>> ge...@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.
>>   

Re: [PATCH] phy: rcar-gen3-usb3: Initial support for xhci phy

[Petre Pircalabu]

Hi Geert, Shimoda-san,

First, thank you very much for your comments. I will refactor the
patch and send a v2 patchset as soon as possible.

To my understanding, although the RCAR Gen3 HW manual references the
clk frequencies (50MHz for XTAL and 100MHz for external clock) the
actual register description only allows choosing between between an
on-chip clock and an external reference signal (no clock rate /
divisor is specified in the PHY registers ).
In my opinion, the "use-on-chip-clk" parameter was meant to
differentiate between the internal/external references, and not to by
a specific clock rate. My only concern here is that the device tree
description of the hardware might be a little difficult to understand
when compared to the actual description from SOC's HW manual.

Many thanks for your support,
Petre



On Thu, May 18, 2017 at 3:42 PM, Geert Uytterhoeven
 wrote:
> Hi Shimoda-san,
>
> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
>  wrote:
>>> From: Geert Uytterhoeven
>>> Sent: Thursday, May 18, 2017 8:41 PM
>>> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>>>  wrote:
>>> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register 
>>> >> >> block.
>>> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be 
>>> >> >> <0>.
>>> >> >
>>> >> > I think we should add "clocks" property as required.
>>> >> >
>>> >> >> +Optional properties:
>>> >> >
>>> >> > You should add "renesas,use-on-chip-clk" here.
>>> >> > And, the name of "use-on-chip-clk" is not good to me.
>>> >> > FYI, my developing patch names "renesas,usb-extal".
>>
>> I'm sorry for this. I missed description "on-chip clock source is supplied 
>> though
>> USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is 
>> reasonable.
>>
>>> >> Can this be decided at runtime, e.g. by looking at the rates of the 
>>> >> clocks
>>> >> to see which one is available/best suited?
>>> >
>>> > According to the HW manual, this module cannot see which one is 
>>> > available/best suited.
>>> > So, I don't think this can be decided at runtime.
>>>
>>> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
>>> use the other?
>>
>> I still misunderstand your question though, but software cannot look at the 
>> rates
>> of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have 
>> registers
>> for indicating the rates.
>
> If we have "clocks" properties, as you suggested, both clocks will have to
> be described in DT.  Hence Linux will create clock objects for them, and the
> USB PHY driver can call clk_get_rate() on those objects.
>
> If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
> with zero rate should be described in DT.
>
> Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@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] phy: rcar-gen3-usb3: Initial support for xhci phy

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
 wrote:
>> From: Geert Uytterhoeven
>> Sent: Thursday, May 18, 2017 8:41 PM
>> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>>  wrote:
>> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register 
>> >> >> block.
>> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>> >> >
>> >> > I think we should add "clocks" property as required.
>> >> >
>> >> >> +Optional properties:
>> >> >
>> >> > You should add "renesas,use-on-chip-clk" here.
>> >> > And, the name of "use-on-chip-clk" is not good to me.
>> >> > FYI, my developing patch names "renesas,usb-extal".
>
> I'm sorry for this. I missed description "on-chip clock source is supplied 
> though
> USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is 
> reasonable.
>
>> >> Can this be decided at runtime, e.g. by looking at the rates of the clocks
>> >> to see which one is available/best suited?
>> >
>> > According to the HW manual, this module cannot see which one is 
>> > available/best suited.
>> > So, I don't think this can be decided at runtime.
>>
>> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
>> use the other?
>
> I still misunderstand your question though, but software cannot look at the 
> rates
> of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have 
> registers
> for indicating the rates.

If we have "clocks" properties, as you suggested, both clocks will have to
be described in DT.  Hence Linux will create clock objects for them, and the
USB PHY driver can call clk_get_rate() on those objects.

If no clock is connected to USB3S0_CLK_P/USB3S0_CLK_M, a dummy clock
with zero rate should be described in DT.

Cfr. the existing clocks with "clock-frequency = <0>" in r8a7795.dtsi.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven
> Sent: Thursday, May 18, 2017 8:41 PM
> 
> Hi Shimoda-san,
> 
> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>  wrote:
> >> >> +- reg: offset and length of the partial USB 3.0 Host PHY register 
> >> >> block.
> >> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
> >> >
> >> > I think we should add "clocks" property as required.
> >> >
> >> >> +Optional properties:
> >> >
> >> > You should add "renesas,use-on-chip-clk" here.
> >> > And, the name of "use-on-chip-clk" is not good to me.
> >> > FYI, my developing patch names "renesas,usb-extal".

I'm sorry for this. I missed description "on-chip clock source is supplied 
though
USB_EXTAL/USB_XTAL" in the manual. So, this "renesas,use-on-chip-clk" is 
reasonable.

> >> Can this be decided at runtime, e.g. by looking at the rates of the clocks
> >> to see which one is available/best suited?
> >
> > According to the HW manual, this module cannot see which one is 
> > available/best suited.
> > So, I don't think this can be decided at runtime.
> 
> I mean, can't Linux look at the rates of the two clocks, and if one is zero,
> use the other?

I still misunderstand your question though, but software cannot look at the 
rates
of USB3S0_CLK_P/USB3S0_CLK_M and USB_EXTAL/XTAL because the HW doesn't have 
registers
for indicating the rates.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@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] phy: rcar-gen3-usb3: Initial support for xhci phy

[Geert Uytterhoeven]

Hi Shimoda-san,

On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
 wrote:
>> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
>> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>> >
>> > I think we should add "clocks" property as required.
>> >
>> >> +Optional properties:
>> >
>> > You should add "renesas,use-on-chip-clk" here.
>> > And, the name of "use-on-chip-clk" is not good to me.
>> > FYI, my developing patch names "renesas,usb-extal".
>>
>> Can this be decided at runtime, e.g. by looking at the rates of the clocks
>> to see which one is available/best suited?
>
> According to the HW manual, this module cannot see which one is 
> available/best suited.
> So, I don't think this can be decided at runtime.

I mean, can't Linux look at the rates of the two clocks, and if one is zero,
use the other?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Geert-san,

> -Original Message-
> From: Geert Uytterhoeven
> Sent: Thursday, May 18, 2017 5:46 PM
> 
> On Thu, May 18, 2017 at 4:53 AM, Yoshihiro Shimoda
>  wrote:
> >> From: Petre Pircalabu
> >> The USB30PHY of a RCAR-Gen3 XHCI device can select the reference clock
> >> source. The 2 available options are the differential input clock pair
> >> supplied on pins USB3S0_CLK_P / USB3S0_CLK_M (default) and the on-chip
> >> clock source supplied through USB_XTAL/USB_EXTAL.
> >>
> >> The device can be configured to use the on-chip source by adding the
> >> "renesas,use-on-chip-clk" option in the corresponding device tree node.
> >>
> >> Signed-off-by: Petre Pircalabu 
> 
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> >> @@ -0,0 +1,22 @@
> >> +* 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.
> >> +
> >> +Required properties:
> >> +- compatible: "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 
> >> compatible device.
> >
> > I would like to add "renesas,usb3-phy-r8a7795" and 
> > "renesas,usb3-phy-r8a7796".
> 
> "renesas,r8a7795-usb3-phy" etc.?
> 
> (I know all other Renesas PHY drivers use the old scheme)

Oops! Yes, we should be "renesas,-".
(However, "renesas,rcar-gen3-usb3-phy" is OK.)

> >> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
> >> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
> >
> > I think we should add "clocks" property as required.
> >
> >> +Optional properties:
> >
> > You should add "renesas,use-on-chip-clk" here.
> > And, the name of "use-on-chip-clk" is not good to me.
> > FYI, my developing patch names "renesas,usb-extal".
> 
> Can this be decided at runtime, e.g. by looking at the rates of the clocks
> to see which one is available/best suited?

According to the HW manual, this module cannot see which one is available/best 
suited.
So, I don't think this can be decided at runtime.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> ge...@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] phy: rcar-gen3-usb3: Initial support for xhci phy

[Geert Uytterhoeven]

On Thu, May 18, 2017 at 4:53 AM, Yoshihiro Shimoda
 wrote:
>> From: Petre Pircalabu
>> The USB30PHY of a RCAR-Gen3 XHCI device can select the reference clock
>> source. The 2 available options are the differential input clock pair
>> supplied on pins USB3S0_CLK_P / USB3S0_CLK_M (default) and the on-chip
>> clock source supplied through USB_XTAL/USB_EXTAL.
>>
>> The device can be configured to use the on-chip source by adding the
>> "renesas,use-on-chip-clk" option in the corresponding device tree node.
>>
>> Signed-off-by: Petre Pircalabu 

>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
>> @@ -0,0 +1,22 @@
>> +* 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.
>> +
>> +Required properties:
>> +- compatible: "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 
>> compatible device.
>
> I would like to add "renesas,usb3-phy-r8a7795" and "renesas,usb3-phy-r8a7796".

"renesas,r8a7795-usb3-phy" etc.?

(I know all other Renesas PHY drivers use the old scheme)

>> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
>> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.
>
> I think we should add "clocks" property as required.
>
>> +Optional properties:
>
> You should add "renesas,use-on-chip-clk" here.
> And, the name of "use-on-chip-clk" is not good to me.
> FYI, my developing patch names "renesas,usb-extal".

Can this be decided at runtime, e.g. by looking at the rates of the clocks
to see which one is available/best suited?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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] phy: rcar-gen3-usb3: Initial support for xhci phy

[Yoshihiro Shimoda]

Hi Petre,

Thank you for the patch!

I'm also developing a similar driver now :)
(I don't submit it though...)
My developing driver has SSC and VBUS_EN setting.

Anyway, I have some comments about your patch.

> -Original Message-
> From: Petre Pircalabu
> Sent: Thursday, May 18, 2017 2:58 AM
> 
> The USB30PHY of a RCAR-Gen3 XHCI device can select the reference clock
> source. The 2 available options are the differential input clock pair
> supplied on pins USB3S0_CLK_P / USB3S0_CLK_M (default) and the on-chip
> clock source supplied through USB_XTAL/USB_EXTAL.
> 
> The device can be configured to use the on-chip source by adding the
> "renesas,use-on-chip-clk" option in the corresponding device tree node.
> 
> Signed-off-by: Petre Pircalabu 
> ---
>  .../devicetree/bindings/phy/rcar-gen3-phy-usb3.txt |  22 +++
>  arch/arm64/boot/dts/renesas/r8a7795-es1.dtsi   |   8 +
>  arch/arm64/configs/defconfig   |   1 +
>  drivers/phy/Kconfig|   8 +
>  drivers/phy/Makefile   |   1 +
>  drivers/phy/phy-rcar-gen3-usb3.c   | 166 
> +

I think you should separate 3 patches as below:

1) Add phy-rcar-gen3-usb3 driver
2) Add a device node to r8a7795-es1.dtsi
3) Enable the phy driver on defconfig

And when we submit a generic phy driver's patch (e.g. 1) above), we should send 
to the phy maintainer(s).
Also, when we submit a patch that is related to device tree(e.g. both 1) and 2) 
above),
we should send to the device tree maintainer(s).

> 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 000..aa9657c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/rcar-gen3-phy-usb3.txt
> @@ -0,0 +1,22 @@
> +* 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.
> +
> +Required properties:
> +- compatible: "renesas,rcar-gen3-usb3-phy" for a generic R-Car Gen3 
> compatible device.

I would like to add "renesas,usb3-phy-r8a7795" and "renesas,usb3-phy-r8a7796".

> +- reg: offset and length of the partial USB 3.0 Host PHY register block.
> +- #phy-cells: see phy-bindings.txt in the same directory, must be <0>.

I think we should add "clocks" property as required.

> +Optional properties:

You should add "renesas,use-on-chip-clk" here.
And, the name of "use-on-chip-clk" is not good to me.
FYI, my developing patch names "renesas,usb-extal".

> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index a534cf5..aeda949 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_PHY_MIPHY28LP) += 
> phy-miphy28lp.o
>  obj-$(CONFIG_PHY_MIPHY365X)  += phy-miphy365x.o
>  obj-$(CONFIG_PHY_RCAR_GEN2)  += phy-rcar-gen2.o
>  obj-$(CONFIG_PHY_RCAR_GEN3_USB2) += phy-rcar-gen3-usb2.o
> +obj-$(CONFIG_PHY_RCAR_GEN3_USB3) += phy-rcar-gen3-usb3.o

The latest linux-phy.git / next branch doesn't update yet though,
the directory will be changed to ./drivers/phy/renesas.

http://www.spinics.net/lists/linux-usb/msg156875.html

> diff --git a/drivers/phy/phy-rcar-gen3-usb3.c 
> b/drivers/phy/phy-rcar-gen3-usb3.c
> new file mode 100644
> index 000..87fa24d
> --- /dev/null
> +++ b/drivers/phy/phy-rcar-gen3-usb3.c
> @@ -0,0 +1,166 @@
> +/*
> + * Renesas R-Car Gen3 for USB3.0 PHY driver
> + *
> + * Copyright (C) 2017 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> + * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> + * for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 

We can remove this "linux/of_address.h".

> +#include 
> +#include 
> +#include 
> +
> +#define USB30_CLKSET0 0x34
> +#define USB30_CLKSET1 0x36
> +
> +#define USB30_CLKSET0_FSEL_MASK 0x003F
> +#define USB30_CLKSET0_FSEL_USB_XTAL 0x0002
> +#define USB30_CLKSET0_FSEL_USB3S0_CLK   0x0027
> +
> +#define USB30_CLKSET1_PLL_MULTI_MASK0x1FC0
> +#define USB30_CLKSET1_PLL_MULTI_USB_XTAL(0x64 << 6)
> +#define USB30_CLKSET1_PLL_MULTI_USB3S0_CLK  (0x00 << 6)

I prefer the "6" becomes a definition.

> +#define USB30_CLKSET1_REF_CLKDIVBIT(3)
> +#define USB30_CLKSET1_USB_SEL   BIT(0)
> +
> +struct rcar_gen3_usb3_phy {
> + void __iomem *base;
> + struct phy *phy;
> + u8 use_on_chip_clk;