| Message ID | 1409428991-2442-4-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 08/30/2014 10:03 PM, Maxime Ripard wrote: > The current phase API doesn't look into the actual hardware to get the phase > value, but will rather get it from a variable only set by the set_phase > function. > > This will cause issue when the client driver will never call the set_phase > function, where we can end up having a reported phase that will not match what > the hardware has been programmed to by the bootloader or what phase is > programmed out of reset. > > Add a new get_phase function for the drivers to implement so that we can get > this value. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/clk/clk.c | 17 ++++++++++++++--- > include/linux/clk-provider.h | 5 +++++ > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index d87661af0c72..7dbceca694f1 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1797,8 +1797,8 @@ out: > * clk_get_phase - return the phase shift of a clock signal > * @clk: clock signal source > * > - * Returns the phase shift of a clock node in degrees, otherwise returns > - * -EERROR. > + * Returns the phase shift of a clock node in degrees. Any negative > + * values are errors. > */ > int clk_get_phase(struct clk *clk) > { > @@ -1808,7 +1808,18 @@ int clk_get_phase(struct clk *clk) > goto out; > > clk_prepare_lock(); > - ret = clk->phase; > + > + if (clk->phase) { > + ret = clk->phase; > + goto out_unlock; > + } 0 is a valid phase, so this will cause the phase to be read from the hardware each time if the phase is 0. Perhaps make clk->phase signed (if it is not already), init it to -1, and check for it not being -1 ? Regards, Hans > + > + if (!clk->ops->get_phase) > + goto out_unlock; > + > + ret = clk->ops->get_phase(clk->hw); > + > +out_unlock: > clk_prepare_unlock(); > > out: > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 69b20d4c1e1a..abec961092a7 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -130,6 +130,10 @@ struct dentry; > * set then clock accuracy will be initialized to parent accuracy > * or 0 (perfect clock) if clock has no parent. > * > + * @get_phase: Queries the hardware to get the current phase of a clock. > + * Returned values are 0-359 degrees on success, negative > + * error codes on failure. > + * > * @set_phase: Shift the phase this clock signal in degrees specified > * by the second argument. Valid values for degrees are > * 0-359. Return 0 on success, otherwise -EERROR. > @@ -182,6 +186,7 @@ struct clk_ops { > unsigned long parent_rate, u8 index); > unsigned long (*recalc_accuracy)(struct clk_hw *hw, > unsigned long parent_accuracy); > + int (*get_phase)(struct clk_hw *hw); > int (*set_phase)(struct clk_hw *hw, int degrees); > void (*init)(struct clk_hw *hw); > int (*debug_init)(struct clk_hw *hw, struct dentry *dentry); > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Sun, Aug 31, 2014 at 12:15:50PM +0200, Hans de Goede wrote: > Hi, > > On 08/30/2014 10:03 PM, Maxime Ripard wrote: > > The current phase API doesn't look into the actual hardware to get the phase > > value, but will rather get it from a variable only set by the set_phase > > function. > > > > This will cause issue when the client driver will never call the set_phase > > function, where we can end up having a reported phase that will not match what > > the hardware has been programmed to by the bootloader or what phase is > > programmed out of reset. > > > > Add a new get_phase function for the drivers to implement so that we can get > > this value. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > drivers/clk/clk.c | 17 ++++++++++++++--- > > include/linux/clk-provider.h | 5 +++++ > > 2 files changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index d87661af0c72..7dbceca694f1 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -1797,8 +1797,8 @@ out: > > * clk_get_phase - return the phase shift of a clock signal > > * @clk: clock signal source > > * > > - * Returns the phase shift of a clock node in degrees, otherwise returns > > - * -EERROR. > > + * Returns the phase shift of a clock node in degrees. Any negative > > + * values are errors. > > */ > > int clk_get_phase(struct clk *clk) > > { > > @@ -1808,7 +1808,18 @@ int clk_get_phase(struct clk *clk) > > goto out; > > > > clk_prepare_lock(); > > - ret = clk->phase; > > + > > + if (clk->phase) { > > + ret = clk->phase; > > + goto out_unlock; > > + } > > 0 is a valid phase, so this will cause the phase to > be read from the hardware each time if the phase is 0. > > Perhaps make clk->phase signed (if it is not already), init it > to -1, and check for it not being -1 ? Yeah, I'm not really proud of this code either, but yours would expose this -1 into debugfs, so I'm not sure it's really better :) (with -1 being a valid phase too) Maxime
Hi, On 09/01/2014 12:20 PM, Maxime Ripard wrote: > Hi, > > On Sun, Aug 31, 2014 at 12:15:50PM +0200, Hans de Goede wrote: >> Hi, >> >> On 08/30/2014 10:03 PM, Maxime Ripard wrote: >>> The current phase API doesn't look into the actual hardware to get the phase >>> value, but will rather get it from a variable only set by the set_phase >>> function. >>> >>> This will cause issue when the client driver will never call the set_phase >>> function, where we can end up having a reported phase that will not match what >>> the hardware has been programmed to by the bootloader or what phase is >>> programmed out of reset. >>> >>> Add a new get_phase function for the drivers to implement so that we can get >>> this value. >>> >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>> --- >>> drivers/clk/clk.c | 17 ++++++++++++++--- >>> include/linux/clk-provider.h | 5 +++++ >>> 2 files changed, 19 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >>> index d87661af0c72..7dbceca694f1 100644 >>> --- a/drivers/clk/clk.c >>> +++ b/drivers/clk/clk.c >>> @@ -1797,8 +1797,8 @@ out: >>> * clk_get_phase - return the phase shift of a clock signal >>> * @clk: clock signal source >>> * >>> - * Returns the phase shift of a clock node in degrees, otherwise returns >>> - * -EERROR. >>> + * Returns the phase shift of a clock node in degrees. Any negative >>> + * values are errors. >>> */ >>> int clk_get_phase(struct clk *clk) >>> { >>> @@ -1808,7 +1808,18 @@ int clk_get_phase(struct clk *clk) >>> goto out; >>> >>> clk_prepare_lock(); >>> - ret = clk->phase; >>> + >>> + if (clk->phase) { >>> + ret = clk->phase; >>> + goto out_unlock; >>> + } >> >> 0 is a valid phase, so this will cause the phase to >> be read from the hardware each time if the phase is 0. >> >> Perhaps make clk->phase signed (if it is not already), init it >> to -1, and check for it not being -1 ? > > Yeah, I'm not really proud of this code either, but yours would expose > this -1 into debugfs, so I'm not sure it's really better :) > > (with -1 being a valid phase too) According to the comments in the patch adding the original phase functions valid values are 0 - 359. And you could use clk_get_phase from the debugfs code. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Maxime Ripard (2014-08-30 13:03:02) > The current phase API doesn't look into the actual hardware to get the phase > value, but will rather get it from a variable only set by the set_phase > function. > > This will cause issue when the client driver will never call the set_phase > function, where we can end up having a reported phase that will not match what > the hardware has been programmed to by the bootloader or what phase is > programmed out of reset. > > Add a new get_phase function for the drivers to implement so that we can get > this value. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/clk/clk.c | 17 ++++++++++++++--- > include/linux/clk-provider.h | 5 +++++ > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index d87661af0c72..7dbceca694f1 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1797,8 +1797,8 @@ out: > * clk_get_phase - return the phase shift of a clock signal > * @clk: clock signal source > * > - * Returns the phase shift of a clock node in degrees, otherwise returns > - * -EERROR. > + * Returns the phase shift of a clock node in degrees. Any negative > + * values are errors. > */ > int clk_get_phase(struct clk *clk) > { > @@ -1808,7 +1808,18 @@ int clk_get_phase(struct clk *clk) > goto out; > > clk_prepare_lock(); > - ret = clk->phase; > + > + if (clk->phase) { So if a phase is zero, then we fall through and query the hardware? Seems like this case might be hit a lot for clocks that implement .get_phase but never have their phase shifted, and accesses to the registers may be much slower than to memory. Do you expect the phase to be changed behind our backs? E.g. firmware changes it, or coming in and out of idle state, etc. If not then we can store the phase at clock registration time and use a cached value. See how the CLK_GET_RATE_NOCACHE and CLK_GET_ACCURACY_NOCACHE flags are used for details on how we can handle the case where the phase might change behind our back. Regards, Mike > + ret = clk->phase; > + goto out_unlock; > + } > + > + if (!clk->ops->get_phase) > + goto out_unlock; > + > + ret = clk->ops->get_phase(clk->hw); > + > +out_unlock: > clk_prepare_unlock(); > > out: > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 69b20d4c1e1a..abec961092a7 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -130,6 +130,10 @@ struct dentry; > * set then clock accuracy will be initialized to parent accuracy > * or 0 (perfect clock) if clock has no parent. > * > + * @get_phase: Queries the hardware to get the current phase of a clock. > + * Returned values are 0-359 degrees on success, negative > + * error codes on failure. > + * > * @set_phase: Shift the phase this clock signal in degrees specified > * by the second argument. Valid values for degrees are > * 0-359. Return 0 on success, otherwise -EERROR. > @@ -182,6 +186,7 @@ struct clk_ops { > unsigned long parent_rate, u8 index); > unsigned long (*recalc_accuracy)(struct clk_hw *hw, > unsigned long parent_accuracy); > + int (*get_phase)(struct clk_hw *hw); > int (*set_phase)(struct clk_hw *hw, int degrees); > void (*init)(struct clk_hw *hw); > int (*debug_init)(struct clk_hw *hw, struct dentry *dentry); > -- > 2.0.2 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Sep 01, 2014 at 12:00:54PM -0700, Mike Turquette wrote: > Quoting Maxime Ripard (2014-08-30 13:03:02) > > The current phase API doesn't look into the actual hardware to get the phase > > value, but will rather get it from a variable only set by the set_phase > > function. > > > > This will cause issue when the client driver will never call the set_phase > > function, where we can end up having a reported phase that will not match what > > the hardware has been programmed to by the bootloader or what phase is > > programmed out of reset. > > > > Add a new get_phase function for the drivers to implement so that we can get > > this value. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > drivers/clk/clk.c | 17 ++++++++++++++--- > > include/linux/clk-provider.h | 5 +++++ > > 2 files changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index d87661af0c72..7dbceca694f1 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -1797,8 +1797,8 @@ out: > > * clk_get_phase - return the phase shift of a clock signal > > * @clk: clock signal source > > * > > - * Returns the phase shift of a clock node in degrees, otherwise returns > > - * -EERROR. > > + * Returns the phase shift of a clock node in degrees. Any negative > > + * values are errors. > > */ > > int clk_get_phase(struct clk *clk) > > { > > @@ -1808,7 +1808,18 @@ int clk_get_phase(struct clk *clk) > > goto out; > > > > clk_prepare_lock(); > > - ret = clk->phase; > > + > > + if (clk->phase) { > > So if a phase is zero, then we fall through and query the hardware? > Seems like this case might be hit a lot for clocks that implement > .get_phase but never have their phase shifted, and accesses to the > registers may be much slower than to memory. Yep, which is exactly why I'm not that proud of it :) > Do you expect the phase to be changed behind our backs? E.g. firmware > changes it, or coming in and out of idle state, etc. If not then we can > store the phase at clock registration time and use a cached value. This is exactly what happens in our case. Since in order to use the MMC, you have to change the phase of the output and sample clocks, every time the firmware will have to use the MMC, it will change these clocks phase. Which happens pretty much all the time. > See how the CLK_GET_RATE_NOCACHE and CLK_GET_ACCURACY_NOCACHE flags are > used for details on how we can handle the case where the phase might > change behind our back. Ah, yes, of course. Thanks! Maxime
On Mon, Sep 01, 2014 at 01:27:28PM +0200, Hans de Goede wrote: > Hi, > > On 09/01/2014 12:20 PM, Maxime Ripard wrote: > > Hi, > > > > On Sun, Aug 31, 2014 at 12:15:50PM +0200, Hans de Goede wrote: > >> Hi, > >> > >> On 08/30/2014 10:03 PM, Maxime Ripard wrote: > >>> The current phase API doesn't look into the actual hardware to get the phase > >>> value, but will rather get it from a variable only set by the set_phase > >>> function. > >>> > >>> This will cause issue when the client driver will never call the set_phase > >>> function, where we can end up having a reported phase that will not match what > >>> the hardware has been programmed to by the bootloader or what phase is > >>> programmed out of reset. > >>> > >>> Add a new get_phase function for the drivers to implement so that we can get > >>> this value. > >>> > >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >>> --- > >>> drivers/clk/clk.c | 17 ++++++++++++++--- > >>> include/linux/clk-provider.h | 5 +++++ > >>> 2 files changed, 19 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > >>> index d87661af0c72..7dbceca694f1 100644 > >>> --- a/drivers/clk/clk.c > >>> +++ b/drivers/clk/clk.c > >>> @@ -1797,8 +1797,8 @@ out: > >>> * clk_get_phase - return the phase shift of a clock signal > >>> * @clk: clock signal source > >>> * > >>> - * Returns the phase shift of a clock node in degrees, otherwise returns > >>> - * -EERROR. > >>> + * Returns the phase shift of a clock node in degrees. Any negative > >>> + * values are errors. > >>> */ > >>> int clk_get_phase(struct clk *clk) > >>> { > >>> @@ -1808,7 +1808,18 @@ int clk_get_phase(struct clk *clk) > >>> goto out; > >>> > >>> clk_prepare_lock(); > >>> - ret = clk->phase; > >>> + > >>> + if (clk->phase) { > >>> + ret = clk->phase; > >>> + goto out_unlock; > >>> + } > >> > >> 0 is a valid phase, so this will cause the phase to > >> be read from the hardware each time if the phase is 0. > >> > >> Perhaps make clk->phase signed (if it is not already), init it > >> to -1, and check for it not being -1 ? > > > > Yeah, I'm not really proud of this code either, but yours would expose > > this -1 into debugfs, so I'm not sure it's really better :) > > > > (with -1 being a valid phase too) > > According to the comments in the patch adding the original phase functions > valid values are 0 - 359. And you could use clk_get_phase from the debugfs > code. Yep, except that if I see a value -1 as a phase, I would assume that it's 359, but maybe it's just me :) Mike's solution seem great, I'll go for that. Maxime
Hi > Yep, except that if I see a value -1 as a phase, I would assume that > it's 359, but maybe it's just me :) -1 looks suspiciously like an error return value from somewhere...
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index d87661af0c72..7dbceca694f1 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1797,8 +1797,8 @@ out: * clk_get_phase - return the phase shift of a clock signal * @clk: clock signal source * - * Returns the phase shift of a clock node in degrees, otherwise returns - * -EERROR. + * Returns the phase shift of a clock node in degrees. Any negative + * values are errors. */ int clk_get_phase(struct clk *clk) { @@ -1808,7 +1808,18 @@ int clk_get_phase(struct clk *clk) goto out; clk_prepare_lock(); - ret = clk->phase; + + if (clk->phase) { + ret = clk->phase; + goto out_unlock; + } + + if (!clk->ops->get_phase) + goto out_unlock; + + ret = clk->ops->get_phase(clk->hw); + +out_unlock: clk_prepare_unlock(); out: diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 69b20d4c1e1a..abec961092a7 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -130,6 +130,10 @@ struct dentry; * set then clock accuracy will be initialized to parent accuracy * or 0 (perfect clock) if clock has no parent. * + * @get_phase: Queries the hardware to get the current phase of a clock. + * Returned values are 0-359 degrees on success, negative + * error codes on failure. + * * @set_phase: Shift the phase this clock signal in degrees specified * by the second argument. Valid values for degrees are * 0-359. Return 0 on success, otherwise -EERROR. @@ -182,6 +186,7 @@ struct clk_ops { unsigned long parent_rate, u8 index); unsigned long (*recalc_accuracy)(struct clk_hw *hw, unsigned long parent_accuracy); + int (*get_phase)(struct clk_hw *hw); int (*set_phase)(struct clk_hw *hw, int degrees); void (*init)(struct clk_hw *hw); int (*debug_init)(struct clk_hw *hw, struct dentry *dentry);
The current phase API doesn't look into the actual hardware to get the phase value, but will rather get it from a variable only set by the set_phase function. This will cause issue when the client driver will never call the set_phase function, where we can end up having a reported phase that will not match what the hardware has been programmed to by the bootloader or what phase is programmed out of reset. Add a new get_phase function for the drivers to implement so that we can get this value. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- drivers/clk/clk.c | 17 ++++++++++++++--- include/linux/clk-provider.h | 5 +++++ 2 files changed, 19 insertions(+), 3 deletions(-)