[PATCH 1/2] clk: add lpc18xx creg clk driver

[Michael Turquette]

Quoting Joachim Eastwood (2016-02-17 10:24:12)
> On 17 February 2016 at 01:56, Michael Turquette <mturquette at baylibre.com> wrote:
> > Quoting Joachim Eastwood (2016-02-13 06:38:45)
> >> Hi Mike,
> >>
> >> Seems your reply got lost in my mailbox and I didn't notice it before
> >> Stephen replied on the cover letter. Sorry about that.
> >>
> >> On 18 August 2015 at 02:26, Michael Turquette <mturquette at baylibre.com> wrote:
> >> > Quoting Joachim Eastwood (2015-08-13 13:43:11)
> >> >> On 11 August 2015 at 22:41, Michael Turquette <mturquette at baylibre.com> wrote:
> >> >> > Hi Joachim,
> >> >> >
> >> >> > Quoting Joachim Eastwood (2015-07-11 14:48:26)
> >> >> >> +static void __init lpc18xx_creg_clk_init(struct device_node *np)
> >> >> >> +{
> >> >> >> +       const char *clk_32khz_parent;
> >> >> >> +       struct regmap *syscon;
> >> >> >> +
> >> >> >> +       syscon = syscon_node_to_regmap(np->parent);
> >> >> >> +       if (IS_ERR(syscon)) {
> >> >> >> +               pr_err("%s: syscon lookup failed\n", __func__);
> >> >> >> +               return;
> >> >> >> +       }
> >> >> >> +
> >> >> >> +       clk_32khz_parent = of_clk_get_parent_name(np, 0);
> >> >> >> +
> >> >> >> +       clk_creg[CREG_CLK_32KHZ] =
> >> >> >> +               clk_register_creg_clk(&clk_creg_clocks[CREG_CLK_32KHZ],
> >> >> >> +                                     &clk_32khz_parent, syscon);
> >> >> >> +
> >> >> >> +       clk_creg[CREG_CLK_1KHZ] =
> >> >> >> +               clk_register_creg_clk(&clk_creg_clocks[CREG_CLK_1KHZ],
> >> >> >> +                                     &clk_creg_clocks[CREG_CLK_32KHZ].name,
> >> >> >> +                                     syscon);
> >> >> >> +
> >> >> >> +       of_clk_add_provider(np, of_clk_src_onecell_get, &clk_base_data);
> >> >> >> +}
> >> >> >> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
> >> >> >
> >> >> > I'll ask the same question that Stephen asked in your CCU/CGU driver
> >> >> > series: is it necessary to use CLK_OF_DECLARE here or can you use the
> >> >> > platform device model?
> >> >>
> >> >> The 32 kHz clock from the CREG block is a clock parent to the CGU
> >> >> block so it's possible that it will required early. This is all
> >> >> depends on how the boot loader initially configures the CGU.
> >> >>
> >> >> Currently in the DTS for lpc18xx cgu it has:
> >> >> clocks = <&xtal>, <&xtal32>, <...>;
> >> >> xtal32 is just a temporary placeholder until the CREG clock is in place.
> >> >
> >> > Well that seems wrong. Is it just a matter of probe order where you try
> >> > to probe the cgu driver before the creg driver?
> >>
> >> The ideal probe order for the clk drivers on the lpc18xx platform
> >> would be; creg-clk, cgu and ccu.
> >> If the 32k clk is used by any of timers we need creg-clk to enable the
> >> 32k clock and determine the rate.
> >>
> >> Note that the cgu and ccu driver is using CLK_OF_DECLARE now.
> >>
> >>
> >> I have tired to create an overview of the lpc18xx clock system here:
> >> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
> >
> > That's great, thanks a lot for the link and the nice documentation.
> >
> > It's been a while since I've looked at this thread. Do any of the
> > creg-clk, cgu or ccu clock drivers need to use CLK_OF_DECLARE? If they
> > were platform_drivers then you could use -EPROBE_DEFER to solve your
> > ordering issue.
> 
> The clock for the clocksource/timer
> (drivers/clocksource/time-lpc32xx.c) comes from cgu/ccu and can also
> come from creg-clk. So afaik they must use CLK_OF_DECLARE. Or is there
> something I am missing?

No, you're not missing anything. What I want is for clock provider
drivers to actually be real Linux device drivers. Right now the drivers
that only call CLK_OF_DECLARE are essentially just initcall hacks, and
this causes problems down the road when you decide you want your clk
provider driver to have suspend/resume ops, etc. Also I feel that having
proper drivers will be important as runtime pm and the generic power
domains stuff matures.

> Isn't true that most SoC system clock drivers must use CLK_OF_DECLARE
> because the system timer require clocks early? I thought this was the
> whole point.

On ARMv8, mandatory use of architected timers removes the need for the
clk framework to be involved with timer init, so it's definitely not
true for all platforms.

On QCOM's 32-bit platforms they have an always-on clock with a known
fixed rate, so they can either put the fixed-rate clock in DT, or even
just put the frequency as a property into the timer node with no need to
involve the clk framework (which is what they do today). That is also a
bit hacky, but it is why you won't find CLK_OF_DECLARE in
drivers/clk/qcom and all of their drivers are proper platform_drivers.

> 
> Since it is possible that the 32k clock from creg-clk can be use as a
> parent clock for the timer doesn't this mean it also must use
> CLK_OF_DECLARE?

Right, if your 32k clock is a fixed-clock, and if it is provided on the
board (e.g. external extal -> lpc timer) then you could just put the
fixed-rate clock into DT (which uses CLK_OF_DECLARE) and reference that
from your timer node. The rest of the clocks could be registered later
through a platform_driver.

Sounds like your timer clocking scheme is more complicated than that
however.

> If it wasn't possible for creg-clk to be used as a parent clock for
> the timer it could have been a platform device.
> 
> 
> The lpc18xx clock configuration that require creg-clk early would be:
> 32k - > PLL0 -> BASE_M3/M4_CLK -> CLK_M3/M4_TIMER0. See second figure
> on the webpage. Note that this is probably not a common configuration,
> but it is a valid one. (PLL0 accepts input down to 14 kHz)

I was just discussing this with Stephen and we're both curious to know
if the same "nxp,lpc1850-creg-clk" compatible string can be used with
CLK_OF_DECLARE for registering the bare minimum early clks, and also
with a platform_driver using a different table of clocks for late
registration.

The compat string could be the same, and the driver would have different
tables/setup functions to keep from re-registering the same clocks
twice.

I know that nobody likes to be the guinea pig for this type of stuff,
but the lack of participation in the Linux driver model for clk drivers
needs to curbed. Do you think the above proposed scheme could work for
your clock provider drivers?

Regards,
Mike

> 
> 
> regards,
> Joachim Eastwood