Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1711925 > unrolled thread
| Started by | Andrew Jeffery <andrew@aj.id.au> |
|---|---|
| First post | 2017-08-15 09:30 +0200 |
| Last post | 2017-08-24 02:30 +0200 |
| Articles | 7 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Andrew Jeffery <andrew@aj.id.au> - 2017-08-15 09:30 +0200
Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Brendan Higgins <brendanhiggins@google.com> - 2017-08-16 08:40 +0200
Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Joel Stanley <joel@jms.id.au> - 2017-08-16 09:00 +0200
Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2017-08-16 09:10 +0200
Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Cédric Le Goater <clg@kaod.org> - 2017-08-16 09:20 +0200
Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Cédric Le Goater <clg@kaod.org> - 2017-08-16 13:40 +0200
Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency Brendan Higgins <brendanhiggins@google.com> - 2017-08-24 02:30 +0200
| From | Andrew Jeffery <andrew@aj.id.au> |
|---|---|
| Date | 2017-08-15 09:30 +0200 |
| Subject | [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency |
| Message-ID | <ueEsz-6Lw-27@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Brendan Higgins <brendanhiggins@google.com> |
|---|---|
| Date | 2017-08-16 08:40 +0200 |
| Subject | Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency |
| Message-ID | <uf09I-3AX-7@gated-at.bofh.it> |
| In reply to | #1711925 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Joel Stanley <joel@jms.id.au> |
|---|---|
| Date | 2017-08-16 09:00 +0200 |
| Subject | Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency |
| Message-ID | <uf0t4-3Hx-19@gated-at.bofh.it> |
| In reply to | #1711925 |
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
[toc] | [prev] | [next] | [standalone]
| From | Benjamin Herrenschmidt <benh@kernel.crashing.org> |
|---|---|
| Date | 2017-08-16 09:10 +0200 |
| Subject | Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency |
| Message-ID | <uf0CJ-3ZG-5@gated-at.bofh.it> |
| In reply to | #1712724 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Cédric Le Goater <clg@kaod.org> |
|---|---|
| Date | 2017-08-16 09:20 +0200 |
| Subject | Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency |
| Message-ID | <uf0Mp-42X-7@gated-at.bofh.it> |
| In reply to | #1712726 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Cédric Le Goater <clg@kaod.org> |
|---|---|
| Date | 2017-08-16 13:40 +0200 |
| Subject | Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency |
| Message-ID | <uf0CJ-3ZG-7@gated-at.bofh.it> |
| In reply to | #1712724 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Brendan Higgins <brendanhiggins@google.com> |
|---|---|
| Date | 2017-08-24 02:30 +0200 |
| Subject | Re: [PATCH] i2c: aspeed: Retain delay/setup/hold values when configuring bus frequency |
| Message-ID | <uhOc1-7y2-1@gated-at.bofh.it> |
| In reply to | #1711925 |
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>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web