| Message ID | 6dc1c48cac5b2646a55da3079afb72f88e40c3bc.1358937138.git.afzal@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Afzal Mohammed (2013-01-23 03:38:52) > Some of clocks can have a limit on minimum divider value that can be > programmed, prepare for such a support. > > Add a new field min_div for the basic divider clock and a new dynamic > clock divider registration function where minimum divider value can > be specified. Keep behaviour of existing divider clock registration > functions, static initialization helpers as was earlier. > > Signed-off-by: Afzal Mohammed <afzal@ti.com> Hi Afzal, I'd like to understand this a bit better. At first the need for a minimum divider makes a lot of sense, but I want to make sure it gets designed correctly. My first question is whether the minimum divider you plan to use is an actual constraint of the hardware (e.g. the clock controller ip only lets program two bits which divide by 4, 5, 6 or 7, where 4 is the minimum divider) or if this is a functional constraint (e.g. the clock hardware can divide by a lower value, but you never want that since it results in non-functional video/audio/whatever). If this is more of a functional constraint then perhaps a new api like clk_set_min_rate makes more sense. Secondly, have you looked into using the rate-table option provided by the basic divider clock? Can you explain how this is not a good fit for your needs? Perhaps there are too many divisor values so the table would be large? Thanks, Mike
Hi Mike, On Thu, Jan 24, 2013 at 03:10:53, Mike Turquette wrote: > Quoting Afzal Mohammed (2013-01-23 03:38:52) > > Some of clocks can have a limit on minimum divider value that can be > > programmed, prepare for such a support. > > Add a new field min_div for the basic divider clock and a new dynamic > > clock divider registration function where minimum divider value can > > be specified. Keep behaviour of existing divider clock registration > > functions, static initialization helpers as was earlier. > My first question is whether the minimum divider you plan to use is an > actual constraint of the hardware (e.g. the clock controller ip only > lets program two bits which divide by 4, 5, 6 or 7, where 4 is the > minimum divider) or if this is a functional constraint (e.g. the clock > hardware can divide by a lower value, but you never want that since it > results in non-functional video/audio/whatever). If this is more of a > functional constraint then perhaps a new api like clk_set_min_rate makes > more sense. It is a functional constraint: divider has 8 bits and it can have all possible values (0 to 255) and divider value corresponds to value set in the 8 bits. But depending on the modes the minimum value that can be configured (to get display working) varies. Eg. For raster mode (which the driver is presently supporting), it can take a minimum value of 2, while in LIDD (LCD interface display driver) mode it can take a min value of 1. Here min rate is not a constraint w.r.t divider in LCDC IP, but rather min divider. As it is the case, you prefer a clk_divider_set_min_div()? > > Secondly, have you looked into using the rate-table option provided by > the basic divider clock? Can you explain how this is not a good fit for > your needs? Perhaps there are too many divisor values so the table > would be large? Divider values can range from 2-255 (254 possible values), so I believe it is not a suitable candidate here (also divider to values have 1-to-1 mapping) Regards Afzal
Quoting Mohammed, Afzal (2013-01-24 03:29:15) > Hi Mike, > > On Thu, Jan 24, 2013 at 03:10:53, Mike Turquette wrote: > > Quoting Afzal Mohammed (2013-01-23 03:38:52) > > > > Some of clocks can have a limit on minimum divider value that can be > > > programmed, prepare for such a support. > > > > Add a new field min_div for the basic divider clock and a new dynamic > > > clock divider registration function where minimum divider value can > > > be specified. Keep behaviour of existing divider clock registration > > > functions, static initialization helpers as was earlier. > > > My first question is whether the minimum divider you plan to use is an > > actual constraint of the hardware (e.g. the clock controller ip only > > lets program two bits which divide by 4, 5, 6 or 7, where 4 is the > > minimum divider) or if this is a functional constraint (e.g. the clock > > hardware can divide by a lower value, but you never want that since it > > results in non-functional video/audio/whatever). If this is more of a > > functional constraint then perhaps a new api like clk_set_min_rate makes > > more sense. > > It is a functional constraint: divider has 8 bits and it can have > all possible values (0 to 255) and divider value corresponds to > value set in the 8 bits. But depending on the modes the minimum > value that can be configured (to get display working) varies. > Eg. For raster mode (which the driver is presently supporting), it > can take a minimum value of 2, while in LIDD (LCD interface display > driver) mode it can take a min value of 1. > > Here min rate is not a constraint w.r.t divider in LCDC IP, but > rather min divider. > Just so I understand correctly... you are saying that the functional constraint is not caused by the clock rate, but instead by the divider value? For the different modes (raster vs LIDD) is the clock rate the same, or is the clock rate different? What is the clock output rate of the divider in raster mode? What is the clock output rate of the divider in LIDD mode? Thanks, Mike > As it is the case, you prefer a clk_divider_set_min_div()? > > > > > Secondly, have you looked into using the rate-table option provided by > > the basic divider clock? Can you explain how this is not a good fit for > > your needs? Perhaps there are too many divisor values so the table > > would be large? > > Divider values can range from 2-255 (254 possible values), so I believe > it is not a suitable candidate here (also divider to values have 1-to-1 > mapping) > > Regards > Afzal
Hi Mike, On Thu, Jan 24, 2013 at 22:36:30, Mike Turquette wrote: > Quoting Mohammed, Afzal (2013-01-24 03:29:15) > > It is a functional constraint: divider has 8 bits and it can have > > all possible values (0 to 255) and divider value corresponds to > > value set in the 8 bits. But depending on the modes the minimum > > value that can be configured (to get display working) varies. > > Eg. For raster mode (which the driver is presently supporting), it > > can take a minimum value of 2, while in LIDD (LCD interface display > > driver) mode it can take a min value of 1. > > > > Here min rate is not a constraint w.r.t divider in LCDC IP, but > > rather min divider. > Just so I understand correctly... you are saying that the functional > constraint is not caused by the clock rate, but instead by the divider > value? For the different modes (raster vs LIDD) is the clock rate the > same, or is the clock rate different? > What is the clock output rate of the divider in raster mode? What is > the clock output rate of the divider in LIDD mode? Yes, functional constraint in caused by divider value. clock output rate can defined for both modes as follows, p_clk (clock output rate) = lcd_clk (input clock rate) / div, to configure "div", we have r/w 8 bits, so div values can range from 0-255, And IP spec says value "0" and "1" should not be written, in raster mode. Further it says if in LIDD mode it can have values from 0-255, but effect of writing "0" is same as "1". Effect of divider value on output rate is in the same way for both modes as per above expression (except for writing "0" in LIDD mode). The driver supports only raster mode. Regards Afzal Note: link to trm has been mentioned in the earlier reply.
Quoting Mohammed, Afzal (2013-01-25 04:06:41) > Hi Mike, > > On Thu, Jan 24, 2013 at 22:36:30, Mike Turquette wrote: > > Quoting Mohammed, Afzal (2013-01-24 03:29:15) > > > > It is a functional constraint: divider has 8 bits and it can have > > > all possible values (0 to 255) and divider value corresponds to > > > value set in the 8 bits. But depending on the modes the minimum > > > value that can be configured (to get display working) varies. > > > Eg. For raster mode (which the driver is presently supporting), it > > > can take a minimum value of 2, while in LIDD (LCD interface display > > > driver) mode it can take a min value of 1. > > > > > > Here min rate is not a constraint w.r.t divider in LCDC IP, but > > > rather min divider. > > > Just so I understand correctly... you are saying that the functional > > constraint is not caused by the clock rate, but instead by the divider > > value? For the different modes (raster vs LIDD) is the clock rate the > > same, or is the clock rate different? > > > What is the clock output rate of the divider in raster mode? What is > > the clock output rate of the divider in LIDD mode? > > Yes, functional constraint in caused by divider value. > > clock output rate can defined for both modes as follows, > > p_clk (clock output rate) = lcd_clk (input clock rate) / div, > > to configure "div", we have r/w 8 bits, so div values can > range from 0-255, > > And IP spec says value "0" and "1" should not be written, in > raster mode. Further it says if in LIDD mode it can have values > from 0-255, but effect of writing "0" is same as "1". > Afzal, Thank you for the information. In short, the way you program your clock depend on the configuration of your lcdc device. As such I am not sure the basic divider is the right choice for you. You might be better off creating a clock for your IP which takes into account these details. Luckily it is possible to inherit from the basic clock types and create a new type. An example of this is the MXS divider. It uses the basic divider as a "parent class" and adds a busy bit. This is a better approach than putting every feature into the basic divider type. You can see how it was done in drivers/clk/mxs/clk-div.c Let me know what you think. Regards, Mike > Effect of divider value on output rate is in the same way for > both modes as per above expression (except for writing "0" in > LIDD mode). > > The driver supports only raster mode. > > Regards > Afzal > > Note: link to trm has been mentioned in the earlier reply.
Hi Mike, On Sat, Jan 26, 2013 at 04:05:24, Mike Turquette wrote: > Thank you for the information. In short, the way you program your clock > depend on the configuration of your lcdc device. > > As such I am not sure the basic divider is the right choice for you. > You might be better off creating a clock for your IP which takes into > account these details. Luckily it is possible to inherit from the basic > clock types and create a new type. > > An example of this is the MXS divider. It uses the basic divider as a > "parent class" and adds a busy bit. This is a better approach than > putting every feature into the basic divider type. You can see how it > was done in drivers/clk/mxs/clk-div.c > > Let me know what you think. Yes, a derived divider is better. As mentioned in other thread, the modeling of clock nodes (derived plus gates) would bring in considerable (relative to complete driver) code, the advantage being higher pixel clock resolution. Current use cases work as per existing divider setting in driver. Hence now it seems a better decision now is to proceed with logic as in v2 (not using clock nodes). And later depending on the use case requirement, clock tree modeling can be implemented. Thanks for your input. Regards Afzal
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index a9204c6..4025c5a 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -236,7 +236,7 @@ EXPORT_SYMBOL_GPL(clk_divider_ops); static struct clk *_register_divider(struct device *dev, const char *name, const char *parent_name, unsigned long flags, - void __iomem *reg, u8 shift, u8 width, + void __iomem *reg, u8 shift, u8 width, u8 min_div, u8 clk_divider_flags, const struct clk_div_table *table, spinlock_t *lock) { @@ -244,6 +244,11 @@ static struct clk *_register_divider(struct device *dev, const char *name, struct clk *clk; struct clk_init_data init; + if (!min_div) { + pr_err("%s: minimum divider cannot be zero\n", __func__); + return ERR_PTR(-EINVAL); + } + /* allocate the divider */ div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL); if (!div) { @@ -261,6 +266,7 @@ static struct clk *_register_divider(struct device *dev, const char *name, div->reg = reg; div->shift = shift; div->width = width; + div->min_div = min_div; div->flags = clk_divider_flags; div->lock = lock; div->hw.init = &init; @@ -276,6 +282,29 @@ static struct clk *_register_divider(struct device *dev, const char *name, } /** + * clk_register_min_divider - register a divider clock having minimum divider + * constraints with clock framework + * @dev: device registering this clock + * @name: name of this clock + * @parent_name: name of clock's parent + * @flags: framework-specific flags + * @reg: register address to adjust divider + * @shift: number of bits to shift the bitfield + * @width: width of the bitfield + * @min_div: minimum allowable divider + * @clk_divider_flags: divider-specific flags for this clock + * @lock: shared register lock for this clock + */ +struct clk *clk_register_min_divider(struct device *dev, const char *name, + const char *parent_name, unsigned long flags, + void __iomem *reg, u8 shift, u8 width, u8 min_div, + u8 clk_divider_flags, spinlock_t *lock) +{ + return _register_divider(dev, name, parent_name, flags, reg, shift, + width, min_div, clk_divider_flags, NULL, lock); +} + +/** * clk_register_divider - register a divider clock with the clock framework * @dev: device registering this clock * @name: name of this clock @@ -293,7 +322,8 @@ struct clk *clk_register_divider(struct device *dev, const char *name, u8 clk_divider_flags, spinlock_t *lock) { return _register_divider(dev, name, parent_name, flags, reg, shift, - width, clk_divider_flags, NULL, lock); + width, CLK_DIVIDER_MIN_DIV_DEFAULT, clk_divider_flags, + NULL, lock); } /** @@ -317,5 +347,6 @@ struct clk *clk_register_divider_table(struct device *dev, const char *name, spinlock_t *lock) { return _register_divider(dev, name, parent_name, flags, reg, shift, - width, clk_divider_flags, table, lock); + width, CLK_DIVIDER_MIN_DIV_DEFAULT, clk_divider_flags, + table, lock); } diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h index 9c7f580..942a1be 100644 --- a/include/linux/clk-private.h +++ b/include/linux/clk-private.h @@ -105,7 +105,8 @@ struct clk { #define _DEFINE_CLK_DIVIDER(_name, _parent_name, _parent_ptr, \ _flags, _reg, _shift, _width, \ - _divider_flags, _table, _lock) \ + _min_div, _divider_flags, \ + _table, _lock) \ static struct clk _name; \ static const char *_name##_parent_names[] = { \ _parent_name, \ @@ -120,6 +121,7 @@ struct clk { .reg = _reg, \ .shift = _shift, \ .width = _width, \ + .min_div = _min_div, \ .flags = _divider_flags, \ .table = _table, \ .lock = _lock, \ @@ -132,6 +134,7 @@ struct clk { _divider_flags, _lock) \ _DEFINE_CLK_DIVIDER(_name, _parent_name, _parent_ptr, \ _flags, _reg, _shift, _width, \ + CLK_DIVIDER_MIN_DIV_DEFAULT, \ _divider_flags, NULL, _lock) #define DEFINE_CLK_DIVIDER_TABLE(_name, _parent_name, \ @@ -140,6 +143,7 @@ struct clk { _table, _lock) \ _DEFINE_CLK_DIVIDER(_name, _parent_name, _parent_ptr, \ _flags, _reg, _shift, _width, \ + CLK_DIVIDER_MIN_DIV_DEFAULT, \ _divider_flags, _table, _lock) \ #define DEFINE_CLK_MUX(_name, _parent_names, _parents, _flags, \ diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 4989b8a..1c09481 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -248,15 +248,22 @@ struct clk_divider { void __iomem *reg; u8 shift; u8 width; + u8 min_div; u8 flags; const struct clk_div_table *table; spinlock_t *lock; }; +#define CLK_DIVIDER_MIN_DIV_DEFAULT 1 + #define CLK_DIVIDER_ONE_BASED BIT(0) #define CLK_DIVIDER_POWER_OF_TWO BIT(1) extern const struct clk_ops clk_divider_ops; +struct clk *clk_register_min_divider(struct device *dev, const char *name, + const char *parent_name, unsigned long flags, + void __iomem *reg, u8 shift, u8 width, u8 min_div, + u8 clk_divider_flags, spinlock_t *lock); struct clk *clk_register_divider(struct device *dev, const char *name, const char *parent_name, unsigned long flags, void __iomem *reg, u8 shift, u8 width,
Some of clocks can have a limit on minimum divider value that can be programmed, prepare for such a support. Add a new field min_div for the basic divider clock and a new dynamic clock divider registration function where minimum divider value can be specified. Keep behaviour of existing divider clock registration functions, static initialization helpers as was earlier. Signed-off-by: Afzal Mohammed <afzal@ti.com> --- v2: create a new registration function for those that needs to constrain minimum divider value instead of modifying existing registration functions and hence remove modification in other clock files. drivers/clk/clk-divider.c | 37 ++++++++++++++++++++++++++++++++++--- include/linux/clk-private.h | 6 +++++- include/linux/clk-provider.h | 7 +++++++ 3 files changed, 46 insertions(+), 4 deletions(-)