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
<petre.pircalabu@xxxxxxxxx> 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 = <&clk1>, <&clk2>
> 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
> <petre.pircalabu@xxxxxxxxx> 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
>> <geert@xxxxxxxxxxxxxx> wrote:
>>> Hi Shimoda-san,
>>>
>>> On Thu, May 18, 2017 at 2:19 PM, Yoshihiro Shimoda
>>> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
>>>>> From: Geert Uytterhoeven
>>>>> Sent: Thursday, May 18, 2017 8:41 PM
>>>>> On Thu, May 18, 2017 at 1:13 PM, Yoshihiro Shimoda
>>>>> <yoshihiro.shimoda.uh@xxxxxxxxxxx> 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 -- 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
