[PATCH v2 0/1] i2c: aspeed: add proper support fo 24xx clock params

[PATCH v2 0/1] i2c: aspeed: add proper support fo 24xx clock params

[Brendan Higgins]

This fixes a minor issue pointed out by Robi <rburanyi@google.com> and also adds
a couple improvements that he suggested outside of the mailing list.

Tested on the Aspeed 2500.

[PATCH v2 1/1] i2c: aspeed: add proper support fo 24xx clock params

[Brendan Higgins]

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

Re: [PATCH v2 1/1] i2c: aspeed: add proper support fo 24xx clock params

[Rick Altherr]

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
>>
>

Re: [PATCH v2 1/1] i2c: aspeed: add proper support fo 24xx clock params

[Brendan Higgins]

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.

Re: [PATCH v2 1/1] i2c: aspeed: add proper support fo 24xx clock params

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]


> 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...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/1] i2c: aspeed: add proper support fo 24xx clock params

[Brendan Higgins]

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!

Re: [PATCH v2 1/1] i2c: aspeed: add proper support fo 24xx clock params

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]