| Message ID | 20190628151327.206818-1-oshrialkoby85@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | char: tpm: add new driver for tpm i2c ptp | expand |
On Fri, 2019-06-28 at 18:13 +0300, Oshri Alkoby wrote: The long descriptions are still missing. Please take the time and write a proper commit messages that clearly tell what the patch does. Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements that spec so you should only implement a new physical layer. /Jarkko
On 04.07.2019 10:43, Jarkko Sakkinen wrote: > Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements > that spec so you should only implement a new physical layer. I had the same thought. Unfortunately, the I2C-TIS specification introduces two relevant changes compared to tpm_tis/tpm_tis_spi: 1. Locality is not encoded into register addresses anymore, but stored in a separate register. 2. Several register addresses have changed (but still contain compatible contents). I'd still prefer not to duplicate all the high-level logic from tpm_tis_core. But this will probably mean to introduce some new interfaces between tpm_tis_core and the physical layers. Also, shouldn't the new driver be called tpm_tis_i2c, to group it with all the other (TIS) drivers, that implement a vendor-independent protocol? With tpm_i2c_ptp users might assume that ptp is just another vendor. Alexander
On Thu, 2019-07-04 at 13:29 +0200, Alexander Steffen wrote: > On 04.07.2019 10:43, Jarkko Sakkinen wrote: > > Check out tpm_tis_core.c and tpm_tis_spi.c. TPM TIS driver implements > > that spec so you should only implement a new physical layer. > > I had the same thought. Unfortunately, the I2C-TIS specification > introduces two relevant changes compared to tpm_tis/tpm_tis_spi: I doubt that there was any comparison made. > 1. Locality is not encoded into register addresses anymore, but stored > in a separate register. > 2. Several register addresses have changed (but still contain compatible > contents). > > I'd still prefer not to duplicate all the high-level logic from > tpm_tis_core. But this will probably mean to introduce some new > interfaces between tpm_tis_core and the physical layers. Agreed. Some plumbing needs to be done in tpm_tis_core to make it work for this. We definitely do not want to duplicate code that has been field tested for years. > Also, shouldn't the new driver be called tpm_tis_i2c, to group it with > all the other (TIS) drivers, that implement a vendor-independent > protocol? With tpm_i2c_ptp users might assume that ptp is just another > vendor. Yes, absolutely. I guess the driver has been done without looking at what already exist in the TPM kernel stack. /Jarkko
On Thu, 2019-07-04 at 12:48 -0500, Oshri Alkobi wrote: > Alex, Jarkko, thank you very much for your feedbacks! Please configure your email client to use plain text. > I totally agree, there are some duplications that can be common, indeed it > will require some work in tpm_tis_core. > Since I believe it is not going to happen soon, I would suggest to examine > what duplications can currently be dropped from the new driver, so the kernel > will support the PTP I2C interface in the meantime. > I will appreciate getting ideas about any tpm_tis_core logic that currently > can be used as is by the new drive. I rather wait for a solution that integrates with our mature stack for TIS (or these days FIFO) than integrate something half-baked. If you want something in, please do right things right. What you are proposing would mean maintaining duplicate stacks forever. > Since the TIS is an old specification that mostly defines FIFO for TPM1.2 I > would say the name tpm_tis_i2c does not completely reflect its goal. However > we really don't have any problem with any name that the group will agree on. > Does tpm_ptp_i2c sound better than the current name? Absolutely not going to use that name. The naming convention is what it is for other drivers that are adapt tpm_tis_core to different HW interfaces. /Jarkko
On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote: > Thanks for your feedback and sorry for the late response. > > Due to the amount of work required to handle this technical feedback and > project constraints we need to put this task on hold for the near future. > > In the meantime, anyone from the community is welcome to take over this > code and handle the re-design for the benefit of the entire TPM community. Ok, so there is already driver for this called tpm_tis_core. So you go and create a new module, whose name given the framework of things that we already have deployed, is destined to be tpm_tis_i2c. Then you roughly implement a new physical layer by using a callback interface provided to you by tpm_tis_core. The so called re-design was already addressed by Alexander [1]. How hard can it be seriously? [1] https://lkml.org/lkml/2019/7/4/331 /Jarkko
On 15.07.2019 10:08, Tomer Maimon wrote: > Hi Jarkko and All, > > Thanks for your feedback and sorry for the late response. > > > Due to the amount of work required to handle this technical feedback and > project constraints we need to put this task on hold for the near future. > > In the meantime, anyone from the community is welcome to take over this > code and handle the re-design for the benefit of the entire TPM community. Too bad you lack the time to finish this. If somebody else were to pick it up, would it be possible for you to provide a couple of TPMs that implement this protocol, so that it can be properly tested? Alexander
Hi Jarkko and Alexander, We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. At a minimum we thought of: 1. Handling TPM Localities in I2C is different 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry) Besides this, during development we might encounter additional differences between SPI and I2C. We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window. Regards, Eyal. -----Original Message----- From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Sent: Monday, July 15, 2019 12:46 PM To: Tomer Maimon <tmaimon77@gmail.com> Cc: Oshri Alkobi <oshrialkoby85@gmail.com>; Alexander Steffen <Alexander.Steffen@infineon.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; peterhuewe@gmx.de; jgg@ziepe.ca; Arnd Bergmann <arnd@arndb.de>; Greg KH <gregkh@linuxfoundation.org>; IS20 Oshri Alkoby <oshri.alkoby@nuvoton.com>; devicetree <devicetree@vger.kernel.org>; AP MS30 Linux Kernel community <linux-kernel@vger.kernel.org>; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; IS30 Dan Morav <Dan.Morav@nuvoton.com>; IS20 Eyal Cohen <Eyal.Cohen@nuvoton.com> Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp On Mon, Jul 15, 2019 at 11:08:47AM +0300, Tomer Maimon wrote: > Thanks for your feedback and sorry for the late response. > > Due to the amount of work required to handle this technical feedback and > project constraints we need to put this task on hold for the near future. > > In the meantime, anyone from the community is welcome to take over this > code and handle the re-design for the benefit of the entire TPM community. Ok, so there is already driver for this called tpm_tis_core. So you go and create a new module, whose name given the framework of things that we already have deployed, is destined to be tpm_tis_i2c. Then you roughly implement a new physical layer by using a callback interface provided to you by tpm_tis_core. The so called re-design was already addressed by Alexander [1]. How hard can it be seriously? [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2019_7_4_331&d=DwIBAg&c=ue8mO8zgC4VZ4q_aNVKt8G9MC01UFDmisvMR1k-EoDM&r=hjzIxEsS8wf3Ezr__r0ZOjw3kki8jIlQK-SQ5pWhXaM&m=aYs86sTFmxqvlI5rE2AWLGRl0lb9XRuiRKVtsCaXdRM&s=_3MMoq5UISFwMfK4OfgLbA2kMfZVgEfgkaWKqDEUXGw&e= /Jarkko =========================================================================================== The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.
On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: > Hi Jarkko and Alexander, > > We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. Great :) In the meantime, I've done some experiments creating an I2C driver based on tpm_tis_core, see https://patchwork.kernel.org/patch/11049363/ Please have a look at that and provide your feedback (and/or use it as a basis for further implementations). > However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. > At a minimum we thought of: > 1. Handling TPM Localities in I2C is different It turned out not to be that different in the end, see the code mentioned above and my comment here: https://patchwork.kernel.org/cover/11049365/ > 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core That is completely optional, so there is no need to implement it in the beginning. Also, do you expect a huge benefit from that functionality? Are bit flips that much more likely on I2C compared to SPI, which has no CRC at all, but still works fine? > 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors Right, that seems similar to the cr50 issues (https://lkml.org/lkml/2019/7/17/677), so there should probably be a similar way to do it. > 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry) Optimizations are always welcome, but I'd expect basic communication to work already with the current code (though maybe not as efficiently as possible). > Besides this, during development we might encounter additional differences between SPI and I2C. > > We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window. > > Regards, > Eyal.
Hi Alexander, Jarkko and Eyal, A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago. https://patchwork.kernel.org/patch/8628681/ At the time, we have had two concerns : 1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver. 2) Lots changing was already provided by tpm_tis_spi.c on 4.8. That's why Tpm_tis_i2c.c has been postponed. Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august). Best Regards, Benoit -----Original Message----- From: linux-integrity-owner@vger.kernel.org <linux-integrity-owner@vger.kernel.org> On Behalf Of Alexander Steffen Sent: jeudi 18 juillet 2019 19:10 To: Eyal.Cohen@nuvoton.com; jarkko.sakkinen@linux.intel.com; tmaimon77@gmail.com Cc: oshrialkoby85@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; peterhuewe@gmx.de; jgg@ziepe.ca; arnd@arndb.de; gregkh@linuxfoundation.org; oshri.alkoby@nuvoton.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; Dan.Morav@nuvoton.com; oren.tanami@nuvoton.com Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: > Hi Jarkko and Alexander, > > We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. Great :) In the meantime, I've done some experiments creating an I2C driver based on tpm_tis_core, see https://patchwork.kernel.org/patch/11049363/ Please have a look at that and provide your feedback (and/or use it as a basis for further implementations). > However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. > At a minimum we thought of: > 1. Handling TPM Localities in I2C is different It turned out not to be that different in the end, see the code mentioned above and my comment here: https://patchwork.kernel.org/cover/11049365/ > 2. Handling I2C CRC - relevant only to I2C bus hence not supported > today by TIS core That is completely optional, so there is no need to implement it in the beginning. Also, do you expect a huge benefit from that functionality? Are bit flips that much more likely on I2C compared to SPI, which has no CRC at all, but still works fine? > 3. Handling Chip specific issues, since I2C implementation might be > slightly different across the various TPM vendors Right, that seems similar to the cr50 issues (https://lkml.org/lkml/2019/7/17/677), so there should probably be a similar way to do it. > 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according > the TCG Device Driver Guide (optimization on TPM_STS access and > send/recv retry) Optimizations are always welcome, but I'd expect basic communication to work already with the current code (though maybe not as efficiently as possible). > Besides this, during development we might encounter additional differences between SPI and I2C. > > We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window. > > Regards, > Eyal.
Hi Benoit, good to see you're still around. On 30.07.2019 10:39, Benoit HOUYERE wrote: > Hi Alexander, Jarkko and Eyal, > > A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago. > > https://patchwork.kernel.org/patch/8628681/ Thanks for mentioning this. I forgot it exists, since it was still on the old mailing list. > At the time, we have had two concerns : > 1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver. > 2) Lots changing was already provided by tpm_tis_spi.c on 4.8. > > That's why Tpm_tis_i2c.c has been postponed. > > Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august). Could you run your tests against the simple implementation that I posted a while ago (https://patchwork.kernel.org/cover/11049365/) and provide your feedback? Since it is already based on the current tpm_tis_core, it is probably easier to integrate necessary changes there. By the way, has it gotten any easier in the meantime to get hold of your TPMs to use them for kernel tests? Alexander
On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen <Alexander.Steffen@infineon.com> wrote: > > On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: > > Hi Jarkko and Alexander, > > > > We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. > > Great :) In the meantime, I've done some experiments creating an I2C > driver based on tpm_tis_core, see > https://patchwork.kernel.org/patch/11049363/ Please have a look at that > and provide your feedback (and/or use it as a basis for further > implementations). > Sorry for the late response. Thanks Alexander, indeed it looks much simpler. I've checked it with Nuvoton's TPM - basic TPM commands work, I only had to remove the first msg from the read/write I2C transmitting (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in a sequence. Actually it is more efficient to set TPM_LOC_SEL only before locality check/request/relinquish - it is sticky. I still didn't manage to work with interrupts, will debug it. We weren't aware to the implementation of Christophe/ST which looks good and can be complement to yours. If no one is currently working on that, we can prepare a new patch that is based on both. Please let us know. > > However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. > > At a minimum we thought of: > > 1. Handling TPM Localities in I2C is different > > It turned out not to be that different in the end, see the code > mentioned above and my comment here: > https://patchwork.kernel.org/cover/11049365/ > > > 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core > > That is completely optional, so there is no need to implement it in the > beginning. Also, do you expect a huge benefit from that functionality? > Are bit flips that much more likely on I2C compared to SPI, which has no > CRC at all, but still works fine? > I2C is noisy bus with potentially more devices with larger variety than SPI. I2C may have more than one master and may have collisions and/or arbitration. Still we can start w/o CRC for the first stage and add it later. BTW, Christophe already did most of the work (https://patchwork.kernel.org/patch/8628661/) > > 3. Handling Chip specific issues, since I2C implementation might be slightly different across the various TPM vendors > > Right, that seems similar to the cr50 issues > (https://lkml.org/lkml/2019/7/17/677), so there should probably be a > similar way to do it. Got it. We hope things will work for us without having another driver, but it's an option. > > > 4. Modify tpm_tis_send_data and tpm_tis_recv_data to work according the TCG Device Driver Guide (optimization on TPM_STS access and send/recv retry) > > Optimizations are always welcome, but I'd expect basic communication to > work already with the current code (though maybe not as efficiently as > possible). > > > Besides this, during development we might encounter additional differences between SPI and I2C. > > > > We currently target to allocate an eng. to work on this on the second half of August with a goal to have the driver ready for the next kernel merge window. > > > > Regards, > > Eyal.
On 15.08.2019 19:03, Oshri Alkobi wrote: > On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen > <Alexander.Steffen@infineon.com> wrote: >> >> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: >>> Hi Jarkko and Alexander, >>> >>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. >> >> Great :) In the meantime, I've done some experiments creating an I2C >> driver based on tpm_tis_core, see >> https://patchwork.kernel.org/patch/11049363/ Please have a look at that >> and provide your feedback (and/or use it as a basis for further >> implementations). >> > > Sorry for the late response. > > Thanks Alexander, indeed it looks much simpler. > I've checked it with Nuvoton's TPM - basic TPM commands work Nice :) > I only > had to remove the first msg from the read/write I2C transmitting > (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in > a sequence. > Actually it is more efficient to set TPM_LOC_SEL only before locality > check/request/relinquish - it is sticky. There is one problem though: Do we assume that only the kernel driver will communicate with the TPM or might there be something else that also talks to the TPM? If it is only the kernel driver, we could probably skip setting TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use anything else. If something else does its own I2C communication with the TPM, it might write different values to TPM_LOC_SEL at any time, causing the kernel driver to use a different locality than intended. This was the reason I always set TPM_LOC_SEL within the same transaction. > I still didn't manage to work with interrupts, will debug it. Interrupt support might be broken in general at the moment, see this thread: https://www.spinics.net/lists/linux-integrity/msg08663.html > We weren't aware to the implementation of Christophe/ST which looks > good and can be complement to yours. > If no one is currently working on that, we can prepare a new patch > that is based on both. > Please let us know. I won't have the time to do anything, at least for the next two weeks, so feel free to pick it up. >>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. >>> At a minimum we thought of: >>> 1. Handling TPM Localities in I2C is different >> >> It turned out not to be that different in the end, see the code >> mentioned above and my comment here: >> https://patchwork.kernel.org/cover/11049365/ >> >>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core >> >> That is completely optional, so there is no need to implement it in the >> beginning. Also, do you expect a huge benefit from that functionality? >> Are bit flips that much more likely on I2C compared to SPI, which has no >> CRC at all, but still works fine? >> > > I2C is noisy bus with potentially more devices with larger variety > than SPI. I2C may have more than one master and may have collisions > and/or arbitration. If multi-master usage is a concern, there are probably a lot more places in the driver that need to be adapted to deal with concurrent access/data corruption. For now, I'd assume a single master, similar to SPI. Alexander
On Fri, Aug 16, 2019 at 7:12 PM Alexander Steffen <Alexander.Steffen@infineon.com> wrote: > > On 15.08.2019 19:03, Oshri Alkobi wrote: > > On Thu, Jul 18, 2019 at 8:10 PM Alexander Steffen > > <Alexander.Steffen@infineon.com> wrote: > >> > >> On 18.07.2019 14:51, Eyal.Cohen@nuvoton.com wrote: > >>> Hi Jarkko and Alexander, > >>> > >>> We have made an additional code review on the TPM TIS core driver, it looks quite good and we can connect our new I2C driver to this layer. > >> > >> Great :) In the meantime, I've done some experiments creating an I2C > >> driver based on tpm_tis_core, see > >> https://patchwork.kernel.org/patch/11049363/ Please have a look at that > >> and provide your feedback (and/or use it as a basis for further > >> implementations). > >> > > > > Sorry for the late response. > > > > Thanks Alexander, indeed it looks much simpler. > > I've checked it with Nuvoton's TPM - basic TPM commands work > > Nice :) > > > I only > > had to remove the first msg from the read/write I2C transmitting > > (from/to TPM_LOC_SEL), the TPM couldn't handle two register writes in > > a sequence. > > Actually it is more efficient to set TPM_LOC_SEL only before locality > > check/request/relinquish - it is sticky. > > There is one problem though: Do we assume that only the kernel driver > will communicate with the TPM or might there be something else that also > talks to the TPM? > > If it is only the kernel driver, we could probably skip setting > TPM_LOC_SEL at all, since it defaults to 0 and the driver will never use > anything else. If something else does its own I2C communication with the > TPM, it might write different values to TPM_LOC_SEL at any time, causing > the kernel driver to use a different locality than intended. This was > the reason I always set TPM_LOC_SEL within the same transaction. > > > I still didn't manage to work with interrupts, will debug it. > > Interrupt support might be broken in general at the moment, see this > thread: https://www.spinics.net/lists/linux-integrity/msg08663.html > > > We weren't aware to the implementation of Christophe/ST which looks > > good and can be complement to yours. > > If no one is currently working on that, we can prepare a new patch > > that is based on both. > > Please let us know. > > I won't have the time to do anything, at least for the next two weeks, > so feel free to pick it up. > Great, we will start working on it. > >>> However, there are several differences between the SPI interface and the I2C interface that will require changes to the TIS core. > >>> At a minimum we thought of: > >>> 1. Handling TPM Localities in I2C is different > >> > >> It turned out not to be that different in the end, see the code > >> mentioned above and my comment here: > >> https://patchwork.kernel.org/cover/11049365/ > >> > >>> 2. Handling I2C CRC - relevant only to I2C bus hence not supported today by TIS core > >> > >> That is completely optional, so there is no need to implement it in the > >> beginning. Also, do you expect a huge benefit from that functionality? > >> Are bit flips that much more likely on I2C compared to SPI, which has no > >> CRC at all, but still works fine? > >> > > > > I2C is noisy bus with potentially more devices with larger variety > > than SPI. I2C may have more than one master and may have collisions > > and/or arbitration. > > If multi-master usage is a concern, there are probably a lot more places > in the driver that need to be adapted to deal with concurrent > access/data corruption. For now, I'd assume a single master, similar to SPI. > > Alexander Oshri
Hi Steffen,
We have performed test against your simple implementation. For basic test it was ok, however for stress test, your implementation does not take in account NACK and it failed.
In PTP document, in chapter7.2.2.1.2 (Register write with address NACK), it indicates : "it's a good practice to repeat the current cycle using the correct I2C device address".
In other implementation,
https://patchwork.kernel.org/patch/8628681/
tpm_tis_i2c_read_bytes and tpm_tis_i2c_write_bytes handle NACK with a for loop.
+ for (i = 0; i < TPM_RETRY && ret < 0; i++) {
+ tpm_tis_i2c_sleep_guard_time(phy, TPM_I2C_SEND);
+ ret = i2c_master_send(phy->client, &i2c_reg, 1);
+ mod_timer(&phy->guard_timer, phy->guard_time);
+ }
+
+ if (ret < 0)
+ goto exit;
+
+ ret = -1;
+ for (i = 0; i < TPM_RETRY && ret < 0; i++) {
+ tpm_tis_i2c_sleep_guard_time(phy, TPM_I2C_RECV);
+ ret = i2c_master_recv(phy->client, result, len * size);
+ mod_timer(&phy->guard_timer, phy->guard_time);
+ }
I think that we should implement it before to include tpm_tis_i2c.c officially.
Thanks in advance,
Best Regards,
Benoit
-----Original Message-----
From: Alexander Steffen <Alexander.Steffen@infineon.com>
Sent: mardi 30 juillet 2019 19:42
To: Benoit HOUYERE <benoit.houyere@st.com>; Eyal.Cohen@nuvoton.com; jarkko.sakkinen@linux.intel.com; tmaimon77@gmail.com
Cc: oshrialkoby85@gmail.com; robh+dt@kernel.org; mark.rutland@arm.com; peterhuewe@gmx.de; jgg@ziepe.ca; arnd@arndb.de; gregkh@linuxfoundation.org; oshri.alkoby@nuvoton.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-integrity@vger.kernel.org; gcwilson@us.ibm.com; kgoldman@us.ibm.com; nayna@linux.vnet.ibm.com; Dan.Morav@nuvoton.com; oren.tanami@nuvoton.com; Christophe Ricard (christophe.ricard@gmail.com) <christophe.ricard@gmail.com>; Elena WILLIS <elena.willis@st.com>; Olivier COLLART <olivier.collart@st.com>
Subject: Re: [PATCH v2 0/2] char: tpm: add new driver for tpm i2c ptp
Hi Benoit,
good to see you're still around.
On 30.07.2019 10:39, Benoit HOUYERE wrote:
> Hi Alexander, Jarkko and Eyal,
>
> A first I2C TCG patch (tpm_tis_i2c.c) has been proposed in the same time as tpm_tis_spi.c by Christophe 3 years ago.
>
> https://patchwork.kernel.org/patch/8628681/
Thanks for mentioning this. I forgot it exists, since it was still on the old mailing list.
> At the time, we have had two concerns :
> 1) I2C TPM component number, in the market, compliant with new I2C TCG specification to validate new I2C driver.
> 2) Lots changing was already provided by tpm_tis_spi.c on 4.8.
>
> That's why Tpm_tis_i2c.c has been postponed.
>
> Tpm_tis_spi Linux driver is now robust, if we have several different I2C TPM solutions today to validate a tpm_tis_i2c driver, I 'm ready to contribute to it for validation (STmicro TPM) or propose a solution compatible on 5.1 linux driver if needed under timeframe proposed (second half of august).
Could you run your tests against the simple implementation that I posted a while ago (https://patchwork.kernel.org/cover/11049365/) and provide your feedback? Since it is already based on the current tpm_tis_core, it is probably easier to integrate necessary changes there.
By the way, has it gotten any easier in the meantime to get hold of your TPMs to use them for kernel tests?
Alexander
This patch set adds support for TPM devices that implement the I2C interface defined by TCG PTP specification: https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_2.0_r1.03_v22.pdf The driver was tested on Raspberry-Pie 3, using Nuvoton NPCT75X TPM. changes ------- v2: - Extended device tree binding description v1: - Initial version Oshri Alkoby (2): dt-bindings: tpm: add the TPM I2C PTP device tree binding documentation char: tpm: add new driver for tpm i2c ptp .../bindings/security/tpm/tpm-i2c-ptp.txt | 24 + drivers/char/tpm/Kconfig | 22 + drivers/char/tpm/Makefile | 1 + drivers/char/tpm/tpm_i2c_ptp.c | 1099 +++++++++++++++++ 4 files changed, 1146 insertions(+) create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm-i2c-ptp.txt create mode 100644 drivers/char/tpm/tpm_i2c_ptp.c