[PATCH v5 1/5] dm: clk: add stub when CONFIG_CLK is deactivated

Patrick DELAUNAY patrick.delaunay at st.com
Tue Mar 10 10:52:57 CET 2020


Hi Simon,

> From: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> Sent: vendredi 6 mars 2020 12:28
> 
> On Fri, Mar 6, 2020 at 11:01 AM Patrick Delaunay <patrick.delaunay at st.com>
> wrote:
> >
> > Add stub for functions clk_...() when CONFIG_CLK is deactivated.
> >
> > This patch avoids compilation issues for driver using these API
> > without protection (#if CONFIG_IS_ENABLED(CLK))
> >
> > For example, before this patch we have undefined reference to
> > `clk_disable_bulk') for code:
> >   clk_disable_bulk(&priv->clks);
> >   clk_release_bulk(&priv->clks);
> >
> > The bulk stub set and test bulk->count to avoid error for the sequence:
> >
> >   clk_get_bulk(dev, &priv->bulk);
> >         ....
> >   if (clk_enable(&priv>bulk))
> >         return -EIO;
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> >
> > Changes in v5:
> > - use ERR_PTR in clk_get_parent()
> > - force bulk->count = 0 in clk_get_bulk to avoid issue
> >   for next call of clk_enable_bulk / clk_enable_bulk
> 
> I wonder if this is correct. I think it only produces additional code. If you always set
> bulk->count to 0, the enable code will never fail.
> 
> However, the enable function should never be executed as enable returns an error.
> I can imagine the *disable* function could be called, but the return value of this
> function is currently only handled in clk_sandbox_test.c...
>
> Wouldn't it work to just remove all checks for bulk->count and let *all* redefined
> functions return a constant (success or failure)? That would help to keep the code
> small.

I wasn't sure if each stub should raise error or success.

When I code the stub the first time I found one example for API usage without protection
which can cause issue in drivers/net/ftgmac100.c

ftgmac100_ofdata_to_platdata
=> clk_get_bulk(dev, &priv->clks);

ftgmac100_probe(struct udevice *dev)
=> clk_enable_bulk(&priv->clks);

An other example : drivers/power/domain/meson-ee-pwrc.c
   meson_ee_pwrc_probe => clk_get_bulk(dev, &priv->clks);
   meson_ee_pwrc_on => clk_enable_bulk(&priv->clks);

And same in drivers/power/domain/meson-gx-pwrc-vpu.c

But finally after deeper check today, these driver can't handle the proposed clk stubs
(error during probe for clk_get_bulk).

So I agree that I complexify the clk stub only for over protection.

> Looking at linux, clock disable functions have *no* return value that needs to be
> checked, clock enable functions return success when compiled without clock
> support.

I also check the stub for reset function in "./include/reset.h"
- reset_free
- reset_assert
- reset_deassert
- reset_deassert_bulk
- reset_assert_bulk

All these reset stub functions return 0

So to be coherent I will update the patch to return success for 
clk_free, clk_enable, clk_disable, clk_disable_bulk and clk_enable_bulk.

And other functions return error -ENOSYS: request / get_rate / set_rate

> Regards,
> Simon
> 

Regards
Patrick


More information about the U-Boot mailing list