Re: [PATCH v4 2/4] tpm/tpm_i2c_stm_st33: Split tpm_i2c_tpm_st33 in 2 layers (core + phy)

Hi Christophe,

some superficial (and picky) reviews below - in general I like it.

Peter

Am Sonntag, 25. Januar 2015, 22:11:31 schrieb Christophe Ricard:

Defined in i2c.c spi.c -> move to header?

+#define TPM_BUFSIZE			2048

Defined in i2c.c spi.c and st33zp24, (but not used in st33zp24.c)
-> move to header?

already in ../tpm.h

+#define TPM_BUFSIZE			2048

used in spi.c i2c.c and st33zp24.c -> move to header?

+
+#define LOCALITY0			0

used in spi.c and st33zp24.c -> move to header?

+
+enum st33zp24_access {
+	TPM_ACCESS_VALID = 0x80,
+	TPM_ACCESS_ACTIVE_LOCALITY = 0x20,
+	TPM_ACCESS_REQUEST_PENDING = 0x04,
+	TPM_ACCESS_REQUEST_USE = 0x02,
+};
+
+enum st33zp24_status {
+	TPM_STS_VALID = 0x80,
+	TPM_STS_COMMAND_READY = 0x40,
+	TPM_STS_GO = 0x20,
+	TPM_STS_DATA_AVAIL = 0x10,
+	TPM_STS_DATA_EXPECT = 0x08,
+};
+
+enum st33zp24_int_flags {
+	TPM_GLOBAL_INT_ENABLE = 0x80,
+	TPM_INTF_CMD_READY_INT = 0x080,
+	TPM_INTF_FIFO_AVALAIBLE_INT = 0x040,
+	TPM_INTF_WAKE_UP_READY_INT = 0x020,
+	TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
+	TPM_INTF_STS_VALID_INT = 0x002,
+	TPM_INTF_DATA_AVAIL_INT = 0x001,
+};
+
+enum tis_defaults {
+	TIS_SHORT_TIMEOUT = 750,
+	TIS_LONG_TIMEOUT = 2000,
+};
+
+struct st33zp24_dev {
+	struct tpm_chip *chip;
+	void *phy_id;
+	const struct st33zp24_phy_ops *ops;
+	u32 intrs;
+	int io_lpcpd;
+};
+
+/*
+ * clear_interruption clear the pending interrupt.
+ * @param: tpm_dev, the tpm device device.
+ * @return: the interrupt status value.
+ */
+static u8 clear_interruption(struct st33zp24_dev *tpm_dev)
+{
+	u8 interrupt;
+
+	tpm_dev->ops->recv(tpm_dev->phy_id, TPM_INT_STATUS, &interrupt, 1);
+	tpm_dev->ops->send(tpm_dev->phy_id, TPM_INT_STATUS, &interrupt, 1);
+	return interrupt;
+} /* clear_interruption() */
+
+/*
+ * st33zp24_cancel, cancel is not implemented.

cancel is not implemented? then what does the function do?

+ * @param: chip, the tpm_chip description as specified in
driver/char/tpm/tpm.h + */
+static void st33zp24_cancel(struct tpm_chip *chip)
+{
+	struct st33zp24_dev *tpm_dev;
+	u8 data;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	data = TPM_STS_COMMAND_READY;
+	tpm_dev->ops->send(tpm_dev->phy_id, TPM_STS, &data, 1);
+} /* st33zp24_cancel() */
+
+/*
+ * st33zp24_status return the TPM_STS register
+ * @param: chip, the tpm chip description
+ * @return: the TPM_STS register value.
+ */
+static u8 st33zp24_status(struct tpm_chip *chip)
+{
+	struct st33zp24_dev *tpm_dev;
+	u8 data;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	tpm_dev->ops->recv(tpm_dev->phy_id, TPM_STS, &data, 1);
+	return data;
+} /* st33zp24_status() */
+
+/*
+ * check_locality if the locality is active
+ * @param: chip, the tpm chip description
+ * @return: the active locality or -EACCESS.
+ */
+static int check_locality(struct tpm_chip *chip)
+{
+	struct st33zp24_dev *tpm_dev;
+	u8 data;
+	u8 status;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	status = tpm_dev->ops->recv(tpm_dev->phy_id, TPM_ACCESS, &data, 1);
+	if (status && (data &
+		(TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
+		(TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID))
+		return chip->vendor.locality;
+
+	return -EACCES;
+} /* check_locality() */
+
+/*
+ * request_locality request the TPM locality
+ * @param: chip, the chip description
+ * @return: the active locality or negative value.
+ */
+static int request_locality(struct tpm_chip *chip)
+{
+	unsigned long stop;
+	long ret;
+	struct st33zp24_dev *tpm_dev;
+	u8 data;
+
+	if (check_locality(chip) == chip->vendor.locality)
+		return chip->vendor.locality;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	data = TPM_ACCESS_REQUEST_USE;
+	ret = tpm_dev->ops->send(tpm_dev->phy_id, TPM_ACCESS, &data, 1);
+	if (ret < 0)
+		goto end;

return ret;
directly

+
+	stop = jiffies + chip->vendor.timeout_a;
+
+	/* Request locality is usually effective after the request */
+	do {
+		if (check_locality(chip) >= 0)
+			return chip->vendor.locality;
+		msleep(TPM_TIMEOUT);
+	} while (time_before(jiffies, stop));

/* could not get locality */
return -EACCES;

+	ret = -EACCES;
+end:
+	return ret;
+} /* request_locality() */
+
+/*
+ * release_locality release the active locality
+ * @param: chip, the tpm chip description.
+ */
+static void release_locality(struct tpm_chip *chip)
+{
+	struct st33zp24_dev *tpm_dev;
+	u8 data;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+	data = TPM_ACCESS_ACTIVE_LOCALITY;
+
+	tpm_dev->ops->send(tpm_dev->phy_id, TPM_ACCESS, &data, 1);
+}
+
+/*
+ * get_burstcount return the burstcount address 0x19 0x1A

return the burstcount address?

+ * @param: chip, the chip description
+ * return: the burstcount or negative value.
+ */
+static int get_burstcount(struct tpm_chip *chip)
+{
+	unsigned long stop;
+	int burstcnt, status;
+	u8 tpm_reg, temp;
+	struct st33zp24_dev *tpm_dev;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	stop = jiffies + chip->vendor.timeout_d;
+	do {
+		tpm_reg = TPM_STS + 1;
+		status = tpm_dev->ops->recv(tpm_dev->phy_id, tpm_reg, &temp, 1);
+		if (status < 0)
+			goto end;

	return -EBUSY;
directly

+
+		tpm_reg = tpm_reg + 1;

                   TPM_STS + 2;
is more readable

+		burstcnt = temp;
+		status = tpm_dev->ops->recv(tpm_dev->phy_id, tpm_reg, &temp, 1);
+		if (status < 0)
+			goto end;

	return -EBUSY;
directly

+
+		burstcnt |= temp << 8;
+		if (burstcnt)
+			return burstcnt;
+		msleep(TPM_TIMEOUT);
+	} while (time_before(jiffies, stop));
+
+end:
+	return -EBUSY;

	return -EBUSY;
directly above

+} /* get_burstcount() */
+
+
+/*
+ * wait_for_tpm_stat_cond
+ * @param: chip, chip description
+ * @param: mask, expected mask value
+ * @param: check_cancel, does the command expected to be canceled ?
+ * @param: canceled, did we received a cancel request ?
+ * @return: true if status == mask or if the command is canceled.
+ * false in other cases.
+ */
+static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
+				bool check_cancel, bool *canceled)
+{
+	u8 status = chip->ops->status(chip);
+
+	*canceled = false;
+	if ((status & mask) == mask)
+		return true;
+	if (check_cancel && chip->ops->req_canceled(chip, status)) {
+		*canceled = true;
+		return true;
+	}
+	return false;
+}
+
+/*
+ * wait_for_stat wait for a TPM_STS value
+ * @param: chip, the tpm chip description
+ * @param: mask, the value mask to wait
+ * @param: timeout, the timeout
+ * @param: queue, the wait queue.
+ * @param: check_cancel, does the command can be cancelled ?
+ * @return: the tpm status, 0 if success, -ETIME if timeout is reached.
+ */
+static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long
timeout, +			wait_queue_head_t *queue, bool check_cancel)
+{
+	unsigned long stop;
+	int ret;
+	bool canceled = false;
+	bool condition;
+	u32 cur_intrs;
+	u8 status;
+	struct st33zp24_dev *tpm_dev;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	/* check current status */
+	status = st33zp24_status(chip);
+	if ((status & mask) == mask)
+		return 0;
+
+	stop = jiffies + timeout;
+
+	if (chip->vendor.irq) {
+		cur_intrs = tpm_dev->intrs;
+		clear_interruption(tpm_dev);
+		enable_irq(chip->vendor.irq);
+
+again:
+		timeout = stop - jiffies;
+		if ((long) timeout <= 0)
+			return -1;
+
+		ret = wait_event_interruptible_timeout(*queue,
+					cur_intrs != tpm_dev->intrs, timeout);
+		clear_interruption(tpm_dev);
+		condition = wait_for_tpm_stat_cond(chip, mask,
+						   check_cancel, &canceled);
+		if (ret >= 0 && condition) {
+			if (canceled)
+				return -ECANCELED;
+			return 0;
+		}
+		if (ret == -ERESTARTSYS && freezing(current)) {
+			clear_thread_flag(TIF_SIGPENDING);
+			goto again;

ugh... can you please use a do_while loop or something?
I usually dislike jumping to a higher location.

+		}
+		disable_irq_nosync(chip->vendor.irq);
+
+	} else {
+		do {
+			msleep(TPM_TIMEOUT);
+			status = chip->ops->status(chip);
+			if ((status & mask) == mask)
+				return 0;
+		} while (time_before(jiffies, stop));
+	}
+
+	return -ETIME;
+} /* wait_for_stat() */
+
+/*
+ * recv_data receive data
+ * @param: chip, the tpm chip description
+ * @param: buf, the buffer where the data are received
+ * @param: count, the number of data to receive
+ * @return: the number of bytes read from TPM FIFO.
+ */
+static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	int size = 0, burstcnt, len, ret;
+	struct st33zp24_dev *tpm_dev;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	while (size < count &&
+	       wait_for_stat(chip,
+			     TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+			     chip->vendor.timeout_c,
+			     &chip->vendor.read_queue, true) == 0) {
+		burstcnt = get_burstcount(chip);
+		if (burstcnt < 0)
+			return burstcnt;
+		len = min_t(int, burstcnt, count - size);
+		ret = tpm_dev->ops->recv(tpm_dev->phy_id, TPM_DATA_FIFO,
+					 buf + size, len);
+		if (ret < 0)
+			return ret;
+
+		size += len;
+	}
+	return size;
+}
+
+/*
+ * tpm_ioserirq_handler the serirq irq handler
+ * @param: irq, the tpm chip description
+ * @param: dev_id, the description of the chip
+ * @return: the status of the handler.
+ */
+static irqreturn_t tpm_ioserirq_handler(int irq, void *dev_id)
+{
+	struct tpm_chip *chip = dev_id;
+	struct st33zp24_dev *tpm_dev;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	tpm_dev->intrs++;
+	wake_up_interruptible(&chip->vendor.read_queue);
+	disable_irq_nosync(chip->vendor.irq);
+
+	return IRQ_HANDLED;
+} /* tpm_ioserirq_handler() */
+
+/*
+ * st33zp24_send send TPM commands through the I2C bus.
+ *
+ * @param: chip, the tpm_chip description as specified in
driver/char/tpm/tpm.h + * @param: buf,	the buffer to send.
+ * @param: count, the number of bytes to send.
+ * @return: In case of success the number of bytes sent.
+ *			In other case, a < 0 value describing the issue.
+ */
+static int st33zp24_send(struct tpm_chip *chip, unsigned char *buf,
+			 size_t len)
+{
+	u32 status, i, size;
+	int burstcnt = 0;
+	int ret;
+	u8 data;
+	struct st33zp24_dev *tpm_dev;
+
+	if (!chip)
+		return -EBUSY;
+	if (len < TPM_HEADER_SIZE)
+		return -EBUSY;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	ret = request_locality(chip);
+	if (ret < 0)
+		return ret;
+
+	status = st33zp24_status(chip);
+	if ((status & TPM_STS_COMMAND_READY) == 0) {
+		st33zp24_cancel(chip);
+		if (wait_for_stat
+		    (chip, TPM_STS_COMMAND_READY, chip->vendor.timeout_b,
+		     &chip->vendor.read_queue, false) < 0) {
+			ret = -ETIME;
+			goto out_err;
+		}
+	}
+
+	for (i = 0; i < len - 1;) {
+		burstcnt = get_burstcount(chip);
+		if (burstcnt < 0)
+			return burstcnt;
+		size = min_t(int, len - i - 1, burstcnt);
+		ret = tpm_dev->ops->send(tpm_dev->phy_id, TPM_DATA_FIFO,
+					 buf + i, size);
+		if (ret < 0)
+			goto out_err;
+
+		i += size;
+	}
+
+	status = st33zp24_status(chip);
+	if ((status & TPM_STS_DATA_EXPECT) == 0) {
+		ret = -EIO;
+		goto out_err;
+	}
+
+	ret = tpm_dev->ops->send(tpm_dev->phy_id, TPM_DATA_FIFO,
+				 buf + len - 1, 1);
+	if (ret < 0)
+		goto out_err;
+
+	status = st33zp24_status(chip);
+	if ((status & TPM_STS_DATA_EXPECT) != 0) {
+		ret = -EIO;
+		goto out_err;
+	}
+
+	data = TPM_STS_GO;
+	ret = tpm_dev->ops->send(tpm_dev->phy_id, TPM_STS, &data, 1);
+	if (ret < 0)
+		goto out_err;
+
+	return len;
+out_err:
+	st33zp24_cancel(chip);
+	release_locality(chip);
+	return ret;
+}
+
+/*
+ * st33zp24_recv received TPM response through TPM phy.
+ * @param: chip, the tpm_chip description as specified in
driver/char/tpm/tpm.h. + * @param: buf,	the buffer to store datas.
+ * @param: count, the number of bytes to send.
+ * @return: In case of success the number of bytes received.
+ *	    In other case, a < 0 value describing the issue.
+ */
+static int st33zp24_recv(struct tpm_chip *chip, unsigned char *buf,
+			    size_t count)
+{
+	int size = 0;
+	int expected;
+
+	if (!chip)
+		return -EBUSY;
+
+	if (count < TPM_HEADER_SIZE) {
+		size = -EIO;
+		goto out;
+	}
+
+	size = recv_data(chip, buf, TPM_HEADER_SIZE);
+	if (size < TPM_HEADER_SIZE) {
+		dev_err(&chip->dev, "Unable to read header\n");
+		goto out;
+	}
+
+	expected = be32_to_cpu(*(__be32 *)(buf + 2));
+	if (expected > count) {
+		size = -EIO;
+		goto out;
+	}
+
+	size += recv_data(chip, &buf[TPM_HEADER_SIZE],
+			expected - TPM_HEADER_SIZE);
+	if (size < expected) {
+		dev_err(&chip->dev, "Unable to read remainder of result\n");
+		size = -ETIME;
+		goto out;

goto not really needed here, but can be kept.

+	}
+
+out:
+	st33zp24_cancel(chip);
+	release_locality(chip);
+	return size;
+}
+
+/*
+ * st33zp24_req_canceled
+ * @param: chip, the tpm_chip description as specified in
driver/char/tpm/tpm.h. + * @param: status, the TPM status.
+ * @return: Does TPM ready to compute a new command ? true.
+ */
+static bool st33zp24_req_canceled(struct tpm_chip *chip, u8 status)
+{
+	return (status == TPM_STS_COMMAND_READY);
+}
+
+static const struct tpm_class_ops st33zp24_tpm = {
+	.send = st33zp24_send,
+	.recv = st33zp24_recv,
+	.cancel = st33zp24_cancel,
+	.status = st33zp24_status,
+	.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+	.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+	.req_canceled = st33zp24_req_canceled,
+};
+
+/*
+ * st33zp24_probe initialize the TPM device
+ * @param: client, the i2c_client drescription (TPM I2C description).
+ * @param: id, the i2c_device_id struct.
+ * @return: 0 in case of success.
+ *	 -1 in other case.
+ */
+int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops,
+		   struct device *dev, int irq, int io_lpcpd)
+{
+	int ret;
+	u8 intmask = 0;
+	struct tpm_chip *chip;
+	struct st33zp24_dev *tpm_dev;
+
+	chip = tpmm_chip_alloc(dev, &st33zp24_tpm);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	tpm_dev = devm_kzalloc(dev, sizeof(struct st33zp24_dev),
+			       GFP_KERNEL);
+	if (!tpm_dev)
+		return -ENOMEM;
+
+	TPM_VPRIV(chip) = tpm_dev;
+	tpm_dev->phy_id = phy_id;
+	tpm_dev->ops = ops;
+
+	chip->vendor.timeout_a = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+	chip->vendor.timeout_b = msecs_to_jiffies(TIS_LONG_TIMEOUT);
+	chip->vendor.timeout_c = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+	chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
+
+	chip->vendor.locality = LOCALITY0;
+
+	if (irq) {
+		/* INTERRUPT Setup */
+		init_waitqueue_head(&chip->vendor.read_queue);
+		tpm_dev->intrs = 0;
+
+		if (request_locality(chip) != LOCALITY0) {
+			ret = -ENODEV;
+			goto _tpm_clean_answer;
+		}
+
+		clear_interruption(tpm_dev);
+		ret = devm_request_irq(dev, irq, tpm_ioserirq_handler,
+				IRQF_TRIGGER_HIGH, "TPM SERIRQ management",
+				chip);
+		if (ret < 0) {
+			dev_err(&chip->dev , "TPM SERIRQ signals %d not available\n",

                                ^
remove the blank :) 

+				irq);
+			goto _tpm_clean_answer;
+		}
+
+		intmask |= TPM_INTF_CMD_READY_INT
+			|  TPM_INTF_STS_VALID_INT
+			|  TPM_INTF_DATA_AVAIL_INT;
+
+		ret = tpm_dev->ops->send(tpm_dev->phy_id, TPM_INT_ENABLE,
+					 &intmask, 1);
+		if (ret < 0)
+			goto _tpm_clean_answer;
+
+		intmask = TPM_GLOBAL_INT_ENABLE;
+		ret = tpm_dev->ops->send(tpm_dev->phy_id, (TPM_INT_ENABLE + 3),
+					 &intmask, 1);
+		if (ret < 0)
+			goto _tpm_clean_answer;
+
+		chip->vendor.irq = irq;
+
+		disable_irq_nosync(chip->vendor.irq);
+
+		tpm_gen_interrupt(chip);
+	}
+
+	tpm_get_timeouts(chip);
+	tpm_do_selftest(chip);
+
+	return tpm_chip_register(chip);
+_tpm_clean_answer:
+	dev_info(&chip->dev, "TPM initialization fail\n");
+	return ret;
+}
+EXPORT_SYMBOL(st33zp24_probe);
+
+/*
+ * st33zp24_remove remove the TPM device
+ * @param: tpm_data, the tpm phy.
+ * @return: 0 in case of success.
+ */
+int st33zp24_remove(struct tpm_chip *chip)
+{
+	tpm_chip_unregister(chip);
+	return 0;
+}
+EXPORT_SYMBOL(st33zp24_remove);
+
+#ifdef CONFIG_PM_SLEEP
+/*
+ * st33zp24_pm_suspend suspend the TPM device
+ * @param: tpm_data, the tpm phy.
+ * @param: mesg, the power management message.
+ * @return: 0 in case of success.
+ */
+int st33zp24_pm_suspend(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct st33zp24_dev *tpm_dev;
+	int ret = 0;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	if (gpio_is_valid(tpm_dev->io_lpcpd))
+		gpio_set_value(tpm_dev->io_lpcpd, 0);
+	else
+		ret = tpm_pm_suspend(dev);
+
+	return ret;
+} /* st33zp24_pm_suspend() */
+EXPORT_SYMBOL(st33zp24_pm_suspend);
+
+/*
+ * st33zp24_pm_resume resume the TPM device
+ * @param: tpm_data, the tpm phy.
+ * @return: 0 in case of success.
+ */
+int st33zp24_pm_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct st33zp24_dev *tpm_dev;
+	int ret = 0;
+
+	tpm_dev = (struct st33zp24_dev *)TPM_VPRIV(chip);
+
+	if (gpio_is_valid(tpm_dev->io_lpcpd)) {
+		gpio_set_value(tpm_dev->io_lpcpd, 1);
+		ret = wait_for_stat(chip,
+				TPM_STS_VALID, chip->vendor.timeout_b,
+				&chip->vendor.read_queue, false);
+	} else {
+		ret = tpm_pm_resume(dev);
+		if (!ret)
+			tpm_do_selftest(chip);
+	}
+	return ret;
+} /* st33zp24_pm_resume() */
+EXPORT_SYMBOL(st33zp24_pm_resume);
+#endif
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION(DRIVER_DESC);

Maybe also add MODULE_AUTHOR ?
I'd use DRIVER_DESC directly?

in spi.c and i2c.c you use phy_id instead of dev_id - altough it doesn't 
matter to the compiler, I like consistency :)

+};
+
+#ifdef CONFIG_PM_SLEEP
+int st33zp24_pm_suspend(struct device *dev);
+int st33zp24_pm_resume(struct device *dev);
+#endif
+
+int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops,
+		   struct device *dev, int irq, int io_lpcpd);
+int st33zp24_remove(struct tpm_chip *chip);
+#endif /* __LOCAL_ST33ZP24_H__ */

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html