[Skiboot] [PATCH 1/4] i2c: Add i2c_run_req() to crank the state machine for a request

[Claudio Carvalho]

Tested-by: Claudio Carvalho <cclaudio at linux.vnet.ibm.com>

One comment below.

On 11/23/2016 03:45 AM, Stewart Smith wrote:
> Doing everything asynchronously is brilliant, it's exactly what we
> want to do.
> 
> Except... the tpm driver wants to do things synchronously, which isn't
> so cool.
> 
> For reasons that are not yet completely known, we spend an awful lot of
> time in the main thread *not* running pollers (potentially seconds), which
> doesn't bode well for I2C timeouts.
> 
> Since the TPM measure is done in a secondary thread, we do *not* run pollers
> there either (as of 323c8aeb54bd4e0b9004091fcbb4a9daeda2f576 - which is
> roughly as of skiboot 2.1.1).
> 
> But we still need to crank the i2c state machine, so we introduce a call
> to do just that. It will return how long the poll interval should be, so
> that we can time_wait() for a more appropriate time for whatever i2c
> implementation is sitting behind things.
> 
> Without this, it was "easy" to get to a situation where the i2c state machine
> wasn't cranked at all, and you'd hit the i2c timeout (for the issued operation)
> before the poller to crank i2c was ever called.
> 
> Signed-off-by: Stewart Smith <stewart at linux.vnet.ibm.com>
> ---
>  hw/p8-i2c.c                        | 18 ++++++++++++++++++
>  include/i2c.h                      |  8 ++++++++
>  libstb/drivers/tpm_i2c_interface.c | 14 +++++++++-----
>  3 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c
> index 4e29432..12a4d1a 100644
> --- a/hw/p8-i2c.c
> +++ b/hw/p8-i2c.c
> @@ -1064,6 +1064,23 @@ static void p8_i2c_set_request_timeout(struct i2c_request *req,
>  	request->timeout = msecs_to_tb(duration);
>  }
>  
> +static uint64_t p8_i2c_run_request(struct i2c_request *req)
> +{
> +	struct i2c_bus *bus = req->bus;
> +	struct p8_i2c_master_port *port =
> +		container_of(bus, struct p8_i2c_master_port, bus);
> +	struct p8_i2c_master *master = port->master;
> +	uint64_t poll_interval = 0;
> +
> +	lock(&master->lock);
> +	p8_i2c_check_status(master);
> +	p8_i2c_check_work(master);
> +	poll_interval = master->poll_interval;
> +	unlock(&master->lock);
> +
> +	return poll_interval;
> +}
> +
>  static inline uint32_t p8_i2c_get_bit_rate_divisor(uint32_t lb_freq,
>  						   uint32_t bus_speed)
>  {
> @@ -1399,6 +1416,7 @@ static void p8_i2c_init_one(struct dt_node *i2cm, enum p8_i2c_master_type type)
>  		port->bus.alloc_req = p8_i2c_alloc_request;
>  		port->bus.free_req = p8_i2c_free_request;
>  		port->bus.set_req_timeout = p8_i2c_set_request_timeout;
> +		port->bus.run_req = p8_i2c_run_request;
>  		i2c_add_bus(&port->bus);
>  
>  		/* Add OPAL properties to the bus node */
> diff --git a/include/i2c.h b/include/i2c.h
> index 83c6ec5..a10a179 100644
> --- a/include/i2c.h
> +++ b/include/i2c.h
> @@ -28,6 +28,7 @@ struct i2c_bus {
>  	void			(*free_req)(struct i2c_request *req);
>  	void			(*set_req_timeout)(struct i2c_request *req,
>  						   uint64_t duration);
> +	uint64_t		(*run_req)(struct i2c_request *req);
>  };
>  
>  /*
> @@ -89,6 +90,13 @@ static inline void i2c_set_req_timeout(struct i2c_request *req,
>  		req->bus->set_req_timeout(req, duration);
>  }
>  
> +static inline uint64_t i2c_run_req(struct i2c_request *req)
> +{
> +	if (req->bus->run_req)
> +		return req->bus->run_req(req);
> +	return 0;
> +}
> +
>  /* P8 implementation details */
>  extern void p8_i2c_init(void);
>  extern void p8_i2c_interrupt(uint32_t chip_id);
> diff --git a/libstb/drivers/tpm_i2c_interface.c b/libstb/drivers/tpm_i2c_interface.c
> index ba0c5a8..4d136fd 100644
> --- a/libstb/drivers/tpm_i2c_interface.c
> +++ b/libstb/drivers/tpm_i2c_interface.c
> @@ -57,6 +57,7 @@ int tpm_i2c_request_send(int tpm_bus_id, int tpm_dev_addr, int read_write,
>  	int rc, waited, retries, timeout;
>  	struct i2c_request *req;
>  	struct i2c_bus *bus;
> +	uint64_t time_to_wait = 0;
>  
>  	bus = i2c_find_bus_by_id(tpm_bus_id);
>  	if (!bus) {
> @@ -106,9 +107,12 @@ int tpm_i2c_request_send(int tpm_bus_id, int tpm_dev_addr, int read_write,
>  		i2c_queue_req(req);
>  
>  		do {
> -			time_wait_ms(REQ_COMPLETE_POLLING);
> -			waited += REQ_COMPLETE_POLLING;
> -		} while (waited < timeout && rc == 1);
> +			time_to_wait = i2c_run_req(req);
> +			if (!time_to_wait)
> +				time_to_wait = REQ_COMPLETE_POLLING;
> +			time_wait(time_to_wait);
> +			waited += time_to_wait;
> +		} while (tb_to_msecs(waited) < timeout && rc == 1);

With this we will be checking if the request is complete either on a
master->poll_interval or REQ_COMPLETE_POLLING interval. Additionally,
i2c_run_req() forces the request status to be updated on every polling
interval.

Do we need to force the request status to be updated on every polling
interval?

If we have something like below, we may end up checking the status
REQ_COMPLETE_POLLING msec after it was updated. Is that what you are
trying avoid here? rc seems to be updated even if we are running on a
secondary thread. Maybe I don't fully understand the problem explained
in the commit message.

do {
	time_wait_ms(REQ_COMPLETE_POLLING);
} while (rc == 1);


>  
>  		if (rc == OPAL_I2C_NACK_RCVD)
>  			continue;
> @@ -117,10 +121,10 @@ int tpm_i2c_request_send(int tpm_bus_id, int tpm_dev_addr, int read_write,
>  			break;
>  	}
>  
> -	DBG("%s tpm req op=%x offset=%x buf=%016llx buflen=%d delay=%d/%d,"
> +	DBG("%s tpm req op=%x offset=%x buf=%016llx buflen=%d delay=%lu/%d,"
>  	    "rc=%d\n",
>  	    (rc) ? "!!!!" : "----", req->op, req->offset,
> -	    *(uint64_t*) buf, req->rw_len, waited, timeout, rc);
> +	    *(uint64_t*) buf, req->rw_len, tb_to_msecs(waited), timeout, rc);
>  
>  	i2c_free_req(req);
>  	if (rc)
>