[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