[PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Andrew Jeffery]

In addition to the base, low and high clock configuration, the AC timing
register #1 on the AST2400 houses fields controlling:

1. tBUF: Minimum delay between Stop and Start conditions
2. tHDSTA: Hold time for the Start condition
3. tACST: Setup time for Start and Stop conditions, and hold time for the
   Repeated Start condition

These values are defined in hardware on the AST2500 and therefore don't
need to be set.

aspeed_i2c_init_clk() was performing a direct write of the generated
clock values rather than a read/mask/modify/update sequence to retain
tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
fields on the AST2400. This resulted in a delay/setup/hold time of 1
base clock, which in some configurations is not enough for some devices
(e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
bus speed of 100kHz).

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/i2c/busses/i2c-aspeed.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index ee76e6dddc4b..284f8670dbeb 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -53,6 +53,9 @@
 #define ASPEED_I2CD_MASTER_EN				BIT(0)
 
 /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
+#define ASPEED_I2CD_TIME_TBUF_MASK			GENMASK(31, 28)
+#define ASPEED_I2CD_TIME_THDSTA_MASK			GENMASK(27, 24)
+#define ASPEED_I2CD_TIME_TACST_MASK			GENMASK(23, 20)
 #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT			16
 #define ASPEED_I2CD_TIME_SCL_HIGH_MASK			GENMASK(19, 16)
 #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT			12
@@ -744,7 +747,11 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
 	u32 divisor, clk_reg_val;
 
 	divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
-	clk_reg_val = bus->get_clk_reg_val(divisor);
+	clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
+	clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
+			ASPEED_I2CD_TIME_THDSTA_MASK |
+			ASPEED_I2CD_TIME_TACST_MASK);
+	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);
 
-- 
2.11.0

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Brendan Higgins]

On Tue, Aug 15, 2017 at 10:21 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> In addition to the base, low and high clock configuration, the AC timing
> register #1 on the AST2400 houses fields controlling:
>
> 1. tBUF: Minimum delay between Stop and Start conditions
> 2. tHDSTA: Hold time for the Start condition
> 3. tACST: Setup time for Start and Stop conditions, and hold time for the
>    Repeated Start condition
>
> These values are defined in hardware on the AST2500 and therefore don't
> need to be set.
>
> aspeed_i2c_init_clk() was performing a direct write of the generated
> clock values rather than a read/mask/modify/update sequence to retain
> tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> fields on the AST2400. This resulted in a delay/setup/hold time of 1
> base clock, which in some configurations is not enough for some devices
> (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> bus speed of 100kHz).
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index ee76e6dddc4b..284f8670dbeb 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -53,6 +53,9 @@
>  #define ASPEED_I2CD_MASTER_EN                          BIT(0)
>
>  /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_TBUF_MASK                     GENMASK(31, 28)
> +#define ASPEED_I2CD_TIME_THDSTA_MASK                   GENMASK(27, 24)
> +#define ASPEED_I2CD_TIME_TACST_MASK                    GENMASK(23, 20)
>  #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
>  #define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
>  #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
> @@ -744,7 +747,11 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>         u32 divisor, clk_reg_val;
>
>         divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
> -       clk_reg_val = bus->get_clk_reg_val(divisor);
> +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
> +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
> +                       ASPEED_I2CD_TIME_THDSTA_MASK |
> +                       ASPEED_I2CD_TIME_TACST_MASK);
> +       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);
>
> --
> 2.11.0
>

Awesome! I think this might fix an issue we saw on one of our boards.
I am out of the country right now, so I cannot test this myself, until Monday.

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Brendan Higgins]

On Tue, Aug 15, 2017 at 10:21 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> In addition to the base, low and high clock configuration, the AC timing
> register #1 on the AST2400 houses fields controlling:
>
> 1. tBUF: Minimum delay between Stop and Start conditions
> 2. tHDSTA: Hold time for the Start condition
> 3. tACST: Setup time for Start and Stop conditions, and hold time for the
>    Repeated Start condition
>
> These values are defined in hardware on the AST2500 and therefore don't
> need to be set.
>
> aspeed_i2c_init_clk() was performing a direct write of the generated
> clock values rather than a read/mask/modify/update sequence to retain
> tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> fields on the AST2400. This resulted in a delay/setup/hold time of 1
> base clock, which in some configurations is not enough for some devices
> (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> bus speed of 100kHz).
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index ee76e6dddc4b..284f8670dbeb 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -53,6 +53,9 @@
>  #define ASPEED_I2CD_MASTER_EN                          BIT(0)
>
>  /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_TBUF_MASK                     GENMASK(31, 28)
> +#define ASPEED_I2CD_TIME_THDSTA_MASK                   GENMASK(27, 24)
> +#define ASPEED_I2CD_TIME_TACST_MASK                    GENMASK(23, 20)
>  #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
>  #define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
>  #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
> @@ -744,7 +747,11 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>         u32 divisor, clk_reg_val;
>
>         divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
> -       clk_reg_val = bus->get_clk_reg_val(divisor);
> +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
> +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
> +                       ASPEED_I2CD_TIME_THDSTA_MASK |
> +                       ASPEED_I2CD_TIME_TACST_MASK);
> +       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);
>
> --
> 2.11.0
>

Awesome! I think this might fix an issue we saw on one of our boards.
I am out of the country right now, so I cannot test this myself, until Monday.

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

On Tue, Aug 15, 2017 at 4:51 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> In addition to the base, low and high clock configuration, the AC timing
> register #1 on the AST2400 houses fields controlling:
>
> 1. tBUF: Minimum delay between Stop and Start conditions
> 2. tHDSTA: Hold time for the Start condition
> 3. tACST: Setup time for Start and Stop conditions, and hold time for the
>    Repeated Start condition
>
> These values are defined in hardware on the AST2500 and therefore don't
> need to be set.
>
> aspeed_i2c_init_clk() was performing a direct write of the generated
> clock values rather than a read/mask/modify/update sequence to retain
> tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> fields on the AST2400. This resulted in a delay/setup/hold time of 1
> base clock, which in some configurations is not enough for some devices
> (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> bus speed of 100kHz).
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index ee76e6dddc4b..284f8670dbeb 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -53,6 +53,9 @@
>  #define ASPEED_I2CD_MASTER_EN                          BIT(0)
>
>  /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_TBUF_MASK                     GENMASK(31, 28)
> +#define ASPEED_I2CD_TIME_THDSTA_MASK                   GENMASK(27, 24)
> +#define ASPEED_I2CD_TIME_TACST_MASK                    GENMASK(23, 20)
>  #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
>  #define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
>  #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
> @@ -744,7 +747,11 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>         u32 divisor, clk_reg_val;
>
>         divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
> -       clk_reg_val = bus->get_clk_reg_val(divisor);
> +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
> +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
> +                       ASPEED_I2CD_TIME_THDSTA_MASK |
> +                       ASPEED_I2CD_TIME_TACST_MASK);

Instead of keeping the u-boot values (which appear to be hard-coded),
should we instead put the known working values in the register?

Cheers,

Joel

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Joel Stanley]

On Tue, Aug 15, 2017 at 4:51 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> In addition to the base, low and high clock configuration, the AC timing
> register #1 on the AST2400 houses fields controlling:
>
> 1. tBUF: Minimum delay between Stop and Start conditions
> 2. tHDSTA: Hold time for the Start condition
> 3. tACST: Setup time for Start and Stop conditions, and hold time for the
>    Repeated Start condition
>
> These values are defined in hardware on the AST2500 and therefore don't
> need to be set.
>
> aspeed_i2c_init_clk() was performing a direct write of the generated
> clock values rather than a read/mask/modify/update sequence to retain
> tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> fields on the AST2400. This resulted in a delay/setup/hold time of 1
> base clock, which in some configurations is not enough for some devices
> (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> bus speed of 100kHz).
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index ee76e6dddc4b..284f8670dbeb 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -53,6 +53,9 @@
>  #define ASPEED_I2CD_MASTER_EN                          BIT(0)
>
>  /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_TBUF_MASK                     GENMASK(31, 28)
> +#define ASPEED_I2CD_TIME_THDSTA_MASK                   GENMASK(27, 24)
> +#define ASPEED_I2CD_TIME_TACST_MASK                    GENMASK(23, 20)
>  #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
>  #define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
>  #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
> @@ -744,7 +747,11 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>         u32 divisor, clk_reg_val;
>
>         divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
> -       clk_reg_val = bus->get_clk_reg_val(divisor);
> +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
> +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
> +                       ASPEED_I2CD_TIME_THDSTA_MASK |
> +                       ASPEED_I2CD_TIME_TACST_MASK);

Instead of keeping the u-boot values (which appear to be hard-coded),
should we instead put the known working values in the register?

Cheers,

Joel

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Cédric Le Goater]

On 08/16/2017 08:49 AM, Joel Stanley wrote:
> On Tue, Aug 15, 2017 at 4:51 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
>> In addition to the base, low and high clock configuration, the AC timing
>> register #1 on the AST2400 houses fields controlling:
>>
>> 1. tBUF: Minimum delay between Stop and Start conditions
>> 2. tHDSTA: Hold time for the Start condition
>> 3. tACST: Setup time for Start and Stop conditions, and hold time for the
>>    Repeated Start condition
>>
>> These values are defined in hardware on the AST2500 and therefore don't
>> need to be set.
>>
>> aspeed_i2c_init_clk() was performing a direct write of the generated
>> clock values rather than a read/mask/modify/update sequence to retain
>> tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
>> fields on the AST2400. This resulted in a delay/setup/hold time of 1
>> base clock, which in some configurations is not enough for some devices
>> (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
>> bus speed of 100kHz).
>>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> ---
>>  drivers/i2c/busses/i2c-aspeed.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index ee76e6dddc4b..284f8670dbeb 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -53,6 +53,9 @@
>>  #define ASPEED_I2CD_MASTER_EN                          BIT(0)
>>
>>  /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
>> +#define ASPEED_I2CD_TIME_TBUF_MASK                     GENMASK(31, 28)
>> +#define ASPEED_I2CD_TIME_THDSTA_MASK                   GENMASK(27, 24)
>> +#define ASPEED_I2CD_TIME_TACST_MASK                    GENMASK(23, 20)
>>  #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
>>  #define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
>>  #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
>> @@ -744,7 +747,11 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>>         u32 divisor, clk_reg_val;
>>
>>         divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
>> -       clk_reg_val = bus->get_clk_reg_val(divisor);
>> +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
>> +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
>> +                       ASPEED_I2CD_TIME_THDSTA_MASK |
>> +                       ASPEED_I2CD_TIME_TACST_MASK);
> 
> Instead of keeping the u-boot values (which appear to be hard-coded),
> should we instead put the known working values in the register?

Yes. I was wondering where the initial setting was from on the AST400.

C.

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Benjamin Herrenschmidt]

On Wed, 2017-08-16 at 08:53 +0200, Cédric Le Goater wrote:
> > >          divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
> > > -       clk_reg_val = bus->get_clk_reg_val(divisor);
> > > +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
> > > +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
> > > +                       ASPEED_I2CD_TIME_THDSTA_MASK |
> > > +                       ASPEED_I2CD_TIME_TACST_MASK);
> > 
> > Instead of keeping the u-boot values (which appear to be hard-coded),
> > should we instead put the known working values in the register?
> 
> Yes. I was wondering where the initial setting was from on the AST400.

Well, the AST2500 hard codes them in HW, so it makes some amount of
sense to use whatever aspeed platform.S hard codes in u-boot for the
2400. The values look reasonably sane.

If we ever see a need to change them, we can add DT props etc... but
for now I'd just not bother.

The way it is now, at least, if I have problems, I can tweak the values
with devmem and try again without the driver overwriting them :-)

Cheers,
Ben.

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Cédric Le Goater]

On 08/16/2017 08:59 AM, Benjamin Herrenschmidt wrote:
> On Wed, 2017-08-16 at 08:53 +0200, Cédric Le Goater wrote:
>>>>          divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
>>>> -       clk_reg_val = bus->get_clk_reg_val(divisor);
>>>> +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
>>>> +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
>>>> +                       ASPEED_I2CD_TIME_THDSTA_MASK |
>>>> +                       ASPEED_I2CD_TIME_TACST_MASK);
>>>
>>> Instead of keeping the u-boot values (which appear to be hard-coded),
>>> should we instead put the known working values in the register?
>>
>> Yes. I was wondering where the initial setting was from on the AST400.
> 
> Well, the AST2500 hard codes them in HW, so it makes some amount of
> sense to use whatever aspeed platform.S hard codes in u-boot for the
> 2400. The values look reasonably sane.
> 
> If we ever see a need to change them, we can add DT props etc... but
> for now I'd just not bother.
> 
> The way it is now, at least, if I have problems, I can tweak the values
> with devmem and try again without the driver overwriting them :-)

ah yes. that is useful I agree. 

C.

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Brendan Higgins]

On Tue, Aug 15, 2017 at 12:21 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> In addition to the base, low and high clock configuration, the AC timing
> register #1 on the AST2400 houses fields controlling:
>
> 1. tBUF: Minimum delay between Stop and Start conditions
> 2. tHDSTA: Hold time for the Start condition
> 3. tACST: Setup time for Start and Stop conditions, and hold time for the
>    Repeated Start condition
>
> These values are defined in hardware on the AST2500 and therefore don't
> need to be set.
>
> aspeed_i2c_init_clk() was performing a direct write of the generated
> clock values rather than a read/mask/modify/update sequence to retain
> tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> fields on the AST2400. This resulted in a delay/setup/hold time of 1
> base clock, which in some configurations is not enough for some devices
> (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> bus speed of 100kHz).
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index ee76e6dddc4b..284f8670dbeb 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -53,6 +53,9 @@
>  #define ASPEED_I2CD_MASTER_EN                          BIT(0)
>
>  /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_TBUF_MASK                     GENMASK(31, 28)
> +#define ASPEED_I2CD_TIME_THDSTA_MASK                   GENMASK(27, 24)
> +#define ASPEED_I2CD_TIME_TACST_MASK                    GENMASK(23, 20)
>  #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
>  #define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
>  #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
> @@ -744,7 +747,11 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>         u32 divisor, clk_reg_val;
>
>         divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
> -       clk_reg_val = bus->get_clk_reg_val(divisor);
> +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
> +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
> +                       ASPEED_I2CD_TIME_THDSTA_MASK |
> +                       ASPEED_I2CD_TIME_TACST_MASK);
> +       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);
>
> --
> 2.11.0
>

Sorry for the delay.

We tried this out and it fixes a problem similar to the one you described.

Thanks again!

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Tested-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Brendan Higgins]

On Tue, Aug 15, 2017 at 12:21 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> In addition to the base, low and high clock configuration, the AC timing
> register #1 on the AST2400 houses fields controlling:
>
> 1. tBUF: Minimum delay between Stop and Start conditions
> 2. tHDSTA: Hold time for the Start condition
> 3. tACST: Setup time for Start and Stop conditions, and hold time for the
>    Repeated Start condition
>
> These values are defined in hardware on the AST2500 and therefore don't
> need to be set.
>
> aspeed_i2c_init_clk() was performing a direct write of the generated
> clock values rather than a read/mask/modify/update sequence to retain
> tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> fields on the AST2400. This resulted in a delay/setup/hold time of 1
> base clock, which in some configurations is not enough for some devices
> (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> bus speed of 100kHz).
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/i2c/busses/i2c-aspeed.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index ee76e6dddc4b..284f8670dbeb 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -53,6 +53,9 @@
>  #define ASPEED_I2CD_MASTER_EN                          BIT(0)
>
>  /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
> +#define ASPEED_I2CD_TIME_TBUF_MASK                     GENMASK(31, 28)
> +#define ASPEED_I2CD_TIME_THDSTA_MASK                   GENMASK(27, 24)
> +#define ASPEED_I2CD_TIME_TACST_MASK                    GENMASK(23, 20)
>  #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT                        16
>  #define ASPEED_I2CD_TIME_SCL_HIGH_MASK                 GENMASK(19, 16)
>  #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT                 12
> @@ -744,7 +747,11 @@ static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>         u32 divisor, clk_reg_val;
>
>         divisor = DIV_ROUND_UP(bus->parent_clk_frequency, bus->bus_frequency);
> -       clk_reg_val = bus->get_clk_reg_val(divisor);
> +       clk_reg_val = readl(bus->base + ASPEED_I2C_AC_TIMING_REG1);
> +       clk_reg_val &= (ASPEED_I2CD_TIME_TBUF_MASK |
> +                       ASPEED_I2CD_TIME_THDSTA_MASK |
> +                       ASPEED_I2CD_TIME_TACST_MASK);
> +       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);
>
> --
> 2.11.0
>

Sorry for the delay.

We tried this out and it fixes a problem similar to the one you described.

Thanks again!

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Tested-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Wolfram Sang]

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

On Tue, Aug 15, 2017 at 04:51:02PM +0930, Andrew Jeffery wrote:
> In addition to the base, low and high clock configuration, the AC timing
> register #1 on the AST2400 houses fields controlling:
> 
> 1. tBUF: Minimum delay between Stop and Start conditions
> 2. tHDSTA: Hold time for the Start condition
> 3. tACST: Setup time for Start and Stop conditions, and hold time for the
>    Repeated Start condition
> 
> These values are defined in hardware on the AST2500 and therefore don't
> need to be set.
> 
> aspeed_i2c_init_clk() was performing a direct write of the generated
> clock values rather than a read/mask/modify/update sequence to retain
> tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> fields on the AST2400. This resulted in a delay/setup/hold time of 1
> base clock, which in some configurations is not enough for some devices
> (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> bus speed of 100kHz).
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Applied to for-next, thanks! I even considered for-current but it does
not apply there. So, I leave the backporting for the interested parties
:)


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

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Andrew Jeffery]

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

On Mon, 2017-08-28 at 18:07 +0200, Wolfram Sang wrote:
> On Tue, Aug 15, 2017 at 04:51:02PM +0930, Andrew Jeffery wrote:
> > In addition to the base, low and high clock configuration, the AC timing
> > register #1 on the AST2400 houses fields controlling:
> > 
> > 1. tBUF: Minimum delay between Stop and Start conditions
> > 2. tHDSTA: Hold time for the Start condition
> > 3. tACST: Setup time for Start and Stop conditions, and hold time for the
> >    Repeated Start condition
> > 
> > These values are defined in hardware on the AST2500 and therefore don't
> > need to be set.
> > 
> > aspeed_i2c_init_clk() was performing a direct write of the generated
> > clock values rather than a read/mask/modify/update sequence to retain
> > tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> > fields on the AST2400. This resulted in a delay/setup/hold time of 1
> > base clock, which in some configurations is not enough for some devices
> > (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> > bus speed of 100kHz).
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> 
> Applied to for-next, thanks! 

Thanks!

> I even considered for-current but it does
> not apply there. So, I leave the backporting for the interested parties
> :)
> 

It depends on Brendan's clock divisor calculation fix, which appears to
be in for-next but not for-current:

    87b59ff8d1d9 i2c: aspeed: add proper support fo 24xx clock params

I'd argue that Brendan's patch should go in for-current as well,
because it fixes a divisor rounding error for the ast2500 (bus is
clocked faster than requested).

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Andrew Jeffery]

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

On Mon, 2017-08-28 at 18:07 +0200, Wolfram Sang wrote:
> On Tue, Aug 15, 2017 at 04:51:02PM +0930, Andrew Jeffery wrote:
> > In addition to the base, low and high clock configuration, the AC timing
> > register #1 on the AST2400 houses fields controlling:
> > 
> > 1. tBUF: Minimum delay between Stop and Start conditions
> > 2. tHDSTA: Hold time for the Start condition
> > 3. tACST: Setup time for Start and Stop conditions, and hold time for the
> >    Repeated Start condition
> > 
> > These values are defined in hardware on the AST2500 and therefore don't
> > need to be set.
> > 
> > aspeed_i2c_init_clk() was performing a direct write of the generated
> > clock values rather than a read/mask/modify/update sequence to retain
> > tBUF, tHDSTA and tACST, and therefore cleared the tBUF, tHDSTA and tACST
> > fields on the AST2400. This resulted in a delay/setup/hold time of 1
> > base clock, which in some configurations is not enough for some devices
> > (e.g. the MAX31785 fan controller, with an APB of 48MHz and a desired
> > bus speed of 100kHz).
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> 
> Applied to for-next, thanks! 

Thanks!

> I even considered for-current but it does
> not apply there. So, I leave the backporting for the interested parties
> :)
> 

It depends on Brendan's clock divisor calculation fix, which appears to
be in for-next but not for-current:

    87b59ff8d1d9 i2c: aspeed: add proper support fo 24xx clock params

I'd argue that Brendan's patch should go in for-current as well,
because it fixes a divisor rounding error for the ast2500 (bus is
clocked faster than requested).

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Wolfram Sang]

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


> I'd argue that Brendan's patch should go in for-current as well,
> because it fixes a divisor rounding error for the ast2500 (bus is
> clocked faster than requested).

Hmmm, pity, the description said "potential" issue so I decided for
for-next. However, I wouldn't like to reshuffle my branches much so
short before the merge window. So, would it be OK with you that you send
both patches to stable after rc1?


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

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Andrew Jeffery]

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

On Tue, 2017-08-29 at 10:45 +0200, Wolfram Sang wrote:
> > I'd argue that Brendan's patch should go in for-current as well,
> > because it fixes a divisor rounding error for the ast2500 (bus is
> > clocked faster than requested).
> 
> Hmmm, pity, the description said "potential" issue so I decided for
> for-next. However, I wouldn't like to reshuffle my branches much so
> short before the merge window. So, would it be OK with you that you send
> both patches to stable after rc1?
> 

Will do.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency

[Andrew Jeffery]

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

On Tue, 2017-08-29 at 10:45 +0200, Wolfram Sang wrote:
> > I'd argue that Brendan's patch should go in for-current as well,
> > because it fixes a divisor rounding error for the ast2500 (bus is
> > clocked faster than requested).
> 
> Hmmm, pity, the description said "potential" issue so I decided for
> for-next. However, I wouldn't like to reshuffle my branches much so
> short before the merge window. So, would it be OK with you that you send
> both patches to stable after rc1?
> 

Will do.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]