| Message ID | 20210225155909.1853812-2-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: hdmi: Support the 4k @ 60Hz modes | expand |
Quoting Maxime Ripard (2021-02-25 07:59:02) > Some devices might need to access the current available range of a clock > to discover their capabilities. Let's add those accessors. This needs more than two sentences to describe what's required. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ > include/linux/clk.h | 16 ++++++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 8c1d04db990d..b7307d4f090d 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) > } > EXPORT_SYMBOL_GPL(clk_set_max_rate); > > +long clk_get_min_rate(struct clk *clk) I need to read the rest of the patches but I don't see the justification for this sort of API vs. having the consumer constrain the clk frequency that it wants. Is the code that's setting the min/max constraints not the same as the code that's calling this API? Would an OPP table better serve this so the device knows what frequencies are valid?s Please provide the use case/justification in the commit text. Why two functions instead of one function to get both min and max? > +{ > + unsigned long min_rate, max_rate; > + > + if (!clk) > + return 0; > + > + clk_prepare_lock(); Please add a comment indicating why we grab the lock. Yes clk_core_get_boundaries() has a lock held assertion, but I don't know why we care about the lock here besides that we don't want the consumers to change while we calculate the boundaries as it may be some inaccurate number. > + clk_core_get_boundaries(clk->core, &min_rate, &max_rate); > + clk_prepare_unlock(); > + > + return min_rate; > +} > +EXPORT_SYMBOL_GPL(clk_get_min_rate); > + > +long clk_get_max_rate(struct clk *clk) > +{ > + unsigned long min_rate, max_rate; > + > + if (!clk) > + return 0; ULONG_MAX? > + > + clk_prepare_lock(); > + clk_core_get_boundaries(clk->core, &min_rate, &max_rate); > + clk_prepare_unlock(); > + > + return max_rate; > +} > +EXPORT_SYMBOL_GPL(clk_get_max_rate); > + > /** > * clk_get_parent - return the parent of a clk > * @clk: the clk whose parent gets returned > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 31ff1bf1b79f..6f0c00ddf3ac 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -709,6 +709,22 @@ int clk_set_min_rate(struct clk *clk, unsigned long rate); > */ > int clk_set_max_rate(struct clk *clk, unsigned long rate); > > +/** > + * clk_get_min_rate - get the minimum clock rate for a clock source > + * @clk: clock source > + * > + * Returns the minimum rate or negative errno. Hmm? > + */ > +long clk_get_min_rate(struct clk *clk); Why long instead of unsigned long? Error values don't seem to be returned. > + > +/** > + * clk_get_max_rate - get the maximum clock rate for a clock source > + * @clk: clock source > + * > + * Returns the maximum rate or negative errno. > + */ > +long clk_get_max_rate(struct clk *clk); > + > /** > * clk_set_parent - set the parent clock source for this clock > * @clk: clock source > --
Hi Stephen, On Tue, Mar 02, 2021 at 03:18:58PM -0800, Stephen Boyd wrote: > Quoting Maxime Ripard (2021-02-25 07:59:02) > > Some devices might need to access the current available range of a clock > > to discover their capabilities. Let's add those accessors. > > This needs more than two sentences to describe what's required. > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ > > include/linux/clk.h | 16 ++++++++++++++++ > > 2 files changed, 46 insertions(+) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 8c1d04db990d..b7307d4f090d 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) > > } > > EXPORT_SYMBOL_GPL(clk_set_max_rate); > > > > +long clk_get_min_rate(struct clk *clk) > > I need to read the rest of the patches but I don't see the justification > for this sort of API vs. having the consumer constrain the clk frequency > that it wants. Is the code that's setting the min/max constraints not > the same as the code that's calling this API? Would an OPP table better > serve this so the device knows what frequencies are valid?s Please > provide the use case/justification in the commit text. The patch that uses it is the patch 4 The issue I'm trying to solve is that all the RaspberryPi have a configuration file for the firmware, and the firmware is in charge of the clocks communicating through a mailbox interface. By default, one of the main clocks in the system can only reach 500MHz, and that's the range reported by the firmware when queried. However, in order to support display modes with a fairly high bandwidth such as 4k at 60Hz, that clock needs to be raised to at least 550MHz, and the firmware configuration has a special parameter for that one. Setting that parameter will increase the range of the clock to have proper boundaries for that display mode. If a user doesn't enable it and tries to use those display modes, the display will be completely blank. There's no way to query the firmware configuration directly, so we can instead query the range of the clock and see if the firmware enables us to use those modes, warn the user and ignore the modes that wouldn't work. That's what those accessors are here for > Why two functions instead of one function to get both min and max? Since we have clk_set_min_rate and clk_set_max_rate, it made sense to me to mirror that, but I'd be happy to change if you think otherwise I'll address your other commenst Maxime
Quoting Maxime Ripard (2021-03-03 00:45:27) > Hi Stephen, > > On Tue, Mar 02, 2021 at 03:18:58PM -0800, Stephen Boyd wrote: > > Quoting Maxime Ripard (2021-02-25 07:59:02) > > > Some devices might need to access the current available range of a clock > > > to discover their capabilities. Let's add those accessors. > > > > This needs more than two sentences to describe what's required. > > > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > --- > > > drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ > > > include/linux/clk.h | 16 ++++++++++++++++ > > > 2 files changed, 46 insertions(+) > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index 8c1d04db990d..b7307d4f090d 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) > > > } > > > EXPORT_SYMBOL_GPL(clk_set_max_rate); > > > > > > +long clk_get_min_rate(struct clk *clk) > > > > I need to read the rest of the patches but I don't see the justification > > for this sort of API vs. having the consumer constrain the clk frequency > > that it wants. Is the code that's setting the min/max constraints not > > the same as the code that's calling this API? Would an OPP table better > > serve this so the device knows what frequencies are valid?s Please > > provide the use case/justification in the commit text. > > The patch that uses it is the patch 4 > > The issue I'm trying to solve is that all the RaspberryPi have a > configuration file for the firmware, and the firmware is in charge of > the clocks communicating through a mailbox interface. > > By default, one of the main clocks in the system can only reach 500MHz, > and that's the range reported by the firmware when queried. However, in > order to support display modes with a fairly high bandwidth such as 4k > at 60Hz, that clock needs to be raised to at least 550MHz, and the > firmware configuration has a special parameter for that one. Setting > that parameter will increase the range of the clock to have proper > boundaries for that display mode. > > If a user doesn't enable it and tries to use those display modes, the > display will be completely blank. > > There's no way to query the firmware configuration directly, so we can > instead query the range of the clock and see if the firmware enables us > to use those modes, warn the user and ignore the modes that wouldn't > work. That's what those accessors are here for How does the clk driver query the firmware but it can't be done directly by the drm driver? > > > Why two functions instead of one function to get both min and max? > > Since we have clk_set_min_rate and clk_set_max_rate, it made sense to me > to mirror that, but I'd be happy to change if you think otherwise > Does using clk_round_rate() work just as well?
On Tue, Mar 16, 2021 at 06:06:40PM -0700, Stephen Boyd wrote: > Quoting Maxime Ripard (2021-03-03 00:45:27) > > Hi Stephen, > > > > On Tue, Mar 02, 2021 at 03:18:58PM -0800, Stephen Boyd wrote: > > > Quoting Maxime Ripard (2021-02-25 07:59:02) > > > > Some devices might need to access the current available range of a clock > > > > to discover their capabilities. Let's add those accessors. > > > > > > This needs more than two sentences to describe what's required. > > > > > > > > > > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > > > --- > > > > drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ > > > > include/linux/clk.h | 16 ++++++++++++++++ > > > > 2 files changed, 46 insertions(+) > > > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > > index 8c1d04db990d..b7307d4f090d 100644 > > > > --- a/drivers/clk/clk.c > > > > +++ b/drivers/clk/clk.c > > > > @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) > > > > } > > > > EXPORT_SYMBOL_GPL(clk_set_max_rate); > > > > > > > > +long clk_get_min_rate(struct clk *clk) > > > > > > I need to read the rest of the patches but I don't see the justification > > > for this sort of API vs. having the consumer constrain the clk frequency > > > that it wants. Is the code that's setting the min/max constraints not > > > the same as the code that's calling this API? Would an OPP table better > > > serve this so the device knows what frequencies are valid?s Please > > > provide the use case/justification in the commit text. > > > > The patch that uses it is the patch 4 > > > > The issue I'm trying to solve is that all the RaspberryPi have a > > configuration file for the firmware, and the firmware is in charge of > > the clocks communicating through a mailbox interface. > > > > By default, one of the main clocks in the system can only reach 500MHz, > > and that's the range reported by the firmware when queried. However, in > > order to support display modes with a fairly high bandwidth such as 4k > > at 60Hz, that clock needs to be raised to at least 550MHz, and the > > firmware configuration has a special parameter for that one. Setting > > that parameter will increase the range of the clock to have proper > > boundaries for that display mode. > > > > If a user doesn't enable it and tries to use those display modes, the > > display will be completely blank. > > > > There's no way to query the firmware configuration directly, so we can > > instead query the range of the clock and see if the firmware enables us > > to use those modes, warn the user and ignore the modes that wouldn't > > work. That's what those accessors are here for > > How does the clk driver query the firmware but it can't be done > directly by the drm driver? The configuration is done through a text file accessed by the firmware. What I meant was that the kernel cannot access the content of that file to make sure the right options have been enabled. However, it can indeed communicate with the firmware through the extent of the API it provides, but it's fairly limited. In our case, the only way to tell is to look for side effects of the configuration option, ie the maximum rate of the clock that has been increased. > > > Why two functions instead of one function to get both min and max? > > > > Since we have clk_set_min_rate and clk_set_max_rate, it made sense to me > > to mirror that, but I'd be happy to change if you think otherwise > > Does using clk_round_rate() work just as well? I guess it could work too, I'll try it out Maxime
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8c1d04db990d..b7307d4f090d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2407,6 +2407,36 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate) } EXPORT_SYMBOL_GPL(clk_set_max_rate); +long clk_get_min_rate(struct clk *clk) +{ + unsigned long min_rate, max_rate; + + if (!clk) + return 0; + + clk_prepare_lock(); + clk_core_get_boundaries(clk->core, &min_rate, &max_rate); + clk_prepare_unlock(); + + return min_rate; +} +EXPORT_SYMBOL_GPL(clk_get_min_rate); + +long clk_get_max_rate(struct clk *clk) +{ + unsigned long min_rate, max_rate; + + if (!clk) + return 0; + + clk_prepare_lock(); + clk_core_get_boundaries(clk->core, &min_rate, &max_rate); + clk_prepare_unlock(); + + return max_rate; +} +EXPORT_SYMBOL_GPL(clk_get_max_rate); + /** * clk_get_parent - return the parent of a clk * @clk: the clk whose parent gets returned diff --git a/include/linux/clk.h b/include/linux/clk.h index 31ff1bf1b79f..6f0c00ddf3ac 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -709,6 +709,22 @@ int clk_set_min_rate(struct clk *clk, unsigned long rate); */ int clk_set_max_rate(struct clk *clk, unsigned long rate); +/** + * clk_get_min_rate - get the minimum clock rate for a clock source + * @clk: clock source + * + * Returns the minimum rate or negative errno. + */ +long clk_get_min_rate(struct clk *clk); + +/** + * clk_get_max_rate - get the maximum clock rate for a clock source + * @clk: clock source + * + * Returns the maximum rate or negative errno. + */ +long clk_get_max_rate(struct clk *clk); + /** * clk_set_parent - set the parent clock source for this clock * @clk: clock source
Some devices might need to access the current available range of a clock to discover their capabilities. Let's add those accessors. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/clk.c | 30 ++++++++++++++++++++++++++++++ include/linux/clk.h | 16 ++++++++++++++++ 2 files changed, 46 insertions(+)