Re: [RFC 6/9] clk: ti: add support for omap4 module clocks

[Michael Turquette <mturquette@baylibre.com>]

Quoting Tero Kristo (2016-01-03 23:36:05)
> On 01/01/2016 07:48 AM, Michael Turquette wrote:
> > Hi Tero,
> >
> > Quoting Tero Kristo (2015-12-18 05:58:58)
> >> Previously, hwmod core has been used for controlling the hwmod level
> >> clocks. This has certain drawbacks, like being unable to share the
> >> clocks for multiple users, missing usecounting and generally being
> >> totally incompatible with common clock framework.
> >>
> >> Add support for new clock type under the TI clock driver, which will
> >> be used to convert all the existing hwmdo clocks to. This helps to
> >> get rid of the clock related hwmod data from kernel and instead
> >> parsing this from DT.
> >
> > I'm really happy to see this series. Looks pretty good to me.
> >
> >> +static int _omap4_hwmod_clk_enable(struct clk_hw *hw)
> >> +{
> >> +       struct clk_hw_omap *clk =3D to_clk_hw_omap(hw);
> >> +       u32 val;
> >> +       int timeout =3D 0;
> >> +       int ret;
> >> +
> >> +       if (!clk->enable_bit)
> >> +               return 0;
> >> +
> >> +       if (clk->clkdm) {
> >> +               ret =3D ti_clk_ll_ops->clkdm_clk_enable(clk->clkdm, hw=
->clk);
> >> +               if (ret) {
> >> +                       WARN(1,
> >> +                            "%s: could not enable %s's clockdomain %s=
: %d\n",
> >> +                            __func__, clk_hw_get_name(hw),
> >> +                            clk->clkdm_name, ret);
> >> +                       return ret;
> >> +               }
> >> +       }
> >> +
> >> +       val =3D ti_clk_ll_ops->clk_readl(clk->enable_reg);
> >> +
> >> +       val &=3D ~OMAP4_MODULEMODE_MASK;
> >> +       val |=3D clk->enable_bit;
> >> +
> >> +       ti_clk_ll_ops->clk_writel(val, clk->enable_reg);
> >> +
> >> +       /* Wait until module is enabled */
> >> +       while (!_omap4_is_ready(val)) {
> >> +               udelay(1);
> >
> > This should really be a .prepare callback if you plan to keep the delays
> > in there.
> =

> If this is changed to a .prepare, then all OMAP power management is =

> effectively ruined as all clocks are going to be enabled all the time. =


Let's not ruin system PM.

> hwmod core doesn't support .prepare/.enable at the moment that well, and =

> changing that is going to be a big burden (educated guess, haven't =

> checked this yet)... The call chain that comes here is:
> =

> device driver -> pm_runtime -> hwmod_core -> hwmod_clk_enable / disable.

Right, and for calls to pm_runtime_get/put from process context it
should result in a call to clk_prepare_enable/clk_disable_unprepare. I
guess that change is hugely invasive from your statements above?

> =

> The delay within this function should usually be pretty short, just to =

> wait that the module comes up from idle.
> =

> I recall the discussions regarding the udelays within clk_enable/disable =

> calls, but what is the preferred approach then?

There are many cases where a clk only provides .{un}prepare ops and does
NOT provide any .{en,dis}able ops.

> Typically =

> clk_enable/disable just becomes a NOP

Yes, it becomes a NOP (though it is critical that drivers with knowledge
of this do not try to skip the step of calling clk_enable).

> if it is not allowed to wait for =

> hardware to complete transitioning before exiting the function.

The clk.h api clearly states that clk_prepare must be called and
complete before calling clk_enable. So if a clk only provides a .prepare
with delays but no .enable, and a consumer driver complies with that api
rule then we're guaranteed to have a toggling line when clk_enable
returns.

Regards,
Mike

> =

> -Tero
> =

> >
> > Regards,
> > Mike
> >
>=20