| Message ID | 20170728204558.6455-2-brendanhiggins@google.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Is clk_fractional_divider from include/linux/clk-provider.h appropriate here? On Fri, Jul 28, 2017 at 1:45 PM, Brendan Higgins <brendanhiggins@google.com> wrote: > 24xx BMCs have larger clock divider granularity which can cause problems > when trying to set them as 25xx clock dividers; this adds clock setting > code specific to 24xx. > > This also fixes a potential issue where clock dividers were rounded down > instead of up. > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com> > --- > Changes for v2: > - Fixed off by one error to check for divisors with base_clk == 0 > - Simplified some of the divisor math > --- > drivers/i2c/busses/i2c-aspeed.c | 74 ++++++++++++++++++++++++++++++ > +---------- > 1 file changed, 56 insertions(+), 18 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c- > aspeed.c > index f19348328a71..60afab866494 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -132,6 +132,7 @@ struct aspeed_i2c_bus { > /* Synchronizes I/O mem access to base. */ > spinlock_t lock; > struct completion cmd_complete; > + u32 (*get_clk_reg_val)(u32 divisor); > unsigned long parent_clk_frequency; > u32 bus_frequency; > /* Transaction state. */ > @@ -674,7 +675,7 @@ static const struct i2c_algorithm aspeed_i2c_algo = { > #endif /* CONFIG_I2C_SLAVE */ > }; > > -static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) > +static u32 aspeed_i2c_get_clk_reg_val(u32 clk_high_low_max, u32 divisor) > { > u32 base_clk, clk_high, clk_low, tmp; > > @@ -694,16 +695,22 @@ static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) > * Thus, > * SCL_freq = APB_freq / > * ((1 << base_clk) * (clk_high + 1 + clk_low + 1)) > - * The documentation recommends clk_high >= 8 and clk_low >= 7 when > - * possible; this last constraint gives us the following solution: > + * The documentation recommends clk_high >= clk_high_max / 2 and > + * clk_low >= clk_low_max / 2 - 1 when possible; this last > constraint > + * gives us the following solution: > */ > - base_clk = divisor > 33 ? ilog2((divisor - 1) / 32) + 1 : 0; > - tmp = divisor / (1 << base_clk); > - clk_high = tmp / 2 + tmp % 2; > - clk_low = tmp - clk_high; > + base_clk = divisor > clk_high_low_max ? > + ilog2((divisor - 1) / clk_high_low_max) + 1 : 0; > + tmp = (divisor + (1 << base_clk) - 1) >> base_clk; > + clk_low = tmp / 2; > + clk_high = tmp - clk_low; > + > + if (clk_high) > + clk_high--; > + > + if (clk_low) > + clk_low--; > > - clk_high -= 1; > - clk_low -= 1; > > return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT) > & ASPEED_I2CD_TIME_SCL_HIGH_MASK) > @@ -712,13 +719,31 @@ static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) > | (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK); > } > > +static u32 aspeed_i2c_24xx_get_clk_reg_val(u32 divisor) > +{ > + /* > + * clk_high and clk_low are each 3 bits wide, so each can hold a > max > + * value of 8 giving a clk_high_low_max of 16. > + */ > + return aspeed_i2c_get_clk_reg_val(16, divisor); > +} > + > +static u32 aspeed_i2c_25xx_get_clk_reg_val(u32 divisor) > +{ > + /* > + * clk_high and clk_low are each 4 bits wide, so each can hold a > max > + * value of 16 giving a clk_high_low_max of 32. > + */ > + return aspeed_i2c_get_clk_reg_val(32, divisor); > +} > + > /* precondition: bus.lock has been acquired. */ > static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus) > { > u32 divisor, clk_reg_val; > > - divisor = bus->parent_clk_frequency / bus->bus_frequency; > - clk_reg_val = aspeed_i2c_get_clk_reg_val(divisor); > + divisor = DIV_ROUND_UP(bus->parent_clk_frequency, > bus->bus_frequency); > + clk_reg_val = bus->get_clk_reg_val(divisor); > writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1); > writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + > ASPEED_I2C_AC_TIMING_REG2); > > @@ -777,8 +802,22 @@ static int aspeed_i2c_reset(struct aspeed_i2c_bus > *bus) > return ret; > } > > +static const struct of_device_id aspeed_i2c_bus_of_table[] = { > + { > + .compatible = "aspeed,ast2400-i2c-bus", > + .data = aspeed_i2c_24xx_get_clk_reg_val, > + }, > + { > + .compatible = "aspeed,ast2500-i2c-bus", > + .data = aspeed_i2c_25xx_get_clk_reg_val, > + }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table); > + > static int aspeed_i2c_probe_bus(struct platform_device *pdev) > { > + const struct of_device_id *match; > struct aspeed_i2c_bus *bus; > struct clk *parent_clk; > struct resource *res; > @@ -808,6 +847,12 @@ static int aspeed_i2c_probe_bus(struct > platform_device *pdev) > bus->bus_frequency = 100000; > } > > + match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node); > + if (!match) > + bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val; > + else > + bus->get_clk_reg_val = match->data; > + > /* Initialize the I2C adapter */ > spin_lock_init(&bus->lock); > init_completion(&bus->cmd_complete); > @@ -869,13 +914,6 @@ static int aspeed_i2c_remove_bus(struct > platform_device *pdev) > return 0; > } > > -static const struct of_device_id aspeed_i2c_bus_of_table[] = { > - { .compatible = "aspeed,ast2400-i2c-bus", }, > - { .compatible = "aspeed,ast2500-i2c-bus", }, > - { }, > -}; > -MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table); > - > static struct platform_driver aspeed_i2c_bus_driver = { > .probe = aspeed_i2c_probe_bus, > .remove = aspeed_i2c_remove_bus, > -- > 2.14.0.rc0.400.g1c36432dff-goog > >
Is clk_fractional_divider from include/linux/clk-provider.h appropriate here? On Fri, Jul 28, 2017 at 1:57 PM, Rick Altherr <raltherr@google.com> wrote: > Is clk_fractional_divider from include/linux/clk-provider.h appropriate > here? > > On Fri, Jul 28, 2017 at 1:45 PM, Brendan Higgins <brendanhiggins@google.com> > wrote: >> >> 24xx BMCs have larger clock divider granularity which can cause problems >> when trying to set them as 25xx clock dividers; this adds clock setting >> code specific to 24xx. >> >> This also fixes a potential issue where clock dividers were rounded down >> instead of up. >> >> Signed-off-by: Brendan Higgins <brendanhiggins@google.com> >> --- >> Changes for v2: >> - Fixed off by one error to check for divisors with base_clk == 0 >> - Simplified some of the divisor math >> --- >> drivers/i2c/busses/i2c-aspeed.c | 74 >> +++++++++++++++++++++++++++++++---------- >> 1 file changed, 56 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-aspeed.c >> b/drivers/i2c/busses/i2c-aspeed.c >> index f19348328a71..60afab866494 100644 >> --- a/drivers/i2c/busses/i2c-aspeed.c >> +++ b/drivers/i2c/busses/i2c-aspeed.c >> @@ -132,6 +132,7 @@ struct aspeed_i2c_bus { >> /* Synchronizes I/O mem access to base. */ >> spinlock_t lock; >> struct completion cmd_complete; >> + u32 (*get_clk_reg_val)(u32 divisor); >> unsigned long parent_clk_frequency; >> u32 bus_frequency; >> /* Transaction state. */ >> @@ -674,7 +675,7 @@ static const struct i2c_algorithm aspeed_i2c_algo = { >> #endif /* CONFIG_I2C_SLAVE */ >> }; >> >> -static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) >> +static u32 aspeed_i2c_get_clk_reg_val(u32 clk_high_low_max, u32 divisor) >> { >> u32 base_clk, clk_high, clk_low, tmp; >> >> @@ -694,16 +695,22 @@ static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) >> * Thus, >> * SCL_freq = APB_freq / >> * ((1 << base_clk) * (clk_high + 1 + clk_low + 1)) >> - * The documentation recommends clk_high >= 8 and clk_low >= 7 >> when >> - * possible; this last constraint gives us the following solution: >> + * The documentation recommends clk_high >= clk_high_max / 2 and >> + * clk_low >= clk_low_max / 2 - 1 when possible; this last >> constraint >> + * gives us the following solution: >> */ >> - base_clk = divisor > 33 ? ilog2((divisor - 1) / 32) + 1 : 0; >> - tmp = divisor / (1 << base_clk); >> - clk_high = tmp / 2 + tmp % 2; >> - clk_low = tmp - clk_high; >> + base_clk = divisor > clk_high_low_max ? >> + ilog2((divisor - 1) / clk_high_low_max) + 1 : 0; >> + tmp = (divisor + (1 << base_clk) - 1) >> base_clk; >> + clk_low = tmp / 2; >> + clk_high = tmp - clk_low; >> + >> + if (clk_high) >> + clk_high--; >> + >> + if (clk_low) >> + clk_low--; >> >> - clk_high -= 1; >> - clk_low -= 1; >> >> return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT) >> & ASPEED_I2CD_TIME_SCL_HIGH_MASK) >> @@ -712,13 +719,31 @@ static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) >> | (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK); >> } >> >> +static u32 aspeed_i2c_24xx_get_clk_reg_val(u32 divisor) >> +{ >> + /* >> + * clk_high and clk_low are each 3 bits wide, so each can hold a >> max >> + * value of 8 giving a clk_high_low_max of 16. >> + */ >> + return aspeed_i2c_get_clk_reg_val(16, divisor); >> +} >> + >> +static u32 aspeed_i2c_25xx_get_clk_reg_val(u32 divisor) >> +{ >> + /* >> + * clk_high and clk_low are each 4 bits wide, so each can hold a >> max >> + * value of 16 giving a clk_high_low_max of 32. >> + */ >> + return aspeed_i2c_get_clk_reg_val(32, divisor); >> +} >> + >> /* precondition: bus.lock has been acquired. */ >> static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus) >> { >> u32 divisor, clk_reg_val; >> >> - divisor = bus->parent_clk_frequency / bus->bus_frequency; >> - clk_reg_val = aspeed_i2c_get_clk_reg_val(divisor); >> + divisor = DIV_ROUND_UP(bus->parent_clk_frequency, >> bus->bus_frequency); >> + clk_reg_val = bus->get_clk_reg_val(divisor); >> writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1); >> writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + >> ASPEED_I2C_AC_TIMING_REG2); >> >> @@ -777,8 +802,22 @@ static int aspeed_i2c_reset(struct aspeed_i2c_bus >> *bus) >> return ret; >> } >> >> +static const struct of_device_id aspeed_i2c_bus_of_table[] = { >> + { >> + .compatible = "aspeed,ast2400-i2c-bus", >> + .data = aspeed_i2c_24xx_get_clk_reg_val, >> + }, >> + { >> + .compatible = "aspeed,ast2500-i2c-bus", >> + .data = aspeed_i2c_25xx_get_clk_reg_val, >> + }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table); >> + >> static int aspeed_i2c_probe_bus(struct platform_device *pdev) >> { >> + const struct of_device_id *match; >> struct aspeed_i2c_bus *bus; >> struct clk *parent_clk; >> struct resource *res; >> @@ -808,6 +847,12 @@ static int aspeed_i2c_probe_bus(struct >> platform_device *pdev) >> bus->bus_frequency = 100000; >> } >> >> + match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node); >> + if (!match) >> + bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val; >> + else >> + bus->get_clk_reg_val = match->data; >> + >> /* Initialize the I2C adapter */ >> spin_lock_init(&bus->lock); >> init_completion(&bus->cmd_complete); >> @@ -869,13 +914,6 @@ static int aspeed_i2c_remove_bus(struct >> platform_device *pdev) >> return 0; >> } >> >> -static const struct of_device_id aspeed_i2c_bus_of_table[] = { >> - { .compatible = "aspeed,ast2400-i2c-bus", }, >> - { .compatible = "aspeed,ast2500-i2c-bus", }, >> - { }, >> -}; >> -MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table); >> - >> static struct platform_driver aspeed_i2c_bus_driver = { >> .probe = aspeed_i2c_probe_bus, >> .remove = aspeed_i2c_remove_bus, >> -- >> 2.14.0.rc0.400.g1c36432dff-goog >> >
On Fri, Jul 28, 2017 at 2:00 PM, Rick Altherr <raltherr@google.com> wrote: > Is clk_fractional_divider from include/linux/clk-provider.h appropriate here? > Alas, no. clk_fractional_divider is not flexible enough to specify the divider the way that it is represented in the Aspeed 24xx/25xx parts which have the divider expressed as a "base clock" which is always a power of 2 along with the time where SCL is high and the time that the SCL is low in units of base clock. Thus, there are two separate "numerator" values and the denominator is represented as the ilog2 of the actual value. That being said, I could implement this as a custom clock subclass, which would probably be cleaner that what I have done.
> That being said, I could implement this as a custom clock subclass, which > would probably be cleaner that what I have done. Shall I wait for that one or do you want this patch to be included? I don't mind, your call here...
On Sat, Aug 12, 2017 at 4:30 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> That being said, I could implement this as a custom clock subclass, which >> would probably be cleaner that what I have done. > > Shall I wait for that one or do you want this patch to be included? > I don't mind, your call here... > Let's go ahead with this patch. I do not have too much experience with the clock stuff, so I imagine that will probably take some back and forth. Thanks!
On Fri, Jul 28, 2017 at 01:45:58PM -0700, Brendan Higgins wrote: > 24xx BMCs have larger clock divider granularity which can cause problems > when trying to set them as 25xx clock dividers; this adds clock setting > code specific to 24xx. > > This also fixes a potential issue where clock dividers were rounded down > instead of up. > > Signed-off-by: Brendan Higgins <brendanhiggins@google.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index f19348328a71..60afab866494 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -132,6 +132,7 @@ struct aspeed_i2c_bus { /* Synchronizes I/O mem access to base. */ spinlock_t lock; struct completion cmd_complete; + u32 (*get_clk_reg_val)(u32 divisor); unsigned long parent_clk_frequency; u32 bus_frequency; /* Transaction state. */ @@ -674,7 +675,7 @@ static const struct i2c_algorithm aspeed_i2c_algo = { #endif /* CONFIG_I2C_SLAVE */ }; -static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) +static u32 aspeed_i2c_get_clk_reg_val(u32 clk_high_low_max, u32 divisor) { u32 base_clk, clk_high, clk_low, tmp; @@ -694,16 +695,22 @@ static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) * Thus, * SCL_freq = APB_freq / * ((1 << base_clk) * (clk_high + 1 + clk_low + 1)) - * The documentation recommends clk_high >= 8 and clk_low >= 7 when - * possible; this last constraint gives us the following solution: + * The documentation recommends clk_high >= clk_high_max / 2 and + * clk_low >= clk_low_max / 2 - 1 when possible; this last constraint + * gives us the following solution: */ - base_clk = divisor > 33 ? ilog2((divisor - 1) / 32) + 1 : 0; - tmp = divisor / (1 << base_clk); - clk_high = tmp / 2 + tmp % 2; - clk_low = tmp - clk_high; + base_clk = divisor > clk_high_low_max ? + ilog2((divisor - 1) / clk_high_low_max) + 1 : 0; + tmp = (divisor + (1 << base_clk) - 1) >> base_clk; + clk_low = tmp / 2; + clk_high = tmp - clk_low; + + if (clk_high) + clk_high--; + + if (clk_low) + clk_low--; - clk_high -= 1; - clk_low -= 1; return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT) & ASPEED_I2CD_TIME_SCL_HIGH_MASK) @@ -712,13 +719,31 @@ static u32 aspeed_i2c_get_clk_reg_val(u32 divisor) | (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK); } +static u32 aspeed_i2c_24xx_get_clk_reg_val(u32 divisor) +{ + /* + * clk_high and clk_low are each 3 bits wide, so each can hold a max + * value of 8 giving a clk_high_low_max of 16. + */ + return aspeed_i2c_get_clk_reg_val(16, divisor); +} + +static u32 aspeed_i2c_25xx_get_clk_reg_val(u32 divisor) +{ + /* + * clk_high and clk_low are each 4 bits wide, so each can hold a max + * value of 16 giving a clk_high_low_max of 32. + */ + return aspeed_i2c_get_clk_reg_val(32, divisor); +} + /* precondition: bus.lock has been acquired. */ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus) { u32 divisor, clk_reg_val; - divisor = bus->parent_clk_frequency / bus->bus_frequency; - clk_reg_val = aspeed_i2c_get_clk_reg_val(divisor); + divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency); + clk_reg_val = bus->get_clk_reg_val(divisor); writel(clk_reg_val, bus->base + ASPEED_I2C_AC_TIMING_REG1); writel(ASPEED_NO_TIMEOUT_CTRL, bus->base + ASPEED_I2C_AC_TIMING_REG2); @@ -777,8 +802,22 @@ static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus) return ret; } +static const struct of_device_id aspeed_i2c_bus_of_table[] = { + { + .compatible = "aspeed,ast2400-i2c-bus", + .data = aspeed_i2c_24xx_get_clk_reg_val, + }, + { + .compatible = "aspeed,ast2500-i2c-bus", + .data = aspeed_i2c_25xx_get_clk_reg_val, + }, + { }, +}; +MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table); + static int aspeed_i2c_probe_bus(struct platform_device *pdev) { + const struct of_device_id *match; struct aspeed_i2c_bus *bus; struct clk *parent_clk; struct resource *res; @@ -808,6 +847,12 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev) bus->bus_frequency = 100000; } + match = of_match_node(aspeed_i2c_bus_of_table, pdev->dev.of_node); + if (!match) + bus->get_clk_reg_val = aspeed_i2c_24xx_get_clk_reg_val; + else + bus->get_clk_reg_val = match->data; + /* Initialize the I2C adapter */ spin_lock_init(&bus->lock); init_completion(&bus->cmd_complete); @@ -869,13 +914,6 @@ static int aspeed_i2c_remove_bus(struct platform_device *pdev) return 0; } -static const struct of_device_id aspeed_i2c_bus_of_table[] = { - { .compatible = "aspeed,ast2400-i2c-bus", }, - { .compatible = "aspeed,ast2500-i2c-bus", }, - { }, -}; -MODULE_DEVICE_TABLE(of, aspeed_i2c_bus_of_table); - static struct platform_driver aspeed_i2c_bus_driver = { .probe = aspeed_i2c_probe_bus, .remove = aspeed_i2c_remove_bus,
24xx BMCs have larger clock divider granularity which can cause problems when trying to set them as 25xx clock dividers; this adds clock setting code specific to 24xx. This also fixes a potential issue where clock dividers were rounded down instead of up. Signed-off-by: Brendan Higgins <brendanhiggins@google.com> --- Changes for v2: - Fixed off by one error to check for divisors with base_clk == 0 - Simplified some of the divisor math --- drivers/i2c/busses/i2c-aspeed.c | 74 +++++++++++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 18 deletions(-)