| Message ID | 20200825092124.4284-5-dariobin@libero.it |
|---|---|
| State | Changes Requested |
| Delegated to: | Lokesh Vutla |
| Headers | show |
| Series | Add DM support for omap PWM backlight | expand |
Hi Dario, +Stephen Warren On Tue, 25 Aug 2020 at 03:24, Dario Binacchi <dariobin@libero.it> wrote: > > It returns the rate which will be set if you ask clk_set_rate() to set > that rate. It provides a way to query exactly what rate you'll get if > you call clk_set_rate() with that same argument. > So essentially, clk_round_rate() and clk_set_rate() are equivalent > except the former does not modify the clock hardware in any way. > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > --- > > arch/sandbox/include/asm/clk.h | 9 +++++++++ > drivers/clk/clk-uclass.c | 15 +++++++++++++++ > drivers/clk/clk_sandbox.c | 17 +++++++++++++++++ > drivers/clk/clk_sandbox_test.c | 10 ++++++++++ > include/clk-uclass.h | 8 ++++++++ > include/clk.h | 29 +++++++++++++++++++++++++++++ > test/dm/clk.c | 22 ++++++++++++++++++++++ > 7 files changed, 110 insertions(+) > Reviewed-by: Simon Glass <sjg@chromium.org> But I wonder if we should change the set_rate() uclass interface to have a flag value, one of the flags being 'dry run' which doesn't actually set the value? You would still have the same call to the uclass functions clk_set_rate() and clk_round_rate() but the driver API would implement both with calls to set_rate()?
On 8/29/20 5:20 PM, Simon Glass wrote: > Hi Dario, > > +Stephen Warren > On Tue, 25 Aug 2020 at 03:24, Dario Binacchi <dariobin@libero.it> wrote: >> >> It returns the rate which will be set if you ask clk_set_rate() to set >> that rate. It provides a way to query exactly what rate you'll get if >> you call clk_set_rate() with that same argument. >> So essentially, clk_round_rate() and clk_set_rate() are equivalent >> except the former does not modify the clock hardware in any way. >> >> Signed-off-by: Dario Binacchi <dariobin@libero.it> >> --- >> >> arch/sandbox/include/asm/clk.h | 9 +++++++++ >> drivers/clk/clk-uclass.c | 15 +++++++++++++++ >> drivers/clk/clk_sandbox.c | 17 +++++++++++++++++ >> drivers/clk/clk_sandbox_test.c | 10 ++++++++++ >> include/clk-uclass.h | 8 ++++++++ >> include/clk.h | 29 +++++++++++++++++++++++++++++ >> test/dm/clk.c | 22 ++++++++++++++++++++++ >> 7 files changed, 110 insertions(+) >> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > But I wonder if we should change the set_rate() uclass interface to > have a flag value, one of the flags being 'dry run' which doesn't > actually set the value? > > You would still have the same call to the uclass functions > clk_set_rate() and clk_round_rate() but the driver API would implement > both with calls to set_rate()? Linux uses separate clk_ops functions for this purpose > * @recalc_rate Recalculate the rate of this clock, by querying hardware. The > * parent rate is an input parameter. It is up to the caller to > * ensure that the prepare_mutex is held across this call. > * Returns the calculated rate. Optional, but recommended - if > * this op is not set then clock rate will be initialized to 0. > (...snip...) > long (*round_rate)(struct clk_hw *hw, unsigned long rate, > unsigned long *parent_rate); Besides matching their interface, I think there is good reason for keeping these functions separate. Existing clock drivers would need to be rewritten so they don't set the clock rate when you just want to do a dry run. This way, it's very clear when a driver supports recalc_rate. --Sean
On 8/29/20 5:48 PM, Sean Anderson wrote: > > On 8/29/20 5:20 PM, Simon Glass wrote: >> Hi Dario, >> >> +Stephen Warren >> On Tue, 25 Aug 2020 at 03:24, Dario Binacchi <dariobin@libero.it> wrote: >>> >>> It returns the rate which will be set if you ask clk_set_rate() to set >>> that rate. It provides a way to query exactly what rate you'll get if >>> you call clk_set_rate() with that same argument. >>> So essentially, clk_round_rate() and clk_set_rate() are equivalent >>> except the former does not modify the clock hardware in any way. >>> >>> Signed-off-by: Dario Binacchi <dariobin@libero.it> >>> --- >>> >>> arch/sandbox/include/asm/clk.h | 9 +++++++++ >>> drivers/clk/clk-uclass.c | 15 +++++++++++++++ >>> drivers/clk/clk_sandbox.c | 17 +++++++++++++++++ >>> drivers/clk/clk_sandbox_test.c | 10 ++++++++++ >>> include/clk-uclass.h | 8 ++++++++ >>> include/clk.h | 29 +++++++++++++++++++++++++++++ >>> test/dm/clk.c | 22 ++++++++++++++++++++++ >>> 7 files changed, 110 insertions(+) >>> >> >> Reviewed-by: Simon Glass <sjg@chromium.org> >> >> But I wonder if we should change the set_rate() uclass interface to >> have a flag value, one of the flags being 'dry run' which doesn't >> actually set the value? >> >> You would still have the same call to the uclass functions >> clk_set_rate() and clk_round_rate() but the driver API would implement >> both with calls to set_rate()? > > Linux uses separate clk_ops functions for this purpose > >> * @recalc_rate Recalculate the rate of this clock, by querying hardware. The >> * parent rate is an input parameter. It is up to the caller to >> * ensure that the prepare_mutex is held across this call. >> * Returns the calculated rate. Optional, but recommended - if >> * this op is not set then clock rate will be initialized to 0. err, I meant to quote > * @round_rate: Given a target rate as input, returns the closest rate actually > * supported by the clock. The parent rate is an input/output > * parameter. >> (...snip...) >> long (*round_rate)(struct clk_hw *hw, unsigned long rate, >> unsigned long *parent_rate); > > Besides matching their interface, I think there is good reason for > keeping these functions separate. Existing clock drivers would need to > be rewritten so they don't set the clock rate when you just want to do a > dry run. This way, it's very clear when a driver supports recalc_rate. > > --Sean >
> Il 29/08/2020 23:50 Sean Anderson <seanga2@gmail.com> ha scritto: > > > On 8/29/20 5:48 PM, Sean Anderson wrote: > > > > On 8/29/20 5:20 PM, Simon Glass wrote: > >> Hi Dario, > >> > >> +Stephen Warren > >> On Tue, 25 Aug 2020 at 03:24, Dario Binacchi <dariobin@libero.it> wrote: > >>> > >>> It returns the rate which will be set if you ask clk_set_rate() to set > >>> that rate. It provides a way to query exactly what rate you'll get if > >>> you call clk_set_rate() with that same argument. > >>> So essentially, clk_round_rate() and clk_set_rate() are equivalent > >>> except the former does not modify the clock hardware in any way. > >>> > >>> Signed-off-by: Dario Binacchi <dariobin@libero.it> > >>> --- > >>> > >>> arch/sandbox/include/asm/clk.h | 9 +++++++++ > >>> drivers/clk/clk-uclass.c | 15 +++++++++++++++ > >>> drivers/clk/clk_sandbox.c | 17 +++++++++++++++++ > >>> drivers/clk/clk_sandbox_test.c | 10 ++++++++++ > >>> include/clk-uclass.h | 8 ++++++++ > >>> include/clk.h | 29 +++++++++++++++++++++++++++++ > >>> test/dm/clk.c | 22 ++++++++++++++++++++++ > >>> 7 files changed, 110 insertions(+) > >>> > >> > >> Reviewed-by: Simon Glass <sjg@chromium.org> > >> > >> But I wonder if we should change the set_rate() uclass interface to > >> have a flag value, one of the flags being 'dry run' which doesn't > >> actually set the value? > >> > >> You would still have the same call to the uclass functions > >> clk_set_rate() and clk_round_rate() but the driver API would implement > >> both with calls to set_rate()? > > > > Linux uses separate clk_ops functions for this purpose > > > >> * @recalc_rate Recalculate the rate of this clock, by querying hardware. The > >> * parent rate is an input parameter. It is up to the caller to > >> * ensure that the prepare_mutex is held across this call. > >> * Returns the calculated rate. Optional, but recommended - if > >> * this op is not set then clock rate will be initialized to 0. > > err, I meant to quote > > > * @round_rate: Given a target rate as input, returns the closest rate actually > > * supported by the clock. The parent rate is an input/output > > * parameter. > > >> (...snip...) > >> long (*round_rate)(struct clk_hw *hw, unsigned long rate, > >> unsigned long *parent_rate); > > > > Besides matching their interface, I think there is good reason for > > keeping these functions separate. Existing clock drivers would need to > > be rewritten so they don't set the clock rate when you just want to do a > > dry run. This way, it's very clear when a driver supports recalc_rate. I agree. > > > > --Sean > >
diff --git a/arch/sandbox/include/asm/clk.h b/arch/sandbox/include/asm/clk.h index 1163e61026..68a8687f57 100644 --- a/arch/sandbox/include/asm/clk.h +++ b/arch/sandbox/include/asm/clk.h @@ -105,6 +105,15 @@ int sandbox_clk_test_get_bulk(struct udevice *dev); * @return: The rate of the clock. */ ulong sandbox_clk_test_get_rate(struct udevice *dev, int id); +/** + * sandbox_clk_test_round_rate - Ask the sandbox clock test device to round a + * clock's rate. + * + * @dev: The sandbox clock test (client) device. + * @id: The test device's clock ID to configure. + * @return: The rounded rate of the clock. + */ +ulong sandbox_clk_test_round_rate(struct udevice *dev, int id, ulong rate); /** * sandbox_clk_test_set_rate - Ask the sandbox clock test device to set a * clock's rate. diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 934cd5787a..1b5541c3af 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -494,6 +494,21 @@ long long clk_get_parent_rate(struct clk *clk) return pclk->rate; } +ulong clk_round_rate(struct clk *clk, ulong rate) +{ + const struct clk_ops *ops; + + debug("%s(clk=%p, rate=%lu)\n", __func__, clk, rate); + if (!clk_valid(clk)) + return 0; + + ops = clk_dev_ops(clk->dev); + if (!ops->round_rate) + return -ENOSYS; + + return ops->round_rate(clk, rate); +} + ulong clk_set_rate(struct clk *clk, ulong rate) { const struct clk_ops *ops; diff --git a/drivers/clk/clk_sandbox.c b/drivers/clk/clk_sandbox.c index 768fbb7c52..8361b930df 100644 --- a/drivers/clk/clk_sandbox.c +++ b/drivers/clk/clk_sandbox.c @@ -30,6 +30,22 @@ static ulong sandbox_clk_get_rate(struct clk *clk) return priv->rate[clk->id]; } +static ulong sandbox_clk_round_rate(struct clk *clk, ulong rate) +{ + struct sandbox_clk_priv *priv = dev_get_priv(clk->dev); + + if (!priv->probed) + return -ENODEV; + + if (clk->id >= SANDBOX_CLK_ID_COUNT) + return -EINVAL; + + if (!rate) + return -EINVAL; + + return rate; +} + static ulong sandbox_clk_set_rate(struct clk *clk, ulong rate) { struct sandbox_clk_priv *priv = dev_get_priv(clk->dev); @@ -103,6 +119,7 @@ static int sandbox_clk_free(struct clk *clk) } static struct clk_ops sandbox_clk_ops = { + .round_rate = sandbox_clk_round_rate, .get_rate = sandbox_clk_get_rate, .set_rate = sandbox_clk_set_rate, .enable = sandbox_clk_enable, diff --git a/drivers/clk/clk_sandbox_test.c b/drivers/clk/clk_sandbox_test.c index 873383856f..f7b77aa674 100644 --- a/drivers/clk/clk_sandbox_test.c +++ b/drivers/clk/clk_sandbox_test.c @@ -86,6 +86,16 @@ ulong sandbox_clk_test_get_rate(struct udevice *dev, int id) return clk_get_rate(sbct->clkps[id]); } +ulong sandbox_clk_test_round_rate(struct udevice *dev, int id, ulong rate) +{ + struct sandbox_clk_test *sbct = dev_get_priv(dev); + + if (id < 0 || id >= SANDBOX_CLK_TEST_ID_COUNT) + return -EINVAL; + + return clk_round_rate(sbct->clkps[id], rate); +} + ulong sandbox_clk_test_set_rate(struct udevice *dev, int id, ulong rate) { struct sandbox_clk_test *sbct = dev_get_priv(dev); diff --git a/include/clk-uclass.h b/include/clk-uclass.h index dac42dab36..50e8681b55 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -61,6 +61,14 @@ struct clk_ops { * @return 0 if OK, or a negative error code. */ int (*rfree)(struct clk *clock); + /** + * round_rate() - Adjust a rate to the exact rate a clock can provide. + * + * @clk: The clock to manipulate. + * @rate: Desidered clock rate in Hz. + * @return rounded rate in Hz, or -ve error code. + */ + ulong (*round_rate)(struct clk *clk, ulong rate); /** * get_rate() - Get current clock rate. * diff --git a/include/clk.h b/include/clk.h index a62e2efa2c..cce763b05b 100644 --- a/include/clk.h +++ b/include/clk.h @@ -366,6 +366,30 @@ struct clk *clk_get_parent(struct clk *clk); */ long long clk_get_parent_rate(struct clk *clk); +/** + * clk_round_rate() - Adjust a rate to the exact rate a clock can provide + * + * This answers the question "if I were to pass @rate to clk_set_rate(), + * what clock rate would I end up with?" without changing the hardware + * in any way. In other words: + * + * rate = clk_round_rate(clk, r); + * + * and: + * + * clk_set_rate(clk, r); + * rate = clk_get_rate(clk); + * + * are equivalent except the former does not modify the clock hardware + * in any way. + * + * @clk: A clock struct that was previously successfully requested by + * clk_request/get_by_*(). + * @rate: desired clock rate in Hz. + * @return rounded rate in Hz, or -ve error code. + */ +ulong clk_round_rate(struct clk *clk, ulong rate); + /** * clk_set_rate() - Set current clock rate. * @@ -482,6 +506,11 @@ static inline long long clk_get_parent_rate(struct clk *clk) return -ENOSYS; } +static inline ulong clk_round_rate(struct clk *clk, ulong rate) +{ + return -ENOSYS; +} + static inline ulong clk_set_rate(struct clk *clk, ulong rate) { return -ENOSYS; diff --git a/test/dm/clk.c b/test/dm/clk.c index edca3b49f6..21997ed892 100644 --- a/test/dm/clk.c +++ b/test/dm/clk.c @@ -112,6 +112,28 @@ static int dm_test_clk(struct unit_test_state *uts) rate = sandbox_clk_test_set_rate(dev_test, SANDBOX_CLK_TEST_ID_I2C, 0); ut_assert(IS_ERR_VALUE(rate)); + ut_asserteq(10000, sandbox_clk_test_get_rate(dev_test, + SANDBOX_CLK_TEST_ID_SPI)); + ut_asserteq(20000, sandbox_clk_test_get_rate(dev_test, + SANDBOX_CLK_TEST_ID_I2C)); + + ut_asserteq(5000, sandbox_clk_test_round_rate(dev_test, + SANDBOX_CLK_TEST_ID_SPI, + 5000)); + ut_asserteq(7000, sandbox_clk_test_round_rate(dev_test, + SANDBOX_CLK_TEST_ID_I2C, + 7000)); + + ut_asserteq(10000, sandbox_clk_test_get_rate(dev_test, + SANDBOX_CLK_TEST_ID_SPI)); + ut_asserteq(20000, sandbox_clk_test_get_rate(dev_test, + SANDBOX_CLK_TEST_ID_I2C)); + + rate = sandbox_clk_test_round_rate(dev_test, SANDBOX_CLK_TEST_ID_SPI, 0); + ut_assert(IS_ERR_VALUE(rate)); + rate = sandbox_clk_test_round_rate(dev_test, SANDBOX_CLK_TEST_ID_I2C, 0); + ut_assert(IS_ERR_VALUE(rate)); + ut_asserteq(10000, sandbox_clk_test_get_rate(dev_test, SANDBOX_CLK_TEST_ID_SPI)); ut_asserteq(20000, sandbox_clk_test_get_rate(dev_test,
It returns the rate which will be set if you ask clk_set_rate() to set that rate. It provides a way to query exactly what rate you'll get if you call clk_set_rate() with that same argument. So essentially, clk_round_rate() and clk_set_rate() are equivalent except the former does not modify the clock hardware in any way. Signed-off-by: Dario Binacchi <dariobin@libero.it> --- arch/sandbox/include/asm/clk.h | 9 +++++++++ drivers/clk/clk-uclass.c | 15 +++++++++++++++ drivers/clk/clk_sandbox.c | 17 +++++++++++++++++ drivers/clk/clk_sandbox_test.c | 10 ++++++++++ include/clk-uclass.h | 8 ++++++++ include/clk.h | 29 +++++++++++++++++++++++++++++ test/dm/clk.c | 22 ++++++++++++++++++++++ 7 files changed, 110 insertions(+)