| Message ID | 20180919170513.12644-1-fbarrat@linux.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hw/p8-i2c: Fix i2c request timeout | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/make_check | success | Test make_check on branch master |
On 20/9/18 3:05 am, Frederic Barrat wrote: > Commit eb146fac9685 ("core/i2c: Move the timeout field into > i2c_request") simplified a bit how a request timeout is > handled. However there's now some confusion between milliseconds and > timebase increments when defining or using the timeout values, which > breaks i2c requests made for opencapi, and probably others too. > > This patch declares all the timeout in milliseconds and just converts > to timebase at the end of the chain, as needed. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> This fixes problems I've been seeing. Tested-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com> > --- > hw/p8-i2c.c | 10 ++++------ > include/i2c.h | 2 +- > 2 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c > index fa5af7cc..a45769c0 100644 > --- a/hw/p8-i2c.c > +++ b/hw/p8-i2c.c > @@ -374,7 +374,7 @@ static void p8_i2c_reset_timeout(struct p8_i2c_master *master, > uint64_t now = mftb(); > > master->last_update = now; > - schedule_timer_at(&master->timeout, now + req->timeout); > + schedule_timer_at(&master->timeout, now + msecs_to_tb(req->timeout)); > } > > static int p8_i2c_prog_watermark(struct p8_i2c_master *master) > @@ -854,7 +854,7 @@ static void p8_i2c_check_status(struct p8_i2c_master *master) > port = container_of(req->bus, struct p8_i2c_master_port, bus); > now = mftb(); > > - deadline = master->last_update + req->timeout; > + deadline = master->last_update + msecs_to_tb(req->timeout); > > if (status & I2C_STAT_ANY_ERR) > p8_i2c_status_error(port, req, status & I2C_STAT_ANY_ERR, now); > @@ -1584,7 +1584,6 @@ static void p8_i2c_init_one(struct dt_node *i2cm, enum p8_i2c_master_type type) > I2C_TIMEOUT_POLL_MS; > > dt_for_each_child(i2cm, i2cm_port) { > - uint64_t timeout_ms; > uint32_t speed; > > port->port_num = dt_prop_get_u32(i2cm_port, "reg"); > @@ -1598,9 +1597,8 @@ static void p8_i2c_init_one(struct dt_node *i2cm, enum p8_i2c_master_type type) > port->bus.queue_req = p8_i2c_queue_request; > port->bus.run_req = p8_i2c_run_request; > > - timeout_ms = dt_prop_get_u32_def(i2cm_port, "timeout-ms", > - default_timeout); > - port->byte_timeout = msecs_to_tb(timeout_ms); > + port->byte_timeout = dt_prop_get_u32_def(i2cm_port, > + "timeout-ms", default_timeout); > > i2c_add_bus(&port->bus); > list_add_tail(&master->ports, &port->link); > diff --git a/include/i2c.h b/include/i2c.h > index 67f8b995..a06cca0d 100644 > --- a/include/i2c.h > +++ b/include/i2c.h > @@ -61,7 +61,7 @@ struct i2c_request { > int rc, struct i2c_request *req); > void *user_data; /* Client data */ > int retries; > - uint64_t timeout; > + uint64_t timeout; /* in ms */ > }; > > /* Generic i2c */ >
On Thu, Sep 20, 2018 at 3:05 AM, Frederic Barrat <fbarrat@linux.ibm.com> wrote: > Commit eb146fac9685 ("core/i2c: Move the timeout field into > i2c_request") simplified a bit how a request timeout is > handled. However there's now some confusion between milliseconds and > timebase increments when defining or using the timeout values, which > breaks i2c requests made for opencapi, and probably others too. > > This patch declares all the timeout in milliseconds and just converts > to timebase at the end of the chain, as needed. Cool, probably should have done that in the first place. Reviewed-by: Oliver O'Halloran <oohall@gmail.com> > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> > --- > hw/p8-i2c.c | 10 ++++------ > include/i2c.h | 2 +- > 2 files changed, 5 insertions(+), 7 deletions(-) > > diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c > index fa5af7cc..a45769c0 100644 > --- a/hw/p8-i2c.c > +++ b/hw/p8-i2c.c > @@ -374,7 +374,7 @@ static void p8_i2c_reset_timeout(struct p8_i2c_master *master, > uint64_t now = mftb(); > > master->last_update = now; > - schedule_timer_at(&master->timeout, now + req->timeout); > + schedule_timer_at(&master->timeout, now + msecs_to_tb(req->timeout)); > } > > static int p8_i2c_prog_watermark(struct p8_i2c_master *master) > @@ -854,7 +854,7 @@ static void p8_i2c_check_status(struct p8_i2c_master *master) > port = container_of(req->bus, struct p8_i2c_master_port, bus); > now = mftb(); > > - deadline = master->last_update + req->timeout; > + deadline = master->last_update + msecs_to_tb(req->timeout); > > if (status & I2C_STAT_ANY_ERR) > p8_i2c_status_error(port, req, status & I2C_STAT_ANY_ERR, now); > @@ -1584,7 +1584,6 @@ static void p8_i2c_init_one(struct dt_node *i2cm, enum p8_i2c_master_type type) > I2C_TIMEOUT_POLL_MS; > > dt_for_each_child(i2cm, i2cm_port) { > - uint64_t timeout_ms; > uint32_t speed; > > port->port_num = dt_prop_get_u32(i2cm_port, "reg"); > @@ -1598,9 +1597,8 @@ static void p8_i2c_init_one(struct dt_node *i2cm, enum p8_i2c_master_type type) > port->bus.queue_req = p8_i2c_queue_request; > port->bus.run_req = p8_i2c_run_request; > > - timeout_ms = dt_prop_get_u32_def(i2cm_port, "timeout-ms", > - default_timeout); > - port->byte_timeout = msecs_to_tb(timeout_ms); > + port->byte_timeout = dt_prop_get_u32_def(i2cm_port, > + "timeout-ms", default_timeout); > > i2c_add_bus(&port->bus); > list_add_tail(&master->ports, &port->link); > diff --git a/include/i2c.h b/include/i2c.h > index 67f8b995..a06cca0d 100644 > --- a/include/i2c.h > +++ b/include/i2c.h > @@ -61,7 +61,7 @@ struct i2c_request { > int rc, struct i2c_request *req); > void *user_data; /* Client data */ > int retries; > - uint64_t timeout; > + uint64_t timeout; /* in ms */ > }; > > /* Generic i2c */ > -- > 2.17.1 >
On 20/9/18 3:05 am, Frederic Barrat wrote: > Commit eb146fac9685 ("core/i2c: Move the timeout field into > i2c_request") simplified a bit how a request timeout is > handled. However there's now some confusion between milliseconds and > timebase increments when defining or using the timeout values, which > breaks i2c requests made for opencapi, and probably others too. > > This patch declares all the timeout in milliseconds and just converts > to timebase at the end of the chain, as needed. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> Stewart: would be good if we could get this merged sooner rather than later. Andrew
Frederic Barrat <fbarrat@linux.ibm.com> writes: > Commit eb146fac9685 ("core/i2c: Move the timeout field into > i2c_request") simplified a bit how a request timeout is > handled. However there's now some confusion between milliseconds and > timebase increments when defining or using the timeout values, which > breaks i2c requests made for opencapi, and probably others too. > > This patch declares all the timeout in milliseconds and just converts > to timebase at the end of the chain, as needed. > > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> > --- > hw/p8-i2c.c | 10 ++++------ > include/i2c.h | 2 +- > 2 files changed, 5 insertions(+), 7 deletions(-) Thanks, merged to master as of a800fa35b8222996a634baeec27cd2369f7619f5
diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c index fa5af7cc..a45769c0 100644 --- a/hw/p8-i2c.c +++ b/hw/p8-i2c.c @@ -374,7 +374,7 @@ static void p8_i2c_reset_timeout(struct p8_i2c_master *master, uint64_t now = mftb(); master->last_update = now; - schedule_timer_at(&master->timeout, now + req->timeout); + schedule_timer_at(&master->timeout, now + msecs_to_tb(req->timeout)); } static int p8_i2c_prog_watermark(struct p8_i2c_master *master) @@ -854,7 +854,7 @@ static void p8_i2c_check_status(struct p8_i2c_master *master) port = container_of(req->bus, struct p8_i2c_master_port, bus); now = mftb(); - deadline = master->last_update + req->timeout; + deadline = master->last_update + msecs_to_tb(req->timeout); if (status & I2C_STAT_ANY_ERR) p8_i2c_status_error(port, req, status & I2C_STAT_ANY_ERR, now); @@ -1584,7 +1584,6 @@ static void p8_i2c_init_one(struct dt_node *i2cm, enum p8_i2c_master_type type) I2C_TIMEOUT_POLL_MS; dt_for_each_child(i2cm, i2cm_port) { - uint64_t timeout_ms; uint32_t speed; port->port_num = dt_prop_get_u32(i2cm_port, "reg"); @@ -1598,9 +1597,8 @@ static void p8_i2c_init_one(struct dt_node *i2cm, enum p8_i2c_master_type type) port->bus.queue_req = p8_i2c_queue_request; port->bus.run_req = p8_i2c_run_request; - timeout_ms = dt_prop_get_u32_def(i2cm_port, "timeout-ms", - default_timeout); - port->byte_timeout = msecs_to_tb(timeout_ms); + port->byte_timeout = dt_prop_get_u32_def(i2cm_port, + "timeout-ms", default_timeout); i2c_add_bus(&port->bus); list_add_tail(&master->ports, &port->link); diff --git a/include/i2c.h b/include/i2c.h index 67f8b995..a06cca0d 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -61,7 +61,7 @@ struct i2c_request { int rc, struct i2c_request *req); void *user_data; /* Client data */ int retries; - uint64_t timeout; + uint64_t timeout; /* in ms */ }; /* Generic i2c */
Commit eb146fac9685 ("core/i2c: Move the timeout field into i2c_request") simplified a bit how a request timeout is handled. However there's now some confusion between milliseconds and timebase increments when defining or using the timeout values, which breaks i2c requests made for opencapi, and probably others too. This patch declares all the timeout in milliseconds and just converts to timebase at the end of the chain, as needed. Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com> --- hw/p8-i2c.c | 10 ++++------ include/i2c.h | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-)