[PATCH] clk: Don't show the incorrect clock phase

[PATCH] clk: Don't show the incorrect clock phase

[Shawn Lin]

It's found that the clock phase output from clk_summary is
wrong compared to the actual phase reading from the register.

cat /sys/kernel/debug/clk/clk_summary | grep sdio_sample
sdio_sample     0        1        0 50000000          0 -22

It expose an issue that clk core, clk_core_get_phase, always
return the cached core->phase which should be either updated
by calling clk_set_phase or directly from the first place the
clk was registered. When registering the clk, the core->phase
geting from ->get_phase() may return negative value indicating
error. This is quite common since the clk's phase may be highly
related to its praent chain, but it was temporary orphan when
registered, since its parent chain hadn't be ready at that time,
so the clk drivers decide to return error in this case. However,
if no clk_set_phase is called, core->phase would never be updated.
This is wrong, and we should try to update it when all its parent
chain is settled down, like the way of updating clock rate for that.
It's not derserved to complicate the code but just update it if
seeing it's a nagative number when calling clk_core_get_phase, which
would be much simple and enough.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/clk/clk.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0f686a9..b49e4c8 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2370,6 +2370,15 @@ static int clk_core_get_phase(struct clk_core *core)
 	int ret;
 
 	clk_prepare_lock();
+	/*
+	 * clk_set_phase always set the phase in [0, 360],
+	 * so the only reason for core->phase to be nagative
+	 * number is it's a error number propagated back from
+	 * the clk drivers. So we should try to update it if
+	 * possible.
+	 */
+	if (core->phase < 0 && core->ops->get_phase)
+		core->phase = core->ops->get_phase(core->hw);
 	ret = core->phase;
 	clk_prepare_unlock();
 
-- 
1.9.1

Re: [PATCH] clk: Don't show the incorrect clock phase

[Geert Uytterhoeven]

On Thu, Mar 8, 2018 at 8:54 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> It's found that the clock phase output from clk_summary is
> wrong compared to the actual phase reading from the register.
>
> cat /sys/kernel/debug/clk/clk_summary | grep sdio_sample
> sdio_sample     0        1        0 50000000          0 -22
>
> It expose an issue that clk core, clk_core_get_phase, always

exposes

> return the cached core->phase which should be either updated
> by calling clk_set_phase or directly from the first place the
> clk was registered. When registering the clk, the core->phase
> geting from ->get_phase() may return negative value indicating
> error. This is quite common since the clk's phase may be highly
> related to its praent chain, but it was temporary orphan when

parent

> registered, since its parent chain hadn't be ready at that time,
> so the clk drivers decide to return error in this case. However,
> if no clk_set_phase is called, core->phase would never be updated.
> This is wrong, and we should try to update it when all its parent
> chain is settled down, like the way of updating clock rate for that.

chains are settled

> It's not derserved to complicate the code but just update it if

deserved

> seeing it's a nagative number when calling clk_core_get_phase, which

negative

> would be much simple and enough.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
>  drivers/clk/clk.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0f686a9..b49e4c8 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2370,6 +2370,15 @@ static int clk_core_get_phase(struct clk_core *core)
>         int ret;
>
>         clk_prepare_lock();
> +       /*
> +        * clk_set_phase always set the phase in [0, 360],
> +        * so the only reason for core->phase to be nagative

a negative

> +        * number is it's a error number propagated back from

an error

> +        * the clk drivers. So we should try to update it if
> +        * possible.
> +        */
> +       if (core->phase < 0 && core->ops->get_phase)
> +               core->phase = core->ops->get_phase(core->hw);

Can this happen if core->ops->get_phase does not exist?
If yes, we're stuck with a negative number forever?

>         ret = core->phase;
>         clk_prepare_unlock();
>
> --
> 1.9.1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] clk: Don't show the incorrect clock phase

[Shawn Lin]

Hi Geert,

On 2018/3/8 17:40, Geert Uytterhoeven wrote:
> On Thu, Mar 8, 2018 at 8:54 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> It's found that the clock phase output from clk_summary is
>> wrong compared to the actual phase reading from the register.
>>
>> cat /sys/kernel/debug/clk/clk_summary | grep sdio_sample
>> sdio_sample     0        1        0 50000000          0 -22
>>
>> It expose an issue that clk core, clk_core_get_phase, always
> 
> exposes
> 
>> return the cached core->phase which should be either updated
>> by calling clk_set_phase or directly from the first place the
>> clk was registered. When registering the clk, the core->phase
>> geting from ->get_phase() may return negative value indicating
>> error. This is quite common since the clk's phase may be highly
>> related to its praent chain, but it was temporary orphan when
> 
> parent
> 
>> registered, since its parent chain hadn't be ready at that time,
>> so the clk drivers decide to return error in this case. However,
>> if no clk_set_phase is called, core->phase would never be updated.
>> This is wrong, and we should try to update it when all its parent
>> chain is settled down, like the way of updating clock rate for that.
> 
> chains are settled
> 
>> It's not derserved to complicate the code but just update it if
> 
> deserved
> 
>> seeing it's a nagative number when calling clk_core_get_phase, which
> 
> negative
> 
>> would be much simple and enough.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>   drivers/clk/clk.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 0f686a9..b49e4c8 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2370,6 +2370,15 @@ static int clk_core_get_phase(struct clk_core *core)
>>          int ret;
>>
>>          clk_prepare_lock();
>> +       /*
>> +        * clk_set_phase always set the phase in [0, 360],
>> +        * so the only reason for core->phase to be nagative
> 
> a negative
> 
>> +        * number is it's a error number propagated back from
> 
> an error
> 
>> +        * the clk drivers. So we should try to update it if
>> +        * possible.
>> +        */
>> +       if (core->phase < 0 && core->ops->get_phase)
>> +               core->phase = core->ops->get_phase(core->hw);
> 
> Can this happen if core->ops->get_phase does not exist?

Ahh, nope.

> If yes, we're stuck with a negative number forever?

If core->ops->get_phase doesn't exist, it should be zero after
calling clk_register, and only got updated by ->set_phase().
So if 'core->phase < 0' happens, it implies core->ops->get_phase
must exists. But Jerome has a good point, let's move there to see
what is needed here.


> 
>>          ret = core->phase;
>>          clk_prepare_unlock();
>>
>> --
>> 1.9.1
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
> 
> 
> 

Re: [PATCH] clk: Don't show the incorrect clock phase

[Jerome Brunet]

On Thu, 2018-03-08 at 15:54 +0800, Shawn Lin wrote:
> It's found that the clock phase output from clk_summary is
> wrong compared to the actual phase reading from the register.
> 
> cat /sys/kernel/debug/clk/clk_summary | grep sdio_sample
> sdio_sample     0        1        0 50000000          0 -22
> 
> It expose an issue that clk core, clk_core_get_phase, always
> return the cached core->phase which should be either updated
> by calling clk_set_phase or directly from the first place the
> clk was registered. When registering the clk, the core->phase
> geting from ->get_phase() may return negative value indicating
> error. This is quite common since the clk's phase may be highly
> related to its praent chain, but it was temporary orphan when
> registered, since its parent chain hadn't be ready at that time,
> so the clk drivers decide to return error in this case. However,
> if no clk_set_phase is called, core->phase would never be updated.
> This is wrong, and we should try to update it when all its parent
> chain is settled down, like the way of updating clock rate for that.
> It's not derserved to complicate the code but just update it if
> seeing it's a nagative number when calling clk_core_get_phase, which
> would be much simple and enough.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  drivers/clk/clk.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0f686a9..b49e4c8 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2370,6 +2370,15 @@ static int clk_core_get_phase(struct clk_core *core)
>  	int ret;
>  
>  	clk_prepare_lock();
> +	/*
> +	 * clk_set_phase always set the phase in [0, 360],
> +	 * so the only reason for core->phase to be nagative
> +	 * number is it's a error number propagated back from
> +	 * the clk drivers. So we should try to update it if
> +	 * possible.
> +	 */
> +	if (core->phase < 0 && core->ops->get_phase)

Why bother with core->phase < 0 ?
if core->ops->get_phase is available, why not call it anyway ?

The phase may have changed since it was cached.

A typical example would be phases based on adding fixed delays:

* Take a 200MHz clock and set phase to 180 => a 2.5ns delay is set
* Clock rate change to 100MHz but delay is still 2.5ns
  -> cached phase = 180
  -> actual phase = 90

> +		core->phase = core->ops->get_phase(core->hw);
>  	ret = core->phase;
>  	clk_prepare_unlock();
>  

Re: [PATCH] clk: Don't show the incorrect clock phase

[Shawn Lin]

+ Geert

On 2018/3/8 18:04, Jerome Brunet wrote:
> On Thu, 2018-03-08 at 15:54 +0800, Shawn Lin wrote:
>> It's found that the clock phase output from clk_summary is
>> wrong compared to the actual phase reading from the register.
>>
>> cat /sys/kernel/debug/clk/clk_summary | grep sdio_sample
>> sdio_sample     0        1        0 50000000          0 -22
>>
>> It expose an issue that clk core, clk_core_get_phase, always
>> return the cached core->phase which should be either updated
>> by calling clk_set_phase or directly from the first place the
>> clk was registered. When registering the clk, the core->phase
>> geting from ->get_phase() may return negative value indicating
>> error. This is quite common since the clk's phase may be highly
>> related to its praent chain, but it was temporary orphan when
>> registered, since its parent chain hadn't be ready at that time,
>> so the clk drivers decide to return error in this case. However,
>> if no clk_set_phase is called, core->phase would never be updated.
>> This is wrong, and we should try to update it when all its parent
>> chain is settled down, like the way of updating clock rate for that.
>> It's not derserved to complicate the code but just update it if
>> seeing it's a nagative number when calling clk_core_get_phase, which
>> would be much simple and enough.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> ---
>>
>>   drivers/clk/clk.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 0f686a9..b49e4c8 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2370,6 +2370,15 @@ static int clk_core_get_phase(struct clk_core *core)
>>   	int ret;
>>   
>>   	clk_prepare_lock();
>> +	/*
>> +	 * clk_set_phase always set the phase in [0, 360],
>> +	 * so the only reason for core->phase to be nagative
>> +	 * number is it's a error number propagated back from
>> +	 * the clk drivers. So we should try to update it if
>> +	 * possible.
>> +	 */
>> +	if (core->phase < 0 && core->ops->get_phase)
> 
> Why bother with core->phase < 0 ?
> if core->ops->get_phase is available, why not call it anyway ?
> 
> The phase may have changed since it was cached.
> 
> A typical example would be phases based on adding fixed delays:
> 
> * Take a 200MHz clock and set phase to 180 => a 2.5ns delay is set
> * Clock rate change to 100MHz but delay is still 2.5ns
>    -> cached phase = 180
>    -> actual phase = 90

Good point!

(1)So either we should update phase once its rate is updated, for
instance, doing it in clk_change_rate, or

(2)we should always try to get the actual phase here.

If we do anyway call ->get_phase, we should never need core->phase since
it doesn't reflect the actual phase maybe.

Am I right? If yes, which option is more reasonable?


> 
>> +		core->phase = core->ops->get_phase(core->hw);
>>   	ret = core->phase;
>>   	clk_prepare_unlock();
>>   
> 
> 
> 
> 

Re: [PATCH] clk: Don't show the incorrect clock phase

[Jerome Brunet]

On Thu, 2018-03-08 at 19:28 +0800, Shawn Lin wrote:
> > > +    if (core->phase < 0 && core->ops->get_phase)
> > 
> > Why bother with core->phase < 0 ?
> > if core->ops->get_phase is available, why not call it anyway ?
> > 
> > The phase may have changed since it was cached.
> > 
> > A typical example would be phases based on adding fixed delays:
> > 
> > * Take a 200MHz clock and set phase to 180 => a 2.5ns delay is set
> > * Clock rate change to 100MHz but delay is still 2.5ns
> >     -> cached phase = 180
> >     -> actual phase = 90
> 
> Good point!
> 
> (1)So either we should update phase once its rate is updated, for
> instance, doing it in clk_change_rate, or

I don't think this would be a good option. The phase depending on the rate is
just one example. I'm pretty sure one could come up with another example 

> 
> (2)we should always try to get the actual phase here.

If the callback is available, lets use it. I prefer this second option

> 
> If we do anyway call ->get_phase, we should never need core->phase since
> it doesn't reflect the actual phase maybe.

The cached value is used by debugfs so it is still a valuable information. I
would keep it around for now.

> 
> Am I right? If yes, which option is more reasonable?

Re: [PATCH] clk: Don't show the incorrect clock phase

[Jerome Brunet]

On Thu, 2018-03-08 at 12:37 +0100, Jerome Brunet wrote:
> On Thu, 2018-03-08 at 19:28 +0800, Shawn Lin wrote:
> > > > +    if (core->phase < 0 && core->ops->get_phase)
> > > 
> > > Why bother with core->phase < 0 ?
> > > if core->ops->get_phase is available, why not call it anyway ?
> > > 
> > > The phase may have changed since it was cached.
> > > 
> > > A typical example would be phases based on adding fixed delays:
> > > 
> > > * Take a 200MHz clock and set phase to 180 => a 2.5ns delay is set
> > > * Clock rate change to 100MHz but delay is still 2.5ns
> > >     -> cached phase = 180
> > >     -> actual phase = 90
> > 
> > Good point!
> > 
> > (1)So either we should update phase once its rate is updated, for
> > instance, doing it in clk_change_rate, or
> 
> I don't think this would be a good option. The phase depending on the rate is
> just one example. I'm pretty sure one could come up with another example 
> 
> > 
> > (2)we should always try to get the actual phase here.
> 
> If the callback is available, lets use it. I prefer this second option
> 
> > 
> > If we do anyway call ->get_phase, we should never need core->phase since
> > it doesn't reflect the actual phase maybe.
> 
> The cached value is used by debugfs so it is still a valuable information. I
> would keep it around for now.

Oh ! and let's not forget that a clock driver is allowed to implement
set_phase() w/o implementing get_phase() ... so you definitively want to keep
the cached value around

> 
> > 
> > Am I right? If yes, which option is more reasonable?
> 
> 

Re: [PATCH] clk: Don't show the incorrect clock phase

[Shawn Lin]

On 2018/3/8 19:39, Jerome Brunet wrote:
> On Thu, 2018-03-08 at 12:37 +0100, Jerome Brunet wrote:
>> On Thu, 2018-03-08 at 19:28 +0800, Shawn Lin wrote:
>>>>> +    if (core->phase < 0 && core->ops->get_phase)
>>>>
>>>> Why bother with core->phase < 0 ?
>>>> if core->ops->get_phase is available, why not call it anyway ?
>>>>
>>>> The phase may have changed since it was cached.
>>>>
>>>> A typical example would be phases based on adding fixed delays:
>>>>
>>>> * Take a 200MHz clock and set phase to 180 => a 2.5ns delay is set
>>>> * Clock rate change to 100MHz but delay is still 2.5ns
>>>>      -> cached phase = 180
>>>>      -> actual phase = 90
>>>
>>> Good point!
>>>
>>> (1)So either we should update phase once its rate is updated, for
>>> instance, doing it in clk_change_rate, or
>>
>> I don't think this would be a good option. The phase depending on the rate is
>> just one example. I'm pretty sure one could come up with another example
>>
>>>
>>> (2)we should always try to get the actual phase here.
>>
>> If the callback is available, lets use it. I prefer this second option
>>

okay.

>>>
>>> If we do anyway call ->get_phase, we should never need core->phase since
>>> it doesn't reflect the actual phase maybe.
>>
>> The cached value is used by debugfs so it is still a valuable information. I
>> would keep it around for now.
> 
> Oh ! and let's not forget that a clock driver is allowed to implement
> set_phase() w/o implementing get_phase() ... so you definitively want to keep
> the cached value around
> 

Yes, it's a little complicated. I just thought a case that, if the
driver need a fixed phase i.e. by call clk_set_phase(180), but the clk's
phase depends on the clk rate. So if the clk rate changed, the phase is 
changed as well. This isn't good to the IP should sample the data based
on the phase shift but never get notified that the phase is changed.


>>
>>>
>>> Am I right? If yes, which option is more reasonable?
>>
>>
> 
> 
> 
> 

Re: [PATCH] clk: Don't show the incorrect clock phase

[Jerome Brunet]

On Thu, 2018-03-08 at 19:46 +0800, Shawn Lin wrote:
> > > > Good point!
> > > > 
> > > > (1)So either we should update phase once its rate is updated, for
> > > > instance, doing it in clk_change_rate, or
> > > 
> > > I don't think this would be a good option. The phase depending on the rate is
> > > just one example. I'm pretty sure one could come up with another example
> > > 
> > > > 
> > > > (2)we should always try to get the actual phase here.
> > > 
> > > If the callback is available, lets use it. I prefer this second option
> > > 
> 
> okay.
> 
> > > > 
> > > > If we do anyway call ->get_phase, we should never need core->phase since
> > > > it doesn't reflect the actual phase maybe.
> > > 
> > > The cached value is used by debugfs so it is still a valuable information. I
> > > would keep it around for now.
> > 
> > Oh ! and let's not forget that a clock driver is allowed to implement
> > set_phase() w/o implementing get_phase() ... so you definitively want to keep
> > the cached value around
> > 
> 
> Yes, it's a little complicated. I just thought a case that, if the
> driver need a fixed phase i.e. by call clk_set_phase(180), but the clk's
> phase depends on the clk rate. So if the clk rate changed, the phase is 
> changed as well. This isn't good to the IP should sample the data based
> on the phase shift but never get notified that the phase is changed.

I suppose this use case comes from the mmc (most phase users are mmc drivers) ?

The fact that you would want ,for your use case, to lock the phase whatever the
rate changes seems like a choice. It makes sense for sure, but maybe not all
driver would want to enforce that. I'm not sure we should bake that in the
framework itself.

For such case, a solution could be to use clock notifiers to call
clk_set_phase() on rate change,

OR

in the clock driver set_rate() callback, cache the phase at the beginning and
call set_phase() again before returning.

Anyway, let's keep it simple for now and report the phase correctly. We can
still deal these particular situations with some other patches.

Re: [PATCH] clk: Don't show the incorrect clock phase

[Shawn Lin]

On 2018/3/8 20:03, Jerome Brunet wrote:
> On Thu, 2018-03-08 at 19:46 +0800, Shawn Lin wrote:
>>>>> Good point!
>>>>>
>>>>> (1)So either we should update phase once its rate is updated, for
>>>>> instance, doing it in clk_change_rate, or
>>>>
>>>> I don't think this would be a good option. The phase depending on the rate is
>>>> just one example. I'm pretty sure one could come up with another example
>>>>
>>>>>
>>>>> (2)we should always try to get the actual phase here.
>>>>
>>>> If the callback is available, lets use it. I prefer this second option
>>>>
>>
>> okay.
>>
>>>>>
>>>>> If we do anyway call ->get_phase, we should never need core->phase since
>>>>> it doesn't reflect the actual phase maybe.
>>>>
>>>> The cached value is used by debugfs so it is still a valuable information. I
>>>> would keep it around for now.
>>>
>>> Oh ! and let's not forget that a clock driver is allowed to implement
>>> set_phase() w/o implementing get_phase() ... so you definitively want to keep
>>> the cached value around
>>>
>>
>> Yes, it's a little complicated. I just thought a case that, if the
>> driver need a fixed phase i.e. by call clk_set_phase(180), but the clk's
>> phase depends on the clk rate. So if the clk rate changed, the phase is
>> changed as well. This isn't good to the IP should sample the data based
>> on the phase shift but never get notified that the phase is changed.
> 
> I suppose this use case comes from the mmc (most phase users are mmc drivers) ?

Right.

> 
> The fact that you would want ,for your use case, to lock the phase whatever the
> rate changes seems like a choice. It makes sense for sure, but maybe not all
> driver would want to enforce that. I'm not sure we should bake that in the
> framework itself.
> 
> For such case, a solution could be to use clock notifiers to call
> clk_set_phase() on rate change,
> 
> OR
> 
> in the clock driver set_rate() callback, cache the phase at the beginning and
> call set_phase() again before returning.
> 
> Anyway, let's keep it simple for now and report the phase correctly. We can
> still deal these particular situations with some other patches.

Okay, I will update v2 based on your suggestion unless there are any
objections. But for sure, I will think more about these and check out
how to make this particular case works.

Thanks for your comment!

> 
> 
> 
>