| Message ID | 20170815072102.23067-1-andrew@aj.id.au |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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.
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
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.
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.
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.
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>
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 :)
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
> 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?
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
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);
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(-)