Re: [PATCH] Revert "clk: Detect failure to set defaults"

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4240 bytes --]

On Wed, Jan 05, 2022 at 08:56:50PM +0100, Marek Vasut wrote:
> On 1/5/22 20:37, Tom Rini wrote:
> > On Wed, Jan 05, 2022 at 08:35:19PM +0100, Marek Vasut wrote:
> > > On 1/1/22 22:41, Sean Anderson wrote:
> > > > Hi Marek,
> > > 
> > > Hi,
> > > 
> > > > Please CC clock maintainers for future patches.
> > > 
> > > btw. I'm surprised the commit 92f1e9a4b31c0bf0f4f61ab823a6a88657323646 has
> > > zero reviews/acks from clock maintainers.
> > > 
> > > > On 1/1/22 1:51 PM, Marek Vasut wrote:
> > > > > This reverts commit 92f1e9a4b31c0bf0f4f61ab823a6a88657323646.
> > > > > The aforementioned patch causes massive breakage on all platforms which
> > > > > have 'assigned-clock' DT property in their DT which references any clock
> > > > > that are not supported by the platform clock driver. That can easily
> > > > > happen either in SPL, or because the clock driver is reduced. Currently
> > > > > it seems all iMX8M are affected and fail to boot altogether.
> > > > > 
> > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > Cc: Peng Fan <peng.fan@oss.nxp.com>
> > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > ---
> > > > >    drivers/clk/clk-uclass.c | 6 +-----
> > > > >    1 file changed, 1 insertion(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > > > > index f2d26427543..094b1abf13c 100644
> > > > > --- a/drivers/clk/clk-uclass.c
> > > > > +++ b/drivers/clk/clk-uclass.c
> > > > > @@ -846,17 +846,13 @@ void devm_clk_put(struct udevice *dev, struct
> > > > > clk *clk)
> > > > >    int clk_uclass_post_probe(struct udevice *dev)
> > > > >    {
> > > > > -    int ret;
> > > > > -
> > > > >        /*
> > > > >         * when a clock provider is probed. Call clk_set_defaults()
> > > > >         * also after the device is probed. This takes care of cases
> > > > >         * where the DT is used to setup default parents and rates
> > > > >         * using assigned-clocks
> > > > >         */
> > > > > -    ret = clk_set_defaults(dev, CLK_DEFAULTS_POST);
> > > > > -    if (ret)
> > > > > -        return log_ret(ret);
> > > > > +    clk_set_defaults(dev, CLK_DEFAULTS_POST);
> > > > >        return 0;
> > > > >    }
> > > > > 
> > > > 
> > > > See [1] for previous discussion. For more background,
> > > > 
> > > > - Device trees for i.MX are sync'd with Linux.
> > > > - General clock assignments may live in the clock-controller node,
> > > 
> > > clock assignments can be anywhere, even in non-clock-controller nodes.
> > > 
> > > >     including those which U-Boot does not implement, but which Linux does.
> > > >     It's OK to not set up these clocks, but U-Boot doesn't know that and
> > > >     fails.
> > > > 
> > > > We don't necessarily need to revert this commit, but we do need a way to
> > > > say "it's OK not to set the defaults, since we can function without
> > > > them". Tom suggested doing this in the clock driver last time. I think a
> > > > Kconfig or a device tree property would work, perhaps something like
> > > > 'u-boot,clock-defaults-optional'.
> > > 
> > > We didn't need custom DT properties before, Linux doesn't need them either,
> > > so that approach seems wrong.
> > > 
> > > If the clock driver could say "skip unimplemented clock, because I don't
> > > implement them and that is OK", that sounds like the right approach.
> > > 
> > > Unless the 2022.01 release should be completely broken for a lot of
> > > platforms, I would propose we revert the clock uclass patch now and re-add
> > > it right after the release, so we would not roll out a completely broken
> > > release and would have more time to fix this properly.
> > 
> > It'll be no more broken than v2021.10 was for whatever platforms have
> > problems here, yes?  Since that's what has the problematic commit.
> 
> So it seems. Does that mean we are OK with releasing such a broken release,
> even though there is an easy way to improve the situation ?

I will defer to the clock maintainer who I see agrees with your patch.
I was just noting that we'd already shipped one release so a last minute
revert is not something I'm happy about either.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]