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

[Yoshihiro Shimoda]

Hi Geert-san,

> -Original Message-
> From: Geert Uytterhoeven
> Sent: Monday, July 24, 2017 9:59 PM
> 
> Hi Shimoda-san,
> 
> On Mon, Jul 24, 2017 at 12:33 PM, Yoshihiro Shimoda
>  wrote:
> >> From: Geert Uytterhoeven
> >> Sent: Wednesday, July 5, 2017 9:22 PM
> >> On Wed, Jul 5, 2017 at 1:57 PM, Yoshihiro Shimoda
> >>  wrote:
> >> >> From: Geert Uytterhoeven
> >> >> Sent: Wednesday, July 5, 2017 7:09 PM
> >> >> On Wed, Jun 28, 2017 at 8:28 AM, Yoshihiro Shimoda
> >> >>  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?

Oh, this is my fault.
I had assumed a clock driver should use subsys_initcall()...
Like clk-fixed-rate.c, we can use builtin_platform_driver in general.

> What happens if the rcar-usb2-clock-sel uses builtin_platform_driver()?

I tried it today, and then the rcar-usb2-clock-sel works :)
So, I will submit v3 patch.

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 v2] clk: renesas: rcar-usb2-clock-sel: Add R-Car USB 2.0 clock selector PHY

[Geert Uytterhoeven]

Hi Shimoda-san,

On Mon, Jul 24, 2017 at 12:33 PM, Yoshihiro Shimoda
 wrote:
>> From: Geert Uytterhoeven
>> Sent: Wednesday, July 5, 2017 9:22 PM
>> On Wed, Jul 5, 2017 at 1:57 PM, Yoshihiro Shimoda
>>  wrote:
>> >> From: Geert Uytterhoeven
>> >> Sent: Wednesday, July 5, 2017 7:09 PM
>> >> On Wed, Jun 28, 2017 at 8:28 AM, Yoshihiro Shimoda
>> >>  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 -- 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 v2] clk: renesas: rcar-usb2-clock-sel: Add R-Car USB 2.0 clock selector PHY

[Yoshihiro Shimoda]

Hi Stephen-san,

Thank you for your comments!

> From: Stephen Boyd
> Sent: Thursday, July 6, 2017 6:18 AM
> 
> On 06/28, Yoshihiro Shimoda wrote:
> > +
> > +
> > +   platform_set_drvdata(pdev, priv);
> > +   dev_set_drvdata(dev, priv);
> > +
> > +   init.name = "rcar_usb2_clock_sel";
> > +   init.ops = _clock_sel_clock_ops;
> > +   init.flags = CLK_IS_BASIC;
> 
> Please drop CLK_IS_BASIC unless you need it.

I got it.

> > +   init.parent_names = NULL;
> > +   init.num_parents = 0;
> > +   priv->hw.init = 
> > +
> > +   clk = clk_register(NULL, >hw);
> > +   if (IS_ERR(clk))
> > +   return PTR_ERR(clk);
> > +
> > +   error = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> 
> Can you use clk_hw_register() and of_clk_add_hw_provider()
> please?

I could use of_clk_add_hw_provider().

Best regards,
Yoshihiro Shimoda

> > +   if (error)
> > +   return error;
> > +
> > +   return 0;
> > +}
> > +
> > +static const struct dev_pm_ops rcar_usb2_clock_sel_pm_ops = {
> > +   .suspend= rcar_usb2_clock_sel_suspend,
> > +   .resume = rcar_usb2_clock_sel_resume,
> > +};
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

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

[Yoshihiro Shimoda]

Hi Rob,

> -Original Message-
> From: Rob Herring
> Sent: Wednesday, July 5, 2017 11:16 PM
> 
> On Wed, Jun 28, 2017 at 03:28:35PM +0900, Yoshihiro Shimoda 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.
> >
> > Signed-off-by: Yoshihiro Shimoda 
> > ---
> >  This patch is based on the renesas-drivers.git /
> > renesas-drivers-2017-06-27-v4.12-rc7 tag.
> >
> >  Changes from v1:
> >   - Change this driver as a clock driver from a generic phy driver.
> > https://patchwork.kernel.org/patch/9788697/
> >   - Remove "RFC" tag.
> >
> >  .../bindings/clock/renesas,rcar-usb2-clock-sel.txt |  54 ++
> >  drivers/clk/renesas/Kconfig|   5 +
> >  drivers/clk/renesas/Makefile   |   1 +
> >  drivers/clk/renesas/rcar-usb2-clock-sel.c  | 205 
> > +
> >  4 files changed, 265 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> >  create mode 100644 drivers/clk/renesas/rcar-usb2-clock-sel.c
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> > new file mode 100644
> > index 000..75ccafc
> > --- /dev/null
> > +++ 
> > b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> > @@ -0,0 +1,54 @@
> > +* Renesas R-Car USB 2.0 clock selector
> > +
> > +This file provides information on what the device node for the R-Car USB 
> > 2.0
> > +clock selector.
> > +
> > +If you connect an external clock to the USB_EXTAL pin only, you should set
> > +the clock rate to "usb_extal" node only.
> > +If you connect an oscillator to both the USB_XTAL and USB_EXTAL, this 
> > module
> > +is not needed because this is default setting. (Of course, you can set the
> > +clock rates to both "usb_extal" and "usb_xtal" nodes.
> > +
> > +Case 1: An external clock connects to R-Car SoC
> > ++--+   +--- R-Car -+
> > +|External  |---|USB_EXTAL ---> all usb channels|
> > +|clock |   |USB_XTAL   |
> > ++--+   +---+
> > +In this case, we need this driver with "usb_extal" clock.
> > +
> > +Case 2: An oscillator connects to R-Car SoC
> > ++--+   +--- R-Car -+
> > +|Oscillator|---|USB_EXTAL -+-> all usb channels|
> > +|  |---|USB_XTAL --+   |
> > ++--+   +---+
> > +In this case, we don't need this selector.
> > +
> > +Required properties:
> > +- compatible: "renesas,r8a7795-rcar-usb2-clock-sel" if the device is a 
> > part of
> > + an R8A7795 SoC.
> > +  "renesas,r8a7796-rcar-usb2-clock-sel" if the device if a 
> > part of
> > + an R8A7796 SoC.
> > +  "renesas,rcar-gen3-usb2-clock-sel" 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 2.0 clock selector register block.
> > +- clocks: A list of phandles and specifier pairs.
> > +- clock-names: Name of the clocks.
> > + - The functional clock must be "ehci_ohci"
> > + - The USB_EXTAL clock pin must be "usb_extal"
> > + - The USB_XTAL clock pin must be "usb_xtal"
> > +- #clock-cells: Must be 0
> > +
> > +Exxample (R-Car H3):
> > +
> > +usb2_clksel: clock-controller@e6590630 {
> > +compatible = "renesas,r8a77950-rcar-usb2-clock-sel",
> > + "renesas,rcar-gen3-usb2-clock-sel";
> > +reg = <0 0xe6590630 0 0x02>;
> > +clocks = < CPG_MOD 703>, <_extal>, <_xtal>;
> > +clock-names = "ehci_ohci", "usb_extal", "usb_xtal";
> 
> Missing #clock-cells

Oops, I will fix it.

> With that, for the binding:
> 
> Acked-by: Rob Herring 

Thank you for your Acked-by!

Best regards,
Yoshihiro Shimoda

> Rob

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

[Stephen Boyd]

On 06/28, Yoshihiro Shimoda wrote:
> +
> +
> + platform_set_drvdata(pdev, priv);
> + dev_set_drvdata(dev, priv);
> +
> + init.name = "rcar_usb2_clock_sel";
> + init.ops = _clock_sel_clock_ops;
> + init.flags = CLK_IS_BASIC;

Please drop CLK_IS_BASIC unless you need it.

> + init.parent_names = NULL;
> + init.num_parents = 0;
> + priv->hw.init = 
> +
> + clk = clk_register(NULL, >hw);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + error = of_clk_add_provider(np, of_clk_src_simple_get, clk);

Can you use clk_hw_register() and of_clk_add_hw_provider()
please?

> + if (error)
> + return error;
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rcar_usb2_clock_sel_pm_ops = {
> + .suspend= rcar_usb2_clock_sel_suspend,
> + .resume = rcar_usb2_clock_sel_resume,
> +};
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Rob Herring]

On Wed, Jun 28, 2017 at 03:28:35PM +0900, Yoshihiro Shimoda 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.
> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  This patch is based on the renesas-drivers.git /
> renesas-drivers-2017-06-27-v4.12-rc7 tag.
> 
>  Changes from v1:
>   - Change this driver as a clock driver from a generic phy driver.
> https://patchwork.kernel.org/patch/9788697/
>   - Remove "RFC" tag.
> 
>  .../bindings/clock/renesas,rcar-usb2-clock-sel.txt |  54 ++
>  drivers/clk/renesas/Kconfig|   5 +
>  drivers/clk/renesas/Makefile   |   1 +
>  drivers/clk/renesas/rcar-usb2-clock-sel.c  | 205 
> +
>  4 files changed, 265 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
>  create mode 100644 drivers/clk/renesas/rcar-usb2-clock-sel.c
> 
> diff --git 
> a/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt 
> b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> new file mode 100644
> index 000..75ccafc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
> @@ -0,0 +1,54 @@
> +* Renesas R-Car USB 2.0 clock selector
> +
> +This file provides information on what the device node for the R-Car USB 2.0
> +clock selector.
> +
> +If you connect an external clock to the USB_EXTAL pin only, you should set
> +the clock rate to "usb_extal" node only.
> +If you connect an oscillator to both the USB_XTAL and USB_EXTAL, this module
> +is not needed because this is default setting. (Of course, you can set the
> +clock rates to both "usb_extal" and "usb_xtal" nodes.
> +
> +Case 1: An external clock connects to R-Car SoC
> ++--+   +--- R-Car -+
> +|External  |---|USB_EXTAL ---> all usb channels|
> +|clock |   |USB_XTAL   |
> ++--+   +---+
> +In this case, we need this driver with "usb_extal" clock.
> +
> +Case 2: An oscillator connects to R-Car SoC
> ++--+   +--- R-Car -+
> +|Oscillator|---|USB_EXTAL -+-> all usb channels|
> +|  |---|USB_XTAL --+   |
> ++--+   +---+
> +In this case, we don't need this selector.
> +
> +Required properties:
> +- compatible: "renesas,r8a7795-rcar-usb2-clock-sel" if the device is a part 
> of
> +   an R8A7795 SoC.
> +  "renesas,r8a7796-rcar-usb2-clock-sel" if the device if a part 
> of
> +   an R8A7796 SoC.
> +  "renesas,rcar-gen3-usb2-clock-sel" 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 2.0 clock selector register block.
> +- clocks: A list of phandles and specifier pairs.
> +- clock-names: Name of the clocks.
> + - The functional clock must be "ehci_ohci"
> + - The USB_EXTAL clock pin must be "usb_extal"
> + - The USB_XTAL clock pin must be "usb_xtal"
> +- #clock-cells: Must be 0
> +
> +Exxample (R-Car H3):
> +
> +usb2_clksel: clock-controller@e6590630 {
> +compatible = "renesas,r8a77950-rcar-usb2-clock-sel",
> + "renesas,rcar-gen3-usb2-clock-sel";
> +reg = <0 0xe6590630 0 0x02>;
> +clocks = < CPG_MOD 703>, <_extal>, <_xtal>;
> +clock-names = "ehci_ohci", "usb_extal", "usb_xtal";

Missing #clock-cells

With that, for the binding:

Acked-by: Rob Herring 

Rob

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

[Geert Uytterhoeven]

Hi Shimoda-san,

On Wed, Jul 5, 2017 at 1:57 PM, Yoshihiro Shimoda
 wrote:
>> From: Geert Uytterhoeven
>> Sent: Wednesday, July 5, 2017 7:09 PM
>> On Wed, Jun 28, 2017 at 8:28 AM, Yoshihiro Shimoda
>>  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.

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.

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 v2] clk: renesas: rcar-usb2-clock-sel: Add R-Car USB 2.0 clock selector PHY

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven
> Sent: Wednesday, July 5, 2017 7:09 PM
> 
> Hi Simoda-san,
> 
> On Wed, Jun 28, 2017 at 8:28 AM, Yoshihiro Shimoda
>  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.
> >
> > Signed-off-by: Yoshihiro Shimoda 
> 
> Thanks for your patch!

Thank you very much for the comments!

> > --- /dev/null
> > +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
> 
> > +static void usb2_clock_sel_enable_extal_only(struct usb2_clock_sel_priv 
> > *priv)
> > +{
> > +   u16 val = readw(priv->base + USB20_CLKSET0);
> > +
> > +   pr_debug("%s: enter %d %d %x\n", __func__,
> > +priv->extal, priv->xtal, val);
> > +
> > +   if (priv->extal && !priv->xtal && val != CLKSET0_EXTAL_ONLY)
> > +   writew(CLKSET0_EXTAL_ONLY, priv->base + USB20_CLKSET0);
> > +}
> > +
> > +static void usb2_clock_sel_disable_extal_only(struct usb2_clock_sel_priv 
> > *priv)
> > +{
> > +   if (priv->extal && !priv->xtal)
> > +   writew(CLKSET0_PRIVATE, priv->base + USB20_CLKSET0);
> > +}
> > +
> > +static int usb2_clock_sel_enable(struct clk_hw *hw)
> > +{
> > +   usb2_clock_sel_enable_extal_only(to_priv(hw));
> > +
> > +   return 0;
> > +}
> > +
> > +static void usb2_clock_sel_disable(struct clk_hw *hw)
> > +{
> > +   usb2_clock_sel_disable_extal_only(to_priv(hw));
> > +}
> > +
> > +/*
> > + * This module seems a mux, but this driver assumes a gate because
> > + * ehci/ohci platform drivers don't support clk_set_parent() for now.
> > + * If this driver acts as a gate, ehci/ohci-platform drivers don't need
> > + * any modification.
> > + */
> > +static const struct clk_ops usb2_clock_sel_clock_ops = {
> > +   .enable = usb2_clock_sel_enable,
> > +   .disable = usb2_clock_sel_disable,
> > +};
> 
> So you do "smart muxing" in the enable routine above?

Yes.

> > +static int __init rcar_usb2_clock_sel_probe(struct platform_device *pdev)
> > +{
> > +   struct device *dev = >dev;
> > +   struct device_node *np = dev->of_node;
> > +   struct usb2_clock_sel_priv *priv;
> > +   struct resource *res;
> > +   struct clk *clk;
> > +   struct clk_init_data init;
> > +   int error;
> > +
> > +   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +   return -ENOMEM;
> > +
> > +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +   priv->base = devm_ioremap_resource(dev, res);
> > +   if (IS_ERR(priv->base))
> > +   return PTR_ERR(priv->base);
> > +
> > +   priv->ehci_clk = devm_clk_get(dev, "ehci_ohci");
> > +   if (!IS_ERR(priv->ehci_clk)) {
> > +   error = clk_prepare_enable(priv->ehci_clk);
> > +   if (error)
> > +   return error;
> > +   }
> > +
> > +   clk = devm_clk_get(dev, "usb_extal");
> > +   if (!IS_ERR(clk) && !clk_prepare_enable(clk)) {
> > +   priv->extal = !!clk_get_rate(clk);
> > +   clk_disable_unprepare(clk);
> > +   }
> > +   clk = devm_clk_get(dev, "usb_xtal");
> > +   if (!IS_ERR(clk) && !clk_prepare_enable(clk)) {
> > +   priv->xtal = !!clk_get_rate(clk);
> > +   clk_disable_unprepare(clk);
> > +   }
> > +
> > +   if (!priv->extal && !priv->extal) {
> > +   dev_err(dev, "This driver needs usb_extal or usb_xtal\n");
> > +   return -ENOENT;
> 
> Perhaps enabled clocks should be disabled on error?

Oops! I should call clk_disable_unprepare(priv->ehci_clk) here.
I will fix it.

> > +   }
> > +
> > +   platform_set_drvdata(pdev, priv);
> > +   dev_set_drvdata(dev, priv);
> > +
> > +   init.name = "rcar_usb2_clock_sel";
> > +   init.ops = _clock_sel_clock_ops;
> > +   init.flags = CLK_IS_BASIC;
> > +   init.parent_names = NULL;
> > +   init.num_parents = 0;
> > +   priv->hw.init = 
> > +
> > +   clk = clk_register(NULL, >hw);
> > +   if (IS_ERR(clk))
> > +   return PTR_ERR(clk);
> 
> Likewise.

Same here. So, I will use "goto" for it.

> > +   error = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> > +   if (error)
> > +   return error;
> > +
> > +   return 0;
> 
> return of_clk_add_provider(np, of_clk_src_simple_get, clk);

Thanks. I will fix it.

> > +/* 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 

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

[Geert Uytterhoeven]

Hi Simoda-san,

On Wed, Jun 28, 2017 at 8:28 AM, Yoshihiro Shimoda
 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.
>
> Signed-off-by: Yoshihiro Shimoda 

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c

> +static void usb2_clock_sel_enable_extal_only(struct usb2_clock_sel_priv 
> *priv)
> +{
> +   u16 val = readw(priv->base + USB20_CLKSET0);
> +
> +   pr_debug("%s: enter %d %d %x\n", __func__,
> +priv->extal, priv->xtal, val);
> +
> +   if (priv->extal && !priv->xtal && val != CLKSET0_EXTAL_ONLY)
> +   writew(CLKSET0_EXTAL_ONLY, priv->base + USB20_CLKSET0);
> +}
> +
> +static void usb2_clock_sel_disable_extal_only(struct usb2_clock_sel_priv 
> *priv)
> +{
> +   if (priv->extal && !priv->xtal)
> +   writew(CLKSET0_PRIVATE, priv->base + USB20_CLKSET0);
> +}
> +
> +static int usb2_clock_sel_enable(struct clk_hw *hw)
> +{
> +   usb2_clock_sel_enable_extal_only(to_priv(hw));
> +
> +   return 0;
> +}
> +
> +static void usb2_clock_sel_disable(struct clk_hw *hw)
> +{
> +   usb2_clock_sel_disable_extal_only(to_priv(hw));
> +}
> +
> +/*
> + * This module seems a mux, but this driver assumes a gate because
> + * ehci/ohci platform drivers don't support clk_set_parent() for now.
> + * If this driver acts as a gate, ehci/ohci-platform drivers don't need
> + * any modification.
> + */
> +static const struct clk_ops usb2_clock_sel_clock_ops = {
> +   .enable = usb2_clock_sel_enable,
> +   .disable = usb2_clock_sel_disable,
> +};

So you do "smart muxing" in the enable routine above?

> +static int __init rcar_usb2_clock_sel_probe(struct platform_device *pdev)
> +{
> +   struct device *dev = >dev;
> +   struct device_node *np = dev->of_node;
> +   struct usb2_clock_sel_priv *priv;
> +   struct resource *res;
> +   struct clk *clk;
> +   struct clk_init_data init;
> +   int error;
> +
> +   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +   if (!priv)
> +   return -ENOMEM;
> +
> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +   priv->base = devm_ioremap_resource(dev, res);
> +   if (IS_ERR(priv->base))
> +   return PTR_ERR(priv->base);
> +
> +   priv->ehci_clk = devm_clk_get(dev, "ehci_ohci");
> +   if (!IS_ERR(priv->ehci_clk)) {
> +   error = clk_prepare_enable(priv->ehci_clk);
> +   if (error)
> +   return error;
> +   }
> +
> +   clk = devm_clk_get(dev, "usb_extal");
> +   if (!IS_ERR(clk) && !clk_prepare_enable(clk)) {
> +   priv->extal = !!clk_get_rate(clk);
> +   clk_disable_unprepare(clk);
> +   }
> +   clk = devm_clk_get(dev, "usb_xtal");
> +   if (!IS_ERR(clk) && !clk_prepare_enable(clk)) {
> +   priv->xtal = !!clk_get_rate(clk);
> +   clk_disable_unprepare(clk);
> +   }
> +
> +   if (!priv->extal && !priv->extal) {
> +   dev_err(dev, "This driver needs usb_extal or usb_xtal\n");
> +   return -ENOENT;

Perhaps enabled clocks should be disabled on error?

> +   }
> +
> +   platform_set_drvdata(pdev, priv);
> +   dev_set_drvdata(dev, priv);
> +
> +   init.name = "rcar_usb2_clock_sel";
> +   init.ops = _clock_sel_clock_ops;
> +   init.flags = CLK_IS_BASIC;
> +   init.parent_names = NULL;
> +   init.num_parents = 0;
> +   priv->hw.init = 
> +
> +   clk = clk_register(NULL, >hw);
> +   if (IS_ERR(clk))
> +   return PTR_ERR(clk);

Likewise.

> +   error = of_clk_add_provider(np, of_clk_src_simple_get, clk);
> +   if (error)
> +   return error;
> +
> +   return 0;

return of_clk_add_provider(np, of_clk_src_simple_get, clk);

> +/* 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?

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

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

[Yoshihiro Shimoda]

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.

Signed-off-by: Yoshihiro Shimoda 
---
 This patch is based on the renesas-drivers.git /
renesas-drivers-2017-06-27-v4.12-rc7 tag.

 Changes from v1:
  - Change this driver as a clock driver from a generic phy driver.
https://patchwork.kernel.org/patch/9788697/
  - Remove "RFC" tag.

 .../bindings/clock/renesas,rcar-usb2-clock-sel.txt |  54 ++
 drivers/clk/renesas/Kconfig|   5 +
 drivers/clk/renesas/Makefile   |   1 +
 drivers/clk/renesas/rcar-usb2-clock-sel.c  | 205 +
 4 files changed, 265 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
 create mode 100644 drivers/clk/renesas/rcar-usb2-clock-sel.c

diff --git 
a/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt 
b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
new file mode 100644
index 000..75ccafc
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/renesas,rcar-usb2-clock-sel.txt
@@ -0,0 +1,54 @@
+* Renesas R-Car USB 2.0 clock selector
+
+This file provides information on what the device node for the R-Car USB 2.0
+clock selector.
+
+If you connect an external clock to the USB_EXTAL pin only, you should set
+the clock rate to "usb_extal" node only.
+If you connect an oscillator to both the USB_XTAL and USB_EXTAL, this module
+is not needed because this is default setting. (Of course, you can set the
+clock rates to both "usb_extal" and "usb_xtal" nodes.
+
+Case 1: An external clock connects to R-Car SoC
++--+   +--- R-Car -+
+|External  |---|USB_EXTAL ---> all usb channels|
+|clock |   |USB_XTAL   |
++--+   +---+
+In this case, we need this driver with "usb_extal" clock.
+
+Case 2: An oscillator connects to R-Car SoC
++--+   +--- R-Car -+
+|Oscillator|---|USB_EXTAL -+-> all usb channels|
+|  |---|USB_XTAL --+   |
++--+   +---+
+In this case, we don't need this selector.
+
+Required properties:
+- compatible: "renesas,r8a7795-rcar-usb2-clock-sel" if the device is a part of
+ an R8A7795 SoC.
+  "renesas,r8a7796-rcar-usb2-clock-sel" if the device if a part of
+ an R8A7796 SoC.
+  "renesas,rcar-gen3-usb2-clock-sel" 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 2.0 clock selector register block.
+- clocks: A list of phandles and specifier pairs.
+- clock-names: Name of the clocks.
+ - The functional clock must be "ehci_ohci"
+ - The USB_EXTAL clock pin must be "usb_extal"
+ - The USB_XTAL clock pin must be "usb_xtal"
+- #clock-cells: Must be 0
+
+Exxample (R-Car H3):
+
+usb2_clksel: clock-controller@e6590630 {
+compatible = "renesas,r8a77950-rcar-usb2-clock-sel",
+ "renesas,rcar-gen3-usb2-clock-sel";
+reg = <0 0xe6590630 0 0x02>;
+clocks = < CPG_MOD 703>, <_extal>, <_xtal>;
+clock-names = "ehci_ohci", "usb_extal", "usb_xtal";
+};
diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
index 78d1df9..094df5c 100644
--- a/drivers/clk/renesas/Kconfig
+++ b/drivers/clk/renesas/Kconfig
@@ -114,6 +114,11 @@ config CLK_RCAR_GEN3_CPG
bool
select CLK_RENESAS_CPG_MSSR
 
+config CLK_RCAR_USB2_CLOCK_SEL
+   bool "Renesas R-Car USB2 clock selector support"
+   depends on ARCH_RENESAS || COMPILE_TEST
+   help
+ This is a driver for R-Car USB2 clock selector
 
 # Generic
 config CLK_RENESAS_CPG_MSSR
diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefile
index 02d0412..4eec85b 100644
--- a/drivers/clk/renesas/Makefile
+++ b/drivers/clk/renesas/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_CLK_SH73A0)  += clk-sh73a0.o
 obj-$(CONFIG_CLK_RCAR_GEN2)+= clk-rcar-gen2.o
 obj-$(CONFIG_CLK_RCAR_GEN2_CPG)+= rcar-gen2-cpg.o
 obj-$(CONFIG_CLK_RCAR_GEN3_CPG)+= rcar-gen3-cpg.o
+obj-$(CONFIG_CLK_RCAR_USB2_CLOCK_SEL)  += rcar-usb2-clock-sel.o
 
 # Generic
 obj-$(CONFIG_CLK_RENESAS_CPG_MSSR) += renesas-cpg-mssr.o
diff --git a/drivers/clk/renesas/rcar-usb2-clock-sel.c 
b/drivers/clk/renesas/rcar-usb2-clock-sel.c
new file mode 100644
index 000..b48b2fc
--- /dev/null
+++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
@@ -0,0