* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-15 7:21 [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Andrew Jeffery
@ 2017-08-16 6:30 ` Brendan Higgins
2017-08-16 6:49 ` Joel Stanley
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Brendan Higgins @ 2017-08-16 6:30 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-i2c, Wolfram Sang, Benjamin Herrenschmidt, Joel Stanley,
OpenBMC Maillist, linux-aspeed, Linux Kernel Mailing List,
Ryan Chen
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.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
@ 2017-08-16 6:30 ` Brendan Higgins
0 siblings, 0 replies; 16+ messages in thread
From: Brendan Higgins @ 2017-08-16 6:30 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-i2c, Wolfram Sang, Benjamin Herrenschmidt, Joel Stanley,
OpenBMC Maillist, linux-aspeed, Linux Kernel Mailing List,
Ryan Chen
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-15 7:21 [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Andrew Jeffery
@ 2017-08-16 6:49 ` Joel Stanley
2017-08-16 6:49 ` Joel Stanley
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Joel Stanley @ 2017-08-16 6:49 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-i2c, Wolfram Sang, Brendan Higgins, Benjamin Herrenschmidt,
OpenBMC Maillist, linux-aspeed, Linux Kernel Mailing List,
Ryan Chen
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
@ 2017-08-16 6:49 ` Joel Stanley
0 siblings, 0 replies; 16+ messages in thread
From: Joel Stanley @ 2017-08-16 6:49 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-i2c, Wolfram Sang, Brendan Higgins, Benjamin Herrenschmidt,
OpenBMC Maillist, linux-aspeed, Linux Kernel Mailing List,
Ryan Chen
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-16 6:49 ` Joel Stanley
(?)
@ 2017-08-16 6:53 ` Cédric Le Goater
2017-08-16 6:59 ` Benjamin Herrenschmidt
-1 siblings, 1 reply; 16+ messages in thread
From: Cédric Le Goater @ 2017-08-16 6:53 UTC (permalink / raw)
To: Joel Stanley, Andrew Jeffery
Cc: Ryan Chen, linux-aspeed, Wolfram Sang, Benjamin Herrenschmidt,
OpenBMC Maillist, Brendan Higgins, Linux Kernel Mailing List,
linux-i2c
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-16 6:53 ` Cédric Le Goater
@ 2017-08-16 6:59 ` Benjamin Herrenschmidt
2017-08-16 7:15 ` Cédric Le Goater
0 siblings, 1 reply; 16+ messages in thread
From: Benjamin Herrenschmidt @ 2017-08-16 6:59 UTC (permalink / raw)
To: Cédric Le Goater, Joel Stanley, Andrew Jeffery
Cc: Ryan Chen, linux-aspeed, Wolfram Sang, OpenBMC Maillist,
Brendan Higgins, Linux Kernel Mailing List, linux-i2c
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-16 6:59 ` Benjamin Herrenschmidt
@ 2017-08-16 7:15 ` Cédric Le Goater
0 siblings, 0 replies; 16+ messages in thread
From: Cédric Le Goater @ 2017-08-16 7:15 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Joel Stanley, Andrew Jeffery
Cc: Ryan Chen, linux-aspeed, Wolfram Sang, OpenBMC Maillist,
Brendan Higgins, Linux Kernel Mailing List, linux-i2c
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-15 7:21 [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Andrew Jeffery
@ 2017-08-24 0:19 ` Brendan Higgins
2017-08-16 6:49 ` Joel Stanley
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Brendan Higgins @ 2017-08-24 0:19 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-i2c, Wolfram Sang, Benjamin Herrenschmidt, Joel Stanley,
OpenBMC Maillist, linux-aspeed, Linux Kernel Mailing List,
Ryan Chen
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>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
@ 2017-08-24 0:19 ` Brendan Higgins
0 siblings, 0 replies; 16+ messages in thread
From: Brendan Higgins @ 2017-08-24 0:19 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-i2c, Wolfram Sang, Benjamin Herrenschmidt, Joel Stanley,
OpenBMC Maillist, linux-aspeed, Linux Kernel Mailing List,
Ryan Chen
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-15 7:21 [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Andrew Jeffery
` (2 preceding siblings ...)
2017-08-24 0:19 ` Brendan Higgins
@ 2017-08-28 16:07 ` Wolfram Sang
2017-08-29 6:56 ` Andrew Jeffery
3 siblings, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2017-08-28 16:07 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-i2c, brendanhiggins, benh, joel, openbmc, linux-aspeed,
linux-kernel, ryan_chen
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-28 16:07 ` Wolfram Sang
@ 2017-08-29 6:56 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-08-29 6:56 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, brendanhiggins, benh, joel, openbmc, linux-aspeed,
linux-kernel, ryan_chen
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
@ 2017-08-29 6:56 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-08-29 6:56 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, brendanhiggins, benh, joel, openbmc, linux-aspeed,
linux-kernel, ryan_chen
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-29 6:56 ` Andrew Jeffery
(?)
@ 2017-08-29 8:45 ` Wolfram Sang
2017-08-29 8:46 ` Andrew Jeffery
-1 siblings, 1 reply; 16+ messages in thread
From: Wolfram Sang @ 2017-08-29 8:45 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-i2c, brendanhiggins, benh, joel, openbmc, linux-aspeed,
linux-kernel, ryan_chen
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
2017-08-29 8:45 ` Wolfram Sang
@ 2017-08-29 8:46 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-08-29 8:46 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, brendanhiggins, benh, joel, openbmc, linux-aspeed,
linux-kernel, ryan_chen
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency
@ 2017-08-29 8:46 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-08-29 8:46 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, brendanhiggins, benh, joel, openbmc, linux-aspeed,
linux-kernel, ryan_chen
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread