Re: [PATCH v2] clk: renesas: rcar-usb2-clock-sel: Add R-Car USB 2.0 clock selector PHY

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Shimoda-san,

On Mon, Jul 24, 2017 at 12:33 PM, Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
>> From: Geert Uytterhoeven
>> Sent: Wednesday, July 5, 2017 9:22 PM
>> On Wed, Jul 5, 2017 at 1:57 PM, Yoshihiro Shimoda
>> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> From: Geert Uytterhoeven
>> >> Sent: Wednesday, July 5, 2017 7:09 PM
>> >> On Wed, Jun 28, 2017 at 8:28 AM, Yoshihiro Shimoda
>> >> <yoshihiro.shimoda.uh@renesas.com> wrote:
>> >> > R-Car USB 2.0 controller can change the clock source from an oscillator
>> >> > to an external clock via a register. So, this patch adds support
>> >> > the clock source selector as a clock driver.
>>
>> >> > --- /dev/null
>> >> > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
>>
>> >> > +/* Since this driver needs other ccf drivers, this uses subsys_initcall_sync */
>> >> > +subsys_initcall_sync(rcar_usb2_clock_sel_init);
>> >>
>> >> I suppose this is a workaround for the lack of probe deferral support in the
>> >> USB subsystem?
>> >
>> > This is my fault. I added "power-domains" property into this device's node.
>> > After I remove the power-domains like the cpg node, this driver can use subsys_initcall()
>> > instead of subsys_initcall_sync().
>>
>> Does the clock sel module requires the functional clock "ehci_ohci" to be
>> running before you can access its registers?
>> If yes, I think there should be a "power-domains" property.
>
> Yes. But...
>
>> Then, you can simplify the code by calling
>>
>>         pm_runtime_enable(dev);
>>         pm_runtime_get_sync(dev);
>>
>> and remove the explicit handling of the functional clock.
>>
>> That does not solve probe deferral handling in the USB subsystem, though.
>
> I added a debug message at end of cpg_mssr_probe(), and if I used subsys_initcall() on
> rcar-usb2-clock-sel driver, kernel log output below:
> ========================
> [    0.272547] rcar-usb2-clock-sel e6590630.clock-controller: probe deferral not supported
> [    0.273944]  ----------------- cpg_mssr_probe: probed!
> ========================
> So, it seems the renesas-cpg-mssr.c doesn't solve probe deferral handling.
> (The driver renesas-cpg-mssr.c uses platform_driver_probe() for now.)
>
> So, IIUC, this rcar-usb2-clock-sel driver cannot use subsys_initcall().
> Or, should we modify the renesas-cpg-mssr.c somehow?

Drivers should avoid using subsys_initcall().
Why do you use a subsys_initcall()? To avoid probe deferral in the USB
susbystem?
What happens if the rcar-usb2-clock-sel uses builtin_platform_driver()?

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