Re: [PATCH 3/3] clk: keystone: Add sci-clk driver support

[Stephen Boyd <sboyd@codeaurora.org>]

On 12/09, Tero Kristo wrote:
> On 08/12/16 23:10, Stephen Boyd wrote:
> >On 12/08, Tero Kristo wrote:
> >>On 08/12/16 02:13, Stephen Boyd wrote:
> >>>On 10/21, Tero Kristo wrote:
> >>>>diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> >>>>new file mode 100644
> >>>>index 0000000..f6af5bd
> >>>>--- /dev/null
> >>>>+++ b/drivers/clk/keystone/sci-clk.c
> >>
> >>>
> >>>>+
> >>>>+	handle = devm_ti_sci_get_handle(dev);
> >>>>+	if (IS_ERR(handle))
> >>>>+		return PTR_ERR(handle);
> >>>>+
> >>>>+	provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL);
> >>>>+	if (!provider)
> >>>>+		return -ENOMEM;
> >>>>+
> >>>>+	provider->clocks = data;
> >>>>+
> >>>>+	provider->sci = handle;
> >>>>+	provider->ops = &handle->ops.clk_ops;
> >>>>+	provider->dev = dev;
> >>>>+
> >>>>+	ti_sci_init_clocks(provider);
> >>>
> >>>And if this fails?
> >>
> >>Yea this is kind of controversial. ti_sci_init_clocks() can fail if
> >>any of the clocks registered will fail. I decided to have it this
> >>way so that at least some clocks might work in failure cause, and
> >>you might have a booting device instead of total lock-up.
> >>
> >>Obviously it could be done so that if any clock fails, we would
> >>de-register all clocks at that point, but personally I think this is
> >>a worse option.
> >>
> >>ti_sci_init_clocks could probably be modified to continue
> >>registering clocks when a single clock fails though. Currently it
> >>aborts at first failure.
> >>
> >
> >That sounds like a better approach if we don't care about
> >failures to register a clock. Returning a value from a function
> >and not using it isn't really a great design.
> >
> >I worry that if we start returning errors from clk_hw_register()
> >that something will go wrong though, so really I don't know why
> >we want to ignore errors at all. Just for debugging a boot hang?
> >Can't we use early console to at least see that this driver is
> >failing to probe and debug that way?
> 
> Early console can be used to debug that, but it is kind of annoying
> to recompile most of the kernel when you suddenly need to use it.

I thought SERIAL_EARLYCON was selected by drivers that support
it? So there shouldn't be any rebuilding required.

> 
> How about modifying the ti_sci_init_clocks func to print an error
> for each failed clock?

Ok that's fine too. I'd prefer the function had a return type of
void if we're not planning on using the return value, that's all.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project