Revert "clk: fix __clk_init_parent() for single parent clocks"

This reverts commit 2430a94d1e719b7b4af2a65b781a4c036eb22e64.

From the original commit message:
It turned out a problem because there are some single-parent clocks that implement .get_parent() callback and return non-zero index. The SOCFPGA clock is the case; the commit broke the SOCFPGA boards.

It is wrong for a clock to return an index >= num_parents. CCF checks for this condition in clk_core_get_parent_by_index(). This function sets the parent to NULL if index is incoherent, which seems like the only sane choice.

commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks") appears to be a work around installed in the core framework for a problem that is platform specific, and should probably be fixed in the platform code.

Further more, it introduces a problem in a corner case of the mux clock. Take mux with multiple parents, but only one is known, the rest being undocumented. The register reset has one of unknown parent set.

Before commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
* get_parent() is called, register is read and give an unknown index. -> the mux is orphaned.
* a call to set_rate() will reparent the mux to the only known parent.

With commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
* the register is never read.
* parent is wrongly assumed to be the only known one.
As a consequence, all the calculation deriving from the mux will be wrong
* Since we believe the only know parent to be set, set_parent() won't ever be called and the register won't be set with the correct value.

Signed-off-by Jerome Brunet <[email protected]>

---

Quoting Jerome Brunet (2018-12-04 08:32:57)

Quoted 12 lines

This reverts commit 2430a94d1e719b7b4af2a65b781a4c036eb22e64.

From the original commit message:
It turned out a problem because there are some single-parent clocks that implement .get_parent() callback and return non-zero index. The SOCFPGA clock is the case; the commit broke the SOCFPGA boards.

It is wrong for a clock to return an index >= num_parents. CCF checks for this condition in clk_core_get_parent_by_index(). This function sets the parent to NULL if index is incoherent, which seems like the only sane choice.

Quoted 5 lines

commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks")
appears to be a work around installed in the core framework for a problem that is platform specific, and should probably be fixed in the platform code.

Ouch. I see that I even pointed that out in 2016 but never got a reply or a fix patch[1].

Quoted 10 lines

Further more, it introduces a problem in a corner case of the mux clock. Take mux with multiple parents, but only one is known, the rest being undocumented. The register reset has one of unknown parent set.

Before commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
* get_parent() is called, register is read and give an unknown index. -> the mux is orphaned.
* a call to set_rate() will reparent the mux to the only known parent.

Quoted 8 lines

With commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
* the register is never read.
* parent is wrongly assumed to be the only known one.
As a consequence, all the calculation deriving from the mux will be wrong
* Since we believe the only know parent to be set, set_parent() won't ever be called and the register won't be set with the correct value.

Isn't this the broken bad case all over again? Why register a clk as a mux and then only say it has one parent?

Quoted 3 lines

Signed-off-by Jerome Brunet <[email protected]>

---

Is this related to the other patch you sent? Can you send series for related patches please?

[1] https://lkml.kernel.org/r/[email protected]

Quoted 41 lines

Quoting Jerome Brunet (2018-12-04 08:32:57)

Quoted 12 lines

This reverts commit 2430a94d1e719b7b4af2a65b781a4c036eb22e64.

From the original commit message:
It turned out a problem because there are some single-parent clocks that implement .get_parent() callback and return non-zero index. The SOCFPGA clock is the case; the commit broke the SOCFPGA boards.

It is wrong for a clock to return an index >= num_parents. CCF checks for this condition in clk_core_get_parent_by_index(). This function sets the parent to NULL if index is incoherent, which seems like the only sane choice.

Quoted 5 lines

commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks")
appears to be a work around installed in the core framework for a problem that is platform specific, and should probably be fixed in the platform code.

Ouch. I see that I even pointed that out in 2016 but never got a reply or a fix patch[1].

Quoted 10 lines

Further more, it introduces a problem in a corner case of the mux clock. Take mux with multiple parents, but only one is known, the rest being undocumented. The register reset has one of unknown parent set.

Before commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
* get_parent() is called, register is read and give an unknown index. -> the mux is orphaned.
* a call to set_rate() will reparent the mux to the only known parent.

Quoted 8 lines

With commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
* the register is never read.
* parent is wrongly assumed to be the only known one.
As a consequence, all the calculation deriving from the mux will be wrong
* Since we believe the only know parent to be set, set_parent() won't ever be called and the register won't be set with the correct value.

Quoted 2 lines

Isn't this the broken bad case all over again? Why register a clk as a mux and then only say it has one parent?

I understand it is a bit odd but as I explained it is a corner case.

We are really trying to drive a mux here, applying a values will change the clock signal we get. Documentation being what it is, we only know one the parent. The other parent could anything or maybe not connected at all, who know. That is not the important part actually

If such mux was already set to the known entry by default, it would be OK to ignore it. But if it is not, then we CCF to realise that and change the setting accordingly.

This the case of the 'ao_cts_cec' clock in the following patch:
https://lore.kernel.org/patchwork/patch/1021028/

by default the value in the register is 0, but the only one that makes sense for us is 1.

Quoted 8 lines

Quoted 4 lines

Signed-off-by Jerome Brunet <[email protected]>

---

Is this related to the other patch you sent? Can you send series for related patches please?

Quoted 1 line

[1] https://lkml.kernel.org/r/[email protected]

Actually I was intially doing a series, and stopped when my cover letter started with "those are two unrelated patches ..." ;)

I found these things while debugging the same thing but there is no deps between them.

Quoting Jerome Brunet (2018-12-04 11:51:17)

Quoted 68 lines · Stephen Boyd

Quoted 48 lines

Quoting Jerome Brunet (2018-12-04 08:32:57)

Quoted 13 lines

This reverts commit 2430a94d1e719b7b4af2a65b781a4c036eb22e64.

From the original commit message:
It turned out a problem because there are some single-parent clocks that implement .get_parent() callback and return non-zero index. The SOCFPGA clock is the case; the commit broke the SOCFPGA boards.

It is wrong for a clock to return an index >= num_parents. CCF checks for this condition in clk_core_get_parent_by_index(). This function sets
the parent to NULL if index is incoherent, which seems like the only sane choice.

Quoted 7 lines

commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks")
appears to be a work around installed in the core framework for a problem
that is platform specific, and should probably be fixed in the platform
code.

Ouch. I see that I even pointed that out in 2016 but never got a reply or a fix patch[1].

Quoted 12 lines

Further more, it introduces a problem in a corner case of the mux clock.
Take mux with multiple parents, but only one is known, the rest being undocumented. The register reset has one of unknown parent set.

Before commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks"):
* get_parent() is called, register is read and give an unknown index. -> the mux is orphaned.
* a call to set_rate() will reparent the mux to the only known
parent.

Quoted 10 lines

With commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent
clocks"):
* the register is never read.
* parent is wrongly assumed to be the only known one.
As a consequence, all the calculation deriving from the mux will be wrong
* Since we believe the only know parent to be set, set_parent() won't ever be called and the register won't be set with the correct
value.

Quoted 2 lines

Isn't this the broken bad case all over again? Why register a clk as a mux and then only say it has one parent?

I understand it is a bit odd but as I explained it is a corner case.

We are really trying to drive a mux here, applying a values will change the
clock signal we get. Documentation being what it is, we only know one the parent. The other parent could anything or maybe not connected at all, who know. That is not the important part actually

If such mux was already set to the known entry by default, it would be OK to
ignore it. But if it is not, then we CCF to realise that and change the setting accordingly.

This the case of the 'ao_cts_cec' clock in the following patch:
https://lore.kernel.org/patchwork/patch/1021028/

Quoted 3 lines

by default the value in the register is 0, but the only one that makes sense
for us is 1.

Ok. Thanks for the explanation. What do you do to fix this problem in the 'ao_cts_cec' clk implementation?

Sounds like you just rely on
clk_set_rate() to fix this all up for you when the consumer changes the rate?

If we have something that is default parented to something we're not telling the framework about for some reason then one solution would be to have some init code reparent the clk in hardware before registering it with the framework. Basically "fix up" the clk tree in hardware to be consistent with what software can reason about.

This also reminds me of some audio clks I've seen before where the default parent is some external pin and it can be reparented to an internal clk with clk_set_rate/parent. In that case, I think we forced the parent over to the internal clk before registering so that it would always get parented properly in the framework. Right now, this is going to cause problems for plans to probe defer clk_get() on orphan clks.

Maybe this could work better if we allowed 'assigned-clock-parents/rates' to reparent clks regardless of orphan status and/or had some flag that could be set on clks to indicate that we're OK if clk_get() is called on it when it's an orphan because this isn't a problem?

Or we can make the defer on orphan code only defer if the clk has a single parent and it's an orphan and return the clk if there is at least one parent of the clk that has been registered and isn't marked as an orphan. That would need to be recursively applied, so I guess we would update our orphan marking code to indicate that clk_get() on the clk should probe defer or not instead of indicating the clk's orphan status. Doing this would also side-step the problem Rockchip was having where certain parents never appeared but the clk was parented to it in hardware, essentially blocking the clk from ever being touched by consumers.

On the other hand, not having the clk there causes us to do a global search of the clk name space each time we look for parents by the "unknown" index, which in the Rockchip case was quite often because the clk was changing between two other parents with a third one that never got registered. So it may be better to just register an "unknown" clk as a root clk with a rate of 0 that you can then hook everything up to that is hardware reset to this unknown input. That way nothing is orphaned and everyone is happy. We could do this for the audio case I talked about earlier too so when the external pin is left unconnected we could register some 'unconnected' clk and parent the audio clk to it.

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Signed-off-by Jerome Brunet <[email protected]>

---

Is this related to the other patch you sent? Can you send series for related patches please?

Quoted 8 lines

Quoted 1 line

[1] https://lkml.kernel.org/r/[email protected]

Actually I was intially doing a series, and stopped when my cover letter started with "those are two unrelated patches ..." ;)

I found these things while debugging the same thing but there is no deps between them.

Ok.

Quoted 3 lines

Ok. Thanks for the explanation. What do you do to fix this problem in the 'ao_cts_cec' clk implementation?

IMO, there is nothing to fix, the implementation is correct.

Quoted 3 lines

Sounds like you just rely on
clk_set_rate() to fix this all up for you when the consumer changes the rate?

To setup the rate as the customer expect, yes.

Quoted 7 lines

If we have something that is default parented to something we're not telling the framework about for some reason then one solution would be to have some init code reparent the clk in hardware before registering it with the framework. Basically "fix up" the clk tree in hardware to be consistent with what software can reason about.

Should we really ? Something the framework does not know about is not necessarily something wrong.

It would best if we could model the tree completly, but most of the time, we just approximate it the best we can.

The framework just knows how to setup rates, it has no idea what rate needs to be set or when - And I think it is best if does not make any assumption about that.

Pushing it a bit, this unknown parent might important to the boot sequence. How would you know when it is safe to change the setting ?
What would change it to ? It Seems obvious when there is only one known parent, but what if there two ? which one do you pick ? Is it really the role of CCF to make this kind of choice ?

Quoted 8 lines

This also reminds me of some audio clks I've seen before where the default parent is some external pin and it can be reparented to an internal clk with clk_set_rate/parent. In that case, I think we forced the parent over to the internal clk before registering so that it would always get parented properly in the framework. Right now, this is going to cause problems for plans to probe defer clk_get() on orphan clks.

clk_get() on orphaned clock should probably return an error if it is not the case already ? or 0 maybe ?

Quoted 7 lines

Maybe this could work better if we allowed 'assigned-clock-parents/rates' to reparent clks regardless of orphan status and/or had some flag that could be set on clks to indicate that we're OK if clk_get() is called on it when it's an orphan because this isn't a problem?

Not sure I understand your point here.

Quoted 12 lines

Or we can make the defer on orphan code only defer if the clk has a single parent and it's an orphan and return the clk if there is at least one parent of the clk that has been registered and isn't marked as an orphan. That would need to be recursively applied, so I guess we would update our orphan marking code to indicate that clk_get() on the clk should probe defer or not instead of indicating the clk's orphan status. Doing this would also side-step the problem Rockchip was having where certain parents never appeared but the clk was parented to it in hardware, essentially blocking the clk from ever being touched by consumers.

... and now you lost me completly :)

Quoted 12 lines

On the other hand, not having the clk there causes us to do a global search of the clk name space each time we look for parents by the "unknown" index, which in the Rockchip case was quite often because the clk was changing between two other parents with a third one that never got registered. So it may be better to just register an "unknown" clk as a root clk with a rate of 0 that you can then hook everything up to that is hardware reset to this unknown input. That way nothing is orphaned and everyone is happy. We could do this for the audio case I talked about earlier too so when the external pin is left unconnected we could register some 'unconnected' clk and parent the audio clk to it.

We have sometimes a few orphans in the amlogic clock tree, it does not seems to a problem.

I don't really understand the benefit of having a fake clock that would adopt all the orphan ? You guess there is problem I'm not aware of ...

Sorry this email is long.

TL;DR is that I don't think we need to revert the patch as you suggest. Instead, we should fix the driver to indicate NULL as the parent name of the index it tells the framework about.

Quoting Jerome Brunet (2018-12-05 09:20:09)

Quoted 11 lines · Stephen Boyd

Quoted 2 lines

Ok. Thanks for the explanation. What do you do to fix this problem in the 'ao_cts_cec' clk implementation?

IMO, there is nothing to fix, the implementation is correct.

Quoted 3 lines

Sounds like you just rely on
clk_set_rate() to fix this all up for you when the consumer changes the rate?

To setup the rate as the customer expect, yes.

Ok.

Quoted 19 lines

Quoted 5 lines

If we have something that is default parented to something we're not telling the framework about for some reason then one solution would be to have some init code reparent the clk in hardware before registering it with the framework. Basically "fix up" the clk tree in hardware to be consistent with what software can reason about.

Should we really ? Something the framework does not know about is not necessarily something wrong.

It would best if we could model the tree completly, but most of the time, we
just approximate it the best we can.

The framework just knows how to setup rates, it has no idea what rate needs to
be set or when - And I think it is best if does not make any assumption about
that.

Quoted 7 lines

Pushing it a bit, this unknown parent might important to the boot sequence.
How would you know when it is safe to change the setting ? What would change it to ? It Seems obvious when there is only one known parent, but what if there two ? which one do you pick ? Is it really the role
of CCF to make this kind of choice ?

Agreed. We can't mandate this requirement because of what you say. There are pitfalls such as not always knowing if it's safe to change the clk tree before clks are registered.

Quoted 10 lines

Quoted 6 lines

This also reminds me of some audio clks I've seen before where the default parent is some external pin and it can be reparented to an internal clk with clk_set_rate/parent. In that case, I think we forced the parent over to the internal clk before registering so that it would always get parented properly in the framework. Right now, this is going to cause problems for plans to probe defer clk_get() on orphan clks.

clk_get() on orphaned clock should probably return an error if it is not the
case already ? or 0 maybe ?

I'd prefer that it return EPROBE_DEFER if the clk is an orphan, except that it causes problems for this case where muxes move away from something unknown to something known.

Quoted 7 lines

Quoted 5 lines

Maybe this could work better if we allowed 'assigned-clock-parents/rates' to reparent clks regardless of orphan status and/or had some flag that could be set on clks to indicate that we're OK if clk_get() is called on it when it's an orphan because this isn't a problem?

Not sure I understand your point here.

There is a problem with assigned-clock DT properties and orphan clks and making orphan clks defer driver probes.

Quoted 11 lines

Quoted 10 lines

Or we can make the defer on orphan code only defer if the clk has a single parent and it's an orphan and return the clk if there is at least one parent of the clk that has been registered and isn't marked as an orphan. That would need to be recursively applied, so I guess we would update our orphan marking code to indicate that clk_get() on the clk should probe defer or not instead of indicating the clk's orphan status. Doing this would also side-step the problem Rockchip was having where certain parents never appeared but the clk was parented to it in hardware, essentially blocking the clk from ever being touched by consumers.

Quoted 1 line

... and now you lost me completly :)

Hmm alright.

Quoted 14 lines

Quoted 10 lines

On the other hand, not having the clk there causes us to do a global search of the clk name space each time we look for parents by the "unknown" index, which in the Rockchip case was quite often because the clk was changing between two other parents with a third one that never got registered. So it may be better to just register an "unknown" clk as a root clk with a rate of 0 that you can then hook everything up to that is hardware reset to this unknown input. That way nothing is orphaned and everyone is happy. We could do this for the audio case I talked about earlier too so when the external pin is left unconnected we could register some 'unconnected' clk and parent the audio clk to it.

We have sometimes a few orphans in the amlogic clock tree, it does not seems
to a problem.

I suppose drivers aren't calling clk_set_rate() on the orphaned clks and experiencing problems? There are problems around clk rates that could happen if there aren't any proper parents of a mux.

Quoted 3 lines

I don't really understand the benefit of having a fake clock that would adopt
all the orphan ? You guess there is problem I'm not aware of ...

I see three cases that can go wrong if we make orphan clks probe defer clk_get():

1) NULL as the parent name for some parent index that the hardware indicates is the current parent of the clk when the clk is registered.

In this case, we need to make clk_get() succeed and not return -EPROBE_DEFER, so I think we need to make a special "unknown" clk in the framework so that clks aren't orphaned when the hardware is indicating the parent is something we don't (and won't) know anything about.

2) get_parent/set_parent on a clk with num_parents == 1

This seems to be how ao_cts_cec is implemented. This hides the fact that get_parent may return some number above the number of parents

and then
has that quietly make the parent of the clk NULL during registration so that it's orphaned. Then we rely on set_rate() or set_parent() being called to fix the parent chain at a later time.

That is complicated and non-obvious. I'd rather that we specify NULL as a parent in the parent_names array if we're going to return that index to the framework and then have the framework go to case #1. This makes it easier for the framework to assume that [0, num_parents) is the valid range of indices returned from the get_parent() op and a number outside that range is an error, clearing the way for errors to be returned from get_parent() if it somehow fails.

3) A string as the parent name for some parent index that the hardware indicates is the current parent of the clk when the clk is registered, but that parent clk may never be registered with the framework.

I imagine this can happen with an external gpio clk that feeds some SoC internal clk. That gpio could be supplied by some clk signal that the board designer decides to send there, and it could also just "magically appear" by applying some DT overlay. Due to our use of global strings for parent linking, we don't have a good way to figure out that nothing is connected to some clock input signal, making this quite a nightmare to figure out a runtime!

One solution is to replace the parent name with NULL and ignore the runtime update case. This will work and bring us back to case #1. The problem is that if it is connected to something like a GPIO and it defaults to that pin instead of some internal clk we're stuck with an "orphan" clk that we can't clk_get(). I suppose the driver could read DT and see if this pin is not connected and then mark this input parent name as NULL. We can punt on solving the DT overlay problem until later, because we would need some way to dynamically resolve a new parent of a clk and update the tree when the DT node is modified to use a new GPIO.

Quoted 1 line

Sorry this email is long.

I tried reply the best I could to your use cases

Quoted 4 lines

TL;DR is that I don't think we need to revert the patch as you suggest. Instead, we should fix the driver to indicate NULL as the parent name of the index it tells the framework about.

The driver, can't indicate NULL, it just provides the index of the parent. What we are saying is not very far apart.

Drivers implementing get_parent() must have way to tell the framework "I have no idea what I'm connected to ATM"

Since get_parent() return a u8, the only way to signal a problem like this is to return an out of bound value, which the framework already safely understand and maps to NULL, as you request

This is done in the other patch I sent (... and now they are related ;) ...) by returning num_parents in the clk_mux code

This is an important case, whatever the number of parent. I think it makes no sense that a clock with 2 known parents can return this error, a one with only one known parent can't.

This why I still think the change should be reverted.

If you really insist on keeping it, there should at least be a warning when registering a clock with 'get_parent != NULL' and 'num_parents == 1', to let the user know that whatever is implemented there will be ignored ... instead of silently ignoring it. I think it would be a mistake, but at least there would be no suprise.

Earlier, you mention having a special parent that would adopt the the orphan. Actually, it already exists, it is the framework itself. If, instead of returning 0, it returned a special value to let us know we reached the root and there actually no signal, we would be easy to make sure the orphan (with no signal) are never accidently used.

Quoted 6 lines

Quoting Jerome Brunet (2018-12-05 09:20:09)

Quoted 3 lines

We have sometimes a few orphans in the amlogic clock tree, it does not seems
to a problem.

To the best of my knowledge, there will always be orphan in the tree. The root clock (usually an xtal) being a special case of this.

It seems to me that the problem is we can't differentiate between this special orphan and the rest because none will return an error when calling get_rate()

xtal will provide something to round_rate() while other clock which are supposed to be orphaned will defer to the framework which gives a 0 rate.

I think this is your real issue with orphan in the framework. The framework does not distinguish between a signal with rate == 0 and an error (no signal)

Quoted 9 lines

I suppose drivers aren't calling clk_set_rate() on the orphaned clks and experiencing problems? There are problems around clk rates that could happen if there aren't any proper parents of a mux.

Quoted 3 lines

I don't really understand the benefit of having a fake clock that would adopt
all the orphan ? You guess there is problem I'm not aware of ...

Quoted 5 lines

I see three cases that can go wrong if we make orphan clks probe defer clk_get():

1) NULL as the parent name for some parent index that the hardware indicates is the current parent of the clk when the clk is registered.

I don't think you can have NULL has a parent_name

Quoted 6 lines

In this case, we need to make clk_get() succeed and not return -EPROBE_DEFER, so I think we need to make a special "unknown" clk in the framework so that clks aren't orphaned when the hardware is indicating the parent is something we don't (and won't) know anything about.

I think DEFER is returned when clock is unknown, not when it is orphaned.

Quoted 6 lines

2) get_parent/set_parent on a clk with num_parents == 1

This seems to be how ao_cts_cec is implemented. This hides the fact that get_parent may return some number above the number of parents

The index returned will be either 0, to indicate the only known parent, or num_parents (1) to indicate an invalid parent to framework. This is the topic of the other patch I sent you

You must have way for a driver to tell you that it is not connected to anything it knows. That's a very basic error case and this is not specific to num_parents == 1.

With any number of num_parent, there is still a possibility that a driver ends up in configuration where it cannot tell what it is his parent. (invalid combination, reserved values, crappy doc ...)

This is why is important to call get_parent() (if available) even when num_parent == 1. The driver might tell if it is connected as expected - or not

Quoted 6 lines

and then
has that quietly make the parent of the clk NULL during registration so that it's orphaned. Then we rely on set_rate() or set_parent() being called to fix the parent chain at a later time.

Quoted 7 lines

That is complicated and non-obvious. I'd rather that we specify NULL as a parent in the parent_names array if we're going to return that index to the framework and then have the framework go to case #1. This makes it easier for the framework to assume that [0, num_parents) is the valid range of indices returned from the get_parent() op and a number outside that range is an error, clearing the way for errors to be returned from get_parent() if it somehow fails.

The fix I sent is quite simple.

The above would require that:
1) the framework accept NULL in the list of parent (which it forbids ATM AFAIK)
2) Have all drivers account for this a create this fake parent

This seems to be putting more work on the drivers with no real gain for the framework, none that I can see at least.

Quoted 13 lines

3) A string as the parent name for some parent index that the hardware indicates is the current parent of the clk when the clk is registered, but that parent clk may never be registered with the framework.

I imagine this can happen with an external gpio clk that feeds some SoC internal clk. That gpio could be supplied by some clk signal that the board designer decides to send there, and it could also just "magically appear" by applying some DT overlay. Due to our use of global strings for parent linking, we don't have a good way to figure out that nothing is connected to some clock input signal, making this quite a nightmare to figure out a runtime!

Quoted 9 lines

One solution is to replace the parent name with NULL and ignore the runtime update case. This will work and bring us back to case #1. The problem is that if it is connected to something like a GPIO and it defaults to that pin instead of some internal clk we're stuck with an "orphan" clk that we can't clk_get(). I suppose the driver could read DT and see if this pin is not connected and then mark this input parent name as NULL. We can punt on solving the DT overlay problem until later, because we would need some way to dynamically resolve a new parent of a clk and update the tree when the DT node is modified to use a new GPIO.

I don't understand what is the problem here.
In Amlogic we have a lot of this cases, with optional input clocks of the axg- audio clock controller. Many muxes have parent names of clock that don't exists.

CCF will return a 0 rate for these inputs.
It works well as it is, but as said above, I think it would be better if we could distinguish between a signal with 0 rate and no signal at all (error)

Quoting Jerome Brunet (2018-12-07 02:03:19)

Quoted 16 lines · Stephen Boyd

Quoted 1 line

Sorry this email is long.

I tried reply the best I could to your use cases

Quoted 3 lines

TL;DR is that I don't think we need to revert the patch as you suggest. Instead, we should fix the driver to indicate NULL as the parent name of the index it tells the framework about.

The driver, can't indicate NULL, it just provides the index of the parent. What we are saying is not very far apart.

Drivers implementing get_parent() must have way to tell the framework "I have
no idea what I'm connected to ATM"

Quoted 5 lines

Since get_parent() return a u8, the only way to signal a problem like this is
to return an out of bound value, which the framework already safely understand
and maps to NULL, as you request

I'm saying that the clk with two parents but only one 'known' clk parent should have a parent_names array of two strings, where the second element is NULL.

	.parent_names = { "known_clk", NULL }
	.num_parents = 2,

And then the .get_parent op is OK to return '1' and then we know that the clk isn't known to be anything, but it's otherwise a valid index for the clk to be parented to. This is the orphan case. It's not the error case, which would be some value returned from .get_parent() that's >= num_parents.

I am not talking about the return value from get_parent() returning NULL or the driver indicating that the parent is a NULL clk. I'm just saying that the clk driver that wants to return an index that's outside the bounds of the parent_names array needs to do the right thing and either grow the parent_names array to say "it's unknown what's here, I've indicated that with a NULL name", or it needs to return another value outside of the array so the framework knows it's an error.

Quoted 12 lines

This is done in the other patch I sent (... and now they are related ;) ...)
by returning num_parents in the clk_mux code

This is an important case, whatever the number of parent. I think it makes no
sense that a clock with 2 known parents can return this error, a one with only
one known parent can't.
This why I still think the change should be reverted.

Quoted 8 lines

If you really insist on keeping it, there should at least be a warning when
registering a clock with 'get_parent != NULL' and 'num_parents == 1', to let
the user know that whatever is implemented there will be ignored ... instead
of silently ignoring it. I think it would be a mistake, but at least there would be no suprise.

Maybe we should add the warning. At least it would be good to grep around and try to find these cases and update them. The documentation on the get_parent clk op says it's unnecessary right now when 0 or 1 parent exists, so maybe that needs an update too.

Quoted 9 lines

Earlier, you mention having a special parent that would adopt the the orphan.
Actually, it already exists, it is the framework itself. If, instead of returning 0, it returned a special value to let us know we reached the root
and there actually no signal, we would be easy to make sure the orphan (with
no signal) are never accidently used.

I don't understand this part. We have orphan tracking code that knows when a clk is no longer in an orphan chain (i.e. there is a signal there).

Quoted 9 lines

Quoted 4 lines

Quoting Jerome Brunet (2018-12-05 09:20:09)

Quoted 3 lines

We have sometimes a few orphans in the amlogic clock tree, it does not seems
to a problem.

To the best of my knowledge, there will always be orphan in the tree. The root
clock (usually an xtal) being a special case of this.

Quoted 4 lines

It seems to me that the problem is we can't differentiate between this special
orphan and the rest because none will return an error when calling get_rate()

The root clk isn't an orphan. It has num_parents == 0 and is treated specially as such.

Quoted 8 lines

xtal will provide something to round_rate() while other clock which are supposed to be orphaned will defer to the framework which gives a 0 rate.

I think this is your real issue with orphan in the framework. The framework
does not distinguish between a signal with rate == 0 and an error (no signal)

Ok. I'm not sure that rate is the entire problem, but I suppose it's one problem.

Quoted 16 lines

Quoted 15 lines

I suppose drivers aren't calling clk_set_rate() on the orphaned clks and experiencing problems? There are problems around clk rates that could happen if there aren't any proper parents of a mux.

Quoted 4 lines

I don't really understand the benefit of having a fake clock that would
adopt
all the orphan ? You guess there is problem I'm not aware of ...

I see three cases that can go wrong if we make orphan clks probe defer clk_get():

1) NULL as the parent name for some parent index that the hardware indicates is the current parent of the clk when the clk is
registered.

Quoted 1 line

I don't think you can have NULL has a parent_name

Yes, it looks like Mike made that decision when introducing the CCF in 2012. I don't know why it is a problem though. Maybe kstrdup() on a NULL string returns NULl and then errors can't be detected? Should be simple enough to skip those ones when doing the deep copy though.

Quoted 7 lines

Quoted 5 lines

In this case, we need to make clk_get() succeed and not return -EPROBE_DEFER, so I think we need to make a special "unknown" clk in the framework so that clks aren't orphaned when the hardware is indicating the parent is something we don't (and won't) know anything about.

I think DEFER is returned when clock is unknown, not when it is orphaned.

Yes it's only returned right now if the provider node hasn't registered a provider function. We have wanted to extend that further so that clks can't be acquired until the clk is properly rooted in the clk tree instead of being orphaned. It may not matter on amlogic but it matters on other platforms.

Quoted 20 lines

Quoted 4 lines

2) get_parent/set_parent on a clk with num_parents == 1

This seems to be how ao_cts_cec is implemented. This hides the fact that get_parent may return some number above the number of parents

The index returned will be either 0, to indicate the only known parent, or num_parents (1) to indicate an invalid parent to framework. This is the topic
of the other patch I sent you

You must have way for a driver to tell you that it is not connected to anything it knows. That's a very basic error case and this is not specific to
num_parents == 1.

With any number of num_parent, there is still a possibility that a driver ends
up in configuration where it cannot tell what it is his parent. (invalid combination, reserved values, crappy doc ...)

Quoted 3 lines

This is why is important to call get_parent() (if available) even when num_parent == 1. The driver might tell if it is connected as expected - or not

Ok. Thanks for the explanation. What should the framework do when the parent is not known? Mark the clk as not an orphan but parent it to NULL? That might work here.

Quoted 54 lines

Quoted 6 lines

and then
has that quietly make the parent of the clk NULL during registration so that it's orphaned. Then we rely on set_rate() or set_parent() being called to fix the parent chain at a later time.

Quoted 7 lines

That is complicated and non-obvious. I'd rather that we specify NULL as a parent in the parent_names array if we're going to return that index to the framework and then have the framework go to case #1. This makes it easier for the framework to assume that [0, num_parents) is the valid range of indices returned from the get_parent() op and a number outside that range is an error, clearing the way for errors to be returned from get_parent() if it somehow fails.

The fix I sent is quite simple.

The above would require that:
1) the framework accept NULL in the list of parent (which it forbids ATM AFAIK)
2) Have all drivers account for this a create this fake parent

This seems to be putting more work on the drivers with no real gain for the
framework, none that I can see at least.

Quoted 13 lines

3) A string as the parent name for some parent index that the hardware indicates is the current parent of the clk when the clk is
registered,
but that parent clk may never be registered with the framework.

I imagine this can happen with an external gpio clk that feeds some SoC internal clk. That gpio could be supplied by some clk signal that the board designer decides to send there, and it could also just "magically appear" by applying some DT overlay. Due to our use of global strings for parent linking, we don't have a good way to figure out that nothing is connected to some clock input signal, making this quite a nightmare to figure out a runtime!

Quoted 9 lines

One solution is to replace the parent name with NULL and ignore the runtime update case. This will work and bring us back to case #1. The problem is that if it is connected to something like a GPIO and it defaults to that pin instead of some internal clk we're stuck with an "orphan" clk that we can't clk_get(). I suppose the driver could read DT and see if this pin is not connected and then mark this input parent name as NULL. We can punt on solving the DT overlay problem until later, because we would need some way to dynamically resolve a new parent of a clk and update the tree when the DT node is modified to use a new GPIO.

I don't understand what is the problem here.
In Amlogic we have a lot of this cases, with optional input clocks of the axg-
audio clock controller. Many muxes have parent names of clock that don't exists.

Quoted 6 lines

CCF will return a 0 rate for these inputs.
It works well as it is, but as said above, I think it would be better if we
could distinguish between a signal with 0 rate and no signal at all (error)

Ok. It sounds like we want similar things but I'm worried about the orphan probe defer logic getting messed up.

How about we tri-state the get_parent() return value? So now [0, num_parents) means a valid parent is known, num_parents means "I don't know what it is but it must be something valid", and [num_parents + 1, 255] means some error occurred. Sounds obfuscated to me still.

Alternatively, we could make a get_parent_hw() op that returns a pointer to a clk_hw structure of the parent clk. Then drivers can be converted to use this new clk_op instead of get_parent and we can have it return an error pointer when some error happens, NULL when the parent is not known to the software but is otherwise valid, and the parent pointer when the parent is findable. It would still be tri-stated then, but at least it wouldn't be some odd u8 numberspace that it all has to fit into. This patch starts to do that. What do you think? Clk providers would need to implement a clk_ops that mostly does clk_hw_get_parent_by_index(), or they could stash the parent_hw pointer in their structure, or index into it based on some global numberspace in their driver.

-----8<----

drivers/clk/clk.c +45 -3

index af011974d4ec..5fe3c41e0b7b 100644
@@ -2242,14 +2242,84 @@ struct clk *clk_get_parent(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_get_parent);
 
-static struct clk_core *__clk_init_parent(struct clk_core *core)
+static struct clk_core *
+__clk_init_parent(struct clk_core *core, bool update_orphan)
 {
 	u8 index = 0;
+	struct clk_hw *parent_hw = NULL;
 
-	if (core->num_parents > 1 && core->ops->get_parent)
-		index = core->ops->get_parent(core->hw);
+	if (core->ops->get_parent_hw) {
+		parent_hw = core->ops->get_parent_hw(core->hw);
+		/*
+		 * The provider driver doesn't know what the parent is,
+		 * but it's at least something valid, so it's not an
+		 * orphan, just a clk with some unknown parent.
+		 */
+		if (!parent_hw && update_orphan)
+			core->orphan = false;
+	} else {
+		if (core->num_parents > 1 && core->ops->get_parent)
+			index = core->ops->get_parent(core->hw);
+
+		parent_hw = clk_hw_get_parent_by_index(core->hw, index);
+	}
+
+	if (IS_ERR(parent_hw)) {
+		/* Parent clk provider hasn't probed yet, orphan it */
+		if (PTR_ERR(parent_hw) == -EPROBE_DEFER) {
+			if (update_orphan)
+				core->orphan = true;
+
+			return NULL;
+		}
+
+		return ERR_CAST(parent_hw);
+	}
+
+	if (!parent_hw)
+		return NULL;
+
+	return parent_hw->core;
+}
+
+static int clk_init_parent(struct clk_core *core)
+{
+	core->parent = __clk_init_parent(core, true);
+	if (IS_ERR(core->parent))
+		return PTR_ERR(core->parent);
+
+	/*
+	 * Populate core->parent if parent has already been clk_core_init'd.

If

drivers/clk/clk.c +1 -0

+	 * parent has not yet been clk_core_init'd then place clk in the

orphan

drivers/clk/clk.c +30 -3

+	 * list.  If clk doesn't have any parents then place it in the root
+	 * clk list.
+	 *
+	 * Every time a new clk is clk_init'd then we walk the list of orphan
+	 * clocks and re-parent any that are children of the clock currently
+	 * being clk_init'd.
+	 */
+	if (core->parent) {
+		hlist_add_head(&core->child_node,
+				&core->parent->children);
+		core->orphan = core->parent->orphan;
+	} else if (!core->num_parents) {
+		hlist_add_head(&core->child_node, &clk_root_list);
+		core->orphan = false;
+	} else {
+		hlist_add_head(&core->child_node, &clk_orphan_list);
+	}
+
+	return 0;
+}
 
-	return clk_core_get_parent_by_index(core, index);
+static struct clk_core *clk_find_parent(struct clk_core *core)
+{
+	return __clk_init_parent(core, false);
+}
+
+static bool clk_has_parent_op(const struct clk_ops *ops)
+{
+	return ops->get_parent || ops->get_parent_hw;
 }
 
 static void clk_core_reparent(struct clk_core *core,

@@ -3045,14 +3115,14 @@ static int __clk_core_init(struct clk_core *core)
 		goto out;
 	}
 
-	if (core->ops->set_parent && !core->ops->get_parent) {
+	if (core->ops->set_parent && !clk_has_parent_op(core->ops)) {
 		pr_err("%s: %s must implement .get_parent & .set_parent\n",
 		       __func__, core->name);
 		ret = -EINVAL;
 		goto out;
 	}
 
-	if (core->num_parents > 1 && !core->ops->get_parent) {
+	if (core->num_parents > 1 && !clk_has_parent_op(core->ops)) {
 		pr_err("%s: %s must implement .get_parent as it has multi
 parents\n",
 		       __func__, core->name);

drivers/clk/clk.c +0 -4

 		ret = -EINVAL;

@@ -3073,29 +3143,9 @@ static int __clk_core_init(struct clk_core *core)
 				"%s: invalid NULL in %s's .parent_names\n",
 				__func__, core->name);
 
-	core->parent = __clk_init_parent(core);
-
-	/*
-	 * Populate core->parent if parent has already been clk_core_init'd.

If

drivers/clk/clk.c +0 -1

-	 * parent has not yet been clk_core_init'd then place clk in the

orphan

drivers/clk/clk.c +11 -19

-	 * list.  If clk doesn't have any parents then place it in the root
-	 * clk list.
-	 *
-	 * Every time a new clk is clk_init'd then we walk the list of orphan
-	 * clocks and re-parent any that are children of the clock currently
-	 * being clk_init'd.
-	 */
-	if (core->parent) {
-		hlist_add_head(&core->child_node,
-				&core->parent->children);
-		core->orphan = core->parent->orphan;
-	} else if (!core->num_parents) {
-		hlist_add_head(&core->child_node, &clk_root_list);
-		core->orphan = false;
-	} else {
-		hlist_add_head(&core->child_node, &clk_orphan_list);
-		core->orphan = true;
-	}
+	ret = clk_init_parent(core);
+	if (ret)
+		goto out;
 
 	/*
 	 * optional platform-specific magic

@@ -3173,7 +3223,14 @@ static int __clk_core_init(struct clk_core *core)
 	 * parent.
 	 */
 	hlist_for_each_entry_safe(orphan, tmp2, &clk_orphan_list, child_node)
 {
-		struct clk_core *parent = __clk_init_parent(orphan);
+		struct clk_core *parent = clk_find_parent(orphan);
+
+		/*
+		 * Error parent should have been caught before and returned
+		 * as an error during registration.
+		 */
+		if (WARN_ON_ONCE(IS_ERR(parent)))
+			continue;
 
 		/*

drivers/clk/clk.c +0 -0

 		 * We need to use __clk_set_parent_before() and _after() to

include/linux/clk-provider.h +1 -0

index 60c51871b04b..8b84dee942bf 100644
@@ -155,6 +155,14 @@ struct clk_duty {
  *		multiple parents.  It is optional (and unnecessary) for clocks
  *		with 0 or 1 parents.
  *
+ * @get_parent_hw: Queries the hardware to determine the parent of a

clock. The
+ * return value is a clk_hw pointer corresponding to
+ * the parent clock. In short, this function translates the
parent
+ * value read from hardware into a pointer to the clk_hw for that
clk.
+ * Currently only called when the clock is initialized by
+ * __clk_init. This callback is mandatory for clocks with
+ * multiple parents. It is optional for clocks with 0 or 1
parents.
+ *
* @set_rate: Change the rate of this clock. The requested rate is
specified
* by the second argument, which should typically be the return
* of .round_rate call. The third argument gives the parent rate

include/linux/clk-provider.h +1 -0

@@ -238,6 +246,7 @@ struct clk_ops {
 					  struct clk_rate_request *req);
 	int		(*set_parent)(struct clk_hw *hw, u8 index);
 	u8		(*get_parent)(struct clk_hw *hw);
+	struct clk_hw * (*get_parent_hw)(struct clk_hw *hw);
 	int		(*set_rate)(struct clk_hw *hw, unsigned long rate,
 				    unsigned long parent_rate);
 	int		(*set_rate_and_parent)(struct clk_hw *hw,

Quoted 4 lines

Ok. Thanks for the explanation. What should the framework do when the parent is not known? Mark the clk as not an orphan but parent it to NULL? That might work here.

I don't really get the difference in the framework ATM. Is this linked to the 'orphan probe defer logic' WIP you have been mentionning ?

All this below is very interresting but it looks like a very complex way to solve what was initially a very simple problem, with considerations that are still WIP a this stage.

Even with these future constraints in mind, I don't understand how the proposed patch changes anything to the situtation. A clock with a get_parent() callback (and several known parents) may still return an out of bound value ... CCF will still have to deal with this case gracefully, as it is currently doing

This driver creates a gate clk with the possibility to have multiple parents. That can cause problems if the common clk framework tries to call the get_parent() op and gets back a number that's larger than the number of parents the clk says it supports in clk_init_data::num_parents. Let's duplicate the clk_ops structure each time this function is called and drop the get/set parent ops when there is only one parent. This allows the framework to consider a number larger than clk_init_data::num_parents as an error condition of the get_parent() clk op, clearing the way for proper code.

Cc Jerome Brunet <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Dinh Nguyen <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/clk/socfpga/clk-gate.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

drivers/clk/socfpga/clk-gate.c +13 -9

index aa7a6e6a15b6..73e03328d5c5 100644
@@ -176,8 +176,7 @@ static struct clk_ops gateclk_ops = {
        .set_parent = socfpga_clk_set_parent,
 };
 
-static void __init __socfpga_gate_init(struct device_node *node,
-       const struct clk_ops *ops)
+void __init socfpga_gate_init(struct device_node *node)
 {
        u32 clk_gate[2];
        u32 div_reg[3];

@@ -188,12 +187,17 @@ static void __init __socfpga_gate_init(struct device_node *node,
        const char *clk_name = node->name;
        const char *parent_name[SOCFPGA_MAX_PARENTS];
        struct clk_init_data init;
+       struct clk_ops *ops;
        int rc;
 
        socfpga_clk = kzalloc(sizeof(*socfpga_clk), GFP_KERNEL);
        if (WARN_ON(!socfpga_clk))
                return;
 
+       ops = kmemdup(&gateclk_ops, sizeof(gateclk_ops), GFP_KERNEL);
+       if (WARN_ON(!ops))
+               return;
+
        rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
        if (rc)
                clk_gate[0] = 0;

@@ -202,8 +206,8 @@ static void __init __socfpga_gate_init(struct device_node *node,
                socfpga_clk->hw.reg = clk_mgr_base_addr + clk_gate[0];
                socfpga_clk->hw.bit_idx = clk_gate[1];
 
-               gateclk_ops.enable = clk_gate_ops.enable;
-               gateclk_ops.disable = clk_gate_ops.disable;
+               ops->enable = clk_gate_ops.enable;
+               ops->disable = clk_gate_ops.disable;
        }
 
        rc = of_property_read_u32(node, "fixed-divider", &fixed_div);

@@ -234,6 +238,11 @@ static void __init __socfpga_gate_init(struct device_node *node,
        init.flags = 0;
 
        init.num_parents = of_clk_parent_fill(node, parent_name, SOCFPGA_MAX_PARENTS);
+       if (init.num_parents < 2) {
+               ops->get_parent = NULL;
+               ops->set_parent = NULL;
+       }
+
        init.parent_names = parent_name;
        socfpga_clk->hw.hw.init = &init;
 

@@ -246,8 +255,3 @@ static void __init __socfpga_gate_init(struct device_node *node,
        if (WARN_ON(rc))
                return;
 }
-
-void __init socfpga_gate_init(struct device_node *node)
-{
-       __socfpga_gate_init(node, &gateclk_ops);
-}

(Adding Dinh's korg email)

I also wonder if this driver is even used anymore or maybe we can delete it?

Hi Stephen,

Quoted 5 lines · Stephen Boyd

(Adding Dinh's korg email)

I also wonder if this driver is even used anymore or maybe we can delete it?

The armv7 SoCFPGA platforms are using this driver.

Quoting Dinh Nguyen (2018-12-05 07:17:41)

Quoted 9 lines

Hi Stephen,

Quoted 5 lines · Stephen Boyd

(Adding Dinh's korg email)

I also wonder if this driver is even used anymore or maybe we can delete it?

Quoted 2 lines

The armv7 SoCFPGA platforms are using this driver.

Ok and those platforms are booting in kernelci.org as
https://kernelci.org/boot/socfpga_cyclone5_de0_sockit/ perhaps?

Quoted 10 lines

Quoting Dinh Nguyen (2018-12-05 07:17:41)

Quoted 9 lines

Hi Stephen,

Quoted 5 lines · Stephen Boyd

(Adding Dinh's korg email)

I also wonder if this driver is even used anymore or maybe we can delete it?

Quoted 6 lines

Quoted 2 lines

The armv7 SoCFPGA platforms are using this driver.

Ok and those platforms are booting in kernelci.org as
https://kernelci.org/boot/socfpga_cyclone5_de0_sockit/ perhaps?

Yes that's right.

Quoting Dinh Nguyen (2018-12-06 07:16:47)

Quoted 20 lines · Stephen Boyd

Quoted 16 lines

Quoting Dinh Nguyen (2018-12-05 07:17:41)

Quoted 11 lines

Hi Stephen,

Quoted 5 lines · Stephen Boyd

(Adding Dinh's korg email)

I also wonder if this driver is even used anymore or maybe we can delete it?

The armv7 SoCFPGA platforms are using this driver.

Ok and those platforms are booting in kernelci.org as
https://kernelci.org/boot/socfpga_cyclone5_de0_sockit/ perhaps?

Quoted 2 lines

Yes that's right.

Ok. Dinh, can you review this patch? Or test it out? Or do I need to get someone in kernelci to try it out?