Message-ID: <154888381385.169292.12776041058756822056@swboyd.mtv.corp.google.com>
Date:   Wed, 30 Jan 2019 13:30:13 -0800
From:   Stephen Boyd <sboyd@...nel.org>
To:     Jerome Brunet <jbrunet@...libre.com>,
        Michael Turquette <mturquette@...libre.com>
Cc:     linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH 2/9] clk: Introduce get_parent_hw clk op

Quoting Jerome Brunet (2019-01-30 01:53:41)
> 
> It is the value we find in the register after the bootloader. Since we have no
> idea what it is, we must assume it is invalid.

Ok, so my understanding is correct.

> 
> The problem was that there only one known parent for this particular clock,
> and the CCF quirk (call get_parent() only if num_parent == 1) is making things
> difficult for us.
> 
> We found a work around to this. We declare a non existing parent, which make
> num_parent == 2 and force CCF to call get_parent(), and later on set_parent()
> ... It works but it sucks.

Yes that is bad.

>
> Sure, good idea.
> 
> If clk_hw_get_parent_by_index() may return an error, it because get_parent()
> could provide an invalid index.
> 
> Which brings us back to the original point, if it is possible for get_parent()
> to return invalid index, which means out of bounds, it shall be called even if
> num_parent == 1.
> 
> To be coherent, either:
> 1) the return value of get_parent() *MUST* be valid, it is fine to not call it
> if num_parent == 1 because we already know the only possible result.
> 
> 2) the return value of get_parent *MAY* be invalid, then it should be called
> regarless of num_parent.
> 
> IMO, (2) is the only valid option because any existing muxes could already be
> returning invalid indexes. I know we do on Amlogic (with various muxes, even
> with num_parent >= 1) and I'm sure we are not the only ones. Plus a driver
> needs to have a mean to tell CCF that things are not as expected.

Right. Having drivers specify more clks than the kernel knows about or
having the return value of get_parent() be out of range is non-obvious.
I'd rather see any drivers that want to express errors during the parent
initialization phase return an error pointer through the get_parent_hw
clk op instead.

Put another way, if you have any edge case like this you should migrate
to this new clk op and stop using .get_parent and playing tricks with
.num_parents and return values from .get_parent.

> 
> > 
> > so I took
> > the route of adding another callback with semantics that we can define
> > now because there aren't any users. The difference between the two is
> > made intentionally.
> 
> I still think that the drivers relying on this quirk (only 1 AFAIK) are broken
> and should be fixed, rather that keeping the quirk for everyone else. 

Agreed. Use the new API?

> 
> With this quirk, CCF is making an assumption that might be wrong.
> 
> The quirk is very easy put in the get_parent() callback of the said driver, or
> even better, don't provide the callback if it should not be called.
> 
> I understand the need for a cautious approach. It seems I'm only one with that
> issue right now and since I have a work around, there is no rush. But we must
> have plan to make it right.
> 
> To be clear, I'm not against your new API but I don't think it should be a
> reason to keep a broken behavior the framework.
> 

So do you think you can use this new clk_op and ignore the problems with
the .get_parent clk op? Putting effort into fixing the .get_parent
design isn't very useful from my perspective. There's more than just the
problem that we don't call it when .num_parents is 1. There's the
inability to return errors without doing weird things to return an index
out of range and there isn't any way for us to really know if the clk is
an orphan or not. If we can migrate all drivers to use the new clk op
then we can fix these problems too, and deprecate and eventually remove
the broken by design .get_parent clk op API.

BTW, I included this patch in this series because I'm wondering if it
would be useful for there to be a 'parent cookie' pointer that we pass
between the framework and providers instead using a raw index.

	struct clk_parent {
		struct clk_hw *hw;
	};

Then drivers could embed these structs into their parent_data structure
and the framework could pass pointers to these structures in struct
clk_rate_request. That may make it easier for drivers to attach extra
data to the parent for a particular clk when changing parents or rates.
For example, if the index of the parent is not a linear monotonic range
of [0, num_parents) then we could let them throw the index into either a
wrapper structure or a void *data field.

	struct my_clk_parent {
		struct clk_parent parent;
		u32 mask;
		u8 select_bit;
	};

Or
	struct my_clk_parent_data {
		u32 mask;
		u8 select_bit;
	};

	struct clk_parent {
		struct clk_hw *hw;
		void *data;
	};

And have data point to struct my_clk_parent. There's code in various clk
drivers that maps between the framework's view of the parent index and
the providers view, i.e. the hardware index. If we already have the
parent_map structure, maybe we should let that live in the provider
driver near the hardware and then just pass a pointer to it so drivers
can do container_of() or ->data accesses to get any driver data they
associate with that parent index.

If we did something like this, it may reduce code further and let us get
rid of the u8 index in the clk framework entirely. But we may want to
have the .get_parent_hw op return a pointer to a struct clk_parent
instead of a struct clk_hw now to make this easier to implement in the
future. Or this could all be overkill and only help a few drivers skip a
mapping step.

Powered by blists - more mailing lists